Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v3 RESEND] media: vimc: fla: Add virtual flash subdevice
@ 2019-12-03 23:01 Lucas A. M. Magalhães
  2019-12-04 10:03 ` Dafna Hirschfeld
  0 siblings, 1 reply; 4+ messages in thread
From: Lucas A. M. Magalhães @ 2019-12-03 23:01 UTC (permalink / raw)
  To: linux-media
  Cc: hverkuil, linux-kernel, helen.koike, edusbarretto, lkcamp,
	Lucas A . M . Magalhaes

From: Lucas A. M. Magalhaes <lucmaga@gmail.com>

Add a virtual subdevice to simulate the flash control API.
Those are the supported controls:
v4l2-ctl -d /dev/v4l-subdev6 -L
Flash Controls

                       led_mode 0x009c0901 (menu)   : min=0 max=2 default=1 value=1
                                0: Off
                                1: Flash
                                2: Torch
                  strobe_source 0x009c0902 (menu)   : min=0 max=1 default=0 value=0
                                0: Software
                                1: External
                         strobe 0x009c0903 (button) : flags=write-only, execute-on-write
                    stop_strobe 0x009c0904 (button) : flags=write-only, execute-on-write
                  strobe_status 0x009c0905 (bool)   : default=0 value=0 flags=read-only
                 strobe_timeout 0x009c0906 (int)    : min=50 max=400 step=50 default=50 value=400
           intensity_flash_mode 0x009c0907 (int)    : min=23040 max=1499600 step=11718 default=23040 value=23040
           intensity_torch_mode 0x009c0908 (int)    : min=2530 max=187100 step=1460 default=2530 value=2530
            intensity_indicator 0x009c0909 (int)    : min=0 max=255 step=1 default=0 value=0
                         faults 0x009c090a (bitmask): max=0x00000002 default=0x00000000 value=0x00000000

Co-authored-by: Eduardo Barretto <edusbarretto@gmail.com>
Signed-off-by: Eduardo Barretto <edusbarretto@gmail.com>
Signed-off-by: Lucas A. M. Magalhães <lucmaga@gmail.com>

---
Hi,

I've copied some values from another driver (lm3646) to make it more
realistic, as suggested by Hans. All values except for
V4L2_CID_FLASH_INDICATOR_INTENSITY, which I couldn't find any
implementation.

The v4l-compliance is failing. From the documentation
V4L2_CID_FLASH_STROBE should just work if the
V4L2_CID_FLASH_STROBE_SOURCE is "Software" and the
V4L2_CID_FLASH_LED_MODE is "Flash", otherwise it should fail. With the
standard values configured for the V4L2_CID_FLASH_STROBE will not fail.
But during the tests v4l-compliance sets V4L2_CID_FLASH_LED_MODE to
"Torch" and V4L2_CID_FLASH_STROBE_SOURCE to "External" which makes
V4L2_CID_FLASH_STROBE to fail. How do I proceed? Should the
v4l-compliance be changed?

Changes in v3:
	- Fix style errors
	- Use more realistic numbers for the controllers
	- Change from kthread to workqueue
	- Change commit message for the new controllers values

Changes in v2:
	- Fix v4l2-complience errors
	- Add V4L2_CID_FLASH_STROBE_STATUS behavior
	- Add V4L2_CID_FLASH_STROBE restrictions
	- Remove vimc_fla_g_volatile_ctrl
	- Remove unnecessarie V4L2_CID_FLASH_CLASS
	- Change varables names

 drivers/media/platform/vimc/Makefile      |   2 +-
 drivers/media/platform/vimc/vimc-common.c |   2 +
 drivers/media/platform/vimc/vimc-common.h |   4 +
 drivers/media/platform/vimc/vimc-core.c   |   5 +
 drivers/media/platform/vimc/vimc-flash.c  | 248 ++++++++++++++++++++++
 5 files changed, 260 insertions(+), 1 deletion(-)
 create mode 100644 drivers/media/platform/vimc/vimc-flash.c

diff --git a/drivers/media/platform/vimc/Makefile b/drivers/media/platform/vimc/Makefile
index a53b2b532e9f..e759bbb04b14 100644
--- a/drivers/media/platform/vimc/Makefile
+++ b/drivers/media/platform/vimc/Makefile
@@ -1,6 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 vimc-y := vimc-core.o vimc-common.o vimc-streamer.o vimc-capture.o \
-		vimc-debayer.o vimc-scaler.o vimc-sensor.o
+		vimc-debayer.o vimc-scaler.o vimc-sensor.o vimc-flash.o
 
 obj-$(CONFIG_VIDEO_VIMC) += vimc.o
 
diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c
index a3120f4f7a90..cb786de75573 100644
--- a/drivers/media/platform/vimc/vimc-common.c
+++ b/drivers/media/platform/vimc/vimc-common.c
@@ -203,6 +203,8 @@ struct media_pad *vimc_pads_init(u16 num_pads, const unsigned long *pads_flag)
 	struct media_pad *pads;
 	unsigned int i;
 
+	if (!num_pads)
+		return NULL;
 	/* Allocate memory for the pads */
 	pads = kcalloc(num_pads, sizeof(*pads), GFP_KERNEL);
 	if (!pads)
diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
index 698db7c07645..19815f0f4d40 100644
--- a/drivers/media/platform/vimc/vimc-common.h
+++ b/drivers/media/platform/vimc/vimc-common.h
@@ -169,6 +169,10 @@ struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
 				     const char *vcfg_name);
 void vimc_sen_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
 
+struct vimc_ent_device *vimc_fla_add(struct vimc_device *vimc,
+				     const char *vcfg_name);
+void vimc_fla_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
+
 /**
  * vimc_pads_init - initialize pads
  *
diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c
index 6e3e5c91ae39..5f6c750d3d8d 100644
--- a/drivers/media/platform/vimc/vimc-core.c
+++ b/drivers/media/platform/vimc/vimc-core.c
@@ -91,6 +91,11 @@ static struct vimc_ent_config ent_config[] = {
 		.add = vimc_cap_add,
 		.rm = vimc_cap_rm,
 	},
+	{
+		.name = "Flash Controller",
+		.add = vimc_fla_add,
+		.rm = vimc_fla_rm,
+	}
 };
 
 static const struct vimc_ent_link ent_links[] = {
diff --git a/drivers/media/platform/vimc/vimc-flash.c b/drivers/media/platform/vimc/vimc-flash.c
new file mode 100644
index 000000000000..3918beecec57
--- /dev/null
+++ b/drivers/media/platform/vimc/vimc-flash.c
@@ -0,0 +1,248 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * vimc-flash.c Virtual Media Controller Driver
+ *
+ * Copyright (C) 2019
+ * Contributors: Lucas A. M. Magalhães <lamm@lucmaga.dev>
+ *               Eduardo Barretto <edusbarretto@gmail.com>
+ *
+ */
+
+#include <linux/delay.h>
+#include <linux/workqueue.h>
+#include <linux/sched.h>
+#include <linux/vmalloc.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-event.h>
+#include <media/v4l2-subdev.h>
+
+#include "vimc-common.h"
+
+/*
+ * Flash timeout in ms
+ */
+#define VIMC_FLASH_TIMEOUT_MS_MIN 50
+#define VIMC_FLASH_TIMEOUT_MS_MAX 400
+#define VIMC_FLASH_TIMEOUT_MS_STEP 50
+
+/*
+ * Torch intencity in uA
+ */
+#define VIMC_FLASH_TORCH_UA_MIN 2530
+#define VIMC_FLASH_TORCH_UA_MAX 187100
+#define VIMC_FLASH_TORCH_UA_STEP 1460
+
+/*
+ * Flash intencity in uA
+ */
+#define VIMC_FLASH_FLASH_UA_MIN 23040
+#define VIMC_FLASH_FLASH_UA_MAX 1499600
+#define VIMC_FLASH_FLASH_UA_STEP 11718
+
+struct vimc_fla_device {
+	struct vimc_ent_device ved;
+	struct v4l2_subdev sd;
+	struct v4l2_ctrl_handler hdl;
+	int strobe_source;
+	bool is_strobe;
+	int led_mode;
+	int indicator_intensity;
+	int torch_intensity;
+	int flash_intensity;
+	u64 timeout;
+	u64 last_strobe;
+	struct workqueue_struct *wq;
+	struct work_struct work;
+	struct v4l2_ctrl *strobe_status_ctl;
+};
+
+static void vimc_fla_strobe_work(struct work_struct *work)
+{
+	struct vimc_fla_device *vfla =
+		container_of(work, struct vimc_fla_device, work);
+	v4l2_ctrl_s_ctrl(vfla->strobe_status_ctl, true);
+	vfla->last_strobe = ktime_get_ns();
+	while (vfla->is_strobe &&
+	       vfla->last_strobe + vfla->timeout > ktime_get_ns()) {
+		msleep_interruptible(VIMC_FLASH_TIMEOUT_MS_STEP);
+	}
+	v4l2_ctrl_s_ctrl(vfla->strobe_status_ctl, false);
+}
+
+static int vimc_fla_s_ctrl(struct v4l2_ctrl *c)
+{
+	struct vimc_fla_device *vfla =
+		container_of(c->handler, struct vimc_fla_device, hdl);
+
+	switch (c->id) {
+	case V4L2_CID_FLASH_LED_MODE:
+		vfla->led_mode = c->val;
+		return 0;
+	case V4L2_CID_FLASH_STROBE_SOURCE:
+		vfla->strobe_source = c->val;
+		return 0;
+	case V4L2_CID_FLASH_STROBE:
+		if (vfla->led_mode != V4L2_FLASH_LED_MODE_FLASH ||
+		    vfla->strobe_source != V4L2_FLASH_STROBE_SOURCE_SOFTWARE){
+			return -EINVAL;
+		}
+		queue_work(vfla->wq, &vfla->work);
+		return 0;
+	case V4L2_CID_FLASH_STROBE_STATUS:
+		vfla->is_strobe = c->val;
+		return 0;
+	case V4L2_CID_FLASH_STROBE_STOP:
+		vfla->is_strobe = false;
+		return 0;
+	case V4L2_CID_FLASH_TIMEOUT:
+		vfla->timeout = c->val * 1000000; /* MS to NS */
+		return 0;
+	case V4L2_CID_FLASH_INTENSITY:
+		vfla->flash_intensity = c->val;
+		return 0;
+	case V4L2_CID_FLASH_TORCH_INTENSITY:
+		vfla->torch_intensity = c->val;
+		return 0;
+	case V4L2_CID_FLASH_INDICATOR_INTENSITY:
+		vfla->indicator_intensity = c->val;
+		return 0;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static const struct v4l2_ctrl_ops vimc_fla_ctrl_ops = {
+	.s_ctrl = vimc_fla_s_ctrl,
+};
+
+static const struct v4l2_subdev_core_ops vimc_fla_core_ops = {
+	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
+	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
+};
+
+static const struct v4l2_subdev_ops vimc_fla_ops = {
+	.core = &vimc_fla_core_ops,
+};
+
+static void vimc_fla_release(struct v4l2_subdev *sd)
+{
+	struct vimc_fla_device *vfla =
+				container_of(sd, struct vimc_fla_device, sd);
+
+	v4l2_ctrl_handler_free(&vfla->hdl);
+	kfree(vfla);
+}
+
+static const struct v4l2_subdev_internal_ops vimc_fla_int_ops = {
+	.release = vimc_fla_release,
+};
+
+/* initialize device */
+struct vimc_ent_device *vimc_fla_add(struct vimc_device *vimc,
+				     const char *vcfg_name)
+{
+	struct v4l2_device *v4l2_dev = &vimc->v4l2_dev;
+	struct vimc_fla_device *vfla;
+	int ret;
+
+	/* Allocate the vfla struct */
+	vfla = kzalloc(sizeof(*vfla), GFP_KERNEL);
+	if (!vfla)
+		return NULL;
+
+	v4l2_ctrl_handler_init(&vfla->hdl, 4);
+	v4l2_ctrl_new_std_menu(&vfla->hdl, &vimc_fla_ctrl_ops,
+			       V4L2_CID_FLASH_LED_MODE,
+			       V4L2_FLASH_LED_MODE_TORCH, ~0x7,
+			       V4L2_FLASH_LED_MODE_FLASH);
+	v4l2_ctrl_new_std_menu(&vfla->hdl, &vimc_fla_ctrl_ops,
+			       V4L2_CID_FLASH_STROBE_SOURCE, 0x1, ~0x3,
+			       V4L2_FLASH_STROBE_SOURCE_SOFTWARE);
+	v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
+			  V4L2_CID_FLASH_STROBE, 0, 0, 0, 0);
+	v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
+			  V4L2_CID_FLASH_STROBE_STOP, 0, 0, 0, 0);
+	v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
+			  V4L2_CID_FLASH_TIMEOUT, VIMC_FLASH_TIMEOUT_MS_MIN,
+			  VIMC_FLASH_TIMEOUT_MS_MAX,
+			  VIMC_FLASH_TIMEOUT_MS_STEP,
+			  VIMC_FLASH_TIMEOUT_MS_MIN);
+	v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
+			  V4L2_CID_FLASH_TORCH_INTENSITY,
+			  VIMC_FLASH_TORCH_UA_MIN,
+			  VIMC_FLASH_TORCH_UA_MAX,
+			  VIMC_FLASH_TORCH_UA_STEP,
+			  VIMC_FLASH_TORCH_UA_MIN);
+	v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
+			  V4L2_CID_FLASH_INTENSITY,
+			  VIMC_FLASH_FLASH_UA_MIN,
+			  VIMC_FLASH_FLASH_UA_MAX,
+			  VIMC_FLASH_FLASH_UA_STEP,
+			  VIMC_FLASH_FLASH_UA_MIN);
+	v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
+			  V4L2_CID_FLASH_INDICATOR_INTENSITY,
+			  0,
+			  255,
+			  1,
+			  0);
+	v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
+			  V4L2_CID_FLASH_STROBE_STATUS, 0, 1, 1, 0);
+	v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
+			  V4L2_CID_FLASH_FAULT, 0,
+			  V4L2_FLASH_FAULT_TIMEOUT, 0, 0);
+	vfla->sd.ctrl_handler = &vfla->hdl;
+	if (vfla->hdl.error) {
+		ret = vfla->hdl.error;
+		goto err_free_vfla;
+	}
+	vfla->strobe_status_ctl = v4l2_ctrl_find(&vfla->hdl,
+						 V4L2_CID_FLASH_STROBE_STATUS);
+
+	/* Initialize ved and sd */
+	ret = vimc_ent_sd_register(&vfla->ved, &vfla->sd, v4l2_dev,
+				   vcfg_name,
+				   MEDIA_ENT_F_FLASH, 0, NULL,
+				   &vimc_fla_int_ops, &vimc_fla_ops);
+	if (ret)
+		goto err_free_hdl;
+
+	/* Create processing workqueue */
+	vfla->wq = alloc_workqueue("%s", 0, 0, "vimc-flash thread");
+	if (!vfla->wq)
+		goto err_unregister;
+
+	INIT_WORK(&vfla->work, vimc_fla_strobe_work);
+	/* Initialize standard values */
+	vfla->indicator_intensity = 0;
+	vfla->torch_intensity = 0;
+	vfla->flash_intensity = 0;
+	vfla->is_strobe = false;
+	vfla->timeout = 0;
+	vfla->last_strobe = 0;
+	vfla->strobe_source = V4L2_FLASH_STROBE_SOURCE_SOFTWARE;
+	vfla->led_mode = V4L2_FLASH_LED_MODE_FLASH;
+
+	return &vfla->ved;
+
+err_unregister:
+	vimc_ent_sd_unregister(&vfla->ved, &vfla->sd);
+err_free_hdl:
+	v4l2_ctrl_handler_free(&vfla->hdl);
+err_free_vfla:
+	kfree(vfla);
+
+	return NULL;
+}
+
+void vimc_fla_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
+{
+	struct vimc_fla_device *vfla;
+
+	if (!ved)
+		return;
+
+	vfla = container_of(ved, struct vimc_fla_device, ved);
+	destroy_workqueue(vfla->wq);
+	vimc_ent_sd_unregister(ved, &vfla->sd);
+}
-- 
2.23.0


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3 RESEND] media: vimc: fla: Add virtual flash subdevice
  2019-12-03 23:01 [PATCH v3 RESEND] media: vimc: fla: Add virtual flash subdevice Lucas A. M. Magalhães
@ 2019-12-04 10:03 ` Dafna Hirschfeld
  2019-12-04 23:12   ` Lucas Magalhães
  0 siblings, 1 reply; 4+ messages in thread
From: Dafna Hirschfeld @ 2019-12-04 10:03 UTC (permalink / raw)
  To: Lucas A. M. Magalhães, linux-media
  Cc: hverkuil, linux-kernel, helen.koike, edusbarretto, lkcamp

Hi,
Thanks for the patch

On 12/4/19 12:01 AM, Lucas A. M. Magalhães wrote:
> From: Lucas A. M. Magalhaes <lucmaga@gmail.com>
> 
> Add a virtual subdevice to simulate the flash control API.
> Those are the supported controls:
> v4l2-ctl -d /dev/v4l-subdev6 -L
> Flash Controls
> 
>                         led_mode 0x009c0901 (menu)   : min=0 max=2 default=1 value=1
>                                  0: Off
>                                  1: Flash
>                                  2: Torch
>                    strobe_source 0x009c0902 (menu)   : min=0 max=1 default=0 value=0
>                                  0: Software
>                                  1: External
>                           strobe 0x009c0903 (button) : flags=write-only, execute-on-write
>                      stop_strobe 0x009c0904 (button) : flags=write-only, execute-on-write
>                    strobe_status 0x009c0905 (bool)   : default=0 value=0 flags=read-only
>                   strobe_timeout 0x009c0906 (int)    : min=50 max=400 step=50 default=50 value=400
>             intensity_flash_mode 0x009c0907 (int)    : min=23040 max=1499600 step=11718 default=23040 value=23040
>             intensity_torch_mode 0x009c0908 (int)    : min=2530 max=187100 step=1460 default=2530 value=2530
>              intensity_indicator 0x009c0909 (int)    : min=0 max=255 step=1 default=0 value=0
>                           faults 0x009c090a (bitmask): max=0x00000002 default=0x00000000 value=0x00000000
> 
> Co-authored-by: Eduardo Barretto <edusbarretto@gmail.com>
> Signed-off-by: Eduardo Barretto <edusbarretto@gmail.com>
> Signed-off-by: Lucas A. M. Magalhães <lucmaga@gmail.com>
> 
> ---
> Hi,
> 
> I've copied some values from another driver (lm3646) to make it more
> realistic, as suggested by Hans. All values except for
> V4L2_CID_FLASH_INDICATOR_INTENSITY, which I couldn't find any
> implementation.
> 
> The v4l-compliance is failing. From the documentation
> V4L2_CID_FLASH_STROBE should just work if the
> V4L2_CID_FLASH_STROBE_SOURCE is "Software" and the
> V4L2_CID_FLASH_LED_MODE is "Flash", otherwise it should fail. With the
> standard values configured for the V4L2_CID_FLASH_STROBE will not fail.
> But during the tests v4l-compliance sets V4L2_CID_FLASH_LED_MODE to
> "Torch" and V4L2_CID_FLASH_STROBE_SOURCE to "External" which makes
> V4L2_CID_FLASH_STROBE to fail. How do I proceed? Should the
> v4l-compliance be changed?
> 
> Changes in v3:
> 	- Fix style errors
> 	- Use more realistic numbers for the controllers
> 	- Change from kthread to workqueue
> 	- Change commit message for the new controllers values
> 
> Changes in v2:
> 	- Fix v4l2-complience errors
> 	- Add V4L2_CID_FLASH_STROBE_STATUS behavior
> 	- Add V4L2_CID_FLASH_STROBE restrictions
> 	- Remove vimc_fla_g_volatile_ctrl
> 	- Remove unnecessarie V4L2_CID_FLASH_CLASS
> 	- Change varables names
> 
>   drivers/media/platform/vimc/Makefile      |   2 +-
>   drivers/media/platform/vimc/vimc-common.c |   2 +
>   drivers/media/platform/vimc/vimc-common.h |   4 +
>   drivers/media/platform/vimc/vimc-core.c   |   5 +
>   drivers/media/platform/vimc/vimc-flash.c  | 248 ++++++++++++++++++++++
>   5 files changed, 260 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/media/platform/vimc/vimc-flash.c
> 
> diff --git a/drivers/media/platform/vimc/Makefile b/drivers/media/platform/vimc/Makefile
> index a53b2b532e9f..e759bbb04b14 100644
> --- a/drivers/media/platform/vimc/Makefile
> +++ b/drivers/media/platform/vimc/Makefile
> @@ -1,6 +1,6 @@
>   # SPDX-License-Identifier: GPL-2.0
>   vimc-y := vimc-core.o vimc-common.o vimc-streamer.o vimc-capture.o \
> -		vimc-debayer.o vimc-scaler.o vimc-sensor.o
> +		vimc-debayer.o vimc-scaler.o vimc-sensor.o vimc-flash.o
>   
>   obj-$(CONFIG_VIDEO_VIMC) += vimc.o
>   
> diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c
> index a3120f4f7a90..cb786de75573 100644
> --- a/drivers/media/platform/vimc/vimc-common.c
> +++ b/drivers/media/platform/vimc/vimc-common.c
> @@ -203,6 +203,8 @@ struct media_pad *vimc_pads_init(u16 num_pads, const unsigned long *pads_flag)
>   	struct media_pad *pads;
>   	unsigned int i;
>   
> +	if (!num_pads)
> +		return NULL;
Please rebase on top of latest master,
this function was removed in patch 'media: vimc: embed the pads of entities in the entities' structs'

>   	/* Allocate memory for the pads */
>   	pads = kcalloc(num_pads, sizeof(*pads), GFP_KERNEL);
>   	if (!pads)
> diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
> index 698db7c07645..19815f0f4d40 100644
> --- a/drivers/media/platform/vimc/vimc-common.h
> +++ b/drivers/media/platform/vimc/vimc-common.h
> @@ -169,6 +169,10 @@ struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
>   				     const char *vcfg_name);
>   void vimc_sen_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
>   
> +struct vimc_ent_device *vimc_fla_add(struct vimc_device *vimc,
> +				     const char *vcfg_name);
This should be lined with the opening '('

> +void vimc_fla_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
> +
>   /**
>    * vimc_pads_init - initialize pads
>    *
> diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c
> index 6e3e5c91ae39..5f6c750d3d8d 100644
> --- a/drivers/media/platform/vimc/vimc-core.c
> +++ b/drivers/media/platform/vimc/vimc-core.c
> @@ -91,6 +91,11 @@ static struct vimc_ent_config ent_config[] = {
>   		.add = vimc_cap_add,
>   		.rm = vimc_cap_rm,
>   	},
> +	{
> +		.name = "Flash Controller",
> +		.add = vimc_fla_add,
> +		.rm = vimc_fla_rm,
> +	}
>   };
>   
>   static const struct vimc_ent_link ent_links[] = {
> diff --git a/drivers/media/platform/vimc/vimc-flash.c b/drivers/media/platform/vimc/vimc-flash.c
> new file mode 100644
> index 000000000000..3918beecec57
> --- /dev/null
> +++ b/drivers/media/platform/vimc/vimc-flash.c
> @@ -0,0 +1,248 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * vimc-flash.c Virtual Media Controller Driver
> + *
> + * Copyright (C) 2019
> + * Contributors: Lucas A. M. Magalhães <lamm@lucmaga.dev>
> + *               Eduardo Barretto <edusbarretto@gmail.com>
> + *
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/workqueue.h>
> +#include <linux/sched.h>
> +#include <linux/vmalloc.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-event.h>
> +#include <media/v4l2-subdev.h>
> +
> +#include "vimc-common.h"
> +
> +/*
> + * Flash timeout in ms
> + */
> +#define VIMC_FLASH_TIMEOUT_MS_MIN 50
> +#define VIMC_FLASH_TIMEOUT_MS_MAX 400
> +#define VIMC_FLASH_TIMEOUT_MS_STEP 50
> +
> +/*
> + * Torch intencity in uA
> + */
> +#define VIMC_FLASH_TORCH_UA_MIN 2530
> +#define VIMC_FLASH_TORCH_UA_MAX 187100
> +#define VIMC_FLASH_TORCH_UA_STEP 1460
> +
> +/*
> + * Flash intencity in uA
> + */
> +#define VIMC_FLASH_FLASH_UA_MIN 23040
> +#define VIMC_FLASH_FLASH_UA_MAX 1499600
> +#define VIMC_FLASH_FLASH_UA_STEP 11718
> +
> +struct vimc_fla_device {
> +	struct vimc_ent_device ved;
> +	struct v4l2_subdev sd;
> +	struct v4l2_ctrl_handler hdl;
> +	int strobe_source;
> +	bool is_strobe;
> +	int led_mode;
> +	int indicator_intensity;
> +	int torch_intensity;
> +	int flash_intensity;
> +	u64 timeout;
> +	u64 last_strobe;
> +	struct workqueue_struct *wq;
> +	struct work_struct work;
> +	struct v4l2_ctrl *strobe_status_ctl;
> +};
> +
> +static void vimc_fla_strobe_work(struct work_struct *work)
> +{
> +	struct vimc_fla_device *vfla =
> +		container_of(work, struct vimc_fla_device, work);
> +	v4l2_ctrl_s_ctrl(vfla->strobe_status_ctl, true);
> +	vfla->last_strobe = ktime_get_ns();
> +	while (vfla->is_strobe &&
> +	       vfla->last_strobe + vfla->timeout > ktime_get_ns()) {
> +		msleep_interruptible(VIMC_FLASH_TIMEOUT_MS_STEP);
> +	}
> +	v4l2_ctrl_s_ctrl(vfla->strobe_status_ctl, false);
> +}
> +
> +static int vimc_fla_s_ctrl(struct v4l2_ctrl *c)
> +{
> +	struct vimc_fla_device *vfla =
> +		container_of(c->handler, struct vimc_fla_device, hdl);
> +
> +	switch (c->id) {
> +	case V4L2_CID_FLASH_LED_MODE:
> +		vfla->led_mode = c->val;
> +		return 0;
> +	case V4L2_CID_FLASH_STROBE_SOURCE:
> +		vfla->strobe_source = c->val;
> +		return 0;
> +	case V4L2_CID_FLASH_STROBE:
> +		if (vfla->led_mode != V4L2_FLASH_LED_MODE_FLASH ||
> +		    vfla->strobe_source != V4L2_FLASH_STROBE_SOURCE_SOFTWARE){
you can add error/warning debug here,
if you choose not to, then the parentheses are redundant.
Additionally, the opening '{' should come after a space
You can run srctipts/checkpatch.pl before submitting to catch such issues

> +			return -EINVAL;
> +		}
> +		queue_work(vfla->wq, &vfla->work);
> +		return 0;
> +	case V4L2_CID_FLASH_STROBE_STATUS:
> +		vfla->is_strobe = c->val;
> +		return 0;
> +	case V4L2_CID_FLASH_STROBE_STOP:
> +		vfla->is_strobe = false;
> +		return 0;
> +	case V4L2_CID_FLASH_TIMEOUT:
> +		vfla->timeout = c->val * 1000000; /* MS to NS */
> +		return 0;
> +	case V4L2_CID_FLASH_INTENSITY:
> +		vfla->flash_intensity = c->val;
> +		return 0;
> +	case V4L2_CID_FLASH_TORCH_INTENSITY:
> +		vfla->torch_intensity = c->val;
> +		return 0;
> +	case V4L2_CID_FLASH_INDICATOR_INTENSITY:
> +		vfla->indicator_intensity = c->val;
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static const struct v4l2_ctrl_ops vimc_fla_ctrl_ops = {
> +	.s_ctrl = vimc_fla_s_ctrl,
> +};
> +
> +static const struct v4l2_subdev_core_ops vimc_fla_core_ops = {
> +	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> +	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
> +};
> +
> +static const struct v4l2_subdev_ops vimc_fla_ops = {
> +	.core = &vimc_fla_core_ops,
> +};
> +
> +static void vimc_fla_release(struct v4l2_subdev *sd)
> +{
> +	struct vimc_fla_device *vfla =
> +				container_of(sd, struct vimc_fla_device, sd);
one tab is enough
> +
> +	v4l2_ctrl_handler_free(&vfla->hdl);
> +	kfree(vfla);
> +}
> +
> +static const struct v4l2_subdev_internal_ops vimc_fla_int_ops = {
> +	.release = vimc_fla_release,
> +};
> +
> +/* initialize device */
> +struct vimc_ent_device *vimc_fla_add(struct vimc_device *vimc,
> +				     const char *vcfg_name)
> +{
> +	struct v4l2_device *v4l2_dev = &vimc->v4l2_dev;
> +	struct vimc_fla_device *vfla;
> +	int ret;
> +
> +	/* Allocate the vfla struct */
> +	vfla = kzalloc(sizeof(*vfla), GFP_KERNEL);
> +	if (!vfla)
> +		return NULL;
I think it is better to change the return values of the .add calbbacks
to return ERR_PTR upon error and not just NULL so that different types of
errors can be distinguished.
(This is not related specifically to this patch),
If you want you can send a patch fixing that.

> +
> +	v4l2_ctrl_handler_init(&vfla->hdl, 4);
> +	v4l2_ctrl_new_std_menu(&vfla->hdl, &vimc_fla_ctrl_ops,
> +			       V4L2_CID_FLASH_LED_MODE,
> +			       V4L2_FLASH_LED_MODE_TORCH, ~0x7,
> +			       V4L2_FLASH_LED_MODE_FLASH);
> +	v4l2_ctrl_new_std_menu(&vfla->hdl, &vimc_fla_ctrl_ops,
> +			       V4L2_CID_FLASH_STROBE_SOURCE, 0x1, ~0x3,
> +			       V4L2_FLASH_STROBE_SOURCE_SOFTWARE);
> +	v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> +			  V4L2_CID_FLASH_STROBE, 0, 0, 0, 0);
> +	v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> +			  V4L2_CID_FLASH_STROBE_STOP, 0, 0, 0, 0);
> +	v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> +			  V4L2_CID_FLASH_TIMEOUT, VIMC_FLASH_TIMEOUT_MS_MIN,
> +			  VIMC_FLASH_TIMEOUT_MS_MAX,
> +			  VIMC_FLASH_TIMEOUT_MS_STEP,
> +			  VIMC_FLASH_TIMEOUT_MS_MIN);
> +	v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> +			  V4L2_CID_FLASH_TORCH_INTENSITY,
> +			  VIMC_FLASH_TORCH_UA_MIN,
> +			  VIMC_FLASH_TORCH_UA_MAX,
> +			  VIMC_FLASH_TORCH_UA_STEP,
> +			  VIMC_FLASH_TORCH_UA_MIN);
> +	v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> +			  V4L2_CID_FLASH_INTENSITY,
> +			  VIMC_FLASH_FLASH_UA_MIN,
> +			  VIMC_FLASH_FLASH_UA_MAX,
> +			  VIMC_FLASH_FLASH_UA_STEP,
> +			  VIMC_FLASH_FLASH_UA_MIN);
> +	v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> +			  V4L2_CID_FLASH_INDICATOR_INTENSITY,
> +			  0,
> +			  255,
> +			  1,
> +			  0);
why not having one line with "0,255,1,0);" ?

> +	v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> +			  V4L2_CID_FLASH_STROBE_STATUS, 0, 1, 1, 0);
> +	v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> +			  V4L2_CID_FLASH_FAULT, 0,
> +			  V4L2_FLASH_FAULT_TIMEOUT, 0, 0);
> +	vfla->sd.ctrl_handler = &vfla->hdl;
> +	if (vfla->hdl.error) {
> +		ret = vfla->hdl.error;
> +		goto err_free_vfla;
> +	}
> +	vfla->strobe_status_ctl = v4l2_ctrl_find(&vfla->hdl,
> +						 V4L2_CID_FLASH_STROBE_STATUS);
> +
> +	/* Initialize ved and sd */
> +	ret = vimc_ent_sd_register(&vfla->ved, &vfla->sd, v4l2_dev,
> +				   vcfg_name,
> +				   MEDIA_ENT_F_FLASH, 0, NULL,
> +				   &vimc_fla_int_ops, &vimc_fla_ops);
> +	if (ret)
> +		goto err_free_hdl;
> +
> +	/* Create processing workqueue */
> +	vfla->wq = alloc_workqueue("%s", 0, 0, "vimc-flash thread");
> +	if (!vfla->wq)
> +		goto err_unregister;
> +
> +	INIT_WORK(&vfla->work, vimc_fla_strobe_work);
> +	/* Initialize standard values */
> +	vfla->indicator_intensity = 0;
> +	vfla->torch_intensity = 0;
> +	vfla->flash_intensity = 0;
> +	vfla->is_strobe = false;
> +	vfla->timeout = 0;
> +	vfla->last_strobe = 0;
> +	vfla->strobe_source = V4L2_FLASH_STROBE_SOURCE_SOFTWARE;
> +	vfla->led_mode = V4L2_FLASH_LED_MODE_FLASH;
> +
> +	return &vfla->ved;
> +
> +err_unregister:
> +	vimc_ent_sd_unregister(&vfla->ved, &vfla->sd);
> +err_free_hdl:
> +	v4l2_ctrl_handler_free(&vfla->hdl);
> +err_free_vfla:
> +	kfree(vfla);
> +
> +	return NULL;
> +}
> +
> +void vimc_fla_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
> +{
> +	struct vimc_fla_device *vfla;
> +
> +	if (!ved)
> +		return;
> +
> +	vfla = container_of(ved, struct vimc_fla_device, ved);
> +	destroy_workqueue(vfla->wq);
> +	vimc_ent_sd_unregister(ved, &vfla->sd);
> +}
> 
Not sure but I think there are indentation issues in this patch, a tab should be 8 spaces

Thanks,
Dafna

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3 RESEND] media: vimc: fla: Add virtual flash subdevice
  2019-12-04 10:03 ` Dafna Hirschfeld
