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

[Jishnu Prakash]

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.


> > > >    
> > > >      '#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.

Thanks,

Jishnu


> Best regards,
> Krzysztof