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
