userspace API definitions for auto-focus coil

  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...?

> 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.

Something like this?

commit 8dd701d2580e41b06bb2285e6bd59a4f1702b4d8
Author: Pavel <pavel@ucw.cz>
Date:   Mon Jun 13 21:17:15 2016 +0200

    Userspace api, as Sakari asked.
  

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
> 
>
  

Hi!

> > 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.

Ok.

> > 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.

Ok.

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

Yes, merged patch should be in your inbox now.

Thanks,
									Pavel
  

Em Sat, 18 Jun 2016 17:38:46 +0200
Pavel Machek <pavel@ucw.cz> escreveu:

> Hi!
> 
> > > 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.  
> 
> Ok.
> 
> > > 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.  
> 
> Ok.
> 
> > Could you merge this with the driver patch? I've dropped that from my ad5820
> > branch as it does not compile.  
> 
> Yes, merged patch should be in your inbox now.

The V4L2 core changes should be on a separate patch. Btw, you'll also
need to patch documentation to reflect such changes. We're right now
moving from DocBook to ReST markup language. The patches for it are
right now on a separate topic branch (docs-next), to be merged for
Kernel 4.8 on the next merge window.

You should either base the patch on such branch or wait for it to be
merged back mainstream to write such documentation additions.


> 
> Thanks,
> 									Pavel
>
  

On Tue 2016-07-12 20:32:01, Mauro Carvalho Chehab wrote:
1;2802;0c> Em Sat, 18 Jun 2016 17:38:46 +0200
> Pavel Machek <pavel@ucw.cz> escreveu:
> 
> > Hi!
> > 
> > > > 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.  
> > 
> > Ok.
> > 
> > > > 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.  
> > 
> > Ok.
> > 
> > > Could you merge this with the driver patch? I've dropped that from my ad5820
> > > branch as it does not compile.  
> > 
> > Yes, merged patch should be in your inbox now.
> 
> The V4L2 core changes should be on a separate patch. Btw, you'll also
> need to patch documentation to reflect such changes. We're right now
> moving from DocBook to ReST markup language. The patches for it are
> right now on a separate topic branch (docs-next), to be merged for
> Kernel 4.8 on the next merge window.
> 
> You should either base the patch on such branch or wait for it to be
> merged back mainstream to write such documentation additions.

So how many iterations and how many releases does it take to get
trivial driver into the tree?

I did what Sakari asked me to do. The driver is trivial. You can see
pretty easily what I'm changing in the core... and it is not much. Can
you just add your acked-by to it, merge it and me done with it?

If some more docs is required, I can do the docs, but stalling patch
for months and then claiming "hey, we have these new rules you have to
follow" is not a nice thing.

Thanks,

									Pavel
  

On Tue 2016-07-12 20:32:01, Mauro Carvalho Chehab wrote:
> Em Sat, 18 Jun 2016 17:38:46 +0200
> Pavel Machek <pavel@ucw.cz> escreveu:
> 
> > Hi!
> > 
> > > > 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.  
> > 
> > Ok.
> > 
> > > > 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.  
> > 
> > Ok.
> > 
> > > Could you merge this with the driver patch? I've dropped that from my ad5820
> > > branch as it does not compile.  
> > 
> > Yes, merged patch should be in your inbox now.
> 
> The V4L2 core changes should be on a separate patch. Btw, you'll also
> need to patch documentation to reflect such changes. We're right now
> moving from DocBook to ReST markup language. The patches for it are
> right now on a separate topic branch (docs-next), to be merged for
> Kernel 4.8 on the next merge window.

What about: I drop all the functionality but FOCUS_ABSOLUTE, which is
core functionality, anyway, and does not need core changes. When V4L2
core stabilizes, it can be reintroduced.

Best regards,
									Pavel