Hi Sean,
On Mon, 1 Mar 2021 at 16:08, Sean Anderson <sean.ander...@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.ander...@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 <s...@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....@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.ander...@seco.com>
> > + */
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <dm/device_compat.h>
should be at end
> > +#include <log.h>
> > +#include <sysinfo.h>
> > +#include <asm/gpio.h>
> > +
> > +struct sysinfo_gpio_priv {
needs comment
> > + struct gpio_desc *gpios;
> > + int gpio_num, revision;
> > +};
> > +
> > +static int sysinfo_gpio_detect(struct udevice *dev)
> > +{
> > + struct sysinfo_gpio_priv *priv = dev_get_priv(dev);
> > +
> > + priv->revision = dm_gpio_get_values_as_int_base3(priv->gpios,
> > + priv->gpio_num);
> > + return priv->revision < 0 ? priv->revision : 0;
> > +}
> > +
> > +static int sysinfo_gpio_get_int(struct udevice *dev, int id, int *val)
> > +{
> > + struct sysinfo_gpio_priv *priv = dev_get_priv(dev);
> > +
> > + switch (id) {
> > + case SYSINFO_ID_REVISION:
> > + if (priv->revision < 0) {
>
> Looks like I forgot to remove this brace. Will be fixed in v2.
>
> --Sean
>
> > + return priv->revision;
> > + *val = priv->revision;
> > + return 0;
> > + default:
> > + return -EINVAL;
> > + };
> > +}
> > +
> > +static int sysinfo_gpio_get_str(struct udevice *dev, int id, size_t size,
> > char *val)
> > +{
> > + struct sysinfo_gpio_priv *priv = dev_get_priv(dev);
> > +
> > + switch (id) {
> > + case SYSINFO_ID_REVISION: {
> > + const char *name = NULL;
> > + int i, ret;
> > + u32 revision;
> > +
> > + if (priv->revision < 0)
> > + return priv->revision;
> > +
> > + for (i = 0; i < priv->gpio_num; i++) {
> > + ret = dev_read_u32_index(dev, "revisions", i,
> > + &revision);
> > + if (ret) {
> > + if (ret != -EOVERFLOW)
> > + return ret;
> > + break;
> > + }
> > +
> > + if (revision == priv->revision) {
> > + ret = dev_read_string_index(dev, "names", i,
> > + &name);
> > + if (ret < 0)
> > + return ret;
> > + break;
> > + }
> > + }
> > + if (!name)
> > + name = "unknown";
> > +
> > + strncpy(val, name, size);
> > + val[size - 1] = '\0';
> > + return 0;
> > + } default:
> > + return -EINVAL;
> > + };
> > +}
> > +
> > +static const struct sysinfo_ops sysinfo_gpio_ops = {
> > + .detect = sysinfo_gpio_detect,
> > + .get_int = sysinfo_gpio_get_int,
> > + .get_str = sysinfo_gpio_get_str,
> > +};
> > +
> > +static int sysinfo_gpio_probe(struct udevice *dev)
> > +{
> > + int ret;
> > + struct sysinfo_gpio_priv *priv = dev_get_priv(dev);
> > +
> > + priv->gpio_num = gpio_get_list_count(dev, "gpios");
> > + if (priv->gpio_num < 0) {
> > + dev_err(dev, "could not get gpios length (err = %d)\n",
> > + priv->gpio_num);
> > + return priv->gpio_num;
> > + }
> > +
> > + priv->gpios = calloc(priv->gpio_num, sizeof(*priv->gpios));
> > + if (!priv->gpios) {
> > + dev_err(dev, "could not allocate memory for %d gpios\n",
> > + priv->gpio_num);
> > + return -ENOMEM;
> > + }
> > +
> > + ret = gpio_request_list_by_name(dev, "gpios", priv->gpios,
> > + priv->gpio_num, GPIOD_IS_IN);
> > + if (ret != priv->gpio_num) {
> > + dev_err(dev, "could not get gpios (err = %d)\n",
> > + priv->gpio_num);
> > + return ret;
> > + }
> > +
> > + 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?
> > + dev_err(dev, "revisions or names properties missing\n");
> > + return -ENOENT;
> > + }
> > +
> > + priv->revision = -ENOENT;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct udevice_id sysinfo_gpio_ids[] = {
> > + { .compatible = "gpio-sysinfo" },
> > + { /* sentinel */ }
> > +};
> > +
> > +U_BOOT_DRIVER(sysinfo_gpio) = {
> > + .name = "sysinfo_gpio",
> > + .id = UCLASS_SYSINFO,
> > + .of_match = sysinfo_gpio_ids,
> > + .ops = &sysinfo_gpio_ops,
> > + .priv_auto = sizeof(struct sysinfo_gpio_priv),
> > + .probe = sysinfo_gpio_probe,
> > +};
> >
Regards,
Simon
