Skip to contentSkip to conversation

power: add poodle battery driver

PATCH New Dmitry Eremin-Solenikov 2 participants 5 messages Mar 30, 2015 – Apr 11, 2015

lore
Share & tools
lore

Dmitry Eremin-Solenikov

author
#0 lore raw

PATCH 1/1 power: add poodle battery driver

Add a driver supporting battery charging on Sharp SL-5600 (poodle). Voltage and temperature readings are provided through add7846 hwmon interface. Battery voltage is in1_input (mV) and temp in in0_input (values unknown, but should be less than 2441).

Signed-off-by Dmitry Eremin-Solenikov <[email protected]>
---
 drivers/power/Kconfig          |   7 ++
 drivers/power/Makefile         |   1 +
 drivers/power/poodle_battery.c | 250 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 258 insertions(+)
 create mode 100644 drivers/power/poodle_battery.c
drivers/power/Kconfig +7 -0
index 27b751b..678f23e 100644
@@ -137,6 +137,13 @@ config BATTERY_COLLIE
 	  Say Y to enable support for the battery on the Sharp Zaurus
 	  SL-5500 (collie) models.
 
+config BATTERY_POODLE
+	tristate "Sharp SL-5600 (poodle) battery"
+	depends on MACH_POODLE
+	help
+	  Say Y to enable support for the battery on the Sharp Zaurus
+	  SL-5600 (poodle) models.
+
 config BATTERY_IPAQ_MICRO
 	tristate "iPAQ Atmel Micro ASIC battery driver"
 	depends on MFD_IPAQ_MICRO
drivers/power/Makefile +1 -0
index 36f9e0d..ff3e793 100644
@@ -26,6 +26,7 @@ obj-$(CONFIG_BATTERY_PMU)	+= pmu_battery.o
 obj-$(CONFIG_BATTERY_OLPC)	+= olpc_battery.o
 obj-$(CONFIG_BATTERY_TOSA)	+= tosa_battery.o
 obj-$(CONFIG_BATTERY_COLLIE)	+= collie_battery.o
+obj-$(CONFIG_BATTERY_POODLE)	+= poodle_battery.o
 obj-$(CONFIG_BATTERY_IPAQ_MICRO) += ipaq_micro_battery.o
 obj-$(CONFIG_BATTERY_WM97XX)	+= wm97xx_battery.o
 obj-$(CONFIG_BATTERY_SBS)	+= sbs-battery.o
drivers/power/poodle_battery.c new file +250 -0

250 lines not on this page · raw message

Signature · 1 line
2.1.4
#1 lore raw

PATCH 1/1 power: add poodle battery driver

Hi Dmitry,

Quoted 4 lines · Dmitry Eremin-Solenikov

Add a driver supporting battery charging on Sharp SL-5600 (poodle). Voltage and temperature readings are provided through add7846 hwmon interface. Battery voltage is in1_input (mV) and temp in in0_input (values unknown, but should be less than 2441).

You may want to convert add7846 to iio interface, which can be exposed via hwmon and requested by other kernel drivers (e.g. this one).

drivers/power/poodle_battery.c line 27

Quoted 9 lines

[...]

drivers/power/poodle_battery.c new file +8 -0
+struct poodle_bat {
+	int status;
+	struct power_supply psy;
+
+	struct mutex work_lock; /* protects data */
+
+	bool (*is_present)(struct poodle_bat *bat);
+	int technology;

why do you need this for a static type? You can simply set the type directly in the get_property function?

drivers/power/poodle_battery.c line 157

Quoted 13 lines
drivers/power/poodle_battery.c new file +10 -0
+};
 
[...]
 
+static struct gpio poodle_batt_gpios[] = {
+	{ POODLE_GPIO_BAT_COVER,    GPIOF_IN,		"main battery cover" },
+	{ POODLE_GPIO_CHRG_FULL,    GPIOF_IN,		"main battery full" },
+	{ POODLE_GPIO_JK_B,	    GPIOF_OUT_INIT_LOW,	"main charge on" },
+	{ POODLE_GPIO_CHRG_ON,	    GPIOF_OUT_INIT_LOW,	"main charge on" },
+	{ POODLE_GPIO_BYPASS_ON,    GPIOF_OUT_INIT_LOW,	"main charge bypass" },
+	/* on for now */
+	{ POODLE_GPIO_ADC_TEMP_ON,  GPIOF_OUT_INIT_HIGH, "main battery temp" },
+};

