[PATCH 1/4] clk: rockchip: rst-rk3588: Add BIU reset

Shreeya Patel posted 4 patches 1 year, 11 months ago
There is a newer version of this series
[PATCH 1/4] clk: rockchip: rst-rk3588: Add BIU reset
Posted by Shreeya Patel 1 year, 11 months ago
Export hdmirx_biu soft reset id which is required by the hdmirx controller.

Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
---
 drivers/clk/rockchip/rst-rk3588.c               | 1 +
 include/dt-bindings/reset/rockchip,rk3588-cru.h | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/drivers/clk/rockchip/rst-rk3588.c b/drivers/clk/rockchip/rst-rk3588.c
index e855bb8d5413..c4ebc01f1c9c 100644
--- a/drivers/clk/rockchip/rst-rk3588.c
+++ b/drivers/clk/rockchip/rst-rk3588.c
@@ -577,6 +577,7 @@ static const int rk3588_register_offset[] = {
 
 	/* SOFTRST_CON59 */
 	RK3588_CRU_RESET_OFFSET(SRST_A_HDCP1_BIU, 59, 6),
+	RK3588_CRU_RESET_OFFSET(SRST_A_HDMIRX_BIU, 59, 7),
 	RK3588_CRU_RESET_OFFSET(SRST_A_VO1_BIU, 59, 8),
 	RK3588_CRU_RESET_OFFSET(SRST_H_VOP1_BIU, 59, 9),
 	RK3588_CRU_RESET_OFFSET(SRST_H_VOP1_S_BIU, 59, 10),
diff --git a/include/dt-bindings/reset/rockchip,rk3588-cru.h b/include/dt-bindings/reset/rockchip,rk3588-cru.h
index d4264db2a07f..e2fe4bd5f7f0 100644
--- a/include/dt-bindings/reset/rockchip,rk3588-cru.h
+++ b/include/dt-bindings/reset/rockchip,rk3588-cru.h
@@ -751,4 +751,6 @@
 #define SRST_P_TRNG_CHK			658
 #define SRST_TRNG_S			659
 
+#define SRST_A_HDMIRX_BIU		660
+
 #endif
-- 
2.39.2
Re: [PATCH 1/4] clk: rockchip: rst-rk3588: Add BIU reset
Posted by Krzysztof Kozlowski 1 year, 11 months ago
On 16/02/2024 10:49, Shreeya Patel wrote:
> Export hdmirx_biu soft reset id which is required by the hdmirx controller.
> 
> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
> ---
>  drivers/clk/rockchip/rst-rk3588.c               | 1 +
>  include/dt-bindings/reset/rockchip,rk3588-cru.h | 2 ++

Please run scripts/checkpatch.pl and fix reported warnings. Some
warnings can be ignored, but the code here looks like it needs a fix.
Feel free to get in touch if the warning is not clear.

Please do internal review. The internal Collabora review would tell you:
YOU MUST run checkpatch. Then you see errors, so why do you send patch
with errors to the mailing list?

Best regards,
Krzysztof
Re: [PATCH 1/4] clk: rockchip: rst-rk3588: Add BIU reset
Posted by Nicolas Dufresne 1 year, 11 months ago
Le vendredi 16 février 2024 à 11:03 +0100, Krzysztof Kozlowski a écrit :
> On 16/02/2024 10:49, Shreeya Patel wrote:
> > Export hdmirx_biu soft reset id which is required by the hdmirx controller.
> > 
> > Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
> > ---
> >  drivers/clk/rockchip/rst-rk3588.c               | 1 +
> >  include/dt-bindings/reset/rockchip,rk3588-cru.h | 2 ++
> 
> Please run scripts/checkpatch.pl and fix reported warnings. Some
> warnings can be ignored, but the code here looks like it needs a fix.
> Feel free to get in touch if the warning is not clear.
> 
> Please do internal review. The internal Collabora review would tell you:
> YOU MUST run checkpatch. Then you see errors, so why do you send patch
> with errors to the mailing list?

Mistakes helps you learn. Meanwhile, I've triggered our temporary CI which
hopefully will catch this and some more for a better v2. Shreeya, I don't think
this includes any DT automated checks, this is on you.

https://gitlab.collabora.com/linux/build-scripts/-/pipelines/85935

> 
> Best regards,
> Krzysztof
> 
> 
Re: [PATCH 1/4] clk: rockchip: rst-rk3588: Add BIU reset
Posted by Shreeya Patel 1 year, 11 months ago
On Friday, February 16, 2024 15:33 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 16/02/2024 10:49, Shreeya Patel wrote:
> > Export hdmirx_biu soft reset id which is required by the hdmirx controller.
> > 
> > Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
> > ---
> >  drivers/clk/rockchip/rst-rk3588.c               | 1 +
> >  include/dt-bindings/reset/rockchip,rk3588-cru.h | 2 ++
> 
> Please run scripts/checkpatch.pl and fix reported warnings. Some
> warnings can be ignored, but the code here looks like it needs a fix.
> Feel free to get in touch if the warning is not clear.
> 
> Please do internal review. The internal Collabora review would tell you:
> YOU MUST run checkpatch. Then you see errors, so why do you send patch
> with errors to the mailing list?
> 

I am sorry but what errors are you talking about?
I don't see any errors reported by checkpatch :-

shreeya@shreeya:~/collabora/rd/rockchip/torvalds$ ./scripts/checkpatch.pl hdmirx/0001-clk-rockchip-rst-rk3588-Add-BIU-reset.patch
WARNING: DT binding docs and includes should be a separate patch. See: Documentation/devicetree/bindings/submitting-patches.rst

total: 0 errors, 1 warnings, 13 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

hdmirx-v1-1602/0001-clk-rockchip-rst-rk3588-Add-BIU-reset.patch has style problems, please review.

I see the above warning but that looks like a false positive to me.

> Best regards,
> Krzysztof
> 
> _______________________________________________
> Kernel mailing list -- kernel@mailman.collabora.com
> To unsubscribe send an email to kernel-leave@mailman.collabora.com
> This list is managed by https://mailman.collabora.com
Re: [PATCH 1/4] clk: rockchip: rst-rk3588: Add BIU reset
Posted by Krzysztof Kozlowski 1 year, 11 months ago
On 16/02/2024 13:48, Shreeya Patel wrote:
> On Friday, February 16, 2024 15:33 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> On 16/02/2024 10:49, Shreeya Patel wrote:
>>> Export hdmirx_biu soft reset id which is required by the hdmirx controller.
>>>
>>> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
>>> ---
>>>  drivers/clk/rockchip/rst-rk3588.c               | 1 +
>>>  include/dt-bindings/reset/rockchip,rk3588-cru.h | 2 ++
>>
>> Please run scripts/checkpatch.pl and fix reported warnings. Some
>> warnings can be ignored, but the code here looks like it needs a fix.
>> Feel free to get in touch if the warning is not clear.
>>
>> Please do internal review. The internal Collabora review would tell you:
>> YOU MUST run checkpatch. Then you see errors, so why do you send patch
>> with errors to the mailing list?
>>
> 
> I am sorry but what errors are you talking about?
> I don't see any errors reported by checkpatch :-
> 
> shreeya@shreeya:~/collabora/rd/rockchip/torvalds$ ./scripts/checkpatch.pl hdmirx/0001-clk-rockchip-rst-rk3588-Add-BIU-reset.patch
> WARNING: DT binding docs and includes should be a separate patch. See: Documentation/devicetree/bindings/submitting-patches.rst

Here.

> 
> total: 0 errors, 1 warnings, 13 lines checked
> 
> NOTE: For some of the reported defects, checkpatch may be able to
>       mechanically convert to the typical style using --fix or --fix-inplace.
> 
> hdmirx-v1-1602/0001-clk-rockchip-rst-rk3588-Add-BIU-reset.patch has style problems, please review.
> 
> I see the above warning but that looks like a false positive to me.

Why for your patch it would be false positive and for all others would not?

Best regards,
Krzysztof
Re: [PATCH 1/4] clk: rockchip: rst-rk3588: Add BIU reset
Posted by Shreeya Patel 1 year, 11 months ago
On Saturday, February 17, 2024 13:47 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 16/02/2024 13:48, Shreeya Patel wrote:
> > On Friday, February 16, 2024 15:33 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> > 
> >> On 16/02/2024 10:49, Shreeya Patel wrote:
> >>> Export hdmirx_biu soft reset id which is required by the hdmirx controller.
> >>>
> >>> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
> >>> ---
> >>>  drivers/clk/rockchip/rst-rk3588.c               | 1 +
> >>>  include/dt-bindings/reset/rockchip,rk3588-cru.h | 2 ++
> >>
> >> Please run scripts/checkpatch.pl and fix reported warnings. Some
> >> warnings can be ignored, but the code here looks like it needs a fix.
> >> Feel free to get in touch if the warning is not clear.
> >>
> >> Please do internal review. The internal Collabora review would tell you:
> >> YOU MUST run checkpatch. Then you see errors, so why do you send patch
> >> with errors to the mailing list?
> >>
> > 
> > I am sorry but what errors are you talking about?
> > I don't see any errors reported by checkpatch :-
> > 
> > shreeya@shreeya:~/collabora/rd/rockchip/torvalds$ ./scripts/checkpatch.pl hdmirx/0001-clk-rockchip-rst-rk3588-Add-BIU-reset.patch
> > WARNING: DT binding docs and includes should be a separate patch. See: Documentation/devicetree/bindings/submitting-patches.rst
> 
> Here.
> 
> > 
> > total: 0 errors, 1 warnings, 13 lines checked
> > 
> > NOTE: For some of the reported defects, checkpatch may be able to
> >       mechanically convert to the typical style using --fix or --fix-inplace.
> > 
> > hdmirx-v1-1602/0001-clk-rockchip-rst-rk3588-Add-BIU-reset.patch has style problems, please review.
> > 
> > I see the above warning but that looks like a false positive to me.
> 
> Why for your patch it would be false positive and for all others would not?
> 

OK, now I see what you meant. Since we are touching the include file and C file together, this warning was generated.
It was a bit confusing to interpret this as the warning also talks about Documentation which we didn't touch at all.

Anyway, I will create two separate patches for this in v2.


Thanks,
Shreeya Patel

> Best regards,
> Krzysztof
>
Re: [PATCH 1/4] clk: rockchip: rst-rk3588: Add BIU reset
Posted by Krzysztof Kozlowski 1 year, 11 months ago
On 17/02/2024 11:18, Shreeya Patel wrote:
> On Saturday, February 17, 2024 13:47 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> On 16/02/2024 13:48, Shreeya Patel wrote:
>>> On Friday, February 16, 2024 15:33 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>>>
>>>> On 16/02/2024 10:49, Shreeya Patel wrote:
>>>>> Export hdmirx_biu soft reset id which is required by the hdmirx controller.
>>>>>
>>>>> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
>>>>> ---
>>>>>  drivers/clk/rockchip/rst-rk3588.c               | 1 +
>>>>>  include/dt-bindings/reset/rockchip,rk3588-cru.h | 2 ++
>>>>
>>>> Please run scripts/checkpatch.pl and fix reported warnings. Some
>>>> warnings can be ignored, but the code here looks like it needs a fix.
>>>> Feel free to get in touch if the warning is not clear.
>>>>
>>>> Please do internal review. The internal Collabora review would tell you:
>>>> YOU MUST run checkpatch. Then you see errors, so why do you send patch
>>>> with errors to the mailing list?
>>>>
>>>
>>> I am sorry but what errors are you talking about?
>>> I don't see any errors reported by checkpatch :-
>>>
>>> shreeya@shreeya:~/collabora/rd/rockchip/torvalds$ ./scripts/checkpatch.pl hdmirx/0001-clk-rockchip-rst-rk3588-Add-BIU-reset.patch
>>> WARNING: DT binding docs and includes should be a separate patch. See: Documentation/devicetree/bindings/submitting-patches.rst
>>
>> Here.
>>
>>>
>>> total: 0 errors, 1 warnings, 13 lines checked
>>>
>>> NOTE: For some of the reported defects, checkpatch may be able to
>>>       mechanically convert to the typical style using --fix or --fix-inplace.
>>>
>>> hdmirx-v1-1602/0001-clk-rockchip-rst-rk3588-Add-BIU-reset.patch has style problems, please review.
>>>
>>> I see the above warning but that looks like a false positive to me.
>>
>> Why for your patch it would be false positive and for all others would not?
>>
> 
> OK, now I see what you meant. Since we are touching the include file and C file together, this warning was generated.
> It was a bit confusing to interpret this as the warning also talks about Documentation which we didn't touch at all.

Really, no documentation touching? Care to check the full path of the
files you are changing?


> 
> Anyway, I will create two separate patches for this in v2.

I think rules cannot be clearer:
https://elixir.bootlin.com/linux/v6.8-rc4/source/Documentation/devicetree/bindings/submitting-patches.rst#L13


Best regards,
Krzysztof
Re: [PATCH 1/4] clk: rockchip: rst-rk3588: Add BIU reset
Posted by Shreeya Patel 1 year, 11 months ago
On Saturday, February 17, 2024 15:50 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 17/02/2024 11:18, Shreeya Patel wrote:
> > On Saturday, February 17, 2024 13:47 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> > 
> >> On 16/02/2024 13:48, Shreeya Patel wrote:
> >>> On Friday, February 16, 2024 15:33 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> >>>
> >>>> On 16/02/2024 10:49, Shreeya Patel wrote:
> >>>>> Export hdmirx_biu soft reset id which is required by the hdmirx controller.
> >>>>>
> >>>>> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
> >>>>> ---
> >>>>>  drivers/clk/rockchip/rst-rk3588.c               | 1 +
> >>>>>  include/dt-bindings/reset/rockchip,rk3588-cru.h | 2 ++
> >>>>
> >>>> Please run scripts/checkpatch.pl and fix reported warnings. Some
> >>>> warnings can be ignored, but the code here looks like it needs a fix.
> >>>> Feel free to get in touch if the warning is not clear.
> >>>>
> >>>> Please do internal review. The internal Collabora review would tell you:
> >>>> YOU MUST run checkpatch. Then you see errors, so why do you send patch
> >>>> with errors to the mailing list?
> >>>>
> >>>
> >>> I am sorry but what errors are you talking about?
> >>> I don't see any errors reported by checkpatch :-
> >>>
> >>> shreeya@shreeya:~/collabora/rd/rockchip/torvalds$ ./scripts/checkpatch.pl hdmirx/0001-clk-rockchip-rst-rk3588-Add-BIU-reset.patch
> >>> WARNING: DT binding docs and includes should be a separate patch. See: Documentation/devicetree/bindings/submitting-patches.rst
> >>
> >> Here.
> >>
> >>>
> >>> total: 0 errors, 1 warnings, 13 lines checked
> >>>
> >>> NOTE: For some of the reported defects, checkpatch may be able to
> >>>       mechanically convert to the typical style using --fix or --fix-inplace.
> >>>
> >>> hdmirx-v1-1602/0001-clk-rockchip-rst-rk3588-Add-BIU-reset.patch has style problems, please review.
> >>>
> >>> I see the above warning but that looks like a false positive to me.
> >>
> >> Why for your patch it would be false positive and for all others would not?
> >>
> > 
> > OK, now I see what you meant. Since we are touching the include file and C file together, this warning was generated.
> > It was a bit confusing to interpret this as the warning also talks about Documentation which we didn't touch at all.
> 
> Really, no documentation touching? Care to check the full path of the
> files you are changing?
> 

Well, I meant the dt-binding doc for rst-rk3588

> 
> > 
> > Anyway, I will create two separate patches for this in v2.
> 
> I think rules cannot be clearer:
> https://elixir.bootlin.com/linux/v6.8-rc4/source/Documentation/devicetree/bindings/submitting-patches.rst#L13
> 

right, it was my mistake and I think after this nice and kind feedback, I'll never make the same one again :)

Thanks,
Shreeya Patel

> 
> Best regards,
> Krzysztof
> 
> _______________________________________________
> Kernel mailing list -- kernel@mailman.collabora.com
> To unsubscribe send an email to kernel-leave@mailman.collabora.com
> This list is managed by https://mailman.collabora.com