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

[Kate Hsuan]

Hi Tarang,

Thank you for the update.

On Thu, Jul 16, 2026 at 4:44 AM Tarang Raval
<tarang.raval@xxxxxxxxxxxxxxxxx> wrote:
>
> Hi Kate,
>
> > Hi Tarang,
> >
> > Thank you for working on this.
> >
> > On Sat, Jul 11, 2026 at 7:16 AM Tarang Raval
> > <tarang.raval@xxxxxxxxxxxxxxxxx> 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.
> > >
> > > Suggested-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > > Signed-off-by: Tarang Raval <tarang.raval@xxxxxxxxxxxxxxxxx>
> > > ---
> > >
> > > Identified while reviewing Kate Hsuan's imx471 vana con_id patch, whose
> > > two near-identical SONY471A/TBE20A0 entries are collapsed here into
> > > one, backed by a new power_enable_hids_vana[] array.
> > >
> > > This patch is on top of kate's imx471 driver series[1].
> > >
> > > Link: https://lore.kernel.org/linux-media/20260629074026.35490-1-hpa@xxxxxxxxxx/T/#t [1].
> > >
> > > ---
> > >  drivers/platform/x86/intel/int3472/discrete.c | 49 ++++++++++++-------
> > >  1 file changed, 30 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
> > > index adff564bf3fd..f86333e38341 100644
> > > --- a/drivers/platform/x86/intel/int3472/discrete.c
> > > +++ b/drivers/platform/x86/intel/int3472/discrete.c
> > > @@ -123,10 +123,21 @@ 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[] = {
> > Should we have a commit mentioning which laptop model exposed the HID?
> > For example
> >             /* Lenovo X9-14 and X9-15 */
> > > +       "SONY471A",
> >             /* Lenovo X1 Carbon G14 */
>
> I'll update in v2.
>
> > > +       "TBE20A0",
> > > +       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 +146,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 +156,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 = (const char * const[]) { "INT33F0", NULL },
> >
> > I think it is bettter to keep the consistent style, the hids array can
> > be declared individually like you did for the sony sensor.
> >
> > for example,
> > static const char * const power_enable_hids_vdd[] = {
> >       /* mt9m114 */
> >       "INT33F0",
> >       NULL
> > };
> >
> > ...
> >
> >   .hids = power_enable_hids_vdd;
>
> I'll update in v2.
>
> > >                 .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 = (const char * const[]) { "INT347E", NULL },
> > same here.
> >
> > >                 .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 },
> > same here.
>
> This shouldn't be a named array like power_enable_hids_vana. Unlike vana,
> this entry's enable_time_us (45ms) is a HID-specific quirk, not a shared
> property, so a generic power_enable_hids_dvdd array would wrongly imply
> future HIDs added to it inherit that same delay.
>
> Except for this one, I'll declare the other HID arrays individually.
I think keeping the inconsistent style is better for readability, but
let's wait for input from the other reviewers.

>
> > >                 .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 +184,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)
> > > +{
> >
> > It is better to have a NULL guard here.
> >        if (!hids)
> >                return false;
>
> Why ?

>
> There is no case where .hids can legitimately be NULL, since every
> GPIO mapping entry is meaningless without a HID to match against.
>
> so the NULL check isn't needed.
>
> > > +       for (unsigned int i = 0; hids[i]; i++) {

Dereferencing a NULL hids pointer in the for loop will lead to a
kernel crash.  Verifying the input parameter "hids" before
dereferencing it makes this function more defensive.

> > > +               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 +211,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
> > >
> >
> > I tested this patch and it works.
> >
> > Tested-by: Kate Hsuan <hpa@xxxxxxxxxx>
>
> Thanks for testing this patch.
>
> Best Regards,
> Tarang



-- 
BR,
Kate