Re: [PATCH v6 08/12] gpio: Add GPIO driver for the RK805 PMIC

[Jianhong Chen]

å 2017/6/9 20:17, Heiko Stuebner åé:
> 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@xxxxxxxxxxxxxx> wrote:
> >
> > > From: chenjh <chenjh@xxxxxxxxxxxxxx>
> > Full name please.
> git config --global user.name "John Doe"
>
> might do the 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@xxxxxxxxxxxxxx>
> > 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
This driver is not only for rk805, but also intend for rk816 and furtrue 
PMICs.
The rk816 has one multi function pin(TS/GPIO), when setting as gpio, it 
can be configured as output or input.
Here is simple description from manual: "Thermistor input. Connect a 
thermistor from this pin to ground. The thermistor is usually inside the 
battery pack. (multi-function for GPIO) ".

At beginning, I want to name it "gpio-rk8xx.c", but I found rk808 and 
rk818 don't have gpio function. So I abandon this name and use 
"gpio-rk805.c", but still implement
it as a generic driver for furture PMICs to be added.
> > > +#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?
Yes, as explain above,  these "OUTPUT_MODE, INPUTOUT" flags are prepared 
for RK816 or furture PMICs.  Maybe these should be enum are better.
> > > +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?

Becuase I refered to other PMICs, I see most of their gpio driver is 
merged into drivers/gpio/*,  Only a few are added in drivers/pinctrl/*.
So, it is better to merge the driver into drivers/pinctrl/* ?

> > > +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@xxxxxxxxxxxxxxxxxxx
> > http://lists.infradead.org/mailman/listinfo/linux-rockchip

--
éåæ
E-mailïchenjh@xxxxxxxxxxxxxx
çåçèåçåèäæéåå
Fuzhou Rockchip Electronics Co.Ltd
çåççååéçèèäåé89åèäåAå21åæ (350003)
No. 21 Building, A District, No.89,software Boulevard Fuzhou,Fujian,PRC
TELï0591-83991906/07-8573