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

["'Mika Westerberg'" <mika.westerberg@linux.intel.com>]

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?