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

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

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 :-)

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

> Note this already works in my testing with both normal GPIOs from
> the main SoC, as well as with the privacy LED attached to the TP68470
> PMIC used for the back sensor on the Surface Go.
> 
> >> Where as if you look at this patch set adding the privacy-LED GPIO
> >> from the INT3472 (IPU3 / IPU6) to the sensor fwnode is a 1 line change.
> >>
> >> This really by far is the most KISS solution and we have so much
> >> other things which need work that I believe that over-engineering
> >> this is not doing ourselves any favours.

-- 
Regards,

Laurent Pinchart