[PATCH] hxd8-power-control.patch

[Harald Welte]

hxd8-pm-gps:

this driver doesn't actually seem to do anything apart from
probe/remove/suspend/resume.  Where is the interface for
power-up/power-down of the GPS?

> +#define UARTPORT	"/dev/ttySAC1"

this define is not used anywhere.

> +static int __init hxd8_pm_gps_probe(struct platform_device *pdev)
> +{
> +	printk("hxd8_pm_gps probing \n");
> +
> +	/* Initialize GPS Module. */
> +	/* GPC0 High*/
> +	s3c2410_gpio_cfgpin(S3C2410_GPC0, 0x01);

First of all: the GPIO's used should have #define's such as the
GTA01_GPIO_GPS_EN_3V3 names in the gta01 code.  Directly using
S3C2410_GPC0 is not good coding style

Also, using 0x01 is not good style either. Please use the symbolic names
such as S3C2410_GPIO_OUTPUT.

> +#ifdef CONFIG_PM
> +static int hxd8_pm_gps_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	/* GPC0 Low*/
> +	s3c2410_gpio_cfgpin(S3C2410_GPC0, 0x01);
> +	s3c2410_gpio_setpin(S3C2410_GPC0, 0);
> +	return 0;
> +}
> +
> +static int hxd8_pm_gps_resume(struct platform_device *pdev)
> +{
> +	/* GPC0 High*/
> +	s3c2410_gpio_cfgpin(S3C2410_GPC0, 0x01);
> +	s3c2410_gpio_setpin(S3C2410_GPC0, 1);
> +	return 0;
> +}
> +#else

I didn't look at the schematics, but if GPC0 is something like the power
control of GPS, this seems to be wrong.  How can you know, that
unconditionally after resume you want to power up the GPS?  Shouldn't it
come back to the state that was present before suspend?

Also, if this is the case, how do you know that the user always wants
the GPS to go off before suspending?  Doesn't that affect your
time-to-fix?

> +#ifdef CONFIG_PM
> +static int hxd8_gsm_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	/* set Modem CTS/RST/TXD/RXD pin as output to reduce power consumption */
> +	s3c2410_gpio_setpin(S3C2410_GPH0, S3C2410_GPH0_OUTP);
> +	s3c2410_gpio_setpin(S3C2410_GPH1, S3C2410_GPH1_OUTP);
> +	s3c2410_gpio_setpin(S3C2410_GPH2, S3C2410_GPH2_OUTP);
> +	s3c2410_gpio_setpin(S3C2410_GPH3, S3C2410_GPH3_OUTP);

again, please add #defines with reasonable nameees for the GPIO's.

> +	dev_info(&pdev->dev, "hxd8_gsm_probing \n");
> +	s3c2410_gpio_cfgpin(HXD8_GPIO_nGSM_EN, S3C2410_GPB7_OUTP);
> +	s3c2410_gpio_setpin(HXD8_GPIO_nGSM_EN, 1);

please don't unconditionally enable the GSM modem during probe.  Please
use the same logic as GTA01 (off by default, only after sysfs API used:
power up).

also: since [currently] the GSM part of hxd8 and gta01 is the same,
can't we use the same driver?  The GPIO's could be passed into the
driver as platform_resource.

-- 
- Harald Welte <laforge at openmoko.org>          	        http://openmoko.org/
============================================================================
Software for the world's first truly open Free Software mobile phone