From mboxrd@z Thu Jan 1 00:00:00 1970 From: Krzysztof Kozlowski Subject: Re: [PATCH v3 1/2] power: act8945a: add charger driver for ACT8945A Date: Wed, 13 Jan 2016 13:09:46 +0900 Message-ID: <5695CE0A.5010705@samsung.com> References: <1452586185-818-1-git-send-email-wenyou.yang@atmel.com> <1452586185-818-2-git-send-email-wenyou.yang@atmel.com> <5695B252.4060606@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-reply-to: Sender: linux-kernel-owner@vger.kernel.org To: "Yang, Wenyou" , Sebastian Reichel , Dmitry Eremin-Solenikov , David Woodhouse , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala Cc: Javier Martinez Canillas , Lee Jones , "Ferre, Nicolas" , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , "linux-pm@vger.kernel.org" List-Id: linux-pm@vger.kernel.org On 13.01.2016 12:53, Yang, Wenyou wrote: > Hi Krzysztof, >=20 > Thank you! >=20 >> -----Original Message----- >> From: Krzysztof Kozlowski [mailto:k.kozlowski@samsung.com] >> Sent: 2016=E5=B9=B41=E6=9C=8813=E6=97=A5 10:12 >> To: Yang, Wenyou ; Sebastian Reichel >> ; Dmitry Eremin-Solenikov ; Da= vid >> Woodhouse ; Rob Herring ; P= awel >> Moll ; Mark Rutland ; Ian >> Campbell ; Kumar Gala >> Cc: Javier Martinez Canillas ; Lee Jones >> ; Ferre, Nicolas ; li= nux-arm- >> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux- >> pm@vger.kernel.org >> Subject: Re: [PATCH v3 1/2] power: act8945a: add charger driver for = ACT8945A >> >> On 12.01.2016 17:09, Wenyou Yang wrote: >>> This patch adds new driver for Active-semi ACT8945A ActivePath char= ger >>> (part of ACT8945A MFD driver) providing power supply class informat= ion >>> to userspace. >>> >>> The driver is configured through DTS (battery and system related >>> settings) and sysfs entries (timers and input over-voltage threshol= d). >>> >>> Signed-off-by: Wenyou Yang >>> --- >>> >>> Changes in v3: >>> - update the file header with short version license and author lin= e. >>> - remove unused member of struct act8945a_charger, dev. >>> - action due to removing the member of stuct act8945a_dev, dev. >>> - remove the unnecessary print out. >>> - remove the unnecessary act8945a_charger_remove(). >>> - fix align of the code-style. >>> >>> Changes in v2: >>> 1./ Substitute of_property_read_bool() for of_get_property(). >>> 2./ Substitute devm_power_supply_register() for power_supply_regis= ter(). >>> 3./ Use module_platform_driver(), instead of subsys_initcall(). >>> 4./ Substitute MODULE_LICENSE("GPL") for MODULE_LICENSE("GPL v2"). >>> >>> drivers/power/Kconfig | 7 + >>> drivers/power/Makefile | 1 + >>> drivers/power/act8945a_charger.c | 367 >>> ++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 375 insertions(+) >>> create mode 100644 drivers/power/act8945a_charger.c >>> >>> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig index >>> 1ddd13c..ae75211 100644 >>> --- a/drivers/power/Kconfig >>> +++ b/drivers/power/Kconfig >>> @@ -75,6 +75,13 @@ config BATTERY_88PM860X >>> help >>> Say Y here to enable battery monitor for Marvell 88PM860x chip. >>> >>> +config BATTERY_ACT8945A >>> + tristate "Active-semi ACT8945A charger driver" >>> + depends on MFD_ACT8945A >>> + help >>> + Say Y here to enable support for power supply provided by >>> + Active-semi ActivePath ACT8945A charger. >>> + >>> config BATTERY_DS2760 >>> tristate "DS2760 battery driver (HP iPAQ & others)" >>> depends on W1 && W1_SLAVE_DS2760 >>> diff --git a/drivers/power/Makefile b/drivers/power/Makefile index >>> 0e4eab5..e46b75d 100644 >>> --- a/drivers/power/Makefile >>> +++ b/drivers/power/Makefile >>> @@ -17,6 +17,7 @@ obj-$(CONFIG_WM8350_POWER) +=3D >> wm8350_power.o >>> obj-$(CONFIG_TEST_POWER) +=3D test_power.o >>> >>> obj-$(CONFIG_BATTERY_88PM860X) +=3D 88pm860x_battery.o >>> +obj-$(CONFIG_BATTERY_ACT8945A) +=3D act8945a_charger.o >>> obj-$(CONFIG_BATTERY_DS2760) +=3D ds2760_battery.o >>> obj-$(CONFIG_BATTERY_DS2780) +=3D ds2780_battery.o >>> obj-$(CONFIG_BATTERY_DS2781) +=3D ds2781_battery.o >>> diff --git a/drivers/power/act8945a_charger.c >>> b/drivers/power/act8945a_charger.c >>> new file mode 100644 >>> index 0000000..643a2ed >>> --- /dev/null >>> +++ b/drivers/power/act8945a_charger.c >>> @@ -0,0 +1,367 @@ >>> +/* >>> + * Power supply driver for the Active-semi ACT8945A PMIC >>> + * >>> + * Copyright (C) 2015 Atmel Corporation >>> + * >>> + * Author: Wenyou Yang >>> + * >>> + * This program is free software; you can redistribute it and/or >>> +modify >>> + * it under the terms of the GNU General Public License version 2 = as >>> + * published by the Free Software Foundation. >>> + * >>> + */ >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +static const char *act8945a_charger_model =3D "ACT8945A"; static c= onst >>> +char *act8945a_charger_manufacturer =3D "Active-semi"; >>> + >>> +/** >>> + * ACT8945A Charger Register Map >>> + */ >>> + >>> +/* 0x70: Reserved */ >>> +#define ACT8945A_APCH_CFG 0x71 >>> +#define ACT8945A_APCH_STATUS 0x78 >>> +#define ACT8945A_APCH_CTRL 0x79 >>> +#define ACT8945A_APCH_STATE 0x7A >>> + >>> +/* ACT8945A_APCH_CFG */ >>> +#define APCH_CFG_OVPSET (0x03 << 0) >>> +#define APCH_CFG_OVPSET_6V6 (0x0 << 0) >>> +#define APCH_CFG_OVPSET_7V (0x1 << 0) >>> +#define APCH_CFG_OVPSET_7V5 (0x2 << 0) >>> +#define APCH_CFG_OVPSET_8V (0x3 << 0) >> >> Why the tabs after #define? Nooooo... >> >>> +#define APCH_CFG_PRETIMO (0x03 << 2) >>> +#define APCH_CFG_PRETIMO_40_MIN (0x0 << 2) >>> +#define APCH_CFG_PRETIMO_60_MIN (0x1 << 2) >>> +#define APCH_CFG_PRETIMO_80_MIN (0x2 << 2) >>> +#define APCH_CFG_PRETIMO_DISABLED (0x3 << 2) >>> +#define APCH_CFG_TOTTIMO (0x03 << 4) >>> +#define APCH_CFG_TOTTIMO_3_HOUR (0x0 << 4) >>> +#define APCH_CFG_TOTTIMO_4_HOUR (0x1 << 4) >>> +#define APCH_CFG_TOTTIMO_5_HOUR (0x2 << 4) >>> +#define APCH_CFG_TOTTIMO_DISABLED (0x3 << 4) >>> +#define APCH_CFG_SUSCHG (0x01 << 7) >>> + >>> +#define APCH_STATUS_CHGDAT (0x01 << 0) >>> +#define APCH_STATUS_INDAT (0x01 << 1) >>> +#define APCH_STATUS_TEMPDAT (0x01 << 2) >>> +#define APCH_STATUS_TIMRDAT (0x01 << 3) >>> +#define APCH_STATUS_CHGSTAT (0x01 << 4) >>> +#define APCH_STATUS_INSTAT (0x01 << 5) >>> +#define APCH_STATUS_TEMPSTAT (0x01 << 6) >>> +#define APCH_STATUS_TIMRSTAT (0x01 << 7) >>> + >>> +#define APCH_CTRL_CHGEOCOUT (0x01 << 0) >>> +#define APCH_CTRL_INDIS (0x01 << 1) >>> +#define APCH_CTRL_TEMPOUT (0x01 << 2) >>> +#define APCH_CTRL_TIMRPRE (0x01 << 3) >>> +#define APCH_CTRL_CHGEOCIN (0x01 << 4) >>> +#define APCH_CTRL_INCON (0x01 << 5) >>> +#define APCH_CTRL_TEMPIN (0x01 << 6) >>> +#define APCH_CTRL_TIMRTOT (0x01 << 7) >> >> For the STATUS and CTRL use BIT(n) macro. >> >> >>> + >>> +#define APCH_STATE_ACINSTAT (0x01 << 1) >>> +#define APCH_STATE_CSTATE (0x03 << 4) >>> +#define APCH_STATE_CSTATE_SHIFT 4 >>> +#define APCH_STATE_CSTATE_DISABLED 0x00 >>> +#define APCH_STATE_CSTATE_EOC 0x01 >>> +#define APCH_STATE_CSTATE_FAST 0x02 >>> +#define APCH_STATE_CSTATE_PRE 0x03 >>> + >>> +struct act8945a_charger { >>> + struct act8945a_dev *act8945a_dev; >>> + struct power_supply *psy; >>> + >>> + u32 tolal_time_out; >>> + u32 pre_time_out; >>> + u32 input_voltage_threshold; >>> + bool battery_temperature; >>> + int chglev_pin; >>> + int chglev_value; >>> +}; >>> + >>> +static int act8945a_get_charger_state(struct regmap *regmap, int >>> +*val) { >>> + int ret; >>> + unsigned int status, state; >>> + >>> + ret =3D regmap_read(regmap, ACT8945A_APCH_STATUS, &status); >>> + if (ret < 0) >>> + return ret; >>> + >>> + ret =3D regmap_read(regmap, ACT8945A_APCH_STATE, &state); >>> + if (ret < 0) >>> + return ret; >>> + >>> + state &=3D APCH_STATE_CSTATE; >>> + state >>=3D APCH_STATE_CSTATE_SHIFT; >>> + >>> + if (state =3D=3D APCH_STATE_CSTATE_EOC) { >>> + if (status & APCH_STATUS_CHGDAT) >>> + *val =3D POWER_SUPPLY_STATUS_FULL; >>> + else >>> + *val =3D POWER_SUPPLY_STATUS_NOT_CHARGING; >>> + } else if ((state =3D=3D APCH_STATE_CSTATE_FAST) || >>> + (state =3D=3D APCH_STATE_CSTATE_PRE)) { >>> + *val =3D POWER_SUPPLY_STATUS_CHARGING; >>> + } else { >>> + *val =3D POWER_SUPPLY_STATUS_NOT_CHARGING; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +static int act8945a_get_charge_type(struct regmap *regmap, int *va= l) >>> +{ >>> + int ret; >>> + unsigned int state; >>> + >>> + ret =3D regmap_read(regmap, ACT8945A_APCH_STATE, &state); >>> + if (ret < 0) >>> + return ret; >>> + >>> + state &=3D APCH_STATE_CSTATE; >>> + state >>=3D APCH_STATE_CSTATE_SHIFT; >>> + >>> + switch (state) { >>> + case APCH_STATE_CSTATE_PRE: >>> + *val =3D POWER_SUPPLY_CHARGE_TYPE_TRICKLE; >>> + break; >>> + case APCH_STATE_CSTATE_FAST: >>> + *val =3D POWER_SUPPLY_CHARGE_TYPE_FAST; >>> + break; >>> + case APCH_STATE_CSTATE_EOC: >>> + case APCH_STATE_CSTATE_DISABLED: >>> + default: >>> + *val =3D POWER_SUPPLY_CHARGE_TYPE_NONE; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +static int act8945a_get_battery_health(struct act8945a_charger *ch= arger, >>> + struct regmap *regmap, int *val) { >>> + int ret; >>> + unsigned int status; >>> + >>> + ret =3D regmap_read(regmap, ACT8945A_APCH_STATUS, &status); >>> + if (ret < 0) >>> + return ret; >>> + >>> + if (charger->battery_temperature && !(status & >> APCH_STATUS_TEMPDAT)) >>> + *val =3D POWER_SUPPLY_HEALTH_OVERHEAT; >>> + else if (!(status & APCH_STATUS_INDAT)) >>> + *val =3D POWER_SUPPLY_HEALTH_OVERVOLTAGE; >>> + else if (status & APCH_STATUS_TIMRDAT) >>> + *val =3D POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE; >>> + else >>> + *val =3D POWER_SUPPLY_HEALTH_GOOD; >>> + >>> + return 0; >>> +} >>> + >>> +static enum power_supply_property act8945a_charger_props[] =3D { >>> + POWER_SUPPLY_PROP_STATUS, >>> + POWER_SUPPLY_PROP_CHARGE_TYPE, >>> + POWER_SUPPLY_PROP_TECHNOLOGY, >>> + POWER_SUPPLY_PROP_HEALTH, >>> + POWER_SUPPLY_PROP_MODEL_NAME, >>> + POWER_SUPPLY_PROP_MANUFACTURER >>> +}; >>> + >>> +static int act8945a_charger_get_property(struct power_supply *psy, >>> + enum power_supply_property prop, >>> + union power_supply_propval *val) { >>> + struct act8945a_charger *charger =3D power_supply_get_drvdata(psy= ); >>> + struct regmap *regmap =3D charger->act8945a_dev->regmap; >>> + int ret =3D 0; >>> + >>> + switch (prop) { >>> + case POWER_SUPPLY_PROP_STATUS: >>> + ret =3D act8945a_get_charger_state(regmap, &val->intval); >>> + break; >>> + case POWER_SUPPLY_PROP_CHARGE_TYPE: >>> + ret =3D act8945a_get_charge_type(regmap, &val->intval); >>> + break; >>> + case POWER_SUPPLY_PROP_TECHNOLOGY: >>> + val->intval =3D POWER_SUPPLY_TECHNOLOGY_LION; >>> + break; >>> + case POWER_SUPPLY_PROP_HEALTH: >>> + ret =3D act8945a_get_battery_health(charger, >>> + regmap, &val->intval); >>> + break; >>> + case POWER_SUPPLY_PROP_MODEL_NAME: >>> + val->strval =3D act8945a_charger_model; >>> + break; >>> + case POWER_SUPPLY_PROP_MANUFACTURER: >>> + val->strval =3D act8945a_charger_manufacturer; >>> + break; >>> + default: >>> + return -EINVAL; >>> + } >>> + >>> + return ret; >>> +} >>> + >>> +static const struct power_supply_desc act8945a_charger_desc =3D { >>> + .name =3D "act8945a-charger", >>> + .type =3D POWER_SUPPLY_TYPE_BATTERY, >>> + .get_property =3D act8945a_charger_get_property, >>> + .properties =3D act8945a_charger_props, >>> + .num_properties =3D ARRAY_SIZE(act8945a_charger_props), >>> +}; >>> + >>> +#define DEFAULT_TOTAL_TIME_OUT 3 >>> +#define DEFAULT_PRE_TIME_OUT 40 >>> +#define DEFAULT_INPUT_OVP_THRESHOLD 6600 >>> + >>> +static int act8945a_charger_parse_dt(struct device *dev, >>> + struct act8945a_charger *charger) { >>> + struct device_node *np =3D dev->of_node; >>> + enum of_gpio_flags flags; >>> + >>> + if (!np) { >>> + dev_err(dev, "no charger of node\n"); >>> + return -EINVAL; >>> + } >>> + >>> + charger->chglev_pin =3D of_get_named_gpio_flags(np, >>> + "active-semi,chglev-gpio", 0, &flags); >>> + >>> + charger->chglev_value =3D (flags =3D=3D OF_GPIO_ACTIVE_LOW) ? 0 := 1; >>> + >>> + charger->battery_temperature =3D of_property_read_bool(np, >>> + "active-semi,battery_temperature"); >>> + >>> + if (of_property_read_u32(np, "active-semi,input_voltage_threshold= ", >>> + &charger->input_voltage_threshold)) >>> + charger->input_voltage_threshold =3D DEFAULT_PRE_TIME_OUT; >>> + >>> + if (of_property_read_u32(np, "active-semi,precondition_timeout", >>> + &charger->pre_time_out)) >>> + charger->pre_time_out =3D DEFAULT_PRE_TIME_OUT; >>> + >>> + if (of_property_read_u32(np, "active-semi,total_timeout", >>> + &charger->tolal_time_out)) >>> + charger->tolal_time_out =3D DEFAULT_TOTAL_TIME_OUT; >>> + >>> + return 0; >>> +} >>> + >>> +static int act8945a_charger_config(struct act8945a_charger *charge= r) >>> +{ >>> + struct regmap *regmap =3D charger->act8945a_dev->regmap; >>> + u8 value =3D 0; >> >> Regmap uses "unsigned int". I think it is better not to mix it so us= e in the driver >> "unsigned int" for interactions with regmap. >=20 > I checked, it seems they are not mixed in the patch. Or I am wrong? I mean that use "unsigned int value =3D 0;" here. You're going cast it anyway to unsigned int when calling regmap functions. >=20 >> >> [...] >> >> Javier pointed the auto-loading issue before (because you are not pr= oviding any >> device table). Probably that is not what you want. >=20 > Yes, but I still don't understand for now ... Try building this as module and check whether it got automatically loaded during boot. :) Best regards, Krzysztof