Re: [PATCH v1 01/11] media: dt-bindings: starfive,jh7110-camss: add binding document

Re: [PATCH v1 01/11] media: dt-bindings: starfive,jh7110-camss: add binding document

[Krzysztof Kozlowski]

On 02/03/2023 10:19, jack.zhu wrote:
> Add DT binding document for Starfive Camera subsystem driver
> 
> Signed-off-by: jack.zhu <jack.zhu@starfivetech.com>
> ---
>  .../bindings/media/starfive,jh7110-camss.yaml | 150 ++++++++++++++++++
>  1 file changed, 150 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml b/Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml
> new file mode 100644
> index 000000000000..9a34944ca0ab
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml
> @@ -0,0 +1,150 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/media/starfive,jh7110-camss.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"

Drop quotes from both.

> +
> +title: Starfive SoC CAMSS ISP
> +
> +maintainers:
> +  - Jack Zhu <jack.zhu@starfivetech.com>
> +  - Changhuang Liang <changhuang.liang@starfivetech.com>
> +
> +description: |

No need for '|'

> +  The Starfive CAMSS ISP is a Camera interface for Starfive JH7110 SoC.It
> +  consists of a VIN controller(Video In Controller, a top-level control until)
> +  and a ISP.

"an ISP", I think

> +
> +properties:
> +  compatible:
> +    const: starfive,jh7110-camss
> +
> +  reg:
> +    minItems: 2

Drop minItems, no need.

> +    maxItems: 2
> +
> +  reg-names:
> +    items:
> +      - const: syscon
> +      - const: isp
> +
> +  clocks:
> +    minItems: 7

Drop mintems

> +    maxItems: 7
> +
> +  clock-names:
> +    items:
> +      - const: clk_apb_func
> +      - const: clk_wrapper_clk_c
> +      - const: clk_dvp_inv
> +      - const: clk_axiwr
> +      - const: clk_mipi_rx0_pxl
> +      - const: clk_ispcore_2x
> +      - const: clk_isp_axi

Drop "clk" prefix

> +
> +  resets:
> +    minItems: 6

Drop

> +    maxItems: 6
> +
> +  reset-names:
> +    items:
> +      - const: rst_wrapper_p

Drop rst prefix

> +      - const: rst_wrapper_c
> +      - const: rst_axird
> +      - const: rst_axiwr
> +      - const: rst_isp_top_n
> +      - const: rst_isp_top_axi
> +
> +  power-domains:
> +    items:
> +      - description: JH7110 PD ISP - ISP Power Domain Switch Controller.

Drop redundant pieces, e.g. "PD ISP"

> +
> +  interrupts:
> +    minItems: 4

Drop

> +    maxItems: 4
> +
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +
> +    properties:
> +      port@1:

And what about port@0?

> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        unevaluatedProperties: false
> +        description:
> +          Input port for receiving CSI data.
> +
> +        properties:
> +          endpoint@1:

Hm, do you have more than one endpoint in this port? Why unit address?

> +            $ref: video-interfaces.yaml#
> +            unevaluatedProperties: false
> +
> +    required:
> +      - port@1
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - clocks
> +  - clock-names
> +  - resets
> +  - reset-names
> +  - power-domains
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +

Drop blank line

> +    stfcamss: camss@19840000 {

isp@

> +        compatible = "starfive,jh7110-camss";
> +        reg = <0x19840000 0x10000>,
> +            <0x19870000 0x30000>;

All this looks misaligned
> +        reg-names = "syscon", "isp";
> +        clocks = <&ispcrg 0>,
> +            <&ispcrg 13>,

Looks even worse...

> +            <&ispcrg 2>,
> +            <&ispcrg 12>,
> +            <&ispcrg 1>,
> +            <&syscrg 51>,
> +            <&syscrg 52>;
> +        clock-names = "clk_apb_func",
> +            "clk_wrapper_clk_c",
> +            "clk_dvp_inv",
> +            "clk_axiwr",
> +            "clk_mipi_rx0_pxl",
> +            "clk_ispcore_2x",
> +            "clk_isp_axi";
> +        resets = <&ispcrg 0>,
> +            <&ispcrg 1>,
> +            <&ispcrg 10>,
> +            <&ispcrg 11>,
> +            <&syscrg 41>,
> +            <&syscrg 42>;
> +        reset-names = "rst_wrapper_p",
> +            "rst_wrapper_c",
> +            "rst_axird",
> +            "rst_axiwr",
> +            "rst_isp_top_n",
> +            "rst_isp_top_axi";
> +        power-domains = <&pwrc 5>;
> +        interrupts = <92>, <87>, <88>, <90>;
> +
> +        ports {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            port@1 {
> +                reg = <1>;
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +
> +                vin_from_csi2rx: endpoint@1 {
> +                    reg = <1>;
> +                    remote-endpoint = <&csi2rx_to_vin>;
> +                };
> +            };
> +        };
> +    };

Best regards,
Krzysztof

Re: [PATCH v1 01/11] media: dt-bindings: starfive,jh7110-camss: add binding document

[Jack Zhu]

