Re: [PATCH 2/4] media: v4l: fwnode: Parse MiPI DisCo for C-PHY line-orders

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hejssan, Niklas!

On Wed, Nov 20, 2024 at 10:50:30AM +0100, Niklas Söderlund wrote:
> Hello Sakari,
> 
> On 2024-11-20 08:10:42 +0000, Sakari Ailus wrote:
> > Hejssan,
> > 
> > On Tue, Nov 19, 2024 at 11:12:47PM +0100, Niklas Söderlund wrote:
> > > Extend the fwnode parsing to validate and fill in the CSI-2 C-PHY
> > > line-orders order properties as defined in MIPI Discovery and
> > > Configuration (DisCo) Specification for Imaging.
> > > 
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > ---
> > >  drivers/media/v4l2-core/v4l2-fwnode.c | 56 ++++++++++++++++++++++++++-
> > >  include/media/v4l2-mediabus.h         | 21 ++++++++++
> > >  2 files changed, 76 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> > > index f19c8adf2c61..b8b2b7fb685e 100644
> > > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > > @@ -127,7 +127,7 @@ static int v4l2_fwnode_endpoint_parse_csi2_bus(struct fwnode_handle *fwnode,
> > >  {
> > >  	struct v4l2_mbus_config_mipi_csi2 *bus = &vep->bus.mipi_csi2;
> > >  	bool have_clk_lane = false, have_data_lanes = false,
> > > -		have_lane_polarities = false;
> > > +		have_lane_polarities = false, have_line_orders = false;
> > >  	unsigned int flags = 0, lanes_used = 0;
> > >  	u32 array[1 + V4L2_MBUS_CSI2_MAX_DATA_LANES];
> > >  	u32 clock_lane = 0;
> > > @@ -197,6 +197,17 @@ static int v4l2_fwnode_endpoint_parse_csi2_bus(struct fwnode_handle *fwnode,
> > >  		have_lane_polarities = true;
> > >  	}
> > >  
> > > +	rval = fwnode_property_count_u32(fwnode, "line-orders");
> > > +	if (rval > 0) {
> > > +		if (rval != num_data_lanes) {
> > > +			pr_warn("invalid number of line-orders entries (need %u, got %u)\n",
> > > +				num_data_lanes, rval);
> > > +			return -EINVAL;
> > > +		}
> > > +
> > > +		have_line_orders = true;
> > > +	}
> > > +
> > >  	if (!fwnode_property_read_u32(fwnode, "clock-lanes", &v)) {
> > >  		clock_lane = v;
> > >  		pr_debug("clock lane position %u\n", v);
> > > @@ -250,6 +261,49 @@ static int v4l2_fwnode_endpoint_parse_csi2_bus(struct fwnode_handle *fwnode,
> > >  		} else {
> > >  			pr_debug("no lane polarities defined, assuming not inverted\n");
> > >  		}
> > > +
> > > +		if (have_line_orders) {
> > > +			fwnode_property_read_u32_array(fwnode,
> > > +						       "line-orders", array,
> > > +						       num_data_lanes);
> > > +
> > > +			for (i = 0; i < num_data_lanes; i++) {
> > > +				const char *order;
> > > +
> > > +				switch (array[i]) {
> > > +				case 0:
> > > +					order = "ABC";
> > > +					break;
> > > +				case 1:
> > > +					order = "ACB";
> > > +					break;
> > > +				case 2:
> > > +					order = "BAC";
> > > +					break;
> > > +				case 3:
> > > +					order = "BCA";
> > > +					break;
> > > +				case 4:
> > > +					order = "CAB";
> > > +					break;
> > > +				case 5:
> > > +					order = "CBA";
> > > +					break;
> > 
> > Please use an array instead.
> > 
> > > +				default:
> > > +					pr_warn("lane %u invalid line-order assuming ABC (got %u)\n",
> > > +						i, array[i]);
> > > +					bus->line_orders[i] = V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ABC;
> > > +					continue;
> > > +				}
> > > +				bus->line_orders[i] = array[i];
> > > +				pr_debug("lane %u line order %s", i, order);
> > > +			}
> > > +		} else {
> > > +			for (i = 0; i < num_data_lanes; i++)
> > > +				bus->line_orders[i] = V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ABC;
> > 
> > A few lines could be wrapped above.
> 
> I'm not sure I understand this comment. Do you mean I could loop over 
> num_data_lanes and initialize all lines to ABC before checking 
> have_line_orders and that way avoid having to loop here and set the 
> default ABC if we are out-of bounds in the switch?

No, just that you'd wrap lines that are over 80 characters per line, unless
there's some tangible reason to have them like that.

-- 
Med vänliga hälsningar,

Sakari Ailus