diff mbox series

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

Message ID 20230127203729.10205-2-hdegoede@redhat.com
State Accepted
Commit b6e10ff6c23deb7f01d342875f7a69bae067c3c8
Headers show
Series int3472/media privacy LED support | expand

Commit Message

Hans de Goede Jan. 27, 2023, 8:37 p.m. UTC 
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

Comments

Laurent Pinchart Jan. 28, 2023, 1:42 p.m. UTC | #1 
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.
Linus Walleij Jan. 30, 2023, 10:35 p.m. UTC | #2 
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
diff mbox series

Patch

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