On 2023/3/3 16:34, Krzysztof Kozlowski wrote:
> On 02/03/2023 10:19, jack.zhu wrote:
>> Add DT binding document for Starfive Camera subsystem driver
>> 
>> Signed-off-by: jack.zhu <jack.zhu@starfivetech.com>
>> ---
>>  .../bindings/media/starfive,jh7110-camss.yaml | 150 ++++++++++++++++++
>>  1 file changed, 150 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml
>> 
>> diff --git a/Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml b/Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml
>> new file mode 100644
>> index 000000000000..9a34944ca0ab
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml
>> @@ -0,0 +1,150 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +
>> +%YAML 1.2
>> +---
>> +$id: "http://devicetree.org/schemas/media/starfive,jh7110-camss.yaml#"
>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> 
> Drop quotes from both.

OK, I will fix it in V2 version.

> 
>> +
>> +title: Starfive SoC CAMSS ISP
>> +
>> +maintainers:
>> +  - Jack Zhu <jack.zhu@starfivetech.com>
>> +  - Changhuang Liang <changhuang.liang@starfivetech.com>
>> +
>> +description: |
> 
> No need for '|'

OK, I will fix it.

> 
>> +  The Starfive CAMSS ISP is a Camera interface for Starfive JH7110 SoC.It
>> +  consists of a VIN controller(Video In Controller, a top-level control until)
>> +  and a ISP.
> 
> "an ISP", I think

I will revise it.

> 
>> +
>> +properties:
>> +  compatible:
>> +    const: starfive,jh7110-camss
>> +
>> +  reg:
>> +    minItems: 2
> 
> Drop minItems, no need.

OK, I will drop it.

> 
>> +    maxItems: 2
>> +
>> +  reg-names:
>> +    items:
>> +      - const: syscon
>> +      - const: isp
>> +
>> +  clocks:
>> +    minItems: 7
> 
> Drop mintems

I will drop it.

> 
>> +    maxItems: 7
>> +
>> +  clock-names:
>> +    items:
>> +      - const: clk_apb_func
>> +      - const: clk_wrapper_clk_c
>> +      - const: clk_dvp_inv
>> +      - const: clk_axiwr
>> +      - const: clk_mipi_rx0_pxl
>> +      - const: clk_ispcore_2x
>> +      - const: clk_isp_axi
> 
> Drop "clk" prefix

I will drop it.

> 
>> +
>> +  resets:
>> +    minItems: 6
> 
> Drop

I will drop it.

> 
>> +    maxItems: 6
>> +
>> +  reset-names:
>> +    items:
>> +      - const: rst_wrapper_p
> 
> Drop rst prefix

OK, will fix it.

> 
>> +      - const: rst_wrapper_c
>> +      - const: rst_axird
>> +      - const: rst_axiwr
>> +      - const: rst_isp_top_n
>> +      - const: rst_isp_top_axi
>> +
>> +  power-domains:
>> +    items:
>> +      - description: JH7110 PD ISP - ISP Power Domain Switch Controller.
> 
> Drop redundant pieces, e.g. "PD ISP"

OK, will fix it.

> 
>> +
>> +  interrupts:
>> +    minItems: 4
> 
> Drop

OK, will drop it.

> 
>> +    maxItems: 4
>> +
>> +  ports:
>> +    $ref: /schemas/graph.yaml#/properties/ports
>> +
>> +    properties:
>> +      port@1:
> 
> And what about port@0?

port@0 is reserved for DVP sensor, although it is not supported yet.

> 
>> +        $ref: /schemas/graph.yaml#/$defs/port-base
>> +        unevaluatedProperties: false
>> +        description:
>> +          Input port for receiving CSI data.
>> +
>> +        properties:
>> +          endpoint@1:
> 
> Hm, do you have more than one endpoint in this port? Why unit address?

OK, I will change it to "endpoint:"

> 
>> +            $ref: video-interfaces.yaml#
>> +            unevaluatedProperties: false
>> +
>> +    required:
>> +      - port@1
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - reg-names
>> +  - clocks
>> +  - clock-names
>> +  - resets
>> +  - reset-names
>> +  - power-domains
>> +  - interrupts
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +
> 
> Drop blank line

I will drop it.

> 
>> +    stfcamss: camss@19840000 {
> 
> isp@

I will alter it.

> 
>> +        compatible = "starfive,jh7110-camss";
>> +        reg = <0x19840000 0x10000>,
>> +            <0x19870000 0x30000>;
> 
> All this looks misaligned
>> +        reg-names = "syscon", "isp";
>> +        clocks = <&ispcrg 0>,
>> +            <&ispcrg 13>,
> 
> Looks even worse...

I will fix it.

> 
>> +            <&ispcrg 2>,
>> +            <&ispcrg 12>,
>> +            <&ispcrg 1>,
>> +            <&syscrg 51>,
>> +            <&syscrg 52>;
>> +        clock-names = "clk_apb_func",
>> +            "clk_wrapper_clk_c",
>> +            "clk_dvp_inv",
>> +            "clk_axiwr",
>> +            "clk_mipi_rx0_pxl",
>> +            "clk_ispcore_2x",
>> +            "clk_isp_axi";
>> +        resets = <&ispcrg 0>,
>> +            <&ispcrg 1>,
>> +            <&ispcrg 10>,
>> +            <&ispcrg 11>,
>> +            <&syscrg 41>,
>> +            <&syscrg 42>;
>> +        reset-names = "rst_wrapper_p",
>> +            "rst_wrapper_c",
>> +            "rst_axird",
>> +            "rst_axiwr",
>> +            "rst_isp_top_n",
>> +            "rst_isp_top_axi";
>> +        power-domains = <&pwrc 5>;
>> +        interrupts = <92>, <87>, <88>, <90>;
>> +
>> +        ports {
>> +            #address-cells = <1>;
>> +            #size-cells = <0>;
>> +
>> +            port@1 {
>> +                reg = <1>;
>> +                #address-cells = <1>;
>> +                #size-cells = <0>;
>> +
>> +                vin_from_csi2rx: endpoint@1 {
>> +                    reg = <1>;
>> +                    remote-endpoint = <&csi2rx_to_vin>;
>> +                };
>> +            };
>> +        };
>> +    };
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v1 01/11] media: dt-bindings: starfive,jh7110-camss: add binding document

