Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 4 Jun 2023 16:38:55 +0200
From:   Jean Delvare <jdelvare@suse.de>
To:     Marius Hoch <mail@mariushoch.de>
Cc:     linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] i2c: i801: Force no IRQ for Dell Latitude E7450
Message-ID: <20230604163855.5b7ea1e0@endymion.delvare>
In-Reply-To: <20230514103634.235917-2-mail@mariushoch.de>
References: <20230514103634.235917-1-mail@mariushoch.de>
        <20230514103634.235917-2-mail@mariushoch.de>
Organization: SUSE Linux
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Sun, 14 May 2023 12:36:33 +0200, Marius Hoch wrote:
> The Dell Latitude E7450 uses IRQ 18 for the accelerometer,
> but also claims that the SMBus uses IRQ 18. This will
> result in:
> 
> i801_smbus 0000:00:1f.3: PCI INT C: failed to register GSI
> i801_smbus 0000:00:1f.3: Failed to enable SMBus PCI device (-16)
> i801_smbus: probe of 0000:00:1f.3 failed with error -16
> 
> Force the SMBus IRQ to IRQ_NOTCONNECTED in this case, so that
> we fall back to polling, which also seems to be what the (very
> dated) Windows 7 drivers on the Dell Latitude E7450 do.
> 
> This was tested on Dell Latitude E7450.
> 
> Signed-off-by: Marius Hoch <mail@mariushoch.de>
> ---
>  drivers/i2c/busses/i2c-i801.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index ac5326747c51..5fd2ac585160 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1624,6 +1624,20 @@ static void i801_setup_hstcfg(struct i801_priv *priv)
>  	pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hstcfg);
>  }
>  
> +/**

As reported by the kernel test robot, please don't start a comment with
/** unless it's a kernel-doc-style comment.

> + * These DELL devices claim an IRQ for the SMBus (usually 18), but we can't use
> + * it, as its actually for the I2C accelerometer.
> + */
> +static const struct dmi_system_id dmi_force_no_irq[] = {
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E7450"),
> +		},
> +	},
> +	{} /* Terminating entry */
> +};
> +
>  static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  {
>  	int err, i;
> @@ -1657,6 +1671,12 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	if (!(priv->features & FEATURE_BLOCK_BUFFER))
>  		priv->features &= ~FEATURE_BLOCK_PROC;
>  
> +	if (dmi_check_system(dmi_force_no_irq)) {

If the problem is caused by dev->irq being 255, and now that we know
that this value is special on x86, wouldn't it make more sense to
restrict this quirk to CONFIG_X86 and simply check for dev->irq ==
0xff? This would save us the extra effort of maintaining a list of
machines which need the quirk.

> +		/* Force no IRQ for these devices, otherwise pcim_enable_device will fail */
> +		dev->irq = IRQ_NOTCONNECTED;
> +		dev->irq_managed = 1;

This field is undocumented so I have no idea what it does. Is it not
sufficient to set irq to IRQ_NOTCONNECTED?

> +	}
> +
>  	err = pcim_enable_device(dev);
>  	if (err) {
>  		dev_err(&dev->dev, "Failed to enable SMBus PCI device (%d)\n",


-- 
Jean Delvare
SUSE L3 Support