Re: [PATCH v2 3/4] [media] bcm2835-unicam: Driver for CCP2/CSI2 camera interface

[Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 5467 bytes --]

Dave Stevenson <dave.stevenson-FnsA7b+Nu9XbIbC87yuRow@public.gmane.org> writes:
> diff --git a/drivers/media/platform/bcm2835/bcm2835-unicam.c b/drivers/media/platform/bcm2835/bcm2835-unicam.c
> new file mode 100644
> index 0000000..5b1adc3
> --- /dev/null
> +++ b/drivers/media/platform/bcm2835/bcm2835-unicam.c
> @@ -0,0 +1,2192 @@
> +/*
> + * BCM2835 Unicam capture Driver
> + *
> + * Copyright (C) 2017 - Raspberry Pi (Trading) Ltd.
> + *
> + * Dave Stevenson <dave.stevenson-FnsA7b+Nu9XbIbC87yuRow@public.gmane.org>
> + *
> + * Based on TI am437x driver by Benoit Parrot and Lad, Prabhakar and
> + * TI CAL camera interface driver by Benoit Parrot.
> + *

Possible future improvement: this description of the driver is really
nice and could be turned into kernel-doc.

> + * There are two camera drivers in the kernel for BCM283x - this one
> + * and bcm2835-camera (currently in staging).
> + *
> + * This driver is purely the kernel control the Unicam peripheral - there

Maybe "This driver directly controls..."?

> + * is no involvement with the VideoCore firmware. Unicam receives CSI-2
> + * or CCP2 data and writes it into SDRAM. The only processing options are
> + * to repack Bayer data into an alternate format, and applying windowing
> + * (currently not implemented).
> + * It should be possible to connect it to any sensor with a
> + * suitable output interface and V4L2 subdevice driver.
> + *
> + * bcm2835-camera uses with the VideoCore firmware to control the sensor,

"uses the"

> + * Unicam, ISP, and all tuner control loops. Fully processed frames are
> + * delivered to the driver by the firmware. It only has sensor drivers
> + * for Omnivision OV5647, and Sony IMX219 sensors.
> + *
> + * The two drivers are mutually exclusive for the same Unicam instance.
> + * The VideoCore firmware checks the device tree configuration during boot.
> + * If it finds device tree nodes called csi0 or csi1 it will block the
> + * firmware from accessing the peripheral, and bcm2835-camera will
> + * not be able to stream data.

Thanks for describing this here!

> +/*
> + * The peripheral can unpack and repack between several of
> + * the Bayer raw formats, so any Bayer format can be advertised
> + * as the same Bayer order in each of the supported bit depths.
> + * Use lower case to avoid clashing with V4L2_PIX_FMT_SGBRG8
> + * formats.
> + */
> +#define PIX_FMT_ALL_BGGR  v4l2_fourcc('b', 'g', 'g', 'r')
> +#define PIX_FMT_ALL_RGGB  v4l2_fourcc('r', 'g', 'g', 'b')
> +#define PIX_FMT_ALL_GBRG  v4l2_fourcc('g', 'b', 'r', 'g')
> +#define PIX_FMT_ALL_GRBG  v4l2_fourcc('g', 'r', 'b', 'g')

Should thes fourccs be defined in a common v4l2 header, to reserve it
from clashing with others later?

This is really the only question I have about this driver before seeing
it merged.  As far as me wearing my platform maintainer hat, I'm happy
with the driver, and my other little notes are optional.

> +static int unicam_probe(struct platform_device *pdev)
> +{
> +	struct unicam_cfg *unicam_cfg;
> +	struct unicam_device *unicam;
> +	struct v4l2_ctrl_handler *hdl;
> +	struct resource	*res;
> +	int ret;
> +
> +	unicam = devm_kzalloc(&pdev->dev, sizeof(*unicam), GFP_KERNEL);
> +	if (!unicam)
> +		return -ENOMEM;
> +
> +	unicam->pdev = pdev;
> +	unicam_cfg = &unicam->cfg;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	unicam_cfg->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(unicam_cfg->base)) {
> +		unicam_err(unicam, "Failed to get main io block\n");
> +		return PTR_ERR(unicam_cfg->base);
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	unicam_cfg->clk_gate_base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(unicam_cfg->clk_gate_base)) {
> +		unicam_err(unicam, "Failed to get 2nd io block\n");
> +		return PTR_ERR(unicam_cfg->clk_gate_base);
> +	}
> +
> +	unicam->clock = devm_clk_get(&pdev->dev, "lp_clock");
> +	if (IS_ERR(unicam->clock)) {
> +		unicam_err(unicam, "Failed to get clock\n");
> +		return PTR_ERR(unicam->clock);
> +	}
> +
> +	ret = platform_get_irq(pdev, 0);
> +	if (ret <= 0) {
> +		dev_err(&pdev->dev, "No IRQ resource\n");
> +		return -ENODEV;
> +	}
> +	unicam_cfg->irq = ret;
> +
> +	ret = devm_request_irq(&pdev->dev, unicam_cfg->irq, unicam_isr, 0,
> +			       "unicam_capture0", unicam);

Looks like there's no need to keep "irq" in the device private struct.

> +	if (ret) {
> +		dev_err(&pdev->dev, "Unable to request interrupt\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = v4l2_device_register(&pdev->dev, &unicam->v4l2_dev);
> +	if (ret) {
> +		unicam_err(unicam,
> +			   "Unable to register v4l2 device.\n");
> +		return ret;
> +	}
> +
> +	/* Reserve space for the controls */
> +	hdl = &unicam->ctrl_handler;
> +	ret = v4l2_ctrl_handler_init(hdl, 16);
> +	if (ret < 0)
> +		goto probe_out_v4l2_unregister;
> +	unicam->v4l2_dev.ctrl_handler = hdl;
> +
> +	/* set the driver data in platform device */
> +	platform_set_drvdata(pdev, unicam);
> +
> +	ret = of_unicam_connect_subdevs(unicam);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to connect subdevs\n");
> +		goto free_hdl;
> +	}
> +
> +	/* Enabling module functional clock */
> +	pm_runtime_enable(&pdev->dev);

I think pm_runtime is only controlling the power domain for the PHY, not
the clock (which you're handling manually).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]