diff mbox series

[v2,2/2] media: rcar-vin: Mask VNCSI_IFMD register

Message ID 20201116110428.27338-3-jacopo+renesas@jmondi.org (mailing list archive)
State Superseded
Delegated to: Kieran Bingham
Headers show
Series media: rcar-vin: Mask access to VNCSI_IFMD register | expand

Commit Message

Jacopo Mondi Nov. 16, 2020, 11:04 a.m. UTC 
The VNCSI_IFMD register controls the data expansion mode and the
channel routing between the CSI-2 receivers and VIN instances.

According to the chip manual revision 2.20 not all fields are available
for all the SoCs:
- V3M, V3H and E3 do not support the DES1 field has they do not feature
  a CSI20 receiver.
- D3 only supports parallel input, and the whole register shall always
  be written as 0.

Inspect the per-SoC channel routing table where the available CSI-2
instances are reported and configure VNCSI_IFMD accordingly.

This patch upports the BSP change commit f54697394457
("media: rcar-vin: Fix VnCSI_IFMD register access for r8a77990")

Suggested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/platform/rcar-vin/rcar-dma.c | 26 +++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

Comments

Niklas Söderlund Nov. 23, 2020, 3:31 p.m. UTC | #1 
Hi Jacopo,

Thanks for your work.

On 2020-11-16 12:04:28 +0100, Jacopo Mondi wrote:
> The VNCSI_IFMD register controls the data expansion mode and the
> channel routing between the CSI-2 receivers and VIN instances.
> 
> According to the chip manual revision 2.20 not all fields are available
> for all the SoCs:
> - V3M, V3H and E3 do not support the DES1 field has they do not feature
>   a CSI20 receiver.
> - D3 only supports parallel input, and the whole register shall always
>   be written as 0.
> 
> Inspect the per-SoC channel routing table where the available CSI-2
> instances are reported and configure VNCSI_IFMD accordingly.
> 
> This patch upports the BSP change commit f54697394457
> ("media: rcar-vin: Fix VnCSI_IFMD register access for r8a77990")

I really like this approach, nice work.

> 
> Suggested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 26 +++++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index 378514a75bc2..ab6818b34e5a 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -1570,7 +1570,9 @@ int rvin_dma_register(struct rvin_dev *vin, int irq)
>   */
>  int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel)
>  {
> -	u32 ifmd, vnmc;
> +	const struct rvin_group_route *route;
> +	u32 ifmd = 0;
> +	u32 vnmc;
>  	int ret;
>  
>  	ret = pm_runtime_get_sync(vin->dev);
> @@ -1583,9 +1585,27 @@ int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel)
>  	vnmc = rvin_read(vin, VNMC_REG);
>  	rvin_write(vin, vnmc & ~VNMC_VUP, VNMC_REG);
>  
> -	ifmd = VNCSI_IFMD_DES1 | VNCSI_IFMD_DES0 | VNCSI_IFMD_CSI_CHSEL(chsel);
> +	/*
> +	 * Set data expansion mode to "pad with 0s" by inspecting the routes
> +	 * table to find out which bit fields are available in the IFMD
> +	 * register. IFMD_DES1 controls data expansion mode for CSI20/21,
> +	 * IFMD_DES0 controls data expansion mode for CSI40/41.
> +	 */
> +	for (route = vin->info->routes; route->mask; route++) {
> +		/* CSI21 is only available in r8a7795es1 and not documented. */

I would drop this comment as we do not know what will happen in future 
SoCs.

> +		if (route->csi == RVIN_CSI20 || route->csi == RVIN_CSI21)
> +			ifmd |= VNCSI_IFMD_DES1;
> +		else
> +			ifmd |= VNCSI_IFMD_DES0;
>  
> -	rvin_write(vin, ifmd, VNCSI_IFMD_REG);
> +		/* If both have been set stop looping. */
> +		if (ifmd == (VNCSI_IFMD_DES0 | VNCSI_IFMD_DES1))
> +			break;

I would remove the comment here as I think the code is quiet self 
explanatory and reading the comment made me think I was missing 
something obvious ;-)

> +	}

Missing blank line.

With these small nits fixed,

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> +	if (ifmd) {
> +		ifmd |= VNCSI_IFMD_CSI_CHSEL(chsel);
> +		rvin_write(vin, ifmd, VNCSI_IFMD_REG);
> +	}
>  
>  	vin_dbg(vin, "Set IFMD 0x%x\n", ifmd);
>  
> -- 
> 2.29.1
>
Jacopo Mondi Nov. 24, 2020, 9:40 p.m. UTC | #2 
Hi Niklas,

On Mon, Nov 23, 2020 at 04:31:24PM +0100, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> On 2020-11-16 12:04:28 +0100, Jacopo Mondi wrote:
> > The VNCSI_IFMD register controls the data expansion mode and the
> > channel routing between the CSI-2 receivers and VIN instances.
> >
> > According to the chip manual revision 2.20 not all fields are available
> > for all the SoCs:
> > - V3M, V3H and E3 do not support the DES1 field has they do not feature
> >   a CSI20 receiver.
> > - D3 only supports parallel input, and the whole register shall always
> >   be written as 0.
> >
> > Inspect the per-SoC channel routing table where the available CSI-2
> > instances are reported and configure VNCSI_IFMD accordingly.
> >
> > This patch upports the BSP change commit f54697394457
> > ("media: rcar-vin: Fix VnCSI_IFMD register access for r8a77990")
>
> I really like this approach, nice work.
>

