Re: [PATCH v5 4/4] media: platform: Add Renesas RZ/G2L CRU driver

From: Lad, Prabhakar
Date: Tue Nov 22 2022 - 19:15:37 EST

Next message: Reinette Chatre: "Re: [PATCH v8 08/13] x86/resctrl: Add sysfs interface to read mbm_total_bytes_config"
Previous message: Reinette Chatre: "Re: [PATCH v8 07/13] x86/resctrl: Introduce data structure to support monitor configuration"
In reply to: Lad, Prabhakar: "Re: [PATCH v5 4/4] media: platform: Add Renesas RZ/G2L CRU driver"
Next in thread: Sakari Ailus: "Re: [PATCH v5 4/4] media: platform: Add Renesas RZ/G2L CRU driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Laurent,

On Tue, Nov 22, 2022 at 8:28 PM Lad, Prabhakar
<prabhakar.csengg@xxxxxxxxx> wrote:
>
> Hi Laurent,
>
> Thank you for the review.
>
> On Tue, Nov 22, 2022 at 2:00 AM Laurent Pinchart
> <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
> >
> > Hi Prabhakar,
> >
> > Thank you for the patch.
> >
> > On Wed, Nov 02, 2022 at 12:43:29AM +0000, Prabhakar wrote:
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > >
> > > Add v4l driver for Renesas RZ/G2L Camera data Receiving Unit.
> > >
> > > Based on a patch in the BSP by Hien Huynh
> > > <hien.huynh.px@xxxxxxxxxxx>
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > > ---
> > > -v4 -> v5
> > > * Made sure we call pre_streamon/post_streamoff around s_stream
> > > * Made sure to cleanup on error path in s_stream
> > > * Dropped pre_streamon/post_streamoff callbacks from IP subdev
> > > * Moved the CRU start/stop configuration to IP subdev to avoid
> > > recursively calling pre_streamon/post_streamoff callbacks.
> > >
> > > -v3 -> v4
> > > * Undoing the configuration in case s_stream(1) fails
> > > * Made sure we call post_streamoff in the error path of s_stream(1)
> > >
> > > v2 -> v3
> > > * Switched to PM runtime
> > > * Modeled CRU IP block as a subdev
> > > * Dropped explicitly selecting VIDEO_RZG2L_CSI2 for VIDEO_RZG2L_CRU config
> > > * Replaced v4l2_dev_to_cru macro with inline function notifier_to_cru()
> > > * Dropped id parameter from rvin_mc_parse_of()
> > > * Renamed rzg2l_cru_csi2_init() -> rzg2l_cru_media_init()
> > > * Used dev_err_probe() in rzg2l_cru_probe()
> > > * Replaced devm_reset_control_get() -> devm_reset_control_get_exclusive()
> > > * Prefixed HW_BUFFER_MAX and HW_BUFFER_DEFAULT macros with RZG2L_CRU_
> > > * Moved asserting presetn signal from rzg2l_cru_dma_register() to rzg2l_cru_start_streaming_vq()
> > > * Dropped VB2_READ from VB2 io_modes
> > > * Used dev_dbg() in rzg2l_cru_video_register() and rzg2l_cru_video_unregister()
> > > * Got rid of rzg2l_cru_notify()
> > > * Dropped V4L2_CAP_READWRITE from device caps
> > > * Introduced rzg2l_cru_v4l2_init() for initialization.
> > > * Got rid v4l2_pipeline_pm_get() and used PM in ov5645 sensor driver. Patch posted
> > > https://patchwork.linuxtv.org/project/linux-media/patch/20220927201634.750141-1-prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx/
> > >
> > > v1 -> v2
> > > * No change
> > >
> > > RFC v2 -> v1
> > > * Moved the driver to renesas folder
> > > * Fixed review comments pointed by Jacopo
> > >
> > > RFC v1 -> RFC v2
> > > * Dropped group
> > > * Dropped CSI subdev and implemented as new driver
> > > * Dropped "mc_" from function names
> > > * Moved the driver to renesas folder
> > > ---
> > > .../media/platform/renesas/rzg2l-cru/Kconfig | 16 +
> > > .../media/platform/renesas/rzg2l-cru/Makefile | 3 +
> > > .../platform/renesas/rzg2l-cru/rzg2l-core.c | 370 ++++++
> > > .../platform/renesas/rzg2l-cru/rzg2l-cru.h | 169 +++
> > > .../platform/renesas/rzg2l-cru/rzg2l-ip.c | 283 +++++
> > > .../platform/renesas/rzg2l-cru/rzg2l-video.c | 1086 +++++++++++++++++
> > > 6 files changed, 1927 insertions(+)
> > > create mode 100644 drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
> > > create mode 100644 drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > > create mode 100644 drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> > > create mode 100644 drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > >
> > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/Kconfig b/drivers/media/platform/renesas/rzg2l-cru/Kconfig
> > > index 57c40bb499df..b39818c1f053 100644
> > > --- a/drivers/media/platform/renesas/rzg2l-cru/Kconfig
> > > +++ b/drivers/media/platform/renesas/rzg2l-cru/Kconfig
> > > @@ -15,3 +15,19 @@ config VIDEO_RZG2L_CSI2
> > >
> > > To compile this driver as a module, choose M here: the
> > > module will be called rzg2l-csi2.
> > > +
> > > +config VIDEO_RZG2L_CRU
> > > + tristate "RZ/G2L Camera Receiving Unit (CRU) Driver"
> > > + depends on ARCH_RENESAS || COMPILE_TEST
> > > + depends on V4L_PLATFORM_DRIVERS
> > > + depends on VIDEO_DEV && OF
> > > + select MEDIA_CONTROLLER
> > > + select V4L2_FWNODE
> > > + select VIDEOBUF2_DMA_CONTIG
> > > + select VIDEO_V4L2_SUBDEV_API
> > > + help
> > > + Support for Renesas RZ/G2L (and alike SoC's) Camera Receiving
> > > + Unit (CRU) driver.
> > > +
> > > + To compile this driver as a module, choose M here: the
> > > + module will be called rzg2l-cru.
> > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/Makefile b/drivers/media/platform/renesas/rzg2l-cru/Makefile
> > > index 91ea97a944e6..c4db2632874f 100644
> > > --- a/drivers/media/platform/renesas/rzg2l-cru/Makefile
> > > +++ b/drivers/media/platform/renesas/rzg2l-cru/Makefile
> > > @@ -1,3 +1,6 @@
> > > # SPDX-License-Identifier: GPL-2.0
> > >
> > > obj-$(CONFIG_VIDEO_RZG2L_CSI2) += rzg2l-csi2.o
> > > +
> > > +rzg2l-cru-objs = rzg2l-core.o rzg2l-ip.o rzg2l-video.o
> > > +obj-$(CONFIG_VIDEO_RZG2L_CRU) += rzg2l-cru.o
> > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
> > > new file mode 100644
> > > index 000000000000..7a92fcfee84c
> > > --- /dev/null
> > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
> > > @@ -0,0 +1,370 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Driver for Renesas RZ/G2L CRU
> > > + *
> > > + * Copyright (C) 2022 Renesas Electronics Corp.
> > > + *
> > > + * Based on Renesas R-Car VIN
> > > + * Copyright (C) 2011-2013 Renesas Solutions Corp.
> > > + * Copyright (C) 2013 Cogent Embedded, Inc., <source@xxxxxxxxxxxxxxxxxx>
> > > + * Copyright (C) 2008 Magnus Damm
> > > + */
> > > +
> > > +#include <linux/clk.h>
> > > +#include <linux/module.h>
> > > +#include <linux/mod_devicetable.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/of_graph.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/pm_runtime.h>
> > > +
> > > +#include <media/v4l2-fwnode.h>
> > > +#include <media/v4l2-mc.h>
> > > +
> > > +#include "rzg2l-cru.h"
> > > +
> > > +static inline struct rzg2l_cru_dev *notifier_to_cru(struct v4l2_async_notifier *n)
> > > +{
> > > + return container_of(n, struct rzg2l_cru_dev, notifier);
> > > +}
> > > +
> > > +static int rzg2l_cru_csi2_link_notify(struct media_link *link, u32 flags,
> > > + unsigned int notification)
> > > +{
> > > + struct media_entity *entity;
> > > + struct rzg2l_cru_dev *cru;
> > > + int ret;
> > > +
> > > + ret = v4l2_pipeline_link_notify(link, flags, notification);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + /* Only care about link enablement for CRU nodes. */
> > > + if (!(flags & MEDIA_LNK_FL_ENABLED))
> > > + return 0;
> > > +
> > > + cru = container_of(link->graph_obj.mdev, struct rzg2l_cru_dev, mdev);
> > > + /*
> > > + * Don't allow link changes if any entity in the graph is
> > > + * streaming, modifying the CHSEL register fields can disrupt
> > > + * running streams.
> > > + */
> > > + media_device_for_each_entity(entity, &cru->mdev)
> > > + if (media_entity_is_streaming(entity))
> > > + return -EBUSY;
> > > +
> > > + mutex_lock(&cru->mdev_lock);
> > > + if (media_pad_remote_pad_first(&cru->vdev.entity.pads[0]))
> > > + ret = -EMLINK;
> > > + mutex_unlock(&cru->mdev_lock);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static const struct media_device_ops rzg2l_cru_media_ops = {
> > > + .link_notify = rzg2l_cru_csi2_link_notify,
> > > +};
> > > +
> > > +/* -----------------------------------------------------------------------------
> > > + * Group async notifier
> > > + */
> > > +
> > > +static int rzg2l_cru_group_notify_complete(struct v4l2_async_notifier *notifier)
> > > +{
> > > + struct rzg2l_cru_dev *cru = notifier_to_cru(notifier);
> > > + struct media_entity *source, *sink;
> > > + int ret;
> > > +
> > > + ret = media_device_register(&cru->mdev);
> > > + if (ret)
> > > + return ret;
> >
> > I'd move the v4l2_device_register() call here, as it's the V4L2
> > counterpart of the media device, and handling them together would be
> > best.
> >
> OK, but now that we plan to get rid of rzg2l_cru_csi2_link_notify()
> I'll move this call into rzg2l_cru_dma_register().
>
I misread the comment earlier, I will get rid of
rzg2l_cru_csi2_link_notify() and use v4l2_pipeline_link_notify()
instead. And I will also move the media_device_register() into
rzg2l_cru_dma_register().

Cheers,
Prabhakar

Next message: Reinette Chatre: "Re: [PATCH v8 08/13] x86/resctrl: Add sysfs interface to read mbm_total_bytes_config"
Previous message: Reinette Chatre: "Re: [PATCH v8 07/13] x86/resctrl: Introduce data structure to support monitor configuration"
In reply to: Lad, Prabhakar: "Re: [PATCH v5 4/4] media: platform: Add Renesas RZ/G2L CRU driver"
Next in thread: Sakari Ailus: "Re: [PATCH v5 4/4] media: platform: Add Renesas RZ/G2L CRU driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]