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

Philipp Zabel
2015-08-19T07:57:05Z

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

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

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

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

These comments are nice to have.

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

Why reorder?

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

Ok.

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

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

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

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

regards
Philipp