Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 9E02AC433D2
Subject: Re: [PATCH 1/3] iio: adc: Convert the QCOM SPMI ADC bindings to .yaml
 format
To:     Amit Kucheria <amit.kucheria@linaro.org>
Cc:     Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>,
        Matthias Kaehlcke <mka@chromium.org>,
        Linus Walleij <linus.walleij@linaro.org>,
        Stephen Boyd <sboyd@codeaurora.org>,
        Jonathan.Cameron@huawei.com, smohanad@codeaurora.org,
        kgunda@codeaurora.org, aghayal@codeaurora.org,
        Jonathan Cameron <jic23@kernel.org>,
        Hartmut Knaack <knaack.h@gmx.de>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
        Rob Herring <robh+dt@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Andy Gross <andy.gross@linaro.org>, linux-iio@vger.kernel.org,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>,
        linux-arm-msm-owner@vger.kernel.org
References: <1585064650-16235-1-git-send-email-jprakash@codeaurora.org>
 <1585064650-16235-2-git-send-email-jprakash@codeaurora.org>
 <CAHLCerML7vR9X_YxAg=S71n2NiY88toZyGDhxZaUZAvnNX2P+g@mail.gmail.com>
From:   Jishnu Prakash <jprakash@codeaurora.org>
Message-ID: <69b882b0-56ad-1b93-0a9d-2c7f96dd9d32@codeaurora.org>
Date:   Mon, 6 Apr 2020 17:14:47 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.6.0
MIME-Version: 1.0
In-Reply-To: <CAHLCerML7vR9X_YxAg=S71n2NiY88toZyGDhxZaUZAvnNX2P+g@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Amit,

On 4/3/2020 5:34 PM, Amit Kucheria wrote:
>
>> +required:
>> +  - compatible
>> +  - reg
>> +  - '#address-cells'
>> +  - '#size-cells'
>> +  - '#io-channel-cells'
>> +
>> +patternProperties:
>> +  "^[a-z0-9-_@]$":
>> +    type: object
>> +    description: |
>> +      Represents the external channels which are connected to the ADC.
>> +      For compatible property "qcom,spmi-vadc" following channels, also known as
>> +      reference point channels, are used for result calibration and their channel
>> +      configuration nodes should be defined:
>> +      VADC_REF_625MV and/or VADC_SPARE1(based on PMIC version) VADC_REF_1250MV,
>> +      VADC_GND_REF and VADC_VDD_VADC.
> Instead of this note for "qcom,spmi-vadc", you can enforce this
> through checks in YAML grammar.
>
> A simple example can be found in
> Documentation/devicetree/bindings/thermal/qcom-tsens.yaml. Look for
> the if, then, else clause which determines how many interrupts need to
> be defined.

I have gone through tsens and other examples, but I'm not able to get a 
way to apply this kind of constraint, on what child nodes should be present.

In this case, the constraint would have to be that for compatible 
property "qcom,spmi-vadc", there should be at least four child nodes and 
those four should have their "reg" property fixed to the channel values 
mentioned above. I can see how to apply constraints on a single property 
like interrupts in tsens, but I'm not sure if there is a way to specify 
a lower limit to the number of child nodes or something like "there 
should be at least one child node with value 0x9 for its "reg" 
property". I could not find any examples with constraints placed on  
number of occurrences of a child node.

Can you please share an example of such a constraint if you are aware of 
any or suggest some way by which this kind of constraint can be specified?

>
>> +
>> +    properties:
>> +      reg:
>> +        description: |
>> +          ADC channel number.
>> +          See include/dt-bindings/iio/qcom,spmi-vadc.h
>> +
>> +      label:
>> +        description: |
>> +            ADC input of the platform as seen in the schematics.
>> +            For thermistor inputs connected to generic AMUX or GPIO inputs
>> +            these can vary across platform for the same pins. Hence select
>> +            the platform schematics name for this channel. It is required
>> +            for "qcom,spmi-adc5" and "qcom,spmi-adc-rev2".
>> +        allOf:
>> +          - $ref: /schemas/types.yaml#/definitions/string
> You shouldn't need allOf here.
>
> Just a "$ref: /schemas/types.yaml#/definitions/string" should be fine.
> And move it above the description.
>
> Same for all the uses of allOf below.
I'll make the change in the next post.
>
>
>> +      qcom,decimation:
>> +        description: |
>> +            This parameter is used to decrease ADC sampling rate.
>> +            Quicker measurements can be made by reducing decimation ratio.
>> +            - For compatible property "qcom,spmi-vadc", valid values are
>> +              512, 1024, 2048, 4096. If property is not found, default value
>> +              of 512 will be used.
>> +            - For compatible property "qcom,spmi-adc5", valid values are 250, 420
>> +              and 840. If property is not found, default value of 840 is used.
>> +            - For compatible property "qcom,spmi-adc-rev2", valid values are 256,
>> +              512 and 1024. If property is not present, default value is 1024.
>> +        allOf:
>> +          - $ref: /schemas/types.yaml#/definitions/uint32
>> +
> As pointed out by Jonathon, please enforce these by keying off the
> compatible property.
I'll do this for all properties for which it's applicable in the next post.
>
>> +      qcom,pre-scaling:
>> +        description: |
>> +            Used for scaling the channel input signal before the signal is
>> +            fed to VADC. The configuration for this node is to know the
>> +            pre-determined ratio and use it for post scaling. Select one from
>> +            the following options.
> Please improve this description from the old binding. Does <1 3> mean
> the signal is scaled 3x or 1/3x?
<1 3> means it is scaled down to 1/3rd of actual value. I'll add to the 
description in the next post.
>
>> +            <1 1>, <1 3>, <1 4>, <1 6>, <1 20>, <1 8>, <10 81>, <1 10>
>> +            If property is not found default value depending on chip will be used.
>> +        allOf:
>> +          - $ref: /schemas/types.yaml#/definitions/uint32
>> +