[PATCH v2 1/2] dt-bindings: phy: add mediatek mipi csi driver v 0.5

[PATCH v2 1/2] dt-bindings: phy: add mediatek mipi csi driver v 0.5

[Julien Stephan]

From: Florian Sylvestre <fsylvestre@baylibre.com>

This adds the bindings, for the MIPI CD-PHY module v 0.5 embedded in
some Mediatek soc, such as the mt8365

Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
Signed-off-by: Julien Stephan <jstephan@baylibre.com>
---
 .../phy/mediatek,phy-mipi-csi-0-5.yaml        | 62 +++++++++++++++++++
 MAINTAINERS                                   |  6 ++
 2 files changed, 68 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml

diff --git a/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml b/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
new file mode 100644
index 000000000000..5aa8c0b41cdf
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
@@ -0,0 +1,62 @@
+# SPDX-License-Identifier: (GPL-2.0-Only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/mediatek,phy-mipi-csi-0-5.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Mediatek Sensor Interface MIPI CSI CD-PHY
+
+maintainers:
+  - Julien Stephan <jstephan@baylibre.com>
+  - Andy Hsieh <andy.hsieh@mediatek.com>
+
+description:
+  The SENINF CD-PHY is a set of CD-PHY connected to the SENINF CSI-2
+  receivers. The number of PHYs depends on the SoC model.
+  Depending on the soc model, each PHYs can support CDPHY or DPHY only
+
+properties:
+  compatible:
+    enum:
+      - mediatek,phy-mipi-csi-0-5
+
+  reg:
+    maxItems: 1
+
+  '#phy-cells':
+    const: 0
+
+  mediatek,is_cdphy:
+    description:
+      Specify if the current phy support CDPHY configuration
+    type: boolean
+
+required:
+  - compatible
+  - reg
+  - '#phy-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      mipi_rx_csi0: mipi_rx_csi0@11c10000 {
+        compatible = "mediatek,phy-mipi-csi-0-5";
+        reg = <0 0x11C10000 0 0x2000>;
+        status = "okay";
+        mediatek,is_cdphy;
+        #phy-cells = <0>;
+      };
+
+      mipi_rx_csi1: mipi-rx-csi1@11c12000 {
+        compatible = "mediatek,phy-mipi-csi-0-5";
+        reg = <0 0x11C12000 0 0x2000>;
+        status = "disabled";
+        #phy-cells = <0>;
+      };
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 6d54f3193075..44f0ff11e984 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13098,6 +13098,12 @@ F:	Documentation/devicetree/bindings/media/mediatek-vpu.txt
 F:	drivers/media/platform/mediatek/vcodec/
 F:	drivers/media/platform/mediatek/vpu/
 
+MEDIATEK MIPI-CSI CDPHY DRIVER
+M:	Julien Stephan <jstephan@baylibre.com>
+M:	Andy Hsieh <andy.hsieh@mediatek.com>
+S:	Supported
+F:	Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
+
 MEDIATEK MMC/SD/SDIO DRIVER
 M:	Chaotian Jing <chaotian.jing@mediatek.com>
 S:	Maintained
-- 
2.40.0

Re: [PATCH v2 1/2] dt-bindings: phy: add mediatek mipi csi driver v 0.5

[AngeloGioacchino Del Regno]

Il 15/05/23 11:05, Julien Stephan ha scritto:
> From: Florian Sylvestre <fsylvestre@baylibre.com>
> 
> This adds the bindings, for the MIPI CD-PHY module v 0.5 embedded in
> some Mediatek soc, such as the mt8365
> 
> Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
> Signed-off-by: Julien Stephan <jstephan@baylibre.com>
> ---
>   .../phy/mediatek,phy-mipi-csi-0-5.yaml        | 62 +++++++++++++++++++
>   MAINTAINERS                                   |  6 ++
>   2 files changed, 68 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml b/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
> new file mode 100644
> index 000000000000..5aa8c0b41cdf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
> @@ -0,0 +1,62 @@
> +# SPDX-License-Identifier: (GPL-2.0-Only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/mediatek,phy-mipi-csi-0-5.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Mediatek Sensor Interface MIPI CSI CD-PHY
> +
> +maintainers:
> +  - Julien Stephan <jstephan@baylibre.com>
> +  - Andy Hsieh <andy.hsieh@mediatek.com>
> +
> +description:
> +  The SENINF CD-PHY is a set of CD-PHY connected to the SENINF CSI-2
> +  receivers. The number of PHYs depends on the SoC model.
> +  Depending on the soc model, each PHYs can support CDPHY or DPHY only
> +
> +properties:
> +  compatible:
> +    enum:
> +      - mediatek,phy-mipi-csi-0-5

mediatek,mtXXXX-csi-rx sounds great, doesn't it?

> +
> +  reg:
> +    maxItems: 1
> +
> +  '#phy-cells':
> +    const: 0
> +
> +  mediatek,is_cdphy:

No underscores please: mediatek,is-cdphy

> +    description:
> +      Specify if the current phy support CDPHY configuration

Description fits in one line.

     description: Specify if the current phy support CDPHY configuration

> +    type: boolean
> +
> +required:
> +  - compatible
> +  - reg
> +  - '#phy-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    soc {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +
> +      mipi_rx_csi0: mipi_rx_csi0@11c10000 {

csi0_rx: phy@11c10000

> +        compatible = "mediatek,phy-mipi-csi-0-5";
> +        reg = <0 0x11C10000 0 0x2000>;
> +        status = "okay";
> +        mediatek,is_cdphy;
> +        #phy-cells = <0>;
> +      };
> +
> +      mipi_rx_csi1: mipi-rx-csi1@11c12000 {

csi1_rx: phy@11c20000

> +        compatible = "mediatek,phy-mipi-csi-0-5";
> +        reg = <0 0x11C12000 0 0x2000>;
> +        status = "disabled";
> +        #phy-cells = <0>;
> +      };
> +    };
> +...

Regards,
Angelo

Re: [PATCH v2 1/2] dt-bindings: phy: add mediatek mipi csi driver v 0.5

[Krzysztof Kozlowski]

