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

2017-12-19 Thread Mauro Carvalho Chehab 
Em Tue, 19 Dec 2017 10:30:15 +0100
Philipp Zabel  escreveu:

> Hi Mauro,
> 
> On Mon, 2017-12-18 at 17:53 -0200, 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.
> > 
> > Acked-by: Hans Verkuil 
> > Signed-off-by: Mauro Carvalho Chehab   
> 
> If I am not mistaken this is missing a conversion of
> drivers/staging/media/imx/imx-media-csi.c:
> 
> 8<
> diff --git a/drivers/staging/media/imx/imx-media-csi.c 
> b/drivers/staging/media/imx/imx-media-csi.c
> index eb7be5093a9d5..b1daac3a537d9 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -636,9 +636,10 @@ static int csi_setup(struct csi_priv *priv)
>  
>   /* compose mbus_config from the upstream endpoint */
>   mbus_cfg.type = priv->upstream_ep.bus_type;
> - mbus_cfg.flags = (priv->upstream_ep.bus_type == V4L2_MBUS_CSI2) ?
> - priv->upstream_ep.bus.mipi_csi2.flags :
> - priv->upstream_ep.bus.parallel.flags;
> + if (priv->upstream_ep.bus_type == V4L2_MBUS_CSI2)
> + mbus_cfg.csi2_flags = priv->upstream_ep.bus.mipi_csi2.flags;
> + else
> + mbus_cfg.pb_flags = priv->upstream_ep.bus.parallel.flags;
>  
>   /*
>    * we need to pass input frame to CSI interface, but


Oh, thanks for noticing! I really hate having drivers that don't
build with COMPILE_TEST, as that makes a lot harder to check if
something broke.


Thanks,
Mauro

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

2017-12-19 Thread Philipp Zabel 
Hi Mauro,

On Mon, 2017-12-18 at 17:53 -0200, 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.
> 
> Acked-by: Hans Verkuil 
> Signed-off-by: Mauro Carvalho Chehab 

If I am not mistaken this is missing a conversion of
drivers/staging/media/imx/imx-media-csi.c:

8<
diff --git a/drivers/staging/media/imx/imx-media-csi.c 
b/drivers/staging/media/imx/imx-media-csi.c
index eb7be5093a9d5..b1daac3a537d9 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -636,9 +636,10 @@ static int csi_setup(struct csi_priv *priv)
 
    /* compose mbus_config from the upstream endpoint */
    mbus_cfg.type = priv->upstream_ep.bus_type;
-   mbus_cfg.flags = (priv->upstream_ep.bus_type == V4L2_MBUS_CSI2) ?
-   priv->upstream_ep.bus.mipi_csi2.flags :
-   priv->upstream_ep.bus.parallel.flags;
+   if (priv->upstream_ep.bus_type == V4L2_MBUS_CSI2)
+   mbus_cfg.csi2_flags = priv->upstream_ep.bus.mipi_csi2.flags;
+   else
+   mbus_cfg.pb_flags = priv->upstream_ep.bus.parallel.flags;
 
/*
 * we need to pass input frame to CSI interface, but
>8

regards
Philipp

> ---
>  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  | 145 
> ++---
>  27 files changed, 218 insertions(+), 133 deletions(-)
> 
> 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 |
> -

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

2017-12-18 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.

Acked-by: Hans Verkuil 
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  | 145 ++---
 27 files changed, 218 insertions(+), 133 deletions(-)

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 mt9m111_enum_mbus_code(struct v4l2_subdev *sd,
 static int