[PATCH 4/5] sysinfo: Add gpio-sysinfo driver

[Sean Anderson]

On 3/5/21 11:39 AM, Simon Glass wrote:
> Hi Sean,
> 
> On Fri, 5 Mar 2021 at 08:19, Sean Anderson <sean.anderson at seco.com> wrote:
>>
>>
>>
>> On 3/4/21 11:08 PM, Simon Glass wrote:
>>   > Hi Sean,
>>   >
>>   > On Mon, 1 Mar 2021 at 16:08, Sean Anderson <sean.anderson at seco.com> wrote:
>>   >>
>>   >>
>>   >>
>>   >> On 3/1/21 3:46 PM, Sean Anderson wrote:
>>   >>> This uses the newly-added dm_gpio_get_values_as_int_base3 function to
>>   >>> implement a sysinfo device. The revision map is stored in the device tree.
>>   >>>
>>   >>> Signed-off-by: Sean Anderson <sean.anderson at seco.com>
>>   >>> ---
>>   >>>
>>   >>>    .../sysinfo/gpio-sysinfo.txt                  |  37 +++++
>>   >>>    drivers/sysinfo/Kconfig                       |   8 +
>>   >>>    drivers/sysinfo/Makefile                      |   1 +
>>   >>>    drivers/sysinfo/gpio.c                        | 138 ++++++++++++++++++
>>   >>>    4 files changed, 184 insertions(+)
>>   >>>    create mode 100644 doc/device-tree-bindings/sysinfo/gpio-sysinfo.txt
>>   >>>    create mode 100644 drivers/sysinfo/gpio.c
>>   >
>>   > Reviewed-by: Simon Glass <sjg at chromium.org>
>>   >
>>   > Please see below
>>   >
>>   >>>
>>   >>> diff --git a/doc/device-tree-bindings/sysinfo/gpio-sysinfo.txt b/doc/device-tree-bindings/sysinfo/gpio-sysinfo.txt
>>   >>> new file mode 100644
>>   >>> index 0000000000..b5739d94e9
>>   >>> --- /dev/null
>>   >>> +++ b/doc/device-tree-bindings/sysinfo/gpio-sysinfo.txt
>>   >>> @@ -0,0 +1,37 @@
>>   >>> +GPIO-based Sysinfo device
>>   >>> +
>>   >>> +This binding describes several GPIOs which specify a board revision. Each GPIO
>>   >>> +forms a digit in a ternary revision number. This revision is then mapped to a
>>   >>> +name using the revisions and names properties.
>>   >>> +
>>   >>> +Each GPIO may be floating, pulled-up, or pulled-down, mapping to digits 2, 1,
>>   >>> +and 0, respectively. The first GPIO forms the least-significant digit of the
>>   >>> +revision. For example, consider the property
>>   >>> +
>>   >>> +     gpios = <&gpio 0>, <&gpio 1>, <&gpio 2>;
>>   >>> +
>>   >>> +If GPIO 0 is pulled-up, GPIO 1 is pulled-down, and GPIO 2 is floating, then the
>>   >>> +revision would be
>>   >>> +
>>   >>> +     0t201 = 2*9 + 0*3 + 1*3 = 19
>>   >>> +
>>   >>> +If instead GPIO 0 is floating, GPIO 1 is pulled-up, and GPIO 2 is pulled-down,
>>   >>> +then the revision would be
>>   >>> +
>>   >>> +     0t012 = 0*9 + 1*3 + 2*1 = 5
>>   >>> +
>>   >>> +Required properties:
>>   >>> +- compatible: should be "gpio-sysinfo".
>>   >>> +- gpios: should be a list of gpios forming the revision number,
>>   >>> +  least-significant-digit first
>>   >>> +- revisions: a list of known revisions; any revisions not present will have the
>>   >>> +  name "unknown"
>>   >>> +- names: the name of each revision in revisions
>>   >>> +
>>   >>> +Example:
>>   >>> +sysinfo {
>>   >>> +     compatible = "gpio-sysinfo";
>>   >>> +     gpios = <&gpio_a 15>, <&gpio_a 16>, <&gpio_a 17>;
>>   >>> +     revisions = <19>, <5>;
>>   >>> +     names = "rev_a", "foo";
>>   >>> +};
>>   >>> diff --git a/drivers/sysinfo/Kconfig b/drivers/sysinfo/Kconfig
>>   >>> index 85c1e81e41..381dcd8844 100644
>>   >>> --- a/drivers/sysinfo/Kconfig
>>   >>> +++ b/drivers/sysinfo/Kconfig
>>   >>> @@ -30,4 +30,12 @@ config SYSINFO_SMBIOS
>>   >>>          one which provides a way to specify this SMBIOS information in the
>>   >>>          devicetree, without needing any board-specific functionality.
>>   >>>
>>   >>> +config SYSINFO_GPIO
>>   >>> +     bool "Enable gpio sysinfo driver"
>>   >
>>   > depends on DM_GPIO ?
>>   >
>>   >>> +     help
>>   >>> +       Support querying gpios to determine board revision. This uses gpios to
>>   >>> +       form a ternary number (when they are pulled-up, -down, or floating).
>>   >>> +       This ternary number is then mapped to a board revision name using
>>   >>> +       device tree properties.
>>   >>> +
>>   >>>    endif
>>   >>> diff --git a/drivers/sysinfo/Makefile b/drivers/sysinfo/Makefile
>>   >>> index 6d04fcba1d..d9f708b7ea 100644
>>   >>> --- a/drivers/sysinfo/Makefile
>>   >>> +++ b/drivers/sysinfo/Makefile
>>   >>> @@ -4,5 +4,6 @@
>>   >>>    # Mario Six,  Guntermann & Drunck GmbH, mario.six at gdsys.cc
>>   >>>    obj-y += sysinfo-uclass.o
>>   >>>    obj-$(CONFIG_SYSINFO_GAZERBEAM) += gazerbeam.o
>>   >>> +obj-$(CONFIG_SYSINFO_GPIO) += gpio.o
>>   >>>    obj-$(CONFIG_SYSINFO_SANDBOX) += sandbox.o
>>   >>>    obj-$(CONFIG_SYSINFO_SMBIOS) += smbios.o
>>   >>> diff --git a/drivers/sysinfo/gpio.c b/drivers/sysinfo/gpio.c
>>   >>> new file mode 100644
>>   >>> index 0000000000..6a0eff3ec9
>>   >>> --- /dev/null
>>   >>> +++ b/drivers/sysinfo/gpio.c
>>   >>> @@ -0,0 +1,138 @@
>>   >>> +// SPDX-License-Identifier: GPL-2.0+
>>   >>> +/*
>>   >>> + * Copyright (C) 2021 Sean Anderson <sean.anderson at seco.com>
>>   >>> + */
>>   >>> +
>>   >>> +#include <common.h>
>>   >>> +#include <dm.h>
>>   >>> +#include <dm/device_compat.h>
>>   >
>>   > should be at end
>>
>> Can you clarify the ordering rules? I was following [1] which perscribes
>>
>>          <common.h>
>>          <others.h>
>>          <asm/...>
>>          <arch/arm/...>
>>          <linux/...>
>>          "local.h"
>>
>> [1] https://www.denx.de/wiki/U-Boot/CodingStyle#Include_files
> 
> Yes that's right, but dm/ is a directory so it goes with the other
> dirs at the end. I'll update the page to include dm/

Ok, so the general rule should be that directories go last?

--Sean

> [..]
> 
>>   >>> +
>>   >>> +     if (!dev_read_bool(dev, "revisions") || !dev_read_bool(dev, "names")) {
>>   >
>>   > I think this is a bit grubby since they are not bool properties...can
>>   > you use dev_read_prop() instead?
>>
>> I suppose. I think this is cleaner, since dev_read_bool is really an
>> alternatively-named dev_prop_exists.
> 
> Well, adding that would be a nice step and I agree it would be even better!
> 
> Regards,
> Simon
>