On Mon, Sep 22, 2014 at 05:43:36AM +0000, Chang, Rebecca Swee Fun wrote:
> Replied inline. :)
>
> > -----Original Message-----
> > From: Mika Westerberg [mailto:[email protected]]
> > Sent: 18 September, 2014 7:17 PM
> > To: Chang, Rebecca Swee Fun
> > Cc: Linus Walleij; [email protected]; [email protected]
> > 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
> > > <[email protected]>
> > > ---
> > > 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-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
