Skip to contentSkip to conversation

ARM: pxa: magician: Fix Kconfig for magician to always include htc-egpio

PATCH New Petr Cvek 6 participants 68 messages Aug 17, 2015 – Aug 28, 2015 1 message not in the archive

Subsystems (6)
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Petr Cvek

author
#1 lore raw

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

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.

---
 arch/arm/mach-pxa/Kconfig | 1 +
 1 file changed, 1 insertion(+)
arch/arm/mach-pxa/Kconfig +1 -0
index f096836..5c7b6ee 100644
@@ -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"
Signature · 1 line
1.7.12.1
maintainer
#2 lore raw

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

Quoted 5 lines · 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.

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.

Signature · 1 line
Robert
author
#3 lore raw

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

Dne 18.8.2015 v 20:31 Robert Jarzmik napsal(a):

Quoted 8 lines
Quoted 5 lines · 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.

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).

Quoted 6 lines

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

#4 lore raw

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

Am Dienstag, den 18.08.2015, 22:02 +0200 schrieb Petr Cvek:

Quoted 40 lines

Dne 18.8.2015 v 20:31 Robert Jarzmik napsal(a):

Quoted 10 lines
Quoted 6 lines · 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.

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).

Quoted 8 lines

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).

Quoted 6 lines

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

author
#5 lore raw

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

Dne 19.8.2015 v 09:29 Philipp Zabel napsal(a):

Quoted 41 lines

Am Dienstag, den 18.08.2015, 22:02 +0200 schrieb Petr Cvek:

Quoted 40 lines

Dne 18.8.2015 v 20:31 Robert Jarzmik napsal(a):

Quoted 10 lines
Quoted 6 lines · 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.

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).

Quoted 8 lines

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).

Quoted 8 lines
Quoted 6 lines

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

author
#6 lore raw

PATCH v2 2/21 ARM: pxa: magician: Fix indentation and whitespaces

In reply to a message not in the archive

Fix magician.c: indentation and whitespaces.

---
 arch/arm/mach-pxa/magician.c | 356 +++++++++++++++++++++----------------------
 1 file changed, 178 insertions(+), 178 deletions(-)
arch/arm/mach-pxa/magician.c +178 -178
index a9761c2..9e8698a 100644
@@ -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
Signature · 1 line
1.7.12.1
maintainer
#7 lore raw

PATCH v2 2/21 ARM: pxa: magician: Fix indentation and whitespaces

Quoted 3 lines · Petr Cvek

Fix magician.c: indentation and whitespaces.

Unless somebody finds a flaw within the next 2 weeks, I'll queue that up.

Cheers.

--
Robert

#8 lore raw

PATCH v2 2/21 ARM: pxa: magician: Fix indentation and whitespaces

In reply to Petr Cvek

Am Montag, den 17.08.2015, 23:56 +0200 schrieb Petr Cvek:

arch/arm/mach-pxa/magician.c static unsigned long magician_pin_config[]

Quoted 26 lines

Fix magician.c: indentation and whitespaces.

---
 arch/arm/mach-pxa/magician.c | 356 +++++++++++++++++++++------------
----------
 1 file changed, 178 insertions(+), 178 deletions(-)
arch/arm/mach renamed from arch/arm/mach-pxa/magician.c +0 -1
-pxa/magician.c

index a9761c2..9e8698a 100644

arch/arm/mach-pxa/magician.c +2 -2
@@ -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.

Quoted 9 lines
arch/arm/mach-pxa/magician.c +1 -1
 __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?

