From: Heiko Stuebner <heiko@sntech.de>
To: linux-rockchip@lists.infradead.org
Cc: Linus Walleij <linus.walleij@linaro.org>,
Jianhong Chen <chenjh@rock-chips.com>,
Tao Huang <huangtao@rock-chips.com>,
Alexandre Courbot <gnurou@gmail.com>,
zhangqing@rock-chips.com,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
Mark Brown <broonie@kernel.org>,
Dmitry Torokhov <dmitry.torokhov@gmail.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Rob Herring <robh+dt@kernel.org>,
Liam Girdwood <lgirdwood@gmail.com>,
tony.xie@rock-chips.com,
"linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
Linux Input <linux-input@vger.kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
w.egorov@phytec.de
Subject: Re: [PATCH v6 08/12] gpio: Add GPIO driver for the RK805 PMIC
Date: Fri, 09 Jun 2017 14:17:42 +0200 [thread overview]
Message-ID: <2451251.ri2bNrhWeM@phil> (raw)
In-Reply-To: <CACRpkdZRKWPgFoM=Owo+EXU3bA_oymn5_3WRS9cuMr3G7Hdi6Q@mail.gmail.com>
Hi,
Am Freitag, 9. Juni 2017, 13:37:26 CEST schrieb Linus Walleij:
> Heiko, can you please look at this patch.
>
> On Thu, Jun 8, 2017 at 9:30 AM, Jianhong Chen <chenjh@rock-chips.com> wrote:
>
> > From: chenjh <chenjh@rock-chips.com>
>
> Full name please.
git config --global user.name "John Doe"
might do the trick and make this permanent for all your commits :-)
> > RK805 has two configurable GPIOs that can be used for several
> > purposes. These are output only.
> >
> > This driver is generic for other Rockchip PMICs to be added.
> >
> > Signed-off-by: chenjh <chenjh@rock-chips.com>
>
> Dito.
>
> Your commit message says they are output-only, yet you implement
> .direction_input(). So what is is going to be?
So far, I've only seen the rk808 and rk818. Both do not have any
configurable pins.
The rk805 which is a sort of variant of the above, does have the two
pins defined below, but in the manual I could also only find them as
output-only and having no other function than being output-pins.
So I don't really know if all the input- or "gpio-mode"- handling is only
an oversight (copy'n'paste) or if there are yet other rk808 variants around
that can actually be configured as inputs or even non-gpio modes?
I hope Jianhong will be able to answer that.
Heiko
>
> > +#include <linux/i2c.h>
> > +#include <linux/gpio.h>
>
> Only use:
> #include <linux/gpio/driver.h>
>
> > +/*
> > + * @mode: supported modes for this gpio, i.e. OUTPUT_MODE, OUTPUT_MODE...
>
> Are you saying this should be an enum or a set of flags?
>
> > +static int rk805_gpio_get(struct gpio_chip *chip, unsigned offset)
> > +{
> > + int ret, val;
> > + struct rk805_gpio *gpio = gpiochip_get_data(chip);
> > +
> > + ret = regmap_read(gpio->rk808->regmap, gpio->pins[offset].reg, &val);
> > + if (ret) {
> > + dev_err(gpio->dev, "gpio%d not support output mode\n", offset);
> > + return ret;
> > + }
> > +
> > + return (val & gpio->pins[offset].val_msk) ? 1 : 0;
>
> Do this:
>
> return !!(val & gpio->pins[offset].val_msk)
>
> > +static int rk805_gpio_request(struct gpio_chip *chip, unsigned offset)
> > +{
> > + int ret;
> > + struct rk805_gpio *gpio = gpiochip_get_data(chip);
> > +
> > + /* switch to gpio mode */
> > + if (gpio->pins[offset].func_mask) {
> > + ret = regmap_update_bits(gpio->rk808->regmap,
> > + gpio->pins[offset].reg,
> > + gpio->pins[offset].func_mask,
> > + gpio->pins[offset].func_mask);
> > + if (ret) {
> > + dev_err(gpio->dev, "set gpio%d func failed\n", offset);
> > + return ret;
> > + }
> > + }
> > +
> > + return 0;
> > +}
>
> This is pin control. Why don't you implement a proper pin control
> driver for this chip?
>
> If you don't, this will just come back and haunt you.
>
> Why not merge the driver into drivers/pinctrl/* and name it
> pinctrl-rk805.c to begin with?
>
> > +static const struct gpio_chip rk805_chip = {
> > + .label = "rk805-gpio",
> > + .owner = THIS_MODULE,
> > + .direction_input = rk805_gpio_direction_input,
> > + .direction_output = rk805_gpio_direction_output,
>
> Please implement .get_direction()
>
> > + .get = rk805_gpio_get,
> > + .set = rk805_gpio_set,
> > + .request = rk805_gpio_request,
> > + .base = -1,
> > + .ngpio = 2,
> > + .can_sleep = true,
>
> Consider assigning the .names[] array some pin names.
>
> Yours,
> Linus Walleij
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>
>
next prev parent reply other threads:[~2017-06-09 12:18 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-06-08 7:12 [PATCH v6 00/12] rk808: Add RK805 support Jianhong Chen
2017-06-08 7:12 ` [PATCH v6 01/12] mfd: rk808: fix up the chip id get failed Jianhong Chen
2017-06-08 7:12 ` [PATCH v6 02/12] mfd: rk808: add rk805 regs addr and ID Jianhong Chen
2017-06-08 7:12 ` [PATCH v6 03/12] regulator: rk808: Add regulator driver for RK805 Jianhong Chen
2017-06-08 11:11 ` Mark Brown
2017-06-08 7:12 ` [PATCH v6 04/12] mfd: rk808: Add RK805 support Jianhong Chen
2017-06-08 7:27 ` [PATCH v6 05/12] clk: Kconfig: Name RK805 in Kconfig for COMMON_CLK_RK808 Jianhong Chen
2017-06-08 7:28 ` [PATCH v6 06/12] rtc: Kconfig: Name RK805 in Kconfig for RTC_DRV_RK808 Jianhong Chen
2017-06-08 7:28 ` [PATCH v6 07/12] mfd: dt-bindings: Add RK805 device tree bindings document Jianhong Chen
2017-06-08 19:27 ` Rob Herring
2017-06-08 7:30 ` [PATCH v6 08/12] gpio: Add GPIO driver for the RK805 PMIC Jianhong Chen
2017-06-09 11:37 ` Linus Walleij
2017-06-09 12:17 ` Heiko Stuebner [this message]
2017-06-14 12:11 ` Jianhong Chen
2017-06-29 10:29 ` Heiko Stübner
2017-07-21 2:00 ` Jianhong Chen
2017-07-21 8:12 ` Heiko Stuebner
2017-06-08 7:30 ` [PATCH v6 09/12] Input: Add power key driver for Rockchip " Jianhong Chen
2017-06-20 3:06 ` Dmitry Torokhov
2017-06-08 7:31 ` [PATCH v6 10/12] mfd: rk808: Add RK805 gpio support Jianhong Chen
2017-06-08 7:32 ` [PATCH v6 11/12] mfd: rk808: Add RK805 power key support Jianhong Chen
2017-06-08 7:33 ` [PATCH v6 12/12] gpio: dt-bindings: add bindings for Rockchip RK805 PMIC Jianhong Chen
2017-06-09 11:30 ` Linus Walleij
2017-06-12 15:58 ` Rob Herring
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=2451251.ri2bNrhWeM@phil \
--to=heiko@sntech.de \
--cc=broonie@kernel.org \
--cc=chenjh@rock-chips.com \
--cc=devicetree@vger.kernel.org \
--cc=dmitry.torokhov@gmail.com \
--cc=gnurou@gmail.com \
--cc=huangtao@rock-chips.com \
--cc=lgirdwood@gmail.com \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rockchip@lists.infradead.org \
--cc=mark.rutland@arm.com \
--cc=robh+dt@kernel.org \
--cc=tony.xie@rock-chips.com \
--cc=w.egorov@phytec.de \
--cc=zhangqing@rock-chips.com \
/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).