| Message ID | 20221129231149.697154-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ov5693/int3472: Privacy LED handling changes + IPU6 compatibility | expand |
Hi Hans, 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? 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.
Hi Sakari, On 11/30/22 14:41, Sakari Ailus wrote: > Hi Hans, > > 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. 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. Regards, Hans
Hi Hans, On Wed, Nov 30, 2022 at 02:56:46PM +0100, Hans de Goede wrote: > Hi Sakari, > > On 11/30/22 14:41, Sakari Ailus wrote: > > Hi Hans, > > > > 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.
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.
On Wed, Nov 30, 2022 at 05:20:11PM +0200, 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: ... > > 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. The malicious software may power up camera and drive it via user space / separate code flow in the kernel, no? I would stick with power on as it's the most secure side. Even if we 100% know we are _not_ streaming this LED should indicate that it may be turned on at any time, no?
On Wed, Nov 30, 2022 at 06:07:51PM +0200, Andy Shevchenko wrote: > On Wed, Nov 30, 2022 at 05:20:11PM +0200, 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: > > ... > > > > 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. > > The malicious software may power up camera and drive it via user space / > separate code flow in the kernel, no? With correctly written drivers, there should be no way to power up the camera from userspace through the V4L2 API without starting streaming. Also, programming the camera sensor won't be enough to capture images, you need to deal with all the other camera-related IP cores which are controlled through V4L2, and doing so will start streaming in the camera sensor driver through the normal API anyway. > I would stick with power on as it's the most secure side. Even if we 100% know > we are _not_ streaming this LED should indicate that it may be turned on at any > time, no? Ideally, the privacy LED should be controlled automatically by the hardware without software intervention, and should be wired to a camera streaming signal. In many cases it's wired to the power rails instead, which is extremely annoying. I'd rather avoid this annoyance when the LED is GPIO-controlled.
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
Hi, On 11/30/22 15:52, Sakari Ailus wrote: > Hi Hans, > > On Wed, Nov 30, 2022 at 02:56:46PM +0100, Hans de Goede wrote: >> Hi Sakari, >> >> On 11/30/22 14:41, Sakari Ailus wrote: >>> Hi Hans, >>> >>> 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. 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, Hans
Hi Hans, 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 ?), 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 ? > 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.
Hi, On 12/2/22 11:54, Laurent Pinchart wrote: > Hi Hans, > > 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. 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. 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. Regards, Hans > >> 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. >
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.
On Fri, Dec 2, 2022 at 1:50 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Fri, Dec 02, 2022 at 12:21:12PM +0100, Hans de Goede wrote: > > On 12/2/22 11:54, Laurent Pinchart wrote: ... > > 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 :-) I believe it's a misunderstanding here due to missing words at Hans' statement, i..e. "..., which do not provide the descriptions in _DSD() method." So, no, x86 does not need board files generally speaking. The problem here is some departments of some big companies that didn't get ACPI properly or at all.
On Fri, Dec 02, 2022 at 01:53:55PM +0200, Andy Shevchenko wrote: > On Fri, Dec 2, 2022 at 1:50 PM Laurent Pinchart wrote: > > On Fri, Dec 02, 2022 at 12:21:12PM +0100, Hans de Goede wrote: > > > On 12/2/22 11:54, Laurent Pinchart wrote: > > ... > > > > 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 :-) > > I believe it's a misunderstanding here due to missing words at Hans' > statement, i..e. > "..., which do not provide the descriptions in _DSD() method." > > So, no, x86 does not need board files generally speaking. The problem > here is some departments of some big companies that didn't get ACPI > properly or at all. When it comes to camera support, that seems to cover an overwhelming majority of systems, if not all of them.
On Fri, Dec 2, 2022 at 2:14 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Fri, Dec 02, 2022 at 01:53:55PM +0200, Andy Shevchenko wrote: > > On Fri, Dec 2, 2022 at 1:50 PM Laurent Pinchart wrote: > > > On Fri, Dec 02, 2022 at 12:21:12PM +0100, Hans de Goede wrote: > > > > On 12/2/22 11:54, Laurent Pinchart wrote: ... > > > > 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 :-) > > > > I believe it's a misunderstanding here due to missing words at Hans' > > statement, i..e. > > "..., which do not provide the descriptions in _DSD() method." > > > > So, no, x86 does not need board files generally speaking. The problem > > here is some departments of some big companies that didn't get ACPI > > properly or at all. > > When it comes to camera support, that seems to cover an overwhelming > majority of systems, if not all of them. Unfortunately :-(
On Fri, Dec 02, 2022 at 02:14:46PM +0200, Laurent Pinchart wrote: > On Fri, Dec 02, 2022 at 01:53:55PM +0200, Andy Shevchenko wrote: > > On Fri, Dec 2, 2022 at 1:50 PM Laurent Pinchart wrote: > > > On Fri, Dec 02, 2022 at 12:21:12PM +0100, Hans de Goede wrote: > > > > On 12/2/22 11:54, Laurent Pinchart wrote: > > > > ... > > > > > > 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 :-) > > > > I believe it's a misunderstanding here due to missing words at Hans' > > statement, i..e. > > "..., which do not provide the descriptions in _DSD() method." > > > > So, no, x86 does not need board files generally speaking. The problem > > here is some departments of some big companies that didn't get ACPI > > properly or at all. > > When it comes to camera support, that seems to cover an overwhelming > majority of systems, if not all of them. Not those shipped with ChromeOS. In the future the BIOS folks would ideally base this on MIPI DisCo for Imaging. The spec should be out soon: <URL:https://www.mipi.org/specifications/mipi-disco-imaging>
Hi Hans, On Wed, Nov 30, 2022 at 05:34:55PM +0100, Hans de Goede wrote: > 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. Most of the common (e.g. camera sensor related) properties are parsed by the V4L2 framework, not by drivers. I'm not saying no to having privacy-led parsing in a single driver but instead of adding more of this in drivers we should have a common solution for this.
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@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 :-) 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
diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c index a97ec132ba3a..e3c3bed69ad6 100644 --- a/drivers/media/i2c/ov5693.c +++ b/drivers/media/i2c/ov5693.c @@ -156,6 +156,7 @@ struct ov5693_device { struct gpio_desc *reset; struct gpio_desc *powerdown; + struct gpio_desc *privacy_led; struct regulator_bulk_data supplies[OV5693_NUM_SUPPLIES]; struct clk *xvclk; @@ -789,6 +790,7 @@ static int ov5693_sensor_init(struct ov5693_device *ov5693) static void ov5693_sensor_powerdown(struct ov5693_device *ov5693) { + gpiod_set_value_cansleep(ov5693->privacy_led, 0); gpiod_set_value_cansleep(ov5693->reset, 1); gpiod_set_value_cansleep(ov5693->powerdown, 1); @@ -818,6 +820,7 @@ static int ov5693_sensor_powerup(struct ov5693_device *ov5693) gpiod_set_value_cansleep(ov5693->powerdown, 0); gpiod_set_value_cansleep(ov5693->reset, 0); + gpiod_set_value_cansleep(ov5693->privacy_led, 1); usleep_range(5000, 7500); @@ -1325,6 +1328,13 @@ static int ov5693_configure_gpios(struct ov5693_device *ov5693) return PTR_ERR(ov5693->powerdown); } + ov5693->privacy_led = devm_gpiod_get_optional(ov5693->dev, "privacy-led", + GPIOD_OUT_LOW); + if (IS_ERR(ov5693->privacy_led)) { + dev_err(ov5693->dev, "Error fetching privacy-led GPIO\n"); + return PTR_ERR(ov5693->privacy_led); + } + return 0; }
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. --- drivers/media/i2c/ov5693.c | 10 ++++++++++ 1 file changed, 10 insertions(+)