[v6,1/5] media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present

  Make v4l2_async_register_subdev_sensor() try to get a privacy LED
associated with the sensor and extend the call_s_stream() wrapper to
enable/disable the privacy LED if found.

This makes the core handle privacy LED control, rather then having to
duplicate this code in all the sensor drivers.

Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v6:
- Add v4l2_subdev_privacy_led_get()/_put() helpers
- At least the _put helper is necessary for cleanup on errors later on in
  v4l2_async_register_subdev_sensor()
- This puts all the LED related coded into a single file (v4l2-subdev.c)
  removing the need to build the async + fwnode code into videodev.ko,
  so that patch is dropped
- Move the (non-error-exit) cleanup from v4l2_subdev_cleanup() to
   v4l2_async_unregister_subdev()

Changes in v4 (requested by Laurent Pinchart):
- Move the led_get() call to v4l2_async_register_subdev_sensor() and
  make errors other then -ENOENT fail the register() call.
- Move the led_disable_sysfs() call to be done at led_get() time, instead
  of only disabling the sysfs interface when the sensor is streaming.
---
 drivers/media/v4l2-core/v4l2-async.c       |  4 ++
 drivers/media/v4l2-core/v4l2-fwnode.c      |  7 ++++
 drivers/media/v4l2-core/v4l2-subdev-priv.h | 14 +++++++
 drivers/media/v4l2-core/v4l2-subdev.c      | 44 ++++++++++++++++++++++
 include/media/v4l2-subdev.h                |  3 ++
 5 files changed, 72 insertions(+)
 create mode 100644 drivers/media/v4l2-core/v4l2-subdev-priv.h
  

