Re: [PATCH v8 09/14] media: rkisp1: add rockchip isp1 core driver

[Helen Koike <helen.koike@collabora.com>]

Hi Sakari,

Thanks for your review, just some comments/questions below:

On 8/7/19 12:27 PM, Sakari Ailus wrote:
> Hi Helen,
> 
> On Tue, Jul 30, 2019 at 03:42:51PM -0300, Helen Koike wrote:
>> From: Jacob Chen <jacob2.chen@rock-chips.com>
>>
>> Add the core driver for rockchip isp1.
>>
>> Signed-off-by: Jacob Chen <jacob2.chen@rock-chips.com>
>> Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com>
>> Signed-off-by: Yichong Zhong <zyc@rock-chips.com>
>> Signed-off-by: Jacob Chen <cc@rock-chips.com>
>> Signed-off-by: Eddie Cai <eddie.cai.linux@gmail.com>
>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>> Signed-off-by: Allon Huang <allon.huang@rock-chips.com>
>> Signed-off-by: Tomasz Figa <tfiga@chromium.org>
>> [fixed compilation and run time errors regarding new v4l2 async API]
>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> [Add missing module device table]
>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
>> [update for upstream]
>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>
>> ---
>>
>> Changes in v8: None
>> Changes in v7:
>> - VIDEO_ROCKCHIP_ISP1 selects VIDEOBUF2_VMALLOC
>> - add PHY_ROCKCHIP_DPHY as a dependency for VIDEO_ROCKCHIP_ISP1
>> - Fix compilation and runtime errors due to bitrotting
>> The code has bit-rotten since March 2018, fix compilation errors.
>> The new V4L2 async notifier API requires notifiers to be initialized by
>> a call to v4l2_async_notifier_init() before being used, do so.
>> - Add missing module device table
>> - use clk_bulk framework
>> - add missing notifiers cleanups
>> - s/strlcpy/strscpy
>> - normalize bus_info name
>> - fix s_stream error path, stream_cnt wans't being decremented properly
>> - use devm_platform_ioremap_resource() helper
>> - s/deice/device
>> - redesign: remove mipi/csi subdevice, sensors connect directly to the
>> isp subdevice in the media topology now.
>> - remove "saved_state" member from rkisp1_stream struct
>> - Reverse the order of MIs
>> - Simplify MI interrupt handling
>> Rather than adding unnecessary indirection, just use stream index to
>> handle MI interrupt enable/disable/clear, since the stream index matches
>> the order of bits now, thanks to previous patch. While at it, remove
>> some dead code.
>> - code styling and checkpatch fixes
>>
>>  drivers/media/platform/Kconfig                |  12 +
>>  drivers/media/platform/Makefile               |   1 +
>>  drivers/media/platform/rockchip/isp1/Makefile |   7 +
>>  drivers/media/platform/rockchip/isp1/common.h | 101 +++
>>  drivers/media/platform/rockchip/isp1/dev.c    | 675 ++++++++++++++++++
>>  drivers/media/platform/rockchip/isp1/dev.h    |  97 +++
>>  6 files changed, 893 insertions(+)
>>  create mode 100644 drivers/media/platform/rockchip/isp1/Makefile
>>  create mode 100644 drivers/media/platform/rockchip/isp1/common.h
>>  create mode 100644 drivers/media/platform/rockchip/isp1/dev.c
>>  create mode 100644 drivers/media/platform/rockchip/isp1/dev.h
>>
>> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
>> index 89555f9a813f..e0e98937c565 100644
>> --- a/drivers/media/platform/Kconfig
>> +++ b/drivers/media/platform/Kconfig
>> @@ -106,6 +106,18 @@ config VIDEO_QCOM_CAMSS
>>  	select VIDEOBUF2_DMA_SG
>>  	select V4L2_FWNODE
>>  
>> +config VIDEO_ROCKCHIP_ISP1
>> +	tristate "Rockchip Image Signal Processing v1 Unit driver"
>> +	depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
>> +	depends on ARCH_ROCKCHIP || COMPILE_TEST
>> +	select VIDEOBUF2_DMA_CONTIG
>> +	select VIDEOBUF2_VMALLOC
>> +	select V4L2_FWNODE
>> +	select PHY_ROCKCHIP_DPHY
>> +	default n
>> +	---help---
>> +	  Support for ISP1 on the rockchip SoC.
>> +
>>  config VIDEO_S3C_CAMIF
>>  	tristate "Samsung S3C24XX/S3C64XX SoC Camera Interface driver"
>>  	depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API
>> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
>> index 7cbbd925124c..f9fcf8e7c513 100644
>> --- a/drivers/media/platform/Makefile
>> +++ b/drivers/media/platform/Makefile
>> @@ -69,6 +69,7 @@ obj-$(CONFIG_VIDEO_RENESAS_FDP1)	+= rcar_fdp1.o
>>  obj-$(CONFIG_VIDEO_RENESAS_JPU)		+= rcar_jpu.o
>>  obj-$(CONFIG_VIDEO_RENESAS_VSP1)	+= vsp1/
>>  
>> +obj-$(CONFIG_VIDEO_ROCKCHIP_ISP1)	+= rockchip/isp1/
>>  obj-$(CONFIG_VIDEO_ROCKCHIP_RGA)	+= rockchip/rga/
>>  
>>  obj-y	+= omap/
>> diff --git a/drivers/media/platform/rockchip/isp1/Makefile b/drivers/media/platform/rockchip/isp1/Makefile
>> new file mode 100644
>> index 000000000000..72706e80fc8b
>> --- /dev/null
>> +++ b/drivers/media/platform/rockchip/isp1/Makefile
>> @@ -0,0 +1,7 @@
>> +obj-$(CONFIG_VIDEO_ROCKCHIP_ISP1) += 	rockchip-isp1.o
>> +rockchip-isp1-objs 	   += 	rkisp1.o \
>> +				dev.o \
>> +				regs.o \
>> +				isp_stats.o \
>> +				isp_params.o \
>> +				capture.o
>> diff --git a/drivers/media/platform/rockchip/isp1/common.h b/drivers/media/platform/rockchip/isp1/common.h
>> new file mode 100644
>> index 000000000000..606ce2793546
>> --- /dev/null
>> +++ b/drivers/media/platform/rockchip/isp1/common.h
>> @@ -0,0 +1,101 @@
>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
>> +/*
>> + * Rockchip isp1 driver
>> + *
>> + * Copyright (C) 2017 Rockchip Electronics Co., Ltd.
>> + */
>> +
>> +#ifndef _RKISP1_COMMON_H
>> +#define _RKISP1_COMMON_H
>> +
>> +#include <linux/mutex.h>
>> +#include <media/media-device.h>
>> +#include <media/media-entity.h>
>> +#include <media/v4l2-ctrls.h>
>> +#include <media/v4l2-device.h>
>> +#include <media/videobuf2-v4l2.h>
>> +
>> +#define RKISP1_DEFAULT_WIDTH		800
>> +#define RKISP1_DEFAULT_HEIGHT		600
>> +
>> +#define RKISP1_MAX_STREAM		2
>> +#define RKISP1_STREAM_MP		0
>> +#define RKISP1_STREAM_SP		1
>> +
>> +#define RKISP1_PLANE_Y			0
>> +#define RKISP1_PLANE_CB			1
>> +#define RKISP1_PLANE_CR			2
>> +
>> +enum rkisp1_sd_type {
>> +	RKISP1_SD_SENSOR,
>> +	RKISP1_SD_PHY_CSI,
>> +	RKISP1_SD_VCM,
>> +	RKISP1_SD_FLASH,
>> +	RKISP1_SD_MAX,
>> +};
> 
> I wonder if this is a leftover from the driver development time. Same goes
> for the subdevs field in struct rkisp1_device.

