[PATCH v2 01/21] ARM: pxa: magician: Fix Kconfig for magician to always include htc-egpio

[PATCH v2 01/21] ARM: pxa: magician: Fix Kconfig for magician to always include htc-egpio

[Petr Cvek]

The htc-egpio driver should be always included in the kernel as it
controls MMC/charging/IrDA power, which is the only way to get
an useful rootfs.

Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
---
 arch/arm/mach-pxa/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-pxa/Kconfig b/arch/arm/mach-pxa/Kconfig
index f096836..5c7b6ee 100644
--- a/arch/arm/mach-pxa/Kconfig
+++ b/arch/arm/mach-pxa/Kconfig
@@ -294,6 +294,7 @@ config MACH_MAGICIAN
 	bool "Enable HTC Magician Support"
 	select IWMMXT
 	select PXA27x
+	select HTC_EGPIO
 
 config MACH_MIOA701
 	bool "Mitac Mio A701 Support"
-- 
1.7.12.1

[PATCH v2 01/21] ARM: pxa: magician: Fix Kconfig for magician to always include htc-egpio

[Robert Jarzmik]

Petr Cvek <petr.cvek@tul.cz> writes:

> The htc-egpio driver should be always included in the kernel as it
> controls MMC/charging/IrDA power, which is the only way to get
> an useful rootfs.
>
> Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
It's already defined in magician_defconfig, why enforce it in Kconfig ?

And if it's enforced and I want to build a kernel for magician without
MMC/charging/IrDA, why should I be forced to build in HTC_EGPIO ?

Or said differently, does a kernel without HTC_EGPIO is proven to fail 100%
without this option ?

Cheers.

-- 
Robert

[PATCH v2 01/21] ARM: pxa: magician: Fix Kconfig for magician to always include htc-egpio

[Petr Cvek]

Dne 18.8.2015 v 20:31 Robert Jarzmik napsal(a):
> Petr Cvek <petr.cvek@tul.cz> writes:
> 
>> The htc-egpio driver should be always included in the kernel as it
>> controls MMC/charging/IrDA power, which is the only way to get
>> an useful rootfs.
>>
>> Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
> It's already defined in magician_defconfig, why enforce it in Kconfig ?

Good remark, I did not use defconfig at all (as I had to fix from zero configuration + I did not know if defconfig works and will be supported in the future).

I will check if magician_defconfig needs to be updated (probably for leds).

> 
> And if it's enforced and I want to build a kernel for magician without
> MMC/charging/IrDA, why should I be forced to build in HTC_EGPIO ?
> 
> Or said differently, does a kernel without HTC_EGPIO is proven to fail 100%
> without this option ?

