2024-02-28 11:57:05

by Adam Ford

Subject: [PATCH 1/2] drm/bridge: adv7511: Allow IRQ to share GPIO pins

The IRQ registration currently assumes that the GPIO is
dedicated to it, but that may not necessarily be the case.
If the board has another device sharing the IRQ, it won't be
registered and the hot-plug detect fails. This is easily
fixed by add the IRQF_SHARED flag.

Signed-off-by: Adam Ford <[email protected]>

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index b5518ff97165..21f08b2ae265 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -1318,7 +1318,8 @@ static int adv7511_probe(struct i2c_client *i2c)

ret = devm_request_threaded_irq(dev, i2c->irq, NULL,
adv7511_irq_handler,
- IRQF_ONESHOT, dev_name(dev),
+ IRQF_ONESHOT | IRQF_SHARED,
+ dev_name(dev),
adv7511);
if (ret)
goto err_unregister_audio;
--
2.43.0

2024-02-28 16:32:27

by Laurent Pinchart

Subject: Re: [PATCH 1/2] drm/bridge: adv7511: Allow IRQ to share GPIO pins

Hi Adam,

Thank you for the patch.

On Wed, Feb 28, 2024 at 05:37:35AM -0600, Adam Ford wrote:
> The IRQ registration currently assumes that the GPIO is
> dedicated to it, but that may not necessarily be the case.
> If the board has another device sharing the IRQ, it won't be
> registered and the hot-plug detect fails. This is easily
> fixed by add the IRQF_SHARED flag.
>
> Signed-off-by: Adam Ford <[email protected]>
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index b5518ff97165..21f08b2ae265 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -1318,7 +1318,8 @@ static int adv7511_probe(struct i2c_client *i2c)
>
> ret = devm_request_threaded_irq(dev, i2c->irq, NULL,
> adv7511_irq_handler,
> - IRQF_ONESHOT, dev_name(dev),
> + IRQF_ONESHOT | IRQF_SHARED,
> + dev_name(dev),

This looks fine, but the IRQ handler doesn't.

static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd)
{
unsigned int irq0, irq1;
int ret;

ret = regmap_read(adv7511->regmap, ADV7511_REG_INT(0), &irq0);
if (ret < 0)
return ret;

ret = regmap_read(adv7511->regmap, ADV7511_REG_INT(1), &irq1);
if (ret < 0)
return ret;

regmap_write(adv7511->regmap, ADV7511_REG_INT(0), irq0);
regmap_write(adv7511->regmap, ADV7511_REG_INT(1), irq1);

if (process_hpd && irq0 & ADV7511_INT0_HPD && adv7511->bridge.encoder)
schedule_work(&adv7511->hpd_work);

if (irq0 & ADV7511_INT0_EDID_READY || irq1 & ADV7511_INT1_DDC_ERROR) {
adv7511->edid_read = true;

if (adv7511->i2c_main->irq)
wake_up_all(&adv7511->wq);
}

#ifdef CONFIG_DRM_I2C_ADV7511_CEC
adv7511_cec_irq_process(adv7511, irq1);
#endif

return 0;
}

static irqreturn_t adv7511_irq_handler(int irq, void *devid)
{
struct adv7511 *adv7511 = devid;
int ret;

ret = adv7511_irq_process(adv7511, true);
return ret < 0 ? IRQ_NONE : IRQ_HANDLED;
}

The function will return IRQ_HANDLED as long as the registers can be
read, even if they don't report any interrupt.

> adv7511);
> if (ret)
> goto err_unregister_audio;

--
Regards,

Laurent Pinchart

2024-03-03 15:44:26

by Adam Ford

Subject: Re: [PATCH 1/2] drm/bridge: adv7511: Allow IRQ to share GPIO pins

