[PATCH 0/2] media: v4l: fwnode: Improve line-orders parsing

[PATCH 0/2] media: v4l: fwnode: Improve line-orders parsing

[Niklas Söderlund]

Hello Sakari, Geert,

As discussed in the series which added line-orders support [1] the 
method which line-orders are parsed differers from how bus-type is 
parsed. The line-order parsing skipped an intermediary step which aids 
in separating the values used in device tree sources and the values used 
by drivers.

This was done to keep the line-order simple and based on the fact that 
compared to bus-types we are unlikely to get any more line order 
settings, at least not for CSI-2 C-PHY. But as Geert pointed it out this 
series addresses this and moves the line-orders parsing to use the same 
intermediary step as bus-type. We also gain an enum for the line orders 
at the fwnode level which may or may not be use-full down the line which 
we previously did not have.

As an added bonus the parsing code is a tad easier to read as the 
parsing function is already quiet large and the new helper functions to 
match how bus-type is parsed reduces that a bit.

Patch 1/2 adds the missing intermediary enum, while patch 2/2 breaks out 
the line-order parsing into helper functions and a struct to record the 
information. All of this mimics the pattern used for bus-type pattern.

There is no intentional functional change in this series and it have 
been tested on Renesas devices.

1. [PATCH v2 0/4] media: v4l: fwnode: Add support for CSI-2 C-PHY line orders

Niklas Söderlund (2):
  media: v4l: fwnode: Add definitions for CSI-2 C-PHY line-orders
  media: v4l: fwnode: Parse CSI-2 C-PHY line-orders like bus-type

 drivers/media/v4l2-core/v4l2-fwnode.c | 80 ++++++++++++++++++++++-----
 include/media/v4l2-fwnode.h           | 18 ++++++
 2 files changed, 84 insertions(+), 14 deletions(-)

-- 
2.47.1

[PATCH 1/2] media: v4l: fwnode: Add definitions for CSI-2 C-PHY line-orders

[Niklas Söderlund]

Add definitions to match the ones in video-interfaces.h for the C-PHY
line orders defined in MIPI Discovery and Configuration (DisCo)
Specification for Imaging.

This provides a way smiler to what is done for media bus types which
also have named defines in video-interfaces.h that benefit from having
human readable defines in code and not just in device tree sources.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 include/media/v4l2-fwnode.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
index f7c57c776589..fda5d740ede5 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -180,6 +180,24 @@ enum v4l2_fwnode_bus_type {
 	NR_OF_V4L2_FWNODE_BUS_TYPE
 };
 
+/**
+ * enum v4l2_fwnode_csi2_cphy_line_orders_type - CSI-2 C-PHY line order
+ * @V4L2_FWNODE_CSI2_CPHY_LINE_ORDER_ABC: C-PHY line order ABC
+ * @V4L2_FWNODE_CSI2_CPHY_LINE_ORDER_ACB: C-PHY line order ACB
+ * @V4L2_FWNODE_CSI2_CPHY_LINE_ORDER_BAC: C-PHY line order BAC
+ * @V4L2_FWNODE_CSI2_CPHY_LINE_ORDER_BCA: C-PHY line order BCA
+ * @V4L2_FWNODE_CSI2_CPHY_LINE_ORDER_CAB: C-PHY line order CAB
+ * @V4L2_FWNODE_CSI2_CPHY_LINE_ORDER_CBA: C-PHY line order CBA
+ */
+enum v4l2_fwnode_csi2_cphy_line_orders_type {
+	V4L2_FWNODE_CSI2_CPHY_LINE_ORDER_ABC = 0,
+	V4L2_FWNODE_CSI2_CPHY_LINE_ORDER_ACB,
+	V4L2_FWNODE_CSI2_CPHY_LINE_ORDER_BAC,
+	V4L2_FWNODE_CSI2_CPHY_LINE_ORDER_BCA,
+	V4L2_FWNODE_CSI2_CPHY_LINE_ORDER_CAB,
+	V4L2_FWNODE_CSI2_CPHY_LINE_ORDER_CBA
+};
+
 /**
  * v4l2_fwnode_endpoint_parse() - parse all fwnode node properties
  * @fwnode: pointer to the endpoint's fwnode handle
-- 
2.47.1

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

[Niklas Söderlund]

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.

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

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

[Sakari Ailus]

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?

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

> 
> 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

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

[Niklas Söderlund]

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

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

[Geert Uytterhoeven]

Hi Niklas,

On Tue, Jan 7, 2025 at 10:52 AM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> On 2025-01-07 08:36:31 +0000, Sakari Ailus wrote:
> > 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.

If you decide to keep the V4L2_MBUS_* definitions, there are other
(simpler) alternatives than adding explicit translation code:

    enum v4l2_mbus_csi2_cphy_line_orders_type {
            V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ABC =
MEDIA_BUS_CSI2_CPHY_LINE_ORDER_ABC,
            V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ACB =
MEDIA_BUS_CSI2_CPHY_LINE_ORDER_ACB,
            ...
> +};

or

    BUILD_BUG_ON(V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ABC !=
MEDIA_BUS_CSI2_CPHY_LINE_ORDER_ABC);
    BUILD_BUG_ON(V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ACB !=
MEDIA_BUS_CSI2_CPHY_LINE_ORDER_ACB);
    ...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Sakari Ailus]

Hejssan,

On Tue, Jan 07, 2025 at 10:52:19AM +0100, Niklas Söderlund wrote:
> 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.

I wasn't asking dropping the series, but instead get rid of the V4L2_MBUS_
line order definitions altogether, by replacing them by V4L2_FWNODE_
equivalents.

> 
> 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?

I think that would be reasonable. I don't think we need two sets of
definitions that effectively are interchangeable. But that may well be out
of scope of this series.

-- 
Med vänliga hälsningar,

Sakari Ailus