Hi Emil,
On Fri, 3 Jun 2016 10:38:49 +0100 Emil Velikov emil.l.velikov@gmail.com wrote:
Hi Boris.
On 2 June 2016 at 16:00, Boris Brezillon boris.brezillon@free-electrons.com wrote:
+static void sii902x_reset(struct sii902x *sii902x) +{
if (!sii902x->reset_gpio)return;This is wrong (reset_gpio is err_ptr) although we can/should nuke it all together. See below for reasoning.
gpiod_set_value(sii902x->reset_gpio, 1);msleep(100);Ouch that is some juicy number. Can we get a comment with reasoning/origin of it ?
As already explained to Maxime, I just don't know why this is needed, simply because I don't have access to the datasheet and I just based my implementation on another driver. I can add a comment stating that this was extracted from another implementation, but with no explanation on why this is needed.
Meng, do you have any information about startup-time, or something like that?
...
+static void sii902x_bridge_mode_set(struct drm_bridge *bridge,
struct drm_display_mode *mode,struct drm_display_mode *adj)+{
u8 buf[HDMI_INFOFRAME_HEADER_SIZE + HDMI_AVI_INFOFRAME_SIZE];HDMI_INFOFRAME_SIZE(AVI) seems shorter/easier to head imho.
Yep.
struct sii902x *sii902x = bridge_to_sii902x(bridge);struct regmap *regmap = sii902x->regmap;struct hdmi_avi_infoframe frame;int ret;buf[0] = adj->clock;buf[1] = adj->clock >> 8;buf[2] = adj->vrefresh;buf[3] = 0x00;buf[4] = adj->hdisplay;buf[5] = adj->hdisplay >> 8;buf[6] = adj->vdisplay;buf[7] = adj->vdisplay >> 8;buf[8] = SIL902X_TPI_CLK_RATIO_1X | SIL902X_TPI_AVI_PIXEL_REP_NONE |SIL902X_TPI_AVI_PIXEL_REP_BUS_24BIT;buf[9] = SIL902X_TPI_AVI_INPUT_RANGE_AUTO |SIL902X_TPI_AVI_INPUT_COLORSPACE_RGB;Since all of the contents are cleared in hdmi_avi_infoframe_pack, move the above into const video_data[] ?
Something like
const video_data[] = { adj->clock, adj->clock >> 8, ... };
So we would have 2 buffers on the stack? Is this really useful?
ret = regmap_bulk_write(regmap, SIL902X_TPI_VIDEO_DATA, buf, 10);... and use ARRAY_SIZE(video_data) over the hardcoded 10 ?
...
+static int sii902x_bridge_attach(struct drm_bridge *bridge) +{
struct sii902x *sii902x = bridge_to_sii902x(bridge);struct drm_device *drm = bridge->dev;int ret;drm_connector_helper_add(&sii902x->connector,&sii902x_connector_helper_funcs);if (!drm_core_check_feature(drm, DRIVER_ATOMIC)) {dev_err(&sii902x->i2c->dev,"sii902x driver is only compatible with DRM devices supporting atomic updates");return -ENOTSUPP;}ret = drm_connector_init(drm, &sii902x->connector,&sii902x_connector_funcs,DRM_MODE_CONNECTOR_HDMIA);Side note: seems like most places in DRM do not check the return value (~80 vs ~20). I wonder how badly/likely are things to explode.
Yep. I tend to always check return code, but if you say it's useless (and error-prone) I can remove it.
...
+static int sii902x_probe(struct i2c_client *client,
const struct i2c_device_id *id)+{
...
sii902x->reset_gpio = devm_gpiod_get_optional(dev, "reset",GPIOD_OUT_LOW);if (IS_ERR(sii902x->reset_gpio))dev_warn(dev, "Failed to retrieve/request reset gpio: %ld\n",PTR_ERR(sii902x->reset_gpio));Documentation says "Required" not optional. The above should be updated and one should error out if missing, right ?
Actually I was asked to make it optional, just forgot to update the documentation. This being said, devm_gpiod_get_optional() returns NULL if the property is not defined in the DT and an error code if the error comes from the GPIO layer, so I should just switch back to dev_err() and return the error code here.
This would make the test in sii902x_reset() valid again.
...
if (client->irq > 0) {I was always confused which is the correct way to check this >= 0 vs > 0. DRM has both :-\ Do you have any suggestions, should be 'mass convert' DRM to use only one of the two ?
Not sure 0 is a valid irq number anymore, so I don't think it's really important, but I can change it if you want.
Regards,
Boris