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

[Hans de Goede <hdegoede@redhat.com>]

Hi Saikari,

On 21-Jan-25 9:08 AM, Sakari Ailus wrote:
> Hi Hans,
> 
> On Mon, Jan 20, 2025 at 04:08:45PM +0100, Hans de Goede wrote:
>> Hi Sakari,
>>
>> On 20-Jan-25 3:21 PM, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> On Mon, Jan 20, 2025 at 02:39:39PM +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@linux.intel.com>
>>>>
>>>> Sorry but no this won't fly. If we go this route the amount of
>>>> quirk code in the int3472 driver is going to get out of control.
>>>>
>>>> Since you are matching this mapping on the sensor-type, this
>>>> should be handled in sensor specific code, IOW in the sensor driver.
>>>>
>>>> Also see my reply on the linux-media list here:
>>>>
>>>> https://lore.kernel.org/linux-media/0a0b765e-b342-4433-9c6c-07da78f0f63b@redhat.com/
>>>>
>>>> Sorry but I have to hard nack this. There has to be some line
>>>> where the pdx86 glue code stops bending over backwards and
>>>> where some of the burden of supporting more then just devicetree
>>>> moves to the sensor drivers.
>>>>
>>>> *especially* since this mapping is going to be different per sensor-driver,
>>>> with there being *no consistency at all* in the GPIO/pin names used in
>>>> the sensor drivers just looking at drivers/media/i2c/ov*.c I see all of:
>>>
>>> I beg you to reconsider as I have to disagree with your assessment, for the
>>> following reasons:
>>>
>>> The "reset" naming used by the int3472 driver comes from the int3472 driver
>>> / Windows specific ACPI amendments, not from the ACPI standard nor sensor
>>> datasheets.
>>
>> Yet looking at the datasheet it is a more correct name then "enable".
> 
> Possibly in this instance, but whether it is or not is still besides the
> point: this may not be the name on the datasheet.
> 
>>
>>> Also in a proper ACPI implementation we wouldn't be dealing
>>> with such GPIOs at all, instead this would simply work through ACPI power
>>> resources.
>>
>> GPIOs being handles as ACPI power-resources is not something which is
>> typically done.
>>
>> This was actually done correct for the atomisp devices, clks and regulators
>> are power-resources there, but the GPIOs are listed as plain ACPI GPIO
>> resources under the sensor ACPI-fwnode. And ACPI GPIO resources do not
>> have a name/label only an index. So drivers which need GPIOs and want 
>> to work on ACPI platforms need an index -> label map, see for example:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/input/touchscreen/goodix.c#n811
>>
>> note how this is handled in the driver, and is not expected to be
>> handled by some platform specific code.
> 
> The power resources do contain GPIO handling when it is required for
> implementing the device's power on / off sequences. FWIW, on Chromebooks
> this "just works" without the IPU bridge and without .
> 
>>
>>> There generally isn't a single datasheet name used for such signals across
>>> sensor vendors (or even sensors from a single vendor). Thus the assumption
>>> made in the int3472 driver isn't correct, even if DT bindings were using
>>> the vendor-provided GPIO signal name from datasheets as-is.
>>>
>>> We've done quite a bit of work to make the system firmware (including
>>> design differences or outright bugs) invisible to the drivers elsewhere, I
>>> don't see why we should make an exception here. To add to that, mapping the
>>> GPIO name / function to what the driver expects is trivially done in the
>>> int3472 driver, as this patch shows.
>>
>> IMHO it is not so trivial done, you are assuming a 1:1 mapping for all
>> laptop/tablet models this is not necessarily true. E.g. on atomisp tablets
>> there typically is a standard camera connector with both reset and powerdown
>> signals and the atomisp _DSM equivalent of the INT3472 GPIO map _DSM
>> typically contains both signals. But on sensors with only 1 reset pin
>> it is unclear which of the 2 is actually used. One model camera module
>> with sensor X may connect the sensor's single xshutdown pin to
>> the powerdown signal on the standard camera connector, while another
>> camera module may use the reset signal on the standard connector.
>>
>> There are 2 ways to handle this:
>>
>> 1. Make the driver deal with the fact that there may be 2 GPIOs,
>> treating both as optional and driving both low/high at the same time
>> since only 1 will actually be used. This is somewhat ugly on the driver
>> side, but then fixes things for all tablets/laptops out there using
>> this sensor model.
>>
>> 2. Make this the platform glue's problem and require the platform code
>> to have per laptop/tablet model quirks using DMI matching meaning that
>> instead of things just working OOTB for models not added to the quirk
>> table, we now need users to report an issue and then manually fix
>> that for every model under the sun. Which is very much sub optimal.
> 
> In our case here there's really no difference DMI-wise, is there? In both
> cases the GPIO handling is determined based on the device. Besides, my
> patch is technically better in the sense that it is explicitly using an
> existing firmware interface instead of adding driver support for a firmware
> interface that doesn't exist (neither as a firmware interface definition
> nor an implementation of one).
> 
>>
>> See e.g.:
>>
>> https://github.com/jwrdegoede/linux-sunxi/commit/e43be8f19b5913a2e4bd715e7eef88fd909a2d1d
> 
> I guess we don't have DT bindings for a (virtual) device that would allow
> controlling one or more actual GPIOs using a single virtual one?
> 
>>
>> (which I have not posted upstream yet since I don't have the mt9m114
>> driver working on atomisp models yet)
>>
>> I foresee similar problems with the INT3472 stuff. On Andy's device
>> we need to map reset to enable, but maybe next time it is powerdown ?
>>
>> Multiply these kinda per laptop/tablet model issues by the amount
>> of different sensors which there are and this becomes a big 
>> mess of per-sensor + per-laptop-model quirks in the int3472 code,
>> where as these things can typically be handled reasonable well
>> inside the sensor driver.
> 
> Again, I don't want to add DMI checks in client drivers if that can
> be reasonably handled elsewhere.

Ok, good that was my main concern.

<snip>

As discussed on IRC lets move forward with doing the mapping in the INT3472
as the patch from this thread does.

But I do want to see the mapping code be a bit more generic, I'll reply
to the original patch with a suggestion on how to do that.

Regards,

Hans