@ 2019-12-04 23:12   ` Lucas Magalhães
  2019-12-05 18:01     ` Dafna Hirschfeld
  0 siblings, 1 reply; 4+ messages in thread
From: Lucas Magalhães @ 2019-12-04 23:12 UTC (permalink / raw)
  To: Dafna Hirschfeld
  Cc: linux-media, Hans Verkuil, linux-kernel, Helen Koike,
	Eduardo Barretto, lkcamp

Hi Dafna,
Thanks for the review.

On Wed, Dec 4, 2019 at 7:03 AM Dafna Hirschfeld
<dafna.hirschfeld@collabora.com> wrote:
>
> Hi,
> Thanks for the patch
>
> On 12/4/19 12:01 AM, Lucas A. M. Magalhães wrote:
> > From: Lucas A. M. Magalhaes <lucmaga@gmail.com>
> >
> > Add a virtual subdevice to simulate the flash control API.
> > Those are the supported controls:
> > v4l2-ctl -d /dev/v4l-subdev6 -L
> > Flash Controls
> >
> >                         led_mode 0x009c0901 (menu)   : min=0 max=2 default=1 value=1
> >                                  0: Off
> >                                  1: Flash
> >                                  2: Torch
> >                    strobe_source 0x009c0902 (menu)   : min=0 max=1 default=0 value=0
> >                                  0: Software
> >                                  1: External
> >                           strobe 0x009c0903 (button) : flags=write-only, execute-on-write
> >                      stop_strobe 0x009c0904 (button) : flags=write-only, execute-on-write
> >                    strobe_status 0x009c0905 (bool)   : default=0 value=0 flags=read-only
> >                   strobe_timeout 0x009c0906 (int)    : min=50 max=400 step=50 default=50 value=400
> >             intensity_flash_mode 0x009c0907 (int)    : min=23040 max=1499600 step=11718 default=23040 value=23040
> >             intensity_torch_mode 0x009c0908 (int)    : min=2530 max=187100 step=1460 default=2530 value=2530
> >              intensity_indicator 0x009c0909 (int)    : min=0 max=255 step=1 default=0 value=0
> >                           faults 0x009c090a (bitmask): max=0x00000002 default=0x00000000 value=0x00000000
> >
> > Co-authored-by: Eduardo Barretto <edusbarretto@gmail.com>
> > Signed-off-by: Eduardo Barretto <edusbarretto@gmail.com>
> > Signed-off-by: Lucas A. M. Magalhães <lucmaga@gmail.com>
> >
> > ---
> > Hi,
> >
> > I've copied some values from another driver (lm3646) to make it more
> > realistic, as suggested by Hans. All values except for
> > V4L2_CID_FLASH_INDICATOR_INTENSITY, which I couldn't find any
> > implementation.
> >
> > The v4l-compliance is failing. From the documentation
> > V4L2_CID_FLASH_STROBE should just work if the
> > V4L2_CID_FLASH_STROBE_SOURCE is "Software" and the
> > V4L2_CID_FLASH_LED_MODE is "Flash", otherwise it should fail. With the
> > standard values configured for the V4L2_CID_FLASH_STROBE will not fail.
> > But during the tests v4l-compliance sets V4L2_CID_FLASH_LED_MODE to
> > "Torch" and V4L2_CID_FLASH_STROBE_SOURCE to "External" which makes
> > V4L2_CID_FLASH_STROBE to fail. How do I proceed? Should the
> > v4l-compliance be changed?
> >
> > Changes in v3:
> >       - Fix style errors
> >       - Use more realistic numbers for the controllers
> >       - Change from kthread to workqueue
> >       - Change commit message for the new controllers values
> >
> > Changes in v2:
> >       - Fix v4l2-complience errors
> >       - Add V4L2_CID_FLASH_STROBE_STATUS behavior
> >       - Add V4L2_CID_FLASH_STROBE restrictions
> >       - Remove vimc_fla_g_volatile_ctrl
> >       - Remove unnecessarie V4L2_CID_FLASH_CLASS
> >       - Change varables names
> >
> >   drivers/media/platform/vimc/Makefile      |   2 +-
> >   drivers/media/platform/vimc/vimc-common.c |   2 +
> >   drivers/media/platform/vimc/vimc-common.h |   4 +
> >   drivers/media/platform/vimc/vimc-core.c   |   5 +
> >   drivers/media/platform/vimc/vimc-flash.c  | 248 ++++++++++++++++++++++
> >   5 files changed, 260 insertions(+), 1 deletion(-)
> >   create mode 100644 drivers/media/platform/vimc/vimc-flash.c
> >
> > diff --git a/drivers/media/platform/vimc/Makefile b/drivers/media/platform/vimc/Makefile
> > index a53b2b532e9f..e759bbb04b14 100644
> > --- a/drivers/media/platform/vimc/Makefile
> > +++ b/drivers/media/platform/vimc/Makefile
> > @@ -1,6 +1,6 @@
> >   # SPDX-License-Identifier: GPL-2.0
> >   vimc-y := vimc-core.o vimc-common.o vimc-streamer.o vimc-capture.o \
> > -             vimc-debayer.o vimc-scaler.o vimc-sensor.o
> > +             vimc-debayer.o vimc-scaler.o vimc-sensor.o vimc-flash.o
> >
> >   obj-$(CONFIG_VIDEO_VIMC) += vimc.o
> >
> > diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c
> > index a3120f4f7a90..cb786de75573 100644
> > --- a/drivers/media/platform/vimc/vimc-common.c
> > +++ b/drivers/media/platform/vimc/vimc-common.c
> > @@ -203,6 +203,8 @@ struct media_pad *vimc_pads_init(u16 num_pads, const unsigned long *pads_flag)
> >       struct media_pad *pads;
> >       unsigned int i;
> >
> > +     if (!num_pads)
> > +             return NULL;
> Please rebase on top of latest master,
> this function was removed in patch 'media: vimc: embed the pads of entities in the entities' structs'
>
Ok.

