RE: [PATCH 1/3] gpio: sch: Consolidate similar algorithms

[Chang, Rebecca Swee Fun]

> -----Original Message-----
> From: 'Mika Westerberg' [mailto:mika.westerberg@xxxxxxxxxxxxxxx]
> Sent: 22 September, 2014 5:25 PM
> To: Chang, Rebecca Swee Fun
> Cc: Linus Walleij; linux-gpio@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx]
> > > Sent: 18 September, 2014 7:17 PM
> > > To: Chang, Rebecca Swee Fun
> > > Cc: Linus Walleij; linux-gpio@xxxxxxxxxxxxxxx;
> > > linux-kernel@xxxxxxxxxxxxxxx
> > > 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@xxxxxxxxx>
> > > > ---
> > > >  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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/