Hejssan, Niklas!
On Wed, Nov 20, 2024 at 10:50:30AM +0100, Niklas Söderlund wrote:
> Hello Sakari,
>
> On 2024-11-20 08:10:42 +0000, Sakari Ailus wrote:
> > Hejssan,
> >
> > On Tue, Nov 19, 2024 at 11:12:47PM +0100, Niklas Söderlund 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@xxxxxxxxxxxx>
> > > ---
> > > drivers/media/v4l2-core/v4l2-fwnode.c | 56 ++++++++++++++++++++++++++-
> > > include/media/v4l2-mediabus.h | 21 ++++++++++
> > > 2 files changed, 76 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> > > index f19c8adf2c61..b8b2b7fb685e 100644
> > > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > > @@ -127,7 +127,7 @@ static int v4l2_fwnode_endpoint_parse_csi2_bus(struct fwnode_handle *fwnode,
> > > {
> > > struct v4l2_mbus_config_mipi_csi2 *bus = &vep->bus.mipi_csi2;
> > > bool have_clk_lane = false, have_data_lanes = false,
> > > - have_lane_polarities = false;
> > > + have_lane_polarities = false, have_line_orders = false;
> > > unsigned int flags = 0, lanes_used = 0;
> > > u32 array[1 + V4L2_MBUS_CSI2_MAX_DATA_LANES];
> > > u32 clock_lane = 0;
> > > @@ -197,6 +197,17 @@ static int v4l2_fwnode_endpoint_parse_csi2_bus(struct fwnode_handle *fwnode,
> > > have_lane_polarities = true;
> > > }
> > >
> > > + rval = fwnode_property_count_u32(fwnode, "line-orders");
> > > + if (rval > 0) {
> > > + if (rval != num_data_lanes) {
> > > + pr_warn("invalid number of line-orders entries (need %u, got %u)\n",
> > > + num_data_lanes, rval);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + have_line_orders = true;
> > > + }
> > > +
> > > if (!fwnode_property_read_u32(fwnode, "clock-lanes", &v)) {
> > > clock_lane = v;
> > > pr_debug("clock lane position %u\n", v);
> > > @@ -250,6 +261,49 @@ 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++) {
> > > + const char *order;
> > > +
> > > + switch (array[i]) {
> > > + case 0:
> > > + order = "ABC";
> > > + break;
> > > + case 1:
> > > + order = "ACB";
> > > + break;
> > > + case 2:
> > > + order = "BAC";
> > > + break;
> > > + case 3:
> > > + order = "BCA";
> > > + break;
> > > + case 4:
> > > + order = "CAB";
> > > + break;
> > > + case 5:
> > > + order = "CBA";
> > > + break;
> >
> > Please use an array instead.
> >
> > > + default:
> > > + 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];
> > > + pr_debug("lane %u line order %s", i, order);
> > > + }
> > > + } else {
> > > + for (i = 0; i < num_data_lanes; i++)
> > > + bus->line_orders[i] = V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ABC;
> >
> > A few lines could be wrapped above.
>
> I'm not sure I understand this comment. Do you mean I could loop over
> num_data_lanes and initialize all lines to ABC before checking
> have_line_orders and that way avoid having to loop here and set the
> default ABC if we are out-of bounds in the switch?
No, just that you'd wrap lines that are over 80 characters per line, unless
there's some tangible reason to have them like that.
--
Med vänliga hälsningar,
Sakari Ailus
