[PATCH] make neo1973_pm_gps.c suck less

Vladimir Koutny vlado at moko.ksp.sk
Mon Jan 19 10:05:25 CET 2009


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Werner,

Werner Almesberger wrote:
> Here's a bunch of changes to neo1973_pm_gps.c. The thing I set out to
> change was just the initial power state, but there's so much wrong in
> there, I couldn't resist slaying some evil on the way:

any reason my 2 patches (or at least the first one which fixes a power-on
resume bug) didn't get included yet? (see your/om-kernel mailbox around Jan5;
probably won't apply directly after your patch but are rather trivial anyway)

Vladimir


> 
> - update the copyright year
> - show slightly more confidence in operator precedence
> - simplify #ifdef rat's nest a little
> - rename "pwron" sysfs file to "power_on", to conform to all the rest
> - wrap lines longer than 80 columns
> - use PTR_ERR, not cast
> - reduce redundancy and increase readability in code patching
>   gta01_gps_sysfs_entries
> - on GTA02 and if booted by u-boot, GPS is powered on. Power it off,
>   like on GTA01 or with Qi.
> 
> It's still nasty, but slightly less painful to look at now. Tested on
> GTA02 and tested that it still compiles for GTA01.
> 
> The name change from "pwron" to "power_on" may be controversial. If
> this causes undue misery for distribution makers, please holler.
> 
> Signed-off-by: Werner Almesberger <werner at openmoko.org>
> 
> ---
> 
> Index: ktrack/arch/arm/plat-s3c24xx/neo1973_pm_gps.c
> ===================================================================
> --- ktrack.orig/arch/arm/plat-s3c24xx/neo1973_pm_gps.c	2009-01-17 12:12:44.000000000 -0200
> +++ ktrack/arch/arm/plat-s3c24xx/neo1973_pm_gps.c	2009-01-17 12:28:54.000000000 -0200
> @@ -1,7 +1,7 @@
>  /*
>   * GPS Power Management code for the FIC Neo1973 GSM Phone
>   *
> - * (C) 2007 by Openmoko Inc.
> + * (C) 2007-2009 by Openmoko Inc.
>   * Author: Harald Welte <laforge at openmoko.org>
>   * All rights reserved.
>   *
> @@ -29,6 +29,7 @@
>  
>  /* For GTA02 */
>  #include <mach/gta02.h>
> +#include <linux/mfd/pcf50633/core.h>
>  
>  #include <linux/regulator/consumer.h>
>  
> @@ -294,10 +295,10 @@
>  			/* don't let RX from unpowered GPS float */
>  			s3c2410_gpio_pullup(S3C2410_GPH5, 1);
>  		}
> -		if ((on) && (!neo1973_gps.power_was_on))
> +		if (on && !neo1973_gps.power_was_on)
>  			regulator_enable(neo1973_gps.regulator);
>  
> -		if ((!on) && (neo1973_gps.power_was_on))
> +		if (!on && neo1973_gps.power_was_on)
>  			regulator_disable(neo1973_gps.regulator);
>  	}
>  
> @@ -325,10 +326,7 @@
>  {
>  	int ret = 0;
>  
> -	if (!strcmp(attr->attr.name, "pwron"))
> -#ifdef CONFIG_MACH_NEO1973_GTA01
> -	{
> -#endif
> +	if (!strcmp(attr->attr.name, "power_on")) {
>  		ret = gps_pwron_get();
>  #ifdef CONFIG_MACH_NEO1973_GTA01
>  	} else if (!strcmp(attr->attr.name, "power_avdd_3v")) {
> @@ -344,8 +342,8 @@
>  	} else if (!strcmp(attr->attr.name, "power_core_1v5") ||
>  		   !strcmp(attr->attr.name, "power_vdd_core_1v5")) {
>  		ret = gps_power_1v5_get();
> -	}
>  #endif
> +	}
>  	if (ret)
>  		return strlcpy(buf, "1\n", 3);
>  	else
> @@ -358,10 +356,7 @@
>  {
>  	unsigned long on = simple_strtoul(buf, NULL, 10);
>  
> -	if (!strcmp(attr->attr.name, "pwron"))
> -#ifdef CONFIG_MACH_NEO1973_GTA01
> -{
> -#endif
> +	if (!strcmp(attr->attr.name, "power_on")) {
>  		gps_pwron_set(on);
>  #ifdef CONFIG_MACH_NEO1973_GTA01
>  	} else if (!strcmp(attr->attr.name, "power_avdd_3v")) {
> @@ -377,8 +372,8 @@
>  	} else if (!strcmp(attr->attr.name, "power_core_1v5") ||
>  		   !strcmp(attr->attr.name, "power_vdd_core_1v5")) {
>  		gps_power_1v5_set(on);
> -	}
>  #endif
> +	}
>  	return count;
>  }
>  
> @@ -543,11 +538,11 @@
>  #define gta01_pm_gps_resume	NULL
>  #endif
>  
> -static DEVICE_ATTR(pwron, 0644, power_gps_read, power_gps_write);
> +static DEVICE_ATTR(power_on, 0644, power_gps_read, power_gps_write);
>  
>  
>  static struct attribute *gta01_gps_sysfs_entries[] = {
> -	&dev_attr_pwron.attr,
> +	&dev_attr_power_on.attr,
>  #ifdef CONFIG_MACH_NEO1973_GTA01
>  	&dev_attr_power_avdd_3v.attr,
>  	&dev_attr_reset.attr,
> @@ -566,7 +561,7 @@
>  };
>  
>  static struct attribute *gta02_gps_sysfs_entries[] = {
> -	&dev_attr_pwron.attr,
> +	&dev_attr_power_on.attr,
>  	NULL
>  };
>  
> @@ -584,16 +579,21 @@
>  		case GTA01v3_SYSTEM_REV:
>  			break;
>  		case GTA01v4_SYSTEM_REV:
> -			s3c2410_gpio_cfgpin(GTA01_GPIO_GPS_RESET, S3C2410_GPIO_OUTPUT);
> +			s3c2410_gpio_cfgpin(GTA01_GPIO_GPS_RESET,
> +			    S3C2410_GPIO_OUTPUT);
>  			break;
>  		case GTA01Bv3_SYSTEM_REV:
>  		case GTA01Bv4_SYSTEM_REV:
> -			s3c2410_gpio_cfgpin(GTA01_GPIO_GPS_EN_3V3, S3C2410_GPIO_OUTPUT);
> +			s3c2410_gpio_cfgpin(GTA01_GPIO_GPS_EN_3V3,
> +			    S3C2410_GPIO_OUTPUT);
>  			/* fallthrough */
>  		case GTA01Bv2_SYSTEM_REV:
> -			s3c2410_gpio_cfgpin(GTA01_GPIO_GPS_EN_2V8, S3C2410_GPIO_OUTPUT);
> -			s3c2410_gpio_cfgpin(GTA01_GPIO_GPS_EN_3V, S3C2410_GPIO_OUTPUT);
> -			s3c2410_gpio_cfgpin(GTA01_GPIO_GPS_RESET, S3C2410_GPIO_OUTPUT);
> +			s3c2410_gpio_cfgpin(GTA01_GPIO_GPS_EN_2V8,
> +			    S3C2410_GPIO_OUTPUT);
> +			s3c2410_gpio_cfgpin(GTA01_GPIO_GPS_EN_3V,
> +			    S3C2410_GPIO_OUTPUT);
> +			s3c2410_gpio_cfgpin(GTA01_GPIO_GPS_RESET,
> +			    S3C2410_GPIO_OUTPUT);
>  			break;
>  		default:
>  			dev_warn(&pdev->dev, "Unknown GTA01 Revision 0x%x, "
> @@ -607,22 +607,24 @@
>  		gps_power_sequence_down();
>  
>  		switch (system_rev) {
> +			int entries = ARRAY_SIZE(gta01_gps_sysfs_entries);
>  		case GTA01v3_SYSTEM_REV:
>  		case GTA01v4_SYSTEM_REV:
>  		case GTA01Bv2_SYSTEM_REV:
> -			gta01_gps_sysfs_entries[ARRAY_SIZE(gta01_gps_sysfs_entries)-3] =
> -						&dev_attr_power_tcxo_2v8.attr;
> +			gta01_gps_sysfs_entries[entries-3] =
> +			    &dev_attr_power_tcxo_2v8.attr;
>  			break;
>  		case GTA01Bv3_SYSTEM_REV:
>  		case GTA01Bv4_SYSTEM_REV:
> -			gta01_gps_sysfs_entries[ARRAY_SIZE(gta01_gps_sysfs_entries)-3] =
> -						&dev_attr_power_core_1v5.attr;
> -			gta01_gps_sysfs_entries[ARRAY_SIZE(gta01_gps_sysfs_entries)-2] =
> -						&dev_attr_power_vdd_core_1v5.attr;
> +			gta01_gps_sysfs_entries[entries-3] =
> +			    &dev_attr_power_core_1v5.attr;
> +			gta01_gps_sysfs_entries[entries-2] =
> +			    &dev_attr_power_vdd_core_1v5.attr;
>  			break;
>  		}
>  #endif
> -		return sysfs_create_group(&pdev->dev.kobj, &gta01_gps_attr_group);
> +		return sysfs_create_group(&pdev->dev.kobj,
> +		    &gta01_gps_attr_group);
>  	}
>  
>  	if (machine_is_neo1973_gta02()) {
> @@ -635,9 +637,9 @@
>  			neo1973_gps.regulator = regulator_get(
>  							&pdev->dev, "RF_3V");
>  			if (IS_ERR(neo1973_gps.regulator)) {
> -				dev_err(&pdev->dev, "probe failed %d\n",
> -						    (int)neo1973_gps.regulator);
> -				return (int)neo1973_gps.regulator;
> +				dev_err(&pdev->dev, "probe failed %ld\n",
> +				    PTR_ERR(neo1973_gps.regulator));
> +				return PTR_ERR(neo1973_gps.regulator);
>  			}
>  
>  			dev_info(&pdev->dev, "FIC Neo1973 GPS Power Management:"
> @@ -648,9 +650,21 @@
>  				"AGPS PM features not available!!!\n",
>  				system_rev);
>  			return -1;
> -			break;
>  		}
> -		return sysfs_create_group(&pdev->dev.kobj, &gta02_gps_attr_group);
> +
> +		/*
> +		 * u-boot enables LDO5 (GPS), which doesn't make sense and
> +		 * causes confusion. We therefore disable the regulator here.
> +		 *
> +		 * We don't do this through the regulator API because we'd have
> +		 * to second-guess some of its internal logic and make it do
> +		 * something that isn't really part of its design.
> +		 */
> +		pcf50633_reg_write(gta02_pcf_pdata.pcf,
> +		    PCF50633_REG_LDO5ENA, 0);
> +
> +		return sysfs_create_group(&pdev->dev.kobj,
> +		    &gta02_gps_attr_group);
>  	}
>  	return -1;
>  }
> 

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iJwEAQECAAYFAkl0QlUACgkQutgEj9ZLuzSGvgP/Yn0x1jEgxIjV8Zps0G6Y4hx+
+5W0SaYwlbezdDvCLwRzaC6saQq8ODdu50hhzziewNGVApP0EiFnalieoo/8R9EQ
endrY/TC+he2SZIxCvu5IoLDZhZ1KW6SyC/VwwZAsmOF1jnIq55FW747PQHUr5GX
vpOwHia2WBcaOqeEKIg=
=wip7
-----END PGP SIGNATURE-----



More information about the openmoko-kernel mailing list