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.
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"helpThis 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 maskdiff --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?
+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@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