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@xxxxxxxxxx> >>>>>>>> --- >>>>>>>> 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@xxxxxxxxxxxxxxxx/ >>>>>> >>>>>> 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
