From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751879AbbCIDq2 (ORCPT ); Sun, 8 Mar 2015 23:46:28 -0400 Received: from mailout2.samsung.com ([203.254.224.25]:50211 "EHLO mailout2.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751596AbbCIDqK (ORCPT ); Sun, 8 Mar 2015 23:46:10 -0400 X-AuditID: cbfee68f-f791c6d000004834-c6-54fd177c3611 Message-id: <54FD177B.1050200@samsung.com> Date: Mon, 09 Mar 2015 12:46:03 +0900 From: Beomho Seo User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-version: 1.0 To: Sebastian Reichel Cc: broonie@kernel.org, robh+dt@kernel.org, pawel.moll@arm.com, mark.rutland@arm.com, ijc+devicetree@hellion.org.uk, galak@codeaurora.org, linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org, devicetree@vger.kernel.org, lee.jones@linaro.org, cw00.choi@samsung.com, sangbae90.lee@samsung.com, inki.dae@samsung.com, sw0312.kim@samsung.com, Dmitry Eremin-Solenikov , David Woodhouse Subject: Re: [PATCH v5 1/2] power: rt5033_charger: Add RT5033 charger device driver References: <1425864191-4121-1-git-send-email-beomho.seo@samsung.com> <1425864191-4121-2-git-send-email-beomho.seo@samsung.com> <20150309015020.GA941@earth> In-reply-to: <20150309015020.GA941@earth> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrGIsWRmVeSWpSXmKPExsWyRsSkWLdG/G+IwYsv3BZTHz5hs7j+5Tmr xaQn75kt5h85x2oxceVkZov+NwtZLc69WsloMen+BBaL+1+PMlpc3jWHzeJz7xFGi6XXLzJZ TJi+lsWide8Rdovjnw6yWJzeXWIxY/JLNgdBjzXz1jB6XO7rZfLYOesuu8fK5V/YPDav0PLY tKqTzePOtT1sHn1bVjF6fN4kF8AZxWWTkpqTWZZapG+XwJXx9O1O9oJ9HhXrdrQzNjDus+5i 5OSQEDCReLj9CCOELSZx4d56ti5GLg4hgaWMErc697HDFH190cMCkVjEKPHo31wo5zWjxM1l j1lAqngFtCSe7NsIZrMIqEpM3P4WzGYT0JR4P+UKkM3BISoQIXH7MidEuaDEj8n3wEpEBNQk 3l96CjaTWWAbs8Sdm42MIPXCAqESz3dnQOxawCjR/mAvE0gDJ9DM+c+awWYyC+hJ3L+oBRJm FpCX2LzmLTNIvYTAWg6JCTfXMULcIyDxbfIhsHoJAVmJTQeYIR6TlDi44gbLBEaxWUhOmoUw dRaSqQsYmVcxiqYWJBcUJ6UXGesVJ+YWl+al6yXn525iBEb/6X/P+ncw3j1gfYhRgINRiYd3 x4k/IUKsiWXFlbmHGE2BjpjILCWanA9MMXkl8YbGZkYWpiamxkbmlmZK4rwLpX4GCwmkJ5ak ZqemFqQWxReV5qQWH2Jk4uCUamDcIBn46kd0zpmSngOGjcGKHE8OPm627WTeZ1qr/kXbWlKE ZUbbt4d60/Ycf/+j/0Qi8xumo40H7hYzuPHFT1Lcx1yjrj39e+mt3CNuCw+YCZ+cHK/NtXuG 6qJJDN59Mp26LdU25jKl7jI39tp2Tj39+UWb9o/OC9+s9j2+LibQtKhk2Y9PdkZKLMUZiYZa zEXFiQD2UcS7+QIAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrPKsWRmVeSWpSXmKPExsVy+t9jAd0a8b8hBlf0LaY+fMJmcf3Lc1aL SU/eM1vMP3KO1WLiysnMFv1vFrJanHu1ktFi0v0JLBb3vx5ltLi8aw6bxefeI4wWS69fZLKY MH0ti0Xr3iPsFsc/HWSxOL27xGLG5JdsDoIea+atYfS43NfL5LFz1l12j5XLv7B5bF6h5bFp VSebx51re9g8+rasYvT4vEkugDOqgdEmIzUxJbVIITUvOT8lMy/dVsk7ON453tTMwFDX0NLC XEkhLzE31VbJxSdA1y0zB+gVJYWyxJxSoFBAYnGxkr4dpgmhIW66FjCNEbq+IUFwPUYGaCBh DWPG07c72Qv2eVSs29HO2MC4z7qLkZNDQsBE4uuLHhYIW0ziwr31bF2MXBxCAosYJR79m8sC 4bxmlLi57DFYFa+AlsSTfRvBbBYBVYmJ29+C2WwCmhLvp1wBsjk4RAUiJG5f5oQoF5T4Mfke WImIgJrE+0tPwWYyC2xjlrhzs5ERpF5YIFTi+e4MiF0LGCXaH+xlAmngBJo5/1kz2ExmAT2J +xe1QMLMAvISm9e8ZZ7AKDALyYpZCFWzkFQtYGRexSiaWpBcUJyUnmuoV5yYW1yal66XnJ+7 iRGcWJ5J7WBc2WBxiFGAg1GJh3fHiT8hQqyJZcWVuYcYJTiYlUR4RU8ChXhTEiurUovy44tK c1KLDzGaAv0/kVlKNDkfmPTySuINjU3MjCyNzA0tjIzNlcR5lezbQoQE0hNLUrNTUwtSi2D6 mDg4pRoYO4y1PV6xFgacXWHzNvKOXkGA6KEUnaK40O/n0yzWFJ8I+6D54+gZCb3C9ds2Xrm2 texFlNrKhU8atlmHsAVJ9xxV+KUVWFO41Lvcs/jgtPgjQtkeRn02P0Oy+MX3/8uL35WUNrOl IKujfYa5/pGbZedNelfcb1pSI+38JmLPrvgsucunvA8rsRRnJBpqMRcVJwIAEYlcpkIDAAA= DLP-Filter: Pass X-MTR: 20000000000000000@CPGS X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 03/09/2015 10:50 AM, Sebastian Reichel wrote: > Hi Beomho, > > On Mon, Mar 09, 2015 at 10:23:10AM +0900, Beomho Seo wrote: >> This patch add device driver of Richtek RT5033 PMIC. The driver support >> switching charger. rt5033 charger provide three charging mode. >> Three charging mode are pre charge mode, fast cahrge mode and constant voltage >> mode. They are have vary charge rate, charge parameters. The charge parameters >> can be controlled by i2c interface. > > Driver looks mostly ok, but I have some comments [inline]. > >> Cc: Sebastian Reichel >> Cc: Dmitry Eremin-Solenikov >> Cc: David Woodhouse >> Signed-off-by: Beomho Seo >> Acked-by: Chanwoo Choi >> --- >> Changes in v5 >> - none. >> Changes in v4 >> - Change power supply type to POWER_SUPPLY_TYPE_MAINS. >> Changes in v3 >> Changes in v2 >> - none >> >> drivers/power/Kconfig | 8 + >> drivers/power/Makefile | 1 + >> drivers/power/rt5033_charger.c | 485 ++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 494 insertions(+) >> create mode 100644 drivers/power/rt5033_charger.c >> >> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig >> index 27b751b..67e9af7 100644 >> --- a/drivers/power/Kconfig >> +++ b/drivers/power/Kconfig >> @@ -418,6 +418,14 @@ config BATTERY_RT5033 >> The fuelgauge calculates and determines the battery state of charge >> according to battery open circuit voltage. >> >> +config CHARGER_RT5033 >> + tristate "RT5033 battery charger support" >> + depends on MFD_RT5033 >> + help >> + This adds support for battery charger in Richtek RT5033 PMIC. >> + The device supports pre-charge mode, fast charge mode and >> + constant voltage mode. >> + >> source "drivers/power/reset/Kconfig" >> >> endif # POWER_SUPPLY >> diff --git a/drivers/power/Makefile b/drivers/power/Makefile >> index 36f9e0d..c5d72e0 100644 >> --- a/drivers/power/Makefile >> +++ b/drivers/power/Makefile >> @@ -59,6 +59,7 @@ obj-$(CONFIG_CHARGER_BQ2415X) += bq2415x_charger.o >> obj-$(CONFIG_CHARGER_BQ24190) += bq24190_charger.o >> obj-$(CONFIG_CHARGER_BQ24735) += bq24735-charger.o >> obj-$(CONFIG_POWER_AVS) += avs/ >> +obj-$(CONFIG_POWER_RT5033) += rt5033_charger.o > > this should be > > obj-$(CONFIG_CHARGER_RT5033) += rt5033_charger.o > > according to your Kconfig patch. How did you test it actually? > Sorry, My mistake. This patch have been tested on my board. I will double-check before send patch set. >> obj-$(CONFIG_CHARGER_SMB347) += smb347-charger.o >> obj-$(CONFIG_CHARGER_TPS65090) += tps65090-charger.o >> obj-$(CONFIG_POWER_RESET) += reset/ >> diff --git a/drivers/power/rt5033_charger.c b/drivers/power/rt5033_charger.c >> new file mode 100644 >> index 0000000..7f8f6c3 >> --- /dev/null >> +++ b/drivers/power/rt5033_charger.c >> >> [...] >> >> +static int rt5033_get_charger_current(struct rt5033_charger *charger, >> + enum power_supply_property psp) >> +{ >> + struct regmap *regmap = charger->rt5033->regmap; >> + unsigned int state, reg_data, data; >> + >> + if (psp == POWER_SUPPLY_PROP_CURRENT_MAX) >> + return RT5033_CHG_MAX_CURRENT; > > drop this psp check and the psp parameter to this function, so that > the function only takes care of the current current. > OK. I will remove above lines. >> + regmap_read(regmap, RT5033_REG_CHG_CTRL5, ®_data); >> + >> + state = (reg_data >> RT5033_CHGCTRL5_ICHG_SHIFT) & 0xf; >> + >> + if (state > RT5033_CHG_MAX_CURRENT) >> + state = RT5033_CHG_MAX_CURRENT; >> + >> + data = state * 100 + 700; >> + >> + return data; >> +} >> + >> +static int rt5033_get_charge_voltage(struct rt5033_charger *charger, >> + enum power_supply_property psp) >> +{ >> + struct regmap *regmap = charger->rt5033->regmap; >> + unsigned int state, reg_data, data; >> + >> + if (psp == POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX) >> + return RT5033_CHARGER_CONST_VOLTAGE_LIMIT_MAX; > > drop this psp check and the psp parameter to this function, so that > the function only takes care of the current voltage. > OK. I will remove above lines. >> + regmap_read(regmap, RT5033_REG_CHG_CTRL2, ®_data); >> + >> + state = reg_data >> 2; >> + >> + data = RT5033_CHARGER_CONST_VOLTAGE_LIMIT_MIN + >> + RT5033_CHARGER_CONST_VOLTAGE_STEP_NUM * state; >> + >> + if (data > RT5033_CHARGER_CONST_VOLTAGE_LIMIT_MAX) >> + data = RT5033_CHARGER_CONST_VOLTAGE_LIMIT_MAX; >> + >> + return data; >> +} >> >> [...] >> >> + >> +static enum power_supply_property rt5033_charger_props[] = { >> + POWER_SUPPLY_PROP_STATUS, >> + POWER_SUPPLY_PROP_CHARGE_TYPE, >> + POWER_SUPPLY_PROP_CURRENT_NOW, >> + POWER_SUPPLY_PROP_CURRENT_MAX, >> + POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE, >> + POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX, >> + POWER_SUPPLY_PROP_MODEL_NAME, >> + POWER_SUPPLY_PROP_MANUFACTURER, >> +}; >> + >> +static int rt5033_charger_get_property(struct power_supply *psy, >> + enum power_supply_property psp, >> + union power_supply_propval *val) >> +{ >> + struct rt5033_charger *charger = container_of(psy, >> + struct rt5033_charger, psy); >> + int ret = 0; >> + >> + switch (psp) { >> + case POWER_SUPPLY_PROP_STATUS: >> + val->intval = rt5033_get_charger_state(charger); >> + break; >> + case POWER_SUPPLY_PROP_CHARGE_TYPE: >> + val->intval = rt5033_get_charger_type(charger); >> + break; >> + case POWER_SUPPLY_PROP_CURRENT_NOW: >> + case POWER_SUPPLY_PROP_CURRENT_MAX: >> + val->intval = rt5033_get_charger_current(charger, psp); >> + break; > > remove the special handling of POWER_SUPPLY_PROP_CURRENT_MAX in > rt5033_get_charger_current() and do it like this: > > case POWER_SUPPLY_PROP_CURRENT_NOW: > val->intval = rt5033_get_charger_current(charger); > break; > case POWER_SUPPLY_PROP_CURRENT_MAX: > val->intval = RT5033_CHG_MAX_CURRENT; > break; > OK. I will change comply with your comment. >> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE: >> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX: >> + val->intval = rt5033_get_charge_voltage(charger, psp); >> + break; > > same as current handling: > > case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE: > val->intval = rt5033_get_charge_voltage(charger); > break; > case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX: > val->intval = RT5033_CHARGER_CONST_VOLTAGE_LIMIT_MAX; > break; > Ditto. >> + case POWER_SUPPLY_PROP_MODEL_NAME: >> + val->strval = RT5033_CHARGER_MODEL; >> + break; >> + case POWER_SUPPLY_PROP_MANUFACTURER: >> + val->strval = RT5033_MANUFACTURER; >> + break; >> + default: >> + return -EINVAL; >> + } >> + >> + return ret; >> +} >> >> [...] >> >> +static int rt5033_charger_probe(struct platform_device *pdev) >> +{ >> + struct rt5033_charger *charger; >> + struct rt5033_dev *rt5033 = dev_get_drvdata(pdev->dev.parent); >> + int ret; >> + >> + charger = devm_kzalloc(&pdev->dev, sizeof(*charger), GFP_KERNEL); >> + if (!charger) >> + return -ENOMEM; >> + >> + platform_set_drvdata(pdev, charger); >> + charger->dev = &pdev->dev; >> + charger->rt5033 = rt5033; >> + >> + charger->chg = rt5033_charger_dt_init(pdev); >> + if (IS_ERR_OR_NULL(charger->chg)) >> + return -ENODEV; >> + >> + ret = rt5033_charger_reg_init(charger); >> + if (ret) >> + return ret; >> + >> + charger->psy.name = "rt5033-charger", >> + charger->psy.type = POWER_SUPPLY_TYPE_MAINS, >> + charger->psy.properties = rt5033_charger_props, >> + charger->psy.num_properties = ARRAY_SIZE(rt5033_charger_props), >> + charger->psy.get_property = rt5033_charger_get_property, >> + >> + ret = power_supply_register(&pdev->dev, &charger->psy); >> + if (ret) { >> + dev_err(&pdev->dev, "Failed to register power supply\n"); >> + return ret; >> + } > > We have devm_power_supply_register() now, which would result in > complete removal of the rt5033_charger_remove() function :) > OK, I will change comply with your comment. >> + return 0; >> +} >> + >> +static int rt5033_charger_remove(struct platform_device *pdev) >> +{ >> + struct rt5033_charger *charger = platform_get_drvdata(pdev); >> + >> + power_supply_unregister(&charger->psy); >> + >> + return 0; >> +} >> + >> +static const struct platform_device_id rt5033_charger_id[] = { >> + { "rt5033-charger", }, >> + { } >> +}; >> +MODULE_DEVICE_TABLE(platform, rt5033_charger_id); >> + >> +static struct platform_driver rt5033_charger_driver = { >> + .driver = { >> + .name = "rt5033-charger", >> + }, >> + .probe = rt5033_charger_probe, >> + .remove = rt5033_charger_remove, >> + .id_table = rt5033_charger_id, >> +}; >> +module_platform_driver(rt5033_charger_driver); >> + >> +MODULE_DESCRIPTION("Richtek RT5033 charger driver"); >> +MODULE_AUTHOR("Beomho Seo "); >> +MODULE_LICENSE("GPL"); > > This should be > > MODULE_LICENSE("GPL v2"); > > -- Sebastian > Thanks your comment. I will change this patch as soon as I possibly can. Best regards, Beomho Seo