mbox series

[RFC,0/8] media: Drop .set_mbus_config(), improve .get_mbus_config()

Message ID 20220103162414.27723-1-laurent.pinchart+renesas@ideasonboard.com (mailing list archive)
Headers show
Series media: Drop .set_mbus_config(), improve .get_mbus_config() | expand

Message

Laurent Pinchart Jan. 3, 2022, 4:24 p.m. UTC
Hello,

This patch series reworks the V4L2 subdev .get_mbus_config() and
.set_mbus_config() operations to improve the former and drop the latter.

These subdev operations originate from soc-camera (for those who
remember the framework), and were designed to let a transmitter and a
receiver negotiate the physical configuration of the bus that connects
them. The operations use bitflags to represent bus parameters, with
supported options set by the caller of .set_mbus_config(), and selected
options among those returned by the callee. This mechanism is
deprecated, as selection of the bus configuration has long been moved to
the firmware interface (DT or ACPI), and usage of bitflags prevents from
adding more complex configuration parameters (timings in particular).

As .set_mbus_config() is deprecated and used by one pair of drivers only
(pxa_camera and ov6650), it wasn't difficult to drop usage of that
operation in patches 1/8 and 2/8, and remove the operation itself in
patch 3/8.

With that operation gone, .get_mbus_config() can be moved from bitflags
to structures. It turned out that the needed data structures were
already present in v4l2_fwnode.h. Patch 4/8 moves them to
v4l2_mediabus.h (and renames them to drop the fwnode mention, as they're
not specific to the fwnode API), and patch 5/8 makes use of them.
Patches 6/8 to 8/8 then removes media bus configuration bitflags that
are unneeded (and now unused).

The series is an RFC as not everything has been converted from bitflags
to named fields in structures. In particular, the parallel bus flags
haven't been touched at all. Patch 8/8 shows how mutually exclusive
flags can be reworked to drop one of them. We then need to decide
whether to keep expressing the flag as macros, or move to C bitfields
with dedicated structure member names. I didn't want to include this
change in the RFC before getting feedback on the general approach
(feedback on those specific questions will also be appreciated).

Laurent Pinchart (8):
  media: pxa_camera: Drop usage of .set_mbus_config()
  media: i2c: ov6650: Drop implementation of .set_mbus_config()
  media: v4l2-subdev: Drop .set_mbus_config() operation
  media: v4l2-fwnode: Move bus config structure to v4l2_mediabus.h
  media: v4l2-mediabus: Use structures to describe bus configuration
  media: v4l2-mediabus: Drop legacy V4L2_MBUS_CSI2_*_LANE flags
  media: v4l2-mediabus: Drop legacy V4L2_MBUS_CSI2_CHANNEL_* flags
  media: v4l2-mediabus: Drop V4L2_MBUS_CSI2_CONTINUOUS_CLOCK flag

 drivers/gpu/ipu-v3/ipu-csi.c                  |   6 +-
 drivers/media/i2c/adv7180.c                   |  10 +-
 drivers/media/i2c/adv748x/adv748x-csi2.c      |  18 +--
 drivers/media/i2c/ml86v7667.c                 |   5 +-
 drivers/media/i2c/mt9m001.c                   |   8 +-
 drivers/media/i2c/mt9m111.c                   |  16 +--
 drivers/media/i2c/ov5648.c                    |   4 +-
 drivers/media/i2c/ov6650.c                    |  51 ++-------
 drivers/media/i2c/ov8865.c                    |   4 +-
 drivers/media/i2c/ov9640.c                    |   8 +-
 drivers/media/i2c/tc358743.c                  |  26 +----
 drivers/media/i2c/tvp5150.c                   |   6 +-
 drivers/media/platform/pxa_camera.c           |  21 ++--
 drivers/media/platform/qcom/camss/camss.c     |   2 +-
 drivers/media/platform/rcar-vin/rcar-csi2.c   |  16 +--
 drivers/media/platform/rcar-vin/rcar-vin.h    |   2 +-
 drivers/media/platform/stm32/stm32-dcmi.c     |   2 +-
 .../platform/sunxi/sun4i-csi/sun4i_csi.h      |   2 +-
 .../platform/sunxi/sun4i-csi/sun4i_dma.c      |   2 +-
 drivers/media/platform/ti-vpe/cal-camerarx.c  |   6 +-
 drivers/media/v4l2-core/v4l2-fwnode.c         |  16 ++-
 drivers/media/v4l2-core/v4l2-subdev.c         |   8 --
 drivers/staging/media/imx/imx-media-csi.c     |   7 +-
 drivers/staging/media/imx/imx6-mipi-csi2.c    |  25 +----
 drivers/staging/media/imx/imx7-mipi-csis.c    |   2 +-
 drivers/staging/media/imx/imx8mq-mipi-csi2.c  |   2 +-
 drivers/staging/media/max96712/max96712.c     |   2 +-
 include/media/v4l2-fwnode.h                   |  61 +---------
 include/media/v4l2-mediabus.h                 | 104 ++++++++++++------
 include/media/v4l2-subdev.h                   |  13 ---
 30 files changed, 168 insertions(+), 287 deletions(-)


