| Message ID | 08ccd96f-deca-72b9-e14e-917434736ca3@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | i2c: i801: Series with minor improvements | expand |
Hi Heiner, On Fri, 15 Apr 2022 18:55:10 +0200, Heiner Kallweit wrote: > Host notification uses an interrupt, therefore it makes sense only if > interrupts are enabled. It would be nice to have this comment in the code itself. > Make this dependency explicit by removing > FEATURE_HOST_NOTIFY if FEATURE_IRQ isn't set. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/i2c/busses/i2c-i801.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c > index c481f121d..eccdc7035 100644 > --- a/drivers/i2c/busses/i2c-i801.c > +++ b/drivers/i2c/busses/i2c-i801.c > @@ -1756,6 +1756,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) > dev_info(&dev->dev, "SMBus using %s\n", > priv->features & FEATURE_IRQ ? "PCI interrupt" : "polling"); > > + if (!(priv->features & FEATURE_IRQ)) > + priv->features &= ~FEATURE_HOST_NOTIFY; Earlier in this function, there's an action which depends on the FEATURE_HOST_NOTIFY flag being set. While this will only result in a useless action and wouldn't cause a bug as far as I can see, wouldn't it be cleaner to move that piece of code *after* the check you're adding? > + > i801_add_tco(priv); > > snprintf(priv->adapter.name, sizeof(priv->adapter.name), Looks good, tested OK on my system (non-regression only, I'm not using the Host Notify feature).
On 07.06.2022 14:48, Jean Delvare wrote: > Hi Heiner, > > On Fri, 15 Apr 2022 18:55:10 +0200, Heiner Kallweit wrote: >> Host notification uses an interrupt, therefore it makes sense only if >> interrupts are enabled. > > It would be nice to have this comment in the code itself. > OK >> Make this dependency explicit by removing >> FEATURE_HOST_NOTIFY if FEATURE_IRQ isn't set. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/i2c/busses/i2c-i801.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c >> index c481f121d..eccdc7035 100644 >> --- a/drivers/i2c/busses/i2c-i801.c >> +++ b/drivers/i2c/busses/i2c-i801.c >> @@ -1756,6 +1756,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) >> dev_info(&dev->dev, "SMBus using %s\n", >> priv->features & FEATURE_IRQ ? "PCI interrupt" : "polling"); >> >> + if (!(priv->features & FEATURE_IRQ)) >> + priv->features &= ~FEATURE_HOST_NOTIFY; > > Earlier in this function, there's an action which depends on the > FEATURE_HOST_NOTIFY flag being set. While this will only result in a > useless action and wouldn't cause a bug as far as I can see, wouldn't > it be cleaner to move that piece of code *after* the check you're > adding? > Yes, this would be better. Will be part of v2. >> + >> i801_add_tco(priv); >> >> snprintf(priv->adapter.name, sizeof(priv->adapter.name), > > Looks good, tested OK on my system (non-regression only, I'm not using > the Host Notify feature). >
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index c481f121d..eccdc7035 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -1756,6 +1756,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) dev_info(&dev->dev, "SMBus using %s\n", priv->features & FEATURE_IRQ ? "PCI interrupt" : "polling"); + if (!(priv->features & FEATURE_IRQ)) + priv->features &= ~FEATURE_HOST_NOTIFY; + i801_add_tco(priv); snprintf(priv->adapter.name, sizeof(priv->adapter.name),
Host notification uses an interrupt, therefore it makes sense only if interrupts are enabled. Make this dependency explicit by removing FEATURE_HOST_NOTIFY if FEATURE_IRQ isn't set. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/i2c/busses/i2c-i801.c | 3 +++ 1 file changed, 3 insertions(+)