Hello Simon,
On 10/03/2015 04:28 PM, Simon Glass wrote:
Hi Przemyslaw,
On 21 September 2015 at 13:26, Przemyslaw Marczak <p.marc...@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.marc...@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.marc...@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.marc...@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.marc...@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.marc...@samsung.com
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
