The fwnode_irq_get_byname() was changed to not return 0 upon failure so
return value check can be adjusted to reflect the change.
Signed-off-by: Matti Vaittinen <[email protected]>
Reviewed-by: Sakari Ailus <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
Depends on the mentioned return value change which is in patch 1/2. The
return value change does also cause a functional change here. Eg. when
IRQ mapping fails, the fwnode_irq_get_byname() no longer returns zero.
This will cause also the probe here to return nonzero failure. I guess
this is desired behaviour.
---
drivers/i2c/i2c-smbus.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
index 07c92c8495a3..d0cc4b7903ed 100644
--- a/drivers/i2c/i2c-smbus.c
+++ b/drivers/i2c/i2c-smbus.c
@@ -130,7 +130,7 @@ static int smbalert_probe(struct i2c_client *ara,
} else {
irq = fwnode_irq_get_byname(dev_fwnode(adapter->dev.parent),
"smbus_alert");
- if (irq <= 0)
+ if (irq < 0)
return irq;
}
--
2.37.3
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]
On Tue, Oct 25, 2022 at 06:12:11PM +0300, Matti Vaittinen wrote:
> The fwnode_irq_get_byname() was changed to not return 0 upon failure so
> return value check can be adjusted to reflect the change.
...
> Depends on the mentioned return value change which is in patch 1/2. The
> return value change does also cause a functional change here. Eg. when
> IRQ mapping fails, the fwnode_irq_get_byname() no longer returns zero.
> This will cause also the probe here to return nonzero failure. I guess
> this is desired behaviour.
The entire error handling there looks suspicious.
The 'struct i2c_smbus_alert_setup' description says:
"If irq is not specified, the smbus_alert driver doesn't take care of
interrupt handling. In that case it is up to the I2C bus driver to
either handle the interrupts or to poll for alerts."
So, the question is, shouldn't we just drop the check completely?
--
With Best Regards,
Andy Shevchenko
On 10/25/22 19:30, Andy Shevchenko wrote:
> On Tue, Oct 25, 2022 at 06:12:11PM +0300, Matti Vaittinen wrote:
>> The fwnode_irq_get_byname() was changed to not return 0 upon failure so
>> return value check can be adjusted to reflect the change.
>
> ...
>
>> Depends on the mentioned return value change which is in patch 1/2. The
>> return value change does also cause a functional change here. Eg. when
>> IRQ mapping fails, the fwnode_irq_get_byname() no longer returns zero.
>> This will cause also the probe here to return nonzero failure. I guess
>> this is desired behaviour.
>
> The entire error handling there looks suspicious.
>
> The 'struct i2c_smbus_alert_setup' description says:
>
> "If irq is not specified, the smbus_alert driver doesn't take care of
> interrupt handling. In that case it is up to the I2C bus driver to
> either handle the interrupts or to poll for alerts."
>
> So, the question is, shouldn't we just drop the check completely?
I don't really know what this means. Does it mean that if IRQ is not
provided, the driver needs to take care of alerts (in which case the
check here is very valid because IRQ is required for smbus_alert
driver). Or does it mean that only the IRQ handling is omitted while the
smbus_alert driver should do all the other stuff (what ever that is) as
usual. In this case this check indeed feels wrong.
I would appreciate someone with more insight to this driver to take a
look at it.
Yours
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On 10/27/22 08:40, Matti Vaittinen wrote:
> On 10/25/22 19:30, Andy Shevchenko wrote:
>> On Tue, Oct 25, 2022 at 06:12:11PM +0300, Matti Vaittinen wrote:
>>> The fwnode_irq_get_byname() was changed to not return 0 upon failure so
>>> return value check can be adjusted to reflect the change.
>>
>> ...
>>
>>> Depends on the mentioned return value change which is in patch 1/2. The
>>> return value change does also cause a functional change here. Eg. when
>>> IRQ mapping fails, the fwnode_irq_get_byname() no longer returns zero.
>>> This will cause also the probe here to return nonzero failure. I guess
>>> this is desired behaviour.
>>
>> The entire error handling there looks suspicious.
>>
>> The 'struct i2c_smbus_alert_setup' description says:
>>
>> "If irq is not specified, the smbus_alert driver doesn't take care of
>> interrupt handling. In that case it is up to the I2C bus driver to
>> either handle the interrupts or to poll for alerts."
>>
>> So, the question is, shouldn't we just drop the check completely?
>
> I don't really know what this means. Does it mean that if IRQ is not
> provided, the driver needs to take care of alerts (in which case the
> check here is very valid because IRQ is required for smbus_alert
> driver). Or does it mean that only the IRQ handling is omitted while the
> smbus_alert driver should do all the other stuff (what ever that is) as
> usual. In this case this check indeed feels wrong.
>
> I would appreciate someone with more insight to this driver to take a
> look at it.
Wolfram, do you have the required insight?
What would be the best way to proceed? I see 3 options:
1. fix the return value as is done by this series.
https://lore.kernel.org/all/[email protected]/
=> Will cause the i2c-smbus probe to return failure also if IRQ
mapping fails.
2. apply the 1/1 from the series "as is" - but drop the return value
check for fwnode_irq_get_byname() altogether as was suggested by Andy
3. drop this series and apply the documentation fix suggested in:
https://lore.kernel.org/all/[email protected]/
Thoughts anyone?
Yours
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~