I would prefer if you use the "new" gpiod interface, see include/linux/gpio/consumer.h

This obviously refers to the whole driver.

Quoted 8 lines

[...]

drivers/power/poodle_battery.c new file +6 -0
+static int poodle_battery_probe(struct platform_device *dev)
+{
+	int ret;
+
+	ret = gpio_request_array(poodle_batt_gpios,
+				 ARRAY_SIZE(poodle_batt_gpios));

drivers/power/poodle_battery.c line 199

Quoted 10 lines
drivers/power/poodle_battery.c new file +10 -0
+	if (ret)
+		return ret;
+
+	mutex_init(&poodle_bat_main.work_lock);
+
+	INIT_WORK(&bat_work, poodle_bat_work);
+
+	ret = power_supply_register(&dev->dev, &poodle_bat_main.psy);
+	if (ret)
+		goto err_psy_reg_main;

we have devm_power_supply_register() now.

Quoted 17 lines
drivers/power/poodle_battery.c new file +17 -0
+	ret = devm_request_irq(&dev->dev, gpio_to_irq(POODLE_GPIO_CHRG_FULL),
+				poodle_bat_gpio_isr,
+				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+				"main full", NULL);
+	if (ret)
+		goto err_psy_irq;
+
+	ret = devm_request_irq(&dev->dev, gpio_to_irq(POODLE_GPIO_BAT_COVER),
+				poodle_bat_gpio_isr,
+				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+				"battery cover", NULL);
+	if (ret)
+		goto err_psy_irq;
+
+	return 0;
+
+err_psy_irq:

drivers/power/poodle_battery.c

Quoted 10 lines
drivers/power/poodle_battery.c new file +8 -0
+	power_supply_unregister(&poodle_bat_main.psy);
+
+	cancel_work_sync(&bat_work);
+err_psy_reg_main:
+	gpio_free_array(poodle_batt_gpios, ARRAY_SIZE(poodle_batt_gpios));
+
+	return ret;
+}
 
[...]

-- Sebastian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150406/9d42b504/attachment.sig>

author
#2 lore raw

PATCH 1/1 power: add poodle battery driver

Hello,

2015-04-06 19:44 GMT+03:00, Sebastian Reichel <[email protected]>:

Quoted 11 lines

Hi Dmitry,

Quoted 4 lines · Dmitry Eremin-Solenikov

Add a driver supporting battery charging on Sharp SL-5600 (poodle). Voltage and temperature readings are provided through add7846 hwmon interface. Battery voltage is in1_input (mV) and temp in in0_input (values unknown, but should be less than 2441).

You may want to convert add7846 to iio interface, which can be exposed via hwmon and requested by other kernel drivers (e.g. this one).

This needs a more careful thought. I have several devices in mind, which provide touchscreen + ADC interfaces, where ADC is used for battery voltage and temperature sensing. Sometimes additional GPIOs are in play. Thus I'd like for now to sustain from changing ads7846 driver. Or, at least to delay such changes, iff there will be no real design/ideas how to handle such devices in a generic enough way.

Quoted 13 lines

drivers/power/poodle_battery.c line 27

Quoted 10 lines

[...]

drivers/power/poodle_battery.c new file +8 -0
+struct poodle_bat {
+ int status;
+ struct power_supply psy;
+
+ struct mutex work_lock; /* protects data */
+
+ bool (*is_present)(struct poodle_bat *bat);
+ int technology;

why do you need this for a static type? You can simply set the type directly in the get_property function?

ack

Quoted 32 lines
Quoted 4 lines
drivers/power/poodle_battery.c new file +1 -0
+};
 
[...]

drivers/power/poodle_battery.c line 157

Quoted 10 lines
drivers/power/poodle_battery.c new file +9 -0
 