On 15/05/2023 11:05, Julien Stephan wrote:
> From: Florian Sylvestre <fsylvestre@baylibre.com>
> 
> This adds the bindings, for the MIPI CD-PHY module v 0.5 embedded in
> some Mediatek soc, such as the mt8365
> 
> Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
> Signed-off-by: Julien Stephan <jstephan@baylibre.com>

What are the changes? IOW: changelog here or in cover letter.

Subject: you have some multiple spaces.

Subject: drop driver. Bindings are not for drivers.

> ---
>  .../phy/mediatek,phy-mipi-csi-0-5.yaml        | 62 +++++++++++++++++++
>  MAINTAINERS                                   |  6 ++
>  2 files changed, 68 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml b/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
> new file mode 100644
> index 000000000000..5aa8c0b41cdf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
> @@ -0,0 +1,62 @@
> +# SPDX-License-Identifier: (GPL-2.0-Only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/mediatek,phy-mipi-csi-0-5.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Mediatek Sensor Interface MIPI CSI CD-PHY
> +
> +maintainers:
> +  - Julien Stephan <jstephan@baylibre.com>
> +  - Andy Hsieh <andy.hsieh@mediatek.com>
> +
> +description:
> +  The SENINF CD-PHY is a set of CD-PHY connected to the SENINF CSI-2
> +  receivers. The number of PHYs depends on the SoC model.
> +  Depending on the soc model, each PHYs can support CDPHY or DPHY only
> +
> +properties:
> +  compatible:
> +    enum:
> +      - mediatek,phy-mipi-csi-0-5

SoC based compatibles. 0-5 is odd.

> +
> +  reg:
> +    maxItems: 1
> +
> +  '#phy-cells':
> +    const: 0
> +
> +  mediatek,is_cdphy:

No underscores in node names.

> +    description:
> +      Specify if the current phy support CDPHY configuration

Why this cannot be implied from compatible? Add specific compatibles.


