Re: [PATCH 2/2] media: v4l: fwnode: Parse CSI-2 C-PHY line-orders like bus-type

["Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>]

Hi Sakari,

Tack för din feedback.

On 2025-01-07 08:36:31 +0000, Sakari Ailus wrote:
> Hejssan Niklas,
> 
> Tack för dessa lappar!
> 
> On Sat, Jan 04, 2025 at 08:55:48PM +0100, Niklas Söderlund wrote:
> > Provided a safe-guard from the raw values used in device tree sources
> > and the in-kernel defines used to describe the different line orders.
> > This mimics what have been done for the bus-type property to provide the
> > same safe-guard.
> > 
> > The macros used in device tree sources are defined in video-interfaces.h
> > (MEDIA_BUS_CSI2_CPHY_LINE_ORDER_*) and are only visible to DTS source
> > files. These raw values map directly to the in-kernel names by fwnode
> > defines in v4l2-fwnode.h (V4L2_FWNODE_CSI2_CPHY_LINE_ORDER_*). These
> > fwnode defines are finally translated to defines which are exposed to
> > drivers to act on (V4L2_MBUS_CSI2_CPHY_LINE_ORDER_*).
> > 
> > Previously the translation to values provided to drivers have exploited
> > the fact that the numerical value for each setting are the same for the
> > defines used in device tree sources. While this is unlikely to change
> > this harmonises the bus-type and line-orders parsing to work using the
> > same mechanics, while at the same time make the large CSI-2 parsing
> > function a little more readable.
> 
> Do we in fact need the V4L2_MBUS_ definitions of the line orders at all?

I'm not sure :-)

Geert pointed out in [1] that in comparison to the V4L2_MBUS_ bus-type 
definitions the line-order definitions did not have this intermediary 
step as a safe guard between values used in DTS files and values used in 
V4L2 drivers.

Looking at the original functionality,

    bus->line_orders[i] = array[i];

Seems a bit "hack" compared to what this patch do,


    bus->line_orders[i] = v4l2_fwnode_line_order_to_mbus(array[i]);

But if it's worth the extra churn, and if it in reality provides us with 
a safe-guard between DTS-files and V4L2-drivers I'm not sure. I'm on the 
fence on this one, the one good thing is that it aligns how V4L2_MBUS_ 
macros are parsed.

But if you don't like it and I'm on the fence I'm happy to drop this 
series. This series don't add any extra functionality.

1. CAMuHMdXwqb7vhUeoMKDDJO5dp-V3LmnURZLSC1_ko=YL=cNyUA@mail.gmail.com

> 
> The same could extend to the V4L2_MBUS_ bus type defitions, but that's out
> of scope of this patch.

Out of scope indeed. If we drop this series do we want to try and remove 
them for V4L2_MBUS_ bus-type in future?

> 
> > 
> > Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/v4l2-core/v4l2-fwnode.c | 80 ++++++++++++++++++++++-----
> >  1 file changed, 66 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> > index cb153ce42c45..69f6d1df8c39 100644
> > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > @@ -121,6 +121,70 @@ v4l2_fwnode_mbus_type_to_string(enum v4l2_mbus_type type)
> >  	return conv ? conv->name : "not found";
> >  }
> >  
> > +static const struct v4l2_fwnode_csi2_cphy_line_orders_conv {
> > +	enum v4l2_fwnode_csi2_cphy_line_orders_type fwnode_order;
> > +	enum v4l2_mbus_csi2_cphy_line_orders_type mbus_order;
> > +	const char *name;
> > +} csi2_cphy_line_orders[] = {
> > +	{
> > +		V4L2_FWNODE_CSI2_CPHY_LINE_ORDER_ABC,
> > +		V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ABC,
> > +		"ABC",
> > +	}, {
> > +		V4L2_FWNODE_CSI2_CPHY_LINE_ORDER_ACB,
> > +		V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ACB,
> > +		"ACB",
> > +	}, {
> > +		V4L2_FWNODE_CSI2_CPHY_LINE_ORDER_BAC,
> > +		V4L2_MBUS_CSI2_CPHY_LINE_ORDER_BAC,
> > +		"BAC",
> > +	}, {
> > +		V4L2_FWNODE_CSI2_CPHY_LINE_ORDER_BCA,
> > +		V4L2_MBUS_CSI2_CPHY_LINE_ORDER_BCA,
> > +		"BCA",
> > +	}, {
> > +		V4L2_FWNODE_CSI2_CPHY_LINE_ORDER_CAB,
> > +		V4L2_MBUS_CSI2_CPHY_LINE_ORDER_CAB,
> > +		"CAB",
> > +	}, {
> > +		V4L2_FWNODE_CSI2_CPHY_LINE_ORDER_CBA,
> > +		V4L2_MBUS_CSI2_CPHY_LINE_ORDER_CBA,
> > +		"CBA",
> > +	}
> > +};
> > +
> > +static const struct v4l2_fwnode_csi2_cphy_line_orders_conv *
> > +get_v4l2_fwnode_line_order_conv_by_fwnode_order(enum v4l2_fwnode_csi2_cphy_line_orders_type order)
> > +{
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(csi2_cphy_line_orders); i++)
> > +		if (csi2_cphy_line_orders[i].fwnode_order == order)
> > +			return &csi2_cphy_line_orders[i];
> > +
> > +	/* The default line order is ABC */
> > +	pr_warn("invalid line-order assuming ABC (got %u)\n", order);
> > +	return &csi2_cphy_line_orders[0];
> > +}
> > +
> > +static enum v4l2_mbus_csi2_cphy_line_orders_type
> > +v4l2_fwnode_line_order_to_mbus(enum v4l2_fwnode_csi2_cphy_line_orders_type order)
> > +{
> > +	const struct v4l2_fwnode_csi2_cphy_line_orders_conv *conv =
> > +		get_v4l2_fwnode_line_order_conv_by_fwnode_order(order);
> > +
> > +	return conv->mbus_order;
> > +}
> > +
> > +static const char *
> > +v4l2_fwnode_line_order_to_string(enum v4l2_fwnode_csi2_cphy_line_orders_type order)
> > +{
> > +	const struct v4l2_fwnode_csi2_cphy_line_orders_conv *conv =
> > +		get_v4l2_fwnode_line_order_conv_by_fwnode_order(order);
> > +
> > +	return conv->name;
> > +}
> > +
> >  static int v4l2_fwnode_endpoint_parse_csi2_bus(struct fwnode_handle *fwnode,
> >  					       struct v4l2_fwnode_endpoint *vep,
> >  					       enum v4l2_mbus_type bus_type)
> > @@ -268,21 +332,9 @@ static int v4l2_fwnode_endpoint_parse_csi2_bus(struct fwnode_handle *fwnode,
> >  						       num_data_lanes);
> >  
> >  			for (i = 0; i < num_data_lanes; i++) {
> > -				static const char * const orders[] = {
> > -					"ABC", "ACB", "BAC", "BCA", "CAB", "CBA"
> > -				};
> > -
> > -				if (array[i] >= ARRAY_SIZE(orders)) {
> > -					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];
> > +				bus->line_orders[i] = v4l2_fwnode_line_order_to_mbus(array[i]);
> >  				pr_debug("lane %u line order %s", i,
> > -					 orders[array[i]]);
> > +					 v4l2_fwnode_line_order_to_string(array[i]));
> >  			}
> >  		} else {
> >  			for (i = 0; i < num_data_lanes; i++)
> > -- 
> > 2.47.1
> > 
> 
> -- 
> Med vänliga hälsningar,
> 
> Sakari Ailus

-- 
Kind Regards,
Niklas Söderlund