base-commit: 68b9bcc8a534cd11fe55f8bc82f948aae7d81b3c

Comments

Jacopo Mondi Jan. 9, 2022, 2:36 p.m. UTC | #1
Hi Laurent,

On Mon, Jan 03, 2022 at 06:24:06PM +0200, Laurent Pinchart wrote:
> Hello,
>
> This patch series reworks the V4L2 subdev .get_mbus_config() and
> .set_mbus_config() operations to improve the former and drop the latter.
>
> These subdev operations originate from soc-camera (for those who
> remember the framework), and were designed to let a transmitter and a
> receiver negotiate the physical configuration of the bus that connects
> them. The operations use bitflags to represent bus parameters, with
> supported options set by the caller of .set_mbus_config(), and selected
> options among those returned by the callee. This mechanism is
> deprecated, as selection of the bus configuration has long been moved to
> the firmware interface (DT or ACPI), and usage of bitflags prevents from
> adding more complex configuration parameters (timings in particular).
>
> As .set_mbus_config() is deprecated and used by one pair of drivers only
> (pxa_camera and ov6650), it wasn't difficult to drop usage of that
> operation in patches 1/8 and 2/8, and remove the operation itself in
> patch 3/8.
>
> With that operation gone, .get_mbus_config() can be moved from bitflags
> to structures. It turned out that the needed data structures were
> already present in v4l2_fwnode.h. Patch 4/8 moves them to
> v4l2_mediabus.h (and renames them to drop the fwnode mention, as they're
> not specific to the fwnode API), and patch 5/8 makes use of them.

great, when the soc_camera version was dropped, the next thing to do
was to move away from bitflags and move to structure fields...

> Patches 6/8 to 8/8 then removes media bus configuration bitflags that
> are unneeded (and now unused).

Parallel might require a bit more of work, but csi2 already has a
single flag

/* Clock non-continuous mode support. */
#define V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK	BIT(0)

so it should be trivial to replace 'flags' with a boolean
'clock_noncontinuous' field.

One things leaves me a bit confused: we're now mixing run-time
configurable paramters (eg the number of data lanes in use in the
current streaming sessions) and paramters which come from DT and are
usually fixed by the HW design like lanes ordering.

Is this ok in your opinion ?

>
> The series is an RFC as not everything has been converted from bitflags
> to named fields in structures. In particular, the parallel bus flags
> haven't been touched at all. Patch 8/8 shows how mutually exclusive
> flags can be reworked to drop one of them. We then need to decide
> whether to keep expressing the flag as macros, or move to C bitfields
> with dedicated structure member names. I didn't want to include this
> change in the RFC before getting feedback on the general approach
> (feedback on those specific questions will also be appreciated).
>

