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.
--
Regards,
Sakari Ailus
