* Re: [PATCH] drm/bridge: adv7511: Fix Intermittent EDID failures
2024-05-21 1:16 [PATCH] drm/bridge: adv7511: Fix Intermittent EDID failures Adam Ford
@ 2024-05-21 21:12 ` kernel test robot
2024-05-21 21:51 ` Adam Ford
2024-05-22 9:48 ` Dmitry Baryshkov
2 siblings, 0 replies; 5+ messages in thread
From: kernel test robot @ 2024-05-21 21:12 UTC (permalink / raw)
To: Adam Ford, dri-devel
Cc: llvm, oe-kbuild-all, dmitry.baryshkov, ictor.liu, sui.jingfeng,
aford, Adam Ford, Andrzej Hajda, Neil Armstrong, Robert Foss,
Laurent Pinchart, Jonas Karlman, Jernej Skrabec,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
David Airlie, Daniel Vetter, linux-kernel
Hi Adam,
kernel test robot noticed the following build warnings:
[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on drm/drm-next drm-exynos/exynos-drm-next drm-intel/for-linux-next drm-tip/drm-tip linus/master drm-intel/for-linux-next-fixes v6.9 next-20240521]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Adam-Ford/drm-bridge-adv7511-Fix-Intermittent-EDID-failures/20240521-091841
base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link: https://lore.kernel.org/r/20240521011614.496421-1-aford173%40gmail.com
patch subject: [PATCH] drm/bridge: adv7511: Fix Intermittent EDID failures
config: i386-buildonly-randconfig-002-20240521 (https://download.01.org/0day-ci/archive/20240522/202405220445.DW2U7DXR-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240522/202405220445.DW2U7DXR-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405220445.DW2U7DXR-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c:507:35: warning: variable 'cec_status' is uninitialized when used here [-Wuninitialized]
507 | if (irq_status == IRQ_HANDLED || cec_status == IRQ_HANDLED)
| ^~~~~~~~~~
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c:472:16: note: initialize the variable 'cec_status' to silence this warning
472 | int cec_status;
| ^
| = 0
1 warning generated.
vim +/cec_status +507 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
505
506 /* If there is no IRQ to handle, exit indicating no IRQ data */
> 507 if (irq_status == IRQ_HANDLED || cec_status == IRQ_HANDLED)
508 return IRQ_HANDLED;
509
510 return IRQ_NONE;
511 }
512
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/bridge: adv7511: Fix Intermittent EDID failures
2024-05-21 1:16 [PATCH] drm/bridge: adv7511: Fix Intermittent EDID failures Adam Ford
2024-05-21 21:12 ` kernel test robot
@ 2024-05-21 21:51 ` Adam Ford
2024-05-22 1:58 ` Liu Ying
2024-05-22 9:48 ` Dmitry Baryshkov
2 siblings, 1 reply; 5+ messages in thread
From: Adam Ford @ 2024-05-21 21:51 UTC (permalink / raw)
To: dri-devel
Cc: dmitry.baryshkov, sui.jingfeng, aford, Andrzej Hajda,
Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman,
Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Daniel Vetter, linux-kernel,
Liu Ying
On Mon, May 20, 2024 at 8:16 PM Adam Ford <aford173@gmail.com> wrote:
>
> In the process of adding support for shared IRQ pins, a scenario
> was accidentally created where adv7511_irq_process returned
> prematurely causing the EDID to fail randomly.
>
> Since the interrupt handler is broken up into two main helper functions,
> update both of them to treat the helper functions as IRQ handlers. These
> IRQ routines process their respective tasks as before, but if they
> determine that actual work was done, mark the respective IRQ status
> accordingly, and delay the check until everything has been processed.
>
> This should guarantee the helper functions don't return prematurely
> while still returning proper values of either IRQ_HANDLED or IRQ_NONE.
>
> Reported by: Liu Ying <victor.liu@nxp.com>
> Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins")
> Signed-off-by: Adam Ford <aford173@gmail.com>
+ Liu
Sorry about the e-mail address copy-paste error.
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> index ea271f62b214..ec0b7f3d889c 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> @@ -401,7 +401,7 @@ struct adv7511 {
>
> #ifdef CONFIG_DRM_I2C_ADV7511_CEC
> int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511);
> -void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1);
> +int adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1);
> #else
> static inline int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511)
> {
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> index 44451a9658a3..4efb2cabf1b5 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> @@ -119,7 +119,7 @@ static void adv7511_cec_rx(struct adv7511 *adv7511, int rx_buf)
> cec_received_msg(adv7511->cec_adap, &msg);
> }
>
> -void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
> +int adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
> {
> unsigned int offset = adv7511->info->reg_cec_offset;
> const u32 irq_tx_mask = ADV7511_INT1_CEC_TX_READY |
> @@ -130,17 +130,21 @@ void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
> ADV7511_INT1_CEC_RX_READY3;
> unsigned int rx_status;
> int rx_order[3] = { -1, -1, -1 };
> - int i;
> + int i, ret = 0;
> + int irq_status = IRQ_NONE;
>
> - if (irq1 & irq_tx_mask)
> + if (irq1 & irq_tx_mask) {
> adv_cec_tx_raw_status(adv7511, irq1);
> + irq_status = IRQ_HANDLED;
> + }
>
> if (!(irq1 & irq_rx_mask))
> - return;
> + return irq_status;
>
> - if (regmap_read(adv7511->regmap_cec,
> - ADV7511_REG_CEC_RX_STATUS + offset, &rx_status))
> - return;
> + ret = regmap_read(adv7511->regmap_cec,
> + ADV7511_REG_CEC_RX_STATUS + offset, &rx_status);
> + if (ret < 0)
> + return ret;
>
> /*
> * ADV7511_REG_CEC_RX_STATUS[5:0] contains the reception order of RX
> @@ -172,6 +176,8 @@ void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
>
> adv7511_cec_rx(adv7511, rx_buf);
> }
> +
> + return IRQ_HANDLED;
> }
>
> static int adv7511_cec_adap_enable(struct cec_adapter *adap, bool enable)
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 66ccb61e2a66..56dd2d5a0376 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -469,6 +469,8 @@ static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd)
> {
> unsigned int irq0, irq1;
> int ret;
> + int cec_status;
> + int irq_status = IRQ_NONE;
>
> ret = regmap_read(adv7511->regmap, ADV7511_REG_INT(0), &irq0);
> if (ret < 0)
> @@ -478,38 +480,41 @@ 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 data */
> - if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) &&
> - !(irq1 & ADV7511_INT1_DDC_ERROR))
> - return -ENODATA;
> -
> 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)
> + if (process_hpd && irq0 & ADV7511_INT0_HPD && adv7511->bridge.encoder) {
> schedule_work(&adv7511->hpd_work);
> + irq_status = IRQ_HANDLED;
> + }
>
> 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);
> + irq_status = IRQ_HANDLED;
> }
>
> #ifdef CONFIG_DRM_I2C_ADV7511_CEC
> - adv7511_cec_irq_process(adv7511, irq1);
> + cec_status = adv7511_cec_irq_process(adv7511, irq1);
> +
> + if (cec_status < 0)
> + return cec_status;
> #endif
>
> - return 0;
> + /* If there is no IRQ to handle, exit indicating no IRQ data */
> + if (irq_status == IRQ_HANDLED || cec_status == IRQ_HANDLED)
> + return IRQ_HANDLED;
> +
> + return IRQ_NONE;
> }
>
> 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;
> + return adv7511_irq_process(adv7511, true);
> }
>
> /* -----------------------------------------------------------------------------
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/bridge: adv7511: Fix Intermittent EDID failures
2024-05-21 21:51 ` Adam Ford
@ 2024-05-22 1:58 ` Liu Ying
0 siblings, 0 replies; 5+ messages in thread
From: Liu Ying @ 2024-05-22 1:58 UTC (permalink / raw)
To: Adam Ford, dri-devel
Cc: dmitry.baryshkov, sui.jingfeng, aford, Andrzej Hajda,
Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman,
Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Daniel Vetter, linux-kernel
On 5/22/24 05:51, Adam Ford wrote:
> On Mon, May 20, 2024 at 8:16 PM Adam Ford <aford173@gmail.com> wrote:
>>
>> In the process of adding support for shared IRQ pins, a scenario
>> was accidentally created where adv7511_irq_process returned
>> prematurely causing the EDID to fail randomly.
>>
>> Since the interrupt handler is broken up into two main helper functions,
>> update both of them to treat the helper functions as IRQ handlers. These
>> IRQ routines process their respective tasks as before, but if they
>> determine that actual work was done, mark the respective IRQ status
>> accordingly, and delay the check until everything has been processed.
>>
>> This should guarantee the helper functions don't return prematurely
>> while still returning proper values of either IRQ_HANDLED or IRQ_NONE.
>>
>> Reported by: Liu Ying <victor.liu@nxp.com>
s/Reported by/Reported-by/
>> Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins")
>> Signed-off-by: Adam Ford <aford173@gmail.com>
>
> + Liu
>
> Sorry about the e-mail address copy-paste error.
No worries.
With this patch, it looks EDID retrieval works ok for me without
interrupt requested.
Tested-by: Liu Ying <victor.liu@nxp.com> # i.MX8MP EVK ADV7535 EDID retrieval w/o IRQ
>>
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> index ea271f62b214..ec0b7f3d889c 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> @@ -401,7 +401,7 @@ struct adv7511 {
>>
>> #ifdef CONFIG_DRM_I2C_ADV7511_CEC
>> int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511);
>> -void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1);
>> +int adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1);
>> #else
>> static inline int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511)
>> {
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
>> index 44451a9658a3..4efb2cabf1b5 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
>> @@ -119,7 +119,7 @@ static void adv7511_cec_rx(struct adv7511 *adv7511, int rx_buf)
>> cec_received_msg(adv7511->cec_adap, &msg);
>> }
>>
>> -void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
>> +int adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
>> {
>> unsigned int offset = adv7511->info->reg_cec_offset;
>> const u32 irq_tx_mask = ADV7511_INT1_CEC_TX_READY |
>> @@ -130,17 +130,21 @@ void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
>> ADV7511_INT1_CEC_RX_READY3;
>> unsigned int rx_status;
>> int rx_order[3] = { -1, -1, -1 };
>> - int i;
>> + int i, ret = 0;
>> + int irq_status = IRQ_NONE;
>>
>> - if (irq1 & irq_tx_mask)
>> + if (irq1 & irq_tx_mask) {
>> adv_cec_tx_raw_status(adv7511, irq1);
>> + irq_status = IRQ_HANDLED;
>> + }
>>
>> if (!(irq1 & irq_rx_mask))
>> - return;
>> + return irq_status;
>>
>> - if (regmap_read(adv7511->regmap_cec,
>> - ADV7511_REG_CEC_RX_STATUS + offset, &rx_status))
>> - return;
>> + ret = regmap_read(adv7511->regmap_cec,
>> + ADV7511_REG_CEC_RX_STATUS + offset, &rx_status);
>> + if (ret < 0)
>> + return ret;
>>
>> /*
>> * ADV7511_REG_CEC_RX_STATUS[5:0] contains the reception order of RX
>> @@ -172,6 +176,8 @@ void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
>>
>> adv7511_cec_rx(adv7511, rx_buf);
>> }
>> +
>> + return IRQ_HANDLED;
>> }
>>
>> static int adv7511_cec_adap_enable(struct cec_adapter *adap, bool enable)
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> index 66ccb61e2a66..56dd2d5a0376 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> @@ -469,6 +469,8 @@ static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd)
>> {
>> unsigned int irq0, irq1;
>> int ret;
>> + int cec_status;
>> + int irq_status = IRQ_NONE;
>>
>> ret = regmap_read(adv7511->regmap, ADV7511_REG_INT(0), &irq0);
>> if (ret < 0)
>> @@ -478,38 +480,41 @@ 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 data */
>> - if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) &&
>> - !(irq1 & ADV7511_INT1_DDC_ERROR))
>> - return -ENODATA;
>> -
>> 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)
>> + if (process_hpd && irq0 & ADV7511_INT0_HPD && adv7511->bridge.encoder) {
>> schedule_work(&adv7511->hpd_work);
>> + irq_status = IRQ_HANDLED;
>> + }
>>
>> 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);
>> + irq_status = IRQ_HANDLED;
>> }
>>
>> #ifdef CONFIG_DRM_I2C_ADV7511_CEC
>> - adv7511_cec_irq_process(adv7511, irq1);
>> + cec_status = adv7511_cec_irq_process(adv7511, irq1);
>> +
>> + if (cec_status < 0)
>> + return cec_status;
>> #endif
>>
>> - return 0;
>> + /* If there is no IRQ to handle, exit indicating no IRQ data */
>> + if (irq_status == IRQ_HANDLED || cec_status == IRQ_HANDLED)
>> + return IRQ_HANDLED;
>> +
>> + return IRQ_NONE;
>> }
>>
>> 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;
>> + return adv7511_irq_process(adv7511, true);
>> }
>>
>> /* -----------------------------------------------------------------------------
>> --
>> 2.43.0
>>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/bridge: adv7511: Fix Intermittent EDID failures
2024-05-21 1:16 [PATCH] drm/bridge: adv7511: Fix Intermittent EDID failures Adam Ford
2024-05-21 21:12 ` kernel test robot
2024-05-21 21:51 ` Adam Ford
@ 2024-05-22 9:48 ` Dmitry Baryshkov
2 siblings, 0 replies; 5+ messages in thread
From: Dmitry Baryshkov @ 2024-05-22 9:48 UTC (permalink / raw)
To: Adam Ford
Cc: dri-devel, ictor.liu, sui.jingfeng, aford, Andrzej Hajda,
Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman,
Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Daniel Vetter, linux-kernel
On Mon, May 20, 2024 at 08:16:14PM -0500, Adam Ford wrote:
> In the process of adding support for shared IRQ pins, a scenario
> was accidentally created where adv7511_irq_process returned
> prematurely causing the EDID to fail randomly.
>
> Since the interrupt handler is broken up into two main helper functions,
> update both of them to treat the helper functions as IRQ handlers. These
> IRQ routines process their respective tasks as before, but if they
> determine that actual work was done, mark the respective IRQ status
> accordingly, and delay the check until everything has been processed.
>
> This should guarantee the helper functions don't return prematurely
> while still returning proper values of either IRQ_HANDLED or IRQ_NONE.
>
> Reported by: Liu Ying <victor.liu@nxp.com>
> Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins")
> Signed-off-by: Adam Ford <aford173@gmail.com>
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> index ea271f62b214..ec0b7f3d889c 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> @@ -401,7 +401,7 @@ struct adv7511 {
>
> #ifdef CONFIG_DRM_I2C_ADV7511_CEC
> int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511);
> -void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1);
> +int adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1);
> #else
> static inline int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511)
> {
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> index 44451a9658a3..4efb2cabf1b5 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> @@ -119,7 +119,7 @@ static void adv7511_cec_rx(struct adv7511 *adv7511, int rx_buf)
> cec_received_msg(adv7511->cec_adap, &msg);
> }
>
> -void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
> +int adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
> {
> unsigned int offset = adv7511->info->reg_cec_offset;
> const u32 irq_tx_mask = ADV7511_INT1_CEC_TX_READY |
> @@ -130,17 +130,21 @@ void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
> ADV7511_INT1_CEC_RX_READY3;
> unsigned int rx_status;
> int rx_order[3] = { -1, -1, -1 };
> - int i;
> + int i, ret = 0;
> + int irq_status = IRQ_NONE;
>
> - if (irq1 & irq_tx_mask)
> + if (irq1 & irq_tx_mask) {
> adv_cec_tx_raw_status(adv7511, irq1);
> + irq_status = IRQ_HANDLED;
> + }
>
> if (!(irq1 & irq_rx_mask))
> - return;
> + return irq_status;
>
> - if (regmap_read(adv7511->regmap_cec,
> - ADV7511_REG_CEC_RX_STATUS + offset, &rx_status))
> - return;
> + ret = regmap_read(adv7511->regmap_cec,
> + ADV7511_REG_CEC_RX_STATUS + offset, &rx_status);
> + if (ret < 0)
> + return ret;
Ok, maybe I was wrong with my previous suggestion. The code starts to
look more and more clumsy. Do we really care about error status at all?
Maybe it's enough to return IRQ_NONE here from the IRQ handlers?
>
> /*
> * ADV7511_REG_CEC_RX_STATUS[5:0] contains the reception order of RX
> @@ -172,6 +176,8 @@ void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
>
> adv7511_cec_rx(adv7511, rx_buf);
> }
> +
> + return IRQ_HANDLED;
> }
>
> static int adv7511_cec_adap_enable(struct cec_adapter *adap, bool enable)
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 66ccb61e2a66..56dd2d5a0376 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -469,6 +469,8 @@ static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd)
> {
> unsigned int irq0, irq1;
> int ret;
> + int cec_status;
cec_status ends up being unset if CEC is disabled.
> + int irq_status = IRQ_NONE;
>
> ret = regmap_read(adv7511->regmap, ADV7511_REG_INT(0), &irq0);
> if (ret < 0)
> @@ -478,38 +480,41 @@ 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 data */
> - if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) &&
> - !(irq1 & ADV7511_INT1_DDC_ERROR))
> - return -ENODATA;
> -
> 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)
> + if (process_hpd && irq0 & ADV7511_INT0_HPD && adv7511->bridge.encoder) {
> schedule_work(&adv7511->hpd_work);
> + irq_status = IRQ_HANDLED;
> + }
>
> 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);
> + irq_status = IRQ_HANDLED;
> }
>
> #ifdef CONFIG_DRM_I2C_ADV7511_CEC
> - adv7511_cec_irq_process(adv7511, irq1);
> + cec_status = adv7511_cec_irq_process(adv7511, irq1);
> +
> + if (cec_status < 0)
> + return cec_status;
> #endif
>
> - return 0;
> + /* If there is no IRQ to handle, exit indicating no IRQ data */
> + if (irq_status == IRQ_HANDLED || cec_status == IRQ_HANDLED)
> + return IRQ_HANDLED;
> +
> + return IRQ_NONE;
> }
>
> 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;
> + return adv7511_irq_process(adv7511, true);
This should be return ret < 0 ? IRQ_NONE : ret. We should not be
returning negative error via irqreturn_t.
> }
>
> /* -----------------------------------------------------------------------------
> --
> 2.43.0
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 5+ messages in thread