There's also an opportunity to use v4l2_mbus_config as part of
v4l2_fwnode_endpoint instead of repeating the same fields ?

struct v4l2_fwnode_endpoint {
	struct fwnode_endpoint base;
	/*
	 * Fields below this line will be zeroed by
	 * v4l2_fwnode_endpoint_parse()
	 */
        -------------------------------------------------------------
	enum v4l2_mbus_type bus_type;
	struct {
		struct v4l2_mbus_config_parallel parallel;
		struct v4l2_mbus_config_mipi_csi1 mipi_csi1;
		struct v4l2_mbus_config_mipi_csi2 mipi_csi2;
	} bus;
        -------------------------------------------------------------
	u64 *link_frequencies;
	unsigned int nr_of_link_frequencies;
};

struct v4l2_mbus_config {
	enum v4l2_mbus_type type;
	union {
		struct v4l2_mbus_config_parallel parallel;
		struct v4l2_mbus_config_mipi_csi1 mipi_csi1;
		struct v4l2_mbus_config_mipi_csi2 mipi_csi2;
	} bus;
};


> Laurent Pinchart (8):
>   media: pxa_camera: Drop usage of .set_mbus_config()
>   media: i2c: ov6650: Drop implementation of .set_mbus_config()
>   media: v4l2-subdev: Drop .set_mbus_config() operation
>   media: v4l2-fwnode: Move bus config structure to v4l2_mediabus.h
>   media: v4l2-mediabus: Use structures to describe bus configuration
>   media: v4l2-mediabus: Drop legacy V4L2_MBUS_CSI2_*_LANE flags
>   media: v4l2-mediabus: Drop legacy V4L2_MBUS_CSI2_CHANNEL_* flags
>   media: v4l2-mediabus: Drop V4L2_MBUS_CSI2_CONTINUOUS_CLOCK flag
>
>  drivers/gpu/ipu-v3/ipu-csi.c                  |   6 +-
>  drivers/media/i2c/adv7180.c                   |  10 +-
>  drivers/media/i2c/adv748x/adv748x-csi2.c      |  18 +--
>  drivers/media/i2c/ml86v7667.c                 |   5 +-
>  drivers/media/i2c/mt9m001.c                   |   8 +-
>  drivers/media/i2c/mt9m111.c                   |  16 +--
>  drivers/media/i2c/ov5648.c                    |   4 +-
>  drivers/media/i2c/ov6650.c                    |  51 ++-------
>  drivers/media/i2c/ov8865.c                    |   4 +-
>  drivers/media/i2c/ov9640.c                    |   8 +-
>  drivers/media/i2c/tc358743.c                  |  26 +----
>  drivers/media/i2c/tvp5150.c                   |   6 +-
>  drivers/media/platform/pxa_camera.c           |  21 ++--
>  drivers/media/platform/qcom/camss/camss.c     |   2 +-
>  drivers/media/platform/rcar-vin/rcar-csi2.c   |  16 +--
>  drivers/media/platform/rcar-vin/rcar-vin.h    |   2 +-
>  drivers/media/platform/stm32/stm32-dcmi.c     |   2 +-
>  .../platform/sunxi/sun4i-csi/sun4i_csi.h      |   2 +-
>  .../platform/sunxi/sun4i-csi/sun4i_dma.c      |   2 +-
>  drivers/media/platform/ti-vpe/cal-camerarx.c  |   6 +-
>  drivers/media/v4l2-core/v4l2-fwnode.c         |  16 ++-
>  drivers/media/v4l2-core/v4l2-subdev.c         |   8 --
>  drivers/staging/media/imx/imx-media-csi.c     |   7 +-
>  drivers/staging/media/imx/imx6-mipi-csi2.c    |  25 +----
>  drivers/staging/media/imx/imx7-mipi-csis.c    |   2 +-
>  drivers/staging/media/imx/imx8mq-mipi-csi2.c  |   2 +-
>  drivers/staging/media/max96712/max96712.c     |   2 +-
>  include/media/v4l2-fwnode.h                   |  61 +---------
>  include/media/v4l2-mediabus.h                 | 104 ++++++++++++------
>  include/media/v4l2-subdev.h                   |  13 ---
>  30 files changed, 168 insertions(+), 287 deletions(-)
>
>
> base-commit: 68b9bcc8a534cd11fe55f8bc82f948aae7d81b3c
> --
> Regards,
>
> Laurent Pinchart
>
Laurent Pinchart Jan. 9, 2022, 10:55 p.m. UTC | #2
Hi Jacopo,

