Re: [PATCH v2 2/2] pinctrl: nuvoton: add NPCM7xx pinctrl and GPIO driver
Hi Linus,
On 30 July 2018 at 00:59, Linus Walleij wrote:
> On Thu, Jul 26, 2018 at 2:01 AM Tomer Maimon wrote:
>
> > I initialize bgpio as follow:
> >
> > ret = bgpio_init(>gpio_bank[id].gc,
> > pctrl->dev, 4,
> > pctrl->gpio_bank[id].base +
> > NPCM7XX_GP_N_DIN,
> > pctrl->gpio_bank[id].base +
> > NPCM7XX_GP_N_DOUT,
> > NULL,
> > NULL,
> > pctrl->gpio_bank[id].base +
> > NPCM7XX_GP_N_IEM,
> > BGPIOF_READ_OUTPUT_REG_SET);
>
> (...)
> > The problem occur when reading the GPIO value from bgpio_get_set
> function,
> > because the directions value are inverse it reading the wrong I/O
> registers
> >
> > For direction out it reading dat register (instead of set register)
> >
> > For direction in it calling set register (instead of dat register)
>
> Hm I don't quite get it... sorry. Maybe if you show your fix and what
> you expect to happen I can understand better?
>
Of course, in the last patch sent three days a go (V3 -
https://patchwork.ozlabs.org/patch/949942/) I did as follow to workaround
the issue:
int (*get)(struct gpio_chip *chip, unsigned offset);
int (*get_multiple)(struct gpio_chip *chip, unsigned long *mask,
unsigned long *bits);
..
static int npcmgpio_get_value(struct gpio_chip *chip, unsigned int offset)
{
struct npcm7xx_gpio *bank = gpiochip_get_data(chip);
unsigned long tmp_bgpio_dir = bank->gc.bgpio_dir;
int val;
/*
* sets bgpio_dir parameter value to the opposite value
* for calling the right registers in bgpio_get_set
* function
*/
* bank->gc.bgpio_dir = ~bank->gc.bgpio_dir; val =
bank->get(chip, offset); bank->gc.bgpio_dir = tmp_bgpio_dir;*
return val;
}
static int npcmgpio_get_multiple_value(struct gpio_chip *chip,
unsigned long *mask, unsigned long
*bits)
{
struct npcm7xx_gpio *bank = gpiochip_get_data(chip);
unsigned long tmp_bgpio_dir = bank->gc.bgpio_dir;
int val;
/*
* sets bgpio_dir parameter value to the opposite value
* for calling the right registers in bgpio_get_set_multiple
* function
*/
* bank->gc.bgpio_dir = ~bank->gc.bgpio_dir; val =
bank->get_multiple(chip, mask, bits); bank->gc.bgpio_dir =
tmp_bgpio_dir;*
return val;
}
..
pctrl->gpio_bank[id].get = pctrl->gpio_bank[id].gc.get;
pctrl->gpio_bank[id].gc.get = npcmgpio_get_value;
pctrl->gpio_bank[id].get_multiple = ptrl->gpio_bank[id].gc.get_mul
tiple;
pctrl->gpio_bank[id].gc.get_multiple = npcmgpio_get_multiple_value;
but it is not that good solution, because the bold commands are not atomic
(locked) operations.
>
> Do you mean that because you write the inverse value to
> IEM this happens, and the BGPIO code assumes that
> you always write 1 to set a line as input and 0 to set it
> as output?
>
yes, because of it the bgpio_get_set and bgpio_get_set_multiple setting the
opposite data registers .
>
> I would say if this causes the problem we should just add
> a new BGPIOF_INVERTED_REG_DIR with comment in
> include/linux/gpio/driver.h and make the necessary fix to
> respect this flag in the gpio-mmio.c core so it works right.
>
> If you do this as a separate patch I would be grateful :)
>
Sure, I will send a separate patch later on to overcome it.
>
> Yours,
> Linus Walleij
>
Thanks,
Tomer
Re: [PATCH v2 2/2] pinctrl: nuvoton: add NPCM7xx pinctrl and GPIO driver
Hi Linus,
On 30 July 2018 at 00:59, Linus Walleij wrote:
> On Thu, Jul 26, 2018 at 2:01 AM Tomer Maimon wrote:
>
> > I initialize bgpio as follow:
> >
> > ret = bgpio_init(>gpio_bank[id].gc,
> > pctrl->dev, 4,
> > pctrl->gpio_bank[id].base +
> > NPCM7XX_GP_N_DIN,
> > pctrl->gpio_bank[id].base +
> > NPCM7XX_GP_N_DOUT,
> > NULL,
> > NULL,
> > pctrl->gpio_bank[id].base +
> > NPCM7XX_GP_N_IEM,
> > BGPIOF_READ_OUTPUT_REG_SET);
>
> (...)
> > The problem occur when reading the GPIO value from bgpio_get_set
> function,
> > because the directions value are inverse it reading the wrong I/O
> registers
> >
> > For direction out it reading dat register (instead of set register)
> >
> > For direction in it calling set register (instead of dat register)
>
> Hm I don't quite get it... sorry. Maybe if you show your fix and what
> you expect to happen I can understand better?
>
Of course, in the last patch sent three days a go (V3 -
https://patchwork.ozlabs.org/patch/949942/) I did as follow to workaround
the issue:
int (*get)(struct gpio_chip *chip, unsigned offset);
int (*get_multiple)(struct gpio_chip *chip, unsigned long *mask,
unsigned long *bits);
..
static int npcmgpio_get_value(struct gpio_chip *chip, unsigned int offset)
{
struct npcm7xx_gpio *bank = gpiochip_get_data(chip);
unsigned long tmp_bgpio_dir = bank->gc.bgpio_dir;
int val;
/*
* sets bgpio_dir parameter value to the opposite value
* for calling the right registers in bgpio_get_set
* function
*/
* bank->gc.bgpio_dir = ~bank->gc.bgpio_dir; val =
bank->get(chip, offset); bank->gc.bgpio_dir = tmp_bgpio_dir;*
return val;
}
static int npcmgpio_get_multiple_value(struct gpio_chip *chip,
unsigned long *mask, unsigned long
*bits)
{
struct npcm7xx_gpio *bank = gpiochip_get_data(chip);
unsigned long tmp_bgpio_dir = bank->gc.bgpio_dir;
int val;
/*
* sets bgpio_dir parameter value to the opposite value
* for calling the right registers in bgpio_get_set_multiple
* function
*/
* bank->gc.bgpio_dir = ~bank->gc.bgpio_dir; val =
bank->get_multiple(chip, mask, bits); bank->gc.bgpio_dir =
tmp_bgpio_dir;*
return val;
}
..
pctrl->gpio_bank[id].get = pctrl->gpio_bank[id].gc.get;
pctrl->gpio_bank[id].gc.get = npcmgpio_get_value;
pctrl->gpio_bank[id].get_multiple = ptrl->gpio_bank[id].gc.get_mul
tiple;
pctrl->gpio_bank[id].gc.get_multiple = npcmgpio_get_multiple_value;
but it is not that good solution, because the bold commands are not atomic
(locked) operations.
>
> Do you mean that because you write the inverse value to
> IEM this happens, and the BGPIO code assumes that
> you always write 1 to set a line as input and 0 to set it
> as output?
>
yes, because of it the bgpio_get_set and bgpio_get_set_multiple setting the
opposite data registers .
>
> I would say if this causes the problem we should just add
> a new BGPIOF_INVERTED_REG_DIR with comment in
> include/linux/gpio/driver.h and make the necessary fix to
> respect this flag in the gpio-mmio.c core so it works right.
>
> If you do this as a separate patch I would be grateful :)
>
Sure, I will send a separate patch later on to overcome it.
>
> Yours,
> Linus Walleij
>
Thanks,
Tomer
Re: [PATCH v2 2/2] pinctrl: nuvoton: add NPCM7xx pinctrl and GPIO driver
On Thu, Jul 26, 2018 at 2:01 AM Tomer Maimon wrote: > I initialize bgpio as follow: > > ret = bgpio_init(>gpio_bank[id].gc, > pctrl->dev, 4, > pctrl->gpio_bank[id].base + > NPCM7XX_GP_N_DIN, > pctrl->gpio_bank[id].base + > NPCM7XX_GP_N_DOUT, > NULL, > NULL, > pctrl->gpio_bank[id].base + > NPCM7XX_GP_N_IEM, > BGPIOF_READ_OUTPUT_REG_SET); (...) > The problem occur when reading the GPIO value from bgpio_get_set function, > because the directions value are inverse it reading the wrong I/O registers > > For direction out it reading dat register (instead of set register) > > For direction in it calling set register (instead of dat register) Hm I don't quite get it... sorry. Maybe if you show your fix and what you expect to happen I can understand better? Do you mean that because you write the inverse value to IEM this happens, and the BGPIO code assumes that you always write 1 to set a line as input and 0 to set it as output? I would say if this causes the problem we should just add a new BGPIOF_INVERTED_REG_DIR with comment in include/linux/gpio/driver.h and make the necessary fix to respect this flag in the gpio-mmio.c core so it works right. If you do this as a separate patch I would be grateful :) Yours, Linus Walleij
Re: [PATCH v2 2/2] pinctrl: nuvoton: add NPCM7xx pinctrl and GPIO driver
On Thu, Jul 26, 2018 at 2:01 AM Tomer Maimon wrote: > I initialize bgpio as follow: > > ret = bgpio_init(>gpio_bank[id].gc, > pctrl->dev, 4, > pctrl->gpio_bank[id].base + > NPCM7XX_GP_N_DIN, > pctrl->gpio_bank[id].base + > NPCM7XX_GP_N_DOUT, > NULL, > NULL, > pctrl->gpio_bank[id].base + > NPCM7XX_GP_N_IEM, > BGPIOF_READ_OUTPUT_REG_SET); (...) > The problem occur when reading the GPIO value from bgpio_get_set function, > because the directions value are inverse it reading the wrong I/O registers > > For direction out it reading dat register (instead of set register) > > For direction in it calling set register (instead of dat register) Hm I don't quite get it... sorry. Maybe if you show your fix and what you expect to happen I can understand better? Do you mean that because you write the inverse value to IEM this happens, and the BGPIO code assumes that you always write 1 to set a line as input and 0 to set it as output? I would say if this causes the problem we should just add a new BGPIOF_INVERTED_REG_DIR with comment in include/linux/gpio/driver.h and make the necessary fix to respect this flag in the gpio-mmio.c core so it works right. If you do this as a separate patch I would be grateful :) Yours, Linus Walleij
Re: [PATCH v2 2/2] pinctrl: nuvoton: add NPCM7xx pinctrl and GPIO driver
Hi Linus,
Thanks a lot for your comments.
Sorry for my late reply, I was on vacation.
The last days I have been working to move NPCM pinctrl GPIO to GENERIC
GPIO, most of the work have been done but I had the some issue.
I initialize bgpio as follow:
ret = bgpio_init(>gpio_bank[id].gc,
pctrl->dev, 4,
pctrl->gpio_bank[id].base +
NPCM7XX_GP_N_DIN,
pctrl->gpio_bank[id].base +
NPCM7XX_GP_N_DOUT,
NULL,
NULL,
pctrl->gpio_bank[id].base +
NPCM7XX_GP_N_IEM,
BGPIOF_READ_OUTPUT_REG_SET);
After doing it, the directions functions I used are:
bgpio_dir_out_inv, bgpio_dir_in_inv, bgpio_get_dir_inv
and the I/O get function is bgpio_get_set
By using inv directions:
direction out = 0 (gc->bgpio_dir &= ~bgpio_line2mask(gc, gpio))
direction in = 1 (gc->bgpio_dir |= bgpio_line2mask(gc, gpio))
The problem occur when reading the GPIO value from bgpio_get_set
function, because the directions value are inverse it reading the
wrong I/O registers
For direction out it reading dat register (instead of set register)
For direction in it calling set register (instead of dat register)
if (gc->bgpio_dir & pinmask)
return !!(gc->read_reg(gc->reg_set) & pinmask);
else
return !!(gc->read_reg(gc->reg_dat) & pinmask);
the same issue occur at bgpio_get_set_multiple function.
Maybe in bgpio_dir parameter direction out should be in both cases 1
and direction in = 0.
for now i did a local fix in the npcm pinctrl driver by setting
bgpio_dir parameters as direction out = 1 and direction in = 0.
Thanks a lot,
Tomer
On 13 July 2018 at 11:51, Linus Walleij wrote:
> On Thu, Jul 12, 2018 at 11:42 PM Tomer Maimon wrote:
>
> > Add Nuvoton BMC NPCM750/730/715/705 Pinmux and
> > GPIO controller driver.
> >
> > Signed-off-by: Tomer Maimon
>
> (...)
> > +++ b/drivers/pinctrl/nuvoton/pinctrl-npcm7xx.c
> > @@ -0,0 +1,2089 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (c) 2016-2018 Nuvoton Technology corporation.
> > +// Copyright (c) 2016, Dell Inc
> > +
> > +#include
> > +#include
>
> As this is a driver you should only include
>
> > +#include
> > +#include
> > +#include
>
> If you need syscon then the driver should select or depend
> on MFD_SYSCON in Kconfig.
>
> > +#include
> > +#include
> > +#include
> > +#include
>
> Do you really need this include?
>
> > +/* Structure for register banks */
> > +struct NPCM7XX_GPIO {
>
> Can we have this lowercase? Please?
>
> > + void __iomem*base;
> > + struct gpio_chipgc;
> > + int irqbase;
> > + int irq;
> > + spinlock_t lock;
> > + void*priv;
> > + struct irq_chip irq_chip;
> > + u32 pinctrl_id;
> > +};
>
> So each GPIO bank has its own gpio chip and register
> base, that is NICE! Because then it looks like you can
> select GPIO_GENERIC and use the MMIO GPIO helper
> library to handle the registers. Let's look into that
> option!
>
> > +struct NPCM7xx_pinctrl {
> > + struct pinctrl_dev *pctldev;
> > + struct device *dev;
> > + struct NPCM7XX_GPIO gpio_bank[NPCM7XX_GPIO_BANK_NUM];
> > + struct irq_domain *domain;
>
> I wonder why the pin controller needs and IRQ domain but
> I keep reading the code and I might find out...
>
> > +enum operand {
> > + op_set,
> > + op_getbit,
> > + op_setbit,
> > + op_clrbit,
> > +};
>
> This looks complicated. I suspect you can use GPIO_GENERIC
> to set/get and clear bits in the register(s).
>
> > +/* Perform locked bit operations on GPIO registers */
> > +static int gpio_bitop(struct NPCM7XX_GPIO *bank, int op, unsigned int
> offset,
> > + int reg)
> > +{
> > + unsigned long flags;
> > + u32 mask, val;
> > +
> > + mask = (1L << offset);
> > + spin_lock_irqsave(>lock, flags);
> > + switch (op) {
> > + case op_set:
> > + iowrite32(mask, bank->base + reg);
> > + break;
> > + case op_getbit:
> > + mask &= ioread32(bank->base + reg);
> > + break;
> > + case op_setbit:
> > + val = ioread32(bank->base + reg);
> > + iowrite32(val | mask, bank->base + reg);
> > + break;
> > + case op_clrbit:
> > + val = ioread32(bank->base + reg);
> > + iowrite32(val & (~mask), bank->base + reg);
> > + break;
> > + }
> > +
Re: [PATCH v2 2/2] pinctrl: nuvoton: add NPCM7xx pinctrl and GPIO driver
Hi Linus,
Thanks a lot for your comments.
Sorry for my late reply, I was on vacation.
The last days I have been working to move NPCM pinctrl GPIO to GENERIC
GPIO, most of the work have been done but I had the some issue.
I initialize bgpio as follow:
ret = bgpio_init(>gpio_bank[id].gc,
pctrl->dev, 4,
pctrl->gpio_bank[id].base +
NPCM7XX_GP_N_DIN,
pctrl->gpio_bank[id].base +
NPCM7XX_GP_N_DOUT,
NULL,
NULL,
pctrl->gpio_bank[id].base +
NPCM7XX_GP_N_IEM,
BGPIOF_READ_OUTPUT_REG_SET);
After doing it, the directions functions I used are:
bgpio_dir_out_inv, bgpio_dir_in_inv, bgpio_get_dir_inv
and the I/O get function is bgpio_get_set
By using inv directions:
direction out = 0 (gc->bgpio_dir &= ~bgpio_line2mask(gc, gpio))
direction in = 1 (gc->bgpio_dir |= bgpio_line2mask(gc, gpio))
The problem occur when reading the GPIO value from bgpio_get_set
function, because the directions value are inverse it reading the
wrong I/O registers
For direction out it reading dat register (instead of set register)
For direction in it calling set register (instead of dat register)
if (gc->bgpio_dir & pinmask)
return !!(gc->read_reg(gc->reg_set) & pinmask);
else
return !!(gc->read_reg(gc->reg_dat) & pinmask);
the same issue occur at bgpio_get_set_multiple function.
Maybe in bgpio_dir parameter direction out should be in both cases 1
and direction in = 0.
for now i did a local fix in the npcm pinctrl driver by setting
bgpio_dir parameters as direction out = 1 and direction in = 0.
Thanks a lot,
Tomer
On 13 July 2018 at 11:51, Linus Walleij wrote:
> On Thu, Jul 12, 2018 at 11:42 PM Tomer Maimon wrote:
>
> > Add Nuvoton BMC NPCM750/730/715/705 Pinmux and
> > GPIO controller driver.
> >
> > Signed-off-by: Tomer Maimon
>
> (...)
> > +++ b/drivers/pinctrl/nuvoton/pinctrl-npcm7xx.c
> > @@ -0,0 +1,2089 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (c) 2016-2018 Nuvoton Technology corporation.
> > +// Copyright (c) 2016, Dell Inc
> > +
> > +#include
> > +#include
>
> As this is a driver you should only include
>
> > +#include
> > +#include
> > +#include
>
> If you need syscon then the driver should select or depend
> on MFD_SYSCON in Kconfig.
>
> > +#include
> > +#include
> > +#include
> > +#include
>
> Do you really need this include?
>
> > +/* Structure for register banks */
> > +struct NPCM7XX_GPIO {
>
> Can we have this lowercase? Please?
>
> > + void __iomem*base;
> > + struct gpio_chipgc;
> > + int irqbase;
> > + int irq;
> > + spinlock_t lock;
> > + void*priv;
> > + struct irq_chip irq_chip;
> > + u32 pinctrl_id;
> > +};
>
> So each GPIO bank has its own gpio chip and register
> base, that is NICE! Because then it looks like you can
> select GPIO_GENERIC and use the MMIO GPIO helper
> library to handle the registers. Let's look into that
> option!
>
> > +struct NPCM7xx_pinctrl {
> > + struct pinctrl_dev *pctldev;
> > + struct device *dev;
> > + struct NPCM7XX_GPIO gpio_bank[NPCM7XX_GPIO_BANK_NUM];
> > + struct irq_domain *domain;
>
> I wonder why the pin controller needs and IRQ domain but
> I keep reading the code and I might find out...
>
> > +enum operand {
> > + op_set,
> > + op_getbit,
> > + op_setbit,
> > + op_clrbit,
> > +};
>
> This looks complicated. I suspect you can use GPIO_GENERIC
> to set/get and clear bits in the register(s).
>
> > +/* Perform locked bit operations on GPIO registers */
> > +static int gpio_bitop(struct NPCM7XX_GPIO *bank, int op, unsigned int
> offset,
> > + int reg)
> > +{
> > + unsigned long flags;
> > + u32 mask, val;
> > +
> > + mask = (1L << offset);
> > + spin_lock_irqsave(>lock, flags);
> > + switch (op) {
> > + case op_set:
> > + iowrite32(mask, bank->base + reg);
> > + break;
> > + case op_getbit:
> > + mask &= ioread32(bank->base + reg);
> > + break;
> > + case op_setbit:
> > + val = ioread32(bank->base + reg);
> > + iowrite32(val | mask, bank->base + reg);
> > + break;
> > + case op_clrbit:
> > + val = ioread32(bank->base + reg);
> > + iowrite32(val & (~mask), bank->base + reg);
> > + break;
> > + }
> > +
Re: [PATCH v2 2/2] pinctrl: nuvoton: add NPCM7xx pinctrl and GPIO driver
On Thu, Jul 12, 2018 at 11:42 PM Tomer Maimon wrote:
> Add Nuvoton BMC NPCM750/730/715/705 Pinmux and
> GPIO controller driver.
>
> Signed-off-by: Tomer Maimon
(...)
> +++ b/drivers/pinctrl/nuvoton/pinctrl-npcm7xx.c
> @@ -0,0 +1,2089 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2016-2018 Nuvoton Technology corporation.
> +// Copyright (c) 2016, Dell Inc
> +
> +#include
> +#include
As this is a driver you should only include
> +#include
> +#include
> +#include
If you need syscon then the driver should select or depend
on MFD_SYSCON in Kconfig.
> +#include
> +#include
> +#include
> +#include
Do you really need this include?
> +/* Structure for register banks */
> +struct NPCM7XX_GPIO {
Can we have this lowercase? Please?
> + void __iomem*base;
> + struct gpio_chipgc;
> + int irqbase;
> + int irq;
> + spinlock_t lock;
> + void*priv;
> + struct irq_chip irq_chip;
> + u32 pinctrl_id;
> +};
So each GPIO bank has its own gpio chip and register
base, that is NICE! Because then it looks like you can
select GPIO_GENERIC and use the MMIO GPIO helper
library to handle the registers. Let's look into that
option!
> +struct NPCM7xx_pinctrl {
> + struct pinctrl_dev *pctldev;
> + struct device *dev;
> + struct NPCM7XX_GPIO gpio_bank[NPCM7XX_GPIO_BANK_NUM];
> + struct irq_domain *domain;
I wonder why the pin controller needs and IRQ domain but
I keep reading the code and I might find out...
> +enum operand {
> + op_set,
> + op_getbit,
> + op_setbit,
> + op_clrbit,
> +};
This looks complicated. I suspect you can use GPIO_GENERIC
to set/get and clear bits in the register(s).
> +/* Perform locked bit operations on GPIO registers */
> +static int gpio_bitop(struct NPCM7XX_GPIO *bank, int op, unsigned int offset,
> + int reg)
> +{
> + unsigned long flags;
> + u32 mask, val;
> +
> + mask = (1L << offset);
> + spin_lock_irqsave(>lock, flags);
> + switch (op) {
> + case op_set:
> + iowrite32(mask, bank->base + reg);
> + break;
> + case op_getbit:
> + mask &= ioread32(bank->base + reg);
> + break;
> + case op_setbit:
> + val = ioread32(bank->base + reg);
> + iowrite32(val | mask, bank->base + reg);
> + break;
> + case op_clrbit:
> + val = ioread32(bank->base + reg);
> + iowrite32(val & (~mask), bank->base + reg);
> + break;
> + }
> + spin_unlock_irqrestore(>lock, flags);
> + return !!mask;
> +}
This is essentially a reimplementation of drivers/gpio/gpio-mmio.c
(GPIO_GENERIC, also using a spinlock to protect the registers)
so let's use that instead :)
There are drivers already that reuse the spinlock inside the
generic GPIO chip to protect their other registers like for
IRQ registers.
> +static int npcmgpio_get_direction(struct gpio_chip *chip, unsigned int
> offset)
> +{
> + struct NPCM7XX_GPIO *bank = gpiochip_get_data(chip);
> + u32 oe, ie;
> +
> + /* Get Input & Output state */
> + ie = gpio_bitop(bank, op_getbit, offset, NPCM7XX_GP_N_IEM);
> + oe = gpio_bitop(bank, op_getbit, offset, NPCM7XX_GP_N_OE);
> + if (ie && !oe)
> + return GPIOF_DIR_IN;
> + else if (oe && !ie)
> + return GPIOF_DIR_OUT;
These are consumer flags and should not be used in drivers.
Use plain 0/1 instead.
Anyways the problem goes away with GPIO_GENERIC.
> +static int npcmgpio_direction_input(struct gpio_chip *chip, unsigned int
> offset)
> +{
> + return pinctrl_gpio_direction_input(offset + chip->base);
> +}
It's a bit tricksy to get this to work with GPIO_GENERIC.
After calling bgpio_init() you need to overwrite the assigned
.direction_input handler with this and then direct back to the
one assigned by GPIO_GENERIC.
Something like this:
1. Add two indirection pointers to the npcm7xx_gpio state container:
struct npcm7xx_gpio {
(...)
int (*direction_input)(struct gpio_chip *chip, unsigned offset);
int (*direction_output)(struct gpio_chip *chip, unsigned offset,
int value);
(...)
};
2. Save the pointers
struct npcm7xx_gpio *npcm;
bgpio_init( ... register setup ...)
npcm->direction_input = npcm->gc.direction_input;
npcm->direction_output = npcm->gc.direction_output;
npcm->gc.direction_input = npcmgpio_direction_input;
npcm->gc.direction_output = npcmgpio_direction_output;
3. Modify the functions like that:
static int npcmgpio_direction_input(struct gpio_chip *chip, unsigned int offset)
{
struct npcm7xx_gpio *npcm = gpiochip_get_data(chip);
int ret;
ret = pinctrl_gpio_direction_input(offset + chip->base);
if (ret)
Re: [PATCH v2 2/2] pinctrl: nuvoton: add NPCM7xx pinctrl and GPIO driver
On Thu, Jul 12, 2018 at 11:42 PM Tomer Maimon wrote:
> Add Nuvoton BMC NPCM750/730/715/705 Pinmux and
> GPIO controller driver.
>
> Signed-off-by: Tomer Maimon
(...)
> +++ b/drivers/pinctrl/nuvoton/pinctrl-npcm7xx.c
> @@ -0,0 +1,2089 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2016-2018 Nuvoton Technology corporation.
> +// Copyright (c) 2016, Dell Inc
> +
> +#include
> +#include
As this is a driver you should only include
> +#include
> +#include
> +#include
If you need syscon then the driver should select or depend
on MFD_SYSCON in Kconfig.
> +#include
> +#include
> +#include
> +#include
Do you really need this include?
> +/* Structure for register banks */
> +struct NPCM7XX_GPIO {
Can we have this lowercase? Please?
> + void __iomem*base;
> + struct gpio_chipgc;
> + int irqbase;
> + int irq;
> + spinlock_t lock;
> + void*priv;
> + struct irq_chip irq_chip;
> + u32 pinctrl_id;
> +};
So each GPIO bank has its own gpio chip and register
base, that is NICE! Because then it looks like you can
select GPIO_GENERIC and use the MMIO GPIO helper
library to handle the registers. Let's look into that
option!
> +struct NPCM7xx_pinctrl {
> + struct pinctrl_dev *pctldev;
> + struct device *dev;
> + struct NPCM7XX_GPIO gpio_bank[NPCM7XX_GPIO_BANK_NUM];
> + struct irq_domain *domain;
I wonder why the pin controller needs and IRQ domain but
I keep reading the code and I might find out...
> +enum operand {
> + op_set,
> + op_getbit,
> + op_setbit,
> + op_clrbit,
> +};
This looks complicated. I suspect you can use GPIO_GENERIC
to set/get and clear bits in the register(s).
> +/* Perform locked bit operations on GPIO registers */
> +static int gpio_bitop(struct NPCM7XX_GPIO *bank, int op, unsigned int offset,
> + int reg)
> +{
> + unsigned long flags;
> + u32 mask, val;
> +
> + mask = (1L << offset);
> + spin_lock_irqsave(>lock, flags);
> + switch (op) {
> + case op_set:
> + iowrite32(mask, bank->base + reg);
> + break;
> + case op_getbit:
> + mask &= ioread32(bank->base + reg);
> + break;
> + case op_setbit:
> + val = ioread32(bank->base + reg);
> + iowrite32(val | mask, bank->base + reg);
> + break;
> + case op_clrbit:
> + val = ioread32(bank->base + reg);
> + iowrite32(val & (~mask), bank->base + reg);
> + break;
> + }
> + spin_unlock_irqrestore(>lock, flags);
> + return !!mask;
> +}
This is essentially a reimplementation of drivers/gpio/gpio-mmio.c
(GPIO_GENERIC, also using a spinlock to protect the registers)
so let's use that instead :)
There are drivers already that reuse the spinlock inside the
generic GPIO chip to protect their other registers like for
IRQ registers.
> +static int npcmgpio_get_direction(struct gpio_chip *chip, unsigned int
> offset)
> +{
> + struct NPCM7XX_GPIO *bank = gpiochip_get_data(chip);
> + u32 oe, ie;
> +
> + /* Get Input & Output state */
> + ie = gpio_bitop(bank, op_getbit, offset, NPCM7XX_GP_N_IEM);
> + oe = gpio_bitop(bank, op_getbit, offset, NPCM7XX_GP_N_OE);
> + if (ie && !oe)
> + return GPIOF_DIR_IN;
> + else if (oe && !ie)
> + return GPIOF_DIR_OUT;
These are consumer flags and should not be used in drivers.
Use plain 0/1 instead.
Anyways the problem goes away with GPIO_GENERIC.
> +static int npcmgpio_direction_input(struct gpio_chip *chip, unsigned int
> offset)
> +{
> + return pinctrl_gpio_direction_input(offset + chip->base);
> +}
It's a bit tricksy to get this to work with GPIO_GENERIC.
After calling bgpio_init() you need to overwrite the assigned
.direction_input handler with this and then direct back to the
one assigned by GPIO_GENERIC.
Something like this:
1. Add two indirection pointers to the npcm7xx_gpio state container:
struct npcm7xx_gpio {
(...)
int (*direction_input)(struct gpio_chip *chip, unsigned offset);
int (*direction_output)(struct gpio_chip *chip, unsigned offset,
int value);
(...)
};
2. Save the pointers
struct npcm7xx_gpio *npcm;
bgpio_init( ... register setup ...)
npcm->direction_input = npcm->gc.direction_input;
npcm->direction_output = npcm->gc.direction_output;
npcm->gc.direction_input = npcmgpio_direction_input;
npcm->gc.direction_output = npcmgpio_direction_output;
3. Modify the functions like that:
static int npcmgpio_direction_input(struct gpio_chip *chip, unsigned int offset)
{
struct npcm7xx_gpio *npcm = gpiochip_get_data(chip);
int ret;
ret = pinctrl_gpio_direction_input(offset + chip->base);
if (ret)
