Hi Laurent Pinchart,
Thanks for the feedback.
> Subject: Re: [PATCH 1/7] drm: adv7511: Add struct adv7511_chip_info and use
> i2c_get_match_data()
>
> Hi Biju,
>
> Thank you for the patch.
>
> On Sun, Aug 13, 2023 at 07:05:06PM +0100, Biju Das wrote:
> > Add struct adv7511_chip_info to handle hw differences between various
> > chips rather checking against the 'type' variable in various places.
> > Replace 'adv->type'->'info->type' by moving variable 'type' from
> > struct adv7511 to struct adv7511_chip_info.
> >
> > Replace of_device_get_match_data() and ID lookup for retrieving match
> > data with i2c_get_match_data() by adding adv7511_chip_info as device
> > data for both OF and ID tables.
>
> As commented in similar patches, please try to explain in the commit
> message the reason *why* the change is good and/or needed. It's quite
> straightforward for me to understand the change to i2c_get_match_data()
> given the discussions we've had recently, but it would be more difficult
> without that background information.
Ok will add the info. Basically it simplifies the probe() by
replacing of_device_get_match_data() and ID lookup for retrieving
Match data.
>
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/bridge/adv7511/adv7511.h | 6 +-
> > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 68 +++++++++++---------
> > drivers/gpu/drm/bridge/adv7511/adv7533.c | 4 +-
> > 3 files changed, 46 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> > b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> > index 17445800248d..59e8ef10d72e 100644
> > --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> > @@ -333,6 +333,10 @@ enum adv7511_type {
> >
> > #define ADV7511_MAX_ADDRS 3
> >
> > +struct adv7511_chip_info {
> > + enum adv7511_type type;
> > +};
> > +
> > struct adv7511 {
> > struct i2c_client *i2c_main;
> > struct i2c_client *i2c_edid;
> > @@ -377,7 +381,7 @@ struct adv7511 {
> > u8 num_dsi_lanes;
> > bool use_timing_gen;
> >
> > - enum adv7511_type type;
> > + const struct adv7511_chip_info *info;
> > struct platform_device *audio_pdev;
> >
> > struct cec_adapter *cec_adap;
> > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > index 2611afd2c1c1..013d8d640ef4 100644
> > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > @@ -354,7 +354,7 @@ static void __adv7511_power_on(struct adv7511
> *adv7511)
> > * first few seconds after enabling the output. On the other hand
> > * adv7535 require to enable HPD Override bit for proper HPD.
> > */
> > - if (adv7511->type == ADV7535)
> > + if (adv7511->info->type == ADV7535)
> > regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER2,
> > ADV7535_REG_POWER2_HPD_OVERRIDE,
> > ADV7535_REG_POWER2_HPD_OVERRIDE); @@ -373,7
> +373,7 @@ static
> > void adv7511_power_on(struct adv7511 *adv7511)
> > */
> > regcache_sync(adv7511->regmap);
> >
> > - if (adv7511->type == ADV7533 || adv7511->type == ADV7535)
> > + if (adv7511->info->type == ADV7533 || adv7511->info->type ==
> > +ADV7535)
> > adv7533_dsi_power_on(adv7511);
> > adv7511->powered = true;
> > }
> > @@ -381,7 +381,7 @@ static void adv7511_power_on(struct adv7511
> > *adv7511) static void __adv7511_power_off(struct adv7511 *adv7511) {
> > /* TODO: setup additional power down modes */
> > - if (adv7511->type == ADV7535)
> > + if (adv7511->info->type == ADV7535)
> > regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER2,
> > ADV7535_REG_POWER2_HPD_OVERRIDE, 0);
> >
> > @@ -397,7 +397,7 @@ static void __adv7511_power_off(struct adv7511
> > *adv7511) static void adv7511_power_off(struct adv7511 *adv7511) {
> > __adv7511_power_off(adv7511);
> > - if (adv7511->type == ADV7533 || adv7511->type == ADV7535)
> > + if (adv7511->info->type == ADV7533 || adv7511->info->type ==
> > +ADV7535)
> > adv7533_dsi_power_off(adv7511);
> > adv7511->powered = false;
> > }
> > @@ -682,7 +682,7 @@ adv7511_detect(struct adv7511 *adv7511, struct
> drm_connector *connector)
> > status = connector_status_disconnected;
> > } else {
> > /* Renable HPD sensing */
> > - if (adv7511->type == ADV7535)
> > + if (adv7511->info->type == ADV7535)
> > regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER2,
> > ADV7535_REG_POWER2_HPD_OVERRIDE,
> > ADV7535_REG_POWER2_HPD_OVERRIDE); @@ -
> 786,7 +786,7 @@ static
> > void adv7511_mode_set(struct adv7511 *adv7511,
> > else
> > low_refresh_rate = ADV7511_LOW_REFRESH_RATE_NONE;
> >
> > - if (adv7511->type == ADV7511)
> > + if (adv7511->info->type == ADV7511)
> > regmap_update_bits(adv7511->regmap, 0xfb,
> > 0x6, low_refresh_rate << 1);
> > else
> > @@ -921,7 +921,7 @@ static enum drm_mode_status
> > adv7511_bridge_mode_valid(struct drm_bridge *bridge, {
> > struct adv7511 *adv = bridge_to_adv7511(bridge);
> >
> > - if (adv->type == ADV7533 || adv->type == ADV7535)
> > + if (adv->info->type == ADV7533 || adv->info->type == ADV7535)
> > return adv7533_mode_valid(adv, mode);
> > else
> > return adv7511_mode_valid(adv, mode); @@ -1009,7 +1009,7 @@
> static
> > int adv7511_init_regulators(struct adv7511 *adv)
> > unsigned int i;
> > int ret;
> >
> > - if (adv->type == ADV7511) {
> > + if (adv->info->type == ADV7511) {
> > supply_names = adv7511_supply_names;
> > adv->num_supplies = ARRAY_SIZE(adv7511_supply_names);
> > } else {
> > @@ -1093,7 +1093,7 @@ static int adv7511_init_cec_regmap(struct adv7511
> *adv)
> > goto err;
> > }
> >
> > - if (adv->type == ADV7533 || adv->type == ADV7535) {
> > + if (adv->info->type == ADV7533 || adv->info->type == ADV7535) {
> > ret = adv7533_patch_cec_registers(adv);
> > if (ret)
> > goto err;
> > @@ -1192,7 +1192,7 @@ static int adv7511_parse_dt(struct device_node
> > *np,
> >
> > static int adv7511_probe(struct i2c_client *i2c) {
> > - const struct i2c_device_id *id = i2c_client_get_device_id(i2c);
> > + static const struct adv7511_chip_info *info;
> > struct adv7511_link_config link_config;
> > struct adv7511 *adv7511;
> > struct device *dev = &i2c->dev;
> > @@ -1206,18 +1206,16 @@ static int adv7511_probe(struct i2c_client *i2c)
> > if (!adv7511)
> > return -ENOMEM;
> >
> > + info = i2c_get_match_data(i2c);
> > +
> > adv7511->i2c_main = i2c;
> > adv7511->powered = false;
> > adv7511->status = connector_status_disconnected;
> > -
> > - if (dev->of_node)
> > - adv7511->type = (enum
> adv7511_type)of_device_get_match_data(dev);
> > - else
> > - adv7511->type = id->driver_data;
> > + adv7511->info = info;
>
> You can drop the local info variable and write
OK.
>
> adv7511->info = i2c_get_match_data(i2c);
>
> >
> > memset(&link_config, 0, sizeof(link_config));
> >
> > - if (adv7511->type == ADV7511)
> > + if (adv7511->info->type == ADV7511)
> > ret = adv7511_parse_dt(dev->of_node, &link_config);
> > else
> > ret = adv7533_parse_dt(dev->of_node, adv7511); @@ -1254,7
> +1252,7
> > @@ static int adv7511_probe(struct i2c_client *i2c)
> > goto uninit_regulators;
> > dev_dbg(dev, "Rev. %d\n", val);
> >
> > - if (adv7511->type == ADV7511)
> > + if (info->type == ADV7511)
>
> And use adv7511->info->type here and below, the same way you use it above.
OK.
Cheers,
Biju
>
> With this addressed,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
>
> > ret = regmap_register_patch(adv7511->regmap,
> > adv7511_fixed_registers,
> > ARRAY_SIZE(adv7511_fixed_registers));
> > @@ -1306,7 +1304,7 @@ static int adv7511_probe(struct i2c_client *i2c)
> >
> > i2c_set_clientdata(i2c, adv7511);
> >
> > - if (adv7511->type == ADV7511)
> > + if (info->type == ADV7511)
> > adv7511_set_link_config(adv7511, &link_config);
> >
> > ret = adv7511_cec_init(dev, adv7511); @@ -1325,7 +1323,7 @@ static
> > int adv7511_probe(struct i2c_client *i2c)
> >
> > adv7511_audio_init(dev, adv7511);
> >
> > - if (adv7511->type == ADV7533 || adv7511->type == ADV7535) {
> > + if (info->type == ADV7533 || info->type == ADV7535) {
> > ret = adv7533_attach_dsi(adv7511);
> > if (ret)
> > goto err_unregister_audio;
> > @@ -1368,22 +1366,34 @@ static void adv7511_remove(struct i2c_client
> *i2c)
> > i2c_unregister_device(adv7511->i2c_edid);
> > }
> >
> > +static const struct adv7511_chip_info adv7511_chip_info = {
> > + .type = ADV7511
> > +};
> > +
> > +static const struct adv7511_chip_info adv7533_chip_info = {
> > + .type = ADV7533
> > +};
> > +
> > +static const struct adv7511_chip_info adv7535_chip_info = {
> > + .type = ADV7535
> > +};
> > +
> > static const struct i2c_device_id adv7511_i2c_ids[] = {
> > - { "adv7511", ADV7511 },
> > - { "adv7511w", ADV7511 },
> > - { "adv7513", ADV7511 },
> > - { "adv7533", ADV7533 },
> > - { "adv7535", ADV7535 },
> > + { "adv7511", (kernel_ulong_t)&adv7511_chip_info },
> > + { "adv7511w", (kernel_ulong_t)&adv7511_chip_info },
> > + { "adv7513", (kernel_ulong_t)&adv7511_chip_info },
> > + { "adv7533", (kernel_ulong_t)&adv7533_chip_info },
> > + { "adv7535", (kernel_ulong_t)&adv7535_chip_info },
> > { }
> > };
> > MODULE_DEVICE_TABLE(i2c, adv7511_i2c_ids);
> >
> > static const struct of_device_id adv7511_of_ids[] = {
> > - { .compatible = "adi,adv7511", .data = (void *)ADV7511 },
> > - { .compatible = "adi,adv7511w", .data = (void *)ADV7511 },
> > - { .compatible = "adi,adv7513", .data = (void *)ADV7511 },
> > - { .compatible = "adi,adv7533", .data = (void *)ADV7533 },
> > - { .compatible = "adi,adv7535", .data = (void *)ADV7535 },
> > + { .compatible = "adi,adv7511", .data = &adv7511_chip_info },
> > + { .compatible = "adi,adv7511w", .data = &adv7511_chip_info },
> > + { .compatible = "adi,adv7513", .data = &adv7511_chip_info },
> > + { .compatible = "adi,adv7533", .data = &adv7533_chip_info },
> > + { .compatible = "adi,adv7535", .data = &adv7535_chip_info },
> > { }
> > };
> > MODULE_DEVICE_TABLE(of, adv7511_of_ids); diff --git
> > a/drivers/gpu/drm/bridge/adv7511/adv7533.c
> > b/drivers/gpu/drm/bridge/adv7511/adv7533.c
> > index 7e3e56441aed..c452c4dc1c3f 100644
> > --- a/drivers/gpu/drm/bridge/adv7511/adv7533.c
> > +++ b/drivers/gpu/drm/bridge/adv7511/adv7533.c
> > @@ -108,11 +108,11 @@ enum drm_mode_status adv7533_mode_valid(struct
> adv7511 *adv,
> > u8 bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> >
> > /* Check max clock for either 7533 or 7535 */
> > - if (mode->clock > (adv->type == ADV7533 ? 80000 : 148500))
> > + if (mode->clock > (adv->info->type == ADV7533 ? 80000 : 148500))
> > return MODE_CLOCK_HIGH;
> >
> > /* Check max clock for each lane */
> > - max_lane_freq = (adv->type == ADV7533 ? 800000 : 891000);
> > + max_lane_freq = (adv->info->type == ADV7533 ? 800000 : 891000);
> >
> > if (mode->clock * bpp > max_lane_freq * adv->num_dsi_lanes)
> > return MODE_CLOCK_HIGH;
>
> --
> Regards,
>
> Laurent Pinchart
