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.
1. CAMuHMdXwqb7vhUeoMKDDJO5dp-V3LmnURZLSC1_ko=YL=cNyUA@xxxxxxxxxxxxxx
>
> 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?
>
> >
> > Suggested-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > ---
> > drivers/media/v4l2-core/v4l2-fwnode.c | 80 ++++++++++++++++++++++-----
> > 1 file changed, 66 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> > index cb153ce42c45..69f6d1df8c39 100644
> > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > @@ -121,6 +121,70 @@ v4l2_fwnode_mbus_type_to_string(enum v4l2_mbus_type type)
> > return conv ? conv->name : "not found";
> > }
> >
> > +static const struct v4l2_fwnode_csi2_cphy_line_orders_conv {
> > + enum v4l2_fwnode_csi2_cphy_line_orders_type fwnode_order;
> > + enum v4l2_mbus_csi2_cphy_line_orders_type mbus_order;
> > + const char *name;
> > +} csi2_cphy_line_orders[] = {
> > + {
> > + V4L2_FWNODE_CSI2_CPHY_LINE_ORDER_ABC,
> > + V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ABC,
> > + "ABC",
> > + }, {
> > + V4L2_FWNODE_CSI2_CPHY_LINE_ORDER_ACB,
> > + V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ACB,
> > + "ACB",
> > + }, {
> > + V4L2_FWNODE_CSI2_CPHY_LINE_ORDER_BAC,
> > + V4L2_MBUS_CSI2_CPHY_LINE_ORDER_BAC,
> > + "BAC",
> > + }, {
> > + V4L2_FWNODE_CSI2_CPHY_LINE_ORDER_BCA,
> > + V4L2_MBUS_CSI2_CPHY_LINE_ORDER_BCA,
> > + "BCA",
> > + }, {
> > + V4L2_FWNODE_CSI2_CPHY_LINE_ORDER_CAB,
> > + V4L2_MBUS_CSI2_CPHY_LINE_ORDER_CAB,
> > + "CAB",
> > + }, {
> > + V4L2_FWNODE_CSI2_CPHY_LINE_ORDER_CBA,
> > + V4L2_MBUS_CSI2_CPHY_LINE_ORDER_CBA,
> > + "CBA",
> > + }
> > +};
> > +
> > +static const struct v4l2_fwnode_csi2_cphy_line_orders_conv *
> > +get_v4l2_fwnode_line_order_conv_by_fwnode_order(enum v4l2_fwnode_csi2_cphy_line_orders_type order)
> > +{
> > + unsigned int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(csi2_cphy_line_orders); i++)
> > + if (csi2_cphy_line_orders[i].fwnode_order == order)
> > + return &csi2_cphy_line_orders[i];
> > +
> > + /* The default line order is ABC */
> > + pr_warn("invalid line-order assuming ABC (got %u)\n", order);
> > + return &csi2_cphy_line_orders[0];
> > +}
> > +
> > +static enum v4l2_mbus_csi2_cphy_line_orders_type
> > +v4l2_fwnode_line_order_to_mbus(enum v4l2_fwnode_csi2_cphy_line_orders_type order)
> > +{
> > + const struct v4l2_fwnode_csi2_cphy_line_orders_conv *conv =
> > + get_v4l2_fwnode_line_order_conv_by_fwnode_order(order);
> > +
> > + return conv->mbus_order;
> > +}
> > +
> > +static const char *
> > +v4l2_fwnode_line_order_to_string(enum v4l2_fwnode_csi2_cphy_line_orders_type order)
> > +{
> > + const struct v4l2_fwnode_csi2_cphy_line_orders_conv *conv =
> > + get_v4l2_fwnode_line_order_conv_by_fwnode_order(order);
> > +
> > + return conv->name;
> > +}
> > +
> > static int v4l2_fwnode_endpoint_parse_csi2_bus(struct fwnode_handle *fwnode,
> > struct v4l2_fwnode_endpoint *vep,
> > enum v4l2_mbus_type bus_type)
> > @@ -268,21 +332,9 @@ static int v4l2_fwnode_endpoint_parse_csi2_bus(struct fwnode_handle *fwnode,
> > num_data_lanes);
> >
> > for (i = 0; i < num_data_lanes; i++) {
> > - static const char * const orders[] = {
> > - "ABC", "ACB", "BAC", "BCA", "CAB", "CBA"
> > - };
> > -
> > - if (array[i] >= ARRAY_SIZE(orders)) {
> > - pr_warn("lane %u invalid line-order assuming ABC (got %u)\n",
> > - i, array[i]);
> > - bus->line_orders[i] =
> > - V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ABC;
> > - continue;
> > - }
> > -
> > - bus->line_orders[i] = array[i];
> > + bus->line_orders[i] = v4l2_fwnode_line_order_to_mbus(array[i]);
> > pr_debug("lane %u line order %s", i,
> > - orders[array[i]]);
> > + v4l2_fwnode_line_order_to_string(array[i]));
> > }
> > } else {
> > for (i = 0; i < num_data_lanes; i++)
> > --
> > 2.47.1
> >
>
> --
> Med vänliga hälsningar,
>
> Sakari Ailus
--
Kind Regards,
Niklas Söderlund
