Hi Hans,
On Tue, Jan 21, 2025 at 01:35:49PM +0100, Hans de Goede wrote:
> Hi,
>
> On 21-Jan-25 12:11 PM, Sakari Ailus wrote:
> > 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.
>
> Hmm, by itself that is a good idea, but ...
>
> >> unsigned int polarity;
> >> };
> >>
> >> static const struct int3472_gpio_map[] = {
> >> { "INT347E", INT3472_GPIO_TYPE_RESET, INT3472_GPIO_TYPE_RESET, "enable", GPIO_ACTIVE_HIGH },
>
> ... that changes this line from the above to:
>
> { "INT347E", "enable", INT3472_GPIO_TYPE_RESET, INT3472_GPIO_TYPE_RESET, GPIO_ACTIVE_HIGH },
>
> which IMHO is slightly less logical/readable. I would like to keep
> the ACPI-hid and the type_from next to each other. So I have a slight
> preference for keeping things as I suggested.
Works for me.
--
Sakari Ailus
