On Thu, 2006-03-09 at 13:38 +0100, Pavel Machek wrote:
> Hi!
>
> This is collie battery sensing code. It differs from sharpsl code a
> bit -- because it is dependend on ucb1x00, not on platform bus.
>
> I guess I should reorganize #include's and remove #if 0-ed
> code. Anything else
Basically looks good. Could probably use a
s/printk/dev_dbg(sharpsl_pm.dev, /. I've made a few other comments
below.
Just for my interest, can you summarise the status of PM and charging on
collie with this code?
> diff --git a/arch/arm/mach-sa1100/collie_pm.c b/arch/arm/mach-sa1100/collie_pm.c
> new file mode 100644
> index 0000000..491d03a
> --- /dev/null
> +++ b/arch/arm/mach-sa1100/collie_pm.c
> @@ -0,0 +1,324 @@
> +/*
> + * Based on spitz_pm.c and sharp code.
> + *
> + * Copyright (C) 2001 SHARP
> + * Copyright 2005 Pavel Machek <[email protected]>
> + *
> + * 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 <asm/apm.h>
> +#include <asm/irq.h>
> +#include <asm/mach-types.h>
> +#include <asm/hardware.h>
> +#include <asm/hardware/scoop.h>
> +#include <asm/dma.h>
> +#include <linux/platform_device.h>
> +
> +#include <asm/arch/collie.h>
> +#include "../mach-pxa/sharpsl.h"
Do you need anything in the above header? If so, it should probably be
in asm/hardware/sharpsl_pm.h
> +#include "../drivers/mfd/ucb1x00.h"
> +#include <asm/mach/sharpsl_param.h>
> +
> +#ifdef CONFIG_MCP_UCB1200_TS
> +#error Battery interferes with touchscreen
> +#endif
Is this a case of bad locking or something more serious?
> +static struct ucb1x00 *ucb;
> +
> +#define ConvRevise(x) ( ( ad_revise * x ) / 652 )
> +#define ADCtoPower(x) (( 330 * x * 2 ) / 1024 )
> +
> +static int ad_revise = 0;
The = 0 is unneeded.
> +static void collie_charger_init(void)
> +{
> + int err;
> +
> + /* get ad revise */
> + if (sharpsl_param.adadj != -1) {
> + ad_revise = sharpsl_param.adadj;
> + } else {
> + ad_revise = 0;
> + }
ad_revise is already 0...
> +
> + /* 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;
> + }
> +
> + /* Register interrupt handler. */
> + 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 (!ucb)
> + printk(KERN_CRIT "ucb is null!\n");
Should it return here? Given this, We might need to think about error
handling in the init function.
> + /* 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_charge_led(int val)
> +{
> + if (val == SHARPSL_LED_ERROR) {
> + dev_dbg(sharpsl_pm.dev, "Charge LED Error\n");
> + } else if (val == SHARPSL_LED_ON) {
> + dev_dbg(sharpsl_pm.dev, "Charge LED On\n");
> + } else {
> + dev_dbg(sharpsl_pm.dev, "Charge LED Off\n");
> + }
> +}
For reference, we can move this into the core if the LED subsystem is
accepted into mainline as we'll just need a platform independent
trigger.
> +static void collie_charge(int on)
> +{
> + if (on) {
> + printk("Should start charger\n");
> + } else {
> + printk("Should stop charger\n");
> + }
This can be removed before mainline?
> +#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) {
> + SCOOP_REG_GPWR |= COLLIE_SCP_CHARGE_ON;
> + } else {
> + SCOOP_REG_GPWR &= ~COLLIE_SCP_CHARGE_ON;
Ick. Use arch/arm/common/scoop.c to do this. Something like:
#include <asm/hardware/scoop.h>
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;
> +}
> +
> +static int collie_acin_status(void)
> +{
> + int ret = GPLR & COLLIE_GPIO_AC_IN;
> + printk("AC status = %d\n", ret);
> + return ret;
> +}
> +
> +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;
> +
> + collie_read_temp();
> + collie_read_backup_battery();
> + 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.
> + */
> +
> + printk("Main battery = %d(%d)\n",ADCtoPower((voltage+ConvRevise(voltage))),voltage);
> +
> + if ( voltage != -1 ) {
> + return ADCtoPower((voltage+ConvRevise(voltage)));
> + } 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 int collie_read_acin(void)
> +{
> + return 0x1;
> +}
> +
> +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 collie_read_acin();
> + case SHARPSL_STATUS_ACIN:
> + case SHARPSL_STATUS_LOCK:
> + case SHARPSL_STATUS_CHRGFULL:
> + case SHARPSL_STATUS_FATAL:
> + default:
> + return ~0;
> + }
> +#if 0
> +It should be okay. Nobody really needs those.
> + .gpio_batlock = COLLIE_GPIO_BAT_COVER,
> + .gpio_acin = COLLIE_GPIO_AC_IN,
> + .gpio_batfull = COLLIE_GPIO_CHRG_FULL,
> + .gpio_fatal = COLLIE_GPIO_FATAL_BAT,
> +#endif
> +}
> +
> +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,
> +};
> +
> +extern struct sharpsl_pm_status sharpsl_pm;
> +
> +static int __init collie_pm_add(struct ucb1x00_dev *pdev)
> +{
> + sharpsl_pm.dev = NULL;
> + sharpsl_pm.machinfo = &collie_pm_machinfo;
> + ucb = pdev->ucb;
> + return sharpsl_pm_init();
> +}
I don't understand how this is supposed to work at all. For a start,
sharpsl_pm.c says "static int __devinit sharpsl_pm_init(void)" so that
function isn't available. I've just noticed your further patches
although I still don't like this.
The correct approach is to register a platform device called
"sharpsl-pm" in collie_pm_add() which the driver will then see and
attach to. I'd also not register the platform device if ucb is NULL for
whatever reason.
By setting sharpsl_pm.dev = NULL you're also going to miss out on
suspend/resume calls and risk breaking things like
dev_dbg(sharpsl_pm.dev, xxx).
Richard
> +static int collie_pm_remove(struct ucb1x00_dev *pdev)
> +{
> + return sharpsl_pm_done();
> +}
> +
> +static struct ucb1x00_driver collie_pm_driver = {
> + .add = collie_pm_add,
> + .remove = collie_pm_remove,
> +};
> +
> +static int __init collie_pm_init(void)
> +{
> + return ucb1x00_register_driver(&collie_pm_driver);
> +}
> +
> +static void __exit collie_pm_exit(void)
> +{
> + ucb1x00_unregister_driver(&collie_pm_driver);
> +}
> +
> +late_initcall(collie_pm_init);
> +module_exit(collie_pm_exit);
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
