Message-ID: <38824a96-804d-84a2-2750-be1325b2e1d2@linaro.org>
Date:   Thu, 16 Feb 2023 11:03:39 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH 1/2] dt-bindings: regulator: Add bindings for Richtek
 RT5739
Content-Language: en-US
To:     ChiYuan Huang <cy_huang@richtek.com>
Cc:     broonie@kernel.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, lgirdwood@gmail.com,
        u0084500@gmail.com, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org
References: <1676426457-1389-1-git-send-email-cy_huang@richtek.com>
 <1676426457-1389-2-git-send-email-cy_huang@richtek.com>
 <9c1f4d0e-83c9-014e-bdb1-38b4b0fb2bbf@linaro.org>
 <20230216095714.GA14578@linuxcarl2.richtek.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20230216095714.GA14578@linuxcarl2.richtek.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 16/02/2023 10:57, ChiYuan Huang wrote:
> On Thu, Feb 16, 2023 at 10:12:15AM +0100, Krzysztof Kozlowski wrote:
>> On 15/02/2023 03:00, cy_huang@richtek.com wrote:
>>> From: ChiYuan Huang <cy_huang@richtek.com>
>>>
>>> Add the binding document for Richtek RT5739.
>>
>> Subject: drop second/last, redundant "bindings for". The "dt-bindings"
>> prefix is already stating that these are bindings.
>>
> Then, refine it to "dt-bindings: regulator: Add Richtek RT5739 document" 

I propose also to drop "document" - it is also redundant. Can bindings
be something else than document?


>>>
>>> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
>>> ---
>>>  .../bindings/regulator/richtek,rt5739.yaml         | 80 ++++++++++++++++++++++
>>>  1 file changed, 80 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/regulator/richtek,rt5739.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/regulator/richtek,rt5739.yaml b/Documentation/devicetree/bindings/regulator/richtek,rt5739.yaml
>>> new file mode 100644
>>> index 00000000..7dc4f78
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/regulator/richtek,rt5739.yaml
>>> @@ -0,0 +1,80 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/regulator/richtek,rt5739.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Richtek RT5739 2.4MHz 3.5A Step-Down Converter
>>> +
>>> +maintainers:
>>> +  - ChiYuan Huang <cy_huang@richtek.com>
>>> +
>>> +description: |
>>> +  The RT5739 is a step-down switching voltage regulator that delivers a
>>> +  digitally programmable output from an input voltage supply of 2.5V to 5.5V.
>>> +  The output voltage is programmed through an I2C interface capable of
>>> +  operating up to 3.4MHz.
>>> +
>>> +  Using a proprietary architecture with synchronous rectification, the RT5739
>>> +  is capable of delivering 3.5A continuously at over 80% efficiency,
>>> +  maintaining that efficiency at load current as low as 10mA. The regulator
>>> +  operates at a normal fixed frequency of 2.4MHz, which reduces the value of
>>> +  the external components. 
>>
>> Can we drop the marketing from kernel? Last part of sentence is not
>> related to this submission at all. The internal frequency also looks
>> unrelated to the topic...
>>
> Okay, too much marketing text. I'll shorten it and simply describe the function or
> voltage range only. 
>>> Additional output capacitance can be added to
>>> +  improve regulation during load transients without affecting stability.
>>> +
>>> +allOf:
>>> +  - $ref: regulator.yaml#
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - richtek,rt5739
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  enable-gpios:
>>> +    maxItems: 1
>>> +
>>> +  richtek,vsel-active-high:
>>> +    description: |
>>> +      If property is present, use the 'VSEL1' register group for buck control.
>>> +      Else, use the 'VSEL0' register group. This depends on external hardware
>>> +      'VSEL' pin connecton.
>>> +    type: boolean
>>> +
>>> +  regulator-allowed-modes:
>>> +    description: |
>>> +      buck allowed operating mode
>>> +        0: Auto PFM/PWM mode
>>> +        1: Forced PWM mode
>>> +    maxItems: 2
>>> +    items:
>>> +      enum: [0, 1]
>>
>> So you always require two items? Thus I wonder what's the point of
>> having it in DT? To skip the property entirely if none of the modes are
>> allowed?
>>
> Not always need two. So does it mean no need to describe the 'maxItems' and 'Items'.

Your minItems is 2, so you always need two. If you accept one, the add
minItems: 1.


Best regards,
Krzysztof