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

[Simon Glass]

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/
[..]

>  >>> +
>  >>> +     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