Re: [PATCH 2/2] media: v4l: fwnode: Parse CSI-2 C-PHY line-orders like bus-type

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hejssan,

On Tue, Jan 07, 2025 at 10:52:19AM +0100, Niklas Söderlund wrote:
> Hi Sakari,
> 
> Tack för din feedback.
> 
> On 2025-01-07 08:36:31 +0000, Sakari Ailus wrote:
> > Hejssan Niklas,
> > 
> > Tack för dessa lappar!
> > 
> > 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.
> 
> But if you don't like it and I'm on the fence I'm happy to drop this 
> series. This series don't add any extra functionality.

I wasn't asking dropping the series, but instead get rid of the V4L2_MBUS_
line order definitions altogether, by replacing them by V4L2_FWNODE_
equivalents.

> 
> 1. CAMuHMdXwqb7vhUeoMKDDJO5dp-V3LmnURZLSC1_ko=YL=cNyUA@mail.gmail.com
> 
> > 
> > The same could extend to the V4L2_MBUS_ bus type defitions, but that's out
> > of scope of this patch.
> 
> Out of scope indeed. If we drop this series do we want to try and remove 
> them for V4L2_MBUS_ bus-type in future?

I think that would be reasonable. I don't think we need two sets of
definitions that effectively are interchangeable. But that may well be out
of scope of this series.

-- 
Med vänliga hälsningar,

Sakari Ailus