[PATCH] ravb: remove erroneous comment

[Niklas Söderlund]

When addressing a review comment in a early version of the offending
patch a comment where left in which should have been removed. Remove the
comment to keep it consistent with the code.

Fixes: 75efa06f457bbed3 ("ravb: add support for changing MTU")
Reported-by: Sergei Shtylyov 
Signed-off-by: Niklas Söderlund 
---
 drivers/net/ethernet/renesas/ravb_main.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c 
b/drivers/net/ethernet/renesas/ravb_main.c
index 54a6265da7a06460..68f122140966d4de 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -346,7 +346,6 @@ static int ravb_ring_init(struct net_device *ndev, int q)
int ring_size;
int i;
 
-   /* +16 gets room from the status from the card. */
priv->rx_buf_sz = (ndev->mtu <= 1492 ? PKT_BUF_SZ : ndev->mtu) +
ETH_HLEN + VLAN_HLEN;
 
-- 
2.16.2

[PATCH v3 2/2] arm64: dts: renesas: eagle: add I2C0 support

[Sergei Shtylyov]

Define the Eagle board dependent part of the I2C0 device node.

The I2C0 bus is populated by ON Semiconductor PCA9653 I/O expander and
Analog Devices ADV7511W HDMI transmitter (but we're only describing the
former chip now).

Based on the original (and large) patch by Vladimir Barinov.

Signed-off-by: Vladimir Barinov 
Signed-off-by: Sergei Shtylyov 
Reviewed-by: Geert Uytterhoeven 

---
Changes in version 3:
- added Geert's tag.

Changes in version 2:
- refreshed with the patch adding EtherAVB pins removed for now...

 arch/arm64/boot/dts/renesas/r8a77970-eagle.dts |   20 
 1 file changed, 20 insertions(+)

Index: renesas/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
===
--- renesas.orig/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
+++ renesas/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
@@ -55,7 +55,27 @@
clock-frequency = <32768>;
 };
 
