Re: [U-Boot] [PATCH V2 06/11] dm: adc: add simple ADC uclass implementation

[Przemyslaw Marczak]

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 000..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 000..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 000..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, );
+   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 000..57b9281

Re: [U-Boot] [PATCH V2 06/11] dm: adc: add simple ADC uclass implementation

[Simon Glass]

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.

>
> 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 000..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 000..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 000..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?

> +
> +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, );
> +   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 

[U-Boot] [PATCH V2 06/11] dm: adc: add simple ADC uclass implementation

[Przemyslaw Marczak]

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

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 000..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:
+ - 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 000..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 000..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)
+
+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;
+}
+
+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;
+
+   ret = uclass_get_device_by_name(UCLASS_ADC, name, );
+   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 000..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