[Krzysztof Kozlowski]

On 02/03/2023 10:19, jack.zhu wrote:
> Add DT binding document for Starfive Camera subsystem driver
> 
> Signed-off-by: jack.zhu <jack.zhu@starfivetech.com>

Also:

Subject: drop second/last, redundant "add binding document". The
"dt-bindings" prefix is already stating that these are bindings and it
is a document. Write something useful instead.

Best regards,
Krzysztof

Re: [PATCH v1 01/11] media: dt-bindings: starfive,jh7110-camss: add binding document

[Jack Zhu]

On 2023/3/3 16:35, Krzysztof Kozlowski wrote:
> On 02/03/2023 10:19, jack.zhu wrote:
>> Add DT binding document for Starfive Camera subsystem driver
>> 
>> Signed-off-by: jack.zhu <jack.zhu@starfivetech.com>
> 
> Also:
> 
> Subject: drop second/last, redundant "add binding document". The
> "dt-bindings" prefix is already stating that these are bindings and it
> is a document. Write something useful instead.

Ok, I will revise the Subject

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v1 04/11] MAINTAINERS: add Starfive Camera subsystem driver

[Krzysztof Kozlowski]

On 02/03/2023 10:19, jack.zhu wrote:
> Add an entry for Starfive Camera subsystem driver.
> 
> Signed-off-by: jack.zhu <jack.zhu@starfivetech.com>
> ---
>  MAINTAINERS | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8ddef8669efb..a202deb4cb1a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19906,6 +19906,15 @@ M:	Ion Badulescu <ionut@badula.org>
>  S:	Odd Fixes
>  F:	drivers/net/ethernet/adaptec/starfire*
>  
> +STARFIVE CAMERA SUBSYSTEM DRIVER
> +M:	Jack Zhu <jack.zhu@starfivetech.com>
> +M:	Changhuang Liang <changhuang.liang@starfivetech.com>
> +L:	linux-media@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/admin-guide/media/starfive_camss.rst
> +F:	Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml

Why only one binding, not all of them?

Best regards,
Krzysztof

Re: [PATCH v1 04/11] MAINTAINERS: add Starfive Camera subsystem driver

[Jack Zhu]

On 2023/3/3 16:42, Krzysztof Kozlowski wrote:
> On 02/03/2023 10:19, jack.zhu wrote:
>> Add an entry for Starfive Camera subsystem driver.
>> 
>> Signed-off-by: jack.zhu <jack.zhu@starfivetech.com>
>> ---
>>  MAINTAINERS | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>> 
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 8ddef8669efb..a202deb4cb1a 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -19906,6 +19906,15 @@ M:	Ion Badulescu <ionut@badula.org>
>>  S:	Odd Fixes
>>  F:	drivers/net/ethernet/adaptec/starfire*
>>  
>> +STARFIVE CAMERA SUBSYSTEM DRIVER
>> +M:	Jack Zhu <jack.zhu@starfivetech.com>
>> +M:	Changhuang Liang <changhuang.liang@starfivetech.com>
>> +L:	linux-media@vger.kernel.org
>> +S:	Maintained
>> +F:	Documentation/admin-guide/media/starfive_camss.rst
>> +F:	Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml
> 
> Why only one binding, not all of them?

As we communicated in the previous emails, The file starfive,jh7110-mipi-csi2.yaml
will no longer be used. I will convert cdns,csi2rx.txt to cdns,csi2rx.yaml and add some
new properties. The "cdns,csi2rx.yaml" will be added in "CADENCE MIPI-CSI2 BRIDGES".

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v1 11/11] media: starfive: enable building

[Krzysztof Kozlowski]

On 02/03/2023 10:19, jack.zhu wrote:
> Add Kconfig and Makefie, update platform/Kconfig and platform/Makefile
> to enable building of the Starfive Camera subsystem driver.
> 
> Signed-off-by: jack.zhu <jack.zhu@starfivetech.com>
> ---
>  drivers/media/platform/Kconfig           |  1 +
>  drivers/media/platform/Makefile          |  1 +
>  drivers/media/platform/starfive/Kconfig  | 18 ++++++++++++++++++
>  drivers/media/platform/starfive/Makefile | 14 ++++++++++++++

This is not a separate commit. If it were, it would mean you just added
dead code in previous commits, so why adding dead code in first place?

Squash it.

Best regards,
Krzysztof

Re: [PATCH v1 11/11] media: starfive: enable building

[Jack Zhu]

