* [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