Re: [PATCH v3 1/1] platform/x86: int3472: Call "reset" GPIO "enable" for INT347E

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, Jan 22, 2025 at 12:43:44PM +0200, 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. "reset" with
> active low polarity remains the default GPIO name for other devices.

...

> +/**
> + * 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. i2c-INT347E

W/o "i2c-" part.

> + * @type_from: The GPIO type from ACPI ?SDT
> + * @type_to: The assigned GPIO type, typically same as type_from

@type_from

> + * @func: The function, e.g. "enable"

Should we speak in terms of GPIOLIB, like connection ID ?

> + * @polarity: GPIO_ACTIVE_{HIGH,LOW}

Please, avoid using patterns with the defined constants. It's better to have
this written as

 * @polarity: One of %GPIO_ACTIVE_HIGH, %GPIO_ACTIVE_LOW

> + */

> +	const char *hid;
> +	u8 type_from;
> +	u8 type_to;
> +	const char *func;
> +	unsigned int polarity;

Hmm... In other cases we usually use

	bool active_low;

Can we do the same here?

> +};

...

> -	int3472_get_func_and_polarity(type, &func, &polarity);
> +	int3472_get_func_and_polarity(int3472->sensor, &type, &func,
> +				      &polarity);

AFAIK, we don't have hard attachment to the 80-[character limit rule, please
use more room on the previous line.

-- 
With Best Regards,
Andy Shevchenko