[PATCH 2/12] pxa: remove the pin configuration from the keypad driver

[PATCH 2/12] pxa: remove the pin configuration from the keypad driver

[eric miao]

>From dbd62bced0f789765d1823f66af792933c6b46a1 Mon Sep 17 00:00:00 2001
From: eric miao <eric.miao@marvell.com>
Date: Tue, 22 Jan 2008 16:34:12 +0800
Subject: [PATCH] pxa: remove the pin configuration from the keypad driver

The pin configurations will slowly be moved to the board specific code
at initialization thus to make the driver more generic.

Signed-off-by: eric miao <eric.miao@marvell.com>
---
 drivers/input/keyboard/pxa27x_keypad.c   |    4 ----
 include/asm-arm/arch-pxa/pxa27x_keypad.h |    1 -
 2 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/drivers/input/keyboard/pxa27x_keypad.c
b/drivers/input/keyboard/pxa27x_keypad.c
index 06c1d5a..43fb63d 100644
--- a/drivers/input/keyboard/pxa27x_keypad.c
+++ b/drivers/input/keyboard/pxa27x_keypad.c
@@ -208,10 +208,6 @@ static int __devinit pxa27x_keypad_probe(struct
platform_device *pdev)
 	if (error)
 		goto err_free_irq;

-	/* Setup GPIOs. */
-	for (i = 0; i < pdata->nr_rows + pdata->nr_cols; i++)
-		pxa_gpio_mode(pdata->gpio_modes[i]);
-
 	/*
 	 * Store rows/cols info into keyboard registers.
 	 */