> >       /* Allocate memory for the pads */
> >       pads = kcalloc(num_pads, sizeof(*pads), GFP_KERNEL);
> >       if (!pads)
> > diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
> > index 698db7c07645..19815f0f4d40 100644
> > --- a/drivers/media/platform/vimc/vimc-common.h
> > +++ b/drivers/media/platform/vimc/vimc-common.h
> > @@ -169,6 +169,10 @@ struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
> >                                    const char *vcfg_name);
> >   void vimc_sen_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
> >
> > +struct vimc_ent_device *vimc_fla_add(struct vimc_device *vimc,
> > +                                  const char *vcfg_name);
> This should be lined with the opening '('
>
That's strange. I'm sure it's not like this here. Maybe my smtp is alterating
my code for some reason. I will look this.

> > +void vimc_fla_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
> > +
> >   /**
> >    * vimc_pads_init - initialize pads
> >    *
> > diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c
> > index 6e3e5c91ae39..5f6c750d3d8d 100644
> > --- a/drivers/media/platform/vimc/vimc-core.c
> > +++ b/drivers/media/platform/vimc/vimc-core.c
> > @@ -91,6 +91,11 @@ static struct vimc_ent_config ent_config[] = {
> >               .add = vimc_cap_add,
> >               .rm = vimc_cap_rm,
> >       },
> > +     {
> > +             .name = "Flash Controller",
> > +             .add = vimc_fla_add,
> > +             .rm = vimc_fla_rm,
> > +     }
> >   };
> >
> >   static const struct vimc_ent_link ent_links[] = {
> > diff --git a/drivers/media/platform/vimc/vimc-flash.c b/drivers/media/platform/vimc/vimc-flash.c
> > new file mode 100644
> > index 000000000000..3918beecec57
> > --- /dev/null
> > +++ b/drivers/media/platform/vimc/vimc-flash.c
> > @@ -0,0 +1,248 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * vimc-flash.c Virtual Media Controller Driver
> > + *
> > + * Copyright (C) 2019
> > + * Contributors: Lucas A. M. Magalhães <lamm@lucmaga.dev>
> > + *               Eduardo Barretto <edusbarretto@gmail.com>
> > + *
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/workqueue.h>
> > +#include <linux/sched.h>
> > +#include <linux/vmalloc.h>
> > +#include <media/v4l2-ctrls.h>
> > +#include <media/v4l2-event.h>
> > +#include <media/v4l2-subdev.h>
> > +
> > +#include "vimc-common.h"
> > +
> > +/*
> > + * Flash timeout in ms
> > + */
> > +#define VIMC_FLASH_TIMEOUT_MS_MIN 50
> > +#define VIMC_FLASH_TIMEOUT_MS_MAX 400
> > +#define VIMC_FLASH_TIMEOUT_MS_STEP 50
> > +
> > +/*
> > + * Torch intencity in uA
> > + */
> > +#define VIMC_FLASH_TORCH_UA_MIN 2530
> > +#define VIMC_FLASH_TORCH_UA_MAX 187100
> > +#define VIMC_FLASH_TORCH_UA_STEP 1460
> > +
> > +/*
> > + * Flash intencity in uA
> > + */
> > +#define VIMC_FLASH_FLASH_UA_MIN 23040
> > +#define VIMC_FLASH_FLASH_UA_MAX 1499600
> > +#define VIMC_FLASH_FLASH_UA_STEP 11718
> > +
> > +struct vimc_fla_device {
> > +     struct vimc_ent_device ved;
> > +     struct v4l2_subdev sd;
> > +     struct v4l2_ctrl_handler hdl;
> > +     int strobe_source;
> > +     bool is_strobe;
> > +     int led_mode;
> > +     int indicator_intensity;
> > +     int torch_intensity;
> > +     int flash_intensity;
> > +     u64 timeout;
> > +     u64 last_strobe;
> > +     struct workqueue_struct *wq;
> > +     struct work_struct work;
> > +     struct v4l2_ctrl *strobe_status_ctl;
> > +};
> > +
> > +static void vimc_fla_strobe_work(struct work_struct *work)
> > +{
> > +     struct vimc_fla_device *vfla =
> > +             container_of(work, struct vimc_fla_device, work);
> > +     v4l2_ctrl_s_ctrl(vfla->strobe_status_ctl, true);
> > +     vfla->last_strobe = ktime_get_ns();
> > +     while (vfla->is_strobe &&
> > +            vfla->last_strobe + vfla->timeout > ktime_get_ns()) {
> > +             msleep_interruptible(VIMC_FLASH_TIMEOUT_MS_STEP);
> > +     }
> > +     v4l2_ctrl_s_ctrl(vfla->strobe_status_ctl, false);
> > +}
> > +
> > +static int vimc_fla_s_ctrl(struct v4l2_ctrl *c)
> > +{
> > +     struct vimc_fla_device *vfla =
> > +             container_of(c->handler, struct vimc_fla_device, hdl);
> > +
> > +     switch (c->id) {
> > +     case V4L2_CID_FLASH_LED_MODE:
> > +             vfla->led_mode = c->val;
> > +             return 0;
> > +     case V4L2_CID_FLASH_STROBE_SOURCE:
> > +             vfla->strobe_source = c->val;
> > +             return 0;
> > +     case V4L2_CID_FLASH_STROBE:
> > +             if (vfla->led_mode != V4L2_FLASH_LED_MODE_FLASH ||
> > +                 vfla->strobe_source != V4L2_FLASH_STROBE_SOURCE_SOFTWARE){
> you can add error/warning debug here,
> if you choose not to, then the parentheses are redundant.
> Additionally, the opening '{' should come after a space
> You can run srctipts/checkpatch.pl before submitting to catch such issues
>
> > +                     return -EINVAL;
> > +             }
> > +             queue_work(vfla->wq, &vfla->work);
> > +             return 0;
> > +     case V4L2_CID_FLASH_STROBE_STATUS:
> > +             vfla->is_strobe = c->val;
> > +             return 0;
> > +     case V4L2_CID_FLASH_STROBE_STOP:
> > +             vfla->is_strobe = false;
> > +             return 0;
> > +     case V4L2_CID_FLASH_TIMEOUT:
> > +             vfla->timeout = c->val * 1000000; /* MS to NS */
> > +             return 0;
> > +     case V4L2_CID_FLASH_INTENSITY:
> > +             vfla->flash_intensity = c->val;
> > +             return 0;
> > +     case V4L2_CID_FLASH_TORCH_INTENSITY:
> > +             vfla->torch_intensity = c->val;
> > +             return 0;
> > +     case V4L2_CID_FLASH_INDICATOR_INTENSITY:
> > +             vfla->indicator_intensity = c->val;
> > +             return 0;
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +     return 0;
> > +}
> > +
> > +static const struct v4l2_ctrl_ops vimc_fla_ctrl_ops = {
> > +     .s_ctrl = vimc_fla_s_ctrl,
> > +};
> > +
> > +static const struct v4l2_subdev_core_ops vimc_fla_core_ops = {
> > +     .subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> > +     .unsubscribe_event = v4l2_event_subdev_unsubscribe,
> > +};
> > +
> > +static const struct v4l2_subdev_ops vimc_fla_ops = {
> > +     .core = &vimc_fla_core_ops,
> > +};
> > +
> > +static void vimc_fla_release(struct v4l2_subdev *sd)
> > +{
> > +     struct vimc_fla_device *vfla =
> > +                             container_of(sd, struct vimc_fla_device, sd);
> one tab is enough
> > +
> > +     v4l2_ctrl_handler_free(&vfla->hdl);
> > +     kfree(vfla);
> > +}
> > +
> > +static const struct v4l2_subdev_internal_ops vimc_fla_int_ops = {
> > +     .release = vimc_fla_release,
> > +};
> > +
> > +/* initialize device */
> > +struct vimc_ent_device *vimc_fla_add(struct vimc_device *vimc,
> > +                                  const char *vcfg_name)
> > +{
> > +     struct v4l2_device *v4l2_dev = &vimc->v4l2_dev;
> > +     struct vimc_fla_device *vfla;
> > +     int ret;
> > +
> > +     /* Allocate the vfla struct */
> > +     vfla = kzalloc(sizeof(*vfla), GFP_KERNEL);
> > +     if (!vfla)
> > +             return NULL;
> I think it is better to change the return values of the .add calbbacks
> to return ERR_PTR upon error and not just NULL so that different types of
> errors can be distinguished.
> (This is not related specifically to this patch),
> If you want you can send a patch fixing that.
>

Sure. I will change it.

> > +
> > +     v4l2_ctrl_handler_init(&vfla->hdl, 4);
> > +     v4l2_ctrl_new_std_menu(&vfla->hdl, &vimc_fla_ctrl_ops,
> > +                            V4L2_CID_FLASH_LED_MODE,
> > +                            V4L2_FLASH_LED_MODE_TORCH, ~0x7,
> > +                            V4L2_FLASH_LED_MODE_FLASH);
> > +     v4l2_ctrl_new_std_menu(&vfla->hdl, &vimc_fla_ctrl_ops,
> > +                            V4L2_CID_FLASH_STROBE_SOURCE, 0x1, ~0x3,
> > +                            V4L2_FLASH_STROBE_SOURCE_SOFTWARE);
> > +     v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> > +                       V4L2_CID_FLASH_STROBE, 0, 0, 0, 0);
> > +     v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> > +                       V4L2_CID_FLASH_STROBE_STOP, 0, 0, 0, 0);
> > +     v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> > +                       V4L2_CID_FLASH_TIMEOUT, VIMC_FLASH_TIMEOUT_MS_MIN,
> > +                       VIMC_FLASH_TIMEOUT_MS_MAX,
> > +                       VIMC_FLASH_TIMEOUT_MS_STEP,
> > +                       VIMC_FLASH_TIMEOUT_MS_MIN);
> > +     v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> > +                       V4L2_CID_FLASH_TORCH_INTENSITY,
> > +                       VIMC_FLASH_TORCH_UA_MIN,
> > +                       VIMC_FLASH_TORCH_UA_MAX,
> > +                       VIMC_FLASH_TORCH_UA_STEP,
> > +                       VIMC_FLASH_TORCH_UA_MIN);
> > +     v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> > +                       V4L2_CID_FLASH_INTENSITY,
> > +                       VIMC_FLASH_FLASH_UA_MIN,
> > +                       VIMC_FLASH_FLASH_UA_MAX,
> > +                       VIMC_FLASH_FLASH_UA_STEP,
> > +                       VIMC_FLASH_FLASH_UA_MIN);
> > +     v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> > +                       V4L2_CID_FLASH_INDICATOR_INTENSITY,
> > +                       0,
> > +                       255,
> > +                       1,
> > +                       0);
> why not having one line with "0,255,1,0);" ?
>
> > +     v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> > +                       V4L2_CID_FLASH_STROBE_STATUS, 0, 1, 1, 0);
> > +     v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> > +                       V4L2_CID_FLASH_FAULT, 0,
> > +                       V4L2_FLASH_FAULT_TIMEOUT, 0, 0);
> > +     vfla->sd.ctrl_handler = &vfla->hdl;
> > +     if (vfla->hdl.error) {
> > +             ret = vfla->hdl.error;
> > +             goto err_free_vfla;
> > +     }
> > +     vfla->strobe_status_ctl = v4l2_ctrl_find(&vfla->hdl,
> > +                                              V4L2_CID_FLASH_STROBE_STATUS);
> > +
> > +     /* Initialize ved and sd */
> > +     ret = vimc_ent_sd_register(&vfla->ved, &vfla->sd, v4l2_dev,
> > +                                vcfg_name,
> > +                                MEDIA_ENT_F_FLASH, 0, NULL,
> > +                                &vimc_fla_int_ops, &vimc_fla_ops);
> > +     if (ret)
> > +             goto err_free_hdl;
> > +
> > +     /* Create processing workqueue */
> > +     vfla->wq = alloc_workqueue("%s", 0, 0, "vimc-flash thread");
> > +     if (!vfla->wq)
> > +             goto err_unregister;
> > +
> > +     INIT_WORK(&vfla->work, vimc_fla_strobe_work);
> > +     /* Initialize standard values */
> > +     vfla->indicator_intensity = 0;
> > +     vfla->torch_intensity = 0;
> > +     vfla->flash_intensity = 0;
> > +     vfla->is_strobe = false;
> > +     vfla->timeout = 0;
> > +     vfla->last_strobe = 0;
> > +     vfla->strobe_source = V4L2_FLASH_STROBE_SOURCE_SOFTWARE;
> > +     vfla->led_mode = V4L2_FLASH_LED_MODE_FLASH;
> > +
> > +     return &vfla->ved;
> > +
> > +err_unregister:
> > +     vimc_ent_sd_unregister(&vfla->ved, &vfla->sd);
> > +err_free_hdl:
> > +     v4l2_ctrl_handler_free(&vfla->hdl);
> > +err_free_vfla:
> > +     kfree(vfla);
> > +
> > +     return NULL;
> > +}
> > +
> > +void vimc_fla_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
> > +{
> > +     struct vimc_fla_device *vfla;
> > +
> > +     if (!ved)
> > +             return;
> > +
> > +     vfla = container_of(ved, struct vimc_fla_device, ved);
> > +     destroy_workqueue(vfla->wq);
> > +     vimc_ent_sd_unregister(ved, &vfla->sd);
> > +}
> >
> Not sure but I think there are indentation issues in this patch, a tab should be 8 spaces

