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.
>> };
>>
>>> +
>>> +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.
Great, thank you.
Regards,
Hans
