[v5] media: rcar-isp: Add Renesas R-Car Image Signal Processor driver

  Add a V4L2 driver for Renesas R-Car Image Signal Processor. The driver
supports the R-Car V3U SoC where the ISP IP sits between the R-Car CSI-2
receiver and VIN and filters the CSI-2 data based on VC/DT and directs
the video stream to different VIN IPs.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
* Changes since v4
- Update to the new async function names.
- Do not depend on external s_power, instead control the device power at stream start/stop.
- Do not look at remote upstream subdevice format when starting (or at all).
- Serialise access for format variables in get_fmt and set_fmt.
- Make use of v4l2_subdev_link_validate_default()
- Add mutex_destroy() on error paths and in remove function.
- Propperly disable runtime PM if subdev registration fails.

* Changes since v3
- Fix spelling in header information.
- Add parentheses around macro expansions.
- Add explicit blocks to for-loop body.
- Add documentation for mutex.

* Changes since v2
- Add description of IPS capabilities and that only channel selection is
  currently supported.
- Update signature of v4l2_subdev_pad_ops set_fmt.
- Switch to pm_runtime_resume_and_get().
- Make better use of devm_kzalloc().
---
 MAINTAINERS                       |   1 +
 drivers/media/platform/Kconfig    |  16 +
 drivers/media/platform/Makefile   |   1 +
 drivers/media/platform/rcar-isp.c | 517 ++++++++++++++++++++++++++++++
 4 files changed, 535 insertions(+)
 create mode 100644 drivers/media/platform/rcar-isp.c
  

