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

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

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