Re: [PATCH 1/6] media: ov5693: Add support for a privacy-led GPIO

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

Hi,

On 12/2/22 12:49, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Fri, Dec 02, 2022 at 12:21:12PM +0100, Hans de Goede wrote:
>> On 12/2/22 11:54, Laurent Pinchart wrote:
>>> On Wed, Nov 30, 2022 at 05:34:55PM +0100, Hans de Goede wrote:
>>>> On 11/30/22 15:52, Sakari Ailus wrote:
>>>>> On Wed, Nov 30, 2022 at 02:56:46PM +0100, Hans de Goede wrote:
>>>>>> On 11/30/22 14:41, Sakari Ailus wrote:
>>>>>>> On Wed, Nov 30, 2022 at 12:11:44AM +0100, Hans de Goede wrote:
>>>>>>>> Add support for a privacy-led GPIO.
>>>>>>>>
>>>>>>>> Making the privacy LED to controlable from userspace, as using the LED
>>>>>>>> class subsystem would do, would make it too easy for spy-ware to disable
>>>>>>>> the LED.
>>>>>>>>
>>>>>>>> To avoid this have the sensor driver directly control the LED.
>>>>>>>>
>>>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>>>> ---
>>>>>>>> Note an additional advantage of directly controlling the GPIO is that
>>>>>>>> GPIOs are tied directly to consumer devices. Where as with a LED class
>>>>>>>> device, there would need to be some mechanism to tie the right LED
>>>>>>>> (e.g front or back) to the right sensor.
>>>>>>>
>>>>>>> Thanks for the patch.
>>>>>>>
>>>>>>> This approach has the drawback that support needs to be added for each
>>>>>>> sensor separately. Any idea how many sensor drivers might need this?
>>>>>>
>>>>>> Quite a few probably. But as discussed here I plan to write a generic
>>>>>> sensor_power helper library since many sensor drivers have a lot of
>>>>>> boilerplate code to get clks + regulators + enable/reset gpios. The plan
>>>>>> is to add support for a "privacy-led" to this library so that all sensors
>>>>>> which use this get support for free.
>>>>>
>>>>> I'm not sure how well this could be generalised. While most sensors do
>>>>> something similar there are subtle differences. If those can be taken into
>>>>> account I guess it should be doable. But would it simplify things or reduce
>>>>> the number of lines of code as a whole?
>>>>>
>>>>> The privacy LED is separate from sensor, including its power on/off
>>>>> sequences which suggests it could be at least as well be handled
>>>>> separately.
>>>>>
>>>>>> Laurent pointed out that some sensors may have more complex power-up
>>>>>> sequence demands, which is true. But looking at existing drivers
>>>>>> then many follow a std simple pattern which can be supported in
>>>>>> a helper-library.
>>>>>>
>>>>>>> Most implementations have privacy LED hard-wired to the sensor's power
>>>>>>> rails so it'll be lit whenever the sensor is powered on.
>>>>>>>
>>>>>>> If there would be more than just a couple of these I'd instead create a LED
>>>>>>> class device and hook it up to the sensor in V4L2.
>>>>>>
>>>>>> A LED cladd device will allow userspace to override the privacy-led
>>>>>> value which is considered bad from a privacy point of view. This
>>>>>> was actually already discussed here:
>>>>>>
>>>>>> https://lore.kernel.org/platform-driver-x86/e5d8913c-13ba-3b11-94bc-5d1ee1d736b0@ideasonboard.com/
>>>>>>
>>>>>> See the part of the thread on the cover-letter with Dan, Laurent
>>>>>> and me participating.
>>>>>>
>>>>>> And a LED class device also will be a challenge to bind to the right
>>>>>> sensor on devices with more then one sensor, where as mentioned
>>>>>> above using GPIO-mappings give us the binding to the right sensor
>>>>>> for free.
>>>>>
>>>>> Whether the privacy LED is controlled via the LED framework or GPIO doesn't
>>>>> really matter from this PoV, it could be controlled via the V4L2 framework
>>>>> in both cases. It might not be very pretty but I think I'd prefer that than
>>>>> putting this in either drivers or some sensor power sequence helper
>>>>> library.
>>>>
>>>> In sensors described in ACPI, esp. the straight forward described sensors
>>>> on atomisp2 devices, the GPIO resources inluding the LED one are listed
>>>> as resources of the i2c_client for the sensor.
>>>>
>>>> And in a sense the same applies to later IPU3 / IPU6 devices where there
>>>> is a separate INT3472 device describing all the GPIOS which is also
>>>> tied to a specific sensor and we currently map all the GPIOs from
>>>> the INT3472 device to the sensor.
>>>>
>>>> So it looks like that at least for x86/ACPI windows devices if the
>>>> LED has its own GPIO the hardware description clearly counts that
>>>> as part of the sensor's GPIOs. So the sensor driver has direct
>>>> access to this, where as any v4l2 framework driver would needed
>>>> to start poking inside the fwnode of the sensor which really
>>>> isn't pretty.
>>>
>>> Let me try to understand it better. Looking at the platforms you mention
>>> above, it seems that the way to retrieve the GPIO is platform-specific,
>>> isn't it ? Can the atomisp2 (is that IPU2 ?)
>>
>> Yes, sorta, Intel back then called it an ISP not an IPU, but the
>> Android x86 code which we have for it also refers to work enabling
>> IPU3 support, so definitely the same lineage of ISPs/IPUs.
>>
>>> , IPU3 and IPU6 expose the
>>> GPIO in the same way, or would we need code that, for instance, acquires
>>> the GPIO through different names (or even different APIs) for the same
>>> sensor on different platforms ?
>>
>> Long answer:
>>
>> On the atomisp2 platforms the GPIO is directly listed as a GPIO resource
>> of the i2c_client. Now ACPI resources use GPIO-indexes where as
>> the standard Linux GPIO APIs use GPIO names, so we need an index -> name
>> map in drivers/platform/x86 glue code.
>>
>> Note the need for an index -> name map is standard for all GPIOs
>> on ACPI platforms.
> 
> It's funny how ARM platforms were criticized for their need of board
> files, with x86/ACPI being revered as a saint. Now we have DT on ARM and
> x86 needs board files :-)

