Re: [PATCH v2 2/4] media: v4l: fwnode: Parse MiPI DisCo for C-PHY line-orders

["Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>]

Hi Geert,

Thanks for your review.

On 2024-12-27 14:31:11 +0100, Geert Uytterhoeven wrote:
> Hi Niklas,
> 
> On Thu, Nov 21, 2024 at 2:41 PM Niklas Söderlund
> <niklas.soderlund+renesas@ragnatech.se> wrote:
> > Extend the fwnode parsing to validate and fill in the CSI-2 C-PHY
> > line-orders order properties as defined in MIPI Discovery and
> > Configuration (DisCo) Specification for Imaging.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> > * Changes since v1
> > - Use array instead of switch to get printable line order string for
> >   debug output.
> > - Wrap lines harder for 80 chars instead of 100, but keep string formats
> >   on same line even if they break the 80 chars.
> 
> Thanks for your patch, which is now commit 573b4adddbd22baf
> ("media: v4l: fwnode: Parse MiPI DisCo for C-PHY line-orders") in
> media/master.
> 
> > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > @@ -250,6 +261,36 @@ static int v4l2_fwnode_endpoint_parse_csi2_bus(struct fwnode_handle *fwnode,
> >                 } else {
> >                         pr_debug("no lane polarities defined, assuming not inverted\n");
> >                 }
> > +
> > +               if (have_line_orders) {
> > +                       fwnode_property_read_u32_array(fwnode,
> > +                                                      "line-orders", array,
> > +                                                      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] > 5) {
> > +                                       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];
> 
> This does not do any translation (unlike v4l2_fwnode_bus_type_to_mbus()
> to translate from MEDIA_BUS_TYPE_* to V4L2_MBUS_* definitions) ...
> 
> > +                               pr_debug("lane %u line order %s", i,
> > +                                        orders[array[i]]);
> > +                       }
> > +               } else {
> > +                       for (i = 0; i < num_data_lanes; i++)
> > +                               bus->line_orders[i] =
> > +                                       V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ABC;
> > +
> > +                       pr_debug("no line orders defined, assuming ABC\n");
> > +               }
> >         }
> >
> >         return 0;
> > diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> > index 5bce6e423e94..e7f019f68c8d 100644
> > --- a/include/media/v4l2-mediabus.h
> > +++ b/include/media/v4l2-mediabus.h
> > @@ -73,6 +73,24 @@
> >
> >  #define V4L2_MBUS_CSI2_MAX_DATA_LANES          8
> >
> > +/**
> > + * enum v4l2_mbus_csi2_cphy_line_orders_type - CSI-2 C-PHY line order
> > + * @V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ABC: C-PHY line order ABC (default)
> > + * @V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ACB: C-PHY line order ACB
> > + * @V4L2_MBUS_CSI2_CPHY_LINE_ORDER_BAC: C-PHY line order BAC
> > + * @V4L2_MBUS_CSI2_CPHY_LINE_ORDER_BCA: C-PHY line order BCA
> > + * @V4L2_MBUS_CSI2_CPHY_LINE_ORDER_CAB: C-PHY line order CAB
> > + * @V4L2_MBUS_CSI2_CPHY_LINE_ORDER_CBA: C-PHY line order CBA
> > + */
> > +enum v4l2_mbus_csi2_cphy_line_orders_type {
> > +       V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ABC,
> > +       V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ACB,
> > +       V4L2_MBUS_CSI2_CPHY_LINE_ORDER_BAC,
> > +       V4L2_MBUS_CSI2_CPHY_LINE_ORDER_BCA,
> > +       V4L2_MBUS_CSI2_CPHY_LINE_ORDER_CAB,
> > +       V4L2_MBUS_CSI2_CPHY_LINE_ORDER_CBA,
> > +};
> 
> ... hence these values must be identical to the corresponding
> MEDIA_BUS_CSI2_CPHY_LINE_ORDER_* in
> include/dt-bindings/media/video-interfaces.h.
> So please provide a safe-guard to make sure they do not become out
> of sync.

This is a good point, I will create a follow up patch to address this.  
Thanks for spotting it!

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> -- 
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> 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

-- 
Kind Regards,
Niklas Söderlund