On Sun, Jan 09, 2022 at 03:36:24PM +0100, Jacopo Mondi wrote:
> On Mon, Jan 03, 2022 at 06:24:06PM +0200, Laurent Pinchart wrote:
> > Hello,
> >
> > This patch series reworks the V4L2 subdev .get_mbus_config() and
> > .set_mbus_config() operations to improve the former and drop the latter.
> >
> > These subdev operations originate from soc-camera (for those who
> > remember the framework), and were designed to let a transmitter and a
> > receiver negotiate the physical configuration of the bus that connects
> > them. The operations use bitflags to represent bus parameters, with
> > supported options set by the caller of .set_mbus_config(), and selected
> > options among those returned by the callee. This mechanism is
> > deprecated, as selection of the bus configuration has long been moved to
> > the firmware interface (DT or ACPI), and usage of bitflags prevents from
> > adding more complex configuration parameters (timings in particular).
> >
> > As .set_mbus_config() is deprecated and used by one pair of drivers only
> > (pxa_camera and ov6650), it wasn't difficult to drop usage of that
> > operation in patches 1/8 and 2/8, and remove the operation itself in
> > patch 3/8.
> >
> > With that operation gone, .get_mbus_config() can be moved from bitflags
> > to structures. It turned out that the needed data structures were
> > already present in v4l2_fwnode.h. Patch 4/8 moves them to
> > v4l2_mediabus.h (and renames them to drop the fwnode mention, as they're
> > not specific to the fwnode API), and patch 5/8 makes use of them.
> 
> great, when the soc_camera version was dropped, the next thing to do
> was to move away from bitflags and move to structure fields...

There's an endless supply of cleanups to be done.

> > Patches 6/8 to 8/8 then removes media bus configuration bitflags that
> > are unneeded (and now unused).
> 
> Parallel might require a bit more of work, but csi2 already has a
> single flag
> 
> /* Clock non-continuous mode support. */
> #define V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK	BIT(0)
> 
> so it should be trivial to replace 'flags' with a boolean
> 'clock_noncontinuous' field.
> 
> One things leaves me a bit confused: we're now mixing run-time
> configurable paramters (eg the number of data lanes in use in the
> current streaming sessions) and paramters which come from DT and are
> usually fixed by the HW design like lanes ordering.
> 
> Is this ok in your opinion ?

The .get_mbus_config() operation is meant to report the runtime
configuration, which is constrained by hardware limitations (for
instance the number of data lanes routed on the board, as expressed in
DT, or the ability to support non-continuous clock mode, which is an
intrinsic property of the device, known by the driver). When multiple
options are possible within the constraints of the platform, how to
select between them is currently unspecified. If the need arises, we'll
have to study the use cases and find a solution.

