| Message ID | 1410943745-9354-2-git-send-email-rebecca.swee.fun.chang@intel.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Wed, Sep 17, 2014 at 04:49:03PM +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> > --- > 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..2189c22 100644 > --- a/drivers/gpio/gpio-sch.c > +++ b/drivers/gpio/gpio-sch.c > @@ -43,6 +43,8 @@ struct sch_gpio { > > #define to_sch_gpio(c) container_of(c, struct sch_gpio, chip) > > +static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int val); > + Is it possible to move the sch_gpio_set() function here instead of forward declaring it? > static unsigned sch_gpio_offset(struct sch_gpio *sch, unsigned gpio, > unsigned reg) > { > @@ -63,94 +65,89 @@ 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_enable(struct sch_gpio *sch, unsigned gpio, unsigned reg) I don't see much value in doing this. To me sch_gpio_enable(sch, gpio) is more intuitive than sch_gpio_enable(sch, gpio, GEN). Why do I need to pass register to enable function in the first place? It should know better how to enable the GPIO, right? Same goes for others. > { > 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_disable(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_enable(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) > { > struct sch_gpio *sch = to_sch_gpio(gc); > - u8 curr_dirs; > - unsigned short offset, bit; > > 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); > - if (curr_dirs & (1 << bit)) > - outb(curr_dirs & ~(1 << bit), sch->iobase + offset); > - > + sch_gpio_disable(sch, gpio_num, GIO); > spin_unlock(&sch->lock); > > /* > @@ -166,6 +163,20 @@ static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num, > return 0; > } > > +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); > + > + spin_lock(&sch->lock); > + sch_gpio_reg_set(gc, gpio_num, GLV, val); > + spin_unlock(&sch->lock); > +} > + > static struct gpio_chip sch_gpio_chip = { > .label = "sch_gpio", > .owner = THIS_MODULE, > @@ -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_enable(sch, 8, GEN); > + sch_gpio_enable(sch, 9, GEN); For example here, which one is more readable? > /* > * SUS_GPIO[2:0] enabled by default > * Enable SUS_GPIO3 resume powered gpio explicitly > */ > - sch_gpio_enable(sch, 13); > + sch_gpio_enable(sch, 13, GEN); Or here? > break; > > case PCI_DEVICE_ID_INTEL_ITC_LPC: > -- > 1.7.9.5 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- 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
Replied inline. :) > -----Original Message----- > From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com] > Sent: 18 September, 2014 7:17 PM > To: Chang, Rebecca Swee Fun > Cc: Linus Walleij; linux-gpio@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH 1/3] gpio: sch: Consolidate similar algorithms > > On Wed, Sep 17, 2014 at 04:49:03PM +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> > > --- > > 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..2189c22 100644 > > --- a/drivers/gpio/gpio-sch.c > > +++ b/drivers/gpio/gpio-sch.c > > @@ -43,6 +43,8 @@ struct sch_gpio { > > > > #define to_sch_gpio(c) container_of(c, struct sch_gpio, chip) > > > > +static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int > > +val); > > + > > Is it possible to move the sch_gpio_set() function here instead of forward > declaring it? Yes, it is possible. There is a reason I submitted the code in this structure. By putting the sch_gpio_set() function below will makes the diff patch looks neat and easy for review. If it doesn't make sense to make the patch for easy reviewing, I can change by moving the function above. > > > static unsigned sch_gpio_offset(struct sch_gpio *sch, unsigned gpio, > > unsigned reg) > > { > > @@ -63,94 +65,89 @@ 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_enable(struct sch_gpio *sch, unsigned gpio, > > +unsigned reg) > > I don't see much value in doing this. > > To me sch_gpio_enable(sch, gpio) is more intuitive than sch_gpio_enable(sch, > gpio, GEN). Why do I need to pass register to enable function in the first place? > It should know better how to enable the GPIO, right? > > Same goes for others. The register values are required when it comes to IRQ handling. By passing in the registers values, we can make full use of the algorithms without introducing extra/similar algorithms to compute other register offset values. For example, we have other offset values to handle such as:- GTPE 0x0C GTNE 0x10 GGPE 0x14 GSMI 0x18 GTS 0x1C CGNMIEN 0x40 RGNMIEN 0x44 Regards Rebecca > > > { > > 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_disable(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_enable(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) > > { > > struct sch_gpio *sch = to_sch_gpio(gc); > > - u8 curr_dirs; > > - unsigned short offset, bit; > > > > 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); > > - if (curr_dirs & (1 << bit)) > > - outb(curr_dirs & ~(1 << bit), sch->iobase + offset); > > - > > + sch_gpio_disable(sch, gpio_num, GIO); > > spin_unlock(&sch->lock); > > > > /* > > @@ -166,6 +163,20 @@ static int sch_gpio_direction_out(struct gpio_chip > *gc, unsigned gpio_num, > > return 0; > > } > > > > +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); > > + > > + spin_lock(&sch->lock); > > + sch_gpio_reg_set(gc, gpio_num, GLV, val); > > + spin_unlock(&sch->lock); > > +} > > + > > static struct gpio_chip sch_gpio_chip = { > > .label = "sch_gpio", > > .owner = THIS_MODULE, > > @@ -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_enable(sch, 8, GEN); > > + sch_gpio_enable(sch, 9, GEN); > > For example here, which one is more readable? > > > /* > > * SUS_GPIO[2:0] enabled by default > > * Enable SUS_GPIO3 resume powered gpio explicitly > > */ > > - sch_gpio_enable(sch, 13); > > + sch_gpio_enable(sch, 13, GEN); > > Or here? > > > break; > > > > case PCI_DEVICE_ID_INTEL_ITC_LPC: > > -- > > 1.7.9.5 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe > > linux-kernel" in the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > Please read the FAQ at http://www.tux.org/lkml/ -- 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 Mon, Sep 22, 2014 at 05:43:36AM +0000, Chang, Rebecca Swee Fun wrote: > Replied inline. :) > > > -----Original Message----- > > From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com] > > Sent: 18 September, 2014 7:17 PM > > To: Chang, Rebecca Swee Fun > > Cc: Linus Walleij; linux-gpio@vger.kernel.org; linux-kernel@vger.kernel.org > > Subject: Re: [PATCH 1/3] gpio: sch: Consolidate similar algorithms > > > > On Wed, Sep 17, 2014 at 04:49:03PM +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> > > > --- > > > 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..2189c22 100644 > > > --- a/drivers/gpio/gpio-sch.c > > > +++ b/drivers/gpio/gpio-sch.c > > > @@ -43,6 +43,8 @@ struct sch_gpio { > > > > > > #define to_sch_gpio(c) container_of(c, struct sch_gpio, chip) > > > > > > +static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int > > > +val); > > > + > > > > Is it possible to move the sch_gpio_set() function here instead of forward > > declaring it? > > Yes, it is possible. There is a reason I submitted the code in this > structure. By putting the sch_gpio_set() function below will makes the > diff patch looks neat and easy for review. If it doesn't make sense > to make the patch for easy reviewing, I can change by moving the > function above. I think that we are interested in the end result (e.g) the driver and if we can avoid forward declarations the better. > > > > > static unsigned sch_gpio_offset(struct sch_gpio *sch, unsigned gpio, > > > unsigned reg) > > > { > > > @@ -63,94 +65,89 @@ 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_enable(struct sch_gpio *sch, unsigned gpio, > > > +unsigned reg) > > > > I don't see much value in doing this. > > > > To me sch_gpio_enable(sch, gpio) is more intuitive than sch_gpio_enable(sch, > > gpio, GEN). Why do I need to pass register to enable function in the first place? > > It should know better how to enable the GPIO, right? > > > > Same goes for others. > > The register values are required when it comes to IRQ handling. By > passing in the registers values, we can make full use of the > algorithms without introducing extra/similar algorithms to compute > other register offset values. > For example, we have other offset values to handle such as:- > GTPE 0x0C > GTNE 0x10 > GGPE 0x14 > GSMI 0x18 > GTS 0x1C > CGNMIEN 0x40 > RGNMIEN 0x44 Well, can we at least call it something else than sch_gpio_enable()? Perhaps sch_gpio_set_value() or so? -- 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
> -----Original Message----- > From: 'Mika Westerberg' [mailto:mika.westerberg@linux.intel.com] > Sent: 22 September, 2014 5:25 PM > To: Chang, Rebecca Swee Fun > Cc: Linus Walleij; linux-gpio@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH 1/3] gpio: sch: Consolidate similar algorithms > > On Mon, Sep 22, 2014 at 05:43:36AM +0000, Chang, Rebecca Swee Fun wrote: > > Replied inline. :) > > > > > -----Original Message----- > > > From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com] > > > Sent: 18 September, 2014 7:17 PM > > > To: Chang, Rebecca Swee Fun > > > Cc: Linus Walleij; linux-gpio@vger.kernel.org; > > > linux-kernel@vger.kernel.org > > > Subject: Re: [PATCH 1/3] gpio: sch: Consolidate similar algorithms > > > > > > On Wed, Sep 17, 2014 at 04:49:03PM +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> > > > > --- > > > > 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..2189c22 100644 > > > > --- a/drivers/gpio/gpio-sch.c > > > > +++ b/drivers/gpio/gpio-sch.c > > > > @@ -43,6 +43,8 @@ struct sch_gpio { > > > > > > > > #define to_sch_gpio(c) container_of(c, struct sch_gpio, chip) > > > > > > > > +static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, > > > > +int val); > > > > + > > > > > > Is it possible to move the sch_gpio_set() function here instead of > > > forward declaring it? > > > > Yes, it is possible. There is a reason I submitted the code in this > > structure. By putting the sch_gpio_set() function below will makes the > > diff patch looks neat and easy for review. If it doesn't make sense > > to make the patch for easy reviewing, I can change by moving the > > function above. > > I think that we are interested in the end result (e.g) the driver and if we can > avoid forward declarations the better. Alright. I can move it up to avoid the forward declaration. > > > > > > > > static unsigned sch_gpio_offset(struct sch_gpio *sch, unsigned gpio, > > > > unsigned reg) > > > > { > > > > @@ -63,94 +65,89 @@ 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_enable(struct sch_gpio *sch, unsigned gpio, > > > > +unsigned reg) > > > > > > I don't see much value in doing this. > > > > > > To me sch_gpio_enable(sch, gpio) is more intuitive than > > > sch_gpio_enable(sch, gpio, GEN). Why do I need to pass register to enable > function in the first place? > > > It should know better how to enable the GPIO, right? > > > > > > Same goes for others. > > > > The register values are required when it comes to IRQ handling. By > > passing in the registers values, we can make full use of the > > algorithms without introducing extra/similar algorithms to compute > > other register offset values. > > For example, we have other offset values to handle such as:- > > GTPE 0x0C > > GTNE 0x10 > > GGPE 0x14 > > GSMI 0x18 > > GTS 0x1C > > CGNMIEN 0x40 > > RGNMIEN 0x44 > > Well, can we at least call it something else than sch_gpio_enable()? > Perhaps sch_gpio_set_value() or so? sch_gpio_set_value() sounds good. After think twice, I intend to merge sch_gpio_enable() and sch_gpio_disable() into one functions. Using variable "enable" as an indicator, I can control whether to enable or disable when calling the function. Here is my draft: static void sch_gpio_set_value(struct sch_gpio *sch, unsigned gpio, unsigned reg, unsigned int enable) { unsigned long flags; unsigned short offset, bit; u8 set; spin_lock_irqsave(&sch->lock, flags); offset = sch_gpio_offset(sch, gpio, reg); bit = sch_gpio_bit(sch, gpio); set = inb(sch->iobase + offset); if (enable) outb(set | BIT(bit), sch->iobase + offset); else outb(disable & ~BIT(bit), sch->iobase + offset); spin_unlock_irqrestore(&sch->lock, flags); } Do you think this make sense? I am in the progress of implementing and testing. Rebecca -- 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 Wed, Sep 24, 2014 at 12:55:07AM +0000, Chang, Rebecca Swee Fun wrote: > > > The register values are required when it comes to IRQ handling. By > > > passing in the registers values, we can make full use of the > > > algorithms without introducing extra/similar algorithms to compute > > > other register offset values. > > > For example, we have other offset values to handle such as:- > > > GTPE 0x0C > > > GTNE 0x10 > > > GGPE 0x14 > > > GSMI 0x18 > > > GTS 0x1C > > > CGNMIEN 0x40 > > > RGNMIEN 0x44 > > > > Well, can we at least call it something else than sch_gpio_enable()? > > Perhaps sch_gpio_set_value() or so? > > sch_gpio_set_value() sounds good. After think twice, I intend to merge > sch_gpio_enable() and sch_gpio_disable() into one functions. Using > variable "enable" as an indicator, I can control whether to enable or > disable when calling the function. Here is my draft: Actually sch_gpio_set_value() is too close to sch_gpio_set() which sets the GPIO to 1 or 0. How about sch_gpio_register_set() or something along those lines? And I don't think it is good idea to add yet another functionality, like enable there. Please leave sch_gpio_enable()/sch_gpio_disable() as is. -- 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
> -----Original Message----- > From: 'Mika Westerberg' [mailto:mika.westerberg@linux.intel.com] > Sent: 24 September, 2014 5:51 PM > To: Chang, Rebecca Swee Fun > Cc: Linus Walleij; linux-gpio@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH 1/3] gpio: sch: Consolidate similar algorithms > > On Wed, Sep 24, 2014 at 12:55:07AM +0000, Chang, Rebecca Swee Fun wrote: > > > > The register values are required when it comes to IRQ handling. By > > > > passing in the registers values, we can make full use of the > > > > algorithms without introducing extra/similar algorithms to compute > > > > other register offset values. > > > > For example, we have other offset values to handle such as:- > > > > GTPE 0x0C > > > > GTNE 0x10 > > > > GGPE 0x14 > > > > GSMI 0x18 > > > > GTS 0x1C > > > > CGNMIEN 0x40 > > > > RGNMIEN 0x44 > > > > > > Well, can we at least call it something else than sch_gpio_enable()? > > > Perhaps sch_gpio_set_value() or so? > > > > sch_gpio_set_value() sounds good. After think twice, I intend to merge > > sch_gpio_enable() and sch_gpio_disable() into one functions. Using > > variable "enable" as an indicator, I can control whether to enable or > > disable when calling the function. Here is my draft: > > Actually sch_gpio_set_value() is too close to sch_gpio_set() which sets the GPIO > to 1 or 0. How about sch_gpio_register_set() or something along those lines? > > And I don't think it is good idea to add yet another functionality, like enable > there. Please leave sch_gpio_enable()/sch_gpio_disable() as is. Alright, I will change the sch_gpio_enable()/sch_gpio_disable() into sch_gpio_register_set()/sch_gpio_register_clear(). Thanks. Rebecca -- 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
diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c index 99720c8..2189c22 100644 --- a/drivers/gpio/gpio-sch.c +++ b/drivers/gpio/gpio-sch.c @@ -43,6 +43,8 @@ struct sch_gpio { #define to_sch_gpio(c) container_of(c, struct sch_gpio, chip) +static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int val); + static unsigned sch_gpio_offset(struct sch_gpio *sch, unsigned gpio, unsigned reg) { @@ -63,94 +65,89 @@ 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_enable(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_disable(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_enable(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) { struct sch_gpio *sch = to_sch_gpio(gc); - u8 curr_dirs; - unsigned short offset, bit; 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); - if (curr_dirs & (1 << bit)) - outb(curr_dirs & ~(1 << bit), sch->iobase + offset); - + sch_gpio_disable(sch, gpio_num, GIO); spin_unlock(&sch->lock); /* @@ -166,6 +163,20 @@ static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num, return 0; } +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); + + spin_lock(&sch->lock); + sch_gpio_reg_set(gc, gpio_num, GLV, val); + spin_unlock(&sch->lock); +} + static struct gpio_chip sch_gpio_chip = { .label = "sch_gpio", .owner = THIS_MODULE, @@ -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_enable(sch, 8, GEN); + sch_gpio_enable(sch, 9, GEN); /* * SUS_GPIO[2:0] enabled by default * Enable SUS_GPIO3 resume powered gpio explicitly */ - sch_gpio_enable(sch, 13); + sch_gpio_enable(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(-)