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. > > 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.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? > + > +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.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(). > + > + /** > + * 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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot