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.
--
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
