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

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

Hi,

On 11/30/22 16:20, Laurent Pinchart wrote:
> On Wed, Nov 30, 2022 at 02:52:50PM +0000, 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?
> 
> While I think we need a camera sensor helper, I also doubt managing the
> power sequence in the helper would help much. The privacy LED, however,
> could be handled there.

From a quick peek most of the sensor drivers I've looked at
(which is only a few) do:

-bulk-enable-regulators
-enable 1 clk
-set bunch of gpios
-sleep a bit

Since this requires to first get all the resources for this, which
needs error checking + reporting and then requires also error
checking the actual enabling + rollback on failure this is quite
a bit of code duplicated against many sensor drivers.

I agree that if a sensor does not fit in this model, that it then
should not use the helper and just open code the sequence but
I believe that for a bunch of sensor drivers with a simple
power-on sequence this can remove a bunch of code duplication.

Anways this is a clear case of the proof is in the tasting of
the pudding. So when I can make some time for this I'll submit
a patch series with the helper + converting a couple of sensors
(those which I can test) and then we can see from there.

>> 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.
> 
> And if the privacy LED is controllable through a GPIO, I think it should
> be turned on at stream on time, not at power on time. That would allow
> things like reading the OTP data from the sensor without flashing the
> privacy LED.

Agreed.

Regards,

Hans