Re: [PATCH 04/24] media: v4l2-mediabus: convert flags to enums and document them

2017-12-18 Thread Mauro Carvalho Chehab 
Em Wed, 11 Oct 2017 23:26:44 +0200
Pavel Machek  escreveu:

> On Mon 2017-10-09 07:19:10, Mauro Carvalho Chehab wrote:
> > There is a mess with media bus flags: there are two sets of
> > flags, one used by parallel and ITU-R BT.656 outputs,
> > and another one for CSI2.
> > 
> > Depending on the type, the same bit has different meanings.
> >   
> 
> > @@ -86,11 +125,22 @@ enum v4l2_mbus_type {
> >  /**
> >   * struct v4l2_mbus_config - media bus configuration
> >   * @type:  in: interface type
> > - * @flags: in / out: configuration flags, depending on @type
> > + * @pb_flags:  in / out: configuration flags, if @type is
> > + * %V4L2_MBUS_PARALLEL or %V4L2_MBUS_BT656.
> > + * @csi2_flags:in / out: configuration flags, if @type is
> > + * %V4L2_MBUS_CSI2.
> > + * @flag:  access flags, no matter the @type.
> > + * Used just to avoid needing to rewrite the logic inside
> > + * soc_camera and pxa_camera drivers. Don't use on newer
> > + * drivers!
> >   */
> >  struct v4l2_mbus_config {
> > enum v4l2_mbus_type type;
> > -   unsigned int flags;
> > +   union {
> > +   enum v4l2_mbus_parallel_and_bt656_flags pb_flags;
> > +   enum v4l2_mbus_csi2_flags   csi2_flags;
> > +   unsigned intflag;
> > +   };
> >  };
> >  
> >  static inline void v4l2_fill_pix_format(struct v4l2_pix_format
> >  *pix_fmt,  
> 
> The flags->flag conversion is quite subtle, and "flag" is confusing
> because there is more than one inside. What about something like
> __legacy_flags?

Good idea.

> 
>   Pavel

Thanks,
Mauro

[PATCH] media: v4l2-mediabus: convert flags to enums and document them

There is a mess with media bus flags: there are two sets of
flags, one used by parallel and ITU-R BT.656 outputs,
and another one for CSI2.

Depending on the type, the same bit has different meanings.

That's very confusing, and counter-intuitive. So, split them
into two sets of flags, inside an enum.

This way, it becomes clearer that there are two separate sets
of flags. It also makes easier if CSI1, CSP, CSI3, etc. would
need a different set of flags.

As a side effect, enums can be documented via kernel-docs,
so there will be an improvement at flags documentation.

Unfortunately, soc_camera and pxa_camera do a mess with
the flags, using either one set of flags without proper
checks about the type. That could be fixed, but, as both drivers
are obsolete and in the process of cleanings, I opted to just
keep the behavior, using an unsigned int inside those two
drivers.

Acked-by: Hans Verkuil 
Signed-off-by: Mauro Carvalho Chehab 

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 25d24a3f10a7..4bf25a72ef4f 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -743,16 +743,16 @@ static int adv7180_g_mbus_config(struct v4l2_subdev *sd,
 
if (state->chip_info->flags & ADV7180_FLAG_MIPI_CSI2) {
cfg->type = V4L2_MBUS_CSI2;
-   cfg->flags = V4L2_MBUS_CSI2_1_LANE |
-   V4L2_MBUS_CSI2_CHANNEL_0 |
-   V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
+   cfg->csi2_flags = V4L2_MBUS_CSI2_1_LANE
+ | V4L2_MBUS_CSI2_CHANNEL_0
+ | V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
} else {
/*
 * The ADV7180 sensor supports BT.601/656 output modes.
 * The BT.656 is default and not yet configurable by s/w.
 */
-   cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING |
-V4L2_MBUS_DATA_ACTIVE_HIGH;
+   cfg->pb_flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING
+   | V4L2_MBUS_DATA_ACTIVE_HIGH;
cfg->type = V4L2_MBUS_BT656;
}
 
diff --git a/drivers/media/i2c/ml86v7667.c b/drivers/media/i2c/ml86v7667.c
index 57ef901edb06..a25114d0c31f 100644
--- a/drivers/media/i2c/ml86v7667.c
+++ b/drivers/media/i2c/ml86v7667.c
@@ -226,8 +226,9 @@ static int ml86v7667_fill_fmt(struct v4l2_subdev *sd,
 static int ml86v7667_g_mbus_config(struct v4l2_subdev *sd,
   struct v4l2_mbus_config *cfg)
 {
-   cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING |
-V4L2_MBUS_DATA_ACTIVE_HIGH;
+   cfg->pb_flags = V4L2_MBUS_MASTER
+   | V4L2_MBUS_PCLK_SAMPLE_RISING
+   | V4L2_MBUS_DATA_ACTIVE_HIGH;
cfg->type = V4L2_MBUS_BT656;
 
return 0;
diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index b1665d97e0fd..d9698b535080 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -857,9 +857,11 @@ static int

Re: [PATCH 04/24] media: v4l2-mediabus: convert flags to enums and document them

2017-10-11 Thread Pavel Machek 
On Mon 2017-10-09 07:19:10, Mauro Carvalho Chehab wrote:
> There is a mess with media bus flags: there are two sets of
> flags, one used by parallel and ITU-R BT.656 outputs,
> and another one for CSI2.
> 
> Depending on the type, the same bit has different meanings.
> 

> @@ -86,11 +125,22 @@ enum v4l2_mbus_type {
>  /**
>   * struct v4l2_mbus_config - media bus configuration
>   * @type:in: interface type
> - * @flags:   in / out: configuration flags, depending on @type
> + * @pb_flags:in / out: configuration flags, if @type is
> + *   %V4L2_MBUS_PARALLEL or %V4L2_MBUS_BT656.
> + * @csi2_flags:  in / out: configuration flags, if @type is
> + *   %V4L2_MBUS_CSI2.
> + * @flag:access flags, no matter the @type.
> + *   Used just to avoid needing to rewrite the logic inside
> + *   soc_camera and pxa_camera drivers. Don't use on newer
> + *   drivers!
>   */
>  struct v4l2_mbus_config {
>   enum v4l2_mbus_type type;
> - unsigned int flags;
> + union {
> + enum v4l2_mbus_parallel_and_bt656_flags pb_flags;
> + enum v4l2_mbus_csi2_flags   csi2_flags;
> + unsigned intflag;
> + };
>  };
>  
>  static inline void v4l2_fill_pix_format(struct v4l2_pix_format
>  *pix_fmt,

The flags->flag conversion is quite subtle, and "flag" is confusing
because there is more than one inside. What about something like
__legacy_flags?

Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH 04/24] media: v4l2-mediabus: convert flags to enums and document them

2017-10-09 Thread Hans Verkuil 
On 09/10/17 12:19, Mauro Carvalho Chehab wrote:
> There is a mess with media bus flags: there are two sets of
> flags, one used by parallel and ITU-R BT.656 outputs,
> and another one for CSI2.
> 
> Depending on the type, the same bit has different meanings.
> 
> That's very confusing, and counter-intuitive. So, split them
> into two sets of flags, inside an enum.
> 
> This way, it becomes clearer that there are two separate sets
> of flags. It also makes easier if CSI1, CSP, CSI3, etc. would
> need a different set of flags.
> 
> As a side effect, enums can be documented via kernel-docs,
> so there will be an improvement at flags documentation.
> 
> Unfortunately, soc_camera and pxa_camera do a mess with
> the flags, using either one set of flags without proper
> checks about the type. That could be fixed, but, as both drivers
> are obsolete and in the process of cleanings, I opted to just
> keep the behavior, using an unsigned int inside those two
> drivers.
> 
> Signed-off-by: Mauro Carvalho Chehab 

Acked-by: Hans Verkuil 

Nice cleanup.

Regards,

Hans

[PATCH 04/24] media: v4l2-mediabus: convert flags to enums and document them

2017-10-09 Thread Mauro Carvalho Chehab 
There is a mess with media bus flags: there are two sets of
flags, one used by parallel and ITU-R BT.656 outputs,
and another one for CSI2.

Depending on the type, the same bit has different meanings.

That's very confusing, and counter-intuitive. So, split them
into two sets of flags, inside an enum.

This way, it becomes clearer that there are two separate sets
of flags. It also makes easier if CSI1, CSP, CSI3, etc. would
need a different set of flags.

As a side effect, enums can be documented via kernel-docs,
so there will be an improvement at flags documentation.

Unfortunately, soc_camera and pxa_camera do a mess with
the flags, using either one set of flags without proper
checks about the type. That could be fixed, but, as both drivers
are obsolete and in the process of cleanings, I opted to just
keep the behavior, using an unsigned int inside those two
drivers.

Signed-off-by: Mauro Carvalho Chehab 
---
 drivers/media/i2c/adv7180.c|  10 +-
 drivers/media/i2c/ml86v7667.c  |   5 +-
 drivers/media/i2c/mt9m111.c|   8 +-
 drivers/media/i2c/ov6650.c |  19 +--
 drivers/media/i2c/soc_camera/imx074.c  |   6 +-
 drivers/media/i2c/soc_camera/mt9m001.c |  10 +-
 drivers/media/i2c/soc_camera/mt9t031.c |  11 +-
 drivers/media/i2c/soc_camera/mt9t112.c |  11 +-
 drivers/media/i2c/soc_camera/mt9v022.c |  16 ++-
 drivers/media/i2c/soc_camera/ov5642.c  |   5 +-
 drivers/media/i2c/soc_camera/ov772x.c  |  10 +-
 drivers/media/i2c/soc_camera/ov9640.c  |  10 +-
 drivers/media/i2c/soc_camera/ov9740.c  |  10 +-
 drivers/media/i2c/soc_camera/rj54n1cb0c.c  |  12 +-
 drivers/media/i2c/soc_camera/tw9910.c  |  13 +-
 drivers/media/i2c/tc358743.c   |  10 +-
 drivers/media/i2c/tvp5150.c|   6 +-
 drivers/media/platform/pxa_camera.c|   8 +-
 drivers/media/platform/rcar-vin/rcar-core.c|   4 +-
 drivers/media/platform/rcar-vin/rcar-dma.c |   4 +-
 .../platform/soc_camera/sh_mobile_ceu_camera.c |   2 +-
 drivers/media/platform/soc_camera/soc_camera.c |   3 +-
 .../platform/soc_camera/soc_camera_platform.c  |   2 +-
 drivers/media/platform/soc_camera/soc_mediabus.c   |   2 +-
 drivers/media/v4l2-core/v4l2-fwnode.c  |   5 +-
 include/media/v4l2-fwnode.h|   4 +-
 include/media/v4l2-mediabus.h  | 144 ++---
 27 files changed, 217 insertions(+), 133 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 3df28f2f9b38..c220504049de 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -743,16 +743,16 @@ static int adv7180_g_mbus_config(struct v4l2_subdev *sd,
 
if (state->chip_info->flags & ADV7180_FLAG_MIPI_CSI2) {
cfg->type = V4L2_MBUS_CSI2;
-   cfg->flags = V4L2_MBUS_CSI2_1_LANE |
-   V4L2_MBUS_CSI2_CHANNEL_0 |
-   V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
+   cfg->csi2_flags = V4L2_MBUS_CSI2_1_LANE
+ | V4L2_MBUS_CSI2_CHANNEL_0
+ | V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
} else {
/*
 * The ADV7180 sensor supports BT.601/656 output modes.
 * The BT.656 is default and not yet configurable by s/w.
 */
-   cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING |
-V4L2_MBUS_DATA_ACTIVE_HIGH;
+   cfg->pb_flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING
+   | V4L2_MBUS_DATA_ACTIVE_HIGH;
cfg->type = V4L2_MBUS_BT656;
}
 
diff --git a/drivers/media/i2c/ml86v7667.c b/drivers/media/i2c/ml86v7667.c
index 57ef901edb06..a25114d0c31f 100644
--- a/drivers/media/i2c/ml86v7667.c
+++ b/drivers/media/i2c/ml86v7667.c
@@ -226,8 +226,9 @@ static int ml86v7667_fill_fmt(struct v4l2_subdev *sd,
 static int ml86v7667_g_mbus_config(struct v4l2_subdev *sd,
   struct v4l2_mbus_config *cfg)
 {
-   cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING |
-V4L2_MBUS_DATA_ACTIVE_HIGH;
+   cfg->pb_flags = V4L2_MBUS_MASTER
+   | V4L2_MBUS_PCLK_SAMPLE_RISING
+   | V4L2_MBUS_DATA_ACTIVE_HIGH;
cfg->type = V4L2_MBUS_BT656;
 
return 0;
diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index 99b992e46702..53c406f87aa7 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -857,9 +857,11 @@ static int mt9m111_enum_mbus_code(struct v4l2_subdev *sd,
 static int mt9m111_g_mbus_config(struct v4l2_subdev *sd,