On 2023/3/3 16:43, Krzysztof Kozlowski wrote:
> On 02/03/2023 10:19, jack.zhu wrote:
>> Add Kconfig and Makefie, update platform/Kconfig and platform/Makefile
>> to enable building of the Starfive Camera subsystem driver.
>> 
>> Signed-off-by: jack.zhu <jack.zhu@starfivetech.com>
>> ---
>>  drivers/media/platform/Kconfig           |  1 +
>>  drivers/media/platform/Makefile          |  1 +
>>  drivers/media/platform/starfive/Kconfig  | 18 ++++++++++++++++++
>>  drivers/media/platform/starfive/Makefile | 14 ++++++++++++++
> 
> This is not a separate commit. If it were, it would mean you just added
> dead code in previous commits, so why adding dead code in first place?
> 

The previous patches are made according to the module function.I think
it is helpful to explain the composition of the code file. 

stf_camss[patch 9] as a platform device manages all resources including
ISP and VIN. ISP/VIN [patch 7/8]as a sub-device needs to access other
resources managed by stf_camss.There is mutual reference between them.
Therefore, this patch is used for the overall compilation of the starfive
directory.

> Squash it.
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v1 11/11] media: starfive: enable building

[Krzysztof Kozlowski]

On 07/03/2023 10:46, Jack Zhu wrote:
> 
> 
> On 2023/3/3 16:43, Krzysztof Kozlowski wrote:
>> On 02/03/2023 10:19, jack.zhu wrote:
>>> Add Kconfig and Makefie, update platform/Kconfig and platform/Makefile
>>> to enable building of the Starfive Camera subsystem driver.
>>>
>>> Signed-off-by: jack.zhu <jack.zhu@starfivetech.com>
>>> ---
>>>  drivers/media/platform/Kconfig           |  1 +
>>>  drivers/media/platform/Makefile          |  1 +
>>>  drivers/media/platform/starfive/Kconfig  | 18 ++++++++++++++++++
>>>  drivers/media/platform/starfive/Makefile | 14 ++++++++++++++
>>
>> This is not a separate commit. If it were, it would mean you just added
>> dead code in previous commits, so why adding dead code in first place?
>>
> 
> The previous patches are made according to the module function.I think
> it is helpful to explain the composition of the code file. 
> 
> stf_camss[patch 9] as a platform device manages all resources including
> ISP and VIN. ISP/VIN [patch 7/8]as a sub-device needs to access other
> resources managed by stf_camss.There is mutual reference between them.
> Therefore, this patch is used for the overall compilation of the starfive
> directory.

So previous code is dead? Again, what is the reason for adding dead
code? Mutual reference is not the answer.

Best regards,
Krzysztof

Re: [PATCH v1 11/11] media: starfive: enable building

[Jack Zhu]

On 2023/3/8 18:33, Krzysztof Kozlowski wrote:
> On 07/03/2023 10:46, Jack Zhu wrote:
>> 
>> 
>> On 2023/3/3 16:43, Krzysztof Kozlowski wrote:
>>> On 02/03/2023 10:19, jack.zhu wrote:
>>>> Add Kconfig and Makefie, update platform/Kconfig and platform/Makefile
>>>> to enable building of the Starfive Camera subsystem driver.
>>>>
>>>> Signed-off-by: jack.zhu <jack.zhu@starfivetech.com>
>>>> ---
>>>>  drivers/media/platform/Kconfig           |  1 +
>>>>  drivers/media/platform/Makefile          |  1 +
>>>>  drivers/media/platform/starfive/Kconfig  | 18 ++++++++++++++++++
>>>>  drivers/media/platform/starfive/Makefile | 14 ++++++++++++++
>>>
>>> This is not a separate commit. If it were, it would mean you just added
>>> dead code in previous commits, so why adding dead code in first place?
>>>
>> 
>> The previous patches are made according to the module function.I think
>> it is helpful to explain the composition of the code file. 
>> 
>> stf_camss[patch 9] as a platform device manages all resources including
>> ISP and VIN. ISP/VIN [patch 7/8]as a sub-device needs to access other
>> resources managed by stf_camss.There is mutual reference between them.
>> Therefore, this patch is used for the overall compilation of the starfive
>> directory.
> 
> So previous code is dead? Again, what is the reason for adding dead
> code? Mutual reference is not the answer.
> 

Maybe I need to merge the previous patches[6-11] into one patch to submit?

> Best regards,
> Krzysztof
> 

Re: [PATCH v1 11/11] media: starfive: enable building

[Krzysztof Kozlowski]

