* [PATCH v2] media: vimc: fla: Add virtual flash subdevice
@ 2019-09-15 18:44 Lucas A. M. Magalhães
2019-09-18 2:08 ` [Lkcamp] " Guilherme Alcarde Gallo
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Lucas A. M. Magalhães @ 2019-09-15 18:44 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=0 value=0
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=1 max=10 step=1 default=10 value=10
intensity_flash_mode 0x009c0907 (int) : min=0 max=255 step=1 default=255 value=255
intensity_torch_mode 0x009c0908 (int) : min=0 max=255 step=1 default=255 value=255
intensity_indicator 0x009c0909 (int) : min=0 max=255 step=1 default=255 value=255
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,
This patch depends on the patch series
"Collapse vimc into single monolithic driver" version 4.
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
- Changes to apply over v4 of patch
"Collapse vimc into single monolithic driver"
---
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 | 200 ++++++++++++++++++++++
5 files changed, 212 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 236412ad7548..a1fbbc8066d3 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 a1218578cb9a..312723b4ed8a 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..637e7d0a5919
--- /dev/null
+++ b/drivers/media/platform/vimc/vimc-flash.c
@@ -0,0 +1,200 @@
+// 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/kthread.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"
+
+#define VIMC_FLASH_TIMEOUT_STEP 10000
+#define VIMC_FLASH_TIMEOUT_MAX 50000000
+
+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 task_struct *kthread;
+};
+
+void vimc_fla_set_strobe_status(struct v4l2_ctrl_handler *hdl, bool value){
+ struct v4l2_ctrl *c;
+ c = v4l2_ctrl_find(hdl, V4L2_CID_FLASH_STROBE_STATUS);
+ if (!c) return;
+ v4l2_ctrl_s_ctrl(c, value);
+
+}
+
+static int vimc_fla_strobe_thread(void *data)
+{
+ struct vimc_fla_device *vfla = data;
+ vimc_fla_set_strobe_status(&vfla->hdl, vfla->is_strobe);
+ vfla->last_strobe = ktime_get_ns();
+ while(vfla->is_strobe &&
+ vfla->last_strobe + vfla->timeout > ktime_get_ns()){
+ msleep_interruptible(VIMC_FLASH_TIMEOUT_STEP/1000);
+ }
+ vimc_fla_set_strobe_status(&vfla->hdl, false);
+ return 0;
+}
+
+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 -EILSEQ;
+ }
+ vfla->is_strobe = true;
+ vfla->kthread = kthread_run(vimc_fla_strobe_thread, vfla, "vimc-flash thread");
+ 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;
+ 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;
+ }
+ 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,
+};
+
+/* 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_NONE);
+ 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, 0,
+ VIMC_FLASH_TIMEOUT_MAX,
+ VIMC_FLASH_TIMEOUT_STEP,
+ VIMC_FLASH_TIMEOUT_STEP);
+ v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
+ V4L2_CID_FLASH_TORCH_INTENSITY, 0, 255, 1, 255);
+ v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
+ V4L2_CID_FLASH_INTENSITY, 0, 255, 1, 255);
+ v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
+ V4L2_CID_FLASH_INDICATOR_INTENSITY, 0, 255, 1, 255);
+ 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;
+ }
+
+ /* Initialize ved and sd */
+ ret = vimc_ent_sd_register(&vfla->ved, &vfla->sd, v4l2_dev,
+ vcfg_name,
+ MEDIA_ENT_F_FLASH, 0, NULL,
+ NULL, &vimc_fla_ops);
+ if (ret)
+ goto err_free_hdl;
+
+ /* 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->led_mode = V4L2_FLASH_LED_MODE_NONE;
+
+ return &vfla->ved;
+
+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);
+ vimc_ent_sd_unregister(ved, &vfla->sd);
+}
--
2.23.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Lkcamp] [PATCH v2] media: vimc: fla: Add virtual flash subdevice
2019-09-15 18:44 [PATCH v2] media: vimc: fla: Add virtual flash subdevice Lucas A. M. Magalhães
@ 2019-09-18 2:08 ` Guilherme Alcarde Gallo
2019-09-20 11:32 ` Hans Verkuil
2019-09-24 15:34 ` Dafna Hirschfeld
2 siblings, 0 replies; 5+ messages in thread
From: Guilherme Alcarde Gallo @ 2019-09-18 2:08 UTC (permalink / raw)
To: Lucas A. M. Magalhães
Cc: linux-media, linux-kernel, Hans Verkuil, Helen Koike, lkcamp
Hi Lucas!
How are you?
I just have one comment about the strobe thread. Please check it below.
On Sun, Sep 15, 2019 at 3:44 PM Lucas A. M. Magalhães <lucmaga@gmail.com> 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=0 value=0
> 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=1 max=10 step=1 default=10 value=10
> intensity_flash_mode 0x009c0907 (int) : min=0 max=255 step=1 default=255 value=255
> intensity_torch_mode 0x009c0908 (int) : min=0 max=255 step=1 default=255 value=255
> intensity_indicator 0x009c0909 (int) : min=0 max=255 step=1 default=255 value=255
> 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,
>
> This patch depends on the patch series
> "Collapse vimc into single monolithic driver" version 4.
>
> 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
> - Changes to apply over v4 of patch
> "Collapse vimc into single monolithic driver"
> ---
> 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 | 200 ++++++++++++++++++++++
> 5 files changed, 212 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 236412ad7548..a1fbbc8066d3 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 a1218578cb9a..312723b4ed8a 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..637e7d0a5919
> --- /dev/null
> +++ b/drivers/media/platform/vimc/vimc-flash.c
> @@ -0,0 +1,200 @@
> +// 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/kthread.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"
> +
> +#define VIMC_FLASH_TIMEOUT_STEP 10000
> +#define VIMC_FLASH_TIMEOUT_MAX 50000000
> +
> +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 task_struct *kthread;
> +};
> +
> +void vimc_fla_set_strobe_status(struct v4l2_ctrl_handler *hdl, bool value){
> + struct v4l2_ctrl *c;
> + c = v4l2_ctrl_find(hdl, V4L2_CID_FLASH_STROBE_STATUS);
> + if (!c) return;
> + v4l2_ctrl_s_ctrl(c, value);
> +
> +}
> +
> +static int vimc_fla_strobe_thread(void *data)
> +{
> + struct vimc_fla_device *vfla = data;
> + vimc_fla_set_strobe_status(&vfla->hdl, vfla->is_strobe);
This code runs in another thread, but, at this point, the resource
pointed by *data
may be freed just after vimc_fla_strobe_thread is being spawned.
Maybe you should protect this critical region during this thread execution.
> + vfla->last_strobe = ktime_get_ns();
> + while(vfla->is_strobe &&
> + vfla->last_strobe + vfla->timeout > ktime_get_ns()){
> + msleep_interruptible(VIMC_FLASH_TIMEOUT_STEP/1000);
> + }
> + vimc_fla_set_strobe_status(&vfla->hdl, false);
> + return 0;
> +}
> +
> +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 -EILSEQ;
> + }
> + vfla->is_strobe = true;
> + vfla->kthread = kthread_run(vimc_fla_strobe_thread, vfla, "vimc-flash thread");
> + 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;
> + 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;
> + }
> + 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,
> +};
> +
> +/* 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_NONE);
> + 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, 0,
> + VIMC_FLASH_TIMEOUT_MAX,
> + VIMC_FLASH_TIMEOUT_STEP,
> + VIMC_FLASH_TIMEOUT_STEP);
> + v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> + V4L2_CID_FLASH_TORCH_INTENSITY, 0, 255, 1, 255);
> + v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> + V4L2_CID_FLASH_INTENSITY, 0, 255, 1, 255);
> + v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> + V4L2_CID_FLASH_INDICATOR_INTENSITY, 0, 255, 1, 255);
> + 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;
> + }
> +
> + /* Initialize ved and sd */
> + ret = vimc_ent_sd_register(&vfla->ved, &vfla->sd, v4l2_dev,
> + vcfg_name,
> + MEDIA_ENT_F_FLASH, 0, NULL,
> + NULL, &vimc_fla_ops);
> + if (ret)
> + goto err_free_hdl;
> +
> + /* 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->led_mode = V4L2_FLASH_LED_MODE_NONE;
> +
> + return &vfla->ved;
> +
> +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);
> + vimc_ent_sd_unregister(ved, &vfla->sd);
> +}
> --
> 2.23.0
>
>
> _______________________________________________
> Lkcamp mailing list
> Lkcamp@lists.libreplanetbr.org
> https://lists.libreplanetbr.org/mailman/listinfo/lkcamp
Regards,
Guilherme
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] media: vimc: fla: Add virtual flash subdevice
2019-09-15 18:44 [PATCH v2] media: vimc: fla: Add virtual flash subdevice Lucas A. M. Magalhães
2019-09-18 2:08 ` [Lkcamp] " Guilherme Alcarde Gallo
@ 2019-09-20 11:32 ` Hans Verkuil
2019-09-28 17:02 ` Lucas Magalhães
2019-09-24 15:34 ` Dafna Hirschfeld
2 siblings, 1 reply; 5+ messages in thread
From: Hans Verkuil @ 2019-09-20 11:32 UTC (permalink / raw)
To: Lucas A. M. Magalhães, linux-media
Cc: linux-kernel, helen.koike, edusbarretto, lkcamp
On 9/15/19 8:44 PM, 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=0 value=0
> 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=1 max=10 step=1 default=10 value=10
> intensity_flash_mode 0x009c0907 (int) : min=0 max=255 step=1 default=255 value=255
> intensity_torch_mode 0x009c0908 (int) : min=0 max=255 step=1 default=255 value=255
> intensity_indicator 0x009c0909 (int) : min=0 max=255 step=1 default=255 value=255
> 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,
>
> This patch depends on the patch series
> "Collapse vimc into single monolithic driver" version 4.
>
> 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
> - Changes to apply over v4 of patch
> "Collapse vimc into single monolithic driver"
> ---
> 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 | 200 ++++++++++++++++++++++
> 5 files changed, 212 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 236412ad7548..a1fbbc8066d3 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 a1218578cb9a..312723b4ed8a 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..637e7d0a5919
> --- /dev/null
> +++ b/drivers/media/platform/vimc/vimc-flash.c
> @@ -0,0 +1,200 @@
> +// 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/kthread.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"
> +
> +#define VIMC_FLASH_TIMEOUT_STEP 10000
> +#define VIMC_FLASH_TIMEOUT_MAX 50000000
What's the time unit for these defines? Just make that part of the name,
e.g. VIMC_FLASH_TIMEOUT_MS_STEP (or NS or US, whatever the unit is).
> +
> +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 task_struct *kthread;
> +};
> +
> +void vimc_fla_set_strobe_status(struct v4l2_ctrl_handler *hdl, bool value){
Please use proper coding style, so { should be on a new line...
> + struct v4l2_ctrl *c;
...and add an empty line between variable declarations and the following code.
> + c = v4l2_ctrl_find(hdl, V4L2_CID_FLASH_STROBE_STATUS);
> + if (!c) return;
'return' should be on the next line.
> + v4l2_ctrl_s_ctrl(c, value);
> +
no spurious empty line at the end.
> +}
How about this:
static void vimc_fla_set_strobe_status(struct v4l2_ctrl_handler *hdl, bool value)
{
struct v4l2_ctrl *c = v4l2_ctrl_find(hdl, V4L2_CID_FLASH_STROBE_STATUS);
if (c)
v4l2_ctrl_s_ctrl(c, value);
}
Even better: just store the V4L2_CID_FLASH_STROBE_STATUS v4l2_ctrl pointer in
struct vimc_fla_device when the control is created and use that pointer directly.
> +
> +static int vimc_fla_strobe_thread(void *data)
> +{
> + struct vimc_fla_device *vfla = data;
empty line
> + vimc_fla_set_strobe_status(&vfla->hdl, vfla->is_strobe);
Hmm, this really boils down to a simple:
v4l2_ctrl_s_ctrl(vfla->strobe_status_ctrl, 1);
So you don't need the vimc_fla_set_strobe_status at all.
> + vfla->last_strobe = ktime_get_ns();
> + while(vfla->is_strobe &&
space after 'while'.
Please run 'checkpatch.pl --strict' over your patch!
> + vfla->last_strobe + vfla->timeout > ktime_get_ns()){
> + msleep_interruptible(VIMC_FLASH_TIMEOUT_STEP/1000);
> + }
> + vimc_fla_set_strobe_status(&vfla->hdl, false);
> + return 0;
> +}
> +
> +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 -EILSEQ;
> + }
> + vfla->is_strobe = true;
> + vfla->kthread = kthread_run(vimc_fla_strobe_thread, vfla, "vimc-flash thread");
What if the thread is already running?
I wonder what existing flash drivers do if V4L2_CID_FLASH_STROBE is called
repeatedly. Perhaps returning EBUSY if strobe is still active makes sense here.
It would also be a nice feature if keeping the strobe on for more than X seconds
would create a V4L2_FLASH_FAULT_LED_OVER_TEMPERATURE fault.
> + 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;
> + 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;
> + }
> + 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,
> +};
> +
> +/* 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_NONE);
> + 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, 0,
> + VIMC_FLASH_TIMEOUT_MAX,
> + VIMC_FLASH_TIMEOUT_STEP,
> + VIMC_FLASH_TIMEOUT_STEP);
> + v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> + V4L2_CID_FLASH_TORCH_INTENSITY, 0, 255, 1, 255);
> + v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> + V4L2_CID_FLASH_INTENSITY, 0, 255, 1, 255);
> + v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> + V4L2_CID_FLASH_INDICATOR_INTENSITY, 0, 255, 1, 255);
Can you look at existing flash drivers and copy the min/max/step/def values?
The values here are rather arbitrary. It would be nice if it was a bit more
realistic.
> + 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;
> + }
> +
> + /* Initialize ved and sd */
> + ret = vimc_ent_sd_register(&vfla->ved, &vfla->sd, v4l2_dev,
> + vcfg_name,
> + MEDIA_ENT_F_FLASH, 0, NULL,
> + NULL, &vimc_fla_ops);
> + if (ret)
> + goto err_free_hdl;
> +
> + /* 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->led_mode = V4L2_FLASH_LED_MODE_NONE;
> +
> + return &vfla->ved;
> +
> +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);
> + vimc_ent_sd_unregister(ved, &vfla->sd);
> +}
>
Regards,
Hans
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] media: vimc: fla: Add virtual flash subdevice
2019-09-15 18:44 [PATCH v2] media: vimc: fla: Add virtual flash subdevice Lucas A. M. Magalhães
2019-09-18 2:08 ` [Lkcamp] " Guilherme Alcarde Gallo
2019-09-20 11:32 ` Hans Verkuil
@ 2019-09-24 15:34 ` Dafna Hirschfeld
2 siblings, 0 replies; 5+ messages in thread
From: Dafna Hirschfeld @ 2019-09-24 15:34 UTC (permalink / raw)
To: Lucas A. M. Magalhães, linux-media
Cc: hverkuil, linux-kernel, helen.koike, edusbarretto, lkcamp
Hi,
On Sun, 2019-09-15 at 15:44 -0300, 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=0 value=0
> 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=1 max=10 step=1 default=10 value=10
> intensity_flash_mode 0x009c0907 (int) : min=0 max=255 step=1 default=255 value=255
> intensity_torch_mode 0x009c0908 (int) : min=0 max=255 step=1 default=255 value=255
> intensity_indicator 0x009c0909 (int) : min=0 max=255 step=1 default=255 value=255
> 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,
>
> This patch depends on the patch series
> "Collapse vimc into single monolithic driver" version 4.
>
> 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
> - Changes to apply over v4 of patch
> "Collapse vimc into single monolithic driver"
> ---
> 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 | 200 ++++++++++++++++++++++
> 5 files changed, 212 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 236412ad7548..a1fbbc8066d3 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 a1218578cb9a..312723b4ed8a 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..637e7d0a5919
> --- /dev/null
> +++ b/drivers/media/platform/vimc/vimc-flash.c
> @@ -0,0 +1,200 @@
> +// 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/kthread.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"
> +
> +#define VIMC_FLASH_TIMEOUT_STEP 10000
> +#define VIMC_FLASH_TIMEOUT_MAX 50000000
> +
> +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 task_struct *kthread;
> +};
> +
> +void vimc_fla_set_strobe_status(struct v4l2_ctrl_handler *hdl, bool value){
> + struct v4l2_ctrl *c;
> + c = v4l2_ctrl_find(hdl, V4L2_CID_FLASH_STROBE_STATUS);
> + if (!c) return;
> + v4l2_ctrl_s_ctrl(c, value);
> +
> +}
> +
> +static int vimc_fla_strobe_thread(void *data)
> +{
> + struct vimc_fla_device *vfla = data;
> + vimc_fla_set_strobe_status(&vfla->hdl, vfla->is_strobe);
> + vfla->last_strobe = ktime_get_ns();
> + while(vfla->is_strobe &&
> + vfla->last_strobe + vfla->timeout > ktime_get_ns()){
> + msleep_interruptible(VIMC_FLASH_TIMEOUT_STEP/1000);
> + }
> + vimc_fla_set_strobe_status(&vfla->hdl, false);
> + return 0;
> +}
> +
> +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 -EILSEQ;
> + }
> + vfla->is_strobe = true;
> + vfla->kthread = kthread_run(vimc_fla_strobe_thread, vfla, "vimc-flash thread");
> + 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;
> + 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;
Should you add here a "default" case returning error?
> + }
> + 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,
> +};
> +
> +/* 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_NONE);
> + 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, 0,
> + VIMC_FLASH_TIMEOUT_MAX,
> + VIMC_FLASH_TIMEOUT_STEP,
> + VIMC_FLASH_TIMEOUT_STEP);
> + v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> + V4L2_CID_FLASH_TORCH_INTENSITY, 0, 255, 1, 255);
> + v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> + V4L2_CID_FLASH_INTENSITY, 0, 255, 1, 255);
> + v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> + V4L2_CID_FLASH_INDICATOR_INTENSITY, 0, 255, 1, 255);
> + 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;
> + }
> +
> + /* Initialize ved and sd */
> + ret = vimc_ent_sd_register(&vfla->ved, &vfla->sd, v4l2_dev,
> + vcfg_name,
> + MEDIA_ENT_F_FLASH, 0, NULL,
> + NULL, &vimc_fla_ops);
I think you are missing a v4l2_subdev_internal_ops* as the 8th argument
with a release callback that frees the ctrl handler, similar to vimc-sensor
Regards,
Dafna
> + if (ret)
> + goto err_free_hdl;
> +
> + /* 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->led_mode = V4L2_FLASH_LED_MODE_NONE;
> +
> + return &vfla->ved;
> +
> +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);
> + vimc_ent_sd_unregister(ved, &vfla->sd);
> +}
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] media: vimc: fla: Add virtual flash subdevice
2019-09-20 11:32 ` Hans Verkuil
@ 2019-09-28 17:02 ` Lucas Magalhães
0 siblings, 0 replies; 5+ messages in thread
From: Lucas Magalhães @ 2019-09-28 17:02 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, linux-kernel, Helen Koike, edusbarretto, lkcamp
Hi Hans,
Thanks for the review. Sorry about the style mistakes, will be careful
next time.
Just a couple of questions.
On Fri, Sep 20, 2019 at 8:32 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> > +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 -EILSEQ;
> > + }
> > + vfla->is_strobe = true;
> > + vfla->kthread = kthread_run(vimc_fla_strobe_thread, vfla, "vimc-flash thread");
>
> What if the thread is already running?
>
> I wonder what existing flash drivers do if V4L2_CID_FLASH_STROBE is called
> repeatedly. Perhaps returning EBUSY if strobe is still active makes sense here.
>
> It would also be a nice feature if keeping the strobe on for more than X seconds
> would create a V4L2_FLASH_FAULT_LED_OVER_TEMPERATURE fault.
>
How would you expect this? At this point I will never cross the maximum timeout
configured. I don't expect a driver to fail if I set a value within
the configuration
borders.
> > + 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_NONE);
> > + 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, 0,
> > + VIMC_FLASH_TIMEOUT_MAX,
> > + VIMC_FLASH_TIMEOUT_STEP,
> > + VIMC_FLASH_TIMEOUT_STEP);
> > + v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> > + V4L2_CID_FLASH_TORCH_INTENSITY, 0, 255, 1, 255);
> > + v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> > + V4L2_CID_FLASH_INTENSITY, 0, 255, 1, 255);
> > + v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,V4L2_CID_FLASH_INDICATOR_INTENSITY
> > + V4L2_CID_FLASH_INDICATOR_INTENSITY, 0, 255, 1, 255);
>
> Can you look at existing flash drivers and copy the min/max/step/def values?
>
> The values here are rather arbitrary. It would be nice if it was a bit more
> realistic.
I didn't found any driver implementing
V4L2_CID_FLASH_INDICATOR_INTENSITY. Do you have
any examples for this? For the other ones I'm copying the lm3646 for
the other ones.
Regards,
Lucas
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-09-28 17:02 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-15 18:44 [PATCH v2] media: vimc: fla: Add virtual flash subdevice Lucas A. M. Magalhães
2019-09-18 2:08 ` [Lkcamp] " Guilherme Alcarde Gallo
2019-09-20 11:32 ` Hans Verkuil
2019-09-28 17:02 ` Lucas Magalhães
2019-09-24 15:34 ` Dafna Hirschfeld
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).