CPM1: implement GPIO API
Subsystems (2)
This is based on [PATCH RFC 3/7] [POWERPC] CPM2: implement GPIO API.
--- arch/powerpc/platforms/8xx/Kconfig | 1 + arch/powerpc/sysdev/commproc.c | 199 +++++++++++++++++++++++++++++++++++- 2 files changed, 199 insertions(+), 1 deletions(-)
arch/powerpc/platforms/8xx/Kconfig +1 -0
@@ -4,6 +4,7 @@ config FADS config CPM1 bool select CPM + select GENERIC_GPIO choice prompt "8xx Machine Type"
arch/powerpc/sysdev/commproc.c +198 -1
250 lines not on this page · raw message
Signature · 1 line
1.5.3.7
arch/powerpc/sysdev/commproc.c static cpic8xx_t __iomem *cpic_reg; · line 62
Quoted 2 lines · Jochen Friedrich
arch/powerpc/sysdev/commproc.c +2
-0+static spinlock_t *cpm1_port_locks; +static int cpm1_num_ports;
Having an array of spinlocks is rather unusual and normally not necessary. Did you measure a significant performance impact by using a global lock for all ports?
If not, I would recommend simplifying this.
arch/powerpc/sysdev/commproc.c int cpm1_clk_setup(enum cpm_clk_target target, int clock, int mode) · line 806
Quoted 5 lines
arch/powerpc/sysdev/commproc.c +5
-0+EXPORT_SYMBOL_GPL (gpio_request); +EXPORT_SYMBOL_GPL (gpio_direction_input); +EXPORT_SYMBOL_GPL (gpio_direction_output); +EXPORT_SYMBOL_GPL (gpio_get_value); +EXPORT_SYMBOL_GPL (gpio_set_value);
All these function names are rather generic identifiers, but you export them from a platform specific file. I'd say they should either have a more specific name space (e.g. cpm1_gpio_request), or should be implemented in a more generic way.
Arnd <><
Quoted 9 lines · Arnd Bergmann
arch/powerpc/sysdev/commproc.c static cpic8xx_t __iomem *cpic_reg; · line 62
Quoted 2 lines · Jochen Friedrich
arch/powerpc/sysdev/commproc.c +2
-0+static spinlock_t *cpm1_port_locks; +static int cpm1_num_ports;Having an array of spinlocks is rather unusual and normally not necessary. Did you measure a significant performance impact by using a global lock for all ports?
If not, I would recommend simplifying this.
I disagree. Why should we force bank A to stomp on bank B, if we can prevent this in few lines of trivial code? It doesn't need any measuring or simplifying IMO, what's so complexing in array of spinlocks guarding array of banks?
But, you can repeat that you really want to get rid of this array -- and I'll silently remove it. Not because I gave up, but mostly because with gpiolib we'll able to embed spinlock into per-bank gpio_chip structure, thus eventually they will come back. :-))
Quoted 10 lines
arch/powerpc/sysdev/commproc.c int cpm1_clk_setup(enum cpm_clk_target target, int clock, int mode) · line 806
Quoted 5 lines
arch/powerpc/sysdev/commproc.c +5
-0+EXPORT_SYMBOL_GPL (gpio_request); +EXPORT_SYMBOL_GPL (gpio_direction_input); +EXPORT_SYMBOL_GPL (gpio_direction_output); +EXPORT_SYMBOL_GPL (gpio_get_value); +EXPORT_SYMBOL_GPL (gpio_set_value);All these function names are rather generic identifiers, but you export them from a platform specific file. I'd say they should either have a more specific name space (e.g.
cpm1_gpio_request), or should be implemented in a more generic way.
No. This is how gpio api is working currently. With gpiolib[1], most of these functions will be controller-specific. IIRC, gpiolib is still in early development stage, so, for now, we have to limit us to one gpio chip controller.
This works great for us: CPM, CPM2 and QE shouldn't appear on the single crystal.
I'm not sure if gpiolib will hit 2.6.25, really. Maybe not. So I'd rather stick with limited gpio api, and later convert it to gpiolib -- it will be really trivial. Just rename functions and wrap them around gpio_chip structure.
The bright side of smooth transition is that API itself isn't changing, GPIO API/GPIOLIB API is fully interchangeable, their differences are in implementation details.
Thanks,
Signature · 4 lines
Anton Vorontsov email: [email protected] backup email: [email protected] irc://irc.freenode.net/bd2
Quoted 7 lines · Anton Vorontsov
No. This is how gpio api is working currently. With gpiolib[1], most of these functions will be controller-specific. IIRC, gpiolib is still in early development stage, so, for now, we have to limit us to one gpio chip controller.
This works great for us: CPM, CPM2 and QE shouldn't appear on the single crystal.
But at least the latter two should be able to co-exist in a single kernel image... Oh well, all we can do is hope the saner API makes it in soon. :-)
-Scott
arch/powerpc/sysdev/commproc.c static cpic8xx_t __iomem *cpic_reg; · line 84
Quoted 12 lines · Jochen Friedrich
arch/powerpc/sysdev/commproc.c +12
-0+int cpm_init_par_io (void ) +{ +int ret; +struct device_node *np; +const u32 *num_ports; +int i; + + np =of_find_node_by_name (NULL ,"par_io" ); +if (!np) { + ret = -ENOENT ; + goto err0; + }
Shouldn't this lookup be by compatible (something like fsl,cpm1-gpio would be good)?
arch/powerpc/sysdev/commproc.c int cpm1_clk_setup(enum cpm_clk_target target, int clock, int mode) · line 737
Quoted 7 lines
arch/powerpc/sysdev/commproc.c +7
-0+int gpio_request (unsigned int gpio,const char *label) +{ +if (!cpm1_port_locks) +return -ENODEV ; + +if (gpio /32 > cpm1_num_ports) +return -EINVAL ;
Shouldn't this be ">="?
Review
Quoted 1 line
(quoted diff) +1
-0+ return 0;
No already-requested check?
arch/powerpc/sysdev/commproc.c int cpm1_clk_setup(enum cpm_clk_target target, int clock, int mode) · line 740
Quoted 2 lines
arch/powerpc/sysdev/commproc.c +2
-0+} +EXPORT_SYMBOL_GPL (gpio_request);
This is an API, not internals; can we stick with plain EXPORT_SYMBOL()?
-Scott
Quoted 4 lines · Scott Wood
arch/powerpc/sysdev/commproc.c int cpm1_clk_setup(enum cpm_clk_target target, int clock, int mode) · line 740
Quoted 2 lines
arch/powerpc/sysdev/commproc.c +2
-0+} +EXPORT_SYMBOL_GPL (gpio_request);This is an API, not internals; can we stick with plain
EXPORT_SYMBOL()?
The architecture independent API posted by David Brownell uses _GPL, see http://lkml.org/lkml/2007/11/9/141.
I'd vote for _GPL on all new interfaces anyway, but I guess this one is really David's decision since he came up with the API.
Arnd <><