[v2,2/2] gpio: ds4520: Add ADI DS4520 GPIO Expander Support
Commit Message
The DS4520 is a 9-bit nonvolatile (NV) I/O expander.
It offers users a digitally programmable alternative
to hardware jumpers and mechanical switches that are
being used to control digital logic node.
Signed-off-by: Okan Sahin <okan.sahin@analog.com>
---
drivers/gpio/Kconfig | 11 +++++
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-ds4520.c | 83 ++++++++++++++++++++++++++++++++++++++
3 files changed, 95 insertions(+)
create mode 100644 drivers/gpio/gpio-ds4520.c
Comments
[Please include any former reviewers in new versions.]
> The DS4520 is a 9-bit nonvolatile (NV) I/O expander.
> It offers users a digitally programmable alternative
> to hardware jumpers and mechanical switches that are
> being used to control digital logic node.
Ok, what I just noticed is that this is an open-drain output buffer
with an optional pull-up, that should really go into the commit
message.
Also the commit message is misleading "it offers users a digitally
programmable alternative to hardware jumpers". While the hardware is
capable of that, this driver doesn't make use of it.
> + config.reg_dat_base = base + IO_STATUS0;
> + config.reg_set_base = base + PULLUP0;
> + config.reg_dir_out_base = base + IO_CONTROL0;
Given the above, I don't think this is correct. You pull the line low if
the line is in input mode (?). The line will be pulled low if the
corresponding bit in IO_CONTROL is zero. A one means, the pin is
floating. With open-drain buffers there are usually an external pull-ups,
so I'd treat the internal pull-up as optional and it is not necessary to
switch the actual line state.
In that case the following should be sufficient:
config.reg_dat_base = base + IO_STATUS0;
config.reg_set_base = base + IO_CONTROL0;
I'm not sure about the direction though. Technically speaking there is
no direction register. I'm not familiar with how open drain output are
modeled in linux. I'd expect the above is enough. Bartosz/Linus/Andy?
To enable the optional pull-up, you should refer to .set_config.
(You don't need to disable the pull-up if you pull the line low).
Regarding the SEE bit and wear out: The SEE bit seem to be shadowed by the
EEPROM, so if someone is setting the SEE bit it will be persisent. Changing
direction or output value will result in an EEPROM write and might wear out
the EEPROM. I'd like to hear others opinion on that. The worst case write
cycles are 50000. Fail the probe if the SEE bit is set seems not ideal.
Just ignoring that problem for now (or at least warn the user)?
-michael
On Tue, May 2, 2023 at 10:44 AM Michael Walle <michael@walle.cc> wrote:
> I'm not sure about the direction though. Technically speaking there is
> no direction register. I'm not familiar with how open drain output are
> modeled in linux. I'd expect the above is enough. Bartosz/Linus/Andy?
Linux has no special concept of open drain/source, it just sees the
.set(), .set_multiple() callbacks in gpio_chip to set the output value(s).
How that happens physically is an electrical question, Linux just expects
it to happen.
.get_direction() is however partly a software concept here, so you might
need to maintain a state in the driver for this. Linux will emulate open
drain when requested by simply putting the line into input (high-impedance)
mode for a high output, but only actively drive it low, which in most cases
is physically the same as open drain.
I imagine the direction would be unknown at probe() and only go into
a definitive input/output state after .set_direction() is called so some
tri-state enum?
.set_config() can be called with the special parameter
PIN_CONFIG_DRIVE_OPEN_DRAIN if the driver on the line can be
put explicitly into open drain state. If the hardware is permanently like
that, there is not much point in implementing it.
Yours,
Linus Walleij
Tue, May 02, 2023 at 02:05:16AM +0300, Okan Sahin kirjoitti:
> The DS4520 is a 9-bit nonvolatile (NV) I/O expander.
> It offers users a digitally programmable alternative
> to hardware jumpers and mechanical switches that are
> being used to control digital logic node.
...
> +#include <linux/device.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/gpio/regmap.h>
> +#include <linux/i2c.h>
Missing property.h.
> +#include <linux/regmap.h>
...
> +#define NUMBER_OF_GPIO 9
> +
> +#define PULLUP0 0xF0
> +#define IO_CONTROL0 0xF2
> +#define IO_STATUS0 0xF8
No namespace for the above?
...
> + struct gpio_regmap_config config = {0};
0 is not needed.
> + ngpio = NUMBER_OF_GPIO;
Do you really need this? Can Device Tree be sufficient here? (We have a
GPIO-wide property for that).
...
> + ret = device_property_read_u32(dev, "reg", &base);
> + if (ret)
> + return -EINVAL;
Why shadowing error?
...
> + regmap = devm_regmap_init_i2c(client, &ds4520_regmap_config);
> + if (IS_ERR(regmap)) {
> + ret = PTR_ERR(regmap);
> + dev_err_probe(dev, ret,
> + "Failed to allocate register map\n");
> + return ret;
return dev_err_probe();
> + }
...
> + config.ngpio = ngpio;
Why do you use temporary variable ngpio and not assign directly here?
@@ -1000,6 +1000,17 @@ config GPIO_ADNP
enough to represent all pins, but the driver will assume a
register layout for 64 pins (8 registers).
+config GPIO_DS4520
+ tristate "DS4520 I2C GPIO expander"
+ select REGMAP_I2C
+ select GPIO_REGMAP
+ help
+ GPIO driver for ADI DS4520 I2C-based GPIO expander.
+ Say yes here to enable the GPIO driver for the ADI DS4520 chip.
+
+ To compile this driver as a module, choose M here: the module will
+ be called gpio-ds4520.
+
config GPIO_GW_PLD
tristate "Gateworks PLD GPIO Expander"
depends on OF_GPIO
@@ -52,6 +52,7 @@ obj-$(CONFIG_GPIO_DA9052) += gpio-da9052.o
obj-$(CONFIG_GPIO_DA9055) += gpio-da9055.o
obj-$(CONFIG_GPIO_DAVINCI) += gpio-davinci.o
obj-$(CONFIG_GPIO_DLN2) += gpio-dln2.o
+obj-$(CONFIG_GPIO_DS4520) += gpio-ds4520.o
obj-$(CONFIG_GPIO_DWAPB) += gpio-dwapb.o
obj-$(CONFIG_GPIO_EIC_SPRD) += gpio-eic-sprd.o
obj-$(CONFIG_GPIO_EM) += gpio-em.o
new file mode 100644
@@ -0,0 +1,83 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2023 Analog Devices, Inc.
+ * Driver for the DS4520 I/O Expander
+ */
+
+#include <linux/device.h>
+#include <linux/gpio/driver.h>
+#include <linux/gpio/regmap.h>
+#include <linux/i2c.h>
+#include <linux/regmap.h>
+
+#define NUMBER_OF_GPIO 9
+
+#define PULLUP0 0xF0
+#define IO_CONTROL0 0xF2
+#define IO_STATUS0 0xF8
+
+static const struct regmap_config ds4520_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+};
+
+static int ds4520_gpio_probe(struct i2c_client *client)
+{
+ struct gpio_regmap_config config = {0};
+ struct device *dev = &client->dev;
+ struct regmap *regmap;
+ u32 ngpio;
+ u32 base;
+ int ret;
+
+ ngpio = NUMBER_OF_GPIO;
+
+ ret = device_property_read_u32(dev, "reg", &base);
+ if (ret)
+ return -EINVAL;
+ regmap = devm_regmap_init_i2c(client, &ds4520_regmap_config);
+ if (IS_ERR(regmap)) {
+ ret = PTR_ERR(regmap);
+ dev_err_probe(dev, ret,
+ "Failed to allocate register map\n");
+ return ret;
+ }
+
+ config.regmap = regmap;
+ config.parent = dev;
+ config.ngpio = ngpio;
+
+ config.reg_dat_base = base + IO_STATUS0;
+ config.reg_set_base = base + PULLUP0;
+ config.reg_dir_out_base = base + IO_CONTROL0;
+
+ return PTR_ERR_OR_ZERO(devm_gpio_regmap_register(dev, &config));
+}
+
+static const struct of_device_id ds4520_gpio_of_match_table[] = {
+ {
+ .compatible = "adi,ds4520-gpio"
+ },
+ { }
+};
+MODULE_DEVICE_TABLE(of, ds4520_gpio_of_match_table);
+
+static const struct i2c_device_id ds4520_gpio_id_table[] = {
+ { "ds4520-gpio" },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, ds4520_gpio_id_table);
+
+static struct i2c_driver ds4520_gpio_driver = {
+ .driver = {
+ .name = "ds4520-gpio",
+ .of_match_table = ds4520_gpio_of_match_table,
+ },
+ .probe_new = ds4520_gpio_probe,
+ .id_table = ds4520_gpio_id_table,
+};
+module_i2c_driver(ds4520_gpio_driver);
+
+MODULE_DESCRIPTION("DS4520 I/O Expander");
+MODULE_AUTHOR("Okan Sahin <okan.sahin@analog.com>");
+MODULE_LICENSE("GPL");