[PATCH 0/3] Improve V4L2 fwnode framework usability and documentation

[PATCH 0/3] Improve V4L2 fwnode framework usability and documentation

[Sakari Ailus]

Hi all,

This set makes it possible to set defaults for multiple bus types on a
single call by using a little bit more memory. The documentation is also
improved to allow getting the bus type from DT when bindings allow that.

Sakari Ailus (3):
  v4l2-fwnode: Make number of data lanes a character
  v4l2-fwnode: Make bus configuration a struct
  v4l2-fwnode: Document changes usage patterns of
    v4l2_fwnode_endpoint_parse

 include/media/v4l2-fwnode.h | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

-- 
2.27.0

[PATCH 1/3] v4l2-fwnode: Make number of data lanes a character

[Sakari Ailus]

The maximum is currently four (4). No short is needed.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 include/media/v4l2-fwnode.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
index c47b70636e42..81e7eb123294 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -40,7 +40,7 @@ struct v4l2_fwnode_bus_mipi_csi2 {
 	unsigned int flags;
 	unsigned char data_lanes[V4L2_FWNODE_CSI2_MAX_DATA_LANES];
 	unsigned char clock_lane;
-	unsigned short num_data_lanes;
+	unsigned char num_data_lanes;
 	bool lane_polarities[1 + V4L2_FWNODE_CSI2_MAX_DATA_LANES];
 };
 
-- 
2.27.0

[PATCH 2/3] v4l2-fwnode: Make bus configuration a struct

[Sakari Ailus]

The bus specific parameters were an union. This made providing bus
specific defaults impossible as the memory used to store the defaults for
multiple different busses was the same.

