[U-Boot] [PATCH V2 06/11] dm: adc: add simple ADC uclass implementation

[Przemyslaw Marczak <p.marczak@samsung.com>]

Hello Simon,

On 10/03/2015 04:28 PM, Simon Glass wrote:
> Hi Przemyslaw,
>
> On 21 September 2015 at 13:26, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
>> This commit adds:
>> - new uclass id: UCLASS_ADC
>> - new uclass driver: drivers/adc/adc-uclass.c
>>
>> The uclass's implementation is as simple as needed and provides functions:
>> - adc_init() - init ADC conversion
>> - adc_data() - convert and return data
>> - adc_data_mask() - return ADC data mask
>> - adc_channel_single_shot() - function for single ADC convertion
>>
>> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
>> ---
>> Changes V2:
>> - new commit - introduce ADC uclass driver
>> ---
>>   drivers/Kconfig          |  2 ++
>>   drivers/Makefile         |  1 +
>>   drivers/adc/Kconfig      |  8 +++++
>>   drivers/adc/Makefile     |  8 +++++
>>   drivers/adc/adc-uclass.c | 76 +++++++++++++++++++++++++++++++++++++++++
>>   include/adc.h            | 88 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   include/dm/uclass-id.h   |  1 +
>>   7 files changed, 184 insertions(+)
>>   create mode 100644 drivers/adc/Kconfig
>>   create mode 100644 drivers/adc/Makefile
>>   create mode 100644 drivers/adc/adc-uclass.c
>>   create mode 100644 include/adc.h
>
> Sorry I have quite a lot of questions and comments on this.
>

Yes, ok.

>>
>> diff --git a/drivers/Kconfig b/drivers/Kconfig
>> index 63c92c5..ad9ae3a 100644
>> --- a/drivers/Kconfig
>> +++ b/drivers/Kconfig
>> @@ -4,6 +4,8 @@ source "drivers/core/Kconfig"
>>
>>   # types of drivers sorted in alphabetical order
>>
>> +source "drivers/adc/Kconfig"
>> +
>>   source "drivers/block/Kconfig"
>>
>>   source "drivers/clk/Kconfig"
>> diff --git a/drivers/Makefile b/drivers/Makefile
>> index 9d0a595..d7d5e9f 100644
>> --- a/drivers/Makefile
>> +++ b/drivers/Makefile
>> @@ -35,6 +35,7 @@ obj-$(CONFIG_SPL_SATA_SUPPORT) += block/
>>
>>   else
>>
>> +obj-y += adc/
>>   obj-$(CONFIG_DM_DEMO) += demo/
>>   obj-$(CONFIG_BIOSEMU) += bios_emulator/
>>   obj-y += block/
>> diff --git a/drivers/adc/Kconfig b/drivers/adc/Kconfig
>> new file mode 100644
>> index 0000000..1cb1a8d
>> --- /dev/null
>> +++ b/drivers/adc/Kconfig
>> @@ -0,0 +1,8 @@
>> +config ADC
>> +       bool "Enable ADC drivers using Driver Model"
>> +       help
>> +         This allows drivers to be provided for ADCs to drive their features,
>> +         trough simple ADC uclass driver interface, with operations:
>
> through a simple
>

Ok.

