Hi Niklas,

On Wed, Mar 20, 2019 at 08:50:15PM +0100, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your patch.
>
> On 2019-03-16 16:48:01 +0100, Jacopo Mondi wrote:
> > Use the D-PHY configuration reported by the remote subdevice in its
> > frame descriptor to configure the interface.
> >
> > Store the number of lanes reported through the 'data-lanes' DT property
> > as the number of phyisically available lanes, which might not correspond
> > to the number of lanes actually in use.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 71 +++++++++++++--------
> >  1 file changed, 43 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index 6c46bcc0ee83..70b9a8165a6e 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -375,8 +375,8 @@ struct rcar_csi2 {
> >  	struct mutex lock;
> >  	int stream_count;
> >
> > -	unsigned short lanes;
> > -	unsigned char lane_swap[4];
> > +	unsigned short available_data_lanes;
> > +	unsigned short num_data_lanes;
>
> I don't like this, I'm sorry :-(
>
> I think you should keep lanes and lane_swap and most code touching them
> intact. And drop num_data_lanes as it's only used when starting and only
> valid for one 'session' and should not be cached in the data structure
> unnecessary.
>
> Furthermore I think involving lane swapping in the runtime configurable
> parameters is a bad idea. Or do you see a scenario where lanes could be
> swapped in runtime? In my view DT should describe which physical lanes
> are connected and how. And the runtime configuration part should only
> deal with how many lanes are used for the particular capture session.
>

Yeah, it's dumb, it was noted in the review of the framework
side as well..

I'll drop anything related to lane re-ordering, so I should not need
to touch 'lanes' and 'lane_swap'. I need 'num_data_lanes' in 'struct
rcar_csi2' though, as it is used in a few function called by
rcsi2_start().

Thanks
  j

> >  };
> >
> >  static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd)
> > @@ -424,7 +424,7 @@ static int rcsi2_get_remote_frame_desc(struct rcar_csi2 *priv,
> >  	if (ret)
> >  		return -ENODEV;
> >
> > -	if (fd->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) {
> > +	if (fd->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2_DPHY) {
> >  		dev_err(priv->dev, "Frame desc do not describe CSI-2 link");
> >  		return -EINVAL;
> >  	}
> > @@ -438,7 +438,7 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
> >
> >  	/* Wait for the clock and data lanes to enter LP-11 state. */
> >  	for (timeout = 0; timeout <= 20; timeout++) {
> > -		const u32 lane_mask = (1 << priv->lanes) - 1;
> > +		const u32 lane_mask = (1 << priv->num_data_lanes) - 1;
> >
> >  		if ((rcsi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATECKL)  &&
> >  		    (rcsi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask)
> > @@ -511,14 +511,15 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv,
> >  	 * bps = link_freq * 2
> >  	 */
> >  	mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp;
> > -	do_div(mbps, priv->lanes * 1000000);
> > +	do_div(mbps, priv->num_data_lanes * 1000000);
> >
> >  	return mbps;
> >  }
> >
> >  static int rcsi2_start(struct rcar_csi2 *priv)
> >  {
> > -	u32 phycnt, vcdt = 0, vcdt2 = 0;
> > +	struct v4l2_mbus_frame_desc_entry_csi2_dphy *dphy;
> > +	u32 phycnt, vcdt = 0, vcdt2 = 0, lswap = 0;
> >  	struct v4l2_mbus_frame_desc fd;
> >  	unsigned int i;
> >  	int mbps, ret;
> > @@ -548,8 +549,26 @@ static int rcsi2_start(struct rcar_csi2 *priv)
> >  			entry->bus.csi2.channel, entry->bus.csi2.data_type);
> >  	}
> >
> > +	/* Get description of the D-PHY media bus configuration. */
> > +	dphy = &fd.phy.csi2_dphy;
> > +	if (dphy->clock_lane != 0) {
> > +		dev_err(priv->dev,
> > +			"CSI-2 configuration not supported: clock at index %u",
> > +			dphy->clock_lane);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (dphy->num_data_lanes > priv->available_data_lanes ||
> > +	    dphy->num_data_lanes == 3) {
> > +		dev_err(priv->dev,
> > +			"Number of CSI-2 data lanes not supported: %u",
> > +			dphy->num_data_lanes);
> > +		return -EINVAL;
> > +	}
> > +	priv->num_data_lanes = dphy->num_data_lanes;
> > +
> >  	phycnt = PHYCNT_ENABLECLK;
> > -	phycnt |= (1 << priv->lanes) - 1;
> > +	phycnt |= (1 << priv->num_data_lanes) - 1;
> >
> >  	mbps = rcsi2_calc_mbps(priv, &fd);
> >  	if (mbps < 0)
> > @@ -566,12 +585,11 @@ static int rcsi2_start(struct rcar_csi2 *priv)
> >  	rcsi2_write(priv, VCDT_REG, vcdt);
> >  	if (vcdt2)
> >  		rcsi2_write(priv, VCDT2_REG, vcdt2);
> > +
> >  	/* Lanes are zero indexed. */
> > -	rcsi2_write(priv, LSWAP_REG,
> > -		    LSWAP_L0SEL(priv->lane_swap[0] - 1) |
> > -		    LSWAP_L1SEL(priv->lane_swap[1] - 1) |
> > -		    LSWAP_L2SEL(priv->lane_swap[2] - 1) |
> > -		    LSWAP_L3SEL(priv->lane_swap[3] - 1));
> > +	for (i = 0; i < priv->num_data_lanes; ++i)
> > +		lswap |= (dphy->data_lanes[i] - 1) << (i * 2);
> > +	rcsi2_write(priv, LSWAP_REG, lswap);
> >
> >  	/* Start */
> >  	if (priv->info->init_phtw) {
> > @@ -822,7 +840,7 @@ static const struct v4l2_async_notifier_operations rcar_csi2_notify_ops = {
> >  static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
> >  			    struct v4l2_fwnode_endpoint *vep)
> >  {
> > -	unsigned int i;
> > +	unsigned int num_data_lanes;
> >
> >  	/* Only port 0 endpoint 0 is valid. */
> >  	if (vep->base.port || vep->base.id)
> > @@ -833,24 +851,21 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
> >  		return -EINVAL;
> >  	}
> >
> > -	priv->lanes = vep->bus.mipi_csi2.num_data_lanes;
> > -	if (priv->lanes != 1 && priv->lanes != 2 && priv->lanes != 4) {
> > +	num_data_lanes = vep->bus.mipi_csi2.num_data_lanes;
> > +	switch (num_data_lanes) {
> > +	case 1:
> > +		/* fallthrough */
> > +	case 2:
> > +		/* fallthrough */
> > +	case 4:
> > +		priv->available_data_lanes = num_data_lanes;
> > +		break;
> > +	default:
>
> Nit pick, I don't like this switch statement and would prefer the
> original construct with an if.
>
> >  		dev_err(priv->dev, "Unsupported number of data-lanes: %u\n",
> > -			priv->lanes);
> > +			num_data_lanes);
> >  		return -EINVAL;
> >  	}
> >
> > -	for (i = 0; i < ARRAY_SIZE(priv->lane_swap); i++) {
> > -		priv->lane_swap[i] = i < priv->lanes ?
> > -			vep->bus.mipi_csi2.data_lanes[i] : i;
> > -
> > -		/* Check for valid lane number. */
> > -		if (priv->lane_swap[i] < 1 || priv->lane_swap[i] > 4) {
> > -			dev_err(priv->dev, "data-lanes must be in 1-4 range\n");
> > -			return -EINVAL;
> > -		}
> > -	}
> > -
> >  	return 0;
> >  }
> >
> > @@ -1235,7 +1250,7 @@ static int rcsi2_probe(struct platform_device *pdev)
> >  	if (ret < 0)
> >  		goto error;
> >
> > -	dev_info(priv->dev, "%d lanes found\n", priv->lanes);
> > +	dev_info(priv->dev, "%d lanes found\n", priv->available_data_lanes);
> >
> >  	return 0;
> >
> > --
> > 2.21.0
> >
>
> --
> Regards,
> Niklas Söderlund