+static struct gpio poodle_batt_gpios[] = {
+ { POODLE_GPIO_BAT_COVER,    GPIOF_IN, "main battery cover" },
+ { POODLE_GPIO_CHRG_FULL,    GPIOF_IN, "main battery full" },
+ { POODLE_GPIO_JK_B,    GPIOF_OUT_INIT_LOW, "main charge on" },
+ { POODLE_GPIO_CHRG_ON,    GPIOF_OUT_INIT_LOW, "main charge on" },
+ { POODLE_GPIO_BYPASS_ON,    GPIOF_OUT_INIT_LOW, "main charge bypass" },
+ /* on for now */
+ { POODLE_GPIO_ADC_TEMP_ON,  GPIOF_OUT_INIT_HIGH, "main battery temp" },
+};

I would prefer if you use the "new" gpiod interface, see include/linux/gpio/consumer.h

This obviously refers to the whole driver.

Quoted 12 lines

[...]

drivers/power/poodle_battery.c new file +10 -0
+static int poodle_battery_probe(struct platform_device *dev)
+{
+ int ret;
+
+ ret = gpio_request_array(poodle_batt_gpios,
+ ARRAY_SIZE(poodle_batt_gpios));
+ if (ret)
+ return ret;
+
+ mutex_init(&poodle_bat_main.work_lock);
Quoted 8 lines

drivers/power/poodle_battery.c line 199

Quoted 6 lines
drivers/power/poodle_battery.c new file +6 -0
+
+ INIT_WORK(&bat_work, poodle_bat_work);
+
+ ret = power_supply_register(&dev->dev, &poodle_bat_main.psy);
+ if (ret)
+ goto err_psy_reg_main;

we have devm_power_supply_register() now.

Not so sure about it. I'd like to cancel work after unregistering power_supply. I think I'll even switch from devm_request_irq back to request_irq to be able to cancel IRQs before canceling work.

Quoted 30 lines
Quoted 17 lines

+ ret = devm_request_irq(&dev->dev, gpio_to_irq(POODLE_GPIO_CHRG_FULL),
+ poodle_bat_gpio_isr,
+ IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+ "main full", NULL);
+ if (ret)
+ goto err_psy_irq;

drivers/power/poodle_battery.c new file +11 -0
+
+ ret = devm_request_irq(&dev->dev, gpio_to_irq(POODLE_GPIO_BAT_COVER),
+ poodle_bat_gpio_isr,
+ IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+ "battery cover", NULL);
+ if (ret)
+ goto err_psy_irq;
+
+ return 0;
+
+err_psy_irq:

drivers/power/poodle_battery.c

Quoted 10 lines
drivers/power/poodle_battery.c new file +8 -0
+ power_supply_unregister(&poodle_bat_main.psy);
+
+ cancel_work_sync(&bat_work);
+err_psy_reg_main:
+ gpio_free_array(poodle_batt_gpios, ARRAY_SIZE(poodle_batt_gpios));
+
+ return ret;
+}
 
[...]

-- Sebastian

Signature · 2 lines
With best wishes
Dmitry
#3 lore raw

PATCH 1/1 power: add poodle battery driver

Hi,

Quoted 10 lines · Dmitry Eremin-Solenikov
Quoted 9 lines · Dmitry Eremin-Solenikov
Quoted 4 lines

Add a driver supporting battery charging on Sharp SL-5600 (poodle). Voltage and temperature readings are provided through add7846 hwmon interface. Battery voltage is in1_input (mV) and temp in in0_input (values unknown, but should be less than 2441).

You may want to convert add7846 to iio interface, which can be exposed via hwmon and requested by other kernel drivers (e.g. this one).

Quoted 7 lines

This needs a more careful thought. I have several devices in mind, which provide touchscreen + ADC interfaces, where ADC is used for battery voltage and temperature sensing. Sometimes additional GPIOs are in play. Thus I'd like for now to sustain from changing ads7846 driver. Or, at least to delay such changes, iff there will be no real design/ideas how to handle such devices in a generic enough way.

If you have more of these devices it's even more appealing to use the IIO interface. You can basically keep the touchscreen part of the ads7846 driver and replace hwmon with IIO. Then other kernel drivers can request an IIO channel, but you are still able to use hwmon interface from userspace via iio-hwmon.

