Message-ID: <CAA8EJppsn2zsAXem-m=9U8izhtAZmVe62xS5qdkwJmFTqV30gA@mail.gmail.com>
Date: Fri, 16 Feb 2024 12:48:28 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Jishnu Prakash <quic_jprakash@...cinc.com>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>, 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, 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

Hi Jishnu,


On Fri, 16 Feb 2024 at 12:39, Jishnu Prakash <quic_jprakash@...cinc.com> wrote:

Please disable sending HTML emails in your email client. It is
generally frowned upon, it complicates replying, it breaks quotations,
etc.

>
> 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:
>
> 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).
>
> 2.Adding this device's bindings in the existing qcom,spmi-vadc.yaml file 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.)
>
>
> Changes since v1:
> - Updated properties separately for all compatibles to clarify usage
>   of new properties and updates in usage of old properties for ADC5 Gen3.
> - Avoided updating 'adc7' name to 'adc5 gen2' and just left a comment
>   mentioning this convention.
> - Used predefined channel IDs in individual PMIC channel definitions
>   instead of numeric IDs.
> - Addressed other comments from reviewers.
>
>
> +          per PMIC in the PMIC-specific files in include/dt-bindings/iio/adc.
> +
> +      label:
> +        $ref: /schemas/types.yaml#/definitions/string
>
> Why do you need it in the first place? Don't you miss some $ref?
>
>
> This is just meant to show the ADC channel name in DT for our reference. I'll check if I can use adc.yaml, which includes this property already, as a reference in this case.
>
>
> +        description: |
>
> Do not need '|' unless you need to preserve formatting. Applies everywhere.
>
>
>
> +            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.
> +
>
> +      qcom,adc-tm:
> +        description: |
> +            Indicates if ADC_TM monitoring is done on this channel.
> +            Defined for compatible property "qcom,spmi-adc5-gen3".

You are describing qcom,spmi-adc5-gen3, are you not? So this phrase
adds nothing.

> +            This is the same functionality as in the existing QCOM ADC_TM
> +            device, documented at devicetree/bindings/thermal/qcom-spmi-adc-tm5.yaml.
> +        type: boolean
> +
>
> Why do you duplicate entire vadc file? Why it cannot be part of that
> file? Oh wait, it was in v2.
>
> You now duplicated a lot of property definitions without clear reason.
> If this is intention, then you need to put them in common schema.
>
>
> Many of the properties used for earlier QCOM VADC devices will be used for this device too.....do you mean I can add a new schema file (named something like qcom,vadc.yaml) and move common properties into it (like qcom,hw-settle-time, qcom,decimation, etc) from this file and qcom,spmi-vadc.yaml?
>
> Can I do it in the same patch or should it be a separate patch coming before this one ?

I'd say, separate patch. Move first, extend later.

>
>
>
> diff --git a/include/dt-bindings/iio/adc/qcom,spmi-adc7-smb139x.h b/include/dt-bindings/iio/adc/qcom,spmi-adc7-smb139x.h
>
> index c0680d1285cf..750a526af2c1 100644
> --- a/include/dt-bindings/iio/adc/qcom,spmi-adc7-smb139x.h
> +++ b/include/dt-bindings/iio/adc/qcom,spmi-adc7-smb139x.h
> @@ -6,7 +6,7 @@
>  #ifndef _DT_BINDINGS_QCOM_SPMI_VADC_SMB139X_H
>  #define _DT_BINDINGS_QCOM_SPMI_VADC_SMB139X_H
>
> -#include <dt-bindings/iio/qcom,spmi-vadc.h>
> +#include <dt-bindings/iio/adc/qcom,spmi-vadc.h>
>
> ? How is it related?
>
>
> This should have gone into patch 1, I'll fix it in the next patch series.
>
> I'll address all your other comments in the next patchset.
>
> Thanks,
>
> Jishnu
>
>
>
>  #define SMB139x_1_ADC7_SMB_TEMP (SMB139x_1_SID << 8 | ADC7_SMB_TEMP)
>  #define SMB139x_1_ADC7_ICHG_SMB (SMB139x_1_SID << 8 | ADC7_ICHG_SMB)
> diff --git a/include/dt-bindings/iio/adc/qcom,spmi-vadc.h b/include/dt-bindings/iio/adc/qcom,spmi-vadc.h
> index ef07ecd4d585..cfe653d945a4 100644
> --- a/include/dt-bindings/iio/adc/qcom,spmi-vadc.h
> +++ b/include/dt-bindings/iio/adc/qcom,spmi-vadc.h
> @@ -1,6 +1,8 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
>  /*
>   * Copyright (c) 2012-2014,2018,2020 The Linux Foundation. All rights reserved.
> + *
>
> Drop stray blank line
>
> Best regards,
> Krzysztof
>


-- 
With best wishes
Dmitry

Powered by blists - more mailing lists