+ {
+   pinctrl-0 = <_pins>;
+   pinctrl-names = "default";
+
+   status = "okay";
+   clock-frequency = <40>;
+
+   io_expander: gpio@20 {
+   compatible = "onnn,pca9654";
+   reg = <0x20>;
+   gpio-controller;
+   #gpio-cells = <2>;
+   };
+};
+
  {
+   i2c0_pins: i2c0 {
+   groups = "i2c0";
+   function = "i2c0";
+   };
+
scif0_pins: scif0 {
groups = "scif0_data";
function = "scif0";

[PATCH v3 1/2] arm64: dts: renesas: r8a77970: add I2C support

[Sergei Shtylyov]

Define the generic R8A77970 parts of the I2C[0-4] device node.

Based on the original (and large) patch by Daisuke Matsushita
.

Signed-off-by: Vladimir Barinov 
Signed-off-by: Sergei Shtylyov 
Reviewed-by: Geert Uytterhoeven 

---
Changes in version 3:
- added SYS-DMAC2 references to the "dmas"/"dma-names" props;
- added Geert's tag.

Changes in version 2:
- made  use of  the SYSC power domain #define's;
- moved the I2C nodes before HSCIF nodes.

 arch/arm64/boot/dts/renesas/r8a77970.dtsi |   93 ++
 1 file changed, 93 insertions(+)

Index: renesas/arch/arm64/boot/dts/renesas/r8a77970.dtsi
===
--- renesas.orig/arch/arm64/boot/dts/renesas/r8a77970.dtsi
+++ renesas/arch/arm64/boot/dts/renesas/r8a77970.dtsi
@@ -19,6 +19,14 @@
#address-cells = <2>;
#size-cells = <2>;
 
+   aliases {
+   i2c0 = 
+   i2c1 = 
+   i2c2 = 
+   i2c3 = 
+   i2c4 = 
+   };
+
psci {
compatible = "arm,psci-1.0", "arm,psci-0.2";
method = "smc";
@@ -338,6 +346,91 @@
   <_ds1 22>, <_ds1 23>;
};
 
+   i2c0: i2c@e650 {
+   compatible = "renesas,i2c-r8a77970",
+"renesas,rcar-gen3-i2c";
+   reg = <0 0xe650 0 0x40>;
+   interrupts = ;
+   clocks = < CPG_MOD 931>;
+   power-domains = < R8A77970_PD_ALWAYS_ON>;
+   resets = < 931>;
+   dmas = < 0x91>, < 0x90>,
+  < 0x91>, < 0x90>;
+   dma-names = "tx", "rx", "tx", "rx";
+   i2c-scl-internal-delay-ns = <6>;
+   #address-cells = <1>;
+   #size-cells = <0>;
+   status = "disabled";
+   };
+
+   i2c1: i2c@e6508000 {
+   compatible = "renesas,i2c-r8a77970",
+"renesas,rcar-gen3-i2c";
+   reg = <0 0xe6508000 0 0x40>;
+   interrupts = ;
+   clocks = < CPG_MOD 930>;
+   power-domains = < R8A77970_PD_ALWAYS_ON>;
+   resets = < 930>;
+   dmas = < 0x93>, < 0x92>,
+  < 0x93>, < 0x92>;
+   dma-names = "tx", "rx", "tx", "rx";
+   i2c-scl-internal-delay-ns = <6>;
+   #address-cells = <1>;
+   #size-cells = <0>;
+   status = "disabled";
+   };
+
+   i2c2: i2c@e651 {
+   compatible = "renesas,i2c-r8a77970",
+"renesas,rcar-gen3-i2c";
+   reg = <0 0xe651 0 0x40>;
+   interrupts = ;
+   clocks = < CPG_MOD 929>;
+   power-domains = < R8A77970_PD_ALWAYS_ON>;
+   resets = < 929>;
+   dmas = < 0x95>, < 0x94>,
+  < 0x95>, < 0x94>;
+   dma-names = "tx", "rx", "tx", "rx";
+   i2c-scl-internal-delay-ns = <6>;
+   #address-cells = <1>;
+   #size-cells = <0>;
+   status = "disabled";
+   };
+
+   i2c3: i2c@e66d {
+   compatible = "renesas,i2c-r8a77970",
+"renesas,rcar-gen3-i2c";
+   reg = <0 0xe66d 0 0x40>;
+   interrupts = ;
+   clocks = < CPG_MOD 928>;
+   power-domains = < R8A77970_PD_ALWAYS_ON>;
+   resets = < 928>;
+   dmas = < 0x97>, < 0x96>,
+  < 0x97>, < 0x96>;
+   dma-names = "tx", "rx", "tx", "rx";
+   i2c-scl-internal-delay-ns = <6>;
+   #address-cells = <1>;
+   #size-cells = <0>;
+   status = "disabled";
+   };
+
+   i2c4: i2c@e66d8000 {
+   compatible = "renesas,i2c-r8a77970",
+"renesas,rcar-gen3-i2c";
+   reg = <0 0xe66d8000 0 0x40>;
+   interrupts = ;
+   clocks = < CPG_MOD 927>;
+   power-domains = < R8A77970_PD_ALWAYS_ON>;
+   resets = < 927>;
+   dmas = < 0x99>, < 0x98>,
+  < 0x99>, < 0x98>;
+ 

[PATCH v3 0/2] Add R8A77970/Eagle I2C support

[Sergei Shtylyov]

Hello!

Here's the set of 2 patches against Simon Horman's 'renesas.git' repo's
'renesas-devel-20180301-v4.16-rc3' tag. We're adding the R8A77970 I2C nodes
and then describing the PCA9654 I/O expander connected to the I2C0 bus.

[1/2] arm64: dts: renesas: r8a77970: add I2C support
[2/2] arm64: dts: renesas: eagle: add I2C0 support

WBR, Sergei

Re: [PATCH v11 16/32] rcar-vin: read subdevice format for crop only when needed

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2018-03-02 13:06:27 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Friday, 2 March 2018 03:57:35 EET Niklas Söderlund wrote:
> > Instead of caching the subdevice format each time the video device
> > format is set read it directly when it's needed. As it turns out the
> > format is only needed when figuring out the max rectangle for cropping.
> > 
> > This simplifies the code and makes it clearer what the source format is
> > used for.
> > 
> > Signed-off-by: Niklas Söderlund 
> > ---
> >  drivers/media/platform/rcar-vin/rcar-v4l2.c | 158 +
> >  drivers/media/platform/rcar-vin/rcar-vin.h  |  12 ---
> >  2 files changed, 80 insertions(+), 90 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> > 3290e603b44cdf3a..55640c6b2a1200ca 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > @@ -144,67 +144,62 @@ static int rvin_format_align(struct rvin_dev *vin,
> > struct v4l2_pix_format *pix) * V4L2
> >   */
> > 
> > -static void rvin_reset_crop_compose(struct rvin_dev *vin)
> > +static int rvin_get_vin_format_from_source(struct rvin_dev *vin,
> > +  struct v4l2_pix_format *pix)
> >  {
> > +   struct v4l2_subdev_format fmt = {
> > +   .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > +   .pad = vin->digital->source_pad,
> > +   };
> > +   int ret;
> > +
> > +   ret = v4l2_subdev_call(vin_to_source(vin), pad, get_fmt, NULL, );
> > +   if (ret)
> > +   return ret;
> > +
> > +   v4l2_fill_pix_format(pix, );
> > +
> > +   return rvin_format_align(vin, pix);
> > +}
> > +
> > +static int rvin_reset_format(struct rvin_dev *vin)
> > +{
> > +   int ret;
> > +
> > +   ret = rvin_get_vin_format_from_source(vin, >format);
> > +   if (ret)
> > +   return ret;
> > +
> > vin->crop.top = vin->crop.left = 0;
> > -   vin->crop.width = vin->source.width;
> > -   vin->crop.height = vin->source.height;
> > +   vin->crop.width = vin->format.width;
> > +   vin->crop.height = vin->format.height;
> > 
> > vin->compose.top = vin->compose.left = 0;
> > vin->compose.width = vin->format.width;
> > vin->compose.height = vin->format.height;
> > -}
> > -
> > -static int rvin_reset_format(struct rvin_dev *vin)
> > -{
> > -   struct v4l2_subdev_format fmt = {
> > -   .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > -   };
> > -   struct v4l2_mbus_framefmt *mf = 
> > -   int ret;
> > -
> > -   fmt.pad = vin->digital->source_pad;
> > -
> > -   ret = v4l2_subdev_call(vin_to_source(vin), pad, get_fmt, NULL, );
> > -   if (ret)
> > -   return ret;
> > -
> > -   vin->format.width   = mf->width;
> > -   vin->format.height  = mf->height;
> > -   vin->format.colorspace  = mf->colorspace;
> > -   vin->format.field   = mf->field;
> > -
> > -   rvin_reset_crop_compose(vin);
> > -
> > -   vin->format.bytesperline = rvin_format_bytesperline(>format);
> > -   vin->format.sizeimage = rvin_format_sizeimage(>format);
> > 
> > return 0;
> >  }
> > 
> > -static int __rvin_try_format_source(struct rvin_dev *vin,
> > -   u32 which,
> > -   struct v4l2_pix_format *pix,
> > -   struct rvin_source_fmt *source)
> > +static int rvin_try_format(struct rvin_dev *vin, u32 which,
> > +  struct v4l2_pix_format *pix,
> > +  struct v4l2_rect *crop, struct v4l2_rect *compose)
> >  {
> > -   struct v4l2_subdev *sd;
> > +   struct v4l2_subdev *sd = vin_to_source(vin);
> > struct v4l2_subdev_pad_config *pad_cfg;
> > struct v4l2_subdev_format format = {
> > .which = which,
> > +   .pad = vin->digital->source_pad,
> > };
> > enum v4l2_field field;
> > u32 width, height;
> > int ret;
> > 
> > -   sd = vin_to_source(vin);
> > -
> > -   v4l2_fill_mbus_format(, pix, vin->digital->code);
> > -
> > pad_cfg = v4l2_subdev_alloc_pad_config(sd);
> > if (pad_cfg == NULL)
> > return -ENOMEM;
> > 
> > -   format.pad = vin->digital->source_pad;
> > +   v4l2_fill_mbus_format(, pix, vin->digital->code);
> > 
> > /* Allow the video device to override field and to scale */
> > field = pix->field;
> > @@ -217,34 +212,34 @@ static int __rvin_try_format_source(struct rvin_dev
> > *vin,
> > 
> > v4l2_fill_pix_format(pix, );
> > 
> > -   source->width = pix->width;
> > -   source->height = pix->height;
> > +   crop->top = crop->left = 0;
> > +   crop->width = pix->width;
> > +   crop->height = pix->height;
> > +
> > +   /*
> > +* If source is ALTERNATE the driver will use the VIN hardware
> > +* to INTERLACE it. The crop height then needs to be doubled.
> > +*/
> > +   if (pix->field == V4L2_FIELD_ALTERNATE)
> > +   

Re: [PATCH v11 12/32] rcar-vin: fix handling of single field frames (top, bottom and alternate fields)

[Niklas Söderlund]

Hi Laurent,

Thanks for your comments.

On 2018-03-02 12:48:55 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Friday, 2 March 2018 03:57:31 EET Niklas Söderlund wrote:
> > There was never proper support in the VIN driver to deliver ALTERNATING
> > field format to user-space, remove this field option. The problem is
> > that ALTERNATING filed order requires the sequence numbers of buffers
> 
> s/filed/field/
> 
> > returned to userspace to reflect if fields where dropped or not,
> > something which is not possible with the VIN drivers capture logic.
> > 
> > The VIN driver can still capture from a video source which delivers
> > frames in ALTERNATING field order, but needs to combine them using the
> > VIN hardware into INTERLACED field order. Before this change if a source
> > was delivering fields using ALTERNATE the driver would default to
> > combining them using this hardware feature. Only if the user explicitly
> > requested ALTERNATE filed order would incorrect frames be delivered.
> > 
> > The height should not be cut in half for the format for TOP or BOTTOM
> > fields settings. This was a mistake and it was made visible by the
> > scaling refactoring. Correct behavior is that the user should request a
> > frame size that fits the half height frame reflected in the field
> > setting. If not the VIN will do its best to scale the top or bottom to
> > the requested format and cropping and scaling do not work as expected.
> > 
> > Signed-off-by: Niklas Söderlund 
> > ---
> >  drivers/media/platform/rcar-vin/rcar-dma.c  | 15 +--
> >  drivers/media/platform/rcar-vin/rcar-v4l2.c | 40 +++---
> >  2 files changed, 10 insertions(+), 45 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> > b/drivers/media/platform/rcar-vin/rcar-dma.c index
> > fd14be20a6604d7a..c8831e189d362c8b 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -617,7 +617,6 @@ static int rvin_setup(struct rvin_dev *vin)
> > case V4L2_FIELD_INTERLACED_BT:
> > vnmc = VNMC_IM_FULL | VNMC_FOC;
> > break;
> > -   case V4L2_FIELD_ALTERNATE:
> 
> Just to make sure I understand things correctly, field will never be 
> V4L2_FIELD_ALTERNATE after this patch, right ?

That is the intention, but as you noticed bellow it's not perfect until 
4 patches later in the series, see bellow.

> 
> > case V4L2_FIELD_NONE:
> > if (vin->continuous) {
> > vnmc = VNMC_IM_ODD_EVEN;
> > @@ -757,18 +756,6 @@ static int rvin_get_active_slot(struct rvin_dev *vin,
> > u32 vnms) return 0;
> >  }
> > 
> > -static enum v4l2_field rvin_get_active_field(struct rvin_dev *vin, u32
> > vnms)
> > -{
> > -   if (vin->format.field == V4L2_FIELD_ALTERNATE) {
> > -   /* If FS is set it's a Even field */
> > -   if (vnms & VNMS_FS)
> > -   return V4L2_FIELD_BOTTOM;
> > -   return V4L2_FIELD_TOP;
> > -   }
> > -
> > -   return vin->format.field;
> > -}
> > -
> >  static void rvin_set_slot_addr(struct rvin_dev *vin, int slot, dma_addr_t
> > addr) {
> > const struct rvin_video_format *fmt;
> > @@ -941,7 +928,7 @@ static irqreturn_t rvin_irq(int irq, void *data)
> > goto done;
> > 
> > /* Capture frame */
> > -   vin->queue_buf[slot]->field = rvin_get_active_field(vin, vnms);
> > +   vin->queue_buf[slot]->field = vin->format.field;
> > vin->queue_buf[slot]->sequence = sequence;
> > vin->queue_buf[slot]->vb2_buf.timestamp = ktime_get_ns();
> > vb2_buffer_done(>queue_buf[slot]->vb2_buf, VB2_BUF_STATE_DONE);
> > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> > ebcd78b1bb6e8cb6..cef9070884d93ba6 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > @@ -121,33 +121,6 @@ static int rvin_reset_format(struct rvin_dev *vin)
> > vin->format.colorspace  = mf->colorspace;
> > vin->format.field   = mf->field;
> > 
> > -   /*
> > -* If the subdevice uses ALTERNATE field mode and G_STD is
> > -* implemented use the VIN HW to combine the two fields to
> > -* one INTERLACED frame. The ALTERNATE field mode can still
> > -* be requested in S_FMT and be respected, this is just the
> > -* default which is applied at probing or when S_STD is called.
> > -*/
> > -   if (vin->format.field == V4L2_FIELD_ALTERNATE &&
> > -   v4l2_subdev_has_op(vin_to_source(vin), video, g_std))
> > -   vin->format.field = V4L2_FIELD_INTERLACED;
> > -
> > -   switch (vin->format.field) {
> > -   case V4L2_FIELD_TOP:
> > -   case V4L2_FIELD_BOTTOM:
> > -   case V4L2_FIELD_ALTERNATE:
> > -   vin->format.height /= 2;
> > -   break;
> > -   case V4L2_FIELD_NONE:
> > -   case V4L2_FIELD_INTERLACED_TB:
> > -   case 

Re: [PATCH v11 15/32] rcar-vin: break out format alignment and checking

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2018-03-02 11:53:54 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Friday, 2 March 2018 03:57:34 EET Niklas Söderlund wrote:
> > Part of the format alignment and checking can be shared with the Gen3
> > format handling. Break that part out to a separate function.
> > 
> > Signed-off-by: Niklas Söderlund 
> > ---
> >  drivers/media/platform/rcar-vin/rcar-v4l2.c | 96 +++---
> >  1 file changed, 54 insertions(+), 42 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> > b94ca9ffb1d3b323..3290e603b44cdf3a 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > @@ -87,6 +87,59 @@ static u32 rvin_format_sizeimage(struct v4l2_pix_format
> > *pix) return pix->bytesperline * pix->height;
> >  }
> > 
> > +static int rvin_format_align(struct rvin_dev *vin, struct v4l2_pix_format
> > *pix)
> > +{
> > +   u32 walign;
> > +
> > +   if (!rvin_format_from_pixel(pix->pixelformat) ||
> > +   (vin->info->model == RCAR_M1 &&
> > +pix->pixelformat == V4L2_PIX_FMT_XBGR32))
> > +   pix->pixelformat = RVIN_DEFAULT_FORMAT;
> > +
> > +   switch (pix->field) {
> > +   case V4L2_FIELD_TOP:
> > +   case V4L2_FIELD_BOTTOM:
> > +   case V4L2_FIELD_NONE:
> > +   case V4L2_FIELD_INTERLACED_TB:
> > +   case V4L2_FIELD_INTERLACED_BT:
> > +   case V4L2_FIELD_INTERLACED:
> > +   break;
> > +   case V4L2_FIELD_ALTERNATE:
> > +   /*
> > +* Driver do not (yet) support outputting ALTERNATE to a
> > +* userspace. It does support outputting INTERLACED so use
> > +* the VIN hardware to combine the two fields.
> > +*/
> > +   pix->field = V4L2_FIELD_INTERLACED;
> > +   pix->height *= 2;
> > +   break;
> > +   default:
> > +   pix->field = RVIN_DEFAULT_FIELD;
> > +   break;
> > +   }
> > +
> > +   /* HW limit width to a multiple of 32 (2^5) for NV16 else 2 (2^1) */
> > +   walign = vin->format.pixelformat == V4L2_PIX_FMT_NV16 ? 5 : 1;
> > +
> > +   /* Limit to VIN capabilities */
> > +   v4l_bound_align_image(>width, 2, vin->info->max_width, walign,
> > + >height, 4, vin->info->max_height, 2, 0);
> > +
> > +   pix->bytesperline = rvin_format_bytesperline(pix);
> > +   pix->sizeimage = rvin_format_sizeimage(pix);
> > +
> > +   if (vin->info->model == RCAR_M1 &&
> > +   pix->pixelformat == V4L2_PIX_FMT_XBGR32) {
> > +   vin_err(vin, "pixel format XBGR32 not supported on M1\n");
> > +   return -EINVAL;
> > +   }
> 
> This can't happen as you set the pixel format to the default earlier in the 
> same case.

This is the result of a bad re-ordering of the patches for v11 and 
should be removed. Thanks for spotting this! Sometime I get blind to my 
own code.

> 
> > +   vin_dbg(vin, "Format %ux%u bpl: %d size: %d\n",
> > +   pix->width, pix->height, pix->bytesperline, pix->sizeimage);
> 
> While at it you could use %u for bpl and size.

Thanks.

> 
> > +
> > +   return 0;
> > +}
> > +
> >  /* 
> >   * V4L2
> >   */
> > @@ -184,55 +237,14 @@ static int __rvin_try_format(struct rvin_dev *vin,
> >  struct v4l2_pix_format *pix,
> >  struct rvin_source_fmt *source)
> >  {
> > -   u32 walign;
> > int ret;
> > 
> > -   if (!rvin_format_from_pixel(pix->pixelformat) ||
> > -   (vin->info->model == RCAR_M1 &&
> > -pix->pixelformat == V4L2_PIX_FMT_XBGR32))
> > -   pix->pixelformat = RVIN_DEFAULT_FORMAT;
> > -
> > /* Limit to source capabilities */
> > ret = __rvin_try_format_source(vin, which, pix, source);
> 
> This functions uses the pixel format. Isn't it a problem that you don't first 
> set it to the default if the requested pixel format isn't supported ?

Hum, yes it might be! I never thought about it but yes it's probably 
best to keep this check here. Thanks!

> 
> > if (ret)
> > return ret;
> > 
> > -   switch (pix->field) {
> > -   case V4L2_FIELD_TOP:
> > -   case V4L2_FIELD_BOTTOM:
> > -   case V4L2_FIELD_NONE:
> > -   case V4L2_FIELD_INTERLACED_TB:
> > -   case V4L2_FIELD_INTERLACED_BT:
> > -   case V4L2_FIELD_INTERLACED:
> > -   break;
> > -   case V4L2_FIELD_ALTERNATE:
> > -   /*
> > -* Driver do not (yet) support outputting ALTERNATE to a
> > -* userspace. It does support outputting INTERLACED so use
> > -* the VIN hardware to combine the two fields.
> > -*/
> > -   pix->field = V4L2_FIELD_INTERLACED;
> > -   pix->height *= 2;
> > -   break;
> > -   default:
> > -   pix->field = RVIN_DEFAULT_FIELD;
> > -   break;
> > -   }
> > -
> > - 

Re: [PATCH v11 28/32] rcar-vin: add link notify for Gen3

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2018-03-02 14:00:19 +0200, Laurent Pinchart wrote:

[snip]

> > +
> > +   /* If any entity is in use don't allow link changes. */
> > +   media_device_for_each_entity(entity, >mdev)
> > +   if (entity->use_count)
> > +   return -EBUSY;
> 
> This means that you disallow link setup when any video node is open. 
> According 
> to the comment above, isn't it stream_count that you want to check ? If so 
> the 
> MC core does it for you (unless you create links with the 
> MEDIA_LNK_FL_DYNAMIC 
> flag set), see __media_entity_setup_link().

You are correct that the comment and code don't align. I rather update 
the comment and keep denying link enablement if any video devices are 
open. I'm sure this is not a real issue but this group concept feels a 
bit fragile, so better safe then sorry. Or do you feel there is a 
benefit for the user to be able to change the graph with video nodes 
open? We can always loosen the constraint later if it becomes a problem 
but introducing it if we would need it could be considered a regression?


> 
> > +   mutex_lock(>lock);
> > +
> > +   /*
> > +* Figure out which VIN the link concern's and lookup
> > +* which master VIN controls the routes for that VIN.
> > +*/
> 
> Can't you simply use a container_of to cast from vdev to vin ?

Thank you for this, made the code much more readable!

> 
> > +   vdev = media_entity_to_video_device(link->sink->entity);
> > +   for (i = 0; i < RCAR_VIN_NUM; i++) {
> > +   if (group->vin[i] && >vin[i]->vdev == vdev) {
> > +   vin = group->vin[i];
> > +   master_id = rvin_group_id_to_master(vin->id);
> > +   break;
> > +   }
> > +   }

[snip]

-- 
Regards,
Niklas Söderlund

Re: [PATCH v11 22/32] rcar-vin: force default colorspace for media centric mode

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2018-03-02 11:59:14 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Friday, 2 March 2018 03:57:41 EET Niklas Söderlund wrote:
> > When the VIN driver is running in media centric mode (on Gen3) the
> > colorspace is not retrieved from the video source instead the user is
> > expected to set it as part of the format. There is no way for the VIN
> > driver to validated the colorspace requested by user-space, this creates
> > a problem where validation tools fail. Until the user requested
> > colorspace can be validated lets force it to the driver default.
> 
> The problem here is that the V4L2 specification clearly documents the 
> colorspace fields as being set by drivers for capture devices. Using the 
> values supplied by userspace thus wouldn't comply with the API. The API has 
> to 
> be updated to allow us to implement this feature, but until then Hans wants 
> the userspace to be set by the driver to a fixed value. Could you update the 
> commit message accordingly, as well as the comment below ?

Yes, your description of the issue is better I will rephrase my commit 
message and comment.

> 
> > Signed-off-by: Niklas Söderlund 
> > ---
> >  drivers/media/platform/rcar-vin/rcar-v4l2.c | 16 ++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> > 8d92710efffa7276..02f3100ed30db63c 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > @@ -675,12 +675,24 @@ static const struct v4l2_ioctl_ops rvin_ioctl_ops = {
> >   * V4L2 Media Controller
> >   */
> > 
> > +static int rvin_mc_try_format(struct rvin_dev *vin, struct v4l2_pix_format
> > *pix) +{
> > +   /*
> > +* There is no way to validate the colorspace provided by the
> > +* user. Until it can be validated force colorspace to the
> > +* driver default.
> > +*/
> > +   pix->colorspace = RVIN_DEFAULT_COLORSPACE;
> 
> Should you also set the xfer_func, quantization and ycbcr_enc ?

You are correct, I will set these fields as well.

> 
> Apart from that,
> 
> Reviewed-by: Laurent Pinchart 

Thank you. As I have rewritten the commit message and set more fields 
then just the colorspace I have not picked up this tag for the next 
version.

> 
> > +
> > +   return rvin_format_align(vin, pix);
> > +}
> > +
> >  static int rvin_mc_try_fmt_vid_cap(struct file *file, void *priv,
> >struct v4l2_format *f)
> >  {
> > struct rvin_dev *vin = video_drvdata(file);
> > 
> > -   return rvin_format_align(vin, >fmt.pix);
> > +   return rvin_mc_try_format(vin, >fmt.pix);
> >  }
> > 
> >  static int rvin_mc_s_fmt_vid_cap(struct file *file, void *priv,
> > @@ -692,7 +704,7 @@ static int rvin_mc_s_fmt_vid_cap(struct file *file, void
> > *priv, if (vb2_is_busy(>queue))
> > return -EBUSY;
> > 
> > -   ret = rvin_format_align(vin, >fmt.pix);
> > +   ret = rvin_mc_try_format(vin, >fmt.pix);
> > if (ret)
> > return ret;
> 
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v11 19/32] rcar-vin: add function to manipulate Gen3 chsel value

[Niklas Söderlund]

Hi Laurent,

Thank you for your feedback.

On 2018-03-02 13:31:47 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Friday, 2 March 2018 03:57:38 EET Niklas Söderlund wrote:
> > On Gen3 the CSI-2 routing is controlled by the VnCSI_IFMD register. One
> > feature of this register is that it's only present in the VIN0 and VIN4
> > instances. The register in VIN0 controls the routing for VIN0-3 and the
> > register in VIN4 controls routing for VIN4-7.
> > 
> > To be able to control routing from a media device this function is need
> > to control runtime PM for the subgroup master (VIN0 and VIN4). The
> > subgroup master must be switched on before the register is manipulated,
> > once the operation is complete it's safe to switch the master off and
> > the new routing will still be in effect.
> > 
> > Signed-off-by: Niklas Söderlund 
> 
> Reviewed-by: Laurent Pinchart 

Thank you.

> 
> > ---
> >  drivers/media/platform/rcar-vin/rcar-dma.c | 38 +++
> >  drivers/media/platform/rcar-vin/rcar-vin.h |  2 ++
> >  2 files changed, 40 insertions(+)
> 
> By the way it would be useful if you added per-patch changelogs. You can 
> capture them in the commit message below a --- line.

I'm feeling the pain already of not doing this. I have in the past tried 
to keep such change long under a --- line like you suggest. But 
something in my work flow at that time caused the --- to be dropped and 
I never figured out what caused it.

I will try it again and see if I can figure out what caused it and how I 
can work around it. Thanks for brining this up.

> 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> > b/drivers/media/platform/rcar-vin/rcar-dma.c index
> > 57bb288b3ca67a60..3fb9c325285c5a5a 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -16,6 +16,7 @@
> > 
> >  #include 
> >  #include 
> > +#include 
> > 
> >  #include 
> > 
> > @@ -1228,3 +1229,40 @@ int rvin_dma_register(struct rvin_dev *vin, int irq)
> > 
> > return ret;
> >  }
> > +
> > +/* 
> > + * Gen3 CHSEL manipulation
> > + */
> > +
> > +/*
> > + * There is no need to have locking around changing the routing
> > + * as it's only possible to do so when no VIN in the group is
> > + * streaming so nothing can race with the VNMC register.
> > + */
> > +int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel)
> > +{
> > +   u32 ifmd, vnmc;
> > +   int ret;
> > +
> > +   ret = pm_runtime_get_sync(vin->dev);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   /* Make register writes take effect immediately. */
> > +   vnmc = rvin_read(vin, VNMC_REG);
> > +   rvin_write(vin, vnmc & ~VNMC_VUP, VNMC_REG);
> > +
> > +   ifmd = VNCSI_IFMD_DES2 | VNCSI_IFMD_DES1 | VNCSI_IFMD_DES0 |
> > +   VNCSI_IFMD_CSI_CHSEL(chsel);
> > +
> > +   rvin_write(vin, ifmd, VNCSI_IFMD_REG);
> > +
> > +   vin_dbg(vin, "Set IFMD 0x%x\n", ifmd);
> > +
> > +   /* Restore VNMC. */
> > +   rvin_write(vin, vnmc, VNMC_REG);
> > +
> > +   pm_runtime_put(vin->dev);
> > +
> > +   return ret;
> > +}
> > diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h
> > b/drivers/media/platform/rcar-vin/rcar-vin.h index
> > b3802651eaa78ea9..666308946eb4994d 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> > @@ -165,4 +165,6 @@ const struct rvin_video_format
> > *rvin_format_from_pixel(u32 pixelformat); /* Cropping, composing and
> > scaling */
> >  void rvin_crop_scale_comp(struct rvin_dev *vin);
> > 
> > +int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel);
> > +
> >  #endif
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v11 17/32] rcar-vin: move media bus configuration to struct rvin_info

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2018-03-02 13:26:58 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Friday, 2 March 2018 03:57:36 EET Niklas Söderlund wrote:
> > Bus configuration will once the driver is extended to support Gen3
> > contain information not specific to only the directly connected parallel
> > subdevice. Move it to struct rvin_dev to show it's not always coupled
> > to the parallel subdevice.
> 
> The subject line still mentions rvin_info. Are you so emotionally attached to 
> it that you have trouble fixing that ? ;-)

I had fixed this, but when I reviewed the patches before I sent them out 
I miss read the diff and it looked like I had forgotten to fix this and 
I broke it again. So in short it seems my subconscious are still 
emotionally attached to rvin_info :-)

> 
> > Signed-off-by: Niklas Söderlund 
> > Reviewed-by: Hans Verkuil 
> > ---
> >  drivers/media/platform/rcar-vin/rcar-core.c | 18 +-
> >  drivers/media/platform/rcar-vin/rcar-dma.c  | 11 ++-
> >  drivers/media/platform/rcar-vin/rcar-v4l2.c |  2 +-
> >  drivers/media/platform/rcar-vin/rcar-vin.h  |  9 -
> >  4 files changed, 20 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> > b/drivers/media/platform/rcar-vin/rcar-core.c index
> > cc863e4ec9a4d4b3..449175c3133e42c6 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > @@ -65,10 +65,10 @@ static int rvin_digital_subdevice_attach(struct rvin_dev
> > *vin, vin->digital->sink_pad = ret < 0 ? 0 : ret;
> > 
> > /* Find compatible subdevices mbus format */
> > -   vin->digital->code = 0;
> > +   vin->mbus_code = 0;
> > code.index = 0;
> > code.pad = vin->digital->source_pad;
> > -   while (!vin->digital->code &&
> > +   while (!vin->mbus_code &&
> >!v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL, )) {
> > code.index++;
> > switch (code.code) {
> > @@ -76,16 +76,16 @@ static int rvin_digital_subdevice_attach(struct rvin_dev
> > *vin, case MEDIA_BUS_FMT_UYVY8_2X8:
> > case MEDIA_BUS_FMT_UYVY10_2X10:
> > case MEDIA_BUS_FMT_RGB888_1X24:
> > -   vin->digital->code = code.code;
> > +   vin->mbus_code = code.code;
> > vin_dbg(vin, "Found media bus format for %s: %d\n",
> > -   subdev->name, vin->digital->code);
> > +   subdev->name, vin->mbus_code);
> > break;
> > default:
> > break;
> > }
> > }
> > 
> > -   if (!vin->digital->code) {
> > +   if (!vin->mbus_code) {
> > vin_err(vin, "Unsupported media bus format for %s\n",
> > subdev->name);
> > return -EINVAL;
> > @@ -190,16 +190,16 @@ static int rvin_digital_parse_v4l2(struct device *dev,
> > if (vep->base.port || vep->base.id)
> > return -ENOTCONN;
> > 
> > -   rvge->mbus_cfg.type = vep->bus_type;
> > +   vin->mbus_cfg.type = vep->bus_type;
> > 
> > -   switch (rvge->mbus_cfg.type) {
> > +   switch (vin->mbus_cfg.type) {
> > case V4L2_MBUS_PARALLEL:
> > vin_dbg(vin, "Found PARALLEL media bus\n");
> > -   rvge->mbus_cfg.flags = vep->bus.parallel.flags;
> > +   vin->mbus_cfg.flags = vep->bus.parallel.flags;
> > break;
> > case V4L2_MBUS_BT656:
> > vin_dbg(vin, "Found BT656 media bus\n");
> > -   rvge->mbus_cfg.flags = 0;
> > +   vin->mbus_cfg.flags = 0;
> > break;
> > default:
> > vin_err(vin, "Unknown media bus type\n");
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> > b/drivers/media/platform/rcar-vin/rcar-dma.c index
> > c8831e189d362c8b..4ebf76c30a3e9117 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -633,7 +633,7 @@ static int rvin_setup(struct rvin_dev *vin)
> > /*
> >  * Input interface
> >  */
> > -   switch (vin->digital->code) {
> > +   switch (vin->mbus_code) {
> > case MEDIA_BUS_FMT_YUYV8_1X16:
> > /* BT.601/BT.1358 16bit YCbCr422 */
> > vnmc |= VNMC_INF_YUV16;
> > @@ -641,7 +641,7 @@ static int rvin_setup(struct rvin_dev *vin)
> > break;
> > case MEDIA_BUS_FMT_UYVY8_2X8:
> > /* BT.656 8bit YCbCr422 or BT.601 8bit YCbCr422 */
> > -   vnmc |= vin->digital->mbus_cfg.type == V4L2_MBUS_BT656 ?
> > +   vnmc |= vin->mbus_cfg.type == V4L2_MBUS_BT656 ?
> > VNMC_INF_YUV8_BT656 : VNMC_INF_YUV8_BT601;
> > input_is_yuv = true;
> > break;
> > @@ -650,7 +650,7 @@ static int rvin_setup(struct rvin_dev *vin)
> > break;
> > case MEDIA_BUS_FMT_UYVY10_2X10:
> >