Quoted 21 lines

[...]

Quoted 10 lines
Quoted 10 lines
drivers/power/poodle_battery.c new file +10 -0
+static int poodle_battery_probe(struct platform_device *dev)
+{
+ int ret;
+
+ ret = gpio_request_array(poodle_batt_gpios,
+ ARRAY_SIZE(poodle_batt_gpios));
+ if (ret)
+ return ret;
+
+ mutex_init(&poodle_bat_main.work_lock);
Quoted 8 lines

drivers/power/poodle_battery.c line 199

Quoted 6 lines
drivers/power/poodle_battery.c new file +6 -0
+
+ INIT_WORK(&bat_work, poodle_bat_work);
+
+ ret = power_supply_register(&dev->dev, &poodle_bat_main.psy);
+ if (ret)
+ goto err_psy_reg_main;

we have devm_power_supply_register() now.

Quoted 3 lines

Not so sure about it. I'd like to cancel work after unregistering power_supply. I think I'll even switch from devm_request_irq back to request_irq to be able to cancel IRQs before canceling work.

Why?

-- Sebastian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150407/fa506729/attachment.sig>

author
#4 lore raw

PATCH 1/1 power: add poodle battery driver

Hello,

2015-04-07 18:40 GMT+03:00 Sebastian Reichel <[email protected]>:

Quoted 21 lines

Hi,

Quoted 10 lines · Dmitry Eremin-Solenikov
Quoted 9 lines · Dmitry Eremin-Solenikov
Quoted 4 lines

Add a driver supporting battery charging on Sharp SL-5600 (poodle). Voltage and temperature readings are provided through add7846 hwmon interface. Battery voltage is in1_input (mV) and temp in in0_input (values unknown, but should be less than 2441).

You may want to convert add7846 to iio interface, which can be exposed via hwmon and requested by other kernel drivers (e.g. this one).

Quoted 7 lines

This needs a more careful thought. I have several devices in mind, which provide touchscreen + ADC interfaces, where ADC is used for battery voltage and temperature sensing. Sometimes additional GPIOs are in play. Thus I'd like for now to sustain from changing ads7846 driver. Or, at least to delay such changes, iff there will be no real design/ideas how to handle such devices in a generic enough way.

Quoted 5 lines

If you have more of these devices it's even more appealing to use the IIO interface. You can basically keep the touchscreen part of the ads7846 driver and replace hwmon with IIO. Then other kernel drivers can request an IIO channel, but you are still able to use hwmon interface from userspace via iio-hwmon.

There are more chips like this, not all of them are ads7846. I'll think about your suggestion. However I do not see this as a first priority task.

Quoted 22 lines
Quoted 22 lines

[...]

Quoted 18 lines
Quoted 6 lines
drivers/power/poodle_battery.c new file +6 -0
+static int poodle_battery_probe(struct platform_device *dev)
+{
+ int ret;
+
+ ret = gpio_request_array(poodle_batt_gpios,
+ ARRAY_SIZE(poodle_batt_gpios));

drivers/power/poodle_battery.c line 199

Quoted 10 lines
drivers/power/poodle_battery.c new file +10 -0
+ if (ret)
+ return ret;
+
+ mutex_init(&poodle_bat_main.work_lock);
+
+ INIT_WORK(&bat_work, poodle_bat_work);
+
+ ret = power_supply_register(&dev->dev, &poodle_bat_main.psy);
+ if (ret)
+ goto err_psy_reg_main;

we have devm_power_supply_register() now.

Quoted 3 lines
Quoted 3 lines

Not so sure about it. I'd like to cancel work after unregistering power_supply. I think I'll even switch from devm_request_irq back to request_irq to be able to cancel IRQs before canceling work.

To be sure that there are no pending work items, after canceling the work in driver remove callback. Basically I'd like to be sure of the following sequence:

- shut down IRQs, so that no external events can schedule work
- unregister psy so replugging AC can not schedule work
- cancel all pending work
- free memory

Would you have any suggestions other than doing first two unregistrations by hand?

Signature · 2 lines
With best wishes
Dmitry