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@xxxxxxxxxxxxxxx .
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,
