On 3 June 2016 at 11:02, Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx> wrote: > Hi Emil, > > On Fri, 3 Jun 2016 10:38:49 +0100 > Emil Velikov <emil.l.velikov@xxxxxxxxx> wrote: > >> 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 ? > > 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. > Considering we don't hear from Meng, that sounds perfectly reasonable imho. > 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? > IMHO It makes things substantially clearer, but if you/others disagree, feel free to ignore. >> >> > + 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. > Not sure which one is the correct thing to do. My gut goes "keep it as is for the moment". I only pointed it out since DRM subsystem was quite inconsistent about 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. > Nice :-) >> >> ... >> >> > + >> > + 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. > Thanks. In that case I'd leave it as-is. At some point we'll get a brave soul that goes through DRM. But that would be some other day. Thanks Emil _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel