Re: [RESEND PATCH v3] platform: int3472: discrete: Support multiple HIDs per GPIO map entry

Hi Tarang,

Thank you for your work.

On Wed, Jul 29, 2026 at 1:58 PM Tarang Raval
<[email protected]> wrote:
>
> Each int3472_gpio_map entry currently maps exactly one ACPI HID to a
> GPIO quirk. As more sensors needing the same quirk are identified, this
> means adding a full duplicate table entry per HID, differing only in
> the HID string, which does not scale.
>
> Change int3472_gpio_map::hid to a NULL-terminated hids array so a
> single entry can match any number of ACPI HIDs, letting new HIDs be
> added to the relevant array instead of duplicating quirk entries.
>
> Cc: [email protected]
> Suggested-by: Sakari Ailus <[email protected]>
> Signed-off-by: Tarang Raval <[email protected]>
> Tested-by: Kate Hsuan <[email protected]>
> Reviewed-by: Hans de Goede <[email protected]>
> Reviewed-by: Daniel Scally <[email protected]>
> ---
> Change-log:
>
> v2 -> v3:
>
> - Move the comments after the HID strings and include the sensor name,
>   as suggested by Hans.
> - Add the Reviewed-by tags from Hans and Daniel.
>
> v1 -> v2:
>
> - Add per-HID comments to power_enable_hids_vana[] (Lenovo X9-14/X9-15
>   for SONY471A, Lenovo X1 Carbon G14 for TBE20A0)
> - Convert INT33F0 (mt9m114), INT347E (ov7251) entries to use named
>   power_enable_hids_*[] arrays instead of anonymous compound literals,
>   for style consistency
> - Did not convert OVTI08F4 (ov08x40) to a named array, and did not add
>   a NULL guard in int3472_gpio_map_hids_match(): not fully convinced
>   these changes are needed yet, but open to further discussion on both
>
> ---
>  drivers/platform/x86/intel/int3472/discrete.c | 59 +++++++++++++------
>  1 file changed, 40 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
> index adff564bf3fd..6c729fcfce5d 100644
> --- a/drivers/platform/x86/intel/int3472/discrete.c
> +++ b/drivers/platform/x86/intel/int3472/discrete.c
> @@ -123,10 +123,31 @@ skl_int3472_gpiod_get_from_temp_lookup(struct int3472_discrete_device *int3472,
>         return desc;
>  }
>
> +/*
> + * Other vana-supply users (e.g. ST, Toshiba, Sony sensors) can be added to
> + * this array instead of adding new quirk table entries.
> + */
> +static const char * const power_enable_hids_vana[] = {
> +       "SONY471A", /* imx471 on Lenovo X9-14 and X9-15 */
> +       "TBE20A0", /* imx471 on Lenovo X1 Carbon G14 */
> +       NULL
> +};
> +
> +static const char * const power_enable_hids_vdd[] = {
> +       "INT33F0", /* mt9m114 */
> +       NULL
> +};
> +
> +static const char * const power_enable_hids_enable[] = {
> +       "INT347E", /* ov7251 */
> +       NULL
> +};
> +
>  /**
>   * struct int3472_gpio_map - Map GPIOs to whatever is expected by the
>   * sensor driver (as in DT bindings)
> - * @hid: The ACPI HID of the device without the instance number e.g. INT347E
> + * @hids: NULL-terminated array of ACPI HIDs of the devices without the
> + * instance number e.g. INT347E
>   * @type_from: The GPIO type from ACPI ?SDT
>   * @type_to: The assigned GPIO type, typically same as @type_from
>   * @enable_time_us: Enable time in usec for GPIOs mapped to regulators
> @@ -135,7 +156,7 @@ skl_int3472_gpiod_get_from_temp_lookup(struct int3472_discrete_device *int3472,
>   * GPIO_ACTIVE_HIGH otherwise
>   */
>  struct int3472_gpio_map {
> -       const char *hid;
> +       const char * const *hids;
>         u8 type_from;
>         u8 type_to;
>         bool polarity_low;
> @@ -145,38 +166,27 @@ struct int3472_gpio_map {
>
>  static const struct int3472_gpio_map int3472_gpio_map[] = {
>         {       /* mt9m114 designs declare a powerdown pin which controls the regulators */
> -               .hid = "INT33F0",
> +               .hids = power_enable_hids_vdd,
>                 .type_from = INT3472_GPIO_TYPE_POWERDOWN,
>                 .type_to = INT3472_GPIO_TYPE_POWER_ENABLE,
>                 .con_id = "vdd",
>                 .enable_time_us = GPIO_REGULATOR_ENABLE_TIME,
>         },
>         {       /* ov7251 driver / DT-bindings expect "enable" as con_id for reset */
> -               .hid = "INT347E",
> +               .hids = power_enable_hids_enable,
>                 .type_from = INT3472_GPIO_TYPE_RESET,
>                 .type_to = INT3472_GPIO_TYPE_RESET,
>                 .con_id = "enable",
>         },
>         {       /* ov08x40's handshake pin needs a 45 ms delay on some HP laptops */
> -               .hid = "OVTI08F4",
> +               .hids = (const char * const[]) { "OVTI08F4", NULL },
>                 .type_from = INT3472_GPIO_TYPE_HANDSHAKE,
>                 .type_to = INT3472_GPIO_TYPE_HANDSHAKE,
>                 .con_id = "dvdd",
>                 .enable_time_us = 45 * USEC_PER_MSEC,
>         },
> -       {       /* imx471 expects "vana" as con_id for power enable */
> -               .hid = "SONY471A",
> -               .type_from = INT3472_GPIO_TYPE_POWER_ENABLE,
> -               .type_to = INT3472_GPIO_TYPE_POWER_ENABLE,
> -               .con_id = "vana",
> -               .enable_time_us = GPIO_REGULATOR_ENABLE_TIME,
> -       },
> -       {
> -               /*
> -                * imx471 (on Lenovo ThinkPads X1 G14) expects "vana" as con_id
> -                * for power enable
> -                */
> -               .hid = "TBE20A0",
> +       {       /* Sensors which expect "vana" as con_id for power enable */
> +               .hids = power_enable_hids_vana,
>                 .type_from = INT3472_GPIO_TYPE_POWER_ENABLE,
>                 .type_to = INT3472_GPIO_TYPE_POWER_ENABLE,
>                 .con_id = "vana",
> @@ -184,6 +194,17 @@ static const struct int3472_gpio_map int3472_gpio_map[] = {
>         },
>  };
>
> +static bool int3472_gpio_map_hids_match(struct acpi_device *adev,
> +                                       const char * const *hids)
> +{
> +       for (unsigned int i = 0; hids[i]; i++) {
> +               if (acpi_dev_hid_uid_match(adev, hids[i], NULL))
> +                       return true;
> +       }
> +
> +       return false;
> +}
> +
>  static void int3472_get_con_id_and_polarity(struct int3472_discrete_device *int3472, u8 *type,
>                                             const char **con_id, unsigned long *gpio_flags,
>                                             unsigned int *enable_time_us)
> @@ -200,7 +221,7 @@ static void int3472_get_con_id_and_polarity(struct int3472_discrete_device *int3
>                 if (*type != int3472_gpio_map[i].type_from)
>                         continue;
>
> -               if (!acpi_dev_hid_uid_match(adev, int3472_gpio_map[i].hid, NULL))
> +               if (!int3472_gpio_map_hids_match(adev, int3472_gpio_map[i].hids))
>                         continue;
>
>                 dev_dbg(int3472->dev, "mapping type 0x%02x pin to 0x%02x %s\n",
> --
> 2.34.1
>

It is good for me.

Reviewed-by: Kate Hsuan <[email protected]>


-- 
BR,
Kate