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;
Message-ID: <2648642f-3078-aebf-7920-7eceed044dc8@mariushoch.de>
Date:   Sun, 18 Jun 2023 15:08:03 +0200
MIME-Version: 1.0
Subject: Re: [PATCH 1/2] i2c: i801: Force no IRQ for Dell Latitude E7450
Content-Language: en-US
To:     Jean Delvare <jdelvare@suse.de>
Cc:     linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230514103634.235917-1-mail@mariushoch.de>
 <20230514103634.235917-2-mail@mariushoch.de>
 <20230604163855.5b7ea1e0@endymion.delvare>
From:   Marius Hoch <mail@mariushoch.de>
In-Reply-To: <20230604163855.5b7ea1e0@endymion.delvare>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Jean,

On 04/06/2023 16:38, Jean Delvare wrote:
> 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?
If irq_managed is not set our irq value will be entirely ignored it 
seems (thus leading to the same code path/ failure initially outlined).
>
>> +	}
>> +
>>   	err = pcim_enable_device(dev);
>>   	if (err) {
>>   		dev_err(&dev->dev, "Failed to enable SMBus PCI device (%d)\n",
>