On 14/09/2021 16:00, Niklas Söderlund wrote:
> Add a V4L2 driver for Renesas R-Car Image Signal Processor. The driver
> supports the R-Car V3U SoC where the ISP IP sits between the R-Car CSI-2
> receiver and VIN and filters the CSI-2 data based on VC/DT and directs
> the video stream to different VIN IPs.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
> * Changes since v4
> - Update to the new async function names.
> - Do not depend on external s_power, instead control the device power at stream start/stop.
> - Do not look at remote upstream subdevice format when starting (or at all).
> - Serialise access for format variables in get_fmt and set_fmt.
> - Make use of v4l2_subdev_link_validate_default()
> - Add mutex_destroy() on error paths and in remove function.
> - Propperly disable runtime PM if subdev registration fails.
> 
> * Changes since v3
> - Fix spelling in header information.
> - Add parentheses around macro expansions.
> - Add explicit blocks to for-loop body.
> - Add documentation for mutex.
> 
> * Changes since v2
> - Add description of IPS capabilities and that only channel selection is
>   currently supported.
> - Update signature of v4l2_subdev_pad_ops set_fmt.
> - Switch to pm_runtime_resume_and_get().
> - Make better use of devm_kzalloc().
> ---
>  MAINTAINERS                       |   1 +
>  drivers/media/platform/Kconfig    |  16 +
>  drivers/media/platform/Makefile   |   1 +
>  drivers/media/platform/rcar-isp.c | 517 ++++++++++++++++++++++++++++++
>  4 files changed, 535 insertions(+)
>  create mode 100644 drivers/media/platform/rcar-isp.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 08c9612e769af24a..8164d1bd0a4df351 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11575,6 +11575,7 @@ T:	git git://linuxtv.org/media_tree.git
>  F:	Documentation/devicetree/bindings/media/renesas,csi2.yaml
>  F:	Documentation/devicetree/bindings/media/renesas,isp.yaml
>  F:	Documentation/devicetree/bindings/media/renesas,vin.yaml
> +F:	drivers/media/platform/rcar-isp.c
>  F:	drivers/media/platform/rcar-vin/
>  
>  MEDIA DRIVERS FOR RENESAS - VSP1
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index aa277a19e2759617..b95e9368110c7848 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -200,6 +200,22 @@ config VIDEO_TI_CAL_MC
>  
>  endif # VIDEO_TI_CAL
>  
> +config VIDEO_RCAR_ISP
> +	tristate "R-Car Image Signal Processor (ISP)"
> +	depends on VIDEO_V4L2 && OF
> +	depends on ARCH_RENESAS || COMPILE_TEST
> +	select MEDIA_CONTROLLER
> +	select VIDEO_V4L2_SUBDEV_API
> +	select RESET_CONTROLLER
> +	select V4L2_FWNODE
> +	help
> +	  Support for Renesas R-Car Image Signal Processor (ISP).
> +	  Enable this to support the Renesas R-Car Image Signal
> +	  Processor (ISP).
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called rcar-isp.
> +
>  endif # V4L_PLATFORM_DRIVERS
>  
>  menuconfig V4L_MEM2MEM_DRIVERS
> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> index 73ce083c2fc6b51b..a148553babfc0944 100644
> --- a/drivers/media/platform/Makefile
> +++ b/drivers/media/platform/Makefile
> @@ -63,6 +63,7 @@ obj-$(CONFIG_VIDEO_AM437X_VPFE)		+= am437x/
>  
>  obj-$(CONFIG_VIDEO_XILINX)		+= xilinx/
>  
> +obj-$(CONFIG_VIDEO_RCAR_ISP)		+= rcar-isp.o
>  obj-$(CONFIG_VIDEO_RCAR_VIN)		+= rcar-vin/
>  
>  obj-$(CONFIG_VIDEO_ATMEL_ISC)		+= atmel/
> diff --git a/drivers/media/platform/rcar-isp.c b/drivers/media/platform/rcar-isp.c
> new file mode 100644
> index 0000000000000000..1055ca87975c8676
> --- /dev/null
> +++ b/drivers/media/platform/rcar-isp.c
> @@ -0,0 +1,517 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2021 Renesas Electronics Corp.
> + *
> + * Driver for Renesas R-Car ISP Channel Selector
> + *
> + * The ISP hardware is capable of more than just channel selection, features
> + * such as demosaicing, white balance control and color space conversion are
> + * also possible. These more advanced features are not supported by the driver
> + * due to lack of documentation.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/reset.h>
> +
> +#include <media/v4l2-subdev.h>
> +
> +#define ISPINPUTSEL0_REG				0x0008
> +#define ISPINPUTSEL0_SEL_CSI0				BIT(31)
> +
> +#define ISPSTART_REG					0x0014
> +#define ISPSTART_START					0xffff
> +#define ISPSTART_STOP					0x0000
> +
> +#define ISPPROCMODE_DT_REG(n)				(0x1100 + (0x4 * (n)))
> +#define ISPPROCMODE_DT_PROC_MODE_VC3(pm)		(((pm) & 0x3f) << 24)
> +#define ISPPROCMODE_DT_PROC_MODE_VC2(pm)		(((pm) & 0x3f) << 16)
> +#define ISPPROCMODE_DT_PROC_MODE_VC1(pm)		(((pm) & 0x3f) << 8)
> +#define ISPPROCMODE_DT_PROC_MODE_VC0(pm)		((pm) & 0x3f)
> +
> +#define ISPCS_FILTER_ID_CH_REG(n)			(0x3000 + (0x0100 * (n)))
> +
> +#define ISPCS_DT_CODE03_CH_REG(n)			(0x3008 + (0x100 * (n)))
> +#define ISPCS_DT_CODE03_EN3				BIT(31)
> +#define ISPCS_DT_CODE03_DT3(dt)				(((dt) & 0x3f) << 24)
> +#define ISPCS_DT_CODE03_EN2				BIT(23)
> +#define ISPCS_DT_CODE03_DT2(dt)				(((dt) & 0x3f) << 16)
> +#define ISPCS_DT_CODE03_EN1				BIT(15)
> +#define ISPCS_DT_CODE03_DT1(dt)				(((dt) & 0x3f) << 8)
> +#define ISPCS_DT_CODE03_EN0				BIT(7)
> +#define ISPCS_DT_CODE03_DT0(dt)				((dt) & 0x3f)
> +
> +struct rcar_isp_format {
> +	u32 code;
> +	unsigned int datatype;
> +	unsigned int procmode;
> +};
> +
> +static const struct rcar_isp_format rcar_isp_formats[] = {
> +	{ .code = MEDIA_BUS_FMT_RGB888_1X24,	.datatype = 0x24, .procmode = 0x15 },
> +	{ .code = MEDIA_BUS_FMT_Y10_1X10,	.datatype = 0x2b, .procmode = 0x10 },
> +	{ .code = MEDIA_BUS_FMT_UYVY8_1X16,	.datatype = 0x1e, .procmode = 0x0c },
> +	{ .code = MEDIA_BUS_FMT_YUYV8_1X16,	.datatype = 0x1e, .procmode = 0x0c },
> +	{ .code = MEDIA_BUS_FMT_UYVY8_2X8,	.datatype = 0x1e, .procmode = 0x0c },
> +	{ .code = MEDIA_BUS_FMT_YUYV10_2X10,	.datatype = 0x1e, .procmode = 0x0c },
> +};
> +
> +static const struct rcar_isp_format *risp_code_to_fmt(unsigned int code)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(rcar_isp_formats); i++) {
> +		if (rcar_isp_formats[i].code == code)
> +			return &rcar_isp_formats[i];
> +	}
> +
> +	return NULL;
> +}
> +
> +enum rcar_isp_input {
> +	RISP_CSI_INPUT0,
> +	RISP_CSI_INPUT1,
> +};
> +
> +enum rcar_isp_pads {
> +	RCAR_ISP_SINK,
> +	RCAR_ISP_PORT0,
> +	RCAR_ISP_PORT1,
> +	RCAR_ISP_PORT2,
> +	RCAR_ISP_PORT3,
> +	RCAR_ISP_PORT4,
> +	RCAR_ISP_PORT5,
> +	RCAR_ISP_PORT6,
> +	RCAR_ISP_PORT7,
> +	RCAR_ISP_MAX_PAD,

Hmm, I don't like this name. How about RCAR_ISP_NUM_PADS?

Since the maximum pad is RCAR_ISP_PORT7, not RCAR_ISP_MAX_PAD.

> +};
> +
> +struct rcar_isp {
> +	struct device *dev;
> +	void __iomem *base;
> +	struct reset_control *rstc;
> +
> +	enum rcar_isp_input csi_input;
> +
> +	struct v4l2_subdev subdev;
> +	struct media_pad pads[RCAR_ISP_MAX_PAD];
> +
> +	struct v4l2_async_notifier notifier;
> +	struct v4l2_subdev *remote;
> +	unsigned int remote_pad;
> +
> +	struct mutex lock; /* Protects mf and stream_count. */
> +	struct v4l2_mbus_framefmt mf;
> +	int stream_count;
> +};
> +
> +static inline struct rcar_isp *sd_to_isp(struct v4l2_subdev *sd)
> +{
> +	return container_of(sd, struct rcar_isp, subdev);
> +}
> +
> +static inline struct rcar_isp *notifier_to_isp(struct v4l2_async_notifier *n)
> +{
> +	return container_of(n, struct rcar_isp, notifier);
> +}
> +
> +static void risp_write(struct rcar_isp *isp, u32 offset, u32 value)
> +{
> +	iowrite32(value, isp->base + offset);
> +}
> +
> +static u32 risp_read(struct rcar_isp *isp, u32 offset)
> +{
> +	return ioread32(isp->base + offset);
> +}
> +
> +static int risp_power_on(struct rcar_isp *isp)
> +{
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(isp->dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = reset_control_deassert(isp->rstc);
> +	if (ret < 0) {
> +		pm_runtime_put(isp->dev);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void risp_power_off(struct rcar_isp *isp)
> +{
> +	reset_control_assert(isp->rstc);
> +	pm_runtime_put(isp->dev);
> +}
> +
> +static int risp_start(struct rcar_isp *isp)
> +{
> +	const struct rcar_isp_format *format;
> +	unsigned int vc;
> +	u32 sel_csi = 0;
> +	int ret;
> +
> +	format = risp_code_to_fmt(isp->mf.code);
> +	if (!format) {
> +		dev_err(isp->dev, "Unsupported bus format\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = risp_power_on(isp);
> +	if (ret) {
> +		dev_err(isp->dev, "Failed to power on ISP\n");
> +		return ret;
> +	}
> +
> +	/* Select CSI-2 input source. */
> +	if (isp->csi_input == RISP_CSI_INPUT1)
> +		sel_csi = ISPINPUTSEL0_SEL_CSI0;
> +
> +	risp_write(isp, ISPINPUTSEL0_REG,
> +		   risp_read(isp, ISPINPUTSEL0_REG) | sel_csi);
> +
> +	/* Configure Channel Selector. */
> +	for (vc = 0; vc < 4; vc++) {
> +		u8 ch = vc + 4;
> +		u8 dt = format->datatype;
> +
> +		risp_write(isp, ISPCS_FILTER_ID_CH_REG(ch), BIT(vc));
> +		risp_write(isp, ISPCS_DT_CODE03_CH_REG(ch),
> +			   ISPCS_DT_CODE03_EN3 | ISPCS_DT_CODE03_DT3(dt) |
> +			   ISPCS_DT_CODE03_EN2 | ISPCS_DT_CODE03_DT2(dt) |
> +			   ISPCS_DT_CODE03_EN1 | ISPCS_DT_CODE03_DT1(dt) |
> +			   ISPCS_DT_CODE03_EN0 | ISPCS_DT_CODE03_DT0(dt));
> +	}
> +
> +	/* Setup processing method. */
> +	risp_write(isp, ISPPROCMODE_DT_REG(format->datatype),
> +		   ISPPROCMODE_DT_PROC_MODE_VC3(format->procmode) |
> +		   ISPPROCMODE_DT_PROC_MODE_VC2(format->procmode) |
> +		   ISPPROCMODE_DT_PROC_MODE_VC1(format->procmode) |
> +		   ISPPROCMODE_DT_PROC_MODE_VC0(format->procmode));
> +
> +	/* Start ISP. */
> +	risp_write(isp, ISPSTART_REG, ISPSTART_START);
> +
> +	ret = v4l2_subdev_call(isp->remote, video, s_stream, 1);
> +	if (ret)
> +		risp_power_off(isp);
> +
> +	return ret;
> +}
> +
> +static void risp_stop(struct rcar_isp *isp)
> +{
> +	v4l2_subdev_call(isp->remote, video, s_stream, 0);
> +
> +	/* Stop ISP. */
> +	risp_write(isp, ISPSTART_REG, ISPSTART_STOP);
> +
> +	risp_power_off(isp);
> +}
> +
> +static int risp_s_stream(struct v4l2_subdev *sd, int enable)
> +{
> +	struct rcar_isp *isp = sd_to_isp(sd);
> +	int ret = 0;
> +
> +	mutex_lock(&isp->lock);
> +
> +	if (!isp->remote) {
> +		ret = -ENODEV;
> +		goto out;
> +	}
> +
> +	if (enable && isp->stream_count == 0) {
> +		ret = risp_start(isp);
> +		if (ret)
> +			goto out;
> +	} else if (!enable && isp->stream_count == 1) {
> +		risp_stop(isp);
> +	}
> +
> +	isp->stream_count += enable ? 1 : -1;
> +out:
> +	mutex_unlock(&isp->lock);
> +
> +	return ret;
> +}
> +
> +static const struct v4l2_subdev_video_ops risp_video_ops = {
> +	.s_stream = risp_s_stream,
> +};
> +
> +static int risp_set_pad_format(struct v4l2_subdev *sd,
> +			       struct v4l2_subdev_state *sd_state,
> +			       struct v4l2_subdev_format *format)
> +{
> +	struct rcar_isp *isp = sd_to_isp(sd);
> +	struct v4l2_mbus_framefmt *framefmt;
> +
> +	mutex_lock(&isp->lock);
> +
> +	if (!risp_code_to_fmt(format->format.code))
> +		format->format.code = rcar_isp_formats[0].code;
> +
> +	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> +		isp->mf = format->format;
> +	} else {
> +		framefmt = v4l2_subdev_get_try_format(sd, sd_state, 0);
> +		*framefmt = format->format;
> +	}
> +
> +	mutex_unlock(&isp->lock);
> +
> +	return 0;
> +}
> +
> +static int risp_get_pad_format(struct v4l2_subdev *sd,
> +			       struct v4l2_subdev_state *sd_state,
> +			       struct v4l2_subdev_format *format)
> +{
> +	struct rcar_isp *isp = sd_to_isp(sd);
> +
> +	mutex_lock(&isp->lock);
> +
> +	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> +		format->format = isp->mf;
> +	else
> +		format->format = *v4l2_subdev_get_try_format(sd, sd_state, 0);
> +
> +	mutex_unlock(&isp->lock);
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_subdev_pad_ops risp_pad_ops = {
> +	.set_fmt = risp_set_pad_format,
> +	.get_fmt = risp_get_pad_format,
> +	.link_validate = v4l2_subdev_link_validate_default,
> +};
> +
> +static const struct v4l2_subdev_ops rcar_isp_subdev_ops = {
> +	.video	= &risp_video_ops,
> +	.pad	= &risp_pad_ops,
> +};
> +
> +/* -----------------------------------------------------------------------------
> + * Async handling and registration of subdevices and links
> + */
> +
> +static int risp_notify_bound(struct v4l2_async_notifier *notifier,
> +			     struct v4l2_subdev *subdev,
> +			     struct v4l2_async_subdev *asd)
> +{
> +	struct rcar_isp *isp = notifier_to_isp(notifier);
> +	int pad;
> +
> +	pad = media_entity_get_fwnode_pad(&subdev->entity, asd->match.fwnode,
> +					  MEDIA_PAD_FL_SOURCE);
> +	if (pad < 0) {
> +		dev_err(isp->dev, "Failed to find pad for %s\n", subdev->name);
> +		return pad;
> +	}
> +
> +	isp->remote = subdev;
> +	isp->remote_pad = pad;
> +
> +	dev_dbg(isp->dev, "Bound %s pad: %d\n", subdev->name, pad);
> +
> +	return media_create_pad_link(&subdev->entity, pad,
> +				     &isp->subdev.entity, 0,
> +				     MEDIA_LNK_FL_ENABLED |
> +				     MEDIA_LNK_FL_IMMUTABLE);
> +}
> +
> +static void risp_notify_unbind(struct v4l2_async_notifier *notifier,
> +			       struct v4l2_subdev *subdev,
> +			       struct v4l2_async_subdev *asd)
> +{
> +	struct rcar_isp *isp = notifier_to_isp(notifier);
> +
> +	isp->remote = NULL;
> +
> +	dev_dbg(isp->dev, "Unbind %s\n", subdev->name);
> +}
> +
> +static const struct v4l2_async_notifier_operations risp_notify_ops = {
> +	.bound = risp_notify_bound,
> +	.unbind = risp_notify_unbind,
> +};
> +
> +static int risp_parse_dt(struct rcar_isp *isp)
> +{
> +	struct v4l2_async_subdev *asd;
> +	struct fwnode_handle *fwnode;
> +	struct fwnode_handle *ep;
> +	unsigned int id;
> +	int ret;
> +
> +	for (id = 0; id < 2; id++) {
> +		ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(isp->dev),
> +						     0, id, 0);
> +		if (ep)
> +			break;
> +	}
> +
> +	if (!ep) {
> +		dev_err(isp->dev, "Not connected to subdevice\n");
> +		return -EINVAL;
> +	}
> +
> +	if (id == 1)
> +		isp->csi_input = RISP_CSI_INPUT1;
> +
> +	fwnode = fwnode_graph_get_remote_endpoint(ep);
> +	fwnode_handle_put(ep);
> +
> +	dev_dbg(isp->dev, "Found '%pOF'\n", to_of_node(fwnode));
> +
> +	v4l2_async_nf_init(&isp->notifier);
> +	isp->notifier.ops = &risp_notify_ops;
> +
> +	asd = v4l2_async_nf_add_fwnode(&isp->notifier, fwnode,
> +				       struct v4l2_async_subdev);
> +	fwnode_handle_put(fwnode);
> +	if (IS_ERR(asd))
> +		return PTR_ERR(asd);
> +
> +	ret = v4l2_async_subdev_nf_register(&isp->subdev, &isp->notifier);
> +	if (ret)
> +		v4l2_async_nf_cleanup(&isp->notifier);
> +
> +	return ret;
> +}
> +
> +/* -----------------------------------------------------------------------------
> + * Platform Device Driver
> + */
> +
> +static const struct media_entity_operations risp_entity_ops = {
> +	.link_validate = v4l2_subdev_link_validate,
> +};
> +
> +static int risp_probe_resources(struct rcar_isp *isp,
> +				struct platform_device *pdev)
> +{
> +	struct resource *res;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	isp->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(isp->base))
> +		return PTR_ERR(isp->base);
> +
> +	isp->rstc = devm_reset_control_get(&pdev->dev, NULL);
> +
> +	return PTR_ERR_OR_ZERO(isp->rstc);
> +}
> +
> +static const struct of_device_id risp_of_id_table[] = {
> +	{ .compatible = "renesas,r8a779a0-isp" },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, risp_of_id_table);
> +
> +static int risp_probe(struct platform_device *pdev)
> +{
> +	struct rcar_isp *isp;
> +	unsigned int i;
> +	int ret;
> +
> +	isp = devm_kzalloc(&pdev->dev, sizeof(*isp), GFP_KERNEL);
> +	if (!isp)
> +		return -ENOMEM;
> +
> +	isp->dev = &pdev->dev;
> +
> +	mutex_init(&isp->lock);
> +
> +	ret = risp_probe_resources(isp, pdev);
> +	if (ret) {
> +		dev_err(isp->dev, "Failed to get resources\n");
> +		goto error_mutex;
> +	}
> +
> +	platform_set_drvdata(pdev, isp);
> +
> +	pm_runtime_enable(&pdev->dev);
> +
> +	ret = risp_parse_dt(isp);
> +	if (ret)
> +		goto error_pm;
> +
> +	isp->subdev.owner = THIS_MODULE;
> +	isp->subdev.dev = &pdev->dev;
> +	v4l2_subdev_init(&isp->subdev, &rcar_isp_subdev_ops);
> +	v4l2_set_subdevdata(&isp->subdev, &pdev->dev);
> +	snprintf(isp->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s %s",
> +		 KBUILD_MODNAME, dev_name(&pdev->dev));
> +	isp->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
> +
> +	isp->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;

Is this correct? Based on the description it is really more a MEDIA_ENT_F_VID_MUX.

Regards,

	Hans

> +	isp->subdev.entity.ops = &risp_entity_ops;
> +
> +	isp->pads[RCAR_ISP_SINK].flags = MEDIA_PAD_FL_SINK;
> +	for (i = RCAR_ISP_PORT0; i < RCAR_ISP_MAX_PAD; i++)
> +		isp->pads[i].flags = MEDIA_PAD_FL_SOURCE;
> +
> +	ret = media_entity_pads_init(&isp->subdev.entity, RCAR_ISP_MAX_PAD,
> +				     isp->pads);
> +	if (ret)
> +		goto error_notifier;
> +
> +	ret = v4l2_async_register_subdev(&isp->subdev);
> +	if (ret < 0)
> +		goto error_notifier;
> +
> +	dev_info(isp->dev, "Using CSI-2 input: %u\n", isp->csi_input);
> +
> +	return 0;
> +error_notifier:
> +	v4l2_async_nf_unregister(&isp->notifier);
> +	v4l2_async_nf_cleanup(&isp->notifier);
> +error_pm:
> +	pm_runtime_disable(&pdev->dev);
> +error_mutex:
> +	mutex_destroy(&isp->lock);
> +
> +	return ret;
> +}
> +
> +static int risp_remove(struct platform_device *pdev)
> +{
> +	struct rcar_isp *isp = platform_get_drvdata(pdev);
> +
> +	v4l2_async_nf_unregister(&isp->notifier);
> +	v4l2_async_nf_cleanup(&isp->notifier);
> +
> +	v4l2_async_unregister_subdev(&isp->subdev);
> +
> +	pm_runtime_disable(&pdev->dev);
> +
> +	mutex_destroy(&isp->lock);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver rcar_isp_driver = {
> +	.driver = {
> +		.name = "rcar-isp",
> +		.of_match_table = risp_of_id_table,
> +	},
> +	.probe = risp_probe,
> +	.remove = risp_remove,
> +};
> +
> +module_platform_driver(rcar_isp_driver);
> +
> +MODULE_AUTHOR("Niklas Söderlund <niklas.soderlund@ragnatech.se>");
> +MODULE_DESCRIPTION("Renesas R-Car ISP Channel Selector driver");
> +MODULE_LICENSE("GPL");
>
  

Hi Niklas,

On Tue, Sep 14, 2021 at 04:00:57PM +0200, Niklas Söderlund wrote:
> Add a V4L2 driver for Renesas R-Car Image Signal Processor. The driver
> supports the R-Car V3U SoC where the ISP IP sits between the R-Car CSI-2
> receiver and VIN and filters the CSI-2 data based on VC/DT and directs
> the video stream to different VIN IPs.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
> * Changes since v4
> - Update to the new async function names.
> - Do not depend on external s_power, instead control the device power at stream start/stop.
> - Do not look at remote upstream subdevice format when starting (or at all).
> - Serialise access for format variables in get_fmt and set_fmt.
> - Make use of v4l2_subdev_link_validate_default()
> - Add mutex_destroy() on error paths and in remove function.
> - Propperly disable runtime PM if subdev registration fails.
>
> * Changes since v3
> - Fix spelling in header information.
> - Add parentheses around macro expansions.
> - Add explicit blocks to for-loop body.
> - Add documentation for mutex.
>
> * Changes since v2
> - Add description of IPS capabilities and that only channel selection is
>   currently supported.
> - Update signature of v4l2_subdev_pad_ops set_fmt.
> - Switch to pm_runtime_resume_and_get().
> - Make better use of devm_kzalloc().
> ---
>  MAINTAINERS                       |   1 +
>  drivers/media/platform/Kconfig    |  16 +
>  drivers/media/platform/Makefile   |   1 +
>  drivers/media/platform/rcar-isp.c | 517 ++++++++++++++++++++++++++++++
>  4 files changed, 535 insertions(+)
>  create mode 100644 drivers/media/platform/rcar-isp.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 08c9612e769af24a..8164d1bd0a4df351 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11575,6 +11575,7 @@ T:	git git://linuxtv.org/media_tree.git
>  F:	Documentation/devicetree/bindings/media/renesas,csi2.yaml
>  F:	Documentation/devicetree/bindings/media/renesas,isp.yaml
>  F:	Documentation/devicetree/bindings/media/renesas,vin.yaml
> +F:	drivers/media/platform/rcar-isp.c
>  F:	drivers/media/platform/rcar-vin/
>
>  MEDIA DRIVERS FOR RENESAS - VSP1
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index aa277a19e2759617..b95e9368110c7848 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -200,6 +200,22 @@ config VIDEO_TI_CAL_MC
>
>  endif # VIDEO_TI_CAL
>
> +config VIDEO_RCAR_ISP
> +	tristate "R-Car Image Signal Processor (ISP)"
> +	depends on VIDEO_V4L2 && OF
> +	depends on ARCH_RENESAS || COMPILE_TEST
> +	select MEDIA_CONTROLLER
> +	select VIDEO_V4L2_SUBDEV_API
> +	select RESET_CONTROLLER
> +	select V4L2_FWNODE
> +	help
> +	  Support for Renesas R-Car Image Signal Processor (ISP).
> +	  Enable this to support the Renesas R-Car Image Signal
> +	  Processor (ISP).
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called rcar-isp.
> +
>  endif # V4L_PLATFORM_DRIVERS
>
>  menuconfig V4L_MEM2MEM_DRIVERS
> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> index 73ce083c2fc6b51b..a148553babfc0944 100644
> --- a/drivers/media/platform/Makefile
> +++ b/drivers/media/platform/Makefile
> @@ -63,6 +63,7 @@ obj-$(CONFIG_VIDEO_AM437X_VPFE)		+= am437x/
>
>  obj-$(CONFIG_VIDEO_XILINX)		+= xilinx/
>
> +obj-$(CONFIG_VIDEO_RCAR_ISP)		+= rcar-isp.o
>  obj-$(CONFIG_VIDEO_RCAR_VIN)		+= rcar-vin/
>
>  obj-$(CONFIG_VIDEO_ATMEL_ISC)		+= atmel/
> diff --git a/drivers/media/platform/rcar-isp.c b/drivers/media/platform/rcar-isp.c
> new file mode 100644
> index 0000000000000000..1055ca87975c8676
> --- /dev/null
> +++ b/drivers/media/platform/rcar-isp.c
> @@ -0,0 +1,517 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2021 Renesas Electronics Corp.
> + *
> + * Driver for Renesas R-Car ISP Channel Selector
> + *
> + * The ISP hardware is capable of more than just channel selection, features
> + * such as demosaicing, white balance control and color space conversion are
> + * also possible. These more advanced features are not supported by the driver
> + * due to lack of documentation.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/reset.h>
> +
> +#include <media/v4l2-subdev.h>
> +
> +#define ISPINPUTSEL0_REG				0x0008
> +#define ISPINPUTSEL0_SEL_CSI0				BIT(31)
> +
> +#define ISPSTART_REG					0x0014
> +#define ISPSTART_START					0xffff
> +#define ISPSTART_STOP					0x0000
> +
> +#define ISPPROCMODE_DT_REG(n)				(0x1100 + (0x4 * (n)))
> +#define ISPPROCMODE_DT_PROC_MODE_VC3(pm)		(((pm) & 0x3f) << 24)
> +#define ISPPROCMODE_DT_PROC_MODE_VC2(pm)		(((pm) & 0x3f) << 16)
> +#define ISPPROCMODE_DT_PROC_MODE_VC1(pm)		(((pm) & 0x3f) << 8)
> +#define ISPPROCMODE_DT_PROC_MODE_VC0(pm)		((pm) & 0x3f)
> +
> +#define ISPCS_FILTER_ID_CH_REG(n)			(0x3000 + (0x0100 * (n)))
> +
> +#define ISPCS_DT_CODE03_CH_REG(n)			(0x3008 + (0x100 * (n)))
> +#define ISPCS_DT_CODE03_EN3				BIT(31)
> +#define ISPCS_DT_CODE03_DT3(dt)				(((dt) & 0x3f) << 24)
> +#define ISPCS_DT_CODE03_EN2				BIT(23)
> +#define ISPCS_DT_CODE03_DT2(dt)				(((dt) & 0x3f) << 16)
> +#define ISPCS_DT_CODE03_EN1				BIT(15)
> +#define ISPCS_DT_CODE03_DT1(dt)				(((dt) & 0x3f) << 8)
> +#define ISPCS_DT_CODE03_EN0				BIT(7)
> +#define ISPCS_DT_CODE03_DT0(dt)				((dt) & 0x3f)
> +
> +struct rcar_isp_format {
> +	u32 code;
> +	unsigned int datatype;
> +	unsigned int procmode;
> +};
> +
> +static const struct rcar_isp_format rcar_isp_formats[] = {
> +	{ .code = MEDIA_BUS_FMT_RGB888_1X24,	.datatype = 0x24, .procmode = 0x15 },
> +	{ .code = MEDIA_BUS_FMT_Y10_1X10,	.datatype = 0x2b, .procmode = 0x10 },
> +	{ .code = MEDIA_BUS_FMT_UYVY8_1X16,	.datatype = 0x1e, .procmode = 0x0c },
> +	{ .code = MEDIA_BUS_FMT_YUYV8_1X16,	.datatype = 0x1e, .procmode = 0x0c },
> +	{ .code = MEDIA_BUS_FMT_UYVY8_2X8,	.datatype = 0x1e, .procmode = 0x0c },
> +	{ .code = MEDIA_BUS_FMT_YUYV10_2X10,	.datatype = 0x1e, .procmode = 0x0c },
                                            ^ tab                ^ space

> +};
> +
> +static const struct rcar_isp_format *risp_code_to_fmt(unsigned int code)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(rcar_isp_formats); i++) {
> +		if (rcar_isp_formats[i].code == code)
> +			return &rcar_isp_formats[i];
> +	}
> +
> +	return NULL;
> +}
> +
> +enum rcar_isp_input {
> +	RISP_CSI_INPUT0,
> +	RISP_CSI_INPUT1,
> +};
> +
> +enum rcar_isp_pads {
> +	RCAR_ISP_SINK,
> +	RCAR_ISP_PORT0,
> +	RCAR_ISP_PORT1,
> +	RCAR_ISP_PORT2,
> +	RCAR_ISP_PORT3,
> +	RCAR_ISP_PORT4,
> +	RCAR_ISP_PORT5,
> +	RCAR_ISP_PORT6,
> +	RCAR_ISP_PORT7,
> +	RCAR_ISP_MAX_PAD,
> +};
> +
> +struct rcar_isp {
> +	struct device *dev;
> +	void __iomem *base;
> +	struct reset_control *rstc;
> +
> +	enum rcar_isp_input csi_input;
> +
> +	struct v4l2_subdev subdev;
> +	struct media_pad pads[RCAR_ISP_MAX_PAD];
> +
> +	struct v4l2_async_notifier notifier;
> +	struct v4l2_subdev *remote;
> +	unsigned int remote_pad;
> +
> +	struct mutex lock; /* Protects mf and stream_count. */
> +	struct v4l2_mbus_framefmt mf;
> +	int stream_count;
> +};
> +
> +static inline struct rcar_isp *sd_to_isp(struct v4l2_subdev *sd)
> +{
> +	return container_of(sd, struct rcar_isp, subdev);
> +}
> +
> +static inline struct rcar_isp *notifier_to_isp(struct v4l2_async_notifier *n)
> +{
> +	return container_of(n, struct rcar_isp, notifier);
> +}
> +
> +static void risp_write(struct rcar_isp *isp, u32 offset, u32 value)
> +{
> +	iowrite32(value, isp->base + offset);
> +}
> +
> +static u32 risp_read(struct rcar_isp *isp, u32 offset)
> +{
> +	return ioread32(isp->base + offset);
> +}
> +
> +static int risp_power_on(struct rcar_isp *isp)
> +{
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(isp->dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = reset_control_deassert(isp->rstc);
> +	if (ret < 0) {
> +		pm_runtime_put(isp->dev);
> +		return ret;
> +	}

I still think you should define a .resume and .suspend callbacks
through something like SIMPLE_DEV_PM_OPS which assert/deassert the
reset controller and just call pm_runtime_resume_and_get() and
pm_runtime_put() at risp_start() and risp_stop() times.

Otherwise am I wrong thinking that pm_runtime is only here for refcounting ?

> +
> +	return 0;
> +}
> +
> +static void risp_power_off(struct rcar_isp *isp)
> +{
> +	reset_control_assert(isp->rstc);
> +	pm_runtime_put(isp->dev);
> +}
> +
> +static int risp_start(struct rcar_isp *isp)
> +{
> +	const struct rcar_isp_format *format;
> +	unsigned int vc;
> +	u32 sel_csi = 0;
> +	int ret;
> +
> +	format = risp_code_to_fmt(isp->mf.code);
> +	if (!format) {
> +		dev_err(isp->dev, "Unsupported bus format\n");
> +		return -EINVAL;
> +	}

I don't think this can now happen as isp->mf.code is set through
set_fmt and it's adjusted

> +
> +	ret = risp_power_on(isp);
> +	if (ret) {
> +		dev_err(isp->dev, "Failed to power on ISP\n");
> +		return ret;
> +	}
> +
> +	/* Select CSI-2 input source. */
> +	if (isp->csi_input == RISP_CSI_INPUT1)
> +		sel_csi = ISPINPUTSEL0_SEL_CSI0;
> +
> +	risp_write(isp, ISPINPUTSEL0_REG,
> +		   risp_read(isp, ISPINPUTSEL0_REG) | sel_csi);
> +
> +	/* Configure Channel Selector. */

If I'm not mistaken we have a fixed routing from channel 4-7 to
VIN[0-3] as routing is controlled by VC/DT matching. If that's the
case, should this be recorded ?

> +	for (vc = 0; vc < 4; vc++) {
> +		u8 ch = vc + 4;
> +		u8 dt = format->datatype;
> +
> +		risp_write(isp, ISPCS_FILTER_ID_CH_REG(ch), BIT(vc));
> +		risp_write(isp, ISPCS_DT_CODE03_CH_REG(ch),
> +			   ISPCS_DT_CODE03_EN3 | ISPCS_DT_CODE03_DT3(dt) |
> +			   ISPCS_DT_CODE03_EN2 | ISPCS_DT_CODE03_DT2(dt) |
> +			   ISPCS_DT_CODE03_EN1 | ISPCS_DT_CODE03_DT1(dt) |
> +			   ISPCS_DT_CODE03_EN0 | ISPCS_DT_CODE03_DT0(dt));
> +	}
> +
> +	/* Setup processing method. */
> +	risp_write(isp, ISPPROCMODE_DT_REG(format->datatype),
> +		   ISPPROCMODE_DT_PROC_MODE_VC3(format->procmode) |
> +		   ISPPROCMODE_DT_PROC_MODE_VC2(format->procmode) |
> +		   ISPPROCMODE_DT_PROC_MODE_VC1(format->procmode) |
> +		   ISPPROCMODE_DT_PROC_MODE_VC0(format->procmode));
> +
> +	/* Start ISP. */
> +	risp_write(isp, ISPSTART_REG, ISPSTART_START);
> +
> +	ret = v4l2_subdev_call(isp->remote, video, s_stream, 1);
> +	if (ret)
> +		risp_power_off(isp);
> +
> +	return ret;
> +}
> +
> +static void risp_stop(struct rcar_isp *isp)
> +{
> +	v4l2_subdev_call(isp->remote, video, s_stream, 0);
> +
> +	/* Stop ISP. */
> +	risp_write(isp, ISPSTART_REG, ISPSTART_STOP);
> +
> +	risp_power_off(isp);
> +}
> +
> +static int risp_s_stream(struct v4l2_subdev *sd, int enable)
> +{
> +	struct rcar_isp *isp = sd_to_isp(sd);
> +	int ret = 0;
> +
> +	mutex_lock(&isp->lock);
> +
> +	if (!isp->remote) {
> +		ret = -ENODEV;
> +		goto out;
> +	}
> +
> +	if (enable && isp->stream_count == 0) {
> +		ret = risp_start(isp);
> +		if (ret)
> +			goto out;
> +	} else if (!enable && isp->stream_count == 1) {
> +		risp_stop(isp);
> +	}
> +
> +	isp->stream_count += enable ? 1 : -1;
> +out:
> +	mutex_unlock(&isp->lock);
> +
> +	return ret;
> +}
> +
> +static const struct v4l2_subdev_video_ops risp_video_ops = {
> +	.s_stream = risp_s_stream,
> +};
> +
> +static int risp_set_pad_format(struct v4l2_subdev *sd,
> +			       struct v4l2_subdev_state *sd_state,
> +			       struct v4l2_subdev_format *format)
> +{
> +	struct rcar_isp *isp = sd_to_isp(sd);
> +	struct v4l2_mbus_framefmt *framefmt;
> +
> +	mutex_lock(&isp->lock);
> +
> +	if (!risp_code_to_fmt(format->format.code))
> +		format->format.code = rcar_isp_formats[0].code;
> +
> +	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> +		isp->mf = format->format;
> +	} else {
> +		framefmt = v4l2_subdev_get_try_format(sd, sd_state, 0);
> +		*framefmt = format->format;
> +	}
> +
> +	mutex_unlock(&isp->lock);
> +
> +	return 0;
> +}
> +
> +static int risp_get_pad_format(struct v4l2_subdev *sd,
> +			       struct v4l2_subdev_state *sd_state,
> +			       struct v4l2_subdev_format *format)
> +{
> +	struct rcar_isp *isp = sd_to_isp(sd);
> +
> +	mutex_lock(&isp->lock);
> +
> +	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> +		format->format = isp->mf;
> +	else
> +		format->format = *v4l2_subdev_get_try_format(sd, sd_state, 0);
> +
> +	mutex_unlock(&isp->lock);
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_subdev_pad_ops risp_pad_ops = {
> +	.set_fmt = risp_set_pad_format,
> +	.get_fmt = risp_get_pad_format,
> +	.link_validate = v4l2_subdev_link_validate_default,
> +};
> +
> +static const struct v4l2_subdev_ops rcar_isp_subdev_ops = {
> +	.video	= &risp_video_ops,
> +	.pad	= &risp_pad_ops,
> +};
> +
> +/* -----------------------------------------------------------------------------
> + * Async handling and registration of subdevices and links
> + */
> +
> +static int risp_notify_bound(struct v4l2_async_notifier *notifier,
> +			     struct v4l2_subdev *subdev,
> +			     struct v4l2_async_subdev *asd)
> +{
> +	struct rcar_isp *isp = notifier_to_isp(notifier);
> +	int pad;
> +
> +	pad = media_entity_get_fwnode_pad(&subdev->entity, asd->match.fwnode,
> +					  MEDIA_PAD_FL_SOURCE);
> +	if (pad < 0) {
> +		dev_err(isp->dev, "Failed to find pad for %s\n", subdev->name);
> +		return pad;
> +	}
> +
> +	isp->remote = subdev;
> +	isp->remote_pad = pad;

remote_pad is not used anywhere now that the format comes from the
local pad.

> +
> +	dev_dbg(isp->dev, "Bound %s pad: %d\n", subdev->name, pad);
> +
> +	return media_create_pad_link(&subdev->entity, pad,
> +				     &isp->subdev.entity, 0,
> +				     MEDIA_LNK_FL_ENABLED |
> +				     MEDIA_LNK_FL_IMMUTABLE);
> +}
> +
> +static void risp_notify_unbind(struct v4l2_async_notifier *notifier,
> +			       struct v4l2_subdev *subdev,
> +			       struct v4l2_async_subdev *asd)
> +{
> +	struct rcar_isp *isp = notifier_to_isp(notifier);
> +
> +	isp->remote = NULL;
> +
> +	dev_dbg(isp->dev, "Unbind %s\n", subdev->name);
> +}
> +
> +static const struct v4l2_async_notifier_operations risp_notify_ops = {
> +	.bound = risp_notify_bound,
> +	.unbind = risp_notify_unbind,
> +};
> +
> +static int risp_parse_dt(struct rcar_isp *isp)
> +{
> +	struct v4l2_async_subdev *asd;
> +	struct fwnode_handle *fwnode;
> +	struct fwnode_handle *ep;
> +	unsigned int id;
> +	int ret;
> +
> +	for (id = 0; id < 2; id++) {
> +		ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(isp->dev),
> +						     0, id, 0);
> +		if (ep)
> +			break;
> +	}
> +
> +	if (!ep) {
> +		dev_err(isp->dev, "Not connected to subdevice\n");
> +		return -EINVAL;
> +	}
> +
> +	if (id == 1)
> +		isp->csi_input = RISP_CSI_INPUT1;
> +
> +	fwnode = fwnode_graph_get_remote_endpoint(ep);
> +	fwnode_handle_put(ep);
> +
> +	dev_dbg(isp->dev, "Found '%pOF'\n", to_of_node(fwnode));
> +
> +	v4l2_async_nf_init(&isp->notifier);
> +	isp->notifier.ops = &risp_notify_ops;
> +
> +	asd = v4l2_async_nf_add_fwnode(&isp->notifier, fwnode,
> +				       struct v4l2_async_subdev);
> +	fwnode_handle_put(fwnode);
> +	if (IS_ERR(asd))
> +		return PTR_ERR(asd);
> +
> +	ret = v4l2_async_subdev_nf_register(&isp->subdev, &isp->notifier);
> +	if (ret)
> +		v4l2_async_nf_cleanup(&isp->notifier);
> +
> +	return ret;
> +}
> +
> +/* -----------------------------------------------------------------------------
> + * Platform Device Driver
> + */
> +
> +static const struct media_entity_operations risp_entity_ops = {
> +	.link_validate = v4l2_subdev_link_validate,
> +};
> +
> +static int risp_probe_resources(struct rcar_isp *isp,
> +				struct platform_device *pdev)
> +{
> +	struct resource *res;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	isp->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(isp->base))
> +		return PTR_ERR(isp->base);
> +
> +	isp->rstc = devm_reset_control_get(&pdev->dev, NULL);
> +
> +	return PTR_ERR_OR_ZERO(isp->rstc);
> +}
> +
> +static const struct of_device_id risp_of_id_table[] = {
> +	{ .compatible = "renesas,r8a779a0-isp" },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, risp_of_id_table);
> +
> +static int risp_probe(struct platform_device *pdev)
> +{
> +	struct rcar_isp *isp;
> +	unsigned int i;
> +	int ret;
> +
> +	isp = devm_kzalloc(&pdev->dev, sizeof(*isp), GFP_KERNEL);
> +	if (!isp)
> +		return -ENOMEM;
> +
> +	isp->dev = &pdev->dev;
> +
> +	mutex_init(&isp->lock);
> +
> +	ret = risp_probe_resources(isp, pdev);
> +	if (ret) {
> +		dev_err(isp->dev, "Failed to get resources\n");
> +		goto error_mutex;
> +	}
> +
> +	platform_set_drvdata(pdev, isp);
> +
> +	pm_runtime_enable(&pdev->dev);
> +
> +	ret = risp_parse_dt(isp);
> +	if (ret)
> +		goto error_pm;
> +
> +	isp->subdev.owner = THIS_MODULE;
> +	isp->subdev.dev = &pdev->dev;
> +	v4l2_subdev_init(&isp->subdev, &rcar_isp_subdev_ops);
> +	v4l2_set_subdevdata(&isp->subdev, &pdev->dev);
> +	snprintf(isp->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s %s",
> +		 KBUILD_MODNAME, dev_name(&pdev->dev));
> +	isp->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
> +
> +	isp->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
> +	isp->subdev.entity.ops = &risp_entity_ops;
> +
> +	isp->pads[RCAR_ISP_SINK].flags = MEDIA_PAD_FL_SINK;
> +	for (i = RCAR_ISP_PORT0; i < RCAR_ISP_MAX_PAD; i++)
> +		isp->pads[i].flags = MEDIA_PAD_FL_SOURCE;
> +
> +	ret = media_entity_pads_init(&isp->subdev.entity, RCAR_ISP_MAX_PAD,
> +				     isp->pads);
> +	if (ret)
> +		goto error_notifier;
> +
> +	ret = v4l2_async_register_subdev(&isp->subdev);
> +	if (ret < 0)
> +		goto error_notifier;
> +
> +	dev_info(isp->dev, "Using CSI-2 input: %u\n", isp->csi_input);
> +
> +	return 0;

Maybe an empy line to highlight the error path

Anyway, all minors, you'll have a new version anyway and pm_runtime
usage has been under the eyes of Sakari and Hans for 5 versions and
they have not complained so I guess it's fine.

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

> +error_notifier:
> +	v4l2_async_nf_unregister(&isp->notifier);
> +	v4l2_async_nf_cleanup(&isp->notifier);
> +error_pm:
> +	pm_runtime_disable(&pdev->dev);
> +error_mutex:
> +	mutex_destroy(&isp->lock);
> +
> +	return ret;
> +}
> +
> +static int risp_remove(struct platform_device *pdev)
> +{
> +	struct rcar_isp *isp = platform_get_drvdata(pdev);
> +
> +	v4l2_async_nf_unregister(&isp->notifier);
> +	v4l2_async_nf_cleanup(&isp->notifier);
> +
> +	v4l2_async_unregister_subdev(&isp->subdev);
> +
> +	pm_runtime_disable(&pdev->dev);
> +
> +	mutex_destroy(&isp->lock);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver rcar_isp_driver = {
> +	.driver = {
> +		.name = "rcar-isp",
> +		.of_match_table = risp_of_id_table,
> +	},
> +	.probe = risp_probe,
> +	.remove = risp_remove,
> +};
> +
> +module_platform_driver(rcar_isp_driver);
> +
> +MODULE_AUTHOR("Niklas Söderlund <niklas.soderlund@ragnatech.se>");
> +MODULE_DESCRIPTION("Renesas R-Car ISP Channel Selector driver");
> +MODULE_LICENSE("GPL");


> --
> 2.33.0
>
  

Hi Hans, Niklas,

On Mon, Oct 04, 2021 at 11:11:55AM +0200, Hans Verkuil wrote:
> > +	isp->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
> 
> Is this correct? Based on the description it is really more a MEDIA_ENT_F_VID_MUX.

We also have MEDIA_ENT_F_PROC_VIDEO_ISP nowadays. Just a note. But VID_MUX
would seem appropriate based on what it appears to be doing.

Niklas: what does the device do with the data other than redirect it to
some of its outputs?
  

Hello,

On 2021-10-04 13:28:07 +0300, Sakari Ailus wrote:
> Hi Hans, Niklas,
> 
> On Mon, Oct 04, 2021 at 11:11:55AM +0200, Hans Verkuil wrote:
> > > +	isp->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
> > 
> > Is this correct? Based on the description it is really more a MEDIA_ENT_F_VID_MUX.
> 
> We also have MEDIA_ENT_F_PROC_VIDEO_ISP nowadays. Just a note. But VID_MUX
> would seem appropriate based on what it appears to be doing.
> 
> Niklas: what does the device do with the data other than redirect it to
> some of its outputs?

It have other ISP capabilities. I would love to extend the driver to 
support it, but the documentation I have only covers whats in this 
driver, muxing. I don't suspect this will change so I agree VID_MUX make 
most sens.

> 
> -- 
> Sakari Ailus
  

Hi Jacopo,

Thanks for your review.

On 2021-10-04 12:21:59 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Tue, Sep 14, 2021 at 04:00:57PM +0200, Niklas Söderlund wrote:
> > Add a V4L2 driver for Renesas R-Car Image Signal Processor. The driver
> > supports the R-Car V3U SoC where the ISP IP sits between the R-Car CSI-2
> > receiver and VIN and filters the CSI-2 data based on VC/DT and directs
> > the video stream to different VIN IPs.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> > * Changes since v4
> > - Update to the new async function names.
> > - Do not depend on external s_power, instead control the device power at stream start/stop.
> > - Do not look at remote upstream subdevice format when starting (or at all).
> > - Serialise access for format variables in get_fmt and set_fmt.
> > - Make use of v4l2_subdev_link_validate_default()
> > - Add mutex_destroy() on error paths and in remove function.
> > - Propperly disable runtime PM if subdev registration fails.
> >
> > * Changes since v3
> > - Fix spelling in header information.
> > - Add parentheses around macro expansions.
> > - Add explicit blocks to for-loop body.
> > - Add documentation for mutex.
> >
> > * Changes since v2
> > - Add description of IPS capabilities and that only channel selection is
> >   currently supported.
> > - Update signature of v4l2_subdev_pad_ops set_fmt.
> > - Switch to pm_runtime_resume_and_get().
> > - Make better use of devm_kzalloc().
> > ---
> >  MAINTAINERS                       |   1 +
> >  drivers/media/platform/Kconfig    |  16 +
> >  drivers/media/platform/Makefile   |   1 +
> >  drivers/media/platform/rcar-isp.c | 517 ++++++++++++++++++++++++++++++
> >  4 files changed, 535 insertions(+)
> >  create mode 100644 drivers/media/platform/rcar-isp.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 08c9612e769af24a..8164d1bd0a4df351 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -11575,6 +11575,7 @@ T:	git git://linuxtv.org/media_tree.git
> >  F:	Documentation/devicetree/bindings/media/renesas,csi2.yaml
> >  F:	Documentation/devicetree/bindings/media/renesas,isp.yaml
> >  F:	Documentation/devicetree/bindings/media/renesas,vin.yaml
> > +F:	drivers/media/platform/rcar-isp.c
> >  F:	drivers/media/platform/rcar-vin/
> >
> >  MEDIA DRIVERS FOR RENESAS - VSP1
> > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> > index aa277a19e2759617..b95e9368110c7848 100644
> > --- a/drivers/media/platform/Kconfig
> > +++ b/drivers/media/platform/Kconfig
> > @@ -200,6 +200,22 @@ config VIDEO_TI_CAL_MC
> >
> >  endif # VIDEO_TI_CAL
> >
> > +config VIDEO_RCAR_ISP
> > +	tristate "R-Car Image Signal Processor (ISP)"
> > +	depends on VIDEO_V4L2 && OF
> > +	depends on ARCH_RENESAS || COMPILE_TEST
> > +	select MEDIA_CONTROLLER
> > +	select VIDEO_V4L2_SUBDEV_API
> > +	select RESET_CONTROLLER
> > +	select V4L2_FWNODE
> > +	help
> > +	  Support for Renesas R-Car Image Signal Processor (ISP).
> > +	  Enable this to support the Renesas R-Car Image Signal
> > +	  Processor (ISP).
> > +
> > +	  To compile this driver as a module, choose M here: the
> > +	  module will be called rcar-isp.
> > +
> >  endif # V4L_PLATFORM_DRIVERS
> >
> >  menuconfig V4L_MEM2MEM_DRIVERS
> > diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> > index 73ce083c2fc6b51b..a148553babfc0944 100644
> > --- a/drivers/media/platform/Makefile
> > +++ b/drivers/media/platform/Makefile
> > @@ -63,6 +63,7 @@ obj-$(CONFIG_VIDEO_AM437X_VPFE)		+= am437x/
> >
> >  obj-$(CONFIG_VIDEO_XILINX)		+= xilinx/
> >
> > +obj-$(CONFIG_VIDEO_RCAR_ISP)		+= rcar-isp.o
> >  obj-$(CONFIG_VIDEO_RCAR_VIN)		+= rcar-vin/
> >
> >  obj-$(CONFIG_VIDEO_ATMEL_ISC)		+= atmel/
> > diff --git a/drivers/media/platform/rcar-isp.c b/drivers/media/platform/rcar-isp.c
> > new file mode 100644
> > index 0000000000000000..1055ca87975c8676
> > --- /dev/null
> > +++ b/drivers/media/platform/rcar-isp.c
> > @@ -0,0 +1,517 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2021 Renesas Electronics Corp.
> > + *
> > + * Driver for Renesas R-Car ISP Channel Selector
> > + *
> > + * The ISP hardware is capable of more than just channel selection, features
> > + * such as demosaicing, white balance control and color space conversion are
> > + * also possible. These more advanced features are not supported by the driver
> > + * due to lack of documentation.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/reset.h>
> > +
> > +#include <media/v4l2-subdev.h>
> > +
> > +#define ISPINPUTSEL0_REG				0x0008
> > +#define ISPINPUTSEL0_SEL_CSI0				BIT(31)
> > +
> > +#define ISPSTART_REG					0x0014
> > +#define ISPSTART_START					0xffff
> > +#define ISPSTART_STOP					0x0000
> > +
> > +#define ISPPROCMODE_DT_REG(n)				(0x1100 + (0x4 * (n)))
> > +#define ISPPROCMODE_DT_PROC_MODE_VC3(pm)		(((pm) & 0x3f) << 24)
> > +#define ISPPROCMODE_DT_PROC_MODE_VC2(pm)		(((pm) & 0x3f) << 16)
> > +#define ISPPROCMODE_DT_PROC_MODE_VC1(pm)		(((pm) & 0x3f) << 8)
> > +#define ISPPROCMODE_DT_PROC_MODE_VC0(pm)		((pm) & 0x3f)
> > +
> > +#define ISPCS_FILTER_ID_CH_REG(n)			(0x3000 + (0x0100 * (n)))
> > +
> > +#define ISPCS_DT_CODE03_CH_REG(n)			(0x3008 + (0x100 * (n)))
> > +#define ISPCS_DT_CODE03_EN3				BIT(31)
> > +#define ISPCS_DT_CODE03_DT3(dt)				(((dt) & 0x3f) << 24)
> > +#define ISPCS_DT_CODE03_EN2				BIT(23)
> > +#define ISPCS_DT_CODE03_DT2(dt)				(((dt) & 0x3f) << 16)
> > +#define ISPCS_DT_CODE03_EN1				BIT(15)
> > +#define ISPCS_DT_CODE03_DT1(dt)				(((dt) & 0x3f) << 8)
> > +#define ISPCS_DT_CODE03_EN0				BIT(7)
> > +#define ISPCS_DT_CODE03_DT0(dt)				((dt) & 0x3f)
> > +
> > +struct rcar_isp_format {
> > +	u32 code;
> > +	unsigned int datatype;
> > +	unsigned int procmode;
> > +};
> > +
> > +static const struct rcar_isp_format rcar_isp_formats[] = {
> > +	{ .code = MEDIA_BUS_FMT_RGB888_1X24,	.datatype = 0x24, .procmode = 0x15 },
> > +	{ .code = MEDIA_BUS_FMT_Y10_1X10,	.datatype = 0x2b, .procmode = 0x10 },
> > +	{ .code = MEDIA_BUS_FMT_UYVY8_1X16,	.datatype = 0x1e, .procmode = 0x0c },
> > +	{ .code = MEDIA_BUS_FMT_YUYV8_1X16,	.datatype = 0x1e, .procmode = 0x0c },
> > +	{ .code = MEDIA_BUS_FMT_UYVY8_2X8,	.datatype = 0x1e, .procmode = 0x0c },
> > +	{ .code = MEDIA_BUS_FMT_YUYV10_2X10,	.datatype = 0x1e, .procmode = 0x0c },
>                                             ^ tab                ^ space
> 
> > +};
> > +
> > +static const struct rcar_isp_format *risp_code_to_fmt(unsigned int code)
> > +{
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(rcar_isp_formats); i++) {
> > +		if (rcar_isp_formats[i].code == code)
> > +			return &rcar_isp_formats[i];
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> > +enum rcar_isp_input {
> > +	RISP_CSI_INPUT0,
> > +	RISP_CSI_INPUT1,
> > +};
> > +
> > +enum rcar_isp_pads {
> > +	RCAR_ISP_SINK,
> > +	RCAR_ISP_PORT0,
> > +	RCAR_ISP_PORT1,
> > +	RCAR_ISP_PORT2,
> > +	RCAR_ISP_PORT3,
> > +	RCAR_ISP_PORT4,
> > +	RCAR_ISP_PORT5,
> > +	RCAR_ISP_PORT6,
> > +	RCAR_ISP_PORT7,
> > +	RCAR_ISP_MAX_PAD,
> > +};
> > +
> > +struct rcar_isp {
> > +	struct device *dev;
> > +	void __iomem *base;
> > +	struct reset_control *rstc;
> > +
> > +	enum rcar_isp_input csi_input;
> > +
> > +	struct v4l2_subdev subdev;
> > +	struct media_pad pads[RCAR_ISP_MAX_PAD];
> > +
> > +	struct v4l2_async_notifier notifier;
> > +	struct v4l2_subdev *remote;
> > +	unsigned int remote_pad;
> > +
> > +	struct mutex lock; /* Protects mf and stream_count. */
> > +	struct v4l2_mbus_framefmt mf;
> > +	int stream_count;
> > +};
> > +
> > +static inline struct rcar_isp *sd_to_isp(struct v4l2_subdev *sd)
> > +{
> > +	return container_of(sd, struct rcar_isp, subdev);
> > +}
> > +
> > +static inline struct rcar_isp *notifier_to_isp(struct v4l2_async_notifier *n)
> > +{
> > +	return container_of(n, struct rcar_isp, notifier);
> > +}
> > +
> > +static void risp_write(struct rcar_isp *isp, u32 offset, u32 value)
> > +{
> > +	iowrite32(value, isp->base + offset);
> > +}
> > +
> > +static u32 risp_read(struct rcar_isp *isp, u32 offset)
> > +{
> > +	return ioread32(isp->base + offset);
> > +}
> > +
> > +static int risp_power_on(struct rcar_isp *isp)
> > +{
> > +	int ret;
> > +
> > +	ret = pm_runtime_resume_and_get(isp->dev);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = reset_control_deassert(isp->rstc);
> > +	if (ret < 0) {
> > +		pm_runtime_put(isp->dev);
> > +		return ret;
> > +	}
> 
> I still think you should define a .resume and .suspend callbacks
> through something like SIMPLE_DEV_PM_OPS which assert/deassert the
> reset controller and just call pm_runtime_resume_and_get() and
> pm_runtime_put() at risp_start() and risp_stop() times.

I wrestled a bit with this in other subdevices involved in the R-Car 
capture chain. I was about to convert all of them to what you suggest 
here but ran into issues with suspend/resume to s2disk IIRC if there was 
a ongoing capture when suspending it would not start correctly. Having 
it in the s_stream call path like this ensured the proper power on of 
the different devices.

I'm sure it can be solved using SIMPLE_DEV_PM_OPS but I have not yet dug 
deep enough in all the drivers to figure it all out.

> 
> Otherwise am I wrong thinking that pm_runtime is only here for refcounting ?

It also (depending on platform) controls the module clocks.

> 
> > +
> > +	return 0;
> > +}
> > +
> > +static void risp_power_off(struct rcar_isp *isp)
> > +{
> > +	reset_control_assert(isp->rstc);
> > +	pm_runtime_put(isp->dev);
> > +}
> > +
> > +static int risp_start(struct rcar_isp *isp)
> > +{
> > +	const struct rcar_isp_format *format;
> > +	unsigned int vc;
> > +	u32 sel_csi = 0;
> > +	int ret;
> > +
> > +	format = risp_code_to_fmt(isp->mf.code);
> > +	if (!format) {
> > +		dev_err(isp->dev, "Unsupported bus format\n");
> > +		return -EINVAL;
> > +	}
> 
> I don't think this can now happen as isp->mf.code is set through
> set_fmt and it's adjusted

Ture. But I get mail from code cover testers that always add back this 
check so I thought I just added it myself this time ;-)

> 
> > +
> > +	ret = risp_power_on(isp);
> > +	if (ret) {
> > +		dev_err(isp->dev, "Failed to power on ISP\n");
> > +		return ret;
> > +	}
> > +
> > +	/* Select CSI-2 input source. */
> > +	if (isp->csi_input == RISP_CSI_INPUT1)
> > +		sel_csi = ISPINPUTSEL0_SEL_CSI0;
> > +
> > +	risp_write(isp, ISPINPUTSEL0_REG,
> > +		   risp_read(isp, ISPINPUTSEL0_REG) | sel_csi);
> > +
> > +	/* Configure Channel Selector. */
> 
> If I'm not mistaken we have a fixed routing from channel 4-7 to
> VIN[0-3] as routing is controlled by VC/DT matching. If that's the
> case, should this be recorded ?

I like it the way it is :-)

> 
> > +	for (vc = 0; vc < 4; vc++) {
> > +		u8 ch = vc + 4;
> > +		u8 dt = format->datatype;
> > +
> > +		risp_write(isp, ISPCS_FILTER_ID_CH_REG(ch), BIT(vc));
> > +		risp_write(isp, ISPCS_DT_CODE03_CH_REG(ch),
> > +			   ISPCS_DT_CODE03_EN3 | ISPCS_DT_CODE03_DT3(dt) |
> > +			   ISPCS_DT_CODE03_EN2 | ISPCS_DT_CODE03_DT2(dt) |
> > +			   ISPCS_DT_CODE03_EN1 | ISPCS_DT_CODE03_DT1(dt) |
> > +			   ISPCS_DT_CODE03_EN0 | ISPCS_DT_CODE03_DT0(dt));
> > +	}
> > +
> > +	/* Setup processing method. */
> > +	risp_write(isp, ISPPROCMODE_DT_REG(format->datatype),
> > +		   ISPPROCMODE_DT_PROC_MODE_VC3(format->procmode) |
> > +		   ISPPROCMODE_DT_PROC_MODE_VC2(format->procmode) |
> > +		   ISPPROCMODE_DT_PROC_MODE_VC1(format->procmode) |
> > +		   ISPPROCMODE_DT_PROC_MODE_VC0(format->procmode));
> > +
> > +	/* Start ISP. */
> > +	risp_write(isp, ISPSTART_REG, ISPSTART_START);
> > +
> > +	ret = v4l2_subdev_call(isp->remote, video, s_stream, 1);
> > +	if (ret)
> > +		risp_power_off(isp);
> > +
> > +	return ret;
> > +}
> > +
> > +static void risp_stop(struct rcar_isp *isp)
> > +{
> > +	v4l2_subdev_call(isp->remote, video, s_stream, 0);
> > +
> > +	/* Stop ISP. */
> > +	risp_write(isp, ISPSTART_REG, ISPSTART_STOP);
> > +
> > +	risp_power_off(isp);
> > +}
> > +
> > +static int risp_s_stream(struct v4l2_subdev *sd, int enable)
> > +{
> > +	struct rcar_isp *isp = sd_to_isp(sd);
> > +	int ret = 0;
> > +
> > +	mutex_lock(&isp->lock);
> > +
> > +	if (!isp->remote) {
> > +		ret = -ENODEV;
> > +		goto out;
> > +	}
> > +
> > +	if (enable && isp->stream_count == 0) {
> > +		ret = risp_start(isp);
> > +		if (ret)
> > +			goto out;
> > +	} else if (!enable && isp->stream_count == 1) {
> > +		risp_stop(isp);
> > +	}
> > +
> > +	isp->stream_count += enable ? 1 : -1;
> > +out:
> > +	mutex_unlock(&isp->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct v4l2_subdev_video_ops risp_video_ops = {
> > +	.s_stream = risp_s_stream,
> > +};
> > +
> > +static int risp_set_pad_format(struct v4l2_subdev *sd,
> > +			       struct v4l2_subdev_state *sd_state,
> > +			       struct v4l2_subdev_format *format)
> > +{
> > +	struct rcar_isp *isp = sd_to_isp(sd);
> > +	struct v4l2_mbus_framefmt *framefmt;
> > +
> > +	mutex_lock(&isp->lock);
> > +
> > +	if (!risp_code_to_fmt(format->format.code))
> > +		format->format.code = rcar_isp_formats[0].code;
> > +
> > +	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > +		isp->mf = format->format;
> > +	} else {
> > +		framefmt = v4l2_subdev_get_try_format(sd, sd_state, 0);
> > +		*framefmt = format->format;
> > +	}
> > +
> > +	mutex_unlock(&isp->lock);
> > +
> > +	return 0;
> > +}
> > +
> > +static int risp_get_pad_format(struct v4l2_subdev *sd,
> > +			       struct v4l2_subdev_state *sd_state,
> > +			       struct v4l2_subdev_format *format)
> > +{
> > +	struct rcar_isp *isp = sd_to_isp(sd);
> > +
> > +	mutex_lock(&isp->lock);
> > +
> > +	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > +		format->format = isp->mf;
> > +	else
> > +		format->format = *v4l2_subdev_get_try_format(sd, sd_state, 0);
> > +
> > +	mutex_unlock(&isp->lock);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct v4l2_subdev_pad_ops risp_pad_ops = {
> > +	.set_fmt = risp_set_pad_format,
> > +	.get_fmt = risp_get_pad_format,
> > +	.link_validate = v4l2_subdev_link_validate_default,
> > +};
> > +
> > +static const struct v4l2_subdev_ops rcar_isp_subdev_ops = {
> > +	.video	= &risp_video_ops,
> > +	.pad	= &risp_pad_ops,
> > +};
> > +
> > +/* -----------------------------------------------------------------------------
> > + * Async handling and registration of subdevices and links
> > + */
> > +
> > +static int risp_notify_bound(struct v4l2_async_notifier *notifier,
> > +			     struct v4l2_subdev *subdev,
> > +			     struct v4l2_async_subdev *asd)
> > +{
> > +	struct rcar_isp *isp = notifier_to_isp(notifier);
> > +	int pad;
> > +
> > +	pad = media_entity_get_fwnode_pad(&subdev->entity, asd->match.fwnode,
> > +					  MEDIA_PAD_FL_SOURCE);
> > +	if (pad < 0) {
> > +		dev_err(isp->dev, "Failed to find pad for %s\n", subdev->name);
> > +		return pad;
> > +	}
> > +
> > +	isp->remote = subdev;
> > +	isp->remote_pad = pad;
> 
> remote_pad is not used anywhere now that the format comes from the
> local pad.

Good catch, thanks!

> 
> > +
> > +	dev_dbg(isp->dev, "Bound %s pad: %d\n", subdev->name, pad);
> > +
> > +	return media_create_pad_link(&subdev->entity, pad,
> > +				     &isp->subdev.entity, 0,
> > +				     MEDIA_LNK_FL_ENABLED |
> > +				     MEDIA_LNK_FL_IMMUTABLE);
> > +}
> > +
> > +static void risp_notify_unbind(struct v4l2_async_notifier *notifier,
> > +			       struct v4l2_subdev *subdev,
> > +			       struct v4l2_async_subdev *asd)
> > +{
> > +	struct rcar_isp *isp = notifier_to_isp(notifier);
> > +
> > +	isp->remote = NULL;
> > +
> > +	dev_dbg(isp->dev, "Unbind %s\n", subdev->name);
> > +}
> > +
> > +static const struct v4l2_async_notifier_operations risp_notify_ops = {
> > +	.bound = risp_notify_bound,
> > +	.unbind = risp_notify_unbind,
> > +};
> > +
> > +static int risp_parse_dt(struct rcar_isp *isp)
> > +{
> > +	struct v4l2_async_subdev *asd;
> > +	struct fwnode_handle *fwnode;
> > +	struct fwnode_handle *ep;
> > +	unsigned int id;
> > +	int ret;
> > +
> > +	for (id = 0; id < 2; id++) {
> > +		ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(isp->dev),
> > +						     0, id, 0);
> > +		if (ep)
> > +			break;
> > +	}
> > +
> > +	if (!ep) {
> > +		dev_err(isp->dev, "Not connected to subdevice\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (id == 1)
> > +		isp->csi_input = RISP_CSI_INPUT1;
> > +
> > +	fwnode = fwnode_graph_get_remote_endpoint(ep);
> > +	fwnode_handle_put(ep);
> > +
> > +	dev_dbg(isp->dev, "Found '%pOF'\n", to_of_node(fwnode));
> > +
> > +	v4l2_async_nf_init(&isp->notifier);
> > +	isp->notifier.ops = &risp_notify_ops;
> > +
> > +	asd = v4l2_async_nf_add_fwnode(&isp->notifier, fwnode,
> > +				       struct v4l2_async_subdev);
> > +	fwnode_handle_put(fwnode);
> > +	if (IS_ERR(asd))
> > +		return PTR_ERR(asd);
> > +
> > +	ret = v4l2_async_subdev_nf_register(&isp->subdev, &isp->notifier);
> > +	if (ret)
> > +		v4l2_async_nf_cleanup(&isp->notifier);
> > +
> > +	return ret;
> > +}
> > +
> > +/* -----------------------------------------------------------------------------
> > + * Platform Device Driver
> > + */
> > +
> > +static const struct media_entity_operations risp_entity_ops = {
> > +	.link_validate = v4l2_subdev_link_validate,
> > +};
> > +
> > +static int risp_probe_resources(struct rcar_isp *isp,
> > +				struct platform_device *pdev)
> > +{
> > +	struct resource *res;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	isp->base = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR(isp->base))
> > +		return PTR_ERR(isp->base);
> > +
> > +	isp->rstc = devm_reset_control_get(&pdev->dev, NULL);
> > +
> > +	return PTR_ERR_OR_ZERO(isp->rstc);
> > +}
> > +
> > +static const struct of_device_id risp_of_id_table[] = {
> > +	{ .compatible = "renesas,r8a779a0-isp" },
> > +	{ /* sentinel */ },
> > +};
> > +MODULE_DEVICE_TABLE(of, risp_of_id_table);
> > +
> > +static int risp_probe(struct platform_device *pdev)
> > +{
> > +	struct rcar_isp *isp;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	isp = devm_kzalloc(&pdev->dev, sizeof(*isp), GFP_KERNEL);
> > +	if (!isp)
> > +		return -ENOMEM;
> > +
> > +	isp->dev = &pdev->dev;
> > +
> > +	mutex_init(&isp->lock);
> > +
> > +	ret = risp_probe_resources(isp, pdev);
> > +	if (ret) {
> > +		dev_err(isp->dev, "Failed to get resources\n");
> > +		goto error_mutex;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, isp);
> > +
> > +	pm_runtime_enable(&pdev->dev);
> > +
> > +	ret = risp_parse_dt(isp);
> > +	if (ret)
> > +		goto error_pm;
> > +
> > +	isp->subdev.owner = THIS_MODULE;
> > +	isp->subdev.dev = &pdev->dev;
> > +	v4l2_subdev_init(&isp->subdev, &rcar_isp_subdev_ops);
> > +	v4l2_set_subdevdata(&isp->subdev, &pdev->dev);
> > +	snprintf(isp->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s %s",
> > +		 KBUILD_MODNAME, dev_name(&pdev->dev));
> > +	isp->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
> > +
> > +	isp->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
> > +	isp->subdev.entity.ops = &risp_entity_ops;
> > +
> > +	isp->pads[RCAR_ISP_SINK].flags = MEDIA_PAD_FL_SINK;
> > +	for (i = RCAR_ISP_PORT0; i < RCAR_ISP_MAX_PAD; i++)
> > +		isp->pads[i].flags = MEDIA_PAD_FL_SOURCE;
> > +
> > +	ret = media_entity_pads_init(&isp->subdev.entity, RCAR_ISP_MAX_PAD,
> > +				     isp->pads);
> > +	if (ret)
> > +		goto error_notifier;
> > +
> > +	ret = v4l2_async_register_subdev(&isp->subdev);
> > +	if (ret < 0)
> > +		goto error_notifier;
> > +
> > +	dev_info(isp->dev, "Using CSI-2 input: %u\n", isp->csi_input);
> > +
> > +	return 0;
> 
> Maybe an empy line to highlight the error path

I think it's a matter of preference, I (almost) never inject empty lines 
for labels :-)

> 
> Anyway, all minors, you'll have a new version anyway and pm_runtime
> usage has been under the eyes of Sakari and Hans for 5 versions and
> they have not complained so I guess it's fine.
> 
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks!

> 
> Thanks
>    j
> 
> > +error_notifier:
> > +	v4l2_async_nf_unregister(&isp->notifier);
> > +	v4l2_async_nf_cleanup(&isp->notifier);
> > +error_pm:
> > +	pm_runtime_disable(&pdev->dev);
> > +error_mutex:
> > +	mutex_destroy(&isp->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int risp_remove(struct platform_device *pdev)
> > +{
> > +	struct rcar_isp *isp = platform_get_drvdata(pdev);
> > +
> > +	v4l2_async_nf_unregister(&isp->notifier);
> > +	v4l2_async_nf_cleanup(&isp->notifier);
> > +
> > +	v4l2_async_unregister_subdev(&isp->subdev);
> > +
> > +	pm_runtime_disable(&pdev->dev);
> > +
> > +	mutex_destroy(&isp->lock);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct platform_driver rcar_isp_driver = {
> > +	.driver = {
> > +		.name = "rcar-isp",
> > +		.of_match_table = risp_of_id_table,
> > +	},
> > +	.probe = risp_probe,
> > +	.remove = risp_remove,
> > +};
> > +
> > +module_platform_driver(rcar_isp_driver);
> > +
> > +MODULE_AUTHOR("Niklas Söderlund <niklas.soderlund@ragnatech.se>");
> > +MODULE_DESCRIPTION("Renesas R-Car ISP Channel Selector driver");
> > +MODULE_LICENSE("GPL");
> 
> 
> > --
> > 2.33.0
> >
  

Hi Niklas,

On Tue, Oct 05, 2021 at 08:56:09PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your review.
>
> On 2021-10-04 12:21:59 +0200, Jacopo Mondi wrote:
> > Hi Niklas,
> >
> > On Tue, Sep 14, 2021 at 04:00:57PM +0200, Niklas Söderlund wrote:
> > > Add a V4L2 driver for Renesas R-Car Image Signal Processor. The driver
> > > supports the R-Car V3U SoC where the ISP IP sits between the R-Car CSI-2
> > > receiver and VIN and filters the CSI-2 data based on VC/DT and directs
> > > the video stream to different VIN IPs.
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > ---
> > > * Changes since v4
> > > - Update to the new async function names.
> > > - Do not depend on external s_power, instead control the device power at stream start/stop.
> > > - Do not look at remote upstream subdevice format when starting (or at all).
> > > - Serialise access for format variables in get_fmt and set_fmt.
> > > - Make use of v4l2_subdev_link_validate_default()
> > > - Add mutex_destroy() on error paths and in remove function.
> > > - Propperly disable runtime PM if subdev registration fails.
> > >
> > > * Changes since v3
> > > - Fix spelling in header information.
> > > - Add parentheses around macro expansions.
> > > - Add explicit blocks to for-loop body.
> > > - Add documentation for mutex.
> > >
> > > * Changes since v2
> > > - Add description of IPS capabilities and that only channel selection is
> > >   currently supported.
> > > - Update signature of v4l2_subdev_pad_ops set_fmt.
> > > - Switch to pm_runtime_resume_and_get().
> > > - Make better use of devm_kzalloc().
> > > ---
> > >  MAINTAINERS                       |   1 +
> > >  drivers/media/platform/Kconfig    |  16 +
> > >  drivers/media/platform/Makefile   |   1 +
> > >  drivers/media/platform/rcar-isp.c | 517 ++++++++++++++++++++++++++++++
> > >  4 files changed, 535 insertions(+)
> > >  create mode 100644 drivers/media/platform/rcar-isp.c
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 08c9612e769af24a..8164d1bd0a4df351 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -11575,6 +11575,7 @@ T:	git git://linuxtv.org/media_tree.git
> > >  F:	Documentation/devicetree/bindings/media/renesas,csi2.yaml
> > >  F:	Documentation/devicetree/bindings/media/renesas,isp.yaml
> > >  F:	Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > +F:	drivers/media/platform/rcar-isp.c
> > >  F:	drivers/media/platform/rcar-vin/
> > >
> > >  MEDIA DRIVERS FOR RENESAS - VSP1
> > > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> > > index aa277a19e2759617..b95e9368110c7848 100644
> > > --- a/drivers/media/platform/Kconfig
> > > +++ b/drivers/media/platform/Kconfig
> > > @@ -200,6 +200,22 @@ config VIDEO_TI_CAL_MC
> > >
> > >  endif # VIDEO_TI_CAL
> > >
> > > +config VIDEO_RCAR_ISP
> > > +	tristate "R-Car Image Signal Processor (ISP)"
> > > +	depends on VIDEO_V4L2 && OF
> > > +	depends on ARCH_RENESAS || COMPILE_TEST
> > > +	select MEDIA_CONTROLLER
> > > +	select VIDEO_V4L2_SUBDEV_API
> > > +	select RESET_CONTROLLER
> > > +	select V4L2_FWNODE
> > > +	help
> > > +	  Support for Renesas R-Car Image Signal Processor (ISP).
> > > +	  Enable this to support the Renesas R-Car Image Signal
> > > +	  Processor (ISP).
> > > +
> > > +	  To compile this driver as a module, choose M here: the
> > > +	  module will be called rcar-isp.
> > > +
> > >  endif # V4L_PLATFORM_DRIVERS
> > >
> > >  menuconfig V4L_MEM2MEM_DRIVERS
> > > diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> > > index 73ce083c2fc6b51b..a148553babfc0944 100644
> > > --- a/drivers/media/platform/Makefile
> > > +++ b/drivers/media/platform/Makefile
> > > @@ -63,6 +63,7 @@ obj-$(CONFIG_VIDEO_AM437X_VPFE)		+= am437x/
> > >
> > >  obj-$(CONFIG_VIDEO_XILINX)		+= xilinx/
> > >
> > > +obj-$(CONFIG_VIDEO_RCAR_ISP)		+= rcar-isp.o
> > >  obj-$(CONFIG_VIDEO_RCAR_VIN)		+= rcar-vin/
> > >
> > >  obj-$(CONFIG_VIDEO_ATMEL_ISC)		+= atmel/
> > > diff --git a/drivers/media/platform/rcar-isp.c b/drivers/media/platform/rcar-isp.c
> > > new file mode 100644
> > > index 0000000000000000..1055ca87975c8676
> > > --- /dev/null
> > > +++ b/drivers/media/platform/rcar-isp.c
> > > @@ -0,0 +1,517 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (C) 2021 Renesas Electronics Corp.
> > > + *
> > > + * Driver for Renesas R-Car ISP Channel Selector
> > > + *
> > > + * The ISP hardware is capable of more than just channel selection, features
> > > + * such as demosaicing, white balance control and color space conversion are
> > > + * also possible. These more advanced features are not supported by the driver
> > > + * due to lack of documentation.
> > > + */
> > > +
> > > +#include <linux/module.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/pm_runtime.h>
> > > +#include <linux/reset.h>
> > > +
> > > +#include <media/v4l2-subdev.h>
> > > +
> > > +#define ISPINPUTSEL0_REG				0x0008
> > > +#define ISPINPUTSEL0_SEL_CSI0				BIT(31)
> > > +
> > > +#define ISPSTART_REG					0x0014
> > > +#define ISPSTART_START					0xffff
> > > +#define ISPSTART_STOP					0x0000
> > > +
> > > +#define ISPPROCMODE_DT_REG(n)				(0x1100 + (0x4 * (n)))
> > > +#define ISPPROCMODE_DT_PROC_MODE_VC3(pm)		(((pm) & 0x3f) << 24)
> > > +#define ISPPROCMODE_DT_PROC_MODE_VC2(pm)		(((pm) & 0x3f) << 16)
> > > +#define ISPPROCMODE_DT_PROC_MODE_VC1(pm)		(((pm) & 0x3f) << 8)
> > > +#define ISPPROCMODE_DT_PROC_MODE_VC0(pm)		((pm) & 0x3f)
> > > +
> > > +#define ISPCS_FILTER_ID_CH_REG(n)			(0x3000 + (0x0100 * (n)))
> > > +
> > > +#define ISPCS_DT_CODE03_CH_REG(n)			(0x3008 + (0x100 * (n)))
> > > +#define ISPCS_DT_CODE03_EN3				BIT(31)
> > > +#define ISPCS_DT_CODE03_DT3(dt)				(((dt) & 0x3f) << 24)
> > > +#define ISPCS_DT_CODE03_EN2				BIT(23)
> > > +#define ISPCS_DT_CODE03_DT2(dt)				(((dt) & 0x3f) << 16)
> > > +#define ISPCS_DT_CODE03_EN1				BIT(15)
> > > +#define ISPCS_DT_CODE03_DT1(dt)				(((dt) & 0x3f) << 8)
> > > +#define ISPCS_DT_CODE03_EN0				BIT(7)
> > > +#define ISPCS_DT_CODE03_DT0(dt)				((dt) & 0x3f)
> > > +
> > > +struct rcar_isp_format {
> > > +	u32 code;
> > > +	unsigned int datatype;
> > > +	unsigned int procmode;
> > > +};
> > > +
> > > +static const struct rcar_isp_format rcar_isp_formats[] = {
> > > +	{ .code = MEDIA_BUS_FMT_RGB888_1X24,	.datatype = 0x24, .procmode = 0x15 },
> > > +	{ .code = MEDIA_BUS_FMT_Y10_1X10,	.datatype = 0x2b, .procmode = 0x10 },
> > > +	{ .code = MEDIA_BUS_FMT_UYVY8_1X16,	.datatype = 0x1e, .procmode = 0x0c },
> > > +	{ .code = MEDIA_BUS_FMT_YUYV8_1X16,	.datatype = 0x1e, .procmode = 0x0c },
> > > +	{ .code = MEDIA_BUS_FMT_UYVY8_2X8,	.datatype = 0x1e, .procmode = 0x0c },
> > > +	{ .code = MEDIA_BUS_FMT_YUYV10_2X10,	.datatype = 0x1e, .procmode = 0x0c },
> >                                             ^ tab                ^ space
> >
> > > +};
> > > +
> > > +static const struct rcar_isp_format *risp_code_to_fmt(unsigned int code)
> > > +{
> > > +	unsigned int i;
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(rcar_isp_formats); i++) {
> > > +		if (rcar_isp_formats[i].code == code)
> > > +			return &rcar_isp_formats[i];
> > > +	}
> > > +
> > > +	return NULL;
> > > +}
> > > +
> > > +enum rcar_isp_input {
> > > +	RISP_CSI_INPUT0,
> > > +	RISP_CSI_INPUT1,
> > > +};
> > > +
> > > +enum rcar_isp_pads {
> > > +	RCAR_ISP_SINK,
> > > +	RCAR_ISP_PORT0,
> > > +	RCAR_ISP_PORT1,
> > > +	RCAR_ISP_PORT2,
> > > +	RCAR_ISP_PORT3,
> > > +	RCAR_ISP_PORT4,
> > > +	RCAR_ISP_PORT5,
> > > +	RCAR_ISP_PORT6,
> > > +	RCAR_ISP_PORT7,
> > > +	RCAR_ISP_MAX_PAD,
> > > +};
> > > +
> > > +struct rcar_isp {
> > > +	struct device *dev;
> > > +	void __iomem *base;
> > > +	struct reset_control *rstc;
> > > +
> > > +	enum rcar_isp_input csi_input;
> > > +
> > > +	struct v4l2_subdev subdev;
> > > +	struct media_pad pads[RCAR_ISP_MAX_PAD];
> > > +
> > > +	struct v4l2_async_notifier notifier;
> > > +	struct v4l2_subdev *remote;
> > > +	unsigned int remote_pad;
> > > +
> > > +	struct mutex lock; /* Protects mf and stream_count. */
> > > +	struct v4l2_mbus_framefmt mf;
> > > +	int stream_count;
> > > +};
> > > +
> > > +static inline struct rcar_isp *sd_to_isp(struct v4l2_subdev *sd)
> > > +{
> > > +	return container_of(sd, struct rcar_isp, subdev);
> > > +}
> > > +
> > > +static inline struct rcar_isp *notifier_to_isp(struct v4l2_async_notifier *n)
> > > +{
> > > +	return container_of(n, struct rcar_isp, notifier);
> > > +}
> > > +
> > > +static void risp_write(struct rcar_isp *isp, u32 offset, u32 value)
> > > +{
> > > +	iowrite32(value, isp->base + offset);
> > > +}
> > > +
> > > +static u32 risp_read(struct rcar_isp *isp, u32 offset)
> > > +{
> > > +	return ioread32(isp->base + offset);
> > > +}
> > > +
> > > +static int risp_power_on(struct rcar_isp *isp)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = pm_runtime_resume_and_get(isp->dev);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	ret = reset_control_deassert(isp->rstc);
> > > +	if (ret < 0) {
> > > +		pm_runtime_put(isp->dev);
> > > +		return ret;
> > > +	}
> >
> > I still think you should define a .resume and .suspend callbacks
> > through something like SIMPLE_DEV_PM_OPS which assert/deassert the
> > reset controller and just call pm_runtime_resume_and_get() and
> > pm_runtime_put() at risp_start() and risp_stop() times.
>
> I wrestled a bit with this in other subdevices involved in the R-Car
> capture chain. I was about to convert all of them to what you suggest
> here but ran into issues with suspend/resume to s2disk IIRC if there was
> a ongoing capture when suspending it would not start correctly. Having
> it in the s_stream call path like this ensured the proper power on of
> the different devices.

I was not suggesting to move the the power up/down sequences out of
s_stream, that's fine. My suggestion was to

        int risp_resume() {
                reset_control_deassert()
        }

        int risp_susped() {
                reset_control_deassert()
        }

        int s_stream(bool enable) {
                if (enable)
                        pm_runtime_resume_and_get();
                else
                        pm_runtime_put();
        }

        SIMPLE_DEV_PM_OPS(risp_pm_ops, risp_suspend, risp_resume);

        static struct platform_driver rcar_isp_driver = {
                .driver = {
                        .name = "rcar-isp",
                        .of_match_table = risp_of_id_table,
                        .pm = &risp_pm_ops,
                },
                .probe = risp_probe,
                .remove = risp_remove,
        };

>
> I'm sure it can be solved using SIMPLE_DEV_PM_OPS but I have not yet dug
> deep enough in all the drivers to figure it all out.
>
> >
> > Otherwise am I wrong thinking that pm_runtime is only here for refcounting ?
>
> It also (depending on platform) controls the module clocks.
>
> >
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void risp_power_off(struct rcar_isp *isp)
> > > +{
> > > +	reset_control_assert(isp->rstc);
> > > +	pm_runtime_put(isp->dev);
> > > +}
> > > +
> > > +static int risp_start(struct rcar_isp *isp)
> > > +{
> > > +	const struct rcar_isp_format *format;
> > > +	unsigned int vc;
> > > +	u32 sel_csi = 0;
> > > +	int ret;
> > > +
> > > +	format = risp_code_to_fmt(isp->mf.code);
> > > +	if (!format) {
> > > +		dev_err(isp->dev, "Unsupported bus format\n");
> > > +		return -EINVAL;
> > > +	}
> >
> > I don't think this can now happen as isp->mf.code is set through
> > set_fmt and it's adjusted
>
> Ture. But I get mail from code cover testers that always add back this
> check so I thought I just added it myself this time ;-)
>
> >
> > > +
> > > +	ret = risp_power_on(isp);
> > > +	if (ret) {
> > > +		dev_err(isp->dev, "Failed to power on ISP\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	/* Select CSI-2 input source. */
> > > +	if (isp->csi_input == RISP_CSI_INPUT1)
> > > +		sel_csi = ISPINPUTSEL0_SEL_CSI0;
> > > +
> > > +	risp_write(isp, ISPINPUTSEL0_REG,
> > > +		   risp_read(isp, ISPINPUTSEL0_REG) | sel_csi);
> > > +
> > > +	/* Configure Channel Selector. */
> >
> > If I'm not mistaken we have a fixed routing from channel 4-7 to
> > VIN[0-3] as routing is controlled by VC/DT matching. If that's the
> > case, should this be recorded ?
>
> I like it the way it is :-)
>

Up to you, just to be clear, I was just suggesting a comment, nothing
more.

Thanks
   j

> >
> > > +	for (vc = 0; vc < 4; vc++) {
> > > +		u8 ch = vc + 4;
> > > +		u8 dt = format->datatype;
> > > +
> > > +		risp_write(isp, ISPCS_FILTER_ID_CH_REG(ch), BIT(vc));
> > > +		risp_write(isp, ISPCS_DT_CODE03_CH_REG(ch),
> > > +			   ISPCS_DT_CODE03_EN3 | ISPCS_DT_CODE03_DT3(dt) |
> > > +			   ISPCS_DT_CODE03_EN2 | ISPCS_DT_CODE03_DT2(dt) |
> > > +			   ISPCS_DT_CODE03_EN1 | ISPCS_DT_CODE03_DT1(dt) |
> > > +			   ISPCS_DT_CODE03_EN0 | ISPCS_DT_CODE03_DT0(dt));
> > > +	}
> > > +
> > > +	/* Setup processing method. */
> > > +	risp_write(isp, ISPPROCMODE_DT_REG(format->datatype),
> > > +		   ISPPROCMODE_DT_PROC_MODE_VC3(format->procmode) |
> > > +		   ISPPROCMODE_DT_PROC_MODE_VC2(format->procmode) |
> > > +		   ISPPROCMODE_DT_PROC_MODE_VC1(format->procmode) |
> > > +		   ISPPROCMODE_DT_PROC_MODE_VC0(format->procmode));
> > > +
> > > +	/* Start ISP. */
> > > +	risp_write(isp, ISPSTART_REG, ISPSTART_START);
> > > +
> > > +	ret = v4l2_subdev_call(isp->remote, video, s_stream, 1);
> > > +	if (ret)
> > > +		risp_power_off(isp);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static void risp_stop(struct rcar_isp *isp)
> > > +{
> > > +	v4l2_subdev_call(isp->remote, video, s_stream, 0);
> > > +
> > > +	/* Stop ISP. */
> > > +	risp_write(isp, ISPSTART_REG, ISPSTART_STOP);
> > > +
> > > +	risp_power_off(isp);
> > > +}
> > > +
> > > +static int risp_s_stream(struct v4l2_subdev *sd, int enable)
> > > +{
> > > +	struct rcar_isp *isp = sd_to_isp(sd);
> > > +	int ret = 0;
> > > +
> > > +	mutex_lock(&isp->lock);
> > > +
> > > +	if (!isp->remote) {
> > > +		ret = -ENODEV;
> > > +		goto out;
> > > +	}
> > > +
> > > +	if (enable && isp->stream_count == 0) {
> > > +		ret = risp_start(isp);
> > > +		if (ret)
> > > +			goto out;
> > > +	} else if (!enable && isp->stream_count == 1) {
> > > +		risp_stop(isp);
> > > +	}
> > > +
> > > +	isp->stream_count += enable ? 1 : -1;
> > > +out:
> > > +	mutex_unlock(&isp->lock);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static const struct v4l2_subdev_video_ops risp_video_ops = {
> > > +	.s_stream = risp_s_stream,
> > > +};
> > > +
> > > +static int risp_set_pad_format(struct v4l2_subdev *sd,
> > > +			       struct v4l2_subdev_state *sd_state,
> > > +			       struct v4l2_subdev_format *format)
> > > +{
> > > +	struct rcar_isp *isp = sd_to_isp(sd);
> > > +	struct v4l2_mbus_framefmt *framefmt;
> > > +
> > > +	mutex_lock(&isp->lock);
> > > +
> > > +	if (!risp_code_to_fmt(format->format.code))
> > > +		format->format.code = rcar_isp_formats[0].code;
> > > +
> > > +	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > > +		isp->mf = format->format;
> > > +	} else {
> > > +		framefmt = v4l2_subdev_get_try_format(sd, sd_state, 0);
> > > +		*framefmt = format->format;
> > > +	}
> > > +
> > > +	mutex_unlock(&isp->lock);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int risp_get_pad_format(struct v4l2_subdev *sd,
> > > +			       struct v4l2_subdev_state *sd_state,
> > > +			       struct v4l2_subdev_format *format)
> > > +{
> > > +	struct rcar_isp *isp = sd_to_isp(sd);
> > > +
> > > +	mutex_lock(&isp->lock);
> > > +
> > > +	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > > +		format->format = isp->mf;
> > > +	else
> > > +		format->format = *v4l2_subdev_get_try_format(sd, sd_state, 0);
> > > +
> > > +	mutex_unlock(&isp->lock);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct v4l2_subdev_pad_ops risp_pad_ops = {
> > > +	.set_fmt = risp_set_pad_format,
> > > +	.get_fmt = risp_get_pad_format,
> > > +	.link_validate = v4l2_subdev_link_validate_default,
> > > +};
> > > +
> > > +static const struct v4l2_subdev_ops rcar_isp_subdev_ops = {
> > > +	.video	= &risp_video_ops,
> > > +	.pad	= &risp_pad_ops,
> > > +};
> > > +
> > > +/* -----------------------------------------------------------------------------
> > > + * Async handling and registration of subdevices and links
> > > + */
> > > +
> > > +static int risp_notify_bound(struct v4l2_async_notifier *notifier,
> > > +			     struct v4l2_subdev *subdev,
> > > +			     struct v4l2_async_subdev *asd)
> > > +{
> > > +	struct rcar_isp *isp = notifier_to_isp(notifier);
> > > +	int pad;
> > > +
> > > +	pad = media_entity_get_fwnode_pad(&subdev->entity, asd->match.fwnode,
> > > +					  MEDIA_PAD_FL_SOURCE);
> > > +	if (pad < 0) {
> > > +		dev_err(isp->dev, "Failed to find pad for %s\n", subdev->name);
> > > +		return pad;
> > > +	}
> > > +
> > > +	isp->remote = subdev;
> > > +	isp->remote_pad = pad;
> >
> > remote_pad is not used anywhere now that the format comes from the
> > local pad.
>
> Good catch, thanks!
>
> >
> > > +
> > > +	dev_dbg(isp->dev, "Bound %s pad: %d\n", subdev->name, pad);
> > > +
> > > +	return media_create_pad_link(&subdev->entity, pad,
> > > +				     &isp->subdev.entity, 0,
> > > +				     MEDIA_LNK_FL_ENABLED |
> > > +				     MEDIA_LNK_FL_IMMUTABLE);
> > > +}
> > > +
> > > +static void risp_notify_unbind(struct v4l2_async_notifier *notifier,
> > > +			       struct v4l2_subdev *subdev,
> > > +			       struct v4l2_async_subdev *asd)
> > > +{
> > > +	struct rcar_isp *isp = notifier_to_isp(notifier);
> > > +
> > > +	isp->remote = NULL;
> > > +
> > > +	dev_dbg(isp->dev, "Unbind %s\n", subdev->name);
> > > +}
> > > +
> > > +static const struct v4l2_async_notifier_operations risp_notify_ops = {
> > > +	.bound = risp_notify_bound,
> > > +	.unbind = risp_notify_unbind,
> > > +};
> > > +
> > > +static int risp_parse_dt(struct rcar_isp *isp)
> > > +{
> > > +	struct v4l2_async_subdev *asd;
> > > +	struct fwnode_handle *fwnode;
> > > +	struct fwnode_handle *ep;
> > > +	unsigned int id;
> > > +	int ret;
> > > +
> > > +	for (id = 0; id < 2; id++) {
> > > +		ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(isp->dev),
> > > +						     0, id, 0);
> > > +		if (ep)
> > > +			break;
> > > +	}
> > > +
> > > +	if (!ep) {
> > > +		dev_err(isp->dev, "Not connected to subdevice\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	if (id == 1)
> > > +		isp->csi_input = RISP_CSI_INPUT1;
> > > +
> > > +	fwnode = fwnode_graph_get_remote_endpoint(ep);
> > > +	fwnode_handle_put(ep);
> > > +
> > > +	dev_dbg(isp->dev, "Found '%pOF'\n", to_of_node(fwnode));
> > > +
> > > +	v4l2_async_nf_init(&isp->notifier);
> > > +	isp->notifier.ops = &risp_notify_ops;
> > > +
> > > +	asd = v4l2_async_nf_add_fwnode(&isp->notifier, fwnode,
> > > +				       struct v4l2_async_subdev);
> > > +	fwnode_handle_put(fwnode);
> > > +	if (IS_ERR(asd))
> > > +		return PTR_ERR(asd);
> > > +
> > > +	ret = v4l2_async_subdev_nf_register(&isp->subdev, &isp->notifier);
> > > +	if (ret)
> > > +		v4l2_async_nf_cleanup(&isp->notifier);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +/* -----------------------------------------------------------------------------
> > > + * Platform Device Driver
> > > + */
> > > +
> > > +static const struct media_entity_operations risp_entity_ops = {
> > > +	.link_validate = v4l2_subdev_link_validate,
> > > +};
> > > +
> > > +static int risp_probe_resources(struct rcar_isp *isp,
> > > +				struct platform_device *pdev)
> > > +{
> > > +	struct resource *res;
> > > +
> > > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +	isp->base = devm_ioremap_resource(&pdev->dev, res);
> > > +	if (IS_ERR(isp->base))
> > > +		return PTR_ERR(isp->base);
> > > +
> > > +	isp->rstc = devm_reset_control_get(&pdev->dev, NULL);
> > > +
> > > +	return PTR_ERR_OR_ZERO(isp->rstc);
> > > +}
> > > +
> > > +static const struct of_device_id risp_of_id_table[] = {
> > > +	{ .compatible = "renesas,r8a779a0-isp" },
> > > +	{ /* sentinel */ },
> > > +};
> > > +MODULE_DEVICE_TABLE(of, risp_of_id_table);
> > > +
> > > +static int risp_probe(struct platform_device *pdev)
> > > +{
> > > +	struct rcar_isp *isp;
> > > +	unsigned int i;
> > > +	int ret;
> > > +
> > > +	isp = devm_kzalloc(&pdev->dev, sizeof(*isp), GFP_KERNEL);
> > > +	if (!isp)
> > > +		return -ENOMEM;
> > > +
> > > +	isp->dev = &pdev->dev;
> > > +
> > > +	mutex_init(&isp->lock);
> > > +
> > > +	ret = risp_probe_resources(isp, pdev);
> > > +	if (ret) {
> > > +		dev_err(isp->dev, "Failed to get resources\n");
> > > +		goto error_mutex;
> > > +	}
> > > +
> > > +	platform_set_drvdata(pdev, isp);
> > > +
> > > +	pm_runtime_enable(&pdev->dev);
> > > +
> > > +	ret = risp_parse_dt(isp);
> > > +	if (ret)
> > > +		goto error_pm;
> > > +
> > > +	isp->subdev.owner = THIS_MODULE;
> > > +	isp->subdev.dev = &pdev->dev;
> > > +	v4l2_subdev_init(&isp->subdev, &rcar_isp_subdev_ops);
> > > +	v4l2_set_subdevdata(&isp->subdev, &pdev->dev);
> > > +	snprintf(isp->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s %s",
> > > +		 KBUILD_MODNAME, dev_name(&pdev->dev));
> > > +	isp->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
> > > +
> > > +	isp->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
> > > +	isp->subdev.entity.ops = &risp_entity_ops;
> > > +
> > > +	isp->pads[RCAR_ISP_SINK].flags = MEDIA_PAD_FL_SINK;
> > > +	for (i = RCAR_ISP_PORT0; i < RCAR_ISP_MAX_PAD; i++)
> > > +		isp->pads[i].flags = MEDIA_PAD_FL_SOURCE;
> > > +
> > > +	ret = media_entity_pads_init(&isp->subdev.entity, RCAR_ISP_MAX_PAD,
> > > +				     isp->pads);
> > > +	if (ret)
> > > +		goto error_notifier;
> > > +
> > > +	ret = v4l2_async_register_subdev(&isp->subdev);
> > > +	if (ret < 0)
> > > +		goto error_notifier;
> > > +
> > > +	dev_info(isp->dev, "Using CSI-2 input: %u\n", isp->csi_input);
> > > +
> > > +	return 0;
> >
> > Maybe an empy line to highlight the error path
>
> I think it's a matter of preference, I (almost) never inject empty lines
> for labels :-)
>
> >
> > Anyway, all minors, you'll have a new version anyway and pm_runtime
> > usage has been under the eyes of Sakari and Hans for 5 versions and
> > they have not complained so I guess it's fine.
> >
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
> Thanks!
>
> >
> > Thanks
> >    j
> >
> > > +error_notifier:
> > > +	v4l2_async_nf_unregister(&isp->notifier);
> > > +	v4l2_async_nf_cleanup(&isp->notifier);
> > > +error_pm:
> > > +	pm_runtime_disable(&pdev->dev);
> > > +error_mutex:
> > > +	mutex_destroy(&isp->lock);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int risp_remove(struct platform_device *pdev)
> > > +{
> > > +	struct rcar_isp *isp = platform_get_drvdata(pdev);
> > > +
> > > +	v4l2_async_nf_unregister(&isp->notifier);
> > > +	v4l2_async_nf_cleanup(&isp->notifier);
> > > +
> > > +	v4l2_async_unregister_subdev(&isp->subdev);
> > > +
> > > +	pm_runtime_disable(&pdev->dev);
> > > +
> > > +	mutex_destroy(&isp->lock);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static struct platform_driver rcar_isp_driver = {
> > > +	.driver = {
> > > +		.name = "rcar-isp",
> > > +		.of_match_table = risp_of_id_table,
> > > +	},
> > > +	.probe = risp_probe,
> > > +	.remove = risp_remove,
> > > +};
> > > +
> > > +module_platform_driver(rcar_isp_driver);
> > > +
> > > +MODULE_AUTHOR("Niklas Söderlund <niklas.soderlund@ragnatech.se>");
> > > +MODULE_DESCRIPTION("Renesas R-Car ISP Channel Selector driver");
> > > +MODULE_LICENSE("GPL");
> >
> >
> > > --
> > > 2.33.0
> > >
>
> --
> Regards,
> Niklas Söderlund
  

Hi Jacopo,

On 2021-10-06 11:11:22 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Tue, Oct 05, 2021 at 08:56:09PM +0200, Niklas Söderlund wrote:
> > Hi Jacopo,
> >
> > Thanks for your review.
> >
> > On 2021-10-04 12:21:59 +0200, Jacopo Mondi wrote:
> > > Hi Niklas,
> > >
> > > On Tue, Sep 14, 2021 at 04:00:57PM +0200, Niklas Söderlund wrote:
> > > > Add a V4L2 driver for Renesas R-Car Image Signal Processor. The driver
> > > > supports the R-Car V3U SoC where the ISP IP sits between the R-Car CSI-2
> > > > receiver and VIN and filters the CSI-2 data based on VC/DT and directs
> > > > the video stream to different VIN IPs.
> > > >
> > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > > ---
> > > > * Changes since v4
> > > > - Update to the new async function names.
> > > > - Do not depend on external s_power, instead control the device power at stream start/stop.
> > > > - Do not look at remote upstream subdevice format when starting (or at all).
> > > > - Serialise access for format variables in get_fmt and set_fmt.
> > > > - Make use of v4l2_subdev_link_validate_default()
> > > > - Add mutex_destroy() on error paths and in remove function.
> > > > - Propperly disable runtime PM if subdev registration fails.
> > > >
> > > > * Changes since v3
> > > > - Fix spelling in header information.
> > > > - Add parentheses around macro expansions.
> > > > - Add explicit blocks to for-loop body.
> > > > - Add documentation for mutex.
> > > >
> > > > * Changes since v2
> > > > - Add description of IPS capabilities and that only channel selection is
> > > >   currently supported.
> > > > - Update signature of v4l2_subdev_pad_ops set_fmt.
> > > > - Switch to pm_runtime_resume_and_get().
> > > > - Make better use of devm_kzalloc().
> > > > ---
> > > >  MAINTAINERS                       |   1 +
> > > >  drivers/media/platform/Kconfig    |  16 +
> > > >  drivers/media/platform/Makefile   |   1 +
> > > >  drivers/media/platform/rcar-isp.c | 517 ++++++++++++++++++++++++++++++
> > > >  4 files changed, 535 insertions(+)
> > > >  create mode 100644 drivers/media/platform/rcar-isp.c
> > > >
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index 08c9612e769af24a..8164d1bd0a4df351 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -11575,6 +11575,7 @@ T:	git git://linuxtv.org/media_tree.git
> > > >  F:	Documentation/devicetree/bindings/media/renesas,csi2.yaml
> > > >  F:	Documentation/devicetree/bindings/media/renesas,isp.yaml
> > > >  F:	Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > +F:	drivers/media/platform/rcar-isp.c
> > > >  F:	drivers/media/platform/rcar-vin/
> > > >
> > > >  MEDIA DRIVERS FOR RENESAS - VSP1
> > > > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> > > > index aa277a19e2759617..b95e9368110c7848 100644
> > > > --- a/drivers/media/platform/Kconfig
> > > > +++ b/drivers/media/platform/Kconfig
> > > > @@ -200,6 +200,22 @@ config VIDEO_TI_CAL_MC
> > > >
> > > >  endif # VIDEO_TI_CAL
> > > >
> > > > +config VIDEO_RCAR_ISP
> > > > +	tristate "R-Car Image Signal Processor (ISP)"
> > > > +	depends on VIDEO_V4L2 && OF
> > > > +	depends on ARCH_RENESAS || COMPILE_TEST
> > > > +	select MEDIA_CONTROLLER
> > > > +	select VIDEO_V4L2_SUBDEV_API
> > > > +	select RESET_CONTROLLER
> > > > +	select V4L2_FWNODE
> > > > +	help
> > > > +	  Support for Renesas R-Car Image Signal Processor (ISP).
> > > > +	  Enable this to support the Renesas R-Car Image Signal
> > > > +	  Processor (ISP).
> > > > +
> > > > +	  To compile this driver as a module, choose M here: the
> > > > +	  module will be called rcar-isp.
> > > > +
> > > >  endif # V4L_PLATFORM_DRIVERS
> > > >
> > > >  menuconfig V4L_MEM2MEM_DRIVERS
> > > > diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> > > > index 73ce083c2fc6b51b..a148553babfc0944 100644
> > > > --- a/drivers/media/platform/Makefile
> > > > +++ b/drivers/media/platform/Makefile
> > > > @@ -63,6 +63,7 @@ obj-$(CONFIG_VIDEO_AM437X_VPFE)		+= am437x/
> > > >
> > > >  obj-$(CONFIG_VIDEO_XILINX)		+= xilinx/
> > > >
> > > > +obj-$(CONFIG_VIDEO_RCAR_ISP)		+= rcar-isp.o
> > > >  obj-$(CONFIG_VIDEO_RCAR_VIN)		+= rcar-vin/
> > > >
> > > >  obj-$(CONFIG_VIDEO_ATMEL_ISC)		+= atmel/
> > > > diff --git a/drivers/media/platform/rcar-isp.c b/drivers/media/platform/rcar-isp.c
> > > > new file mode 100644
> > > > index 0000000000000000..1055ca87975c8676
> > > > --- /dev/null
> > > > +++ b/drivers/media/platform/rcar-isp.c
> > > > @@ -0,0 +1,517 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Copyright (C) 2021 Renesas Electronics Corp.
> > > > + *
> > > > + * Driver for Renesas R-Car ISP Channel Selector
> > > > + *
> > > > + * The ISP hardware is capable of more than just channel selection, features
> > > > + * such as demosaicing, white balance control and color space conversion are
> > > > + * also possible. These more advanced features are not supported by the driver
> > > > + * due to lack of documentation.
> > > > + */
> > > > +
> > > > +#include <linux/module.h>
> > > > +#include <linux/mutex.h>
> > > > +#include <linux/of_device.h>
> > > > +#include <linux/platform_device.h>
> > > > +#include <linux/pm_runtime.h>
> > > > +#include <linux/reset.h>
> > > > +
> > > > +#include <media/v4l2-subdev.h>
> > > > +
> > > > +#define ISPINPUTSEL0_REG				0x0008
> > > > +#define ISPINPUTSEL0_SEL_CSI0				BIT(31)
> > > > +
> > > > +#define ISPSTART_REG					0x0014
> > > > +#define ISPSTART_START					0xffff
> > > > +#define ISPSTART_STOP					0x0000
> > > > +
> > > > +#define ISPPROCMODE_DT_REG(n)				(0x1100 + (0x4 * (n)))
> > > > +#define ISPPROCMODE_DT_PROC_MODE_VC3(pm)		(((pm) & 0x3f) << 24)
> > > > +#define ISPPROCMODE_DT_PROC_MODE_VC2(pm)		(((pm) & 0x3f) << 16)
> > > > +#define ISPPROCMODE_DT_PROC_MODE_VC1(pm)		(((pm) & 0x3f) << 8)
> > > > +#define ISPPROCMODE_DT_PROC_MODE_VC0(pm)		((pm) & 0x3f)
> > > > +
> > > > +#define ISPCS_FILTER_ID_CH_REG(n)			(0x3000 + (0x0100 * (n)))
> > > > +
> > > > +#define ISPCS_DT_CODE03_CH_REG(n)			(0x3008 + (0x100 * (n)))
> > > > +#define ISPCS_DT_CODE03_EN3				BIT(31)
> > > > +#define ISPCS_DT_CODE03_DT3(dt)				(((dt) & 0x3f) << 24)
> > > > +#define ISPCS_DT_CODE03_EN2				BIT(23)
> > > > +#define ISPCS_DT_CODE03_DT2(dt)				(((dt) & 0x3f) << 16)
> > > > +#define ISPCS_DT_CODE03_EN1				BIT(15)
> > > > +#define ISPCS_DT_CODE03_DT1(dt)				(((dt) & 0x3f) << 8)
> > > > +#define ISPCS_DT_CODE03_EN0				BIT(7)
> > > > +#define ISPCS_DT_CODE03_DT0(dt)				((dt) & 0x3f)
> > > > +
> > > > +struct rcar_isp_format {
> > > > +	u32 code;
> > > > +	unsigned int datatype;
> > > > +	unsigned int procmode;
> > > > +};
> > > > +
> > > > +static const struct rcar_isp_format rcar_isp_formats[] = {
> > > > +	{ .code = MEDIA_BUS_FMT_RGB888_1X24,	.datatype = 0x24, .procmode = 0x15 },
> > > > +	{ .code = MEDIA_BUS_FMT_Y10_1X10,	.datatype = 0x2b, .procmode = 0x10 },
> > > > +	{ .code = MEDIA_BUS_FMT_UYVY8_1X16,	.datatype = 0x1e, .procmode = 0x0c },
> > > > +	{ .code = MEDIA_BUS_FMT_YUYV8_1X16,	.datatype = 0x1e, .procmode = 0x0c },
> > > > +	{ .code = MEDIA_BUS_FMT_UYVY8_2X8,	.datatype = 0x1e, .procmode = 0x0c },
> > > > +	{ .code = MEDIA_BUS_FMT_YUYV10_2X10,	.datatype = 0x1e, .procmode = 0x0c },
> > >                                             ^ tab                ^ space
> > >
> > > > +};
> > > > +
> > > > +static const struct rcar_isp_format *risp_code_to_fmt(unsigned int code)
> > > > +{
> > > > +	unsigned int i;
> > > > +
> > > > +	for (i = 0; i < ARRAY_SIZE(rcar_isp_formats); i++) {
> > > > +		if (rcar_isp_formats[i].code == code)
> > > > +			return &rcar_isp_formats[i];
> > > > +	}
> > > > +
> > > > +	return NULL;
> > > > +}
> > > > +
> > > > +enum rcar_isp_input {
> > > > +	RISP_CSI_INPUT0,
> > > > +	RISP_CSI_INPUT1,
> > > > +};
> > > > +
> > > > +enum rcar_isp_pads {
> > > > +	RCAR_ISP_SINK,
> > > > +	RCAR_ISP_PORT0,
> > > > +	RCAR_ISP_PORT1,
> > > > +	RCAR_ISP_PORT2,
> > > > +	RCAR_ISP_PORT3,
> > > > +	RCAR_ISP_PORT4,
> > > > +	RCAR_ISP_PORT5,
> > > > +	RCAR_ISP_PORT6,
> > > > +	RCAR_ISP_PORT7,
> > > > +	RCAR_ISP_MAX_PAD,
> > > > +};
> > > > +
> > > > +struct rcar_isp {
> > > > +	struct device *dev;
> > > > +	void __iomem *base;
> > > > +	struct reset_control *rstc;
> > > > +
> > > > +	enum rcar_isp_input csi_input;
> > > > +
> > > > +	struct v4l2_subdev subdev;
> > > > +	struct media_pad pads[RCAR_ISP_MAX_PAD];
> > > > +
> > > > +	struct v4l2_async_notifier notifier;
> > > > +	struct v4l2_subdev *remote;
> > > > +	unsigned int remote_pad;
> > > > +
> > > > +	struct mutex lock; /* Protects mf and stream_count. */
> > > > +	struct v4l2_mbus_framefmt mf;
> > > > +	int stream_count;
> > > > +};
> > > > +
> > > > +static inline struct rcar_isp *sd_to_isp(struct v4l2_subdev *sd)
> > > > +{
> > > > +	return container_of(sd, struct rcar_isp, subdev);
> > > > +}
> > > > +
> > > > +static inline struct rcar_isp *notifier_to_isp(struct v4l2_async_notifier *n)
> > > > +{
> > > > +	return container_of(n, struct rcar_isp, notifier);
> > > > +}
> > > > +
> > > > +static void risp_write(struct rcar_isp *isp, u32 offset, u32 value)
> > > > +{
> > > > +	iowrite32(value, isp->base + offset);
> > > > +}
> > > > +
> > > > +static u32 risp_read(struct rcar_isp *isp, u32 offset)
> > > > +{
> > > > +	return ioread32(isp->base + offset);
> > > > +}
> > > > +
> > > > +static int risp_power_on(struct rcar_isp *isp)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	ret = pm_runtime_resume_and_get(isp->dev);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	ret = reset_control_deassert(isp->rstc);
> > > > +	if (ret < 0) {
> > > > +		pm_runtime_put(isp->dev);
> > > > +		return ret;
> > > > +	}
> > >
> > > I still think you should define a .resume and .suspend callbacks
> > > through something like SIMPLE_DEV_PM_OPS which assert/deassert the
> > > reset controller and just call pm_runtime_resume_and_get() and
> > > pm_runtime_put() at risp_start() and risp_stop() times.
> >
> > I wrestled a bit with this in other subdevices involved in the R-Car
> > capture chain. I was about to convert all of them to what you suggest
> > here but ran into issues with suspend/resume to s2disk IIRC if there was
> > a ongoing capture when suspending it would not start correctly. Having
> > it in the s_stream call path like this ensured the proper power on of
> > the different devices.
> 
> I was not suggesting to move the the power up/down sequences out of
> s_stream, that's fine. My suggestion was to
> 
>         int risp_resume() {
>                 reset_control_deassert()
>         }
> 
>         int risp_susped() {
>                 reset_control_deassert()
>         }
> 
>         int s_stream(bool enable) {
>                 if (enable)
>                         pm_runtime_resume_and_get();
>                 else
>                         pm_runtime_put();
>         }
> 
>         SIMPLE_DEV_PM_OPS(risp_pm_ops, risp_suspend, risp_resume);
> 
>         static struct platform_driver rcar_isp_driver = {
>                 .driver = {
>                         .name = "rcar-isp",
>                         .of_match_table = risp_of_id_table,
>                         .pm = &risp_pm_ops,
>                 },
>                 .probe = risp_probe,
>                 .remove = risp_remove,
>         };

I understood this, the issue I was refering to is that the callbacks 
from SIMPLE_DEV_PM_OPS in this design would race with the callbacks from 
rcar-vin SIMPLE_DEV_PM_OPS if there is an active stream when the system 
is suspended/resumed.

If two drivers in the pipeline implement SIMPLE_DEV_PM_OPS operations 
like you suggest here what happens when the system resumes? Right now if 
there was an active stream rcar-vin will issue a s_stream(1) call in 
it's resume callback. This then races with the design here as the 
s_stream(1) call might have completed before or during these callbacks 
are running, resetting the ISP unexpectedly.

It is less likely for IP blocks and I think I opted for this design when 
I added suspend/resume support for R-Car VIN after fighting with ADV7482 
I2C interface when resuming. I'm sure we can improve here but right now 
this is the way to go IMHO. We can look at the whole suspend/resume 
problem on top as it concerns mostly the interactions between drivers in 
a generic pipeline and is not specific to this driver.

> 
> >
> > I'm sure it can be solved using SIMPLE_DEV_PM_OPS but I have not yet dug
> > deep enough in all the drivers to figure it all out.
> >
> > >
> > > Otherwise am I wrong thinking that pm_runtime is only here for refcounting ?
> >
> > It also (depending on platform) controls the module clocks.
> >
> > >
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static void risp_power_off(struct rcar_isp *isp)
> > > > +{
> > > > +	reset_control_assert(isp->rstc);
> > > > +	pm_runtime_put(isp->dev);
> > > > +}
> > > > +
> > > > +static int risp_start(struct rcar_isp *isp)
> > > > +{
> > > > +	const struct rcar_isp_format *format;
> > > > +	unsigned int vc;
> > > > +	u32 sel_csi = 0;
> > > > +	int ret;
> > > > +
> > > > +	format = risp_code_to_fmt(isp->mf.code);
> > > > +	if (!format) {
> > > > +		dev_err(isp->dev, "Unsupported bus format\n");
> > > > +		return -EINVAL;
> > > > +	}
> > >
> > > I don't think this can now happen as isp->mf.code is set through
> > > set_fmt and it's adjusted
> >
> > Ture. But I get mail from code cover testers that always add back this
> > check so I thought I just added it myself this time ;-)
> >
> > >
> > > > +
> > > > +	ret = risp_power_on(isp);
> > > > +	if (ret) {
> > > > +		dev_err(isp->dev, "Failed to power on ISP\n");
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	/* Select CSI-2 input source. */
> > > > +	if (isp->csi_input == RISP_CSI_INPUT1)
> > > > +		sel_csi = ISPINPUTSEL0_SEL_CSI0;
> > > > +
> > > > +	risp_write(isp, ISPINPUTSEL0_REG,
> > > > +		   risp_read(isp, ISPINPUTSEL0_REG) | sel_csi);
> > > > +
> > > > +	/* Configure Channel Selector. */
> > >
> > > If I'm not mistaken we have a fixed routing from channel 4-7 to
> > > VIN[0-3] as routing is controlled by VC/DT matching. If that's the
> > > case, should this be recorded ?
> >
> > I like it the way it is :-)
> >
> 
> Up to you, just to be clear, I was just suggesting a comment, nothing
> more.
> 
> Thanks
>    j
> 
> > >
> > > > +	for (vc = 0; vc < 4; vc++) {
> > > > +		u8 ch = vc + 4;
> > > > +		u8 dt = format->datatype;
> > > > +
> > > > +		risp_write(isp, ISPCS_FILTER_ID_CH_REG(ch), BIT(vc));
> > > > +		risp_write(isp, ISPCS_DT_CODE03_CH_REG(ch),
> > > > +			   ISPCS_DT_CODE03_EN3 | ISPCS_DT_CODE03_DT3(dt) |
> > > > +			   ISPCS_DT_CODE03_EN2 | ISPCS_DT_CODE03_DT2(dt) |
> > > > +			   ISPCS_DT_CODE03_EN1 | ISPCS_DT_CODE03_DT1(dt) |
> > > > +			   ISPCS_DT_CODE03_EN0 | ISPCS_DT_CODE03_DT0(dt));
> > > > +	}
> > > > +
> > > > +	/* Setup processing method. */
> > > > +	risp_write(isp, ISPPROCMODE_DT_REG(format->datatype),
> > > > +		   ISPPROCMODE_DT_PROC_MODE_VC3(format->procmode) |
> > > > +		   ISPPROCMODE_DT_PROC_MODE_VC2(format->procmode) |
> > > > +		   ISPPROCMODE_DT_PROC_MODE_VC1(format->procmode) |
> > > > +		   ISPPROCMODE_DT_PROC_MODE_VC0(format->procmode));
> > > > +
> > > > +	/* Start ISP. */
> > > > +	risp_write(isp, ISPSTART_REG, ISPSTART_START);
> > > > +
> > > > +	ret = v4l2_subdev_call(isp->remote, video, s_stream, 1);
> > > > +	if (ret)
> > > > +		risp_power_off(isp);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static void risp_stop(struct rcar_isp *isp)
> > > > +{
> > > > +	v4l2_subdev_call(isp->remote, video, s_stream, 0);
> > > > +
> > > > +	/* Stop ISP. */
> > > > +	risp_write(isp, ISPSTART_REG, ISPSTART_STOP);
> > > > +
> > > > +	risp_power_off(isp);
> > > > +}
> > > > +
> > > > +static int risp_s_stream(struct v4l2_subdev *sd, int enable)
> > > > +{
> > > > +	struct rcar_isp *isp = sd_to_isp(sd);
> > > > +	int ret = 0;
> > > > +
> > > > +	mutex_lock(&isp->lock);
> > > > +
> > > > +	if (!isp->remote) {
> > > > +		ret = -ENODEV;
> > > > +		goto out;
> > > > +	}
> > > > +
> > > > +	if (enable && isp->stream_count == 0) {
> > > > +		ret = risp_start(isp);
> > > > +		if (ret)
> > > > +			goto out;
> > > > +	} else if (!enable && isp->stream_count == 1) {
> > > > +		risp_stop(isp);
> > > > +	}
> > > > +
> > > > +	isp->stream_count += enable ? 1 : -1;
> > > > +out:
> > > > +	mutex_unlock(&isp->lock);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static const struct v4l2_subdev_video_ops risp_video_ops = {
> > > > +	.s_stream = risp_s_stream,
> > > > +};
> > > > +
> > > > +static int risp_set_pad_format(struct v4l2_subdev *sd,
> > > > +			       struct v4l2_subdev_state *sd_state,
> > > > +			       struct v4l2_subdev_format *format)
> > > > +{
> > > > +	struct rcar_isp *isp = sd_to_isp(sd);
> > > > +	struct v4l2_mbus_framefmt *framefmt;
> > > > +
> > > > +	mutex_lock(&isp->lock);
> > > > +
> > > > +	if (!risp_code_to_fmt(format->format.code))
> > > > +		format->format.code = rcar_isp_formats[0].code;
> > > > +
> > > > +	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > > > +		isp->mf = format->format;
> > > > +	} else {
> > > > +		framefmt = v4l2_subdev_get_try_format(sd, sd_state, 0);
> > > > +		*framefmt = format->format;
> > > > +	}
> > > > +
> > > > +	mutex_unlock(&isp->lock);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int risp_get_pad_format(struct v4l2_subdev *sd,
> > > > +			       struct v4l2_subdev_state *sd_state,
> > > > +			       struct v4l2_subdev_format *format)
> > > > +{
> > > > +	struct rcar_isp *isp = sd_to_isp(sd);
> > > > +
> > > > +	mutex_lock(&isp->lock);
> > > > +
> > > > +	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > > > +		format->format = isp->mf;
> > > > +	else
> > > > +		format->format = *v4l2_subdev_get_try_format(sd, sd_state, 0);
> > > > +
> > > > +	mutex_unlock(&isp->lock);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static const struct v4l2_subdev_pad_ops risp_pad_ops = {
> > > > +	.set_fmt = risp_set_pad_format,
> > > > +	.get_fmt = risp_get_pad_format,
> > > > +	.link_validate = v4l2_subdev_link_validate_default,
> > > > +};
> > > > +
> > > > +static const struct v4l2_subdev_ops rcar_isp_subdev_ops = {
> > > > +	.video	= &risp_video_ops,
> > > > +	.pad	= &risp_pad_ops,
> > > > +};
> > > > +
> > > > +/* -----------------------------------------------------------------------------
> > > > + * Async handling and registration of subdevices and links
> > > > + */
> > > > +
> > > > +static int risp_notify_bound(struct v4l2_async_notifier *notifier,
> > > > +			     struct v4l2_subdev *subdev,
> > > > +			     struct v4l2_async_subdev *asd)
> > > > +{
> > > > +	struct rcar_isp *isp = notifier_to_isp(notifier);
> > > > +	int pad;
> > > > +
> > > > +	pad = media_entity_get_fwnode_pad(&subdev->entity, asd->match.fwnode,
> > > > +					  MEDIA_PAD_FL_SOURCE);
> > > > +	if (pad < 0) {
> > > > +		dev_err(isp->dev, "Failed to find pad for %s\n", subdev->name);
> > > > +		return pad;
> > > > +	}
> > > > +
> > > > +	isp->remote = subdev;
> > > > +	isp->remote_pad = pad;
> > >
> > > remote_pad is not used anywhere now that the format comes from the
> > > local pad.
> >
> > Good catch, thanks!
> >
> > >
> > > > +
> > > > +	dev_dbg(isp->dev, "Bound %s pad: %d\n", subdev->name, pad);
> > > > +
> > > > +	return media_create_pad_link(&subdev->entity, pad,
> > > > +				     &isp->subdev.entity, 0,
> > > > +				     MEDIA_LNK_FL_ENABLED |
> > > > +				     MEDIA_LNK_FL_IMMUTABLE);
> > > > +}
> > > > +
> > > > +static void risp_notify_unbind(struct v4l2_async_notifier *notifier,
> > > > +			       struct v4l2_subdev *subdev,
> > > > +			       struct v4l2_async_subdev *asd)
> > > > +{
> > > > +	struct rcar_isp *isp = notifier_to_isp(notifier);
> > > > +
> > > > +	isp->remote = NULL;
> > > > +
> > > > +	dev_dbg(isp->dev, "Unbind %s\n", subdev->name);
> > > > +}
> > > > +
> > > > +static const struct v4l2_async_notifier_operations risp_notify_ops = {
> > > > +	.bound = risp_notify_bound,
> > > > +	.unbind = risp_notify_unbind,
> > > > +};
> > > > +
> > > > +static int risp_parse_dt(struct rcar_isp *isp)
> > > > +{
> > > > +	struct v4l2_async_subdev *asd;
> > > > +	struct fwnode_handle *fwnode;
> > > > +	struct fwnode_handle *ep;
> > > > +	unsigned int id;
> > > > +	int ret;
> > > > +
> > > > +	for (id = 0; id < 2; id++) {
> > > > +		ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(isp->dev),
> > > > +						     0, id, 0);
> > > > +		if (ep)
> > > > +			break;
> > > > +	}
> > > > +
> > > > +	if (!ep) {
> > > > +		dev_err(isp->dev, "Not connected to subdevice\n");
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	if (id == 1)
> > > > +		isp->csi_input = RISP_CSI_INPUT1;
> > > > +
> > > > +	fwnode = fwnode_graph_get_remote_endpoint(ep);
> > > > +	fwnode_handle_put(ep);
> > > > +
> > > > +	dev_dbg(isp->dev, "Found '%pOF'\n", to_of_node(fwnode));
> > > > +
> > > > +	v4l2_async_nf_init(&isp->notifier);
> > > > +	isp->notifier.ops = &risp_notify_ops;
> > > > +
> > > > +	asd = v4l2_async_nf_add_fwnode(&isp->notifier, fwnode,
> > > > +				       struct v4l2_async_subdev);
> > > > +	fwnode_handle_put(fwnode);
> > > > +	if (IS_ERR(asd))
> > > > +		return PTR_ERR(asd);
> > > > +
> > > > +	ret = v4l2_async_subdev_nf_register(&isp->subdev, &isp->notifier);
> > > > +	if (ret)
> > > > +		v4l2_async_nf_cleanup(&isp->notifier);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +/* -----------------------------------------------------------------------------
> > > > + * Platform Device Driver
> > > > + */
> > > > +
> > > > +static const struct media_entity_operations risp_entity_ops = {
> > > > +	.link_validate = v4l2_subdev_link_validate,
> > > > +};
> > > > +
> > > > +static int risp_probe_resources(struct rcar_isp *isp,
> > > > +				struct platform_device *pdev)
> > > > +{
> > > > +	struct resource *res;
> > > > +
> > > > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > > +	isp->base = devm_ioremap_resource(&pdev->dev, res);
> > > > +	if (IS_ERR(isp->base))
> > > > +		return PTR_ERR(isp->base);
> > > > +
> > > > +	isp->rstc = devm_reset_control_get(&pdev->dev, NULL);
> > > > +
> > > > +	return PTR_ERR_OR_ZERO(isp->rstc);
> > > > +}
> > > > +
> > > > +static const struct of_device_id risp_of_id_table[] = {
> > > > +	{ .compatible = "renesas,r8a779a0-isp" },
> > > > +	{ /* sentinel */ },
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, risp_of_id_table);
> > > > +
> > > > +static int risp_probe(struct platform_device *pdev)
> > > > +{
> > > > +	struct rcar_isp *isp;
> > > > +	unsigned int i;
> > > > +	int ret;
> > > > +
> > > > +	isp = devm_kzalloc(&pdev->dev, sizeof(*isp), GFP_KERNEL);
> > > > +	if (!isp)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	isp->dev = &pdev->dev;
> > > > +
> > > > +	mutex_init(&isp->lock);
> > > > +
> > > > +	ret = risp_probe_resources(isp, pdev);
> > > > +	if (ret) {
> > > > +		dev_err(isp->dev, "Failed to get resources\n");
> > > > +		goto error_mutex;
> > > > +	}
> > > > +
> > > > +	platform_set_drvdata(pdev, isp);
> > > > +
> > > > +	pm_runtime_enable(&pdev->dev);
> > > > +
> > > > +	ret = risp_parse_dt(isp);
> > > > +	if (ret)
> > > > +		goto error_pm;
> > > > +
> > > > +	isp->subdev.owner = THIS_MODULE;
> > > > +	isp->subdev.dev = &pdev->dev;
> > > > +	v4l2_subdev_init(&isp->subdev, &rcar_isp_subdev_ops);
> > > > +	v4l2_set_subdevdata(&isp->subdev, &pdev->dev);
> > > > +	snprintf(isp->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s %s",
> > > > +		 KBUILD_MODNAME, dev_name(&pdev->dev));
> > > > +	isp->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
> > > > +
> > > > +	isp->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
> > > > +	isp->subdev.entity.ops = &risp_entity_ops;
> > > > +
> > > > +	isp->pads[RCAR_ISP_SINK].flags = MEDIA_PAD_FL_SINK;
> > > > +	for (i = RCAR_ISP_PORT0; i < RCAR_ISP_MAX_PAD; i++)
> > > > +		isp->pads[i].flags = MEDIA_PAD_FL_SOURCE;
> > > > +
> > > > +	ret = media_entity_pads_init(&isp->subdev.entity, RCAR_ISP_MAX_PAD,
> > > > +				     isp->pads);
> > > > +	if (ret)
> > > > +		goto error_notifier;
> > > > +
> > > > +	ret = v4l2_async_register_subdev(&isp->subdev);
> > > > +	if (ret < 0)
> > > > +		goto error_notifier;
> > > > +
> > > > +	dev_info(isp->dev, "Using CSI-2 input: %u\n", isp->csi_input);
> > > > +
> > > > +	return 0;
> > >
> > > Maybe an empy line to highlight the error path
> >
> > I think it's a matter of preference, I (almost) never inject empty lines
> > for labels :-)
> >
> > >
> > > Anyway, all minors, you'll have a new version anyway and pm_runtime
> > > usage has been under the eyes of Sakari and Hans for 5 versions and
> > > they have not complained so I guess it's fine.
> > >
> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> >
> > Thanks!
> >
> > >
> > > Thanks
> > >    j
> > >
> > > > +error_notifier:
> > > > +	v4l2_async_nf_unregister(&isp->notifier);
> > > > +	v4l2_async_nf_cleanup(&isp->notifier);
> > > > +error_pm:
> > > > +	pm_runtime_disable(&pdev->dev);
> > > > +error_mutex:
> > > > +	mutex_destroy(&isp->lock);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static int risp_remove(struct platform_device *pdev)
> > > > +{
> > > > +	struct rcar_isp *isp = platform_get_drvdata(pdev);
> > > > +
> > > > +	v4l2_async_nf_unregister(&isp->notifier);
> > > > +	v4l2_async_nf_cleanup(&isp->notifier);
> > > > +
> > > > +	v4l2_async_unregister_subdev(&isp->subdev);
> > > > +
> > > > +	pm_runtime_disable(&pdev->dev);
> > > > +
> > > > +	mutex_destroy(&isp->lock);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static struct platform_driver rcar_isp_driver = {
> > > > +	.driver = {
> > > > +		.name = "rcar-isp",
> > > > +		.of_match_table = risp_of_id_table,
> > > > +	},
> > > > +	.probe = risp_probe,
> > > > +	.remove = risp_remove,
> > > > +};
> > > > +
> > > > +module_platform_driver(rcar_isp_driver);
> > > > +
> > > > +MODULE_AUTHOR("Niklas Söderlund <niklas.soderlund@ragnatech.se>");
> > > > +MODULE_DESCRIPTION("Renesas R-Car ISP Channel Selector driver");
> > > > +MODULE_LICENSE("GPL");
> > >
> > >
> > > > --
> > > > 2.33.0
> > > >
> >
> > --
> > Regards,
> > Niklas Söderlund