[PATCH] added pca9570 driver

[PATCH] added pca9570 driver

[Chris Dew]

First, apologies, this is my first Linux kernel patch in at least 15 
years.  While I have spent a few hours reading various pieces of 
documentation about the Linux kernel development processes, I have 
probably missed some critical files entirely.

Also, there are probably a few coding issues in this patch.

This driver has only been tested to work on our custom ARM board, where 
we give it the following device tree info:

arch/arm/boot/dts/imx6qdl-smarcfimx6.dtsi:

...
&i2c1 {
         ...
         pca9570: pca9570@24 {
                 compatible = "pca9570";
                 reg = <0x24>;
                 gpio-controller;
         };
};

I have some questions:

- Is the "value", with which struct gpio_chip.set is called, guaranteed 
to be 0 or 1?

- Do I need to implement any form of locking around this driver, or is 
that done for me in layers above?

- The pca9571 is an 8-bit version of this chip.  Is leaving pca9571 
support to a later patch the best idea?  (I do not have one to test, nor 
any way to test one, at the moment.)

- This code was written and tested against a vendor-modified 4.1.15 
kernel tree.  This patch is made against the stable 4.9.9.  To make it 
compile against the newer kernel I had to replace references to struct 
gpio_chip.dev with struct gpio_chip.parent, as that struct member had 
been renamed between kernel versions.

- Should this driver be in the staging directory?

   - If so, how to do this, as staging does not seem to have either a 
staging/gpio nor a staging/include/linux/i2c directory to put the 
driver's files in.

Please let me know what else I need to read and do, to make this patch 
into something that stands a chance of being accepted upstream.

All the best,

Chris.


---
  drivers/gpio/Kconfig        |   7 ++
  drivers/gpio/Makefile       |   1 +
  drivers/gpio/gpio-pca9570.c | 253 
++++++++++++++++++++++++++++++++++++++++++++
  include/linux/i2c/pca9570.h |  22 ++++
  4 files changed, 283 insertions(+)
  create mode 100644 drivers/gpio/gpio-pca9570.c
  create mode 100644 include/linux/i2c/pca9570.h

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 0504307..a068cdb 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -762,6 +762,13 @@ config GPIO_PCA953X

  	  40 bits:	pca9505, pca9698

+config GPIO_PCA9570
+	tristate "PCA9570 GPO expander"
+	depends on I2C
+	help
+	  Say yes here to provide access to the PCA9570 SMBus GPO expander,
+	  made by NXP.
+
  config GPIO_PCA953X_IRQ
  	bool "Interrupt controller support for PCA953x"
  	depends on GPIO_PCA953X=y
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index becb96c..ec7946f 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -90,6 +90,7 @@ obj-$(CONFIG_GPIO_MXS)		+= gpio-mxs.o
  obj-$(CONFIG_GPIO_OCTEON)	+= gpio-octeon.o
  obj-$(CONFIG_GPIO_OMAP)		+= gpio-omap.o
  obj-$(CONFIG_GPIO_PCA953X)	+= gpio-pca953x.o
+obj-$(CONFIG_GPIO_PCA9570)	+= gpio-pca9570.o
  obj-$(CONFIG_GPIO_PCF857X)	+= gpio-pcf857x.o
  obj-$(CONFIG_GPIO_PCH)		+= gpio-pch.o
  obj-$(CONFIG_GPIO_PCI_IDIO_16)	+= gpio-pci-idio-16.o