On 08/03/2023 12:03, Jack Zhu wrote:
> 
> 
> On 2023/3/8 18:33, Krzysztof Kozlowski wrote:
>> On 07/03/2023 10:46, Jack Zhu wrote:
>>>
>>>
>>> On 2023/3/3 16:43, Krzysztof Kozlowski wrote:
>>>> On 02/03/2023 10:19, jack.zhu wrote:
>>>>> Add Kconfig and Makefie, update platform/Kconfig and platform/Makefile
>>>>> to enable building of the Starfive Camera subsystem driver.
>>>>>
>>>>> Signed-off-by: jack.zhu <jack.zhu@starfivetech.com>
>>>>> ---
>>>>>  drivers/media/platform/Kconfig           |  1 +
>>>>>  drivers/media/platform/Makefile          |  1 +
>>>>>  drivers/media/platform/starfive/Kconfig  | 18 ++++++++++++++++++
>>>>>  drivers/media/platform/starfive/Makefile | 14 ++++++++++++++
>>>>
>>>> This is not a separate commit. If it were, it would mean you just added
>>>> dead code in previous commits, so why adding dead code in first place?
>>>>
>>>
>>> The previous patches are made according to the module function.I think
>>> it is helpful to explain the composition of the code file. 
>>>
>>> stf_camss[patch 9] as a platform device manages all resources including
>>> ISP and VIN. ISP/VIN [patch 7/8]as a sub-device needs to access other
>>> resources managed by stf_camss.There is mutual reference between them.
>>> Therefore, this patch is used for the overall compilation of the starfive
>>> directory.
>>
>> So previous code is dead? Again, what is the reason for adding dead
>> code? Mutual reference is not the answer.
>>
> 
> Maybe I need to merge the previous patches[6-11] into one patch to submit?

I gave you the recommendation in my first reply. What's wrong with it?

Best regards,
Krzysztof

Re: [PATCH v1 11/11] media: starfive: enable building

[Jack Zhu]

On 2023/3/8 19:07, Krzysztof Kozlowski wrote:
> On 08/03/2023 12:03, Jack Zhu wrote:
>> 
>> 
>> On 2023/3/8 18:33, Krzysztof Kozlowski wrote:
>>> On 07/03/2023 10:46, Jack Zhu wrote:
>>>>
>>>>
>>>> On 2023/3/3 16:43, Krzysztof Kozlowski wrote:
>>>>> On 02/03/2023 10:19, jack.zhu wrote:
>>>>>> Add Kconfig and Makefie, update platform/Kconfig and platform/Makefile
>>>>>> to enable building of the Starfive Camera subsystem driver.
>>>>>>
>>>>>> Signed-off-by: jack.zhu <jack.zhu@starfivetech.com>
>>>>>> ---
>>>>>>  drivers/media/platform/Kconfig           |  1 +
>>>>>>  drivers/media/platform/Makefile          |  1 +
>>>>>>  drivers/media/platform/starfive/Kconfig  | 18 ++++++++++++++++++
>>>>>>  drivers/media/platform/starfive/Makefile | 14 ++++++++++++++
>>>>>
>>>>> This is not a separate commit. If it were, it would mean you just added
>>>>> dead code in previous commits, so why adding dead code in first place?
>>>>>
>>>>
>>>> The previous patches are made according to the module function.I think
>>>> it is helpful to explain the composition of the code file. 
>>>>
>>>> stf_camss[patch 9] as a platform device manages all resources including
>>>> ISP and VIN. ISP/VIN [patch 7/8]as a sub-device needs to access other
>>>> resources managed by stf_camss.There is mutual reference between them.
>>>> Therefore, this patch is used for the overall compilation of the starfive
>>>> directory.
>>>
>>> So previous code is dead? Again, what is the reason for adding dead
>>> code? Mutual reference is not the answer.
>>>
>> 
>> Maybe I need to merge the previous patches[6-11] into one patch to submit?
> 
> I gave you the recommendation in my first reply. What's wrong with it?
> 

Thank you for your suggestion and the recommended action which I just confirmed
it in detail in our previous messages.I agree with you that this is
not a separate commit.I will merge them in the next version to be a single commit.

This is my first time to submit code to the community.There are many things to learn.
I sincerely thank you for your patient help and advice.

> Best regards,
> Krzysztof
> 

Re: [PATCH v1 10/11] media: uapi: add user space interfaces

[Krzysztof Kozlowski]

On 02/03/2023 10:19, jack.zhu wrote:
> This patch adds user space interfaces for ISP.

Do not use "This commit/patch".
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95


Why this is a separate commit? User-space API is usually added with its
implementation. Otherwise how does your code even compile?

Best regards,
Krzysztof

Re: [PATCH v1 10/11] media: uapi: add user space interfaces

[Jack Zhu]

On 2023/3/3 16:44, Krzysztof Kozlowski wrote:
> On 02/03/2023 10:19, jack.zhu wrote:
>> This patch adds user space interfaces for ISP.
> 
> Do not use "This commit/patch".
> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
> 

OK, I will fix it.

> 
> Why this is a separate commit? User-space API is usually added with its
> implementation. Otherwise how does your code even compile?
> 

OK, I will merge them.
thank you for your comments.

> Best regards,
> Krzysztof
> 

Re: [PATCH v1 06/11] media: starfive: add ISP driver files

[Krzysztof Kozlowski]

On 02/03/2023 10:19, jack.zhu wrote:
> Add base driver for Starfive Image Signal Processing Unit which
> handles the data streams from the CSI2 receiver.
> 
> Signed-off-by: jack.zhu <jack.zhu@starfivetech.com>
> ---
>  drivers/media/platform/starfive/stf_isp.c     | 1079 ++++++++++++++
>  drivers/media/platform/starfive/stf_isp.h     |  183 +++
>  .../media/platform/starfive/stf_isp_hw_ops.c  | 1286 +++++++++++++++++
>  3 files changed, 2548 insertions(+)
>  create mode 100644 drivers/media/platform/starfive/stf_isp.c
>  create mode 100644 drivers/media/platform/starfive/stf_isp.h
>  create mode 100644 drivers/media/platform/starfive/stf_isp_hw_ops.c