Quoted 8 lines
arch/arm/mach-pxa/magician.c +0 -3
@@ -133,46 +133,45 @@ static struct pxaficp_platform_data 
 magician_ficp_info = {
  */
 
 #define INIT_KEY(_code, _gpio, _desc)	\
-	{				\
-		.code   = KEY_##_code,	\
-		.gpio   = _gpio,	\

arch/arm/mach-pxa/magician.c static struct pxaficp_platform_data · line 143

Quoted 10 lines
arch/arm/mach-pxa/magician.c +6 -3
-		.desc   = _desc,	\
-		.type   = EV_KEY,	\
-		.wakeup = 1,		\
+	{						\
+		.code	= KEY_##_code,	\
+		.gpio	= _gpio,	\
+		.desc	= _desc,	\
+		.type	= EV_KEY,	\
+		.wakeup	= 1,		\
 	}

Ok.

Quoted 60 lines
arch/arm/mach-pxa/magician.c +5 -15
 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"),

arch/arm/mach-pxa/magician.c +1 -0
+	INIT_KEY(UP,		GPIO93_MAGICIAN_KEY_UP,		> "Up 

button"),

arch/arm/mach-pxa/magician.c +1 -0
+	INIT_KEY(DOWN,		GPIO94_MAGICIAN_KEY_DOWN,	"Dow

n button"),

arch/arm/mach-pxa/magician.c +1 -0
+	INIT_KEY(LEFT,		GPIO95_MAGICIAN_KEY_LEFT,	"Lef

t button"),

arch/arm/mach-pxa/magician.c +1 -0
+	INIT_KEY(RIGHT,		GPIO96_MAGICIAN_KEY_RIGHT,	> "Right 

button"),

arch/arm/mach-pxa/magician.c +1 -0
+	INIT_KEY(KPENTER,	GPIO97_MAGICIAN_KEY_ENTER,	"Act

ion button"),

arch/arm/mach-pxa/magician.c +1 -0
+	INIT_KEY(RECORD,	GPIO98_MAGICIAN_KEY_RECORD,	"Rec

ord button"),

arch/arm/mach-pxa/magician.c +1 -0
+	INIT_KEY(VOLUMEUP,	GPIO100_MAGICIAN_KEY_VOL_UP,	"Vol

ume up"),

arch/arm/mach-pxa/magician.c +1 -0
+	INIT_KEY(VOLUMEDOWN,	GPIO101_MAGICIAN_KEY_VOL_DOWN,	"Vol

ume down"),

arch/arm/mach-pxa/magician.c +1 -0
+	INIT_KEY(PHONE,		GPIO102_MAGICIAN_KEY_PHONE,	> "Phone 

button"),

arch/arm/mach-pxa/magician.c +1 -0
+	INIT_KEY(PLAY,		GPIO99_MAGICIAN_HEADPHONE_IN,	"Hea
Quoted 2 lines

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.

Quoted 5 lines
  static struct gpio_keys_platform_data gpio_keys_data = {
arch/arm/mach-pxa/magician.c +1 -1
-	.buttons  = magician_button_table,
+	.buttons = magician_button_table,
 	.nbuttons = ARRAY_SIZE(magician_button_table),
 };

arch/arm/mach-pxa/magician.c static struct pxaficp_platform_data magician_ficp_info = { · line 173

Quoted 10 lines
arch/arm/mach-pxa/magician.c +2 -2
 
 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?

Quoted 13 lines

/*
* EGPIO (Xilinx CPLD)
*

arch/arm/mach-pxa/magician.c +3 -3
@@ -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,

arch/arm/mach-pxa/magician.c static struct platform_device gpio_keys = { · line 192

Quoted 10 lines
arch/arm/mach-pxa/magician.c +3 -3
 	},
 	[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.

Quoted 146 lines
arch/arm/mach-pxa/magician.c +33 -33
@@ -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,
 	},

};

arch/arm/mach-pxa/magician.c +8 -8
@@ -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,

};

arch/arm/mach-pxa/magician.c +8 -10
@@ -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,

arch/arm/mach-pxa/magician.c static void magician_backlight_exit(struct · line 394

Quoted 10 lines
arch/arm/mach-pxa/magician.c +5 -3
-	.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

author
#9 lore raw

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

In reply to a message not in the archive

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

---
 arch/arm/mach-pxa/magician.c | 121 +++++++++++++++++++++++++++++++------------
 1 file changed, 89 insertions(+), 32 deletions(-)
arch/arm/mach-pxa/magician.c +89 -32
index 9e8698a..57da133 100644
@@ -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
Signature · 1 line
1.7.12.1
maintainer
#10 lore raw

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

Quoted 40 lines · Petr Cvek

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

---
 arch/arm/mach-pxa/magician.c | 121 +++++++++++++++++++++++++++++++------------
 1 file changed, 89 insertions(+), 32 deletions(-)
arch/arm/mach-pxa/magician.c +24 -0
index 9e8698a..57da133 100644
@@ -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)

arch/arm/mach-pxa/magician.c line 47

Quoted 10 lines
arch/arm/mach-pxa/magician.c +10 -0
+ * 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 ...

arch/arm/mach-pxa/magician.c line 52

Quoted 5 lines
arch/arm/mach-pxa/magician.c +5 -0
+ * 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.

arch/arm/mach-pxa/magician.c static unsigned long magician_pin_config[] __initdata = { · line 168

Quoted 11 lines
arch/arm/mach-pxa/magician.c +2 -1
@@ -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.

Signature · 1 line
Robert
author
#11 lore raw

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

Dne 18.8.2015 v 20:39 Robert Jarzmik napsal(a):

Quoted 45 lines
Quoted 43 lines · Petr Cvek

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

---
 arch/arm/mach-pxa/magician.c | 121 +++++++++++++++++++++++++++++++------------
 1 file changed, 89 insertions(+), 32 deletions(-)
arch/arm/mach-pxa/magician.c +27 -0
index 9e8698a..57da133 100644
@@ -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)
Quoted 8 lines

arch/arm/mach-pxa/magician.c line 47

Quoted 7 lines
arch/arm/mach-pxa/magician.c +7 -0
+ * 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).

Quoted 10 lines

arch/arm/mach-pxa/magician.c line 52

Quoted 6 lines
arch/arm/mach-pxa/magician.c +5 -0
+ * 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.

Quoted 9 lines
Quoted 9 lines
arch/arm/mach-pxa/magician.c +1 -1
@@ -120,11 +160,12 @@ static unsigned long magician_pin_config[] __initdata = {
 };
 
 /*
- * IRDA
+ * IrDA
  */
 
Quoted 5 lines

arch/arm/mach-pxa/magician.c static unsigned long magician_pin_config[] __initdata = { · line 168

Quoted 3 lines
arch/arm/mach-pxa/magician.c +1 -0
 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).

#12 lore raw

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

In reply to Petr Cvek

Am Montag, den 17.08.2015, 23:57 +0200 schrieb Petr Cvek:

Quoted 57 lines

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

---
 arch/arm/mach-pxa/magician.c | 121 +++++++++++++++++++++++++++++++--
----------
 1 file changed, 89 insertions(+), 32 deletions(-)
arch/arm/mach renamed from arch/arm/mach-pxa/magician.c +0 -1
-pxa/magician.c

index 9e8698a..57da133 100644

arch/arm/mach-pxa/magician.c +30 -0
@@ -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?

Quoted 5 lines

+ * 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.

Quoted 39 lines

#include <linux/kernel.h>

arch/arm/mach-pxa/magician.c +6 -6
@@ -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,
Quoted 1 line

GPIO109_MMC_DAT_1,

These comments are nice to have.

Quoted 25 lines
arch/arm/mach-pxa/magician.c +1 -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,
arch/arm/mach-pxa/magician.c +2 -1
 	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,
 };

arch/arm/mach-pxa/magician.c static struct platform_device gpio_keys = { · line 220

Quoted 8 lines
arch/arm/mach-pxa/magician.c +2 -2
@@ -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?

Quoted 25 lines
arch/arm/mach-pxa/magician.c +5 -1
 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] = {
arch/arm/mach-pxa/magician.c +1 -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[] = {

arch/arm/mach-pxa/magician.c static struct pxafb_mode_info samsung_modes[] = · line 314

Quoted 6 lines
arch/arm/mach-pxa/magician.c +1 -1
@@ -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.

arch/arm/mach-pxa/magician.c static void toppoly_lcd_power(int on, struct · line 328

Quoted 13 lines
arch/arm/mach-pxa/magician.c +1 -3
 	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?

Quoted 29 lines
arch/arm/mach-pxa/magician.c +0 -0
 		gpio_set_value(GPIO105_MAGICIAN_LCD_POWER_2, 0);
 		udelay(500);
arch/arm/mach-pxa/magician.c +1 -2
 		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);

arch/arm/mach-pxa/magician.c +0 -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);
arch/arm/mach-pxa/magician.c +0 -0
 		mdelay(30);
@@ -346,8 +387,8 @@ static struct pxafb_mach_info samsung_info = {
  */
 

[...]

Quoted 8 lines
arch/arm/mach-pxa/magician.c +4 -0
+/*
+ * pda_power Li-ion charger
+ */
+
 static struct pda_power_pdata power_supply_info = {
 	.init		= power_supply_init,
 	.is_ac_online	= magician_is_ac_online,

arch/arm/mach-pxa/magician.c static struct platform_device power_supply = { · line 635

Quoted 6 lines
arch/arm/mach-pxa/magician.c +1 -1
@@ -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

author
#13 lore raw

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

In reply to a message not in the archive

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

---
 arch/arm/mach-pxa/include/mach/magician.h |  39 ++++---
 arch/arm/mach-pxa/magician.c              | 166 +++++++++++++++++++++++-------
 2 files changed, 153 insertions(+), 52 deletions(-)
arch/arm/mach-pxa/include/mach/magician.h +24 -15
index ba6a6e1..d19e504 100644
@@ -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_ */
arch/arm/mach-pxa/magician.c +129 -37

267 lines not on this page · raw message

Signature · 1 line
1.7.12.1
maintainer
#14 lore raw

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

Quoted 14 lines · Petr Cvek

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

---
 arch/arm/mach-pxa/include/mach/magician.h |  39 ++++---
 arch/arm/mach-pxa/magician.c              | 166 +++++++++++++++++++++++-------
 2 files changed, 153 insertions(+), 52 deletions(-)
arch/arm/mach-pxa/include/mach/magician.h +0 -0
index ba6a6e1..d19e504 100644

arch/arm/mach-pxa/include/mach/magician.h line 24

Quoted 5 lines
arch/arm/mach-pxa/include/mach/magician.h +1 -1
@@ -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 ?

arch/arm/mach-pxa/include/mach/magician.h line 27

Quoted 4 lines
arch/arm/mach-pxa/include/mach/magician.h +1 -1
 #define GPIO11_MAGICIAN_GSM_OUT1		11
 #define GPIO13_MAGICIAN_CPLD_IRQ		13
-#define GPIO18_MAGICIAN_UNKNOWN			18
+#define GPIO18_MAGICIAN_UNKNOWN		18

Ditto.

arch/arm/mach-pxa/include/mach/magician.h line 38

Quoted 10 lines
arch/arm/mach-pxa/include/mach/magician.h +4 -2
@@ -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 ?

arch/arm/mach-pxa/include/mach/magician.h line 56

Quoted 12 lines
arch/arm/mach-pxa/include/mach/magician.h +1 -4
 #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 ?

Quoted 14 lines
arch/arm/mach-pxa/include/mach/magician.h +5 -0
+#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
 

arch/arm/mach-pxa/include/mach/magician.h line 85

Quoted 5 lines
arch/arm/mach-pxa/include/mach/magician.h +1 -1
@@ -78,7 +82,7 @@
  * CPLD EGPIOs
  */
 
-#define MAGICIAN_EGPIO_BASE			PXA_NR_BUILTIN_GPIO
+#define MAGICIAN_EGPIO_BASE	PXA_NR_BUILTIN_GPIO

Ditto ?

Quoted 1 line

From here onward, I'll suppose you'll catch all whitespace/indentation changes

and extract them towards patch 01/21 ...

arch/arm/mach-pxa/include/mach/magician.h line 117

Quoted 8 lines

/* input */

arch/arm/mach-pxa/include/mach/magician.h +4 -2
-#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 ?

arch/arm/mach-pxa/include/mach/magician.h

Quoted 9 lines
arch/arm/mach-pxa/include/mach/magician.h +0 -3
 	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 ?

Quoted 5 lines
arch/arm/mach-pxa/include/mach/magician.h +5 -0
+	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

arch/arm/mach-pxa/include/mach/magician.h

Quoted 10 lines
arch/arm/mach-pxa/include/mach/magician.h +10 -0
+	 * 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 :)

arch/arm/mach-pxa/include/mach/magician.h static struct htc_egpio_chip egpio_chips[] = { · line 329

Quoted 8 lines
arch/arm/mach-pxa/include/mach/magician.h +2 -1
@@ -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 ?

Quoted 21 lines
arch/arm/mach-pxa/include/mach/magician.h +6 -10
@@ -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);

arch/arm/mach-pxa/include/mach/magician.h static void samsung_lcd_power(int on, struct fb_var_screeninfo *si) · line 442

Quoted 10 lines
arch/arm/mach-pxa/include/mach/magician.h +6 -4
-		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.

Signature · 3 lines
Robert

PS: My workforce is done for today, next review tomorrow.
author
#15 lore raw

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

Dne 18.8.2015 v 21:01 Robert Jarzmik napsal(a):

Quoted 3 lines

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

OK

Quoted 12 lines

arch/arm/mach-pxa/include/mach/magician.h line 117

Quoted 9 lines

/* input */

arch/arm/mach-pxa/include/mach/magician.h +4 -2
-#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).

Quoted 7 lines
Quoted 7 lines
arch/arm/mach-pxa/magician.c +0 -0
 	GPIO10_GPIO,	/* GSM_IRQ */
 	GPIO13_GPIO,	/* CPLD_IRQ */
 	GPIO107_GPIO,	/* DS1WM_IRQ */
 	GPIO108_GPIO,	/* GSM_READY */
 	GPIO115_GPIO,	/* nPEN_IRQ */
Quoted 5 lines

arch/arm/mach-pxa/magician.c static unsigned long magician_pin_config[] __initdata = { · line 159

Quoted 3 lines
arch/arm/mach-pxa/magician.c +0 -3
-	/* 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

Quoted 11 lines
Quoted 11 lines
arch/arm/mach-pxa/magician.c +10 -0
+	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
Quoted 8 lines

arch/arm/mach-pxa/magician.c static unsigned long magician_pin_config[] __initdata = { · line 242

Quoted 5 lines
arch/arm/mach-pxa/magician.c +5 -0
+	 * 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?

Quoted 12 lines

arch/arm/mach-pxa/magician.c static struct htc_egpio_chip egpio_chips[] = { · line 329

Quoted 9 lines
arch/arm/mach-pxa/magician.c +2 -1
@@ -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.

Quoted 25 lines
Quoted 25 lines
arch/arm/mach-pxa/magician.c +6 -14
@@ -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);
Quoted 8 lines

arch/arm/mach-pxa/magician.c static void samsung_lcd_power(int on, struct fb_var_screeninfo *si) · line 442

Quoted 6 lines
arch/arm/mach-pxa/magician.c +6 -0
+		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.

Quoted 3 lines

Cheers.

Best regards,
Petr

author
#16 lore raw

PATCH v2 5/21 ARM: pxa: magician: Add support for ADS7846 touchscreen

In reply to a message not in the archive

Add support for ADS7846 touchscreen.

---
 arch/arm/mach-pxa/magician.c | 86 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)
arch/arm/mach-pxa/magician.c +86 -0
index bc206f8..fc1c035 100644
@@ -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")
Signature · 1 line
1.7.12.1
maintainer
#17 lore raw

PATCH v2 5/21 ARM: pxa: magician: Add support for ADS7846 touchscreen

arch/arm/mach-pxa/magician.c static struct i2c_pxa_platform_data i2c_info = { · line 910

Quoted 5 lines · Petr Cvek

Add support for ADS7846 touchscreen.

arch/arm/mach-pxa/magician.c +4 -0
+
+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

author
#18 lore raw

PATCH v2 6/21 ARM: pxa: magician: Add normal and power I2C definition

In reply to a message not in the archive

Add normal and power I2C definition to HTC Magician.

---
 arch/arm/mach-pxa/magician.c | 38 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)
arch/arm/mach-pxa/magician.c +36 -2
index fc1c035..702e8e0 100644
@@ -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);
Signature · 1 line
1.7.12.1
#19 lore raw

PATCH v2 6/21 ARM: pxa: magician: Add normal and power I2C definition

Am Montag, den 17.08.2015, 23:59 +0200 schrieb Petr Cvek:

Quoted 1 line

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.

Quoted 9 lines

/*
+ * PXA I2C power controller
+ */

arch/arm/mach-pxa/magician.c +5 -0
+
+static struct i2c_pxa_platform_data magician_i2c_power_info = {
+	.fast_mode = 0,
+	.use_pio = 0,
+};

[...]

arch/arm/mach-pxa/magician.c static void __init magician_init(void) · line 1012

Quoted 6 lines
arch/arm/mach-pxa/magician.c +1 -1
@@ -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

maintainer
#20 lore raw

PATCH v2 6/21 ARM: pxa: magician: Add normal and power I2C definition

In reply to Petr Cvek

arch/arm/mach-pxa/magician.c static struct platform_device strataflash = { · line 879

Quoted 7 lines · Petr Cvek
arch/arm/mach-pxa/magician.c +2 -1
@@ -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 ...

Quoted 5 lines
arch/arm/mach-pxa/magician.c +5 -0
+/*
+ * GPIO I2C normal controller (alternative)
+ */
+
+static struct i2c_gpio_platform_data rtc_device_data = {

arch/arm/mach-pxa/magician.c static struct platform_device strataflash = { · line 905

Quoted 10 lines
arch/arm/mach-pxa/magician.c +10 -0
+	.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.

Signature · 1 line
Robert
author
#21 lore raw

PATCH v2 7/21 ARM: pxa: magician: Fix IrDA pdata and redundant GPIO request

In reply to a message not in the archive

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

---
 arch/arm/mach-pxa/magician.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
arch/arm/mach-pxa/magician.c +2 -2
index 702e8e0..7519238 100644
@@ -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)
Signature · 1 line
1.7.12.1
maintainer
#22 lore raw

PATCH v2 7/21 ARM: pxa: magician: Fix IrDA pdata and redundant GPIO request

Quoted 1 line · Petr Cvek

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

author
#23 lore raw

PATCH v2 8/21 ARM: pxa: magician: Add StrataFlash Vpp GPIO and alternative driver

In reply to a message not in the archive

Add StrataFlash Vpp GPIO and alternative driver.

---
 arch/arm/mach-pxa/magician.c | 76 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 73 insertions(+), 3 deletions(-)
arch/arm/mach-pxa/magician.c +73 -3
index 7519238..7331b34 100644
@@ -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 */
Signature · 1 line
1.7.12.1
author
#24 lore raw

PATCH v2 9/21 ARM: pxa: magician: Add OV9640 camera support

In reply to a message not in the archive

Add OV9640 camera support for HTC Magician.

---
 arch/arm/mach-pxa/magician.c | 63 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)
arch/arm/mach-pxa/magician.c +63 -0
index 7331b34..63604e1 100644
@@ -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")
Signature · 1 line
1.7.12.1
maintainer
#25 lore raw

PATCH v2 9/21 ARM: pxa: magician: Add OV9640 camera support

Review

Quoted 1 line · Petr Cvek
(quoted diff) +1 -0
+	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

#26 lore raw

PATCH v2 9/21 ARM: pxa: magician: Add OV9640 camera support

Quoted 9 lines · On Thursday 20 August 2015 21:48:20 Robert Jarzmik wrote:
Quoted 1 line · Petr Cvek

+ 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

author
#27 lore raw

PATCH v2 9/21 ARM: pxa: magician: Add OV9640 camera support

Dne 20.8.2015 v 22:26 Arnd Bergmann napsal(a):

Quoted 11 lines · On Thursday 20 August 2015 21:48:20 Robert Jarzmik wrote:
Quoted 9 lines
Quoted 1 line · Petr Cvek

+ 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.

Quoted 5 lines

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

#28 lore raw

PATCH v2 9/21 ARM: pxa: magician: Add OV9640 camera support

Quoted 18 lines · On Friday 21 August 2015 00:39:30 Petr Cvek wrote:

Dne 20.8.2015 v 22:26 Arnd Bergmann napsal(a):

Quoted 11 lines · On Thursday 20 August 2015 21:48:20 Robert Jarzmik wrote:
Quoted 9 lines
Quoted 1 line · Petr Cvek

+ 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.

Quoted 5 lines

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

Arnd

Quoted 7 lines

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

maintainer
#29 lore raw

PATCH v2 9/21 ARM: pxa: magician: Add OV9640 camera support

I shrunk a bit the mailing list.

Quoted 8 lines · Arnd Bergmann
Quoted 7 lines · On Friday 21 August 2015 00:39:30 Petr Cvek wrote:

Dne 20.8.2015 v 22:26 Arnd Bergmann napsal(a):

Quoted 2 lines · On Thursday 20 August 2015 21:48:20 Robert Jarzmik wrote:
Quoted 1 line

Petr Cvek <[email protected]> writes:

Datasheet says:

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

Quoted 7 lines
Quoted 3 lines

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.

Signature · 1 line
Robert
author
#30 lore raw

PATCH v2 9/21 ARM: pxa: magician: Add OV9640 camera support

Dne 21.8.2015 v 19:36 Robert Jarzmik napsal(a):

Quoted 32 lines

I shrunk a bit the mailing list.

Quoted 8 lines · Arnd Bergmann
Quoted 7 lines · On Friday 21 August 2015 00:39:30 Petr Cvek wrote:

Dne 20.8.2015 v 22:26 Arnd Bergmann napsal(a):

Quoted 2 lines · On Thursday 20 August 2015 21:48:20 Robert Jarzmik wrote:
Quoted 1 line

Petr Cvek <[email protected]> writes:

Datasheet says:

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

Quoted 7 lines
Quoted 3 lines

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).

Quoted 8 lines

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?

Quoted 2 lines

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).

Quoted 3 lines

Cheers.

maintainer
#31 lore raw

PATCH v2 9/21 ARM: pxa: magician: Add OV9640 camera support

Quoted 7 lines · Petr Cvek
Quoted 5 lines

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

Quoted 2 lines

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

Good.

Cheers.

Signature · 1 line
Robert
#32 lore raw

PATCH v2 9/21 ARM: pxa: magician: Add OV9640 camera support

In reply to Arnd Bergmann
Quoted 23 lines · Arnd Bergmann
Quoted 22 lines · On Friday 21 August 2015 00:39:30 Petr Cvek wrote:

Dne 20.8.2015 v 22:26 Arnd Bergmann napsal(a):

Quoted 11 lines · On Thursday 20 August 2015 21:48:20 Robert Jarzmik wrote:
Quoted 9 lines
Quoted 1 line · Petr Cvek

+ 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.

Quoted 5 lines

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

Quoted 7 lines
Quoted 3 lines

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.

Signature · 4 lines
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
author
#33 lore raw

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

In reply to a message not in the archive

Add support for UDA1380 sound.

---
 arch/arm/mach-pxa/magician.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)
arch/arm/mach-pxa/magician.c +31 -0
index 63604e1..9decd86 100644
@@ -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);
 
Signature · 1 line
1.7.12.1
maintainer
#34 lore raw

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

arch/arm/mach-pxa/magician.c static struct pxamci_platform_data magician_mci_info = { · line 914

Quoted 6 lines · Petr Cvek
arch/arm/mach-pxa/magician.c +6 -0
+/*
+ * 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

author
#35 lore raw

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

Dne 20.8.2015 v 21:51 Robert Jarzmik napsal(a):

Quoted 10 lines

arch/arm/mach-pxa/magician.c static struct pxamci_platform_data magician_mci_info = { · line 914

Quoted 6 lines · Petr Cvek
arch/arm/mach-pxa/magician.c +6 -0
+/*
+ * 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).

Quoted 6 lines

Cheers.

--
Robert

maintainer
#36 lore raw

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

Quoted 19 lines · Petr Cvek

Dne 20.8.2015 v 21:51 Robert Jarzmik napsal(a):

Quoted 10 lines

arch/arm/mach-pxa/magician.c static struct pxamci_platform_data magician_mci_info = { · line 914

Quoted 6 lines · Petr Cvek
arch/arm/mach-pxa/magician.c +6 -0
+/*
+ * 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).

Quoted 6 lines

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.

Signature · 5 lines
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.
author
#37 lore raw

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

In reply to a message not in the archive

Add support for MAX1586 Vcore regulator.

---
 arch/arm/mach-pxa/magician.c | 50 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)
arch/arm/mach-pxa/magician.c +50 -0
index 9decd86..8ca0b67 100644
@@ -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);
Signature · 1 line
1.7.12.1
maintainer
#38 lore raw

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

Quoted 3 lines · Petr Cvek

Add support for MAX1586 Vcore regulator.

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

author
#39 lore raw

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

Dne 20.8.2015 v 21:32 Robert Jarzmik napsal(a):

Quoted 4 lines
Quoted 2 lines · Petr Cvek

Add support for MAX1586 Vcore regulator.

Quoted 9 lines
Quoted 1 line

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.

Quoted 5 lines

Cheers.

--
Robert

maintainer
#40 lore raw

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

Quoted 8 lines · Petr Cvek

Dne 20.8.2015 v 21:32 Robert Jarzmik napsal(a):

Quoted 7 lines
Quoted 3 lines · Petr Cvek

Add support for MAX1586 Vcore regulator.

I'll leave that for a week and if no objections queue it.

Quoted 8 lines
Quoted 6 lines

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.

Signature · 1 line
Robert
author
#41 lore raw

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

In reply to a message not in the archive

Add support for PXA27x UDC to HTC Magician.

---
 arch/arm/mach-pxa/magician.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)
arch/arm/mach-pxa/magician.c +39 -1
index 8ca0b67..23b59a0 100644
@@ -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);
Signature · 1 line
1.7.12.1
maintainer
#42 lore raw

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

Quoted 23 lines · Petr Cvek

Add support for PXA27x UDC to HTC Magician.

---
 arch/arm/mach-pxa/magician.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)
arch/arm/mach-pxa/magician.c +4 -0
index 8ca0b67..23b59a0 100644
@@ -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"
 

arch/arm/mach-pxa/magician.c static struct platform_device magician_camera = { · line 705

Quoted 4 lines
arch/arm/mach-pxa/magician.c +1 -0
@@ -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.

arch/arm/mach-pxa/magician.c static struct platform_device magician_camera = { · line 715

Quoted 10 lines
arch/arm/mach-pxa/magician.c +10 -0
+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 ?

arch/arm/mach-pxa/magician.c static struct platform_device magician_camera = { · line 720

Quoted 4 lines
arch/arm/mach-pxa/magician.c +4 -0
+static int magician_udc_is_connected(void)
+{
+	/* Shared with pda_power or gpio-vbus */
+	return my_usb_online;

gpio_get_value() something ?

arch/arm/mach-pxa/magician.c static struct platform_device magician_camera = { · line 725

Quoted 5 lines
arch/arm/mach-pxa/magician.c +5 -0
+}
+
+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.

arch/arm/mach-pxa/magician.c static struct platform_device *devices[] __initdata = { · line 1211

Quoted 6 lines
arch/arm/mach-pxa/magician.c +2 -0
@@ -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.

Signature · 1 line
Robert
author
#43 lore raw

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

Dne 20.8.2015 v 19:23 Robert Jarzmik napsal(a):

Quoted 31 lines
Quoted 23 lines · Petr Cvek

Add support for PXA27x UDC to HTC Magician.

---
 arch/arm/mach-pxa/magician.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)
arch/arm/mach-pxa/magician.c +4 -0
index 8ca0b67..23b59a0 100644
@@ -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"
 

arch/arm/mach-pxa/magician.c static struct platform_device magician_camera = { · line 705

Quoted 4 lines
arch/arm/mach-pxa/magician.c +1 -0
@@ -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.

Quoted 12 lines

arch/arm/mach-pxa/magician.c static struct platform_device magician_camera = { · line 715

Quoted 10 lines
arch/arm/mach-pxa/magician.c +10 -0
+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).

Quoted 7 lines

arch/arm/mach-pxa/magician.c static struct platform_device magician_camera = { · line 720

Quoted 5 lines
arch/arm/mach-pxa/magician.c +4 -0
+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.

Quoted 8 lines

arch/arm/mach-pxa/magician.c static struct platform_device magician_camera = { · line 725

Quoted 5 lines
arch/arm/mach-pxa/magician.c +5 -0
+}
+
+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.

Quoted 9 lines

arch/arm/mach-pxa/magician.c static struct platform_device *devices[] __initdata = { · line 1211

Quoted 7 lines
arch/arm/mach-pxa/magician.c +2 -0
@@ -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).

Quoted 3 lines

Cheers.

Best regards,
Petr

maintainer
#44 lore raw

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

Quoted 15 lines · Petr Cvek

Dne 20.8.2015 v 19:23 Robert Jarzmik napsal(a):

Quoted 13 lines

arch/arm/mach-pxa/magician.c static struct platform_device magician_camera = { · line 715

Quoted 10 lines · Petr Cvek
arch/arm/mach-pxa/magician.c +10 -0
+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 ?

Quoted 2 lines

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.

Quoted 19 lines
Quoted 7 lines

arch/arm/mach-pxa/magician.c static struct platform_device magician_camera = { · line 720

Quoted 5 lines
arch/arm/mach-pxa/magician.c +4 -0
+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.

Quoted 8 lines

arch/arm/mach-pxa/magician.c static struct platform_device magician_camera = { · line 725

Quoted 5 lines
arch/arm/mach-pxa/magician.c +5 -0
+}
+
+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.

Quoted 8 lines

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()
Quoted 9 lines
Quoted 8 lines

arch/arm/mach-pxa/magician.c static struct platform_device *devices[] __initdata = { · line 1211

Quoted 6 lines
arch/arm/mach-pxa/magician.c +2 -0
@@ -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 ?

Quoted 7 lines

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.

Signature · 1 line
Robert
author
#45 lore raw

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

In reply to a message not in the archive

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

---
 arch/arm/mach-pxa/magician.c | 83 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 73 insertions(+), 10 deletions(-)
arch/arm/mach-pxa/magician.c +73 -10
index 23b59a0..50c5325 100644
@@ -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),
Signature · 1 line
1.7.12.1
author
#46 lore raw

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

Dne 18.8.2015 v 00:03 Petr Cvek napsal(a):

arch/arm/mach-pxa/magician.c static struct platform_device gpio_vbus = { · line 824

Quoted 6 lines
arch/arm/mach-pxa/magician.c +5 -0
+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

author
#47 lore raw

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

In reply to a message not in the archive

Fix PXA USB OHCI port enable flags.

---
 arch/arm/mach-pxa/magician.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
arch/arm/mach-pxa/magician.c +3 -1
index 50c5325..d17125e 100644
@@ -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,
 };
 
 /*
Signature · 1 line
1.7.12.1
author
#48 lore raw

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

In reply to a message not in the archive

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

---
 arch/arm/mach-pxa/magician.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
arch/arm/mach-pxa/magician.c +11 -0
index d17125e..2ec630e 100644
@@ -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")
Signature · 1 line
1.7.12.1
maintainer
#49 lore raw

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

Quoted 1 line · Petr Cvek

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.

author
#50 lore raw

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

In reply to a message not in the archive

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

---
 arch/arm/mach-pxa/magician.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)
arch/arm/mach-pxa/magician.c +21 -1
index 2ec630e..780cc3d 100644
@@ -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,
 };
 
 /*
Signature · 1 line
1.7.12.1
maintainer
#51 lore raw

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

Quoted 1 line · Petr Cvek

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

author
#52 lore raw

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

Dne 20.8.2015 v 22:01 Robert Jarzmik napsal(a):

Quoted 11 lines
Quoted 1 line · Petr Cvek

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

maintainer
#53 lore raw

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

Quoted 6 lines · Petr Cvek

Dne 20.8.2015 v 22:01 Robert Jarzmik napsal(a):

Quoted 5 lines
Quoted 1 line · Petr Cvek

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

Here, I don't understand, the commit message is too short.

Quoted 7 lines
Quoted 6 lines

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.

Quoted 3 lines

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 ?

Quoted 9 lines

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 */
Quoted 6 lines

#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.

Signature · 1 line
Robert
author
#54 lore raw

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

In reply to a message not in the archive

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

---
 arch/arm/mach-pxa/magician.c | 45 ++------------------------------------------
 1 file changed, 2 insertions(+), 43 deletions(-)
arch/arm/mach-pxa/magician.c +2 -43
index 780cc3d..d8994dd 100644
@@ -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
  */
Signature · 1 line
1.7.12.1
maintainer
#55 lore raw

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

Quoted 1 line · Petr Cvek

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

author
#56 lore raw

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

In reply to a message not in the archive

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

---
 drivers/mfd/htc-pasic3.c       | 54 ++++++++++++++++-----------
 include/linux/mfd/htc-pasic3.h | 85 +++++++++++++++++++++++++++++++-----------
 2 files changed, 97 insertions(+), 42 deletions(-)
drivers/mfd/htc-pasic3.c +33 -21
index e88d4f6..16e156d 100644
@@ -3,6 +3,9 @@
  *
  * Copyright (C) 2006 Philipp Zabel <[email protected]>
  *
+ * 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;
include/linux/mfd/htc-pasic3.h +64 -21

107 lines not on this page · raw message

Signature · 1 line
1.7.12.1
#57 lore raw

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

Quoted 14 lines · Petr Cvek

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

---
 drivers/mfd/htc-pasic3.c       | 54 ++++++++++++++++-----------
 include/linux/mfd/htc-pasic3.h | 85 +++++++++++++++++++++++++++++++-----------
 2 files changed, 97 insertions(+), 42 deletions(-)
drivers/mfd/htc-pasic3.c +0 -0
index e88d4f6..16e156d 100644

drivers/mfd/htc-pasic3.c line 8

Quoted 6 lines
drivers/mfd/htc-pasic3.c +3 -0
@@ -3,6 +3,9 @@
  *
  * Copyright (C) 2006 Philipp Zabel <[email protected]>
  *
+ * 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.

Quoted 85 lines
drivers/mfd/htc-pasic3.c +24 -14
  * 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");
 	}

drivers/mfd/htc-pasic3.c static int __init pasic3_probe(struct platform_device *pdev) · line 189

Quoted 10 lines
drivers/mfd/htc-pasic3.c +2 -7
 
-	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.

drivers/mfd/htc-pasic3.c static int __init pasic3_probe(struct platform_device *pdev) · line 192

Quoted 3 lines
drivers/mfd/htc-pasic3.c +3 -0
+		if (leds_pdata) {
+			leds_pdata->mfd_dev = dev;
+			platform_device_register(pdata->pasic3_leds);

What's the idea here?

Quoted 37 lines
drivers/mfd/htc-pasic3.c +1 -0
+		}
 	}
 
 	return 0;
include/linux/mfd/htc-pasic3.h +12 -6
index 3d3ed67..91a27b6 100644
@@ -2,6 +2,7 @@
  * HTC PASIC3 driver - LEDs and DS1WM
  *
  * Copyright (c) 2007 Philipp Zabel <[email protected]>
+ * Copyright (c) 2015 Petr Cvek <[email protected]>
  *
  * 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)

include/linux/mfd/htc-pasic3.h line 41

Quoted 10 lines
include/linux/mfd/htc-pasic3.h +7 -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.

Quoted 59 lines
include/linux/mfd/htc-pasic3.h +37 -14
  */
-#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 {

include/linux/mfd/htc-pasic3.h line 96

Quoted 10 lines
include/linux/mfd/htc-pasic3.h +8 -0
+	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.

Quoted 1 line
include/linux/mfd/htc-pasic3.h +0 -0
 #endif
Signature · 4 lines
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
author
#58 lore raw

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

Dne 18.8.2015 v 08:52 Lee Jones napsal(a):

Quoted 25 lines · Petr Cvek
Quoted 15 lines

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

---
 drivers/mfd/htc-pasic3.c       | 54 ++++++++++++++++-----------
 include/linux/mfd/htc-pasic3.h | 85 +++++++++++++++++++++++++++++++-----------
 2 files changed, 97 insertions(+), 42 deletions(-)
drivers/mfd/htc-pasic3.c +0 -0
index e88d4f6..16e156d 100644

drivers/mfd/htc-pasic3.c line 8

Quoted 6 lines
drivers/mfd/htc-pasic3.c +3 -0
@@ -3,6 +3,9 @@
  *
  * Copyright (C) 2006 Philipp Zabel <[email protected]>
  *
+ * 2015:	Added registers for LED and RESET, see htc-pasic3.h
+ *		-- Petr Cvek
+ *

This is pretty unconventional.

Quoted 1 line

Please look to see what others have done.

LED support: Petr Cvek <[email protected]>

OK?

Quoted 33 lines
Quoted 20 lines
drivers/mfd/htc-pasic3.c +3 -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");
 	}

drivers/mfd/htc-pasic3.c static int __init pasic3_probe(struct platform_device *pdev) · line 189

Quoted 10 lines
drivers/mfd/htc-pasic3.c +2 -7
 
-	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?

Quoted 8 lines

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

drivers/mfd/htc-pasic3.c static int __init pasic3_probe(struct platform_device *pdev) · line 192

Quoted 3 lines
drivers/mfd/htc-pasic3.c +3 -0
+		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

#59 lore raw

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

Quoted 26 lines · Petr Cvek

Dne 18.8.2015 v 08:52 Lee Jones napsal(a):

Quoted 25 lines · Petr Cvek
Quoted 15 lines

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

---
 drivers/mfd/htc-pasic3.c       | 54 ++++++++++++++++-----------
 include/linux/mfd/htc-pasic3.h | 85 +++++++++++++++++++++++++++++++-----------
 2 files changed, 97 insertions(+), 42 deletions(-)
drivers/mfd/htc-pasic3.c +0 -0
index e88d4f6..16e156d 100644

drivers/mfd/htc-pasic3.c line 8

Quoted 6 lines
drivers/mfd/htc-pasic3.c +3 -0
@@ -3,6 +3,9 @@
  *
  * Copyright (C) 2006 Philipp Zabel <[email protected]>
  *
+ * 2015:	Added registers for LED and RESET, see htc-pasic3.h
+ *		-- Petr Cvek
+ *

This is pretty unconventional.

Quoted 3 lines
Quoted 1 line

Please look to see what others have done.

LED support: Petr Cvek <[email protected]>

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 <[email protected]>

... will do fine.

Quoted 42 lines
Quoted 33 lines
Quoted 20 lines
drivers/mfd/htc-pasic3.c +3 -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");
 	}

drivers/mfd/htc-pasic3.c static int __init pasic3_probe(struct platform_device *pdev) · line 189

Quoted 10 lines
drivers/mfd/htc-pasic3.c +2 -7
 
-	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?

Quoted 8 lines

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

drivers/mfd/htc-pasic3.c static int __init pasic3_probe(struct platform_device *pdev) · line 192

Quoted 3 lines
drivers/mfd/htc-pasic3.c +3 -0
+		if (leds_pdata) {
+			leds_pdata->mfd_dev = dev;
+			platform_device_register(pdata->pasic3_leds);

What's the idea here?

Quoted 5 lines

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?

Signature · 4 lines
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
author
#60 lore raw

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

In reply to a message not in the archive

Add support for LEDs inside the PASIC3 chip.

---
 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
drivers/leds/Kconfig +9 -0
index 9ad35f7..516ba65 100644
@@ -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"
 
drivers/leds/Makefile +1 -0
index 8d6a24a..b1c659c 100644
@@ -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
drivers/leds/leds-pasic3.c new file +232 -0

232 lines not on this page · raw message

Signature · 1 line
1.7.12.1
author
#61 lore raw

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

Dne 18.8.2015 v 00:06 Petr Cvek napsal(a):

Quoted 1 line

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).

#62 lore raw

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

In reply to Petr Cvek

Hi Petr,

Quoted 48 lines · Petr Cvek

Add support for LEDs inside the PASIC3 chip.

---
  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

drivers/leds/Kconfig +9 -0
index 9ad35f7..516ba65 100644
@@ -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"
 
drivers/leds/Makefile +1 -0
index 8d6a24a..b1c659c 100644
@@ -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
drivers/leds/leds-pasic3.c new file +0 -0
new file mode 100644
index 0000000..0f95230

drivers/leds/leds-pasic3.c line 2

Quoted 2 lines
drivers/leds/leds-pasic3.c new file +2 -0
@@ -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?

Quoted 23 lines
drivers/leds/leds-pasic3.c new file +23 -0
+ *
+ * Copyright (c) 2015 Petr Cvek <[email protected]>
+ *
+ * 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

drivers/leds/leds-pasic3.c line 35

Quoted 10 lines
drivers/leds/leds-pasic3.c new file +10 -0
+ *	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.

Quoted 13 lines
drivers/leds/leds-pasic3.c new file +13 -0
+
+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) {

drivers/leds/leds-pasic3.c line 58

Quoted 10 lines
drivers/leds/leds-pasic3.c new file +10 -0
+		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.

drivers/leds/leds-pasic3.c line 69

Quoted 11 lines
drivers/leds/leds-pasic3.c new file +11 -0
+
+			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++;

Quoted 26 lines
drivers/leds/leds-pasic3.c new file +26 -0
+
+		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 {

drivers/leds/leds-pasic3.c line 106

Quoted 10 lines
drivers/leds/leds-pasic3.c new file +10 -0
+		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?

Quoted 28 lines
drivers/leds/leds-pasic3.c new file +28 -0
+
+	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;
+	}
+

drivers/leds/leds-pasic3.c line 144

Quoted 10 lines
drivers/leds/leds-pasic3.c new file +10 -0
+	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.

Quoted 89 lines
drivers/leds/leds-pasic3.c new file +88 -0
+		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 <[email protected]>");
+MODULE_DESCRIPTION("HTC Magician PASIC3 LED driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:leds-pasic3");
Signature · 2 lines
Best Regards,
Jacek Anaszewski
author
#63 lore raw

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

Dne 18.8.2015 v 12:27 Jacek Anaszewski napsal(a):

Quoted 9 lines

Hi Petr,

Quoted 2 lines · Petr Cvek

+/*
+ * 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).

Quoted 3 lines
Quoted 1 line

+spinlock_t lock;

Please move it to the struct pasic3_leds_data.

OK

Quoted 33 lines
Quoted 14 lines
drivers/leds/leds-pasic3.c new file +13 -0
+
+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) {

drivers/leds/leds-pasic3.c line 58

Quoted 10 lines
drivers/leds/leds-pasic3.c new file +10 -0
+        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:

Quoted 9 lines

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);

Quoted 5 lines

drivers/leds/leds-pasic3.c line 144

Quoted 2 lines
drivers/leds/leds-pasic3.c new file +2 -0
+        ret = gpio_request(pdata->power_gpio,
+            "Red-Blue LED Power");

Please use devm_gpio_request. Then you will not need pasic3_led_remove.

OK

#64 lore raw

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

Am Dienstag, den 18.08.2015, 23:48 +0200 schrieb Petr Cvek:

Quoted 10 lines

Dne 18.8.2015 v 12:27 Jacek Anaszewski napsal(a):

Quoted 9 lines

Hi Petr,

Quoted 3 lines · Petr Cvek

+/*
+ * AIC3 (or PASIC3) LED driver for HTC Magician/Alpine/.. (not Compaq ASIC3)

s/AIC3/ASIC3/

Quoted 8 lines
Quoted 1 line

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

#65 lore raw

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

In reply to Petr Cvek
Quoted 4 lines · Petr Cvek

Dne 18.8.2015 v 12:27 Jacek Anaszewski napsal(a):

Quoted 3 lines

Hi Petr,

On 08/18/2015 12:06 AM, Petr Cvek wrote:

Quoted 8 lines
Quoted 6 lines
Quoted 2 lines

+/*
+ * 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.

Quoted 45 lines
Quoted 3 lines
Quoted 1 line

+spinlock_t lock;

Please move it to the struct pasic3_leds_data.

OK

Quoted 39 lines
Quoted 14 lines
drivers/leds/leds-pasic3.c new file +13 -0
+
+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) {

drivers/leds/leds-pasic3.c line 58

Quoted 10 lines
drivers/leds/leds-pasic3.c new file +10 -0
+        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++;

Quoted 6 lines
Quoted 3 lines

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.

Quoted 7 lines

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.

Quoted 10 lines
Quoted 6 lines

drivers/leds/leds-pasic3.c line 144

Quoted 3 lines
drivers/leds/leds-pasic3.c new file +2 -0
+        ret = gpio_request(pdata->power_gpio,
+            "Red-Blue LED Power");

Please use devm_gpio_request. Then you will not need pasic3_led_remove.

OK

Signature · 2 lines
Best Regards,
Jacek Anaszewski
author
#66 lore raw

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

In reply to a message not in the archive

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

---
 arch/arm/mach-pxa/magician.c | 54 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)
arch/arm/mach-pxa/magician.c +53 -1
index d8994dd..5f02ce4 100644
@@ -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 = {
Signature · 1 line
1.7.12.1
author
#67 lore raw

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

In reply to a message not in the archive

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

---
 arch/arm/mach-pxa/magician.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
arch/arm/mach-pxa/magician.c +3 -2
index 5f02ce4..af86350 100644
@@ -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")
Signature · 1 line
1.7.12.1
maintainer
#68 lore raw

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

Quoted 4 lines · Petr Cvek

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

With Philipp's ack.

Signature · 1 line
Robert