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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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.

> 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.

> > 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.

-- 
Regards,

Laurent Pinchart