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