> > The series is an RFC as not everything has been converted from bitflags
> > to named fields in structures. In particular, the parallel bus flags
> > haven't been touched at all. Patch 8/8 shows how mutually exclusive
> > flags can be reworked to drop one of them. We then need to decide
> > whether to keep expressing the flag as macros, or move to C bitfields
> > with dedicated structure member names. I didn't want to include this
> > change in the RFC before getting feedback on the general approach
> > (feedback on those specific questions will also be appreciated).
> 
> There's also an opportunity to use v4l2_mbus_config as part of
> v4l2_fwnode_endpoint instead of repeating the same fields ?
> 
> struct v4l2_fwnode_endpoint {
> 	struct fwnode_endpoint base;
> 	/*
> 	 * Fields below this line will be zeroed by
> 	 * v4l2_fwnode_endpoint_parse()
> 	 */
>         -------------------------------------------------------------
> 	enum v4l2_mbus_type bus_type;
> 	struct {
> 		struct v4l2_mbus_config_parallel parallel;
> 		struct v4l2_mbus_config_mipi_csi1 mipi_csi1;
> 		struct v4l2_mbus_config_mipi_csi2 mipi_csi2;
> 	} bus;
>         -------------------------------------------------------------
> 	u64 *link_frequencies;
> 	unsigned int nr_of_link_frequencies;
> };
> 
> struct v4l2_mbus_config {
> 	enum v4l2_mbus_type type;
> 	union {
> 		struct v4l2_mbus_config_parallel parallel;
> 		struct v4l2_mbus_config_mipi_csi1 mipi_csi1;
> 		struct v4l2_mbus_config_mipi_csi2 mipi_csi2;
> 	} bus;
> };

I've thought about it, but not that the former groups the bus-specific
data in a struct, while the latter uses a union. Whether or not we could
use the same in both cases is an issue I have decided not to think about
at this stage :-)

> > Laurent Pinchart (8):
> >   media: pxa_camera: Drop usage of .set_mbus_config()
> >   media: i2c: ov6650: Drop implementation of .set_mbus_config()
> >   media: v4l2-subdev: Drop .set_mbus_config() operation
> >   media: v4l2-fwnode: Move bus config structure to v4l2_mediabus.h
> >   media: v4l2-mediabus: Use structures to describe bus configuration
> >   media: v4l2-mediabus: Drop legacy V4L2_MBUS_CSI2_*_LANE flags
> >   media: v4l2-mediabus: Drop legacy V4L2_MBUS_CSI2_CHANNEL_* flags
> >   media: v4l2-mediabus: Drop V4L2_MBUS_CSI2_CONTINUOUS_CLOCK flag
> >
> >  drivers/gpu/ipu-v3/ipu-csi.c                  |   6 +-
> >  drivers/media/i2c/adv7180.c                   |  10 +-
> >  drivers/media/i2c/adv748x/adv748x-csi2.c      |  18 +--
> >  drivers/media/i2c/ml86v7667.c                 |   5 +-
> >  drivers/media/i2c/mt9m001.c                   |   8 +-
> >  drivers/media/i2c/mt9m111.c                   |  16 +--
> >  drivers/media/i2c/ov5648.c                    |   4 +-
> >  drivers/media/i2c/ov6650.c                    |  51 ++-------
> >  drivers/media/i2c/ov8865.c                    |   4 +-
> >  drivers/media/i2c/ov9640.c                    |   8 +-
> >  drivers/media/i2c/tc358743.c                  |  26 +----
> >  drivers/media/i2c/tvp5150.c                   |   6 +-
> >  drivers/media/platform/pxa_camera.c           |  21 ++--
> >  drivers/media/platform/qcom/camss/camss.c     |   2 +-
> >  drivers/media/platform/rcar-vin/rcar-csi2.c   |  16 +--
> >  drivers/media/platform/rcar-vin/rcar-vin.h    |   2 +-
> >  drivers/media/platform/stm32/stm32-dcmi.c     |   2 +-
> >  .../platform/sunxi/sun4i-csi/sun4i_csi.h      |   2 +-
> >  .../platform/sunxi/sun4i-csi/sun4i_dma.c      |   2 +-
> >  drivers/media/platform/ti-vpe/cal-camerarx.c  |   6 +-
> >  drivers/media/v4l2-core/v4l2-fwnode.c         |  16 ++-
> >  drivers/media/v4l2-core/v4l2-subdev.c         |   8 --
> >  drivers/staging/media/imx/imx-media-csi.c     |   7 +-
> >  drivers/staging/media/imx/imx6-mipi-csi2.c    |  25 +----
> >  drivers/staging/media/imx/imx7-mipi-csis.c    |   2 +-
> >  drivers/staging/media/imx/imx8mq-mipi-csi2.c  |   2 +-
> >  drivers/staging/media/max96712/max96712.c     |   2 +-
> >  include/media/v4l2-fwnode.h                   |  61 +---------
> >  include/media/v4l2-mediabus.h                 | 104 ++++++++++++------
> >  include/media/v4l2-subdev.h                   |  13 ---
> >  30 files changed, 168 insertions(+), 287 deletions(-)
> >
> >
> > base-commit: 68b9bcc8a534cd11fe55f8bc82f948aae7d81b3c
Sakari Ailus Jan. 10, 2022, 8:42 a.m. UTC | #3
Hi Laurent, Jacopo,

