Hi Enric,
On Thu, 2 Jun 2016 23:47:23 +0200 Enric Balletbo Serra eballetbo@gmail.com 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