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

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

[Adam Ford]

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),
 						adv7511);
 		if (ret)
 			goto err_unregister_audio;
-- 
2.43.0

[PATCH 2/2] arm64: dts: imx8mp-beacon-kit: Enable HDMI bridge HPD

[Adam Ford]

The DSI to HDMI bridge supports hot-plut-detect, but the
driver didn't previously support a shared IRQ GPIO.  With
the driver updated, the interrupt can be added to the bridge.

Signed-off-by: Adam Ford <aford173@gmail.com>

diff --git a/arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts b/arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts
index 8de4dd268908..d854c94ec997 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts
@@ -405,6 +405,8 @@ adv_bridge: hdmi@3d {
 		compatible = "adi,adv7535";
 		reg = <0x3d>, <0x3c>, <0x3e>, <0x3f>;
 		reg-names = "main", "cec", "edid", "packet";
+		interrupt-parent = <&gpio4>;
+		interrupts = <27 IRQ_TYPE_EDGE_FALLING>;
 		adi,dsi-lanes = <4>;
 		#sound-dai-cells = <0>;
 
-- 
2.43.0

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

[Laurent Pinchart]

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;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] arm64: dts: imx8mp-beacon-kit: Enable HDMI bridge HPD

[Laurent Pinchart]

Hi Adam,

Thank you for the patch.

On Wed, Feb 28, 2024 at 05:37:36AM -0600, Adam Ford wrote:
> The DSI to HDMI bridge supports hot-plut-detect, but the
> driver didn't previously support a shared IRQ GPIO.  With
> the driver updated, the interrupt can be added to the bridge.

You can reflow the commit message to 72 columns.

> Signed-off-by: Adam Ford <aford173@gmail.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> diff --git a/arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts b/arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts
> index 8de4dd268908..d854c94ec997 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts
> +++ b/arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts
> @@ -405,6 +405,8 @@ adv_bridge: hdmi@3d {
>  		compatible = "adi,adv7535";
>  		reg = <0x3d>, <0x3c>, <0x3e>, <0x3f>;
>  		reg-names = "main", "cec", "edid", "packet";
> +		interrupt-parent = <&gpio4>;
> +		interrupts = <27 IRQ_TYPE_EDGE_FALLING>;
>  		adi,dsi-lanes = <4>;
>  		#sound-dai-cells = <0>;
>  

-- 
Regards,

Laurent Pinchart

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

[Adam Ford]

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

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

[Laurent Pinchart]

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;

-- 
Regards,

Laurent Pinchart