| Message ID | 1413431475-12799-2-git-send-email-rebecca.swee.fun.chang@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Thu, Oct 16, 2014 at 11:51:13AM +0800, Chang Rebecca Swee Fun wrote: > Consolidating similar algorithms into common functions to make > GPIO SCH simpler and manageable. > > Signed-off-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 16, 2014 at 12:51 PM, Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com> wrote: > Consolidating similar algorithms into common functions to make > GPIO SCH simpler and manageable. > > Signed-off-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com> > --- > drivers/gpio/gpio-sch.c | 95 ++++++++++++++++++++++++++--------------------- > 1 file changed, 53 insertions(+), 42 deletions(-) > > diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c > index 99720c8..6e89be9 100644 > --- a/drivers/gpio/gpio-sch.c > +++ b/drivers/gpio/gpio-sch.c > @@ -63,94 +63,105 @@ static unsigned sch_gpio_bit(struct sch_gpio *sch, unsigned gpio) > return gpio % 8; > } > > -static void sch_gpio_enable(struct sch_gpio *sch, unsigned gpio) > +static void sch_gpio_register_set(struct sch_gpio *sch, unsigned gpio, > + unsigned reg) > { > unsigned short offset, bit; > u8 enable; > > spin_lock(&sch->lock); ... > > - offset = sch_gpio_offset(sch, gpio, GEN); > + offset = sch_gpio_offset(sch, gpio, reg); > bit = sch_gpio_bit(sch, gpio); > > enable = inb(sch->iobase + offset); I see identical blocks of code in sch_gpio_register_clear(), sch_gpio_reg_get() and sch_gpio_reg_set() (and possibly other functions?). Maybe this could be factorized into an inline function that would return the "enable" variable? Also, "enable" looks like the wrong name here. The exact same result is later called "disable" and "curr_dirs" later. > - if (!(enable & (1 << bit))) > - outb(enable | (1 << bit), sch->iobase + offset); > + if (!(enable & BIT(bit))) > + outb(enable | BIT(bit), sch->iobase + offset); > > spin_unlock(&sch->lock); > } > > -static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned gpio_num) > +static void sch_gpio_register_clear(struct sch_gpio *sch, unsigned gpio, > + unsigned reg) > { > - struct sch_gpio *sch = to_sch_gpio(gc); > - u8 curr_dirs; > unsigned short offset, bit; > + u8 disable; > > spin_lock(&sch->lock); > > - offset = sch_gpio_offset(sch, gpio_num, GIO); > - bit = sch_gpio_bit(sch, gpio_num); > - > - curr_dirs = inb(sch->iobase + offset); > + offset = sch_gpio_offset(sch, gpio, reg); > + bit = sch_gpio_bit(sch, gpio); > > - if (!(curr_dirs & (1 << bit))) > - outb(curr_dirs | (1 << bit), sch->iobase + offset); > + disable = inb(sch->iobase + offset); > + if (disable & BIT(bit)) > + outb(disable & ~BIT(bit), sch->iobase + offset); > > spin_unlock(&sch->lock); > - return 0; > } > > -static int sch_gpio_get(struct gpio_chip *gc, unsigned gpio_num) > +static int sch_gpio_reg_get(struct gpio_chip *gc, unsigned gpio, unsigned reg) > { > struct sch_gpio *sch = to_sch_gpio(gc); > - int res; > unsigned short offset, bit; > + u8 curr_dirs; > > - offset = sch_gpio_offset(sch, gpio_num, GLV); > - bit = sch_gpio_bit(sch, gpio_num); > + offset = sch_gpio_offset(sch, gpio, reg); > + bit = sch_gpio_bit(sch, gpio); > > - res = !!(inb(sch->iobase + offset) & (1 << bit)); > + curr_dirs = !!(inb(sch->iobase + offset) & BIT(bit)); > > - return res; > + return curr_dirs; > } > > -static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int val) > +static void sch_gpio_reg_set(struct gpio_chip *gc, unsigned gpio, unsigned reg, > + int val) > { > struct sch_gpio *sch = to_sch_gpio(gc); > - u8 curr_vals; > unsigned short offset, bit; > + u8 curr_dirs; > > - spin_lock(&sch->lock); > - > - offset = sch_gpio_offset(sch, gpio_num, GLV); > - bit = sch_gpio_bit(sch, gpio_num); > + offset = sch_gpio_offset(sch, gpio, reg); > + bit = sch_gpio_bit(sch, gpio); > > - curr_vals = inb(sch->iobase + offset); > + curr_dirs = inb(sch->iobase + offset); > > if (val) > - outb(curr_vals | (1 << bit), sch->iobase + offset); > + outb(curr_dirs | BIT(bit), sch->iobase + offset); > else > - outb((curr_vals & ~(1 << bit)), sch->iobase + offset); > + outb((curr_dirs & ~BIT(bit)), sch->iobase + offset); > +} Mmmm this really looks like sch_gpio_register_set() and sch_gpio_register_clear() could just call sch_gpio_reg_set() right after acquiring the spinlock. Also the names are very similar and it is not clear why you need two different functions here. Couldn't you call sch_gpio_reg_set(gc, gpio, reg, 1) in place of sch_gpio_register_set() and sch_gpio_reg_set(gc, gpio, reg, 0) for sch_gpio_register_clear()? You may need to acquire the spinlock before some call sites, but that's preferable to having very similar functions bearing a very similar name IMHO. > > +static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned gpio_num) > +{ > + struct sch_gpio *sch = to_sch_gpio(gc); > + > + spin_lock(&sch->lock); > + sch_gpio_register_set(sch, gpio_num, GIO); So here you are acquiring sch->lock, then calling sch_gpio_register_set() which will try to acquire the same spinlock first thing. Wouldn't that deadlock? Have you tested changing the direction of a GPIO? This again speaks in favor or reducing the number of similar functions in this file. Unless I misunderstood something there are still some issues that make this file harder to understand than it should, and which can sometimes bork the system altogether. It is a good idea to cleanup this file, but please try to go one step further - this should simplify locking and ultimately get rid of the locking issues. And hopefully I will also take less time to review v4. :P -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks for the review comments. Please check my reply below. > -----Original Message----- > From: Alexandre Courbot [mailto:gnurou@gmail.com] > Sent: 17 October, 2014 4:44 PM > To: Chang, Rebecca Swee Fun > Cc: Linus Walleij; Westerberg, Mika; GPIO Subsystem Mailing List; Linux Kernel > Mailing List; Denis Turischev > Subject: Re: [PATCHv3 1/3] gpio: sch: Consolidate similar algorithms > > On Thu, Oct 16, 2014 at 12:51 PM, Chang Rebecca Swee Fun > <rebecca.swee.fun.chang@intel.com> wrote: > > Consolidating similar algorithms into common functions to make GPIO > > SCH simpler and manageable. > > > > Signed-off-by: Chang Rebecca Swee Fun > > <rebecca.swee.fun.chang@intel.com> > > --- > > drivers/gpio/gpio-sch.c | 95 ++++++++++++++++++++++++++-------------------- > - > > 1 file changed, 53 insertions(+), 42 deletions(-) > > > > diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c index > > 99720c8..6e89be9 100644 > > --- a/drivers/gpio/gpio-sch.c > > +++ b/drivers/gpio/gpio-sch.c > > @@ -63,94 +63,105 @@ static unsigned sch_gpio_bit(struct sch_gpio *sch, > unsigned gpio) > > return gpio % 8; > > } > > > > -static void sch_gpio_enable(struct sch_gpio *sch, unsigned gpio) > > +static void sch_gpio_register_set(struct sch_gpio *sch, unsigned gpio, > > + unsigned reg) > > { > > unsigned short offset, bit; > > u8 enable; > > > > spin_lock(&sch->lock); > > ... > > > > > - offset = sch_gpio_offset(sch, gpio, GEN); > > + offset = sch_gpio_offset(sch, gpio, reg); > > bit = sch_gpio_bit(sch, gpio); > > > > enable = inb(sch->iobase + offset); > > I see identical blocks of code in sch_gpio_register_clear(), > sch_gpio_reg_get() and sch_gpio_reg_set() (and possibly other functions?). > Maybe this could be factorized into an inline function that would return the > "enable" variable? > > Also, "enable" looks like the wrong name here. The exact same result is later > called "disable" and "curr_dirs" later. Referring to your comments below, this will be get rid of after the sch_gpio_register_set() and sch_gpio_register_clear() being replaced by sch_gpio_reg_set(gc, gpio, reg, 1) and sch_gpio_reg_set(gc, gpio, reg, 0). There will be less identical block of "offset", "bit", and "enable" nor "disable". So there is no need to factorize the identical blocks into inline function. > > > - if (!(enable & (1 << bit))) > > - outb(enable | (1 << bit), sch->iobase + offset); > > + if (!(enable & BIT(bit))) > > + outb(enable | BIT(bit), sch->iobase + offset); > > > > spin_unlock(&sch->lock); > > } > > > > -static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned > > gpio_num) > > +static void sch_gpio_register_clear(struct sch_gpio *sch, unsigned gpio, > > + unsigned reg) > > { > > - struct sch_gpio *sch = to_sch_gpio(gc); > > - u8 curr_dirs; > > unsigned short offset, bit; > > + u8 disable; > > > > spin_lock(&sch->lock); > > > > - offset = sch_gpio_offset(sch, gpio_num, GIO); > > - bit = sch_gpio_bit(sch, gpio_num); > > - > > - curr_dirs = inb(sch->iobase + offset); > > + offset = sch_gpio_offset(sch, gpio, reg); > > + bit = sch_gpio_bit(sch, gpio); > > > > - if (!(curr_dirs & (1 << bit))) > > - outb(curr_dirs | (1 << bit), sch->iobase + offset); > > + disable = inb(sch->iobase + offset); > > + if (disable & BIT(bit)) > > + outb(disable & ~BIT(bit), sch->iobase + offset); > > > > spin_unlock(&sch->lock); > > - return 0; > > } > > > > -static int sch_gpio_get(struct gpio_chip *gc, unsigned gpio_num) > > +static int sch_gpio_reg_get(struct gpio_chip *gc, unsigned gpio, > > +unsigned reg) > > { > > struct sch_gpio *sch = to_sch_gpio(gc); > > - int res; > > unsigned short offset, bit; > > + u8 curr_dirs; > > > > - offset = sch_gpio_offset(sch, gpio_num, GLV); > > - bit = sch_gpio_bit(sch, gpio_num); > > + offset = sch_gpio_offset(sch, gpio, reg); > > + bit = sch_gpio_bit(sch, gpio); > > > > - res = !!(inb(sch->iobase + offset) & (1 << bit)); > > + curr_dirs = !!(inb(sch->iobase + offset) & BIT(bit)); > > > > - return res; > > + return curr_dirs; > > } > > > > -static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int > > val) > > +static void sch_gpio_reg_set(struct gpio_chip *gc, unsigned gpio, unsigned > reg, > > + int val) > > { > > struct sch_gpio *sch = to_sch_gpio(gc); > > - u8 curr_vals; > > unsigned short offset, bit; > > + u8 curr_dirs; > > > > - spin_lock(&sch->lock); > > - > > - offset = sch_gpio_offset(sch, gpio_num, GLV); > > - bit = sch_gpio_bit(sch, gpio_num); > > + offset = sch_gpio_offset(sch, gpio, reg); > > + bit = sch_gpio_bit(sch, gpio); > > > > - curr_vals = inb(sch->iobase + offset); > > + curr_dirs = inb(sch->iobase + offset); > > > > if (val) > > - outb(curr_vals | (1 << bit), sch->iobase + offset); > > + outb(curr_dirs | BIT(bit), sch->iobase + offset); > > else > > - outb((curr_vals & ~(1 << bit)), sch->iobase + offset); > > + outb((curr_dirs & ~BIT(bit)), sch->iobase + offset); } > > Mmmm this really looks like sch_gpio_register_set() and > sch_gpio_register_clear() could just call sch_gpio_reg_set() right after > acquiring the spinlock. Also the names are very similar and it is not clear why > you need two different functions here. Couldn't you call sch_gpio_reg_set(gc, > gpio, reg, 1) in place of > sch_gpio_register_set() and sch_gpio_reg_set(gc, gpio, reg, 0) for > sch_gpio_register_clear()? You may need to acquire the spinlock before some > call sites, but that's preferable to having very similar functions bearing a very > similar name IMHO. Thanks for pointing that out. I think I can replaced all sch_gpio_register_set() and sch_gpio_register_clear() with sch_gpio_reg_set(gc, gpio, reg, 0/1). Regarding the spinlock, in fact, I have tested using the GPIO driver through sysfs. I didn't encounter any deadlock issue, but the double locking is really an issue. This double lock problem can also be fix by calling sch_gpio_reg_set(gc, gpio, reg, 0/1) in place of sch_gpio_register_set() and sch_gpio_register_clear(). > > > > > +static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned > > +gpio_num) { > > + struct sch_gpio *sch = to_sch_gpio(gc); > > + > > + spin_lock(&sch->lock); > > + sch_gpio_register_set(sch, gpio_num, GIO); > > So here you are acquiring sch->lock, then calling > sch_gpio_register_set() which will try to acquire the same spinlock first thing. > Wouldn't that deadlock? Have you tested changing the direction of a GPIO? This > again speaks in favor or reducing the number of similar functions in this file. > > Unless I misunderstood something there are still some issues that make this file > harder to understand than it should, and which can sometimes bork the system > altogether. It is a good idea to cleanup this file, but please try to go one step > further - this should simplify locking and ultimately get rid of the locking issues. > > And hopefully I will also take less time to review v4. :P Thanks for the review. I will take note on the locking part and resend later or next week. Appreciate your review comments. Thank you! Rebecca
diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c index 99720c8..6e89be9 100644 --- a/drivers/gpio/gpio-sch.c +++ b/drivers/gpio/gpio-sch.c @@ -63,94 +63,105 @@ static unsigned sch_gpio_bit(struct sch_gpio *sch, unsigned gpio) return gpio % 8; } -static void sch_gpio_enable(struct sch_gpio *sch, unsigned gpio) +static void sch_gpio_register_set(struct sch_gpio *sch, unsigned gpio, + unsigned reg) { unsigned short offset, bit; u8 enable; spin_lock(&sch->lock); - offset = sch_gpio_offset(sch, gpio, GEN); + offset = sch_gpio_offset(sch, gpio, reg); bit = sch_gpio_bit(sch, gpio); enable = inb(sch->iobase + offset); - if (!(enable & (1 << bit))) - outb(enable | (1 << bit), sch->iobase + offset); + if (!(enable & BIT(bit))) + outb(enable | BIT(bit), sch->iobase + offset); spin_unlock(&sch->lock); } -static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned gpio_num) +static void sch_gpio_register_clear(struct sch_gpio *sch, unsigned gpio, + unsigned reg) { - struct sch_gpio *sch = to_sch_gpio(gc); - u8 curr_dirs; unsigned short offset, bit; + u8 disable; spin_lock(&sch->lock); - offset = sch_gpio_offset(sch, gpio_num, GIO); - bit = sch_gpio_bit(sch, gpio_num); - - curr_dirs = inb(sch->iobase + offset); + offset = sch_gpio_offset(sch, gpio, reg); + bit = sch_gpio_bit(sch, gpio); - if (!(curr_dirs & (1 << bit))) - outb(curr_dirs | (1 << bit), sch->iobase + offset); + disable = inb(sch->iobase + offset); + if (disable & BIT(bit)) + outb(disable & ~BIT(bit), sch->iobase + offset); spin_unlock(&sch->lock); - return 0; } -static int sch_gpio_get(struct gpio_chip *gc, unsigned gpio_num) +static int sch_gpio_reg_get(struct gpio_chip *gc, unsigned gpio, unsigned reg) { struct sch_gpio *sch = to_sch_gpio(gc); - int res; unsigned short offset, bit; + u8 curr_dirs; - offset = sch_gpio_offset(sch, gpio_num, GLV); - bit = sch_gpio_bit(sch, gpio_num); + offset = sch_gpio_offset(sch, gpio, reg); + bit = sch_gpio_bit(sch, gpio); - res = !!(inb(sch->iobase + offset) & (1 << bit)); + curr_dirs = !!(inb(sch->iobase + offset) & BIT(bit)); - return res; + return curr_dirs; } -static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int val) +static void sch_gpio_reg_set(struct gpio_chip *gc, unsigned gpio, unsigned reg, + int val) { struct sch_gpio *sch = to_sch_gpio(gc); - u8 curr_vals; unsigned short offset, bit; + u8 curr_dirs; - spin_lock(&sch->lock); - - offset = sch_gpio_offset(sch, gpio_num, GLV); - bit = sch_gpio_bit(sch, gpio_num); + offset = sch_gpio_offset(sch, gpio, reg); + bit = sch_gpio_bit(sch, gpio); - curr_vals = inb(sch->iobase + offset); + curr_dirs = inb(sch->iobase + offset); if (val) - outb(curr_vals | (1 << bit), sch->iobase + offset); + outb(curr_dirs | BIT(bit), sch->iobase + offset); else - outb((curr_vals & ~(1 << bit)), sch->iobase + offset); + outb((curr_dirs & ~BIT(bit)), sch->iobase + offset); +} +static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned gpio_num) +{ + struct sch_gpio *sch = to_sch_gpio(gc); + + spin_lock(&sch->lock); + sch_gpio_register_set(sch, gpio_num, GIO); spin_unlock(&sch->lock); + return 0; } -static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num, - int val) +static int sch_gpio_get(struct gpio_chip *gc, unsigned gpio_num) +{ + return sch_gpio_reg_get(gc, gpio_num, GLV); +} + +static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int val) { struct sch_gpio *sch = to_sch_gpio(gc); - u8 curr_dirs; - unsigned short offset, bit; spin_lock(&sch->lock); + sch_gpio_reg_set(gc, gpio_num, GLV, val); + spin_unlock(&sch->lock); +} - offset = sch_gpio_offset(sch, gpio_num, GIO); - bit = sch_gpio_bit(sch, gpio_num); - - curr_dirs = inb(sch->iobase + offset); - if (curr_dirs & (1 << bit)) - outb(curr_dirs & ~(1 << bit), sch->iobase + offset); +static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num, + int val) +{ + struct sch_gpio *sch = to_sch_gpio(gc); + spin_lock(&sch->lock); + sch_gpio_register_clear(sch, gpio_num, GIO); spin_unlock(&sch->lock); /* @@ -209,13 +220,13 @@ static int sch_gpio_probe(struct platform_device *pdev) * GPIO7 is configured by the CMC as SLPIOVR * Enable GPIO[9:8] core powered gpios explicitly */ - sch_gpio_enable(sch, 8); - sch_gpio_enable(sch, 9); + sch_gpio_register_set(sch, 8, GEN); + sch_gpio_register_set(sch, 9, GEN); /* * SUS_GPIO[2:0] enabled by default * Enable SUS_GPIO3 resume powered gpio explicitly */ - sch_gpio_enable(sch, 13); + sch_gpio_register_set(sch, 13, GEN); break; case PCI_DEVICE_ID_INTEL_ITC_LPC:
Consolidating similar algorithms into common functions to make GPIO SCH simpler and manageable. Signed-off-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com> --- drivers/gpio/gpio-sch.c | 95 ++++++++++++++++++++++++++--------------------- 1 file changed, 53 insertions(+), 42 deletions(-)