[U-Boot] [PATCH V2 06/11] dm: adc: add simple ADC uclass implementation
Przemyslaw Marczak
p.marczak at samsung.com
Tue Oct 13 13:57:30 CEST 2015
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 at 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 at 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 at 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 at 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 at 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
More information about the U-Boot
mailing list