Messages in this thread | | | Subject | Re: [PATCH v3 RESEND] media: vimc: fla: Add virtual flash subdevice | From | Dafna Hirschfeld <> | Date | Thu, 5 Dec 2019 19:01:37 +0100 |
| |
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 >
| |