It is a left over, I'm removing them for the next version, thanks.

> 
>> +
>> +/* One structure per video node */
>> +struct rkisp1_vdev_node {
>> +	struct vb2_queue buf_queue;
>> +	/* vfd lock */
>> +	struct mutex vlock;
>> +	struct video_device vdev;
>> +	struct media_pad pad;
>> +};
>> +
>> +enum rkisp1_fmt_pix_type {
>> +	FMT_YUV,
>> +	FMT_RGB,
>> +	FMT_BAYER,
>> +	FMT_JPEG,
>> +	FMT_MAX
>> +};
>> +
>> +enum rkisp1_fmt_raw_pat_type {
>> +	RAW_RGGB = 0,
>> +	RAW_GRBG,
>> +	RAW_GBRG,
>> +	RAW_BGGR,
>> +};
>> +
>> +struct rkisp1_buffer {
>> +	struct vb2_v4l2_buffer vb;
>> +	struct list_head queue;
>> +	union {
>> +		u32 buff_addr[VIDEO_MAX_PLANES];
>> +		void *vaddr[VIDEO_MAX_PLANES];
>> +	};
>> +};
>> +
>> +struct rkisp1_dummy_buffer {
>> +	void *vaddr;
>> +	dma_addr_t dma_addr;
>> +	u32 size;
>> +};
>> +
>> +extern int rkisp1_debug;
>> +
>> +static inline
>> +struct rkisp1_vdev_node *vdev_to_node(struct video_device *vdev)
>> +{
>> +	return container_of(vdev, struct rkisp1_vdev_node, vdev);
>> +}
>> +
>> +static inline struct rkisp1_vdev_node *queue_to_node(struct vb2_queue *q)
>> +{
>> +	return container_of(q, struct rkisp1_vdev_node, buf_queue);
>> +}
>> +
>> +static inline struct rkisp1_buffer *to_rkisp1_buffer(struct vb2_v4l2_buffer *vb)
>> +{
>> +	return container_of(vb, struct rkisp1_buffer, vb);
>> +}
>> +
>> +static inline struct vb2_queue *to_vb2_queue(struct file *file)
>> +{
>> +	struct rkisp1_vdev_node *vnode = video_drvdata(file);
>> +
>> +	return &vnode->buf_queue;
>> +}
>> +
>> +#endif /* _RKISP1_COMMON_H */
>> diff --git a/drivers/media/platform/rockchip/isp1/dev.c b/drivers/media/platform/rockchip/isp1/dev.c
>> new file mode 100644
>> index 000000000000..2b4a67e1a3b5
>> --- /dev/null
>> +++ b/drivers/media/platform/rockchip/isp1/dev.c
>> @@ -0,0 +1,675 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Rockchip isp1 driver
>> + *
>> + * Copyright (C) 2017 Rockchip Electronics Co., Ltd.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_graph.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/pinctrl/consumer.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/phy/phy-mipi-dphy.h>
>> +
>> +#include "common.h"
>> +#include "regs.h"
>> +
>> +struct isp_match_data {
>> +	const char * const *clks;
>> +	int size;
> 
> unsigned int

ack

> 
>> +};
>> +
>> +struct sensor_async_subdev {
>> +	struct v4l2_async_subdev asd;
>> +	struct v4l2_mbus_config mbus;
>> +	unsigned int lanes;
>> +};
>> +
>> +int rkisp1_debug;
>> +module_param_named(debug, rkisp1_debug, int, 0644);
>> +MODULE_PARM_DESC(debug, "Debug level (0-1)");
> 
> Have you thought of using dynamic debug instead?

