Hi Hans,
Thanks for the review.
On Tue, Jan 21, 2025 at 10:42:41AM +0100, Hans de Goede wrote:
> 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;
I'll place func before the u8 fields for fewer holes.
> 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.
I'll address these in v3 along with Ilpo's comments.
--
Kind regards,
Sakari Ailus