diff --git a/drivers/gpio/gpio-pca9570.c b/drivers/gpio/gpio-pca9570.c
new file mode 100644
index 0000000..b3dc0e3
--- /dev/null
+++ b/drivers/gpio/gpio-pca9570.c
@@ -0,0 +1,253 @@
+/*
+ * Driver for pca9570 I2C GPO expanders
+ * Note: 9570 is at 0x24 - these value must be set
+ * in device tree.
+ *
+ * Copyright (C) 2017 Chris Dew, Thorcom Systems Ltd.
+ *
+ * Derived from drivers/gpio/gpio-max732x.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/gpio/driver.h>
+#include <linux/interrupt.h>
+#include <linux/i2c.h>
+#include <linux/i2c/pca9570.h>
+#include <linux/of.h>
+
+#define PCA9570_GPIO_COUNT 4
+
+enum {
+	PCA9570,
+};
+
+static const struct i2c_device_id pca9570_id[] = {
+	{ "pca9570", PCA9570 },
+	{ },
+};
+MODULE_DEVICE_TABLE(i2c, pca9570_id);
+
+#ifdef CONFIG_OF
+static const struct of_device_id pca9570_of_table[] = {
+	{ .compatible = "nxp,pca9570" }, /* FIXME: I just put "nxp" in here as 
other drivers had a manufacturer name */
+	{ }
+};
+MODULE_DEVICE_TABLE(of, pca9570_of_table);
+#endif
+
+
+struct pca9570_chip {
+	struct gpio_chip gpio_chip;
+	struct i2c_client *client;
+	struct mutex lock; /* FIXME: does this driver need to do any locking? */
+	uint8_t	reg;
+};
+
+static inline struct pca9570_chip *to_pca9570(struct gpio_chip *gc)
+{
+	return container_of(gc, struct pca9570_chip, gpio_chip);
+}
+
+static struct pca9570_platform_data *of_gpio_pca9570(struct device *dev)
+{
+	struct pca9570_platform_data *pdata;
+
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+
+	pdata->gpio_base = -1;
+
+	return pdata;
+}
+
+static int pca9570_readb(struct pca9570_chip *chip, uint8_t *val)
+{
+	int ret;
+
+	ret = i2c_smbus_read_byte(chip->client);
+	if (ret < 0) {
+		dev_err(&chip->client->dev, "failed reading\n");
+		return ret;
+	}
+
+	*val = (uint8_t)ret;
+	return 0;
+}
+
+static int pca9570_writeb(struct pca9570_chip *chip, uint8_t val)
+{
+	int ret;
+
+	ret = i2c_smbus_write_byte(chip->client, val);
+	if (ret < 0) {
+		dev_err(&chip->client->dev, "failed writing\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int pca9570_gpio_get_value(struct gpio_chip *gc, unsigned off)
+{
+	struct pca9570_chip *chip = to_pca9570(gc);
+
+	uint8_t mask = (uint8_t) 1 << off;
+	return chip->reg & mask;
+}
+
+static void pca9570_gpio_set_value(struct gpio_chip *gc, unsigned off,
+					int val)
+{
+	struct pca9570_chip *chip = to_pca9570(gc);
+
+	uint8_t mask = (uint8_t) 1 << off;
+
+	chip->reg &= ~mask;
+	chip->reg |= mask * val;
+
+	pca9570_writeb(chip, chip->reg);
+}
+
+static int pca9570_setup_gpio(struct pca9570_chip *chip,
+					const struct i2c_device_id *id,
+					unsigned gpio_start)
+{
+	struct gpio_chip *gc = &chip->gpio_chip;
+
+	gc->set = pca9570_gpio_set_value;
+	gc->get = pca9570_gpio_get_value;
+
+	gc->can_sleep = true;
+
+	gc->base = gpio_start;
+	gc->ngpio = PCA9570_GPIO_COUNT;
+	gc->label = chip->client->name;
+	gc->owner = THIS_MODULE;
+
+	gc->parent = &chip->client->dev;
+
+	return PCA9570_GPIO_COUNT;
+}
+
+static int pca9570_probe(struct i2c_client *client,
+				   const struct i2c_device_id *id)
+{
+	struct pca9570_platform_data *pdata;
+	struct device_node *node;
+	struct pca9570_chip *chip;
+	int ret = 0, nr_port;
+
+	pdata = dev_get_platdata(&client->dev);
+	node = client->dev.of_node;
+
+	if (!pdata && node)
+		pdata = of_gpio_pca9570(&client->dev);
+
+	if (!pdata) {
+		dev_dbg(&client->dev, "no platform data\n");
+		return -EINVAL;
+	}
+
+	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
+	if (chip == NULL)
+		return -ENOMEM;
+	chip->client = client;
+
+	nr_port = pca9570_setup_gpio(chip, id, pdata->gpio_base);
+	chip->gpio_chip.parent = &client->dev;
+
+	ret = gpiochip_add(&chip->gpio_chip);
+	if (ret)
+		goto out_failed;
+
+	if (pdata && pdata->setup) {
+		ret = pdata->setup(client, chip->gpio_chip.base,
+				chip->gpio_chip.ngpio, pdata->context);
+		if (ret < 0)
+			dev_warn(&client->dev, "setup failed, %d\n", ret);
+	}
+
+	i2c_set_clientdata(client, chip);
+
+	ret = pca9570_readb(chip, &chip->reg);
+	if (ret)
+		goto out_failed;
+
+	return 0;
+
+out_failed:
+	if (chip->client)
+		i2c_unregister_device(chip->client);
+
+	return -1;
+}
+
+static int pca9570_remove(struct i2c_client *client)
+{
+	struct pca9570_platform_data *pdata = dev_get_platdata(&client->dev);
+	struct pca9570_chip *chip = i2c_get_clientdata(client);
+
+	if (pdata && pdata->teardown) {
+		int ret;
+
+		ret = pdata->teardown(client, chip->gpio_chip.base,
+				chip->gpio_chip.ngpio, pdata->context);
+		if (ret < 0) {
+			dev_err(&client->dev, "%s failed, %d\n",
+					"teardown", ret);
+			return ret;
+		}
+	}
+
+	gpiochip_remove(&chip->gpio_chip);
+
+	return 0;
+}
+
+static struct i2c_driver pca9570_driver = {
+	.driver = {
+		.name		= "pca9570",
+		.owner		= THIS_MODULE,
+		.of_match_table	= of_match_ptr(pca9570_of_table),
+	},
+	.probe		= pca9570_probe,
+	.remove		= pca9570_remove,
+	.id_table	= pca9570_id,
+};
+
+static int __init pca9570_init(void)
+{
+	return i2c_add_driver(&pca9570_driver);
+}
+/* register after i2c postcore initcall and before
+ * subsys initcalls that may rely on these GPIOs
+ */
+subsys_initcall(pca9570_init);
+
+static void __exit pca9570_exit(void)
+{
+	i2c_del_driver(&pca9570_driver);
+}
+module_exit(pca9570_exit);
+
+MODULE_AUTHOR("Chris Dew <chris.dew@thorcom.co.uk>");
+MODULE_DESCRIPTION("pca9570");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/i2c/pca9570.h b/include/linux/i2c/pca9570.h
new file mode 100644
index 0000000..77207ed
--- /dev/null
+++ b/include/linux/i2c/pca9570.h
@@ -0,0 +1,22 @@
+#ifndef __LINUX_I2C_PCA9570_H
+#define __LINUX_I2C_PCA9570_H
+
+/* platform data for the PCA9570 4 GPO expander driver */
+
+struct pca9570_platform_data {
+	/* number of the first GPIO */
+	unsigned	gpio_base;
+
+	/* interrupt base */
+	int		irq_base;
+
+	void		*context;	/* param to setup/teardown */
+
+	int		(*setup)(struct i2c_client *client,
+				unsigned gpio, unsigned ngpio,
+				void *context);
+	int		(*teardown)(struct i2c_client *client,
+				unsigned gpio, unsigned ngpio,
+				void *context);
+};
+#endif /* __LINUX_I2C_PCA9570_H */
-- 
2.7.4

Re: [PATCH] added pca9570 driver

[Greg KH]

On Mon, May 08, 2017 at 12:31:52PM +0100, Chris Dew wrote:
> First, apologies, this is my first Linux kernel patch in at least 15 years.
> While I have spent a few hours reading various pieces of documentation about
> the Linux kernel development processes, I have probably missed some critical
> files entirely.
> 
> Also, there are probably a few coding issues in this patch.
> 
> This driver has only been tested to work on our custom ARM board, where we
> give it the following device tree info:
> 
> arch/arm/boot/dts/imx6qdl-smarcfimx6.dtsi:
> 
> ...
> &i2c1 {
>         ...
>         pca9570: pca9570@24 {
>                 compatible = "pca9570";
>                 reg = <0x24>;
>                 gpio-controller;
>         };
> };
> 
> I have some questions:
> 
> - Is the "value", with which struct gpio_chip.set is called, guaranteed to
> be 0 or 1?
> 
> - Do I need to implement any form of locking around this driver, or is that
> done for me in layers above?
> 
> - The pca9571 is an 8-bit version of this chip.  Is leaving pca9571 support
> to a later patch the best idea?  (I do not have one to test, nor any way to
> test one, at the moment.)

These are all good questions to ask on the gpio mailing list.  Use
scripts/get_maintainer.pl to determine what people, and lists, to cc:
for a patch.  That will get you to the right people.

> - This code was written and tested against a vendor-modified 4.1.15 kernel
> tree.  This patch is made against the stable 4.9.9.  To make it compile
> against the newer kernel I had to replace references to struct gpio_chip.dev
> with struct gpio_chip.parent, as that struct member had been renamed between
> kernel versions.

4.9.9 is really old as well, you need to make a patch against Linus's
tree at the least, and linux-next is usually much better.  We can't go
back in time and add new drivers to old kernels.

> - Should this driver be in the staging directory?

Why would it belong there?  What needs to be done to it to get it out of
staging?  Hint, you need a TODO file for a staging driver listing that
type of thing...

Minor comments on your patch:


You need a proper changelog and signed-off-by: line, please read
Documentation/SubmittingPatches for all of the info you need.

> 
> 
> ---
>  drivers/gpio/Kconfig        |   7 ++
>  drivers/gpio/Makefile       |   1 +
>  drivers/gpio/gpio-pca9570.c | 253
> ++++++++++++++++++++++++++++++++++++++++++++

Your patch is line-wapped :(

>  include/linux/i2c/pca9570.h |  22 ++++
>  4 files changed, 283 insertions(+)
>  create mode 100644 drivers/gpio/gpio-pca9570.c
>  create mode 100644 include/linux/i2c/pca9570.h

Why do you need a .h file?

> +/*
> + * Driver for pca9570 I2C GPO expanders
> + * Note: 9570 is at 0x24 - these value must be set
> + * in device tree.
> + *
> + * Copyright (C) 2017 Chris Dew, Thorcom Systems Ltd.
> + *
> + * Derived from drivers/gpio/gpio-max732x.c
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.

Do you really mean "any later version"?

> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.

Always run scripts/checkpatch.pl on your patch, so you don't get grumpyu
maintainers telling you to run scripts/checkpatch.pl on your patch...

> +out_failed:
> +	if (chip->client)
> +		i2c_unregister_device(chip->client);
> +
> +	return -1;

Don't make up random error values, use proper -E codes instead.


thanks,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] added pca9570 driver

[Dan Carpenter]

This should just go to the gpio people and not staging.
The address is linux-gpio@vger.kernel.org  CC the Device Tree people for
answers the device tree questions: devicetree@vger.kernel.org
The subject should be:  [PATCH] gpio: pca9570: add pca9570 driver

The patch has a line wrapping problem so it doesn't apply properly.
Read Documentation/process/email-clients.rst

On Mon, May 08, 2017 at 12:31:52PM +0100, Chris Dew wrote:
> First, apologies, this is my first Linux kernel patch in at least 15 years.
> While I have spent a few hours reading various pieces of documentation about
> the Linux kernel development processes, I have probably missed some critical
> files entirely.
> 
> Also, there are probably a few coding issues in this patch.

There are some, yes.  Run scripts/checkpatch.pl --strict on your patch.

> 
> This driver has only been tested to work on our custom ARM board, where we
> give it the following device tree info:
> 
> arch/arm/boot/dts/imx6qdl-smarcfimx6.dtsi:
> 
> ...
> &i2c1 {
>         ...
>         pca9570: pca9570@24 {
>                 compatible = "pca9570";
>                 reg = <0x24>;
>                 gpio-controller;
>         };
> };
> 
> I have some questions:
> 
> - Is the "value", with which struct gpio_chip.set is called, guaranteed to
> be 0 or 1?
> 
> - Do I need to implement any form of locking around this driver, or is that
> done for me in layers above?
> 
> - The pca9571 is an 8-bit version of this chip.  Is leaving pca9571 support
> to a later patch the best idea?  (I do not have one to test, nor any way to
> test one, at the moment.)

My view is that you can't really hurt anything by adding broken support
for something that currently has no support.  But note the untested
parts in your commit message.

> 
> - This code was written and tested against a vendor-modified 4.1.15 kernel
> tree.  This patch is made against the stable 4.9.9.  To make it compile
> against the newer kernel I had to replace references to struct gpio_chip.dev
> with struct gpio_chip.parent, as that struct member had been renamed between
> kernel versions.
> 

This suggests that you didn't write the original?  Please give
authorship credit.  Use From: Some Name <whatever@email.com> as the
first line of the email.  Otherwise, just write everything against
linux-next because we don't care about the history of it.

> - Should this driver be in the staging directory?
> 

Nah.

>   - If so, how to do this, as staging does not seem to have either a
> staging/gpio nor a staging/include/linux/i2c directory to put the driver's
> files in.
> 
> Please let me know what else I need to read and do, to make this patch into
> something that stands a chance of being accepted upstream.
> 
> All the best,
> 
> Chris.
> 

You need a Signed-Off-By on the real patch.

I've never reviewed GPIO driver, and don't know much about ARM or device
tree so take my review with a grain of salt.

> 
> ---
>  drivers/gpio/Kconfig        |   7 ++
>  drivers/gpio/Makefile       |   1 +
>  drivers/gpio/gpio-pca9570.c | 253
> ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/i2c/pca9570.h |  22 ++++
>  4 files changed, 283 insertions(+)
>  create mode 100644 drivers/gpio/gpio-pca9570.c
>  create mode 100644 include/linux/i2c/pca9570.h
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 0504307..a068cdb 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -762,6 +762,13 @@ config GPIO_PCA953X
> 
>  	  40 bits:	pca9505, pca9698
> 
> +config GPIO_PCA9570
> +	tristate "PCA9570 GPO expander"
> +	depends on I2C
> +	help
> +	  Say yes here to provide access to the PCA9570 SMBus GPO expander,
> +	  made by NXP.
> +
>  config GPIO_PCA953X_IRQ
>  	bool "Interrupt controller support for PCA953x"
>  	depends on GPIO_PCA953X=y
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index becb96c..ec7946f 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -90,6 +90,7 @@ obj-$(CONFIG_GPIO_MXS)		+= gpio-mxs.o
>  obj-$(CONFIG_GPIO_OCTEON)	+= gpio-octeon.o
>  obj-$(CONFIG_GPIO_OMAP)		+= gpio-omap.o
>  obj-$(CONFIG_GPIO_PCA953X)	+= gpio-pca953x.o
> +obj-$(CONFIG_GPIO_PCA9570)	+= gpio-pca9570.o
>  obj-$(CONFIG_GPIO_PCF857X)	+= gpio-pcf857x.o
>  obj-$(CONFIG_GPIO_PCH)		+= gpio-pch.o
>  obj-$(CONFIG_GPIO_PCI_IDIO_16)	+= gpio-pci-idio-16.o
> diff --git a/drivers/gpio/gpio-pca9570.c b/drivers/gpio/gpio-pca9570.c
> new file mode 100644
> index 0000000..b3dc0e3
> --- /dev/null
> +++ b/drivers/gpio/gpio-pca9570.c
> @@ -0,0 +1,253 @@
> +/*
> + * Driver for pca9570 I2C GPO expanders
> + * Note: 9570 is at 0x24 - these value must be set
> + * in device tree.

This can fit on the line before.

> + *
> + * Copyright (C) 2017 Chris Dew, Thorcom Systems Ltd.
> + *
> + * Derived from drivers/gpio/gpio-max732x.c
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/interrupt.h>
> +#include <linux/i2c.h>
> +#include <linux/i2c/pca9570.h>
> +#include <linux/of.h>
> +
> +#define PCA9570_GPIO_COUNT 4
> +
> +enum {
> +	PCA9570,
> +};
> +
> +static const struct i2c_device_id pca9570_id[] = {
> +	{ "pca9570", PCA9570 },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(i2c, pca9570_id);
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id pca9570_of_table[] = {
> +	{ .compatible = "nxp,pca9570" }, /* FIXME: I just put "nxp" in here as other drivers had a manufacturer name */
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, pca9570_of_table);
> +#endif
> +
> +
> +struct pca9570_chip {
> +	struct gpio_chip gpio_chip;
> +	struct i2c_client *client;
> +	struct mutex lock; /* FIXME: does this driver need to do any locking? */
> +	uint8_t	reg;

Replace all uint8_t style types with u8 style.

> +};
> +
> +static inline struct pca9570_chip *to_pca9570(struct gpio_chip *gc)
> +{
> +	return container_of(gc, struct pca9570_chip, gpio_chip);
> +}
> +
> +static struct pca9570_platform_data *of_gpio_pca9570(struct device *dev)
> +{
> +	struct pca9570_platform_data *pdata;
> +
> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return NULL;
> +
> +	pdata->gpio_base = -1;

This is really setting it to UINT_MAX.  Is this required?  TBH I don't
really understand how any of the pdata stuff is used.  Presumably it has
something to do with device tree???  I'm not familiar with this stuff.
But hopefully -1 feels like a magic number and slightly off given that
->gpio_base is unsigned.

> +
> +	return pdata;
> +}
> +
> +static int pca9570_readb(struct pca9570_chip *chip, uint8_t *val)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte(chip->client);
> +	if (ret < 0) {
> +		dev_err(&chip->client->dev, "failed reading\n");
> +		return ret;
> +	}
> +
> +	*val = (uint8_t)ret;
> +	return 0;
> +}
> +
> +static int pca9570_writeb(struct pca9570_chip *chip, uint8_t val)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_write_byte(chip->client, val);
> +	if (ret < 0) {

Use if (ret) here because it doesn't return positive values.

> +		dev_err(&chip->client->dev, "failed writing\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int pca9570_gpio_get_value(struct gpio_chip *gc, unsigned off)
> +{
> +	struct pca9570_chip *chip = to_pca9570(gc);
> +
> +	uint8_t mask = (uint8_t) 1 << off;
> +	return chip->reg & mask;

Don't put a blank in the middle of the declarations.  Put a blank after.
Remove the cast because it is a no-op and not required.  Don't put a
space after casts because the casts are high precedence operations.

> +}
> +
> +static void pca9570_gpio_set_value(struct gpio_chip *gc, unsigned off,
> +					int val)
> +{
> +	struct pca9570_chip *chip = to_pca9570(gc);
> +
> +	uint8_t mask = (uint8_t) 1 << off;

same.

> +
> +	chip->reg &= ~mask;
> +	chip->reg |= mask * val;

These lines are weird.  Oh...  val is only 1 and 0.  That's hella nasty.
I hate how hard you made me think about it.  Use the BIT() macro.

	u8 bit = BIT(off);

	if (val)
		chip->reg &= ~bit;
	else
		chip->reg |= bit;


These ->get/set() functions should probably have locking.

> +
> +	pca9570_writeb(chip, chip->reg);
> +}
> +
> +static int pca9570_setup_gpio(struct pca9570_chip *chip,
> +					const struct i2c_device_id *id,
> +					unsigned gpio_start)
> +{
> +	struct gpio_chip *gc = &chip->gpio_chip;
> +
> +	gc->set = pca9570_gpio_set_value;
> +	gc->get = pca9570_gpio_get_value;
> +
> +	gc->can_sleep = true;
> +
> +	gc->base = gpio_start;
> +	gc->ngpio = PCA9570_GPIO_COUNT;
> +	gc->label = chip->client->name;
> +	gc->owner = THIS_MODULE;
> +
> +	gc->parent = &chip->client->dev;
> +
> +	return PCA9570_GPIO_COUNT;

We never use this return value.  Just make it void.

> +}
> +
> +static int pca9570_probe(struct i2c_client *client,
> +				   const struct i2c_device_id *id)
> +{
> +	struct pca9570_platform_data *pdata;
> +	struct device_node *node;
> +	struct pca9570_chip *chip;
> +	int ret = 0, nr_port;
> +
> +	pdata = dev_get_platdata(&client->dev);
> +	node = client->dev.of_node;
> +
> +	if (!pdata && node)
> +		pdata = of_gpio_pca9570(&client->dev);
> +
> +	if (!pdata) {
> +		dev_dbg(&client->dev, "no platform data\n");
> +		return -EINVAL;
> +	}
> +
> +	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> +	if (chip == NULL)
> +		return -ENOMEM;
> +	chip->client = client;
> +
> +	nr_port = pca9570_setup_gpio(chip, id, pdata->gpio_base);
> +	chip->gpio_chip.parent = &client->dev;
> +
> +	ret = gpiochip_add(&chip->gpio_chip);
> +	if (ret)
> +		goto out_failed;

Use a label name which indicates what the goto does.  "goto unregister;"
Except why do we need to call i2c_unregister_device() where did we
register that stuff?  It feels like a layering violation to me.

> +
> +	if (pdata && pdata->setup) {
> +		ret = pdata->setup(client, chip->gpio_chip.base,
> +				chip->gpio_chip.ngpio, pdata->context);
> +		if (ret < 0)
> +			dev_warn(&client->dev, "setup failed, %d\n", ret);


Is this really the error handling you want?  It's not required to
succeed?  Can we check "if (ret) " consistently?  Does this really
return positives?  (Not rhetorical, I'm too lazy to check).

> +	}
> +
> +	i2c_set_clientdata(client, chip);
> +
> +	ret = pca9570_readb(chip, &chip->reg);
> +	if (ret)
> +		goto out_failed;

We should free up other resources: gpiochip_remove(), pdata->teardown()

> +
> +	return 0;
> +
> +out_failed:

As a personal favour to me, please don't use one err style error
handling.  See my essay on the subject.

https://plus.google.com/106378716002406849458/posts/dnanfhQ4mHQ

> +	if (chip->client)
            ^^^^^^^^^^^^
This is always true at this point.

> +		i2c_unregister_device(chip->client);
> +
> +	return -1;
               ^^
Should be "return ret;"

> +}
> +
> +static int pca9570_remove(struct i2c_client *client)
> +{
> +	struct pca9570_platform_data *pdata = dev_get_platdata(&client->dev);
> +	struct pca9570_chip *chip = i2c_get_clientdata(client);
> +
> +	if (pdata && pdata->teardown) {
> +		int ret;
> +
> +		ret = pdata->teardown(client, chip->gpio_chip.base,
> +				chip->gpio_chip.ngpio, pdata->context);
> +		if (ret < 0) {

		if (ret) {


regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel