platform/x86: serial-multi-instantiate: allow single GpioInt IRQ for INT3515

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.

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

    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]);
   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,

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,