Yes this is a bit painful. Although most of the INT3472 code is not
board specific, it calls _DSM (device-specific-methods) which
the windows drivers use and then translates that to GPIO mappings.

For the non separate PMIC case the _DSM gives us a u8 containing a type
for each GPIO listed, which can be one of: /reset, clk-enable,
regulator-enable, /powerdown or privacy-led and then we "inject" those
into the fwnode for the i2c_client (with the clk / regulator using
the clk/regulator framework).

>> On IPU3 / IPU6 most (all?) of the power-seq (and privacy-led) related
>> resources like GPIOs are all described in an INT3472 ACPI device,
>> and the drivers/platform/x86/intel/int3472/*.c code then adds
>> GPIO-lookup table entries to the sensor's i2c_client pointing
>> to these GPIOS.
>>
>> So in the end for both the ISP2 and the IPU3/IPU6 which have
>> some code (outside of the media subsystem) abstracting away
>> all this platform specific shenanigans and mapping
>> the GPIOs to the sensor's i2c_client device so that a standard:
>>
>> 	sensor->pled_gpiod = gpiod_get(&i2c_client->dev, "privacy-led");
>>
>> Call should work on all of ISP2/IPU3/IPU6 (and presumably also
>> IPU4 if we ever get around to that).
>>
>> ###
>>
>> Short answer to your question:
>>
>> "would we need code that, for instance, acquires the GPIO through
>> different names (or even different APIs) for the same
>> sensor on different platforms ?"
>>
>> No the media subsystem sensor drivers should not need code to
>> deal with any platform differences, this should all be abstracted
>> away by the platform glue code under drivers/platform/x86, which
>> is glue which we need regardless of how we solve this.
>>
>> With that glue in place, a simple / standard:
>>
>> 	sensor->pled_gpiod = gpiod_get(&i2c_client->dev, "privacy-led");
>>
>> should work for all of ISP2 + IPU3 + IPU6 and this does already work
>> in my current testing done on IPU3 + IPU6.
> 
> Can I assume that "privacy-led" will be the right GPIO name not only
> across different platforms but also across different sensors ?

Yes. After this series we always map GPIO for which the _DSM returns 
the privacy-led value in the returned type field to a "privacy-led"
GPIO, the mapping code for this is sensor independent.

Regards,

Hans