Re: [PATCH] platform/x86: serial-multi-instantiate: allow single GpioInt IRQ for INT3515

[Hans de Goede <hdegoede@redhat.com>]

Hi Michał,

On 12/12/23 01:39, Michał Kopeć wrote:
> On some devices, such as the Lenovo ThinkPad T14 Gen1 (AMD), there is only one
> GpioInt resource defined for all i2c device instances. Handle this case
> appropriately by autodetecting the irq type and allowing fallback to the first
> IRQ index for the second, third and fourth tps6598x instances.

This suggests that the IRQ at index 0 gets used for all i2c_clients
if there is no IRQ at index > 0, but that is not what this patch
is actually doing, it is assigning the error value to i2c_client->irq,
which will then get passed to the i2c-driver for the client which
may very well expect that any value other then 0 actually is a valid IRQ.

So what do you actually want to do here, use the IRQ at index 0 as
shared IRQ for all clients (seems sensible) or just give the
other clients no IRQ? If you want to give them no IRQ then please
make smi_get_irq() return 0 when there is an error and the optional
flag is set.

Note one more review remark inline below.

> Additionally, to use the `platform_get_irq_optional` function to silence errors
> that may not be relevant if the IRQ is optional. In cases where the IRQ is not
> optional, `dev_err_probe` is still triggered, so other devices will not be
> affected by this change.
> 
> Signed-off-by: Michał Kopeć <michal@nozomi.space>
> ---
> .../platform/x86/serial-multi-instantiate.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/platform/x86/serial-multi-instantiate.c b/drivers/platform/x86/serial-multi-instantiate.c
> index 8158e3cf5d6d..1c4cc44d5a88 100644
> --- a/drivers/platform/x86/serial-multi-instantiate.c
> +++ b/drivers/platform/x86/serial-multi-instantiate.c
> @@ -23,6 +23,8 @@
> #define IRQ_RESOURCE_APIC 2
> #define IRQ_RESOURCE_AUTO 3
> 
> +#define IRQ_OPTIONAL BIT(2)
> +
> enum smi_bus_type {
>  SMI_I2C,
>  SMI_SPI,
> @@ -59,7 +61,7 @@ static int smi_get_irq(struct platform_device *pdev, struct acpi_device *adev,
>    dev_dbg(&pdev->dev, "Using gpio irq\n");
>    break;
>   }
> - ret = platform_get_irq(pdev, inst->irq_idx);
> + ret = platform_get_irq_optional(pdev, inst->irq_idx);
>   if (ret > 0) {
>    dev_dbg(&pdev->dev, "Using platform irq\n");
>    break;
> @@ -69,12 +71,12 @@ static int smi_get_irq(struct platform_device *pdev, struct acpi_device *adev,
>   ret = acpi_dev_gpio_irq_get(adev, inst->irq_idx);
>   break;
>  case IRQ_RESOURCE_APIC:
> - ret = platform_get_irq(pdev, inst->irq_idx);
> + ret = platform_get_irq_optional(pdev, inst->irq_idx);
>   break;
>  default:
>   return 0;
>  }
> - if (ret < 0)
> + if (ret < 0 && !inst->flags & IRQ_OPTIONAL)
>   return dev_err_probe(&pdev->dev, ret, "Error requesting irq at index %d\n",
>          inst->irq_idx);
> 
> @@ -210,6 +212,8 @@ static int smi_i2c_probe(struct platform_device *pdev, struct smi *smi,
>   board_info.dev_name = name;
> 
>   ret = smi_get_irq(pdev, adev, &inst_array[i]);
> + if (ret < 0 && inst_array[i].flags & IRQ_OPTIONAL)
> + ret = smi_get_irq(pdev, adev, &inst_array[0]);

It seems something went wrong with the patch here, you now
have both the old and the new code here.

Note that when you make  smi_get_irq() return 0 for "no-irq"
instead of an error you do not need to make any changes here
at all.

For v2 please also send this patch to platform-driver-x86@vger.kernel.org .

Regards,

Hans



>   if (ret < 0)
>    goto error;
>   board_info.irq = ret;
> @@ -309,10 +313,11 @@ static const struct smi_node bsg2150_data = {
> 
> static const struct smi_node int3515_data = {
>  .instances = {
> - { "tps6598x", IRQ_RESOURCE_APIC, 0 },
> - { "tps6598x", IRQ_RESOURCE_APIC, 1 },
> - { "tps6598x", IRQ_RESOURCE_APIC, 2 },
> - { "tps6598x", IRQ_RESOURCE_APIC, 3 },
> + { "tps6598x", IRQ_RESOURCE_AUTO, 0 },
> + /* On some platforms only one shared GpioInt is defined */
> + { "tps6598x", IRQ_RESOURCE_AUTO | IRQ_OPTIONAL, 1 },
> + { "tps6598x", IRQ_RESOURCE_AUTO | IRQ_OPTIONAL, 2 },
> + { "tps6598x", IRQ_RESOURCE_AUTO | IRQ_OPTIONAL, 3 },
>   {}
>  },
>  .bus_type = SMI_I2C,