[PATCH v2 2/2] mach-ux500: Add CG2900 devices

[PATCH v2 2/2] mach-ux500: Add CG2900 devices

[Par-Gunnar Hjalmdahl]

This patch adds the board specific data for the CG2900 driver
on a UX500 board.

Signed-off-by: Par-Gunnar Hjalmdahl <par-gunnar.p.hjalmdahl@stericsson.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 arch/arm/mach-ux500/board-mop500.c |    2 ++
 arch/arm/mach-ux500/board-mop500.h |    3 +++
 2 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
index d8e5a52..16b7343 100644
--- a/arch/arm/mach-ux500/board-mop500.c
+++ b/arch/arm/mach-ux500/board-mop500.c
@@ -417,6 +417,8 @@ static void __init mop500_init_machine(void)
 
 	platform_device_register(&ab8500_device);
 
+	cg2900_init_board();
+
 	i2c_register_board_info(0, mop500_i2c0_devices,
 				ARRAY_SIZE(mop500_i2c0_devices));
 	i2c_register_board_info(2, mop500_i2c2_devices,
diff --git a/arch/arm/mach-ux500/board-mop500.h b/arch/arm/mach-ux500/board-mop500.h
index 56722f4..7c89993 100644
--- a/arch/arm/mach-ux500/board-mop500.h
+++ b/arch/arm/mach-ux500/board-mop500.h
@@ -39,4 +39,7 @@ void __init mop500_pins_init(void);
 void mop500_uib_i2c_add(int busnum, struct i2c_board_info *info,
 		unsigned n);
 
+/* Function located in drivers/staging/cg2900 */
+extern void cg2900_init_board(void);
+
 #endif
-- 
1.7.4.1

Re: [PATCH v2 2/2] mach-ux500: Add CG2900 devices

[Arnd Bergmann]

On Monday 28 March 2011, Par-Gunnar Hjalmdahl wrote:
> diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
> index d8e5a52..16b7343 100644
> --- a/arch/arm/mach-ux500/board-mop500.c
> +++ b/arch/arm/mach-ux500/board-mop500.c
> @@ -417,6 +417,8 @@ static void __init mop500_init_machine(void)
>  
>         platform_device_register(&ab8500_device);
>  
> +       cg2900_init_board();
> +
>         i2c_register_board_info(0, mop500_i2c0_devices,
>                                 ARRAY_SIZE(mop500_i2c0_devices));
>         i2c_register_board_info(2, mop500_i2c2_devices,

Not exactly what I had in mind, but probably good enough for a start.
This adds a dependency from core code to the staging driver now,
which shouldn't be there. I suppose we can add

"Clean up device registration path to register the main device from board code"
 
to the TODO file.

	Arnd

Re: [PATCH v2 2/2] mach-ux500: Add CG2900 devices

[Greg KH]

On Mon, Mar 28, 2011 at 03:00:38PM +0200, Arnd Bergmann wrote:
> On Monday 28 March 2011, Par-Gunnar Hjalmdahl wrote:
> > diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
> > index d8e5a52..16b7343 100644
> > --- a/arch/arm/mach-ux500/board-mop500.c
> > +++ b/arch/arm/mach-ux500/board-mop500.c
> > @@ -417,6 +417,8 @@ static void __init mop500_init_machine(void)
> >  
> >         platform_device_register(&ab8500_device);
> >  
> > +       cg2900_init_board();
> > +
> >         i2c_register_board_info(0, mop500_i2c0_devices,
> >                                 ARRAY_SIZE(mop500_i2c0_devices));
> >         i2c_register_board_info(2, mop500_i2c2_devices,
> 
> Not exactly what I had in mind, but probably good enough for a start.
> This adds a dependency from core code to the staging driver now,
> which shouldn't be there. I suppose we can add
> 
> "Clean up device registration path to register the main device from board code"
>  
> to the TODO file.

No, please do not make any core code dependant on a staging driver, this
isn't ok, it needs to be stand-alone, or at the least, the rest of the
kernel needs to be able to be built with no staging drivers enabled.

thanks,

greg k-h

RE: [PATCH v2 2/2] mach-ux500: Add CG2900 devices

[Par-Gunnar HJALMDAHL]

> >
> > Not exactly what I had in mind, but probably good enough for a start.
> > This adds a dependency from core code to the staging driver now,
> > which shouldn't be there. I suppose we can add
> >
> > "Clean up device registration path to register the main device from
> board code"
> >
> > to the TODO file.
>=20
> No, please do not make any core code dependant on a staging driver,
> this
> isn't ok, it needs to be stand-alone, or at the least, the rest of the
> kernel needs to be able to be built with no staging drivers enabled.
>=20
> thanks,
>=20
> greg k-h

But how should I then do this? As I understood it I was told that I should
call an init function, but I was not allowed to add any staging folder
inclusion in the board config makefile. And now I can't do any extern
declaration either. I don't really see how I could do it then.

The only thing I can think of is to use platform device and driver for
the cg2900_init. But I wouldn't call that to call an init-function, but tha=
t
might be OK for this purpose?

Thanks,
P-G

Re: [PATCH v2 2/2] mach-ux500: Add CG2900 devices

[Greg KH]

On Mon, Mar 28, 2011 at 04:29:44PM +0200, Par-Gunnar HJALMDAHL wrote:
> > >
> > > Not exactly what I had in mind, but probably good enough for a start.
> > > This adds a dependency from core code to the staging driver now,
> > > which shouldn't be there. I suppose we can add
> > >
> > > "Clean up device registration path to register the main device from
> > board code"
> > >
> > > to the TODO file.
> > 
> > No, please do not make any core code dependant on a staging driver,
> > this
> > isn't ok, it needs to be stand-alone, or at the least, the rest of the
> > kernel needs to be able to be built with no staging drivers enabled.
> > 
> > thanks,
> > 
> > greg k-h
> 
> But how should I then do this? As I understood it I was told that I should
> call an init function, but I was not allowed to add any staging folder
> inclusion in the board config makefile. And now I can't do any extern
> declaration either. I don't really see how I could do it then.

Why can't you add a staging folder inclusion?  You could do that, and
properly set up your .h file so that if the driver is not built, your
code still properly builds and runs.

thanks,

greg k-h

Re: [PATCH v2 2/2] mach-ux500: Add CG2900 devices

[Vitaly Wool]

On Mon, Mar 28, 2011 at 11:03 AM, Par-Gunnar Hjalmdahl
<par-gunnar.p.hjalmdahl@stericsson.com> wrote:
>
> +/* Function located in drivers/staging/cg2900 */
> +extern void cg2900_init_board(void);
> +

I'm confused. Where in the drivers/staging is this function? Couldn't
find it in the previous patch.

~Vitaly

Re: [PATCH v2 2/2] mach-ux500: Add CG2900 devices

[Arnd Bergmann]

On Monday 28 March 2011, Par-Gunnar HJALMDAHL wrote:
> But how should I then do this? As I understood it I was told that I should
> call an init function, but I was not allowed to add any staging folder
> inclusion in the board config makefile. And now I can't do any extern
> declaration either. I don't really see how I could do it then.
> 
> The only thing I can think of is to use platform device and driver for
> the cg2900_init. But I wouldn't call that to call an init-function, but that
> might be OK for this purpose?

There are at least two ways to do it:

* As Linus Walleij explained, use an initcall instead of calling a
  function from the board init code. "initcall" here refers to the
  interfaces from include/linux/init.h, e.g. module_init(). This
  will result in the function getting called at module load time,
  or at some point during bootup when it's built into the kernel.
  This initcall needs to check if you are running on the right board,
  using machine_is_xxx().

* As I explained, register a simple platform_device with the resources
  for the entire cg2900 device from the board code in a way that
  is independent of the actual driver. The driver code can then
  register all the subdevices from the cg2900_probe function.
  The code in the architecture would consist out of a single
  call to platform_device_register_simple() plus the resources.

Either way is fine with me.

	Arnd

Re: [PATCH v2 2/2] mach-ux500: Add CG2900 devices

[Arnd Bergmann]

On Monday 28 March 2011, Vitaly Wool wrote:
> On Mon, Mar 28, 2011 at 11:03 AM, Par-Gunnar Hjalmdahl
> <par-gunnar.p.hjalmdahl@stericsson.com> wrote:
> >
> > +/* Function located in drivers/staging/cg2900 */
> > +extern void cg2900_init_board(void);
> > +
> 
> I'm confused. Where in the drivers/staging is this function? Couldn't
> find it in the previous patch.

It's in v2 of patch 2/2, in

drivers/staging/cg2900/board-mop500-cg2900.c

	Arnd

Re: [PATCH v2 2/2] mach-ux500: Add CG2900 devices

[Arnd Bergmann]

On Monday 28 March 2011, Greg KH wrote:
> > 
> > But how should I then do this? As I understood it I was told that I should
> > call an init function, but I was not allowed to add any staging folder
> > inclusion in the board config makefile. And now I can't do any extern
> > declaration either. I don't really see how I could do it then.
> 
> Why can't you add a staging folder inclusion?  You could do that, and
> properly set up your .h file so that if the driver is not built, your
> code still properly builds and runs.

That would work as well, along with the two solutions I suggested.

I believe Pär-Gunnar was trying to avoid #ifdefs, and the patch actually
contains alternative files implementing cg2900_init_board as a stub
when CONFIG_CG2900 is set, so the intent was clearly there:

@@ -0,0 +1,18 @@
+#
+# Makefile for ST-Ericsson CG2900 connectivity combo controller
+#
+
+ccflags-y :=                                   \
+       -Idrivers/staging/cg2900/include \
+       -Iarch/arm/mach-ux500
+
+ifeq ($(CONFIG_CG2900),n)
+obj-y  += board-mop500-nocg2900.o
+export-objs                    := board-mop500-nocg2900.o
+else
+obj-$(CONFIG_CG2900)   += board-mop500-cg2900.o devices-cg2900.o
+export-objs                    := board-mop500-cg2900.o
+endif
+

I don't think that this works though, because the directory
is ignored when CONFIG_CG2900 is not set, and if it did work,
it would cause an empty function to be included in every
single kernel, not even limited to the ARM architecture.

	Arnd

RE: [PATCH v2 2/2] mach-ux500: Add CG2900 devices

[Par-Gunnar HJALMDAHL]

I was asked to break out the CG2900 specific stuff from mach-ux500
folder and put it into the staging folder.
So the function exist in the new file
drivers/staging/cg2900/board-mop500-cg2900.c
(As you point out the file and the function did not exist in
the previous patch).

/P-G

> -----Original Message-----
> From: Vitaly Wool [mailto:vitalywool@gmail.com]
> Sent: den 28 mars 2011 16:40
> To: Par-Gunnar HJALMDAHL
> Cc: Greg Kroah-Hartman; devel@driverdev.osuosl.org; Linus Walleij;
> linux-kernel@vger.kernel.org; linux-bluetooth@vger.kernel.org; Pavan
> Savoy; Alan Cox; Arnd Bergmann; Marcel Holtmann; Lukasz Rymanowski;
> Linus WALLEIJ; Par-Gunnar Hjalmdahl; Lee Jones; Mathieu Poirier
> Subject: Re: [PATCH v2 2/2] mach-ux500: Add CG2900 devices
>=20
> On Mon, Mar 28, 2011 at 11:03 AM, Par-Gunnar Hjalmdahl
> <par-gunnar.p.hjalmdahl@stericsson.com> wrote:
> >
> > +/* Function located in drivers/staging/cg2900 */
> > +extern void cg2900_init_board(void);
> > +
>=20
> I'm confused. Where in the drivers/staging is this function? Couldn't
> find it in the previous patch.
>=20
> ~Vitaly