linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Sakari Ailus <sakari.ailus@iki.fi>
Cc: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>,
	dave.stevenson@raspberrypi.com, devicetree@vger.kernel.org,
	kernel-list@raspberrypi.com,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, linux-media@vger.kernel.org,
	linux-rpi-kernel@lists.infradead.org, lukasz@jany.st,
	mchehab@kernel.org, naush@raspberrypi.com, robh@kernel.org,
	tomi.valkeinen@ideasonboard.com,
	bcm-kernel-feedback-list@broadcom.com, stefan.wahren@i2se.com
Subject: Re: [PATCH v5 04/11] media: bcm2835-unicam: Add support for CCP2/CSI2 camera interface
Date: Mon, 3 Jul 2023 00:47:11 +0300	[thread overview]
Message-ID: <20230702214711.GC16995@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20230702214505.GB16995@pendragon.ideasonboard.com>

Hi Sakari,

On Mon, Jul 03, 2023 at 12:45:07AM +0300, Laurent Pinchart wrote:
> On Sun, Jul 02, 2023 at 06:18:21PM +0000, Sakari Ailus wrote:
> > On Sun, Jul 02, 2023 at 06:23:56PM +0300, Laurent Pinchart wrote:
> > > On Fri, Feb 25, 2022 at 11:29:18AM +0200, Sakari Ailus wrote:
> > > > On Tue, Feb 08, 2022 at 04:50:20PM +0100, Jean-Michel Hautbois wrote:
> > > > > Add driver for the Unicam camera receiver block on BCM283x processors.
> > > > > It is represented as two video device nodes: unicam-image and
> > > > > unicam-embedded which are connected to an internal subdev (named
> > > > > unicam-subdev) in order to manage streams routing.
> > > > > 
> > > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> > > > > 
> > > > > ---
> > > > > v4:
> > > > >   - Add the vendor prefox for DT name
> > > > >   - Use the reg-names in DT parsing
> > > > >   - Remove MAINTAINERS entry
> > > > > 
> > > > > v3 main changes:
> > > > >   - Change code organization
> > > > >   - Remove unused variables
> > > > >   - Correct the fmt_meta functions
> > > > >   - Rewrite the start/stop streaming
> > > > >     - You can now start the image node alone, but not the metadata one
> > > > >     - The buffers are allocated per-node
> > > > >     - only the required stream is started, if the route exists and is
> > > > >       enabled
> > > > >   - Prefix the macros with UNICAM_ to not have too generic names
> > > > >   - Drop colorspace support
> > > > >     -> This is causing issues in the try-fmt v4l2-compliance test
> > > > >   test VIDIOC_G_FMT: OK
> > > > > 	fail: v4l2-test-formats.cpp(363): colorspace >= 0xff
> > > > > 	fail: v4l2-test-formats.cpp(465): testColorspace(!node->is_io_mc, pix.pixelformat, pix.colorspace, pix.ycbcr_enc, pix.quantization)
> > > > >   test VIDIOC_TRY_FMT: FAIL
> > > > > 	fail: v4l2-test-formats.cpp(363): colorspace >= 0xff
> > > > > 	fail: v4l2-test-formats.cpp(465): testColorspace(!node->is_io_mc, pix.pixelformat, pix.colorspace, pix.ycbcr_enc, pix.quantization)
> > > > >   test VIDIOC_S_FMT: FAIL
> > > > > 
> > > > > v2: Remove the unicam_{info,debug,error} macros and use
> > > > > dev_dbg/dev_err instead.
> > > > > ---
> > > > >  drivers/media/platform/Kconfig                |    1 +
> > > > >  drivers/media/platform/Makefile               |    2 +
> > > > >  drivers/media/platform/bcm2835/Kconfig        |   21 +
> > > > >  drivers/media/platform/bcm2835/Makefile       |    3 +
> > > > >  .../platform/bcm2835/bcm2835-unicam-regs.h    |  253 ++
> > > > >  .../media/platform/bcm2835/bcm2835-unicam.c   | 2570 +++++++++++++++++
> > > > >  6 files changed, 2850 insertions(+)
> > > > >  create mode 100644 drivers/media/platform/bcm2835/Kconfig
> > > > >  create mode 100644 drivers/media/platform/bcm2835/Makefile
> > > > >  create mode 100644 drivers/media/platform/bcm2835/bcm2835-unicam-regs.h
> > > > >  create mode 100644 drivers/media/platform/bcm2835/bcm2835-unicam.c
> > > 
> > > [snip]
> > > 
> > > > > diff --git a/drivers/media/platform/bcm2835/bcm2835-unicam-regs.h b/drivers/media/platform/bcm2835/bcm2835-unicam-regs.h
> > > > > new file mode 100644
> > > > > index 000000000000..b8d297076a02
> > > > > --- /dev/null
> > > > > +++ b/drivers/media/platform/bcm2835/bcm2835-unicam-regs.h
> > > 
> > > [snip]
> > > 
> > > > > +static int unicam_connect_of_subdevs(struct unicam_device *unicam)
> > > > > +{
> > > > > +	struct v4l2_fwnode_endpoint ep = { };
> > > > > +	struct fwnode_handle *ep_handle;
> > > > > +	struct v4l2_async_subdev *asd;
> > > > > +	unsigned int lane;
> > > > > +	int ret = -EINVAL;
> > > > > +
> > > > > +	if (of_property_read_u32(unicam->dev->of_node, "brcm,num-data-lanes",
> > > > > +				 &unicam->max_data_lanes) < 0) {
> > > > 
> > > > As you're already using fwnode API below, you could use
> > > > device_property_read_u32() here.
> > > > 
> > > > You can then replace of_device.h by mod_devicetable.h. Up to you.
> > > > 
> > > > > +		dev_err(unicam->dev, "DT property %s not set\n",
> > > > > +			"brcm,num-data-lanes");
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +
> > > > > +	/* Get the local endpoint and remote device. */
> > > > > +	ep_handle = fwnode_graph_get_endpoint_by_id(dev_fwnode(unicam->dev),
> > > > > +						    0, 0,
> > > > > +						    FWNODE_GRAPH_ENDPOINT_NEXT);
> > > > > +	if (!ep_handle) {
> > > > > +		dev_err(unicam->dev, "No endpoint\n");
> > > > > +		return -ENODEV;
> > > > > +	}
> > > > > +
> > > > > +	/* Parse the local endpoint and validate its configuration. */
> > > > > +	if (v4l2_fwnode_endpoint_alloc_parse(ep_handle, &ep)) {
> > > > 
> > > > As you don't need link-frequencies property parsing, you should use
> > > > v4l2_fwnode_endpoint_parse(). That avoids having to call
> > > > v4l2_fwnode_endpoint_free().
> > > > 
> > > > > +		dev_err(unicam->dev, "could not parse endpoint\n");
> > > > > +		goto cleanup_exit;
> > > > > +	}
> > > > > +
> > > > > +	dev_dbg(unicam->dev, "parsed local endpoint, bus_type %u\n",
> > > > > +		ep.bus_type);
> > > > > +
> > > > > +	unicam->bus_type = ep.bus_type;
> > > > > +
> > > > > +	switch (ep.bus_type) {
> > > > > +	case V4L2_MBUS_CSI2_DPHY:
> > > > > +		switch (ep.bus.mipi_csi2.num_data_lanes) {
> > > > > +		case 1:
> > > > > +		case 2:
> > > > > +		case 4:
> > > > > +			break;
> > > > > +
> > > > > +		default:
> > > > > +			dev_err(unicam->dev, "%u data lanes not supported\n",
> > > > > +				ep.bus.mipi_csi2.num_data_lanes);
> > > > > +			goto cleanup_exit;
> > > > > +		}
> > > > > +
> > > > > +		for (lane = 0; lane < ep.bus.mipi_csi2.num_data_lanes; lane++) {
> > > > > +			if (ep.bus.mipi_csi2.data_lanes[lane] != lane + 1) {
> > > > > +				dev_err(unicam->dev, "data lanes reordering not supported\n");
> > > > > +				goto cleanup_exit;
> > > > > +			}
> > > > > +		}
> > > > > +
> > > > > +		if (ep.bus.mipi_csi2.num_data_lanes > unicam->max_data_lanes) {
> > > > > +			dev_err(unicam->dev, "endpoint requires %u data lanes when %u are supported\n",
> > > > > +				ep.bus.mipi_csi2.num_data_lanes,
> > > > > +				unicam->max_data_lanes);
> > > > > +		}
> > > > > +
> > > > > +		unicam->active_data_lanes = ep.bus.mipi_csi2.num_data_lanes;
> > > > > +		unicam->bus_flags = ep.bus.mipi_csi2.flags;
> > > > > +
> > > > > +		break;
> > > > > +
> > > > > +	case V4L2_MBUS_CCP2:
> > > > > +		if (ep.bus.mipi_csi1.clock_lane != 0 ||
> > > > > +		    ep.bus.mipi_csi1.data_lane != 1) {
> > > > > +			dev_err(unicam->dev, "unsupported lanes configuration\n");
> > > > 
> > > > If the hardware doesn't support lane remapping for CCP2, then that should
> > > > be reflected in DT bindings, i.e. data-lanes isn't relevant. There's no
> > > > need to check that here.
> > > 
> > > Should the above check for CSI-2 be dropped as well then ?
> > 
> > Same for CSI-2, too: if there's nothing to configure there (lane remapping)
> > there's no need to validate that part of the DT either.
> 
> OK, I'll drop that.

Actually, I'm wondering if it would make sense to tell the parsing
functions whether lane reordering is supported or not. The checks could
then be moved to the framework. What do you think ?

> > > > > +			goto cleanup_exit;
> > > > > +		}
> > > > > +
> > > > > +		unicam->max_data_lanes = 1;
> > > > > +		unicam->active_data_lanes = 1;
> > > > > +		unicam->bus_flags = ep.bus.mipi_csi1.strobe;
> > > > > +		break;
> > > > > +
> > > > > +	default:
> > > > > +		/* Unsupported bus type */
> > > > > +		dev_err(unicam->dev, "unsupported bus type %u\n",
> > > > > +			ep.bus_type);
> > > > > +		goto cleanup_exit;
> > > > > +	}
> > > > > +
> > > > > +	dev_dbg(unicam->dev, "%s bus, %u data lanes, flags=0x%08x\n",
> > > > > +		unicam->bus_type == V4L2_MBUS_CSI2_DPHY ? "CSI-2" : "CCP2",
> > > > > +		unicam->active_data_lanes, unicam->bus_flags);
> > > > 
> > > > V4l2-fwnode already prints this information I believe.
> > > 
> > > True. It does so with pr_debug() though, it would be nice to use
> > > dev_dbg(). That's a candidate for a separate fix of course.
> > 
> > The reason for using pr_debug() is that the device isn't used by the fwnode
> > framework. Would you add that just for debug prints? Note that the device
> > nodes themselves are already being printed so it adds little to the
> > usefulness of the messages.
> 
> I'll send patches, we can discuss their usefulness there.

-- 
Regards,

Laurent Pinchart

  reply	other threads:[~2023-07-02 21:47 UTC|newest]

Thread overview: 57+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-02-08 15:50 [PATCH v5 00/11] Add support for BCM2835 camera interface (unicam) Jean-Michel Hautbois
2022-02-08 15:50 ` [PATCH v5 01/11] media: v4l: Add V4L2-PIX-FMT-Y12P format Jean-Michel Hautbois
2022-02-08 15:50 ` [PATCH v5 02/11] media: v4l: Add V4L2-PIX-FMT-Y14P format Jean-Michel Hautbois
2022-02-08 15:50 ` [PATCH v5 03/11] dt-bindings: media: Add bindings for bcm2835-unicam Jean-Michel Hautbois
2022-02-09 18:56   ` Rob Herring
2022-02-13 15:48   ` Stefan Wahren
2022-02-14  9:39     ` Maxime Ripard
2022-02-14  9:54       ` Laurent Pinchart
2022-02-14 11:32         ` Stefan Wahren
2022-02-21  7:10           ` Laurent Pinchart
2022-02-21 10:03             ` Maxime Ripard
2022-02-21 12:45             ` Stefan Wahren
2022-02-21 12:52               ` Laurent Pinchart
2022-02-25  8:19   ` Sakari Ailus
2022-02-08 15:50 ` [PATCH v5 04/11] media: bcm2835-unicam: Add support for CCP2/CSI2 camera interface Jean-Michel Hautbois
2022-02-08 21:00   ` Stefan Wahren
2022-02-13 12:52     ` Laurent Pinchart
2022-02-13 11:17   ` Stefan Wahren
2022-02-13 12:49     ` Laurent Pinchart
2022-02-20 10:01   ` Stefan Wahren
2022-02-20 10:08     ` Laurent Pinchart
2022-02-21  9:55   ` Laurent Pinchart
2022-02-25  9:29   ` Sakari Ailus
2023-07-02 15:23     ` Laurent Pinchart
2023-07-02 18:18       ` Sakari Ailus
2023-07-02 21:45         ` Laurent Pinchart
2023-07-02 21:47           ` Laurent Pinchart [this message]
2023-07-02 21:56             ` Sakari Ailus
2023-07-02 22:01               ` Laurent Pinchart
2023-07-02 22:20                 ` Sakari Ailus
2023-07-02 22:28                   ` Laurent Pinchart
2023-07-02 22:33                     ` Sakari Ailus
2023-07-02 21:53           ` Sakari Ailus
2023-07-02 21:58             ` Laurent Pinchart
2022-02-08 15:50 ` [PATCH v5 05/11] media: MAINTAINERS: add bcm2835 unicam driver Jean-Michel Hautbois
2022-02-08 15:58   ` Laurent Pinchart
2022-02-08 15:50 ` [PATCH v5 06/11] ARM: dts: bcm2711: Add unicam CSI nodes Jean-Michel Hautbois
2022-02-13 10:35   ` Stefan Wahren
2022-02-13 13:51     ` Stefan Wahren
2022-02-23 14:34   ` [PATCH v5.1 1/2] ARM: dts: bcm2835-rpi: Move the firmware clocks Jean-Michel Hautbois
2022-02-23 14:34     ` [PATCH v5.1 2/2] ARM: dts: bcm2711: Add unicam CSI nodes Jean-Michel Hautbois
2022-02-24 17:03       ` Stefan Wahren
2022-02-24 17:07         ` Jean-Michel Hautbois
2022-02-24 21:26           ` Stefan Wahren
2022-02-23 14:41     ` [PATCH v5.1 1/2] ARM: dts: bcm2835-rpi: Move the firmware clocks Maxime Ripard
2022-02-08 15:50 ` [PATCH v5 07/11] media: imx219: Rename mbus codes array Jean-Michel Hautbois
2022-02-08 15:50 ` [PATCH v5 08/11] media: imx219: Switch from open to init_cfg Jean-Michel Hautbois
2022-02-08 16:02   ` Laurent Pinchart
2022-02-08 16:05     ` Laurent Pinchart
2022-02-08 15:50 ` [PATCH v5 09/11] media: imx219: Introduce the set_routing operation Jean-Michel Hautbois
2022-02-21  7:17   ` Laurent Pinchart
2022-02-08 15:50 ` [PATCH v5 10/11] media: imx219: use a local v4l2_subdev to simplify reading Jean-Michel Hautbois
2022-02-08 15:50 ` [PATCH v5 11/11] media: imx219: Add support for the V4L2 subdev active state Jean-Michel Hautbois
2022-02-21  7:25   ` Laurent Pinchart
2022-02-16 20:57 ` [PATCH v5 00/11] Add support for BCM2835 camera interface (unicam) Stefan Wahren
2022-02-20 14:30   ` Jean-Michel Hautbois
2022-02-26 17:18     ` Stefan Wahren

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230702214711.GC16995@pendragon.ideasonboard.com \
    --to=laurent.pinchart@ideasonboard.com \
    --cc=bcm-kernel-feedback-list@broadcom.com \
    --cc=dave.stevenson@raspberrypi.com \
    --cc=devicetree@vger.kernel.org \
    --cc=jeanmichel.hautbois@ideasonboard.com \
    --cc=kernel-list@raspberrypi.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-rpi-kernel@lists.infradead.org \
    --cc=lukasz@jany.st \
    --cc=mchehab@kernel.org \
    --cc=naush@raspberrypi.com \
    --cc=robh@kernel.org \
    --cc=sakari.ailus@iki.fi \
    --cc=stefan.wahren@i2se.com \
    --cc=tomi.valkeinen@ideasonboard.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).