| Message ID | 20200908085121.864-4-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve V4L2 fwnode framework usability and documentation | expand |
The subject should have been: "[PATCH 3/3] v4l2-fwnode: Document new usage patterns of v4l2_fwnode_endpoint_parse".
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. > *
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.
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 >
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. *
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(-)