Patchwork [3/3] mc13xxx: implicitly enable leds and buttons

login
register
mail settings
Submitter Philippe Rétornaz
Date July 9, 2011, 11:06 p.m.
Message ID <1310252796-10596-4-git-send-email-philippe.retornaz@epfl.ch>
Download mbox | patch
Permalink /patch/104034/
State New
Headers show

Comments

Philippe Rétornaz - July 9, 2011, 11:06 p.m.
The leds and buttons subdevices cannot be used without additional
platform data.

Use the presence of the platform data to enable the device instead
of an additional flag.

Signed-off-by: Philippe Rétornaz <philippe.retornaz@epfl.ch>
---
 arch/arm/mach-imx/mach-mx31moboard.c |    2 +-
 drivers/mfd/mc13xxx-core.c           |    4 ++--
 include/linux/mfd/mc13783.h          |    1 -
 include/linux/mfd/mc13xxx.h          |    2 --
 4 files changed, 3 insertions(+), 6 deletions(-)
Uwe Kleine-König - July 10, 2011, 8:04 a.m.
Hello,

On Sun, Jul 10, 2011 at 01:06:36AM +0200, Philippe Rétornaz wrote:
> The leds and buttons subdevices cannot be used without additional
> platform data.
> 
> Use the presence of the platform data to enable the device instead
> of an additional flag.
I guess you could make some people happy by splitting this patch into:

	- mfd/mc13xxx: implicitly enable leds and buttons (and
	  regulators?)
	- drop MC13XXX_USE_... in arch code

because the patches have different paths into mainline (unless it's
handled otherwise).

And I wonder why you introduce MC13XXX_USE_BUTTON in the first place.

Best regards
Uwe
Mark Brown - July 10, 2011, 8:37 a.m.
On Sun, Jul 10, 2011 at 10:04:50AM +0200, Uwe Kleine-König wrote:

> 	- mfd/mc13xxx: implicitly enable leds and buttons (and
> 	  regulators?)

It's nicer to enable regulators unconditionally, even if they're not
controlled by software in the system reading their state can still be
useful.
Philippe Rétornaz - July 10, 2011, 11:33 a.m.
Le dimanche 10 juillet 2011 10:04:50, Uwe Kleine-König a écrit :
> Hello,
> 
> On Sun, Jul 10, 2011 at 01:06:36AM +0200, Philippe Rétornaz wrote:
> > The leds and buttons subdevices cannot be used without additional
> > platform data.
> > 
> > Use the presence of the platform data to enable the device instead
> > of an additional flag.
> 
> I guess you could make some people happy by splitting this patch into:
> 
> 	- mfd/mc13xxx: implicitly enable leds and buttons (and
> 	  regulators?)
> 	- drop MC13XXX_USE_... in arch code

But this mean that between the two commit the build will be broken or the 
behavior will change.  Which will break bissections.

For the regulator stuff, I don't want to change the behavior of others board as 
some (mx31lite) enable the regualtor without platform data. That's why I did 
not changed it. 
Moreover for the mc13892 chip (which share the same flag), the probe() does 
modify some registers, so it's not just some cosmetic change, it will change 
the behavior on some boards.

BTW I will be on holydays next week so I won't be able to produce new patches 
soon.

Thanks,

Philippe
Uwe Kleine-König - July 10, 2011, 2:15 p.m.
Hello Philippe,

On Sun, Jul 10, 2011 at 01:33:09PM +0200, Philippe Rétornaz wrote:
> Le dimanche 10 juillet 2011 10:04:50, Uwe Kleine-König a écrit :
> > Hello,
> > 
> > On Sun, Jul 10, 2011 at 01:06:36AM +0200, Philippe Rétornaz wrote:
> > > The leds and buttons subdevices cannot be used without additional
> > > platform data.
> > > 
> > > Use the presence of the platform data to enable the device instead
> > > of an additional flag.
> > 
> > I guess you could make some people happy by splitting this patch into:
> > 
> > 	- mfd/mc13xxx: implicitly enable leds and buttons (and
> > 	  regulators?)
> > 	- drop MC13XXX_USE_... in arch code
> 
> But this mean that between the two commit the build will be broken or the 
> behavior will change.  Which will break bissections.
If you don't remove the MC13XXX_USE_... constants the unconverted boards
should still compile.
> 
> For the regulator stuff, I don't want to change the behavior of others
> board as some (mx31lite) enable the regualtor without platform data.
> That's why I did not changed it. 
If having pdata=NULL is a valid config then either we need to keep the
constant for regulators or (as Mark suggested) register unconditionally.

