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