2024-03-20 14:15:05

by Depeng Shao

[permalink] [raw]
Subject: [PATCH v2 4/8] media: qcom: camss: Add new params for csid_device

CSID gen3 has a new register block which is named as
CSID top, it controls the output of CSID, since the
CSID can connect to SFE or original VFE in CSID gen3.
The register update is moved to CSID from VFE in CSID
gen3.
So, adding top_base and reg_update variables in csid
device structure for CSID gen3.

Signed-off-by: Depeng Shao <[email protected]>
---
drivers/media/platform/qcom/camss/camss-csid.h | 2 ++
1 file changed, 2 insertions(+)

diff --git a/drivers/media/platform/qcom/camss/camss-csid.h b/drivers/media/platform/qcom/camss/camss-csid.h
index 4a9e5a2d1f92..ca654b007441 100644
--- a/drivers/media/platform/qcom/camss/camss-csid.h
+++ b/drivers/media/platform/qcom/camss/camss-csid.h
@@ -162,7 +162,9 @@ struct csid_device {
struct v4l2_subdev subdev;
struct media_pad pads[MSM_CSID_PADS_NUM];
void __iomem *base;
+ void __iomem *top_base;
u32 irq;
+ u32 reg_update;
char irq_name[30];
struct camss_clock *clock;
int nclocks;
--
2.17.1



2024-03-20 15:27:37

by Bryan O'Donoghue

[permalink] [raw]
Subject: Re: [PATCH v2 4/8] media: qcom: camss: Add new params for csid_device

On 20/03/2024 14:11, Depeng Shao wrote:
> CSID gen3 has a new register block which is named as
> CSID top, it controls the output of CSID, since the
> CSID can connect to SFE or original VFE in CSID gen3.
> The register update is moved to CSID from VFE in CSID
> gen3.
> So, adding top_base and reg_update variables in csid
> device structure for CSID gen3.

You need to define three letter acronyms (TLAs) - within reason - the
first time you use them.

There is no concept of an "SFE" upstream - please define what the SFE is
in the commit log "to the Sensor Front End (SFE)" then you can refer
back to the acronym liberally.

---
bod

2024-03-20 15:53:19

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 4/8] media: qcom: camss: Add new params for csid_device

On 20/03/2024 15:11, Depeng Shao wrote:
> CSID gen3 has a new register block which is named as
> CSID top, it controls the output of CSID, since the
> CSID can connect to SFE or original VFE in CSID gen3.
> The register update is moved to CSID from VFE in CSID
> gen3.
> So, adding top_base and reg_update variables in csid
> device structure for CSID gen3.

Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597

>
> Signed-off-by: Depeng Shao <[email protected]>
> ---
> drivers/media/platform/qcom/camss/camss-csid.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/media/platform/qcom/camss/camss-csid.h b/drivers/media/platform/qcom/camss/camss-csid.h
> index 4a9e5a2d1f92..ca654b007441 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid.h
> +++ b/drivers/media/platform/qcom/camss/camss-csid.h
> @@ -162,7 +162,9 @@ struct csid_device {
> struct v4l2_subdev subdev;
> struct media_pad pads[MSM_CSID_PADS_NUM];
> void __iomem *base;
> + void __iomem *top_base;
> u32 irq;
> + u32 reg_update;
> char irq_name[30];

This is pointless. The are no users of this!

Sorry, don't add random code here or there without concept.

Best regards,
Krzysztof


2024-03-25 15:13:31

by Depeng Shao

[permalink] [raw]
Subject: Re: [PATCH v2 4/8] media: qcom: camss: Add new params for csid_device

Hi Krzysztof,

On 3/20/2024 11:53 PM, Krzysztof Kozlowski wrote:
> On 20/03/2024 15:11, Depeng Shao wrote:
>> CSID gen3 has a new register block which is named as
>> CSID top, it controls the output of CSID, since the
>> CSID can connect to SFE or original VFE in CSID gen3.
>> The register update is moved to CSID from VFE in CSID
>> gen3.
>> So, adding top_base and reg_update variables in csid
>> device structure for CSID gen3.
>
> Please wrap commit message according to Linux coding style / submission
> process (neither too early nor over the limit):
> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
>

Thanks, will update it.

>>
>> Signed-off-by: Depeng Shao <[email protected]>
>> ---
>> drivers/media/platform/qcom/camss/camss-csid.h | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/media/platform/qcom/camss/camss-csid.h b/drivers/media/platform/qcom/camss/camss-csid.h
>> index 4a9e5a2d1f92..ca654b007441 100644
>> --- a/drivers/media/platform/qcom/camss/camss-csid.h
>> +++ b/drivers/media/platform/qcom/camss/camss-csid.h
>> @@ -162,7 +162,9 @@ struct csid_device {
>> struct v4l2_subdev subdev;
>> struct media_pad pads[MSM_CSID_PADS_NUM];
>> void __iomem *base;
>> + void __iomem *top_base;
>> u32 irq;
>> + u32 reg_update;
>> char irq_name[30];
>
> This is pointless. The are no users of this!
>
> Sorry, don't add random code here or there without concept.
>

For the old code, they are new concept, since it is new block in the
hardware, I just want to highlight them.

But thanks for the comment, will update the code based on your comment.

> Best regards,
> Krzysztof
>

Thanks,
Depeng