On Wed, Feb 28, 2024 at 10:31 AM Laurent Pinchart
<[email protected]> wrote:
>
> Hi Adam,
>
> Thank you for the patch.
>
> On Wed, Feb 28, 2024 at 05:37:35AM -0600, Adam Ford wrote:
> > The IRQ registration currently assumes that the GPIO is
> > dedicated to it, but that may not necessarily be the case.
> > If the board has another device sharing the IRQ, it won't be
> > registered and the hot-plug detect fails. This is easily
> > fixed by add the IRQF_SHARED flag.
> >
> > Signed-off-by: Adam Ford <[email protected]>
> >
> > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > index b5518ff97165..21f08b2ae265 100644
> > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > @@ -1318,7 +1318,8 @@ static int adv7511_probe(struct i2c_client *i2c)
> >
> > ret = devm_request_threaded_irq(dev, i2c->irq, NULL,
> > adv7511_irq_handler,
> > - IRQF_ONESHOT, dev_name(dev),
> > + IRQF_ONESHOT | IRQF_SHARED,
> > + dev_name(dev),
>
> This looks fine, but the IRQ handler doesn't.

Thanks for the review.

>
> static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd)
> {
> unsigned int irq0, irq1;
> int ret;
>
> ret = regmap_read(adv7511->regmap, ADV7511_REG_INT(0), &irq0);
> if (ret < 0)
> return ret;
>
> ret = regmap_read(adv7511->regmap, ADV7511_REG_INT(1), &irq1);
> if (ret < 0)
> return ret;

If I did a check here and returned if there was no IRQ to handle,
would that be sufficient?

--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -477,6 +477,11 @@ static int adv7511_irq_process(struct adv7511
*adv7511, bool process_hpd)
if (ret < 0)
return ret;

+ /* If there is no IRQ to handle, exit indicating no IRQ handled */
+ if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) &&
+ !(irq1 & ADV7511_INT1_DDC_ERROR))
+ return -1;
+
regmap_write(adv7511->regmap, ADV7511_REG_INT(0), irq0);
regmap_write(adv7511->regmap, ADV7511_REG_INT(1), irq1);

With this, I would expect adv7511_irq_handler to return IRQ_NONE. If
you're OK with that approach, I can do that. If you want me to merge
adv7511_irq_handler, and adv7511_irq_process, I can do that too to
make the return codes a little more intuitive.

adam

>
> regmap_write(adv7511->regmap, ADV7511_REG_INT(0), irq0);
> regmap_write(adv7511->regmap, ADV7511_REG_INT(1), irq1);
>
> if (process_hpd && irq0 & ADV7511_INT0_HPD && adv7511->bridge.encoder)
> schedule_work(&adv7511->hpd_work);
>
> if (irq0 & ADV7511_INT0_EDID_READY || irq1 & ADV7511_INT1_DDC_ERROR) {
> adv7511->edid_read = true;
>
> if (adv7511->i2c_main->irq)
> wake_up_all(&adv7511->wq);
> }
>
> #ifdef CONFIG_DRM_I2C_ADV7511_CEC
> adv7511_cec_irq_process(adv7511, irq1);
> #endif
>
> return 0;
> }
>
> static irqreturn_t adv7511_irq_handler(int irq, void *devid)
> {
> struct adv7511 *adv7511 = devid;
> int ret;
>
> ret = adv7511_irq_process(adv7511, true);
> return ret < 0 ? IRQ_NONE : IRQ_HANDLED;
> }
>
> The function will return IRQ_HANDLED as long as the registers can be
> read, even if they don't report any interrupt.
>
> > adv7511);
> > if (ret)
> > goto err_unregister_audio;
>
> --
> Regards,
>
> Laurent Pinchart

2024-03-03 16:02:20

by Laurent Pinchart

Subject: Re: [PATCH 1/2] drm/bridge: adv7511: Allow IRQ to share GPIO pins