> +}
> +
> +static int stf_isp_reg_read(struct stf_isp_dev *isp_dev, void *arg)
> +{
> +	void __iomem *ispbase = stf_isp_get_ispbase(isp_dev);
> +	struct isp_reg_param *reg_param = arg;


Didn't you add now code which does not even build and is not bisectable?


Best regards,
Krzysztof

Re: [PATCH v1 06/11] media: starfive: add ISP driver files

[Jack Zhu]

On 2023/3/3 16:45, Krzysztof Kozlowski wrote:
> On 02/03/2023 10:19, jack.zhu wrote:
>> Add base driver for Starfive Image Signal Processing Unit which
>> handles the data streams from the CSI2 receiver.
>> 
>> Signed-off-by: jack.zhu <jack.zhu@starfivetech.com>
>> ---
>>  drivers/media/platform/starfive/stf_isp.c     | 1079 ++++++++++++++
>>  drivers/media/platform/starfive/stf_isp.h     |  183 +++
>>  .../media/platform/starfive/stf_isp_hw_ops.c  | 1286 +++++++++++++++++
>>  3 files changed, 2548 insertions(+)
>>  create mode 100644 drivers/media/platform/starfive/stf_isp.c
>>  create mode 100644 drivers/media/platform/starfive/stf_isp.h
>>  create mode 100644 drivers/media/platform/starfive/stf_isp_hw_ops.c
> 
> 
>> +}
>> +
>> +static int stf_isp_reg_read(struct stf_isp_dev *isp_dev, void *arg)
>> +{
>> +	void __iomem *ispbase = stf_isp_get_ispbase(isp_dev);
>> +	struct isp_reg_param *reg_param = arg;
> 
> 
> Didn't you add now code which does not even build and is not bisectable?
> 

use patch 11 to build all files in the starfive directory.

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v1 06/11] media: starfive: add ISP driver files

[Krzysztof Kozlowski]

On 07/03/2023 10:53, Jack Zhu wrote:
> 
> 
> On 2023/3/3 16:45, Krzysztof Kozlowski wrote:
>> On 02/03/2023 10:19, jack.zhu wrote:
>>> Add base driver for Starfive Image Signal Processing Unit which
>>> handles the data streams from the CSI2 receiver.
>>>
>>> Signed-off-by: jack.zhu <jack.zhu@starfivetech.com>
>>> ---
>>>  drivers/media/platform/starfive/stf_isp.c     | 1079 ++++++++++++++
>>>  drivers/media/platform/starfive/stf_isp.h     |  183 +++
>>>  .../media/platform/starfive/stf_isp_hw_ops.c  | 1286 +++++++++++++++++
>>>  3 files changed, 2548 insertions(+)
>>>  create mode 100644 drivers/media/platform/starfive/stf_isp.c
>>>  create mode 100644 drivers/media/platform/starfive/stf_isp.h
>>>  create mode 100644 drivers/media/platform/starfive/stf_isp_hw_ops.c
>>
>>
>>> +}
>>> +
>>> +static int stf_isp_reg_read(struct stf_isp_dev *isp_dev, void *arg)
>>> +{
>>> +	void __iomem *ispbase = stf_isp_get_ispbase(isp_dev);
>>> +	struct isp_reg_param *reg_param = arg;
>>
>>
>> Didn't you add now code which does not even build and is not bisectable?
>>
> 
> use patch 11 to build all files in the starfive directory.
> 

Not related. So let me put it very simple:

1. Does this code build or not?
2. Do you certify that code is 100% bisectable?


Best regards,
Krzysztof

Re: [PATCH v1 06/11] media: starfive: add ISP driver files

[Jack Zhu]

On 2023/3/8 18:34, Krzysztof Kozlowski wrote:
> On 07/03/2023 10:53, Jack Zhu wrote:
>> 
>> 
>> On 2023/3/3 16:45, Krzysztof Kozlowski wrote:
>>> On 02/03/2023 10:19, jack.zhu wrote:
>>>> Add base driver for Starfive Image Signal Processing Unit which
>>>> handles the data streams from the CSI2 receiver.
>>>>
>>>> Signed-off-by: jack.zhu <jack.zhu@starfivetech.com>
>>>> ---
>>>>  drivers/media/platform/starfive/stf_isp.c     | 1079 ++++++++++++++
>>>>  drivers/media/platform/starfive/stf_isp.h     |  183 +++
>>>>  .../media/platform/starfive/stf_isp_hw_ops.c  | 1286 +++++++++++++++++
>>>>  3 files changed, 2548 insertions(+)
>>>>  create mode 100644 drivers/media/platform/starfive/stf_isp.c
>>>>  create mode 100644 drivers/media/platform/starfive/stf_isp.h
>>>>  create mode 100644 drivers/media/platform/starfive/stf_isp_hw_ops.c
>>>
>>>
>>>> +}
>>>> +
>>>> +static int stf_isp_reg_read(struct stf_isp_dev *isp_dev, void *arg)
>>>> +{
>>>> +	void __iomem *ispbase = stf_isp_get_ispbase(isp_dev);
>>>> +	struct isp_reg_param *reg_param = arg;
>>>
>>>
>>> Didn't you add now code which does not even build and is not bisectable?
>>>
>> 
>> use patch 11 to build all files in the starfive directory.
>> 
> 
> Not related. So let me put it very simple:
> 
> 1. Does this code build or not?
> 2. Do you certify that code is 100% bisectable?

No, Maybe I need to merge the previous patches[6-11] into one patch to submit?

> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v1 02/11] media: dt-bindings: starfive,jh7110-mipi-csi2: add binding docmuent