> +    type: boolean
> +
> +required:
> +  - compatible
> +  - reg
> +  - '#phy-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    soc {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +
> +      mipi_rx_csi0: mipi_rx_csi0@11c10000 {

No underscores in node names. How this is v2?

> +        compatible = "mediatek,phy-mipi-csi-0-5";
> +        reg = <0 0x11C10000 0 0x2000>;
> +        status = "okay";

Drop

> +        mediatek,is_cdphy;
> +        #phy-cells = <0>;
> +      };
> +
> +      mipi_rx_csi1: mipi-rx-csi1@11c12000 {
> +        compatible = "mediatek,phy-mipi-csi-0-5";
> +        reg = <0 0x11C12000 0 0x2000>;
> +        status = "disabled";

???


Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: phy: add mediatek mipi csi driver v 0.5

[Julien Stephan]

On Tue, May 16, 2023 at 10:07:47AM +0200, Krzysztof Kozlowski wrote:
> On 15/05/2023 11:05, Julien Stephan wrote:
> > From: Florian Sylvestre <fsylvestre@baylibre.com>
> >
> > This adds the bindings, for the MIPI CD-PHY module v 0.5 embedded in
> > some Mediatek soc, such as the mt8365
> >
> > Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
> > Signed-off-by: Julien Stephan <jstephan@baylibre.com>
>
> What are the changes? IOW: changelog here or in cover letter.
>
Hi Krzysztof,
I added a changelog in the cover letter, but I will try to be more
descritpive next time. Changes from v1 are mainly style issues fixed
(mostly from your first review)

> Subject: you have some multiple spaces.
>
> Subject: drop driver. Bindings are not for drivers.
>
> > ---
> >  .../phy/mediatek,phy-mipi-csi-0-5.yaml        | 62 +++++++++++++++++++
> >  MAINTAINERS                                   |  6 ++
> >  2 files changed, 68 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml b/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
> > new file mode 100644
> > index 000000000000..5aa8c0b41cdf
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
> > @@ -0,0 +1,62 @@
> > +# SPDX-License-Identifier: (GPL-2.0-Only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/phy/mediatek,phy-mipi-csi-0-5.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Mediatek Sensor Interface MIPI CSI CD-PHY
> > +
> > +maintainers:
> > +  - Julien Stephan <jstephan@baylibre.com>
> > +  - Andy Hsieh <andy.hsieh@mediatek.com>
> > +
> > +description:
> > +  The SENINF CD-PHY is a set of CD-PHY connected to the SENINF CSI-2
> > +  receivers. The number of PHYs depends on the SoC model.
> > +  Depending on the soc model, each PHYs can support CDPHY or DPHY only
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - mediatek,phy-mipi-csi-0-5
>
> SoC based compatibles. 0-5 is odd.
>
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  '#phy-cells':
> > +    const: 0
> > +
> > +  mediatek,is_cdphy:
>
> No underscores in node names.
>
> > +    description:
> > +      Specify if the current phy support CDPHY configuration
>
> Why this cannot be implied from compatible? Add specific compatibles.
>
>
This cannot be implied by compatible because the number of phys depends
on the soc and each phy can be either D-PHY only or CD-PHY capable.
For example mt8365 has 2 phy: CSI0 and CSI1. CSI1 is DPHY only and CSI0 is CD-PHY

Regards,
Julien
> > +    type: boolean
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - '#phy-cells'
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    soc {
> > +      #address-cells = <2>;
> > +      #size-cells = <2>;
> > +
> > +      mipi_rx_csi0: mipi_rx_csi0@11c10000 {
>
> No underscores in node names. How this is v2?
>
> > +        compatible = "mediatek,phy-mipi-csi-0-5";
> > +        reg = <0 0x11C10000 0 0x2000>;
> > +        status = "okay";
>
> Drop
>
> > +        mediatek,is_cdphy;
> > +        #phy-cells = <0>;
> > +      };
> > +
> > +      mipi_rx_csi1: mipi-rx-csi1@11c12000 {
> > +        compatible = "mediatek,phy-mipi-csi-0-5";
> > +        reg = <0 0x11C12000 0 0x2000>;
> > +        status = "disabled";
>
> ???
>
>
> Best regards,
> Krzysztof
>

Re: [PATCH v2 1/2] dt-bindings: phy: add mediatek mipi csi driver v 0.5

[Krzysztof Kozlowski]

On 16/05/2023 11:41, Julien Stephan wrote:
> On Tue, May 16, 2023 at 10:07:47AM +0200, Krzysztof Kozlowski wrote:
>> On 15/05/2023 11:05, Julien Stephan wrote:
>>> From: Florian Sylvestre <fsylvestre@baylibre.com>
>>>
>>> This adds the bindings, for the MIPI CD-PHY module v 0.5 embedded in
>>> some Mediatek soc, such as the mt8365
>>>
>>> Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
>>> Signed-off-by: Julien Stephan <jstephan@baylibre.com>
>>
>> What are the changes? IOW: changelog here or in cover letter.
>>
> Hi Krzysztof,
> I added a changelog in the cover letter, but I will try to be more
> descritpive next time. Changes from v1 are mainly style issues fixed
> (mostly from your first review)

What do you mean by "in cover letter"? There is no cover letter.

> 
>> Subject: you have some multiple spaces.
>>
>> Subject: drop driver. Bindings are not for drivers.
>>
>>> ---
>>>  .../phy/mediatek,phy-mipi-csi-0-5.yaml        | 62 +++++++++++++++++++
>>>  MAINTAINERS                                   |  6 ++
>>>  2 files changed, 68 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml b/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
>>> new file mode 100644
>>> index 000000000000..5aa8c0b41cdf
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
>>> @@ -0,0 +1,62 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-Only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/phy/mediatek,phy-mipi-csi-0-5.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Mediatek Sensor Interface MIPI CSI CD-PHY
>>> +
>>> +maintainers:
>>> +  - Julien Stephan <jstephan@baylibre.com>
>>> +  - Andy Hsieh <andy.hsieh@mediatek.com>
>>> +
>>> +description:
>>> +  The SENINF CD-PHY is a set of CD-PHY connected to the SENINF CSI-2
>>> +  receivers. The number of PHYs depends on the SoC model.
>>> +  Depending on the soc model, each PHYs can support CDPHY or DPHY only
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - mediatek,phy-mipi-csi-0-5
>>
>> SoC based compatibles. 0-5 is odd.
>>
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  '#phy-cells':
>>> +    const: 0
>>> +
>>> +  mediatek,is_cdphy:
>>
>> No underscores in node names.
>>
>>> +    description:
>>> +      Specify if the current phy support CDPHY configuration
>>
>> Why this cannot be implied from compatible? Add specific compatibles.
>>
>>
> This cannot be implied by compatible because the number of phys depends
> on the soc and each phy can be either D-PHY only or CD-PHY capable.
> For example mt8365 has 2 phy: CSI0 and CSI1. CSI1 is DPHY only and CSI0 is CD-PHY

So it is SoC specific so why it cannot be implied by compatible? I don't
understand. You will have SoC specific compatibles, right? or you just
ignored my comments here?

>>

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: phy: add mediatek mipi csi driver v 0.5

[Kevin Hilman]

Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes:

> On 16/05/2023 11:41, Julien Stephan wrote:
>> On Tue, May 16, 2023 at 10:07:47AM +0200, Krzysztof Kozlowski wrote:
>>> On 15/05/2023 11:05, Julien Stephan wrote:
>>>> From: Florian Sylvestre <fsylvestre@baylibre.com>
>>>>
>>>> This adds the bindings, for the MIPI CD-PHY module v 0.5 embedded in
>>>> some Mediatek soc, such as the mt8365
>>>>
>>>> Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
>>>> Signed-off-by: Julien Stephan <jstephan@baylibre.com>
>>>
>>> What are the changes? IOW: changelog here or in cover letter.
>>>
>> Hi Krzysztof,
>> I added a changelog in the cover letter, but I will try to be more
>> descritpive next time. Changes from v1 are mainly style issues fixed
>> (mostly from your first review)
>
> What do you mean by "in cover letter"? There is no cover letter.

Julien, your cover letter[1] was sent to a a different list of
recipients than the patches, and most important for this thread, it was
*not* sent to the devictree list. So I'm guessing that's why Krzysztof
doesn't see it in his devicetree review queue.  Generally, you should
have the same list of recipients for the cover letter as the patches
since reviewers/maintainers generally filter mail based on which mailing
lists are in to/cc.

>> 
>>> Subject: you have some multiple spaces.
>>>
>>> Subject: drop driver. Bindings are not for drivers.
>>>
>>>> ---
>>>>  .../phy/mediatek,phy-mipi-csi-0-5.yaml        | 62 +++++++++++++++++++
>>>>  MAINTAINERS                                   |  6 ++
>>>>  2 files changed, 68 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml b/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
>>>> new file mode 100644
>>>> index 000000000000..5aa8c0b41cdf
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
>>>> @@ -0,0 +1,62 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-Only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/phy/mediatek,phy-mipi-csi-0-5.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Mediatek Sensor Interface MIPI CSI CD-PHY
>>>> +
>>>> +maintainers:
>>>> +  - Julien Stephan <jstephan@baylibre.com>
>>>> +  - Andy Hsieh <andy.hsieh@mediatek.com>
>>>> +
>>>> +description:
>>>> +  The SENINF CD-PHY is a set of CD-PHY connected to the SENINF CSI-2
>>>> +  receivers. The number of PHYs depends on the SoC model.
>>>> +  Depending on the soc model, each PHYs can support CDPHY or DPHY only
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    enum:
>>>> +      - mediatek,phy-mipi-csi-0-5
>>>
>>> SoC based compatibles. 0-5 is odd.
>>>
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  '#phy-cells':
>>>> +    const: 0
>>>> +
>>>> +  mediatek,is_cdphy:
>>>
>>> No underscores in node names.
>>>
>>>> +    description:
>>>> +      Specify if the current phy support CDPHY configuration
>>>
>>> Why this cannot be implied from compatible? Add specific compatibles.
>>>
>>>
>> This cannot be implied by compatible because the number of phys depends
>> on the soc and each phy can be either D-PHY only or CD-PHY capable.
>> For example mt8365 has 2 phy: CSI0 and CSI1. CSI1 is DPHY only and CSI0 is CD-PHY
>
> So it is SoC specific so why it cannot be implied by compatible? I don't
> understand. You will have SoC specific compatibles, right? or you just
> ignored my comments here?

Julien, I think you had SoC specific compatibles in an earlier version
but then changed it to be generic based on reviewer feedback.  However,
that earlier version of the driver was trying to do a bunch of SoC
specific logic internally and support multiple SoCs.  You've now greatly
simplified the driver, with only a few SoC specific decisions needed.
These can be implied by the driver based SoC specific compatible, as
Krzysztof suggests, so you should just go back to having SoC specific
compatibles.

Kevin

[1] https://lore.kernel.org/linux-mediatek/20230515090551.1251389-1-jstephan@baylibre.com/#r

Re: [PATCH v2 1/2] dt-bindings: phy: add mediatek mipi csi driver v 0.5

[Krzysztof Kozlowski]

On 16/05/2023 19:00, Kevin Hilman wrote:
>>>>> +  compatible:
>>>>> +    enum:
>>>>> +      - mediatek,phy-mipi-csi-0-5
>>>>
>>>> SoC based compatibles. 0-5 is odd.
>>>>
>>>>> +
>>>>> +  reg:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  '#phy-cells':
>>>>> +    const: 0
>>>>> +
>>>>> +  mediatek,is_cdphy:
>>>>
>>>> No underscores in node names.
>>>>
>>>>> +    description:
>>>>> +      Specify if the current phy support CDPHY configuration
>>>>
>>>> Why this cannot be implied from compatible? Add specific compatibles.
>>>>
>>>>
>>> This cannot be implied by compatible because the number of phys depends
>>> on the soc and each phy can be either D-PHY only or CD-PHY capable.
>>> For example mt8365 has 2 phy: CSI0 and CSI1. CSI1 is DPHY only and CSI0 is CD-PHY
>>
>> So it is SoC specific so why it cannot be implied by compatible? I don't
>> understand. You will have SoC specific compatibles, right? or you just
>> ignored my comments here?
> 
> Julien, I think you had SoC specific compatibles in an earlier version
> but then changed it to be generic based on reviewer feedback.  However,
> that earlier version of the driver was trying to do a bunch of SoC
> specific logic internally and support multiple SoCs.  You've now greatly
> simplified the driver, with only a few SoC specific decisions needed.
> These can be implied by the driver based SoC specific compatible, as
> Krzysztof suggests, so you should just go back to having SoC specific
> compatibles.
> 

Yes. If there is common part, e.g. several SoCs use the same device with
same programming model, then the generic recommendation is to have
SoC-based fallback (used also in the driver) and SoC-specific compatibles.

Second accepted solution is to have generic fallback which does not use
SoC in the compatible (and of course mandatory SoC-specific comaptibles).

Third is to use versioned IP blocks.

The second case also would work, if it is applicable to you (you really
have fallback matching all devices). Third solution depends on your
versioning and Rob expressed dislike about it many times.

We had many discussions on mailing lists, thus simplifying the review -
I recommend the first choice. For a better recommendation you should say
a bit more about the block in different SoCs.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: phy: add mediatek mipi csi driver v 0.5

[Kevin Hilman]

Hi Krzysztof, 

Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes:

> On 16/05/2023 19:00, Kevin Hilman wrote:
>>>>>> +  compatible:
>>>>>> +    enum:
>>>>>> +      - mediatek,phy-mipi-csi-0-5
>>>>>
>>>>> SoC based compatibles. 0-5 is odd.
>>>>>
>>>>>> +
>>>>>> +  reg:
>>>>>> +    maxItems: 1
>>>>>> +
>>>>>> +  '#phy-cells':
>>>>>> +    const: 0
>>>>>> +
>>>>>> +  mediatek,is_cdphy:
>>>>>
>>>>> No underscores in node names.
>>>>>
>>>>>> +    description:
>>>>>> +      Specify if the current phy support CDPHY configuration
>>>>>
>>>>> Why this cannot be implied from compatible? Add specific compatibles.
>>>>>
>>>>>
>>>> This cannot be implied by compatible because the number of phys depends
>>>> on the soc and each phy can be either D-PHY only or CD-PHY capable.
>>>> For example mt8365 has 2 phy: CSI0 and CSI1. CSI1 is DPHY only and CSI0 is CD-PHY
>>>
>>> So it is SoC specific so why it cannot be implied by compatible? I don't
>>> understand. You will have SoC specific compatibles, right? or you just
>>> ignored my comments here?
>> 
>> Julien, I think you had SoC specific compatibles in an earlier version
>> but then changed it to be generic based on reviewer feedback.  However,
>> that earlier version of the driver was trying to do a bunch of SoC
>> specific logic internally and support multiple SoCs.  You've now greatly
>> simplified the driver, with only a few SoC specific decisions needed.
>> These can be implied by the driver based SoC specific compatible, as
>> Krzysztof suggests, so you should just go back to having SoC specific
>> compatibles.
>> 
>
> Yes. If there is common part, e.g. several SoCs use the same device with
> same programming model, then the generic recommendation is to have
> SoC-based fallback (used also in the driver) and SoC-specific compatibles.
>
> Second accepted solution is to have generic fallback which does not use
> SoC in the compatible (and of course mandatory SoC-specific comaptibles).
>
> Third is to use versioned IP blocks.
>
> The second case also would work, if it is applicable to you (you really
> have fallback matching all devices). Third solution depends on your
> versioning and Rob expressed dislike about it many times.
>
> We had many discussions on mailing lists, thus simplifying the review -
> I recommend the first choice. For a better recommendation you should say
> a bit more about the block in different SoCs.

I'll try to say a bit more about the PHY block, but in fact, it's not
just about differences between SoCs. On the same SoC, 2 different PHYs
may have different features/capabilities.

For example, on MT8365, There are 2 PHYs: CSI0 and CSI1.  CSI0 can
function as a C-PHY or a D-PHY, but CSI1 can only function as D-PHY
(used as the example in the binding patch[1].)  On another related SoC,
there are 3 PHYs, where CSI0 is C-D but CSI1 & CSI2 are only D.

So that's why it seems (at least to me) that while we need SoC
compatible, it's not enough.  We also need properties to describe
PHY-specific features (e.g. C-D PHY)

Of course, we could rely only on SoC-specific compatibles describe this.
But then driver will need an SoC-specific table with the number of PHYs
and per-PHY features for each SoC encoded in the driver.  Since the
driver otherwise doesn't (and shouldn't, IMHO) need to know how many
PHYs are on each SoC, I suggested to Julien that perhaps the additional
propery was the better solution.  

To me it seems redundant to have the driver encode PHYs-per-SoC info,
when the per-SoC DT is going to have the same info, so my suggestion was
to simplify the driver and have this kind of hardware description in the
DT, and keep the driver simple, but we are definitely open to learning
the "right way" of doing this.

Thanks for your review and guidance,

Kevin

[1] https://lore.kernel.org/linux-mediatek/20230515090551.1251389-2-jstephan@baylibre.com/

Re: [PATCH v2 1/2] dt-bindings: phy: add mediatek mipi csi driver v 0.5

[Krzysztof Kozlowski]

On 16/05/2023 23:31, Kevin Hilman wrote:

>> Third is to use versioned IP blocks.
>>
>> The second case also would work, if it is applicable to you (you really
>> have fallback matching all devices). Third solution depends on your
>> versioning and Rob expressed dislike about it many times.
>>
>> We had many discussions on mailing lists, thus simplifying the review -
>> I recommend the first choice. For a better recommendation you should say
>> a bit more about the block in different SoCs.
> 
> I'll try to say a bit more about the PHY block, but in fact, it's not
> just about differences between SoCs. On the same SoC, 2 different PHYs
> may have different features/capabilities.
> 
> For example, on MT8365, There are 2 PHYs: CSI0 and CSI1.  CSI0 can
> function as a C-PHY or a D-PHY, but CSI1 can only function as D-PHY
> (used as the example in the binding patch[1].)  On another related SoC,
> there are 3 PHYs, where CSI0 is C-D but CSI1 & CSI2 are only D.
> 
> So that's why it seems (at least to me) that while we need SoC
> compatible, it's not enough.  We also need properties to describe
> PHY-specific features (e.g. C-D PHY)

I recall the same or very similar case... It bugs me now, but
unfortunately I cannot find it.

> 
> Of course, we could rely only on SoC-specific compatibles describe this.
> But then driver will need an SoC-specific table with the number of PHYs
> and per-PHY features for each SoC encoded in the driver.  Since the
> driver otherwise doesn't (and shouldn't, IMHO) need to know how many
> PHYs are on each SoC, I suggested to Julien that perhaps the additional
> propery was the better solution.

Phys were modeled as separate device instances, so you would need
difference in compatible to figure out which phy is it.

Other way could be to create device for all phys and use phy-cells=1.
Whether it makes sense, depends on the actual datasheet - maybe the
split phy per device is artificial? There is one PHY block with two
address ranges for each PHY - CSI0 and CSI1 - but it is actually one
block? You should carefully check this because once design is chosen,
you won't be able to go back to other and it might be a problem (e.g.
there is some top-level block for powering on all CSI instances).


