From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
To: Sungbo Eo <mans0n@gorani.run>
Cc: Linus Walleij <linus.walleij@linaro.org>,
LKML <linux-kernel@vger.kernel.org>,
linux-gpio <linux-gpio@vger.kernel.org>
Subject: Re: [PATCH] gpio: add GPO driver for PCA9570
Date: Wed, 24 Jun 2020 14:29:35 +0200 [thread overview]
Message-ID: <CAMpxmJVvU1Q2OHYfqqAMcojniQ6TSK+n5AejddwZ=pArtUWNYg@mail.gmail.com> (raw)
In-Reply-To: <20200623060526.29922-1-mans0n@gorani.run>
wt., 23 cze 2020 o 08:05 Sungbo Eo <mans0n@gorani.run> napisał(a):
>
> This patch adds support for the PCA9570 I2C GPO expander.
>
> Signed-off-by: Sungbo Eo <mans0n@gorani.run>
Hi Sungbo, this looks pretty good, but there are some nits listed below.
> ---
> Tested in kernel 5.4 on an ipq40xx platform.
>
> This is my first time submitting a whole driver patch, and I'm not really familiar with this PCA expander series.
> Please let me know how I can improve this patch further. (Do I also need to document the DT compatible string?)
>
Yes, you should send a separate patch to DT maintainers with DT
binding document (preferably in YAML). Please see
Documentations/devicetree/bindings for more info.
> FYI there's an unmerged patch for this chip.
> http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2017-May/105602.html
> I don't have PCA9571 either so I didn't add support for it.
> ---
> drivers/gpio/Kconfig | 8 ++
> drivers/gpio/Makefile | 1 +
> drivers/gpio/gpio-pca9570.c | 159 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 168 insertions(+)
> create mode 100644 drivers/gpio/gpio-pca9570.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index c6b5c65c8405..d10dcb81b841 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -962,6 +962,14 @@ config GPIO_PCA953X_IRQ
> Say yes here to enable the pca953x to be used as an interrupt
> controller. It requires the driver to be built in the kernel.
>
> +config GPIO_PCA9570
> + tristate "PCA9570 4-Bit I2C GPO expander"
> + help
> + Say yes here to enable the GPO driver for the NXP PCA9570 chip.
> +
> + To compile this driver as a module, choose M here: the module will
> + be called gpio-pca9570.
> +
> config GPIO_PCF857X
> tristate "PCF857x, PCA{85,96}7x, and MAX732[89] I2C GPIO expanders"
> select GPIOLIB_IRQCHIP
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 1e4894e0bf0f..33cb40c28a61 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -110,6 +110,7 @@ obj-$(CONFIG_GPIO_OCTEON) += gpio-octeon.o
> obj-$(CONFIG_GPIO_OMAP) += gpio-omap.o
> obj-$(CONFIG_GPIO_PALMAS) += gpio-palmas.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_PCIE_IDIO_24) += gpio-pcie-idio-24.o
> diff --git a/drivers/gpio/gpio-pca9570.c b/drivers/gpio/gpio-pca9570.c
> new file mode 100644
> index 000000000000..9ed01554f5df
> --- /dev/null
> +++ b/drivers/gpio/gpio-pca9570.c
> @@ -0,0 +1,159 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Driver for PCA9570 I2C GPO expander
> + *
> + * Copyright (C) 2020 Sungbo Eo <mans0n@gorani.run>
> + *
> + * Based on gpio-tpic2810.c
> + * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com/
> + * Andrew F. Davis <afd@ti.com>
> + */
> +
> +#include <linux/gpio/driver.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +
> +/**
> + * struct pca9570 - GPIO driver data
> + * @chip: GPIO controller chip
> + * @client: I2C device pointer
> + * @buffer: Buffer for device register
> + * @lock: Protects write sequences
> + */
> +struct pca9570 {
> + struct gpio_chip chip;
> + struct i2c_client *client;
> + u8 buffer;
> + struct mutex lock;
> +};
> +
> +static void pca9570_set(struct gpio_chip *chip, unsigned offset, int value);
> +
Please just move this function here instead of declaring it and
implementing it later.
[snip!]
> +
> +static int pca9570_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct pca9570 *gpio;
> + int ret;
> +
> + gpio = devm_kzalloc(&client->dev, sizeof(*gpio), GFP_KERNEL);
> + if (!gpio)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(client, gpio);
> +
> + gpio->chip = template_chip;
> + gpio->chip.parent = &client->dev;
> +
> + gpio->client = client;
> +
> + mutex_init(&gpio->lock);
> +
> + ret = gpiochip_add_data(&gpio->chip, gpio);
Why not devm_gpiochip_add_data()? You could drop the remove callback.
> + if (ret < 0) {
> + dev_err(&client->dev, "Unable to register gpiochip\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int pca9570_remove(struct i2c_client *client)
> +{
> + struct pca9570 *gpio = i2c_get_clientdata(client);
> +
> + gpiochip_remove(&gpio->chip);
> +
> + return 0;
> +}
> +
[snip!]
Bartosz
next prev parent reply other threads:[~2020-06-24 12:29 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-23 6:05 [PATCH] gpio: add GPO driver for PCA9570 Sungbo Eo
2020-06-23 8:31 ` Michael Walle
2020-06-23 12:22 ` Sungbo Eo
2020-06-23 12:47 ` Michael Walle
2020-06-24 13:33 ` Andy Shevchenko
2020-06-24 13:46 ` Michael Walle
2020-06-24 14:52 ` Andy Shevchenko
2020-06-27 16:58 ` Michael Walle
2020-06-24 12:29 ` Bartosz Golaszewski [this message]
2020-06-24 13:01 ` Andy Shevchenko
2020-06-25 7:34 ` Sungbo Eo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAMpxmJVvU1Q2OHYfqqAMcojniQ6TSK+n5AejddwZ=pArtUWNYg@mail.gmail.com' \
--to=bgolaszewski@baylibre.com \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mans0n@gorani.run \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).