Hi Sakari,
On 20-Jan-25 11:17 AM, Sakari Ailus wrote:
> The DT bindings for ov7251 specify "enable" GPIO (xshutdown in
> documentation) but the int3472 indiscriminately provides this as a "reset"
> GPIO to sensor drivers. Take this into account by assigning it as "enable"
> with active high polarity for INT347E devices, i.e. ov7251.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> ---
> since v1:
>
> - Fixed device name string.
>
> drivers/platform/x86/intel/int3472/discrete.c | 45 ++++++++++++++++---
> 1 file changed, 40 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
> index d881b2cfcdfc..6404ef1eb4a7 100644
> --- a/drivers/platform/x86/intel/int3472/discrete.c
> +++ b/drivers/platform/x86/intel/int3472/discrete.c
> @@ -122,13 +122,47 @@ skl_int3472_gpiod_get_from_temp_lookup(struct int3472_discrete_device *int3472,
> return desc;
> }
>
> -static void int3472_get_func_and_polarity(u8 type, const char **func, u32 *polarity)
> +/**
> + * struct int3472_reset_gpio_map - Map "reset" GPIO to whatever is expected by
> + * the sensor driver (as in DT bindings)
> + * @devname: The name of the device without the instance number e.g. i2c-INT347E
> + * @func: The function, e.g. "enable"
> + * @polarity: GPIO_ACTIVE_{HIGH,LOW}
> + */
> +static const struct int3472_reset_gpio_map {
> + const char *devname;
Instead of using a devname match this should be changed to using
proper ACPI HID matching.
Instead of passing int3472->sensor_name as extra argument to
int3472_get_func_and_polarity(), pass int3472->sensor which
is a "struct acpi_device *" and then use e.g. :
acpi_dev_hid_uid_match(int3472->sensor, "INT347E", NULL)
but then with the new sensor function argument
> + const char *func;
> + unsigned int polarity;
> +} int3472_reset_gpio_map[] = {
> + { "i2c-INT347E", "enable", GPIO_ACTIVE_HIGH },
> +};
Rather then having a mechanism for just the reset pin, I would prefer
to be able to remap any type to any type.
So I would like to see this struct changed to e.g. :
static struct int3472_gpio_map {
const char *hid;
u8 type_from;
u8 type_to;
const char *func;
unsigned int polarity;
};
static const struct int3472_gpio_map[] = {
{ "INT347E", INT3472_GPIO_TYPE_RESET, INT3472_GPIO_TYPE_RESET, "enable", GPIO_ACTIVE_HIGH },
};
> +
> +static void int3472_get_func_and_polarity(const char *sensor_name, u8 type,
And change type to a * here ("u8 *type) so that its contents can be
overwritten by the mapping code
> + const char **func, u32 *polarity)
So the new function prototype would become:
static void int3472_get_func_and_polarity(struct acpi_device *sensor, u8 *type,
const char **func, u32 *polarity)
> {
and do the for loop here before the (now) "switch (*type)":
for (i = 0; i < ARRAY_SIZE(int3472_gpio_map); i++) {
if (*type != int3472_reset_gpio_map[i].type_from ||
!acpi_dev_hid_uid_match(sensor, int3472_reset_gpio_map[i].hid, NULL))
continue;
*type = int3472_reset_gpio_map[i].type_to;
*func = int3472_reset_gpio_map[i].func;
*polarity = int3472_reset_gpio_map[i].polarity;
return;
}
This should give us a lot more flexibility for future mappings.
Regards,
Hans
> switch (type) {
> - case INT3472_GPIO_TYPE_RESET:
> - *func = "reset";
> - *polarity = GPIO_ACTIVE_LOW;
> + case INT3472_GPIO_TYPE_RESET: {
> + const struct int3472_reset_gpio_map *map = NULL;
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(int3472_reset_gpio_map); i++) {
> + if (strncmp(sensor_name, int3472_reset_gpio_map[i].devname,
> + strlen(int3472_reset_gpio_map[i].devname)))
> + continue;
> +
> + map = &int3472_reset_gpio_map[i];
> + break;
> + }
> +
> + if (map) {
> + *func = map->func;
> + *polarity = map->polarity;
> + } else {
> + *func = "reset";
> + *polarity = GPIO_ACTIVE_LOW;
> + }
> break;
> + }
> case INT3472_GPIO_TYPE_POWERDOWN:
> *func = "powerdown";
> *polarity = GPIO_ACTIVE_LOW;
> @@ -217,7 +251,8 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
>
> type = FIELD_GET(INT3472_GPIO_DSM_TYPE, obj->integer.value);
>
> - int3472_get_func_and_polarity(type, &func, &polarity);
> + int3472_get_func_and_polarity(int3472->sensor_name, type, &func,
> + &polarity);
>
> pin = FIELD_GET(INT3472_GPIO_DSM_PIN, obj->integer.value);
> if (pin != agpio->pin_table[0])
