* Re: [RFC PATCH V2 4/4] platform: mtk-isp: Add Mediatek FD driver
From: Tomasz Figa @ 2019-08-02 8:28 UTC (permalink / raw)
To: Jerry-ch Chen
Cc: devicetree, Sean.Cheng, laurent.pinchart+renesas, Rynn.Wu,
christie.yu, srv_heupstream, po-yang.huang, suleiman, shik,
jungo.lin, sj.huang, yuzhao, hans.verkuil, zwisler, frederic.chen,
matthias.bgg, linux-mediatek, mchehab, linux-arm-kernel,
linux-media
In-Reply-To: <1562661672-22439-5-git-send-email-Jerry-Ch.chen@mediatek.com>
Hi Jerry,
On Tue, Jul 09, 2019 at 04:41:12PM +0800, Jerry-ch Chen wrote:
> From: Jerry-ch Chen <jerry-ch.chen@mediatek.com>
>
> This patch adds the driver of Face Detection (FD) unit in
> Mediatek camera system, providing face detection function.
>
> The mtk-isp directory will contain drivers for multiple IP
> blocks found in Mediatek ISP system. It will include ISP Pass 1
> driver (CAM), sensor interface driver, DIP driver and face
> detection driver.
>
> Signed-off-by: Jerry-ch Chen <jerry-ch.chen@mediatek.com>
> ---
> drivers/media/platform/Makefile | 2 +
> drivers/media/platform/mtk-isp/fd/Makefile | 5 +
> drivers/media/platform/mtk-isp/fd/mtk_fd.h | 157 +++
> drivers/media/platform/mtk-isp/fd/mtk_fd_40.c | 1259 +++++++++++++++++++++++++
> include/uapi/linux/v4l2-controls.h | 4 +
> 5 files changed, 1427 insertions(+)
> create mode 100644 drivers/media/platform/mtk-isp/fd/Makefile
> create mode 100644 drivers/media/platform/mtk-isp/fd/mtk_fd.h
> create mode 100644 drivers/media/platform/mtk-isp/fd/mtk_fd_40.c
>
Thanks for the patch! I finally got a chance to fully review the code. Sorry
for the delay. Please check my comments inline.
> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> index e6deb25..8b817cc 100644
> --- a/drivers/media/platform/Makefile
> +++ b/drivers/media/platform/Makefile
> @@ -94,6 +94,8 @@ obj-$(CONFIG_VIDEO_MEDIATEK_MDP) += mtk-mdp/
>
> obj-$(CONFIG_VIDEO_MEDIATEK_JPEG) += mtk-jpeg/
>
> +obj-$(CONFIG_VIDEO_MEDIATEK_FD) += mtk-isp/fd/
> +
> obj-$(CONFIG_VIDEO_QCOM_CAMSS) += qcom/camss/
>
> obj-$(CONFIG_VIDEO_QCOM_VENUS) += qcom/venus/
> diff --git a/drivers/media/platform/mtk-isp/fd/Makefile b/drivers/media/platform/mtk-isp/fd/Makefile
> new file mode 100644
> index 0000000..9b1c501
> --- /dev/null
> +++ b/drivers/media/platform/mtk-isp/fd/Makefile
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +mtk-fd-objs += mtk_fd_40.o
> +
> +obj-$(CONFIG_VIDEO_MEDIATEK_FD) += mtk-fd.o
> \ No newline at end of file
> diff --git a/drivers/media/platform/mtk-isp/fd/mtk_fd.h b/drivers/media/platform/mtk-isp/fd/mtk_fd.h
> new file mode 100644
> index 0000000..289999b
> --- /dev/null
> +++ b/drivers/media/platform/mtk-isp/fd/mtk_fd.h
> @@ -0,0 +1,157 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +//
> +// Copyright (c) 2018 MediaTek Inc.
> +
> +#ifndef __MTK_FD_HW_H__
> +#define __MTK_FD_HW_H__
> +
> +#include <linux/io.h>
> +#include <linux/types.h>
> +#include <linux/platform_device.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
> +#include <media/videobuf2-v4l2.h>
> +
> +#define MTK_FD_OUTPUT_MIN_WIDTH 26U
> +#define MTK_FD_OUTPUT_MIN_HEIGHT 26U
> +#define MTK_FD_OUTPUT_MAX_WIDTH 640U
> +#define MTK_FD_OUTPUT_MAX_HEIGHT 480U
> +
> +/* Control the user defined image widths and heights
> + * to be scaled and performed face detection in FD HW.
> + * MTK FD support up to 14 user defined image sizes to perform face detection.
> + */
> +#define V4L2_CID_MTK_FD_SCALE_IMG_WIDTH (V4L2_CID_USER_MTK_FD_BASE + 1)
> +#define V4L2_CID_MTK_FD_SCALE_IMG_HEIGHT (V4L2_CID_USER_MTK_FD_BASE + 2)
> +
> +/* Control the numbers of user defined image sizes.
> + * The default value is 0 which means user is not going
> + * to define the specific image sizes.
> + */
> +#define V4L2_CID_MTK_FD_SCALE_IMG_NUM (V4L2_CID_USER_MTK_FD_BASE + 3)
> +
> +/* Control the Face Pose to be detected.
> + * Here describe the value as following:
> + * {0, detect the front face with rotation from 0 to 270 degrees},
> + * {1, detect the front face with rotation from 0 to 240 and 300 degrees},
> + * {2, detect the front face with rotation from 0 to 240 and 330 degrees},
> + * {3, detect the front face with rotation from 0 to 240 and left side face}.
> + */
> +#define V4L2_CID_MTK_FD_DETECT_POSE (V4L2_CID_USER_MTK_FD_BASE + 4)
> +#define V4L2_CID_MTK_FD_DETECT_SPEEDUP (V4L2_CID_USER_MTK_FD_BASE + 5)
> +#define V4L2_CID_MTK_FD_EXTRA_MODEL (V4L2_CID_USER_MTK_FD_BASE + 6)
> +
> +/* We reserve 16 controls for this driver. */
> +#define V4L2_CID_MTK_FD_MAX 16
> +
> +#define ENABLE_FD 0x111
> +#define FD_HW_ENABLE 0x4
> +#define FD_INT_EN 0x15c
> +#define FD_INT 0x168
> +#define FD_RESULT 0x178
> +#define FD_IRQ_MASK 0x001
> +
> +#define RS_MAX_BUF_SIZE 2288788
> +#define FD_MAX_SPEEDUP 7
> +#define FD_MAX_POSE_VAL 0xfffffffffffffff
> +#define FD_DEF_POSE_VAL 0x3ff
> +#define MAX_FD_SEL_NUM 1026
> +
> +/* The max. number of face sizes could be detected, for feature scaling */
> +#define FACE_SIZE_NUM_MAX 14
> +/* FACE_SIZE_NUM_MAX + 1, first scale for input image W/H */
> +#define FD_SCALE_NUM 15
> +
> +enum fd_state {
> + FD_ENQ,
> + FD_CBD,
> +};
> +
> +enum fd_img_format {
> + FMT_VYUY = 2,
> + FMT_UYVY,
> + FMT_YVYU,
> + FMT_YUYV,
> + FMT_UNKNOWN
> +};
Please use #define macros for hardware/firmware interface definitions.
> +
> +struct fd_buffer {
> + __u32 scp_addr; /* used by SCP */
> + __u32 dma_addr; /* used by DMA HW */
> +} __packed;
> +
> +enum fd_scp_cmd {
> + FD_CMD_INIT,
> + FD_CMD_ENQUEUE,
> +};
Ditto.
> +
> +struct fd_user_output {
> + __u64 results[MAX_FD_SEL_NUM];
> + __u16 number;
> +};
> +
> +struct user_param {
> + u8 fd_pose;
> + u8 fd_speedup;
> + u8 fd_extra_model;
> + u8 scale_img_num;
> + u8 src_img_fmt;
> + __u16 scale_img_width[FD_SCALE_NUM];
> + __u16 scale_img_height[FD_SCALE_NUM];
> +} __packed;
> +
> +struct fd_hw_param {
> + struct fd_buffer src_img;
> + struct fd_buffer user_result;
> + struct user_param user_param;
> +} __packed;
> +
> +struct cmd_init_info {
> + struct fd_buffer fd_manager;
> + __u32 rs_dma_addr;
> +} __packed;
> +
> +struct ipi_message {
> + u8 cmd_id;
> + union {
> + struct cmd_init_info init_param;
> + struct fd_hw_param hw_param;
> + };
> +} __packed;
> +
> +struct mtk_fd_hw {
> + struct clk *fd_clk;
> + struct rproc *rproc_handle;
> + struct platform_device *scp_pdev;
> + struct fd_buffer scp_mem;
> + wait_queue_head_t wq;
> + void __iomem *fd_base;
> + atomic_t fd_user_cnt;
> + enum fd_state state;
> + u32 fd_irq_result;
> + /* Ensure only one job in hw */
> + struct mutex fd_hw_lock;
> +};
> +
> +struct mtk_fd_dev {
> + struct platform_device *pdev;
> + struct v4l2_device v4l2_dev;
> + struct v4l2_m2m_dev *m2m_dev;
> + struct media_device mdev;
> + struct video_device vfd;
> + struct mtk_fd_hw fd_hw;
Could we just put the contents of that struct directly here? That should
simplify dereference chains in the code.
> + /* Lock for V4L2 operations */
> + struct mutex vfd_lock;
> +};
> +
> +struct mtk_fd_ctx {
> + struct mtk_fd_dev *fd_dev;
> + struct device *dev;
> + struct v4l2_fh fh;
> + struct v4l2_ctrl_handler hdl;
> + struct v4l2_pix_format_mplane src_fmt;
> + struct v4l2_meta_format dst_fmt;
> + struct user_param user_param;
> +};
> +
> +#endif/*__MTK_FD_HW_H__*/
> diff --git a/drivers/media/platform/mtk-isp/fd/mtk_fd_40.c b/drivers/media/platform/mtk-isp/fd/mtk_fd_40.c
> new file mode 100644
> index 0000000..246d3aa
> --- /dev/null
> +++ b/drivers/media/platform/mtk-isp/fd/mtk_fd_40.c
> @@ -0,0 +1,1259 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright (c) 2018 MediaTek Inc.
> +
> +#include <linux/clk.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/interrupt.h>
> +#include <linux/jiffies.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_data/mtk_scp.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/remoteproc.h>
> +#include <media/videobuf2-dma-contig.h>
> +#include <media/videobuf2-v4l2.h>
> +#include <linux/wait.h>
> +#include <media/v4l2-ioctl.h>
> +#include <media/v4l2-event.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-mem2mem.h>
> +#include <media/videobuf2-core.h>
> +
> +#include "mtk_fd.h"
> +
> +static struct v4l2_meta_format fw_param_fmts[] = {
const?
Also, isn't this the buffer with the detected faces rather than params?
> + {
> + .dataformat = V4L2_META_FMT_MTISP_PARAMS,
This should use its own format.
> + .buffersize = 1024 * 30,
Please define a C struct for this buffer and use sizeof() here.
> + },
> +};
Actually, is there a reason to have this array at all if there is only 1
format supported? I think we could just put the right constants directly in
the code.
> +
> +static const struct v4l2_pix_format_mplane in_img_fmts[] = {
> + {
> + .width = MTK_FD_OUTPUT_MAX_WIDTH,
> + .height = MTK_FD_OUTPUT_MAX_HEIGHT,
> + .pixelformat = V4L2_PIX_FMT_VYUY,
> + .colorspace = V4L2_COLORSPACE_BT2020,
> + .field = V4L2_FIELD_NONE,
> + .num_planes = 1,
> + },
> + {
> + .width = MTK_FD_OUTPUT_MAX_WIDTH,
> + .height = MTK_FD_OUTPUT_MAX_HEIGHT,
> + .pixelformat = V4L2_PIX_FMT_YUYV,
> + .colorspace = V4L2_COLORSPACE_BT2020,
> + .field = V4L2_FIELD_NONE,
> + .num_planes = 1,
> + },
> + {
> + .width = MTK_FD_OUTPUT_MAX_WIDTH,
> + .height = MTK_FD_OUTPUT_MAX_HEIGHT,
> + .pixelformat = V4L2_PIX_FMT_YVYU,
> + .colorspace = V4L2_COLORSPACE_BT2020,
> + .field = V4L2_FIELD_NONE,
> + .num_planes = 1,
> + },
> + {
> + .width = MTK_FD_OUTPUT_MAX_WIDTH,
> + .height = MTK_FD_OUTPUT_MAX_HEIGHT,
> + .pixelformat = V4L2_PIX_FMT_UYVY,
> + .colorspace = V4L2_COLORSPACE_BT2020,
> + .field = V4L2_FIELD_NONE,
> + .num_planes = 1,
If all the formats have the same values for a field, there is no reason to
have the field initialized here. Just make initialize them to the constant
values inside TRY_FMT.
Hmm, are the interleaved YUV formats the only formats supported by this
hardware? That would be quite unfortunate given the formats supported by the
other IP blocks on this SoC use the more typical planar formats.
> + },
> +};
> +
> +#define NUM_FORMATS ARRAY_SIZE(in_img_fmts)
> +
> +static inline struct mtk_fd_dev *mtk_fd_hw_to_dev(struct mtk_fd_hw *fd_hw)
> +{
> + return container_of(fd_hw, struct mtk_fd_dev, fd_hw);
> +}
> +
> +static inline struct mtk_fd_ctx *fh_to_ctx(struct v4l2_fh *fh)
> +{
> + return container_of(fh, struct mtk_fd_ctx, fh);
> +}
> +
> +static int mtk_fd_load_scp(struct mtk_fd_hw *fd_hw)
> +{
> + struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> + struct device *dev = &fd_dev->pdev->dev;
> + phandle rproc_phandle;
> + int ret;
> +
> + /* init scp */
> + fd_hw->scp_pdev = scp_get_pdev(fd_dev->pdev);
> + if (!fd_hw->scp_pdev) {
> + dev_err(dev, "Failed to get scp device\n");
> + return -ENODEV;
> + }
> +
> + if (of_property_read_u32(fd_dev->pdev->dev.of_node, "mediatek,scp",
> + &rproc_phandle)) {
> + dev_err(dev, "Could not get scp device\n");
> + return -EINVAL;
> + }
> +
> + fd_hw->rproc_handle = rproc_get_by_phandle(rproc_phandle);
> + if (!fd_hw->rproc_handle) {
> + dev_err(dev, "Could not get FD's rproc_handle\n");
> + return -EINVAL;
> + }
> +
> + ret = rproc_boot(fd_hw->rproc_handle);
> + if (ret < 0) {
> + /**
> + * Return 0 if downloading firmware successfully,
> + * otherwise it is failed
> + */
> + dev_err(dev, "rproc_boot failed\n");
> + return -ENODEV;
> + }
> +
> + return ret;
> +}
> +
> +static dma_addr_t mtk_fd_hw_alloc_rs_dma_addr(struct mtk_fd_hw *fd_hw)
> +{
> + struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> + struct device *dev = &fd_dev->pdev->dev;
> + void *va;
> + dma_addr_t dma_handle;
> +
> + va = dma_alloc_coherent(dev, RS_MAX_BUF_SIZE, &dma_handle, GFP_KERNEL);
I see this allocated here, but I don't see this freed anywhere.
> + if (!va) {
> + dev_err(dev, "dma_alloc null va\n");
> + return -ENOMEM;
> + }
> + memset(va, 0, RS_MAX_BUF_SIZE);
> +
> + return dma_handle;
> +}
> +
> +static int mtk_fd_send_ipi_init(struct mtk_fd_hw *fd_hw)
> +{
> + struct ipi_message fd_init_msg;
> + dma_addr_t rs_dma_addr;
> +
> + fd_init_msg.cmd_id = FD_CMD_INIT;
> +
> + fd_init_msg.init_param.fd_manager.scp_addr = fd_hw->scp_mem.scp_addr;
> + fd_init_msg.init_param.fd_manager.dma_addr = fd_hw->scp_mem.dma_addr;
> +
> + rs_dma_addr = mtk_fd_hw_alloc_rs_dma_addr(fd_hw);
> + if (!rs_dma_addr)
> + return -ENOMEM;
> +
> + fd_init_msg.init_param.rs_dma_addr = rs_dma_addr;
> +
> + return scp_ipi_send(fd_hw->scp_pdev, SCP_IPI_FD_CMD, &fd_init_msg,
> + sizeof(fd_init_msg), 0);
> +}
> +
> +static int mtk_fd_hw_enable(struct mtk_fd_hw *fd_hw)
> +{
> + int ret;
> +
> + ret = mtk_fd_load_scp(fd_hw);
> + if (ret)
> + return ret;
> +
> + ret = mtk_fd_send_ipi_init(fd_hw);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int mtk_fd_hw_connect(struct mtk_fd_hw *fd_hw)
> +{
> + struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> + s32 usercount;
> +
> + mutex_lock(&fd_hw->fd_hw_lock);
First of all, we don't need a mutex here, because all the V4L2 ioctls are
already running with the video device mutex.
> + usercount = atomic_inc_return(&fd_hw->fd_user_cnt);
If we already use a mutex, there is no point in using atomic types.
> + if (usercount == 1) {
> + pm_runtime_get_sync(&fd_dev->pdev->dev);
> + if (mtk_fd_hw_enable(fd_hw)) {
> + pm_runtime_put_sync(&fd_dev->pdev->dev);
> + atomic_dec_return(&fd_hw->fd_user_cnt);
> + mutex_unlock(&fd_hw->fd_hw_lock);
> + return -EINVAL;
> + }
> + }
This is a simple mem-to-mem device, so there is no reason to keep it active
all the time it's streaming. Please just get the runtime PM counter when
queuing a job to the hardware and release it when the job finishes.
I guess we might still want to do the costly operations like rproc_boot()
when we start streaming, though.
> + mutex_unlock(&fd_hw->fd_hw_lock);
> +
> + return 0;
> +}
> +
> +static int mtk_fd_wait_irq(struct mtk_fd_hw *fd_hw)
> +{
> + int timeout;
> + struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> + struct device *dev = &fd_dev->pdev->dev;
> +
> + timeout = wait_event_interruptible_timeout
> + (fd_hw->wq, (fd_hw->fd_irq_result & FD_IRQ_MASK),
> + usecs_to_jiffies(1000000));
> + if (!timeout) {
> + dev_err(dev, "%s timeout, %d\n", __func__,
> + fd_hw->fd_irq_result);
> + return -EAGAIN;
> + } else if (!(fd_hw->fd_irq_result & FD_IRQ_MASK)) {
> + dev_err(dev, "%s No IRQ mask:0x%8x\n",
> + __func__, fd_hw->fd_irq_result);
> + return -EINVAL;
> + }
> + fd_hw->fd_irq_result = 0;
> +
> + return 0;
> +}
> +
> +static void mtk_fd_hw_disconnect(struct mtk_fd_hw *fd_hw)
> +{
> + struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> + s32 usercount;
> +
> + mutex_lock(&fd_hw->fd_hw_lock);
> + atomic_dec_return(&fd_hw->fd_user_cnt);
> + usercount = atomic_read(&fd_hw->fd_user_cnt);
> + if (usercount == 0) {
> + if (fd_hw->state == FD_ENQ)
> + mtk_fd_wait_irq(fd_hw);
This shouldn't be possible to happen as the queues should be already stopped
at this point.
> +
> + pm_runtime_put_sync(&fd_dev->pdev->dev);
Any reason to use pm_runtime_put_sync() over pm_runtime_put()?
> + rproc_shutdown(fd_hw->rproc_handle);
> + rproc_put(fd_hw->rproc_handle);
> + }
> + mutex_unlock(&fd_hw->fd_hw_lock);
> +}
> +
> +static void mtk_fd_hw_job_finish(struct mtk_fd_hw *fd_hw,
> + struct fd_hw_param *fd_param,
> + enum vb2_buffer_state vb_state)
> +{
> + struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> + struct mtk_fd_ctx *ctx;
> + struct device *dev = &fd_dev->pdev->dev;
> + struct vb2_buffer *src_vb, *dst_vb;
> + struct vb2_v4l2_buffer *src_vbuf = NULL, *dst_vbuf = NULL;
> +
> + ctx = v4l2_m2m_get_curr_priv(fd_dev->m2m_dev);
> + if (!ctx) {
> + dev_err(dev, "Instance released before end of transaction\n");
> + return;
> + }
This shouldn't be possible. I suspect you're seeing this because
.stop_streaming is not implemented correctly. See my comment there.
> +
> + src_vb = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> + if (WARN_ON(!src_vb))
> + return;
This shouldn't be possible.
> + src_vbuf = to_vb2_v4l2_buffer(src_vb);
> +
> + dst_vb = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> + if (WARN_ON(!dst_vb))
> + return;
Ditto.
> + dst_vbuf = to_vb2_v4l2_buffer(dst_vb);
> +
> + dst_vbuf->vb2_buf.timestamp = src_vbuf->vb2_buf.timestamp;
> + dst_vbuf->timecode = src_vbuf->timecode;
> + dst_vbuf->flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
> + dst_vbuf->flags |= src_vbuf->flags & V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
We should be able to just use v4l2_m2m_buf_copy_metadata().
> +
> + v4l2_m2m_buf_done(src_vbuf, vb_state);
> + v4l2_m2m_buf_done(dst_vbuf, vb_state);
> + v4l2_m2m_job_finish(fd_dev->m2m_dev, ctx->fh.m2m_ctx);
> +}
> +
> +static int mtk_fd_hw_job_exec(struct mtk_fd_hw *fd_hw,
> + struct fd_hw_param *fd_param,
> + void *output_vaddr)
> +{
> + struct fd_user_output *fd_output;
> + struct ipi_message fd_ipi_msg;
> + int ret;
> + u32 num;
> +
> + if (fd_param->user_param.src_img_fmt == FMT_UNKNOWN)
> + goto param_err;
Is this possible?
> +
> + mutex_lock(&fd_hw->fd_hw_lock);
> + fd_hw->state = FD_ENQ;
What is this state for?
> + fd_ipi_msg.cmd_id = FD_CMD_ENQUEUE;
> + memcpy(&fd_ipi_msg.hw_param, fd_param, sizeof(fd_ipi_msg.hw_param));
> + ret = scp_ipi_send(fd_hw->scp_pdev, SCP_IPI_FD_CMD, &fd_ipi_msg,
> + sizeof(fd_ipi_msg), 0);
> + if (ret)
> + goto buf_err;
> +
> + ret = mtk_fd_wait_irq(fd_hw);
> + if (ret)
> + goto buf_err;
This function is called from device_run and it shouldn't be synchronous. It
should only queue the job to the hardware to be handled asynchronously when
it finishes.
> +
> + num = readl(fd_hw->fd_base + FD_RESULT);
> + /* Disable FD ISR */
> + writel(0x0, fd_hw->fd_base + FD_INT_EN);
> +
> + fd_output = (struct fd_user_output *)output_vaddr;
> + fd_output->number = num;
> + fd_hw->state = FD_CBD;
> + mutex_unlock(&fd_hw->fd_hw_lock);
> +
> + mtk_fd_hw_job_finish(fd_hw, fd_param, VB2_BUF_STATE_DONE);
> + return 0;
> +
> +buf_err:
> + mutex_unlock(&fd_hw->fd_hw_lock);
> +param_err:
> + mtk_fd_hw_job_finish(fd_hw, fd_param, VB2_BUF_STATE_ERROR);
> + return ret;
> +}
> +
> +static int mtk_fd_vb2_buf_out_validate(struct vb2_buffer *vb)
> +{
> + struct vb2_v4l2_buffer *v4l2_buf = to_vb2_v4l2_buffer(vb);
> +
> + v4l2_buf->field = V4L2_FIELD_NONE;
We need to fail with -EINVAL if v4l2_buf->field was different than
V4L2_FIELD_ANY or _NONE.
> +
> + return 0;
> +}
> +
> +static int mtk_fd_vb2_buf_prepare(struct vb2_buffer *vb)
> +{
> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> + struct vb2_queue *vq = vb->vb2_queue;
> + struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
> + struct device *dev = ctx->dev;
> + struct v4l2_pix_format_mplane *pixfmt;
> +
> + switch (vq->type) {
> + case V4L2_BUF_TYPE_META_CAPTURE:
> + if (vb2_plane_size(vb, 0) < ctx->dst_fmt.buffersize) {
> + dev_err(dev, "meta size %d is too small\n");
Please make this dev_dbg(), because userspace misbehavior must not trigger
kernel error logs.
> + return -EINVAL;
> + }
> + break;
> + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> + pixfmt = &ctx->src_fmt;
> +
> + if (vbuf->field == V4L2_FIELD_ANY)
> + vbuf->field = V4L2_FIELD_NONE;
> +
> + if (vb->num_planes != 1 || vbuf->field != V4L2_FIELD_NONE) {
> + dev_err(dev, "plane or field %d not supported\n",
> + vb->num_planes, vbuf->field);
Ditto.
> + return -EINVAL;
> + }
> + if (vb2_plane_size(vb, 0) < pixfmt->plane_fmt[0].sizeimage) {
> + dev_err(dev, "plane %d is too small\n");
Ditto.
> + return -EINVAL;
> + }
> + break;
> + default:
> + dev_err(dev, "invalid queue type: %d\n", vq->type);
> + return -EINVAL;
We shouldn't need to handle this.
> + }
> +
> + return 0;
> +}
> +
> +static void mtk_fd_vb2_buf_queue(struct vb2_buffer *vb)
> +{
> + struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> +
> + v4l2_m2m_buf_queue(ctx->fh.m2m_ctx, vbuf);
> +}
> +
> +static int mtk_fd_vb2_queue_setup(struct vb2_queue *vq,
> + unsigned int *num_buffers,
> + unsigned int *num_planes,
> + unsigned int sizes[],
> + struct device *alloc_devs[])
> +{
> + struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
> + struct device *dev = ctx->dev;
> + unsigned int size;
> +
> + switch (vq->type) {
> + case V4L2_BUF_TYPE_META_CAPTURE:
> + size = ctx->dst_fmt.buffersize;
> + break;
> + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> + size = ctx->src_fmt.plane_fmt[0].sizeimage;
> + break;
> + default:
> + dev_err(dev, "invalid queue type: %d\n", vq->type);
We should need to handle this.
> + return -EINVAL;
> + }
> +
> + if (!*num_planes) {
> + *num_planes = 1;
> + sizes[0] = size;
> + }
We need to handle the case when *num_planes is non-zero. We then need to
check if it's 1 and *sizes >= size and fail with -EINVAL if not.
> +
> + return 0;
> +}
> +
> +static int mtk_fd_vb2_start_streaming(struct vb2_queue *vq, unsigned int count)
> +{
> + struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
> +
> + return mtk_fd_hw_connect(&ctx->fd_dev->fd_hw);
This would be called twice for every context, once for the VIDEO_OUTPUT
queue and once for the META_CAPTURE queue. Perhaps it would make sense to
just do this for the VIDEO_OUTPUT queue?
> +}
> +
> +static void mtk_fd_vb2_stop_streaming(struct vb2_queue *vq)
> +{
> + struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
> + struct vb2_buffer *vb;
How do we guarantee here that the hardware isn't still accessing the buffers
removed below?
> +
> + if (V4L2_TYPE_IS_OUTPUT(vq->type))
> + vb = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> + else
> + vb = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> +
> + while (vb) {
> + v4l2_m2m_buf_done(to_vb2_v4l2_buffer(vb), VB2_BUF_STATE_ERROR);
> + if (V4L2_TYPE_IS_OUTPUT(vq->type))
> + vb = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> + else
> + vb = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> + }
We can use v4l2_m2m_buf_remove(). Also we can move the call into the loop
condition:
while ((vb == v4l2_m2m_buf_remove(...)))
v4l2_m2m_buf_done(...);
> +
> + mtk_fd_hw_disconnect(&ctx->fd_dev->fd_hw);
This should also probably be done only for 1 queue. Since the VIDEO_OUTPUT
queue supports requestes, I'd consider it the master one.
> +}
> +
> +static void mtk_fd_vb2_request_complete(struct vb2_buffer *vb)
> +{
> + struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
> +
> + v4l2_ctrl_request_complete(vb->req_obj.req, &ctx->hdl);
> +}
> +
> +static void mtk_fd_fill_pixfmt_mp(struct v4l2_pix_format_mplane *dfmt,
> + const struct v4l2_pix_format_mplane *sfmt)
> +{
> + dfmt->width = sfmt->width;
> + dfmt->height = sfmt->height;
> + dfmt->pixelformat = sfmt->pixelformat;
> + dfmt->field = sfmt->field;
> + dfmt->colorspace = sfmt->colorspace;
> + dfmt->num_planes = sfmt->num_planes;
> +
> + /* Use default */
> + dfmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> + dfmt->quantization = V4L2_QUANTIZATION_DEFAULT;
> + dfmt->xfer_func =
> + V4L2_MAP_XFER_FUNC_DEFAULT(dfmt->colorspace);
> + dfmt->plane_fmt[0].bytesperline = dfmt->width * 2;
> + dfmt->plane_fmt[0].sizeimage =
> + dfmt->height * dfmt->plane_fmt[0].bytesperline;
> + memset(dfmt->reserved, 0, sizeof(dfmt->reserved));
> +}
Could we unify this function with mtk_fd_m2m_try_fmt_out_mp()? That function
should be almost directly reusable for the default format initialization +/-
the arguments passed to it.
> +
> +static const struct v4l2_pix_format_mplane *mtk_fd_find_fmt(u32 format)
> +{
> + unsigned int i;
> + const struct v4l2_pix_format_mplane *dev_fmt;
> +
> + for (i = 0; i < NUM_FORMATS; i++) {
> + dev_fmt = &in_img_fmts[i];
> + if (dev_fmt->pixelformat == format)
> + return dev_fmt;
> + }
> +
> + return NULL;
> +}
> +
> +static int mtk_fd_m2m_querycap(struct file *file, void *fh,
> + struct v4l2_capability *cap)
> +{
> + struct mtk_fd_dev *fd_dev = video_drvdata(file);
> + struct device *dev = &fd_dev->pdev->dev;
> +
> + strscpy(cap->driver, dev->driver->name, sizeof(cap->driver));
Please use dev_driver_string().
> + strscpy(cap->card, fd_dev->vfd.name, sizeof(cap->card));
I think we can also make this dev_driver_string().
> + snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s",
> + dev_name(&fd_dev->pdev->dev));
> +
> + return 0;
> +}
> +
> +static int mtk_fd_m2m_enum_fmt_out_mp(struct file *file, void *fh,
> + struct v4l2_fmtdesc *f)
> +{
> + int i;
> +
> + for (i = 0; i < NUM_FORMATS; ++i) {
> + if (i == f->index) {
> + f->pixelformat = in_img_fmts[i].pixelformat;
> + return 0;
> + }
> + }
Why don't we just check if f->index is within the [0, ARRAY_SIZE()-1] bounds
and then just use it to index the array directly?
> +
> + return -EINVAL;
> +}
> +
> +static int mtk_fd_m2m_try_fmt_out_mp(struct file *file,
> + void *fh,
> + struct v4l2_format *f)
I think we could just shorten the function prefixes to "mtk_fd_".
> +{
> + struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
> + const struct v4l2_pix_format_mplane *fmt;
> +
> + fmt = mtk_fd_find_fmt(pix_mp->pixelformat);
> + if (!fmt) {
> + /* Get default img fmt */
> + fmt = &in_img_fmts[0];
> + f->fmt.pix.pixelformat = fmt->pixelformat;
> + }
> +
> + /* Use default */
> + pix_mp->field = fmt->field;
> + pix_mp->colorspace = fmt->colorspace;
> + pix_mp->num_planes = fmt->num_planes;
> + pix_mp->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> + pix_mp->quantization = V4L2_QUANTIZATION_DEFAULT;
> + pix_mp->xfer_func =
> + V4L2_MAP_XFER_FUNC_DEFAULT(pix_mp->colorspace);
> +
> + /* Keep user setting as possible */
> + pix_mp->width = clamp(pix_mp->width,
> + MTK_FD_OUTPUT_MIN_WIDTH,
> + MTK_FD_OUTPUT_MAX_WIDTH);
> + pix_mp->height = clamp(pix_mp->height,
> + MTK_FD_OUTPUT_MIN_HEIGHT,
> + MTK_FD_OUTPUT_MAX_HEIGHT);
> +
> + pix_mp->plane_fmt[0].bytesperline = pix_mp->width * 2;
Please add a comment explaining why this is always 2.
> + pix_mp->plane_fmt[0].sizeimage =
> + pix_mp->plane_fmt[0].bytesperline * pix_mp->height;
> + memset(pix_mp->plane_fmt[0].reserved, 0,
> + sizeof(pix_mp->plane_fmt[0].reserved));
No need to zero this here. The core will handle it.
> +
> + return 0;
> +}
> +
> +static int mtk_fd_m2m_g_fmt_out_mp(struct file *file, void *fh,
> + struct v4l2_format *f)
> +{
> + struct mtk_fd_ctx *ctx = fh_to_ctx(fh);
> +
> + f->fmt.pix_mp = ctx->src_fmt;
> +
> + return 0;
> +}
> +
> +static int mtk_fd_m2m_s_fmt_out_mp(struct file *file, void *fh,
> + struct v4l2_format *f)
> +{
> + struct mtk_fd_ctx *ctx = fh_to_ctx(fh);
> + struct vb2_queue *vq;
> +
> + /* Change not allowed if queue is streaming. */
> + vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
> + if (vb2_is_streaming(vq) || vb2_is_busy(vq)) {
vb2_is_busy() is a superset of vb2_is_streaming(), so it's enough to just
check the former.
> + dev_dbg(ctx->dev, "vb2_is_streaming or vb2_is_busy");
> + return -EBUSY;
> + }
> +
> + mtk_fd_m2m_try_fmt_out_mp(file, fh, f);
> + ctx->src_fmt = f->fmt.pix_mp;
> +
> + return 0;
> +}
> +
> +static int mtk_fd_m2m_enum_fmt_meta_cap(struct file *file, void *fh,
> + struct v4l2_fmtdesc *f)
> +{
> + if (f->index)
> + return -EINVAL;
> +
> + strscpy(f->description, "Face detection result",
> + sizeof(f->description));
> + f->pixelformat = fw_param_fmts[0].dataformat;
> + f->flags = 0;
> +
> + return 0;
> +}
> +
> +static int mtk_fd_m2m_g_fmt_meta_cap(struct file *file, void *fh,
> + struct v4l2_format *f)
> +{
> + f->fmt.meta.dataformat = fw_param_fmts[0].dataformat;
> + f->fmt.meta.buffersize = fw_param_fmts[0].buffersize;
> +
> + return 0;
> +}
> +
> +static const struct vb2_ops mtk_fd_vb2_ops = {
> + .queue_setup = mtk_fd_vb2_queue_setup,
> + .buf_out_validate = mtk_fd_vb2_buf_out_validate,
> + .buf_prepare = mtk_fd_vb2_buf_prepare,
> + .buf_queue = mtk_fd_vb2_buf_queue,
> + .start_streaming = mtk_fd_vb2_start_streaming,
> + .stop_streaming = mtk_fd_vb2_stop_streaming,
> + .wait_prepare = vb2_ops_wait_prepare,
> + .wait_finish = vb2_ops_wait_finish,
> + .buf_request_complete = mtk_fd_vb2_request_complete,
> +};
> +
> +static const struct v4l2_ioctl_ops mtk_fd_v4l2_video_out_ioctl_ops = {
> + .vidioc_querycap = mtk_fd_m2m_querycap,
> + .vidioc_enum_fmt_vid_out_mplane = mtk_fd_m2m_enum_fmt_out_mp,
> + .vidioc_g_fmt_vid_out_mplane = mtk_fd_m2m_g_fmt_out_mp,
> + .vidioc_s_fmt_vid_out_mplane = mtk_fd_m2m_s_fmt_out_mp,
> + .vidioc_try_fmt_vid_out_mplane = mtk_fd_m2m_try_fmt_out_mp,
> + .vidioc_enum_fmt_meta_cap = mtk_fd_m2m_enum_fmt_meta_cap,
> + .vidioc_g_fmt_meta_cap = mtk_fd_m2m_g_fmt_meta_cap,
> + .vidioc_s_fmt_meta_cap = mtk_fd_m2m_g_fmt_meta_cap,
> + .vidioc_try_fmt_meta_cap = mtk_fd_m2m_g_fmt_meta_cap,
> + .vidioc_reqbufs = v4l2_m2m_ioctl_reqbufs,
> + .vidioc_create_bufs = v4l2_m2m_ioctl_create_bufs,
> + .vidioc_expbuf = v4l2_m2m_ioctl_expbuf,
> + .vidioc_prepare_buf = v4l2_m2m_ioctl_prepare_buf,
> + .vidioc_querybuf = v4l2_m2m_ioctl_querybuf,
> + .vidioc_qbuf = v4l2_m2m_ioctl_qbuf,
> + .vidioc_dqbuf = v4l2_m2m_ioctl_dqbuf,
> + .vidioc_streamon = v4l2_m2m_ioctl_streamon,
> + .vidioc_streamoff = v4l2_m2m_ioctl_streamoff,
> + .vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
> + .vidioc_unsubscribe_event = v4l2_event_unsubscribe,
> +};
> +
> +static int
> +mtk_fd_queue_init(void *priv, struct vb2_queue *src_vq,
> + struct vb2_queue *dst_vq)
> +{
> + struct mtk_fd_ctx *ctx = priv;
> + int ret;
> +
> + src_vq->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> + src_vq->io_modes = VB2_MMAP | VB2_DMABUF;
> + src_vq->supports_requests = true;
> + src_vq->drv_priv = ctx;
> + src_vq->ops = &mtk_fd_vb2_ops;
> + src_vq->mem_ops = &vb2_dma_contig_memops;
> + src_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
> + src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> + src_vq->lock = &ctx->fd_dev->vfd_lock;
> + src_vq->dev = ctx->fd_dev->v4l2_dev.dev;
> +
> + ret = vb2_queue_init(src_vq);
> + if (ret)
> + return ret;
> +
> + dst_vq->type = V4L2_BUF_TYPE_META_CAPTURE;
> + dst_vq->io_modes = VB2_MMAP | VB2_DMABUF;
> + dst_vq->drv_priv = ctx;
> + dst_vq->ops = &mtk_fd_vb2_ops;
> + dst_vq->mem_ops = &vb2_dma_contig_memops;
> + dst_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
> + dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> + dst_vq->lock = &ctx->fd_dev->vfd_lock;
> + dst_vq->dev = ctx->fd_dev->v4l2_dev.dev;
> +
> + return vb2_queue_init(dst_vq);
> +}
> +
> +static int mtk_fd_dev_g_ctrl(struct v4l2_ctrl *ctrl)
> +{
> + struct mtk_fd_ctx *ctx = ctrl->priv;
> + int i;
> +
> + switch (ctrl->id) {
> + case V4L2_CID_MTK_FD_SCALE_IMG_WIDTH:
> + for (i = 0; i < ctrl->elems; i++)
> + ctrl->p_new.p_u16[i] =
> + ctx->user_param.scale_img_width[i];
> + break;
> + case V4L2_CID_MTK_FD_SCALE_IMG_HEIGHT:
> + for (i = 0; i < ctrl->elems; i++)
> + ctrl->p_new.p_u16[i] =
> + ctx->user_param.scale_img_height[i];
> + break;
> + case V4L2_CID_MTK_FD_DETECT_POSE:
> + ctrl->val = ctx->user_param.fd_pose;
> + break;
> + case V4L2_CID_MTK_FD_DETECT_SPEEDUP:
> + ctrl->val = ctx->user_param.fd_speedup;
> + break;
> + case V4L2_CID_MTK_FD_SCALE_IMG_NUM:
> + ctrl->val = ctx->user_param.scale_img_num;
> + break;
> + case V4L2_CID_MTK_FD_EXTRA_MODEL:
> + ctrl->val = ctx->user_param.fd_extra_model;
> + break;
> + default:
> + dev_dbg(ctx->dev, "%s: unexpected control: 0x%x:%d",
> + __func__, ctrl->id, ctrl->val);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int mtk_fd_dev_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> + struct mtk_fd_ctx *ctx = ctrl->priv;
> + int i;
> +
> + switch (ctrl->id) {
> + case V4L2_CID_MTK_FD_SCALE_IMG_WIDTH:
> + for (i = 0; i < ctrl->elems; i++)
> + ctx->user_param.scale_img_width[i] =
> + ctrl->p_new.p_u16[i];
> + break;
> + case V4L2_CID_MTK_FD_SCALE_IMG_HEIGHT:
> + for (i = 0; i < ctrl->elems; i++)
> + ctx->user_param.scale_img_height[i] =
> + ctrl->p_new.p_u16[i];
> + break;
> + case V4L2_CID_MTK_FD_DETECT_POSE:
> + ctx->user_param.fd_pose = ctrl->val;
> + break;
> + case V4L2_CID_MTK_FD_DETECT_SPEEDUP:
> + ctx->user_param.fd_speedup = ctrl->val;
> + break;
> + case V4L2_CID_MTK_FD_SCALE_IMG_NUM:
> + ctx->user_param.scale_img_num = ctrl->val;
> + break;
> + case V4L2_CID_MTK_FD_EXTRA_MODEL:
> + ctx->user_param.fd_extra_model = ctrl->val;
> + break;
> + default:
> + dev_dbg(ctx->dev, "%s: unexpected control: 0x%x:%d",
> + __func__, ctrl->id, ctrl->val);
> + return -EINVAL;
No need to handle this, as the framework will only pass the controls we
registered earlier to this function.
> + }
> +
> + return 0;
> +}
> +
> +static const struct v4l2_ctrl_ops mtk_fd_dev_ctrl_ops = {
> + .g_volatile_ctrl = mtk_fd_dev_g_ctrl,
I don't see any volatile controls below. No need to implement this callback.
> + .s_ctrl = mtk_fd_dev_s_ctrl,
I think you might not even need to implement this function (or just provide
a dummy one that returns 0 if its required) if you just use the controls
directly when preparing the job for the hardware. Check v4l2_ctrl_find().
> +};
> +
> +struct v4l2_ctrl_config mtk_fd_controls[] = {
> + {
> + .ops = &mtk_fd_dev_ctrl_ops,
> + .id = V4L2_CID_MTK_FD_SCALE_IMG_WIDTH,
> + .name = "FD scale image widths",
> + .type = V4L2_CTRL_TYPE_U16,
> + .min = MTK_FD_OUTPUT_MIN_WIDTH,
> + .max = MTK_FD_OUTPUT_MAX_WIDTH,
> + .step = 1,
> + .def = MTK_FD_OUTPUT_MAX_WIDTH,
> + .dims = { FD_SCALE_NUM },
Something wrong with indentation here.
> + },
> + {
> + .ops = &mtk_fd_dev_ctrl_ops,
> + .id = V4L2_CID_MTK_FD_SCALE_IMG_HEIGHT,
> + .name = "FD scale image heights",
> + .type = V4L2_CTRL_TYPE_U16,
> + .min = MTK_FD_OUTPUT_MIN_HEIGHT,
> + .max = MTK_FD_OUTPUT_MAX_HEIGHT,
> + .step = 1,
> + .def = MTK_FD_OUTPUT_MAX_HEIGHT,
> + .dims = { FD_SCALE_NUM },
> + },
> + {
> + .ops = &mtk_fd_dev_ctrl_ops,
> + .id = V4L2_CID_MTK_FD_SCALE_IMG_NUM,
> + .name = "FD scale size counts",
> + .type = V4L2_CTRL_TYPE_INTEGER,
> + .min = 0,
> + .max = FACE_SIZE_NUM_MAX,
> + .step = 1,
> + .def = 0,
> + },
> + {
> + .ops = &mtk_fd_dev_ctrl_ops,
> + .id = V4L2_CID_MTK_FD_DETECT_POSE,
> + .name = "FD detect face pose",
> + .type = V4L2_CTRL_TYPE_INTEGER,
> + .min = 0,
> + .max = 3,
> + .step = 1,
> + .def = 0,
> + },
> + {
> + .ops = &mtk_fd_dev_ctrl_ops,
> + .id = V4L2_CID_MTK_FD_DETECT_SPEEDUP,
> + .name = "FD detection speedup",
> + .type = V4L2_CTRL_TYPE_INTEGER,
> + .min = 0,
> + .max = FD_MAX_SPEEDUP,
> + .step = 1,
> + .def = 0,
> + },
> + {
> + .ops = &mtk_fd_dev_ctrl_ops,
> + .id = V4L2_CID_MTK_FD_EXTRA_MODEL,
> + .name = "FD use extra model",
> + .type = V4L2_CTRL_TYPE_BOOLEAN,
> + .min = 0,
> + .max = 1,
> + .step = 1,
> + .def = 0,
> + },
> +};
> +
> +static int mtk_fd_ctrls_setup(struct mtk_fd_ctx *ctx)
> +{
> + struct v4l2_ctrl_handler *hdl = &ctx->hdl;
> + struct v4l2_ctrl *ctl;
> + int i;
> +
> + v4l2_ctrl_handler_init(hdl, V4L2_CID_MTK_FD_MAX);
> + if (hdl->error)
> + return hdl->error;
> +
> + for (i = 0; i < ARRAY_SIZE(mtk_fd_controls); i++) {
> + ctl = v4l2_ctrl_new_custom(hdl, &mtk_fd_controls[i], ctx);
> + if (hdl->error) {
> + v4l2_ctrl_handler_free(hdl);
> + dev_err(ctx->dev, "Failed to register controls:%d", i);
> + return hdl->error;
> + }
> + }
> +
> + ctx->fh.ctrl_handler = &ctx->hdl;
> + v4l2_ctrl_handler_setup(hdl);
> +
> + return 0;
> +}
> +
> +static unsigned int get_fd_img_fmt(unsigned int fourcc)
> +{
> + switch (fourcc) {
> + case V4L2_PIX_FMT_VYUY:
> + return FMT_VYUY;
> + case V4L2_PIX_FMT_YUYV:
> + return FMT_YUYV;
> + case V4L2_PIX_FMT_YVYU:
> + return FMT_YVYU;
> + case V4L2_PIX_FMT_UYVY:
> + return FMT_UYVY;
> + default:
> + return FMT_UNKNOWN;
> + }
> +}
> +
> +static void init_ctx_fmt(struct mtk_fd_ctx *ctx)
> +{
> + const struct v4l2_pix_format_mplane *fmt;
> +
> + /* Initialize M2M source fmt */
> + fmt = &in_img_fmts[0];
> + mtk_fd_fill_pixfmt_mp(&ctx->src_fmt, fmt);
> +
> + /* Initialize M2M destination fmt */
> + ctx->dst_fmt.buffersize = fw_param_fmts[0].buffersize;
> + ctx->dst_fmt.dataformat = fw_param_fmts[0].dataformat;
Why not just make dst_fmt a pointer and assign &fw_params_fmt[0] there?
> +}
> +
> +/*
> + * V4L2 file operations.
> + */
> +static int mtk_vfd_open(struct file *filp)
> +{
> + struct mtk_fd_dev *fd_dev = video_drvdata(filp);
> + struct video_device *vdev = video_devdata(filp);
> + struct mtk_fd_ctx *ctx;
> + int ret;
> +
> + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> + if (!ctx)
> + return -ENOMEM;
> +
> + ctx->fd_dev = fd_dev;
> + ctx->dev = &fd_dev->pdev->dev;
> +
> + v4l2_fh_init(&ctx->fh, vdev);
> + filp->private_data = &ctx->fh;
> +
> + init_ctx_fmt(ctx);
> +
> + ret = mtk_fd_ctrls_setup(ctx);
> + if (ret) {
> + dev_err(ctx->dev, "Failed to set up controls:%d\n", ret);
> + goto err_fh_ctrl;
> + }
> +
> + ctx->fh.m2m_ctx = v4l2_m2m_ctx_init(fd_dev->m2m_dev, ctx,
> + &mtk_fd_queue_init);
> + if (IS_ERR(ctx->fh.m2m_ctx)) {
> + ret = PTR_ERR(ctx->fh.m2m_ctx);
> + goto err_init_ctx;
> + }
> +
> + v4l2_fh_add(&ctx->fh);
> +
> + return 0;
> +
> +err_init_ctx:
> + v4l2_ctrl_handler_free(&ctx->hdl);
> +err_fh_ctrl:
> + v4l2_fh_exit(&ctx->fh);
> + kfree(ctx);
> +
> + return ret;
> +}
> +
> +static int mtk_vfd_release(struct file *filp)
> +{
> + struct mtk_fd_ctx *ctx = container_of(filp->private_data,
> + struct mtk_fd_ctx, fh);
> +
> + v4l2_m2m_ctx_release(ctx->fh.m2m_ctx);
> +
> + v4l2_ctrl_handler_free(&ctx->hdl);
> + v4l2_fh_del(&ctx->fh);
> + v4l2_fh_exit(&ctx->fh);
> +
> + kfree(ctx);
> +
> + return 0;
> +}
> +
> +static const struct v4l2_file_operations fd_video_fops = {
> + .owner = THIS_MODULE,
> + .open = mtk_vfd_open,
> + .release = mtk_vfd_release,
> + .poll = v4l2_m2m_fop_poll,
> + .unlocked_ioctl = video_ioctl2,
> + .mmap = v4l2_m2m_fop_mmap,
We also need the compat_ioctl here for 32-bit userspace on 64-bit kernels.
> +};
> +
> +static void mtk_fd_device_run(void *priv)
> +{
> + struct mtk_fd_ctx *ctx = priv;
> + struct vb2_v4l2_buffer *src_buf, *dst_buf;
> + struct fd_hw_param fd_param;
> + void *fd_result_vaddr;
> +
> + src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> + dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
> +
> + memset(&fd_param, 0, sizeof(fd_param));
> +
> + fd_param.src_img.dma_addr =
> + vb2_dma_contig_plane_dma_addr(&src_buf->vb2_buf, 0);
> + fd_param.user_result.dma_addr =
> + vb2_dma_contig_plane_dma_addr(&dst_buf->vb2_buf, 0);
> + fd_result_vaddr = vb2_plane_vaddr(&dst_buf->vb2_buf, 0);
> +
> + ctx->user_param.src_img_fmt = get_fd_img_fmt(ctx->src_fmt.pixelformat);
> + memcpy(&fd_param.user_param, &ctx->user_param, sizeof(ctx->user_param));
> +
> + /* Complete request controls if any */
> + v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req, &ctx->hdl);
> +
> + mtk_fd_hw_job_exec(&ctx->fd_dev->fd_hw, &fd_param, fd_result_vaddr);
> +}
> +
> +static struct v4l2_m2m_ops fd_m2m_ops = {
> + .device_run = mtk_fd_device_run,
> +};
> +
> +static int mtk_fd_request_validate(struct media_request *req)
> +{
> + unsigned int count;
> +
> + count = vb2_request_buffer_cnt(req);
> + if (!count)
> + return -ENOENT;
Why -ENOENT?
> + else if (count > 1)
> + return -EINVAL;
> +
> + return vb2_request_validate(req);
> +}
> +
> +static const struct media_device_ops fd_m2m_media_ops = {
> + .req_validate = mtk_fd_request_validate,
> + .req_queue = v4l2_m2m_request_queue,
> +};
> +
> +static int mtk_fd_video_device_register(struct mtk_fd_dev *fd_dev)
> +{
> + struct video_device *vfd = &fd_dev->vfd;
> + struct v4l2_m2m_dev *m2m_dev = fd_dev->m2m_dev;
> + struct device *dev = &fd_dev->pdev->dev;
> + int function, ret;
> +
> + vfd->fops = &fd_video_fops;
> + vfd->release = video_device_release;
> + vfd->lock = &fd_dev->vfd_lock;
> + vfd->v4l2_dev = &fd_dev->v4l2_dev;
> + vfd->vfl_dir = VFL_DIR_M2M;
> + vfd->device_caps = V4L2_CAP_STREAMING | V4L2_CAP_VIDEO_OUTPUT_MPLANE |
> + V4L2_CAP_META_CAPTURE;
> + vfd->ioctl_ops = &mtk_fd_v4l2_video_out_ioctl_ops;
> +
> + strscpy(vfd->name, "MTK-FD-V4L2", sizeof(vfd->name));
Please use dev_driver_string().
> + video_set_drvdata(vfd, fd_dev);
> +
> + ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0);
> + if (ret) {
> + dev_err(dev, "Failed to register video device\n");
> + goto err_free_dev;
> + }
> +
> + function = MEDIA_ENT_F_PROC_VIDEO_DECODER;
MEDIA_ENT_F_PROC_VIDEO_STATISTICS fits here much more.
Also nit: Any reason to have this assigned to this intermediate variable
rather than just directly passed to the function?
> + ret = v4l2_m2m_register_media_controller(m2m_dev, vfd, function);
> + if (ret) {
> + dev_err(dev, "Failed to init mem2mem media controller\n");
> + goto err_unreg_video;
> + }
> + return 0;
> +
> +err_unreg_video:
> + video_unregister_device(vfd);
> +err_free_dev:
> + video_device_release(vfd);
> + return ret;
> +}
> +
> +static int mtk_fd_dev_v4l2_init(struct mtk_fd_dev *fd_dev)
> +{
> + struct media_device *mdev = &fd_dev->mdev;
> + struct device *dev = &fd_dev->pdev->dev;
> + int ret;
> +
> + ret = v4l2_device_register(dev, &fd_dev->v4l2_dev);
> + if (ret) {
> + dev_err(dev, "Failed to register v4l2 device\n");
> + return ret;
> + }
> +
> + fd_dev->m2m_dev = v4l2_m2m_init(&fd_m2m_ops);
> + if (IS_ERR(fd_dev->m2m_dev)) {
> + dev_err(dev, "Failed to init mem2mem device\n");
> + ret = PTR_ERR(fd_dev->m2m_dev);
> + goto fail_m2m_dev;
Please name the labels after the next clean-up step to be done, not the
failure.
> + }
> +
> + mdev->dev = dev;
> + strscpy(mdev->model, "MTK-FD-V4L2", sizeof(mdev->model));
Could we just use dev_driver_string() here instead?
> + snprintf(mdev->bus_info, sizeof(mdev->bus_info),
> + "platform:%s", dev_name(dev));
> + media_device_init(mdev);
> + mdev->ops = &fd_m2m_media_ops;
> + fd_dev->v4l2_dev.mdev = mdev;
> +
> + ret = mtk_fd_video_device_register(fd_dev);
> + if (ret) {
> + dev_err(dev, "Failed to register video device\n");
> + goto err_vdev;
> + }
> +
> + ret = media_device_register(mdev);
> + if (ret) {
> + dev_err(dev, "Failed to register mem2mem media device\n");
> + goto fail_mdev;
> + }
> +
> + return 0;
> +
> +fail_mdev:
> + v4l2_m2m_unregister_media_controller(fd_dev->m2m_dev);
> + video_unregister_device(&fd_dev->vfd);
> + video_device_release(&fd_dev->vfd);
> +err_vdev:
> + media_device_cleanup(mdev);
> + v4l2_m2m_release(fd_dev->m2m_dev);
> +fail_m2m_dev:
> + v4l2_device_unregister(&fd_dev->v4l2_dev);
> + return ret;
> +}
> +
> +static void mtk_fd_dev_v4l2_release(struct mtk_fd_dev *fd_dev)
> +{
> + v4l2_m2m_unregister_media_controller(fd_dev->m2m_dev);
> + video_unregister_device(&fd_dev->vfd);
> + video_device_release(&fd_dev->vfd);
> + media_device_cleanup(&fd_dev->mdev);
> + v4l2_m2m_release(fd_dev->m2m_dev);
> + v4l2_device_unregister(&fd_dev->v4l2_dev);
> +}
> +
> +static irqreturn_t mtk_fd_irq(int irq, void *data)
> +{
> + struct mtk_fd_hw *fd_hw = (struct mtk_fd_hw *)data;
> +
> + fd_hw->fd_irq_result = readl(fd_hw->fd_base + FD_INT);
We should be able to just handle the job completion directly from here.
> + wake_up_interruptible(&fd_hw->wq);
> + return IRQ_HANDLED;
> +}
> +
> +static int mtk_fd_hw_get_scp_mem(struct mtk_fd_hw *fd_hw,
> + struct fd_buffer *scp_mem)
> +{
> + struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> + struct device *dev = &fd_dev->pdev->dev;
> + dma_addr_t addr;
> + u32 size;
> +
> + scp_mem->scp_addr = scp_get_reserve_mem_phys(SCP_FD_MEM_ID);
> + size = scp_get_reserve_mem_size(SCP_FD_MEM_ID);
> + if (!scp_mem->scp_addr || !size)
> + return -EPROBE_DEFER;
Why -EPROBE_DEFER? Should we have some way to distinguish between the SCP
driver not being initialized yet and some other error?
> +
> + /* get dma addr address */
> + addr = dma_map_page_attrs(dev, phys_to_page(scp_mem->scp_addr), 0,
> + size, DMA_BIDIRECTIONAL,
Should this be really bidirectional?
> + DMA_ATTR_SKIP_CPU_SYNC);
> + if (dma_mapping_error(dev, addr)) {
> + scp_mem->scp_addr = 0;
> + dev_err(dev, "Failed to map scp addr\n");
> + return -ENOMEM;
> + }
> + scp_mem->dma_addr = addr;
> +
> + return 0;
> +}
> +
> +static int mtk_fd_probe(struct platform_device *pdev)
> +{
> + struct mtk_fd_dev *fd_dev;
nit: Perhaps just fd, to have shorter code?
> + struct device *dev = &pdev->dev;
> + struct mtk_fd_hw *fd_hw;
> + struct resource *res;
> + int irq;
> + int ret;
> +
> + fd_dev = devm_kzalloc(&pdev->dev, sizeof(*fd_dev), GFP_KERNEL);
> + if (!fd_dev)
> + return -ENOMEM;
> +
> + dev_set_drvdata(dev, fd_dev);
> + fd_hw = &fd_dev->fd_hw;
> + fd_dev->pdev = pdev;
Is pdev useful for anything? Perhaps you want to store &pdev->dev instead?
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + dev_err(dev, "no IRQ:%d resource info\n", irq);
> + return irq;
> + }
> + ret = devm_request_irq(dev, irq, mtk_fd_irq, IRQF_SHARED,
> + dev_driver_string(dev),
> + fd_hw);
Why not just fd_dev?
> + if (ret) {
> + dev_err(dev, "req_irq fail:%d\n", irq);
> + return ret;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + fd_hw->fd_base = devm_ioremap_resource(dev, res);
> + if (!fd_hw->fd_base) {
> + dev_err(dev, "unable to get fd reg base\n");
> + return PTR_ERR(fd_hw->fd_base);
> + }
> +
> + fd_hw->fd_clk = devm_clk_get(dev, "fd");
> + if (IS_ERR(fd_hw->fd_clk)) {
> + dev_err(dev, "cannot get fd_clk_img_fd clock\n");
> + return PTR_ERR(fd_hw->fd_clk);
> + }
> +
> + ret = mtk_fd_hw_get_scp_mem(fd_hw, &fd_hw->scp_mem);
> + if (ret) {
> + dev_err(dev, "scp memory init failed: %d\n", ret);
nit: Could we make the error messages a bit more consistent? For example
"Failed to get IRQ resource (%d)", "Failed to request IRQ (%d)", "Failed to
... (%d)", etc.
> + return ret;
> + }
> +
> + atomic_set(&fd_hw->fd_user_cnt, 0);
> + init_waitqueue_head(&fd_hw->wq);
> + mutex_init(&fd_dev->vfd_lock);
> + mutex_init(&fd_hw->fd_hw_lock);
> + pm_runtime_enable(dev);
> +
> + ret = mtk_fd_dev_v4l2_init(fd_dev);
> + if (ret) {
> + mutex_destroy(&fd_dev->fd_hw.fd_hw_lock);
> + mutex_destroy(&fd_dev->vfd_lock);
> + pm_runtime_disable(&pdev->dev);
Please move the clean-up to an error path on the bottom of the function.
> + dev_err(dev, "v4l2 init failed: %d\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int mtk_fd_remove(struct platform_device *pdev)
> +{
> + struct mtk_fd_dev *fd_dev = dev_get_drvdata(&pdev->dev);
> +
> + mtk_fd_dev_v4l2_release(fd_dev);
> + mutex_destroy(&fd_dev->fd_hw.fd_hw_lock);
> + mutex_destroy(&fd_dev->vfd_lock);
> + pm_runtime_disable(&pdev->dev);
The order of calls in remove should normally be the opposite to probe.
> +
> + return 0;
> +}
> +
> +static int mtk_fd_suspend(struct device *dev)
> +{
> + struct mtk_fd_dev *fd_dev = dev_get_drvdata(dev);
> +
> + if (pm_runtime_suspended(dev))
> + return 0;
> +
> + /* suspend FD HW */
> + writel(0x0, fd_dev->fd_hw.fd_base + FD_HW_ENABLE);
> + writel(0x0, fd_dev->fd_hw.fd_base + FD_INT_EN);
> + clk_disable_unprepare(fd_dev->fd_hw.fd_clk);
> + return 0;
> +}
> +
> +static int mtk_fd_resume(struct device *dev)
> +{
> + struct mtk_fd_dev *fd_dev = dev_get_drvdata(dev);
> + int ret;
> +
> + if (pm_runtime_suspended(dev))
> + return 0;
> +
> + ret = clk_prepare_enable(fd_dev->fd_hw.fd_clk);
> + if (ret < 0) {
> + dev_dbg(dev, "open fd clk failed\n");
> + clk_disable_unprepare(fd_dev->fd_hw.fd_clk);
> + }
> +
> + /* resume FD HW */
> + writel(ENABLE_FD, fd_dev->fd_hw.fd_base + FD_HW_ENABLE);
> + if (fd_dev->fd_hw.state == FD_ENQ)
> + writel(0x1, fd_dev->fd_hw.fd_base + FD_INT_EN);
> + return 0;
> +}
> +
> +static const struct dev_pm_ops mtk_fd_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(mtk_fd_suspend, mtk_fd_resume)
> + SET_RUNTIME_PM_OPS(mtk_fd_suspend, mtk_fd_resume, NULL)
We need separate runtime and system PM ops. Their behavior is expected to be
different.
For runtime PM ops, you the functions should just unconditionally power on
or power off the device.
For system PM ops, suspend needs to make sure that no further job is queued
to the hardware and that any job being already processed by the hardware is
completed. Resume needs to resume the processing if there are any further
jobs to be queued to the hardware.
> +};
> +
> +static const struct of_device_id mtk_fd_of_ids[] = {
> + { .compatible = "mediatek,mt8183-fd", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, mtk_fd_of_ids);
> +
> +static struct platform_driver mtk_fd_driver = {
> + .probe = mtk_fd_probe,
> + .remove = mtk_fd_remove,
> + .driver = {
> + .name = "mtk-fd-4.0",
> + .of_match_table = of_match_ptr(mtk_fd_of_ids),
> + .pm = &mtk_fd_pm_ops,
> + }
> +};
> +module_platform_driver(mtk_fd_driver);
> +
> +MODULE_DESCRIPTION("Mediatek FD driver");
> +MODULE_LICENSE("GPL");
GPL v2
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH/RFC 10/12] arm64: dts: renesas: r8a774c0: Point LVDS0 to its companion LVDS1
From: Laurent Pinchart @ 2019-08-02 8:27 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Kieran Bingham, Jacopo Mondi, Rob Herring, Mark Rutland,
Simon Horman, Magnus Damm, linux-renesas-soc, devicetree,
Geert Uytterhoeven, Chris Paterson, Biju Das
In-Reply-To: <1564731249-22671-11-git-send-email-fabrizio.castro@bp.renesas.com>
Hi Fabrizio,
Thank you for the patch.
On Fri, Aug 02, 2019 at 08:34:07AM +0100, Fabrizio Castro wrote:
> Add the new renesas,companion property to the LVDS0 node to point to the
> companion LVDS encoder LVDS1.
> Based on similar work from Laurent Pinchart for the r8a7799[05].
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
and taken in my tree.
> ---
> arch/arm64/boot/dts/renesas/r8a774c0.dtsi | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/renesas/r8a774c0.dtsi b/arch/arm64/boot/dts/renesas/r8a774c0.dtsi
> index e7b5bf2..b36d3b08 100644
> --- a/arch/arm64/boot/dts/renesas/r8a774c0.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a774c0.dtsi
> @@ -1844,6 +1844,8 @@
> resets = <&cpg 727>;
> status = "disabled";
>
> + renesas,companion = <&lvds1>;
> +
> ports {
> #address-cells = <1>;
> #size-cells = <0>;
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH] ARM: dts: stm32: add DFSDM pins to stm32mp157c
From: Alexandre Torgue @ 2019-08-02 8:09 UTC (permalink / raw)
To: Olivier Moysan, linux-stm32, robh, mark.rutland, devicetree,
linux-arm-kernel, linux-kernel
In-Reply-To: <1564645567-13156-1-git-send-email-olivier.moysan@st.com>
Hi Olivier
On 8/1/19 9:46 AM, Olivier Moysan wrote:
> Add DFSDM pins to stm32mp157c.
>
> Signed-off-by: Olivier Moysan <olivier.moysan@st.com>
> ---
> arch/arm/boot/dts/stm32mp157-pinctrl.dtsi | 39 +++++++++++++++++++++++++++++++
> 1 file changed, 39 insertions(+)
>
> diff --git a/arch/arm/boot/dts/stm32mp157-pinctrl.dtsi b/arch/arm/boot/dts/stm32mp157-pinctrl.dtsi
> index 9eaec9bf8cb8..f96a928cbc49 100644
> --- a/arch/arm/boot/dts/stm32mp157-pinctrl.dtsi
> +++ b/arch/arm/boot/dts/stm32mp157-pinctrl.dtsi
> @@ -230,6 +230,45 @@
> };
> };
>
I use to only take pinconfig which are used in board. So please resend
with the "board patch".
regards
Alex
> + dfsdm_clkout_pins_a: dfsdm-clkout-pins-0 {
> + pins {
> + pinmux = <STM32_PINMUX('B', 13, AF3)>; /* DFSDM_CKOUT */
> + bias-disable;
> + drive-push-pull;
> + slew-rate = <0>;
> + };
> + };
> +
> + dfsdm_clkout_sleep_pins_a: dfsdm-clkout-sleep-pins-0 {
> + pins {
> + pinmux = <STM32_PINMUX('B', 13, ANALOG)>; /* DFSDM_CKOUT */
> + };
> + };
> +
> + dfsdm_data1_pins_a: dfsdm-data1-pins-0 {
> + pins {
> + pinmux = <STM32_PINMUX('C', 3, AF3)>; /* DFSDM_DATA1 */
> + };
> + };
> +
> + dfsdm_data1_sleep_pins_a: dfsdm-data1-sleep-pins-0 {
> + pins {
> + pinmux = <STM32_PINMUX('C', 3, ANALOG)>; /* DFSDM_DATA1 */
> + };
> + };
> +
> + dfsdm_data3_pins_a: dfsdm-data3-pins-0 {
> + pins {
> + pinmux = <STM32_PINMUX('F', 13, AF6)>; /* DFSDM_DATA3 */
> + };
> + };
> +
> + dfsdm_data3_sleep_pins_a: dfsdm-data3-sleep-pins-0 {
> + pins {
> + pinmux = <STM32_PINMUX('F', 13, ANALOG)>; /* DFSDM_DATA3 */
> + };
> + };
> +
> ethernet0_rgmii_pins_a: rgmii-0 {
> pins1 {
> pinmux = <STM32_PINMUX('G', 5, AF11)>, /* ETH_RGMII_CLK125 */
>
^ permalink raw reply
* Re: [PATCH/RFC 04/12] dt-bindings: display: Add bindings for Advantech IDK-2121WR
From: Laurent Pinchart @ 2019-08-02 8:03 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Kieran Bingham, Jacopo Mondi, Thierry Reding, David Airlie,
Daniel Vetter, Rob Herring, Mark Rutland, Sam Ravnborg, dri-devel,
devicetree, linux-kernel, Simon Horman, Geert Uytterhoeven,
Chris Paterson, Biju Das, linux-renesas-soc
In-Reply-To: <1564731249-22671-5-git-send-email-fabrizio.castro@bp.renesas.com>
Hi Fabrizio,
Thank you for the patch.
On Fri, Aug 02, 2019 at 08:34:01AM +0100, Fabrizio Castro wrote:
> This panel is handled through the generic lvds-panel bindings,
> so only needs its additional compatible specified.
>
> Some panel specific documentation can be found here:
s/panel specific/panel-specific/
> https://buy.advantech.eu/Displays/Embedded-LCD-Kits-High-Brightness/model-IDK-2121WR-K2FHA2E.htm
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> ---
> .../display/panel/advantech,idk-2121wr.txt | 62 ++++++++++++++++++++++
> 1 file changed, 62 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.txt
>
> diff --git a/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.txt b/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.txt
> new file mode 100644
> index 0000000..70b15b6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.txt
> @@ -0,0 +1,62 @@
> +Advantech Co., Ltd. IDK-2121WR 21.5" LVDS panel
> +===============================================
> +
> +Required properties:
> +- compatible: should be "advantech,idk-2121wr" followed by "panel-lvds"
> +
> +This binding is compatible with the lvds-panel binding, which is specified
> +in panel-lvds.txt in this directory.
How about adding "The panel operates in dual-link mode and thus requires
two port nodes." ?
> +
> +Example
> +-------
> +
> + panel {
> + compatible = "advantech,idk-2121wr", "panel-lvds";
> +
> + width-mm = <476>;
> + height-mm = <268>;
> +
> + data-mapping = "vesa-24";
> +
> + panel-timing {
> + clock-frequency = <148500000>;
> + hactive = <1920>;
> + vactive = <1080>;
> + hsync-len = <44>;
> + hfront-porch = <88>;
> + hback-porch = <148>;
> + vfront-porch = <4>;
> + vback-porch = <36>;
> + vsync-len = <5>;
> + };
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> + lvds0_panel_in: endpoint {
> + remote-endpoint = <&lvds0_out>;
> + };
> + };
> +
> + port@1 {
> + reg = <1>;
> + lvds1_panel_in: endpoint {
> + remote-endpoint = <&lvds1_out>;
> + };
> + };
> + };
> + };
> +
> + backlight: backlight {
> + compatible = "pwm-backlight";
> + pwms = <&pwm5 0 50000>;
> +
> + brightness-levels = <0 4 8 16 32 64 128 255>;
> + default-brightness-level = <6>;
> +
> + power-supply = <®_12p0v>;
> + enable-gpios = <&gpio6 12 GPIO_ACTIVE_HIGH>;
> + };
I think you can drop the backlight here, it's a bit out of scope.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH] ARM: dts: stm32: add phy-dsi-supply property on stm32mp157c-ev1
From: Yannick FERTRE @ 2019-08-02 8:01 UTC (permalink / raw)
To: Alexandre TORGUE, Maxime Coquelin, Rob Herring, Mark Rutland,
linux-stm32@st-md-mailman.stormreply.com,
linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, Benjamin GAIGNARD, Philippe CORNU,
Fabrice GASNIER
In-Reply-To: <346d04ad-17ed-40c8-f10a-b13a2ea79d92@st.com>
Many thanks Alex.
On 8/2/19 9:36 AM, Alexandre Torgue wrote:
> Hi Yannick
>
> On 7/29/19 4:29 PM, Yannick Fertré wrote:
>> The dsi physical layer is powered by the 1v8 power controller supply.
>>
>> Signed-off-by: Yannick Fertré <yannick.fertre@st.com>
>> ---
>> arch/arm/boot/dts/stm32mp157c-ev1.dts | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/arm/boot/dts/stm32mp157c-ev1.dts
>> b/arch/arm/boot/dts/stm32mp157c-ev1.dts
>> index feb8f77..19d69d0 100644
>> --- a/arch/arm/boot/dts/stm32mp157c-ev1.dts
>> +++ b/arch/arm/boot/dts/stm32mp157c-ev1.dts
>> @@ -101,6 +101,7 @@
>> &dsi {
>> #address-cells = <1>;
>> #size-cells = <0>;
>> + phy-dsi-supply = <®18>;
>> status = "okay";
>> ports {
>>
>
> Applied on stm32-next.
>
> Thanks.
> Alex
--
Yannick Fertré | TINA: 166 7152 | Tel: +33 244027152 | Mobile: +33 620600270
Microcontrollers and Digital ICs Group | Microcontrolleurs Division
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH/RFC 03/12] dt-bindings: panel: lvds: Add dual-link LVDS display support
From: Laurent Pinchart @ 2019-08-02 8:00 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Kieran Bingham, Jacopo Mondi, Thierry Reding, David Airlie,
Daniel Vetter, Rob Herring, Mark Rutland, Sam Ravnborg, dri-devel,
devicetree, linux-kernel, Simon Horman, Geert Uytterhoeven,
Chris Paterson, Biju Das, linux-renesas-soc
In-Reply-To: <1564731249-22671-4-git-send-email-fabrizio.castro@bp.renesas.com>
Hi Fabrizio,
Thank you for the patch.
On Fri, Aug 02, 2019 at 08:34:00AM +0100, Fabrizio Castro wrote:
> Dual-link LVDS displays have two ports, therefore document this
> with the bindings.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> ---
> .../bindings/display/panel/panel-lvds.txt | 91 ++++++++++++++++------
> 1 file changed, 67 insertions(+), 24 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/display/panel/panel-lvds.txt b/Documentation/devicetree/bindings/display/panel/panel-lvds.txt
> index 250850a..07795441 100644
> --- a/Documentation/devicetree/bindings/display/panel/panel-lvds.txt
> +++ b/Documentation/devicetree/bindings/display/panel/panel-lvds.txt
> @@ -41,7 +41,8 @@ Required nodes:
>
> - panel-timing: See panel-common.txt.
> - ports: See panel-common.txt. These bindings require a single port subnode
> - corresponding to the panel LVDS input.
> + (for a single link display) or two port subnodes (for a dual link display)
> + corresponding to the panel LVDS input(s).
I think you should expand this a bit to explain what the ports
correspond to in the dual link mode.
> LVDS data mappings are defined as follows.
> @@ -92,30 +93,72 @@ CTL3: 0
> Example
> -------
>
> -panel {
> - compatible = "mitsubishi,aa121td01", "panel-lvds";
> -
> - width-mm = <261>;
> - height-mm = <163>;
> -
> - data-mapping = "jeida-24";
> -
> - panel-timing {
> - /* 1280x800 @60Hz */
> - clock-frequency = <71000000>;
> - hactive = <1280>;
> - vactive = <800>;
> - hsync-len = <70>;
> - hfront-porch = <20>;
> - hback-porch = <70>;
> - vsync-len = <5>;
> - vfront-porch = <3>;
> - vback-porch = <15>;
> +Single port:
> + panel {
> + compatible = "mitsubishi,aa121td01", "panel-lvds";
> +
> + width-mm = <261>;
> + height-mm = <163>;
> +
> + data-mapping = "jeida-24";
> +
> + panel-timing {
> + /* 1280x800 @60Hz */
> + clock-frequency = <71000000>;
> + hactive = <1280>;
> + vactive = <800>;
> + hsync-len = <70>;
> + hfront-porch = <20>;
> + hback-porch = <70>;
> + vsync-len = <5>;
> + vfront-porch = <3>;
> + vback-porch = <15>;
> + };
> +
> + port {
> + panel_in: endpoint {
> + remote-endpoint = <&lvds_encoder>;
> + };
> + };
> };
>
> - port {
> - panel_in: endpoint {
> - remote-endpoint = <&lvds_encoder>;
> +Two ports:
> + panel {
> + compatible = "advantech,idk-2121wr", "panel-lvds";
> +
> + width-mm = <476>;
> + height-mm = <268>;
> +
> + data-mapping = "vesa-24";
> +
> + panel-timing {
> + clock-frequency = <148500000>;
> + hactive = <1920>;
> + vactive = <1080>;
> + hsync-len = <44>;
> + hfront-porch = <88>;
> + hback-porch = <148>;
> + vfront-porch = <4>;
> + vback-porch = <36>;
> + vsync-len = <5>;
> + };
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> + lvds0_panel_in: endpoint {
I would name the label panel_in0 and panel_in1 below to have a common
prefix showing that both refer to the same panel.
> + remote-endpoint = <&lvds0_out>;
> + };
> + };
> +
> + port@1 {
> + reg = <1>;
> + lvds1_panel_in: endpoint {
> + remote-endpoint = <&lvds1_out>;
> + };
> + };
> };
> };
> -};
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH/RFC 01/12] dt-bindings: display: renesas: lvds: RZ/G2E needs renesas,companion too
From: Laurent Pinchart @ 2019-08-02 7:48 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Mark Rutland, devicetree, Chris Paterson, Geert Uytterhoeven,
Simon Horman, David Airlie, Kieran Bingham, linux-kernel,
dri-devel, Biju Das, linux-renesas-soc, Rob Herring, Jacopo Mondi
In-Reply-To: <1564731249-22671-2-git-send-email-fabrizio.castro@bp.renesas.com>
Hello Fabrizio,
Thank you for the patch.
On Fri, Aug 02, 2019 at 08:33:58AM +0100, Fabrizio Castro wrote:
> Document RZ/G2E support for property renesas,companion.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
and taken in my tree.
> ---
> Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt b/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt
> index c6a196d..dece79e 100644
> --- a/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt
> @@ -49,9 +49,9 @@ Each port shall have a single endpoint.
> Optional properties:
>
> - renesas,companion : phandle to the companion LVDS encoder. This property is
> - mandatory for the first LVDS encoder on D3 and E3 SoCs, and shall point to
> - the second encoder to be used as a companion in dual-link mode. It shall not
> - be set for any other LVDS encoder.
> + mandatory for the first LVDS encoder on R-Car D3, R-Car E3, and RZ/G2E SoCs,
> + and shall point to the second encoder to be used as a companion in dual-link
> + mode. It shall not be set for any other LVDS encoder.
>
>
> Example:
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH/RFC 02/12] dt-bindings: display: renesas: lvds: Document renesas,swap-data
From: Laurent Pinchart @ 2019-08-02 7:44 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Kieran Bingham, Jacopo Mondi, David Airlie, Daniel Vetter,
Rob Herring, Mark Rutland, dri-devel, linux-renesas-soc,
devicetree, linux-kernel, Simon Horman, Geert Uytterhoeven,
Chris Paterson, Biju Das
In-Reply-To: <1564731249-22671-3-git-send-email-fabrizio.castro@bp.renesas.com>
Hi Fabrizio,
Thank you for the patch.
On Fri, Aug 02, 2019 at 08:33:59AM +0100, Fabrizio Castro wrote:
> R-Car D3, R-Car E3, and RZ/G2E support dual-link mode.
> In such a mode, the first LVDS encoder emits even data, and the
> second LVDS encoder emits odd data. This patch documents property
> renesas,swap-data, used to swap even and odd data around.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> ---
> Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt b/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt
> index dece79e..8980179 100644
> --- a/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt
> @@ -52,6 +52,11 @@ Optional properties:
> mandatory for the first LVDS encoder on R-Car D3, R-Car E3, and RZ/G2E SoCs,
> and shall point to the second encoder to be used as a companion in dual-link
> mode. It shall not be set for any other LVDS encoder.
> +- renesas,swap-data : when in dual-link mode, the first LVDS encoder normally
> + emits even data, and the second LVDS encoder emits odd data. When property
> + renesas,swap-data is specified, the data emitted by the two encoders will be
> + swapped around. This property can only be used in conjunction with property
> + renesas,companion.
>From an LVDS encoder point of view this is more a configuration option
than a description of the hardware. Wouldn't it be better for the LVDS
sink to report which of the odd or even pixels it expects on each of its
endpoints ? The LVDS encoder driver could then query that at runtime and
configure itself accordingly. Ideally this should be queried through the
drm_bridge_timings structure (or through a similar mean), not through
DT. An LVDS sink that has a fixed mapping of odd/even pixels to
endpoints wouldn't need the information to be specified in DT at all.
>
>
> Example:
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH v9 04/18] kunit: test: add kunit_stream a std::stream like logger
From: John Ogness @ 2019-08-02 7:37 UTC (permalink / raw)
To: Brendan Higgins
Cc: Petr Mladek, Jeff Dike, Kevin Hilman, Logan Gunthorpe,
Michael Ellerman, Daniel Vetter, Amir Goldstein, Frank Rowand,
Steven Rostedt, Kees Cook, David Rientjes, kunit-dev,
Kieran Bingham, Peter Zijlstra, Randy Dunlap, Joel Stanley,
Luis Chamberlain, Rob Herring, Stephen Boyd, shuah, wfg, Greg KH
In-Reply-To: <CAFd5g46iAhDZ5C_chi7oYLVOkwcoj6+0nw+kPWuXhqWwWKd9jA@mail.gmail.com>
On 2019-08-01, Brendan Higgins <brendanhiggins@google.com> wrote:
> On Fri, Jul 26, 2019 at 1:31 AM Petr Mladek <pmladek@suse.com> wrote:
>> On Thu 2019-07-25 13:21:12, Brendan Higgins wrote:
>>> On Wed, Jul 24, 2019 at 12:31 AM Petr Mladek <pmladek@suse.com> wrote:
>>>> On Mon 2019-07-22 16:54:10, Stephen Boyd wrote:
>>>>> Quoting Brendan Higgins (2019-07-22 15:30:49)
>>>>>> On Mon, Jul 22, 2019 at 1:03 PM Stephen Boyd <sboyd@kernel.org> wrote:
>>>>>>> What's the calling context of the assertions and expectations? I
>>>>>>> still don't like the fact that string stream needs to allocate
>>>>>>> buffers and throw them into a list somewhere because the calling
>>>>>>> context matters there.
>>>>>>
>>>>>> The calling context is the same as before, which is anywhere.
>>>>>
>>>>> Ok. That's concerning then.
>>>>>
>>>>>>> I'd prefer we just wrote directly to the console/log via printk
>>>>>>> instead. That way things are simple because we use the existing
>>>>>>> buffering path of printk, but maybe there's some benefit to the
>>>>>>> string stream that I don't see? Right now it looks like it
>>>>>>> builds a string and then dumps it to printk so I'm sort of lost
>>>>>>> what the benefit is over just writing directly with printk.
>>>>>>
>>>>>> It's just buffering it so the whole string gets printed
>>>>>> uninterrupted. If we were to print out piecemeal to printk,
>>>>>> couldn't we have another call to printk come in causing it to
>>>>>> garble the KUnit message we are in the middle of printing?
>>>>>
>>>>> Yes, printing piecemeal by calling printk many times could lead to
>>>>> interleaving of messages if something else comes in such as an
>>>>> interrupt printing something. Printk has some support to hold
>>>>> "records" but I'm not sure how that would work here because
>>>>> KERN_CONT talks about only being used early on in boot code. I
>>>>> haven't looked at printk in detail though so maybe I'm all wrong
>>>>> and KERN_CONT just works?
>>>>
>>>> KERN_CONT does not guarantee that the message will get printed
>>>> together. The pieces get interleaved with messages printed in
>>>> parallel.
>>>>
>>>> Note that KERN_CONT was originally really meant to be used only
>>>> during boot. It was later used more widely and ended in the best
>>>> effort category.
>>>>
>>>> There were several attempts to make it more reliable. But it was
>>>> always either too complicated or error prone or both.
>>>>
>>>> You need to use your own buffering if you rely want perfect output.
>>>> The question is if it is really worth the complexity. Also note
>>>> that any buffering reduces the chance that the messages will reach
>>>> the console.
>>>
>>> Seems like that settles it then. Thanks!
>>>
>>>> BTW: There is a work in progress on a lockless printk ring buffer.
>>>> It will make printk() more secure regarding deadlocks. But it might
>>>> make transparent handling of continuous lines even more tricky.
>>>>
>>>> I guess that local buffering, before calling printk(), will be
>>>> even more important then. Well, it might really force us to create
>>>> an API for it.
>>>
>>> Cool! Can you CC me on that discussion?
>>
>> Adding John Oggness into CC.
>>
>> John, please CC Brendan Higgins on the patchsets eventually switching
>> printk() into the lockless buffer. The test framework is going to
>> do its own buffering to keep the related messages together.
>>
>> The lockless ringbuffer might make handling of related (partial)
>> lines worse or better. It might justify KUnit's extra buffering
>> or it might allow to get rid of it.
>
> Thanks for CC'ing me on the printk ringbuffer thread. It looks like it
> actually probably won't affect my needs for KUnit logging. The biggest
> reason I need some sort of buffering system is to be able to compose
> messages piece meal into a single message that will be printed out to
> the user as a single message with no messages from other printk
> callers printed out in the middle of mine.
printk has this same requirement for its CONT messages. You can read
about how I propose to implement that here[0], using a separate prb
ringbuffer for buffered storage until all the pieces are available.
It is not my goal that multiple subsystems start making use of the prb
ringbuffer. However, its features can be attractive if you don't want to
worry about multiple writers/readers or context (including NMI). Before
writing "yet another ringbuffer" [1] it might be worth the effort to at
least see if one of the existing implementations can work (or be
extended to work) for you.
John Ogness
[0] https://lkml.kernel.org/r/87imt2bl0k.fsf@linutronix.de
[1] https://lwn.net/Articles/789603/
> The prb does look interesting; however, it appears that to get the
> semantics that I need, I would have to put my entire message in a
> single data block and would consequently need to know the size of my
> message a priori, which is problematic. Consequently, it seems as
> though I will probably need to compose my entire message using my own
> buffering system.
>
>>>> Note that stroring the messages into the printk log is basically
>>>> safe in any context. It uses temporary per-CPU buffers for
>>>> recursive messages and in NMI. The only problem is panic() when
>>>> some CPU gets stuck with the lock taken. This will get solved by
>>>> the lockless ringbuffer. Also the temporary buffers will not be
>>>> necessary any longer.
>>>
>>> Sure, I think Stephen's concern is all the supporting code that is
>>> involved. Not printk specifically. It just means a lot more of KUnit
>>> has to be IRQ safe.
>>
>> I see.
>>
>> BTW: I wonder if KUnit could reuse the existing seq_buf
>> implementation for buffering messages.
>>
>> I am sorry if it has already been proposed and rejected for some
>> reason. I might have missed it. Feel free to just point me to
>> same older mail.
>
> Yeah, we discussed it briefly here:
>
> https://lkml.org/lkml/2019/5/17/497
>
> Looks like I forgot to include my reasoning in the commit text, sorry
> about that.
>
>>>> Much bigger problems are with consoles. There are many of them. It
>>>> means a lot of code and more locks involved, including scheduler
>>>> locks. Note that console lock is a semaphore.
>>>
>>> That shouldn't affect us though, right? As long as we continue to
>>> use the printk interface?
>>
>> I guess that it should not affect KUnit.
>>
>> The only problem might be if the testing framework calls printk()
>> inside scheduler or console code. And only when the tested code
>> uses the same locks that will be used by the called printk().
>
> Yeah, well printk will not be our only problem in those instances.
>
>> To be honest I do not fully understand KUnit design. I am not
>> completely sure how the tested code is isolated from the running
>> system. Namely, I do not know if the tested code shares
>> the same locks with the system running the test.
>
> No worries, I don't expect printk to be the hang up in those cases. It
> sounds like KUnit has a long way to evolve before printk is going to
> be a limitation.
>
> Thanks!
^ permalink raw reply
* Re: [PATCH] ARM: dts: stm32: add phy-dsi-supply property on stm32mp157c-ev1
From: Alexandre Torgue @ 2019-08-02 7:36 UTC (permalink / raw)
To: Yannick Fertré, Maxime Coquelin, Rob Herring, Mark Rutland,
linux-stm32, linux-arm-kernel, devicetree, linux-kernel,
Benjamin Gaignard, Philippe Cornu, Fabrice Gasnier
In-Reply-To: <1564410548-20436-1-git-send-email-yannick.fertre@st.com>
Hi Yannick
On 7/29/19 4:29 PM, Yannick Fertré wrote:
> The dsi physical layer is powered by the 1v8 power controller supply.
>
> Signed-off-by: Yannick Fertré <yannick.fertre@st.com>
> ---
> arch/arm/boot/dts/stm32mp157c-ev1.dts | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/boot/dts/stm32mp157c-ev1.dts b/arch/arm/boot/dts/stm32mp157c-ev1.dts
> index feb8f77..19d69d0 100644
> --- a/arch/arm/boot/dts/stm32mp157c-ev1.dts
> +++ b/arch/arm/boot/dts/stm32mp157c-ev1.dts
> @@ -101,6 +101,7 @@
> &dsi {
> #address-cells = <1>;
> #size-cells = <0>;
> + phy-dsi-supply = <®18>;
> status = "okay";
>
> ports {
>
Applied on stm32-next.
Thanks.
Alex
^ permalink raw reply
* [PATCH/RFC 12/12] arm64: dts: renesas: Add EK874 board with idk-2121wr display support
From: Fabrizio Castro @ 2019-08-02 7:34 UTC (permalink / raw)
To: Laurent Pinchart, Kieran Bingham, Jacopo Mondi, Rob Herring,
Mark Rutland
Cc: Fabrizio Castro, Simon Horman, Magnus Damm, linux-renesas-soc,
devicetree, Geert Uytterhoeven, Chris Paterson, Biju Das,
ebiharaml
In-Reply-To: <1564731249-22671-1-git-send-email-fabrizio.castro@bp.renesas.com>
The EK874 is advertised as compatible with panel IDK-2121WR from
Advantech, however the panel isn't sold alongside the board.
A new dts, adding everything that's required to get the panel to
to work with the EK874, is the most convenient way to support the
EK874 when it's connected to the IDK-2121WR.
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
---
arch/arm64/boot/dts/renesas/Makefile | 3 +-
.../boot/dts/renesas/r8a774c0-ek874-idk-2121wr.dts | 112 +++++++++++++++++++++
2 files changed, 114 insertions(+), 1 deletion(-)
create mode 100644 arch/arm64/boot/dts/renesas/r8a774c0-ek874-idk-2121wr.dts
diff --git a/arch/arm64/boot/dts/renesas/Makefile b/arch/arm64/boot/dts/renesas/Makefile
index 42b74c2..ce48478 100644
--- a/arch/arm64/boot/dts/renesas/Makefile
+++ b/arch/arm64/boot/dts/renesas/Makefile
@@ -1,7 +1,8 @@
# SPDX-License-Identifier: GPL-2.0
dtb-$(CONFIG_ARCH_R8A774A1) += r8a774a1-hihope-rzg2m.dtb
dtb-$(CONFIG_ARCH_R8A774A1) += r8a774a1-hihope-rzg2m-ex.dtb
-dtb-$(CONFIG_ARCH_R8A774C0) += r8a774c0-cat874.dtb r8a774c0-ek874.dtb
+dtb-$(CONFIG_ARCH_R8A774C0) += r8a774c0-cat874.dtb r8a774c0-ek874.dtb \
+ r8a774c0-ek874-idk-2121wr.dtb
dtb-$(CONFIG_ARCH_R8A7795) += r8a7795-salvator-x.dtb r8a7795-h3ulcb.dtb
dtb-$(CONFIG_ARCH_R8A7795) += r8a7795-h3ulcb-kf.dtb
dtb-$(CONFIG_ARCH_R8A7795) += r8a7795-salvator-xs.dtb
diff --git a/arch/arm64/boot/dts/renesas/r8a774c0-ek874-idk-2121wr.dts b/arch/arm64/boot/dts/renesas/r8a774c0-ek874-idk-2121wr.dts
new file mode 100644
index 0000000..d989998
--- /dev/null
+++ b/arch/arm64/boot/dts/renesas/r8a774c0-ek874-idk-2121wr.dts
@@ -0,0 +1,112 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Device Tree Source for the Silicon Linux RZ/G2E evaluation kit (EK874),
+ * connected to an Advantech IDK-2121WR 21.5" LVDS panel
+ *
+ * Copyright (C) 2019 Renesas Electronics Corp.
+ */
+
+#include "r8a774c0-ek874.dts"
+
+/ {
+ backlight: backlight {
+ compatible = "pwm-backlight";
+ pwms = <&pwm5 0 50000>;
+
+ brightness-levels = <0 4 8 16 32 64 128 255>;
+ default-brightness-level = <6>;
+
+ power-supply = <®_12p0v>;
+ enable-gpios = <&gpio6 12 GPIO_ACTIVE_HIGH>;
+ };
+
+ panel-lvds {
+ compatible = "advantech,idk-2121wr", "panel-lvds";
+
+ width-mm = <476>;
+ height-mm = <268>;
+
+ data-mapping = "vesa-24";
+
+ panel-timing {
+ clock-frequency = <148500000>;
+ hactive = <1920>;
+ vactive = <1080>;
+ hsync-len = <44>;
+ hfront-porch = <88>;
+ hback-porch = <148>;
+ vfront-porch = <4>;
+ vback-porch = <36>;
+ vsync-len = <5>;
+ };
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+ lvds0_panel_in: endpoint {
+ remote-endpoint = <&lvds0_out>;
+ };
+ };
+
+ port@1 {
+ reg = <1>;
+ lvds1_panel_in: endpoint {
+ remote-endpoint = <&lvds1_out>;
+ };
+ };
+ };
+ };
+};
+
+&gpio0 {
+ lvds-connector-en-gpio{
+ gpio-hog;
+ gpios = <17 GPIO_ACTIVE_HIGH>;
+ output-low;
+ line-name = "lvds-connector-en-gpio";
+ };
+};
+
+&lvds0 {
+ renesas,swap-data;
+
+ ports {
+ port@1 {
+ lvds0_out: endpoint {
+ remote-endpoint = <&lvds0_panel_in>;
+ };
+ };
+ };
+};
+
+&lvds1 {
+ status = "okay";
+
+ clocks = <&cpg CPG_MOD 727>, <&x13_clk>, <&extal_clk>;
+ clock-names = "fck", "dclkin.0", "extal";
+
+ ports {
+ port@1 {
+ lvds1_out: endpoint {
+ remote-endpoint = <&lvds1_panel_in>;
+ };
+ };
+ };
+};
+
+&pfc {
+ pwm5_pins: pwm5 {
+ groups = "pwm5_a";
+ function = "pwm5";
+ };
+};
+
+&pwm5 {
+ pinctrl-0 = <&pwm5_pins>;
+ pinctrl-names = "default";
+
+ status = "okay";
+};
--
2.7.4
^ permalink raw reply related
* [PATCH/RFC 11/12] arm64: dts: renesas: cat874: Add definition for 12V regulator
From: Fabrizio Castro @ 2019-08-02 7:34 UTC (permalink / raw)
To: Geert Uytterhoeven, Laurent Pinchart, Kieran Bingham,
Jacopo Mondi, Rob Herring, Mark Rutland
Cc: Fabrizio Castro, Simon Horman, Magnus Damm, linux-renesas-soc,
devicetree, Chris Paterson, Biju Das, ebiharaml
In-Reply-To: <1564731249-22671-1-git-send-email-fabrizio.castro@bp.renesas.com>
Power rail "D12.0V" comes straight from the power barrel connector,
and it's used in both main board and sub board.
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
---
arch/arm64/boot/dts/renesas/r8a774c0-cat874.dts | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/arch/arm64/boot/dts/renesas/r8a774c0-cat874.dts b/arch/arm64/boot/dts/renesas/r8a774c0-cat874.dts
index 46a77ee..651383c 100644
--- a/arch/arm64/boot/dts/renesas/r8a774c0-cat874.dts
+++ b/arch/arm64/boot/dts/renesas/r8a774c0-cat874.dts
@@ -65,6 +65,15 @@
reg = <0x0 0x48000000 0x0 0x78000000>;
};
+ reg_12p0v: regulator-12p0v {
+ compatible = "regulator-fixed";
+ regulator-name = "D12.0V";
+ regulator-min-microvolt = <12000000>;
+ regulator-max-microvolt = <12000000>;
+ regulator-boot-on;
+ regulator-always-on;
+ };
+
sound: sound {
compatible = "simple-audio-card";
--
2.7.4
^ permalink raw reply related
* [PATCH/RFC 10/12] arm64: dts: renesas: r8a774c0: Point LVDS0 to its companion LVDS1
From: Fabrizio Castro @ 2019-08-02 7:34 UTC (permalink / raw)
To: Laurent Pinchart, Kieran Bingham, Jacopo Mondi, Rob Herring,
Mark Rutland
Cc: Fabrizio Castro, Simon Horman, Magnus Damm, linux-renesas-soc,
devicetree, Geert Uytterhoeven, Chris Paterson, Biju Das
In-Reply-To: <1564731249-22671-1-git-send-email-fabrizio.castro@bp.renesas.com>
Add the new renesas,companion property to the LVDS0 node to point to the
companion LVDS encoder LVDS1.
Based on similar work from Laurent Pinchart for the r8a7799[05].
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
---
arch/arm64/boot/dts/renesas/r8a774c0.dtsi | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm64/boot/dts/renesas/r8a774c0.dtsi b/arch/arm64/boot/dts/renesas/r8a774c0.dtsi
index e7b5bf2..b36d3b08 100644
--- a/arch/arm64/boot/dts/renesas/r8a774c0.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a774c0.dtsi
@@ -1844,6 +1844,8 @@
resets = <&cpg 727>;
status = "disabled";
+ renesas,companion = <&lvds1>;
+
ports {
#address-cells = <1>;
#size-cells = <0>;
--
2.7.4
^ permalink raw reply related
* [PATCH/RFC 04/12] dt-bindings: display: Add bindings for Advantech IDK-2121WR
From: Fabrizio Castro @ 2019-08-02 7:34 UTC (permalink / raw)
To: Laurent Pinchart, Kieran Bingham, Jacopo Mondi, Thierry Reding,
David Airlie, Daniel Vetter, Rob Herring, Mark Rutland
Cc: Fabrizio Castro, Sam Ravnborg, dri-devel, devicetree,
linux-kernel, Simon Horman, Geert Uytterhoeven, Chris Paterson,
Biju Das, linux-renesas-soc
In-Reply-To: <1564731249-22671-1-git-send-email-fabrizio.castro@bp.renesas.com>
This panel is handled through the generic lvds-panel bindings,
so only needs its additional compatible specified.
Some panel specific documentation can be found here:
https://buy.advantech.eu/Displays/Embedded-LCD-Kits-High-Brightness/model-IDK-2121WR-K2FHA2E.htm
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
---
.../display/panel/advantech,idk-2121wr.txt | 62 ++++++++++++++++++++++
1 file changed, 62 insertions(+)
create mode 100644 Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.txt
diff --git a/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.txt b/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.txt
new file mode 100644
index 0000000..70b15b6
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.txt
@@ -0,0 +1,62 @@
+Advantech Co., Ltd. IDK-2121WR 21.5" LVDS panel
+===============================================
+
+Required properties:
+- compatible: should be "advantech,idk-2121wr" followed by "panel-lvds"
+
+This binding is compatible with the lvds-panel binding, which is specified
+in panel-lvds.txt in this directory.
+
+Example
+-------
+
+ panel {
+ compatible = "advantech,idk-2121wr", "panel-lvds";
+
+ width-mm = <476>;
+ height-mm = <268>;
+
+ data-mapping = "vesa-24";
+
+ panel-timing {
+ clock-frequency = <148500000>;
+ hactive = <1920>;
+ vactive = <1080>;
+ hsync-len = <44>;
+ hfront-porch = <88>;
+ hback-porch = <148>;
+ vfront-porch = <4>;
+ vback-porch = <36>;
+ vsync-len = <5>;
+ };
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+ lvds0_panel_in: endpoint {
+ remote-endpoint = <&lvds0_out>;
+ };
+ };
+
+ port@1 {
+ reg = <1>;
+ lvds1_panel_in: endpoint {
+ remote-endpoint = <&lvds1_out>;
+ };
+ };
+ };
+ };
+
+ backlight: backlight {
+ compatible = "pwm-backlight";
+ pwms = <&pwm5 0 50000>;
+
+ brightness-levels = <0 4 8 16 32 64 128 255>;
+ default-brightness-level = <6>;
+
+ power-supply = <®_12p0v>;
+ enable-gpios = <&gpio6 12 GPIO_ACTIVE_HIGH>;
+ };
--
2.7.4
^ permalink raw reply related
* [PATCH/RFC 03/12] dt-bindings: panel: lvds: Add dual-link LVDS display support
From: Fabrizio Castro @ 2019-08-02 7:34 UTC (permalink / raw)
To: Laurent Pinchart, Kieran Bingham, Jacopo Mondi, Thierry Reding,
David Airlie, Daniel Vetter, Rob Herring, Mark Rutland
Cc: Fabrizio Castro, Sam Ravnborg, dri-devel, devicetree,
linux-kernel, Simon Horman, Geert Uytterhoeven, Chris Paterson,
Biju Das, linux-renesas-soc
In-Reply-To: <1564731249-22671-1-git-send-email-fabrizio.castro@bp.renesas.com>
Dual-link LVDS displays have two ports, therefore document this
with the bindings.
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
---
.../bindings/display/panel/panel-lvds.txt | 91 ++++++++++++++++------
1 file changed, 67 insertions(+), 24 deletions(-)
diff --git a/Documentation/devicetree/bindings/display/panel/panel-lvds.txt b/Documentation/devicetree/bindings/display/panel/panel-lvds.txt
index 250850a..07795441 100644
--- a/Documentation/devicetree/bindings/display/panel/panel-lvds.txt
+++ b/Documentation/devicetree/bindings/display/panel/panel-lvds.txt
@@ -41,7 +41,8 @@ Required nodes:
- panel-timing: See panel-common.txt.
- ports: See panel-common.txt. These bindings require a single port subnode
- corresponding to the panel LVDS input.
+ (for a single link display) or two port subnodes (for a dual link display)
+ corresponding to the panel LVDS input(s).
LVDS data mappings are defined as follows.
@@ -92,30 +93,72 @@ CTL3: 0
Example
-------
-panel {
- compatible = "mitsubishi,aa121td01", "panel-lvds";
-
- width-mm = <261>;
- height-mm = <163>;
-
- data-mapping = "jeida-24";
-
- panel-timing {
- /* 1280x800 @60Hz */
- clock-frequency = <71000000>;
- hactive = <1280>;
- vactive = <800>;
- hsync-len = <70>;
- hfront-porch = <20>;
- hback-porch = <70>;
- vsync-len = <5>;
- vfront-porch = <3>;
- vback-porch = <15>;
+Single port:
+ panel {
+ compatible = "mitsubishi,aa121td01", "panel-lvds";
+
+ width-mm = <261>;
+ height-mm = <163>;
+
+ data-mapping = "jeida-24";
+
+ panel-timing {
+ /* 1280x800 @60Hz */
+ clock-frequency = <71000000>;
+ hactive = <1280>;
+ vactive = <800>;
+ hsync-len = <70>;
+ hfront-porch = <20>;
+ hback-porch = <70>;
+ vsync-len = <5>;
+ vfront-porch = <3>;
+ vback-porch = <15>;
+ };
+
+ port {
+ panel_in: endpoint {
+ remote-endpoint = <&lvds_encoder>;
+ };
+ };
};
- port {
- panel_in: endpoint {
- remote-endpoint = <&lvds_encoder>;
+Two ports:
+ panel {
+ compatible = "advantech,idk-2121wr", "panel-lvds";
+
+ width-mm = <476>;
+ height-mm = <268>;
+
+ data-mapping = "vesa-24";
+
+ panel-timing {
+ clock-frequency = <148500000>;
+ hactive = <1920>;
+ vactive = <1080>;
+ hsync-len = <44>;
+ hfront-porch = <88>;
+ hback-porch = <148>;
+ vfront-porch = <4>;
+ vback-porch = <36>;
+ vsync-len = <5>;
+ };
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+ lvds0_panel_in: endpoint {
+ remote-endpoint = <&lvds0_out>;
+ };
+ };
+
+ port@1 {
+ reg = <1>;
+ lvds1_panel_in: endpoint {
+ remote-endpoint = <&lvds1_out>;
+ };
+ };
};
};
-};
--
2.7.4
^ permalink raw reply related
* [PATCH/RFC 02/12] dt-bindings: display: renesas: lvds: Document renesas,swap-data
From: Fabrizio Castro @ 2019-08-02 7:33 UTC (permalink / raw)
To: Laurent Pinchart, Kieran Bingham, Jacopo Mondi, David Airlie,
Daniel Vetter, Rob Herring, Mark Rutland
Cc: Fabrizio Castro, dri-devel, linux-renesas-soc, devicetree,
linux-kernel, Simon Horman, Geert Uytterhoeven, Chris Paterson,
Biju Das
In-Reply-To: <1564731249-22671-1-git-send-email-fabrizio.castro@bp.renesas.com>
R-Car D3, R-Car E3, and RZ/G2E support dual-link mode.
In such a mode, the first LVDS encoder emits even data, and the
second LVDS encoder emits odd data. This patch documents property
renesas,swap-data, used to swap even and odd data around.
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
---
Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt b/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt
index dece79e..8980179 100644
--- a/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt
+++ b/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt
@@ -52,6 +52,11 @@ Optional properties:
mandatory for the first LVDS encoder on R-Car D3, R-Car E3, and RZ/G2E SoCs,
and shall point to the second encoder to be used as a companion in dual-link
mode. It shall not be set for any other LVDS encoder.
+- renesas,swap-data : when in dual-link mode, the first LVDS encoder normally
+ emits even data, and the second LVDS encoder emits odd data. When property
+ renesas,swap-data is specified, the data emitted by the two encoders will be
+ swapped around. This property can only be used in conjunction with property
+ renesas,companion.
Example:
--
2.7.4
^ permalink raw reply related
* [PATCH/RFC 01/12] dt-bindings: display: renesas: lvds: RZ/G2E needs renesas,companion too
From: Fabrizio Castro @ 2019-08-02 7:33 UTC (permalink / raw)
To: Laurent Pinchart, Kieran Bingham, Jacopo Mondi, David Airlie,
Daniel Vetter, Rob Herring, Mark Rutland
Cc: Fabrizio Castro, dri-devel, linux-renesas-soc, devicetree,
linux-kernel, Simon Horman, Geert Uytterhoeven, Chris Paterson,
Biju Das
In-Reply-To: <1564731249-22671-1-git-send-email-fabrizio.castro@bp.renesas.com>
Document RZ/G2E support for property renesas,companion.
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
---
Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt b/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt
index c6a196d..dece79e 100644
--- a/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt
+++ b/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt
@@ -49,9 +49,9 @@ Each port shall have a single endpoint.
Optional properties:
- renesas,companion : phandle to the companion LVDS encoder. This property is
- mandatory for the first LVDS encoder on D3 and E3 SoCs, and shall point to
- the second encoder to be used as a companion in dual-link mode. It shall not
- be set for any other LVDS encoder.
+ mandatory for the first LVDS encoder on R-Car D3, R-Car E3, and RZ/G2E SoCs,
+ and shall point to the second encoder to be used as a companion in dual-link
+ mode. It shall not be set for any other LVDS encoder.
Example:
--
2.7.4
^ permalink raw reply related
* [PATCH/RFC 00/12] Add dual-LVDS panel support to EK874
From: Fabrizio Castro @ 2019-08-02 7:33 UTC (permalink / raw)
To: Geert Uytterhoeven, Laurent Pinchart, Kieran Bingham,
Jacopo Mondi, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Thierry Reding
Cc: Fabrizio Castro, Sam Ravnborg, Simon Horman, Magnus Damm,
dri-devel, linux-renesas-soc, devicetree, Chris Paterson,
Biju Das, linux-kernel, ebiharaml
Dear All,
this series adds support for dual-LVDS panel IDK-2121WR
from Advantech:
https://buy.advantech.eu/Displays/Embedded-LCD-Kits-High-Brightness/model-IDK-2121WR-K2FHA2E.htm
Dual link support is very recent for R-Car Gen3, and I couldn't
find much on dual link panels in the kernel either, therefore
comments are very welcome to get this right.
The panel doesn't come with the EK874 kit, but it's advertised as
supported, therefore this series adds a new dts file to support
the configuration of the EK874 + IDK-2121WR.
Finally, this series depends on a fix that's still pending:
https://patchwork.kernel.org/patch/11054755/
Thanks,
Fab
Fabrizio Castro (12):
dt-bindings: display: renesas: lvds: RZ/G2E needs renesas,companion
too
dt-bindings: display: renesas: lvds: Document renesas,swap-data
dt-bindings: panel: lvds: Add dual-link LVDS display support
dt-bindings: display: Add bindings for Advantech IDK-2121WR
drm: rcar-du: lvds: Add data swap support
drm: rcar-du: lvds: Do not look at ports for identifying bridges
drm: rcar-du: lvds: Add support for dual link panels
drm: rcar-du: lvds: Fix bridge_to_rcar_lvds
drm: rcar-du: lvds: Fix companion's mode
arm64: dts: renesas: r8a774c0: Point LVDS0 to its companion LVDS1
arm64: dts: renesas: cat874: Add definition for 12V regulator
arm64: dts: renesas: Add EK874 board with idk-2121wr display support
.../bindings/display/bridge/renesas,lvds.txt | 11 +-
.../display/panel/advantech,idk-2121wr.txt | 62 ++++++++++++
.../bindings/display/panel/panel-lvds.txt | 91 ++++++++++++-----
arch/arm64/boot/dts/renesas/Makefile | 3 +-
arch/arm64/boot/dts/renesas/r8a774c0-cat874.dts | 9 ++
.../boot/dts/renesas/r8a774c0-ek874-idk-2121wr.dts | 112 +++++++++++++++++++++
arch/arm64/boot/dts/renesas/r8a774c0.dtsi | 2 +
drivers/gpu/drm/rcar-du/rcar_lvds.c | 65 ++++++------
8 files changed, 291 insertions(+), 64 deletions(-)
create mode 100644 Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.txt
create mode 100644 arch/arm64/boot/dts/renesas/r8a774c0-ek874-idk-2121wr.dts
--
2.7.4
^ permalink raw reply
* Re: [PATCH 1/4] dt-bindings: counter: new bindings for TI eQEP
From: William Breathitt Gray @ 2019-08-02 7:25 UTC (permalink / raw)
To: Rob Herring
Cc: David Lechner, linux-iio, linux-omap, devicetree, Mark Rutland,
Jonathan Cameron, Benoît Cousson, Tony Lindgren,
Thierry Reding, linux-kernel, linux-pwm
In-Reply-To: <20190727204836.1514265d@archlinux>
On Sat, Jul 27, 2019 at 08:48:36PM +0100, Jonathan Cameron wrote:
> On Mon, 22 Jul 2019 10:45:35 -0500
> David Lechner <david@lechnology.com> wrote:
>
> > This documents device tree binding for the Texas Instruments Enhanced
> > Quadrature Encoder Pulse (eQEP) Module found in various TI SoCs.
> >
> > Signed-off-by: David Lechner <david@lechnology.com>
>
> Up to William given it is a counter binding, (unless Rob overrules)
> but new bindings are generally preferred as yaml.
>
> Content looks fine to me.
>
> Thanks,
>
> Jonathan
Rob,
Would you prefer these bindings as yaml, or shall I accept them as they
are now?
William Breathitt Gray
>
> > ---
> > .../devicetree/bindings/counter/ti-eqep.txt | 18 ++++++++++++++++++
> > 1 file changed, 18 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/counter/ti-eqep.txt
> >
> > diff --git a/Documentation/devicetree/bindings/counter/ti-eqep.txt b/Documentation/devicetree/bindings/counter/ti-eqep.txt
> > new file mode 100644
> > index 000000000000..fbcebc2c2cc2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/counter/ti-eqep.txt
> > @@ -0,0 +1,18 @@
> > +Texas Instruments Enhanced Quadrature Encoder Pulse (eQEP) Module
> > +
> > +Required properties:
> > +- compatible: Must be "ti,am3352-eqep".
> > +- reg: Physical base address and size of the registers map.
> > +- clocks: Handle to the PWM's functional clock.
> > +- clock-names: Must be "fck".
> > +- interrupts: Handle to the eQEP event interrupt
> > +
> > +Example:
> > +
> > + eqep0: eqep@180 {
> > + compatible = "ti,am3352-eqep";
> > + reg = <0x180 0x80>;
> > + clocks = <&l4ls_gclk>;
> > + clock-names = "fck";
> > + interrupts = <79>;
> > + };
>
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: pinctrl: qcom: Add SC7180 pinctrl binding
From: Rajendra Nayak @ 2019-08-02 6:53 UTC (permalink / raw)
To: Vinod Koul
Cc: linus.walleij, bjorn.andersson, linux-arm-msm, agross, robh+dt,
linux-gpio, devicetree, linux-kernel, Jitendra Sharma,
Vivek Gautam
In-Reply-To: <20190802063317.GB12733@vkoul-mobl.Dlink>
On 8/2/2019 12:03 PM, Vinod Koul wrote:
> On 02-08-19, 09:45, Rajendra Nayak wrote:
>> From: Jitendra Sharma <shajit@codeaurora.org>
>>
>> Add the binding for the TLMM pinctrl block found in the SC7180 platform
>>
>> Signed-off-by: Jitendra Sharma <shajit@codeaurora.org>
>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
>> [rnayak: Fix some copy-paste issues, sort and fix functions]
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>> ---
>
> changes since v1: ..?
>
>> +- reg-names:
>> + Usage: required
>> + Value type: <prop-encoded-array>
>> + Defintiion: names for the cells of reg, must contain "north", "south"
>
> s/Defintiion/Definition
>
>> +Example:
>> +
>> + tlmm: pinctrl@3000000 {
>
> this should be: pinctrl@3500000
>
> with these two nitpicks fixed:
Thanks Vinod for the review. I will fix these and respin, after I wait
a while to see if there is any more feedback :)
>
> Reviewed-by: Vinod Koul <vkoul@kernel.org>
>
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply
* Re: [PATCH v9 0/7] Solve postboot supplier cleanup and optimize probe ordering
From: Greg Kroah-Hartman @ 2019-08-02 6:37 UTC (permalink / raw)
To: Frank Rowand
Cc: Saravana Kannan, Rob Herring, Mark Rutland, Rafael J. Wysocki,
devicetree, linux-kernel, David Collins, kernel-team
In-Reply-To: <6366cb2a-65ea-cb44-f765-f246f3fb3bf9@gmail.com>
On Thu, Aug 01, 2019 at 12:59:25PM -0700, Frank Rowand wrote:
> On 8/1/19 12:32 PM, Greg Kroah-Hartman wrote:
> > On Thu, Aug 01, 2019 at 12:28:13PM -0700, Frank Rowand wrote:
> >> Hi Greg,
> >>
> >> On 7/31/19 11:12 PM, Greg Kroah-Hartman wrote:
> >>> On Wed, Jul 31, 2019 at 03:17:13PM -0700, Saravana Kannan wrote:
> >>>> Add device-links to track functional dependencies between devices
> >>>> after they are created (but before they are probed) by looking at
> >>>> their common DT bindings like clocks, interconnects, etc.
> >>>>
> >>>> Having functional dependencies automatically added before the devices
> >>>> are probed, provides the following benefits:
> >>>>
> >>>> - Optimizes device probe order and avoids the useless work of
> >>>> attempting probes of devices that will not probe successfully
> >>>> (because their suppliers aren't present or haven't probed yet).
> >>>>
> >>>> For example, in a commonly available mobile SoC, registering just
> >>>> one consumer device's driver at an initcall level earlier than the
> >>>> supplier device's driver causes 11 failed probe attempts before the
> >>>> consumer device probes successfully. This was with a kernel with all
> >>>> the drivers statically compiled in. This problem gets a lot worse if
> >>>> all the drivers are loaded as modules without direct symbol
> >>>> dependencies.
> >>>>
> >>>> - Supplier devices like clock providers, interconnect providers, etc
> >>>> need to keep the resources they provide active and at a particular
> >>>> state(s) during boot up even if their current set of consumers don't
> >>>> request the resource to be active. This is because the rest of the
> >>>> consumers might not have probed yet and turning off the resource
> >>>> before all the consumers have probed could lead to a hang or
> >>>> undesired user experience.
> >>>>
> >>>> Some frameworks (Eg: regulator) handle this today by turning off
> >>>> "unused" resources at late_initcall_sync and hoping all the devices
> >>>> have probed by then. This is not a valid assumption for systems with
> >>>> loadable modules. Other frameworks (Eg: clock) just don't handle
> >>>> this due to the lack of a clear signal for when they can turn off
> >>>> resources. This leads to downstream hacks to handle cases like this
> >>>> that can easily be solved in the upstream kernel.
> >>>>
> >>>> By linking devices before they are probed, we give suppliers a clear
> >>>> count of the number of dependent consumers. Once all of the
> >>>> consumers are active, the suppliers can turn off the unused
> >>>> resources without making assumptions about the number of consumers.
> >>>>
> >>>> By default we just add device-links to track "driver presence" (probe
> >>>> succeeded) of the supplier device. If any other functionality provided
> >>>> by device-links are needed, it is left to the consumer/supplier
> >>>> devices to change the link when they probe.
> >>>
> >>> All now queued up in my driver-core-testing branch, and if 0-day is
> >>> happy with this, will move it to my "real" driver-core-next branch in a
> >>> day or so to get included in linux-next.
> >>
> >> I have been slow in getting my review out.
> >>
> >> This patch series is not yet ready for sending to Linus, so if putting
> >> this in linux-next implies that it will be in your next pull request
> >> to Linus, please do not put it in linux-next.
> >
> > It means that it will be in my pull request for 5.4-rc1, many many
> > waeeks away from now.
>
> If you are willing to revert the series before the pull request _if_ I
> have significant review issues in the next couple of days, then I am happy
> to see the patches get exposure in linux-next.
If you have significant review issues, yes, I will be glad to revert them.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v2 2/2] pinctrl: qcom: Add SC7180 pinctrl driver
From: Vinod Koul @ 2019-08-02 6:34 UTC (permalink / raw)
To: Rajendra Nayak
Cc: linus.walleij, bjorn.andersson, linux-arm-msm, agross, robh+dt,
linux-gpio, devicetree, linux-kernel, Jitendra Sharma,
Vivek Gautam
In-Reply-To: <20190802041507.12365-2-rnayak@codeaurora.org>
On 02-08-19, 09:45, Rajendra Nayak wrote:
> From: Jitendra Sharma <shajit@codeaurora.org>
>
> Add initial pinctrl driver to support pin configuration with
> pinctrl framework for SC7180
Reviewed-by: Vinod Koul <vkoul@kernel.org>
--
~Vinod
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: pinctrl: qcom: Add SC7180 pinctrl binding
From: Vinod Koul @ 2019-08-02 6:33 UTC (permalink / raw)
To: Rajendra Nayak
Cc: linus.walleij, bjorn.andersson, linux-arm-msm, agross, robh+dt,
linux-gpio, devicetree, linux-kernel, Jitendra Sharma,
Vivek Gautam
In-Reply-To: <20190802041507.12365-1-rnayak@codeaurora.org>
On 02-08-19, 09:45, Rajendra Nayak wrote:
> From: Jitendra Sharma <shajit@codeaurora.org>
>
> Add the binding for the TLMM pinctrl block found in the SC7180 platform
>
> Signed-off-by: Jitendra Sharma <shajit@codeaurora.org>
> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
> [rnayak: Fix some copy-paste issues, sort and fix functions]
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
changes since v1: ..?
> +- reg-names:
> + Usage: required
> + Value type: <prop-encoded-array>
> + Defintiion: names for the cells of reg, must contain "north", "south"
s/Defintiion/Definition
> +Example:
> +
> + tlmm: pinctrl@3000000 {
this should be: pinctrl@3500000
with these two nitpicks fixed:
Reviewed-by: Vinod Koul <vkoul@kernel.org>
--
~Vinod
^ permalink raw reply
* Re: [PATCH] dt-bindings: Add pxe1610 as a trivial device
From: Joel Stanley @ 2019-08-02 6:31 UTC (permalink / raw)
To: Vijay Khemka
Cc: Rob Herring, Mark Rutland, Jiri Kosina, Guenter Roeck, Herbert Xu,
Patrick Venture, Ard Biesheuvel, Anson Huang, Jeremy Gebben,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
openbmc @ lists . ozlabs . org, linux-aspeed@lists.ozlabs.org,
Sai Dasari
In-Reply-To: <F7BAC53E-925E-4FA4-815D-ACB82DF8D240@fb.com>
Add pxe1610 as a trivial device
On Tue, 23 Jul 2019 at 17:14, Vijay Khemka <vijaykhemka@fb.com> wrote:
>
> On 7/23/19, 7:53 AM, "Rob Herring" <robh+dt@kernel.org> wrote:
>
> On Tue, Jul 23, 2019 at 8:50 AM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Mon, Jul 22, 2019 at 6:46 PM Vijay Khemka <vijaykhemka@fb.com> wrote:
> > >
> > > The pxe1610 is a voltage regulator from Infineon. It also supports
> > > other VRs pxe1110 and pxm1310 from Infineon.
>
> What happened to the other compatibles? S/w doesn't need to know the
> differences?
> As far as driver is concerned, it doesn't need to know differences.
You have these three IDs in the driver:
pxm1310
pxm1310
pxe1610
So all three could be listed in the documentation?
Rob, is this what you wanted Vijay to do?
^ permalink raw reply
* Re: [PATCH 0/3] ARM: dts: aspeed: Deprecate g[45]-style compatibles
From: Joel Stanley @ 2019-08-02 6:15 UTC (permalink / raw)
To: Andrew Jeffery
Cc: Linus Walleij, linux-aspeed, Lee Jones, Rob Herring, Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, linux-kernel@vger.kernel.org, open list:GPIO SUBSYSTEM
In-Reply-To: <3691f6cb-2451-43f7-9f00-d5693071ba59@www.fastmail.com>
On Thu, 1 Aug 2019 at 05:45, Andrew Jeffery <andrew@aj.id.au> wrote:
>
>
>
> On Tue, 30 Jul 2019, at 10:27, Andrew Jeffery wrote:
> >
> >
> > On Tue, 30 Jul 2019, at 07:23, Linus Walleij wrote:
> > > On Wed, Jul 24, 2019 at 10:13 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> > >
> > > > It's probably best if we push the three patches all through one tree rather
> > > > than fragmenting. Is everyone happy if Joel applies them to the aspeed tree?
> > >
> > > If you are sure it will not collide with parallell work in the
> > > pinctrl tree, yes.
> > > Acked-by: Linus Walleij <linus.walleij@linaro.org>
> > >
> > > (If it does collide I'd prefer to take the pinctrl patches and fix the
> > > conflicts in my tree.)
> >
> > Fair enough, I don't know the answer so I'll poke around. I don't
> > really mind
> > where the series goes in, I just want to avoid landing only part of it
> > if I split it up.
>
> Okay, it currently conflicts with my cleanup-devicetree-warnings series.
>
> Joel, do you mind if Linus takes this series through the pinctrl tree, given
> the fix to the devicetrees is patch 1/3?
It depends if you plan more changes to that part of the device tree
this merge window :)
Linus, perhaps the safer option is for me to take 1/3 through my tree
and you can take the rest through yours?
Cheers,
Joel
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox