linux-kernel - Re: [PATCH 06/11] iio: adc: Add QCOM PMIC5 Gen3 ADC bindings

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]

Message-ID: <a4d5293f-43ee-487c-a540-5a1f65e3c7d6@linaro.org>
Date:   Thu, 16 Nov 2023 12:40:13 +0100
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Jishnu Prakash <quic_jprakash@...cinc.com>, agross@...nel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linus.walleij@...aro.org, Jonathan.Cameron@...wei.com,
        sboyd@...nel.org, dmitry.baryshkov@...aro.org,
        quic_subbaram@...cinc.com, quic_collinsd@...cinc.com,
        quic_kamalw@...cinc.com, marijn.suijten@...ainline.org,
        andriy.shevchenko@...ux.intel.com,
        Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Luca Weiss <luca@...tu.xyz>, linux-iio@...r.kernel.org,
        linux-arm-msm@...r.kernel.org
Cc:     linux-arm-msm-owner@...r.kernel.org
Subject: Re: [PATCH 06/11] iio: adc: Add QCOM PMIC5 Gen3 ADC bindings

On 16/11/2023 04:23, Jishnu Prakash wrote:
> Hi Krzysztof,
> 
> On 10/23/2023 12:06 PM, Krzysztof Kozlowski wrote:
>> On 23/10/2023 08:14, Jishnu Prakash wrote:
>>> Hi Krzysztof,
>>>
>>> On 7/9/2023 10:53 PM, Krzysztof Kozlowski wrote:
>>>
>>>>>      reg:
>>>>>        description: VADC base address in the SPMI PMIC register map
>>>>> -    maxItems: 1
>>>>> +    minItems: 1
>>>> Why? This does not make any sense. With previous patches it looks like
>>>> random set of changes.
>>> The idea here is to convey that reg can have multiple values for ADC5
>>> Gen3 as there can be more than one peripheral used for ADC, so there can
>>> be multiple base addresses. I'll try to make this more clear in the next
>>> patchset.
>> You cannot remove constraints from an entry.
> 
> 
> In this case, minItems: 1 will remain true for all other ADC devices 
> documented here, but it will not be true for ADC5 Gen3, as this one can 
> have multiple base addresses if more than one SDAM is used for ADC. I'll 
> update this separately for each compatible, keeping it the same for the 
> older ones, hope that should work.

You responded like you disagree with me, so just in case:  my comment
stays. If you resend the same, I will NAK it.

> 
> 
>>>>>    
>>>>>      '#address-cells':
>>>>>        const: 1
>>>>> @@ -38,6 +39,12 @@ properties:
>>>>>      '#size-cells':
>>>>>        const: 0
>>>>>    
>>>>>    
>>>>> +      qcom,adc-tm-type:
>>>>> +        description: |
>>>>> +            Indicates if ADC_TM monitoring is done on this channel.
>>>> Description does not match property name.
>>> You mean it sounds more like an enum which can take several values
>>> rather than just a boolean? I can update it to "qcom,adc-tm" if that
>>> looks better.
>> The property name suggests this is type of monitoring. Property
>> description says this will enable ADC_TM monitoring. These two do not match.
>>
>> Except that I wonder now whether this is a property of hardware at
>> all... What is this monitoring? By the driver?
> 
> 
> The property description is right, this property is used to indicate 
> that one of the configurable channels on the ADC SDAMs will be used for 
> ADC_TM functionality, for periodically monitoring this particular ADC 
> channel . This is the exact same functionality as in the existing QCOM 
> ADC_TM device, documented at 
> devicetree/bindings/thermal/qcom-spmi-adc-tm5.yaml. I'll mention this 
> too in the description.
> 
> It can be considered a property of the hardware as the monitoring is 
> done by a sequence under PBS (Programmable Boot Sequence, can be 
> considered firmware), which periodically gets the channel reading and 
> checks it against upper/lower thresholds set by clients of this driver, 
> for threshold violations.
> 
> 
>> ...
>>
>>>>>        then:
>>>>>          patternProperties:
>>>>> @@ -299,7 +315,7 @@ examples:
>>>>>                    label = "xo_therm";
>>>>>                };
>>>>>    
> 
>>>>> diff --git a/include/dt-bindings/iio/qcom,spmi-adc5-gen3-pm8550.h b/include/dt-bindings/iio/qcom,spmi-adc5-gen3-pm8550.h
>>>>> new file mode 100644
>>>>> index 000000000000..74e6e2f6f9ed
>>>>> --- /dev/null
>>>>> +++ b/include/dt-bindings/iio/qcom,spmi-adc5-gen3-pm8550.h
>>>>> @@ -0,0 +1,48 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>>> Dual license.
>>> I think we do have an internal rule by which we do have to add these two
>>> licenses....I'll check again and update them if required.
>> Just to be clear: your internal rules are your internal affair. We
>> expect here dual license.
> 
> 
> I misunderstood what you meant earlier, I understand now that 
> "GPL-2.0-only" is wrong, I'll update it.

If only you run checkpatch before sending patches...

Best regards,
Krzysztof