diff mbox series

[v4,2/2] gpio: ds4520: Add ADI DS4520 GPIO Expander Support

Message ID 20230727085239.4326-3-okan.sahin@analog.com
State New
Headers
Series Add DS4520 GPIO Expander Support |

Commit Message

Sahin, Okan July 27, 2023, 8:52 a.m. UTC 
  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 | 89 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 101 insertions(+)
 create mode 100644 drivers/gpio/gpio-ds4520.c

Comments

Krzysztof Kozlowski July 27, 2023, 9:03 a.m. UTC | #1 
On 27/07/2023 10:52, Okan Sahin wrote:
> 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>

...

> +static int ds4520_gpio_probe(struct i2c_client *client)
> +{
> +	struct gpio_regmap_config config = { };
> +	struct device *dev = &client->dev;
> +	struct regmap *regmap;
> +	u32 ngpio;
> +	u32 base;
> +	int ret;
> +
> +	ret = device_property_read_u32(dev, "reg", &base);
> +	if (ret) {
> +		dev_err_probe(dev, ret,
> +			  "Missing 'reg' property.\n");
> +		return -EINVAL;

Nope.

> +	}
> +
> +	ret = device_property_read_u32(dev, "ngpios", &ngpio);
> +	if (ret) {
> +		dev_err_probe(dev, ret,
> +			  "Missing 'ngpios' property.\n");
> +		return -EINVAL;

Nope.

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

That's not correct syntax. What did you receive in previous
comments/feedback?

Best regards,
Krzysztof
Krzysztof Kozlowski July 27, 2023, 9:05 a.m. UTC | #2 
On 27/07/2023 11:03, Krzysztof Kozlowski wrote:
> On 27/07/2023 10:52, Okan Sahin wrote:
>> 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>
> 
> ...
> 
>> +static int ds4520_gpio_probe(struct i2c_client *client)
>> +{
>> +	struct gpio_regmap_config config = { };
>> +	struct device *dev = &client->dev;
>> +	struct regmap *regmap;
>> +	u32 ngpio;
>> +	u32 base;
>> +	int ret;
>> +
>> +	ret = device_property_read_u32(dev, "reg", &base);
>> +	if (ret) {
>> +		dev_err_probe(dev, ret,
>> +			  "Missing 'reg' property.\n");
>> +		return -EINVAL;
> 
> Nope.
> 
>> +	}
>> +
>> +	ret = device_property_read_u32(dev, "ngpios", &ngpio);
>> +	if (ret) {
>> +		dev_err_probe(dev, ret,
>> +			  "Missing 'ngpios' property.\n");
>> +		return -EINVAL;
> 
> Nope.
> 
>> +	}
>> +
>> +	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;
> 
> That's not correct syntax. What did you receive in previous
> comments/feedback?

Hm, I might be mixing patches, so maybe you never received feedback on
this. Anyway, all these three lines must be one line:

return dev_err_probe()

Previous places should be fixed similar way.

Best regards,
Krzysztof
Bartosz Golaszewski July 27, 2023, 9:09 a.m. UTC | #3 
On Thu, Jul 27, 2023 at 10:53 AM Okan Sahin <okan.sahin@analog.com> wrote:
>
> 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 | 89 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 101 insertions(+)
>  create mode 100644 drivers/gpio/gpio-ds4520.c
>
> 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..0a9fdbfed6ee
> --- /dev/null
> +++ b/drivers/gpio/gpio-ds4520.c
> @@ -0,0 +1,89 @@
> +// 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/property.h>
> +#include <linux/regmap.h>
> +
> +#define DS4520_PULLUP0         0xF0
> +#define DS4520_IO_CONTROL0     0xF2
> +#define DS4520_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 = { };
> +       struct device *dev = &client->dev;
> +       struct regmap *regmap;
> +       u32 ngpio;
> +       u32 base;
> +       int ret;
> +
> +       ret = device_property_read_u32(dev, "reg", &base);
> +       if (ret) {
> +               dev_err_probe(dev, ret,
> +                         "Missing 'reg' property.\n");
> +               return -EINVAL;
> +       }
> +
> +       ret = device_property_read_u32(dev, "ngpios", &ngpio);
> +       if (ret) {
> +               dev_err_probe(dev, ret,
> +                         "Missing 'ngpios' property.\n");
> +               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 + DS4520_IO_STATUS0;
> +       config.reg_set_base = base + DS4520_PULLUP0;
> +       config.reg_dir_out_base = base + DS4520_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,

I missed this previously but i2c drivers have completed the conversion
to the new probe syntax, so please use regular probe.

And please address Krzysztof's comments as well.

Thanks,
Bart

> +       .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");
> --
> 2.30.2
>
diff mbox series

Patch

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..0a9fdbfed6ee
--- /dev/null
+++ b/drivers/gpio/gpio-ds4520.c
@@ -0,0 +1,89 @@ 
+// 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/property.h>
+#include <linux/regmap.h>
+
+#define DS4520_PULLUP0		0xF0
+#define DS4520_IO_CONTROL0	0xF2
+#define DS4520_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 = { };
+	struct device *dev = &client->dev;
+	struct regmap *regmap;
+	u32 ngpio;
+	u32 base;
+	int ret;
+
+	ret = device_property_read_u32(dev, "reg", &base);
+	if (ret) {
+		dev_err_probe(dev, ret,
+			  "Missing 'reg' property.\n");
+		return -EINVAL;
+	}
+
+	ret = device_property_read_u32(dev, "ngpios", &ngpio);
+	if (ret) {
+		dev_err_probe(dev, ret,
+			  "Missing 'ngpios' property.\n");
+		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 + DS4520_IO_STATUS0;
+	config.reg_set_base = base + DS4520_PULLUP0;
+	config.reg_dir_out_base = base + DS4520_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");