Re: [PATCH v3 3/9] v4l: platform: Add Renesas CEU driver
Hi Jacopo,
Thank you for the patch.
On Thursday, 4 January 2018 18:03:11 EET Jacopo Mondi wrote:
> Add driver for Renesas Capture Engine Unit (CEU).
>
> The CEU interface supports capturing 'data' (YUV422) and 'images'
> (NV[12|21|16|61]).
>
> This driver aims to replace the soc_camera-based sh_mobile_ceu one.
>
> Tested with ov7670 camera sensor, providing YUYV_2X8 data on Renesas RZ
> platform GR-Peach.
>
> Tested with ov7725 camera sensor on SH4 platform Migo-R.
>
> Signed-off-by: Jacopo Mondi
> ---
> drivers/media/platform/Kconfig |9 +
> drivers/media/platform/Makefile |1 +
> drivers/media/platform/renesas-ceu.c | 1649 +++
> 3 files changed, 1659 insertions(+)
> create mode 100644 drivers/media/platform/renesas-ceu.c
[snip]
> diff --git a/drivers/media/platform/renesas-ceu.c
> b/drivers/media/platform/renesas-ceu.c new file mode 100644
> index 000..a614859
> --- /dev/null
> +++ b/drivers/media/platform/renesas-ceu.c
[snip]
> +/*
> + * struct ceu_data - Platform specific CEU data
> + * @irq_mask: CETCR mask with all interrupt sources enabled. The mask
> differs
> + * between SH4 and RZ platforms.
> + */
> +struct ceu_data {
> + u32 irq_mask;
> +};
> +
> +const struct ceu_data ceu_data_rz = {
> + .irq_mask = CEU_CETCR_ALL_IRQS_RZ,
> +};
> +
> +const struct ceu_data ceu_data_sh4 = {
> + .irq_mask = CEU_CETCR_ALL_IRQS_SH4,
> +};
These two can be const.
> +#if IS_ENABLED(CONFIG_OF)
> +static const struct of_device_id ceu_of_match[] = {
> + { .compatible = "renesas,r7s72100-ceu", .data = &ceu_data_rz },
> + { .compatible = "renesas,ceu", .data = &ceu_data_rz },
Do you need both ? What's your policy for compatible strings ? As far as I
understand there's no generic CEU, as the SH4 and RZ versions are not
compatible. Should the "renesas,ceu" compatible string then be replaced by
"renesas,rz-ceu" and the first entry dropped ?
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, ceu_of_match);
> +#endif
> +
> +static int ceu_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + const struct ceu_data *ceu_data;
> + struct ceu_device *ceudev;
> + struct resource *res;
> + unsigned int irq;
> + int num_subdevs;
> + int ret;
> +
> + ceudev = kzalloc(sizeof(*ceudev), GFP_KERNEL);
> + if (!ceudev)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, ceudev);
> + ceudev->dev = dev;
> +
> + INIT_LIST_HEAD(&ceudev->capture);
> + spin_lock_init(&ceudev->lock);
> + mutex_init(&ceudev->mlock);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + ceudev->base = devm_ioremap_resource(dev, res);
> + if (IS_ERR(ceudev->base))
> + goto error_free_ceudev;
> +
> + ret = platform_get_irq(pdev, 0);
> + if (ret < 0) {
> + dev_err(dev, "Failed to request irq: %d\n", ret);
s/request/get/ (it was correct in v2).
> + goto error_free_ceudev;
> + }
> + irq = ret;
> +
> + ret = devm_request_irq(dev, irq, ceu_irq,
> +0, dev_name(dev), ceudev);
> + if (ret) {
> + dev_err(&pdev->dev, "Unable to register CEU interrupt.\n");
s/register/request/ (this is the message that should have been changed).
> + return ret;
You're leaking ceudev here.
> + }
> +
> + pm_runtime_enable(dev);
> +
> + ret = v4l2_device_register(dev, &ceudev->v4l2_dev);
> + if (ret)
> + goto error_pm_disable;
> +
> + if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
> + ceu_data = of_match_device(ceu_of_match, dev)->data;
> + num_subdevs = ceu_parse_dt(ceudev);
> + } else if (dev->platform_data) {
> + /* Assume SH4 if booting with platform data. */
> + ceu_data = &ceu_data_sh4;
> + num_subdevs = ceu_parse_platform_data(ceudev,
> + dev->platform_data);
> + } else {
> + num_subdevs = -EINVAL;
> + }
> +
> + if (num_subdevs < 0) {
> + ret = num_subdevs;
> + goto error_v4l2_unregister;
> + }
> + ceudev->irq_mask = ceu_data->irq_mask;
> +
> + ceudev->notifier.v4l2_dev = &ceudev->v4l2_dev;
> + ceudev->notifier.subdevs= ceudev->asds;
> + ceudev->notifier.num_subdevs= num_subdevs;
> + ceudev->notifier.ops= &ceu_notify_ops;
> + ret = v4l2_async_notifier_register(&ceudev->v4l2_dev,
> +&ceudev->notifier);
> + if (ret)
> + goto error_v4l2_unregister;
> +
> + dev_info(dev, "Renesas Capture Engine Unit %s\n", dev_name(dev));
> +
> + return 0;
> +
> +error_v4l2_unregister:
> + v4l2_device_unregister(&ceudev->v4l2_dev);
> +error_pm_disable:
> + pm_runtime_disable(dev);
> +error_free_ceudev:
> + kfree(ceudev);
> +
> + return ret;
> +}
--
Regards
[PATCH v3 3/9] v4l: platform: Add Renesas CEU driver
Add driver for Renesas Capture Engine Unit (CEU). The CEU interface supports capturing 'data' (YUV422) and 'images' (NV[12|21|16|61]). This driver aims to replace the soc_camera-based sh_mobile_ceu one. Tested with ov7670 camera sensor, providing YUYV_2X8 data on Renesas RZ platform GR-Peach. Tested with ov7725 camera sensor on SH4 platform Migo-R. Signed-off-by: Jacopo Mondi --- drivers/media/platform/Kconfig |9 + drivers/media/platform/Makefile |1 + drivers/media/platform/renesas-ceu.c | 1649 ++ 3 files changed, 1659 insertions(+) create mode 100644 drivers/media/platform/renesas-ceu.c diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig index fd0c998..fe7bd26 100644 --- a/drivers/media/platform/Kconfig +++ b/drivers/media/platform/Kconfig @@ -144,6 +144,15 @@ config VIDEO_STM32_DCMI To compile this driver as a module, choose M here: the module will be called stm32-dcmi. +config VIDEO_RENESAS_CEU + tristate "Renesas Capture Engine Unit (CEU) driver" + depends on VIDEO_DEV && VIDEO_V4L2 && HAS_DMA + depends on ARCH_SHMOBILE || ARCH_R7S72100 || COMPILE_TEST + select VIDEOBUF2_DMA_CONTIG + select V4L2_FWNODE + ---help--- + This is a v4l2 driver for the Renesas CEU Interface + source "drivers/media/platform/soc_camera/Kconfig" source "drivers/media/platform/exynos4-is/Kconfig" source "drivers/media/platform/am437x/Kconfig" diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile index 003b0bb..6580a6b 100644 --- a/drivers/media/platform/Makefile +++ b/drivers/media/platform/Makefile @@ -62,6 +62,7 @@ obj-$(CONFIG_VIDEO_SH_VOU)+= sh_vou.o obj-$(CONFIG_SOC_CAMERA) += soc_camera/ obj-$(CONFIG_VIDEO_RCAR_DRIF) += rcar_drif.o +obj-$(CONFIG_VIDEO_RENESAS_CEU)+= renesas-ceu.o obj-$(CONFIG_VIDEO_RENESAS_FCP)+= rcar-fcp.o obj-$(CONFIG_VIDEO_RENESAS_FDP1) += rcar_fdp1.o obj-$(CONFIG_VIDEO_RENESAS_JPU)+= rcar_jpu.o diff --git a/drivers/media/platform/renesas-ceu.c b/drivers/media/platform/renesas-ceu.c new file mode 100644 index 000..a614859 --- /dev/null +++ b/drivers/media/platform/renesas-ceu.c @@ -0,0 +1,1649 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * V4L2 Driver for Renesas Capture Engine Unit (CEU) interface + * Copyright (C) 2017-2018 Jacopo Mondi + * + * Based on soc-camera driver "soc_camera/sh_mobile_ceu_camera.c" + * Copyright (C) 2008 Magnus Damm + * + * Based on V4L2 Driver for PXA camera host - "pxa_camera.c", + * Copyright (C) 2006, Sascha Hauer, Pengutronix + * Copyright (C) 2008, Guennadi Liakhovetski + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#define DRIVER_NAME"renesas-ceu" + +/* CEU registers offsets and masks. */ +#define CEU_CAPSR 0x00 /* Capture start register */ +#define CEU_CAPCR 0x04 /* Capture control register*/ +#define CEU_CAMCR 0x08 /* Capture interface control register */ +#define CEU_CAMOR 0x10 /* Capture interface offset register */ +#define CEU_CAPWR 0x14 /* Capture interface width register*/ +#define CEU_CAIFR 0x18 /* Capture interface input format register */ +#define CEU_CRCNTR 0x28 /* CEU register control register */ +#define CEU_CRCMPR 0x2c /* CEU register forcible control register */ +#define CEU_CFLCR 0x30 /* Capture filter control register */ +#define CEU_CFSZR 0x34 /* Capture filter size clip register */ +#define CEU_CDWDR 0x38 /* Capture destination width register */ +#define CEU_CDAYR 0x3c /* Capture data address Y register */ +#define CEU_CDACR 0x40 /* Capture data address C register */ +#define CEU_CFWCR 0x5c /* Firewall operation control register */ +#define CEU_CDOCR 0x64 /* Capture data output control register*/ +#define CEU_CEIER 0x70 /* Capture event interrupt enable register */ +#define CEU_CETCR 0x74 /* Capture event flag clear register */ +#define CEU_CSTSR 0x7c /* Capture status register */ +#define CEU_CSRTR 0x80 /* Capture software reset register */ + +/* Data synchronous fetch mode. */ +#define CEU_CAMCR_JPEG BIT(4) + +/* Input components ordering: CEU_CAMCR.DTARY field. */ +#define CEU_CAMCR_DTARY_8_UYVY (0x00 << 8) +#define CEU_CAMCR_DTARY_8_VYUY (0x01 << 8) +#define CEU_CAMCR_DTARY_8_YUYV (0x02 << 8) +#define CEU_CAMCR_DTARY_8_YVYU (0x03 << 8) +/* TODO: input components ordering for 16 bits input. */ + +/* Bus transfer MTU. */ +#define CEU_CAPC
