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

[Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>]

On 8/15/24 02:53, Bryan O'Donoghue wrote:
> 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@linaro.org>
>> Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>
>> ---
>>    .../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()
> 

I believe here two different output formats is unnecessary, let's just stick
to one or another version (not important which one) for all platforms.

In any case for sake of simplicity there should be just one generic function,
and, if for whatever reason it is necessary to print out different formats,
this shall be selected in runtime within a single shared function.

FWIW, I'm quite happy with the current version.

Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>

--
Best wishes,
Vladimir