Re: [PATCH 1/6] media: qcom: camss: Add support for TFE (Spectra 340)

Konrad Dybcio
2025-04-16T14:31:39Z

On 4/16/25 2:09 PM, Loic Poulain wrote:
> Add support for TFE (Thin Front End) found in QCM2290.
>
> Signed-off-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
> ---
> drivers/media/platform/qcom/camss/Makefile | 1 +
> .../media/platform/qcom/camss/camss-vfe-340.c | 281 ++++++++++++++++++
> drivers/media/platform/qcom/camss/camss-vfe.h | 1 +
> 3 files changed, 283 insertions(+)
> create mode 100644 drivers/media/platform/qcom/camss/camss-vfe-340.c
>
> diff --git a/drivers/media/platform/qcom/camss/Makefile b/drivers/media/platform/qcom/camss/Makefile
> index d26a9c24a430..719898f5d32b 100644
> --- a/drivers/media/platform/qcom/camss/Makefile
> +++ b/drivers/media/platform/qcom/camss/Makefile
> @@ -17,6 +17,7 @@ qcom-camss-objs += \
> camss-vfe-4-7.o \
> camss-vfe-4-8.o \
> camss-vfe-17x.o \
> + camss-vfe-340.o \
> camss-vfe-480.o \
> camss-vfe-680.o \
> camss-vfe-780.o \
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe-340.c b/drivers/media/platform/qcom/camss/camss-vfe-340.c
> new file mode 100644
> index 000000000000..fc454d66e1d2
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/camss-vfe-340.c
> @@ -0,0 +1,281 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Qualcomm MSM Camera Subsystem - VFE (Video Front End) Module 340 (TFE)
> + *
> + * Copyright (c) 2025 Qualcomm Technologies, Inc.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +
> +#include "camss.h"
> +#include "camss-vfe.h"
> +
> +#define TFE_GLOBAL_RESET_CMD (0x014)
> +#define TFE_GLOBAL_RESET_CMD_CORE BIT(0)
> +
> +#define TFE_REG_UPDATE_CMD (0x02C)

Please uniformly use lowercase hex

> +
> +#define TFE_IRQ_CMD (0x030)
> +#define TFE_IRQ_CMD_CLEAR BIT(0)
> +#define TFE_IRQ_MASK_0 (0x034)
> +#define TFE_IRQ_MASK_0_RST_DONE BIT(0)
> +#define TFE_IRQ_MASK_0_BUS_WR BIT(1)
> +#define TFE_IRQ_MASK_1 (0x038)
> +#define TFE_IRQ_MASK_2 (0x03c)
> +#define TFE_IRQ_CLEAR_0 (0x040)
> +
> +#define TFE_IRQ_STATUS_0 (0x04c)
> +
> +#define BUS_REG(a) (0xa00 + (a))
> +
> +#define TFE_BUS_IRQ_MASK_0 BUS_REG(0x18)
> +#define TFE_BUS_IRQ_MASK_RUP_DONE_ALL 0x000f
> +#define TFE_BUS_IRQ_MASK_RUP_DONE(src) BIT(src)
> +#define TFE_BUS_IRQ_MASK_BUF_DONE_ALL 0xff00

You can use GENMASK to define the fields and FIELD_PREP(field, x) to
fill it out

[...]

> +static inline unsigned int __regupdate_iface(enum vfe_line_id line_id)
> +{
> + switch (line_id) {
> + case VFE_LINE_RDI0:
> + return 1;
> + case VFE_LINE_RDI1:
> + return 2;
> + case VFE_LINE_RDI2:
> + return 3;
> + case VFE_LINE_PIX:
> + default:
> + return 0;

Warning?

> + }
> +}
> +
> +static inline unsigned int __regupdate_line(unsigned int iface)
> +{
> + if (!iface)
> + return VFE_LINE_PIX;
> + if (iface < 4)
> + return VFE_LINE_RDI0 + (iface - 1);
> +
> + return VFE_LINE_NONE;
> +}
> +
> +static inline unsigned int __subgroup_line(unsigned int subgroup)
> +{
> + switch (subgroup) {
> + case 5:
> + return VFE_LINE_RDI0;
> + case 6:
> + return VFE_LINE_RDI1;
> + case 7:
> + return VFE_LINE_RDI2;
> + default:
> + return VFE_LINE_PIX;
> + }
> +
> + return VFE_LINE_NONE;
> +}

All these translations looks a little fragile.. Not sure if it's in the
scope of this series, but adding an op to do this (or a static map)
sounds reasonable

[...]

> +static void vfe_wm_start(struct vfe_device *vfe, u8 rdi, struct vfe_line *line)
> +{
> + struct v4l2_pix_format_mplane *pix =
> + &line->video_out.active_fmt.fmt.pix_mp;

weird linebreak

Konrad