> 
> To me it seems redundant to have the driver encode PHYs-per-SoC info,
> when the per-SoC DT is going to have the same info, so my suggestion was
> to simplify the driver and have this kind of hardware description in the
> DT, and keep the driver simple, but we are definitely open to learning
> the "right way" of doing this.

The property then is reasonable. It should not be bool, though, because
it does not scale. There can be next block which supports only D-PHY on
CSI0 and C-PHY on CSI1? Maybe some enum or list, depending on possible
configurations.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: phy: add mediatek mipi csi driver v 0.5

[Kevin Hilman]

Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes:

> On 16/05/2023 23:31, Kevin Hilman wrote:
>
>>> Third is to use versioned IP blocks.
>>>
>>> The second case also would work, if it is applicable to you (you really
>>> have fallback matching all devices). Third solution depends on your
>>> versioning and Rob expressed dislike about it many times.
>>>
>>> We had many discussions on mailing lists, thus simplifying the review -
>>> I recommend the first choice. For a better recommendation you should say
>>> a bit more about the block in different SoCs.
>> 
>> I'll try to say a bit more about the PHY block, but in fact, it's not
>> just about differences between SoCs. On the same SoC, 2 different PHYs
>> may have different features/capabilities.
>> 
>> For example, on MT8365, There are 2 PHYs: CSI0 and CSI1.  CSI0 can
>> function as a C-PHY or a D-PHY, but CSI1 can only function as D-PHY
>> (used as the example in the binding patch[1].)  On another related SoC,
>> there are 3 PHYs, where CSI0 is C-D but CSI1 & CSI2 are only D.
>> 
>> So that's why it seems (at least to me) that while we need SoC
>> compatible, it's not enough.  We also need properties to describe
>> PHY-specific features (e.g. C-D PHY)
>
> I recall the same or very similar case... It bugs me now, but
> unfortunately I cannot find it.
>
>> 
>> Of course, we could rely only on SoC-specific compatibles describe this.
>> But then driver will need an SoC-specific table with the number of PHYs
>> and per-PHY features for each SoC encoded in the driver.  Since the
>> driver otherwise doesn't (and shouldn't, IMHO) need to know how many
>> PHYs are on each SoC, I suggested to Julien that perhaps the additional
>> propery was the better solution.
>
> Phys were modeled as separate device instances, so you would need
> difference in compatible to figure out which phy is it.
>
> Other way could be to create device for all phys and use phy-cells=1.
> Whether it makes sense, depends on the actual datasheet - maybe the
> split phy per device is artificial? There is one PHY block with two
> address ranges for each PHY - CSI0 and CSI1 - but it is actually one
> block? You should carefully check this because once design is chosen,
> you won't be able to go back to other and it might be a problem (e.g.
> there is some top-level block for powering on all CSI instances).

