Re: [PATCH 1/3] i2c: adv748x: store number of CSI-2 lanes described in device tree

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Kieran,

On Tuesday, 18 September 2018 13:37:55 EEST Kieran Bingham wrote:
> On 18/09/18 11:28, Laurent Pinchart wrote:
> > On Tuesday, 18 September 2018 13:19:39 EEST Kieran Bingham wrote:
> >> On 18/09/18 02:45, Niklas Söderlund wrote:
> >>> The adv748x CSI-2 transmitters TXA and TXB can use different number of
> >>> lines to transmit data on. In order to be able configure the device
> >>> correctly this information need to be parsed from device tree and stored
> >>> in each TX private data structure.
> >>> 
> >>> TXA supports 1, 2 and 4 lanes while TXB supports 1 lane.
> >> 
> >> Am I right in assuming that it is the CSI device which specifies the
> >> number of lanes in their DT?
> > 
> > Do you mean the CSI-2 receiver ? Both the receiver and the transmitter
> > should specify the data lanes in their DT node.
> 
> Yes, I should have said CSI-2 receiver.
> 
> Aha - so *both* sides of the link have to specify the lanes and
> presumably match with each other?

Yes, they should certainly match :-)

> >> Could we make this clear in the commit log (and possibly an extra
> >> comment in the code). At first I was assuming we would have to declare
> >> the number of lanes in the ADV748x TX DT node, but I don't think that's
> >> the case.
> >> 
> >>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >>> ---
> >>> 
> >>>  drivers/media/i2c/adv748x/adv748x-core.c | 49 ++++++++++++++++++++++++
> >>>  drivers/media/i2c/adv748x/adv748x.h      |  1 +
> >>>  2 files changed, 50 insertions(+)
> >>> 
> >>> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> >>> b/drivers/media/i2c/adv748x/adv748x-core.c index
> >>> 85c027bdcd56748d..a93f8ea89a228474 100644
> >>> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> >>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c

[snip]

> >>> +static int adv748x_parse_csi2_lanes(struct adv748x_state *state,
> >>> +				    unsigned int port,
> >>> +				    struct device_node *ep)
> >>> +{
> >>> +	struct v4l2_fwnode_endpoint vep;
> >>> +	unsigned int num_lanes;
> >>> +	int ret;
> >>> +
> >>> +	if (port != ADV748X_PORT_TXA && port != ADV748X_PORT_TXB)
> >>> +		return 0;
> >>> +
> >>> +	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &vep);
> >>> +	if (ret)
> >>> +		return ret;
> >>> +
> >>> +	num_lanes = vep.bus.mipi_csi2.num_data_lanes;
> >>> +
> >> 
> >> If I'm not mistaken we are parsing /someone elses/ DT node here (the CSI
> >> receiver or such).
> > 
> > Aren't we parsing our own endpoint ? The ep argument comes from ep_np in
> > adv748x_parse_dt(), and that's the endpoint iterator used with
> > 
> > 	for_each_endpoint_of_node(state->dev->of_node, ep_np)
> 
> Bah - my head was polluted with the async subdevice stuff where we were
> getting the endpoint of the other device, but of course that's
> completely unrelated here.
> 
> >> Is it now guaranteed on the mipi_csi2 bus to have the (correct) lanes
> >> defined?
> >> 
> >> Do we need to fall back to some safe defaults at all (1 lane?) ?
> >> Actually - perhaps there is no safe default. I guess if the lanes aren't
> >> configured correctly we're not going to get a good signal at the other
> >> end.
> > 
> > The endpoints should contain a data-lanes property. That's the case in the
> > mainline DT sources, but it's not explicitly stated as a mandatory
> > property. I think we should update the bindings.
> 
> Yes, - as this code change is making the property mandatory - we should
> certainly state that in the bindings, unless we can fall back to a
> sensible default (perhaps the max supported on that component?)

I'm not sure there's a sensible default, I'd rather specify it explicitly. 
Note that the data-lanes property doesn't just specify the number of lanes, 
but also how they are remapped, when that feature is supported by the CSI-2 
transmitter or receiver.

> >>> +	if (vep.base.port == ADV748X_PORT_TXA) {
> >>> +		if (num_lanes != 1 && num_lanes != 2 && num_lanes != 4) {
> >>> +			adv_err(state, "TXA: Invalid number (%d) of lanes\n",
> >>> +				num_lanes);
> >>> +			return -EINVAL;
> >>> +		}
> >>> +
> >>> +		state->txa.num_lanes = num_lanes;
> >>> +		adv_dbg(state, "TXA: using %d lanes\n", state->txa.num_lanes);
> >>> +	}
> >>> +
> >>> +	if (vep.base.port == ADV748X_PORT_TXB) {
> >>> +		if (num_lanes != 1) {
> >>> +			adv_err(state, "TXB: Invalid number (%d) of lanes\n",
> >>> +				num_lanes);
> >>> +			return -EINVAL;
> >>> +		}
> >>> +
> >>> +		state->txb.num_lanes = num_lanes;
> >>> +		adv_dbg(state, "TXB: using %d lanes\n", state->txb.num_lanes);
> >>> +	}
> >>> +
> >>> +	return 0;
> >>> +}

[snip]

-- 
Regards,

Laurent Pinchart