This is strange. I already had style issues on the v2 so I runned the
checkpatch.pl on the
patch before sent and it didn't point this identation issues.

Here is the checkpatch.pl output. It could be the case that my smtp is
alterating the
it before transmission.

$ scripts/checkpatch.pl
patchset/0001-media-vimc-fla-Add-virtual-flash-subdevice.patch
WARNING: Possible unwrapped commit description (prefer a maximum 75
chars per line)
#14:
                       led_mode 0x009c0901 (menu)   : min=0 max=2
default=1 value=1

WARNING: Non-standard signature: Co-authored-by:
#30:
Co-authored-by: Eduardo Barretto <edusbarretto@gmail.com>

WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#131:
new file mode 100644

total: 0 errors, 3 warnings, 284 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

patchset/0001-media-vimc-fla-Add-virtual-flash-subdevice.patch has
style problems, please review.

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


Thanks,
Lucas

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3 RESEND] media: vimc: fla: Add virtual flash subdevice
  2019-12-04 23:12   ` Lucas Magalhães
@ 2019-12-05 18:01     ` Dafna Hirschfeld
  0 siblings, 0 replies; 4+ messages in thread
From: Dafna Hirschfeld @ 2019-12-05 18:01 UTC (permalink / raw)
  To: Lucas Magalhães
  Cc: linux-media, Hans Verkuil, linux-kernel, Helen Koike,
	Eduardo Barretto, lkcamp

