Re: [PATCH V4 3/5] iio: adc: Add support for PMIC7 ADC

From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Jishnu Prakash <jprakash@codeaurora.org>
Cc: agross@kernel.org, Bjorn Andersson <bjorn.andersson@linaro.org>,
	devicetree <devicetree@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Matthias Kaehlcke <mka@chromium.org>,
	Linus Walleij <linus.walleij@linaro.org>,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	Amit Kucheria <amit.kucheria@verdurent.com>,
	smohanad@codeaurora.org, kgunda@codeaurora.org,
	aghayal@codeaurora.org, Jonathan Cameron <jic23@kernel.org>,
	Hartmut Knaack <knaack.h@gmx.de>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
	linux-arm-msm@vger.kernel.org,
	linux-iio <linux-iio@vger.kernel.org>,
	linux-arm-msm-owner@vger.kernel.org
Subject: Re: [PATCH V4 3/5] iio: adc: Add support for PMIC7 ADC
Date: Wed, 13 May 2020 12:48:07 +0300	[thread overview]
Message-ID: <CAHp75VedM+=+m8WF=zPpUcizgCGareYuBzUfjKwesozVSY_gKg@mail.gmail.com> (raw)
In-Reply-To: <1589361736-816-4-git-send-email-jprakash@codeaurora.org>

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().

-- 
With Best Regards,
Andy Shevchenko