We're pretty sure these are multiple instances of the IP block as they
can operate completely independently. 

>> 
>> To me it seems redundant to have the driver encode PHYs-per-SoC info,
>> when the per-SoC DT is going to have the same info, so my suggestion was
>> to simplify the driver and have this kind of hardware description in the
>> DT, and keep the driver simple, but we are definitely open to learning
>> the "right way" of doing this.
>
> The property then is reasonable. It should not be bool, though, because
> it does not scale. There can be next block which supports only D-PHY on
> CSI0 and C-PHY on CSI1? Maybe some enum or list, depending on possible
> configurations.

OK, looks like include/dt-bindings/phy/phy.y already has

  #define PHY_TYPE_DPHY		10
  #define PHY_TYPE_CPHY		11

we'll add a PHY_TYPE_CDPHY and use that.   Sound reasonable?

Kevin

Re: [PATCH v2 1/2] dt-bindings: phy: add mediatek mipi csi driver v 0.5

[Krzysztof Kozlowski]

On 22/05/2023 21:15, Kevin Hilman wrote:
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes:
> 
>> On 16/05/2023 23:31, Kevin Hilman wrote:
>>
>>>> Third is to use versioned IP blocks.
>>>>
>>>> The second case also would work, if it is applicable to you (you really
>>>> have fallback matching all devices). Third solution depends on your
>>>> versioning and Rob expressed dislike about it many times.
>>>>
>>>> We had many discussions on mailing lists, thus simplifying the review -
>>>> I recommend the first choice. For a better recommendation you should say
>>>> a bit more about the block in different SoCs.
>>>
>>> I'll try to say a bit more about the PHY block, but in fact, it's not
>>> just about differences between SoCs. On the same SoC, 2 different PHYs
>>> may have different features/capabilities.
>>>
>>> For example, on MT8365, There are 2 PHYs: CSI0 and CSI1.  CSI0 can
>>> function as a C-PHY or a D-PHY, but CSI1 can only function as D-PHY
>>> (used as the example in the binding patch[1].)  On another related SoC,
>>> there are 3 PHYs, where CSI0 is C-D but CSI1 & CSI2 are only D.
>>>
>>> So that's why it seems (at least to me) that while we need SoC
>>> compatible, it's not enough.  We also need properties to describe
>>> PHY-specific features (e.g. C-D PHY)
>>
>> I recall the same or very similar case... It bugs me now, but
>> unfortunately I cannot find it.
>>
>>>
>>> Of course, we could rely only on SoC-specific compatibles describe this.
>>> But then driver will need an SoC-specific table with the number of PHYs
>>> and per-PHY features for each SoC encoded in the driver.  Since the
>>> driver otherwise doesn't (and shouldn't, IMHO) need to know how many
>>> PHYs are on each SoC, I suggested to Julien that perhaps the additional
>>> propery was the better solution.
>>
>> Phys were modeled as separate device instances, so you would need
>> difference in compatible to figure out which phy is it.
>>
>> Other way could be to create device for all phys and use phy-cells=1.
>> Whether it makes sense, depends on the actual datasheet - maybe the
>> split phy per device is artificial? There is one PHY block with two
>> address ranges for each PHY - CSI0 and CSI1 - but it is actually one
>> block? You should carefully check this because once design is chosen,
>> you won't be able to go back to other and it might be a problem (e.g.
>> there is some top-level block for powering on all CSI instances).
> 
> We're pretty sure these are multiple instances of the IP block as they
> can operate completely independently. 
> 
>>>
>>> To me it seems redundant to have the driver encode PHYs-per-SoC info,
>>> when the per-SoC DT is going to have the same info, so my suggestion was
>>> to simplify the driver and have this kind of hardware description in the
>>> DT, and keep the driver simple, but we are definitely open to learning
>>> the "right way" of doing this.
>>
>> The property then is reasonable. It should not be bool, though, because
>> it does not scale. There can be next block which supports only D-PHY on
>> CSI0 and C-PHY on CSI1? Maybe some enum or list, depending on possible
>> configurations.
> 
> OK, looks like include/dt-bindings/phy/phy.y already has
> 
>   #define PHY_TYPE_DPHY		10
>   #define PHY_TYPE_CPHY		11
> 
> we'll add a PHY_TYPE_CDPHY and use that.   Sound reasonable?

