* [rfc] separate sharpsl_pm initialization from sysfs code @ 2006-03-09 12:42 Pavel Machek 2006-03-09 13:33 ` Richard Purdie 0 siblings, 1 reply; 5+ messages in thread From: Pavel Machek @ 2006-03-09 12:42 UTC (permalink / raw) To: rpurdie, lenz, kernel list, Russell King Hi! On collie, battery sensing code is not on platform bus -- it is on ucb1x00. Is this acceptable way to make sharpsl_pm useful for collie? It separates code that is bus-independent, so collie can call only code it needs. Signed-off-by: Pavel Machek <pavel@suse.cz> diff --git a/arch/arm/common/sharpsl_pm.c b/arch/arm/common/sharpsl_pm.c index 978d32e..3482cc0 100644 --- a/arch/arm/common/sharpsl_pm.c +++ b/arch/arm/common/sharpsl_pm.c @@ -765,18 +787,13 @@ static void sharpsl_apm_get_power_status static struct pm_ops sharpsl_pm_ops = { .pm_disk_mode = PM_DISK_FIRMWARE, .prepare = pxa_pm_prepare, .enter = corgi_pxa_pm_enter, .finish = pxa_pm_finish, }; -static int __init sharpsl_pm_probe(struct platform_device *pdev) +int sharpsl_pm_init(void) { - if (!pdev->dev.platform_data) - return -EINVAL; - - sharpsl_pm.dev = &pdev->dev; - sharpsl_pm.machinfo = pdev->dev.platform_data; sharpsl_pm.charge_mode = CHRG_OFF; sharpsl_pm.flags = 0; @@ -788,9 +807,6 @@ static int __init sharpsl_pm_probe(struc sharpsl_pm.machinfo->init(); - device_create_file(&pdev->dev, &dev_attr_battery_percentage); - device_create_file(&pdev->dev, &dev_attr_battery_voltage); - apm_get_power_status = sharpsl_apm_get_power_status; pm_set_ops(&sharpsl_pm_ops); @@ -800,13 +816,10 @@ static int __init sharpsl_pm_probe(struc return 0; } -static int sharpsl_pm_remove(struct platform_device *pdev) +int sharpsl_pm_done(void) { pm_set_ops(NULL); - device_remove_file(&pdev->dev, &dev_attr_battery_percentage); - device_remove_file(&pdev->dev, &dev_attr_battery_voltage); - sharpsl_pm.machinfo->exit(); del_timer_sync(&sharpsl_pm.chrg_full_timer); @@ -815,6 +828,32 @@ static int sharpsl_pm_remove(struct plat return 0; } +static int __init sharpsl_pm_probe(struct platform_device *pdev) +{ + if (!pdev->dev.platform_data) + return -EINVAL; + + sharpsl_pm.dev = &pdev->dev; + sharpsl_pm.machinfo = pdev->dev.platform_data; + + sharpsl_pm_init(); + + device_create_file(&pdev->dev, &dev_attr_battery_percentage); + device_create_file(&pdev->dev, &dev_attr_battery_voltage); + + return 0; +} + +static int sharpsl_pm_remove(struct platform_device *pdev) +{ + sharpsl_pm_done(); + + device_remove_file(&pdev->dev, &dev_attr_battery_percentage); + device_remove_file(&pdev->dev, &dev_attr_battery_voltage); + + return 0; +} + static struct platform_driver sharpsl_pm_driver = { .probe = sharpsl_pm_probe, .remove = sharpsl_pm_remove, -- Web maintainer for suspend.sf.net (www.sf.net/projects/suspend) wanted... ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [rfc] separate sharpsl_pm initialization from sysfs code 2006-03-09 12:42 [rfc] separate sharpsl_pm initialization from sysfs code Pavel Machek @ 2006-03-09 13:33 ` Richard Purdie 2006-03-10 18:07 ` Pavel Machek 0 siblings, 1 reply; 5+ messages in thread From: Richard Purdie @ 2006-03-09 13:33 UTC (permalink / raw) To: Pavel Machek; +Cc: kernel list, lenz On Thu, 2006-03-09 at 13:42 +0100, Pavel Machek wrote: > Hi! > > On collie, battery sensing code is not on platform bus -- it is on > ucb1x00. Is this acceptable way to make sharpsl_pm useful for collie? > It separates code that is bus-independent, so collie can call only > code it needs. I'm not sure why the battery sensing code being on a non-platform bus prevents you from registering a platform device as mentioned in my other email? If you register the device, you then get the benefit of all the features of the common code like the sysfs attributes and common charging code. Its not really designed to be used outside the device model and I suspect trying to do so is just going to mean lots more nasty changes to it in the future... Richard ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [rfc] separate sharpsl_pm initialization from sysfs code 2006-03-09 13:33 ` Richard Purdie @ 2006-03-10 18:07 ` Pavel Machek 2006-03-13 0:02 ` Russell King 0 siblings, 1 reply; 5+ messages in thread From: Pavel Machek @ 2006-03-10 18:07 UTC (permalink / raw) To: Richard Purdie; +Cc: kernel list, lenz Hi! > > On collie, battery sensing code is not on platform bus -- it is on > > ucb1x00. Is this acceptable way to make sharpsl_pm useful for collie? > > It separates code that is bus-independent, so collie can call only > > code it needs. > > I'm not sure why the battery sensing code being on a non-platform bus > prevents you from registering a platform device as mentioned in my other > email? If you register the device, you then get the benefit of all the > features of the common code like the sysfs attributes and common > charging code. Its not really designed to be used outside the device > model and I suspect trying to do so is just going to mean lots more > nasty changes to it in the future... Does this look better? If so, I'll cleanup #if 0s /comments/printks and send it as a proper diff... Pavel /* * Based on spitz_pm.c and sharp code. * * Copyright (C) 2001 SHARP * Copyright 2005 Pavel Machek <pavel@suse.cz> * * Distribute under GPLv2. */ #include <linux/module.h> #include <linux/stat.h> #include <linux/init.h> #include <linux/kernel.h> #include <linux/delay.h> #include <linux/interrupt.h> #include <linux/device.h> #include <linux/platform_device.h> #include <asm/irq.h> #include <asm/mach-types.h> #include <asm/hardware.h> #include <asm/hardware/scoop.h> #include <asm/dma.h> #include <asm/arch/collie.h> #include <asm/mach/sharpsl_param.h> #include <asm/hardware/sharpsl_pm.h> #include "../drivers/mfd/ucb1x00.h" #ifdef CONFIG_MCP_UCB1200_TS #error Battery interferes with touchscreen #endif static struct ucb1x00 *ucb; static int ad_revise; #define ADCtoPower(x) ((330 * x * 2) / 1024) static void collie_charger_init(void) { int err; if (sharpsl_param.adadj != -1) { ad_revise = sharpsl_param.adadj; } /* Register interrupt handler. */ if ((err = request_irq(COLLIE_IRQ_GPIO_AC_IN, sharpsl_ac_isr, SA_INTERRUPT, "ACIN", sharpsl_ac_isr))) { printk("Could not get irq %d.\n", COLLIE_IRQ_GPIO_AC_IN); return; } if ((err = request_irq(COLLIE_IRQ_GPIO_CO, sharpsl_chrg_full_isr, SA_INTERRUPT, "CO", sharpsl_chrg_full_isr))) { free_irq(COLLIE_IRQ_GPIO_AC_IN, sharpsl_ac_isr); printk("Could not get irq %d.\n", COLLIE_IRQ_GPIO_CO); return; } #if 0 if (!ucb) printk(KERN_CRIT "ucb is null!\n"); #endif /* Set transition detect */ ucb1x00_enable_irq(ucb, COLLIE_GPIO_AC_IN, UCB_RISING); ucb1x00_enable_irq(ucb, COLLIE_GPIO_CO, UCB_RISING); ucb1x00_adc_enable(ucb); ucb1x00_io_set_dir(ucb, 0, COLLIE_TC35143_GPIO_MBAT_ON | COLLIE_TC35143_GPIO_TMP_ON | COLLIE_TC35143_GPIO_BBAT_ON); return; } static void collie_measure_temp(int on) { if (on) ucb1x00_io_write(ucb, COLLIE_TC35143_GPIO_TMP_ON, 0); else ucb1x00_io_write(ucb, 0, COLLIE_TC35143_GPIO_TMP_ON); } static void collie_charge(int on) { if (on) { printk("Should start charger\n"); } else { printk("Should stop charger\n"); } #ifdef I_AM_SURE #define CF_BUF_CTRL_BASE 0xF0800000 #define SCOOP_REG(adr) (*(volatile unsigned short*)(CF_BUF_CTRL_BASE+(adr))) #define SCOOP_REG_GPWR SCOOP_REG(SCOOP_GPWR) if (on) { set_scoop_gpio(&colliescoop_device.dev, COLLIE_SCP_CHARGE_ON); } else { reset_scoop_gpio(&colliescoop_device.dev, COLLIE_SCP_CHARGE_ON); } #endif } static void collie_discharge(int on) { } static void collie_discharge1(int on) { } static void collie_presuspend(void) { } static void collie_postsuspend(void) { } static int collie_should_wakeup(unsigned int resume_on_alarm) { return 0; } static unsigned long collie_charger_wakeup(void) { return 0; } int collie_read_backup_battery(void) { int voltage; /* Gives 75..130 */ ucb1x00_io_write(ucb, COLLIE_TC35143_GPIO_BBAT_ON, 0); voltage = ucb1x00_adc_read(ucb, UCB_ADC_INP_AD1, UCB_SYNC); ucb1x00_io_write(ucb, 0, COLLIE_TC35143_GPIO_BBAT_ON); printk("Backup battery = %d(%d)\n", ADCtoPower(voltage), voltage); return ADCtoPower(voltage); } int collie_read_main_battery(void) { int voltage, voltage_rev, voltage_volts; #if 0 collie_read_temp(); collie_read_backup_battery(); #endif ucb1x00_io_write(ucb, 0, COLLIE_TC35143_GPIO_BBAT_ON); ucb1x00_io_write(ucb, COLLIE_TC35143_GPIO_MBAT_ON, 0); voltage = ucb1x00_adc_read(ucb, UCB_ADC_INP_AD1, UCB_SYNC); ucb1x00_io_write(ucb, 0, COLLIE_TC35143_GPIO_MBAT_ON); /* gives values 160..255 with battery removed... and 145..255 with battery inserted. (on AC) On battery, it goes as low as 80. */ voltage_rev = voltage + ((ad_revise * voltage) / 652); voltage_volts = ADCtoPower(voltage_rev); printk("Main battery = %d(%d)\n", voltage_volts, voltage); if ( voltage != -1 ) { return voltage_volts; } else { return voltage; } } int collie_read_temp(void) { int voltage; /* temp must be > 973, main battery must be < 465 */ /* FIXME sharpsl_pm.c has both conditions negated? */ ucb1x00_io_write(ucb, COLLIE_TC35143_GPIO_TMP_ON, 0); voltage = ucb1x00_adc_read(ucb, UCB_ADC_INP_AD0, UCB_SYNC); ucb1x00_io_write(ucb, 0, COLLIE_TC35143_GPIO_TMP_ON); /* >1010 = battery removed. 460 = 22C ? higer = lower temp ? */ printk("Battery temp = %d\n", voltage); return voltage; } static unsigned long read_devdata(int which) { switch (which) { case SHARPSL_BATT_VOLT: return collie_read_main_battery(); case SHARPSL_BATT_TEMP: return collie_read_temp(); case SHARPSL_ACIN_VOLT: return 0x1; case SHARPSL_STATUS_ACIN: { int ret = GPLR & COLLIE_GPIO_AC_IN; printk("AC status = %d\n", ret); return ret; } case SHARPSL_STATUS_FATAL: { int ret = GPLR & COLLIE_GPIO_MAIN_BAT_LOW; printk("Fatal bat = %d\n", ret); return ret; } default: return ~0; } } struct battery_thresh spitz_battery_levels_noac[] = { #ifdef FOO_FRONTLIGHT { 368, 100}, { 358, 25}, { 356, 5}, { 0, 0}, #else { 378, 100}, { 365, 25}, { 363, 5}, { 0, 0}, #endif }; struct battery_thresh spitz_battery_levels_acin[] = { #ifdef FOO_FRONTLIGHT { 368, 100}, { 358, 25}, { 356, 5}, { 0, 0}, #else { 378, 100}, { 365, 25}, { 363, 5}, { 0, 0}, #endif }; struct sharpsl_charger_machinfo collie_pm_machinfo = { .init = collie_charger_init, .read_devdata = read_devdata, .discharge = collie_discharge, .discharge1 = collie_discharge1, .charge = collie_charge, .measure_temp = collie_measure_temp, .presuspend = collie_presuspend, .postsuspend = collie_postsuspend, .charger_wakeup = collie_charger_wakeup, .should_wakeup = collie_should_wakeup, .bat_levels = 3, .bat_levels_noac = spitz_battery_levels_noac, .bat_levels_acin = spitz_battery_levels_acin, .status_high_acin = 368, .status_low_acin = 358, .status_high_noac = 368, .status_low_noac = 358, }; static int __init collie_pm_ucb_add(struct ucb1x00_dev *pdev) { sharpsl_pm.machinfo = &collie_pm_machinfo; ucb = pdev->ucb; return 0; } static struct ucb1x00_driver collie_pm_ucb_driver = { .add = collie_pm_ucb_add, }; static struct platform_device *collie_pm_device; static int __init collie_pm_init(void) { int ret; collie_pm_device = platform_device_alloc("sharpsl-pm", -1); if (!collie_pm_device) return -ENOMEM; collie_pm_device->dev.platform_data = &collie_pm_machinfo; ret = platform_device_add(collie_pm_device); if (ret) platform_device_put(collie_pm_device); if (!ret) ret = ucb1x00_register_driver(&collie_pm_ucb_driver); return ret; } static void __exit collie_pm_exit(void) { ucb1x00_unregister_driver(&collie_pm_ucb_driver); platform_device_unregister(collie_pm_device); } module_init(collie_pm_init); module_exit(collie_pm_exit); -- 6: { ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [rfc] separate sharpsl_pm initialization from sysfs code 2006-03-10 18:07 ` Pavel Machek @ 2006-03-13 0:02 ` Russell King 2006-03-22 9:37 ` Pavel Machek 0 siblings, 1 reply; 5+ messages in thread From: Russell King @ 2006-03-13 0:02 UTC (permalink / raw) To: Pavel Machek; +Cc: Richard Purdie, kernel list, lenz On Fri, Mar 10, 2006 at 07:07:20PM +0100, Pavel Machek wrote: > /* Register interrupt handler. */ > if ((err = request_irq(COLLIE_IRQ_GPIO_AC_IN, sharpsl_ac_isr, SA_INTERRUPT, > "ACIN", sharpsl_ac_isr))) { > printk("Could not get irq %d.\n", COLLIE_IRQ_GPIO_AC_IN); > return; > } > if ((err = request_irq(COLLIE_IRQ_GPIO_CO, sharpsl_chrg_full_isr, SA_INTERRUPT, > "CO", sharpsl_chrg_full_isr))) { > free_irq(COLLIE_IRQ_GPIO_AC_IN, sharpsl_ac_isr); > printk("Could not get irq %d.\n", COLLIE_IRQ_GPIO_CO); > return; > } Shouldn't these be ucb1x00_hook_irq()'s? Shouldn't you only enable the ADC when you need to use it? This driver makes no calls to ucb1x00_enable() and ucb1x00_disable(). They're part of power management, and if your driver doesn't appear to require them to work, that means some other code is buggy (or you're keeping the ADC always enabled.) In order to get maximum power savings, you should ensure that drivers only call ucb1x00_enable() when they need to and ucb1x00_disable() as soon as they can complete the operation. Ditto for ucb1x00_adc_enable() and ucb1x00_adc_disable() - don't use ucb1x00_enable() and ucb1x00_adc_enable() from a probe/initialisation function and then leave it alone. Unless you want to needlessly waste power. -- Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: 2.6 Serial core ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [rfc] separate sharpsl_pm initialization from sysfs code 2006-03-13 0:02 ` Russell King @ 2006-03-22 9:37 ` Pavel Machek 0 siblings, 0 replies; 5+ messages in thread From: Pavel Machek @ 2006-03-22 9:37 UTC (permalink / raw) To: Richard Purdie, kernel list, lenz Hi! > > /* Register interrupt handler. */ > > if ((err = request_irq(COLLIE_IRQ_GPIO_AC_IN, sharpsl_ac_isr, SA_INTERRUPT, > > "ACIN", sharpsl_ac_isr))) { > > printk("Could not get irq %d.\n", COLLIE_IRQ_GPIO_AC_IN); > > return; > > } > > if ((err = request_irq(COLLIE_IRQ_GPIO_CO, sharpsl_chrg_full_isr, SA_INTERRUPT, > > "CO", sharpsl_chrg_full_isr))) { > > free_irq(COLLIE_IRQ_GPIO_AC_IN, sharpsl_ac_isr); > > printk("Could not get irq %d.\n", COLLIE_IRQ_GPIO_CO); > > return; > > } > > Shouldn't these be ucb1x00_hook_irq()'s? > > Shouldn't you only enable the ADC when you need to use it? Yes, I should. > This driver makes no calls to ucb1x00_enable() and ucb1x00_disable(). > They're part of power management, and if your driver doesn't appear to > require them to work, that means some other code is buggy (or you're > keeping the ADC always enabled.) Keeping ADC always enabled, which interferes with touchscreen, badly. This fixes it, and allows touchscreen and battery to coexist. (I'll clean it up and properly submit). Pavel Enable/disable adc as needed. Now battery and touchscreen code seems to cooperate. --- commit 4907317f98c84569eb6c3d9c24b5b177fe60981b tree 2998b0eebdd138b56740575d98f02e1cecaf4c34 parent e89a9fd829a3cd639f93ec0291251f6a99916226 author <pavel@amd.ucw.cz> Wed, 22 Mar 2006 10:22:10 +0100 committer <pavel@amd.ucw.cz> Wed, 22 Mar 2006 10:22:10 +0100 arch/arm/mach-sa1100/collie_pm.c | 8 +++++++- drivers/mfd/ucb1x00-ts.c | 4 +++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/arch/arm/mach-sa1100/collie_pm.c b/arch/arm/mach-sa1100/collie_pm.c index 1618ade..6a9f833 100644 --- a/arch/arm/mach-sa1100/collie_pm.c +++ b/arch/arm/mach-sa1100/collie_pm.c @@ -70,7 +70,6 @@ static void collie_charger_init(void) ucb1x00_enable_irq(ucb, COLLIE_GPIO_AC_IN, UCB_RISING); ucb1x00_enable_irq(ucb, COLLIE_GPIO_CO, UCB_RISING); - ucb1x00_adc_enable(ucb); ucb1x00_io_set_dir(ucb, 0, COLLIE_TC35143_GPIO_MBAT_ON | COLLIE_TC35143_GPIO_TMP_ON | COLLIE_TC35143_GPIO_BBAT_ON); return; @@ -139,11 +138,14 @@ int collie_read_backup_battery(void) { int voltage; + ucb1x00_adc_enable(ucb); + /* Gives 75..130 */ ucb1x00_io_write(ucb, COLLIE_TC35143_GPIO_BBAT_ON, 0); voltage = ucb1x00_adc_read(ucb, UCB_ADC_INP_AD1, UCB_SYNC); ucb1x00_io_write(ucb, 0, COLLIE_TC35143_GPIO_BBAT_ON); + ucb1x00_adc_disable(ucb); printk("Backup battery = %d(%d)\n", ADCtoPower(voltage), voltage); @@ -158,6 +160,7 @@ int collie_read_main_battery(void) collie_read_temp(); collie_read_backup_battery(); #endif + ucb1x00_adc_enable(ucb); ucb1x00_io_write(ucb, 0, COLLIE_TC35143_GPIO_BBAT_ON); ucb1x00_io_write(ucb, COLLIE_TC35143_GPIO_MBAT_ON, 0); voltage = ucb1x00_adc_read(ucb, UCB_ADC_INP_AD1, UCB_SYNC); @@ -170,6 +173,7 @@ int collie_read_main_battery(void) On battery, it goes as low as 80. */ + ucb1x00_adc_disable(ucb); voltage_rev = voltage + ((ad_revise * voltage) / 652); voltage_volts = ADCtoPower(voltage_rev); @@ -190,9 +194,11 @@ int collie_read_temp(void) /* temp must be > 973, main battery must be < 465 */ /* FIXME sharpsl_pm.c has both conditions negated? */ + ucb1x00_adc_enable(ucb); ucb1x00_io_write(ucb, COLLIE_TC35143_GPIO_TMP_ON, 0); voltage = ucb1x00_adc_read(ucb, UCB_ADC_INP_AD0, UCB_SYNC); ucb1x00_io_write(ucb, 0, COLLIE_TC35143_GPIO_TMP_ON); + ucb1x00_adc_disable(ucb); /* >1010 = battery removed. 460 = 22C ? -- Picture of sleeping (Linux) penguin wanted... ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2006-03-22 9:38 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2006-03-09 12:42 [rfc] separate sharpsl_pm initialization from sysfs code Pavel Machek 2006-03-09 13:33 ` Richard Purdie 2006-03-10 18:07 ` Pavel Machek 2006-03-13 0:02 ` Russell King 2006-03-22 9:37 ` Pavel Machek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox