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 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@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
>
> > +#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@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