[Krzysztof Kozlowski]

On 02/03/2023 10:19, jack.zhu wrote:
> Add DT binding document for Starfive MIPI CSI2 receiver

Subject: drop second/last, redundant "add binding document". The
"dt-bindings" prefix is already stating that these are bindings and it
is a document. Write something useful instead.

> 
> Signed-off-by: jack.zhu <jack.zhu@starfivetech.com>
> ---
>  .../media/starfive,jh7110-mipi-csi2.yaml      | 177 ++++++++++++++++++
>  1 file changed, 177 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/starfive,jh7110-mipi-csi2.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/starfive,jh7110-mipi-csi2.yaml b/Documentation/devicetree/bindings/media/starfive,jh7110-mipi-csi2.yaml
> new file mode 100644
> index 000000000000..6569fac9e856
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/starfive,jh7110-mipi-csi2.yaml
> @@ -0,0 +1,177 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)

Why for patch 1 and 2 you are using difference SPDX?

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/starfive,jh7110-mipi-csi2.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Starfive JH7110 MIPI CSI-2 receiver
> +
> +maintainers:
> +  - Jack Zhu <jack.zhu@starfivetech.com>
> +  - Changhuang Liang <changhuang.liang@starfivetech.com>
> +
> +description: |-

Drop |-

> +  The JH7110 MIPI CSI-2 receiver device is responsible for handling CSI2
> +  protocol based camera sensor data stream.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - starfive,jh7110-csi2rx
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: CSI2Rx system clock
> +      - description: Gated Register bank clock for APB interface
> +      - description: pixel Clock for Stream interface 0
> +      - description: pixel Clock for Stream interface 1
> +      - description: pixel Clock for Stream interface 2
> +      - description: pixel Clock for Stream interface 3
> +
> +  clock-names:
> +    items:
> +      - const: sys_clk
> +      - const: p_clk
> +      - const: pixel_if0_clk
> +      - const: pixel_if1_clk
> +      - const: pixel_if2_clk
> +      - const: pixel_if3_clk

Drop _clk suffixes

> +
> +  resets:
> +    items:
> +      - description: CSI2Rx system reset
> +      - description: Gated Register bank reset for APB interface
> +      - description: pixel reset for Stream interface 0
> +      - description: pixel reset for Stream interface 1
> +      - description: pixel reset for Stream interface 2
> +      - description: pixel reset for Stream interface 3
> +
> +  reset-names:
> +    items:
> +      - const: sys_rst

Drop _rst suffixes

> +      - const: p_rst
> +      - const: pixel_if0_rst
> +      - const: pixel_if1_rst
> +      - const: pixel_if2_rst
> +      - const: pixel_if3_rst
> +
> +  phys:
> +    maxItems: 1
> +    description: MIPI D-PHY
> +
> +  phy-names:
> +    items:
> +      - const: dphy
> +
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        unevaluatedProperties: false
> +        description:
> +          Input port node, single endpoint describing the CSI-2 transmitter.
> +
> +        properties:
> +          endpoint:
> +            $ref: video-interfaces.yaml#
> +            unevaluatedProperties: false
> +
> +            properties:
> +              bus-type:
> +                items:

This is not an array.

> +                  - const: 4
> +
> +              clock-lanes:
> +                maxItems: 1

Not an array...

> +
> +              data-lanes:
> +                minItems: 1
> +                maxItems: 4
> +                items:
> +                  maximum: 4
> +
> +            required:
> +              - bus-type

Since this is fixed 4, do you actually require it? Why?

> +              - clock-lanes
> +              - data-lanes
> +
> +      port@1:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description:
> +          Output port node
> +
> +    required:
> +      - port@0
> +      - port@1
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - resets
> +  - reset-names
> +  - phys
> +  - phy-names
> +  - ports
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +

Drop blank line