Hi

On 12/5/19 12:12 AM, Lucas Magalhães wrote:
> Hi Dafna,
> Thanks for the review.
> 
> On Wed, Dec 4, 2019 at 7:03 AM Dafna Hirschfeld
> <dafna.hirschfeld@collabora.com> wrote:
>>
>> Hi,
>> Thanks for the patch
>>
>> On 12/4/19 12:01 AM, Lucas A. M. Magalhães wrote:
>>> From: Lucas A. M. Magalhaes <lucmaga@gmail.com>
>>>
>>> Add a virtual subdevice to simulate the flash control API.
>>> Those are the supported controls:
>>> v4l2-ctl -d /dev/v4l-subdev6 -L
>>> Flash Controls
>>>
>>>                          led_mode 0x009c0901 (menu)   : min=0 max=2 default=1 value=1
>>>                                   0: Off
>>>                                   1: Flash
>>>                                   2: Torch
>>>                     strobe_source 0x009c0902 (menu)   : min=0 max=1 default=0 value=0
>>>                                   0: Software
>>>                                   1: External
>>>                            strobe 0x009c0903 (button) : flags=write-only, execute-on-write
>>>                       stop_strobe 0x009c0904 (button) : flags=write-only, execute-on-write
>>>                     strobe_status 0x009c0905 (bool)   : default=0 value=0 flags=read-only
>>>                    strobe_timeout 0x009c0906 (int)    : min=50 max=400 step=50 default=50 value=400
>>>              intensity_flash_mode 0x009c0907 (int)    : min=23040 max=1499600 step=11718 default=23040 value=23040
>>>              intensity_torch_mode 0x009c0908 (int)    : min=2530 max=187100 step=1460 default=2530 value=2530
>>>               intensity_indicator 0x009c0909 (int)    : min=0 max=255 step=1 default=0 value=0
>>>                            faults 0x009c090a (bitmask): max=0x00000002 default=0x00000000 value=0x00000000
>>>
>>> Co-authored-by: Eduardo Barretto <edusbarretto@gmail.com>
>>> Signed-off-by: Eduardo Barretto <edusbarretto@gmail.com>
>>> Signed-off-by: Lucas A. M. Magalhães <lucmaga@gmail.com>
>>>
>>> ---
>>> Hi,
>>>
>>> I've copied some values from another driver (lm3646) to make it more
>>> realistic, as suggested by Hans. All values except for
>>> V4L2_CID_FLASH_INDICATOR_INTENSITY, which I couldn't find any
>>> implementation.
>>>
>>> The v4l-compliance is failing. From the documentation
>>> V4L2_CID_FLASH_STROBE should just work if the
>>> V4L2_CID_FLASH_STROBE_SOURCE is "Software" and the
>>> V4L2_CID_FLASH_LED_MODE is "Flash", otherwise it should fail. With the
>>> standard values configured for the V4L2_CID_FLASH_STROBE will not fail.
>>> But during the tests v4l-compliance sets V4L2_CID_FLASH_LED_MODE to
>>> "Torch" and V4L2_CID_FLASH_STROBE_SOURCE to "External" which makes
>>> V4L2_CID_FLASH_STROBE to fail. How do I proceed? Should the
>>> v4l-compliance be changed?
>>>
>>> Changes in v3:
>>>        - Fix style errors
>>>        - Use more realistic numbers for the controllers
>>>        - Change from kthread to workqueue
>>>        - Change commit message for the new controllers values
>>>
>>> Changes in v2:
>>>        - Fix v4l2-complience errors
>>>        - Add V4L2_CID_FLASH_STROBE_STATUS behavior
>>>        - Add V4L2_CID_FLASH_STROBE restrictions
>>>        - Remove vimc_fla_g_volatile_ctrl
>>>        - Remove unnecessarie V4L2_CID_FLASH_CLASS
>>>        - Change varables names
>>>
>>>    drivers/media/platform/vimc/Makefile      |   2 +-
>>>    drivers/media/platform/vimc/vimc-common.c |   2 +
>>>    drivers/media/platform/vimc/vimc-common.h |   4 +
>>>    drivers/media/platform/vimc/vimc-core.c   |   5 +
>>>    drivers/media/platform/vimc/vimc-flash.c  | 248 ++++++++++++++++++++++
>>>    5 files changed, 260 insertions(+), 1 deletion(-)
>>>    create mode 100644 drivers/media/platform/vimc/vimc-flash.c
>>>
>>> diff --git a/drivers/media/platform/vimc/Makefile b/drivers/media/platform/vimc/Makefile
>>> index a53b2b532e9f..e759bbb04b14 100644
>>> --- a/drivers/media/platform/vimc/Makefile
>>> +++ b/drivers/media/platform/vimc/Makefile
>>> @@ -1,6 +1,6 @@
>>>    # SPDX-License-Identifier: GPL-2.0
>>>    vimc-y := vimc-core.o vimc-common.o vimc-streamer.o vimc-capture.o \
>>> -             vimc-debayer.o vimc-scaler.o vimc-sensor.o
>>> +             vimc-debayer.o vimc-scaler.o vimc-sensor.o vimc-flash.o
>>>
>>>    obj-$(CONFIG_VIDEO_VIMC) += vimc.o
>>>
>>> diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c
>>> index a3120f4f7a90..cb786de75573 100644
>>> --- a/drivers/media/platform/vimc/vimc-common.c
>>> +++ b/drivers/media/platform/vimc/vimc-common.c
>>> @@ -203,6 +203,8 @@ struct media_pad *vimc_pads_init(u16 num_pads, const unsigned long *pads_flag)
>>>        struct media_pad *pads;
>>>        unsigned int i;
>>>
>>> +     if (!num_pads)
>>> +             return NULL;
>> Please rebase on top of latest master,
>> this function was removed in patch 'media: vimc: embed the pads of entities in the entities' structs'
>>
> Ok.
> 
>>>        /* Allocate memory for the pads */
>>>        pads = kcalloc(num_pads, sizeof(*pads), GFP_KERNEL);
>>>        if (!pads)
>>> diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
>>> index 698db7c07645..19815f0f4d40 100644
>>> --- a/drivers/media/platform/vimc/vimc-common.h
>>> +++ b/drivers/media/platform/vimc/vimc-common.h
>>> @@ -169,6 +169,10 @@ struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
>>>                                     const char *vcfg_name);
>>>    void vimc_sen_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
>>>
>>> +struct vimc_ent_device *vimc_fla_add(struct vimc_device *vimc,
>>> +                                  const char *vcfg_name);
>> This should be lined with the opening '('
>>
> That's strange. I'm sure it's not like this here. Maybe my smtp is alterating
> my code for some reason. I will look this.
Hi, apparently it is my email client that does not show the alignment correctly
sorry for the false comment