On Sun, Mar 03, 2024 at 09:44:03AM -0600, Adam Ford wrote:
> On Wed, Feb 28, 2024 at 10:31 AM Laurent Pinchart wrote:
> > On Wed, Feb 28, 2024 at 05:37:35AM -0600, Adam Ford wrote:
> > > The IRQ registration currently assumes that the GPIO is
> > > dedicated to it, but that may not necessarily be the case.
> > > If the board has another device sharing the IRQ, it won't be
> > > registered and the hot-plug detect fails. This is easily
> > > fixed by add the IRQF_SHARED flag.
> > >
> > > Signed-off-by: Adam Ford <[email protected]>
> > >
> > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > > index b5518ff97165..21f08b2ae265 100644
> > > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > > @@ -1318,7 +1318,8 @@ static int adv7511_probe(struct i2c_client *i2c)
> > >
> > > ret = devm_request_threaded_irq(dev, i2c->irq, NULL,
> > > adv7511_irq_handler,
> > > - IRQF_ONESHOT, dev_name(dev),
> > > + IRQF_ONESHOT | IRQF_SHARED,
> > > + dev_name(dev),
> >
> > This looks fine, but the IRQ handler doesn't.
>
> Thanks for the review.
>
> > static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd)
> > {
> > unsigned int irq0, irq1;
> > int ret;
> >
> > ret = regmap_read(adv7511->regmap, ADV7511_REG_INT(0), &irq0);
> > if (ret < 0)
> > return ret;
> >
> > ret = regmap_read(adv7511->regmap, ADV7511_REG_INT(1), &irq1);
> > if (ret < 0)
> > return ret;
>
> If I did a check here and returned if there was no IRQ to handle,
> would that be sufficient?
>
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -477,6 +477,11 @@ static int adv7511_irq_process(struct adv7511
> *adv7511, bool process_hpd)
> if (ret < 0)
> return ret;
>
> + /* If there is no IRQ to handle, exit indicating no IRQ handled */
> + if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) &&
> + !(irq1 & ADV7511_INT1_DDC_ERROR))

If these are the only interrupt sources that the driver enables, this is
fine.

> + return -1;

Maybe a defined error code instead ?

> +
> regmap_write(adv7511->regmap, ADV7511_REG_INT(0), irq0);
> regmap_write(adv7511->regmap, ADV7511_REG_INT(1), irq1);
>
> With this, I would expect adv7511_irq_handler to return IRQ_NONE. If
> you're OK with that approach, I can do that. If you want me to merge
> adv7511_irq_handler, and adv7511_irq_process, I can do that too to
> make the return codes a little more intuitive.
>
> >
> > regmap_write(adv7511->regmap, ADV7511_REG_INT(0), irq0);
> > regmap_write(adv7511->regmap, ADV7511_REG_INT(1), irq1);
> >
> > if (process_hpd && irq0 & ADV7511_INT0_HPD && adv7511->bridge.encoder)
> > schedule_work(&adv7511->hpd_work);
> >
> > if (irq0 & ADV7511_INT0_EDID_READY || irq1 & ADV7511_INT1_DDC_ERROR) {
> > adv7511->edid_read = true;
> >
> > if (adv7511->i2c_main->irq)
> > wake_up_all(&adv7511->wq);
> > }
> >
> > #ifdef CONFIG_DRM_I2C_ADV7511_CEC
> > adv7511_cec_irq_process(adv7511, irq1);
> > #endif
> >
> > return 0;
> > }
> >
> > static irqreturn_t adv7511_irq_handler(int irq, void *devid)
> > {
> > struct adv7511 *adv7511 = devid;
> > int ret;
> >
> > ret = adv7511_irq_process(adv7511, true);
> > return ret < 0 ? IRQ_NONE : IRQ_HANDLED;
> > }
> >
> > The function will return IRQ_HANDLED as long as the registers can be
> > read, even if they don't report any interrupt.
> >
> > > adv7511);
> > > if (ret)
> > > goto err_unregister_audio;

--
Regards,

Laurent Pinchart