This should just go to the gpio people and not staging.
The address is linux-gpio@xxxxxxxxxxxxxxx CC the Device Tree people for
answers the device tree questions: devicetree@xxxxxxxxxxxxxxx
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@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
