* [PATCH] platform: int3472: discrete: Support multiple HIDs per GPIO map entry
@ 2026-07-11 11:13 Tarang Raval
2026-07-16 7:19 ` Kate Hsuan
0 siblings, 1 reply; 4+ messages in thread
From: Tarang Raval @ 2026-07-11 11:13 UTC (permalink / raw)
To: sakari.ailus, hansg
Cc: hpa, Tarang Raval, Daniel Scally, Ilpo Järvinen,
platform-driver-x86, linux-kernel
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@linux.intel.com>
Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
---
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@redhat.com/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[] = {
+ "SONY471A",
+ "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 },
.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 },
.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 +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)
+{
+ 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 +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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] platform: int3472: discrete: Support multiple HIDs per GPIO map entry
2026-07-11 11:13 [PATCH] platform: int3472: discrete: Support multiple HIDs per GPIO map entry Tarang Raval
@ 2026-07-16 7:19 ` Kate Hsuan
2026-07-16 8:43 ` Tarang Raval
0 siblings, 1 reply; 4+ messages in thread
From: Kate Hsuan @ 2026-07-16 7:19 UTC (permalink / raw)
To: Tarang Raval
Cc: sakari.ailus, hansg, Daniel Scally, Ilpo Järvinen,
platform-driver-x86, linux-kernel
Hi Tarang,
Thank you for working on this.
On Sat, Jul 11, 2026 at 7:16 AM Tarang Raval
<tarang.raval@siliconsignals.io> 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@linux.intel.com>
> Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
> ---
>
> 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@redhat.com/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 */
> + "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;
> .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.
> .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;
> + 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 +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@redhat.com>
--
BR,
Kate
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] platform: int3472: discrete: Support multiple HIDs per GPIO map entry
2026-07-16 7:19 ` Kate Hsuan
@ 2026-07-16 8:43 ` Tarang Raval
2026-07-17 3:50 ` Kate Hsuan
0 siblings, 1 reply; 4+ messages in thread
From: Tarang Raval @ 2026-07-16 8:43 UTC (permalink / raw)
To: Kate Hsuan
Cc: sakari.ailus, hansg, Daniel Scally, Ilpo Järvinen,
platform-driver-x86, linux-kernel
Hi Kate,
> Hi Tarang,
>
> Thank you for working on this.
>
> On Sat, Jul 11, 2026 at 7:16 AM Tarang Raval
> <tarang.raval@siliconsignals.io> 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@linux.intel.com>
> > Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
> > ---
> >
> > 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@redhat.com/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.
> > .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++) {
> > + 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@redhat.com>
Thanks for testing this patch.
Best Regards,
Tarang
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] platform: int3472: discrete: Support multiple HIDs per GPIO map entry
2026-07-16 8:43 ` Tarang Raval
@ 2026-07-17 3:50 ` Kate Hsuan
0 siblings, 0 replies; 4+ messages in thread
From: Kate Hsuan @ 2026-07-17 3:50 UTC (permalink / raw)
To: Tarang Raval
Cc: sakari.ailus, hansg, Daniel Scally, Ilpo Järvinen,
platform-driver-x86, linux-kernel
Hi Tarang,
Thank you for the update.
On Thu, Jul 16, 2026 at 4:44 AM Tarang Raval
<tarang.raval@siliconsignals.io> wrote:
>
> Hi Kate,
>
> > Hi Tarang,
> >
> > Thank you for working on this.
> >
> > On Sat, Jul 11, 2026 at 7:16 AM Tarang Raval
> > <tarang.raval@siliconsignals.io> 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@linux.intel.com>
> > > Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
> > > ---
> > >
> > > 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@redhat.com/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@redhat.com>
>
> Thanks for testing this patch.
>
> Best Regards,
> Tarang
--
BR,
Kate
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-07-17 3:50 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-07-11 11:13 [PATCH] platform: int3472: discrete: Support multiple HIDs per GPIO map entry Tarang Raval
2026-07-16 7:19 ` Kate Hsuan
2026-07-16 8:43 ` Tarang Raval
2026-07-17 3:50 ` Kate Hsuan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox