Re: [PATCH v9 22/28] rcar-vin: add chsel information to rvin_info

Laurent Pinchart
2017-12-08T20:37:34Z

Hi Niklas,

Thank you for the patch.

On Friday, 8 December 2017 03:08:36 EET Niklas Söderlund wrote:
> Each Gen3 SoC has a limited set of predefined routing possibilities for
> which CSI-2 device and virtual channel can be routed to which VIN
> instance. Prepare to store this information in the struct rvin_info.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> drivers/media/platform/rcar-vin/rcar-vin.h | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h
> b/drivers/media/platform/rcar-vin/rcar-vin.h index
> 5f736a3500b6e10f..41bf24aa8a1a0aed 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -35,6 +35,9 @@
> /* Max number on VIN instances that can be in a system */
> #define RCAR_VIN_NUM 8
>
> +/* Max number of CHSEL values for any Gen3 SoC */
> +#define RCAR_CHSEL_MAX 6
> +
> enum chip_id {
> RCAR_H1,
> RCAR_M1,
> @@ -91,6 +94,22 @@ struct rvin_graph_entity {
>
> struct rvin_group;
>
> +/** struct rvin_group_chsel - Map a CSI-2 receiver and channel to a CHSEL
> value
> + * @csi: VIN internal number for CSI-2 device
> + * @chan: Output channel of the CSI-2 receiver. Each R-Car CSI-2
> + * receiver has four output channels facing the VIN
> + * devices, each channel can carry one CSI-2 Virtual
> + * Channel (VC) and there are no correlation between
> + * output channel number and CSI-2 VC. It's up to the
> + * CSI-2 receiver driver to configure which VC is
> + * outputted on which channel, the VIN devices only
> + * cares about output channels.
> + */
> +struct rvin_group_chsel {
> + enum rvin_csi_id csi;
> + unsigned int chan;
> +};
> +
> /**
> * struct rvin_info - Information about the particular VIN implementation
> * @chip: type of VIN chip
> @@ -98,6 +117,9 @@ struct rvin_group;
> *
> * max_width: max input width the VIN supports
> * max_height: max input height the VIN supports
> + *
> + * num_chsels: number of possible chsel values for this VIN
> + * chsels: routing table VIN <-> CSI-2 for the chsel values
> */
> struct rvin_info {
> enum chip_id chip;
> @@ -105,6 +127,9 @@ struct rvin_info {
>
> unsigned int max_width;
> unsigned int max_height;
> +
> + unsigned int num_chsels;
> + struct rvin_group_chsel chsels[RCAR_VIN_NUM][RCAR_CHSEL_MAX];

That will result in a quite sparse array. I wonder whether we couldn't instead
have an array of num_chsels elements storing the CSI receiver ID, the CSI
receiver output channel, and the VIN ID.

Furthermore, shouldn't the CSI receiver ID be specified in DT using the
renesas,id property like we do for the VIN instances, instead of through the
endpoint number ? I know this will require a bit of refactoring, but I think
it would stimplify both the DT bindings and the code as we wouldn't hardcode a
fixed set of CSI receivers.

> };
>
> /**

--
Regards,

Laurent Pinchart