Re: [PATCH v2 2/2] iio: adc: Add Spreadtrum SC27XX PMICs ADC support

[Baolin Wang]

Hi Jonathan,

On 22 June 2018 at 22:26, Jonathan Cameron <jonathan.cameron@xxxxxxxxxx> wrote:
> On Mon, 18 Jun 2018 18:47:18 +0800
> Baolin Wang <baolin.wang@xxxxxxxxxx> wrote:
>
>> Hi Jonathan,
>>
>> On 18 June 2018 at 18:20, Jonathan Cameron <jic23@xxxxxxxxxxxxxxxxxxxxx> wrote:
>> >
>> >
>> > On 17 June 2018 09:03:04 BST, Baolin Wang <baolin.wang@xxxxxxxxxx> wrote:
>> >>Hi Jonathan,
>> >>
>> >>On 17 June 2018 at 02:35, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>> >>> On Fri, 15 Jun 2018 15:03:36 +0800
>> >>> Baolin Wang <baolin.wang@xxxxxxxxxx> wrote:
>> >>>
>> >>>> From: Freeman Liu <freeman.liu@xxxxxxxxxxxxxx>
>> >>>>
>> >>>> The Spreadtrum SC27XX PMICs ADC controller contains 32 channels,
>> >>>> which is used to sample voltages with 12 bits conversion.
>> >>>>
>> >>>> Signed-off-by: Freeman Liu <freeman.liu@xxxxxxxxxxxxxx>
>> >>>> Signed-off-by: Baolin Wang <baolin.wang@xxxxxxxxxx>
>> >>>
>> >>> Hi,
>> >>>
>> >>> There are some race conditions around the probe and remove.
>> >>> More care is needed when we have a mixture of managed and unmanaged
>> >>cleanup
>> >>> like here.
>> >>
>> >>Thanks to point the race issue.
>> >>
>> >>>
>> >>> I'm not understanding the way you have exposed a simple _raw and
>> >>_scale
>> >>> attributes with what looks to be different scaling to that applied
>> >>> in _processed.   As I say below, we should not have both of those
>> >>interface
>> >>> options anyway.  The ABI is that (X_raw + X_offset)*X_scale =
>> >>X_processed.
>> >>> (with defaults of X_scale = 1 and X_offset = 0).
>> >>
>> >>See below comments.
>> >>
>> >>>
>> >>> Please rename to avoid using wild cards in the name.  That's gone
>> >>> wrong so many times in the past you wouldn't believe it!
>> >>> Hmm Awkward though if the MFD is already upstream. Ah well, I guess
>> >>> for consistency we should follow that and groan when it goes wrong.
>> >>
>> >>Can I rename to be 'sprd-pmic-adc.c'? I can not rename it as
>> >>'sc2731-adc', we have differnet PMICs (SC2730, SC2731, SC2720 etc.),
>> >>but they are all integrated the same ADC controller.
>> >
>> > You can rename as this is a common problem throughout drivers. There is no good solution.
>> >
>> > Given mfd naming, just leave it with wild cards as better than a name no one will recognise
>>
>> OK.
>>
>> >>
>> >>>> ---
>> >>>> Changes since v1:
>> >>>>  - Add const for static structures definition.
>> >>>>  - Change SC27XX_ADC_TO_VOLTAGE macro to be one function.
>> >>>>  - Move channel scale accessing into mutex protection.
>> >>>>  - Fix some typos.
>> >>>> ---
>> >>>>  drivers/iio/adc/Kconfig      |   10 +
>> >>>>  drivers/iio/adc/Makefile     |    1 +
>> >>>>  drivers/iio/adc/sc27xx_adc.c |  547
>> >>++++++++++++++++++++++++++++++++++++++++++
>> >>>>  3 files changed, 558 insertions(+)
>> >>>>  create mode 100644 drivers/iio/adc/sc27xx_adc.c
>> >>>>
>> >>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> >>>> index 9da7907..985b73e 100644
>> >>>> --- a/drivers/iio/adc/Kconfig
>> >>>> +++ b/drivers/iio/adc/Kconfig
>> >>>> @@ -621,6 +621,16 @@ config ROCKCHIP_SARADC
>> >>>>         To compile this driver as a module, choose M here: the
>> >>>>         module will be called rockchip_saradc.
>> >>>>
>> >>>> +config SC27XX_ADC
>> >>>> +     tristate "Spreadtrum SC27xx series PMICs ADC"
>> >>>> +     depends on MFD_SC27XX_PMIC || COMPILE_TEST
>> >>>> +     help
>> >>>> +       Say yes here to build support for the integrated ADC inside
>> >>the
>> >>>> +       Spreadtrum SC27xx series PMICs.
>> >>>> +
>> >>>> +       This driver can also be built as a module. If so, the module
>> >>>> +       will be called sc27xx_adc.
>> >>>> +
>> >>>>  config SPEAR_ADC
>> >>>>       tristate "ST SPEAr ADC"
>> >>>>       depends on PLAT_SPEAR || COMPILE_TEST
>> >>>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> >>>> index 28a9423..03db7b5 100644
>> >>>> --- a/drivers/iio/adc/Makefile
>> >>>> +++ b/drivers/iio/adc/Makefile
>> >>>> @@ -59,6 +59,7 @@ obj-$(CONFIG_QCOM_SPMI_VADC) += qcom-spmi-vadc.o
>> >>>>  obj-$(CONFIG_QCOM_PM8XXX_XOADC) += qcom-pm8xxx-xoadc.o
>> >>>>  obj-$(CONFIG_RCAR_GYRO_ADC) += rcar-gyroadc.o
>> >>>>  obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
>> >>>> +obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o
>> >>>>  obj-$(CONFIG_SPEAR_ADC) += spear_adc.o
>> >>>>  obj-$(CONFIG_STX104) += stx104.o
>> >>>>  obj-$(CONFIG_SUN4I_GPADC) += sun4i-gpadc-iio.o
>> >>>> diff --git a/drivers/iio/adc/sc27xx_adc.c
>> >>b/drivers/iio/adc/sc27xx_adc.c
>> >>>> new file mode 100644
>> >>>> index 0000000..52e5b74
>> >>>> --- /dev/null
>> >>>> +++ b/drivers/iio/adc/sc27xx_adc.c
>> >>>
>> >>> In general (i.e. when we notice in time) we don't allow wild cards in
>> >>names.
>> >>> Far too many times we did this in the past and ended up with later
>> >>parts
>> >>> that fitted the name, but could not be supported by the driver.
>> >>>
>> >>> The convention is to name everything after the first part supported.
>> >>> So here, sc2731. (I relaxed my thoughts on this later having seen the
>> >>mfd
>> >>> has this naming - so there are no ideal options left..)
>> >>
>> >>Like I explained above, maybe change to 'sprd_pmic_adc.c', is this OK
>> >>for you?
>> > Goes wrong just as quickly as wild cards...
>>
>> OK.
>>
>> >>>> +
>> >>>> +static int sc27xx_adc_convert_volt(struct sc27xx_adc_data *data,
>> >>int channel,
>> >>>> +                                int scale, int raw_adc)
>> >>>> +{
>> >>>> +     u32 numerator, denominator;
>> >>>> +     u32 volt;
>> >>>> +
>> >>>> +     /*
>> >>>> +      * Convert ADC values to voltage values according to the
>> >>linear graph,
>> >>>> +      * and channel 5 and channel 1 has been calibrated, so we can
>> >>just
>> >>>> +      * return the voltage values calculated by the linear graph.
>> >>But other
>> >>>> +      * channels need be calculated to the real voltage values with
>> >>the
>> >>>> +      * voltage ratio.
>> >>>> +      */
>> >>>> +     switch (channel) {
>> >>>> +     case 5:
>> >>>> +             return sc27xx_adc_to_volt(&big_scale_graph, raw_adc);
>> >>>> +
>> >>>> +     case 1:
>> >>>> +             return sc27xx_adc_to_volt(&small_scale_graph,
>> >>raw_adc);
>> >>>> +
>> >>>> +     default:
>> >>>> +             volt = sc27xx_adc_to_volt(&small_scale_graph,
>> >>raw_adc);
>> >>>> +             break;
>> >>>> +     }
>> >>>
>> >>> This looks a lot more complex than simple scaling that is indicated
>> >>by the
>> >>> raw and scale attributes? They can't both be right..
>> >>
>> >>Since this is special for our ADC controller, we have 2 channels that
>> >>has been calibrated in hardware, but for other
>> >>none-calibrated-channels, we should care about the channel voltage
>> >>ratio when converting to a real voltage values, that is because some
>> >>channel's voltage is larger so we need one voltage ratio to sample the
>> >>ADC values.
>> >
>> > It's still a question of one or the other. Channels should not do processed and raw without a very good reason.
>>
>> I think I have explained why we need our special processed approach as below.
>>
>> >
>> > Issue with processed is that you can't easily do buffered chrdev streaming in future.
>> >
>> >
>> >>
>> >>>> +
>> >>>> +     sc27xx_adc_volt_ratio(data, channel, scale, &numerator,
>> >>&denominator);
>> >>>> +
>> >>>> +     return (volt * denominator + numerator / 2) / numerator;
>> >>>> +}
>> >>>> +
>> >>>> +static int sc27xx_adc_read_processed(struct sc27xx_adc_data *data,
>> >>>> +                                  int channel, int scale, int *val)
>> >>>> +{
>> >>>> +     int ret, raw_adc;
>> >>>> +
>> >>>> +     ret = sc27xx_adc_read(data, channel, scale, &raw_adc);
>> >>>> +     if (ret)
>> >>>> +             return ret;
>> >>>> +
>> >>>> +     *val = sc27xx_adc_convert_volt(data, channel, scale, raw_adc);
>> >>>> +     return 0;
>> >>>> +}
>> >>>> +
>> >>>> +static int sc27xx_adc_read_raw(struct iio_dev *indio_dev,
>> >>>> +                            struct iio_chan_spec const *chan,
>> >>>> +                            int *val, int *val2, long mask)
>> >>>> +{
>> >>>> +     struct sc27xx_adc_data *data = iio_priv(indio_dev);
>> >>>> +     int scale, ret, tmp;
>> >>>> +
>> >>>> +     switch (mask) {
>> >>>> +     case IIO_CHAN_INFO_RAW:
>> >>>> +     case IIO_CHAN_INFO_AVERAGE_RAW:
>> >>>> +             mutex_lock(&indio_dev->mlock);
>> >>>> +             scale = data->channel_scale[chan->channel];
>> >>>> +             ret = sc27xx_adc_read(data, chan->channel, scale,
>> >>&tmp);
>> >>>> +             mutex_unlock(&indio_dev->mlock);
>> >>>> +
>> >>>> +             if (ret)
>> >>>> +                     return ret;
>> >>>> +
>> >>>> +             *val = tmp;
>> >>>> +             return IIO_VAL_INT;
>> >>>> +
>> >>>> +     case IIO_CHAN_INFO_PROCESSED:
>> >>>> +             mutex_lock(&indio_dev->mlock);
>> >>>> +             scale = data->channel_scale[chan->channel];
>> >>>> +             ret = sc27xx_adc_read_processed(data, chan->channel,
>> >>scale,
>> >>>> +                                             &tmp);
>> >>>
>> >>> To keep to the rule of 'one way to read a value' we don't tend to
>> >>support
>> >>> both raw and processed.  The only exception is made for devices where
>> >>we got
>> >>> this wrong in the first place and so have to support both to avoid a
>> >>potential
>> >>> regression due to ABI changes.
>> >>>
>> >>> If it is a simple linear scaling (like here I think) then the
>> >>preferred option
>> >>> is to not supply the processed version.  Just do raw.
>> >>
>> >>Unfortunately, we can not use the formula ( (X_raw + X_offset)*X_scale
>> >>= X_processed) for our ADC controller to get a processed value.
>> >>Firstly, the ADC hardware will do the sampling with the scale value.
>> >
>> > Hmm fair enough, scale is fine for that. But don't provide raw unless real voltage is scale *raw
>> >
>> >>Secondly we should convert a raw value to a voltage value by the
>> >>linear graph table, for some channels, we should also use the channel
>> >>voltage ratio to get a real voltage value. So I think we should keep
>> >>our special processed approach for consumers.
>> >
>> > That's fine but drop the raw access or you are not obeying the abi.
>>
>> Sorry, I think I did not get your points. Could you elaborate on why
>> we can not provide raw and processed?
>> I saw many drivers not only provide the raw access but also the
>> processed access. Especially for our special processed approach, I
>> think the raw access and the processed access are both needed for
>> consumers. Thanks.
>>
>
> That's not true.  There are a few 'very special cases' where this happens.
> We had cases where the driver was already putting out RAW values when
> it turned out there was no sensible way of converting it to processed values
> (non linear as in your case).  As the RAW interface was existing ABI we had
> to maintain it even though in some senses this was a bug.
>
> The other cases that look a bit like this are when multiple input sensors
> are fused to produce a computed output value.  This happens with light
> sensors.  However, the raw and processed channels are different channels
> (as there is no one to one mapping).
>
> It is not uncommon to provide a 'mixture' of raw and processed but they
> are not for the same channels.
>
> So in general we simply do not allow both to be provided for a single channel.
> There is no useful purpose in doing so and it complicates the exposed ABI.

Thanks for your patient explanation, now I understood.

-- 
Baolin.wang
Best Regards