| Message ID | 20230501230517.4491-3-okan.sahin@analog.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add DS4520 GPIO Expander Support | expand |
[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?
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 13be729710f2..5f89e46d6411 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -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 diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index c048ba003367..6f8656d5d617 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -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 diff --git a/drivers/gpio/gpio-ds4520.c b/drivers/gpio/gpio-ds4520.c new file mode 100644 index 000000000000..3358c2faa787 --- /dev/null +++ b/drivers/gpio/gpio-ds4520.c @@ -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");
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