Make it struct instead. It's not large so the size isn't really an issue.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 include/media/v4l2-fwnode.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
index 81e7eb123294..d04f39b60096 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -78,7 +78,7 @@ struct v4l2_fwnode_bus_mipi_csi1 {
  * struct v4l2_fwnode_endpoint - the endpoint data structure
  * @base: fwnode endpoint of the v4l2_fwnode
  * @bus_type: bus type
- * @bus: union with bus configuration data structure
+ * @bus: bus configuration data structure
  * @bus.parallel: embedded &struct v4l2_fwnode_bus_parallel.
  *		  Used if the bus is parallel.
  * @bus.mipi_csi1: embedded &struct v4l2_fwnode_bus_mipi_csi1.
@@ -99,7 +99,7 @@ struct v4l2_fwnode_endpoint {
 	 * v4l2_fwnode_endpoint_parse()
 	 */
 	enum v4l2_mbus_type bus_type;
-	union {
+	struct {
 		struct v4l2_fwnode_bus_parallel parallel;
 		struct v4l2_fwnode_bus_mipi_csi1 mipi_csi1;
 		struct v4l2_fwnode_bus_mipi_csi2 mipi_csi2;
-- 
2.27.0

[PATCH 3/3] v4l2-fwnode: Document changes usage patterns of v4l2_fwnode_endpoint_parse

[Sakari Ailus]

Document that it is possible to provide defaults for multiple bus types to
v4l2_fwnode_endpoint_parse and v4l2_fwnode_endpoint_alloc_parse. Also
underline the fact that detecting the bus type without bus-type property
is only for the old drivers.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 include/media/v4l2-fwnode.h | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
index d04f39b60096..ccaa5693df14 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -226,11 +226,10 @@ struct v4l2_fwnode_connector {
  * call this function once the correct type is found --- with a default
  * configuration valid for that type.
  *
- * As a compatibility means guessing the bus type is also supported by setting
- * @vep.bus_type to V4L2_MBUS_UNKNOWN. The caller may not provide a default
- * configuration in this case as the defaults are specific to a given bus type.
- * This functionality is deprecated and should not be used in new drivers and it
- * is only supported for CSI-2 D-PHY, parallel and Bt.656 buses.
+ * It is also allowed to set @vep.bus_type to V4L2_MBUS_UNKNOWN. USING THIS
+ * FEATURE REQUIRES "bus-type" PROPERTY IN DT BINDINGS. For old drivers,
+ * guessing @vep.bus_type between CSI-2 D-PHY, parallel and Bt.656 busses is
+ * supported. NEVER RELY ON GUESSING @vep.bus_type IN NEW DRIVERS!
  *
  * The function does not change the V4L2 fwnode endpoint state if it fails.
  *
@@ -269,11 +268,10 @@ void v4l2_fwnode_endpoint_free(struct v4l2_fwnode_endpoint *vep);
  * call this function once the correct type is found --- with a default
  * configuration valid for that type.
  *
- * As a compatibility means guessing the bus type is also supported by setting
- * @vep.bus_type to V4L2_MBUS_UNKNOWN. The caller may not provide a default
- * configuration in this case as the defaults are specific to a given bus type.
- * This functionality is deprecated and should not be used in new drivers and it
- * is only supported for CSI-2 D-PHY, parallel and Bt.656 buses.
+ * It is also allowed to set @vep.bus_type to V4L2_MBUS_UNKNOWN. USING THIS
+ * FEATURE REQUIRES "bus-type" PROPERTY IN DT BINDINGS. For old drivers,
+ * guessing @vep.bus_type between CSI-2 D-PHY, parallel and Bt.656 busses is
+ * supported. NEVER RELY ON GUESSING @vep.bus_type IN NEW DRIVERS!
  *
  * The function does not change the V4L2 fwnode endpoint state if it fails.
  *
-- 
2.27.0

Re: [PATCH 3/3] v4l2-fwnode: Document changes usage patterns of v4l2_fwnode_endpoint_parse

[Sakari Ailus]

The subject should have been: "[PATCH 3/3] v4l2-fwnode: Document new usage
patterns of v4l2_fwnode_endpoint_parse".

-- 
Sakari Ailus

Re: [PATCH 1/3] v4l2-fwnode: Make number of data lanes a character

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Tue, Sep 08, 2020 at 11:51:19AM +0300, Sakari Ailus wrote:
> The maximum is currently four (4). No short is needed.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  include/media/v4l2-fwnode.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> index c47b70636e42..81e7eb123294 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -40,7 +40,7 @@ struct v4l2_fwnode_bus_mipi_csi2 {
>  	unsigned int flags;
>  	unsigned char data_lanes[V4L2_FWNODE_CSI2_MAX_DATA_LANES];
>  	unsigned char clock_lane;
> -	unsigned short num_data_lanes;
> +	unsigned char num_data_lanes;
>  	bool lane_polarities[1 + V4L2_FWNODE_CSI2_MAX_DATA_LANES];

That's a 4 bytes gain, it's useful.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  };

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] v4l2-fwnode: Make bus configuration a struct

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Tue, Sep 08, 2020 at 11:51:20AM +0300, Sakari Ailus wrote:
> The bus specific parameters were an union. This made providing bus
> specific defaults impossible as the memory used to store the defaults for
> multiple different busses was the same.
> 
> Make it struct instead. It's not large so the size isn't really an issue.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  include/media/v4l2-fwnode.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> index 81e7eb123294..d04f39b60096 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -78,7 +78,7 @@ struct v4l2_fwnode_bus_mipi_csi1 {
>   * struct v4l2_fwnode_endpoint - the endpoint data structure
>   * @base: fwnode endpoint of the v4l2_fwnode
>   * @bus_type: bus type
> - * @bus: union with bus configuration data structure
> + * @bus: bus configuration data structure
>   * @bus.parallel: embedded &struct v4l2_fwnode_bus_parallel.
>   *		  Used if the bus is parallel.
>   * @bus.mipi_csi1: embedded &struct v4l2_fwnode_bus_mipi_csi1.
> @@ -99,7 +99,7 @@ struct v4l2_fwnode_endpoint {
>  	 * v4l2_fwnode_endpoint_parse()
>  	 */
>  	enum v4l2_mbus_type bus_type;
> -	union {
> +	struct {
>  		struct v4l2_fwnode_bus_parallel parallel;
>  		struct v4l2_fwnode_bus_mipi_csi1 mipi_csi1;
>  		struct v4l2_fwnode_bus_mipi_csi2 mipi_csi2;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/3] v4l2-fwnode: Document changes usage patterns of v4l2_fwnode_endpoint_parse

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Tue, Sep 08, 2020 at 11:51:21AM +0300, Sakari Ailus wrote:
> Document that it is possible to provide defaults for multiple bus types to
> v4l2_fwnode_endpoint_parse and v4l2_fwnode_endpoint_alloc_parse. Also
> underline the fact that detecting the bus type without bus-type property
> is only for the old drivers.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  include/media/v4l2-fwnode.h | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> index d04f39b60096..ccaa5693df14 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -226,11 +226,10 @@ struct v4l2_fwnode_connector {
>   * call this function once the correct type is found --- with a default
>   * configuration valid for that type.
>   *
> - * As a compatibility means guessing the bus type is also supported by setting
> - * @vep.bus_type to V4L2_MBUS_UNKNOWN. The caller may not provide a default
> - * configuration in this case as the defaults are specific to a given bus type.
> - * This functionality is deprecated and should not be used in new drivers and it
> - * is only supported for CSI-2 D-PHY, parallel and Bt.656 buses.
> + * It is also allowed to set @vep.bus_type to V4L2_MBUS_UNKNOWN. USING THIS
> + * FEATURE REQUIRES "bus-type" PROPERTY IN DT BINDINGS. For old drivers,
> + * guessing @vep.bus_type between CSI-2 D-PHY, parallel and Bt.656 busses is

While at it, s/Bt/BT/.

> + * supported. NEVER RELY ON GUESSING @vep.bus_type IN NEW DRIVERS!

That's fairly clear :-)

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I wonder if, as a further improvement, we should turn the enum
v4l2_mbus_type into a bitfield, to let drivers tell which bus types they
support. The helpers could then return an error if the bus type reported
by the firmware doesn't match.

>   *
>   * The function does not change the V4L2 fwnode endpoint state if it fails.
>   *
> @@ -269,11 +268,10 @@ void v4l2_fwnode_endpoint_free(struct v4l2_fwnode_endpoint *vep);
>   * call this function once the correct type is found --- with a default
>   * configuration valid for that type.
>   *
> - * As a compatibility means guessing the bus type is also supported by setting
> - * @vep.bus_type to V4L2_MBUS_UNKNOWN. The caller may not provide a default
> - * configuration in this case as the defaults are specific to a given bus type.
> - * This functionality is deprecated and should not be used in new drivers and it
> - * is only supported for CSI-2 D-PHY, parallel and Bt.656 buses.
> + * It is also allowed to set @vep.bus_type to V4L2_MBUS_UNKNOWN. USING THIS
> + * FEATURE REQUIRES "bus-type" PROPERTY IN DT BINDINGS. For old drivers,
> + * guessing @vep.bus_type between CSI-2 D-PHY, parallel and Bt.656 busses is
> + * supported. NEVER RELY ON GUESSING @vep.bus_type IN NEW DRIVERS!
>   *
>   * The function does not change the V4L2 fwnode endpoint state if it fails.
>   *

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/3] v4l2-fwnode: Document changes usage patterns of v4l2_fwnode_endpoint_parse

[Sakari Ailus]

Hi Laurent,

On Tue, Sep 08, 2020 at 03:51:07PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.

Thank you for the review!

> 
> On Tue, Sep 08, 2020 at 11:51:21AM +0300, Sakari Ailus wrote:
> > Document that it is possible to provide defaults for multiple bus types to
> > v4l2_fwnode_endpoint_parse and v4l2_fwnode_endpoint_alloc_parse. Also
> > underline the fact that detecting the bus type without bus-type property
> > is only for the old drivers.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  include/media/v4l2-fwnode.h | 18 ++++++++----------
> >  1 file changed, 8 insertions(+), 10 deletions(-)
> > 
> > diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> > index d04f39b60096..ccaa5693df14 100644
> > --- a/include/media/v4l2-fwnode.h
> > +++ b/include/media/v4l2-fwnode.h
> > @@ -226,11 +226,10 @@ struct v4l2_fwnode_connector {
> >   * call this function once the correct type is found --- with a default
> >   * configuration valid for that type.
> >   *
> > - * As a compatibility means guessing the bus type is also supported by setting
> > - * @vep.bus_type to V4L2_MBUS_UNKNOWN. The caller may not provide a default
> > - * configuration in this case as the defaults are specific to a given bus type.
> > - * This functionality is deprecated and should not be used in new drivers and it
> > - * is only supported for CSI-2 D-PHY, parallel and Bt.656 buses.
> > + * It is also allowed to set @vep.bus_type to V4L2_MBUS_UNKNOWN. USING THIS
> > + * FEATURE REQUIRES "bus-type" PROPERTY IN DT BINDINGS. For old drivers,
> > + * guessing @vep.bus_type between CSI-2 D-PHY, parallel and Bt.656 busses is
> 
> While at it, s/Bt/BT/.

Yes.

> 
> > + * supported. NEVER RELY ON GUESSING @vep.bus_type IN NEW DRIVERS!
> 
> That's fairly clear :-)
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> I wonder if, as a further improvement, we should turn the enum
> v4l2_mbus_type into a bitfield, to let drivers tell which bus types they
> support. The helpers could then return an error if the bus type reported
> by the firmware doesn't match.

I agree. That'd be less work for the caller. That does require changing a
bunch of drivers though. Unless we add another field for that purpose, just
like the I²C framework did. I guess it's not necessary. Ideally it'd be
done so that trying to use it the old way would generate a compiler
warning.

-- 
Regards,

Sakari Ailus

Re: [PATCH 3/3] v4l2-fwnode: Document changes usage patterns of v4l2_fwnode_endpoint_parse

[Jacopo Mondi]

Hi Sakari,

On Tue, Sep 08, 2020 at 11:51:21AM +0300, Sakari Ailus wrote:
> Document that it is possible to provide defaults for multiple bus types to
> v4l2_fwnode_endpoint_parse and v4l2_fwnode_endpoint_alloc_parse. Also
> underline the fact that detecting the bus type without bus-type property
> is only for the old drivers.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  include/media/v4l2-fwnode.h | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> index d04f39b60096..ccaa5693df14 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -226,11 +226,10 @@ struct v4l2_fwnode_connector {
>   * call this function once the correct type is found --- with a default
>   * configuration valid for that type.
>   *
> - * As a compatibility means guessing the bus type is also supported by setting
> - * @vep.bus_type to V4L2_MBUS_UNKNOWN. The caller may not provide a default
> - * configuration in this case as the defaults are specific to a given bus type.
> - * This functionality is deprecated and should not be used in new drivers and it
> - * is only supported for CSI-2 D-PHY, parallel and Bt.656 buses.
> + * It is also allowed to set @vep.bus_type to V4L2_MBUS_UNKNOWN. USING THIS
> + * FEATURE REQUIRES "bus-type" PROPERTY IN DT BINDINGS. For old drivers,
> + * guessing @vep.bus_type between CSI-2 D-PHY, parallel and Bt.656 busses is
> + * supported. NEVER RELY ON GUESSING @vep.bus_type IN NEW DRIVERS!

As per the discussion in:
[PATCH v4 1/3] media: i2c: ov772x: Parse endpoint properties
this leaves a bit of gray area on how to address cases where older dts
do not report any "bus-type" property, as in the case of PARALLEL vs
BT.656 Prabhakar has, it gets impossible to detect mis-configurations.

I have suggested him two different behaviours, depending if 'bus-type'
is present in the fwnode or not, but I'm not sure that's the best way
forward. What do you think ?

>   *
>   * The function does not change the V4L2 fwnode endpoint state if it fails.
>   *
> @@ -269,11 +268,10 @@ void v4l2_fwnode_endpoint_free(struct v4l2_fwnode_endpoint *vep);
>   * call this function once the correct type is found --- with a default
>   * configuration valid for that type.
>   *
> - * As a compatibility means guessing the bus type is also supported by setting
> - * @vep.bus_type to V4L2_MBUS_UNKNOWN. The caller may not provide a default
> - * configuration in this case as the defaults are specific to a given bus type.
> - * This functionality is deprecated and should not be used in new drivers and it
> - * is only supported for CSI-2 D-PHY, parallel and Bt.656 buses.
> + * It is also allowed to set @vep.bus_type to V4L2_MBUS_UNKNOWN. USING THIS
> + * FEATURE REQUIRES "bus-type" PROPERTY IN DT BINDINGS. For old drivers,
> + * guessing @vep.bus_type between CSI-2 D-PHY, parallel and Bt.656 busses is
> + * supported. NEVER RELY ON GUESSING @vep.bus_type IN NEW DRIVERS!
>   *
>   * The function does not change the V4L2 fwnode endpoint state if it fails.
>   *
> --
> 2.27.0
>