Yes. Currently it is usually used as phy-cells argument (after the phy
number/lane/ID), but cdns,phy-type and intel,phy-mode use it directly as
property in provider. In both cases they have a bit different meaning
than yours. You want to list all supported modes or narrow/restrict
them. Maybe hisilicon,fixed-mode fits your purpose?

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: phy: add mediatek mipi csi driver v 0.5

[Julien Stephan]

On Tue, May 30, 2023 at 10:53:31AM +0200, Krzysztof Kozlowski wrote:
> On 22/05/2023 21:15, Kevin Hilman wrote:
> > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes:
> >
> >> On 16/05/2023 23:31, Kevin Hilman wrote:
> >>
> >>>> Third is to use versioned IP blocks.
> >>>>
> >>>> The second case also would work, if it is applicable to you (you really
> >>>> have fallback matching all devices). Third solution depends on your
> >>>> versioning and Rob expressed dislike about it many times.
> >>>>
> >>>> We had many discussions on mailing lists, thus simplifying the review -
> >>>> I recommend the first choice. For a better recommendation you should say
> >>>> a bit more about the block in different SoCs.
> >>>
> >>> I'll try to say a bit more about the PHY block, but in fact, it's not
> >>> just about differences between SoCs. On the same SoC, 2 different PHYs
> >>> may have different features/capabilities.
> >>>
> >>> For example, on MT8365, There are 2 PHYs: CSI0 and CSI1.  CSI0 can
> >>> function as a C-PHY or a D-PHY, but CSI1 can only function as D-PHY
> >>> (used as the example in the binding patch[1].)  On another related SoC,
> >>> there are 3 PHYs, where CSI0 is C-D but CSI1 & CSI2 are only D.
> >>>
> >>> So that's why it seems (at least to me) that while we need SoC
> >>> compatible, it's not enough.  We also need properties to describe
> >>> PHY-specific features (e.g. C-D PHY)
> >>
> >> I recall the same or very similar case... It bugs me now, but
> >> unfortunately I cannot find it.
> >>
> >>>
> >>> Of course, we could rely only on SoC-specific compatibles describe this.
> >>> But then driver will need an SoC-specific table with the number of PHYs
> >>> and per-PHY features for each SoC encoded in the driver.  Since the
> >>> driver otherwise doesn't (and shouldn't, IMHO) need to know how many
> >>> PHYs are on each SoC, I suggested to Julien that perhaps the additional
> >>> propery was the better solution.
> >>
> >> Phys were modeled as separate device instances, so you would need
> >> difference in compatible to figure out which phy is it.
> >>
> >> Other way could be to create device for all phys and use phy-cells=1.
> >> Whether it makes sense, depends on the actual datasheet - maybe the
> >> split phy per device is artificial? There is one PHY block with two
> >> address ranges for each PHY - CSI0 and CSI1 - but it is actually one
> >> block? You should carefully check this because once design is chosen,
> >> you won't be able to go back to other and it might be a problem (e.g.
> >> there is some top-level block for powering on all CSI instances).
> >
> > We're pretty sure these are multiple instances of the IP block as they
> > can operate completely independently.
> >
> >>>
> >>> To me it seems redundant to have the driver encode PHYs-per-SoC info,
> >>> when the per-SoC DT is going to have the same info, so my suggestion was
> >>> to simplify the driver and have this kind of hardware description in the
> >>> DT, and keep the driver simple, but we are definitely open to learning
> >>> the "right way" of doing this.
> >>
> >> The property then is reasonable. It should not be bool, though, because
> >> it does not scale. There can be next block which supports only D-PHY on
> >> CSI0 and C-PHY on CSI1? Maybe some enum or list, depending on possible
> >> configurations.
> >
> > OK, looks like include/dt-bindings/phy/phy.y already has
> >
> >   #define PHY_TYPE_DPHY		10
> >   #define PHY_TYPE_CPHY		11
> >
> > we'll add a PHY_TYPE_CDPHY and use that.   Sound reasonable?
>
> Yes. Currently it is usually used as phy-cells argument (after the phy
> number/lane/ID), but cdns,phy-type and intel,phy-mode use it directly as
> property in provider. In both cases they have a bit different meaning
> than yours. You want to list all supported modes or narrow/restrict
> them. Maybe hisilicon,fixed-mode fits your purpose?
>

