Re: [PATCH v8 2/3] media: rockchip: Add a driver for Rockhip's camera interface

[Paul Kocialkowski]

Hi,

On Mon 23 Oct 23, 15:28, Michael Riesch wrote:
> Typo in the subject: "Rockhip's" -> "Rockchip's"
> I think this typo has been in there for a while now ;-)

Great hips make for great dancing!

> On 10/16/23 11:00, Mehdi Djait wrote:
> > Introduce a driver for the camera interface on some Rockchip platforms.
> > 
> > This controller supports CSI2 and BT656 interfaces, but for
> > now only the BT656 interface could be tested, hence it's the only one
> > that's supported in the first version of this driver.
> 
> "CSI2" -> "MIPI CSI-2" ?
> "BT656" -> "BT.656" ?
> Also, additional interfaces are supported by some units, e.g., the
> RK3568 VICAP also supports BT.1120.
> 
> But most likely it becomes too complex to list everything, and it would
> be better if you simply described the unit in the PX30. I think this
> would clarify the commit message a lot.

For now I would just stick to mentionning parallel (aka DVP). Indeed we don't
need to list every possible parallel setup and MIPI CSI-2 is not supported
in the current version of the driver.

> > This controller can be fond on PX30, RK1808, RK3128 and RK3288,
> > but for now it's only been tested on PX30.
> > 
> > Most of this driver was written following the BSP driver from rockchip,
> 
> "rockchip" -> "Rockchip"
> 
> > removing the parts that either didn't fit correctly the guidelines, or
> > that couldn't be tested.
> > 
> > In the BSP, this driver is known as the "cif" driver, but this was
> > renamed to "vip" to better fit the controller denomination in the
> > datasheet.
> > 
> > This basic version doesn't support cropping nor scaling, and is only
> > designed with one SDTV video decoder being attached to it a any time.
> > 
> > This version uses the "pingpong" mode of the controller, which is a
> > double-buffering mechanism.
> > 
> > Signed-off-by: Mehdi Djait <mehdi.djait@xxxxxxxxxxx>
> 
> Two things below:
> 
> >[...]
> > diff --git a/drivers/media/platform/rockchip/vip/dev.h b/drivers/media/platform/rockchip/vip/dev.h
> > new file mode 100644
> > index 000000000000..eb9cd8f20ffc
> > --- /dev/null
> > +++ b/drivers/media/platform/rockchip/vip/dev.h
> > @@ -0,0 +1,163 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Rockchip VIP Driver
> > + *
> > + * Copyright (C) 2018 Rockchip Electronics Co., Ltd.
> > + * Copyright (C) 2023 Mehdi Djait <mehdi.djait@xxxxxxxxxxx>
> > + */
> > +
> > +#ifndef _RK_VIP_DEV_H
> > +#define _RK_VIP_DEV_H
> > +
> > +#include <linux/clk.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 VIP_DRIVER_NAME		"rk_vip"
> > +#define VIP_VIDEODEVICE_NAME	"stream_vip"
> > +
> > +#define RK_VIP_MAX_BUS_CLK	8
> > +#define RK_VIP_MAX_SENSOR	2
> > +#define RK_VIP_MAX_RESET	5
> > +#define RK_VIP_MAX_CSI_CHANNEL	4
> > +
> > +#define RK_VIP_DEFAULT_WIDTH	640
> > +#define RK_VIP_DEFAULT_HEIGHT	480
> > +
> > +#define write_vip_reg(base, addr, val)  writel(val, (addr) + (base))
> > +#define read_vip_reg(base, addr) readl((addr) + (base))
> 
> Please provide those helpers as proper inline functions. As to the
> naming, the "_reg" suffix seems unnecessary.
> 
> Alternatively, you could consider converting the driver to use regmap.

Come to think of it, I feel like it would make more sense to have an inline
function which is given a struct rk_vip_device instead of having to dereference
it every time in the caller to access the base address.

