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