> Moreover for the mc13892 chip (which share the same flag), the probe() does 
> modify some registers, so it's not just some cosmetic change, it will change 
> the behavior on some boards.
no fear, that's what the -rc phase is there for. When improving
something you often have to change behaviour.

Best regards
Uwe

Patch

diff --git a/arch/arm/mach-imx/mach-mx31moboard.c b/arch/arm/mach-imx/mach-mx31moboard.c
index 447143b..c4cd6b8 100644
--- a/arch/arm/mach-imx/mach-mx31moboard.c
+++ b/arch/arm/mach-imx/mach-mx31moboard.c
@@ -279,7 +279,7 @@  static struct mc13xxx_platform_data moboard_pmic = {
 	.leds = &moboard_leds,
 	.buttons = &moboard_buttons,
 	.flags = MC13XXX_USE_REGULATOR | MC13XXX_USE_RTC |
-		MC13XXX_USE_ADC | MC13XXX_USE_LED | MC13XXX_USE_BUTTON,
+		MC13XXX_USE_ADC,
 };
 
 static struct spi_board_info moboard_spi_board_info[] __initdata = {
diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c
index 44edf78..130781f 100644
--- a/drivers/mfd/mc13xxx-core.c
+++ b/drivers/mfd/mc13xxx-core.c
@@ -774,11 +774,11 @@  err_revision:
 	if (pdata->flags & MC13XXX_USE_TOUCHSCREEN)
 		mc13xxx_add_subdevice(mc13xxx, "%s-ts");
 
-	if (pdata->flags & MC13XXX_USE_LED)
+	if (pdata->leds)
 		mc13xxx_add_subdevice_pdata(mc13xxx, "%s-led",
 				pdata->leds, sizeof(*pdata->leds));
 
-	if (pdata->flags & MC13XXX_USE_BUTTON)
+	if (pdata->buttons)
 		mc13xxx_add_subdevice_pdata(mc13xxx, "%s-pwrbutton",
 				pdata->buttons, sizeof(*pdata->buttons));
 
diff --git a/include/linux/mfd/mc13783.h b/include/linux/mfd/mc13783.h
index bd4fbac..61dca4c 100644
--- a/include/linux/mfd/mc13783.h
+++ b/include/linux/mfd/mc13783.h
@@ -114,7 +114,6 @@  static inline int mc13783_irq_ack(struct mc13783 *mc13783, int irq)
 #define MC13783_USE_ADC		MC13XXX_USE_ADC
 #define MC13783_USE_RTC		MC13XXX_USE_RTC
 #define MC13783_USE_REGULATOR	MC13XXX_USE_REGULATOR
-#define MC13783_USE_LED		MC13XXX_USE_LED
 
 #define MC13783_ADC_MODE_TS		1
 #define MC13783_ADC_MODE_SINGLE_CHAN	2
diff --git a/include/linux/mfd/mc13xxx.h b/include/linux/mfd/mc13xxx.h
index 22d8930..6c4b854 100644
--- a/include/linux/mfd/mc13xxx.h
+++ b/include/linux/mfd/mc13xxx.h
@@ -159,8 +159,6 @@  struct mc13xxx_platform_data {
 #define MC13XXX_USE_ADC		(1 << 2)
 #define MC13XXX_USE_RTC		(1 << 3)
 #define MC13XXX_USE_REGULATOR	(1 << 4)
-#define MC13XXX_USE_LED		(1 << 5)
-#define MC13XXX_USE_BUTTON	(1 << 6)
 	unsigned int flags;
 
 	struct mc13xxx_regulator_platform_data regulators;