From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Tue, 13 Oct 2015 13:57:30 +0200 Subject: [U-Boot] [PATCH V2 06/11] dm: adc: add simple ADC uclass implementation In-Reply-To: References: <1440770374-11501-1-git-send-email-p.marczak@samsung.com> <1442838403-27777-1-git-send-email-p.marczak@samsung.com> <1442838403-27777-7-git-send-email-p.marczak@samsung.com> Message-ID: <561CF1AA.3010802@samsung.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hello Simon, On 10/03/2015 04:28 PM, Simon Glass wrote: > Hi Przemyslaw, > > On 21 September 2015 at 13:26, Przemyslaw Marczak 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 >> --- >> 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 >> +# >> +# 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 >> + * >> + * SPDX-License-Identifier: GPL-2.0+ >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#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 >> + * >> + * 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