Hi Krzysztof,

Thanks for the suggestion, using something like hisilicon,fixed-node
looks like a good fit.
With mediatek,fixed-node, by default CSI node will be considered as
CD-PHY capable (unless the fixed-mode property is set.) so I won't need
anymore the new define PHY_TYPE_CDPHY introduced in v3.

Also introducing mediatek,fixed-mode suggests that PHYs not declaring
the fixed mode property support mode selection, so I suggest to add a
phy argument (#phy-cells = <1>) to select the mode (D or C mode).
Exactly what is done by the hsilicon driver.

How does that sound to you?

Best,
Julien
> Best regards,
> Krzysztof
>

Re: [PATCH v2 1/2] dt-bindings: phy: add mediatek mipi csi driver v 0.5

[Rob Herring]

On Mon, Jun 05, 2023 at 10:44:22AM +0200, Julien Stephan wrote:
> On Tue, May 30, 2023 at 10:53:31AM +0200, Krzysztof Kozlowski wrote:
> > On 22/05/2023 21:15, Kevin Hilman wrote:
> > > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes:
> > >
> > >> On 16/05/2023 23:31, Kevin Hilman wrote:
> > >>
> > >>>> Third is to use versioned IP blocks.
> > >>>>
> > >>>> The second case also would work, if it is applicable to you (you really
> > >>>> have fallback matching all devices). Third solution depends on your
> > >>>> versioning and Rob expressed dislike about it many times.
> > >>>>
> > >>>> We had many discussions on mailing lists, thus simplifying the review -
> > >>>> I recommend the first choice. For a better recommendation you should say
> > >>>> a bit more about the block in different SoCs.
> > >>>
> > >>> I'll try to say a bit more about the PHY block, but in fact, it's not
> > >>> just about differences between SoCs. On the same SoC, 2 different PHYs
> > >>> may have different features/capabilities.
> > >>>
> > >>> For example, on MT8365, There are 2 PHYs: CSI0 and CSI1.  CSI0 can
> > >>> function as a C-PHY or a D-PHY, but CSI1 can only function as D-PHY
> > >>> (used as the example in the binding patch[1].)  On another related SoC,
> > >>> there are 3 PHYs, where CSI0 is C-D but CSI1 & CSI2 are only D.
> > >>>
> > >>> So that's why it seems (at least to me) that while we need SoC
> > >>> compatible, it's not enough.  We also need properties to describe
> > >>> PHY-specific features (e.g. C-D PHY)
> > >>
> > >> I recall the same or very similar case... It bugs me now, but
> > >> unfortunately I cannot find it.
> > >>
> > >>>
> > >>> Of course, we could rely only on SoC-specific compatibles describe this.
> > >>> But then driver will need an SoC-specific table with the number of PHYs
> > >>> and per-PHY features for each SoC encoded in the driver.  Since the
> > >>> driver otherwise doesn't (and shouldn't, IMHO) need to know how many
> > >>> PHYs are on each SoC, I suggested to Julien that perhaps the additional
> > >>> propery was the better solution.
> > >>
> > >> Phys were modeled as separate device instances, so you would need
> > >> difference in compatible to figure out which phy is it.
> > >>
> > >> Other way could be to create device for all phys and use phy-cells=1.
> > >> Whether it makes sense, depends on the actual datasheet - maybe the
> > >> split phy per device is artificial? There is one PHY block with two
> > >> address ranges for each PHY - CSI0 and CSI1 - but it is actually one
> > >> block? You should carefully check this because once design is chosen,
> > >> you won't be able to go back to other and it might be a problem (e.g.
> > >> there is some top-level block for powering on all CSI instances).
> > >
> > > We're pretty sure these are multiple instances of the IP block as they
> > > can operate completely independently.
> > >
> > >>>
> > >>> To me it seems redundant to have the driver encode PHYs-per-SoC info,
> > >>> when the per-SoC DT is going to have the same info, so my suggestion was
> > >>> to simplify the driver and have this kind of hardware description in the
> > >>> DT, and keep the driver simple, but we are definitely open to learning
> > >>> the "right way" of doing this.
> > >>
> > >> The property then is reasonable. It should not be bool, though, because
> > >> it does not scale. There can be next block which supports only D-PHY on
> > >> CSI0 and C-PHY on CSI1? Maybe some enum or list, depending on possible
> > >> configurations.
> > >
> > > OK, looks like include/dt-bindings/phy/phy.y already has
> > >
> > >   #define PHY_TYPE_DPHY		10
> > >   #define PHY_TYPE_CPHY		11
> > >
> > > we'll add a PHY_TYPE_CDPHY and use that.   Sound reasonable?

