[PATCH 4/5] sysinfo: Add gpio-sysinfo driver
Sean Anderson
sean.anderson at seco.com
Fri Mar 5 16:19:47 CET 2021
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
>
>>> +#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",
>>> + prhttps://www.denx.de/wiki/U-Boot/CodingStyleiv->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?
I suppose. I think this is cleaner, since dev_read_bool is really an
alternatively-named dev_prop_exists.
--Sean
>>> + 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
>
More information about the U-Boot
mailing list