[PATCH v5 1/2] drm/bridge: Add sii902x driver

[Emil Velikov]

On 3 June 2016 at 11:02, Boris Brezillon
<boris.brezillon at free-electrons.com> wrote:
> Hi Emil,
>
> On Fri, 3 Jun 2016 10:38:49 +0100
> Emil Velikov <emil.l.velikov at gmail.com> wrote:
>
>> Hi Boris.
>>
>> On 2 June 2016 at 16:00, Boris Brezillon
>> <boris.brezillon at 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.
>
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