Hi Boris. On 2 June 2016 at 16:00, Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx> 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 ? ... > +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. > + 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[] ? > + 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. ... > +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 ? ... > + > + 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 ? Regards, Emil -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html