Re: [PATCH 08/13] media: qcom: camss: csid: Move common code into csid core

[Bryan O'Donoghue]

On 12/08/2024 15:41, Depeng Shao wrote:
> The get hw version and src pad code functions can be common code in csid
> core file, then the csid driver of different hw version can reuse them,
> rather than adding duplicate code in csid driver for each version.
>
> Suggested-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
> Signed-off-by: Depeng Shao <quic_depengs@xxxxxxxxxxx>
> ---
>   .../platform/qcom/camss/camss-csid-4-1.c      | 19 -----
>   .../platform/qcom/camss/camss-csid-4-7.c      | 42 ----------
>   .../platform/qcom/camss/camss-csid-gen2.c     | 60 ---------------
>   .../media/platform/qcom/camss/camss-csid.c    | 77 +++++++++++++++++++
>   .../media/platform/qcom/camss/camss-csid.h    | 21 +++++
>   5 files changed, 98 insertions(+), 121 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/camss/camss-csid-4-1.c b/drivers/media/platform/qcom/camss/camss-csid-4-1.c
> index c95861420502..6998e1c52895 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid-4-1.c
> +++ b/drivers/media/platform/qcom/camss/camss-csid-4-1.c
> @@ -17,7 +17,6 @@
>   #include "camss-csid-gen1.h"
>   #include "camss.h"
>   
> -#define CAMSS_CSID_HW_VERSION		0x0
>   #define CAMSS_CSID_CORE_CTRL_0		0x004
>   #define CAMSS_CSID_CORE_CTRL_1		0x008
>   #define CAMSS_CSID_RST_CMD		0x00c
> @@ -139,15 +138,6 @@ static int csid_configure_testgen_pattern(struct csid_device *csid, s32 val)
>   	return 0;
>   }
>   
> -static u32 csid_hw_version(struct csid_device *csid)
> -{
> -	u32 hw_version = readl_relaxed(csid->base + CAMSS_CSID_HW_VERSION);
> -
> -	dev_dbg(csid->camss->dev, "CSID HW Version = 0x%08x\n", hw_version);
> -
> -	return hw_version;
> -}
>   
> -static u32 csid_hw_version(struct csid_device *csid)
> -{
> -	u32 hw_version = readl_relaxed(csid->base + CAMSS_CSID_HW_VERSION);
> -
> -	dev_dbg(csid->camss->dev, "CSID HW Version = 0x%08x\n", hw_version);
> -
> -	return hw_version;
> -}

Is it also the case with csid-4-1 and csid-47 that the HW version is in 
the format x.x.x - because you're removing this printout which just 
prints the register in favour of a later function that decodes that 
register into three parts.

Suggest having a csid_hw_version_gen1() for these two and a 
csid_hw_version_gen2() which does the x.x.x print instead.

=> camss-csid-4-1.c and camss-csid-4-7.c will have hw_version = 
csid_hw_version_gen1()

=> camss-csid-gen2, camss-csid-790 will have hw_version = hw_version_gen2()

---
bod