mbox series

[V4,0/5] iio: adc: Add support for QCOM SPMI PMIC7 ADC

Message ID 1589361736-816-1-git-send-email-jprakash@codeaurora.org
Headers show
Series iio: adc: Add support for QCOM SPMI PMIC7 ADC | expand

Message

Jishnu Prakash May 13, 2020, 9:22 a.m. UTC
The following changes are made in V4:

Made some recommended minor changes in the third patch. Removed
the info member from adc5_data, and adc exit function, to be
added back in fifth patch.

Added a fifth patch to clean up code common to PMIC5 and PMIC7 ADC.

Jishnu Prakash (5):
  iio: adc: Convert the QCOM SPMI ADC bindings to .yaml format
  iio: adc: Add PMIC7 ADC bindings
  iio: adc: Add support for PMIC7 ADC
  iio: adc: Update error checks and debug prints
  iio: adc: Clean up ADC code common to PMIC5 and PMIC7

 .../devicetree/bindings/iio/adc/qcom,spmi-vadc.txt | 173 -------------
 .../bindings/iio/adc/qcom,spmi-vadc.yaml           | 278 +++++++++++++++++++++
 drivers/iio/adc/qcom-spmi-adc5.c                   | 266 ++++++++++++++++++--
 drivers/iio/adc/qcom-vadc-common.c                 | 260 +++++++++++++++++++
 drivers/iio/adc/qcom-vadc-common.h                 |  15 ++
 include/dt-bindings/iio/qcom,spmi-adc7-pm8350.h    |  67 +++++
 include/dt-bindings/iio/qcom,spmi-adc7-pm8350b.h   |  88 +++++++
 include/dt-bindings/iio/qcom,spmi-adc7-pmk8350.h   |  46 ++++
 include/dt-bindings/iio/qcom,spmi-adc7-pmr735a.h   |  28 +++
 include/dt-bindings/iio/qcom,spmi-adc7-pmr735b.h   |  28 +++
 include/dt-bindings/iio/qcom,spmi-vadc.h           |  78 +++++-
 11 files changed, 1134 insertions(+), 193 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt
 create mode 100644 Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml
 create mode 100644 include/dt-bindings/iio/qcom,spmi-adc7-pm8350.h
 create mode 100644 include/dt-bindings/iio/qcom,spmi-adc7-pm8350b.h
 create mode 100644 include/dt-bindings/iio/qcom,spmi-adc7-pmk8350.h
 create mode 100644 include/dt-bindings/iio/qcom,spmi-adc7-pmr735a.h
 create mode 100644 include/dt-bindings/iio/qcom,spmi-adc7-pmr735b.h

Comments

Andy Shevchenko May 13, 2020, 9:48 a.m. UTC | #1
On Wed, May 13, 2020 at 12:23 PM Jishnu Prakash <jprakash@codeaurora.org> wrote:
>
> The ADC architecture on PMIC7 is changed as compared to PMIC5. The
> major change from PMIC5 is that all SW communication to ADC goes through
> PMK8350, which communicates with other PMICs through PBS when the ADC
> on PMK8350 works in master mode. The SID register is used to identify the
> PMICs with which the PBS needs to communicate. Add support for the same.

> +#define ADC7_CONV_TIMEOUT                      msecs_to_jiffies(10)

...

> +       ret = adc5_read(adc, ADC5_USR_DIG_PARAM, buf, sizeof(buf));
> +       if (ret < 0)

Is ' < 0' part necessary?
Ditto for same cases in other places in the code.

> +               return ret;

...

> +       switch (mask) {
> +       case IIO_CHAN_INFO_PROCESSED:
> +               ret = adc7_do_conversion(adc, prop, chan,
> +                                       &adc_code_volt, &adc_code_cur);
> +               if (ret)
> +                       return ret;
> +
> +               ret = qcom_adc5_hw_scale(prop->scale_fn_type,
> +                       &adc5_prescale_ratios[prop->prescale],
> +                       adc->data,
> +                       adc_code_volt, val);
> +
> +               if (ret)
> +                       return ret;
> +
> +               return IIO_VAL_INT;
> +       default:
> +               return -EINVAL;
> +       }

> +
> +       return 0;

Dead code?

...

> +static int qcom_vadc7_scale_hw_calib_die_temp(
> +                               const struct vadc_prescale_ratio *prescale,
> +                               const struct adc5_data *data,
> +                               u16 adc_code, int *result_mdec)
> +{
> +
> +       int voltage, vtemp0, temp, i = ARRAY_SIZE(adcmap7_die_temp) - 1;

How assignment to i is useful?

> +       voltage = qcom_vadc_scale_code_voltage_factor(adc_code,
> +                               prescale, data, 1);
> +
> +       if (adcmap7_die_temp[0].x > voltage) {
> +               *result_mdec = DIE_TEMP_ADC7_SCALE_1;
> +               return 0;

> +       } else if (adcmap7_die_temp[i].x <= voltage) {

Redundant 'else'.

> +               *result_mdec = DIE_TEMP_ADC7_MAX;
> +               return 0;
> +       }
> +
> +       for (i = 0; i < ARRAY_SIZE(adcmap7_die_temp); i++)
> +               if (adcmap7_die_temp[i].x > voltage)
> +                       break;
> +
> +       vtemp0 = adcmap7_die_temp[i - 1].x;
> +       voltage = voltage - vtemp0;
> +       temp = div64_s64(voltage * DIE_TEMP_ADC7_SCALE_FACTOR,
> +               adcmap7_die_temp[i - 1].y);
> +       temp += DIE_TEMP_ADC7_SCALE_1 + (DIE_TEMP_ADC7_SCALE_2 * (i - 1));
> +       *result_mdec = temp;
> +
> +       return 0;
> +}

...

> +#define RATIO_MAX_ADC7         0x4000

Hmm... Why the last is in hex? Is it related to amount of bits in the
hardware? Then probably better to use BIT().
Jishnu Prakash May 22, 2020, 11:58 a.m. UTC | #2
Hi Andy,

On 5/13/2020 3:19 PM, Andy Shevchenko wrote:
> On Wed, May 13, 2020 at 12:23 PM Jishnu Prakash <jprakash@codeaurora.org> wrote:
>> Change pr_err/pr_debug statements to dev_err/dev_dbg for
>> increased clarity. Also clean up some return value checks.
> 'Also' on the commit message == 'split this to two'.
I'll do the ret value changes in the third patch in my next post, 
according to your comment there.
> But here is a ping pong style of patches (you introduce a problem in
> one patch and fix it in the following).
I'll try to avoid this in the next post
>