[[PATCH,v1] ] media: rkisp1: Fix filter mode register configuration to handle enable and DNR bits separately

25158 diff mbox series

The current implementation in rkisp1_flt_config()
treats the arg->mode field as a boolean for enabling DNR mode,
which doesn't properly separate the filter
enable bit from the DNR mode bit
in the RKISP1_CIF_ISP_FILT_MODE register.

Update the register write to explicitly check individual bits:

Bit 0 of arg->mode controls the filter enable (RKISP1_CIF_ISP_FLT_ENA)
Bit 1 of arg->mode controls the DNR mode (RKISP1_CIF_ISP_FLT_MODE_DNR)
This ensures that the filter enable and DNR mode
can be configured independently through the mode field,

Signed-off-by: Rui Wang <rui.wang@ideasonboard.com>
---
 drivers/media/platform/rockchip/rkisp1/rkisp1-params.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Hi Rui,


This patch should be sent to the linux-media maililng list.

Please read
 https://www.kernel.org/doc/html/latest/process/submitting-patches.html

Quoting Rui Wang (2025-11-23 20:32:28)

> The current implementation in rkisp1_flt_config()
> treats the arg->mode field as a boolean for enabling DNR mode,
> which doesn't properly separate the filter
> enable bit from the DNR mode bit
> in the RKISP1_CIF_ISP_FILT_MODE register.

This looks like it could be wrapped to at least 72 characters per line
for commit messages please:

The current implementation in rkisp1_flt_config() treats the arg->mode
field as a boolean for enabling DNR mode, which doesn't properly
separate the filter enable bit from the DNR mode bit in the
RKISP1_CIF_ISP_FILT_MODE register.

> Update the register write to explicitly check individual bits:
> 
> Bit 0 of arg->mode controls the filter enable (RKISP1_CIF_ISP_FLT_ENA)
> Bit 1 of arg->mode controls the DNR mode (RKISP1_CIF_ISP_FLT_MODE_DNR)
> This ensures that the filter enable and DNR mode
> can be configured independently through the mode field,
> 
> Signed-off-by: Rui Wang <rui.wang@ideasonboard.com>
> ---
>  drivers/media/platform/rockchip/rkisp1/rkisp1-params.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> index b28f4140c8a3..077269848058 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> @@ -408,7 +408,8 @@ static void rkisp1_flt_config(struct rkisp1_params *params,
>                      arg->lum_weight);
>  
>         rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_FILT_MODE,
> -                    (arg->mode ? RKISP1_CIF_ISP_FLT_MODE_DNR : 0) |
> +                    ((arg->mode & (1 << 1)) ? RKISP1_CIF_ISP_FLT_MODE_DNR : 0) |
> +                    ((arg->mode & (1 << 0)) ? RKISP1_CIF_ISP_FLT_ENA : 0) |

In the Linux kernel we have a macro called BIT() for tis, so we should
replace 1<<1 and 1<<0 with BIT(1) and BIT(0).

But the usual practise is to give the bits names as well to improve
readbility so it should be more like

#define RKISP1_FILT_MODE_ENABLE	BIT(0)
#define RKISP1_FILT_MODE_DNR	BIT(1)

somewhere appropriate to then use here.

I wonder if this register value should be split out from the
rkisp1_write() call too if it would make it more readable... I think
it's getting long and complex...

--
Regards

Kieran

>                      RKISP1_CIF_ISP_FLT_CHROMA_V_MODE(arg->chr_v_mode) |
>                      RKISP1_CIF_ISP_FLT_CHROMA_H_MODE(arg->chr_h_mode) |
>                      RKISP1_CIF_ISP_FLT_GREEN_STAGE1(arg->grn_stage1));
> -- 
> 2.43.0
>