> 
>>> +void vimc_fla_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
>>> +
>>>    /**
>>>     * vimc_pads_init - initialize pads
>>>     *
>>> diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c
>>> index 6e3e5c91ae39..5f6c750d3d8d 100644
>>> --- a/drivers/media/platform/vimc/vimc-core.c
>>> +++ b/drivers/media/platform/vimc/vimc-core.c
>>> @@ -91,6 +91,11 @@ static struct vimc_ent_config ent_config[] = {
>>>                .add = vimc_cap_add,
>>>                .rm = vimc_cap_rm,
>>>        },
>>> +     {
>>> +             .name = "Flash Controller",
>>> +             .add = vimc_fla_add,
>>> +             .rm = vimc_fla_rm,
>>> +     }
>>>    };
>>>
>>>    static const struct vimc_ent_link ent_links[] = {
>>> diff --git a/drivers/media/platform/vimc/vimc-flash.c b/drivers/media/platform/vimc/vimc-flash.c
>>> new file mode 100644
>>> index 000000000000..3918beecec57
>>> --- /dev/null
>>> +++ b/drivers/media/platform/vimc/vimc-flash.c
>>> @@ -0,0 +1,248 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * vimc-flash.c Virtual Media Controller Driver
>>> + *
>>> + * Copyright (C) 2019
>>> + * Contributors: Lucas A. M. Magalhães <lamm@lucmaga.dev>
>>> + *               Eduardo Barretto <edusbarretto@gmail.com>
>>> + *
>>> + */
>>> +
>>> +#include <linux/delay.h>
>>> +#include <linux/workqueue.h>
>>> +#include <linux/sched.h>
>>> +#include <linux/vmalloc.h>
>>> +#include <media/v4l2-ctrls.h>
>>> +#include <media/v4l2-event.h>
>>> +#include <media/v4l2-subdev.h>
>>> +
>>> +#include "vimc-common.h"
>>> +
>>> +/*
>>> + * Flash timeout in ms
>>> + */
>>> +#define VIMC_FLASH_TIMEOUT_MS_MIN 50
>>> +#define VIMC_FLASH_TIMEOUT_MS_MAX 400
>>> +#define VIMC_FLASH_TIMEOUT_MS_STEP 50
>>> +
>>> +/*
>>> + * Torch intencity in uA
>>> + */
>>> +#define VIMC_FLASH_TORCH_UA_MIN 2530
>>> +#define VIMC_FLASH_TORCH_UA_MAX 187100
>>> +#define VIMC_FLASH_TORCH_UA_STEP 1460
>>> +
>>> +/*
>>> + * Flash intencity in uA
>>> + */
>>> +#define VIMC_FLASH_FLASH_UA_MIN 23040
>>> +#define VIMC_FLASH_FLASH_UA_MAX 1499600
>>> +#define VIMC_FLASH_FLASH_UA_STEP 11718
>>> +
>>> +struct vimc_fla_device {
>>> +     struct vimc_ent_device ved;
>>> +     struct v4l2_subdev sd;
>>> +     struct v4l2_ctrl_handler hdl;
>>> +     int strobe_source;
>>> +     bool is_strobe;
>>> +     int led_mode;
>>> +     int indicator_intensity;
>>> +     int torch_intensity;
>>> +     int flash_intensity;
>>> +     u64 timeout;
>>> +     u64 last_strobe;
>>> +     struct workqueue_struct *wq;
>>> +     struct work_struct work;
>>> +     struct v4l2_ctrl *strobe_status_ctl;
>>> +};
>>> +
>>> +static void vimc_fla_strobe_work(struct work_struct *work)
>>> +{
>>> +     struct vimc_fla_device *vfla =
>>> +             container_of(work, struct vimc_fla_device, work);
>>> +     v4l2_ctrl_s_ctrl(vfla->strobe_status_ctl, true);
>>> +     vfla->last_strobe = ktime_get_ns();
>>> +     while (vfla->is_strobe &&
>>> +            vfla->last_strobe + vfla->timeout > ktime_get_ns()) {
>>> +             msleep_interruptible(VIMC_FLASH_TIMEOUT_MS_STEP);
>>> +     }
>>> +     v4l2_ctrl_s_ctrl(vfla->strobe_status_ctl, false);
>>> +}
>>> +
>>> +static int vimc_fla_s_ctrl(struct v4l2_ctrl *c)
>>> +{
>>> +     struct vimc_fla_device *vfla =
>>> +             container_of(c->handler, struct vimc_fla_device, hdl);
>>> +
>>> +     switch (c->id) {
>>> +     case V4L2_CID_FLASH_LED_MODE:
>>> +             vfla->led_mode = c->val;
>>> +             return 0;
>>> +     case V4L2_CID_FLASH_STROBE_SOURCE:
>>> +             vfla->strobe_source = c->val;
>>> +             return 0;
>>> +     case V4L2_CID_FLASH_STROBE:
>>> +             if (vfla->led_mode != V4L2_FLASH_LED_MODE_FLASH ||
>>> +                 vfla->strobe_source != V4L2_FLASH_STROBE_SOURCE_SOFTWARE){
>> you can add error/warning debug here,
>> if you choose not to, then the parentheses are redundant.
>> Additionally, the opening '{' should come after a space
>> You can run srctipts/checkpatch.pl before submitting to catch such issues
>>
>>> +                     return -EINVAL;
>>> +             }
>>> +             queue_work(vfla->wq, &vfla->work);
>>> +             return 0;
>>> +     case V4L2_CID_FLASH_STROBE_STATUS:
>>> +             vfla->is_strobe = c->val;
>>> +             return 0;
>>> +     case V4L2_CID_FLASH_STROBE_STOP:
>>> +             vfla->is_strobe = false;
>>> +             return 0;
>>> +     case V4L2_CID_FLASH_TIMEOUT:
>>> +             vfla->timeout = c->val * 1000000; /* MS to NS */
>>> +             return 0;
>>> +     case V4L2_CID_FLASH_INTENSITY:
>>> +             vfla->flash_intensity = c->val;
>>> +             return 0;
>>> +     case V4L2_CID_FLASH_TORCH_INTENSITY:
>>> +             vfla->torch_intensity = c->val;
>>> +             return 0;
>>> +     case V4L2_CID_FLASH_INDICATOR_INTENSITY:
>>> +             vfla->indicator_intensity = c->val;
>>> +             return 0;
>>> +     default:
>>> +             return -EINVAL;
>>> +     }
>>> +     return 0;
>>> +}
>>> +
>>> +static const struct v4l2_ctrl_ops vimc_fla_ctrl_ops = {
>>> +     .s_ctrl = vimc_fla_s_ctrl,
>>> +};
>>> +
>>> +static const struct v4l2_subdev_core_ops vimc_fla_core_ops = {
>>> +     .subscribe_event = v4l2_ctrl_subdev_subscribe_event,
>>> +     .unsubscribe_event = v4l2_event_subdev_unsubscribe,
>>> +};
>>> +
>>> +static const struct v4l2_subdev_ops vimc_fla_ops = {
>>> +     .core = &vimc_fla_core_ops,
>>> +};
>>> +
>>> +static void vimc_fla_release(struct v4l2_subdev *sd)
>>> +{
>>> +     struct vimc_fla_device *vfla =
>>> +                             container_of(sd, struct vimc_fla_device, sd);
>> one tab is enough
>>> +
>>> +     v4l2_ctrl_handler_free(&vfla->hdl);
>>> +     kfree(vfla);
>>> +}
>>> +
>>> +static const struct v4l2_subdev_internal_ops vimc_fla_int_ops = {
>>> +     .release = vimc_fla_release,
>>> +};
>>> +
>>> +/* initialize device */
>>> +struct vimc_ent_device *vimc_fla_add(struct vimc_device *vimc,
>>> +                                  const char *vcfg_name)
>>> +{
>>> +     struct v4l2_device *v4l2_dev = &vimc->v4l2_dev;
>>> +     struct vimc_fla_device *vfla;
>>> +     int ret;
>>> +
>>> +     /* Allocate the vfla struct */
>>> +     vfla = kzalloc(sizeof(*vfla), GFP_KERNEL);
>>> +     if (!vfla)
>>> +             return NULL;
>> I think it is better to change the return values of the .add calbbacks
>> to return ERR_PTR upon error and not just NULL so that different types of
>> errors can be distinguished.
>> (This is not related specifically to this patch),
>> If you want you can send a patch fixing that.
>>
> 
> Sure. I will change it.
cool, thanks
> 
>>> +
>>> +     v4l2_ctrl_handler_init(&vfla->hdl, 4);
>>> +     v4l2_ctrl_new_std_menu(&vfla->hdl, &vimc_fla_ctrl_ops,
>>> +                            V4L2_CID_FLASH_LED_MODE,
>>> +                            V4L2_FLASH_LED_MODE_TORCH, ~0x7,
>>> +                            V4L2_FLASH_LED_MODE_FLASH);
>>> +     v4l2_ctrl_new_std_menu(&vfla->hdl, &vimc_fla_ctrl_ops,
>>> +                            V4L2_CID_FLASH_STROBE_SOURCE, 0x1, ~0x3,
>>> +                            V4L2_FLASH_STROBE_SOURCE_SOFTWARE);
>>> +     v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
>>> +                       V4L2_CID_FLASH_STROBE, 0, 0, 0, 0);
>>> +     v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
>>> +                       V4L2_CID_FLASH_STROBE_STOP, 0, 0, 0, 0);
>>> +     v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
>>> +                       V4L2_CID_FLASH_TIMEOUT, VIMC_FLASH_TIMEOUT_MS_MIN,
>>> +                       VIMC_FLASH_TIMEOUT_MS_MAX,
>>> +                       VIMC_FLASH_TIMEOUT_MS_STEP,
>>> +                       VIMC_FLASH_TIMEOUT_MS_MIN);
>>> +     v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
>>> +                       V4L2_CID_FLASH_TORCH_INTENSITY,
>>> +                       VIMC_FLASH_TORCH_UA_MIN,
>>> +                       VIMC_FLASH_TORCH_UA_MAX,
>>> +                       VIMC_FLASH_TORCH_UA_STEP,
>>> +                       VIMC_FLASH_TORCH_UA_MIN);
>>> +     v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
>>> +                       V4L2_CID_FLASH_INTENSITY,
>>> +                       VIMC_FLASH_FLASH_UA_MIN,
>>> +                       VIMC_FLASH_FLASH_UA_MAX,
>>> +                       VIMC_FLASH_FLASH_UA_STEP,
>>> +                       VIMC_FLASH_FLASH_UA_MIN);
>>> +     v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
>>> +                       V4L2_CID_FLASH_INDICATOR_INTENSITY,
>>> +                       0,
>>> +                       255,
>>> +                       1,
>>> +                       0);
>> why not having one line with "0,255,1,0);" ?
>>
>>> +     v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
>>> +                       V4L2_CID_FLASH_STROBE_STATUS, 0, 1, 1, 0);
>>> +     v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
>>> +                       V4L2_CID_FLASH_FAULT, 0,
>>> +                       V4L2_FLASH_FAULT_TIMEOUT, 0, 0);
>>> +     vfla->sd.ctrl_handler = &vfla->hdl;
>>> +     if (vfla->hdl.error) {
>>> +             ret = vfla->hdl.error;
>>> +             goto err_free_vfla;
>>> +     }
>>> +     vfla->strobe_status_ctl = v4l2_ctrl_find(&vfla->hdl,
>>> +                                              V4L2_CID_FLASH_STROBE_STATUS);
>>> +
>>> +     /* Initialize ved and sd */
>>> +     ret = vimc_ent_sd_register(&vfla->ved, &vfla->sd, v4l2_dev,
>>> +                                vcfg_name,
>>> +                                MEDIA_ENT_F_FLASH, 0, NULL,
>>> +                                &vimc_fla_int_ops, &vimc_fla_ops);
>>> +     if (ret)
>>> +             goto err_free_hdl;
>>> +
>>> +     /* Create processing workqueue */
>>> +     vfla->wq = alloc_workqueue("%s", 0, 0, "vimc-flash thread");
>>> +     if (!vfla->wq)
>>> +             goto err_unregister;
>>> +
>>> +     INIT_WORK(&vfla->work, vimc_fla_strobe_work);
>>> +     /* Initialize standard values */
>>> +     vfla->indicator_intensity = 0;
>>> +     vfla->torch_intensity = 0;
>>> +     vfla->flash_intensity = 0;
>>> +     vfla->is_strobe = false;
>>> +     vfla->timeout = 0;
>>> +     vfla->last_strobe = 0;
>>> +     vfla->strobe_source = V4L2_FLASH_STROBE_SOURCE_SOFTWARE;
>>> +     vfla->led_mode = V4L2_FLASH_LED_MODE_FLASH;
>>> +
>>> +     return &vfla->ved;
>>> +
>>> +err_unregister:
>>> +     vimc_ent_sd_unregister(&vfla->ved, &vfla->sd);
>>> +err_free_hdl:
>>> +     v4l2_ctrl_handler_free(&vfla->hdl);
>>> +err_free_vfla:
>>> +     kfree(vfla);
>>> +
>>> +     return NULL;
>>> +}
>>> +
>>> +void vimc_fla_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
>>> +{
>>> +     struct vimc_fla_device *vfla;
>>> +
>>> +     if (!ved)
>>> +             return;
>>> +
>>> +     vfla = container_of(ved, struct vimc_fla_device, ved);
>>> +     destroy_workqueue(vfla->wq);
>>> +     vimc_ent_sd_unregister(ved, &vfla->sd);
>>> +}
>>>
>> Not sure but I think there are indentation issues in this patch, a tab should be 8 spaces
> 
> This is strange. I already had style issues on the v2 so I runned the
> checkpatch.pl on the
> patch before sent and it didn't point this identation issues.
> 
> Here is the checkpatch.pl output. It could be the case that my smtp is
> alterating the
> it before transmission.
Sorry, as I wrote, this is my thunderbird client