No, because these defines are the 1 mode used for a specific connection, 
not the set of supported modes.

> >
> > Yes. Currently it is usually used as phy-cells argument (after the phy
> > number/lane/ID), but cdns,phy-type and intel,phy-mode use it directly as
> > property in provider. In both cases they have a bit different meaning
> > than yours. You want to list all supported modes or narrow/restrict
> > them. Maybe hisilicon,fixed-mode fits your purpose?

There is also 'phy-type' to set the mode/type in the provider.

If we need to list all possible modes, then you need to justify why 
that's needed and then define a property which is a mask of the 
existing type defines.

> >
> 
> Hi Krzysztof,
> 
> Thanks for the suggestion, using something like hisilicon,fixed-node
> looks like a good fit.
> With mediatek,fixed-node, by default CSI node will be considered as
> CD-PHY capable (unless the fixed-mode property is set.) so I won't need
> anymore the new define PHY_TYPE_CDPHY introduced in v3.
> 
> Also introducing mediatek,fixed-mode suggests that PHYs not declaring
> the fixed mode property support mode selection, so I suggest to add a
> phy argument (#phy-cells = <1>) to select the mode (D or C mode).
> Exactly what is done by the hsilicon driver.
> 
> How does that sound to you?

I don't follow the need for fixed-mode, but agree you should use a phy 
arg. Can't you just assume the D-PHY only instance will only have D-PHY 
for the arg?

Rob

Re: [PATCH v2 1/2] dt-bindings: phy: add mediatek mipi csi driver v 0.5

[Chunfeng Yun (云春峰)]

On Tue, 2023-05-16 at 11:41 +0200, Julien Stephan wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Tue, May 16, 2023 at 10:07:47AM +0200, Krzysztof Kozlowski wrote:
> > On 15/05/2023 11:05, Julien Stephan wrote:
> > > From: Florian Sylvestre <fsylvestre@baylibre.com>
> > > 
> > > This adds the bindings, for the MIPI CD-PHY module v 0.5 embedded
> > > in
> > > some Mediatek soc, such as the mt8365
> > > 
> > > Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
> > > Signed-off-by: Julien Stephan <jstephan@baylibre.com>
> > 
> > What are the changes? IOW: changelog here or in cover letter.
> > 
> 
> Hi Krzysztof,
> I added a changelog in the cover letter, but I will try to be more
> descritpive next time. Changes from v1 are mainly style issues fixed
> (mostly from your first review)
> 
> > Subject: you have some multiple spaces.
> > 
> > Subject: drop driver. Bindings are not for drivers.
> > 
> > > ---
> > >  .../phy/mediatek,phy-mipi-csi-0-5.yaml        | 62
> > > +++++++++++++++++++
> > >  MAINTAINERS                                   |  6 ++
> > >  2 files changed, 68 insertions(+)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-
> > > 5.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/phy/mediatek,phy-
> > > mipi-csi-0-5.yaml
> > > b/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-
> > > 5.yaml
> > > new file mode 100644
> > > index 000000000000..5aa8c0b41cdf
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-
> > > csi-0-5.yaml
> > > @@ -0,0 +1,62 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-Only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: 
> > > https://urldefense.com/v3/__http://devicetree.org/schemas/phy/mediatek,phy-mipi-csi-0-5.yaml*__;Iw!!CTRNKA9wMg0ARbw!kUABjbQO6-_jrANKiqoLPxZkh_0LAplQwlgCG9lhmCKm2ec8pCItYUB3-zQ2PGBnlAcgsBhwrOgi5qeilrddZ5s$
> > > +$schema: 
> > > https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!kUABjbQO6-_jrANKiqoLPxZkh_0LAplQwlgCG9lhmCKm2ec8pCItYUB3-zQ2PGBnlAcgsBhwrOgi5qeiABoSu-M$
> > > +
> > > +title: Mediatek Sensor Interface MIPI CSI CD-PHY
> > > +
> > > +maintainers:
> > > +  - Julien Stephan <jstephan@baylibre.com>
> > > +  - Andy Hsieh <andy.hsieh@mediatek.com>
> > > +
> > > +description:
> > > +  The SENINF CD-PHY is a set of CD-PHY connected to the SENINF
> > > CSI-2
> > > +  receivers. The number of PHYs depends on the SoC model.
> > > +  Depending on the soc model, each PHYs can support CDPHY or
> > > DPHY only
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - mediatek,phy-mipi-csi-0-5
> > 
> > SoC based compatibles. 0-5 is odd.
> > 
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  '#phy-cells':
> > > +    const: 0
> > > +
> > > +  mediatek,is_cdphy:
> > 
> > No underscores in node names.
> > 
> > > +    description:
> > > +      Specify if the current phy support CDPHY configuration
> > 
> > Why this cannot be implied from compatible? Add specific
> > compatibles.
> > 
> > 
> 
> This cannot be implied by compatible because the number of phys
> depends
> on the soc and each phy can be either D-PHY only or CD-PHY capable.
> For example mt8365 has 2 phy: CSI0 and CSI1. CSI1 is DPHY only and
> CSI0 is CD-PHY

Do the user need to know which type phy he will use?

> 
> Regards,
> Julien
> > > +    type: boolean
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - '#phy-cells'
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    soc {
> > > +      #address-cells = <2>;
> > > +      #size-cells = <2>;
> > > +
> > > +      mipi_rx_csi0: mipi_rx_csi0@11c10000 {
> > 
> > No underscores in node names. How this is v2?
> > 
> > > +        compatible = "mediatek,phy-mipi-csi-0-5";
> > > +        reg = <0 0x11C10000 0 0x2000>;
> > > +        status = "okay";
> > 
> > Drop
> > 
> > > +        mediatek,is_cdphy;
> > > +        #phy-cells = <0>;
> > > +      };
> > > +
> > > +      mipi_rx_csi1: mipi-rx-csi1@11c12000 {
> > > +        compatible = "mediatek,phy-mipi-csi-0-5";
> > > +        reg = <0 0x11C12000 0 0x2000>;
> > > +        status = "disabled";
> > 
> > ???
> > 
> > 
> > Best regards,
> > Krzysztof
> >