Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <a84df850-149e-9656-43fa-1f040368a9f1@linaro.org>
Date:   Fri, 29 Apr 2022 22:22:10 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH v2 1/2] dt-bindings: mtd: partitions: Add binding for
 Sercomm parser
Content-Language: en-US
To:     Mikhail Zhilkin <csharper2005@gmail.com>,
        Miquel Raynal <miquel.raynal@bootlin.com>,
        Richard Weinberger <richard@nod.at>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc:     NOGUCHI Hiroshi <drvlabo@gmail.com>, Karim <Karimdplay@gmail.com>,
        M <x1@disroot.org>, linux-mtd@lists.infradead.org,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
References: <20220406195557.1956-1-csharper2005@gmail.com>
 <20220406195946.2019-1-csharper2005@gmail.com>
 <8d0d8b27-35ff-3693-cf80-897b80c26b4e@linaro.org>
 <57bebf2f-af4c-b2d9-10e5-19e5104946fb@gmail.com>
 <29cfa017-bbaf-3aba-fe1d-06771957dbaa@linaro.org>
 <1b391399-984b-7a63-3265-62ef09caec39@gmail.com>
 <bd533827-b575-caad-c230-af060851b231@gmail.com>
 <db70e53b-7484-43bf-e9c8-b6a2dce1fbb5@linaro.org>
 <25da3f43-c46e-8108-2da9-0e4f2b7cc1a4@gmail.com>
 <b279040a-a782-a2ca-2acb-2d8f07709081@linaro.org>
 <1c19faf9-fc1c-9adf-d038-97b673a8f0be@gmail.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <1c19faf9-fc1c-9adf-d038-97b673a8f0be@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 29/04/2022 17:26, Mikhail Zhilkin wrote:
> On 4/29/2022 9:46 AM, Krzysztof Kozlowski wrote:
> 
>>>> I am sorry, but you changed now a lot in the bindings and it looks
>>>> entirely different. Things previously being correct now are wrong, so
>>>> rather start from your old bindings...
>>>
>>> Looks like I'm a bit confused... I use dual "compatible" in my real dts
>>> and I realized that:
>>>
>>> 1. Therefore I have to use  dual "compatible" in example too:
>>>
>>> compatible = "sercomm,sc-partitions", "fixed-partitions";
>>>
>>> 2. When I'm trying to reuse "fixed-partitions" compatible from
>>> fixed-partitions.yaml in my new .yaml I get "too long" errors.
>> Yes, the fixed-partitions.yaml would have to be changed to allow extension.
> 
> Well.
> 
>>> Real dts:
>>>
>>> Link:
>>> https://github.com/openwrt/openwrt/blob/edcc1a9a734bb3fcdc9242025290d3f173e71b78/target/linux/ramips/dts/mt7621_beeline_smartbox-giga.dts#L79
>>>
>>> So, I currently found another solution - to extend fixed-partitions.yaml
>>> with "sercomm,sc-partitions". Is It ok from your side? Can I use this
>>> code in v3?
>> Not really, I don't understand why do you need it 
> 
> The main idea is keeping original Sercomm firmware behavior:
> 
> 1. If dynamic partition map found then use offsets and mtd sizes stored
> in partition map. It's provided by "sercomm,sc-partitions" compatible.
> 
> 2. If dynamic partition map doesn't exist or broken then default values
> (from dts) are used. It's provided by "fixed-partitions" compatible.

Then you need to adjust fixed-partitions for such case. See syscon case
(all over the tree and Documentation/devicetree/bindings/mfd/syscon.yaml).

> 
>> and it does not
>> include our previous talks.
> 
> At the time, I didn't realize how important is it. Understanding began
> to come after dozens of experiments and checking the similar Linux patches.
> 
>>> diff --git
>>> a/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
>>> b/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
>>> index ea4cace6a955..9eebe39a57fb 100644
>>> --- a/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
>>> +++ b/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
>>> @@ -19,7 +19,11 @@ maintainers:
>>>  
>>>  properties:
>>>    compatible:
>>> -    const: fixed-partitions
>>> +    oneOf:
>>> +      - const: fixed-partitions
>>> +      - items:
>>> +          - const: sercomm,sc-partitions
>>> +          - const: fixed-partitions
>>>  
>>>    "#address-cells": true
>>>  
>>> @@ -27,7 +31,18 @@ properties:
>>>  
>>>  patternProperties:
>>>    "@[0-9a-f]+$":
>>> -    $ref: "partition.yaml#"
>>> +    allOf:
>>> +      - $ref: "partition.yaml#"
>>> +      - if:
>>> +          properties:
>>> +            compatible:
>>> +              contains:
>>> +                const: sercomm,sc-partitions
>>> +        then:
>>> +          properties:
>>> +            scpart-id:
>> It still misses vendor prefix and we agreed you don't need it, didn't we?
> 
> Do you mean "sercomm" vendor prefix? If so then we agreed that I include
> it in a separate patch:

There was some misunderstanding then. We talk here about scpart-id name.
Adding vendor prefix cannot be a separate patch because it does not make
much sense. You add new property with wrong name and immediately
change/fix it in next patch.

No, it should have proper name since beginning. The property is not used
in the kernel.

> 
> Link:
> https://lore.kernel.org/linux-mtd/1b391399-984b-7a63-3265-62ef09caec39@gmail.com/
> 
> I'm going to send it in v3:
> 
> ---
> dt-bindings: Add Sercomm (Suzhou) Corporation vendor prefix
> 
> Update Documentation/devicetree/bindings/vendor-prefixes.yaml to include
> "sercomm" as a vendor prefix for "Sercomm (Suzhou) Corporation".
> Company website:
> Link: https://www.sercomm.com/
> 
> Signed-off-by: Mikhail Zhilkin <csharper2005@gmail.com>
> ---
>  Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> index 01430973ecec..65ff22364fb3 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -1082,6 +1082,8 @@ patternProperties:
>      description: Sensirion AG
>    "^sensortek,.*":
>      description: Sensortek Technology Corporation
> +  "^sercomm,.*":
> +    description: Sercomm (Suzhou) Corporation

This can be separate patch, but it's separate issue...


Best regards,
Krzysztof