right, this is being used in v4l2_dbg(), which I can replace by dev_dbg()
that is already covered by dynamic debug iirc.
Should I also replace v4l2_err() by dev_err() (I always get confused by
which log function I should use).

> 
>> +
>> +/**************************** pipeline operations******************************/
>> +
>> +static int __isp_pipeline_prepare(struct rkisp1_pipeline *p,
>> +				  struct media_entity *me)
>> +{
>> +	struct rkisp1_device *dev = container_of(p, struct rkisp1_device, pipe);
>> +	struct v4l2_subdev *sd;
>> +	unsigned int i;
>> +
>> +	p->num_subdevs = 0;
>> +	memset(p->subdevs, 0, sizeof(p->subdevs));
>> +
>> +	while (1) {
>> +		struct media_pad *pad = NULL;
>> +
>> +		/* Find remote source pad */
>> +		for (i = 0; i < me->num_pads; i++) {
>> +			struct media_pad *spad = &me->pads[i];
>> +
>> +			if (!(spad->flags & MEDIA_PAD_FL_SINK))
>> +				continue;
>> +			pad = media_entity_remote_pad(spad);
>> +			if (pad)
>> +				break;
>> +		}
>> +
>> +		if (!pad)
>> +			break;
>> +
>> +		sd = media_entity_to_v4l2_subdev(pad->entity);
>> +		if (sd != &dev->isp_sdev.sd)
>> +			p->subdevs[p->num_subdevs++] = sd;
> 
> How do you make sure you don't overrun the array?

Because the maximum path the topology can have is:
sensor->rkisp->capture

> 
> Instead, I'd avoid maintaining the array in the first place --- the same
> information is available from the MC framework data structures --- see e.g.
> the omap3isp driver.

If I understand correctly, omap3isp navigates through the topology in the same way,
but it just don't save in an array, but I reuse this information in other places,
mostly for power up/down (see below why I don't use v4l2_pipeline_pm_use())

> 
>> +
>> +		me = &sd->entity;
>> +		if (me->num_pads == 1)
>> +			break;
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int __subdev_set_power(struct v4l2_subdev *sd, int on)
>> +{
>> +	int ret;
>> +
>> +	if (!sd)
>> +		return -ENXIO;
>> +
>> +	ret = v4l2_subdev_call(sd, core, s_power, on);
>> +
>> +	return ret != -ENOIOCTLCMD ? ret : 0;
>> +}
>> +
>> +static int __isp_pipeline_s_power(struct rkisp1_pipeline *p, bool on)
> 
> Could you instead use v4l2_pipeline_pm_use()?

Unless I misunderstood this (which is very likely), v4l2_pipeline_pm_use() calls pipeline_pm_power(),
that applies power change to all entities in a pipeline, but if I have two sensors
connected to the ISP, one with link enabled and the other with a disabled link,
I don't want to power both sensors on, just the one participating in that stream. And
if I call v4l2_pipeline_pm_use() it will power on both, which is not what I want.

Does it make sense?

> 
>> +{
>> +	struct rkisp1_device *dev = container_of(p, struct rkisp1_device, pipe);
>> +	int i, ret;
>> +
>> +	if (on) {
>> +		__subdev_set_power(&dev->isp_sdev.sd, true);
>> +
>> +		for (i = p->num_subdevs - 1; i >= 0; --i) {
>> +			ret = __subdev_set_power(p->subdevs[i], true);
>> +			if (ret < 0 && ret != -ENXIO)
>> +				goto err_power_off;
>> +		}
>> +	} else {
>> +		for (i = 0; i < p->num_subdevs; ++i)
>> +			__subdev_set_power(p->subdevs[i], false);
>> +
>> +		__subdev_set_power(&dev->isp_sdev.sd, false);
>> +	}
>> +
>> +	return 0;
>> +
>> +err_power_off:
>> +	for (++i; i < p->num_subdevs; ++i)
>> +		__subdev_set_power(p->subdevs[i], false);
>> +	__subdev_set_power(&dev->isp_sdev.sd, true);
>> +	return ret;
>> +}
>> +
>> +static int rkisp1_pipeline_open(struct rkisp1_pipeline *p,
>> +				struct media_entity *me,
>> +				bool prepare)
>> +{
>> +	int ret;
>> +
>> +	if (WARN_ON(!p || !me))
>> +		return -EINVAL;
>> +	if (atomic_inc_return(&p->power_cnt) > 1)
>> +		return 0;
>> +
>> +	/* go through media graphic and get subdevs */
>> +	if (prepare)
>> +		__isp_pipeline_prepare(p, me);
>> +
>> +	if (!p->num_subdevs)
>> +		return -EINVAL;
>> +
>> +	ret = __isp_pipeline_s_power(p, 1);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>> +static int rkisp1_pipeline_close(struct rkisp1_pipeline *p)
>> +{
>> +	int ret;
>> +
>> +	if (atomic_dec_return(&p->power_cnt) > 0)
>> +		return 0;
>> +	ret = __isp_pipeline_s_power(p, 0);
>> +
>> +	return ret == -ENXIO ? 0 : ret;
>> +}
>> +
>> +/*
>> + * stream-on order: isp_subdev, mipi dphy, sensor
>> + * stream-off order: mipi dphy, sensor, isp_subdev
>> + */
>> +static int rkisp1_pipeline_set_stream(struct rkisp1_pipeline *p, bool on)
>> +{
>> +	struct rkisp1_device *dev = container_of(p, struct rkisp1_device, pipe);
>> +	int i, ret;
>> +
>> +	if ((on && atomic_inc_return(&p->stream_cnt) > 1) ||
>> +	    (!on && atomic_dec_return(&p->stream_cnt) > 0))
>> +		return 0;
>> +
>> +	if (on) {
>> +		ret = v4l2_subdev_call(&dev->isp_sdev.sd, video, s_stream,
>> +				       true);
>> +		if (ret && ret != -ENOIOCTLCMD && ret != -ENODEV) {
>> +			v4l2_err(&dev->v4l2_dev,
>> +				 "s_stream failed on subdevice %s (%d)\n",
>> +				 dev->isp_sdev.sd.name,
>> +				 ret);
>> +			atomic_dec(&p->stream_cnt);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	/* phy -> sensor */
>> +	for (i = 0; i < p->num_subdevs; ++i) {
>> +		ret = v4l2_subdev_call(p->subdevs[i], video, s_stream, on);
>> +		if (on && ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
>> +			goto err_stream_off;
> 
> You should stop after the first external sub-device.
> 
> It seems even the omap3isp driver doesn't do that. It's not easy to spot
> such issues indeed.

I'm not sure I understand, what do you call an external sub-device? Is the sensor
an external subdevice?

> 
>> +	}
>> +
>> +	if (!on)
>> +		v4l2_subdev_call(&dev->isp_sdev.sd, video, s_stream, false);
>> +
>> +	return 0;
>> +
>> +err_stream_off:
>> +	for (--i; i >= 0; --i)
>> +		v4l2_subdev_call(p->subdevs[i], video, s_stream, false);
>> +	v4l2_subdev_call(&dev->isp_sdev.sd, video, s_stream, false);
>> +	atomic_dec(&p->stream_cnt);
>> +	return ret;
>> +}
>> +
>> +/***************************** media controller *******************************/
>> +/* See http://opensource.rock-chips.com/wiki_Rockchip-isp1 for Topology */
> 
> The host appears to be down, or there's a routing problem. Unless this is
> fixed, having such a URL here doesn't do much good. :-I

This is a left over, sorry about that.
I can access the URL now. I'll try to get some of the docs and move to the kernel docs.

> 
>> +
>> +static int rkisp1_create_links(struct rkisp1_device *dev)
>> +{
>> +	struct media_entity *source, *sink;
>> +	struct rkisp1_sensor *sensor;
>> +	unsigned int flags, pad;
>> +	int ret;
>> +
>> +	/* sensor links(or mipi-phy) */
>> +	list_for_each_entry(sensor, &dev->sensors, list) {
>> +		for (pad = 0; pad < sensor->sd->entity.num_pads; pad++)
>> +			if (sensor->sd->entity.pads[pad].flags &
>> +				MEDIA_PAD_FL_SOURCE)
>> +				break;
> 
> Could you use media_entity_get_fwnode_pad() instead?

Yes, I didn't know about it actually, thanks for that, looks cleaner (I'll send in
the next version).

> 
>> +
>> +		if (pad == sensor->sd->entity.num_pads) {
>> +			dev_err(dev->dev,
>> +				"failed to find src pad for %s\n",
>> +				sensor->sd->name);
>> +
>> +			return -ENXIO;
>> +		}
>> +
>> +		ret = media_create_pad_link(
>> +				&sensor->sd->entity, pad,
>> +				&dev->isp_sdev.sd.entity,
>> +				RKISP1_ISP_PAD_SINK,
>> +				list_is_first(&sensor->list, &dev->sensors) ?
>> +				MEDIA_LNK_FL_ENABLED : 0);
>> +		if (ret) {
>> +			dev_err(dev->dev,
>> +				"failed to create link for %s\n",
>> +				sensor->sd->name);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	/* params links */
>> +	source = &dev->params_vdev.vnode.vdev.entity;
>> +	sink = &dev->isp_sdev.sd.entity;
>> +	flags = MEDIA_LNK_FL_ENABLED;
>> +	ret = media_create_pad_link(source, 0, sink,
>> +				       RKISP1_ISP_PAD_SINK_PARAMS, flags);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* create isp internal links */
>> +	/* SP links */
>> +	source = &dev->isp_sdev.sd.entity;
>> +	sink = &dev->stream[RKISP1_STREAM_SP].vnode.vdev.entity;
>> +	ret = media_create_pad_link(source, RKISP1_ISP_PAD_SOURCE_PATH,
>> +				       sink, 0, flags);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* MP links */
>> +	source = &dev->isp_sdev.sd.entity;
>> +	sink = &dev->stream[RKISP1_STREAM_MP].vnode.vdev.entity;
>> +	ret = media_create_pad_link(source, RKISP1_ISP_PAD_SOURCE_PATH,
>> +				       sink, 0, flags);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* 3A stats links */
>> +	source = &dev->isp_sdev.sd.entity;
>> +	sink = &dev->stats_vdev.vnode.vdev.entity;
>> +	return media_create_pad_link(source, RKISP1_ISP_PAD_SOURCE_STATS,
>> +					sink, 0, flags);
> 
> Indentation. Same for the calls to the same function above.

ack

> 
>> +}
>> +
>> +static int subdev_notifier_bound(struct v4l2_async_notifier *notifier,
>> +				 struct v4l2_subdev *sd,
>> +				 struct v4l2_async_subdev *asd)
>> +{
>> +	struct rkisp1_device *isp_dev = container_of(notifier,
>> +						     struct rkisp1_device,
>> +						     notifier);
>> +	struct sensor_async_subdev *s_asd = container_of(asd,
>> +					struct sensor_async_subdev, asd);
>> +	struct rkisp1_sensor *sensor;
>> +
>> +	sensor = devm_kzalloc(isp_dev->dev, sizeof(*sensor), GFP_KERNEL);
>> +	if (!sensor)
>> +		return -ENOMEM;
>> +
>> +	sensor->lanes = s_asd->lanes;
>> +	sensor->mbus = s_asd->mbus;
>> +	sensor->sd = sd;
>> +	sensor->dphy = devm_phy_get(isp_dev->dev, "dphy");
>> +	if (IS_ERR(sensor->dphy)) {
>> +		if (PTR_ERR(sensor->dphy) != -EPROBE_DEFER)
>> +			dev_err(isp_dev->dev, "Couldn't get the MIPI D-PHY\n");
>> +		return PTR_ERR(sensor->dphy);
>> +	}
>> +	phy_init(sensor->dphy);
>> +
>> +	list_add(&sensor->list, &isp_dev->sensors);
> 
> In general, maintaining the information on the external subdevs on your own
> adds complexity to the driver. You can get the information when you need it
> from the data structures maintained by MC (see e.g. the omap3isp driver for
> examples).
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static struct rkisp1_sensor *sd_to_sensor(struct rkisp1_device *dev,
>> +					  struct v4l2_subdev *sd)
>> +{
>> +	struct rkisp1_sensor *sensor;
>> +
>> +	list_for_each_entry(sensor, &dev->sensors, list)
>> +		if (sensor->sd == sd)
>> +			return sensor;
>> +
>> +	return NULL;
>> +}
>> +
>> +static void subdev_notifier_unbind(struct v4l2_async_notifier *notifier,
>> +				   struct v4l2_subdev *sd,
>> +				   struct v4l2_async_subdev *asd)
>> +{
>> +	struct rkisp1_device *isp_dev = container_of(notifier,
>> +						     struct rkisp1_device,
>> +						     notifier);
>> +	struct rkisp1_sensor *sensor = sd_to_sensor(isp_dev, sd);
>> +
>> +	/* TODO: check if a lock is required here */
>> +	list_del(&sensor->list);
>> +
>> +	phy_exit(sensor->dphy);
>> +}
>> +
>> +static int subdev_notifier_complete(struct v4l2_async_notifier *notifier)
>> +{
>> +	struct rkisp1_device *dev = container_of(notifier, struct rkisp1_device,
>> +						 notifier);
>> +	int ret;
>> +
>> +	mutex_lock(&dev->media_dev.graph_mutex);
>> +	ret = rkisp1_create_links(dev);
>> +	if (ret < 0)
>> +		goto unlock;
>> +	ret = v4l2_device_register_subdev_nodes(&dev->v4l2_dev);
>> +	if (ret < 0)
>> +		goto unlock;
>> +
>> +	v4l2_info(&dev->v4l2_dev, "Async subdev notifier completed\n");
>> +
>> +unlock:
>> +	mutex_unlock(&dev->media_dev.graph_mutex);
>> +	return ret;
>> +}
>> +
>> +static int rkisp1_fwnode_parse(struct device *dev,
>> +			       struct v4l2_fwnode_endpoint *vep,
>> +			       struct v4l2_async_subdev *asd)
>> +{
>> +	struct sensor_async_subdev *s_asd =
>> +			container_of(asd, struct sensor_async_subdev, asd);
>> +
>> +	if (vep->bus_type != V4L2_MBUS_CSI2_DPHY) {
>> +		dev_err(dev, "Only CSI2 bus type is currently supported\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (vep->base.port != 0) {
>> +		dev_err(dev, "The ISP has only port 0\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	s_asd->mbus.type = vep->bus_type;
>> +	s_asd->mbus.flags = vep->bus.mipi_csi2.flags;
>> +	s_asd->lanes = vep->bus.mipi_csi2.num_data_lanes;
>> +
>> +	switch (vep->bus.mipi_csi2.num_data_lanes) {
>> +	case 1:
>> +		s_asd->mbus.flags |= V4L2_MBUS_CSI2_1_LANE;
>> +		break;
>> +	case 2:
>> +		s_asd->mbus.flags |= V4L2_MBUS_CSI2_2_LANE;
>> +		break;
>> +	case 3:
>> +		s_asd->mbus.flags |= V4L2_MBUS_CSI2_3_LANE;
>> +		break;
>> +	case 4:
>> +		s_asd->mbus.flags |= V4L2_MBUS_CSI2_4_LANE;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct v4l2_async_notifier_operations subdev_notifier_ops = {
>> +	.bound = subdev_notifier_bound,
>> +	.unbind = subdev_notifier_unbind,
>> +	.complete = subdev_notifier_complete,
>> +};
>> +
>> +static int isp_subdev_notifier(struct rkisp1_device *isp_dev)
>> +{
>> +	struct v4l2_async_notifier *ntf = &isp_dev->notifier;
>> +	struct device *dev = isp_dev->dev;
>> +	int ret;
>> +
>> +	v4l2_async_notifier_init(ntf);
>> +
>> +	ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
>> +		dev, ntf, sizeof(struct sensor_async_subdev), 0,
>> +		rkisp1_fwnode_parse);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (list_empty(&ntf->asd_list))
>> +		return -ENODEV;	/* no endpoint */
>> +
>> +	ntf->ops = &subdev_notifier_ops;
>> +
>> +	return v4l2_async_notifier_register(&isp_dev->v4l2_dev, ntf);
>> +}
>> +
>> +/***************************** platform device *******************************/
>> +
>> +static int rkisp1_register_platform_subdevs(struct rkisp1_device *dev)
>> +{
>> +	int ret;
>> +
>> +	ret = rkisp1_register_isp_subdev(dev, &dev->v4l2_dev);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = rkisp1_register_stream_vdevs(dev);
>> +	if (ret < 0)
>> +		goto err_unreg_isp_subdev;
>> +
>> +	ret = rkisp1_register_stats_vdev(&dev->stats_vdev, &dev->v4l2_dev, dev);
>> +	if (ret < 0)
>> +		goto err_unreg_stream_vdev;
>> +
>> +	ret = rkisp1_register_params_vdev(&dev->params_vdev, &dev->v4l2_dev,
>> +					  dev);
>> +	if (ret < 0)
>> +		goto err_unreg_stats_vdev;
>> +
>> +	ret = isp_subdev_notifier(dev);
>> +	if (ret < 0) {
>> +		v4l2_err(&dev->v4l2_dev,
>> +			 "Failed to register subdev notifier(%d)\n", ret);
>> +		goto err_unreg_params_vdev;
>> +	}
>> +
>> +	return 0;
>> +err_unreg_params_vdev:
>> +	rkisp1_unregister_params_vdev(&dev->params_vdev);
>> +err_unreg_stats_vdev:
>> +	rkisp1_unregister_stats_vdev(&dev->stats_vdev);
>> +err_unreg_stream_vdev:
>> +	rkisp1_unregister_stream_vdevs(dev);
>> +err_unreg_isp_subdev:
>> +	rkisp1_unregister_isp_subdev(dev);
>> +	return ret;
>> +}
>> +
>> +static const char * const rk3399_isp_clks[] = {
>> +	"clk_isp",
>> +	"aclk_isp",
>> +	"hclk_isp",
>> +	"aclk_isp_wrap",
>> +	"hclk_isp_wrap",
>> +};
>> +
>> +static const char * const rk3288_isp_clks[] = {
>> +	"clk_isp",
>> +	"aclk_isp",
>> +	"hclk_isp",
>> +	"pclk_isp_in",
>> +	"sclk_isp_jpe",
>> +};
>> +
>> +static const struct isp_match_data rk3288_isp_clk_data = {
>> +	.clks = rk3288_isp_clks,
>> +	.size = ARRAY_SIZE(rk3288_isp_clks),
>> +};
>> +
>> +static const struct isp_match_data rk3399_isp_clk_data = {
>> +	.clks = rk3399_isp_clks,
>> +	.size = ARRAY_SIZE(rk3399_isp_clks),
>> +};
>> +
>> +static const struct of_device_id rkisp1_plat_of_match[] = {
>> +	{
>> +		.compatible = "rockchip,rk3288-cif-isp",
>> +		.data = &rk3288_isp_clk_data,
>> +	}, {
>> +		.compatible = "rockchip,rk3399-cif-isp",
>> +		.data = &rk3399_isp_clk_data,
>> +	},
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, rkisp1_plat_of_match);
>> +
>> +static irqreturn_t rkisp1_irq_handler(int irq, void *ctx)
>> +{
>> +	struct device *dev = ctx;
>> +	struct rkisp1_device *rkisp1_dev = dev_get_drvdata(dev);
>> +	unsigned int mis_val;
>> +
>> +	mis_val = readl(rkisp1_dev->base_addr + CIF_ISP_MIS);
>> +	if (mis_val)
>> +		rkisp1_isp_isr(mis_val, rkisp1_dev);
>> +
>> +	mis_val = readl(rkisp1_dev->base_addr + CIF_MIPI_MIS);
>> +	if (mis_val)
>> +		rkisp1_mipi_isr(mis_val, rkisp1_dev);
>> +
>> +	mis_val = readl(rkisp1_dev->base_addr + CIF_MI_MIS);
>> +	if (mis_val)
>> +		rkisp1_mi_isr(mis_val, rkisp1_dev);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int rkisp1_plat_probe(struct platform_device *pdev)
>> +{
>> +	struct device_node *node = pdev->dev.of_node;
>> +	const struct isp_match_data *clk_data;
>> +	const struct of_device_id *match;
>> +	struct device *dev = &pdev->dev;
>> +	struct rkisp1_device *isp_dev;
>> +	struct v4l2_device *v4l2_dev;
>> +	unsigned int i;
>> +	int ret, irq;
>> +
>> +	match = of_match_node(rkisp1_plat_of_match, node);
>> +	isp_dev = devm_kzalloc(dev, sizeof(*isp_dev), GFP_KERNEL);
>> +	if (!isp_dev)
>> +		return -ENOMEM;
>> +
>> +	INIT_LIST_HEAD(&isp_dev->sensors);
>> +
>> +	dev_set_drvdata(dev, isp_dev);
>> +	isp_dev->dev = dev;
>> +
>> +	isp_dev->base_addr = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(isp_dev->base_addr))
>> +		return PTR_ERR(isp_dev->base_addr);
>> +
>> +	irq = platform_get_irq(pdev, 0);
>> +	if (irq < 0)
>> +		return irq;
>> +
>> +	ret = devm_request_irq(dev, irq, rkisp1_irq_handler, IRQF_SHARED,
>> +			       dev_driver_string(dev), dev);
>> +	if (ret < 0) {
>> +		dev_err(dev, "request irq failed: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	isp_dev->irq = irq;
>> +	clk_data = match->data;
>> +
>> +	for (i = 0; i < clk_data->size; i++)
>> +		isp_dev->clks[i].id = clk_data->clks[i];
>> +	ret = devm_clk_bulk_get(dev, clk_data->size, isp_dev->clks);
>> +	if (ret)
>> +		return ret;
>> +	isp_dev->clk_size = clk_data->size;
>> +
>> +	atomic_set(&isp_dev->pipe.power_cnt, 0);
>> +	atomic_set(&isp_dev->pipe.stream_cnt, 0);
>> +	isp_dev->pipe.open = rkisp1_pipeline_open;
>> +	isp_dev->pipe.close = rkisp1_pipeline_close;
>> +	isp_dev->pipe.set_stream = rkisp1_pipeline_set_stream;
>> +
>> +	rkisp1_stream_init(isp_dev, RKISP1_STREAM_SP);
>> +	rkisp1_stream_init(isp_dev, RKISP1_STREAM_MP);
>> +
>> +	strscpy(isp_dev->media_dev.model, "rkisp1",
>> +		sizeof(isp_dev->media_dev.model));
>> +	isp_dev->media_dev.dev = &pdev->dev;
>> +	strscpy(isp_dev->media_dev.bus_info,
>> +		"platform: " DRIVER_NAME, sizeof(isp_dev->media_dev.bus_info));
>> +	media_device_init(&isp_dev->media_dev);
>> +
>> +	v4l2_dev = &isp_dev->v4l2_dev;
>> +	v4l2_dev->mdev = &isp_dev->media_dev;
>> +	strscpy(v4l2_dev->name, "rkisp1", sizeof(v4l2_dev->name));
>> +	v4l2_ctrl_handler_init(&isp_dev->ctrl_handler, 5);
>> +	v4l2_dev->ctrl_handler = &isp_dev->ctrl_handler;
>> +
>> +	ret = v4l2_device_register(isp_dev->dev, &isp_dev->v4l2_dev);
>> +	if (ret < 0)
> 
> Once you've initialised the control handler, you'll need to free it in case
> of an error. I.e. add one more label for that purpose near the end.

control handler is not required, I'll remove it for the next version.

> 
>> +		return ret;
>> +
>> +	ret = media_device_register(&isp_dev->media_dev);
>> +	if (ret < 0) {
>> +		v4l2_err(v4l2_dev, "Failed to register media device: %d\n",
>> +			 ret);
>> +		goto err_unreg_v4l2_dev;
>> +	}
>> +
>> +	/* create & register platefom subdev (from of_node) */
>> +	ret = rkisp1_register_platform_subdevs(isp_dev);
>> +	if (ret < 0)
>> +		goto err_unreg_media_dev;
>> +
>> +	pm_runtime_enable(&pdev->dev);
>> +
>> +	return 0;
>> +
>> +err_unreg_media_dev:
>> +	media_device_unregister(&isp_dev->media_dev);
>> +err_unreg_v4l2_dev:
>> +	v4l2_device_unregister(&isp_dev->v4l2_dev);
>> +	return ret;
>> +}
>> +
>> +static int rkisp1_plat_remove(struct platform_device *pdev)
>> +{
>> +	struct rkisp1_device *isp_dev = platform_get_drvdata(pdev);
>> +
>> +	pm_runtime_disable(&pdev->dev);
>> +	media_device_unregister(&isp_dev->media_dev);
>> +	v4l2_async_notifier_unregister(&isp_dev->notifier);
>> +	v4l2_async_notifier_cleanup(&isp_dev->notifier);
>> +	v4l2_device_unregister(&isp_dev->v4l2_dev);
>> +	rkisp1_unregister_params_vdev(&isp_dev->params_vdev);
>> +	rkisp1_unregister_stats_vdev(&isp_dev->stats_vdev);
>> +	rkisp1_unregister_stream_vdevs(isp_dev);
>> +	rkisp1_unregister_isp_subdev(isp_dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static int __maybe_unused rkisp1_runtime_suspend(struct device *dev)
>> +{
>> +	struct rkisp1_device *isp_dev = dev_get_drvdata(dev);
>> +
>> +	clk_bulk_disable_unprepare(isp_dev->clk_size, isp_dev->clks);
>> +	return pinctrl_pm_select_sleep_state(dev);
>> +}
>> +
>> +static int __maybe_unused rkisp1_runtime_resume(struct device *dev)
>> +{
>> +	struct rkisp1_device *isp_dev = dev_get_drvdata(dev);
>> +	int ret;
>> +
>> +	ret = pinctrl_pm_select_default_state(dev);
>> +	if (ret < 0)
>> +		return ret;
>> +	ret = clk_bulk_prepare_enable(isp_dev->clk_size, isp_dev->clks);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct dev_pm_ops rkisp1_plat_pm_ops = {
>> +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>> +				pm_runtime_force_resume)
>> +	SET_RUNTIME_PM_OPS(rkisp1_runtime_suspend, rkisp1_runtime_resume, NULL)
>> +};
>> +
>> +static struct platform_driver rkisp1_plat_drv = {
>> +	.driver = {
>> +		.name = DRIVER_NAME,
>> +		.of_match_table = of_match_ptr(rkisp1_plat_of_match),
>> +		.pm = &rkisp1_plat_pm_ops,
>> +	},
>> +	.probe = rkisp1_plat_probe,
>> +	.remove = rkisp1_plat_remove,
>> +};
>> +
>> +module_platform_driver(rkisp1_plat_drv);
>> +MODULE_AUTHOR("Rockchip Camera/ISP team");
>> +MODULE_DESCRIPTION("Rockchip ISP1 platform driver");
>> +MODULE_LICENSE("Dual BSD/GPL");
> 
> BSD or MIT?

Thanks for spotting this, I'll verify.

> 
>> diff --git a/drivers/media/platform/rockchip/isp1/dev.h b/drivers/media/platform/rockchip/isp1/dev.h
>> new file mode 100644
>> index 000000000000..f7cbee316523
>> --- /dev/null
>> +++ b/drivers/media/platform/rockchip/isp1/dev.h
>> @@ -0,0 +1,97 @@
>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
>> +/*
>> + * Rockchip isp1 driver
>> + *
>> + * Copyright (C) 2017 Rockchip Electronics Co., Ltd.
>> + */
>> +
>> +#ifndef _RKISP1_DEV_H
>> +#define _RKISP1_DEV_H
>> +
>> +#include <linux/clk.h>
>> +
>> +#include "capture.h"
>> +#include "rkisp1.h"
>> +#include "isp_params.h"
>> +#include "isp_stats.h"
>> +
>> +#define DRIVER_NAME "rkisp1"
>> +#define ISP_VDEV_NAME DRIVER_NAME  "_ispdev"
>> +#define SP_VDEV_NAME DRIVER_NAME   "_selfpath"
>> +#define MP_VDEV_NAME DRIVER_NAME   "_mainpath"
>> +#define DMA_VDEV_NAME DRIVER_NAME  "_dmapath"
>> +
>> +#define GRP_ID_SENSOR			BIT(0)
>> +#define GRP_ID_MIPIPHY			BIT(1)
>> +#define GRP_ID_ISP			BIT(2)
>> +#define GRP_ID_ISP_MP			BIT(3)
>> +#define GRP_ID_ISP_SP			BIT(4)
>> +
>> +#define RKISP1_MAX_BUS_CLK	8
>> +#define RKISP1_MAX_SENSOR	2
>> +#define RKISP1_MAX_PIPELINE	4
>> +
>> +/*
>> + * struct rkisp1_pipeline - An ISP hardware pipeline
>> + *
>> + * Capture device call other devices via pipeline
>> + *
>> + * @num_subdevs: number of linked subdevs
>> + * @power_cnt: pipeline power count
>> + * @stream_cnt: stream power count
>> + */
>> +struct rkisp1_pipeline {
>> +	struct media_pipeline pipe;
>> +	int num_subdevs;
>> +	atomic_t power_cnt;
>> +	atomic_t stream_cnt;
>> +	struct v4l2_subdev *subdevs[RKISP1_MAX_PIPELINE];
>> +	int (*open)(struct rkisp1_pipeline *p,
>> +		    struct media_entity *me, bool prepare);
>> +	int (*close)(struct rkisp1_pipeline *p);
>> +	int (*set_stream)(struct rkisp1_pipeline *p, bool on);
>> +};
>> +
>> +/*
>> + * struct rkisp1_sensor - Sensor information
>> + * @mbus: media bus configuration
>> + */
>> +struct rkisp1_sensor {
>> +	struct v4l2_subdev *sd;
>> +	struct v4l2_mbus_config mbus;
>> +	unsigned int lanes;
>> +	struct phy *dphy;
>> +	struct list_head list;
>> +};
> 
> You seem to also have struct sensor_async_subdev that appears to contain
> similar information. Would it be possible to unify the two?
> 
> This would appear to allow you getting rid of functions such as
> sd_to_sensor, for instance.

ack, I managed to get rid of this, and I don't even need to keep them
on a list, I'll submit in the next version.

Thanks a lot for your review
Helen

> 
>> +
>> +/*
>> + * struct rkisp1_device - ISP platform device
>> + * @base_addr: base register address
>> + * @active_sensor: sensor in-use, set when streaming on
>> + * @isp_sdev: ISP sub-device
>> + * @rkisp1_stream: capture video device
>> + * @stats_vdev: ISP statistics output device
>> + * @params_vdev: ISP input parameters device
>> + */
>> +struct rkisp1_device {
>> +	void __iomem *base_addr;
>> +	int irq;
>> +	struct device *dev;
>> +	unsigned int clk_size;
>> +	struct clk_bulk_data clks[RKISP1_MAX_BUS_CLK];
>> +	struct v4l2_device v4l2_dev;
>> +	struct v4l2_ctrl_handler ctrl_handler;
>> +	struct media_device media_dev;
>> +	struct v4l2_async_notifier notifier;
>> +	struct v4l2_subdev *subdevs[RKISP1_SD_MAX];
>> +	struct rkisp1_sensor *active_sensor;
>> +	struct list_head sensors;
>> +	struct rkisp1_isp_subdev isp_sdev;
>> +	struct rkisp1_stream stream[RKISP1_MAX_STREAM];
>> +	struct rkisp1_isp_stats_vdev stats_vdev;
>> +	struct rkisp1_isp_params_vdev params_vdev;
>> +	struct rkisp1_pipeline pipe;
>> +	struct vb2_alloc_ctx *alloc_ctx;
>> +};
>> +
>> +#endif
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel