Hi Enric,
On Thu, 2 Jun 2016 23:47:23 +0200
Enric Balletbo Serra <eballetbo@xxxxxxxxx> wrote:
> > +static int sii902x_get_modes(struct drm_connector *connector)
> > +{
> > + struct sii902x *sii902x = connector_to_sii902x(connector);
> > + struct regmap *regmap = sii902x->regmap;
> > + u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> > + unsigned int status;
> > + struct edid *edid;
> > + int num = 0;
> > + int ret;
> > + int i;
>
> Is the i variable really needed? (see my comments below)
>
> > +
> > + ret = regmap_update_bits(regmap, SIL902X_SYS_CTRL_DATA,
> > + SIL902X_SYS_CTRL_DDC_BUS_REQ,
> > + SIL902X_SYS_CTRL_DDC_BUS_REQ);
> > + if (ret)
> > + return ret;
> > +
> > + i = 0;
>
> You assign i to 0
>
> > + do {
> > + ret = regmap_read(regmap, SIL902X_SYS_CTRL_DATA, &status);
> > + if (ret)
> > + return ret;
> > + i++;
>
> And you increment i, for what?
Oops, this is a leftover from when I was debugging the implementation.
>
> > + } while (!(status & SIL902X_SYS_CTRL_DDC_BUS_GRTD));
> > +
> > + ret = regmap_write(regmap, SIL902X_SYS_CTRL_DATA, status);
> > + if (ret)
> > + return ret;
> > +
> > + edid = drm_get_edid(connector, sii902x->i2c->adapter);
> > + drm_mode_connector_update_edid_property(connector, edid);
> > + if (edid) {
> > + num += drm_add_edid_modes(connector, edid);
>
> This is always 0 + the returned value, so you can do:
> num = drm_add_edid_modes(connector, edid);
> It's more clear for me.
Sure.
>
> > + kfree(edid);
> > + }
> > +
> > + ret = drm_display_info_set_bus_formats(&connector->display_info,
> > + &bus_format, 1);
> > + if (ret)
> > + return ret;
> > +
> > + regmap_read(regmap, SIL902X_SYS_CTRL_DATA, &status);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_update_bits(regmap, SIL902X_SYS_CTRL_DATA,
> > + SIL902X_SYS_CTRL_DDC_BUS_REQ |
> > + SIL902X_SYS_CTRL_DDC_BUS_GRTD, 0);
> > + if (ret)
> > + return ret;
> > +
> > + i = 0;
>
> Again, you can remove the i variable, here and the i++ from the loop below
>
> > + do {
> > + ret = regmap_read(regmap, SIL902X_SYS_CTRL_DATA, &status);
> > + if (ret)
> > + return ret;
> > + i++;
> > + } while (status & (SIL902X_SYS_CTRL_DDC_BUS_REQ |
> > + SIL902X_SYS_CTRL_DDC_BUS_GRTD));
> > +
> > + return num;
> > +}
[...]
> > +static void sii902x_bridge_nop(struct drm_bridge *bridge)
> > +{
> > +}
> > +
>
> You can remove this dummy callback function now.
>
> > +static const struct drm_bridge_funcs sii902x_bridge_funcs = {
> > + .attach = sii902x_bridge_attach,
> > + .mode_set = sii902x_bridge_mode_set,
> > + .disable = sii902x_bridge_disable,
> > + .post_disable = sii902x_bridge_nop,
> > + .pre_enable = sii902x_bridge_nop,
>
> Remove .pre_enable
I guess ->{pre,post}_enable() were mandatory when I started the
development of this driver. I'll drop both.
>
> > + .enable = sii902x_bridge_enable,
> > +};
> > +
[...]
> > +
> > +#ifdef CONFIG_OF
>
> You already depend on OF in Kconfig so this will always be evaluated.
Indeed.
>
> > +static const struct of_device_id sii902x_dt_ids[] = {
> > + { .compatible = "sil,sii9022", },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, sii902x_dt_ids);
> > +#endif
> > +
> > +static const struct i2c_device_id sii902x_i2c_ids[] = {
> > + { "sii9022", 0 },
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(i2c, sii902x_i2c_ids);
> > +
> > +static struct i2c_driver sii902x_driver = {
> > + .probe = sii902x_probe,
> > + .remove = sii902x_remove,
> > + .driver = {
> > + .name = "sii902x",
> > + .of_match_table = of_match_ptr(sii902x_dt_ids),
>
> You already depend on OF in Kconfig so you don't need of_match_ptr()
> here, of_match_ptr(x) will always evaluate to x.
I'll directly pass sii902x_dt_ids.
Thanks for your review.
Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
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
