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

Hi Enric,

On Thu, 2 Jun 2016 23:47:23 +0200
Enric Balletbo Serra [off-list ref] wrote:

quoted

+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)

quoted

+
+       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

quoted

+       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.

quoted

+       } 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.

quoted

+               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

quoted

+       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;
+}

[...]

quoted

+static void sii902x_bridge_nop(struct drm_bridge *bridge)
+{
+}
+  

You can remove this dummy callback function now.

quoted

+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.

quoted

+       .enable = sii902x_bridge_enable,
+};
+

[...]

quoted

+
+#ifdef CONFIG_OF  

You already depend on OF in Kconfig so this will always be evaluated.

Indeed.

quoted

+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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html