[U-Boot] [PATCH] cmd: add ADC cli commands

[Neil Armstrong]

Hi,

On 26/04/2018 16:40, Simon Glass wrote:
> Hi Neil,
> 
> On 23 April 2018 at 08:18, Neil Armstrong <narmstrong at baylibre.com> wrote:
>> Add an 'adc' cli command to get adc devices informations and read single
>> shots datas.
>>
>> Signed-off-by: Neil Armstrong <narmstrong at baylibre.com>
>> ---
>>  cmd/Kconfig  |   7 ++++
>>  cmd/Makefile |   1 +
>>  cmd/adc.c    | 126 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 134 insertions(+)
>>  create mode 100644 cmd/adc.c
> 
> Reviewed-by: Simon Glass <sjg at chromium.org>
> 
> Nits below
> 
>>
>> diff --git a/cmd/Kconfig b/cmd/Kconfig
>> index bc1d2f3..631daee 100644
>> --- a/cmd/Kconfig
>> +++ b/cmd/Kconfig
>> @@ -601,6 +601,13 @@ config CMD_ARMFLASH
>>         help
>>           ARM Ltd reference designs flash partition access
>>
>> +config CMD_ADC
>> +       bool "adc - Access ADC info and data"
>> +       select ADC
>> +       select DM_REGULATOR
>> +       help
>> +         Shows ADC device info and get single shot data;
> 
> Please spell out ADC in the help. Also, what is single-shot data?

I'll add more text.

> 
>> +
>>  config CMD_CLK
>>         bool "clk - Show clock frequencies"
>>         help
>> diff --git a/cmd/Makefile b/cmd/Makefile
>> index c4269ac..4c66353 100644
>> --- a/cmd/Makefile
>> +++ b/cmd/Makefile
>> @@ -14,6 +14,7 @@ obj-y += version.o
>>
>>  # command
>>  obj-$(CONFIG_CMD_AES) += aes.o
>> +obj-$(CONFIG_CMD_ADC) += adc.o
>>  obj-$(CONFIG_CMD_ARMFLASH) += armflash.o
>>  obj-y += blk_common.o
>>  obj-$(CONFIG_SOURCE) += source.o
>> diff --git a/cmd/adc.c b/cmd/adc.c
>> new file mode 100644
>> index 0000000..f82305c
>> --- /dev/null
>> +++ b/cmd/adc.c
>> @@ -0,0 +1,126 @@
>> +/*
>> + * Copyright (C) 2018 BayLibre, SAS
>> + *  Author: Neil Armstrong <narmstrong at baylibre.com>
>> + *
>> + * SPDX-License-Identifier:    GPL-2.0+
>> + */
>> +#include <common.h>
>> +#include <command.h>
>> +#include <dm.h>
>> +#include <adc.h>
>> +
>> +static int do_adc_list(cmd_tbl_t *cmdtp, int flag, int argc,
>> +                      char *const argv[])
>> +{
>> +       struct udevice *dev;
>> +       int ret;
>> +
>> +       ret = uclass_first_device_err(UCLASS_ADC, &dev);
> 
> This will probe the device. Is that what you want?

Is there another way ?
I mean you need to probe to know if the device is actually usable, no ?

> 
>> +       if (ret || !dev) {
> 
> You don't need to check dev, since ret will return -ENODEV if there is
> no device. See the docs for uclass_first_device_err().

Ok

> 
>> +               printf("No available ADC device\n");
> 
> return?

Oops

> 
>> +       }
>> +
>> +       do {
>> +               printf("- %s\n", dev->name);
>> +
>> +               ret = uclass_next_device(&dev);
>> +               if (ret)
>> +                       return CMD_RET_FAILURE;
>> +       } while (dev);
>> +
>> +       return CMD_RET_SUCCESS;
>> +}
>> +
>> +static int do_adc_info(cmd_tbl_t *cmdtp, int flag, int argc,
>> +                      char *const argv[])
>> +{
>> +       struct udevice *dev;
>> +       unsigned int data_mask;
>> +       int ret, vss, vdd;
>> +
>> +       if (argc < 2)
>> +               return CMD_RET_USAGE;
>> +
>> +       ret = uclass_get_device_by_name(UCLASS_ADC, argv[1], &dev);
>> +       if (ret) {
>> +               printf("Unknown ADC device %s\n", argv[1]);
>> +               return CMD_RET_FAILURE;
>> +       }
>> +
>> +       printf("ADC Device '%s' :\n", argv[1]);
>> +
>> +       ret = adc_data_mask(dev, &data_mask);
>> +       if (ret)
>> +               printf("data mask: Error %d\n", ret);
>> +       else
>> +               printf("data mask: %x\n", data_mask);
> 
> You could perhaps have a function to print the error or the value. I'm
> not sure if it is worth it.

I can drop printing the error, only printing what's available.

> 
>> +
>> +       ret = adc_vdd_value(dev, &vdd);
>> +       if (ret)
>> +               printf("vdd: Error %d\n", ret);
>> +       else
>> +               printf("vdd: %duV\n", vdd);
>> +
>> +       ret = adc_vss_value(dev, &vss);
>> +       if (ret)
>> +               printf("vss: Error %d\n", ret);
>> +       else
>> +               printf("vss: %duV\n", vss);
>> +
>> +       return CMD_RET_SUCCESS;
>> +}
>> +
> 
> [..]
> 
> Regards,
> Simon
> 

Thanks,
Neil