diff mbox series

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

Message ID 20240228113737.43434-1-aford173@gmail.com (mailing list archive)
State New, archived
Headers show
Series [1/2] drm/bridge: adv7511: Allow IRQ to share GPIO pins | expand

Commit Message

Adam Ford Feb. 28, 2024, 11:37 a.m. UTC 
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 <aford173@gmail.com>

Comments

Laurent Pinchart Feb. 28, 2024, 4:31 p.m. UTC | #1 
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 <aford173@gmail.com>
> 
> 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;
Adam Ford March 3, 2024, 3:44 p.m. UTC | #2 
On Wed, Feb 28, 2024 at 10:31 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> 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 <aford173@gmail.com>
> >
> > 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
Laurent Pinchart March 3, 2024, 3:52 p.m. UTC | #3 
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 <aford173@gmail.com>
> > >
> > > 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;
diff mbox series

Patch

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;