> +    csi2rx: csi-bridge@19800000 {

Maybe just "csi@"?
> +        compatible = "starfive,jh7110-csi2rx";
> +        reg = <0x19800000 0x10000>;
> +        clocks = <&ispcrg 7>,
> +            <&ispcrg 6>,

Indentation looks odd... did you align it?

> +            <&ispcrg 8>,
> +            <&ispcrg 9>,
> +            <&ispcrg 10>,
> +            <&ispcrg 11>;
> +        clock-names = "sys_clk", "p_clk",
> +            "pixel_if0_clk", "pixel_if1_clk",
> +            "pixel_if2_clk", "pixel_if3_clk";
> +        resets = <&ispcrg 9>,
> +            <&ispcrg 4>,
> +            <&ispcrg 5>,
> +            <&ispcrg 6>,
> +            <&ispcrg 7>,
> +            <&ispcrg 8>;
> +        reset-names = "sys_rst", "p_rst",
> +            "pixel_if0_rst", "pixel_if1_rst",
> +            "pixel_if2_rst", "pixel_if3_rst";
> +        phys = <&csi_phy>;
> +        phy-names = "dphy";


Best regards,
Krzysztof

Re: [PATCH v1 02/11] media: dt-bindings: starfive,jh7110-mipi-csi2: add binding docmuent

[Krzysztof Kozlowski]

On 02/03/2023 10:19, jack.zhu wrote:
> Add DT binding document for Starfive MIPI CSI2 receiver

Ehh... you have entire commit msg to explain what you do here. Yet there
is nothing mentioning that you actually have Cadence MIPI CSI here.

Since you decided to add new bindings, you receive review matching new
bindings. I don't think this is correct approach (duplicated bindings),
but could work for me. However how are you going to solve all the points
of my review?

Best regards,
Krzysztof

Re: [PATCH v1 02/11] media: dt-bindings: starfive,jh7110-mipi-csi2: add binding docmuent

[Jack Zhu]

On 2023/3/3 16:47, Krzysztof Kozlowski wrote:
> On 02/03/2023 10:19, jack.zhu wrote:
>> Add DT binding document for Starfive MIPI CSI2 receiver
> 
> Ehh... you have entire commit msg to explain what you do here. Yet there
> is nothing mentioning that you actually have Cadence MIPI CSI here.
> 
> Since you decided to add new bindings, you receive review matching new
> bindings. I don't think this is correct approach (duplicated bindings),
> but could work for me. However how are you going to solve all the points
> of my review?

Maybe I don't need to add the CSI yaml file, since it already exists on the Linux mainline.

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v1 02/11] media: dt-bindings: starfive,jh7110-mipi-csi2: add binding docmuent

[Krzysztof Kozlowski]

On 07/03/2023 07:41, Jack Zhu wrote:
> 
> 
> On 2023/3/3 16:47, Krzysztof Kozlowski wrote:
>> On 02/03/2023 10:19, jack.zhu wrote:
>>> Add DT binding document for Starfive MIPI CSI2 receiver
>>
>> Ehh... you have entire commit msg to explain what you do here. Yet there
>> is nothing mentioning that you actually have Cadence MIPI CSI here.
>>
>> Since you decided to add new bindings, you receive review matching new
>> bindings. I don't think this is correct approach (duplicated bindings),
>> but could work for me. However how are you going to solve all the points
>> of my review?
> 
> Maybe I don't need to add the CSI yaml file, since it already exists on the Linux mainline.

If you add *only* new compatible, you do not need new binding. If you
add any new properties, then depends, but old binding anyway would need
conversion from TXT.

Best regards,
Krzysztof

Re: [PATCH v1 02/11] media: dt-bindings: starfive,jh7110-mipi-csi2: add binding docmuent

[Jack Zhu]

On 2023/3/7 15:53, Krzysztof Kozlowski wrote:
> On 07/03/2023 07:41, Jack Zhu wrote:
>> 
>> 
>> On 2023/3/3 16:47, Krzysztof Kozlowski wrote:
>>> On 02/03/2023 10:19, jack.zhu wrote:
>>>> Add DT binding document for Starfive MIPI CSI2 receiver
>>>
>>> Ehh... you have entire commit msg to explain what you do here. Yet there
>>> is nothing mentioning that you actually have Cadence MIPI CSI here.
>>>
>>> Since you decided to add new bindings, you receive review matching new
>>> bindings. I don't think this is correct approach (duplicated bindings),
>>> but could work for me. However how are you going to solve all the points
>>> of my review?
>> 
>> Maybe I don't need to add the CSI yaml file, since it already exists on the Linux mainline.
> 
> If you add *only* new compatible, you do not need new binding. If you
> add any new properties, then depends, but old binding anyway would need
> conversion from TXT.

have some new properties, this means that
need to convert cdns,csi2rx.txt to cdns,csi2rx.yaml and add my new attributes?

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v1 02/11] media: dt-bindings: starfive,jh7110-mipi-csi2: add binding docmuent

[Krzysztof Kozlowski]

On 07/03/2023 11:08, Jack Zhu wrote:
> 
> 
> On 2023/3/7 15:53, Krzysztof Kozlowski wrote:
>> On 07/03/2023 07:41, Jack Zhu wrote:
>>>
>>>
>>> On 2023/3/3 16:47, Krzysztof Kozlowski wrote:
>>>> On 02/03/2023 10:19, jack.zhu wrote:
>>>>> Add DT binding document for Starfive MIPI CSI2 receiver
>>>>
>>>> Ehh... you have entire commit msg to explain what you do here. Yet there
>>>> is nothing mentioning that you actually have Cadence MIPI CSI here.
>>>>
>>>> Since you decided to add new bindings, you receive review matching new
>>>> bindings. I don't think this is correct approach (duplicated bindings),
>>>> but could work for me. However how are you going to solve all the points
>>>> of my review?
>>>
>>> Maybe I don't need to add the CSI yaml file, since it already exists on the Linux mainline.
>>
>> If you add *only* new compatible, you do not need new binding. If you
>> add any new properties, then depends, but old binding anyway would need
>> conversion from TXT.
> 
> have some new properties, this means that
> need to convert cdns,csi2rx.txt to cdns,csi2rx.yaml and add my new attributes?

Yes.

Best regards,
Krzysztof