[U-Boot] [PATCH V2 06/11] dm: adc: add simple ADC uclass implementation
Simon Glass
sjg at chromium.org
Sat Oct 3 16:28:04 CEST 2015
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.
>
> 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
> + - 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?
> +
> +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->...
> +}
> +
> +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.
> +
> + 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().
> +
> + /**
> + * 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?
> +
> +/**
> + * 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?
> + */
> +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.
> + *
> + * @name: device's name to search
> + * @channel: device's input channel to init
> + * @data: pointer to convertion output data
conversion
> + */
> +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
More information about the U-Boot
mailing list