I'm happy you like it, almost like you suggested it in first place,
right ? :)

> >
> > Suggested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-dma.c | 26 +++++++++++++++++++---
> >  1 file changed, 23 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > index 378514a75bc2..ab6818b34e5a 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -1570,7 +1570,9 @@ int rvin_dma_register(struct rvin_dev *vin, int irq)
> >   */
> >  int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel)
> >  {
> > -	u32 ifmd, vnmc;
> > +	const struct rvin_group_route *route;
> > +	u32 ifmd = 0;
> > +	u32 vnmc;
> >  	int ret;
> >
> >  	ret = pm_runtime_get_sync(vin->dev);
> > @@ -1583,9 +1585,27 @@ int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel)
> >  	vnmc = rvin_read(vin, VNMC_REG);
> >  	rvin_write(vin, vnmc & ~VNMC_VUP, VNMC_REG);
> >
> > -	ifmd = VNCSI_IFMD_DES1 | VNCSI_IFMD_DES0 | VNCSI_IFMD_CSI_CHSEL(chsel);
> > +	/*
> > +	 * Set data expansion mode to "pad with 0s" by inspecting the routes
> > +	 * table to find out which bit fields are available in the IFMD
> > +	 * register. IFMD_DES1 controls data expansion mode for CSI20/21,
> > +	 * IFMD_DES0 controls data expansion mode for CSI40/41.
> > +	 */
> > +	for (route = vin->info->routes; route->mask; route++) {
> > +		/* CSI21 is only available in r8a7795es1 and not documented. */
>
> I would drop this comment as we do not know what will happen in future
> SoCs.
>
> > +		if (route->csi == RVIN_CSI20 || route->csi == RVIN_CSI21)
> > +			ifmd |= VNCSI_IFMD_DES1;
> > +		else
> > +			ifmd |= VNCSI_IFMD_DES0;
> >
> > -	rvin_write(vin, ifmd, VNCSI_IFMD_REG);
> > +		/* If both have been set stop looping. */
> > +		if (ifmd == (VNCSI_IFMD_DES0 | VNCSI_IFMD_DES1))
> > +			break;
>
> I would remove the comment here as I think the code is quiet self
> explanatory and reading the comment made me think I was missing
> something obvious ;-)
>

Correct, I'm breaking the first rule of avoiding superfluous comments.

> > +	}
>
> Missing blank line.

Was kind of intentional, but I can add one

>
> With these small nits fixed,
>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Thanks
  j

>
> > +	if (ifmd) {
> > +		ifmd |= VNCSI_IFMD_CSI_CHSEL(chsel);
> > +		rvin_write(vin, ifmd, VNCSI_IFMD_REG);
> > +	}
> >
> >  	vin_dbg(vin, "Set IFMD 0x%x\n", ifmd);
> >
> > --
> > 2.29.1
> >
>
> --
> Regards,
> Niklas Söderlund
diff mbox series

Patch

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index 378514a75bc2..ab6818b34e5a 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -1570,7 +1570,9 @@  int rvin_dma_register(struct rvin_dev *vin, int irq)
  */
 int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel)
 {
-	u32 ifmd, vnmc;
+	const struct rvin_group_route *route;
+	u32 ifmd = 0;
+	u32 vnmc;
 	int ret;
 
 	ret = pm_runtime_get_sync(vin->dev);
@@ -1583,9 +1585,27 @@  int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel)
 	vnmc = rvin_read(vin, VNMC_REG);
 	rvin_write(vin, vnmc & ~VNMC_VUP, VNMC_REG);
 
-	ifmd = VNCSI_IFMD_DES1 | VNCSI_IFMD_DES0 | VNCSI_IFMD_CSI_CHSEL(chsel);
+	/*
+	 * Set data expansion mode to "pad with 0s" by inspecting the routes
+	 * table to find out which bit fields are available in the IFMD
+	 * register. IFMD_DES1 controls data expansion mode for CSI20/21,
+	 * IFMD_DES0 controls data expansion mode for CSI40/41.
+	 */
+	for (route = vin->info->routes; route->mask; route++) {
+		/* CSI21 is only available in r8a7795es1 and not documented. */
+		if (route->csi == RVIN_CSI20 || route->csi == RVIN_CSI21)
+			ifmd |= VNCSI_IFMD_DES1;
+		else
+			ifmd |= VNCSI_IFMD_DES0;
 
-	rvin_write(vin, ifmd, VNCSI_IFMD_REG);
+		/* If both have been set stop looping. */
+		if (ifmd == (VNCSI_IFMD_DES0 | VNCSI_IFMD_DES1))
+			break;
+	}
+	if (ifmd) {
+		ifmd |= VNCSI_IFMD_CSI_CHSEL(chsel);
+		rvin_write(vin, ifmd, VNCSI_IFMD_REG);
+	}
 
 	vin_dbg(vin, "Set IFMD 0x%x\n", ifmd);