diff --git a/include/asm-arm/arch-pxa/pxa27x_keypad.h
b/include/asm-arm/arch-pxa/pxa27x_keypad.h
index f19f74a..ef17db6 100644
--- a/include/asm-arm/arch-pxa/pxa27x_keypad.h
+++ b/include/asm-arm/arch-pxa/pxa27x_keypad.h
@@ -4,7 +4,6 @@
 struct pxa27x_keypad_platform_data {
 	int nr_rows, nr_cols;
 	int keycodes[PXAKBD_MAXROW][PXAKBD_MAXCOL];
-	int gpio_modes[PXAKBD_MAXROW + PXAKBD_MAXCOL];

 #ifdef CONFIG_PM
 	u32 reg_kpc;
-- 
1.5.2.5.GIT



-- 
Cheers
- eric

Re: [PATCH 2/12] pxa: remove the pin configuration from the keypad driver

[Dmitry Torokhov]

Hi Eric,

On Wednesday 23 January 2008 02:17, eric miao wrote:
> From dbd62bced0f789765d1823f66af792933c6b46a1 Mon Sep 17 00:00:00 2001
> From: eric miao <eric.miao@marvell.com>
> Date: Tue, 22 Jan 2008 16:34:12 +0800
> Subject: [PATCH] pxa: remove the pin configuration from the keypad driver
> 
> The pin configurations will slowly be moved to the board specific code
> at initialization thus to make the driver more generic.
> 
> Signed-off-by: eric miao <eric.miao@marvell.com>
> ---
>  drivers/input/keyboard/pxa27x_keypad.c   |    4 ----
>  include/asm-arm/arch-pxa/pxa27x_keypad.h |    1 -
>  2 files changed, 0 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/input/keyboard/pxa27x_keypad.c
> b/drivers/input/keyboard/pxa27x_keypad.c
> index 06c1d5a..43fb63d 100644
> --- a/drivers/input/keyboard/pxa27x_keypad.c
> +++ b/drivers/input/keyboard/pxa27x_keypad.c
> @@ -208,10 +208,6 @@ static int __devinit pxa27x_keypad_probe(struct
> platform_device *pdev)
>  	if (error)
>  		goto err_free_irq;
> 
> -	/* Setup GPIOs. */
> -	for (i = 0; i < pdata->nr_rows + pdata->nr_cols; i++)
> -		pxa_gpio_mode(pdata->gpio_modes[i]);
> -

That would require GPIO code to be replicated in every subarch piece. Do you
expect many boards require special GPIO setup or maybe it would make sense
to keep something similar to the code above (possibly have pointer to array
of gpio modes and array size in pdata)? This way simpler boards will just
supply the list and more complex setups can still do it themselves?

-- 
Dmitry

Re: [PATCH 2/12] pxa: remove the pin configuration from the keypad driver

[eric miao]

On Jan 29, 2008 2:24 PM, Dmitry Torokhov <dtor@insightbb.com> wrote:
> Hi Eric,
>
> On Wednesday 23 January 2008 02:17, eric miao wrote:
> > From dbd62bced0f789765d1823f66af792933c6b46a1 Mon Sep 17 00:00:00 2001
> > From: eric miao <eric.miao@marvell.com>
> > Date: Tue, 22 Jan 2008 16:34:12 +0800
> > Subject: [PATCH] pxa: remove the pin configuration from the keypad driver
> >
> > The pin configurations will slowly be moved to the board specific code
> > at initialization thus to make the driver more generic.
> >
> > Signed-off-by: eric miao <eric.miao@marvell.com>
> > ---
> >  drivers/input/keyboard/pxa27x_keypad.c   |    4 ----
> >  include/asm-arm/arch-pxa/pxa27x_keypad.h |    1 -
> >  2 files changed, 0 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/input/keyboard/pxa27x_keypad.c
> > b/drivers/input/keyboard/pxa27x_keypad.c
> > index 06c1d5a..43fb63d 100644
> > --- a/drivers/input/keyboard/pxa27x_keypad.c
> > +++ b/drivers/input/keyboard/pxa27x_keypad.c
> > @@ -208,10 +208,6 @@ static int __devinit pxa27x_keypad_probe(struct
> > platform_device *pdev)
> >       if (error)
> >               goto err_free_irq;
> >
> > -     /* Setup GPIOs. */
> > -     for (i = 0; i < pdata->nr_rows + pdata->nr_cols; i++)
> > -             pxa_gpio_mode(pdata->gpio_modes[i]);
> > -
>
> That would require GPIO code to be replicated in every subarch piece. Do you
> expect many boards require special GPIO setup or maybe it would make sense
> to keep something similar to the code above (possibly have pointer to array
> of gpio modes and array size in pdata)? This way simpler boards will just
> supply the list and more complex setups can still do it themselves?
>

Actually, pin configurations on different platforms vary much, and to keep
the driver generic enough, this stuff should really be pushed to the platform
code.

Currently, we (Nicolas, Russell King and I) are proposing a similar
pin configuration scheme as pxa3xx is currently doing for pxa{25x,27x}.

The specific reasons behind this change are:

1. pxa3xx uses a different mechanism and API to configure pins other
than the pxa{25x,27x}'s pxa_gpio_mode(), i.e., pxa_gpio_mode() is
no longer valid for pxa3xx. So this is really processor specific code
we need to keep out side of the driver

2. The driver should have no assumption to the platform configuration,
that is, when the ->probe() is called, any settings should be ready for
the driver to work, thus including pin configurations.

> --
> Dmitry
>



-- 
Cheers
- eric

Re: [PATCH 2/12] pxa: remove the pin configuration from the keypad driver

[Dmitry Torokhov]

On Tuesday 29 January 2008 01:51, eric miao wrote:
> On Jan 29, 2008 2:24 PM, Dmitry Torokhov <dtor@insightbb.com> wrote:
> > Hi Eric,
> >
> > On Wednesday 23 January 2008 02:17, eric miao wrote:
> > > From dbd62bced0f789765d1823f66af792933c6b46a1 Mon Sep 17 00:00:00 2001
> > > From: eric miao <eric.miao@marvell.com>
> > > Date: Tue, 22 Jan 2008 16:34:12 +0800
> > > Subject: [PATCH] pxa: remove the pin configuration from the keypad driver
> > >
> > > The pin configurations will slowly be moved to the board specific code
> > > at initialization thus to make the driver more generic.
> > >
> > > Signed-off-by: eric miao <eric.miao@marvell.com>
> > > ---
> > >  drivers/input/keyboard/pxa27x_keypad.c   |    4 ----
> > >  include/asm-arm/arch-pxa/pxa27x_keypad.h |    1 -
> > >  2 files changed, 0 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/input/keyboard/pxa27x_keypad.c
> > > b/drivers/input/keyboard/pxa27x_keypad.c
> > > index 06c1d5a..43fb63d 100644
> > > --- a/drivers/input/keyboard/pxa27x_keypad.c
> > > +++ b/drivers/input/keyboard/pxa27x_keypad.c
> > > @@ -208,10 +208,6 @@ static int __devinit pxa27x_keypad_probe(struct
> > > platform_device *pdev)
> > >       if (error)
> > >               goto err_free_irq;
> > >
> > > -     /* Setup GPIOs. */
> > > -     for (i = 0; i < pdata->nr_rows + pdata->nr_cols; i++)
> > > -             pxa_gpio_mode(pdata->gpio_modes[i]);
> > > -
> >
> > That would require GPIO code to be replicated in every subarch piece. Do you
> > expect many boards require special GPIO setup or maybe it would make sense
> > to keep something similar to the code above (possibly have pointer to array
> > of gpio modes and array size in pdata)? This way simpler boards will just
> > supply the list and more complex setups can still do it themselves?
> >
> 
> Actually, pin configurations on different platforms vary much, and to keep
> the driver generic enough, this stuff should really be pushed to the platform
> code.
> 
> Currently, we (Nicolas, Russell King and I) are proposing a similar
> pin configuration scheme as pxa3xx is currently doing for pxa{25x,27x}.
> 
> The specific reasons behind this change are:
> 
> 1. pxa3xx uses a different mechanism and API to configure pins other
> than the pxa{25x,27x}'s pxa_gpio_mode(), i.e., pxa_gpio_mode() is
> no longer valid for pxa3xx. So this is really processor specific code
> we need to keep out side of the driver

Ok, fair enough.

-- 
Dmitry

Re: [PATCH 2/12] pxa: remove the pin configuration from the keypad driver

[Dmitry Baryshkov]

eric miao wrote:

> On Jan 29, 2008 2:24 PM, Dmitry Torokhov <dtor@insightbb.com> wrote:
>> Hi Eric,
>>
>> On Wednesday 23 January 2008 02:17, eric miao wrote:
>> > From dbd62bced0f789765d1823f66af792933c6b46a1 Mon Sep 17 00:00:00
>> > 2001 From: eric miao <eric.miao@marvell.com> Date: Tue, 22 Jan 2008
>> > 16:34:12 +0800 Subject: [PATCH] pxa: remove the pin configuration
>> > from the keypad driver
>> >
>> > The pin configurations will slowly be moved to the board specific
>> > code at initialization thus to make the driver more generic.
>> >
>> > Signed-off-by: eric miao <eric.miao@marvell.com> ---
>> >  drivers/input/keyboard/pxa27x_keypad.c   |    4 ----
>> >  include/asm-arm/arch-pxa/pxa27x_keypad.h |    1 - 2 files changed, 0
>> >  insertions(+), 5 deletions(-)
>> >
>> > diff --git a/drivers/input/keyboard/pxa27x_keypad.c
>> > b/drivers/input/keyboard/pxa27x_keypad.c index 06c1d5a..43fb63d
>> > 100644
>> > --- a/drivers/input/keyboard/pxa27x_keypad.c +++
>> > b/drivers/input/keyboard/pxa27x_keypad.c @@ -208,10 +208,6 @@ static
>> > int __devinit pxa27x_keypad_probe(struct platform_device *pdev)
>> >       if (error)
>> >               goto err_free_irq;
>> >
>> > -     /* Setup GPIOs. */
>> > -     for (i = 0; i < pdata->nr_rows + pdata->nr_cols; i++) -        
>> >     pxa_gpio_mode(pdata->gpio_modes[i]); -
>>
>> That would require GPIO code to be replicated in every subarch piece.
>> Do you expect many boards require special GPIO setup or maybe it would
>> make sense to keep something similar to the code above (possibly have
>> pointer to array of gpio modes and array size in pdata)? This way
>> simpler boards will just supply the list and more complex setups can
>> still do it themselves?
>>
>>
> Actually, pin configurations on different platforms vary much, and to
> keep the driver generic enough, this stuff should really be pushed to
> the platform code.
> 
> Currently, we (Nicolas, Russell King and I) are proposing a similar pin
> configuration scheme as pxa3xx is currently doing for pxa{25x,27x}.
> 
> The specific reasons behind this change are:
> 
> 1. pxa3xx uses a different mechanism and API to configure pins other
> than the pxa{25x,27x}'s pxa_gpio_mode(), i.e., pxa_gpio_mode() is no
> longer valid for pxa3xx. So this is really processor specific code we
> need to keep out side of the driver

Why not use gpio_request/gpio_direction_input? Or they are broken for
pxa3xx?

> 
> 2. The driver should have no assumption to the platform configuration,
> that is, when the ->probe() is called, any settings should be ready for
> the driver to work, thus including pin configurations.

Hmmm. gpio-keys behaves just in the opposite way: it sets all gpios it wants
to use. And IMHO it's the correct way: the code isn't spread between several
files, but instead is kept inside one (generic) file.

-- 
With best wishes
Dmitry

Re: [PATCH 2/12] pxa: remove the pin configuration from the keypad driver

[eric miao]

On Jan 29, 2008 7:54 PM, Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:
>
> eric miao wrote:
>
> > On Jan 29, 2008 2:24 PM, Dmitry Torokhov <dtor@insightbb.com> wrote:
> >> Hi Eric,
> >>
> >> On Wednesday 23 January 2008 02:17, eric miao wrote:
> >> > From dbd62bced0f789765d1823f66af792933c6b46a1 Mon Sep 17 00:00:00
> >> > 2001 From: eric miao <eric.miao@marvell.com> Date: Tue, 22 Jan 2008
> >> > 16:34:12 +0800 Subject: [PATCH] pxa: remove the pin configuration
> >> > from the keypad driver
> >> >
> >> > The pin configurations will slowly be moved to the board specific
> >> > code at initialization thus to make the driver more generic.
> >> >
> >> > Signed-off-by: eric miao <eric.miao@marvell.com> ---
> >> >  drivers/input/keyboard/pxa27x_keypad.c   |    4 ----
> >> >  include/asm-arm/arch-pxa/pxa27x_keypad.h |    1 - 2 files changed, 0
> >> >  insertions(+), 5 deletions(-)
> >> >
> >> > diff --git a/drivers/input/keyboard/pxa27x_keypad.c
> >> > b/drivers/input/keyboard/pxa27x_keypad.c index 06c1d5a..43fb63d
> >> > 100644
> >> > --- a/drivers/input/keyboard/pxa27x_keypad.c +++
> >> > b/drivers/input/keyboard/pxa27x_keypad.c @@ -208,10 +208,6 @@ static
> >> > int __devinit pxa27x_keypad_probe(struct platform_device *pdev)
> >> >       if (error)
> >> >               goto err_free_irq;
> >> >
> >> > -     /* Setup GPIOs. */
> >> > -     for (i = 0; i < pdata->nr_rows + pdata->nr_cols; i++) -
> >> >     pxa_gpio_mode(pdata->gpio_modes[i]); -
> >>
> >> That would require GPIO code to be replicated in every subarch piece.
> >> Do you expect many boards require special GPIO setup or maybe it would
> >> make sense to keep something similar to the code above (possibly have
> >> pointer to array of gpio modes and array size in pdata)? This way
> >> simpler boards will just supply the list and more complex setups can
> >> still do it themselves?
> >>
> >>
> > Actually, pin configurations on different platforms vary much, and to
> > keep the driver generic enough, this stuff should really be pushed to
> > the platform code.
> >
> > Currently, we (Nicolas, Russell King and I) are proposing a similar pin
> > configuration scheme as pxa3xx is currently doing for pxa{25x,27x}.
> >
> > The specific reasons behind this change are:
> >
> > 1. pxa3xx uses a different mechanism and API to configure pins other
> > than the pxa{25x,27x}'s pxa_gpio_mode(), i.e., pxa_gpio_mode() is no
> > longer valid for pxa3xx. So this is really processor specific code we
> > need to keep out side of the driver
>
> Why not use gpio_request/gpio_direction_input? Or they are broken for
> pxa3xx?
>

A big NO here, since keypad controller has dedicated pins whose functions
are _not_ GPIO. So it's not applicable here. Besides, PXA3xx has good
generic GPIO API support, FYI.

> >
> > 2. The driver should have no assumption to the platform configuration,
> > that is, when the ->probe() is called, any settings should be ready for
> > the driver to work, thus including pin configurations.
>
> Hmmm. gpio-keys behaves just in the opposite way: it sets all gpios it wants
> to use. And IMHO it's the correct way: the code isn't spread between several
> files, but instead is kept inside one (generic) file.
>

gpio-keys driver behaves differently since it _must_ know which GPIOs it's
going to control, otherwise the driver just fails. This is not the
case for keypad
controller, where it does not have to care about the pins once the pin
configuration is done.

> --
> With best wishes
> Dmitry
>
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
Cheers
- eric

Re: [PATCH 2/12] pxa: remove the pin configuration from the keypad driver

[Dmitry Baryshkov]

eric miao wrote:

> On Jan 29, 2008 7:54 PM, Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:
>>
>> eric miao wrote:
>>
>> > On Jan 29, 2008 2:24 PM, Dmitry Torokhov <dtor@insightbb.com> wrote:
>> >> Hi Eric,
>> >>
>> >> On Wednesday 23 January 2008 02:17, eric miao wrote:
>> >> > From dbd62bced0f789765d1823f66af792933c6b46a1 Mon Sep 17 00:00:00
>> >> > 2001 From: eric miao <eric.miao@marvell.com> Date: Tue, 22 Jan
>> >> > 2008 16:34:12 +0800 Subject: [PATCH] pxa: remove the pin
>> >> > configuration from the keypad driver
>> >> >
>> >> > The pin configurations will slowly be moved to the board specific
>> >> > code at initialization thus to make the driver more generic.
>> >> >
>> >> > Signed-off-by: eric miao <eric.miao@marvell.com> ---
>> >> >  drivers/input/keyboard/pxa27x_keypad.c   |    4 ----
>> >> >  include/asm-arm/arch-pxa/pxa27x_keypad.h |    1 - 2 files
>> >> >  changed, 0 insertions(+), 5 deletions(-)
>> >> >
>> >> > diff --git a/drivers/input/keyboard/pxa27x_keypad.c
>> >> > b/drivers/input/keyboard/pxa27x_keypad.c index 06c1d5a..43fb63d
>> >> > 100644
>> >> > --- a/drivers/input/keyboard/pxa27x_keypad.c +++
>> >> > b/drivers/input/keyboard/pxa27x_keypad.c @@ -208,10 +208,6 @@
>> >> > static int __devinit pxa27x_keypad_probe(struct platform_device
>> >> > *pdev)
>> >> >       if (error)
>> >> >               goto err_free_irq;
>> >> >
>> >> > -     /* Setup GPIOs. */
>> >> > -     for (i = 0; i < pdata->nr_rows + pdata->nr_cols; i++) -
>> >> >     pxa_gpio_mode(pdata->gpio_modes[i]); -
>> >>
>> >> That would require GPIO code to be replicated in every subarch
>> >> piece. Do you expect many boards require special GPIO setup or maybe
>> >> it would make sense to keep something similar to the code above
>> >> (possibly have pointer to array of gpio modes and array size in
>> >> pdata)? This way simpler boards will just supply the list and more
>> >> complex setups can still do it themselves?
>> >>
>> >>
>> > Actually, pin configurations on different platforms vary much, and to
>> > keep the driver generic enough, this stuff should really be pushed to
>> > the platform code.
>> >
>> > Currently, we (Nicolas, Russell King and I) are proposing a similar
>> > pin configuration scheme as pxa3xx is currently doing for
>> > pxa{25x,27x}.
>> >
>> > The specific reasons behind this change are:
>> >
>> > 1. pxa3xx uses a different mechanism and API to configure pins other
>> > than the pxa{25x,27x}'s pxa_gpio_mode(), i.e., pxa_gpio_mode() is no
>> > longer valid for pxa3xx. So this is really processor specific code we
>> > need to keep out side of the driver
>>
>> Why not use gpio_request/gpio_direction_input? Or they are broken for
>> pxa3xx?
>>
>>
> A big NO here, since keypad controller has dedicated pins whose
> functions are _not_ GPIO. So it's not applicable here. Besides, PXA3xx
> has good generic GPIO API support, FYI.

No need for such sarcasm. Digging into platform w/o docs is a bit hard for me.
It may be my idee fixe, but I would prefer a solution that would add
"alt. modes" to generic GPIO. But... this is abit offtopic for this list.

>> > 2. The driver should have no assumption to the platform
>> > configuration, that is, when the ->probe() is called, any settings
>> > should be ready for the driver to work, thus including pin
>> > configurations.
>>
>> Hmmm. gpio-keys behaves just in the opposite way: it sets all gpios it
>> wants to use. And IMHO it's the correct way: the code isn't spread
>> between several files, but instead is kept inside one (generic) file.
>>
>>
> gpio-keys driver behaves differently since it _must_ know which GPIOs
> it's going to control, otherwise the driver just fails. This is not the
> case for keypad
> controller, where it does not have to care about the pins once the pin
> configuration is done.

Sorry, I misunderstood code.

-- 
With best wishes
Dmitry