Hi Hans,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.2-rc5]
[cannot apply to media-tree/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/media-v4l2-core-Make-the-v4l2-core-code-enable-disable-the-privacy-LED-if-present/20230128-131233
patch link:    https://lore.kernel.org/r/20230127203729.10205-2-hdegoede%40redhat.com
patch subject: [PATCH v6 1/5] media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present
config: riscv-randconfig-r026-20230123 (https://download.01.org/0day-ci/archive/20230128/202301281534.9Z8xRsrX-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 4196ca3278f78c6e19246e54ab0ecb364e37d66a)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv64-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/000ccec1824b3256e3fc1a94079bb953f19faab5
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Hans-de-Goede/media-v4l2-core-Make-the-v4l2-core-code-enable-disable-the-privacy-LED-if-present/20230128-131233
        git checkout 000ccec1824b3256e3fc1a94079bb953f19faab5
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/media/v4l2-core/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/media/v4l2-core/v4l2-subdev.c:1124:20: error: call to undeclared function 'led_get'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
           sd->privacy_led = led_get(sd->dev, "privacy-led");
                             ^
>> drivers/media/v4l2-core/v4l2-subdev.c:1124:18: error: incompatible integer to pointer conversion assigning to 'struct led_classdev *' from 'int' [-Wint-conversion]
           sd->privacy_led = led_get(sd->dev, "privacy-led");
                           ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   2 errors generated.


vim +/led_get +1124 drivers/media/v4l2-core/v4l2-subdev.c

  1120	
  1121	int v4l2_subdev_get_privacy_led(struct v4l2_subdev *sd)
  1122	{
  1123	#if IS_REACHABLE(CONFIG_LEDS_CLASS)
> 1124		sd->privacy_led = led_get(sd->dev, "privacy-led");
  1125		if (IS_ERR(sd->privacy_led) && PTR_ERR(sd->privacy_led) != -ENOENT)
  1126			return dev_err_probe(sd->dev, PTR_ERR(sd->privacy_led), "getting privacy LED\n");
  1127	
  1128		if (!IS_ERR_OR_NULL(sd->privacy_led)) {
  1129			mutex_lock(&sd->privacy_led->led_access);
  1130			led_sysfs_disable(sd->privacy_led);
  1131			led_trigger_remove(sd->privacy_led);
  1132			led_set_brightness(sd->privacy_led, 0);
  1133			mutex_unlock(&sd->privacy_led->led_access);
  1134		}
  1135	#endif
  1136		return 0;
  1137	}
  1138	EXPORT_SYMBOL_GPL(v4l2_subdev_get_privacy_led);
  1139
  

Hi Hans,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.2-rc5 next-20230127]
[cannot apply to media-tree/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/media-v4l2-core-Make-the-v4l2-core-code-enable-disable-the-privacy-LED-if-present/20230128-131233
patch link:    https://lore.kernel.org/r/20230127203729.10205-2-hdegoede%40redhat.com
patch subject: [PATCH v6 1/5] media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present
config: ia64-allyesconfig (https://download.01.org/0day-ci/archive/20230128/202301281654.sPeSxigX-lkp@intel.com/config)
compiler: ia64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/000ccec1824b3256e3fc1a94079bb953f19faab5
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Hans-de-Goede/media-v4l2-core-Make-the-v4l2-core-code-enable-disable-the-privacy-LED-if-present/20230128-131233
        git checkout 000ccec1824b3256e3fc1a94079bb953f19faab5
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=ia64 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=ia64 SHELL=/bin/bash drivers/media/v4l2-core/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/media/v4l2-core/v4l2-subdev.c: In function 'v4l2_subdev_get_privacy_led':
   drivers/media/v4l2-core/v4l2-subdev.c:1124:27: error: implicit declaration of function 'led_get'; did you mean 'key_get'? [-Werror=implicit-function-declaration]
    1124 |         sd->privacy_led = led_get(sd->dev, "privacy-led");
         |                           ^~~~~~~
         |                           key_get
>> drivers/media/v4l2-core/v4l2-subdev.c:1124:25: warning: assignment to 'struct led_classdev *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
    1124 |         sd->privacy_led = led_get(sd->dev, "privacy-led");
         |                         ^
   cc1: some warnings being treated as errors


vim +1124 drivers/media/v4l2-core/v4l2-subdev.c

  1120	
  1121	int v4l2_subdev_get_privacy_led(struct v4l2_subdev *sd)
  1122	{
  1123	#if IS_REACHABLE(CONFIG_LEDS_CLASS)
> 1124		sd->privacy_led = led_get(sd->dev, "privacy-led");
  1125		if (IS_ERR(sd->privacy_led) && PTR_ERR(sd->privacy_led) != -ENOENT)
  1126			return dev_err_probe(sd->dev, PTR_ERR(sd->privacy_led), "getting privacy LED\n");
  1127	
  1128		if (!IS_ERR_OR_NULL(sd->privacy_led)) {
  1129			mutex_lock(&sd->privacy_led->led_access);
  1130			led_sysfs_disable(sd->privacy_led);
  1131			led_trigger_remove(sd->privacy_led);
  1132			led_set_brightness(sd->privacy_led, 0);
  1133			mutex_unlock(&sd->privacy_led->led_access);
  1134		}
  1135	#endif
  1136		return 0;
  1137	}
  1138	EXPORT_SYMBOL_GPL(v4l2_subdev_get_privacy_led);
  1139
  

Hi,

On 1/28/23 08:35, kernel test robot wrote:
> Hi Hans,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on linus/master]
> [also build test ERROR on v6.2-rc5]
> [cannot apply to media-tree/master]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/media-v4l2-core-Make-the-v4l2-core-code-enable-disable-the-privacy-LED-if-present/20230128-131233
> patch link:    https://lore.kernel.org/r/20230127203729.10205-2-hdegoede%40redhat.com
> patch subject: [PATCH v6 1/5] media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present
> config: riscv-randconfig-r026-20230123 (https://download.01.org/0day-ci/archive/20230128/202301281534.9Z8xRsrX-lkp@intel.com/config)
> compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 4196ca3278f78c6e19246e54ab0ecb364e37d66a)
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # install riscv cross compiling tool for clang build
>         # apt-get install binutils-riscv64-linux-gnu
>         # https://github.com/intel-lab-lkp/linux/commit/000ccec1824b3256e3fc1a94079bb953f19faab5
>         git remote add linux-review https://github.com/intel-lab-lkp/linux
>         git fetch --no-tags linux-review Hans-de-Goede/media-v4l2-core-Make-the-v4l2-core-code-enable-disable-the-privacy-LED-if-present/20230128-131233
>         git checkout 000ccec1824b3256e3fc1a94079bb953f19faab5
>         # save the config file
>         mkdir build_dir && cp config build_dir/.config
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv olddefconfig
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/media/v4l2-core/
> 
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
>>> drivers/media/v4l2-core/v4l2-subdev.c:1124:20: error: call to undeclared function 'led_get'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
>            sd->privacy_led = led_get(sd->dev, "privacy-led");
>                              ^
>>> drivers/media/v4l2-core/v4l2-subdev.c:1124:18: error: incompatible integer to pointer conversion assigning to 'struct led_classdev *' from 'int' [-Wint-conversion]
>            sd->privacy_led = led_get(sd->dev, "privacy-led");
>                            ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    2 errors generated.

As mentioned in the cover-letter this series depends on this immutable-branch:

https://lore.kernel.org/platform-driver-x86/Y9QGcA+9nlmOOy2d@google.com/

That branch not being present in the base used by LKP is what is causing this
error.

Regards,

Hans
  

Hi Hans,

On Sat, Jan 28, 2023 at 10:41:15AM +0100, Hans de Goede wrote:
> On 1/28/23 08:35, kernel test robot wrote:
> > Hi Hans,
> > 
> > I love your patch! Yet something to improve:
> > 
> > [auto build test ERROR on linus/master]
> > [also build test ERROR on v6.2-rc5]
> > [cannot apply to media-tree/master]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> > 
> > url:    https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/media-v4l2-core-Make-the-v4l2-core-code-enable-disable-the-privacy-LED-if-present/20230128-131233
> > patch link:    https://lore.kernel.org/r/20230127203729.10205-2-hdegoede%40redhat.com
> > patch subject: [PATCH v6 1/5] media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present
> > config: riscv-randconfig-r026-20230123 (https://download.01.org/0day-ci/archive/20230128/202301281534.9Z8xRsrX-lkp@intel.com/config)
> > compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 4196ca3278f78c6e19246e54ab0ecb364e37d66a)
> > reproduce (this is a W=1 build):
> >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >         chmod +x ~/bin/make.cross
> >         # install riscv cross compiling tool for clang build
> >         # apt-get install binutils-riscv64-linux-gnu
> >         # https://github.com/intel-lab-lkp/linux/commit/000ccec1824b3256e3fc1a94079bb953f19faab5
> >         git remote add linux-review https://github.com/intel-lab-lkp/linux
> >         git fetch --no-tags linux-review Hans-de-Goede/media-v4l2-core-Make-the-v4l2-core-code-enable-disable-the-privacy-LED-if-present/20230128-131233
> >         git checkout 000ccec1824b3256e3fc1a94079bb953f19faab5
> >         # save the config file
> >         mkdir build_dir && cp config build_dir/.config
> >         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv olddefconfig
> >         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/media/v4l2-core/
> > 
> > If you fix the issue, kindly add following tag where applicable
> > | Reported-by: kernel test robot <lkp@intel.com>
> > 
> > All errors (new ones prefixed by >>):
> > 
> >>> drivers/media/v4l2-core/v4l2-subdev.c:1124:20: error: call to undeclared function 'led_get'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
> >            sd->privacy_led = led_get(sd->dev, "privacy-led");
> >                              ^
> >>> drivers/media/v4l2-core/v4l2-subdev.c:1124:18: error: incompatible integer to pointer conversion assigning to 'struct led_classdev *' from 'int' [-Wint-conversion]
> >            sd->privacy_led = led_get(sd->dev, "privacy-led");
> >                            ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >    2 errors generated.
> 
> As mentioned in the cover-letter this series depends on this immutable-branch:
> 
> https://lore.kernel.org/platform-driver-x86/Y9QGcA+9nlmOOy2d@google.com/
> 
> That branch not being present in the base used by LKP is what is causing this
> error.

The --base argument to git-format-patch will record the base commit ID
in the cover letter, that can possibly help bots getting it right.
  

Hi,

On 1/28/23 14:42, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Sat, Jan 28, 2023 at 10:41:15AM +0100, Hans de Goede wrote:
>> On 1/28/23 08:35, kernel test robot wrote:
>>> Hi Hans,
>>>
>>> I love your patch! Yet something to improve:
>>>
>>> [auto build test ERROR on linus/master]
>>> [also build test ERROR on v6.2-rc5]
>>> [cannot apply to media-tree/master]
>>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>>> And when submitting patch, we suggest to use '--base' as documented in
>>> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>>>
>>> url:    https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/media-v4l2-core-Make-the-v4l2-core-code-enable-disable-the-privacy-LED-if-present/20230128-131233
>>> patch link:    https://lore.kernel.org/r/20230127203729.10205-2-hdegoede%40redhat.com
>>> patch subject: [PATCH v6 1/5] media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present
>>> config: riscv-randconfig-r026-20230123 (https://download.01.org/0day-ci/archive/20230128/202301281534.9Z8xRsrX-lkp@intel.com/config)
>>> compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 4196ca3278f78c6e19246e54ab0ecb364e37d66a)
>>> reproduce (this is a W=1 build):
>>>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>>>         chmod +x ~/bin/make.cross
>>>         # install riscv cross compiling tool for clang build
>>>         # apt-get install binutils-riscv64-linux-gnu
>>>         # https://github.com/intel-lab-lkp/linux/commit/000ccec1824b3256e3fc1a94079bb953f19faab5
>>>         git remote add linux-review https://github.com/intel-lab-lkp/linux
>>>         git fetch --no-tags linux-review Hans-de-Goede/media-v4l2-core-Make-the-v4l2-core-code-enable-disable-the-privacy-LED-if-present/20230128-131233
>>>         git checkout 000ccec1824b3256e3fc1a94079bb953f19faab5
>>>         # save the config file
>>>         mkdir build_dir && cp config build_dir/.config
>>>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv olddefconfig
>>>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/media/v4l2-core/
>>>
>>> If you fix the issue, kindly add following tag where applicable
>>> | Reported-by: kernel test robot <lkp@intel.com>
>>>
>>> All errors (new ones prefixed by >>):
>>>
>>>>> drivers/media/v4l2-core/v4l2-subdev.c:1124:20: error: call to undeclared function 'led_get'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
>>>            sd->privacy_led = led_get(sd->dev, "privacy-led");
>>>                              ^
>>>>> drivers/media/v4l2-core/v4l2-subdev.c:1124:18: error: incompatible integer to pointer conversion assigning to 'struct led_classdev *' from 'int' [-Wint-conversion]
>>>            sd->privacy_led = led_get(sd->dev, "privacy-led");
>>>                            ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>    2 errors generated.
>>
>> As mentioned in the cover-letter this series depends on this immutable-branch:
>>
>> https://lore.kernel.org/platform-driver-x86/Y9QGcA+9nlmOOy2d@google.com/
>>
>> That branch not being present in the base used by LKP is what is causing this
>> error.
> 
> The --base argument to git-format-patch will record the base commit ID
> in the cover letter, that can possibly help bots getting it right.

Thanks I was not aware of the --base argument, that is useful to know.

Regards,

Hans
  

Hi Hans,

On Fri, Jan 27, 2023 at 09:37:25PM +0100, Hans de Goede wrote:
> Make v4l2_async_register_subdev_sensor() try to get a privacy LED
> associated with the sensor and extend the call_s_stream() wrapper to
> enable/disable the privacy LED if found.
> 
> This makes the core handle privacy LED control, rather then having to
> duplicate this code in all the sensor drivers.
> 
> Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Please wrap the lines over 80, unless there are tangible reasons to keep
them as-is.

For this patch:

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

On my behalf it can be merged via another tree, I don't expect conflicts.
Also cc Hans Verkuil.

And the rest:

Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Please also see my comment on the 3rd patch.

> ---
> Changes in v6:
> - Add v4l2_subdev_privacy_led_get()/_put() helpers
> - At least the _put helper is necessary for cleanup on errors later on in
>   v4l2_async_register_subdev_sensor()
> - This puts all the LED related coded into a single file (v4l2-subdev.c)
>   removing the need to build the async + fwnode code into videodev.ko,
>   so that patch is dropped
> - Move the (non-error-exit) cleanup from v4l2_subdev_cleanup() to
>    v4l2_async_unregister_subdev()
> 
> Changes in v4 (requested by Laurent Pinchart):
> - Move the led_get() call to v4l2_async_register_subdev_sensor() and
>   make errors other then -ENOENT fail the register() call.
> - Move the led_disable_sysfs() call to be done at led_get() time, instead
>   of only disabling the sysfs interface when the sensor is streaming.
> ---
>  drivers/media/v4l2-core/v4l2-async.c       |  4 ++
>  drivers/media/v4l2-core/v4l2-fwnode.c      |  7 ++++
>  drivers/media/v4l2-core/v4l2-subdev-priv.h | 14 +++++++
>  drivers/media/v4l2-core/v4l2-subdev.c      | 44 ++++++++++++++++++++++
>  include/media/v4l2-subdev.h                |  3 ++
>  5 files changed, 72 insertions(+)
>  create mode 100644 drivers/media/v4l2-core/v4l2-subdev-priv.h
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 2f1b718a9189..d7e9ffc7aa23 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -24,6 +24,8 @@
>  #include <media/v4l2-fwnode.h>
>  #include <media/v4l2-subdev.h>
>  
> +#include "v4l2-subdev-priv.h"
> +
>  static int v4l2_async_nf_call_bound(struct v4l2_async_notifier *n,
>  				    struct v4l2_subdev *subdev,
>  				    struct v4l2_async_subdev *asd)
> @@ -822,6 +824,8 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
>  	if (!sd->async_list.next)
>  		return;
>  
> +	v4l2_subdev_put_privacy_led(sd);
> +
>  	mutex_lock(&list_lock);
>  
>  	__v4l2_async_nf_unregister(sd->subdev_notifier);
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 3d9533c1b202..049c2f2001ea 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -28,6 +28,8 @@
>  #include <media/v4l2-fwnode.h>
>  #include <media/v4l2-subdev.h>
>  
> +#include "v4l2-subdev-priv.h"
> +
>  static const struct v4l2_fwnode_bus_conv {
>  	enum v4l2_fwnode_bus_type fwnode_bus_type;
>  	enum v4l2_mbus_type mbus_type;
> @@ -1302,6 +1304,10 @@ int v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd)
>  
>  	v4l2_async_nf_init(notifier);
>  
> +	ret = v4l2_subdev_get_privacy_led(sd);
> +	if (ret < 0)
> +		goto out_cleanup;
> +
>  	ret = v4l2_async_nf_parse_fwnode_sensor(sd->dev, notifier);
>  	if (ret < 0)
>  		goto out_cleanup;
> @@ -1322,6 +1328,7 @@ int v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd)
>  	v4l2_async_nf_unregister(notifier);
>  
>  out_cleanup:
> +	v4l2_subdev_put_privacy_led(sd);
>  	v4l2_async_nf_cleanup(notifier);
>  	kfree(notifier);
>  
> diff --git a/drivers/media/v4l2-core/v4l2-subdev-priv.h b/drivers/media/v4l2-core/v4l2-subdev-priv.h
> new file mode 100644
> index 000000000000..52391d6d8ab7
> --- /dev/null
> +++ b/drivers/media/v4l2-core/v4l2-subdev-priv.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * V4L2 sub-device pivate header.
> + *
> + * Copyright (C) 2023 Hans de Goede <hdegoede@redhat.com>
> + */
> +
> +#ifndef _V4L2_SUBDEV_PRIV_H_
> +#define _V4L2_SUBDEV_PRIV_H_
> +
> +int v4l2_subdev_get_privacy_led(struct v4l2_subdev *sd);
> +void v4l2_subdev_put_privacy_led(struct v4l2_subdev *sd);
> +
> +#endif
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 4988a25bd8f4..9fd183628285 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -9,6 +9,7 @@
>   */
>  
>  #include <linux/ioctl.h>
> +#include <linux/leds.h>
>  #include <linux/mm.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
> @@ -23,6 +24,8 @@
>  #include <media/v4l2-fh.h>
>  #include <media/v4l2-event.h>
>  
> +#include "v4l2-subdev-priv.h"
> +
>  #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
>  static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
>  {
> @@ -322,6 +325,14 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	int ret;
>  
> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
> +	if (!IS_ERR_OR_NULL(sd->privacy_led)) {
> +		if (enable)
> +			led_set_brightness(sd->privacy_led, sd->privacy_led->max_brightness);
> +		else
> +			led_set_brightness(sd->privacy_led, 0);
> +	}
> +#endif
>  	ret = sd->ops->video->s_stream(sd, enable);
>  
>  	if (!enable && ret < 0) {
> @@ -1090,6 +1101,7 @@ void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
>  	sd->grp_id = 0;
>  	sd->dev_priv = NULL;
>  	sd->host_priv = NULL;
> +	sd->privacy_led = NULL;
>  #if defined(CONFIG_MEDIA_CONTROLLER)
>  	sd->entity.name = sd->name;
>  	sd->entity.obj_type = MEDIA_ENTITY_TYPE_V4L2_SUBDEV;
> @@ -1105,3 +1117,35 @@ void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
>  	v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT, (void *)ev);
>  }
>  EXPORT_SYMBOL_GPL(v4l2_subdev_notify_event);
> +
> +int v4l2_subdev_get_privacy_led(struct v4l2_subdev *sd)
> +{
> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
> +	sd->privacy_led = led_get(sd->dev, "privacy-led");
> +	if (IS_ERR(sd->privacy_led) && PTR_ERR(sd->privacy_led) != -ENOENT)
> +		return dev_err_probe(sd->dev, PTR_ERR(sd->privacy_led), "getting privacy LED\n");
> +
> +	if (!IS_ERR_OR_NULL(sd->privacy_led)) {
> +		mutex_lock(&sd->privacy_led->led_access);
> +		led_sysfs_disable(sd->privacy_led);
> +		led_trigger_remove(sd->privacy_led);
> +		led_set_brightness(sd->privacy_led, 0);
> +		mutex_unlock(&sd->privacy_led->led_access);
> +	}
> +#endif
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_get_privacy_led);
> +
> +void v4l2_subdev_put_privacy_led(struct v4l2_subdev *sd)
> +{
> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
> +	if (!IS_ERR_OR_NULL(sd->privacy_led)) {
> +		mutex_lock(&sd->privacy_led->led_access);
> +		led_sysfs_enable(sd->privacy_led);
> +		mutex_unlock(&sd->privacy_led->led_access);
> +		led_put(sd->privacy_led);
> +	}
> +#endif
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_put_privacy_led);
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index b15fa9930f30..0547313f98cc 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -38,6 +38,7 @@ struct v4l2_subdev;
>  struct v4l2_subdev_fh;
>  struct tuner_setup;
>  struct v4l2_mbus_frame_desc;
> +struct led_classdev;
>  
>  /**
>   * struct v4l2_decode_vbi_line - used to decode_vbi_line
> @@ -982,6 +983,8 @@ struct v4l2_subdev {
>  	 * appropriate functions.
>  	 */
>  
> +	struct led_classdev *privacy_led;
> +
>  	/*
>  	 * TODO: active_state should most likely be changed from a pointer to an
>  	 * embedded field. For the time being it's kept as a pointer to more
  

Hi,

On 1/30/23 11:17, Sakari Ailus wrote:
> Hi Hans,
> 
> On Fri, Jan 27, 2023 at 09:37:25PM +0100, Hans de Goede wrote:
>> Make v4l2_async_register_subdev_sensor() try to get a privacy LED
>> associated with the sensor and extend the call_s_stream() wrapper to
>> enable/disable the privacy LED if found.
>>
>> This makes the core handle privacy LED control, rather then having to
>> duplicate this code in all the sensor drivers.
>>
>> Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>> Acked-by: Linus Walleij <linus.walleij@linaro.org>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Please wrap the lines over 80, unless there are tangible reasons to keep
> them as-is.
> 
> For this patch:
> 
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> On my behalf it can be merged via another tree, I don't expect conflicts.
> Also cc Hans Verkuil.

Thanks.

I've merged the entire series into my pdx86/review-hans branch now:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
(with the lines in this patch shortened to 80 chars).

Once the builders have had some time to play with this branch
(and once I've run some tests to make sure the patches still
work as expected) I will push to platform-drivers-x86/for-next .

Regards,

Hans





> 
> And the rest:
> 
> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> Please also see my comment on the 3rd patch.
> 
>> ---
>> Changes in v6:
>> - Add v4l2_subdev_privacy_led_get()/_put() helpers
>> - At least the _put helper is necessary for cleanup on errors later on in
>>   v4l2_async_register_subdev_sensor()
>> - This puts all the LED related coded into a single file (v4l2-subdev.c)
>>   removing the need to build the async + fwnode code into videodev.ko,
>>   so that patch is dropped
>> - Move the (non-error-exit) cleanup from v4l2_subdev_cleanup() to
>>    v4l2_async_unregister_subdev()
>>
>> Changes in v4 (requested by Laurent Pinchart):
>> - Move the led_get() call to v4l2_async_register_subdev_sensor() and
>>   make errors other then -ENOENT fail the register() call.
>> - Move the led_disable_sysfs() call to be done at led_get() time, instead
>>   of only disabling the sysfs interface when the sensor is streaming.
>> ---
>>  drivers/media/v4l2-core/v4l2-async.c       |  4 ++
>>  drivers/media/v4l2-core/v4l2-fwnode.c      |  7 ++++
>>  drivers/media/v4l2-core/v4l2-subdev-priv.h | 14 +++++++
>>  drivers/media/v4l2-core/v4l2-subdev.c      | 44 ++++++++++++++++++++++
>>  include/media/v4l2-subdev.h                |  3 ++
>>  5 files changed, 72 insertions(+)
>>  create mode 100644 drivers/media/v4l2-core/v4l2-subdev-priv.h
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
>> index 2f1b718a9189..d7e9ffc7aa23 100644
>> --- a/drivers/media/v4l2-core/v4l2-async.c
>> +++ b/drivers/media/v4l2-core/v4l2-async.c
>> @@ -24,6 +24,8 @@
>>  #include <media/v4l2-fwnode.h>
>>  #include <media/v4l2-subdev.h>
>>  
>> +#include "v4l2-subdev-priv.h"
>> +
>>  static int v4l2_async_nf_call_bound(struct v4l2_async_notifier *n,
>>  				    struct v4l2_subdev *subdev,
>>  				    struct v4l2_async_subdev *asd)
>> @@ -822,6 +824,8 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
>>  	if (!sd->async_list.next)
>>  		return;
>>  
>> +	v4l2_subdev_put_privacy_led(sd);
>> +
>>  	mutex_lock(&list_lock);
>>  
>>  	__v4l2_async_nf_unregister(sd->subdev_notifier);
>> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
>> index 3d9533c1b202..049c2f2001ea 100644
>> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
>> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
>> @@ -28,6 +28,8 @@
>>  #include <media/v4l2-fwnode.h>
>>  #include <media/v4l2-subdev.h>
>>  
>> +#include "v4l2-subdev-priv.h"
>> +
>>  static const struct v4l2_fwnode_bus_conv {
>>  	enum v4l2_fwnode_bus_type fwnode_bus_type;
>>  	enum v4l2_mbus_type mbus_type;
>> @@ -1302,6 +1304,10 @@ int v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd)
>>  
>>  	v4l2_async_nf_init(notifier);
>>  
>> +	ret = v4l2_subdev_get_privacy_led(sd);
>> +	if (ret < 0)
>> +		goto out_cleanup;
>> +
>>  	ret = v4l2_async_nf_parse_fwnode_sensor(sd->dev, notifier);
>>  	if (ret < 0)
>>  		goto out_cleanup;
>> @@ -1322,6 +1328,7 @@ int v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd)
>>  	v4l2_async_nf_unregister(notifier);
>>  
>>  out_cleanup:
>> +	v4l2_subdev_put_privacy_led(sd);
>>  	v4l2_async_nf_cleanup(notifier);
>>  	kfree(notifier);
>>  
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev-priv.h b/drivers/media/v4l2-core/v4l2-subdev-priv.h
>> new file mode 100644
>> index 000000000000..52391d6d8ab7
>> --- /dev/null
>> +++ b/drivers/media/v4l2-core/v4l2-subdev-priv.h
>> @@ -0,0 +1,14 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * V4L2 sub-device pivate header.
>> + *
>> + * Copyright (C) 2023 Hans de Goede <hdegoede@redhat.com>
>> + */
>> +
>> +#ifndef _V4L2_SUBDEV_PRIV_H_
>> +#define _V4L2_SUBDEV_PRIV_H_
>> +
>> +int v4l2_subdev_get_privacy_led(struct v4l2_subdev *sd);
>> +void v4l2_subdev_put_privacy_led(struct v4l2_subdev *sd);
>> +
>> +#endif
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index 4988a25bd8f4..9fd183628285 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -9,6 +9,7 @@
>>   */
>>  
>>  #include <linux/ioctl.h>
>> +#include <linux/leds.h>
>>  #include <linux/mm.h>
>>  #include <linux/module.h>
>>  #include <linux/slab.h>
>> @@ -23,6 +24,8 @@
>>  #include <media/v4l2-fh.h>
>>  #include <media/v4l2-event.h>
>>  
>> +#include "v4l2-subdev-priv.h"
>> +
>>  #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
>>  static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
>>  {
>> @@ -322,6 +325,14 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
>>  {
>>  	int ret;
>>  
>> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
>> +	if (!IS_ERR_OR_NULL(sd->privacy_led)) {
>> +		if (enable)
>> +			led_set_brightness(sd->privacy_led, sd->privacy_led->max_brightness);
>> +		else
>> +			led_set_brightness(sd->privacy_led, 0);
>> +	}
>> +#endif
>>  	ret = sd->ops->video->s_stream(sd, enable);
>>  
>>  	if (!enable && ret < 0) {
>> @@ -1090,6 +1101,7 @@ void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
>>  	sd->grp_id = 0;
>>  	sd->dev_priv = NULL;
>>  	sd->host_priv = NULL;
>> +	sd->privacy_led = NULL;
>>  #if defined(CONFIG_MEDIA_CONTROLLER)
>>  	sd->entity.name = sd->name;
>>  	sd->entity.obj_type = MEDIA_ENTITY_TYPE_V4L2_SUBDEV;
>> @@ -1105,3 +1117,35 @@ void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
>>  	v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT, (void *)ev);
>>  }
>>  EXPORT_SYMBOL_GPL(v4l2_subdev_notify_event);
>> +
>> +int v4l2_subdev_get_privacy_led(struct v4l2_subdev *sd)
>> +{
>> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
>> +	sd->privacy_led = led_get(sd->dev, "privacy-led");
>> +	if (IS_ERR(sd->privacy_led) && PTR_ERR(sd->privacy_led) != -ENOENT)
>> +		return dev_err_probe(sd->dev, PTR_ERR(sd->privacy_led), "getting privacy LED\n");
>> +
>> +	if (!IS_ERR_OR_NULL(sd->privacy_led)) {
>> +		mutex_lock(&sd->privacy_led->led_access);
>> +		led_sysfs_disable(sd->privacy_led);
>> +		led_trigger_remove(sd->privacy_led);
>> +		led_set_brightness(sd->privacy_led, 0);
>> +		mutex_unlock(&sd->privacy_led->led_access);
>> +	}
>> +#endif
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_subdev_get_privacy_led);
>> +
>> +void v4l2_subdev_put_privacy_led(struct v4l2_subdev *sd)
>> +{
>> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
>> +	if (!IS_ERR_OR_NULL(sd->privacy_led)) {
>> +		mutex_lock(&sd->privacy_led->led_access);
>> +		led_sysfs_enable(sd->privacy_led);
>> +		mutex_unlock(&sd->privacy_led->led_access);
>> +		led_put(sd->privacy_led);
>> +	}
>> +#endif
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_subdev_put_privacy_led);
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index b15fa9930f30..0547313f98cc 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -38,6 +38,7 @@ struct v4l2_subdev;
>>  struct v4l2_subdev_fh;
>>  struct tuner_setup;
>>  struct v4l2_mbus_frame_desc;
>> +struct led_classdev;
>>  
>>  /**
>>   * struct v4l2_decode_vbi_line - used to decode_vbi_line
>> @@ -982,6 +983,8 @@ struct v4l2_subdev {
>>  	 * appropriate functions.
>>  	 */
>>  
>> +	struct led_classdev *privacy_led;
>> +
>>  	/*
>>  	 * TODO: active_state should most likely be changed from a pointer to an
>>  	 * embedded field. For the time being it's kept as a pointer to more
>
  

On Mon, Jan 30, 2023 at 10:00 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 1/30/23 11:17, Sakari Ailus wrote:
> > Hi Hans,
> >
> > On Fri, Jan 27, 2023 at 09:37:25PM +0100, Hans de Goede wrote:
> >> Make v4l2_async_register_subdev_sensor() try to get a privacy LED
> >> associated with the sensor and extend the call_s_stream() wrapper to
> >> enable/disable the privacy LED if found.
> >>
> >> This makes the core handle privacy LED control, rather then having to
> >> duplicate this code in all the sensor drivers.
> >>
> >> Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >
> > Please wrap the lines over 80, unless there are tangible reasons to keep
> > them as-is.
> >
> > For this patch:
> >
> > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >
> > On my behalf it can be merged via another tree, I don't expect conflicts.
> > Also cc Hans Verkuil.
>
> Thanks.
>
> I've merged the entire series into my pdx86/review-hans branch now:
> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
> (with the lines in this patch shortened to 80 chars).
>
> Once the builders have had some time to play with this branch
> (and once I've run some tests to make sure the patches still
> work as expected) I will push to platform-drivers-x86/for-next .

Excellent progress with this Hans, thanks for investing so heavily
in getting this right after my initial complaints, the result is extremely
appealing an useful.

Yours,
Linus Walleij