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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 16 May 2023 10:00:39 -0700
From:   Kevin Hilman <khilman@...libre.com>
To:     Julien Stephan <jstephan@...libre.com>
Cc:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        robh@...nel.org, chunkuang.hu@...nel.org,
        linux-mediatek@...ts.infradead.org,
        Florian Sylvestre <fsylvestre@...libre.com>,
        Chunfeng Yun <chunfeng.yun@...iatek.com>,
        Andy Hsieh <andy.hsieh@...iatek.com>,
        Vinod Koul <vkoul@...nel.org>,
        Kishon Vijay Abraham I <kishon@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        "moderated list:ARM/Mediatek USB3 PHY DRIVER" 
        <linux-arm-kernel@...ts.infradead.org>,
        "open list:GENERIC PHY FRAMEWORK" <linux-phy@...ts.infradead.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/2] dt-bindings: phy: add mediatek mipi csi driver v
 0.5

Krzysztof Kozlowski <krzysztof.kozlowski@...aro.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@...libre.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@...libre.com>
>>>> Signed-off-by: Julien Stephan <jstephan@...libre.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@...libre.com>
>>>> +  - Andy Hsieh <andy.hsieh@...iatek.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