Dafna

> 
> $ scripts/checkpatch.pl
> patchset/0001-media-vimc-fla-Add-virtual-flash-subdevice.patch
> WARNING: Possible unwrapped commit description (prefer a maximum 75
> chars per line)
> #14:
>                         led_mode 0x009c0901 (menu)   : min=0 max=2
> default=1 value=1
> 
> WARNING: Non-standard signature: Co-authored-by:
> #30:
> Co-authored-by: Eduardo Barretto <edusbarretto@gmail.com>
> 
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #131:
> new file mode 100644
> 
> total: 0 errors, 3 warnings, 284 lines checked
> 
> NOTE: For some of the reported defects, checkpatch may be able to
>        mechanically convert to the typical style using --fix or --fix-inplace.
> 
> patchset/0001-media-vimc-fla-Add-virtual-flash-subdevice.patch has
> style problems, please review.
> 
> NOTE: If any of the errors are false positives, please report
>        them to the maintainer, see CHECKPATCH in MAINTAINERS.
> 
> 
> Thanks,
> Lucas
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-03 23:01 [PATCH v3 RESEND] media: vimc: fla: Add virtual flash subdevice Lucas A. M. Magalhães
2019-12-04 10:03 ` Dafna Hirschfeld
2019-12-04 23:12   ` Lucas Magalhães
2019-12-05 18:01     ` Dafna Hirschfeld

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git