> > +
> > +enum rk_vip_state {
> > +	RK_VIP_STATE_DISABLED,
> > +	RK_VIP_STATE_READY,
> > +	RK_VIP_STATE_STREAMING
> > +};
> > +
> > +enum rk_vip_chip_id {
> > +	CHIP_PX30_VIP,
> > +	CHIP_RK1808_VIP,
> > +	CHIP_RK3128_VIP,
> > +	CHIP_RK3288_VIP
> > +};
> > +
> > +enum host_type_t {
> > +	RK_CSI_RXHOST,
> > +	RK_DSI_RXHOST
> > +};
> > +
> > +struct rk_vip_buffer {
> > +	struct vb2_v4l2_buffer vb;
> > +	struct list_head queue;
> > +	union {
> > +		u32 buff_addr[VIDEO_MAX_PLANES];
> > +		void *vaddr[VIDEO_MAX_PLANES];
> > +	};
> > +};
> > +
> > +struct rk_vip_scratch_buffer {
> > +	void *vaddr;
> > +	dma_addr_t dma_addr;
> > +	u32 size;
> > +};
> > +
> > +static inline struct rk_vip_buffer *to_rk_vip_buffer(struct vb2_v4l2_buffer *vb)
> > +{
> > +	return container_of(vb, struct rk_vip_buffer, vb);
> > +}
> > +
> > +struct rk_vip_sensor_info {
> > +	struct v4l2_subdev *sd;
> > +	int pad;
> > +	struct v4l2_mbus_config mbus;
> > +	int lanes;
> > +	v4l2_std_id std;
> > +};
> > +
> > +struct vip_output_fmt {
> > +	u32 fourcc;
> > +	u32 mbus;
> > +	u32 fmt_val;
> > +	u8 cplanes;
> > +};
> > +
> > +enum vip_fmt_type {
> > +	VIP_FMT_TYPE_YUV = 0,
> > +	VIP_FMT_TYPE_RAW,
> > +};
> > +
> > +struct vip_input_fmt {
> > +	u32 mbus_code;
> > +	u32 dvp_fmt_val;
> > +	u32 csi_fmt_val;
> > +	enum vip_fmt_type fmt_type;
> > +	enum v4l2_field field;
> > +};
> > +
> > +struct rk_vip_stream {
> > +	struct rk_vip_device		*vipdev;
> > +	enum rk_vip_state		state;
> > +	bool				stopping;
> > +	wait_queue_head_t		wq_stopped;
> > +	int				frame_idx;
> > +	int				frame_phase;
> > +
> > +	/* lock between irq and buf_queue */
> > +	spinlock_t			vbq_lock;
> > +	struct vb2_queue		buf_queue;
> > +	struct list_head		buf_head;
> > +	struct rk_vip_scratch_buffer	scratch_buf;
> > +	struct rk_vip_buffer		*buffs[2];
> > +
> > +	/* vfd lock */
> > +	struct mutex			vlock;
> > +	struct video_device		vdev;
> > +	struct media_pad		pad;
> > +
> > +	struct vip_output_fmt		*vip_fmt_out;
> > +	const struct vip_input_fmt	*vip_fmt_in;
> > +	struct v4l2_pix_format_mplane	pixm;
> > +};
> > +
> > +static inline struct rk_vip_stream *to_rk_vip_stream(struct video_device *vdev)
> > +{
> > +	return container_of(vdev, struct rk_vip_stream, vdev);
> > +}
> > +
> > +struct rk_vip_device {
> > +	struct list_head		list;
> > +	struct device			*dev;
> > +	int				irq;
> > +	void __iomem			*base_addr;
> > +	void __iomem			*csi_base;
> > +	struct clk_bulk_data		clks[RK_VIP_MAX_BUS_CLK];
> > +	int				num_clk;
> > +	struct vb2_alloc_ctx		*alloc_ctx;
> > +	bool				iommu_en;
> > +	struct iommu_domain		*domain;
> > +	struct reset_control		*vip_rst;
> > +
> > +	struct v4l2_device		v4l2_dev;
> > +	struct media_device		media_dev;
> > +	struct v4l2_ctrl_handler	ctrl_handler;
> > +	struct v4l2_async_notifier	notifier;
> > +	struct v4l2_async_connection	asd;
> > +	struct rk_vip_sensor_info	sensor;
> 
> Using "sensor" as name does not seem correct. As pointed out above it
> could be a video decoder just as well. Something with "subdevice" maybe?

Agreed. I suggest renaming the struct "rk_vip_sensor_info" -> "rk_cif_remote"
and just calling the member "remote".

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Attachment: signature.asc
Description: PGP signature