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@xxxxxxxxxxxx>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxx>
> + 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
