Re: [PATCH 1/2] i2c: i801: Force no IRQ for Dell Latitude E7450

Jean Delvare
2023-06-04T14:39:01Z

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