>> +         - device enable
>> +         - conversion init
>> +         - conversion start and return data with data mask
>> diff --git a/drivers/adc/Makefile b/drivers/adc/Makefile
>> new file mode 100644
>> index 0000000..c4d9618
>> --- /dev/null
>> +++ b/drivers/adc/Makefile
>> @@ -0,0 +1,8 @@
>> +#
>> +# Copyright (C) 2015 Samsung Electronics
>> +# Przemyslaw Marczak <p.marczak@samsung.com>
>> +#
>> +# SPDX-License-Identifier:     GPL-2.0+
>> +#
>> +
>> +obj-$(CONFIG_ADC) += adc-uclass.o
>> diff --git a/drivers/adc/adc-uclass.c b/drivers/adc/adc-uclass.c
>> new file mode 100644
>> index 0000000..bb71b6e
>> --- /dev/null
>> +++ b/drivers/adc/adc-uclass.c
>> @@ -0,0 +1,76 @@
>> +/*
>> + * Copyright (C) 2015 Samsung Electronics
>> + * Przemyslaw Marczak <p.marczak@samsung.com>
>> + *
>> + * SPDX-License-Identifier:    GPL-2.0+
>> + */
>> +
>> +#include <common.h>
>> +#include <errno.h>
>> +#include <dm.h>
>> +#include <dm/lists.h>
>> +#include <dm/device-internal.h>
>> +#include <dm/uclass-internal.h>
>> +#include <adc.h>
>> +
>> +#define ADC_UCLASS_PLATDATA_SIZE       sizeof(struct adc_uclass_platdata)
>
> Can we drop #define?
>

Don't you think, that single line initialization looks better than those 
with line breaks? This is the purpose of that define.

>> +
>> +int adc_init(struct udevice *dev, int channel)
>> +{
>> +       const struct adc_ops *ops = dev_get_driver_ops(dev);
>> +
>> +       if (ops->adc_init)
>> +               return ops->adc_init(dev, channel);
>> +
>> +       return -ENOSYS;
>
> Let's turn each of these around so that errors are the exception, not
> the normal path.
>
> if (!ops->adc_init)
>     return -ENOSYS;
>
> return ops->...
>

Ok, I will turn it.

>> +}
>> +
>> +int adc_data(struct udevice *dev, unsigned int *data)
>> +{
>> +       const struct adc_ops *ops = dev_get_driver_ops(dev);
>> +
>> +       *data = 0;
>> +
>> +       if (ops->adc_data)
>> +               return ops->adc_data(dev, data);
>> +
>> +       return -ENOSYS;
>> +}
>> +
>> +int adc_data_mask(struct udevice *dev)
>> +{
>> +       struct adc_uclass_platdata *uc_pdata = dev_get_uclass_platdata(dev);
>> +
>> +       if (uc_pdata)
>> +               return uc_pdata->data_mask;
>> +
>> +       return -ENOSYS;
>> +}
>> +
>> +int adc_channel_single_shot(const char *name, int channel, unsigned int *data)
>> +{
>> +       struct udevice *dev;
>> +       int ret;
>> +
>> +       *data = 0;
>
> I don't think we need this assignment. This can be undefined if an
> error is returned.
>

Ok.

>> +
>> +       ret = uclass_get_device_by_name(UCLASS_ADC, name, &dev);
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = adc_init(dev, channel);
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = adc_data(dev, data);
>> +       if (ret)
>> +               return ret;
>> +
>> +       return 0;
>> +}
>> +
>> +UCLASS_DRIVER(adc) = {
>> +       .id     = UCLASS_ADC,
>> +       .name   = "adc",
>> +       .per_device_platdata_auto_alloc_size = ADC_UCLASS_PLATDATA_SIZE,
>> +};
>> diff --git a/include/adc.h b/include/adc.h
>> new file mode 100644
>> index 0000000..57b9281
>> --- /dev/null
>> +++ b/include/adc.h
>> @@ -0,0 +1,88 @@
>> +/*
>> + * Copyright (C) 2015 Samsung Electronics
>> + * Przemyslaw Marczak <p.marczak@samsung.com>
>> + *
>> + * SPDX-License-Identifier:    GPL-2.0+
>> + */
>> +
>> +#ifndef _ADC_H_
>> +#define _ADC_H_
>> +
>> +/**
>> + * struct adc_uclass_platdata - ADC power supply and reference Voltage info
>> + *
>> + * @data_mask     - conversion output data mask
>> + * @channels_num  - number of analog channels input
>> + * @vdd_supply    - positive reference voltage supply
>> + * @vss_supply    - negative reference voltage supply
>> + */
>> +struct adc_uclass_platdata {
>> +       unsigned int data_mask;
>> +       unsigned int channels_num;
>> +       struct udevice *vdd_supply;
>> +       struct udevice *vss_supply;
>> +};
>> +
>> +/**
>> + * struct adc_ops - ADC device operations
>> + */
>> +struct adc_ops {
>> +       /**
>> +        * conversion_init() - init device's default conversion parameters
>> +        *
>> +        * @dev:     ADC device to init
>> +        * @channel: analog channel number
>> +        * @return:  0 if OK, -ve on error
>> +        */
>> +       int (*adc_init)(struct udevice *dev, int channel);
>
> s/adc_init/init/
>
> Same below. Also it seems like this starts the conversion, so how
> about using the name start().
>

Ok, I will modify the API.

>> +
>> +       /**
>> +        * conversion_data() - get output data of given channel conversion
>> +        *
>> +        * @dev:          ADC device to trigger
>> +        * @channel_data: pointer to returned channel's data
>> +        * @return:       0 if OK, -ve on error
>> +        */
>> +       int (*adc_data)(struct udevice *dev, unsigned int *channel_data);
>> +};
>> +
>> +/**
>> + * adc_init() - init device's default conversion parameters for the given
>> + * analog input channel.
>> + *
>> + * @dev:     ADC device to init
>> + * @channel: analog channel number
>> + * @return:  0 if OK, -ve on error
>> + */
>> +int adc_init(struct udevice *dev, int channel);
>
> adc_start()?
>
>> +
>> +/**
>> + * adc_data() - get conversion data for the channel inited by adc_init().
>> + *
>> + * @dev:    ADC device to trigger
>> + * @data:   pointer to returned channel's data
>> + * @return: 0 if OK, -ve on error
>> + */
>> +int adc_data(struct udevice *dev, unsigned int *data);
>
> This seems a bit wonky. Why not pass in the channel with this call?
> What if I want to start conversions on multiple channels at the same
> time?
>