On Mon, Jan 10, 2022 at 12:55:48AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> On Sun, Jan 09, 2022 at 03:36:24PM +0100, Jacopo Mondi wrote:
> > On Mon, Jan 03, 2022 at 06:24:06PM +0200, Laurent Pinchart wrote:
> > > Hello,
> > >
> > > This patch series reworks the V4L2 subdev .get_mbus_config() and
> > > .set_mbus_config() operations to improve the former and drop the latter.
> > >
> > > These subdev operations originate from soc-camera (for those who
> > > remember the framework), and were designed to let a transmitter and a
> > > receiver negotiate the physical configuration of the bus that connects
> > > them. The operations use bitflags to represent bus parameters, with
> > > supported options set by the caller of .set_mbus_config(), and selected
> > > options among those returned by the callee. This mechanism is
> > > deprecated, as selection of the bus configuration has long been moved to
> > > the firmware interface (DT or ACPI), and usage of bitflags prevents from
> > > adding more complex configuration parameters (timings in particular).
> > >
> > > As .set_mbus_config() is deprecated and used by one pair of drivers only
> > > (pxa_camera and ov6650), it wasn't difficult to drop usage of that
> > > operation in patches 1/8 and 2/8, and remove the operation itself in
> > > patch 3/8.
> > >
> > > With that operation gone, .get_mbus_config() can be moved from bitflags
> > > to structures. It turned out that the needed data structures were
> > > already present in v4l2_fwnode.h. Patch 4/8 moves them to
> > > v4l2_mediabus.h (and renames them to drop the fwnode mention, as they're
> > > not specific to the fwnode API), and patch 5/8 makes use of them.
> > 
> > great, when the soc_camera version was dropped, the next thing to do
> > was to move away from bitflags and move to structure fields...
> 
> There's an endless supply of cleanups to be done.
> 
> > > Patches 6/8 to 8/8 then removes media bus configuration bitflags that
> > > are unneeded (and now unused).
> > 
> > Parallel might require a bit more of work, but csi2 already has a
> > single flag
> > 
> > /* Clock non-continuous mode support. */
> > #define V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK	BIT(0)
> > 
> > so it should be trivial to replace 'flags' with a boolean
> > 'clock_noncontinuous' field.
> > 
> > One things leaves me a bit confused: we're now mixing run-time
> > configurable paramters (eg the number of data lanes in use in the
> > current streaming sessions) and paramters which come from DT and are
> > usually fixed by the HW design like lanes ordering.
> > 
> > Is this ok in your opinion ?
> 
> The .get_mbus_config() operation is meant to report the runtime
> configuration, which is constrained by hardware limitations (for
> instance the number of data lanes routed on the board, as expressed in
> DT, or the ability to support non-continuous clock mode, which is an
> intrinsic property of the device, known by the driver). When multiple
> options are possible within the constraints of the platform, how to
> select between them is currently unspecified. If the need arises, we'll
> have to study the use cases and find a solution.
> 
> > > The series is an RFC as not everything has been converted from bitflags
> > > to named fields in structures. In particular, the parallel bus flags
> > > haven't been touched at all. Patch 8/8 shows how mutually exclusive
> > > flags can be reworked to drop one of them. We then need to decide
> > > whether to keep expressing the flag as macros, or move to C bitfields
> > > with dedicated structure member names. I didn't want to include this
> > > change in the RFC before getting feedback on the general approach
> > > (feedback on those specific questions will also be appreciated).
> > 
> > There's also an opportunity to use v4l2_mbus_config as part of
> > v4l2_fwnode_endpoint instead of repeating the same fields ?
> > 
> > struct v4l2_fwnode_endpoint {
> > 	struct fwnode_endpoint base;
> > 	/*
> > 	 * Fields below this line will be zeroed by
> > 	 * v4l2_fwnode_endpoint_parse()
> > 	 */
> >         -------------------------------------------------------------
> > 	enum v4l2_mbus_type bus_type;
> > 	struct {
> > 		struct v4l2_mbus_config_parallel parallel;
> > 		struct v4l2_mbus_config_mipi_csi1 mipi_csi1;
> > 		struct v4l2_mbus_config_mipi_csi2 mipi_csi2;
> > 	} bus;
> >         -------------------------------------------------------------
> > 	u64 *link_frequencies;
> > 	unsigned int nr_of_link_frequencies;
> > };
> > 
> > struct v4l2_mbus_config {
> > 	enum v4l2_mbus_type type;
> > 	union {
> > 		struct v4l2_mbus_config_parallel parallel;
> > 		struct v4l2_mbus_config_mipi_csi1 mipi_csi1;
> > 		struct v4l2_mbus_config_mipi_csi2 mipi_csi2;
> > 	} bus;
> > };
> 
> I've thought about it, but not that the former groups the bus-specific
> data in a struct, while the latter uses a union. Whether or not we could
> use the same in both cases is an issue I have decided not to think about
> at this stage :-)

