| Message ID | 20230813180512.307418-8-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ADV7511 driver enhancements | expand |
On Sun, Aug 13, 2023 at 1:06 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > As per spec, it is allowed to pulse the HPD signal to indicate that the > EDID information has changed. Some monitors do this when they wake up > from standby or are enabled. When the HPD goes low the adv7511 is > reset and the outputs are disabled which might cause the monitor to > go to standby again. To avoid this we ignore the HPD pin for the > first few seconds after enabling the output. On the other hand, > adv7535 require to enable HPD Override bit for proper HPD. > > Add hpd_override_enable feature bit to struct adv7511_chip_info to handle > this scenario. > > While at it, drop the enum adv7511_type as it is unused. It seems like dropping adv7511_type is unrelated to the rest of the patch, and I think it should be split from this into its own patch adam > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > drivers/gpu/drm/bridge/adv7511/adv7511.h | 8 +------- > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 12 +++++------- > 2 files changed, 6 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h > index 627531f48f84..c523ac4c9bc8 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h > @@ -325,22 +325,16 @@ struct adv7511_video_config { > struct hdmi_avi_infoframe avi_infoframe; > }; > > -enum adv7511_type { > - ADV7511, > - ADV7533, > - ADV7535, > -}; > - > #define ADV7511_MAX_ADDRS 3 > > struct adv7511_chip_info { > - enum adv7511_type type; > unsigned long max_mode_clock; > unsigned long max_lane_freq; > const char * const *supply_names; > unsigned int num_supplies; > unsigned has_dsi:1; > unsigned link_config:1; > + unsigned hpd_override_enable:1; > }; > > struct adv7511 { > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > index 6974c267b1d5..7b06a0a21685 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->info->type == ADV7535) > + if (adv7511->info->hpd_override_enable) > regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER2, > ADV7535_REG_POWER2_HPD_OVERRIDE, > ADV7535_REG_POWER2_HPD_OVERRIDE); > @@ -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->info->type == ADV7535) > + if (adv7511->info->hpd_override_enable) > regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER2, > ADV7535_REG_POWER2_HPD_OVERRIDE, 0); > > @@ -682,7 +682,7 @@ adv7511_detect(struct adv7511 *adv7511, struct drm_connector *connector) > status = connector_status_disconnected; > } else { > /* Renable HPD sensing */ > - if (adv7511->info->type == ADV7535) > + if (adv7511->info->hpd_override_enable) > regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER2, > ADV7535_REG_POWER2_HPD_OVERRIDE, > ADV7535_REG_POWER2_HPD_OVERRIDE); > @@ -1360,14 +1360,12 @@ static void adv7511_remove(struct i2c_client *i2c) > } > > static const struct adv7511_chip_info adv7511_chip_info = { > - .type = ADV7511, > .supply_names = adv7511_supply_names, > .num_supplies = ARRAY_SIZE(adv7511_supply_names), > .link_config = 1 > }; > > static const struct adv7511_chip_info adv7533_chip_info = { > - .type = ADV7533, > .max_mode_clock = 80000, > .max_lane_freq = 800000, > .supply_names = adv7533_supply_names, > @@ -1376,12 +1374,12 @@ static const struct adv7511_chip_info adv7533_chip_info = { > }; > > static const struct adv7511_chip_info adv7535_chip_info = { > - .type = ADV7535, > .max_mode_clock = 148500, > .max_lane_freq = 891000, > .supply_names = adv7533_supply_names, > .num_supplies = ARRAY_SIZE(adv7533_supply_names), > - .has_dsi = 1 > + .has_dsi = 1, > + .hpd_override_enable = 1 > }; > > static const struct i2c_device_id adv7511_i2c_ids[] = { > -- > 2.25.1 >
Hi Adam Ford, Thanks for the feedback. > Subject: Re: [PATCH 7/7] drm: adv7511: Add hpd_override_enable feature bit > to struct adv7511_chip_info > > On Sun, Aug 13, 2023 at 1:06 PM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > > > As per spec, it is allowed to pulse the HPD signal to indicate that > > the EDID information has changed. Some monitors do this when they wake > > up from standby or are enabled. When the HPD goes low the adv7511 is > > reset and the outputs are disabled which might cause the monitor to go > > to standby again. To avoid this we ignore the HPD pin for the first > > few seconds after enabling the output. On the other hand, > > adv7535 require to enable HPD Override bit for proper HPD. > > > > Add hpd_override_enable feature bit to struct adv7511_chip_info to > > handle this scenario. > > > > While at it, drop the enum adv7511_type as it is unused. > > It seems like dropping adv7511_type is unrelated to the rest of the patch, > and I think it should be split from this into its own patch With this patch, there is no user for adv7511_type that is the reason it is added here. I thought that is the common practice. Please correct me if that is not the case. Cheers, Biju > > adam > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > drivers/gpu/drm/bridge/adv7511/adv7511.h | 8 +------- > > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 12 +++++------- > > 2 files changed, 6 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h > > b/drivers/gpu/drm/bridge/adv7511/adv7511.h > > index 627531f48f84..c523ac4c9bc8 100644 > > --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h > > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h > > @@ -325,22 +325,16 @@ struct adv7511_video_config { > > struct hdmi_avi_infoframe avi_infoframe; }; > > > > -enum adv7511_type { > > - ADV7511, > > - ADV7533, > > - ADV7535, > > -}; > > - > > #define ADV7511_MAX_ADDRS 3 > > > > struct adv7511_chip_info { > > - enum adv7511_type type; > > unsigned long max_mode_clock; > > unsigned long max_lane_freq; > > const char * const *supply_names; > > unsigned int num_supplies; > > unsigned has_dsi:1; > > unsigned link_config:1; > > + unsigned hpd_override_enable:1; > > }; > > > > struct adv7511 { > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > index 6974c267b1d5..7b06a0a21685 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->info->type == ADV7535) > > + if (adv7511->info->hpd_override_enable) > > regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER2, > > ADV7535_REG_POWER2_HPD_OVERRIDE, > > ADV7535_REG_POWER2_HPD_OVERRIDE); > > @@ -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->info->type == ADV7535) > > + if (adv7511->info->hpd_override_enable) > > regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER2, > > ADV7535_REG_POWER2_HPD_OVERRIDE, > > 0); > > > > @@ -682,7 +682,7 @@ adv7511_detect(struct adv7511 *adv7511, struct > drm_connector *connector) > > status = connector_status_disconnected; > > } else { > > /* Renable HPD sensing */ > > - if (adv7511->info->type == ADV7535) > > + if (adv7511->info->hpd_override_enable) > > regmap_update_bits(adv7511->regmap, > ADV7511_REG_POWER2, > > > ADV7535_REG_POWER2_HPD_OVERRIDE, > > > > ADV7535_REG_POWER2_HPD_OVERRIDE); @@ -1360,14 +1360,12 @@ static void > > adv7511_remove(struct i2c_client *i2c) } > > > > static const struct adv7511_chip_info adv7511_chip_info = { > > - .type = ADV7511, > > .supply_names = adv7511_supply_names, > > .num_supplies = ARRAY_SIZE(adv7511_supply_names), > > .link_config = 1 > > }; > > > > static const struct adv7511_chip_info adv7533_chip_info = { > > - .type = ADV7533, > > .max_mode_clock = 80000, > > .max_lane_freq = 800000, > > .supply_names = adv7533_supply_names, @@ -1376,12 +1374,12 @@ > > static const struct adv7511_chip_info adv7533_chip_info = { }; > > > > static const struct adv7511_chip_info adv7535_chip_info = { > > - .type = ADV7535, > > .max_mode_clock = 148500, > > .max_lane_freq = 891000, > > .supply_names = adv7533_supply_names, > > .num_supplies = ARRAY_SIZE(adv7533_supply_names), > > - .has_dsi = 1 > > + .has_dsi = 1, > > + .hpd_override_enable = 1 > > }; > > > > static const struct i2c_device_id adv7511_i2c_ids[] = { > > -- > > 2.25.1 > >
On Fri, Aug 18, 2023 at 8:35 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > Hi Adam Ford, > > Thanks for the feedback. > > > Subject: Re: [PATCH 7/7] drm: adv7511: Add hpd_override_enable feature bit > > to struct adv7511_chip_info > > > > On Sun, Aug 13, 2023 at 1:06 PM Biju Das <biju.das.jz@bp.renesas.com> > > wrote: > > > > > > As per spec, it is allowed to pulse the HPD signal to indicate that > > > the EDID information has changed. Some monitors do this when they wake > > > up from standby or are enabled. When the HPD goes low the adv7511 is > > > reset and the outputs are disabled which might cause the monitor to go > > > to standby again. To avoid this we ignore the HPD pin for the first > > > few seconds after enabling the output. On the other hand, > > > adv7535 require to enable HPD Override bit for proper HPD. > > > > > > Add hpd_override_enable feature bit to struct adv7511_chip_info to > > > handle this scenario. > > > > > > While at it, drop the enum adv7511_type as it is unused. > > > > It seems like dropping adv7511_type is unrelated to the rest of the patch, > > and I think it should be split from this into its own patch > > With this patch, there is no user for adv7511_type that is the > reason it is added here. I thought that is the common practice. > I wasn't sure. > Please correct me if that is not the case. I'll defer to the maintainers. In general I like the series because it reduces the number of compare evaluations. I'll try to run some tests on a board that I have with a adv7535 this weekend. adam > > Cheers, > Biju > > > > > adam > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > --- > > > drivers/gpu/drm/bridge/adv7511/adv7511.h | 8 +------- > > > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 12 +++++------- > > > 2 files changed, 6 insertions(+), 14 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h > > > b/drivers/gpu/drm/bridge/adv7511/adv7511.h > > > index 627531f48f84..c523ac4c9bc8 100644 > > > --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h > > > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h > > > @@ -325,22 +325,16 @@ struct adv7511_video_config { > > > struct hdmi_avi_infoframe avi_infoframe; }; > > > > > > -enum adv7511_type { > > > - ADV7511, > > > - ADV7533, > > > - ADV7535, > > > -}; > > > - > > > #define ADV7511_MAX_ADDRS 3 > > > > > > struct adv7511_chip_info { > > > - enum adv7511_type type; > > > unsigned long max_mode_clock; > > > unsigned long max_lane_freq; > > > const char * const *supply_names; > > > unsigned int num_supplies; > > > unsigned has_dsi:1; > > > unsigned link_config:1; > > > + unsigned hpd_override_enable:1; > > > }; > > > > > > struct adv7511 { > > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > > b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > > index 6974c267b1d5..7b06a0a21685 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->info->type == ADV7535) > > > + if (adv7511->info->hpd_override_enable) > > > regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER2, > > > ADV7535_REG_POWER2_HPD_OVERRIDE, > > > ADV7535_REG_POWER2_HPD_OVERRIDE); > > > @@ -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->info->type == ADV7535) > > > + if (adv7511->info->hpd_override_enable) > > > regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER2, > > > ADV7535_REG_POWER2_HPD_OVERRIDE, > > > 0); > > > > > > @@ -682,7 +682,7 @@ adv7511_detect(struct adv7511 *adv7511, struct > > drm_connector *connector) > > > status = connector_status_disconnected; > > > } else { > > > /* Renable HPD sensing */ > > > - if (adv7511->info->type == ADV7535) > > > + if (adv7511->info->hpd_override_enable) > > > regmap_update_bits(adv7511->regmap, > > ADV7511_REG_POWER2, > > > > > ADV7535_REG_POWER2_HPD_OVERRIDE, > > > > > > ADV7535_REG_POWER2_HPD_OVERRIDE); @@ -1360,14 +1360,12 @@ static void > > > adv7511_remove(struct i2c_client *i2c) } > > > > > > static const struct adv7511_chip_info adv7511_chip_info = { > > > - .type = ADV7511, > > > .supply_names = adv7511_supply_names, > > > .num_supplies = ARRAY_SIZE(adv7511_supply_names), > > > .link_config = 1 > > > }; > > > > > > static const struct adv7511_chip_info adv7533_chip_info = { > > > - .type = ADV7533, > > > .max_mode_clock = 80000, > > > .max_lane_freq = 800000, > > > .supply_names = adv7533_supply_names, @@ -1376,12 +1374,12 @@ > > > static const struct adv7511_chip_info adv7533_chip_info = { }; > > > > > > static const struct adv7511_chip_info adv7535_chip_info = { > > > - .type = ADV7535, > > > .max_mode_clock = 148500, > > > .max_lane_freq = 891000, > > > .supply_names = adv7533_supply_names, > > > .num_supplies = ARRAY_SIZE(adv7533_supply_names), > > > - .has_dsi = 1 > > > + .has_dsi = 1, > > > + .hpd_override_enable = 1 > > > }; > > > > > > static const struct i2c_device_id adv7511_i2c_ids[] = { > > > -- > > > 2.25.1 > > >
Hi Adam, > Subject: Re: [PATCH 7/7] drm: adv7511: Add hpd_override_enable feature bit > to struct adv7511_chip_info > > On Fri, Aug 18, 2023 at 8:35 AM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > > > Hi Adam Ford, > > > > Thanks for the feedback. > > > > > Subject: Re: [PATCH 7/7] drm: adv7511: Add hpd_override_enable > > > feature bit to struct adv7511_chip_info > > > > > > On Sun, Aug 13, 2023 at 1:06 PM Biju Das > > > <biju.das.jz@bp.renesas.com> > > > wrote: > > > > > > > > As per spec, it is allowed to pulse the HPD signal to indicate > > > > that the EDID information has changed. Some monitors do this when > > > > they wake up from standby or are enabled. When the HPD goes low > > > > the adv7511 is reset and the outputs are disabled which might > > > > cause the monitor to go to standby again. To avoid this we ignore > > > > the HPD pin for the first few seconds after enabling the output. > > > > On the other hand, > > > > adv7535 require to enable HPD Override bit for proper HPD. > > > > > > > > Add hpd_override_enable feature bit to struct adv7511_chip_info to > > > > handle this scenario. > > > > > > > > While at it, drop the enum adv7511_type as it is unused. > > > > > > It seems like dropping adv7511_type is unrelated to the rest of the > > > patch, and I think it should be split from this into its own patch > > > > With this patch, there is no user for adv7511_type that is the reason > > it is added here. I thought that is the common practice. > > > I wasn't sure. > > > Please correct me if that is not the case. > > I'll defer to the maintainers. In general I like the series because it > reduces the number of compare evaluations. I'll try to run some tests on a > board that I have with a adv7535 this weekend. Thank you, Biju > > > > Cheers, > > Biju > > > > > > > > adam > > > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > > --- > > > > drivers/gpu/drm/bridge/adv7511/adv7511.h | 8 +------- > > > > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 12 +++++------- > > > > 2 files changed, 6 insertions(+), 14 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h > > > > b/drivers/gpu/drm/bridge/adv7511/adv7511.h > > > > index 627531f48f84..c523ac4c9bc8 100644 > > > > --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h > > > > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h > > > > @@ -325,22 +325,16 @@ struct adv7511_video_config { > > > > struct hdmi_avi_infoframe avi_infoframe; }; > > > > > > > > -enum adv7511_type { > > > > - ADV7511, > > > > - ADV7533, > > > > - ADV7535, > > > > -}; > > > > - > > > > #define ADV7511_MAX_ADDRS 3 > > > > > > > > struct adv7511_chip_info { > > > > - enum adv7511_type type; > > > > unsigned long max_mode_clock; > > > > unsigned long max_lane_freq; > > > > const char * const *supply_names; > > > > unsigned int num_supplies; > > > > unsigned has_dsi:1; > > > > unsigned link_config:1; > > > > + unsigned hpd_override_enable:1; > > > > }; > > > > > > > > struct adv7511 { > > > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > > > b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > > > index 6974c267b1d5..7b06a0a21685 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->info->type == ADV7535) > > > > + if (adv7511->info->hpd_override_enable) > > > > regmap_update_bits(adv7511->regmap, > ADV7511_REG_POWER2, > > > > ADV7535_REG_POWER2_HPD_OVERRIDE, > > > > > > > > ADV7535_REG_POWER2_HPD_OVERRIDE); @@ -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->info->type == ADV7535) > > > > + if (adv7511->info->hpd_override_enable) > > > > regmap_update_bits(adv7511->regmap, > ADV7511_REG_POWER2, > > > > > > > > ADV7535_REG_POWER2_HPD_OVERRIDE, 0); > > > > > > > > @@ -682,7 +682,7 @@ adv7511_detect(struct adv7511 *adv7511, struct > > > drm_connector *connector) > > > > status = connector_status_disconnected; > > > > } else { > > > > /* Renable HPD sensing */ > > > > - if (adv7511->info->type == ADV7535) > > > > + if (adv7511->info->hpd_override_enable) > > > > regmap_update_bits(adv7511->regmap, > > > ADV7511_REG_POWER2, > > > > > > > ADV7535_REG_POWER2_HPD_OVERRIDE, > > > > > > > > ADV7535_REG_POWER2_HPD_OVERRIDE); @@ -1360,14 +1360,12 @@ static > > > > void adv7511_remove(struct i2c_client *i2c) } > > > > > > > > static const struct adv7511_chip_info adv7511_chip_info = { > > > > - .type = ADV7511, > > > > .supply_names = adv7511_supply_names, > > > > .num_supplies = ARRAY_SIZE(adv7511_supply_names), > > > > .link_config = 1 > > > > }; > > > > > > > > static const struct adv7511_chip_info adv7533_chip_info = { > > > > - .type = ADV7533, > > > > .max_mode_clock = 80000, > > > > .max_lane_freq = 800000, > > > > .supply_names = adv7533_supply_names, @@ -1376,12 +1374,12 > > > > @@ static const struct adv7511_chip_info adv7533_chip_info = { }; > > > > > > > > static const struct adv7511_chip_info adv7535_chip_info = { > > > > - .type = ADV7535, > > > > .max_mode_clock = 148500, > > > > .max_lane_freq = 891000, > > > > .supply_names = adv7533_supply_names, > > > > .num_supplies = ARRAY_SIZE(adv7533_supply_names), > > > > - .has_dsi = 1 > > > > + .has_dsi = 1, > > > > + .hpd_override_enable = 1 > > > > }; > > > > > > > > static const struct i2c_device_id adv7511_i2c_ids[] = { > > > > -- > > > > 2.25.1 > > > >
On Fri, Aug 18, 2023 at 07:41:45AM -0500, Adam Ford wrote: > On Sun, Aug 13, 2023 at 1:06 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > As per spec, it is allowed to pulse the HPD signal to indicate that the > > EDID information has changed. Some monitors do this when they wake up > > from standby or are enabled. When the HPD goes low the adv7511 is > > reset and the outputs are disabled which might cause the monitor to > > go to standby again. To avoid this we ignore the HPD pin for the > > first few seconds after enabling the output. On the other hand, > > adv7535 require to enable HPD Override bit for proper HPD. > > > > Add hpd_override_enable feature bit to struct adv7511_chip_info to handle > > this scenario. > > > > While at it, drop the enum adv7511_type as it is unused. > > It seems like dropping adv7511_type is unrelated to the rest of the > patch, and I think it should be split from this into its own patch Dropping the enum with its last user makes sense, but I think the series may go a bit too far by adding to the info structure bits that don't really describe logical features, but are half made up for the sole purpose of dropping the enum. I would prefer keeping the enum for the handful of places where a type check makes more sense (in my opinion). > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > drivers/gpu/drm/bridge/adv7511/adv7511.h | 8 +------- > > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 12 +++++------- > > 2 files changed, 6 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h > > index 627531f48f84..c523ac4c9bc8 100644 > > --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h > > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h > > @@ -325,22 +325,16 @@ struct adv7511_video_config { > > struct hdmi_avi_infoframe avi_infoframe; > > }; > > > > -enum adv7511_type { > > - ADV7511, > > - ADV7533, > > - ADV7535, > > -}; > > - > > #define ADV7511_MAX_ADDRS 3 > > > > struct adv7511_chip_info { > > - enum adv7511_type type; > > unsigned long max_mode_clock; > > unsigned long max_lane_freq; > > const char * const *supply_names; > > unsigned int num_supplies; > > unsigned has_dsi:1; > > unsigned link_config:1; > > + unsigned hpd_override_enable:1; > > }; > > > > struct adv7511 { > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > index 6974c267b1d5..7b06a0a21685 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->info->type == ADV7535) > > + if (adv7511->info->hpd_override_enable) > > regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER2, > > ADV7535_REG_POWER2_HPD_OVERRIDE, > > ADV7535_REG_POWER2_HPD_OVERRIDE); > > @@ -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->info->type == ADV7535) > > + if (adv7511->info->hpd_override_enable) > > regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER2, > > ADV7535_REG_POWER2_HPD_OVERRIDE, 0); > > > > @@ -682,7 +682,7 @@ adv7511_detect(struct adv7511 *adv7511, struct drm_connector *connector) > > status = connector_status_disconnected; > > } else { > > /* Renable HPD sensing */ > > - if (adv7511->info->type == ADV7535) > > + if (adv7511->info->hpd_override_enable) > > regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER2, > > ADV7535_REG_POWER2_HPD_OVERRIDE, > > ADV7535_REG_POWER2_HPD_OVERRIDE); > > @@ -1360,14 +1360,12 @@ static void adv7511_remove(struct i2c_client *i2c) > > } > > > > static const struct adv7511_chip_info adv7511_chip_info = { > > - .type = ADV7511, > > .supply_names = adv7511_supply_names, > > .num_supplies = ARRAY_SIZE(adv7511_supply_names), > > .link_config = 1 > > }; > > > > static const struct adv7511_chip_info adv7533_chip_info = { > > - .type = ADV7533, > > .max_mode_clock = 80000, > > .max_lane_freq = 800000, > > .supply_names = adv7533_supply_names, > > @@ -1376,12 +1374,12 @@ static const struct adv7511_chip_info adv7533_chip_info = { > > }; > > > > static const struct adv7511_chip_info adv7535_chip_info = { > > - .type = ADV7535, > > .max_mode_clock = 148500, > > .max_lane_freq = 891000, > > .supply_names = adv7533_supply_names, > > .num_supplies = ARRAY_SIZE(adv7533_supply_names), > > - .has_dsi = 1 > > + .has_dsi = 1, > > + .hpd_override_enable = 1 > > }; > > > > static const struct i2c_device_id adv7511_i2c_ids[] = {
Hi Laurent, > Subject: Re: [PATCH 7/7] drm: adv7511: Add hpd_override_enable feature bit > to struct adv7511_chip_info > > On Fri, Aug 18, 2023 at 07:41:45AM -0500, Adam Ford wrote: > > On Sun, Aug 13, 2023 at 1:06 PM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > > > > > As per spec, it is allowed to pulse the HPD signal to indicate that > > > the EDID information has changed. Some monitors do this when they > > > wake up from standby or are enabled. When the HPD goes low the > > > adv7511 is reset and the outputs are disabled which might cause the > > > monitor to go to standby again. To avoid this we ignore the HPD pin > > > for the first few seconds after enabling the output. On the other > > > hand, > > > adv7535 require to enable HPD Override bit for proper HPD. > > > > > > Add hpd_override_enable feature bit to struct adv7511_chip_info to > > > handle this scenario. > > > > > > While at it, drop the enum adv7511_type as it is unused. > > > > It seems like dropping adv7511_type is unrelated to the rest of the > > patch, and I think it should be split from this into its own patch > > Dropping the enum with its last user makes sense, but I think the series > may go a bit too far by adding to the info structure bits that don't really > describe logical features, but are half made up for the sole purpose of > dropping the enum. I would prefer keeping the enum for the handful of > places where a type check makes more sense (in my opinion). Based on your feedback on patch#5 we can decide. Still we can avoid type for hw differences mentioned in patch#5. Cheers, Biju > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > --- > > > drivers/gpu/drm/bridge/adv7511/adv7511.h | 8 +------- > > > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 12 +++++------- > > > 2 files changed, 6 insertions(+), 14 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h > > > b/drivers/gpu/drm/bridge/adv7511/adv7511.h > > > index 627531f48f84..c523ac4c9bc8 100644 > > > --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h > > > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h > > > @@ -325,22 +325,16 @@ struct adv7511_video_config { > > > struct hdmi_avi_infoframe avi_infoframe; }; > > > > > > -enum adv7511_type { > > > - ADV7511, > > > - ADV7533, > > > - ADV7535, > > > -}; > > > - > > > #define ADV7511_MAX_ADDRS 3 > > > > > > struct adv7511_chip_info { > > > - enum adv7511_type type; > > > unsigned long max_mode_clock; > > > unsigned long max_lane_freq; > > > const char * const *supply_names; > > > unsigned int num_supplies; > > > unsigned has_dsi:1; > > > unsigned link_config:1; > > > + unsigned hpd_override_enable:1; > > > }; > > > > > > struct adv7511 { > > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > > b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > > index 6974c267b1d5..7b06a0a21685 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->info->type == ADV7535) > > > + if (adv7511->info->hpd_override_enable) > > > regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER2, > > > ADV7535_REG_POWER2_HPD_OVERRIDE, > > > ADV7535_REG_POWER2_HPD_OVERRIDE); > > > @@ -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->info->type == ADV7535) > > > + if (adv7511->info->hpd_override_enable) > > > regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER2, > > > ADV7535_REG_POWER2_HPD_OVERRIDE, > > > 0); > > > > > > @@ -682,7 +682,7 @@ adv7511_detect(struct adv7511 *adv7511, struct > drm_connector *connector) > > > status = connector_status_disconnected; > > > } else { > > > /* Renable HPD sensing */ > > > - if (adv7511->info->type == ADV7535) > > > + if (adv7511->info->hpd_override_enable) > > > regmap_update_bits(adv7511->regmap, > ADV7511_REG_POWER2, > > > > ADV7535_REG_POWER2_HPD_OVERRIDE, > > > > > > ADV7535_REG_POWER2_HPD_OVERRIDE); @@ -1360,14 +1360,12 @@ static > > > void adv7511_remove(struct i2c_client *i2c) } > > > > > > static const struct adv7511_chip_info adv7511_chip_info = { > > > - .type = ADV7511, > > > .supply_names = adv7511_supply_names, > > > .num_supplies = ARRAY_SIZE(adv7511_supply_names), > > > .link_config = 1 > > > }; > > > > > > static const struct adv7511_chip_info adv7533_chip_info = { > > > - .type = ADV7533, > > > .max_mode_clock = 80000, > > > .max_lane_freq = 800000, > > > .supply_names = adv7533_supply_names, @@ -1376,12 +1374,12 > > > @@ static const struct adv7511_chip_info adv7533_chip_info = { }; > > > > > > static const struct adv7511_chip_info adv7535_chip_info = { > > > - .type = ADV7535, > > > .max_mode_clock = 148500, > > > .max_lane_freq = 891000, > > > .supply_names = adv7533_supply_names, > > > .num_supplies = ARRAY_SIZE(adv7533_supply_names), > > > - .has_dsi = 1 > > > + .has_dsi = 1, > > > + .hpd_override_enable = 1 > > > }; > > > > > > static const struct i2c_device_id adv7511_i2c_ids[] = { > > -- > Regards, > > Laurent Pinchart
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h index 627531f48f84..c523ac4c9bc8 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h @@ -325,22 +325,16 @@ struct adv7511_video_config { struct hdmi_avi_infoframe avi_infoframe; }; -enum adv7511_type { - ADV7511, - ADV7533, - ADV7535, -}; - #define ADV7511_MAX_ADDRS 3 struct adv7511_chip_info { - enum adv7511_type type; unsigned long max_mode_clock; unsigned long max_lane_freq; const char * const *supply_names; unsigned int num_supplies; unsigned has_dsi:1; unsigned link_config:1; + unsigned hpd_override_enable:1; }; struct adv7511 { diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 6974c267b1d5..7b06a0a21685 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->info->type == ADV7535) + if (adv7511->info->hpd_override_enable) regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER2, ADV7535_REG_POWER2_HPD_OVERRIDE, ADV7535_REG_POWER2_HPD_OVERRIDE); @@ -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->info->type == ADV7535) + if (adv7511->info->hpd_override_enable) regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER2, ADV7535_REG_POWER2_HPD_OVERRIDE, 0); @@ -682,7 +682,7 @@ adv7511_detect(struct adv7511 *adv7511, struct drm_connector *connector) status = connector_status_disconnected; } else { /* Renable HPD sensing */ - if (adv7511->info->type == ADV7535) + if (adv7511->info->hpd_override_enable) regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER2, ADV7535_REG_POWER2_HPD_OVERRIDE, ADV7535_REG_POWER2_HPD_OVERRIDE); @@ -1360,14 +1360,12 @@ static void adv7511_remove(struct i2c_client *i2c) } static const struct adv7511_chip_info adv7511_chip_info = { - .type = ADV7511, .supply_names = adv7511_supply_names, .num_supplies = ARRAY_SIZE(adv7511_supply_names), .link_config = 1 }; static const struct adv7511_chip_info adv7533_chip_info = { - .type = ADV7533, .max_mode_clock = 80000, .max_lane_freq = 800000, .supply_names = adv7533_supply_names, @@ -1376,12 +1374,12 @@ static const struct adv7511_chip_info adv7533_chip_info = { }; static const struct adv7511_chip_info adv7535_chip_info = { - .type = ADV7535, .max_mode_clock = 148500, .max_lane_freq = 891000, .supply_names = adv7533_supply_names, .num_supplies = ARRAY_SIZE(adv7533_supply_names), - .has_dsi = 1 + .has_dsi = 1, + .hpd_override_enable = 1 }; static const struct i2c_device_id adv7511_i2c_ids[] = {
As per spec, it is allowed to pulse the HPD signal to indicate that the EDID information has changed. Some monitors do this when they wake up from standby or are enabled. When the HPD goes low the adv7511 is reset and the outputs are disabled which might cause the monitor to go to standby again. To avoid this we ignore the HPD pin for the first few seconds after enabling the output. On the other hand, adv7535 require to enable HPD Override bit for proper HPD. Add hpd_override_enable feature bit to struct adv7511_chip_info to handle this scenario. While at it, drop the enum adv7511_type as it is unused. Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- drivers/gpu/drm/bridge/adv7511/adv7511.h | 8 +------- drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 12 +++++------- 2 files changed, 6 insertions(+), 14 deletions(-)