Re: [PATCH 2/8] i2c: i801: make FEATURE_HOST_NOTIFY dependent on FEATURE_IRQ

[Jean Delvare <jdelvare@suse.de>]

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).

-- 
Jean Delvare
SUSE L3 Support