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 <jacopo+renesas@xxxxxxxxxx>
> ---
> 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 0000000..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,
Laurent Pinchart
