From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752012AbbCIEyY (ORCPT ); Mon, 9 Mar 2015 00:54:24 -0400 Received: from mailout2.samsung.com ([203.254.224.25]:63459 "EHLO mailout2.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751056AbbCIEyU (ORCPT ); Mon, 9 Mar 2015 00:54:20 -0400 X-AuditID: cbfee68d-f79296d000004278-1f-54fd2767e7de Message-id: <54FD2766.2090401@samsung.com> Date: Mon, 09 Mar 2015 13:53:58 +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> <54FD177B.1050200@samsung.com> In-reply-to: <54FD177B.1050200@samsung.com> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrGIsWRmVeSWpSXmKPExsWyRsSkUDdd/W+Iwf+f0hZTHz5hs7j+5Tmr xaQn75kt5h85x2oxceVkZov+NwtZLc69WsloMen+BBaL+1+PMlpc3jWHzeJz7xFGi6XXLzJZ TJi+lsWide8Rdovjnw6yWJzeXWIxY/JLNgdBjzXz1jB6XO7rZfLYOesuu8fK5V/YPDav0PLY tKqTzePOtT1sHn1bVjF6fN4kF8AZxWWTkpqTWZZapG+XwJXx5ONqxoJLfhWn1+1kbGD869DF yMkhIWAicfhDDyOELSZx4d56ti5GLg4hgaWMErvO3GWCKeo//ZMdIjGdUeJ2+10o5zWjRP+x A2wgVbwCWhKrHpwB62ARUJX4ve8F2Fg2AU2J91OusHQxcnCICkRI3L7MCVEuKPFj8j0WEFtE QE3i/aWnLCAzmQW2MUvcudnICFIvLBAq8Xx3BsSu/YwSDddesYI0cApoS/zvecMOUsMsoCdx /6IWSJhZQF5i85q3zCD1EgJ7OCQ6ll5jg7hHQOLb5ENgN0gIyEpsOsAM8ZikxMEVN1gmMIrN QnLSLISps5BMXcDIvIpRNLUguaA4Kb3IUK84Mbe4NC9dLzk/dxMjMPpP/3vWu4Px9gHrQ4wC HIxKPLw7TvwJEWJNLCuuzD3EaAp0xERmKdHkfGCKySuJNzQ2M7IwNTE1NjK3NFMS51WU+hks JJCeWJKanZpakFoUX1Sak1p8iJGJg1OqgVGL65d/2ZYnfPfPnOK4e7EhM/n4kkTdPVvXSoYJ WJke/JYassVdRrp4mdzVybmvhGSv6q7rPPhZaM6FNwoaqes4X7bUtGy91p/66JDxkq2KeeF9 Copi+ee/lGju7ZXMWnXNTqru3NwA/rzzIpcP7phozcc8vfJX3eQfa/dMXfn0zbQfqdHtJ68r sRRnJBpqMRcVJwIAFlBO//kCAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrAKsWRmVeSWpSXmKPExsVy+t9jQd109b8hBs1LBC2mPnzCZnH9y3NW i0lP3jNbzD9yjtVi4srJzBb9bxayWpx7tZLRYtL9CSwW978eZbS4vGsOm8Xn3iOMFkuvX2Sy mDB9LYtF694j7BbHPx1ksTi9u8RixuSXbA6CHmvmrWH0uNzXy+Sxc9Zddo+Vy7+weWxeoeWx aVUnm8eda3vYPPq2rGL0+LxJLoAzqoHRJiM1MSW1SCE1Lzk/JTMv3VbJOzjeOd7UzMBQ19DS wlxJIS8xN9VWycUnQNctMwfoFyWFssScUqBQQGJxsZK+HaYJoSFuuhYwjRG6viFBcD1GBmgg YQ1jxpOPqxkLLvlVnF63k7GB8a9DFyMnh4SAiUT/6Z/sELaYxIV769m6GLk4hASmM0rcbr/L DuG8ZpToP3aADaSKV0BLYtWDM0wgNouAqsTvfS8YQWw2AU2J91OusHQxcnCICkRI3L7MCVEu KPFj8j0WEFtEQE3i/aWnLCAzmQW2MUvcudnICFIvLBAq8Xx3BsSu/YwSDddesYI0cApoS/zv ecMOUsMsoCdx/6IWSJhZQF5i85q3zBMYBWYhWTELoWoWkqoFjMyrGEVTC5ILipPSc430ihNz i0vz0vWS83M3MYJTyzPpHYyrGiwOMQpwMCrx8O448SdEiDWxrLgy9xCjBAezkgiv6EmgEG9K YmVValF+fFFpTmrxIUZToP8nMkuJJucD015eSbyhsYmZkaWRuaGFkbG5kjivkn1biJBAemJJ anZqakFqEUwfEwenVAOjwMvrtlJnF7eLhy3MDRaOkOJJWSob1Fmgv/PqzV+fpTO6mPfOnP+g Rvvb/kv9oukhfVwPZF7tzpn07RMv7/cN6o+ltJdcLI3M3ie7ZL3mqynH//+/P8/5n8k25WtJ nM5f67+7/kp5t9OK6XXV3xjtE5/dG6tOLP518M2kvRci2K2yuO7Vv5diVWIpzkg01GIuKk4E AG6OT7FDAwAA 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 12:46 PM, Beomho Seo wrote: > 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. > RT5033_CHG_MAX_CURRENT is register value(hex). So It is better: case POWER_SUPPLY_PROP_CURRENT_MAX: val->intval = RT5033_CHARGER_FAST_CURRENT_MAX; And then I will fix rt5033_get_charger_current function more readable. >>> + 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 > -- > To unsubscribe from this list: send the line "unsubscribe linux-pm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >