Hi Niklas,
On Tue, Jan 7, 2025 at 10:52 AM Niklas Söderlund
<niklas.soderlund+renesas@xxxxxxxxxxxx> wrote:
> On 2025-01-07 08:36:31 +0000, Sakari Ailus wrote:
> > On Sat, Jan 04, 2025 at 08:55:48PM +0100, Niklas Söderlund wrote:
> > > Provided a safe-guard from the raw values used in device tree sources
> > > and the in-kernel defines used to describe the different line orders.
> > > This mimics what have been done for the bus-type property to provide the
> > > same safe-guard.
> > >
> > > The macros used in device tree sources are defined in video-interfaces.h
> > > (MEDIA_BUS_CSI2_CPHY_LINE_ORDER_*) and are only visible to DTS source
> > > files. These raw values map directly to the in-kernel names by fwnode
> > > defines in v4l2-fwnode.h (V4L2_FWNODE_CSI2_CPHY_LINE_ORDER_*). These
> > > fwnode defines are finally translated to defines which are exposed to
> > > drivers to act on (V4L2_MBUS_CSI2_CPHY_LINE_ORDER_*).
> > >
> > > Previously the translation to values provided to drivers have exploited
> > > the fact that the numerical value for each setting are the same for the
> > > defines used in device tree sources. While this is unlikely to change
> > > this harmonises the bus-type and line-orders parsing to work using the
> > > same mechanics, while at the same time make the large CSI-2 parsing
> > > function a little more readable.
> >
> > Do we in fact need the V4L2_MBUS_ definitions of the line orders at all?
>
> I'm not sure :-)
>
> Geert pointed out in [1] that in comparison to the V4L2_MBUS_ bus-type
> definitions the line-order definitions did not have this intermediary
> step as a safe guard between values used in DTS files and values used in
> V4L2 drivers.
>
> Looking at the original functionality,
>
> bus->line_orders[i] = array[i];
>
> Seems a bit "hack" compared to what this patch do,
>
>
> bus->line_orders[i] = v4l2_fwnode_line_order_to_mbus(array[i]);
>
> But if it's worth the extra churn, and if it in reality provides us with
> a safe-guard between DTS-files and V4L2-drivers I'm not sure. I'm on the
> fence on this one, the one good thing is that it aligns how V4L2_MBUS_
> macros are parsed.
If you decide to keep the V4L2_MBUS_* definitions, there are other
(simpler) alternatives than adding explicit translation code:
enum v4l2_mbus_csi2_cphy_line_orders_type {
V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ABC =
MEDIA_BUS_CSI2_CPHY_LINE_ORDER_ABC,
V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ACB =
MEDIA_BUS_CSI2_CPHY_LINE_ORDER_ACB,
...
> +};
or
BUILD_BUG_ON(V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ABC !=
MEDIA_BUS_CSI2_CPHY_LINE_ORDER_ABC);
BUILD_BUG_ON(V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ACB !=
MEDIA_BUS_CSI2_CPHY_LINE_ORDER_ACB);
...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
