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/cover.1666710197.git.mazziesaccount@xxxxxxxxx/ => 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/Y1dzCCMCDswQFVvO@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ 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! ~~
