lkml.org 
[lkml]   [2016]   [Jun]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] userspace API definitions for auto-focus coil
    Hi Pavel,

    On Mon, Jun 13, 2016 at 09:17:53PM +0200, Pavel Machek wrote:
    > On Sun 2016-06-12 14:22:53, Sakari Ailus wrote:
    > > Hi Pavel,
    > >
    > > On Sun, Jun 12, 2016 at 10:48:11AM +0200, Pavel Machek wrote:
    > > > Hi!
    > > >
    > > > > > >Add userspace API definitions.
    > > > > > >
    > > > > > >Signed-off-by: Pavel Machek <pavel@ucw.cz>
    > > > > > >
    > > > > > >diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
    > > > > > >index b6a357a..23011cc 100644
    > > > > > >--- a/include/uapi/linux/v4l2-controls.h
    > > > > > >+++ b/include/uapi/linux/v4l2-controls.h
    > > > > > >@@ -974,4 +975,9 @@ enum v4l2_detect_md_mode {
    > > > > > > #define V4L2_CID_DETECT_MD_THRESHOLD_GRID (V4L2_CID_DETECT_CLASS_BASE + 3)
    > > > > > > #define V4L2_CID_DETECT_MD_REGION_GRID (V4L2_CID_DETECT_CLASS_BASE + 4)
    > > > > > >
    > > > > > >+/* Control IDs specific to the AD5820 driver as defined by V4L2 */
    > > > > > >+#define V4L2_CID_FOCUS_AD5820_BASE (V4L2_CTRL_CLASS_CAMERA | 0x10af)
    > > > >
    > > > > Please check V4L2_CID_USER_*_BASE. That's how custom controls are handled
    > > > > nowadays.
    > > >
    > > > So something like this?
    > > >
    > > > Thanks,
    > > > Pavel
    > > >
    > > > diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c
    > > > index 2efa5dc1..b04b471 100644
    > > > --- a/drivers/media/i2c/ad5820.c
    > > > +++ b/drivers/media/i2c/ad5820.c
    > > > @@ -40,6 +40,11 @@
    > > > #define AD5820_RAMP_MODE_LINEAR (0 << 3)
    > > > #define AD5820_RAMP_MODE_64_16 (1 << 3)
    > > >
    > > > +/* Control IDs specific to the AD5820 driver as defined by V4L2 */
    > > > +#define V4L2_CID_FOCUS_AD5820_RAMP_TIME (V4L2_CID_USER_AD5820_BASE+0)
    > > > +#define V4L2_CID_FOCUS_AD5820_RAMP_MODE (V4L2_CID_FOCUS_AD5820_BASE+1)
    > > > +
    > > > +
    > >
    > > We could still define these in a header file that can be included by the
    > > user space. Please use V4L2_CID_AD5820 prefix.
    >
    > Not V4L2_CID_USER_AD5820...?

    The rest of the controls have no USER as part of the macro name, so I
    wouldn't use it here either.

    >
    > > A separate header file should be used,
    > > e.g. include/uapi/linux/ad5820.h.
    >
    > Ok, separate header file for 2 lines seemed like a bit of overkill,
    > but why not.

    That follows an existing pattern of how controls have been implemented in
    other drivers.

    >
    > Something like this?

    Yes. A few minor comments below.

    Could you merge this with the driver patch? I've dropped that from my ad5820
    branch as it does not compile.

    >
    > commit 8dd701d2580e41b06bb2285e6bd59a4f1702b4d8
    > Author: Pavel <pavel@ucw.cz>
    > Date: Mon Jun 13 21:17:15 2016 +0200
    >
    > Userspace api, as Sakari asked.
    >
    > diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c
    > index 2efa5dc1..ed3facc 100644
    > --- a/drivers/media/i2c/ad5820.c
    > +++ b/drivers/media/i2c/ad5820.c
    > @@ -32,6 +32,8 @@
    > #include <media/v4l2-device.h>
    > #include <media/v4l2-subdev.h>
    >
    > +#include <uapi/linux/ad5820.h>
    > +
    > #define AD5820_NAME "ad5820"
    >
    > /* Register definitions */
    > @@ -40,6 +42,8 @@
    > #define AD5820_RAMP_MODE_LINEAR (0 << 3)
    > #define AD5820_RAMP_MODE_64_16 (1 << 3)
    >
    > +
    > +

    Extra newlines.

    > #define CODE_TO_RAMP_US(s) ((s) == 0 ? 0 : (1 << ((s) - 1)) * 50)
    > #define RAMP_US_TO_CODE(c) fls(((c) + ((c)>>1)) / 50)
    >
    > @@ -165,13 +169,13 @@ static int ad5820_set_ctrl(struct v4l2_ctrl *ctrl)
    > coil->focus_absolute = ctrl->val;
    > return ad5820_update_hw(coil);
    >
    > - case V4L2_CID_FOCUS_AD5820_RAMP_TIME:
    > + case V4L2_CID_AD5820_RAMP_TIME:
    > code = RAMP_US_TO_CODE(ctrl->val);
    > ctrl->val = CODE_TO_RAMP_US(code);
    > coil->focus_ramp_time = ctrl->val;
    > break;
    >
    > - case V4L2_CID_FOCUS_AD5820_RAMP_MODE:
    > + case V4L2_CID_AD5820_RAMP_MODE:
    > coil->focus_ramp_mode = ctrl->val;
    > break;
    > }
    > @@ -191,7 +195,7 @@ static const char * const ad5820_focus_menu[] = {
    > static const struct v4l2_ctrl_config ad5820_ctrls[] = {
    > {
    > .ops = &ad5820_ctrl_ops,
    > - .id = V4L2_CID_FOCUS_AD5820_RAMP_TIME,
    > + .id = V4L2_CID_AD5820_RAMP_TIME,
    > .type = V4L2_CTRL_TYPE_INTEGER,
    > .name = "Focus ramping time [us]",
    > .min = 0,
    > @@ -202,7 +206,7 @@ static const struct v4l2_ctrl_config ad5820_ctrls[] = {
    > },
    > {
    > .ops = &ad5820_ctrl_ops,
    > - .id = V4L2_CID_FOCUS_AD5820_RAMP_MODE,
    > + .id = V4L2_CID_AD5820_RAMP_MODE,
    > .type = V4L2_CTRL_TYPE_MENU,
    > .name = "Focus ramping mode",
    > .min = 0,
    > diff --git a/include/uapi/linux/ad5820.h b/include/uapi/linux/ad5820.h
    > new file mode 100644
    > index 0000000..d44ba9c
    > --- /dev/null
    > +++ b/include/uapi/linux/ad5820.h
    > @@ -0,0 +1,18 @@
    > +/*
    > + * AD5820 DAC driver for camera voice coil focus.
    > + *
    > + * This program is free software; you can redistribute it and/or
    > + * modify it under the terms of the GNU General Public License
    > + * version 2 as published by the Free Software Foundation.
    > + */
    > +
    > +#ifndef __LINUX_AD5820_H
    > +#define __LINUX_AD5820_H
    > +
    > +#include <linux/v4l2-controls.h>
    > +
    > +/* Control IDs specific to the AD5820 driver as defined by V4L2 */
    > +#define V4L2_CID_AD5820_RAMP_TIME (V4L2_CID_AD5820_BASE+0)

    (V4L2_CID_AD5820_BASE + 0)

    Same below.

    > +#define V4L2_CID_AD5820_RAMP_MODE (V4L2_CID_AD5820_BASE+1)
    > +
    > +#endif
    > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
    > index 23011cc..015e90b 100644
    > --- a/include/uapi/linux/v4l2-controls.h
    > +++ b/include/uapi/linux/v4l2-controls.h
    > @@ -181,6 +181,10 @@ enum v4l2_colorfx {
    > * We reserve 16 controls for this driver. */
    > #define V4L2_CID_USER_TC358743_BASE (V4L2_CID_USER_BASE + 0x1080)
    >
    > +/* The base for the ad5820 driver controls.
    > + * We reserve 16 controls for this driver. */
    > +#define V4L2_CID_AD5820_BASE (V4L2_CID_USER_BASE + 0x1090)
    > +
    > /* MPEG-class control IDs */
    > /* The MPEG controls are applicable to all codec controls
    > * and the 'MPEG' part of the define is historical */
    > @@ -986,9 +990,4 @@ enum v4l2_detect_md_mode {
    > #define V4L2_CID_MODE_SENSITIVITY (V4L2_CID_MODE_CLASS_BASE+6)
    > #define V4L2_CID_MODE_OPSYSCLOCK (V4L2_CID_MODE_CLASS_BASE+7)
    >
    > -/* Control IDs specific to the AD5820 driver as defined by V4L2 */
    > -#define V4L2_CID_FOCUS_AD5820_BASE (V4L2_CTRL_CLASS_CAMERA | 0x10af)
    > -#define V4L2_CID_FOCUS_AD5820_RAMP_TIME (V4L2_CID_FOCUS_AD5820_BASE+0)
    > -#define V4L2_CID_FOCUS_AD5820_RAMP_MODE (V4L2_CID_FOCUS_AD5820_BASE+1)
    > -
    > #endif
    >
    >

    --
    Regards,

    Sakari Ailus
    e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk

    \
     
     \ /
      Last update: 2016-06-18 00:21    [W:4.196 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site