Right, this could be more flexible. I will modify it.

>> +
>> +/**
>> + * adc_data_mask() - get data mask (ADC resolution mask) for given ADC device.
>> + * This can be used if adc uclass platform data is filled.
>> + *
>> + * @dev:    ADC device to check
>> + * @return: 0 if OK, -ve on error
>
> If it always returns 0 unless there is an error, what is the point? Or
> is this comment incorrect?
>

Yes, that is a mistake.

>> + */
>> +int adc_data_mask(struct udevice *dev);
>> +
>> +/**
>> + * adc_channel_single_shot() - get output data of convertion for the ADC
>> + * device's channel. This function search for the device with the given name,
>> + * init the given channel and returns the convertion data.
>
> It also inits the device.
>
> I would prefer a function that finds a device by name and inits it.
>

Ah, ok.

>> + *
>> + * @name:    device's name to search
>> + * @channel: device's input channel to init
>> + * @data:    pointer to convertion output data
>
> conversion
>

Okay.

>> + */
>> +int adc_channel_single_shot(const char *name, int channel, unsigned int *data);
>> +
>> +#endif
>> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
>> index 1eeec74..0f7e7da 100644
>> --- a/include/dm/uclass-id.h
>> +++ b/include/dm/uclass-id.h
>> @@ -25,6 +25,7 @@ enum uclass_id {
>>          UCLASS_SIMPLE_BUS,      /* bus with child devices */
>>
>>          /* U-Boot uclasses start here - in alphabetical order */
>> +       UCLASS_ADC,             /* Analog-to-digital converter */
>>          UCLASS_CLK,             /* Clock source, e.g. used by peripherals */
>>          UCLASS_CPU,             /* CPU, typically part of an SoC */
>>          UCLASS_CROS_EC,         /* Chrome OS EC */
>> --
>> 1.9.1
>>
>
> Regards,
> Simon
>

Thanks for comments. The ADC API is not ideal, however it fits my needs. 
Also it looks like nobody was using ADC before in U-Boot, so I can 
suppose it can be as simple as possible. I will clean this :)

Best regards
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com