Kernel will probably run OK, but the phone will be unusable (for any practical purposes). The EGPIO chip (CPLD) will stay in the state programmed from the previous environment (bootloader or WinCE/Mobile) and drivers fail to init because of the missing EGPIO (and if they init, they will be without any idea about state of the peripherals (no charger detection/charging current stuck on the previous settings, no LCD backlight control, no IrDA, no rootfs, no GSM, no sound, no LEDs).

Different PCB revisions probably use EGPIO pin for LCD power (and at least on my phone, LCD power must have very specific order).

For future (when ROM XIP will work) I would like to be able boot kernel from flash, which itself uses EGPIO for Vpp control (another initial EGPIO configuration). Last possible boot is from JTAG and there will be no previous EGPIO initialization.

Petr

[PATCH v2 01/21] ARM: pxa: magician: Fix Kconfig for magician to always include htc-egpio

[Philipp Zabel]

Am Dienstag, den 18.08.2015, 22:02 +0200 schrieb Petr Cvek:
> Dne 18.8.2015 v 20:31 Robert Jarzmik napsal(a):
> > Petr Cvek <petr.cvek@tul.cz> writes:
> > 
> > > The htc-egpio driver should be always included in the kernel as 
> > > it
> > > controls MMC/charging/IrDA power, which is the only way to get
> > > an useful rootfs.
> > > 
> > > Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
> > It's already defined in magician_defconfig, why enforce it in 
> > Kconfig ?
> 
> Good remark, I did not use defconfig at all (as I had to fix from 
> zero configuration + I did not know if defconfig works and will be 
> supported in the future).
> 
> I will check if magician_defconfig needs to be updated (probably for 
> leds).
> 
> > 
> > And if it's enforced and I want to build a kernel for magician 
> > without
> > MMC/charging/IrDA, why should I be forced to build in HTC_EGPIO ?
> > 
> > Or said differently, does a kernel without HTC_EGPIO is proven to 
> > fail 100%
> > without this option ?
> 
> Kernel will probably run OK, but the phone will be unusable (for any 
> practical purposes). The EGPIO chip (CPLD) will stay in the state 
> programmed from the previous environment (bootloader or WinCE/Mobile) 
> and drivers fail to init because of the missing EGPIO (and if they 
> init, they will be without any idea about state of the peripherals 
> (no charger detection/charging current stuck on the previous 
> settings, no LCD backlight control, no IrDA, no rootfs, no GSM, no 
> sound, no LEDs).
> 
> Different PCB revisions probably use EGPIO pin for LCD power (and at 
> least on my phone, LCD power must have very specific order).
> 
> For future (when ROM XIP will work) I would like to be able boot 
> kernel from flash, which itself uses EGPIO for Vpp control (another 
> initial EGPIO configuration). Last possible boot is from JTAG and 
> there will be no previous EGPIO initialization.
> 
> Petr

I agree with Robert, let's drop this patch.

regards
Philipp

[PATCH v2 01/21] ARM: pxa: magician: Fix Kconfig for magician to always include htc-egpio

[Petr Cvek]

Dne 19.8.2015 v 09:29 Philipp Zabel napsal(a):
> Am Dienstag, den 18.08.2015, 22:02 +0200 schrieb Petr Cvek:
>> Dne 18.8.2015 v 20:31 Robert Jarzmik napsal(a):
>>> Petr Cvek <petr.cvek@tul.cz> writes:
>>>
>>>> The htc-egpio driver should be always included in the kernel as 
>>>> it
>>>> controls MMC/charging/IrDA power, which is the only way to get
>>>> an useful rootfs.
>>>>
>>>> Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
>>> It's already defined in magician_defconfig, why enforce it in 
>>> Kconfig ?
>>
>> Good remark, I did not use defconfig at all (as I had to fix from 
>> zero configuration + I did not know if defconfig works and will be 
>> supported in the future).
>>
>> I will check if magician_defconfig needs to be updated (probably for 
>> leds).
>>
>>>
>>> And if it's enforced and I want to build a kernel for magician 
>>> without
>>> MMC/charging/IrDA, why should I be forced to build in HTC_EGPIO ?
>>>
>>> Or said differently, does a kernel without HTC_EGPIO is proven to 
>>> fail 100%
>>> without this option ?
>>
>> Kernel will probably run OK, but the phone will be unusable (for any 
>> practical purposes). The EGPIO chip (CPLD) will stay in the state 
>> programmed from the previous environment (bootloader or WinCE/Mobile) 
>> and drivers fail to init because of the missing EGPIO (and if they 
>> init, they will be without any idea about state of the peripherals 
>> (no charger detection/charging current stuck on the previous 
>> settings, no LCD backlight control, no IrDA, no rootfs, no GSM, no 
>> sound, no LEDs).
>>
>> Different PCB revisions probably use EGPIO pin for LCD power (and at 
>> least on my phone, LCD power must have very specific order).
>>
>> For future (when ROM XIP will work) I would like to be able boot 
>> kernel from flash, which itself uses EGPIO for Vpp control (another 
>> initial EGPIO configuration). Last possible boot is from JTAG and 
>> there will be no previous EGPIO initialization.
>>
>> Petr
> 
> I agree with Robert, let's drop this patch.
As finishing patchset v3 I've finally got enough deterministic configuration.

Situation A:
	Most code same as v2 patchset (GPIO init, devices, ...)
	RTC patch (not in patchset, but vanilla causes errors during boot)
	Boot from bootloader (htc-tools), kernel from RAM 
	Rootfs on SDHC
	EGPIO driver in kernel
	-> boot
Situation B:
	EGPIO disabled
	-> every EGPIO requests fail (bad LCD readability, but most probably by -EDEFER)
	mci driver fails to start (SD card power EGPIO and others are not found)
	kernel prints cannot open root device ...
	there is kernel panic from rootfs mount (stackdump with some *mount* function name)

-> So it seems EGPIO driver is required for standard boot.

When trying to boot with initrd, the htc-boot will freeze during image upload (around 5 MB of initrd, not enough for rootfs), but maybe I'm doing something wrong. 

-> Without initrd, there is no way to get useful rootfs.

Petr

[PATCH v2 02/21] ARM: pxa: magician: Fix indentation and whitespaces

[Petr Cvek]

Fix magician.c: indentation and whitespaces.

Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
---
 arch/arm/mach-pxa/magician.c | 356 +++++++++++++++++++++----------------------
 1 file changed, 178 insertions(+), 178 deletions(-)

diff --git a/arch/arm/mach-pxa/magician.c b/arch/arm/mach-pxa/magician.c
index a9761c2..9e8698a 100644
--- a/arch/arm/mach-pxa/magician.c
+++ b/arch/arm/mach-pxa/magician.c
@@ -52,8 +52,8 @@ static unsigned long magician_pin_config[] __initdata = {
 	GPIO20_nSDCS_2,
 	GPIO21_nSDCS_3,
 	GPIO15_nCS_1,
-	GPIO78_nCS_2,   /* PASIC3 */
-	GPIO79_nCS_3,   /* EGPIO CPLD */
+	GPIO78_nCS_2,	/* PASIC3 */
+	GPIO79_nCS_3,	/* EGPIO CPLD */
 	GPIO80_nCS_4,
 	GPIO33_nCS_5,
 
@@ -107,7 +107,7 @@ static unsigned long magician_pin_config[] __initdata = {
 	GPIO85_CIF_LV,
 
 	/* Magician specific input GPIOs */
-	GPIO9_GPIO,	/* unknown */
+	GPIO9_GPIO,		/* unknown */
 	GPIO10_GPIO,	/* GSM_IRQ */
 	GPIO13_GPIO,	/* CPLD_IRQ */
 	GPIO107_GPIO,	/* DS1WM_IRQ */
@@ -133,46 +133,45 @@ static struct pxaficp_platform_data magician_ficp_info = {
  */
 
 #define INIT_KEY(_code, _gpio, _desc)	\
-	{				\
-		.code   = KEY_##_code,	\
-		.gpio   = _gpio,	\
-		.desc   = _desc,	\
-		.type   = EV_KEY,	\
-		.wakeup = 1,		\
+	{						\
+		.code	= KEY_##_code,	\
+		.gpio	= _gpio,	\
+		.desc	= _desc,	\
+		.type	= EV_KEY,	\
+		.wakeup	= 1,		\
 	}
 
 static struct gpio_keys_button magician_button_table[] = {
-	INIT_KEY(POWER,      GPIO0_MAGICIAN_KEY_POWER,      "Power button"),
-	INIT_KEY(ESC,        GPIO37_MAGICIAN_KEY_HANGUP,    "Hangup button"),
-	INIT_KEY(F10,        GPIO38_MAGICIAN_KEY_CONTACTS,  "Contacts button"),
-	INIT_KEY(CALENDAR,   GPIO90_MAGICIAN_KEY_CALENDAR,  "Calendar button"),
-	INIT_KEY(CAMERA,     GPIO91_MAGICIAN_KEY_CAMERA,    "Camera button"),
-	INIT_KEY(UP,         GPIO93_MAGICIAN_KEY_UP,        "Up button"),
-	INIT_KEY(DOWN,       GPIO94_MAGICIAN_KEY_DOWN,      "Down button"),
-	INIT_KEY(LEFT,       GPIO95_MAGICIAN_KEY_LEFT,      "Left button"),
-	INIT_KEY(RIGHT,      GPIO96_MAGICIAN_KEY_RIGHT,     "Right button"),
-	INIT_KEY(KPENTER,    GPIO97_MAGICIAN_KEY_ENTER,     "Action button"),
-	INIT_KEY(RECORD,     GPIO98_MAGICIAN_KEY_RECORD,    "Record button"),
-	INIT_KEY(VOLUMEUP,   GPIO100_MAGICIAN_KEY_VOL_UP,   "Volume up"),
-	INIT_KEY(VOLUMEDOWN, GPIO101_MAGICIAN_KEY_VOL_DOWN, "Volume down"),
-	INIT_KEY(PHONE,      GPIO102_MAGICIAN_KEY_PHONE,    "Phone button"),
-	INIT_KEY(PLAY,       GPIO99_MAGICIAN_HEADPHONE_IN,  "Headset button"),
+	INIT_KEY(POWER,		GPIO0_MAGICIAN_KEY_POWER,	"Power button"),
+	INIT_KEY(ESC,		GPIO37_MAGICIAN_KEY_HANGUP,	"Hangup button"),
+	INIT_KEY(F10,		GPIO38_MAGICIAN_KEY_CONTACTS,	"Contacts button"),
+	INIT_KEY(CALENDAR,	GPIO90_MAGICIAN_KEY_CALENDAR,	"Calendar button"),
+	INIT_KEY(CAMERA,	GPIO91_MAGICIAN_KEY_CAMERA,	"Camera button"),
+	INIT_KEY(UP,		GPIO93_MAGICIAN_KEY_UP,		"Up button"),
+	INIT_KEY(DOWN,		GPIO94_MAGICIAN_KEY_DOWN,	"Down button"),
+	INIT_KEY(LEFT,		GPIO95_MAGICIAN_KEY_LEFT,	"Left button"),
+	INIT_KEY(RIGHT,		GPIO96_MAGICIAN_KEY_RIGHT,	"Right button"),
+	INIT_KEY(KPENTER,	GPIO97_MAGICIAN_KEY_ENTER,	"Action button"),
+	INIT_KEY(RECORD,	GPIO98_MAGICIAN_KEY_RECORD,	"Record button"),
+	INIT_KEY(VOLUMEUP,	GPIO100_MAGICIAN_KEY_VOL_UP,	"Volume up"),
+	INIT_KEY(VOLUMEDOWN,	GPIO101_MAGICIAN_KEY_VOL_DOWN,	"Volume down"),
+	INIT_KEY(PHONE,		GPIO102_MAGICIAN_KEY_PHONE,	"Phone button"),
+	INIT_KEY(PLAY,		GPIO99_MAGICIAN_HEADPHONE_IN,	"Headset button"),
 };
 
 static struct gpio_keys_platform_data gpio_keys_data = {
-	.buttons  = magician_button_table,
+	.buttons = magician_button_table,
 	.nbuttons = ARRAY_SIZE(magician_button_table),
 };
 
 static struct platform_device gpio_keys = {
 	.name = "gpio-keys",
-	.dev  = {
+	.dev = {
 		.platform_data = &gpio_keys_data,
 	},
-	.id   = -1,
+	.id = -1,
 };
 
-
 /*
  * EGPIO (Xilinx CPLD)
  *
@@ -181,14 +180,14 @@ static struct platform_device gpio_keys = {
 
 static struct resource egpio_resources[] = {
 	[0] = {
-		.start = PXA_CS3_PHYS,
-		.end   = PXA_CS3_PHYS + 0x20 - 1,
-		.flags = IORESOURCE_MEM,
+		.start	= PXA_CS3_PHYS,
+		.end	= PXA_CS3_PHYS + 0x20 - 1,
+		.flags	= IORESOURCE_MEM,
 	},
 	[1] = {
-		.start = PXA_GPIO_TO_IRQ(GPIO13_MAGICIAN_CPLD_IRQ),
-		.end   = PXA_GPIO_TO_IRQ(GPIO13_MAGICIAN_CPLD_IRQ),
-		.flags = IORESOURCE_IRQ,
+		.start	= PXA_GPIO_TO_IRQ(GPIO13_MAGICIAN_CPLD_IRQ),
+		.end	= PXA_GPIO_TO_IRQ(GPIO13_MAGICIAN_CPLD_IRQ),
+		.flags	= IORESOURCE_IRQ,
 	},
 };
 
@@ -209,20 +208,20 @@ static struct htc_egpio_chip egpio_chips[] = {
 };
 
 static struct htc_egpio_platform_data egpio_info = {
-	.reg_width    = 8,
-	.bus_width    = 32,
-	.irq_base     = IRQ_BOARD_START,
-	.num_irqs     = 4,
-	.ack_register = 3,
-	.chip         = egpio_chips,
-	.num_chips    = ARRAY_SIZE(egpio_chips),
+	.reg_width	= 8,
+	.bus_width	= 32,
+	.irq_base	= IRQ_BOARD_START,
+	.num_irqs	= 4,
+	.ack_register	= 3,
+	.chip		= egpio_chips,
+	.num_chips	= ARRAY_SIZE(egpio_chips),
 };
 
 static struct platform_device egpio = {
-	.name          = "htc-egpio",
-	.id            = -1,
-	.resource      = egpio_resources,
-	.num_resources = ARRAY_SIZE(egpio_resources),
+	.name		= "htc-egpio",
+	.id		= -1,
+	.resource	= egpio_resources,
+	.num_resources	= ARRAY_SIZE(egpio_resources),
 	.dev = {
 		.platform_data = &egpio_info,
 	},
@@ -234,33 +233,33 @@ static struct platform_device egpio = {
 
 static struct pxafb_mode_info toppoly_modes[] = {
 	{
-		.pixclock     = 96153,
-		.bpp          = 16,
-		.xres         = 240,
-		.yres         = 320,
-		.hsync_len    = 11,
-		.vsync_len    = 3,
-		.left_margin  = 19,
-		.upper_margin = 2,
-		.right_margin = 10,
-		.lower_margin = 2,
-		.sync         = 0,
+		.pixclock	= 96153,
+		.bpp		= 16,
+		.xres		= 240,
+		.yres		= 320,
+		.hsync_len	= 11,
+		.vsync_len	= 3,
+		.left_margin	= 19,
+		.upper_margin	= 2,
+		.right_margin	= 10,
+		.lower_margin	= 2,
+		.sync		= 0,
 	},
 };
 
 static struct pxafb_mode_info samsung_modes[] = {
 	{
-		.pixclock     = 96153,
-		.bpp          = 16,
-		.xres         = 240,
-		.yres         = 320,
-		.hsync_len    = 8,
-		.vsync_len    = 4,
-		.left_margin  = 9,
-		.upper_margin = 4,
-		.right_margin = 9,
-		.lower_margin = 4,
-		.sync         = FB_SYNC_HOR_HIGH_ACT | FB_SYNC_VERT_HIGH_ACT,
+		.pixclock	= 96153,
+		.bpp		= 16,
+		.xres		= 240,
+		.yres		= 320,
+		.hsync_len	= 8,
+		.vsync_len	= 4,
+		.left_margin	= 9,
+		.upper_margin	= 4,
+		.right_margin	= 9,
+		.lower_margin	= 4,
+		.sync	= FB_SYNC_HOR_HIGH_ACT | FB_SYNC_VERT_HIGH_ACT,
 	},
 };
 
@@ -326,19 +325,19 @@ static void samsung_lcd_power(int on, struct fb_var_screeninfo *si)
 }
 
 static struct pxafb_mach_info toppoly_info = {
-	.modes           = toppoly_modes,
-	.num_modes       = 1,
-	.fixed_modes     = 1,
+	.modes		= toppoly_modes,
+	.num_modes	= 1,
+	.fixed_modes	= 1,
 	.lcd_conn	= LCD_COLOR_TFT_16BPP,
 	.pxafb_lcd_power = toppoly_lcd_power,
 };
 
 static struct pxafb_mach_info samsung_info = {
-	.modes           = samsung_modes,
-	.num_modes       = 1,
-	.fixed_modes     = 1,
-	.lcd_conn	 = LCD_COLOR_TFT_16BPP | LCD_PCLK_EDGE_FALL |\
-			   LCD_ALTERNATE_MAPPING,
+	.modes		= samsung_modes,
+	.num_modes	= 1,
+	.fixed_modes	= 1,
+	.lcd_conn	= LCD_COLOR_TFT_16BPP | LCD_PCLK_EDGE_FALL |
+		LCD_ALTERNATE_MAPPING,
 	.pxafb_lcd_power = samsung_lcd_power,
 };
 
@@ -374,22 +373,22 @@ static void magician_backlight_exit(struct device *dev)
 }
 
 static struct platform_pwm_backlight_data backlight_data = {
-	.pwm_id         = 0,
-	.max_brightness = 272,
-	.dft_brightness = 100,
-	.pwm_period_ns  = 30923,
-	.enable_gpio    = -1,
-	.init           = magician_backlight_init,
-	.notify         = magician_backlight_notify,
-	.exit           = magician_backlight_exit,
+	.pwm_id		= 0,
+	.max_brightness	= 272,
+	.dft_brightness	= 100,
+	.pwm_period_ns	= 30923,
+	.enable_gpio	= -1,
+	.init		= magician_backlight_init,
+	.notify		= magician_backlight_notify,
+	.exit		= magician_backlight_exit,
 };
 
 static struct platform_device backlight = {
-	.name = "pwm-backlight",
-	.id   = -1,
-	.dev  = {
-		.parent        = &pxa27x_device_pwm0.dev,
-		.platform_data = &backlight_data,
+	.name	= "pwm-backlight",
+	.id	= -1,
+	.dev	= {
+		.parent		= &pxa27x_device_pwm0.dev,
+		.platform_data	= &backlight_data,
 	},
 };
 
@@ -416,9 +415,9 @@ static struct gpio_led_platform_data gpio_led_info = {
 };
 
 static struct platform_device leds_gpio = {
-	.name = "leds-gpio",
-	.id   = -1,
-	.dev  = {
+	.name	= "leds-gpio",
+	.id	= -1,
+	.dev	= {
 		.platform_data = &gpio_led_info,
 	},
 };
@@ -426,37 +425,37 @@ static struct platform_device leds_gpio = {
 static struct pasic3_led pasic3_leds[] = {
 	{
 		.led = {
-			.name            = "magician:red",
+			.name = "magician:red",
 			.default_trigger = "ds2760-battery.0-charging",
 		},
-		.hw_num = 0,
-		.bit2   = PASIC3_BIT2_LED0,
-		.mask   = PASIC3_MASK_LED0,
+		.hw_num	= 0,
+		.bit2	= PASIC3_BIT2_LED0,
+		.mask	= PASIC3_MASK_LED0,
 	},
 	{
 		.led = {
-			.name            = "magician:green",
+			.name = "magician:green",
 			.default_trigger = "ds2760-battery.0-charging-or-full",
 		},
-		.hw_num = 1,
-		.bit2   = PASIC3_BIT2_LED1,
-		.mask   = PASIC3_MASK_LED1,
+		.hw_num	= 1,
+		.bit2	= PASIC3_BIT2_LED1,
+		.mask	= PASIC3_MASK_LED1,
 	},
 	{
 		.led = {
-			.name            = "magician:blue",
+			.name = "magician:blue",
 			.default_trigger = "bluetooth",
 		},
-		.hw_num = 2,
-		.bit2   = PASIC3_BIT2_LED2,
-		.mask   = PASIC3_MASK_LED2,
+		.hw_num	= 2,
+		.bit2	= PASIC3_BIT2_LED2,
+		.mask	= PASIC3_MASK_LED2,
 	},
 };
 
 static struct pasic3_leds_machinfo pasic3_leds_info = {
-	.num_leds   = ARRAY_SIZE(pasic3_leds),
-	.power_gpio = EGPIO_MAGICIAN_LED_POWER,
-	.leds       = pasic3_leds,
+	.num_leds	= ARRAY_SIZE(pasic3_leds),
+	.power_gpio	= EGPIO_MAGICIAN_LED_POWER,
+	.leds		= pasic3_leds,
 };
 
 /*
@@ -465,28 +464,28 @@ static struct pasic3_leds_machinfo pasic3_leds_info = {
 
 static struct resource pasic3_resources[] = {
 	[0] = {
-		.start  = PXA_CS2_PHYS,
+		.start	= PXA_CS2_PHYS,
 		.end	= PXA_CS2_PHYS + 0x1b,
-		.flags  = IORESOURCE_MEM,
+		.flags	= IORESOURCE_MEM,
 	},
 	/* No IRQ handler in the PASIC3, DS1WM needs an external IRQ */
 	[1] = {
-		.start  = PXA_GPIO_TO_IRQ(GPIO107_MAGICIAN_DS1WM_IRQ),
-		.end    = PXA_GPIO_TO_IRQ(GPIO107_MAGICIAN_DS1WM_IRQ),
-		.flags  = IORESOURCE_IRQ | IORESOURCE_IRQ_HIGHEDGE,
+		.start	= PXA_GPIO_TO_IRQ(GPIO107_MAGICIAN_DS1WM_IRQ),
+		.end	= PXA_GPIO_TO_IRQ(GPIO107_MAGICIAN_DS1WM_IRQ),
+		.flags	= IORESOURCE_IRQ | IORESOURCE_IRQ_HIGHEDGE,
 	}
 };
 
 static struct pasic3_platform_data pasic3_platform_data = {
-	.led_pdata  = &pasic3_leds_info,
-	.clock_rate = 4000000,
+	.led_pdata	= &pasic3_leds_info,
+	.clock_rate	= 4000000,
 };
 
 static struct platform_device pasic3 = {
-	.name		= "pasic3",
-	.id		= -1,
-	.num_resources	= ARRAY_SIZE(pasic3_resources),
-	.resource	= pasic3_resources,
+	.name = "pasic3",
+	.id = -1,
+	.num_resources = ARRAY_SIZE(pasic3_resources),
+	.resource = pasic3_resources,
 	.dev = {
 		.platform_data = &pasic3_platform_data,
 	},
@@ -497,21 +496,21 @@ static struct platform_device pasic3 = {
  */
 
 static struct resource gpio_vbus_resource = {
-	.flags = IORESOURCE_IRQ,
-	.start = IRQ_MAGICIAN_VBUS,
-	.end   = IRQ_MAGICIAN_VBUS,
+	.flags	= IORESOURCE_IRQ,
+	.start	= IRQ_MAGICIAN_VBUS,
+	.end	= IRQ_MAGICIAN_VBUS,
 };
 
 static struct gpio_vbus_mach_info gpio_vbus_info = {
 	.gpio_pullup = GPIO27_MAGICIAN_USBC_PUEN,
-	.gpio_vbus   = EGPIO_MAGICIAN_CABLE_STATE_USB,
+	.gpio_vbus = EGPIO_MAGICIAN_CABLE_STATE_USB,
 };
 
 static struct platform_device gpio_vbus = {
-	.name          = "gpio-vbus",
-	.id            = -1,
-	.num_resources = 1,
-	.resource      = &gpio_vbus_resource,
+	.name		= "gpio-vbus",
+	.id		= -1,
+	.num_resources	= 1,
+	.resource	= &gpio_vbus_resource,
 	.dev = {
 		.platform_data = &gpio_vbus_info,
 	},
@@ -541,38 +540,38 @@ static char *magician_supplicants[] = {
 };
 
 static struct pda_power_pdata power_supply_info = {
-	.init            = power_supply_init,
-	.is_ac_online    = magician_is_ac_online,
-	.exit            = power_supply_exit,
-	.supplied_to     = magician_supplicants,
+	.init		= power_supply_init,
+	.is_ac_online	= magician_is_ac_online,
+	.exit		= power_supply_exit,
+	.supplied_to	= magician_supplicants,
 	.num_supplicants = ARRAY_SIZE(magician_supplicants),
 };
 
 static struct resource power_supply_resources[] = {
 	[0] = {
-		.name  = "ac",
-		.flags = IORESOURCE_IRQ | IORESOURCE_IRQ_HIGHEDGE |
-		         IORESOURCE_IRQ_LOWEDGE,
-		.start = IRQ_MAGICIAN_VBUS,
-		.end   = IRQ_MAGICIAN_VBUS,
+		.name	= "ac",
+		.flags	= IORESOURCE_IRQ | IORESOURCE_IRQ_HIGHEDGE |
+			IORESOURCE_IRQ_LOWEDGE,
+		.start	= IRQ_MAGICIAN_VBUS,
+		.end	= IRQ_MAGICIAN_VBUS,
 	},
 	[1] = {
-		.name  = "usb",
-		.flags = IORESOURCE_IRQ | IORESOURCE_IRQ_HIGHEDGE |
-		         IORESOURCE_IRQ_LOWEDGE,
-		.start = IRQ_MAGICIAN_VBUS,
-		.end   = IRQ_MAGICIAN_VBUS,
+		.name	= "usb",
+		.flags	= IORESOURCE_IRQ | IORESOURCE_IRQ_HIGHEDGE |
+			IORESOURCE_IRQ_LOWEDGE,
+		.start	= IRQ_MAGICIAN_VBUS,
+		.end	= IRQ_MAGICIAN_VBUS,
 	},
 };
 
 static struct platform_device power_supply = {
-	.name = "pda-power",
-	.id   = -1,
-	.dev  = {
+	.name	= "pda-power",
+	.id	= -1,
+	.dev = {
 		.platform_data = &power_supply_info,
 	},
-	.resource      = power_supply_resources,
-	.num_resources = ARRAY_SIZE(power_supply_resources),
+	.resource	= power_supply_resources,
+	.num_resources	= ARRAY_SIZE(power_supply_resources),
 };
 
 /*
@@ -586,11 +585,12 @@ static struct regulator_consumer_supply bq24022_consumers[] = {
 
 static struct regulator_init_data bq24022_init_data = {
 	.constraints = {
-		.max_uA         = 500000,
-		.valid_ops_mask = REGULATOR_CHANGE_CURRENT | REGULATOR_CHANGE_STATUS,
+		.max_uA = 500000,
+		.valid_ops_mask = REGULATOR_CHANGE_CURRENT |
+			REGULATOR_CHANGE_STATUS,
 	},
-	.num_consumer_supplies  = ARRAY_SIZE(bq24022_consumers),
-	.consumer_supplies      = bq24022_consumers,
+	.num_consumer_supplies = ARRAY_SIZE(bq24022_consumers),
+	.consumer_supplies = bq24022_consumers,
 };
 
 static struct gpio bq24022_gpios[] = {
@@ -620,9 +620,9 @@ static struct gpio_regulator_config bq24022_info = {
 };
 
 static struct platform_device bq24022 = {
-	.name = "gpio-regulator",
-	.id   = -1,
-	.dev  = {
+	.name	= "gpio-regulator",
+	.id	= -1,
+	.dev	= {
 		.platform_data = &bq24022_info,
 	},
 };
@@ -632,10 +632,10 @@ static struct platform_device bq24022 = {
  */
 
 static int magician_mci_init(struct device *dev,
-				irq_handler_t detect_irq, void *data)
+	irq_handler_t detect_irq, void *data)
 {
 	return request_irq(IRQ_MAGICIAN_SD, detect_irq, 0,
-			   "mmc card detect", data);
+		"mmc card detect", data);
 }
 
 static void magician_mci_exit(struct device *dev, void *data)
@@ -644,13 +644,13 @@ static void magician_mci_exit(struct device *dev, void *data)
 }
 
 static struct pxamci_platform_data magician_mci_info = {
-	.ocr_mask 		= MMC_VDD_32_33|MMC_VDD_33_34,
-	.init     		= magician_mci_init,
-	.exit     		= magician_mci_exit,
+	.ocr_mask	= MMC_VDD_32_33|MMC_VDD_33_34,
+	.init		= magician_mci_init,
+	.exit		= magician_mci_exit,
 	.gpio_card_detect	= -1,
 	.gpio_card_ro		= EGPIO_MAGICIAN_nSD_READONLY,
 	.gpio_card_ro_invert	= 1,
-	.gpio_power		= EGPIO_MAGICIAN_SD_POWER,
+	.gpio_power	= EGPIO_MAGICIAN_SD_POWER,
 };
 
 
@@ -664,7 +664,6 @@ static struct pxaohci_platform_data magician_ohci_info = {
 	.power_budget	= 0,
 };
 
-
 /*
  * StrataFlash
  */
@@ -675,9 +674,9 @@ static void magician_set_vpp(struct platform_device *pdev, int vpp)
 }
 
 static struct resource strataflash_resource = {
-	.start = PXA_CS0_PHYS,
-	.end   = PXA_CS0_PHYS + SZ_64M - 1,
-	.flags = IORESOURCE_MEM,
+	.start	= PXA_CS0_PHYS,
+	.end	= PXA_CS0_PHYS + SZ_64M - 1,
+	.flags	= IORESOURCE_MEM,
 };
 
 static struct physmap_flash_data strataflash_data = {
@@ -686,10 +685,10 @@ static struct physmap_flash_data strataflash_data = {
 };
 
 static struct platform_device strataflash = {
-	.name          = "physmap-flash",
-	.id            = -1,
-	.resource      = &strataflash_resource,
-	.num_resources = 1,
+	.name		= "physmap-flash",
+	.id		= -1,
+	.resource	= &strataflash_resource,
+	.num_resources	= 1,
 	.dev = {
 		.platform_data = &strataflash_data,
 	},
@@ -720,12 +719,12 @@ static struct platform_device *devices[] __initdata = {
 };
 
 static struct gpio magician_global_gpios[] = {
-	{ GPIO13_MAGICIAN_CPLD_IRQ,   GPIOF_IN, "CPLD_IRQ" },
-	{ GPIO107_MAGICIAN_DS1WM_IRQ, GPIOF_IN, "DS1WM_IRQ" },
+	{ GPIO13_MAGICIAN_CPLD_IRQ, GPIOF_IN,		"CPLD_IRQ" },
+	{ GPIO107_MAGICIAN_DS1WM_IRQ, GPIOF_IN,	"DS1WM_IRQ" },
 	{ GPIO104_MAGICIAN_LCD_POWER_1, GPIOF_OUT_INIT_LOW, "LCD power 1" },
 	{ GPIO105_MAGICIAN_LCD_POWER_2, GPIOF_OUT_INIT_LOW, "LCD power 2" },
 	{ GPIO106_MAGICIAN_LCD_POWER_3, GPIOF_OUT_INIT_LOW, "LCD power 3" },
-	{ GPIO83_MAGICIAN_nIR_EN, GPIOF_OUT_INIT_HIGH, "nIR_EN" },
+	{ GPIO83_MAGICIAN_nIR_EN, GPIOF_OUT_INIT_HIGH,	"nIR_EN" },
 };
 
 static void __init magician_init(void)
@@ -754,27 +753,28 @@ static void __init magician_init(void)
 	/* Check LCD type we have */
 	cpld = ioremap_nocache(PXA_CS3_PHYS, 0x1000);
 	if (cpld) {
-		u8 board_id = __raw_readb(cpld+0x14);
+		u8 board_id = __raw_readb(cpld + 0x14);
+
 		iounmap(cpld);
 		system_rev = board_id & 0x7;
 		lcd_select = board_id & 0x8;
 		pr_info("LCD type: %s\n", lcd_select ? "Samsung" : "Toppoly");
 		if (lcd_select && (system_rev < 3))
 			gpio_request_one(GPIO75_MAGICIAN_SAMSUNG_POWER,
-			                 GPIOF_OUT_INIT_LOW, "SAMSUNG_POWER");
-		pxa_set_fb_info(NULL, lcd_select ? &samsung_info : &toppoly_info);
+				GPIOF_OUT_INIT_LOW, "SAMSUNG_POWER");
+		pxa_set_fb_info(NULL,
+			lcd_select ? &samsung_info : &toppoly_info);
 	} else
 		pr_err("LCD detection: CPLD mapping failed\n");
 }
 
-
 MACHINE_START(MAGICIAN, "HTC Magician")
-	.atag_offset = 0x100,
-	.map_io = pxa27x_map_io,
-	.nr_irqs = MAGICIAN_NR_IRQS,
-	.init_irq = pxa27x_init_irq,
-	.handle_irq = pxa27x_handle_irq,
-	.init_machine = magician_init,
+	.atag_offset	= 0x100,
+	.map_io		= pxa27x_map_io,
+	.nr_irqs	= MAGICIAN_NR_IRQS,
+	.init_irq	= pxa27x_init_irq,
+	.handle_irq	= pxa27x_handle_irq,
+	.init_machine	= magician_init,
 	.init_time	= pxa_timer_init,
 	.restart	= pxa_restart,
 MACHINE_END
-- 
1.7.12.1

[PATCH v2 02/21] ARM: pxa: magician: Fix indentation and whitespaces

[Robert Jarzmik]

Petr Cvek <petr.cvek@tul.cz> writes:

> Fix magician.c: indentation and whitespaces.
>
> Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
Unless somebody finds a flaw within the next 2 weeks, I'll queue that up.

Cheers.

--
Robert

[PATCH v2 02/21] ARM: pxa: magician: Fix indentation and whitespaces

[Philipp Zabel]

Am Montag, den 17.08.2015, 23:56 +0200 schrieb Petr Cvek:
> Fix magician.c: indentation and whitespaces.
> 
> Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
> ---
>  arch/arm/mach-pxa/magician.c | 356 +++++++++++++++++++++------------
> ----------
>  1 file changed, 178 insertions(+), 178 deletions(-)
> 
> diff --git a/arch/arm/mach-pxa/magician.c b/arch/arm/mach
> -pxa/magician.c
> index a9761c2..9e8698a 100644
> --- a/arch/arm/mach-pxa/magician.c
> +++ b/arch/arm/mach-pxa/magician.c
> @@ -52,8 +52,8 @@ static unsigned long magician_pin_config[] 
> __initdata = {
>  	GPIO20_nSDCS_2,
>  	GPIO21_nSDCS_3,
>  	GPIO15_nCS_1,
> -	GPIO78_nCS_2,   /* PASIC3 */
> -	GPIO79_nCS_3,   /* EGPIO CPLD */
> +	GPIO78_nCS_2,	/* PASIC3 */
> +	GPIO79_nCS_3,	/* EGPIO CPLD */
>  	GPIO80_nCS_4,
>  	GPIO33_nCS_5,
>  
> @@ -107,7 +107,7 @@ static unsigned long magician_pin_config[] 


Yay.

> __initdata = {
>  	GPIO85_CIF_LV,
>  
>  	/* Magician specific input GPIOs */
> -	GPIO9_GPIO,	/* unknown */
> +	GPIO9_GPIO,		/* unknown */
>  	GPIO10_GPIO,	/* GSM_IRQ */
>  	GPIO13_GPIO,	/* CPLD_IRQ */
>  	GPIO107_GPIO,	/* DS1WM_IRQ */

Nay. Are you using 8-space tabs?

> @@ -133,46 +133,45 @@ static struct pxaficp_platform_data 
> magician_ficp_info = {
>   */
>  
>  #define INIT_KEY(_code, _gpio, _desc)	\
> -	{				\
> -		.code   = KEY_##_code,	\
> -		.gpio   = _gpio,	\
> -		.desc   = _desc,	\
> -		.type   = EV_KEY,	\
> -		.wakeup = 1,		\
> +	{						\
> +		.code	= KEY_##_code,	\
> +		.gpio	= _gpio,	\
> +		.desc	= _desc,	\
> +		.type	= EV_KEY,	\
> +		.wakeup	= 1,		\
>  	}

Ok.

>  static struct gpio_keys_button magician_button_table[] = {
> -	INIT_KEY(POWER,      GPIO0_MAGICIAN_KEY_POWER,      "Power 
> button"),
> -	INIT_KEY(ESC,        GPIO37_MAGICIAN_KEY_HANGUP,    "Hangup 
> button"),
> -	INIT_KEY(F10,        GPIO38_MAGICIAN_KEY_CONTACTS, 
>  "Contacts button"),
> -	INIT_KEY(CALENDAR,   GPIO90_MAGICIAN_KEY_CALENDAR, 
>  "Calendar button"),
> -	INIT_KEY(CAMERA,     GPIO91_MAGICIAN_KEY_CAMERA,    "Camera 
> button"),
> -	INIT_KEY(UP,         GPIO93_MAGICIAN_KEY_UP,        "Up 
> button"),
> -	INIT_KEY(DOWN,       GPIO94_MAGICIAN_KEY_DOWN,      "Down 
> button"),
> -	INIT_KEY(LEFT,       GPIO95_MAGICIAN_KEY_LEFT,      "Left 
> button"),
> -	INIT_KEY(RIGHT,      GPIO96_MAGICIAN_KEY_RIGHT,     "Right 
> button"),
> -	INIT_KEY(KPENTER,    GPIO97_MAGICIAN_KEY_ENTER,     "Action 
> button"),
> -	INIT_KEY(RECORD,     GPIO98_MAGICIAN_KEY_RECORD,    "Record 
> button"),
> -	INIT_KEY(VOLUMEUP,   GPIO100_MAGICIAN_KEY_VOL_UP,   "Volume 
> up"),
> -	INIT_KEY(VOLUMEDOWN, GPIO101_MAGICIAN_KEY_VOL_DOWN, "Volume 
> down"),
> -	INIT_KEY(PHONE,      GPIO102_MAGICIAN_KEY_PHONE,    "Phone 
> button"),
> -	INIT_KEY(PLAY,       GPIO99_MAGICIAN_HEADPHONE_IN,  "Headset 
> button"),
> +	INIT_KEY(POWER,		GPIO0_MAGICIAN_KEY_POWER,	> "Power 
> button"),
> +	INIT_KEY(ESC,		GPIO37_MAGICIAN_KEY_HANGUP,	"Han
> gup button"),
> +	INIT_KEY(F10,		GPIO38_MAGICIAN_KEY_CONTACTS,	"Con
> tacts button"),
> +	INIT_KEY(CALENDAR,	GPIO90_MAGICIAN_KEY_CALENDAR,	"Cal
> endar button"),
> +	INIT_KEY(CAMERA,	GPIO91_MAGICIAN_KEY_CAMERA,	"Cam
> era button"),
> +	INIT_KEY(UP,		GPIO93_MAGICIAN_KEY_UP,		> "Up 
> button"),
> +	INIT_KEY(DOWN,		GPIO94_MAGICIAN_KEY_DOWN,	"Dow
> n button"),
> +	INIT_KEY(LEFT,		GPIO95_MAGICIAN_KEY_LEFT,	"Lef
> t button"),
> +	INIT_KEY(RIGHT,		GPIO96_MAGICIAN_KEY_RIGHT,	> "Right 
> button"),
> +	INIT_KEY(KPENTER,	GPIO97_MAGICIAN_KEY_ENTER,	"Act
> ion button"),
> +	INIT_KEY(RECORD,	GPIO98_MAGICIAN_KEY_RECORD,	"Rec
> ord button"),
> +	INIT_KEY(VOLUMEUP,	GPIO100_MAGICIAN_KEY_VOL_UP,	"Vol
> ume up"),
> +	INIT_KEY(VOLUMEDOWN,	GPIO101_MAGICIAN_KEY_VOL_DOWN,	"Vol
> ume down"),
> +	INIT_KEY(PHONE,		GPIO102_MAGICIAN_KEY_PHONE,	> "Phone 
> button"),
> +	INIT_KEY(PLAY,		GPIO99_MAGICIAN_HEADPHONE_IN,	"Hea
> dset button"),
>  };

I don't like this. The tabs cause long lines and it's not like we
expect additional keys to appear that would warrant the reformatting.

>   static struct gpio_keys_platform_data gpio_keys_data = {
> -	.buttons  = magician_button_table,
> +	.buttons = magician_button_table,
>  	.nbuttons = ARRAY_SIZE(magician_button_table),
>  };
>  
>  static struct platform_device gpio_keys = {
>  	.name = "gpio-keys",
> -	.dev  = {
> +	.dev = {
>  		.platform_data = &gpio_keys_data,
>  	},
> -	.id   = -1,
> +	.id = -1,
>  };

Hm, so are we aligning at =, or are we not?


>  /*
>   * EGPIO (Xilinx CPLD)
>   *
> @@ -181,14 +180,14 @@ static struct platform_device gpio_keys = {
>  
>  static struct resource egpio_resources[] = {
>  	[0] = {
> -		.start = PXA_CS3_PHYS,
> -		.end   = PXA_CS3_PHYS + 0x20 - 1,
> -		.flags = IORESOURCE_MEM,
> +		.start	= PXA_CS3_PHYS,
> +		.end	= PXA_CS3_PHYS + 0x20 - 1,
> +		.flags	= IORESOURCE_MEM,
>  	},
>  	[1] = {
> -		.start = PXA_GPIO_TO_IRQ(GPIO13_MAGICIAN_CPLD_IRQ),
> -		.end   = PXA_GPIO_TO_IRQ(GPIO13_MAGICIAN_CPLD_IRQ),
> -		.flags = IORESOURCE_IRQ,
> +		.start	= PXA_GPIO_TO_IRQ(GPIO13_MAGICIAN_CPLD_IRQ),
> +		.end	= PXA_GPIO_TO_IRQ(GPIO13_MAGICIAN_CPLD_IRQ),
> +		.flags	= IORESOURCE_IRQ,
>  	},
>  };

I'm not a fan of reformatting unless there's a reason (like the mixture
of space and tab warnings). But as long as this doesn't introduce new
issues like long lines, and as long as it's done consequently, I'm fine
with this.

> @@ -209,20 +208,20 @@ static struct htc_egpio_chip egpio_chips[] = {
>  };
>  
>  static struct htc_egpio_platform_data egpio_info = {
> -	.reg_width    = 8,
> -	.bus_width    = 32,
> -	.irq_base     = IRQ_BOARD_START,
> -	.num_irqs     = 4,
> -	.ack_register = 3,
> -	.chip         = egpio_chips,
> -	.num_chips    = ARRAY_SIZE(egpio_chips),
> +	.reg_width	= 8,
> +	.bus_width	= 32,
> +	.irq_base	= IRQ_BOARD_START,
> +	.num_irqs	= 4,
> +	.ack_register	= 3,
> +	.chip		= egpio_chips,
> +	.num_chips	= ARRAY_SIZE(egpio_chips),
>  };
>  
>  static struct platform_device egpio = {
> -	.name          = "htc-egpio",
> -	.id            = -1,
> -	.resource      = egpio_resources,
> -	.num_resources = ARRAY_SIZE(egpio_resources),
> +	.name		= "htc-egpio",
> +	.id		= -1,
> +	.resource	= egpio_resources,
> +	.num_resources	= ARRAY_SIZE(egpio_resources),
>  	.dev = {
>  		.platform_data = &egpio_info,
>  	},
> @@ -234,33 +233,33 @@ static struct platform_device egpio = {
>  
>  static struct pxafb_mode_info toppoly_modes[] = {
>  	{
> -		.pixclock     = 96153,
> -		.bpp          = 16,
> -		.xres         = 240,
> -		.yres         = 320,
> -		.hsync_len    = 11,
> -		.vsync_len    = 3,
> -		.left_margin  = 19,
> -		.upper_margin = 2,
> -		.right_margin = 10,
> -		.lower_margin = 2,
> -		.sync         = 0,
> +		.pixclock	= 96153,
> +		.bpp		= 16,
> +		.xres		= 240,
> +		.yres		= 320,
> +		.hsync_len	= 11,
> +		.vsync_len	= 3,
> +		.left_margin	= 19,
> +		.upper_margin	= 2,
> +		.right_margin	= 10,
> +		.lower_margin	= 2,
> +		.sync		= 0,
>  	},
>  };
>  
>  static struct pxafb_mode_info samsung_modes[] = {
>  	{
> -		.pixclock     = 96153,
> -		.bpp          = 16,
> -		.xres         = 240,
> -		.yres         = 320,
> -		.hsync_len    = 8,
> -		.vsync_len    = 4,
> -		.left_margin  = 9,
> -		.upper_margin = 4,
> -		.right_margin = 9,
> -		.lower_margin = 4,
> -		.sync         = FB_SYNC_HOR_HIGH_ACT | 
> FB_SYNC_VERT_HIGH_ACT,
> +		.pixclock	= 96153,
> +		.bpp		= 16,
> +		.xres		= 240,
> +		.yres		= 320,
> +		.hsync_len	= 8,
> +		.vsync_len	= 4,
> +		.left_margin	= 9,
> +		.upper_margin	= 4,
> +		.right_margin	= 9,
> +		.lower_margin	= 4,
> +		.sync	= FB_SYNC_HOR_HIGH_ACT | 
> FB_SYNC_VERT_HIGH_ACT,
>  	},
>  };
>  
> @@ -326,19 +325,19 @@ static void samsung_lcd_power(int on, struct 
> fb_var_screeninfo *si)
>  }
>  
>  static struct pxafb_mach_info toppoly_info = {
> -	.modes           = toppoly_modes,
> -	.num_modes       = 1,
> -	.fixed_modes     = 1,
> +	.modes		= toppoly_modes,
> +	.num_modes	= 1,
> +	.fixed_modes	= 1,
>  	.lcd_conn	= LCD_COLOR_TFT_16BPP,
>  	.pxafb_lcd_power = toppoly_lcd_power,
>  };
>  
>  static struct pxafb_mach_info samsung_info = {
> -	.modes           = samsung_modes,
> -	.num_modes       = 1,
> -	.fixed_modes     = 1,
> -	.lcd_conn	 = LCD_COLOR_TFT_16BPP | LCD_PCLK_EDGE_FALL 
> |\
> -			   LCD_ALTERNATE_MAPPING,
> +	.modes		= samsung_modes,
> +	.num_modes	= 1,
> +	.fixed_modes	= 1,
> +	.lcd_conn	= LCD_COLOR_TFT_16BPP | LCD_PCLK_EDGE_FALL |
> +		LCD_ALTERNATE_MAPPING,
>  	.pxafb_lcd_power = samsung_lcd_power,
>  };
>  
> @@ -374,22 +373,22 @@ static void magician_backlight_exit(struct 
> device *dev)
>  }
>  
>  static struct platform_pwm_backlight_data backlight_data = {
> -	.pwm_id         = 0,
> -	.max_brightness = 272,
> -	.dft_brightness = 100,
> -	.pwm_period_ns  = 30923,
> -	.enable_gpio    = -1,
> -	.init           = magician_backlight_init,
> -	.notify         = magician_backlight_notify,
> -	.exit           = magician_backlight_exit,
> +	.pwm_id		= 0,
> +	.max_brightness	= 272,
> +	.dft_brightness	= 100,
> +	.pwm_period_ns	= 30923,
> +	.enable_gpio	= -1,
> +	.init		= magician_backlight_init,
> +	.notify		= magician_backlight_notify,
> +	.exit		= magician_backlight_exit,
>  };
>  
>  static struct platform_device backlight = {
> -	.name = "pwm-backlight",
> -	.id   = -1,
> -	.dev  = {
> -		.parent        = &pxa27x_device_pwm0.dev,
> -		.platform_data = &backlight_data,
> +	.name	= "pwm-backlight",
> +	.id	= -1,
> +	.dev	= {
> +		.parent		= &pxa27x_device_pwm0.dev,
> +		.platform_data	= &backlight_data,
>  	},
>  };

So we are aligning, then. Please pick a style and stick with it.
I'll skip the rest for now.

regards
Philipp

[PATCH v2 03/21] ARM: pxa: magician: Fix comments, debug functions and print strings

[Petr Cvek]

Fix comments, debug functions and print strings. Rename backlight
structures to be more specific.

Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
---
 arch/arm/mach-pxa/magician.c | 121 +++++++++++++++++++++++++++++++------------
 1 file changed, 89 insertions(+), 32 deletions(-)

diff --git a/arch/arm/mach-pxa/magician.c b/arch/arm/mach-pxa/magician.c
index 9e8698a..57da133 100644
--- a/arch/arm/mach-pxa/magician.c
+++ b/arch/arm/mach-pxa/magician.c
@@ -11,6 +11,46 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  *
+ *
+ * NOTICE MDA Compact (T-mobile XDA) facts:
+ *   On "LCD type = 1, system_rev = 2" (0x3a in CPLD)
+ * Samsung LTP280QV - valid LCD init sequence sequence:
+ *   powerup: Vdigital->Vanalog->gate Voff->gate Von->data enable
+ *   powerdown: data disable->gate Von->gate Voff->Vanalog->Vdigital
+ * Measured on PCB:
+ *   GPIO106
+ *     Affects VOFF, VON and other voltages
+ *     Probably main reset pin for DC-DC converter
+ *   GPIO75
+ *     Must be AF0+OUT (WM leaves it to AF2+OUT), not LCD signal
+ *     GPIO106 works only when GPIO75 is high (DC-DC power enable?)
+ *     After LCD powerup, value is irrelevant
+ *   GPIO104
+ *     LCD VOFF (gate off voltage)
+ *   GPIO105
+ *     LCD VON (gate on voltage)
+ * FFUART (/dev/ttyS0) WM: 38400,8,n,1,crtscts (GSM data?)
+ * BTUART (/dev/ttyS1) WM: 115200,8,n,1,crtscts (GSM AT commands)
+ *   Use a HTC line discipline: 0x02 channel(==0x16) data 0x02
+ * EGPIO(CPLD) should be always present (controls "all" supply power)
+ *   For rootfs on MMC/SD: EGPIO module controls card power (in kernel)
+ * cpu-freq often freeze platform (errata?, use userspace gov)
+ * Do not use YUV420, (erratum E24) causes LCD hang (forever)
+ * Many GSM related pins are unknown
+ * gpio-rc-recv probably require lowpass filter (sw is OK)
+ * TODO EGPIO_MAGICIAN_IR_RECEIVE_SHUTDOWN
+ * FIXME AC charging blocks IrDA
+ * Unimplemented blocking of camera+power+reset, reset and door open reset
+ *   Registers: htc-pasic3.h (but blocks charging too)
+ * Other PXA machines have wait_for_sync for ADS7846 (during LCD refresh)
+ * pda-power has not optimal charging (accu can be bloated)
+ * TODO Current switching works? Is gpio-regulator/bq24022 required?
+ * pdata supports alternative drivers (mutually exclusive modules)
+ *   i2c-pxa vs i2c-gpio (SCCB)
+ *   pwm_bl vs gpio_backlight
+ *   pxa27x_udc vs phy-gpio-vbus-usb
+ *   physmap-flash vs pxa2xx-flash
+ *
  */
 
 #include <linux/kernel.h>
@@ -57,31 +97,31 @@ static unsigned long magician_pin_config[] __initdata = {
 	GPIO80_nCS_4,
 	GPIO33_nCS_5,
 
-	/* I2C */
+	/* I2C UDA1380 + OV9640 */
 	GPIO117_I2C_SCL,
 	GPIO118_I2C_SDA,
 
-	/* PWM 0 */
+	/* PWM 0 - LCD backlight */
 	GPIO16_PWM0_OUT,
 
-	/* I2S */
+	/* I2S UDA1380 capture */
 	GPIO28_I2S_BITCLK_OUT,
 	GPIO29_I2S_SDATA_IN,
 	GPIO31_I2S_SYNC,
 	GPIO113_I2S_SYSCLK,
 
-	/* SSP 1 */
+	/* SSP 1 UDA1380 playback */
 	GPIO23_SSP1_SCLK,
 	GPIO24_SSP1_SFRM,
 	GPIO25_SSP1_TXD,
 
-	/* SSP 2 */
+	/* SSP 2 TSC2046 touchscreen */
 	GPIO19_SSP2_SCLK,
 	GPIO14_SSP2_SFRM,
 	GPIO89_SSP2_TXD,
 	GPIO88_SSP2_RXD,
 
-	/* MMC */
+	/* MMC/SD/SDHC slot */
 	GPIO32_MMC_CLK,
 	GPIO92_MMC_DAT_0,
 	GPIO109_MMC_DAT_1,
@@ -92,7 +132,7 @@ static unsigned long magician_pin_config[] __initdata = {
 	/* LCD */
 	GPIOxx_LCD_TFT_16BPP,
 
-	/* QCI */
+	/* QCI camera interface */
 	GPIO12_CIF_DD_7,
 	GPIO17_CIF_DD_6,
 	GPIO50_CIF_DD_3,
@@ -120,11 +160,12 @@ static unsigned long magician_pin_config[] __initdata = {
 };
 
 /*
- * IRDA
+ * IrDA
  */
 
 static struct pxaficp_platform_data magician_ficp_info = {
 	.gpio_pwdown		= GPIO83_MAGICIAN_nIR_EN,
+	/* TODO test with FIR dongle */
 	.transceiver_cap	= IR_SIRMODE | IR_OFF,
 };
 
@@ -174,8 +215,8 @@ static struct platform_device gpio_keys = {
 
 /*
  * EGPIO (Xilinx CPLD)
- *
- * 7 32-bit aligned 8-bit registers: 3x output, 1x irq, 3x input
+ * 32-bit aligned
+ * 7x 8-bit registers: 3x output, 1x irq, 3x input
  */
 
 static struct resource egpio_resources[] = {
@@ -197,7 +238,11 @@ static struct htc_egpio_chip egpio_chips[] = {
 		.gpio_base = MAGICIAN_EGPIO(0, 0),
 		.num_gpios = 24,
 		.direction = HTC_EGPIO_OUTPUT,
-		.initial_values = 0x40, /* EGPIO_MAGICIAN_GSM_RESET */
+
+		/*
+		 * Depends on modules configuration
+		 */
+		.initial_values = 0x40,
 	},
 	[1] = {
 		.reg_start = 4,
@@ -228,7 +273,7 @@ static struct platform_device egpio = {
 };
 
 /*
- * LCD - Toppoly TD028STEB1 or Samsung LTP280QV
+ * PXAFB LCD - Toppoly TD028STEB1 or Samsung LTP280QV
  */
 
 static struct pxafb_mode_info toppoly_modes[] = {
@@ -265,10 +310,9 @@ static struct pxafb_mode_info samsung_modes[] = {
 
 static void toppoly_lcd_power(int on, struct fb_var_screeninfo *si)
 {
-	pr_debug("Toppoly LCD power\n");
+	pr_debug("Toppoly LCD power: %s\n", on ? "on" : "off");
 
 	if (on) {
-		pr_debug("on\n");
 		gpio_set_value(EGPIO_MAGICIAN_TOPPOLY_POWER, 1);
 		gpio_set_value(GPIO106_MAGICIAN_LCD_POWER_3, 1);
 		udelay(2000);
@@ -280,8 +324,7 @@ static void toppoly_lcd_power(int on, struct fb_var_screeninfo *si)
 		udelay(2000);
 		gpio_set_value(GPIO105_MAGICIAN_LCD_POWER_2, 1);
 	} else {
-		pr_debug("off\n");
-		msleep(15);
+		mdelay(15);
 		gpio_set_value(GPIO105_MAGICIAN_LCD_POWER_2, 0);
 		udelay(500);
 		gpio_set_value(GPIO104_MAGICIAN_LCD_POWER_1, 0);
@@ -293,10 +336,9 @@ static void toppoly_lcd_power(int on, struct fb_var_screeninfo *si)
 
 static void samsung_lcd_power(int on, struct fb_var_screeninfo *si)
 {
-	pr_debug("Samsung LCD power\n");
+	pr_debug("Samsung LCD power: %s\n", on ? "on" : "off");
 
 	if (on) {
-		pr_debug("on\n");
 		if (system_rev < 3)
 			gpio_set_value(GPIO75_MAGICIAN_SAMSUNG_POWER, 1);
 		else
@@ -309,7 +351,6 @@ static void samsung_lcd_power(int on, struct fb_var_screeninfo *si)
 		gpio_set_value(GPIO105_MAGICIAN_LCD_POWER_2, 1);
 		mdelay(10);
 	} else {
-		pr_debug("off\n");
 		mdelay(10);
 		gpio_set_value(GPIO105_MAGICIAN_LCD_POWER_2, 0);
 		mdelay(30);
@@ -346,8 +387,8 @@ static struct pxafb_mach_info samsung_info = {
  */
 
 static struct gpio magician_bl_gpios[] = {
-	{ EGPIO_MAGICIAN_BL_POWER,  GPIOF_DIR_OUT, "Backlight power" },
-	{ EGPIO_MAGICIAN_BL_POWER2, GPIOF_DIR_OUT, "Backlight power 2" },
+	{ EGPIO_MAGICIAN_BL_POWER, GPIOF_DIR_OUT, "Backlight power" },
+	{ EGPIO_MAGICIAN_BL_POWER2, GPIOF_DIR_OUT, "Extra backlight power" },
 };
 
 static int magician_backlight_init(struct device *dev)
@@ -357,6 +398,7 @@ static int magician_backlight_init(struct device *dev)
 
 static int magician_backlight_notify(struct device *dev, int brightness)
 {
+	pr_debug("Brightness = %i\n", brightness);
 	gpio_set_value(EGPIO_MAGICIAN_BL_POWER, brightness);
 	if (brightness >= 200) {
 		gpio_set_value(EGPIO_MAGICIAN_BL_POWER2, 1);
@@ -372,7 +414,13 @@ static void magician_backlight_exit(struct device *dev)
 	gpio_free_array(ARRAY_AND_SIZE(magician_bl_gpios));
 }
 
-static struct platform_pwm_backlight_data backlight_data = {
+/*
+ * LCD backlight by PWM (main)
+ * PWM frequency for MP1521 should be:
+ *   100-400 Hz = 2?.5*10^6 - 10?*10^6 ns
+ */
+
+static struct platform_pwm_backlight_data pwm_backlight_data = {
 	.pwm_id		= 0,
 	.max_brightness	= 272,
 	.dft_brightness	= 100,
@@ -383,12 +431,12 @@ static struct platform_pwm_backlight_data backlight_data = {
 	.exit		= magician_backlight_exit,
 };
 
-static struct platform_device backlight = {
+static struct platform_device pwm_backlight = {
 	.name	= "pwm-backlight",
 	.id	= -1,
 	.dev	= {
 		.parent		= &pxa27x_device_pwm0.dev,
-		.platform_data	= &backlight_data,
+		.platform_data	= &pwm_backlight_data,
 	},
 };
 
@@ -422,6 +470,10 @@ static struct platform_device leds_gpio = {
 	},
 };
 
+/*
+ * PASIC3 LEDs
+ */
+
 static struct pasic3_led pasic3_leds[] = {
 	{
 		.led = {
@@ -459,7 +511,7 @@ static struct pasic3_leds_machinfo pasic3_leds_info = {
 };
 
 /*
- * PASIC3 with DS1WM
+ * PASIC3 DS1WM
  */
 
 static struct resource pasic3_resources[] = {
@@ -477,8 +529,8 @@ static struct resource pasic3_resources[] = {
 };
 
 static struct pasic3_platform_data pasic3_platform_data = {
-	.led_pdata	= &pasic3_leds_info,
 	.clock_rate	= 4000000,
+	.led_pdata	= &pasic3_leds_info,
 };
 
 static struct platform_device pasic3 = {
@@ -517,7 +569,7 @@ static struct platform_device gpio_vbus = {
 };
 
 /*
- * External power
+ * Charging functions
  */
 
 static int power_supply_init(struct device *dev)
@@ -539,6 +591,10 @@ static char *magician_supplicants[] = {
 	"ds2760-battery.0", "backup-battery"
 };
 
+/*
+ * pda_power Li-ion charger
+ */
+
 static struct pda_power_pdata power_supply_info = {
 	.init		= power_supply_init,
 	.is_ac_online	= magician_is_ac_online,
@@ -575,7 +631,7 @@ static struct platform_device power_supply = {
 };
 
 /*
- * Battery charger
+ * Charging current switch
  */
 
 static struct regulator_consumer_supply bq24022_consumers[] = {
@@ -695,7 +751,7 @@ static struct platform_device strataflash = {
 };
 
 /*
- * I2C
+ * PXA I2C normal controller (main)
  */
 
 static struct i2c_pxa_platform_data i2c_info = {
@@ -709,7 +765,7 @@ static struct i2c_pxa_platform_data i2c_info = {
 static struct platform_device *devices[] __initdata = {
 	&gpio_keys,
 	&egpio,
-	&backlight,
+	&pwm_backlight,
 	&pasic3,
 	&bq24022,
 	&gpio_vbus,
@@ -734,9 +790,10 @@ static void __init magician_init(void)
 	int err;
 
 	pxa2xx_mfp_config(ARRAY_AND_SIZE(magician_pin_config));
+
 	err = gpio_request_array(ARRAY_AND_SIZE(magician_global_gpios));
 	if (err)
-		pr_err("magician: Failed to request GPIOs: %d\n", err);
+		pr_err("magician: Failed to request global GPIOs: %d\n", err);
 
 	pxa_set_ffuart_info(NULL);
 	pxa_set_btuart_info(NULL);
@@ -761,7 +818,7 @@ static void __init magician_init(void)
 		pr_info("LCD type: %s\n", lcd_select ? "Samsung" : "Toppoly");
 		if (lcd_select && (system_rev < 3))
 			gpio_request_one(GPIO75_MAGICIAN_SAMSUNG_POWER,
-				GPIOF_OUT_INIT_LOW, "SAMSUNG_POWER");
+				GPIOF_OUT_INIT_LOW, "Samsung LCD Power");
 		pxa_set_fb_info(NULL,
 			lcd_select ? &samsung_info : &toppoly_info);
 	} else
-- 
1.7.12.1

[PATCH v2 03/21] ARM: pxa: magician: Fix comments, debug functions and print strings

[Robert Jarzmik]

Petr Cvek <petr.cvek@tul.cz> writes:

> Fix comments, debug functions and print strings. Rename backlight
> structures to be more specific.
>
> Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
> ---
>  arch/arm/mach-pxa/magician.c | 121 +++++++++++++++++++++++++++++++------------
>  1 file changed, 89 insertions(+), 32 deletions(-)
>
> diff --git a/arch/arm/mach-pxa/magician.c b/arch/arm/mach-pxa/magician.c
> index 9e8698a..57da133 100644
> --- a/arch/arm/mach-pxa/magician.c
> +++ b/arch/arm/mach-pxa/magician.c
> @@ -11,6 +11,46 @@
>   * it under the terms of the GNU General Public License version 2 as
>   * published by the Free Software Foundation.
>   *
> + *
> + * NOTICE MDA Compact (T-mobile XDA) facts:
> + *   On "LCD type = 1, system_rev = 2" (0x3a in CPLD)
> + * Samsung LTP280QV - valid LCD init sequence sequence:
> + *   powerup: Vdigital->Vanalog->gate Voff->gate Von->data enable
> + *   powerdown: data disable->gate Von->gate Voff->Vanalog->Vdigital
> + * Measured on PCB:
> + *   GPIO106
> + *     Affects VOFF, VON and other voltages
> + *     Probably main reset pin for DC-DC converter
> + *   GPIO75
> + *     Must be AF0+OUT (WM leaves it to AF2+OUT), not LCD signal
> + *     GPIO106 works only when GPIO75 is high (DC-DC power enable?)
> + *     After LCD powerup, value is irrelevant
> + *   GPIO104
> + *     LCD VOFF (gate off voltage)
> + *   GPIO105
> + *     LCD VON (gate on voltage)
> + * FFUART (/dev/ttyS0) WM: 38400,8,n,1,crtscts (GSM data?)
> + * BTUART (/dev/ttyS1) WM: 115200,8,n,1,crtscts (GSM AT commands)
> + *   Use a HTC line discipline: 0x02 channel(==0x16) data 0x02
> + * EGPIO(CPLD) should be always present (controls "all" supply power)
> + *   For rootfs on MMC/SD: EGPIO module controls card power (in kernel)
> + * cpu-freq often freeze platform (errata?, use userspace gov)
> + * Do not use YUV420, (erratum E24) causes LCD hang (forever)
> + * Many GSM related pins are unknown
> + * gpio-rc-recv probably require lowpass filter (sw is OK)
> + * TODO EGPIO_MAGICIAN_IR_RECEIVE_SHUTDOWN
> + * FIXME AC charging blocks IrDA
> + * Unimplemented blocking of camera+power+reset, reset and door open reset
> + *   Registers: htc-pasic3.h (but blocks charging too)
> + * Other PXA machines have wait_for_sync for ADS7846 (during LCD refresh)
> + * pda-power has not optimal charging (accu can be bloated)
> + * TODO Current switching works? Is gpio-regulator/bq24022 required?
I think you can ask Philip, he's the maintainer after all ...

> + * pdata supports alternative drivers (mutually exclusive modules)
> + *   i2c-pxa vs i2c-gpio (SCCB)
> + *   pwm_bl vs gpio_backlight
> + *   pxa27x_udc vs phy-gpio-vbus-usb
> + *   physmap-flash vs pxa2xx-flash

Okay, this chunk is nice documentation, but not code ...
I'd rather have it in : Documentation/arm/pxa/magician.txt or something like
that.

> @@ -120,11 +160,12 @@ static unsigned long magician_pin_config[] __initdata = {
>  };
>  
>  /*
> - * IRDA
> + * IrDA
>   */
>  
>  static struct pxaficp_platform_data magician_ficp_info = {
>  	.gpio_pwdown		= GPIO83_MAGICIAN_nIR_EN,
> +	/* TODO test with FIR dongle */
I don't like this comment. I was not there before, so let it be. You know there
remains a TODO, I know ... but you can cunningly slip it in the commit message.

Cheers.

-- 
Robert

[PATCH v2 03/21] ARM: pxa: magician: Fix comments, debug functions and print strings

[Petr Cvek]

Dne 18.8.2015 v 20:39 Robert Jarzmik napsal(a):
> Petr Cvek <petr.cvek@tul.cz> writes:
> 
>> Fix comments, debug functions and print strings. Rename backlight
>> structures to be more specific.
>>
>> Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
>> ---
>>  arch/arm/mach-pxa/magician.c | 121 +++++++++++++++++++++++++++++++------------
>>  1 file changed, 89 insertions(+), 32 deletions(-)
>>
>> diff --git a/arch/arm/mach-pxa/magician.c b/arch/arm/mach-pxa/magician.c
>> index 9e8698a..57da133 100644
>> --- a/arch/arm/mach-pxa/magician.c
>> +++ b/arch/arm/mach-pxa/magician.c
>> @@ -11,6 +11,46 @@
>>   * it under the terms of the GNU General Public License version 2 as
>>   * published by the Free Software Foundation.
>>   *
>> + *
>> + * NOTICE MDA Compact (T-mobile XDA) facts:
>> + *   On "LCD type = 1, system_rev = 2" (0x3a in CPLD)
>> + * Samsung LTP280QV - valid LCD init sequence sequence:
>> + *   powerup: Vdigital->Vanalog->gate Voff->gate Von->data enable
>> + *   powerdown: data disable->gate Von->gate Voff->Vanalog->Vdigital
>> + * Measured on PCB:
>> + *   GPIO106
>> + *     Affects VOFF, VON and other voltages
>> + *     Probably main reset pin for DC-DC converter
>> + *   GPIO75
>> + *     Must be AF0+OUT (WM leaves it to AF2+OUT), not LCD signal
>> + *     GPIO106 works only when GPIO75 is high (DC-DC power enable?)
>> + *     After LCD powerup, value is irrelevant
>> + *   GPIO104
>> + *     LCD VOFF (gate off voltage)
>> + *   GPIO105
>> + *     LCD VON (gate on voltage)
>> + * FFUART (/dev/ttyS0) WM: 38400,8,n,1,crtscts (GSM data?)
>> + * BTUART (/dev/ttyS1) WM: 115200,8,n,1,crtscts (GSM AT commands)
>> + *   Use a HTC line discipline: 0x02 channel(==0x16) data 0x02
>> + * EGPIO(CPLD) should be always present (controls "all" supply power)
>> + *   For rootfs on MMC/SD: EGPIO module controls card power (in kernel)
>> + * cpu-freq often freeze platform (errata?, use userspace gov)
>> + * Do not use YUV420, (erratum E24) causes LCD hang (forever)
>> + * Many GSM related pins are unknown
>> + * gpio-rc-recv probably require lowpass filter (sw is OK)
>> + * TODO EGPIO_MAGICIAN_IR_RECEIVE_SHUTDOWN
>> + * FIXME AC charging blocks IrDA
>> + * Unimplemented blocking of camera+power+reset, reset and door open reset
>> + *   Registers: htc-pasic3.h (but blocks charging too)
>> + * Other PXA machines have wait_for_sync for ADS7846 (during LCD refresh)
>> + * pda-power has not optimal charging (accu can be bloated)
>> + * TODO Current switching works? Is gpio-regulator/bq24022 required?
> I think you can ask Philip, he's the maintainer after all ...

Ok. Philipp? 

Current switching can be done in pda-power function .set_charge(). But setting the charging current based on AC vs USB cable only is irrelevant as both can deliver 500mA (and Magician is not one of the most power efficient machine, 100mA is barely enough to power). Better thing would be to base the switching on accu level of charge (trickle charging).

> 
>> + * pdata supports alternative drivers (mutually exclusive modules)
>> + *   i2c-pxa vs i2c-gpio (SCCB)
>> + *   pwm_bl vs gpio_backlight
>> + *   pxa27x_udc vs phy-gpio-vbus-usb
>> + *   physmap-flash vs pxa2xx-flash
> 
> Okay, this chunk is nice documentation, but not code ...
> I'd rather have it in : Documentation/arm/pxa/magician.txt or something like
> that.
OK I can move these comments there.

> 
>> @@ -120,11 +160,12 @@ static unsigned long magician_pin_config[] __initdata = {
>>  };
>>  
>>  /*
>> - * IRDA
>> + * IrDA
>>   */
>>  
>>  static struct pxaficp_platform_data magician_ficp_info = {
>>  	.gpio_pwdown		= GPIO83_MAGICIAN_nIR_EN,
>> +	/* TODO test with FIR dongle */
> I don't like this comment. I was not there before, so let it be. You know there
> remains a TODO, I know ... but you can cunningly slip it in the commit message.
Actually I have bought a dongle during last week, but it turned out, it can do only SIR (and poorly :-D). I can try to enable a second magician PCB, but it will takes some time (as I have only one accupack - second burned down). So do I have to remove only comment or FIR flag altogether? (I can find with second magician that it can work like a charm or it is not working - for example due to incompatible IrDA transceiver).

[PATCH v2 03/21] ARM: pxa: magician: Fix comments, debug functions and print strings

[Philipp Zabel]

Am Montag, den 17.08.2015, 23:57 +0200 schrieb Petr Cvek:
> Fix comments, debug functions and print strings. Rename backlight
> structures to be more specific.
> 
> Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
> ---
>  arch/arm/mach-pxa/magician.c | 121 +++++++++++++++++++++++++++++++--
> ----------
>  1 file changed, 89 insertions(+), 32 deletions(-)
> 
> diff --git a/arch/arm/mach-pxa/magician.c b/arch/arm/mach
> -pxa/magician.c
> index 9e8698a..57da133 100644
> --- a/arch/arm/mach-pxa/magician.c
> +++ b/arch/arm/mach-pxa/magician.c
> @@ -11,6 +11,46 @@
>   * it under the terms of the GNU General Public License version 2 as
>   * published by the Free Software Foundation.
>   *
> + *
> + * NOTICE MDA Compact (T-mobile XDA) facts:
> + *   On "LCD type = 1, system_rev = 2" (0x3a in CPLD)
> + * Samsung LTP280QV - valid LCD init sequence sequence:
> + *   powerup: Vdigital->Vanalog->gate Voff->gate Von->data enable
> + *   powerdown: data disable->gate Von->gate Voff->Vanalog->Vdigital
> + * Measured on PCB:
> + *   GPIO106
> + *     Affects VOFF, VON and other voltages
> + *     Probably main reset pin for DC-DC converter
> + *   GPIO75
> + *     Must be AF0+OUT (WM leaves it to AF2+OUT), not LCD signal
> + *     GPIO106 works only when GPIO75 is high (DC-DC power enable?)
> + *     After LCD powerup, value is irrelevant
> + *   GPIO104
> + *     LCD VOFF (gate off voltage)
> + *   GPIO105
> + *     LCD VON (gate on voltage)
> + * FFUART (/dev/ttyS0) WM: 38400,8,n,1,crtscts (GSM data?)
> + * BTUART (/dev/ttyS1) WM: 115200,8,n,1,crtscts (GSM AT commands)
> + *   Use a HTC line discipline: 0x02 channel(==0x16) data 0x02
> + * EGPIO(CPLD) should be always present (controls "all" supply 
> power)
> + *   For rootfs on MMC/SD: EGPIO module controls card power (in 
> kernel)
> + * cpu-freq often freeze platform (errata?, use userspace gov)
> + * Do not use YUV420, (erratum E24) causes LCD hang (forever)
> + * Many GSM related pins are unknown
> + * gpio-rc-recv probably require lowpass filter (sw is OK)
> + * TODO EGPIO_MAGICIAN_IR_RECEIVE_SHUTDOWN
> + * FIXME AC charging blocks IrDA
> + * Unimplemented blocking of camera+power+reset, reset and door open 
> reset
> + *   Registers: htc-pasic3.h (but blocks charging too)
> + * Other PXA machines have wait_for_sync for ADS7846 (during LCD 
> refresh)
> + * pda-power has not optimal charging (accu can be bloated)
> + * TODO Current switching works? Is gpio-regulator/bq24022 required?

> + * pdata supports alternative drivers (mutually exclusive modules)
> + *   i2c-pxa vs i2c-gpio (SCCB)
> + *   pwm_bl vs gpio_backlight
> + *   pxa27x_udc vs phy-gpio-vbus-usb
> + *   physmap-flash vs pxa2xx-flash

This infodump is useful to have, but I don't think it belongs in the
code.
Also, please don't include the mutually exclusive alternatives in
mainline. Let's just pick one option.

>  #include <linux/kernel.h>
> @@ -57,31 +97,31 @@ static unsigned long magician_pin_config[] 
> __initdata = {
>  	GPIO80_nCS_4,
>  	GPIO33_nCS_5,
>  
> -	/* I2C */
> +	/* I2C UDA1380 + OV9640 */
>  	GPIO117_I2C_SCL,
>  	GPIO118_I2C_SDA,
>  
> -	/* PWM 0 */
> +	/* PWM 0 - LCD backlight */
>  	GPIO16_PWM0_OUT,
>  
> -	/* I2S */
> +	/* I2S UDA1380 capture */
>  	GPIO28_I2S_BITCLK_OUT,
>  	GPIO29_I2S_SDATA_IN,
>  	GPIO31_I2S_SYNC,
>  	GPIO113_I2S_SYSCLK,
>  
> -	/* SSP 1 */
> +	/* SSP 1 UDA1380 playback */
>  	GPIO23_SSP1_SCLK,
>  	GPIO24_SSP1_SFRM,
>  	GPIO25_SSP1_TXD,
>  
> -	/* SSP 2 */
> +	/* SSP 2 TSC2046 touchscreen */
>  	GPIO19_SSP2_SCLK,
>  	GPIO14_SSP2_SFRM,
>  	GPIO89_SSP2_TXD,
>  	GPIO88_SSP2_RXD,
>  
> -	/* MMC */
> +	/* MMC/SD/SDHC slot */
>  	GPIO32_MMC_CLK,
>  	GPIO92_MMC_DAT_0,
>  	GPIO109_MMC_DAT_1,

These comments are nice to have.

> @@ -92,7 +132,7 @@ static unsigned long magician_pin_config[] 
> __initdata = {
>  	/* LCD */
>  	GPIOxx_LCD_TFT_16BPP,
>  
> -	/* QCI */
> +	/* QCI camera interface */
>  	GPIO12_CIF_DD_7,
>  	GPIO17_CIF_DD_6,
>  	GPIO50_CIF_DD_3,
> @@ -120,11 +160,12 @@ static unsigned long magician_pin_config[] 
> __initdata = {
>  };
>  
>  /*
> - * IRDA
> + * IrDA
>   */
>  
>  static struct pxaficp_platform_data magician_ficp_info = {
>  	.gpio_pwdown		= GPIO83_MAGICIAN_nIR_EN,
> +	/* TODO test with FIR dongle */
>  	.transceiver_cap	= IR_SIRMODE | IR_OFF,
>  };
> @@ -174,8 +215,8 @@ static struct platform_device gpio_keys = {
>   
>  /*
>   * EGPIO (Xilinx CPLD)
> - *
> - * 7 32-bit aligned 8-bit registers: 3x output, 1x irq, 3x input
> + * 32-bit aligned
> + * 7x 8-bit registers: 3x output, 1x irq, 3x input
>   */

Why reorder?

>  static struct resource egpio_resources[] = {
> @@ -197,7 +238,11 @@ static struct htc_egpio_chip egpio_chips[] = {
>  		.gpio_base = MAGICIAN_EGPIO(0, 0),
>  		.num_gpios = 24,
>  		.direction = HTC_EGPIO_OUTPUT,
> -		.initial_values = 0x40, /* EGPIO_MAGICIAN_GSM_RESET 
> */
> +
> +		/*
> +		 * Depends on modules configuration
> +		 */
> +		.initial_values = 0x40,
>  	},
>  	[1] = {
>  		.reg_start = 4,
> @@ -228,7 +273,7 @@ static struct platform_device egpio = {
>  };
>  
>  /*
> - * LCD - Toppoly TD028STEB1 or Samsung LTP280QV
> + * PXAFB LCD - Toppoly TD028STEB1 or Samsung LTP280QV
>   */
>  
>  static struct pxafb_mode_info toppoly_modes[] = {
> @@ -265,10 +310,9 @@ static struct pxafb_mode_info samsung_modes[] = 
> {
>  
>  static void toppoly_lcd_power(int on, struct fb_var_screeninfo *si)
>  {
> -	pr_debug("Toppoly LCD power\n");
> +	pr_debug("Toppoly LCD power: %s\n", on ? "on" : "off");Ok.

Ok.

> 	if (on) {
> -		pr_debug("on\n");
>  		gpio_set_value(EGPIO_MAGICIAN_TOPPOLY_POWER, 1);
>  		gpio_set_value(GPIO106_MAGICIAN_LCD_POWER_3, 1);
>  		udelay(2000);
> @@ -280,8 +324,7 @@ static void toppoly_lcd_power(int on, struct 
> fb_var_screeninfo *si)
>  		udelay(2000);
>  		gpio_set_value(GPIO105_MAGICIAN_LCD_POWER_2, 1);
>  	} else {
> -		pr_debug("off\n");
> -		msleep(15);
> +		mdelay(15);

This is an unrelated change and should be in a separate patch.
Also maybe better use usleep_range?

>  		gpio_set_value(GPIO105_MAGICIAN_LCD_POWER_2, 0);
>  		udelay(500);
>  		gpio_set_value(GPIO104_MAGICIAN_LCD_POWER_1, 0);
> @@ -293,10 +336,9 @@ static void toppoly_lcd_power(int on, struct 
> fb_var_screeninfo *si)
>  
>  static void samsung_lcd_power(int on, struct fb_var_screeninfo *si)
>  {
> -	pr_debug("Samsung LCD power\n");
> +	pr_debug("Samsung LCD power: %s\n", on ? "on" : "off");
>  
>  	if (on) {
> -		pr_debug("on\n");
>  		if (system_rev < 3)
>  			gpio_set_value(GPIO75_MAGICIAN_SAMSUNG_POWER
> , 1);
>  		else
> @@ -309,7 +351,6 @@ static void samsung_lcd_power(int on, struct 
> fb_var_screeninfo *si)
>  		gpio_set_value(GPIO105_MAGICIAN_LCD_POWER_2, 1);
>  		mdelay(10);
>  	} else {
> -		pr_debug("off\n");
>  		mdelay(10);
>  		gpio_set_value(GPIO105_MAGICIAN_LCD_POWER_2, 0);
>  		mdelay(30);
> @@ -346,8 +387,8 @@ static struct pxafb_mach_info samsung_info = {
>   */
>  
[...]
> +/*
> + * pda_power Li-ion charger
> + */
> +
>  static struct pda_power_pdata power_supply_info = {
>  	.init		= power_supply_init,
>  	.is_ac_online	= magician_is_ac_online,
> @@ -575,7 +631,7 @@ static struct platform_device power_supply = {
>  };
>  
>  /*
> - * Battery charger
> + * Charging current switch
>   */

The bq24022 is a battery charger. The switch is one of its input pins
to allow to limit USB charging current to 100 mA in case the host
doesn't support more.
 
regards
Philipp

[PATCH v2 04/21] ARM: pxa: magician: Add, fix and init (new) GPIOs

[Petr Cvek]

Add new GPIOs, fix some GPIO names and initialization (EGPIO, LCD power on
sequence).

Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
---
 arch/arm/mach-pxa/include/mach/magician.h |  39 ++++---
 arch/arm/mach-pxa/magician.c              | 166 +++++++++++++++++++++++-------
 2 files changed, 153 insertions(+), 52 deletions(-)

diff --git a/arch/arm/mach-pxa/include/mach/magician.h b/arch/arm/mach-pxa/include/mach/magician.h
index ba6a6e1..d19e504 100644
--- a/arch/arm/mach-pxa/include/mach/magician.h
+++ b/arch/arm/mach-pxa/include/mach/magician.h
@@ -21,10 +21,10 @@
 
 #define GPIO0_MAGICIAN_KEY_POWER		0
 #define GPIO9_MAGICIAN_UNKNOWN			9
-#define GPIO10_MAGICIAN_GSM_IRQ			10
+#define GPIO10_MAGICIAN_GSM_IRQ		10
 #define GPIO11_MAGICIAN_GSM_OUT1		11
 #define GPIO13_MAGICIAN_CPLD_IRQ		13
-#define GPIO18_MAGICIAN_UNKNOWN			18
+#define GPIO18_MAGICIAN_UNKNOWN		18
 #define GPIO22_MAGICIAN_VIBRA_EN		22
 #define GPIO26_MAGICIAN_GSM_POWER		26
 #define GPIO27_MAGICIAN_USBC_PUEN		27
@@ -32,8 +32,10 @@
 #define GPIO37_MAGICIAN_KEY_HANGUP		37
 #define GPIO38_MAGICIAN_KEY_CONTACTS		38
 #define GPIO40_MAGICIAN_GSM_OUT2		40
-#define GPIO48_MAGICIAN_UNKNOWN			48
-#define GPIO56_MAGICIAN_UNKNOWN			56
+#define GPIO46_MAGICIAN_IR_RX			46
+#define GPIO47_MAGICIAN_IR_TX			47
+#define GPIO48_MAGICIAN_UNKNOWN		48
+#define GPIO56_MAGICIAN_UNKNOWN		56
 #define GPIO57_MAGICIAN_CAM_RESET		57
 #define GPIO75_MAGICIAN_SAMSUNG_POWER		75
 #define GPIO83_MAGICIAN_nIR_EN			83
@@ -51,15 +53,17 @@
 #define GPIO100_MAGICIAN_KEY_VOL_UP		100
 #define GPIO101_MAGICIAN_KEY_VOL_DOWN 		101
 #define GPIO102_MAGICIAN_KEY_PHONE		102
-#define GPIO103_MAGICIAN_LED_KP			103
-#define GPIO104_MAGICIAN_LCD_POWER_1 		104
-#define GPIO105_MAGICIAN_LCD_POWER_2		105
-#define GPIO106_MAGICIAN_LCD_POWER_3		106
+#define GPIO103_MAGICIAN_LED_KP		103
+#define GPIO104_MAGICIAN_LCD_VOFF_EN		104
+#define GPIO105_MAGICIAN_LCD_VON_EN		105
+#define GPIO106_MAGICIAN_LCD_DCDC_NRESET	106
 #define GPIO107_MAGICIAN_DS1WM_IRQ		107
 #define GPIO108_MAGICIAN_GSM_READY		108
 #define GPIO114_MAGICIAN_UNKNOWN		114
 #define GPIO115_MAGICIAN_nPEN_IRQ		115
 #define GPIO116_MAGICIAN_nCAM_EN		116
+#define GPIO117_MAGICIAN_I2C_SCL		117
+#define GPIO118_MAGICIAN_I2C_SDA		118
 #define GPIO119_MAGICIAN_UNKNOWN		119
 #define GPIO120_MAGICIAN_UNKNOWN		120
 
@@ -78,7 +82,7 @@
  * CPLD EGPIOs
  */
 
-#define MAGICIAN_EGPIO_BASE			PXA_NR_BUILTIN_GPIO
+#define MAGICIAN_EGPIO_BASE	PXA_NR_BUILTIN_GPIO
 #define MAGICIAN_EGPIO(reg,bit) \
 	(MAGICIAN_EGPIO_BASE + 8*reg + bit)
 
@@ -89,25 +93,28 @@
 #define EGPIO_MAGICIAN_GSM_RESET		MAGICIAN_EGPIO(0, 6)
 #define EGPIO_MAGICIAN_LCD_POWER		MAGICIAN_EGPIO(0, 7)
 #define EGPIO_MAGICIAN_SPK_POWER		MAGICIAN_EGPIO(1, 0)
-#define EGPIO_MAGICIAN_EP_POWER			MAGICIAN_EGPIO(1, 1)
+#define EGPIO_MAGICIAN_EP_POWER		MAGICIAN_EGPIO(1, 1)
 #define EGPIO_MAGICIAN_IN_SEL0			MAGICIAN_EGPIO(1, 2)
 #define EGPIO_MAGICIAN_IN_SEL1			MAGICIAN_EGPIO(1, 3)
 #define EGPIO_MAGICIAN_MIC_POWER		MAGICIAN_EGPIO(1, 4)
 #define EGPIO_MAGICIAN_CODEC_RESET		MAGICIAN_EGPIO(1, 5)
 #define EGPIO_MAGICIAN_CODEC_POWER		MAGICIAN_EGPIO(1, 6)
-#define EGPIO_MAGICIAN_BL_POWER			MAGICIAN_EGPIO(1, 7)
-#define EGPIO_MAGICIAN_SD_POWER			MAGICIAN_EGPIO(2, 0)
+#define EGPIO_MAGICIAN_BL_POWER		MAGICIAN_EGPIO(1, 7)
+#define EGPIO_MAGICIAN_SD_POWER		MAGICIAN_EGPIO(2, 0)
 #define EGPIO_MAGICIAN_CARKIT_MIC		MAGICIAN_EGPIO(2, 1)
-#define EGPIO_MAGICIAN_UNKNOWN_WAVEDEV_DLL	MAGICIAN_EGPIO(2, 2)
+#define EGPIO_MAGICIAN_IR_RECEIVE_SHUTDOWN	MAGICIAN_EGPIO(2, 2)
 #define EGPIO_MAGICIAN_FLASH_VPP		MAGICIAN_EGPIO(2, 3)
 #define EGPIO_MAGICIAN_BL_POWER2		MAGICIAN_EGPIO(2, 4)
 #define EGPIO_MAGICIAN_BQ24022_ISET2		MAGICIAN_EGPIO(2, 5)
+#define EGPIO_MAGICIAN_NICD_CHARGE		MAGICIAN_EGPIO(2, 6)
 #define EGPIO_MAGICIAN_GSM_POWER		MAGICIAN_EGPIO(2, 7)
 
 /* input */
 
-#define EGPIO_MAGICIAN_CABLE_STATE_AC		MAGICIAN_EGPIO(4, 0)
-#define EGPIO_MAGICIAN_CABLE_STATE_USB		MAGICIAN_EGPIO(4, 1)
+/* AC=1, USB=0 */
+#define EGPIO_MAGICIAN_CABLE_TYPE		MAGICIAN_EGPIO(4, 0)
+/* =1 when AC or USB cable inserted */
+#define EGPIO_MAGICIAN_CABLE_INSERT1		MAGICIAN_EGPIO(4, 1)
 
 #define EGPIO_MAGICIAN_BOARD_ID0		MAGICIAN_EGPIO(5, 0)
 #define EGPIO_MAGICIAN_BOARD_ID1		MAGICIAN_EGPIO(5, 1)
@@ -116,5 +123,7 @@
 #define EGPIO_MAGICIAN_nSD_READONLY		MAGICIAN_EGPIO(5, 4)
 
 #define EGPIO_MAGICIAN_EP_INSERT		MAGICIAN_EGPIO(6, 1)
+/* FIXME behave like 4,1, may differ for host/device */
+#define EGPIO_MAGICIAN_CABLE_INSERT2		MAGICIAN_EGPIO(6, 4)
 
 #endif /* _MAGICIAN_H_ */
diff --git a/arch/arm/mach-pxa/magician.c b/arch/arm/mach-pxa/magician.c
index 57da133..bc206f8 100644
--- a/arch/arm/mach-pxa/magician.c
+++ b/arch/arm/mach-pxa/magician.c
@@ -4,8 +4,9 @@
  * and T-Mobile MDA Compact.
  *
  * Copyright (c) 2006-2007 Philipp Zabel
+ * Copyright (c) 2014-2015 Petr Cvek (massive rework)
  *
- * Based on hx4700.c, spitz.c and others.
+ * Based on hx4700.c, spitz.c, board-overo.c and others.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -96,13 +97,24 @@ static unsigned long magician_pin_config[] __initdata = {
 	GPIO79_nCS_3,	/* EGPIO CPLD */
 	GPIO80_nCS_4,
 	GPIO33_nCS_5,
+	GPIO49_nPWE,
+	GPIO18_RDY,
 
 	/* I2C UDA1380 + OV9640 */
 	GPIO117_I2C_SCL,
 	GPIO118_I2C_SDA,
 
+	/* Omnivision camera power and reset GPIO */
+	MFP_CFG_OUT(GPIO116, AF0, DRIVE_HIGH),	/* nEnable, active in low */
+	MFP_CFG_OUT(GPIO57, AF0, DRIVE_HIGH),	/* Reset, active in high */
+
+#if IS_ENABLED(CONFIG_PWM_PXA)
 	/* PWM 0 - LCD backlight */
 	GPIO16_PWM0_OUT,
+#else
+	/* Ensure static backlight without any driver */
+	MFP_CFG_OUT(GPIO16, AF0, DRIVE_LOW),	/* Backlight enabled */
+#endif
 
 	/* I2S UDA1380 capture */
 	GPIO28_I2S_BITCLK_OUT,
@@ -129,8 +141,26 @@ static unsigned long magician_pin_config[] __initdata = {
 	GPIO111_MMC_DAT_3,
 	GPIO112_MMC_CMD,
 
-	/* LCD */
-	GPIOxx_LCD_TFT_16BPP,
+	/*
+	 * LCD
+	 * NOTICE Samsung LTP280QV:
+	 *   cannot use GPIOxx_LCD_TFT_16BPP, GPIO75 is LCD power,
+	 * 74 unused (AF0 by bootloader?)
+	 */
+	GPIOxx_LCD_16BPP,
+	GPIO76_LCD_PCLK,
+	GPIO77_LCD_BIAS,	/* data_valid (v/hsync) (ADS7846 sync?) */
+
+	/* NOTICE valid LCD init sequence */
+
+	/* for GPIO75_MAGICIAN_SAMSUNG_POWER */
+	MFP_CFG_OUT(GPIO75, AF0, DRIVE_HIGH),
+	/* for GPIO106_MAGICIAN_LCD_DCDC_NRESET */
+	MFP_CFG_OUT(GPIO106, AF0, DRIVE_HIGH),
+	/* for GPIO104_MAGICIAN_LCD_VOFF_EN */
+	MFP_CFG_OUT(GPIO104, AF0, DRIVE_HIGH),
+	/* for GPIO105_MAGICIAN_LCD_VON_EN */
+	MFP_CFG_OUT(GPIO105, AF0, DRIVE_HIGH),
 
 	/* QCI camera interface */
 	GPIO12_CIF_DD_7,
@@ -147,16 +177,69 @@ static unsigned long magician_pin_config[] __initdata = {
 	GPIO85_CIF_LV,
 
 	/* Magician specific input GPIOs */
-	GPIO9_GPIO,		/* unknown */
 	GPIO10_GPIO,	/* GSM_IRQ */
 	GPIO13_GPIO,	/* CPLD_IRQ */
 	GPIO107_GPIO,	/* DS1WM_IRQ */
 	GPIO108_GPIO,	/* GSM_READY */
 	GPIO115_GPIO,	/* nPEN_IRQ */
 
-	/* I2C */
-	GPIO117_I2C_SCL,
-	GPIO118_I2C_SDA,
+	/* FIXME: probably IrDA transmitter disable */
+	MFP_CFG_OUT(GPIO83, AF0, DRIVE_HIGH),
+
+	/* Vibration motor */
+	MFP_CFG_OUT(GPIO22, AF0, DRIVE_LOW),
+
+	/* Keypad LEDs (red/green phone) */
+	MFP_CFG_OUT(GPIO103, AF0, DRIVE_LOW),
+
+	/* GSM pins */
+	MFP_CFG_OUT(GPIO11, AF0, DRIVE_LOW), /* CPU is unavailable(?) */
+	MFP_CFG_OUT(GPIO26, AF0, DRIVE_LOW), /* GSM power, active high */
+	MFP_CFG_OUT(GPIO86, AF0, DRIVE_HIGH), /* GSM reset, active high */
+
+	/* USB connector */
+	MFP_CFG_OUT(GPIO27, AF0, DRIVE_LOW), /* usbc pull-up enable */
+	MFP_CFG_OUT(GPIO30, AF0, DRIVE_LOW), /* charging enable, active low */
+
+	/* FFUART, FIXME never observed to do something, GSM data? */
+	GPIO34_FFUART_RXD,
+	GPIO35_FFUART_CTS,
+	GPIO36_FFUART_DCD,
+	GPIO39_FFUART_TXD,
+	GPIO41_FFUART_RTS,
+
+	/* BTUART, AT commands/data, HTC port line discipline */
+	GPIO42_BTUART_RXD,
+	GPIO43_BTUART_TXD,
+	GPIO44_BTUART_CTS,
+	GPIO45_BTUART_RTS,
+
+	/* Power I2C, enabled controller will overrule GPIO */
+	GPIO3_GPIO,	/* SCL */
+	GPIO4_GPIO,	/* SDA */
+
+	/* IrDA GPIOs, if pxaficp, it will change AF accordingly to mode */
+	MFP_CFG_IN(GPIO46, AF0),
+	MFP_CFG_OUT(GPIO47, AF0, DRIVE_LOW),
+	MFP_CFG_OUT(GPIO83, AF0, DRIVE_HIGH),
+
+	/* TODO unknown */
+	GPIO9_GPIO,		/* unknown */
+	MFP_CFG_OUT(GPIO40, AF0, DRIVE_LOW),	/* FIXME GSM? */
+	MFP_CFG_OUT(GPIO87, AF0, DRIVE_LOW),	/* FIXME GSM? */
+
+	/* Left GPIOs (undefined here):
+	 * 18, 49 : bootloader=VLIO?, WinM=TODO
+	 * 48 : AF0/out0
+	 * 56 : AF0/out0
+	 * 74 : bootloader=AF0/output
+	 * 86 : bootloader=AF0/input (but should be gsm reset???)
+	 * 114 : AF0/out0
+	 * 119 : AF0/out0
+	 * 120 : AF0/out
+	 * 1 : dedicated reset, AF0/output
+	 * 13 : cpld irq, output (??)
+	 */
 };
 
 /*
@@ -241,8 +324,9 @@ static struct htc_egpio_chip egpio_chips[] = {
 
 		/*
 		 * Depends on modules configuration
+		 * Things like MMC and LCD should be enabled
 		 */
-		.initial_values = 0x40,
+		.initial_values = 0x21a0c0,
 	},
 	[1] = {
 		.reg_start = 4,
@@ -314,22 +398,22 @@ static void toppoly_lcd_power(int on, struct fb_var_screeninfo *si)
 
 	if (on) {
 		gpio_set_value(EGPIO_MAGICIAN_TOPPOLY_POWER, 1);
-		gpio_set_value(GPIO106_MAGICIAN_LCD_POWER_3, 1);
+		gpio_set_value(GPIO106_MAGICIAN_LCD_DCDC_NRESET, 1);
 		udelay(2000);
 		gpio_set_value(EGPIO_MAGICIAN_LCD_POWER, 1);
 		udelay(2000);
 		/* FIXME: enable LCDC here */
 		udelay(2000);
-		gpio_set_value(GPIO104_MAGICIAN_LCD_POWER_1, 1);
+		gpio_set_value(GPIO104_MAGICIAN_LCD_VOFF_EN, 1);
 		udelay(2000);
-		gpio_set_value(GPIO105_MAGICIAN_LCD_POWER_2, 1);
+		gpio_set_value(GPIO105_MAGICIAN_LCD_VON_EN, 1);
 	} else {
 		mdelay(15);
-		gpio_set_value(GPIO105_MAGICIAN_LCD_POWER_2, 0);
+		gpio_set_value(GPIO105_MAGICIAN_LCD_VON_EN, 0);
 		udelay(500);
-		gpio_set_value(GPIO104_MAGICIAN_LCD_POWER_1, 0);
+		gpio_set_value(GPIO104_MAGICIAN_LCD_VOFF_EN, 0);
 		udelay(1000);
-		gpio_set_value(GPIO106_MAGICIAN_LCD_POWER_3, 0);
+		gpio_set_value(GPIO106_MAGICIAN_LCD_DCDC_NRESET, 0);
 		gpio_set_value(EGPIO_MAGICIAN_LCD_POWER, 0);
 	}
 }
@@ -343,21 +427,19 @@ static void samsung_lcd_power(int on, struct fb_var_screeninfo *si)
 			gpio_set_value(GPIO75_MAGICIAN_SAMSUNG_POWER, 1);
 		else
 			gpio_set_value(EGPIO_MAGICIAN_LCD_POWER, 1);
-		mdelay(10);
-		gpio_set_value(GPIO106_MAGICIAN_LCD_POWER_3, 1);
-		mdelay(10);
-		gpio_set_value(GPIO104_MAGICIAN_LCD_POWER_1, 1);
-		mdelay(30);
-		gpio_set_value(GPIO105_MAGICIAN_LCD_POWER_2, 1);
-		mdelay(10);
+		mdelay(6);
+		gpio_set_value(GPIO106_MAGICIAN_LCD_DCDC_NRESET, 1);
+		mdelay(6);	/* Avdd -> Voff >5ms */
+		gpio_set_value(GPIO104_MAGICIAN_LCD_VOFF_EN, 1);
+		mdelay(16);	/* Voff -> Von >(5+10)ms */
+		gpio_set_value(GPIO105_MAGICIAN_LCD_VON_EN, 1);
 	} else {
-		mdelay(10);
-		gpio_set_value(GPIO105_MAGICIAN_LCD_POWER_2, 0);
-		mdelay(30);
-		gpio_set_value(GPIO104_MAGICIAN_LCD_POWER_1, 0);
-		mdelay(10);
-		gpio_set_value(GPIO106_MAGICIAN_LCD_POWER_3, 0);
-		mdelay(10);
+		gpio_set_value(GPIO105_MAGICIAN_LCD_VON_EN, 0);
+		mdelay(16);
+		gpio_set_value(GPIO104_MAGICIAN_LCD_VOFF_EN, 0);
+		mdelay(6);
+		gpio_set_value(GPIO106_MAGICIAN_LCD_DCDC_NRESET, 0);
+		mdelay(6);
 		if (system_rev < 3)
 			gpio_set_value(GPIO75_MAGICIAN_SAMSUNG_POWER, 0);
 		else
@@ -555,7 +637,7 @@ static struct resource gpio_vbus_resource = {
 
 static struct gpio_vbus_mach_info gpio_vbus_info = {
 	.gpio_pullup = GPIO27_MAGICIAN_USBC_PUEN,
-	.gpio_vbus = EGPIO_MAGICIAN_CABLE_STATE_USB,
+	.gpio_vbus = EGPIO_MAGICIAN_CABLE_INSERT1,
 };
 
 static struct platform_device gpio_vbus = {
@@ -574,17 +656,17 @@ static struct platform_device gpio_vbus = {
 
 static int power_supply_init(struct device *dev)
 {
-	return gpio_request(EGPIO_MAGICIAN_CABLE_STATE_AC, "CABLE_STATE_AC");
+	return gpio_request(EGPIO_MAGICIAN_CABLE_TYPE, "CABLE_STATE_AC");
 }
 
 static int magician_is_ac_online(void)
 {
-	return gpio_get_value(EGPIO_MAGICIAN_CABLE_STATE_AC);
+	return gpio_get_value(EGPIO_MAGICIAN_CABLE_INSERT1);
 }
 
 static void power_supply_exit(struct device *dev)
 {
-	gpio_free(EGPIO_MAGICIAN_CABLE_STATE_AC);
+	gpio_free(EGPIO_MAGICIAN_CABLE_TYPE);
 }
 
 static char *magician_supplicants[] = {
@@ -774,12 +856,21 @@ static struct platform_device *devices[] __initdata = {
 	&leds_gpio,
 };
 
+/*
+ * Global GPIOs
+ */
+
 static struct gpio magician_global_gpios[] = {
-	{ GPIO13_MAGICIAN_CPLD_IRQ, GPIOF_IN,		"CPLD_IRQ" },
-	{ GPIO107_MAGICIAN_DS1WM_IRQ, GPIOF_IN,	"DS1WM_IRQ" },
-	{ GPIO104_MAGICIAN_LCD_POWER_1, GPIOF_OUT_INIT_LOW, "LCD power 1" },
-	{ GPIO105_MAGICIAN_LCD_POWER_2, GPIOF_OUT_INIT_LOW, "LCD power 2" },
-	{ GPIO106_MAGICIAN_LCD_POWER_3, GPIOF_OUT_INIT_LOW, "LCD power 3" },
+	{ GPIO13_MAGICIAN_CPLD_IRQ, GPIOF_IN, "CPLD_IRQ" },
+	{ GPIO107_MAGICIAN_DS1WM_IRQ, GPIOF_IN, "DS1WM_IRQ" },
+
+	/* NOTICE valid LCD init sequence */
+	{ GPIO106_MAGICIAN_LCD_DCDC_NRESET, GPIOF_OUT_INIT_HIGH,
+		"LCD DCDC nreset" },
+	{ GPIO104_MAGICIAN_LCD_VOFF_EN, GPIOF_OUT_INIT_HIGH,
+		"LCD VOFF enable" },
+	{ GPIO105_MAGICIAN_LCD_VON_EN, GPIOF_OUT_INIT_HIGH,
+		"LCD VON enable" },
 	{ GPIO83_MAGICIAN_nIR_EN, GPIOF_OUT_INIT_HIGH,	"nIR_EN" },
 };
 
@@ -817,6 +908,7 @@ static void __init magician_init(void)
 		lcd_select = board_id & 0x8;
 		pr_info("LCD type: %s\n", lcd_select ? "Samsung" : "Toppoly");
 		if (lcd_select && (system_rev < 3))
+			/* NOTICE valid LCD init sequence */
 			gpio_request_one(GPIO75_MAGICIAN_SAMSUNG_POWER,
 				GPIOF_OUT_INIT_LOW, "Samsung LCD Power");
 		pxa_set_fb_info(NULL,
-- 
1.7.12.1

[PATCH v2 04/21] ARM: pxa: magician: Add, fix and init (new) GPIOs

[Robert Jarzmik]

Petr Cvek <petr.cvek@tul.cz> writes:

> Add new GPIOs, fix some GPIO names and initialization (EGPIO, LCD power on
> sequence).
>
> Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
> ---
>  arch/arm/mach-pxa/include/mach/magician.h |  39 ++++---
>  arch/arm/mach-pxa/magician.c              | 166 +++++++++++++++++++++++-------
>  2 files changed, 153 insertions(+), 52 deletions(-)
>
> diff --git a/arch/arm/mach-pxa/include/mach/magician.h b/arch/arm/mach-pxa/include/mach/magician.h
> index ba6a6e1..d19e504 100644
> --- a/arch/arm/mach-pxa/include/mach/magician.h
> +++ b/arch/arm/mach-pxa/include/mach/magician.h
> @@ -21,10 +21,10 @@
>  
>  #define GPIO0_MAGICIAN_KEY_POWER		0
>  #define GPIO9_MAGICIAN_UNKNOWN			9
> -#define GPIO10_MAGICIAN_GSM_IRQ			10
> +#define GPIO10_MAGICIAN_GSM_IRQ		10
Whitespace/indentation changes ?

>  #define GPIO11_MAGICIAN_GSM_OUT1		11
>  #define GPIO13_MAGICIAN_CPLD_IRQ		13
> -#define GPIO18_MAGICIAN_UNKNOWN			18
> +#define GPIO18_MAGICIAN_UNKNOWN		18
Ditto.

> @@ -32,8 +32,10 @@
>  #define GPIO37_MAGICIAN_KEY_HANGUP		37
>  #define GPIO38_MAGICIAN_KEY_CONTACTS		38
>  #define GPIO40_MAGICIAN_GSM_OUT2		40
> -#define GPIO48_MAGICIAN_UNKNOWN			48
> -#define GPIO56_MAGICIAN_UNKNOWN			56
> +#define GPIO46_MAGICIAN_IR_RX			46
> +#define GPIO47_MAGICIAN_IR_TX			47
> +#define GPIO48_MAGICIAN_UNKNOWN		48
> +#define GPIO56_MAGICIAN_UNKNOWN		56
Ditto for 48, 56 ?

>  #define GPIO57_MAGICIAN_CAM_RESET		57
>  #define GPIO75_MAGICIAN_SAMSUNG_POWER		75
>  #define GPIO83_MAGICIAN_nIR_EN			83
> @@ -51,15 +53,17 @@
>  #define GPIO100_MAGICIAN_KEY_VOL_UP		100
>  #define GPIO101_MAGICIAN_KEY_VOL_DOWN 		101
>  #define GPIO102_MAGICIAN_KEY_PHONE		102
> -#define GPIO103_MAGICIAN_LED_KP			103
> -#define GPIO104_MAGICIAN_LCD_POWER_1 		104
> -#define GPIO105_MAGICIAN_LCD_POWER_2		105
> -#define GPIO106_MAGICIAN_LCD_POWER_3		106
> +#define GPIO103_MAGICIAN_LED_KP		103
Ditto ?
> +#define GPIO104_MAGICIAN_LCD_VOFF_EN		104
> +#define GPIO105_MAGICIAN_LCD_VON_EN		105
> +#define GPIO106_MAGICIAN_LCD_DCDC_NRESET	106
>  #define GPIO107_MAGICIAN_DS1WM_IRQ		107
>  #define GPIO108_MAGICIAN_GSM_READY		108
>  #define GPIO114_MAGICIAN_UNKNOWN		114
>  #define GPIO115_MAGICIAN_nPEN_IRQ		115
>  #define GPIO116_MAGICIAN_nCAM_EN		116
> +#define GPIO117_MAGICIAN_I2C_SCL		117
> +#define GPIO118_MAGICIAN_I2C_SDA		118
>  #define GPIO119_MAGICIAN_UNKNOWN		119
>  #define GPIO120_MAGICIAN_UNKNOWN		120
>  
> @@ -78,7 +82,7 @@
>   * CPLD EGPIOs
>   */
>  
> -#define MAGICIAN_EGPIO_BASE			PXA_NR_BUILTIN_GPIO
> +#define MAGICIAN_EGPIO_BASE	PXA_NR_BUILTIN_GPIO
Ditto ?

>From here onward, I'll suppose you'll catch all whitespace/indentation changes
and extract them towards patch 01/21 ...

>  /* input */
>  
> -#define EGPIO_MAGICIAN_CABLE_STATE_AC		MAGICIAN_EGPIO(4, 0)
> -#define EGPIO_MAGICIAN_CABLE_STATE_USB		MAGICIAN_EGPIO(4, 1)
> +/* AC=1, USB=0 */
> +#define EGPIO_MAGICIAN_CABLE_TYPE		MAGICIAN_EGPIO(4, 0)
> +/* =1 when AC or USB cable inserted */
> +#define EGPIO_MAGICIAN_CABLE_INSERT1		MAGICIAN_EGPIO(4, 1)
The naming makes me uneasy : it's rather vague "cable insert".
If AC and USB are the only charging methods, why not have something like
"EGPIO_MAGICIAN_POWERED" or the like ?

>  	GPIO10_GPIO,	/* GSM_IRQ */
>  	GPIO13_GPIO,	/* CPLD_IRQ */
>  	GPIO107_GPIO,	/* DS1WM_IRQ */
>  	GPIO108_GPIO,	/* GSM_READY */
>  	GPIO115_GPIO,	/* nPEN_IRQ */
>  
> -	/* I2C */
> -	GPIO117_I2C_SCL,
> -	GPIO118_I2C_SDA,
Okay so that are no I2C devices on magician ? Why keep GPIO119_MAGICIAN_I2C_* at
the beginning of the file then ?

> +	MFP_CFG_OUT(GPIO40, AF0, DRIVE_LOW),	/* FIXME GSM? */
> +	MFP_CFG_OUT(GPIO87, AF0, DRIVE_LOW),	/* FIXME GSM? */
> +
> +	/* Left GPIOs (undefined here):
> +	 * 18, 49 : bootloader=VLIO?, WinM=TODO
> +	 * 48 : AF0/out0
> +	 * 56 : AF0/out0
> +	 * 74 : bootloader=AF0/output
> +	 * 86 : bootloader=AF0/input (but should be gsm reset???)
> +	 * 114 : AF0/out0
> +	 * 119 : AF0/out0
> +	 * 120 : AF0/out
> +	 * 1 : dedicated reset, AF0/output
> +	 * 13 : cpld irq, output (??)
> +	 */
This is not describing current code. This doesn't belong to this file, even if I
know how frustrating it is to reverse engineer all these gpios. I created a wiki
web page will all GPIOs to let go my frustration in the past :)

> @@ -241,8 +324,9 @@ static struct htc_egpio_chip egpio_chips[] = {
>  
>  		/*
>  		 * Depends on modules configuration
> +		 * Things like MMC and LCD should be enabled
>  		 */
> -		.initial_values = 0x40,
> +		.initial_values = 0x21a0c0,
Aren't they enabled by each driver upon its probe() ? What if MMC nor LCD is
compiled in the kernel ?

> @@ -343,21 +427,19 @@ static void samsung_lcd_power(int on, struct fb_var_screeninfo *si)
>  			gpio_set_value(GPIO75_MAGICIAN_SAMSUNG_POWER, 1);
>  		else
>  			gpio_set_value(EGPIO_MAGICIAN_LCD_POWER, 1);
> -		mdelay(10);
> -		gpio_set_value(GPIO106_MAGICIAN_LCD_POWER_3, 1);
> -		mdelay(10);
> -		gpio_set_value(GPIO104_MAGICIAN_LCD_POWER_1, 1);
> -		mdelay(30);
> -		gpio_set_value(GPIO105_MAGICIAN_LCD_POWER_2, 1);
> -		mdelay(10);
> +		mdelay(6);
> +		gpio_set_value(GPIO106_MAGICIAN_LCD_DCDC_NRESET, 1);
> +		mdelay(6);	/* Avdd -> Voff >5ms */
> +		gpio_set_value(GPIO104_MAGICIAN_LCD_VOFF_EN, 1);
> +		mdelay(16);	/* Voff -> Von >(5+10)ms */
> +		gpio_set_value(GPIO105_MAGICIAN_LCD_VON_EN, 1);
>  	} else {
> -		mdelay(10);
> -		gpio_set_value(GPIO105_MAGICIAN_LCD_POWER_2, 0);
> -		mdelay(30);
> -		gpio_set_value(GPIO104_MAGICIAN_LCD_POWER_1, 0);
> -		mdelay(10);
> -		gpio_set_value(GPIO106_MAGICIAN_LCD_POWER_3, 0);
> -		mdelay(10);
> +		gpio_set_value(GPIO105_MAGICIAN_LCD_VON_EN, 0);
> +		mdelay(16);
> +		gpio_set_value(GPIO104_MAGICIAN_LCD_VOFF_EN, 0);
> +		mdelay(6);
> +		gpio_set_value(GPIO106_MAGICIAN_LCD_DCDC_NRESET, 0);
> +		mdelay(6);
You're changing the timings here, right ? This should be an appart patch. What
is the reason of the change by the way ?

Cheers.

-- 
Robert

PS: My workforce is done for today, next review tomorrow.

[PATCH v2 04/21] ARM: pxa: magician: Add, fix and init (new) GPIOs

[Petr Cvek]

Dne 18.8.2015 v 21:01 Robert Jarzmik napsal(a):
> 
> From here onward, I'll suppose you'll catch all whitespace/indentation changes
> and extract them towards patch 01/21 ...

OK

> 
>>  /* input */
>>  
>> -#define EGPIO_MAGICIAN_CABLE_STATE_AC		MAGICIAN_EGPIO(4, 0)
>> -#define EGPIO_MAGICIAN_CABLE_STATE_USB		MAGICIAN_EGPIO(4, 1)
>> +/* AC=1, USB=0 */
>> +#define EGPIO_MAGICIAN_CABLE_TYPE		MAGICIAN_EGPIO(4, 0)
>> +/* =1 when AC or USB cable inserted */
>> +#define EGPIO_MAGICIAN_CABLE_INSERT1		MAGICIAN_EGPIO(4, 1)
> The naming makes me uneasy : it's rather vague "cable insert".
> If AC and USB are the only charging methods, why not have something like
> "EGPIO_MAGICIAN_POWERED" or the like ?

I don't see why not, but there are two GPIOs and it may turn out they have different function. I found it only for power/usb cable insertion. For example they may differ when usb device is connected (magician is now a host) but I did not test it (yet, phone does not generate vbus power).

> 
>>  	GPIO10_GPIO,	/* GSM_IRQ */
>>  	GPIO13_GPIO,	/* CPLD_IRQ */
>>  	GPIO107_GPIO,	/* DS1WM_IRQ */
>>  	GPIO108_GPIO,	/* GSM_READY */
>>  	GPIO115_GPIO,	/* nPEN_IRQ */
>>  
>> -	/* I2C */
>> -	GPIO117_I2C_SCL,
>> -	GPIO118_I2C_SDA,
> Okay so that are no I2C devices on magician ? Why keep GPIO119_MAGICIAN_I2C_* at
> the beginning of the file then ?

It is a duplicite definition.

	http://lxr.free-electrons.com/source/arch/arm/mach-pxa/magician.c#L60

and in the same array again at:

	http://lxr.free-electrons.com/source/arch/arm/mach-pxa/magician.c#L118

> 
>> +	MFP_CFG_OUT(GPIO40, AF0, DRIVE_LOW),	/* FIXME GSM? */
>> +	MFP_CFG_OUT(GPIO87, AF0, DRIVE_LOW),	/* FIXME GSM? */
>> +
>> +	/* Left GPIOs (undefined here):
>> +	 * 18, 49 : bootloader=VLIO?, WinM=TODO
>> +	 * 48 : AF0/out0
>> +	 * 56 : AF0/out0
>> +	 * 74 : bootloader=AF0/output
>> +	 * 86 : bootloader=AF0/input (but should be gsm reset???)
>> +	 * 114 : AF0/out0
>> +	 * 119 : AF0/out0
>> +	 * 120 : AF0/out
>> +	 * 1 : dedicated reset, AF0/output
>> +	 * 13 : cpld irq, output (??)
>> +	 */
> This is not describing current code. This doesn't belong to this file, even if I
> know how frustrating it is to reverse engineer all these gpios. I created a wiki
> web page will all GPIOs to let go my frustration in the past :)

OK I can create file in Documentation for that (at least one pin has a different direction in kernel vs bootloader).

BTW on xda wiki?

> 
>> @@ -241,8 +324,9 @@ static struct htc_egpio_chip egpio_chips[] = {
>>  
>>  		/*
>>  		 * Depends on modules configuration
>> +		 * Things like MMC and LCD should be enabled
>>  		 */
>> -		.initial_values = 0x40,
>> +		.initial_values = 0x21a0c0,
> Aren't they enabled by each driver upon its probe() ? What if MMC nor LCD is
> compiled in the kernel ?
> 

Some of it is from time kernel did not get to rootfs detection: 0x8000 for to be able to read panic errors (before a backlight driver is loaded - which is BTW not working now in vanilla). 0x80 should be for a valid LCD power sequence (broken one causes invalid voltages on LCD power lines). 0x2000 is RESET for soundcard as original 0x40 is for GSM. MMC can be probably changed to "0" now when everything is working. 0x200000 as 500mA charging should be enabled (for safety) as sometimes boot on weak accu can cause overcurrent, which will hardreset the phone.


>> @@ -343,21 +427,19 @@ static void samsung_lcd_power(int on, struct fb_var_screeninfo *si)
>>  			gpio_set_value(GPIO75_MAGICIAN_SAMSUNG_POWER, 1);
>>  		else
>>  			gpio_set_value(EGPIO_MAGICIAN_LCD_POWER, 1);
>> -		mdelay(10);
>> -		gpio_set_value(GPIO106_MAGICIAN_LCD_POWER_3, 1);
>> -		mdelay(10);
>> -		gpio_set_value(GPIO104_MAGICIAN_LCD_POWER_1, 1);
>> -		mdelay(30);
>> -		gpio_set_value(GPIO105_MAGICIAN_LCD_POWER_2, 1);
>> -		mdelay(10);
>> +		mdelay(6);
>> +		gpio_set_value(GPIO106_MAGICIAN_LCD_DCDC_NRESET, 1);
>> +		mdelay(6);	/* Avdd -> Voff >5ms */
>> +		gpio_set_value(GPIO104_MAGICIAN_LCD_VOFF_EN, 1);
>> +		mdelay(16);	/* Voff -> Von >(5+10)ms */
>> +		gpio_set_value(GPIO105_MAGICIAN_LCD_VON_EN, 1);
>>  	} else {
>> -		mdelay(10);
>> -		gpio_set_value(GPIO105_MAGICIAN_LCD_POWER_2, 0);
>> -		mdelay(30);
>> -		gpio_set_value(GPIO104_MAGICIAN_LCD_POWER_1, 0);
>> -		mdelay(10);
>> -		gpio_set_value(GPIO106_MAGICIAN_LCD_POWER_3, 0);
>> -		mdelay(10);
>> +		gpio_set_value(GPIO105_MAGICIAN_LCD_VON_EN, 0);
>> +		mdelay(16);
>> +		gpio_set_value(GPIO104_MAGICIAN_LCD_VOFF_EN, 0);
>> +		mdelay(6);
>> +		gpio_set_value(GPIO106_MAGICIAN_LCD_DCDC_NRESET, 0);
>> +		mdelay(6);
> You're changing the timings here, right ? This should be an appart patch. What
> is the reason of the change by the way ?

Faster reaction, one change from msleep to mdelay (in toppoly LCD) and reading a datasheet for a valid sequence.

> 
> Cheers.
> 
Best regards,
Petr

[PATCH v2 05/21] ARM: pxa: magician: Add support for ADS7846 touchscreen

[Petr Cvek]

Add support for ADS7846 touchscreen.

Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
---
 arch/arm/mach-pxa/magician.c | 86 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

diff --git a/arch/arm/mach-pxa/magician.c b/arch/arm/mach-pxa/magician.c
index bc206f8..fc1c035 100644
--- a/arch/arm/mach-pxa/magician.c
+++ b/arch/arm/mach-pxa/magician.c
@@ -68,6 +68,7 @@
 #include <linux/pwm_backlight.h>
 #include <linux/regulator/driver.h>
 #include <linux/regulator/gpio-regulator.h>
+#include <linux/regulator/fixed.h>
 #include <linux/regulator/machine.h>
 #include <linux/usb/gpio_vbus.h>
 #include <linux/i2c/pxa-i2c.h>
@@ -84,6 +85,10 @@
 #include <linux/platform_data/irda-pxaficp.h>
 #include <linux/platform_data/usb-ohci-pxa27x.h>
 
+#include <linux/spi/spi.h>
+#include <linux/spi/pxa2xx_spi.h>
+#include <linux/spi/ads7846.h>
+
 #include "devices.h"
 #include "generic.h"
 
@@ -766,6 +771,37 @@ static struct platform_device bq24022 = {
 };
 
 /*
+ * fixed regulator for ads7846
+ */
+
+static struct regulator_consumer_supply ads7846_supply =
+	REGULATOR_SUPPLY("vcc", "spi2.0");
+
+static struct regulator_init_data vads7846_regulator = {
+	.constraints = {
+		.valid_ops_mask	= REGULATOR_CHANGE_STATUS,
+	},
+	.num_consumer_supplies = 1,
+	.consumer_supplies = &ads7846_supply,
+};
+
+static struct fixed_voltage_config vads7846 = {
+	.supply_name = "vads7846",
+	.microvolts = 3300000, /* probably */
+	.gpio = -EINVAL,
+	.startup_delay = 0,
+	.init_data = &vads7846_regulator,
+};
+
+static struct platform_device vads7846_device = {
+	.name = "reg-fixed-voltage",
+	.id = 1,
+	.dev = {
+		.platform_data = &vads7846,
+	},
+};
+
+/*
  * MMC/SD
  */
 
@@ -841,6 +877,51 @@ static struct i2c_pxa_platform_data i2c_info = {
 };
 
 /*
+ * Touchscreen
+ */
+
+static struct ads7846_platform_data ads7846_pdata = {
+	.model		= 7846,
+	.x_min		= 0,
+	.y_min		= 0,
+	.x_max		= 4096,
+	.y_max		= 4096,
+	.x_plate_ohms	= 320,	/* measured */
+	.y_plate_ohms	= 500,
+	.pressure_max	= 255,	/* up to 12bit sampling */
+	.debounce_max	= 10,
+	.debounce_tol	= 3,
+	.debounce_rep	= 1,
+	.gpio_pendown	= GPIO115_MAGICIAN_nPEN_IRQ,
+	.keep_vref_on	= 1,	/* FIXME, external Vref? */
+	.vref_delay_usecs = 100,
+	/* .wait_for_sync, GPIO77_LCD_BIAS low noise measure, latency! */
+};
+
+struct pxa2xx_spi_chip tsc2046_chip_info = {
+	.tx_threshold = 1,
+	.rx_threshold = 2,
+	.timeout = 64,
+	.gpio_cs = -1,
+};
+
+static struct pxa2xx_spi_master magician_spi_info = {
+	.num_chipselect = 1,
+/*	.enable_dma = 1, */ /* FIXME probably unnecessary */
+};
+
+static struct spi_board_info ads7846_spi_board_info[] __initdata = {
+	{
+		.modalias		= "ads7846",
+		.bus_num		= 2,
+		.max_speed_hz		= 1857143,
+		.platform_data		= &ads7846_pdata,
+		.controller_data	= &tsc2046_chip_info,
+		.irq = PXA_GPIO_TO_IRQ(GPIO115_MAGICIAN_nPEN_IRQ),
+	},
+};
+
+/*
  * Platform devices
  */
 
@@ -849,6 +930,7 @@ static struct platform_device *devices[] __initdata = {
 	&egpio,
 	&pwm_backlight,
 	&pasic3,
+	&vads7846_device,
 	&bq24022,
 	&gpio_vbus,
 	&power_supply,
@@ -915,6 +997,10 @@ static void __init magician_init(void)
 			lcd_select ? &samsung_info : &toppoly_info);
 	} else
 		pr_err("LCD detection: CPLD mapping failed\n");
+
+	pxa2xx_set_spi_info(2, &magician_spi_info);
+	spi_register_board_info(ARRAY_AND_SIZE(ads7846_spi_board_info));
+
 }
 
 MACHINE_START(MAGICIAN, "HTC Magician")
-- 
1.7.12.1

[PATCH v2 05/21] ARM: pxa: magician: Add support for ADS7846 touchscreen

[Robert Jarzmik]

Petr Cvek <petr.cvek@tul.cz> writes:

> Add support for ADS7846 touchscreen.
> +
> +static struct pxa2xx_spi_master magician_spi_info = {
> +	.num_chipselect = 1,
> +/*	.enable_dma = 1, */ /* FIXME probably unnecessary */
That's up to you. If almost nothing transits on SPI then a DMA is not necessary.

I'll leave that one for a week for others to further comment.

Cheers.

--
Robert

[PATCH v2 06/21] ARM: pxa: magician: Add normal and power I2C definition

[Petr Cvek]

Add normal and power I2C definition to HTC Magician.

Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
---
 arch/arm/mach-pxa/magician.c | 38 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-pxa/magician.c b/arch/arm/mach-pxa/magician.c
index fc1c035..702e8e0 100644
--- a/arch/arm/mach-pxa/magician.c
+++ b/arch/arm/mach-pxa/magician.c
@@ -72,6 +72,8 @@
 #include <linux/regulator/machine.h>
 #include <linux/usb/gpio_vbus.h>
 #include <linux/i2c/pxa-i2c.h>
+#include <linux/i2c-gpio.h>
+#include <linux/i2c.h>
 
 #include <mach/hardware.h>
 #include <asm/mach-types.h>
@@ -873,10 +875,39 @@ static struct platform_device strataflash = {
  */
 
 static struct i2c_pxa_platform_data i2c_info = {
-	.fast_mode = 1,
+	.fast_mode = 0,	/* fast mode seems to be have bit errors */
+	.use_pio = 0, /* no polling */
 };
 
 /*
+ * PXA I2C power controller
+ */
+
+static struct i2c_pxa_platform_data magician_i2c_power_info = {
+	.fast_mode = 0,
+	.use_pio = 0,
+};
+
+/*
+ * GPIO I2C normal controller (alternative)
+ */
+
+static struct i2c_gpio_platform_data rtc_device_data = {
+	.sda_pin = GPIO118_MAGICIAN_I2C_SDA,
+	.scl_pin = GPIO117_MAGICIAN_I2C_SCL,
+	.udelay = 100
+};
+
+static struct platform_device i2c_gpio_bus_alt = {
+	.name = "i2c-gpio",
+	.id = 0,
+	.dev = {
+		.platform_data = &rtc_device_data,
+	}
+};
+
+
+/*
  * Touchscreen
  */
 
@@ -936,6 +967,9 @@ static struct platform_device *devices[] __initdata = {
 	&power_supply,
 	&strataflash,
 	&leds_gpio,
+
+	/* NOTICE mutually exclusive with PXA I2C */
+	&i2c_gpio_bus_alt,
 };
 
 /*
@@ -975,7 +1009,7 @@ static void __init magician_init(void)
 	platform_add_devices(ARRAY_AND_SIZE(devices));
 
 	pxa_set_ficp_info(&magician_ficp_info);
-	pxa27x_set_i2c_power_info(NULL);
+	pxa27x_set_i2c_power_info(&magician_i2c_power_info);
 	pxa_set_i2c_info(&i2c_info);
 	pxa_set_mci_info(&magician_mci_info);
 	pxa_set_ohci_info(&magician_ohci_info);
-- 
1.7.12.1

[PATCH v2 06/21] ARM: pxa: magician: Add normal and power I2C definition

[Philipp Zabel]

Am Montag, den 17.08.2015, 23:59 +0200 schrieb Petr Cvek:
> Add normal and power I2C definition to HTC Magician.

What do you need i2c-gpio for? As I said before, I don't think this
should be in mainline, rather the pxa-i2c driver should be used.

>  
>  /*
> + * PXA I2C power controller
> + */
> +
> +static struct i2c_pxa_platform_data magician_i2c_power_info = {
> +	.fast_mode = 0,
> +	.use_pio = 0,
> +};
[...]
> @@ -975,7 +1009,7 @@ static void __init magician_init(void)
>  	platform_add_devices(ARRAY_AND_SIZE(devices));
>  
>  	pxa_set_ficp_info(&magician_ficp_info);
> -	pxa27x_set_i2c_power_info(NULL);
> +	pxa27x_set_i2c_power_info(&magician_i2c_power_info);

Does this actually have an effect?

regards
Philipp

[PATCH v2 06/21] ARM: pxa: magician: Add normal and power I2C definition

[Robert Jarzmik]

Petr Cvek <petr.cvek@tul.cz> writes:

> @@ -873,10 +875,39 @@ static struct platform_device strataflash = {
>   */
>  
>  static struct i2c_pxa_platform_data i2c_info = {
> -	.fast_mode = 1,
> +	.fast_mode = 0,	/* fast mode seems to be have bit errors */
> +	.use_pio = 0, /* no polling */
This deserves a better commit message : why the switch out of fast_mode, what
was not working, etc ...

> +/*
> + * GPIO I2C normal controller (alternative)
> + */
> +
> +static struct i2c_gpio_platform_data rtc_device_data = {
> +	.sda_pin = GPIO118_MAGICIAN_I2C_SDA,
> +	.scl_pin = GPIO117_MAGICIAN_I2C_SCL,
> +	.udelay = 100
> +};
> +
> +static struct platform_device i2c_gpio_bus_alt = {
> +	.name = "i2c-gpio",
> +	.id = 0,
> +	.dev = {
> +		.platform_data = &rtc_device_data,
rtc_device_data, really ?
Can you add __initdata, I think i2c-gpio copies the data and doesn't use it
anymore after ?

Cheers.

-- 
Robert

[PATCH v2 07/21] ARM: pxa: magician: Fix IrDA pdata and redundant GPIO request

[Petr Cvek]

Fix PXA FICP (IrDA) pdata and redundant GPIO request.

Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
---
 arch/arm/mach-pxa/magician.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-pxa/magician.c b/arch/arm/mach-pxa/magician.c
index 702e8e0..7519238 100644
--- a/arch/arm/mach-pxa/magician.c
+++ b/arch/arm/mach-pxa/magician.c
@@ -256,7 +256,8 @@ static unsigned long magician_pin_config[] __initdata = {
 static struct pxaficp_platform_data magician_ficp_info = {
 	.gpio_pwdown		= GPIO83_MAGICIAN_nIR_EN,
 	/* TODO test with FIR dongle */
-	.transceiver_cap	= IR_SIRMODE | IR_OFF,
+	.transceiver_cap	= IR_SIRMODE | IR_FIRMODE | IR_OFF,
+	.gpio_pwdown_inverted = 0,
 };
 
 /*
@@ -987,7 +988,6 @@ static struct gpio magician_global_gpios[] = {
 		"LCD VOFF enable" },
 	{ GPIO105_MAGICIAN_LCD_VON_EN, GPIOF_OUT_INIT_HIGH,
 		"LCD VON enable" },
-	{ GPIO83_MAGICIAN_nIR_EN, GPIOF_OUT_INIT_HIGH,	"nIR_EN" },
 };
 
 static void __init magician_init(void)
-- 
1.7.12.1

[PATCH v2 07/21] ARM: pxa: magician: Fix IrDA pdata and redundant GPIO request

[Robert Jarzmik]

Petr Cvek <petr.cvek@tul.cz> writes:

> Fix PXA FICP (IrDA) pdata and redundant GPIO request.
Fix what ? What is broken, what is not working before the patch and what will be
working after ? Could I have a better explanation in the commit message please ?

Cheers.

--
Robert

[PATCH v2 08/21] ARM: pxa: magician: Add StrataFlash Vpp GPIO and alternative driver

[Petr Cvek]

Add StrataFlash Vpp GPIO and alternative driver.

Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
---
 arch/arm/mach-pxa/magician.c | 76 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 73 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-pxa/magician.c b/arch/arm/mach-pxa/magician.c
index 7519238..7331b34 100644
--- a/arch/arm/mach-pxa/magician.c
+++ b/arch/arm/mach-pxa/magician.c
@@ -845,20 +845,66 @@ static struct pxaohci_platform_data magician_ohci_info = {
  * StrataFlash
  */
 
+static int magician_flash_init(struct platform_device *pdev)
+{
+	int ret = gpio_request(EGPIO_MAGICIAN_FLASH_VPP, "flash Vpp enable");
+
+	if (ret) {
+		pr_err("Cannot request flash enable GPIO (%i)\n", ret);
+		return ret;
+	}
+
+	ret = gpio_direction_output(EGPIO_MAGICIAN_FLASH_VPP, 1);
+	if (ret) {
+		pr_err("Cannot set direction for flash enable (%i)\n", ret);
+		gpio_free(EGPIO_MAGICIAN_FLASH_VPP);
+	}
+
+	return ret;
+}
+
 static void magician_set_vpp(struct platform_device *pdev, int vpp)
 {
 	gpio_set_value(EGPIO_MAGICIAN_FLASH_VPP, vpp);
 }
 
+static void magician_flash_exit(struct platform_device *pdev)
+{
+	gpio_free(EGPIO_MAGICIAN_FLASH_VPP);
+}
+
 static struct resource strataflash_resource = {
 	.start	= PXA_CS0_PHYS,
 	.end	= PXA_CS0_PHYS + SZ_64M - 1,
 	.flags	= IORESOURCE_MEM,
 };
 
+static struct mtd_partition magician_flash_parts[] = {
+	{
+		.name		= "Bootloader",
+		.offset		= 0x0,
+		.size		= 0x40000,
+		.mask_flags	= MTD_WRITEABLE,	/* EXPERIMENTAL */
+	},
+	{
+		.name	= "Linux Kernel",
+		.offset	= 0x40000,
+		.size	= MTDPART_SIZ_FULL,
+	},
+};
+
+#if 1	/* Choose which one fits you better */
+/*
+ * physmap-flash driver
+ */
+
 static struct physmap_flash_data strataflash_data = {
-	.width = 4,
-	.set_vpp = magician_set_vpp,
+	.width		= 4,
+	.init		= magician_flash_init,
+	.set_vpp	= magician_set_vpp,
+	.exit		= magician_flash_exit,
+	.parts		= magician_flash_parts,
+	.nr_parts	= ARRAY_SIZE(magician_flash_parts),
 };
 
 static struct platform_device strataflash = {
@@ -871,6 +917,30 @@ static struct platform_device strataflash = {
 	},
 };
 
+#else
+
+/*
+ * pxa2xx-flash driver
+ */
+
+static struct flash_platform_data magician_flash_data = {
+	.map_name	= "cfi_probe",
+	.parts		= magician_flash_parts,
+	.nr_parts	= ARRAY_SIZE(magician_flash_parts),
+	.width		= 4,
+};
+
+static struct platform_device strataflash = {
+	.name	= "pxa2xx-flash",
+	.id	= -1,
+	.dev = {
+		.platform_data = &magician_flash_data,
+	},
+	.resource = &strataflash_resource,
+	.num_resources = 1,
+};
+#endif
+
 /*
  * PXA I2C normal controller (main)
  */
@@ -960,13 +1030,13 @@ static struct spi_board_info ads7846_spi_board_info[] __initdata = {
 static struct platform_device *devices[] __initdata = {
 	&gpio_keys,
 	&egpio,
+	&strataflash,
 	&pwm_backlight,
 	&pasic3,
 	&vads7846_device,
 	&bq24022,
 	&gpio_vbus,
 	&power_supply,
-	&strataflash,
 	&leds_gpio,
 
 	/* NOTICE mutually exclusive with PXA I2C */
-- 
1.7.12.1

[PATCH v2 09/21] ARM: pxa: magician: Add OV9640 camera support

[Petr Cvek]

Add OV9640 camera support for HTC Magician.

Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
---
 arch/arm/mach-pxa/magician.c | 63 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/arch/arm/mach-pxa/magician.c b/arch/arm/mach-pxa/magician.c
index 7331b34..63604e1 100644
--- a/arch/arm/mach-pxa/magician.c
+++ b/arch/arm/mach-pxa/magician.c
@@ -91,6 +91,9 @@
 #include <linux/spi/pxa2xx_spi.h>
 #include <linux/spi/ads7846.h>
 
+#include <linux/platform_data/camera-pxa.h>
+#include <media/soc_camera.h>
+
 #include "devices.h"
 #include "generic.h"
 
@@ -634,6 +637,60 @@ static struct platform_device pasic3 = {
 };
 
 /*
+ * SoC Camera
+ */
+
+static struct pxacamera_platform_data magician_pxacamera_pdata = {
+	.flags = PXA_CAMERA_MASTER | PXA_CAMERA_DATAWIDTH_8 |
+		PXA_CAMERA_PCLK_EN | PXA_CAMERA_MCLK_EN,
+	.mclk_10khz = 4800,
+};
+
+static int magician_camera_power(struct device *dev, int power)
+{
+	pr_debug("Camera power = %i\n", power);
+
+	gpio_set_value(GPIO116_MAGICIAN_nCAM_EN, !power);
+	mdelay(3);
+
+	return 0;
+}
+
+static int magician_camera_reset(struct device *dev)
+{
+	pr_debug("Camera reset\n");
+
+	gpio_set_value(GPIO57_MAGICIAN_CAM_RESET, 1);
+	mdelay(3);
+	gpio_set_value(GPIO57_MAGICIAN_CAM_RESET, 0);
+	mdelay(3);
+
+	return 0;
+}
+
+static struct i2c_board_info magician_camera_i2c_board_info = {
+	I2C_BOARD_INFO("ov9640", 0x30),
+	.flags = I2C_CLIENT_SCCB,
+};
+
+static struct soc_camera_link magician_camera_iclink = {
+	.bus_id		= 0,
+	.flags		= SOCAM_DATAWIDTH_8,
+	.i2c_adapter_id	= 0,
+	.board_info	= &magician_camera_i2c_board_info,
+	.power		= magician_camera_power,
+	.reset		= magician_camera_reset,
+};
+
+static struct platform_device magician_camera = {
+	.name = "soc-camera-pdrv",
+	.id = 0,
+	.dev = {
+		.platform_data = &magician_camera_iclink,
+	},
+};
+
+/*
  * USB "Transceiver"
  */
 
@@ -1038,6 +1095,7 @@ static struct platform_device *devices[] __initdata = {
 	&gpio_vbus,
 	&power_supply,
 	&leds_gpio,
+	&magician_camera,
 
 	/* NOTICE mutually exclusive with PXA I2C */
 	&i2c_gpio_bus_alt,
@@ -1058,6 +1116,10 @@ static struct gpio magician_global_gpios[] = {
 		"LCD VOFF enable" },
 	{ GPIO105_MAGICIAN_LCD_VON_EN, GPIOF_OUT_INIT_HIGH,
 		"LCD VON enable" },
+	{ GPIO57_MAGICIAN_CAM_RESET, GPIOF_OUT_INIT_HIGH,
+		"Camera reset" },
+	{ GPIO116_MAGICIAN_nCAM_EN, GPIOF_OUT_INIT_HIGH,
+		"Camera power" },
 };
 
 static void __init magician_init(void)
@@ -1105,6 +1167,7 @@ static void __init magician_init(void)
 	pxa2xx_set_spi_info(2, &magician_spi_info);
 	spi_register_board_info(ARRAY_AND_SIZE(ads7846_spi_board_info));
 
+	pxa_set_camera_info(&magician_pxacamera_pdata);
 }
 
 MACHINE_START(MAGICIAN, "HTC Magician")
-- 
1.7.12.1

[PATCH v2 09/21] ARM: pxa: magician: Add OV9640 camera support

[Robert Jarzmik]

Petr Cvek <petr.cvek@tul.cz> writes:

> +	mdelay(3);
Okay, there are several mdelay(3) in this file.

Could you please test if removing these mdelays prevents the camera from working
? If not, I'd like to have them removed.

Cheers.

--
Robert

[PATCH v2 09/21] ARM: pxa: magician: Add OV9640 camera support

[Arnd Bergmann]

On Thursday 20 August 2015 21:48:20 Robert Jarzmik wrote:
> Petr Cvek <petr.cvek@tul.cz> writes:
> 
> > +     mdelay(3);
> Okay, there are several mdelay(3) in this file.
> 
> Could you please test if removing these mdelays prevents the camera from working
> ? If not, I'd like to have them removed.
> 
> 

FWIW, it might be possible to replace some of them with msleep() calls,
potentially after replacing spinlocks with mutexes where necessary.

	Arnd

[PATCH v2 09/21] ARM: pxa: magician: Add OV9640 camera support

[Petr Cvek]

Dne 20.8.2015 v 22:26 Arnd Bergmann napsal(a):
> On Thursday 20 August 2015 21:48:20 Robert Jarzmik wrote:
>> Petr Cvek <petr.cvek@tul.cz> writes:
>>
>>> +     mdelay(3);
>> Okay, there are several mdelay(3) in this file.
>>
>> Could you please test if removing these mdelays prevents the camera from working
>> ? If not, I'd like to have them removed.
>>
>>
> 
> FWIW, it might be possible to replace some of them with msleep() calls,
> potentially after replacing spinlocks with mutexes where necessary.
> 
> 	Arnd
> 

Datasheet says:

	tS:RESET	Setting time after software/hardware reset	1 ms

So at least one ~1 ms should be left there. Are msleep less than 20ms valid? 

(checkpatch: msleep < 20ms  can sleep for up to 20ms)

Petr

[PATCH v2 09/21] ARM: pxa: magician: Add OV9640 camera support

[Arnd Bergmann]

On Friday 21 August 2015 00:39:30 Petr Cvek wrote:
> Dne 20.8.2015 v 22:26 Arnd Bergmann napsal(a):
> > On Thursday 20 August 2015 21:48:20 Robert Jarzmik wrote:
> >> Petr Cvek <petr.cvek@tul.cz> writes:
> >>
> >>> +     mdelay(3);
> >> Okay, there are several mdelay(3) in this file.
> >>
> >> Could you please test if removing these mdelays prevents the camera from working
> >> ? If not, I'd like to have them removed.
> >>
> >>
> > 
> > FWIW, it might be possible to replace some of them with msleep() calls,
> > potentially after replacing spinlocks with mutexes where necessary.
> > 
> >       Arnd
> > 
> 
> Datasheet says:
> 
>         tS:RESET        Setting time after software/hardware reset      1 ms
> 
> So at least one ~1 ms should be left there. Are msleep less than 20ms valid? 
> 
> (checkpatch: msleep < 20ms  can sleep for up to 20ms)

On most kernels, an msleep(1) will sleep somewhere between 1 and 3 milliseconds
(but could be much longer), while an mdelay(1) tries to sleep around one
millisecond, more or less.

	Arnd

[PATCH v2 09/21] ARM: pxa: magician: Add OV9640 camera support

[Robert Jarzmik]

I shrunk a bit the mailing list.

Arnd Bergmann <arnd@arndb.de> writes:

> On Friday 21 August 2015 00:39:30 Petr Cvek wrote:
>> Dne 20.8.2015 v 22:26 Arnd Bergmann napsal(a):
>> > On Thursday 20 August 2015 21:48:20 Robert Jarzmik wrote:
>> >> Petr Cvek <petr.cvek@tul.cz> writes:
>> Datasheet says:
>> 
>>         tS:RESET        Setting time after software/hardware reset      1 ms
>> 
>> So at least one ~1 ms should be left there. Are msleep less than 20ms valid? 
>> 
>> (checkpatch: msleep < 20ms  can sleep for up to 20ms)
>
> On most kernels, an msleep(1) will sleep somewhere between 1 and 3 milliseconds
> (but could be much longer), while an mdelay(1) tries to sleep around one
> millisecond, more or less.

I have rethought of that a bit more. I'm convinced a delay of "at least 1ms" is
necessary according to the specification, it can also be more.

Moreover, I came to the conclusion this reset sequence is not something that is
"magician" specific (see palmz72_camera_reset() in
.../mach-pxa/palmz72.c). Actually it's not even mach-pxa specific, it's "ov9640"
specific.

Now add this to the fact that it would be good to have a solution working for
devicetree as well, and on any board, and the conclusion I came to was that this
handling deserves to be in ov9640 driver (please correct me if I'm wrong).

The idea behind it is have the reset handled in ov9640, and the gpio provided by
platform data or devicetree.

So Guennadi, is it possible to add a gpio through platform data to ov9640
driver, does it make sense, and would you accept to have the reset handled there
? And if yes, would you, Petr, accept to revamp your patch to have the reset and
power handled in ov9640 ?

Please note that it is a proposal, I'm not forcing anybody, I'd like to choose a
path that agrees with the future push to remove as many machine files as possible.

Cheers.

-- 
Robert

[PATCH v2 09/21] ARM: pxa: magician: Add OV9640 camera support

[Petr Cvek]

Dne 21.8.2015 v 19:36 Robert Jarzmik napsal(a):
> I shrunk a bit the mailing list.
> 
> Arnd Bergmann <arnd@arndb.de> writes:
> 
>> On Friday 21 August 2015 00:39:30 Petr Cvek wrote:
>>> Dne 20.8.2015 v 22:26 Arnd Bergmann napsal(a):
>>>> On Thursday 20 August 2015 21:48:20 Robert Jarzmik wrote:
>>>>> Petr Cvek <petr.cvek@tul.cz> writes:
>>> Datasheet says:
>>>
>>>         tS:RESET        Setting time after software/hardware reset      1 ms
>>>
>>> So at least one ~1 ms should be left there. Are msleep less than 20ms valid? 
>>>
>>> (checkpatch: msleep < 20ms  can sleep for up to 20ms)
>>
>> On most kernels, an msleep(1) will sleep somewhere between 1 and 3 milliseconds
>> (but could be much longer), while an mdelay(1) tries to sleep around one
>> millisecond, more or less.
> 
> I have rethought of that a bit more. I'm convinced a delay of "at least 1ms" is
> necessary according to the specification, it can also be more.
> 
> Moreover, I came to the conclusion this reset sequence is not something that is
> "magician" specific (see palmz72_camera_reset() in
> .../mach-pxa/palmz72.c). Actually it's not even mach-pxa specific, it's "ov9640"
> specific.
> 
> Now add this to the fact that it would be good to have a solution working for
> devicetree as well, and on any board, and the conclusion I came to was that this
> handling deserves to be in ov9640 driver (please correct me if I'm wrong).
> 
> The idea behind it is have the reset handled in ov9640, and the gpio provided by
> platform data or devicetree.
> 
> So Guennadi, is it possible to add a gpio through platform data to ov9640
> driver, does it make sense, and would you accept to have the reset handled there
> ? And if yes, would you, Petr, accept to revamp your patch to have the reset and
> power handled in ov9640 ?
> 

OK, why not, so power and reset gpio with polarity settings?

> Please note that it is a proposal, I'm not forcing anybody, I'd like to choose a
> path that agrees with the future push to remove as many machine files as possible.

Anyway I'm planning to send patch for OV9640 in future too (color correction matrix is not complete and some registers too).

> 
> Cheers.
> 

[PATCH v2 09/21] ARM: pxa: magician: Add OV9640 camera support

[Robert Jarzmik]

Petr Cvek <petr.cvek@tul.cz> writes:

>> So Guennadi, is it possible to add a gpio through platform data to ov9640
>> driver, does it make sense, and would you accept to have the reset handled there
>> ? And if yes, would you, Petr, accept to revamp your patch to have the reset and
>> power handled in ov9640 ?
>> 
>
> OK, why not, so power and reset gpio with polarity settings?
Even better pass ... nothing through platform-data.

I think there is now a gpio binding for devices which works this way :
 - in ov9640 : devm_gpiod_get(dev, "reset")
 - in magician: GPIO_LOOKUP() and gpiod_add_lookup_table()

In this way I think we'll gather at the same time :
 - no new platform data
 - ov9640 will follow the same path for devicetree and platform data

> Anyway I'm planning to send patch for OV9640 in future too (color correction
> matrix is not complete and some registers too).
Good.

Cheers.

-- 
Robert

[PATCH v2 09/21] ARM: pxa: magician: Add OV9640 camera support

[Lee Jones]

On Fri, 21 Aug 2015, Arnd Bergmann wrote:

> On Friday 21 August 2015 00:39:30 Petr Cvek wrote:
> > Dne 20.8.2015 v 22:26 Arnd Bergmann napsal(a):
> > > On Thursday 20 August 2015 21:48:20 Robert Jarzmik wrote:
> > >> Petr Cvek <petr.cvek@tul.cz> writes:
> > >>
> > >>> +     mdelay(3);
> > >> Okay, there are several mdelay(3) in this file.
> > >>
> > >> Could you please test if removing these mdelays prevents the camera from working
> > >> ? If not, I'd like to have them removed.
> > >>
> > >>
> > > 
> > > FWIW, it might be possible to replace some of them with msleep() calls,
> > > potentially after replacing spinlocks with mutexes where necessary.
> > > 
> > >       Arnd
> > > 
> > 
> > Datasheet says:
> > 
> >         tS:RESET        Setting time after software/hardware reset      1 ms
> > 
> > So at least one ~1 ms should be left there. Are msleep less than 20ms valid? 
> > 
> > (checkpatch: msleep < 20ms  can sleep for up to 20ms)
> 
> On most kernels, an msleep(1) will sleep somewhere between 1 and 3 milliseconds
> (but could be much longer), while an mdelay(1) tries to sleep around one
> millisecond, more or less.

If high-res timers are available I'd suggest using usleep_range()
instead.  By providing suitably divergent values usleep_range
de-restricts the kernel, allowing the scheduler to bundle your
wake-up along with others.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v2 10/21] ARM: pxa: magician: Add UDA1380 sound support

[Petr Cvek]

Add support for UDA1380 sound.

Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
---
 arch/arm/mach-pxa/magician.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/arch/arm/mach-pxa/magician.c b/arch/arm/mach-pxa/magician.c
index 63604e1..9decd86 100644
--- a/arch/arm/mach-pxa/magician.c
+++ b/arch/arm/mach-pxa/magician.c
@@ -94,6 +94,8 @@
 #include <linux/platform_data/camera-pxa.h>
 #include <media/soc_camera.h>
 
+#include <sound/uda1380.h>
+
 #include "devices.h"
 #include "generic.h"
 
@@ -887,6 +889,31 @@ static struct pxamci_platform_data magician_mci_info = {
 	.gpio_power	= EGPIO_MAGICIAN_SD_POWER,
 };
 
+/*
+ * AUDIO codec UDA1380
+ */
+
+static struct uda1380_platform_data uda1380_info = {
+	.gpio_power	= EGPIO_MAGICIAN_CODEC_POWER,
+	.gpio_reset	= EGPIO_MAGICIAN_CODEC_RESET,
+	.dac_clk	= UDA1380_DAC_CLK_WSPLL,
+};
+
+static struct i2c_board_info magician_audio_i2c_devices[] = {
+	{
+		I2C_BOARD_INFO("uda1380", 0x18),
+		.platform_data = &uda1380_info,
+	},
+};
+
+/*
+ * Magician Audio controller
+ */
+
+static struct platform_device magician_audio_device = {
+	.name = "magician-audio",
+	.id = -1,
+};
 
 /*
  * USB OHCI
@@ -1096,6 +1123,7 @@ static struct platform_device *devices[] __initdata = {
 	&power_supply,
 	&leds_gpio,
 	&magician_camera,
+	&magician_audio_device,
 
 	/* NOTICE mutually exclusive with PXA I2C */
 	&i2c_gpio_bus_alt,
@@ -1143,6 +1171,9 @@ static void __init magician_init(void)
 	pxa_set_ficp_info(&magician_ficp_info);
 	pxa27x_set_i2c_power_info(&magician_i2c_power_info);
 	pxa_set_i2c_info(&i2c_info);
+	i2c_register_board_info(0,
+		ARRAY_AND_SIZE(magician_audio_i2c_devices));
+
 	pxa_set_mci_info(&magician_mci_info);
 	pxa_set_ohci_info(&magician_ohci_info);
 
-- 
1.7.12.1

[PATCH v2 10/21] ARM: pxa: magician: Add UDA1380 sound support

[Robert Jarzmik]

Petr Cvek <petr.cvek@tul.cz> writes:

> +/*
> + * Magician Audio controller
> + */
> +
> +static struct platform_device magician_audio_device = {
> +	.name = "magician-audio",
Is this already upstream or is it a submission in progress ? Other than that
detail, I'll queue it in a week probably.

Cheers.

--
Robert

[PATCH v2 10/21] ARM: pxa: magician: Add UDA1380 sound support

[Petr Cvek]

Dne 20.8.2015 v 21:51 Robert Jarzmik napsal(a):
> Petr Cvek <petr.cvek@tul.cz> writes:
> 
>> +/*
>> + * Magician Audio controller
>> + */
>> +
>> +static struct platform_device magician_audio_device = {
>> +	.name = "magician-audio",
> Is this already upstream or is it a submission in progress ? Other than that
> detail, I'll queue it in a week probably.

This one is problematic. Vanilla version did not work from at least 3.4 (where I started, but probably even more in past). Somewhere between 3.13 and ~4.1 there is a change which causes it to fail after driver reload (bad pointers I think). UDA codec routes are not constructed completely and in ~4.1 they are refused, there are some swapped control registers and bad DMA definitions. Oh and recording stopped to work too (maybe it is caused by that routes).

I tried to send initial separate patches (for DMA and registers), but it got stuck somewhere (at least it is not in 4.2rcX and I sent them around start of this year).

I can add some of these patches to this patchset (but I didn't started yet on failing reload, record and fixing the routes).

> 
> Cheers.
> 
> --
> Robert
> 

[PATCH v2 10/21] ARM: pxa: magician: Add UDA1380 sound support

[Robert Jarzmik]

Petr Cvek <petr.cvek@tul.cz> writes:

> Dne 20.8.2015 v 21:51 Robert Jarzmik napsal(a):
>> Petr Cvek <petr.cvek@tul.cz> writes:
>> 
>>> +/*
>>> + * Magician Audio controller
>>> + */
>>> +
>>> +static struct platform_device magician_audio_device = {
>>> +	.name = "magician-audio",
>> Is this already upstream or is it a submission in progress ? Other than that
>> detail, I'll queue it in a week probably.
>
> This one is problematic. Vanilla version did not work from at least 3.4 (where I
> started, but probably even more in past). Somewhere between 3.13 and ~4.1 there
> is a change which causes it to fail after driver reload (bad pointers I
> think). UDA codec routes are not constructed completely and in ~4.1 they are
> refused, there are some swapped control registers and bad DMA definitions. Oh
> and recording stopped to work too (maybe it is caused by that routes).
>
> I tried to send initial separate patches (for DMA and registers), but it got
> stuck somewhere (at least it is not in 4.2rcX and I sent them around start of
> this year).
>
> I can add some of these patches to this patchset (but I didn't started yet on
> failing reload, record and fixing the routes).

Ok, but not for this patchset. This would be an apart submission to sound/soc
and Mark Brown more likely.
So I presume my answer is "no, it is not upstream yet, it is a submission under
review". In that case, I'll take it in my "held" tree until you tell me your
patches have been queued up by Mark, and then I'll move them to the for-next
tree.

Cheers.

-- 
Robert

PS: For DMA registers, there will also be an impact by the dmaengine
conversion. I'd like to be joined to the sound/soc submission, to foresee the
conflicts.

[PATCH v2 11/21] ARM: pxa: magician: Add MAX1586 Vcore regulator support

[Petr Cvek]

Add support for MAX1586 Vcore regulator.

Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
---
 arch/arm/mach-pxa/magician.c | 50 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/arch/arm/mach-pxa/magician.c b/arch/arm/mach-pxa/magician.c
index 9decd86..8ca0b67 100644
--- a/arch/arm/mach-pxa/magician.c
+++ b/arch/arm/mach-pxa/magician.c
@@ -96,6 +96,8 @@
 
 #include <sound/uda1380.h>
 
+#include <linux/regulator/max1586.h>
+
 #include "devices.h"
 #include "generic.h"
 
@@ -864,6 +866,52 @@ static struct platform_device vads7846_device = {
 };
 
 /*
+ * Vcore regulator MAX1587A
+ */
+
+static struct regulator_consumer_supply magician_max1587a_consumers[] = {
+	REGULATOR_SUPPLY("vcc_core", NULL),
+};
+
+static struct regulator_init_data magician_max1587a_v3_info = {
+	.constraints = {
+		.name		= "vcc_core range",
+		.min_uV		= 700000,
+		.max_uV		= 1500000,
+		.always_on	= 1,
+		.valid_ops_mask	= REGULATOR_CHANGE_VOLTAGE,
+	},
+	.consumer_supplies	= magician_max1587a_consumers,
+	.num_consumer_supplies	= ARRAY_SIZE(magician_max1587a_consumers),
+};
+
+static struct max1586_subdev_data magician_max1587a_subdevs[] = {
+	{
+		.name		= "vcc_core",
+		.id		= MAX1586_V3,
+		.platform_data	= &magician_max1587a_v3_info,
+	}
+};
+
+static struct max1586_platform_data magician_max1587a_info = {
+	.subdevs     = magician_max1587a_subdevs,
+	.num_subdevs = ARRAY_SIZE(magician_max1587a_subdevs),
+	/*
+	 * NOTICE measured directly on the PCB (board_id == 0x3a), but
+	 * if R24 is present, it will boost the voltage
+	 * (write 1.475V, get 1.645V and smoke)
+	 */
+	.v3_gain     = MAX1586_GAIN_NO_R24,
+};
+
+static struct i2c_board_info magician_pwr_i2c_board_info[] __initdata = {
+	{
+		I2C_BOARD_INFO("max1586", 0x14),
+		.platform_data	= &magician_max1587a_info,
+	},
+};
+
+/*
  * MMC/SD
  */
 
@@ -1173,6 +1221,8 @@ static void __init magician_init(void)
 	pxa_set_i2c_info(&i2c_info);
 	i2c_register_board_info(0,
 		ARRAY_AND_SIZE(magician_audio_i2c_devices));
+	i2c_register_board_info(1,
+		ARRAY_AND_SIZE(magician_pwr_i2c_board_info));
 
 	pxa_set_mci_info(&magician_mci_info);
 	pxa_set_ohci_info(&magician_ohci_info);
-- 
1.7.12.1

[PATCH v2 11/21] ARM: pxa: magician: Add MAX1586 Vcore regulator support

[Robert Jarzmik]

Petr Cvek <petr.cvek@tul.cz> writes:

> Add support for MAX1586 Vcore regulator.
>
> Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
I'll leave that for a week and if no objections queue it.

I would have expected a better commit message : for example, from your patch I
deduce magician has a 1587 regulator, not a 1586 one : that deserves a small
mention in the commit message. Moreover, the Maxim has 2 outputs : one is
connected to VCC_Core of the SoC, what about the other output ?  Is it unknown,
or do you know but don't use it ?

Cheers.

--
Robert

[PATCH v2 11/21] ARM: pxa: magician: Add MAX1586 Vcore regulator support

[Petr Cvek]

Dne 20.8.2015 v 21:32 Robert Jarzmik napsal(a):
> Petr Cvek <petr.cvek@tul.cz> writes:
> 
>> Add support for MAX1586 Vcore regulator.
>>
>> Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
> I'll leave that for a week and if no objections queue it.
> 
> I would have expected a better commit message : for example, from your patch I
> deduce magician has a 1587 regulator, not a 1586 one : that deserves a small
> mention in the commit message. Moreover, the Maxim has 2 outputs : one is
> connected to VCC_Core of the SoC, what about the other output ?  Is it unknown,
> or do you know but don't use it ?
> 
You mean V6 - Usim? MAX1587 does not have this pin implemented. I will note it in commit message.


> Cheers.
> 
> --
> Robert
> 

[PATCH v2 11/21] ARM: pxa: magician: Add MAX1586 Vcore regulator support

[Robert Jarzmik]

Petr Cvek <petr.cvek@tul.cz> writes:

> Dne 20.8.2015 v 21:32 Robert Jarzmik napsal(a):
>> Petr Cvek <petr.cvek@tul.cz> writes:
>> 
>>> Add support for MAX1586 Vcore regulator.
>>>
>>> Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
>> I'll leave that for a week and if no objections queue it.
>> 
>> I would have expected a better commit message : for example, from your patch I
>> deduce magician has a 1587 regulator, not a 1586 one : that deserves a small
>> mention in the commit message. Moreover, the Maxim has 2 outputs : one is
>> connected to VCC_Core of the SoC, what about the other output ?  Is it unknown,
>> or do you know but don't use it ?
>> 
> You mean V6 - Usim? MAX1587 does not have this pin implemented. I will note it
> in commit message.
Excellent, thanks.

Cheers.

-- 
Robert

[PATCH v2 12/21] ARM: pxa: magician: Add PXA27x UDC support

[Petr Cvek]

Add support for PXA27x UDC to HTC Magician.

Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
---
 arch/arm/mach-pxa/magician.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-pxa/magician.c b/arch/arm/mach-pxa/magician.c
index 8ca0b67..23b59a0 100644
--- a/arch/arm/mach-pxa/magician.c
+++ b/arch/arm/mach-pxa/magician.c
@@ -98,6 +98,10 @@
 
 #include <linux/regulator/max1586.h>
 
+#include <linux/platform_data/pxa2xx_udc.h>
+#include <mach/udc.h>
+#include <mach/pxa27x-udc.h>
+
 #include "devices.h"
 #include "generic.h"
 
@@ -698,6 +702,34 @@ static struct platform_device magician_camera = {
  * USB "Transceiver"
  */
 
+#if IS_ENABLED(CONFIG_USB_PXA27X)
+static void magician_udc_command(int cmd)
+{
+	if (cmd == PXA2XX_UDC_CMD_CONNECT)
+		UP2OCR |= UP2OCR_DPPUE | UP2OCR_DPPUBE;
+	else if (cmd == PXA2XX_UDC_CMD_DISCONNECT)
+		UP2OCR &= ~(UP2OCR_DPPUE | UP2OCR_DPPUBE);
+}
+
+/* HACK, shared USB connected state with pda-power */
+int my_usb_online = 1;
+
+static int magician_udc_is_connected(void)
+{
+	/* Shared with pda_power or gpio-vbus */
+	return my_usb_online;
+}
+
+static struct pxa2xx_udc_mach_info magician_udc_info __initdata = {
+	.udc_command		= magician_udc_command,
+	.udc_is_connected	= magician_udc_is_connected,
+	.gpio_pullup		= GPIO27_MAGICIAN_USBC_PUEN,
+};
+
+#else
+
+/* GPIO USB client vbus sensing, when no PXA UDC installed */
+
 static struct resource gpio_vbus_resource = {
 	.flags	= IORESOURCE_IRQ,
 	.start	= IRQ_MAGICIAN_VBUS,
@@ -718,6 +750,7 @@ static struct platform_device gpio_vbus = {
 		.platform_data = &gpio_vbus_info,
 	},
 };
+#endif	/* IS_ENABLED(CONFIG_USB_PXA27X) */
 
 /*
  * Charging functions
@@ -1167,7 +1200,6 @@ static struct platform_device *devices[] __initdata = {
 	&pasic3,
 	&vads7846_device,
 	&bq24022,
-	&gpio_vbus,
 	&power_supply,
 	&leds_gpio,
 	&magician_camera,
@@ -1175,6 +1207,11 @@ static struct platform_device *devices[] __initdata = {
 
 	/* NOTICE mutually exclusive with PXA I2C */
 	&i2c_gpio_bus_alt,
+
+	/* NOTICE mutually exclusive with UDC*/
+#if !(IS_ENABLED(CONFIG_USB_PXA27X))
+	&gpio_vbus,
+#endif
 };
 
 /*
@@ -1226,6 +1263,7 @@ static void __init magician_init(void)
 
 	pxa_set_mci_info(&magician_mci_info);
 	pxa_set_ohci_info(&magician_ohci_info);
+	pxa_set_udc_info(&magician_udc_info);
 
 	/* Check LCD type we have */
 	cpld = ioremap_nocache(PXA_CS3_PHYS, 0x1000);
-- 
1.7.12.1

[PATCH v2 12/21] ARM: pxa: magician: Add PXA27x UDC support

[Robert Jarzmik]

Petr Cvek <petr.cvek@tul.cz> writes:

> Add support for PXA27x UDC to HTC Magician.
>
> Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
> ---
>  arch/arm/mach-pxa/magician.c | 40 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-pxa/magician.c b/arch/arm/mach-pxa/magician.c
> index 8ca0b67..23b59a0 100644
> --- a/arch/arm/mach-pxa/magician.c
> +++ b/arch/arm/mach-pxa/magician.c
> @@ -98,6 +98,10 @@
>  
>  #include <linux/regulator/max1586.h>
>  
> +#include <linux/platform_data/pxa2xx_udc.h>
> +#include <mach/udc.h>
> +#include <mach/pxa27x-udc.h>
> +
>  #include "devices.h"
>  #include "generic.h"
>  
> @@ -698,6 +702,34 @@ static struct platform_device magician_camera = {
>   * USB "Transceiver"
>   */
>  
> +#if IS_ENABLED(CONFIG_USB_PXA27X)
No #if please, just let it out of #ifdefery, no need to.

> +static void magician_udc_command(int cmd)
> +{
> +	if (cmd == PXA2XX_UDC_CMD_CONNECT)
> +		UP2OCR |= UP2OCR_DPPUE | UP2OCR_DPPUBE;
> +	else if (cmd == PXA2XX_UDC_CMD_DISCONNECT)
> +		UP2OCR &= ~(UP2OCR_DPPUE | UP2OCR_DPPUBE);
> +}
> +
> +/* HACK, shared USB connected state with pda-power */
> +int my_usb_online = 1;
Definitely not, a global (ie. non static variable) is not something I'll let
in. Besides, can't a "gpio_get_value()" give the same level of information ?

> +static int magician_udc_is_connected(void)
> +{
> +	/* Shared with pda_power or gpio-vbus */
> +	return my_usb_online;
gpio_get_value() something ?

> +}
> +
> +static struct pxa2xx_udc_mach_info magician_udc_info __initdata = {
> +	.udc_command		= magician_udc_command,
> +	.udc_is_connected	= magician_udc_is_connected,
I don't think udc_is_connected is a field used by pxa27x_udc.c, it it ? The VBus
connection information comes from a transciever, usually from gpio_vbus driver
nowadays.

> @@ -1175,6 +1207,11 @@ static struct platform_device *devices[] __initdata = {
>  
>  	/* NOTICE mutually exclusive with PXA I2C */
>  	&i2c_gpio_bus_alt,
> +
> +	/* NOTICE mutually exclusive with UDC*/
Euh why so ?
In arch/arm/mach-pxa/mioa701.c they cooperate, why can't they for magician ?

Cheers.

-- 
Robert

[PATCH v2 12/21] ARM: pxa: magician: Add PXA27x UDC support

[Petr Cvek]

Dne 20.8.2015 v 19:23 Robert Jarzmik napsal(a):
> Petr Cvek <petr.cvek@tul.cz> writes:
> 
>> Add support for PXA27x UDC to HTC Magician.
>>
>> Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
>> ---
>>  arch/arm/mach-pxa/magician.c | 40 +++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/mach-pxa/magician.c b/arch/arm/mach-pxa/magician.c
>> index 8ca0b67..23b59a0 100644
>> --- a/arch/arm/mach-pxa/magician.c
>> +++ b/arch/arm/mach-pxa/magician.c
>> @@ -98,6 +98,10 @@
>>  
>>  #include <linux/regulator/max1586.h>
>>  
>> +#include <linux/platform_data/pxa2xx_udc.h>
>> +#include <mach/udc.h>
>> +#include <mach/pxa27x-udc.h>
>> +
>>  #include "devices.h"
>>  #include "generic.h"
>>  
>> @@ -698,6 +702,34 @@ static struct platform_device magician_camera = {
>>   * USB "Transceiver"
>>   */
>>  
>> +#if IS_ENABLED(CONFIG_USB_PXA27X)
> No #if please, just let it out of #ifdefery, no need to.
> 
>> +static void magician_udc_command(int cmd)
>> +{
>> +	if (cmd == PXA2XX_UDC_CMD_CONNECT)
>> +		UP2OCR |= UP2OCR_DPPUE | UP2OCR_DPPUBE;
>> +	else if (cmd == PXA2XX_UDC_CMD_DISCONNECT)
>> +		UP2OCR &= ~(UP2OCR_DPPUE | UP2OCR_DPPUBE);
>> +}
>> +
>> +/* HACK, shared USB connected state with pda-power */
>> +int my_usb_online = 1;
> Definitely not, a global (ie. non static variable) is not something I'll let
> in. Besides, can't a "gpio_get_value()" give the same level of information ?

I will gladly remove it, but I want my phone to be able to charge and work as USB device (and maybe as USB host).

> 
>> +static int magician_udc_is_connected(void)
>> +{
>> +	/* Shared with pda_power or gpio-vbus */
>> +	return my_usb_online;
> gpio_get_value() something ?
> 

I need to use EGPIO_MAGICIAN_CABLE_INSERT1 GPIO in pda_power to be able to detect source for charging.

>> +}
>> +
>> +static struct pxa2xx_udc_mach_info magician_udc_info __initdata = {
>> +	.udc_command		= magician_udc_command,
>> +	.udc_is_connected	= magician_udc_is_connected,
> I don't think udc_is_connected is a field used by pxa27x_udc.c, it it ? The VBus
> connection information comes from a transciever, usually from gpio_vbus driver
> nowadays.

Really? Well I guess I took a bad route :-D (when I tried to learn how to add an udc support). As there are two ways to define udc_is_connected, would it be possible to remove this field altogether (only mioa701 and lubbock have relevant use, but if this function is never called, then it is dead code)?

Do you know which code / source is responsible for gpio_vbus -> pxa27x_udc communication about inserted cable? I will need to put some assert for testing.

> 
>> @@ -1175,6 +1207,11 @@ static struct platform_device *devices[] __initdata = {
>>  
>>  	/* NOTICE mutually exclusive with PXA I2C */
>>  	&i2c_gpio_bus_alt,
>> +
>> +	/* NOTICE mutually exclusive with UDC*/
> Euh why so ?
> In arch/arm/mach-pxa/mioa701.c they cooperate, why can't they for magician ?

In mioa701 the GPIO13_nUSB_DETECT GPIO is requested by gpio_vbus driver, used in pda_power (without request) and "used" in PXA UDC driver without request. It is basically similar solution as my (ugly) global variable.

Solution: I will try to use that second new GPIO (EGPIO_MAGICIAN_CABLE_INSERT2) for that, so there is no GPIO request dependency. I may create new names for them (magician.h).

> 
> Cheers.
> 
Best regards,
Petr

[PATCH v2 12/21] ARM: pxa: magician: Add PXA27x UDC support

[Robert Jarzmik]

Petr Cvek <petr.cvek@tul.cz> writes:

> Dne 20.8.2015 v 19:23 Robert Jarzmik napsal(a):
>> Petr Cvek <petr.cvek@tul.cz> writes:
>>> +static void magician_udc_command(int cmd)
>>> +{
>>> +	if (cmd == PXA2XX_UDC_CMD_CONNECT)
>>> +		UP2OCR |= UP2OCR_DPPUE | UP2OCR_DPPUBE;
>>> +	else if (cmd == PXA2XX_UDC_CMD_DISCONNECT)
>>> +		UP2OCR &= ~(UP2OCR_DPPUE | UP2OCR_DPPUBE);
>>> +}
>>> +
>>> +/* HACK, shared USB connected state with pda-power */
>>> +int my_usb_online = 1;
>> Definitely not, a global (ie. non static variable) is not something I'll let
>> in. Besides, can't a "gpio_get_value()" give the same level of information ?
>
> I will gladly remove it, but I want my phone to be able to charge and work as
> USB device (and maybe as USB host).
Okay, I understand that, but see below.

>> 
>>> +static int magician_udc_is_connected(void)
>>> +{
>>> +	/* Shared with pda_power or gpio-vbus */
>>> +	return my_usb_online;
>> gpio_get_value() something ?
>> 
>
> I need to use EGPIO_MAGICIAN_CABLE_INSERT1 GPIO in pda_power to be able to detect source for charging.
>
>>> +}
>>> +
>>> +static struct pxa2xx_udc_mach_info magician_udc_info __initdata = {
>>> +	.udc_command		= magician_udc_command,
>>> +	.udc_is_connected	= magician_udc_is_connected,
>> I don't think udc_is_connected is a field used by pxa27x_udc.c, it it ? The VBus
>> connection information comes from a transciever, usually from gpio_vbus driver
>> nowadays.
>
> Really? Well I guess I took a bad route :-D (when I tried to learn how to add an
> udc support). As there are two ways to define udc_is_connected, would it be
> possible to remove this field altogether (only mioa701 and lubbock have relevant
> use, but if this function is never called, then it is dead code)?
>
> Do you know which code / source is responsible for gpio_vbus -> pxa27x_udc
> communication about inserted cable? I will need to put some assert for
> testing.
Yes.
 - hooking: udc->transceiver = usb_get_phy(USB_PHY_TYPE_USB2);
 - detection: gpio_vbus_work()
   -> usb_gadget_vbus_connect()
     -> pxa_udc_vbus_session()

>>> @@ -1175,6 +1207,11 @@ static struct platform_device *devices[] __initdata = {
>>>  
>>>  	/* NOTICE mutually exclusive with PXA I2C */
>>>  	&i2c_gpio_bus_alt,
>>> +
>>> +	/* NOTICE mutually exclusive with UDC*/
>> Euh why so ?
>> In arch/arm/mach-pxa/mioa701.c they cooperate, why can't they for magician ?
>
> In mioa701 the GPIO13_nUSB_DETECT GPIO is requested by gpio_vbus driver, used in
> pda_power (without request) and "used" in PXA UDC driver without request. It is
> basically similar solution as my (ugly) global variable.
>
> Solution: I will try to use that second new GPIO (EGPIO_MAGICIAN_CABLE_INSERT2)
> for that, so there is no GPIO request dependency. I may create new names for
> them (magician.h).
Cool.
And if by chance you find an even cleaner solution, don't hesitate to modify
mioa701, hein ? ;)

Cheers.

-- 
Robert

[PATCH v2 13/21] ARM: pxa: magician: Fix charging source and add NiCd backup charging

[Petr Cvek]

Fix charging (cable insert functionality shared with UDC). Add charging
control for RTC NiCd accu.

Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
---
 arch/arm/mach-pxa/magician.c | 83 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 73 insertions(+), 10 deletions(-)

diff --git a/arch/arm/mach-pxa/magician.c b/arch/arm/mach-pxa/magician.c
index 23b59a0..50c5325 100644
--- a/arch/arm/mach-pxa/magician.c
+++ b/arch/arm/mach-pxa/magician.c
@@ -756,18 +756,79 @@ static struct platform_device gpio_vbus = {
  * Charging functions
  */
 
-static int power_supply_init(struct device *dev)
+static int magician_supply_init(struct device *dev)
 {
-	return gpio_request(EGPIO_MAGICIAN_CABLE_TYPE, "CABLE_STATE_AC");
+	int ret = -1;
+
+	ret = gpio_request(EGPIO_MAGICIAN_CABLE_TYPE, "AC charger in USB");
+	if (ret) {
+		pr_err("Cannot request AC/USB charger GPIO (%i)\n", ret);
+		goto err_ac;
+	}
+
+	ret = gpio_request(EGPIO_MAGICIAN_CABLE_INSERT1, "Cable inserted");
+	if (ret) {
+		pr_err("Cannot request cable detection GPIO (%i)\n", ret);
+		goto err_usb;
+	}
+
+	return 0;
+
+err_usb:
+	gpio_free(EGPIO_MAGICIAN_CABLE_TYPE);
+err_ac:
+	return ret;
+}
+
+static void magician_set_charge(int flags)
+{
+	/* EGPIO_MAGICIAN_BQ24022_ISET2: =1 500mA, =0 100mA
+	 *
+	 * RTC NiCd voltage can be read from ADS7846
+	 * in0_input=1930, when =0 (after =1 its going up)
+	 * in0_input=1367, when =1
+	 * in0_input=2499, always (without NiCd)
+	 *
+	 * FIXME Will disabled NiCd slow accu discharge?
+	 *
+	 * Charging from USB can be 500mA too
+	 * NOTICE: IrDA=on, Vcore=1V, Fcore=104MHz, everything other=off
+	 *   -> power consumption still over 100mA
+	 */
+
+	if (flags & PDA_POWER_CHARGE_AC) {
+		pr_debug("Charging from AC\n");
+		gpio_set_value(EGPIO_MAGICIAN_NICD_CHARGE, 1);
+	} else if (flags & PDA_POWER_CHARGE_USB) {
+		pr_debug("Charging from USB\n");
+		gpio_set_value(EGPIO_MAGICIAN_NICD_CHARGE, 1);
+	} else {
+		pr_debug("Charging disabled\n");
+		gpio_set_value(EGPIO_MAGICIAN_NICD_CHARGE, 0);
+	}
+
+	gpio_set_value(EGPIO_MAGICIAN_NICD_CHARGE, 1);
 }
 
 static int magician_is_ac_online(void)
 {
-	return gpio_get_value(EGPIO_MAGICIAN_CABLE_INSERT1);
+	return gpio_get_value(EGPIO_MAGICIAN_CABLE_INSERT1) &&
+		gpio_get_value(EGPIO_MAGICIAN_CABLE_TYPE); /* AC=1 */
+}
+
+static int magician_is_usb_online(void)
+{
+	my_usb_online = gpio_get_value(EGPIO_MAGICIAN_CABLE_INSERT1) &&
+		(!gpio_get_value(EGPIO_MAGICIAN_CABLE_TYPE)); /* USB=0 */
+	return my_usb_online;
 }
 
-static void power_supply_exit(struct device *dev)
+static void magician_supply_exit(struct device *dev)
 {
+	/* HACK FIXME EGPIO pin is used with two drivers */
+	my_usb_online = 1;
+
+	gpio_free(EGPIO_MAGICIAN_CABLE_INSERT1);
 	gpio_free(EGPIO_MAGICIAN_CABLE_TYPE);
 }
 
@@ -780,11 +841,13 @@ static char *magician_supplicants[] = {
  */
 
 static struct pda_power_pdata power_supply_info = {
-	.init		= power_supply_init,
-	.is_ac_online	= magician_is_ac_online,
-	.exit		= power_supply_exit,
-	.supplied_to	= magician_supplicants,
-	.num_supplicants = ARRAY_SIZE(magician_supplicants),
+	.init			= magician_supply_init,
+	.exit			= magician_supply_exit,
+	.is_ac_online		= magician_is_ac_online,
+	.is_usb_online		= magician_is_usb_online,
+	.set_charge		= magician_set_charge,
+	.supplied_to		= magician_supplicants,
+	.num_supplicants	= ARRAY_SIZE(magician_supplicants),
 };
 
 static struct resource power_supply_resources[] = {
@@ -847,7 +910,7 @@ static struct gpio_regulator_config bq24022_info = {
 
 	.enable_gpio = GPIO30_MAGICIAN_BQ24022_nCHARGE_EN,
 	.enable_high = 0,
-	.enabled_at_boot = 0,
+	.enabled_at_boot = 1,
 
 	.gpios = bq24022_gpios,
 	.nr_gpios = ARRAY_SIZE(bq24022_gpios),
-- 
1.7.12.1

[PATCH v2 13/21] ARM: pxa: magician: Fix charging source and add NiCd backup charging

[Petr Cvek]

Dne 18.8.2015 v 00:03 Petr Cvek napsal(a):
> +static int magician_is_usb_online(void)
> +{
> +	my_usb_online = gpio_get_value(EGPIO_MAGICIAN_CABLE_INSERT1) &&
> +		(!gpio_get_value(EGPIO_MAGICIAN_CABLE_TYPE)); /* USB=0 */
> +	return my_usb_online;
>  }

This patch requires my_usb_online variable defined by 

	[PATCH v2 12/21] ARM: pxa: magician: Add PXA27x UDC support

because both (UDC and charging) require same EGPIO.


Petr

[PATCH v2 14/21] ARM: pxa: magician: Fix PXA USB OHCI port enable flags

[Petr Cvek]

Fix PXA USB OHCI port enable flags.

Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
---
 arch/arm/mach-pxa/magician.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-pxa/magician.c b/arch/arm/mach-pxa/magician.c
index 50c5325..d17125e 100644
--- a/arch/arm/mach-pxa/magician.c
+++ b/arch/arm/mach-pxa/magician.c
@@ -1065,8 +1065,10 @@ static struct platform_device magician_audio_device = {
 
 static struct pxaohci_platform_data magician_ohci_info = {
 	.port_mode	= PMM_PERPORT_MODE,
-	.flags		= ENABLE_PORT1 | ENABLE_PORT3 | POWER_CONTROL_LOW,
+	/* port1: CSR Bluetooth, port2: OTG with UDC */
+	.flags		= ENABLE_PORT1 | ENABLE_PORT2 | POWER_CONTROL_LOW,
 	.power_budget	= 0,
+	.power_on_delay = 100,
 };
 
 /*
-- 
1.7.12.1

[PATCH v2 15/21] ARM: pxa: magician: Fix PWM backlight regulator

[Petr Cvek]

Fix PWM backlight regulator. It is requested from pwm_backlight_probe().

Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
---
 arch/arm/mach-pxa/magician.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm/mach-pxa/magician.c b/arch/arm/mach-pxa/magician.c
index d17125e..2ec630e 100644
--- a/arch/arm/mach-pxa/magician.c
+++ b/arch/arm/mach-pxa/magician.c
@@ -542,6 +542,14 @@ static struct platform_device pwm_backlight = {
 };
 
 /*
+ * fixed regulator for pwm_backlight
+ */
+
+static struct regulator_consumer_supply pwm_backlight_supply[] = {
+	REGULATOR_SUPPLY("power", "pwm_backlight"),
+};
+
+/*
  * LEDs
  */
 
@@ -1352,6 +1360,9 @@ static void __init magician_init(void)
 	spi_register_board_info(ARRAY_AND_SIZE(ads7846_spi_board_info));
 
 	pxa_set_camera_info(&magician_pxacamera_pdata);
+
+	regulator_register_always_on(0, "power", pwm_backlight_supply,
+		ARRAY_SIZE(pwm_backlight_supply), 5000000);
 }
 
 MACHINE_START(MAGICIAN, "HTC Magician")
-- 
1.7.12.1

[PATCH v2 15/21] ARM: pxa: magician: Fix PWM backlight regulator

[Robert Jarzmik]

Petr Cvek <petr.cvek@tul.cz> writes:

> Fix PWM backlight regulator. It is requested from pwm_backlight_probe().
Even if I know why you have to fix it, somebody else reading your commit message
should understand why this fix is need. You need to explain why it is needed,
ie. what happens without it, and therefore what justifies this patch.

Cheers.

--
Robert

PS: For inspiration, look at Thierry's commit 902e6a0c7eba.

[PATCH v2 16/21] ARM: pxa: magician: Add support for alternative LCD backlight

[Petr Cvek]

Add support for alternative LCD backlight with GPIO (no brightness).

Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
---
 arch/arm/mach-pxa/magician.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-pxa/magician.c b/arch/arm/mach-pxa/magician.c
index 2ec630e..780cc3d 100644
--- a/arch/arm/mach-pxa/magician.c
+++ b/arch/arm/mach-pxa/magician.c
@@ -102,6 +102,8 @@
 #include <mach/udc.h>
 #include <mach/pxa27x-udc.h>
 
+#include <linux/platform_data/gpio_backlight.h>
+
 #include "devices.h"
 #include "generic.h"
 
@@ -549,6 +551,21 @@ static struct regulator_consumer_supply pwm_backlight_supply[] = {
 	REGULATOR_SUPPLY("power", "pwm_backlight"),
 };
 
+/* Backlight controlled by single GPIO (alternative) */
+static struct gpio_backlight_platform_data gpio_backlight_data = {
+/* .fbdev = &pxa_device_fb.dev, */
+	.gpio		= EGPIO_MAGICIAN_BL_POWER,
+	.def_value	= 1,
+	.name		= "backlight",
+};
+
+static struct platform_device gpio_backlight = {
+	.name = "gpio-backlight",
+	.dev = {
+		.platform_data = &gpio_backlight_data,
+	},
+};
+
 /*
  * LEDs
  */
@@ -1269,7 +1286,6 @@ static struct platform_device *devices[] __initdata = {
 	&gpio_keys,
 	&egpio,
 	&strataflash,
-	&pwm_backlight,
 	&pasic3,
 	&vads7846_device,
 	&bq24022,
@@ -1285,6 +1301,10 @@ static struct platform_device *devices[] __initdata = {
 #if !(IS_ENABLED(CONFIG_USB_PXA27X))
 	&gpio_vbus,
 #endif
+
+	/* NOTICE mutually exclusive */
+	&pwm_backlight,
+	&gpio_backlight,
 };
 
 /*
-- 
1.7.12.1

[PATCH v2 16/21] ARM: pxa: magician: Add support for alternative LCD backlight

[Robert Jarzmik]

Petr Cvek <petr.cvek@tul.cz> writes:

> Add support for alternative LCD backlight with GPIO (no brightness).
Here, I don't understand, the commit message is too short.

Are there 2 brightness controls on magician, or are these 2 different magicians
(hardware wise), each having a different backlight control ? Or is it there is a
GPIO to light up the screen and a PWM backlight ?

I have to understand first.

Cheers.

--
Robert

[PATCH v2 16/21] ARM: pxa: magician: Add support for alternative LCD backlight

[Petr Cvek]

Dne 20.8.2015 v 22:01 Robert Jarzmik napsal(a):
> Petr Cvek <petr.cvek@tul.cz> writes:
> 
>> Add support for alternative LCD backlight with GPIO (no brightness).
> Here, I don't understand, the commit message is too short.
> 
> Are there 2 brightness controls on magician, or are these 2 different magicians
> (hardware wise), each having a different backlight control ? Or is it there is a
> GPIO to light up the screen and a PWM backlight ?
> 
> I have to understand first.
> 
I have aimed for a configuration, where user can use PWM xor GPIO backlight. As pin mux can set GPIO or PWM (or UART :-D) on PWM pin, you can choose if you spare few kB (as magician has only 64MB RAM and 64MB flash) on GPIO or will have smooth backlight with PWM.

I have tested both (I think I have added the gpio backlight for debugging a regression in pwm_bl).

Only ugly thing is the GPIO definition:

#if IS_ENABLED(CONFIG_PWM_PXA)
	/* PWM 0 - LCD backlight */
	GPIO16_PWM0_OUT,
#else
	/* Ensure static backlight without any driver */
	MFP_CFG_OUT(GPIO16, AF0, DRIVE_LOW),    /* Backlight enabled */
#endif

Hmmm.. I think I can do this better in pwm-backlight init/exit (same way as pxa_ficp). During init the pin mux will switch to Alternate Function (PWM) and during exit it will switch to GPIO (which can be used by gpio-backlight). This will remove ugly ifdef.

Best regards,
Petr

[PATCH v2 16/21] ARM: pxa: magician: Add support for alternative LCD backlight

[Robert Jarzmik]

Petr Cvek <petr.cvek@tul.cz> writes:

> Dne 20.8.2015 v 22:01 Robert Jarzmik napsal(a):
>> Petr Cvek <petr.cvek@tul.cz> writes:
>> 
>>> Add support for alternative LCD backlight with GPIO (no brightness).
>> Here, I don't understand, the commit message is too short.
>> 
>> Are there 2 brightness controls on magician, or are these 2 different magicians
>> (hardware wise), each having a different backlight control ? Or is it there is a
>> GPIO to light up the screen and a PWM backlight ?
>> 
>> I have to understand first.
>> 
> I have aimed for a configuration, where user can use PWM xor GPIO backlight.
You probably know that in a backlight case GPIO is just a subcase of PWM, with
frequency equal to either 0 or max_backlight_freq.

> As pin mux can set GPIO or PWM (or UART :-D) on PWM pin, you can choose if you
> spare few kB (as magician has only 64MB RAM and 64MB flash) on GPIO or will
> have smooth backlight with PWM.

I'm in front of the same dilemna for 5 years ago with the mioa701 (64MB RAM, 2 *
32MB flash). I chose PWM, judging the PWM stack was worth it.

Moreover, pwm can be a module, loaded or not : what memory do you spare in that
case if you don't load the module ?

> I have tested both (I think I have added the gpio backlight for debugging a regression in pwm_bl).
> Only ugly thing is the GPIO definition:
>
> #if IS_ENABLED(CONFIG_PWM_PXA)
> 	/* PWM 0 - LCD backlight */
> 	GPIO16_PWM0_OUT,
> #else
> 	/* Ensure static backlight without any driver */
> 	MFP_CFG_OUT(GPIO16, AF0, DRIVE_LOW),    /* Backlight enabled */
> #endif
>
> Hmmm.. I think I can do this better in pwm-backlight init/exit (same way as
> pxa_ficp). During init the pin mux will switch to Alternate Function (PWM) and
> during exit it will switch to GPIO (which can be used by gpio-backlight). This
> will remove ugly ifdef.

I don't like changing the muxing after initialization that much : the pin muxing
should represent what there is in hardware, and that is a backlight frequency
switched input pin. The one exception is for hardware workarounds such as the
AC97 reset pin.

I won't take an init/exit with a manipulation of an mfp. I'll let you choose one
way instead :
 - either the GPIO
 - or the PWM (module or not)
Don't forget that this init/exit thing won't be available for a devicetree
magician (if that happens).

Cheers.

-- 
Robert

[PATCH v2 17/21] ARM: pxa: magician: Remove (temporarily) pasic3 LED support

[Petr Cvek]

Temporarily remove pasic3 LEDs definition as it is not used (yet).

Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
---
 arch/arm/mach-pxa/magician.c | 45 ++------------------------------------------
 1 file changed, 2 insertions(+), 43 deletions(-)

diff --git a/arch/arm/mach-pxa/magician.c b/arch/arm/mach-pxa/magician.c
index 780cc3d..d8994dd 100644
--- a/arch/arm/mach-pxa/magician.c
+++ b/arch/arm/mach-pxa/magician.c
@@ -567,7 +567,7 @@ static struct platform_device gpio_backlight = {
 };
 
 /*
- * LEDs
+ * Phone keys backlight, vibra
  */
 
 static struct gpio_led gpio_leds[] = {
@@ -597,46 +597,6 @@ static struct platform_device leds_gpio = {
 };
 
 /*
- * PASIC3 LEDs
- */
-
-static struct pasic3_led pasic3_leds[] = {
-	{
-		.led = {
-			.name = "magician:red",
-			.default_trigger = "ds2760-battery.0-charging",
-		},
-		.hw_num	= 0,
-		.bit2	= PASIC3_BIT2_LED0,
-		.mask	= PASIC3_MASK_LED0,
-	},
-	{
-		.led = {
-			.name = "magician:green",
-			.default_trigger = "ds2760-battery.0-charging-or-full",
-		},
-		.hw_num	= 1,
-		.bit2	= PASIC3_BIT2_LED1,
-		.mask	= PASIC3_MASK_LED1,
-	},
-	{
-		.led = {
-			.name = "magician:blue",
-			.default_trigger = "bluetooth",
-		},
-		.hw_num	= 2,
-		.bit2	= PASIC3_BIT2_LED2,
-		.mask	= PASIC3_MASK_LED2,
-	},
-};
-
-static struct pasic3_leds_machinfo pasic3_leds_info = {
-	.num_leds	= ARRAY_SIZE(pasic3_leds),
-	.power_gpio	= EGPIO_MAGICIAN_LED_POWER,
-	.leds		= pasic3_leds,
-};
-
-/*
  * PASIC3 DS1WM
  */
 
@@ -656,7 +616,7 @@ static struct resource pasic3_resources[] = {
 
 static struct pasic3_platform_data pasic3_platform_data = {
 	.clock_rate	= 4000000,
-	.led_pdata	= &pasic3_leds_info,
+/*	.led_pdata	= &pasic3_leds_info, */
 };
 
 static struct platform_device pasic3 = {
@@ -1232,7 +1192,6 @@ static struct platform_device i2c_gpio_bus_alt = {
 	}
 };
 
-
 /*
  * Touchscreen
  */
-- 
1.7.12.1

[PATCH v2 17/21] ARM: pxa: magician: Remove (temporarily) pasic3 LED support

[Robert Jarzmik]

Petr Cvek <petr.cvek@tul.cz> writes:

> Temporarily remove pasic3 LEDs definition as it is not used (yet).
I don't understand the split between this patch and patch 19/21. Is it for
bisectability, are they symetric (this one removes what the next adds) ?

As the patch 18/21 will probably go through Lee's tree, is there any value in
keeping them apart ?

Cheers.

--
Robert

[PATCH v2 18/21] mfd: htc-pasic3: Prepare driver for leds-pasic3

[Petr Cvek]

Fix register definitions and prepare structures for new leds-pasic3
driver.

Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
---
 drivers/mfd/htc-pasic3.c       | 54 ++++++++++++++++-----------
 include/linux/mfd/htc-pasic3.h | 85 +++++++++++++++++++++++++++++++-----------
 2 files changed, 97 insertions(+), 42 deletions(-)

diff --git a/drivers/mfd/htc-pasic3.c b/drivers/mfd/htc-pasic3.c
index e88d4f6..16e156d 100644
--- a/drivers/mfd/htc-pasic3.c
+++ b/drivers/mfd/htc-pasic3.c
@@ -3,6 +3,9 @@
  *
  * Copyright (C) 2006 Philipp Zabel <philipp.zabel@gmail.com>
  *
+ * 2015:	Added registers for LED and RESET, see htc-pasic3.h
+ *		-- Petr Cvek
+ *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
  * the Free Software Foundation; version 2 of the License.
@@ -21,15 +24,10 @@
 #include <linux/mfd/htc-pasic3.h>
 #include <linux/slab.h>
 
-struct pasic3_data {
-	void __iomem *mapping;
-	unsigned int bus_shift;
-};
-
 #define REG_ADDR  5
 #define REG_DATA  6
 
-#define READ_MODE 0x80
+#define READ_MODE BIT(7)
 
 /*
  * write to a secondary register on the PASIC3
@@ -76,24 +74,36 @@ static struct mfd_cell led_cell __initdata = {
 static int ds1wm_enable(struct platform_device *pdev)
 {
 	struct device *dev = pdev->dev.parent;
-	int c;
+	struct pasic3_data *asic = platform_get_drvdata(pdev);
+	unsigned long flags;
+	u8 c;
+
+	spin_lock_irqsave(&asic->lock, flags);
+
+	c = pasic3_read_register(dev, PASIC3_GPIO);
+	pasic3_write_register(dev, PASIC3_GPIO, c & ~DS1WM_NEN);
 
-	c = pasic3_read_register(dev, 0x28);
-	pasic3_write_register(dev, 0x28, c & 0x7f);
+	spin_unlock_irqrestore(&asic->lock, flags);
 
-	dev_dbg(dev, "DS1WM OWM_EN low (active) %02x\n", c & 0x7f);
+	dev_dbg(dev, "DS1WM OWM_EN low (active) %02lx\n", c & ~DS1WM_NEN);
 	return 0;
 }
 
 static int ds1wm_disable(struct platform_device *pdev)
 {
 	struct device *dev = pdev->dev.parent;
-	int c;
+	struct pasic3_data *asic = platform_get_drvdata(pdev);
+	unsigned long flags;
+	u8 c;
+
+	spin_lock_irqsave(&asic->lock, flags);
+
+	c = pasic3_read_register(dev, PASIC3_GPIO);
+	pasic3_write_register(dev, PASIC3_GPIO, c | DS1WM_NEN);
 
-	c = pasic3_read_register(dev, 0x28);
-	pasic3_write_register(dev, 0x28, c | 0x80);
+	spin_unlock_irqrestore(&asic->lock, flags);
 
-	dev_dbg(dev, "DS1WM OWM_EN high (inactive) %02x\n", c | 0x80);
+	dev_dbg(dev, "DS1WM OWM_EN high (inactive) %02lx\n", c | DS1WM_NEN);
 	return 0;
 }
 
@@ -130,6 +140,7 @@ static int __init pasic3_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct pasic3_data *asic;
 	struct resource *r;
+	struct pasic3_leds_pdata *leds_pdata;
 	int ret;
 	int irq = 0;
 
@@ -162,6 +173,8 @@ static int __init pasic3_probe(struct platform_device *pdev)
 	/* calculate bus shift from mem resource */
 	asic->bus_shift = (resource_size(r) - 5) >> 3;
 
+	spin_lock_init(&asic->lock);
+
 	if (pdata && pdata->clock_rate) {
 		ds1wm_pdata.clock_rate = pdata->clock_rate;
 		/* the first 5 PASIC3 registers control the DS1WM */
@@ -172,13 +185,12 @@ static int __init pasic3_probe(struct platform_device *pdev)
 			dev_warn(dev, "failed to register DS1WM\n");
 	}
 
-	if (pdata && pdata->led_pdata) {
-		led_cell.platform_data = pdata->led_pdata;
-		led_cell.pdata_size = sizeof(struct pasic3_leds_machinfo);
-		ret = mfd_add_devices(&pdev->dev, pdev->id, &led_cell, 1, r,
-				      0, NULL);
-		if (ret < 0)
-			dev_warn(dev, "failed to register LED device\n");
+	if (pdata && pdata->pasic3_leds) {
+		leds_pdata = dev_get_platdata(&pdata->pasic3_leds->dev);
+		if (leds_pdata) {
+			leds_pdata->mfd_dev = dev;
+			platform_device_register(pdata->pasic3_leds);
+		}
 	}
 
 	return 0;
diff --git a/include/linux/mfd/htc-pasic3.h b/include/linux/mfd/htc-pasic3.h
index 3d3ed67..91a27b6 100644
--- a/include/linux/mfd/htc-pasic3.h
+++ b/include/linux/mfd/htc-pasic3.h
@@ -2,6 +2,7 @@
  * HTC PASIC3 driver - LEDs and DS1WM
  *
  * Copyright (c) 2007 Philipp Zabel <philipp.zabel@gmail.com>
+ * Copyright (c) 2015 Petr Cvek <petr.cvek@tul.cz>
  *
  * This file is subject to the terms and conditions of the GNU General Public
  * License.  See the file COPYING in the main directory of this archive for
@@ -18,37 +19,79 @@
 extern void pasic3_write_register(struct device *dev, u32 reg, u8 val);
 extern u8 pasic3_read_register(struct device *dev, u32 reg);
 
-/*
- * mask for registers 0x20,0x21,0x22
- */
-#define PASIC3_MASK_LED0 0x04
-#define PASIC3_MASK_LED1 0x08
-#define PASIC3_MASK_LED2 0x40
+#define PASIC3_CH0_DELAY_ON	0x00
+#define PASIC3_CH0_DELAY_OFF	0x01
+#define PASIC3_CH1_DELAY_ON	0x02
+#define PASIC3_CH1_DELAY_OFF	0x03
+#define PASIC3_CH2_DELAY_ON	0x04
+#define PASIC3_CH2_DELAY_OFF	0x05
+#define PASIC3_DELAY_MASK	0x7f
+
+#define PASIC3_CH_CONTROL	0x06
+#define		R06_CH0_EN		BIT(0)
+#define		R06_CH1_EN		BIT(1)
+#define		R06_CH2_EN		BIT(2)
+#define		R06_CH0_FORCE_ON	BIT(3)
+#define		R06_CH1_FORCE_ON	BIT(4)
+#define		R06_CH2_FORCE_ON	BIT(5)
 
 /*
- * bits in register 0x06
+ * pwm_ch0_out | force_on | (bit0 & bit1 & bit2)
+ * pwm_ch1_out | force_on | (bit0 & bit1 & bit2)
+ * pwm_ch2_out | force_on | (bit2?bit0:(bit0 & ! bit1))
  */
-#define PASIC3_BIT2_LED0 0x08
-#define PASIC3_BIT2_LED1 0x10
-#define PASIC3_BIT2_LED2 0x20
+
+#define PASIC3_MASK_A	0x20
+#define PASIC3_MASK_B	0x21
+#define PASIC3_MASK_C	0x22
+#define		MASK_CH0	BIT(2)
+#define		MASK_CH1	BIT(3)
+#define		MASK_CH2	BIT(6)
+#define PASIC3_GPIO	0x28
+#define		DS1WM_NEN	BIT(7)
+#define PASIC3_SYS	0x2a
+
+/* NORMAL_RST, CAM_PWR_RST and UNKNOWN will autoclear, set STATUS */
+#define		NORMAL_RST	BIT(0)
+#define		CAM_PWR_RST	BIT(1)
+#define		UNKNOWN	BIT(2)
+#define		STATUS_NORMAL_RST	BIT(4)
+#define		STATUS_CAM_PWR_RST	BIT(5)
+#define		STATUS_UNKNOWN		BIT(6)
+
+/* FIXME/TODO reset sources */
+#define PASIC3_RST_EN	0x2c
+#define		EN_NORMAL_RST	0x40
+#define		EN_DOOR_RST	0x42
 
 struct pasic3_led {
-	struct led_classdev         led;
-	unsigned int                hw_num;
-	unsigned int                bit2;
-	unsigned int                mask;
-	struct pasic3_leds_machinfo *pdata;
+	char	name[24];
+	char	default_trigger[80];
+	int	hw_num;
+	u8	bit_blink_en;
+	u8	bit_force_on;
+	u8	bit_mask;
+	u8	reg_delay_on;
+	u8	reg_delay_off;
 };
 
-struct pasic3_leds_machinfo {
-	unsigned int      num_leds;
-	unsigned int      power_gpio;
+struct pasic3_leds_pdata {
 	struct pasic3_led *leds;
+	int num_leds;
+	int power_gpio;
+	int power_gpio_users;
+	struct device *mfd_dev;
 };
 
-struct pasic3_platform_data {
-	struct pasic3_leds_machinfo *led_pdata;
-	unsigned int                 clock_rate;
+struct pasic3_data {
+	void __iomem *mapping;
+	unsigned int bus_shift;
+	spinlock_t lock;
 };
 
+struct pasic3_platform_data {
+	unsigned int clock_rate;
+	struct pasic3_led *leds;
+	struct platform_device *pasic3_leds;
+};
 #endif
-- 
1.7.12.1

[PATCH v2 18/21] mfd: htc-pasic3: Prepare driver for leds-pasic3

[Lee Jones]

On Tue, 18 Aug 2015, Petr Cvek wrote:

> Fix register definitions and prepare structures for new leds-pasic3
> driver.
> 
> Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
> ---
>  drivers/mfd/htc-pasic3.c       | 54 ++++++++++++++++-----------
>  include/linux/mfd/htc-pasic3.h | 85 +++++++++++++++++++++++++++++++-----------
>  2 files changed, 97 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/mfd/htc-pasic3.c b/drivers/mfd/htc-pasic3.c
> index e88d4f6..16e156d 100644
> --- a/drivers/mfd/htc-pasic3.c
> +++ b/drivers/mfd/htc-pasic3.c
> @@ -3,6 +3,9 @@
>   *
>   * Copyright (C) 2006 Philipp Zabel <philipp.zabel@gmail.com>
>   *
> + * 2015:	Added registers for LED and RESET, see htc-pasic3.h
> + *		-- Petr Cvek
> + *

This is pretty unconventional.

Please look to see what others have done.

>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License as published by
>   * the Free Software Foundation; version 2 of the License.
> @@ -21,15 +24,10 @@
>  #include <linux/mfd/htc-pasic3.h>
>  #include <linux/slab.h>
>  
> -struct pasic3_data {
> -	void __iomem *mapping;
> -	unsigned int bus_shift;
> -};
> -
>  #define REG_ADDR  5
>  #define REG_DATA  6
>  
> -#define READ_MODE 0x80
> +#define READ_MODE BIT(7)
>  
>  /*
>   * write to a secondary register on the PASIC3
> @@ -76,24 +74,36 @@ static struct mfd_cell led_cell __initdata = {
>  static int ds1wm_enable(struct platform_device *pdev)
>  {
>  	struct device *dev = pdev->dev.parent;
> -	int c;
> +	struct pasic3_data *asic = platform_get_drvdata(pdev);
> +	unsigned long flags;
> +	u8 c;
> +
> +	spin_lock_irqsave(&asic->lock, flags);
> +
> +	c = pasic3_read_register(dev, PASIC3_GPIO);
> +	pasic3_write_register(dev, PASIC3_GPIO, c & ~DS1WM_NEN);
>  
> -	c = pasic3_read_register(dev, 0x28);
> -	pasic3_write_register(dev, 0x28, c & 0x7f);
> +	spin_unlock_irqrestore(&asic->lock, flags);
>  
> -	dev_dbg(dev, "DS1WM OWM_EN low (active) %02x\n", c & 0x7f);
> +	dev_dbg(dev, "DS1WM OWM_EN low (active) %02lx\n", c & ~DS1WM_NEN);
>  	return 0;
>  }
>  
>  static int ds1wm_disable(struct platform_device *pdev)
>  {
>  	struct device *dev = pdev->dev.parent;
> -	int c;
> +	struct pasic3_data *asic = platform_get_drvdata(pdev);
> +	unsigned long flags;
> +	u8 c;
> +
> +	spin_lock_irqsave(&asic->lock, flags);
> +
> +	c = pasic3_read_register(dev, PASIC3_GPIO);
> +	pasic3_write_register(dev, PASIC3_GPIO, c | DS1WM_NEN);
>  
> -	c = pasic3_read_register(dev, 0x28);
> -	pasic3_write_register(dev, 0x28, c | 0x80);
> +	spin_unlock_irqrestore(&asic->lock, flags);
>  
> -	dev_dbg(dev, "DS1WM OWM_EN high (inactive) %02x\n", c | 0x80);
> +	dev_dbg(dev, "DS1WM OWM_EN high (inactive) %02lx\n", c | DS1WM_NEN);
>  	return 0;
>  }
>  
> @@ -130,6 +140,7 @@ static int __init pasic3_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct pasic3_data *asic;
>  	struct resource *r;
> +	struct pasic3_leds_pdata *leds_pdata;
>  	int ret;
>  	int irq = 0;
>  
> @@ -162,6 +173,8 @@ static int __init pasic3_probe(struct platform_device *pdev)
>  	/* calculate bus shift from mem resource */
>  	asic->bus_shift = (resource_size(r) - 5) >> 3;
>  
> +	spin_lock_init(&asic->lock);
> +
>  	if (pdata && pdata->clock_rate) {
>  		ds1wm_pdata.clock_rate = pdata->clock_rate;
>  		/* the first 5 PASIC3 registers control the DS1WM */
> @@ -172,13 +185,12 @@ static int __init pasic3_probe(struct platform_device *pdev)
>  			dev_warn(dev, "failed to register DS1WM\n");
>  	}
>  
> -	if (pdata && pdata->led_pdata) {
> -		led_cell.platform_data = pdata->led_pdata;
> -		led_cell.pdata_size = sizeof(struct pasic3_leds_machinfo);
> -		ret = mfd_add_devices(&pdev->dev, pdev->id, &led_cell, 1, r,
> -				      0, NULL);
> -		if (ret < 0)
> -			dev_warn(dev, "failed to register LED device\n");
> +	if (pdata && pdata->pasic3_leds) {
> +		leds_pdata = dev_get_platdata(&pdata->pasic3_leds->dev);

Whoa!  You're passing a pointer to a device through pdata?

I don't like that at all.  Please explain.

> +		if (leds_pdata) {
> +			leds_pdata->mfd_dev = dev;
> +			platform_device_register(pdata->pasic3_leds);

What's the idea here?

> +		}
>  	}
>  
>  	return 0;
> diff --git a/include/linux/mfd/htc-pasic3.h b/include/linux/mfd/htc-pasic3.h
> index 3d3ed67..91a27b6 100644
> --- a/include/linux/mfd/htc-pasic3.h
> +++ b/include/linux/mfd/htc-pasic3.h
> @@ -2,6 +2,7 @@
>   * HTC PASIC3 driver - LEDs and DS1WM
>   *
>   * Copyright (c) 2007 Philipp Zabel <philipp.zabel@gmail.com>
> + * Copyright (c) 2015 Petr Cvek <petr.cvek@tul.cz>
>   *
>   * This file is subject to the terms and conditions of the GNU General Public
>   * License.  See the file COPYING in the main directory of this archive for
> @@ -18,37 +19,79 @@
>  extern void pasic3_write_register(struct device *dev, u32 reg, u8 val);
>  extern u8 pasic3_read_register(struct device *dev, u32 reg);
>  
> -/*
> - * mask for registers 0x20,0x21,0x22
> - */
> -#define PASIC3_MASK_LED0 0x04
> -#define PASIC3_MASK_LED1 0x08
> -#define PASIC3_MASK_LED2 0x40
> +#define PASIC3_CH0_DELAY_ON	0x00
> +#define PASIC3_CH0_DELAY_OFF	0x01
> +#define PASIC3_CH1_DELAY_ON	0x02
> +#define PASIC3_CH1_DELAY_OFF	0x03
> +#define PASIC3_CH2_DELAY_ON	0x04
> +#define PASIC3_CH2_DELAY_OFF	0x05
> +#define PASIC3_DELAY_MASK	0x7f
> +
> +#define PASIC3_CH_CONTROL	0x06
> +#define		R06_CH0_EN		BIT(0)
> +#define		R06_CH1_EN		BIT(1)
> +#define		R06_CH2_EN		BIT(2)
> +#define		R06_CH0_FORCE_ON	BIT(3)
> +#define		R06_CH1_FORCE_ON	BIT(4)
> +#define		R06_CH2_FORCE_ON	BIT(5)
>  
>  /*
> - * bits in register 0x06
> + * pwm_ch0_out | force_on | (bit0 & bit1 & bit2)
> + * pwm_ch1_out | force_on | (bit0 & bit1 & bit2)
> + * pwm_ch2_out | force_on | (bit2?bit0:(bit0 & ! bit1))

Please space this out properly.

>   */
> -#define PASIC3_BIT2_LED0 0x08
> -#define PASIC3_BIT2_LED1 0x10
> -#define PASIC3_BIT2_LED2 0x20
> +
> +#define PASIC3_MASK_A	0x20
> +#define PASIC3_MASK_B	0x21
> +#define PASIC3_MASK_C	0x22
> +#define		MASK_CH0	BIT(2)
> +#define		MASK_CH1	BIT(3)
> +#define		MASK_CH2	BIT(6)
> +#define PASIC3_GPIO	0x28
> +#define		DS1WM_NEN	BIT(7)
> +#define PASIC3_SYS	0x2a
> +
> +/* NORMAL_RST, CAM_PWR_RST and UNKNOWN will autoclear, set STATUS */
> +#define		NORMAL_RST	BIT(0)
> +#define		CAM_PWR_RST	BIT(1)
> +#define		UNKNOWN	BIT(2)
> +#define		STATUS_NORMAL_RST	BIT(4)
> +#define		STATUS_CAM_PWR_RST	BIT(5)
> +#define		STATUS_UNKNOWN		BIT(6)
> +
> +/* FIXME/TODO reset sources */
> +#define PASIC3_RST_EN	0x2c
> +#define		EN_NORMAL_RST	0x40
> +#define		EN_DOOR_RST	0x42
>  
>  struct pasic3_led {
> -	struct led_classdev         led;
> -	unsigned int                hw_num;
> -	unsigned int                bit2;
> -	unsigned int                mask;
> -	struct pasic3_leds_machinfo *pdata;
> +	char	name[24];
> +	char	default_trigger[80];
> +	int	hw_num;
> +	u8	bit_blink_en;
> +	u8	bit_force_on;
> +	u8	bit_mask;
> +	u8	reg_delay_on;
> +	u8	reg_delay_off;
>  };
>  
> -struct pasic3_leds_machinfo {
> -	unsigned int      num_leds;
> -	unsigned int      power_gpio;
> +struct pasic3_leds_pdata {
>  	struct pasic3_led *leds;
> +	int num_leds;
> +	int power_gpio;
> +	int power_gpio_users;
> +	struct device *mfd_dev;
>  };
>  
> -struct pasic3_platform_data {
> -	struct pasic3_leds_machinfo *led_pdata;
> -	unsigned int                 clock_rate;
> +struct pasic3_data {
> +	void __iomem *mapping;
> +	unsigned int bus_shift;
> +	spinlock_t lock;
>  };
>  
> +struct pasic3_platform_data {
> +	unsigned int clock_rate;
> +	struct pasic3_led *leds;
> +	struct platform_device *pasic3_leds;
> +};

If you can separate these with a TAB like the rest of the file, that
would be preferred.

>  #endif

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v2 18/21] mfd: htc-pasic3: Prepare driver for leds-pasic3

[Petr Cvek]

Dne 18.8.2015 v 08:52 Lee Jones napsal(a):
> On Tue, 18 Aug 2015, Petr Cvek wrote:
> 
>> Fix register definitions and prepare structures for new leds-pasic3
>> driver.
>>
>> Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
>> ---
>>  drivers/mfd/htc-pasic3.c       | 54 ++++++++++++++++-----------
>>  include/linux/mfd/htc-pasic3.h | 85 +++++++++++++++++++++++++++++++-----------
>>  2 files changed, 97 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/mfd/htc-pasic3.c b/drivers/mfd/htc-pasic3.c
>> index e88d4f6..16e156d 100644
>> --- a/drivers/mfd/htc-pasic3.c
>> +++ b/drivers/mfd/htc-pasic3.c
>> @@ -3,6 +3,9 @@
>>   *
>>   * Copyright (C) 2006 Philipp Zabel <philipp.zabel@gmail.com>
>>   *
>> + * 2015:	Added registers for LED and RESET, see htc-pasic3.h
>> + *		-- Petr Cvek
>> + *
> 
> This is pretty unconventional.
> 
> Please look to see what others have done.

LED support: Petr Cvek <petr.cvek@tul.cz>

OK?

>> @@ -130,6 +140,7 @@ static int __init pasic3_probe(struct platform_device *pdev)
>>  	struct device *dev = &pdev->dev;
>>  	struct pasic3_data *asic;
>>  	struct resource *r;
>> +	struct pasic3_leds_pdata *leds_pdata;
>>  	int ret;
>>  	int irq = 0;
>>  
>> @@ -162,6 +173,8 @@ static int __init pasic3_probe(struct platform_device *pdev)
>>  	/* calculate bus shift from mem resource */
>>  	asic->bus_shift = (resource_size(r) - 5) >> 3;
>>  
>> +	spin_lock_init(&asic->lock);
>> +
>>  	if (pdata && pdata->clock_rate) {
>>  		ds1wm_pdata.clock_rate = pdata->clock_rate;
>>  		/* the first 5 PASIC3 registers control the DS1WM */
>> @@ -172,13 +185,12 @@ static int __init pasic3_probe(struct platform_device *pdev)
>>  			dev_warn(dev, "failed to register DS1WM\n");
>>  	}
>>  
>> -	if (pdata && pdata->led_pdata) {
>> -		led_cell.platform_data = pdata->led_pdata;
>> -		led_cell.pdata_size = sizeof(struct pasic3_leds_machinfo);
>> -		ret = mfd_add_devices(&pdev->dev, pdev->id, &led_cell, 1, r,
>> -				      0, NULL);
>> -		if (ret < 0)
>> -			dev_warn(dev, "failed to register LED device\n");
>> +	if (pdata && pdata->pasic3_leds) {
>> +		leds_pdata = dev_get_platdata(&pdata->pasic3_leds->dev);
> 
> Whoa!  You're passing a pointer to a device through pdata?
> 
> I don't like that at all.  Please explain.

> 
>> +		if (leds_pdata) {
>> +			leds_pdata->mfd_dev = dev;
>> +			platform_device_register(pdata->pasic3_leds);
> 
> What's the idea here?

Actually, I don't know how to do this in a clean way as pasic3_read_register() and pasic3_write_register() require device struct to pass address for accessing the registers. Only way would be to rewrite all functions which call pasic3_*_register() (removing struct device *dev and change it to struct pasic3_data *asic).

Petr

[PATCH v2 18/21] mfd: htc-pasic3: Prepare driver for leds-pasic3

[Lee Jones]

On Tue, 18 Aug 2015, Petr Cvek wrote:

> Dne 18.8.2015 v 08:52 Lee Jones napsal(a):
> > On Tue, 18 Aug 2015, Petr Cvek wrote:
> > 
> >> Fix register definitions and prepare structures for new leds-pasic3
> >> driver.
> >>
> >> Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
> >> ---
> >>  drivers/mfd/htc-pasic3.c       | 54 ++++++++++++++++-----------
> >>  include/linux/mfd/htc-pasic3.h | 85 +++++++++++++++++++++++++++++++-----------
> >>  2 files changed, 97 insertions(+), 42 deletions(-)
> >>
> >> diff --git a/drivers/mfd/htc-pasic3.c b/drivers/mfd/htc-pasic3.c
> >> index e88d4f6..16e156d 100644
> >> --- a/drivers/mfd/htc-pasic3.c
> >> +++ b/drivers/mfd/htc-pasic3.c
> >> @@ -3,6 +3,9 @@
> >>   *
> >>   * Copyright (C) 2006 Philipp Zabel <philipp.zabel@gmail.com>
> >>   *
> >> + * 2015:	Added registers for LED and RESET, see htc-pasic3.h
> >> + *		-- Petr Cvek
> >> + *
> > 
> > This is pretty unconventional.
> > 
> > Please look to see what others have done.
> 
> LED support: Petr Cvek <petr.cvek@tul.cz>

We can see what changes/support you added in the Git log.  No need to
reflect that here at all.  If everyone did that, the headers would be
a complete mess.

Just:
 * Copyright (C) 2015 Petr Cvek <petr.cvek@tul.cz>

... will do fine.

> >> @@ -130,6 +140,7 @@ static int __init pasic3_probe(struct platform_device *pdev)
> >>  	struct device *dev = &pdev->dev;
> >>  	struct pasic3_data *asic;
> >>  	struct resource *r;
> >> +	struct pasic3_leds_pdata *leds_pdata;
> >>  	int ret;
> >>  	int irq = 0;
> >>  
> >> @@ -162,6 +173,8 @@ static int __init pasic3_probe(struct platform_device *pdev)
> >>  	/* calculate bus shift from mem resource */
> >>  	asic->bus_shift = (resource_size(r) - 5) >> 3;
> >>  
> >> +	spin_lock_init(&asic->lock);
> >> +
> >>  	if (pdata && pdata->clock_rate) {
> >>  		ds1wm_pdata.clock_rate = pdata->clock_rate;
> >>  		/* the first 5 PASIC3 registers control the DS1WM */
> >> @@ -172,13 +185,12 @@ static int __init pasic3_probe(struct platform_device *pdev)
> >>  			dev_warn(dev, "failed to register DS1WM\n");
> >>  	}
> >>  
> >> -	if (pdata && pdata->led_pdata) {
> >> -		led_cell.platform_data = pdata->led_pdata;
> >> -		led_cell.pdata_size = sizeof(struct pasic3_leds_machinfo);
> >> -		ret = mfd_add_devices(&pdev->dev, pdev->id, &led_cell, 1, r,
> >> -				      0, NULL);
> >> -		if (ret < 0)
> >> -			dev_warn(dev, "failed to register LED device\n");
> >> +	if (pdata && pdata->pasic3_leds) {
> >> +		leds_pdata = dev_get_platdata(&pdata->pasic3_leds->dev);
> > 
> > Whoa!  You're passing a pointer to a device through pdata?
> > 
> > I don't like that at all.  Please explain.
> 
> > 
> >> +		if (leds_pdata) {
> >> +			leds_pdata->mfd_dev = dev;
> >> +			platform_device_register(pdata->pasic3_leds);
> > 
> > What's the idea here?
> 
> Actually, I don't know how to do this in a clean way as
> pasic3_read_register() and pasic3_write_register() require device
> struct to pass address for accessing the registers. Only way would
> be to rewrite all functions which call pasic3_*_register() (removing
> struct device *dev and change it to struct pasic3_data *asic).

I'm still not entirely sure what the problem is.

Why are you calling platform_device_register() instead of using the
MFD API?

Where does the 'struct device' actually get loaded into pdata?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v2 19/21] leds: leds-pasic3: Add support for PASIC3 LED controller

[Petr Cvek]

Add support for LEDs inside the PASIC3 chip.

Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
---
 drivers/leds/Kconfig       |   9 ++
 drivers/leds/Makefile      |   1 +
 drivers/leds/leds-pasic3.c | 232 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 242 insertions(+)
 create mode 100644 drivers/leds/leds-pasic3.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 9ad35f7..516ba65 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -586,6 +586,15 @@ config LEDS_PM8941_WLED
 	  This option enables support for the 'White' LED block
 	  on Qualcomm PM8941 PMICs.
 
+config LEDS_PASIC3
+	tristate "LED support for the HTC Magician/Alpine PASIC3"
+	depends on LEDS_CLASS
+	depends on HTC_PASIC3
+	select REGMAP
+	help
+	  This option enables support for the PASIC3 chip (different chip
+	  than Compaq ASIC3).
+
 comment "LED Triggers"
 source "drivers/leds/trigger/Kconfig"
 
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 8d6a24a..b1c659c 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -65,6 +65,7 @@ obj-$(CONFIG_LEDS_VERSATILE)		+= leds-versatile.o
 obj-$(CONFIG_LEDS_MENF21BMC)		+= leds-menf21bmc.o
 obj-$(CONFIG_LEDS_PM8941_WLED)		+= leds-pm8941-wled.o
 obj-$(CONFIG_LEDS_KTD2692)		+= leds-ktd2692.o
+obj-$(CONFIG_LEDS_PASIC3)		+= leds-pasic3.o
 
 # LED SPI Drivers
 obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o
diff --git a/drivers/leds/leds-pasic3.c b/drivers/leds/leds-pasic3.c
new file mode 100644
index 0000000..0f95230
--- /dev/null
+++ b/drivers/leds/leds-pasic3.c
@@ -0,0 +1,232 @@
+/*
+ * AIC3 (or PASIC3) LED driver for HTC Magician/Alpine/.. (not Compaq ASIC3)
+ *
+ * Copyright (c) 2015 Petr Cvek <petr.cvek@tul.cz>
+ *
+ * CLK to MS calculation based on leds-asic3.c
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under  the terms of the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the License, or (at your
+ * option) any later version.
+ *
+ */
+
+#include <linux/gpio.h>
+#include <linux/kernel.h>
+#include <linux/leds.h>
+#include <linux/mfd/htc-pasic3.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+/*
+ * 1 tick is around 62-63 ms, blinking settings (on+off):
+ *	Min: 1+1 ticks ~125ms
+ *	Max: 127+127 ticks ~15?875ms
+ * HW sometimes takes time to change (counter overflow after write?)
+ */
+
+#define MS_TO_CLK(ms)	DIV_ROUND_CLOSEST(((ms)*1024), 64000)
+#define CLK_TO_MS(clk)	(((clk)*64000)/1024)
+#define MAX_CLK		254		/* 127 + 127 (2x 7 bit reg) */
+#define MAX_MS		CLK_TO_MS(MAX_CLK)
+
+spinlock_t lock;
+
+static void brightness_set(struct led_classdev *cdev,
+	enum led_brightness value)
+{
+	struct platform_device *pdev = to_platform_device(cdev->dev->parent);
+	struct pasic3_led *led = dev_get_platdata(cdev->dev);
+	struct pasic3_leds_pdata *pdata = dev_get_platdata(&pdev->dev);
+	unsigned long flags;
+	u8 val;
+
+	spin_lock_irqsave(&lock, flags);
+
+	if (value == LED_OFF) {
+		val = pasic3_read_register(pdata->mfd_dev, PASIC3_CH_CONTROL);
+		pasic3_write_register(pdata->mfd_dev, PASIC3_CH_CONTROL,
+			val & ~(led->bit_blink_en | led->bit_force_on));
+
+		val = pasic3_read_register(pdata->mfd_dev, PASIC3_MASK_A);
+		pasic3_write_register(pdata->mfd_dev, PASIC3_MASK_A,
+			val & ~led->bit_mask);
+
+		if (pdata->power_gpio) {
+			pdata->power_gpio_users--;
+
+			if (pdata->power_gpio_users == 0)
+				gpio_set_value(pdata->power_gpio, 0);
+		}
+	} else {
+		if (pdata->power_gpio) {
+			pdata->power_gpio_users++;
+
+			if (pdata->power_gpio_users != 0)
+				gpio_set_value(pdata->power_gpio, 1);
+		}
+
+		val = pasic3_read_register(pdata->mfd_dev, PASIC3_CH_CONTROL);
+		pasic3_write_register(pdata->mfd_dev, PASIC3_CH_CONTROL,
+			val | led->bit_force_on);
+	}
+
+	spin_unlock_irqrestore(&lock, flags);
+}
+
+static int blink_set(struct led_classdev *cdev,
+	unsigned long *delay_on, unsigned long *delay_off)
+{
+	struct platform_device *pdev = to_platform_device(cdev->dev->parent);
+	struct pasic3_led *led = dev_get_platdata(cdev->dev);
+	struct pasic3_leds_pdata *pdata = dev_get_platdata(&pdev->dev);
+	unsigned long flags;
+	u32 on, off;
+	u8 val;
+
+	if (*delay_on > MAX_MS || *delay_off > MAX_MS)
+		return -EINVAL;
+
+	if (*delay_on == 0 && *delay_off == 0) {
+		/* Choose sensible default delays */
+		on = MS_TO_CLK(500);
+		off = MS_TO_CLK(500);
+	} else {
+		on = MS_TO_CLK(*delay_on);
+		off = MS_TO_CLK(*delay_off);
+		if ((on + off) > MAX_CLK)
+			return -EINVAL;
+		/* Minimal on/off value must be 1 */
+		on = on ? on : 1;
+		off = off ? off : 1;
+	}
+
+	spin_lock_irqsave(&lock, flags);
+
+	pasic3_write_register(pdata->mfd_dev, led->reg_delay_on, on);
+	pasic3_write_register(pdata->mfd_dev, led->reg_delay_off, off);
+
+	val = pasic3_read_register(pdata->mfd_dev, PASIC3_CH_CONTROL);
+	pasic3_write_register(pdata->mfd_dev, PASIC3_CH_CONTROL,
+		val | led->bit_blink_en);
+
+	spin_unlock_irqrestore(&lock, flags);
+
+	*delay_on = CLK_TO_MS(on);
+	*delay_off = CLK_TO_MS(off);
+
+	return 0;
+}
+
+static int pasic3_led_probe(struct platform_device *pdev)
+{
+	struct pasic3_leds_pdata *pdata = dev_get_platdata(&pdev->dev);
+	struct led_classdev *cdev;
+	int ret;
+	int i;
+
+	if (!pdata) {
+		dev_err(&pdev->dev, "no platform data\n");
+		return -EINVAL;
+	}
+
+	if (!pdata->leds) {
+		dev_err(&pdev->dev, "no leds data\n");
+		return -EINVAL;
+	}
+
+	if (pdata->power_gpio) {
+		pdata->power_gpio_users = 0;
+
+		ret = gpio_request(pdata->power_gpio,
+			"Red-Blue LED Power");
+		if (ret < 0) {
+			dev_warn(&pdev->dev, "failed to request power GPIO\n");
+			goto err;
+		}
+	}
+
+	spin_lock_init(&lock);
+
+	cdev = devm_kzalloc(&pdev->dev,
+		sizeof(struct led_classdev) * pdata->num_leds, GFP_KERNEL);
+	if (!cdev) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	for (i = 0; i < pdata->num_leds; i++) {
+		cdev[i].name = pdata->leds[i].name;
+		cdev[i].default_trigger = pdata->leds[i].default_trigger;
+		cdev[i].flags = LED_CORE_SUSPENDRESUME;
+		cdev[i].brightness_set = brightness_set;
+		cdev[i].blink_set = blink_set;
+
+		ret = devm_led_classdev_register(&pdev->dev, &cdev[i]);
+		if (ret < 0)
+			goto err;
+
+		cdev[i].dev->platform_data = &pdata->leds[i];
+	}
+
+	return 0;
+
+err:
+	if (pdata->power_gpio)
+		gpio_free(pdata->power_gpio);
+
+	return ret;
+}
+
+static int pasic3_led_remove(struct platform_device *pdev)
+{
+	struct pasic3_leds_pdata *pdata = dev_get_platdata(&pdev->dev);
+
+	if (pdata->power_gpio)
+		gpio_free(pdata->power_gpio);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int pasic3_led_suspend(struct device *dev)
+{
+	struct pasic3_leds_pdata *pdata = dev_get_platdata(dev);
+
+	if (pdata->power_gpio)
+		gpio_set_value(pdata->power_gpio, 0);
+
+	return 0;
+}
+
+static int pasic3_led_resume(struct device *dev)
+{
+	struct pasic3_leds_pdata *pdata = dev_get_platdata(dev);
+
+	if (pdata->power_gpio)
+		gpio_set_value(pdata->power_gpio, 1);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(pasic3_led_pm_ops, pasic3_led_suspend,
+	pasic3_led_resume);
+
+static struct platform_driver pasic3_led_driver = {
+	.probe		= pasic3_led_probe,
+	.remove		= pasic3_led_remove,
+	.driver		= {
+		.name	= "leds-pasic3",
+		.pm	= &pasic3_led_pm_ops,
+	},
+};
+
+module_platform_driver(pasic3_led_driver);
+
+MODULE_AUTHOR("Petr Cvek <petr.cvek@tul.cz>");
+MODULE_DESCRIPTION("HTC Magician PASIC3 LED driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:leds-pasic3");
-- 
1.7.12.1

[PATCH v2 19/21] leds: leds-pasic3: Add support for PASIC3 LED controller

[Petr Cvek]

Dne 18.8.2015 v 00:06 Petr Cvek napsal(a):
> Add support for LEDs inside the PASIC3 chip.

BTW During research I have found there was driver in 

	https://github.com/hackndev/linux-hnd 

but it did not make up to vanilla, so I wrote mine from scratch (actually I
have tested the PASIC3 registers mostly by trial and error - and found some
for case open reset).

[PATCH v2 19/21] leds: leds-pasic3: Add support for PASIC3 LED controller

[Jacek Anaszewski]

Hi Petr,

On 08/18/2015 12:06 AM, Petr Cvek wrote:
> Add support for LEDs inside the PASIC3 chip.
>
> Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
> ---
>   drivers/leds/Kconfig       |   9 ++
>   drivers/leds/Makefile      |   1 +
>   drivers/leds/leds-pasic3.c | 232 +++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 242 insertions(+)
>   create mode 100644 drivers/leds/leds-pasic3.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 9ad35f7..516ba65 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -586,6 +586,15 @@ config LEDS_PM8941_WLED
>   	  This option enables support for the 'White' LED block
>   	  on Qualcomm PM8941 PMICs.
>
> +config LEDS_PASIC3
> +	tristate "LED support for the HTC Magician/Alpine PASIC3"
> +	depends on LEDS_CLASS
> +	depends on HTC_PASIC3
> +	select REGMAP
> +	help
> +	  This option enables support for the PASIC3 chip (different chip
> +	  than Compaq ASIC3).
> +
>   comment "LED Triggers"
>   source "drivers/leds/trigger/Kconfig"
>
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 8d6a24a..b1c659c 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -65,6 +65,7 @@ obj-$(CONFIG_LEDS_VERSATILE)		+= leds-versatile.o
>   obj-$(CONFIG_LEDS_MENF21BMC)		+= leds-menf21bmc.o
>   obj-$(CONFIG_LEDS_PM8941_WLED)		+= leds-pm8941-wled.o
>   obj-$(CONFIG_LEDS_KTD2692)		+= leds-ktd2692.o
> +obj-$(CONFIG_LEDS_PASIC3)		+= leds-pasic3.o
>
>   # LED SPI Drivers
>   obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o
> diff --git a/drivers/leds/leds-pasic3.c b/drivers/leds/leds-pasic3.c
> new file mode 100644
> index 0000000..0f95230
> --- /dev/null
> +++ b/drivers/leds/leds-pasic3.c
> @@ -0,0 +1,232 @@
> +/*
> + * AIC3 (or PASIC3) LED driver for HTC Magician/Alpine/.. (not Compaq ASIC3)

s/AIC3/ASIC3/

I am a bit confused. What's the relation between ASIC3 and PASIC3?

> + *
> + * Copyright (c) 2015 Petr Cvek <petr.cvek@tul.cz>
> + *
> + * CLK to MS calculation based on leds-asic3.c
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under  the terms of the GNU General  Public License as published by the
> + * Free Software Foundation;  either version 2 of the License, or (at your
> + * option) any later version.
> + *
> + */
> +
> +#include <linux/gpio.h>
> +#include <linux/kernel.h>
> +#include <linux/leds.h>
> +#include <linux/mfd/htc-pasic3.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +/*
> + * 1 tick is around 62-63 ms, blinking settings (on+off):
> + *	Min: 1+1 ticks ~125ms
> + *	Max: 127+127 ticks ~15?875ms
> + * HW sometimes takes time to change (counter overflow after write?)
> + */
> +
> +#define MS_TO_CLK(ms)	DIV_ROUND_CLOSEST(((ms)*1024), 64000)
> +#define CLK_TO_MS(clk)	(((clk)*64000)/1024)
> +#define MAX_CLK		254		/* 127 + 127 (2x 7 bit reg) */
> +#define MAX_MS		CLK_TO_MS(MAX_CLK)
> +
> +spinlock_t lock;

Please move it to the struct pasic3_leds_data.

> +
> +static void brightness_set(struct led_classdev *cdev,
> +	enum led_brightness value)
> +{
> +	struct platform_device *pdev = to_platform_device(cdev->dev->parent);
> +	struct pasic3_led *led = dev_get_platdata(cdev->dev);
> +	struct pasic3_leds_pdata *pdata = dev_get_platdata(&pdev->dev);
> +	unsigned long flags;
> +	u8 val;
> +
> +	spin_lock_irqsave(&lock, flags);
> +
> +	if (value == LED_OFF) {
> +		val = pasic3_read_register(pdata->mfd_dev, PASIC3_CH_CONTROL);
> +		pasic3_write_register(pdata->mfd_dev, PASIC3_CH_CONTROL,
> +			val & ~(led->bit_blink_en | led->bit_force_on));
> +
> +		val = pasic3_read_register(pdata->mfd_dev, PASIC3_MASK_A);
> +		pasic3_write_register(pdata->mfd_dev, PASIC3_MASK_A,
> +			val & ~led->bit_mask);
> +
> +		if (pdata->power_gpio) {
> +			pdata->power_gpio_users--;

Now it is possible that power_gpio_users will have negative value.
You should check the value before decrementing it.

> +
> +			if (pdata->power_gpio_users == 0)
> +				gpio_set_value(pdata->power_gpio, 0);
> +		}
> +	} else {
> +		if (pdata->power_gpio) {
> +			pdata->power_gpio_users++;
> +
> +			if (pdata->power_gpio_users != 0)
> +				gpio_set_value(pdata->power_gpio, 1);
> +		}


Now you are setting power_gpio_users to 1 each time brightness is set,
whereas it should be set to 1 only if it was 0.

This could be changed to:

if (pdata->power_gpio_users == 0)
	gpio_set_value(pdata->power_gpio, 1);

pdata->power_gpio_users++;

> +
> +		val = pasic3_read_register(pdata->mfd_dev, PASIC3_CH_CONTROL);
> +		pasic3_write_register(pdata->mfd_dev, PASIC3_CH_CONTROL,
> +			val | led->bit_force_on);
> +	}
> +
> +	spin_unlock_irqrestore(&lock, flags);
> +}
> +
> +static int blink_set(struct led_classdev *cdev,
> +	unsigned long *delay_on, unsigned long *delay_off)
> +{
> +	struct platform_device *pdev = to_platform_device(cdev->dev->parent);
> +	struct pasic3_led *led = dev_get_platdata(cdev->dev);
> +	struct pasic3_leds_pdata *pdata = dev_get_platdata(&pdev->dev);
> +	unsigned long flags;
> +	u32 on, off;
> +	u8 val;
> +	if (*delay_on > MAX_MS || *delay_off > MAX_MS)
> +		return -EINVAL;
> +
> +	if (*delay_on == 0 && *delay_off == 0) {
> +		/* Choose sensible default delays */
> +		on = MS_TO_CLK(500);
> +		off = MS_TO_CLK(500);
> +	} else {
> +		on = MS_TO_CLK(*delay_on);
> +		off = MS_TO_CLK(*delay_off);
> +		if ((on + off) > MAX_CLK)
> +			return -EINVAL;
> +		/* Minimal on/off value must be 1 */
> +		on = on ? on : 1;
> +		off = off ? off : 1;
> +	}
> +
> +	spin_lock_irqsave(&lock, flags);

Shouldn't you check if also power_gpio state doesn't need to be altered
here?

> +
> +	pasic3_write_register(pdata->mfd_dev, led->reg_delay_on, on);
> +	pasic3_write_register(pdata->mfd_dev, led->reg_delay_off, off);
> +
> +	val = pasic3_read_register(pdata->mfd_dev, PASIC3_CH_CONTROL);
> +	pasic3_write_register(pdata->mfd_dev, PASIC3_CH_CONTROL,
> +		val | led->bit_blink_en);
> +
> +	spin_unlock_irqrestore(&lock, flags);
> +
> +	*delay_on = CLK_TO_MS(on);
> +	*delay_off = CLK_TO_MS(off);
> +
> +	return 0;
> +}
> +
> +static int pasic3_led_probe(struct platform_device *pdev)
> +{
> +	struct pasic3_leds_pdata *pdata = dev_get_platdata(&pdev->dev);
> +	struct led_classdev *cdev;
> +	int ret;
> +	int i;
> +
> +	if (!pdata) {
> +		dev_err(&pdev->dev, "no platform data\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!pdata->leds) {
> +		dev_err(&pdev->dev, "no leds data\n");
> +		return -EINVAL;
> +	}
> +
> +	if (pdata->power_gpio) {
> +		pdata->power_gpio_users = 0;
> +
> +		ret = gpio_request(pdata->power_gpio,
> +			"Red-Blue LED Power");

Please use devm_gpio_request. Then you will not need pasic3_led_remove.

> +		if (ret < 0) {
> +			dev_warn(&pdev->dev, "failed to request power GPIO\n");
> +			goto err;
> +		}
> +	}
> +
> +	spin_lock_init(&lock);
> +
> +	cdev = devm_kzalloc(&pdev->dev,
> +		sizeof(struct led_classdev) * pdata->num_leds, GFP_KERNEL);
> +	if (!cdev) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +
> +	for (i = 0; i < pdata->num_leds; i++) {
> +		cdev[i].name = pdata->leds[i].name;
> +		cdev[i].default_trigger = pdata->leds[i].default_trigger;
> +		cdev[i].flags = LED_CORE_SUSPENDRESUME;
> +		cdev[i].brightness_set = brightness_set;
> +		cdev[i].blink_set = blink_set;
> +
> +		ret = devm_led_classdev_register(&pdev->dev, &cdev[i]);
> +		if (ret < 0)
> +			goto err;
> +
> +		cdev[i].dev->platform_data = &pdata->leds[i];
> +	}
> +
> +	return 0;
> +
> +err:
> +	if (pdata->power_gpio)
> +		gpio_free(pdata->power_gpio);
> +
> +	return ret;
> +}
> +
> +static int pasic3_led_remove(struct platform_device *pdev)
> +{
> +	struct pasic3_leds_pdata *pdata = dev_get_platdata(&pdev->dev);
> +
> +	if (pdata->power_gpio)
> +		gpio_free(pdata->power_gpio);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int pasic3_led_suspend(struct device *dev)
> +{
> +	struct pasic3_leds_pdata *pdata = dev_get_platdata(dev);
> +
> +	if (pdata->power_gpio)
> +		gpio_set_value(pdata->power_gpio, 0);
> +
> +	return 0;
> +}
> +
> +static int pasic3_led_resume(struct device *dev)
> +{
> +	struct pasic3_leds_pdata *pdata = dev_get_platdata(dev);
> +
> +	if (pdata->power_gpio)
> +		gpio_set_value(pdata->power_gpio, 1);
> +
> +	return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(pasic3_led_pm_ops, pasic3_led_suspend,
> +	pasic3_led_resume);
> +
> +static struct platform_driver pasic3_led_driver = {
> +	.probe		= pasic3_led_probe,
> +	.remove		= pasic3_led_remove,
> +	.driver		= {
> +		.name	= "leds-pasic3",
> +		.pm	= &pasic3_led_pm_ops,
> +	},
> +};
> +
> +module_platform_driver(pasic3_led_driver);
> +
> +MODULE_AUTHOR("Petr Cvek <petr.cvek@tul.cz>");
> +MODULE_DESCRIPTION("HTC Magician PASIC3 LED driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:leds-pasic3");
>


-- 
Best Regards,
Jacek Anaszewski

[PATCH v2 19/21] leds: leds-pasic3: Add support for PASIC3 LED controller

[Petr Cvek]

Dne 18.8.2015 v 12:27 Jacek Anaszewski napsal(a):
> Hi Petr,
> 
> On 08/18/2015 12:06 AM, Petr Cvek wrote:
>> +/*
>> + * AIC3 (or PASIC3) LED driver for HTC Magician/Alpine/.. (not Compaq ASIC3)
> 
> s/AIC3/ASIC3/
> 
> I am a bit confused. What's the relation between ASIC3 and PASIC3?

Actually both drivers should be renamed to AIC3 (or AIC only, as there is version 2). "HTC_AIC3" is written on the chip and "pasic3" is confusing with compaq asic3 driver, which claims in Kconfig help to be compatible with HTC machines. Compaq ASIC3 has much more functionality and from brief look it seems it has 16 bit registers (HTC_AIC3 has only LEDs, RESET and DS1WM and has 8 bit registers).

>> +spinlock_t lock;
> 
> Please move it to the struct pasic3_leds_data.

OK

> 
>> +
>> +static void brightness_set(struct led_classdev *cdev,
>> +    enum led_brightness value)
>> +{
>> +    struct platform_device *pdev = to_platform_device(cdev->dev->parent);
>> +    struct pasic3_led *led = dev_get_platdata(cdev->dev);
>> +    struct pasic3_leds_pdata *pdata = dev_get_platdata(&pdev->dev);
>> +    unsigned long flags;
>> +    u8 val;
>> +
>> +    spin_lock_irqsave(&lock, flags);
>> +
>> +    if (value == LED_OFF) {
>> +        val = pasic3_read_register(pdata->mfd_dev, PASIC3_CH_CONTROL);
>> +        pasic3_write_register(pdata->mfd_dev, PASIC3_CH_CONTROL,
>> +            val & ~(led->bit_blink_en | led->bit_force_on));
>> +
>> +        val = pasic3_read_register(pdata->mfd_dev, PASIC3_MASK_A);
>> +        pasic3_write_register(pdata->mfd_dev, PASIC3_MASK_A,
>> +            val & ~led->bit_mask);
>> +
>> +        if (pdata->power_gpio) {
>> +            pdata->power_gpio_users--;
> 
> Now it is possible that power_gpio_users will have negative value.
> You should check the value before decrementing it.

> Now you are setting power_gpio_users to 1 each time brightness is set,
> whereas it should be set to 1 only if it was 0.
> 
> This could be changed to:
> 
> if (pdata->power_gpio_users == 0)
>     gpio_set_value(pdata->power_gpio, 1);
> 
> pdata->power_gpio_users++;

> 
> Shouldn't you check if also power_gpio state doesn't need to be altered
> here?
> 


Can the same brightness_set() function be called multiple times with same value (like LED_ON)?

I think I will change it to a different code altogether, power_gpio is shared variable and power can be shut off when two of three leds are disabled (green LED does not use power_gpio). Something like:

enabled_leds |= BIT(hw_num);

or

enabled_leds &= ~BIT(hw_num);

>> +        ret = gpio_request(pdata->power_gpio,
>> +            "Red-Blue LED Power");
> 
> Please use devm_gpio_request. Then you will not need pasic3_led_remove.
> 

OK

[PATCH v2 19/21] leds: leds-pasic3: Add support for PASIC3 LED controller

[Philipp Zabel]

Am Dienstag, den 18.08.2015, 23:48 +0200 schrieb Petr Cvek:
> Dne 18.8.2015 v 12:27 Jacek Anaszewski napsal(a):
> > Hi Petr,
> > 
> > On 08/18/2015 12:06 AM, Petr Cvek wrote:
> > > +/*
> > > + * AIC3 (or PASIC3) LED driver for HTC Magician/Alpine/.. (not 
> > > Compaq ASIC3)
> > 
> > s/AIC3/ASIC3/
> > 
> > I am a bit confused. What's the relation between ASIC3 and PASIC3?
> 
> Actually both drivers should be renamed to AIC3 (or AIC only, as 
> there is version 2). "HTC_AIC3" is written on the chip and "pasic3" 
> is confusing with compaq asic3 driver, which claims in Kconfig help 
> to be compatible with HTC machines. Compaq ASIC3 has much more 
> functionality and from brief look it seems it has 16 bit registers 
> (HTC_AIC3 has only LEDs, RESET and DS1WM and has 8 bit registers).

I think I've got the PASIC3 name from debug strings present in the HTC
bootloader or WinCE binary. This chip (and its predecessor PASIC2 in
HTC Blueangel) are different from the ASIC3 found in hx4700 iPAQs.

regards
Philipp

[PATCH v2 19/21] leds: leds-pasic3: Add support for PASIC3 LED controller

[Jacek Anaszewski]

On 08/18/2015 11:48 PM, Petr Cvek wrote:
> Dne 18.8.2015 v 12:27 Jacek Anaszewski napsal(a):
>> Hi Petr,
>>
>> On 08/18/2015 12:06 AM, Petr Cvek wrote:
>>> +/*
>>> + * AIC3 (or PASIC3) LED driver for HTC Magician/Alpine/.. (not Compaq ASIC3)
>>
>> s/AIC3/ASIC3/
>>
>> I am a bit confused. What's the relation between ASIC3 and PASIC3?
>
> Actually both drivers should be renamed to AIC3 (or AIC only, as there is version 2). "HTC_AIC3" is written on the chip and "pasic3" is confusing with compaq asic3 driver, which claims in Kconfig help to be compatible with HTC machines. Compaq ASIC3 has much more functionality and from brief look it seems it has 16 bit registers (HTC_AIC3 has only LEDs, RESET and DS1WM and has 8 bit registers).

I see, thanks for the explanation.

>>> +spinlock_t lock;
>>
>> Please move it to the struct pasic3_leds_data.
>
> OK
>
>>
>>> +
>>> +static void brightness_set(struct led_classdev *cdev,
>>> +    enum led_brightness value)
>>> +{
>>> +    struct platform_device *pdev = to_platform_device(cdev->dev->parent);
>>> +    struct pasic3_led *led = dev_get_platdata(cdev->dev);
>>> +    struct pasic3_leds_pdata *pdata = dev_get_platdata(&pdev->dev);
>>> +    unsigned long flags;
>>> +    u8 val;
>>> +
>>> +    spin_lock_irqsave(&lock, flags);
>>> +
>>> +    if (value == LED_OFF) {
>>> +        val = pasic3_read_register(pdata->mfd_dev, PASIC3_CH_CONTROL);
>>> +        pasic3_write_register(pdata->mfd_dev, PASIC3_CH_CONTROL,
>>> +            val & ~(led->bit_blink_en | led->bit_force_on));
>>> +
>>> +        val = pasic3_read_register(pdata->mfd_dev, PASIC3_MASK_A);
>>> +        pasic3_write_register(pdata->mfd_dev, PASIC3_MASK_A,
>>> +            val & ~led->bit_mask);
>>> +
>>> +        if (pdata->power_gpio) {
>>> +            pdata->power_gpio_users--;
>>
>> Now it is possible that power_gpio_users will have negative value.
>> You should check the value before decrementing it.
>
>> Now you are setting power_gpio_users to 1 each time brightness is set,
>> whereas it should be set to 1 only if it was 0.
>>
>> This could be changed to:
>>
>> if (pdata->power_gpio_users == 0)
>>      gpio_set_value(pdata->power_gpio, 1);
>>
>> pdata->power_gpio_users++;
>
>>
>> Shouldn't you check if also power_gpio state doesn't need to be altered
>> here?
>>
>
>
> Can the same brightness_set() function be called multiple times with same value (like LED_ON)?

Yes it can. LED core doesn't check if the value isn't already set.

> I think I will change it to a different code altogether, power_gpio is shared variable and power can be shut off when two of three leds are disabled (green LED does not use power_gpio). Something like:
>
> enabled_leds |= BIT(hw_num);
>
> or
>
> enabled_leds &= ~BIT(hw_num);

OK, we'll discuss that after you'll submit the next version.

>
>>> +        ret = gpio_request(pdata->power_gpio,
>>> +            "Red-Blue LED Power");
>>
>> Please use devm_gpio_request. Then you will not need pasic3_led_remove.
>>
>
> OK
>
>


-- 
Best Regards,
Jacek Anaszewski

[PATCH v2 20/21] ARM: pxa: magician: Re-add pdata for new leds-pasic3 driver

[Petr Cvek]

Re-add definition of LEDs PASIC3 to magician.c.

Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
---
 arch/arm/mach-pxa/magician.c | 54 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-pxa/magician.c b/arch/arm/mach-pxa/magician.c
index d8994dd..5f02ce4 100644
--- a/arch/arm/mach-pxa/magician.c
+++ b/arch/arm/mach-pxa/magician.c
@@ -597,6 +597,58 @@ static struct platform_device leds_gpio = {
 };
 
 /*
+ * PASIC3 LEDs
+ */
+
+static struct pasic3_led magician_pasic3_leds[] = {
+	{
+		.hw_num = 0,
+		.name = "magician:red",
+		.default_trigger = "ds2760-battery.0-charging",
+		.bit_blink_en	= R06_CH0_EN,
+		.bit_force_on	= R06_CH0_FORCE_ON,
+		.bit_mask	= MASK_CH0,
+		.reg_delay_on	= PASIC3_CH0_DELAY_ON,
+		.reg_delay_off	= PASIC3_CH0_DELAY_OFF,
+	},
+	{
+		.hw_num = 1,
+		.name = "magician:green",
+		.default_trigger = "ds2760-battery.0-charging-or-full",
+		.bit_blink_en	= R06_CH1_EN,
+		.bit_force_on	= R06_CH1_FORCE_ON,
+		.bit_mask	= MASK_CH1,
+		.reg_delay_on	= PASIC3_CH1_DELAY_ON,
+		.reg_delay_off	= PASIC3_CH1_DELAY_OFF,
+	},
+	{
+		.hw_num = 2,
+		.name = "magician:blue",
+		.default_trigger = "bluetooth",
+		.bit_blink_en	= R06_CH2_EN,
+		.bit_force_on	= R06_CH2_FORCE_ON,
+		.bit_mask	= MASK_CH2,
+		.reg_delay_on	= PASIC3_CH2_DELAY_ON,
+		.reg_delay_off	= PASIC3_CH2_DELAY_OFF,
+	},
+};
+
+static struct pasic3_leds_pdata magician_pasic3_led_pdata = {
+	.leds		= magician_pasic3_leds,
+	.num_leds	= ARRAY_SIZE(magician_pasic3_leds),
+	/* Green LED doesn't use power_gpio */
+	.power_gpio	= EGPIO_MAGICIAN_LED_POWER,
+};
+
+static struct platform_device pasic3_leds = {
+	.name	= "leds-pasic3",
+	.id	= -1,
+	.dev = {
+		.platform_data = &magician_pasic3_led_pdata,
+	},
+};
+
+/*
  * PASIC3 DS1WM
  */
 
@@ -616,7 +668,7 @@ static struct resource pasic3_resources[] = {
 
 static struct pasic3_platform_data pasic3_platform_data = {
 	.clock_rate	= 4000000,
-/*	.led_pdata	= &pasic3_leds_info, */
+	.pasic3_leds	= &pasic3_leds,
 };
 
 static struct platform_device pasic3 = {
-- 
1.7.12.1

[PATCH v2 21/21] ARM: pxa: magician: Move platform_add_devices() to the end of magician_init()

[Petr Cvek]

Move platform_add_devices() to the end of magician_init() for better
clarity of code.

Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
---
 arch/arm/mach-pxa/magician.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-pxa/magician.c b/arch/arm/mach-pxa/magician.c
index 5f02ce4..af86350 100644
--- a/arch/arm/mach-pxa/magician.c
+++ b/arch/arm/mach-pxa/magician.c
@@ -1355,9 +1355,8 @@ static void __init magician_init(void)
 	pxa_set_btuart_info(NULL);
 	pxa_set_stuart_info(NULL);
 
-	platform_add_devices(ARRAY_AND_SIZE(devices));
-
 	pxa_set_ficp_info(&magician_ficp_info);
+
 	pxa27x_set_i2c_power_info(&magician_i2c_power_info);
 	pxa_set_i2c_info(&i2c_info);
 	i2c_register_board_info(0,
@@ -1394,6 +1393,8 @@ static void __init magician_init(void)
 
 	regulator_register_always_on(0, "power", pwm_backlight_supply,
 		ARRAY_SIZE(pwm_backlight_supply), 5000000);
+
+	platform_add_devices(ARRAY_AND_SIZE(devices));
 }
 
 MACHINE_START(MAGICIAN, "HTC Magician")
-- 
1.7.12.1

[PATCH v2 21/21] ARM: pxa: magician: Move platform_add_devices() to the end of magician_init()

[Robert Jarzmik]

Petr Cvek <petr.cvek@tul.cz> writes:

> Move platform_add_devices() to the end of magician_init() for better
> clarity of code.
>
> Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
With Philipp's ack.

-- 
Robert