Date: Wed, 21 Feb 2024 08:19:13 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Jishnu Prakash <quic_jprakash@...cinc.com>, jic23@...nel.org,
 robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
 andersson@...nel.org, konrad.dybcio@...aro.org, lee@...nel.org,
 andriy.shevchenko@...ux.intel.com, daniel.lezcano@...aro.org,
 dmitry.baryshkov@...aro.org
Cc: lars@...afoo.de, luca@...tu.xyz, marijn.suijten@...ainline.org,
 agross@...nel.org, sboyd@...nel.org, rafael@...nel.org, rui.zhang@...el.com,
 lukasz.luba@....com, linus.walleij@...aro.org, quic_subbaram@...cinc.com,
 quic_collinsd@...cinc.com, quic_amelende@...cinc.com,
 quic_kamalw@...cinc.com, kernel@...cinc.com, linux-kernel@...r.kernel.org,
 linux-arm-msm@...r.kernel.org, linux-arm-msm-owner@...r.kernel.org,
 linux-iio@...r.kernel.org, linux-pm@...r.kernel.org,
 devicetree@...r.kernel.org, cros-qcom-dts-watchers@...omium.org
Subject: Re: [PATCH v3 2/3] dt-bindings: iio: adc: Add support for QCOM PMIC5
 Gen3 ADC

On 21/02/2024 06:36, Jishnu Prakash wrote:
> Hi Krzysztof,
> 
> On 2/17/2024 7:43 PM, Krzysztof Kozlowski wrote:
>> On 16/02/2024 11:39, Jishnu Prakash wrote:
>>> Hi Krzysztof,
>>>
>>> On 1/4/2024 1:48 PM, Krzysztof Kozlowski wrote:
>>>> On 31/12/2023 18:12, Jishnu Prakash wrote:
>>>>> For the PMIC5-Gen3 type PMICs, ADC peripheral is present in HW for the
>>>>> following PMICs: PMK8550, PM8550, PM8550B and PM8550VX PMICs.
>>>>>
>>>>> It is similar to PMIC5-Gen2, with SW communication to ADCs on all PMICs
>>>>> going through PBS(Programmable Boot Sequence) firmware through a single
>>>>> register interface. This interface is implemented on an SDAM (Shared
>>>>> Direct Access Memory) peripheral on the master PMIC PMK8550 rather
>>>>> than a dedicated ADC peripheral.
>>>>>
>>>>> Add documentation for PMIC5 Gen3 ADC and macro definitions for ADC
>>>>> channels and virtual channels (combination of ADC channel number and
>>>>> PMIC SID number) per PMIC, to be used by clients of this device.
>>>>>
>>>>> Changes since v2:
>>>>> - Moved ADC5 Gen3 documentation into a separate new file.
>>>> Changelog goes under ---.
>>>>
>>>> Why did you do this? What is the rationale? Sorry, this patchset goes
>>>> nowhere.
>>>
>>>
>>> I'll elaborate this more in the next patchset. There are two main
>>> reasons for adding this documentation in a new file:
>>
>> This was more than a month ago? You reply to my comment with 1.5 months
>> delay?
>>
>> Sorry, I am not in the context and I am not going back to it. I have
>> many other emails where my questions are addressed faster than 1.5 months.
>>
>> The patch is not even in my mailbox, long gone.
>> Why you are making it so difficult for reviewers?
>>
>> You will get answers like I am not in context, sorry. Next time don't
>> respond after 1.5 months.
>>
> 
> You're right - I'll do my best to get back to review comments in a 
> reasonable time frame.
> 
>>
>>>
>>> 1.This device is not exactly like the existing QCOM VADC drivers as it
>>> now combines VADC functionality (reading ADC channel on client request)
>>> with ADC_TM functionality (thermal threshold monitoring).
>>
>> Does no explain touching bindings. Your drivers don't matter for bindings.
>>
>>>
>>> 2.Adding this device's bindings in the existing qcom,spmi-vadc.yaml file
>>
>> No rationale was provided in commit msg.
>>
>>> is not possible as it would require updating some of the existing
>>> top-level constraints. (for the older devices in that file, "reg" and
>>> "interrupts" can have at most one item, while this device can have more
>>> than one item under these properties.)
>>
> 
>> How is this a problem?
> 
> In qcom,spmi-vadc.yaml, we have the following top-level constraints for 
> the "reg" and "interrupts" properties:
> 
>    reg:
>      maxItems: 1
> 
>    interrupts:
>      maxItems: 1
> 
> For the ADC5 Gen3 device being added now, these constraints cannot be 
> followed always, as there may be more than one peripheral under one 
> device instance, each with a corresponding interrupt. For example, the 
> above properties could be like this for a ADC5 Gen3 device:
> 
>      reg = <0x9000>, <0x9100>;
>      interrupts = <0x0 0x90 0x1 IRQ_TYPE_EDGE_RISING>,
>                   <0x0 0x91 0x1 IRQ_TYPE_EDGE_RISING>;
> 
> 
> I could not overwrite the top-level constraints for the new device 
> "qcom,spmi-adc5-gen3" alone in qcom,spmi-vadc.yaml, so I tried to remove 
> the constraints from the top level and add them back conditionally for 
> all the device types separately, but you told me not to remove them 
> (full message: 
> https://lore.kernel.org/linux-iio/832053f4-bd5d-4e58-81bb-1a8188e7f364@linaro.org/)

Because top-level widest constraints must stay, but it is not a problem.
Most of the multi-device bindings work like this. Dozen of Qualcomm. Why
you cannot do this the same way we do for all Qualcomm devices?

> 
> Since these constraints cannot be modified for a specific new device or 

???

> removed, I think the only way to accommodate this new device is to add 
> it in its own new file.
> 
> Is this a sufficient justification for adding this documentation in a 
> new file or do you have any other suggestions?

I already gave you the suggestions and you ignored them. Do like we are
doing for all other drivers. Don't re-invent stuff. Either this fits to
existing schema or come with common schema (and then provide rationale
why it does not fit to existing one).

Best regards,
Krzysztof

Powered by blists - more mailing lists