I think it should be possible to replace that struct by an union. In the
past that probably was not the case.

The defaults for a bus type should be set just before enumerating it.
Drivers should be verified of course.
Laurent Pinchart Feb. 15, 2022, 8:37 a.m. UTC | #4
Hello,

Reviews or 5/8 to 8/8 would be nice :-) Thanks.

On Mon, Jan 03, 2022 at 06:24:06PM +0200, Laurent Pinchart wrote:
> Hello,
> 
> This patch series reworks the V4L2 subdev .get_mbus_config() and
> .set_mbus_config() operations to improve the former and drop the latter.
> 
> These subdev operations originate from soc-camera (for those who
> remember the framework), and were designed to let a transmitter and a
> receiver negotiate the physical configuration of the bus that connects
> them. The operations use bitflags to represent bus parameters, with
> supported options set by the caller of .set_mbus_config(), and selected
> options among those returned by the callee. This mechanism is
> deprecated, as selection of the bus configuration has long been moved to
> the firmware interface (DT or ACPI), and usage of bitflags prevents from
> adding more complex configuration parameters (timings in particular).
> 
> As .set_mbus_config() is deprecated and used by one pair of drivers only
> (pxa_camera and ov6650), it wasn't difficult to drop usage of that
> operation in patches 1/8 and 2/8, and remove the operation itself in
> patch 3/8.
> 
> With that operation gone, .get_mbus_config() can be moved from bitflags
> to structures. It turned out that the needed data structures were
> already present in v4l2_fwnode.h. Patch 4/8 moves them to
> v4l2_mediabus.h (and renames them to drop the fwnode mention, as they're
> not specific to the fwnode API), and patch 5/8 makes use of them.
> Patches 6/8 to 8/8 then removes media bus configuration bitflags that
> are unneeded (and now unused).
> 
> The series is an RFC as not everything has been converted from bitflags
> to named fields in structures. In particular, the parallel bus flags
> haven't been touched at all. Patch 8/8 shows how mutually exclusive
> flags can be reworked to drop one of them. We then need to decide
> whether to keep expressing the flag as macros, or move to C bitfields
> with dedicated structure member names. I didn't want to include this
> change in the RFC before getting feedback on the general approach
> (feedback on those specific questions will also be appreciated).
> 
> Laurent Pinchart (8):
>   media: pxa_camera: Drop usage of .set_mbus_config()
>   media: i2c: ov6650: Drop implementation of .set_mbus_config()
>   media: v4l2-subdev: Drop .set_mbus_config() operation
>   media: v4l2-fwnode: Move bus config structure to v4l2_mediabus.h
>   media: v4l2-mediabus: Use structures to describe bus configuration
>   media: v4l2-mediabus: Drop legacy V4L2_MBUS_CSI2_*_LANE flags
>   media: v4l2-mediabus: Drop legacy V4L2_MBUS_CSI2_CHANNEL_* flags
>   media: v4l2-mediabus: Drop V4L2_MBUS_CSI2_CONTINUOUS_CLOCK flag
> 
>  drivers/gpu/ipu-v3/ipu-csi.c                  |   6 +-
>  drivers/media/i2c/adv7180.c                   |  10 +-
>  drivers/media/i2c/adv748x/adv748x-csi2.c      |  18 +--
>  drivers/media/i2c/ml86v7667.c                 |   5 +-
>  drivers/media/i2c/mt9m001.c                   |   8 +-
>  drivers/media/i2c/mt9m111.c                   |  16 +--
>  drivers/media/i2c/ov5648.c                    |   4 +-
>  drivers/media/i2c/ov6650.c                    |  51 ++-------
>  drivers/media/i2c/ov8865.c                    |   4 +-
>  drivers/media/i2c/ov9640.c                    |   8 +-
>  drivers/media/i2c/tc358743.c                  |  26 +----
>  drivers/media/i2c/tvp5150.c                   |   6 +-
>  drivers/media/platform/pxa_camera.c           |  21 ++--
>  drivers/media/platform/qcom/camss/camss.c     |   2 +-
>  drivers/media/platform/rcar-vin/rcar-csi2.c   |  16 +--
>  drivers/media/platform/rcar-vin/rcar-vin.h    |   2 +-
>  drivers/media/platform/stm32/stm32-dcmi.c     |   2 +-
>  .../platform/sunxi/sun4i-csi/sun4i_csi.h      |   2 +-
>  .../platform/sunxi/sun4i-csi/sun4i_dma.c      |   2 +-
>  drivers/media/platform/ti-vpe/cal-camerarx.c  |   6 +-
>  drivers/media/v4l2-core/v4l2-fwnode.c         |  16 ++-
>  drivers/media/v4l2-core/v4l2-subdev.c         |   8 --
>  drivers/staging/media/imx/imx-media-csi.c     |   7 +-
>  drivers/staging/media/imx/imx6-mipi-csi2.c    |  25 +----
>  drivers/staging/media/imx/imx7-mipi-csis.c    |   2 +-
>  drivers/staging/media/imx/imx8mq-mipi-csi2.c  |   2 +-
>  drivers/staging/media/max96712/max96712.c     |   2 +-
>  include/media/v4l2-fwnode.h                   |  61 +---------
>  include/media/v4l2-mediabus.h                 | 104 ++++++++++++------
>  include/media/v4l2-subdev.h                   |  13 ---
>  30 files changed, 168 insertions(+), 287 deletions(-)
> 
> 
> base-commit: 68b9bcc8a534cd11fe55f8bc82f948aae7d81b3c