From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752258AbbBWBn2 (ORCPT ); Sun, 22 Feb 2015 20:43:28 -0500 Received: from mailout4.samsung.com ([203.254.224.34]:19472 "EHLO mailout4.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752136AbbBWBnZ (ORCPT ); Sun, 22 Feb 2015 20:43:25 -0500 X-AuditID: cbfee68e-f79b46d000002b74-27-54ea85bbe1d9 Message-id: <54EA85BB.4050600@samsung.com> Date: Mon, 23 Feb 2015 10:43:23 +0900 From: Jaewon Kim User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-version: 1.0 To: Lee Jones Cc: linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-pm@vger.kernel.org, Inki Dae , SangBae Lee , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Chanwoo Choi , Sebastian Reichel , Mark Brown , Beomho Seo Subject: Re: [PATCH v3 1/6] mfd: max77843: Add max77843 MFD driver core driver References: <1423025771-4139-1-git-send-email-jaewon02.kim@samsung.com> <1423025771-4139-2-git-send-email-jaewon02.kim@samsung.com> <20150216135155.GI14545@x1> In-reply-to: <20150216135155.GI14545@x1> Content-type: text/plain; charset=utf-8; format=flowed Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrCIsWRmVeSWpSXmKPExsWyRsSkQHd366sQg3XLNC1Of9rGbjH14RM2 i+tfnrNazD9yjtWi/81CVotzr1YyWky6P4HF4v7Xo4wWl3fNYbP43HuE0WLp9YtMFhOmr2Wx aN17hN3i+KeDLBand5c48HusmbeG0eNyXy+Tx8rlX9g8Nq3qZPO4c20Pm0ffllWMHp83yQWw R3HZpKTmZJalFunbJXBlPPz4gLFghkPFrednWRsYrxh3MXJySAiYSPR/2s0IYYtJXLi3nq2L kYtDSGApo8TT3Y+YYIoarkxmgUhMZ5TounyLFcJ5zSjxZfcTsHZeAS2JU/1TgWwODhYBVYn1 O8VAwmwC2hLf1y9mBbFFBSIk5h97zQxRLijxY/I9FpByEQEViXNvzEHCzALfmSVabnKC2MIC fhL9f5YyQ6xazCjxc1orC0iCU0BDon/OQWaIBjOJLy8Ps0LY8hKb17wFa5AQmMohsb1nMjtI gkVAQOLb5ENgyyQEZCU2HWCGeExS4uCKGywTGMVmITlpFpKxs5CMXcDIvIpRNLUguaA4Kb3I SK84Mbe4NC9dLzk/dxMjMLJP/3vWt4Px5gHrQ4wCHIxKPLwNua9ChFgTy4orcw8xmgJdMZFZ SjQ5H5g+8kriDY3NjCxMTUyNjcwtzZTEeROkfgYLCaQnlqRmp6YWpBbFF5XmpBYfYmTi4JRq YJSoWRGUurdS7O+SjuffgmLOHwzmynkm+EXjql901cRbK//e3isle2r5qQrVxyVB0x82GD3U 8wp8zNM0a/6c7Y9OLmax/cAor7z2ea31hErfDfPLj3Jx9sjbNxsJHvZ5++E356QJIkF7g+Yc LN33Zm146EmlWM9bLMvSDvAc7glwCDsc+nHd/GQlluKMREMt5qLiRAC/g0oq5wIAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprHKsWRmVeSWpSXmKPExsVy+t9jQd3dra9CDA5elbM4/Wkbu8XUh0/Y LK5/ec5qMf/IOVaL/jcLWS3OvVrJaDHp/gQWi/tfjzJaXN41h83ic+8RRoul1y8yWUyYvpbF onXvEXaL458Oslic3l3iwO+xZt4aRo/Lfb1MHiuXf2Hz2LSqk83jzrU9bB59W1YxenzeJBfA HtXAaJORmpiSWqSQmpecn5KZl26r5B0c7xxvamZgqGtoaWGupJCXmJtqq+TiE6DrlpkDdLiS QlliTilQKCCxuFhJ3w7ThNAQN10LmMYIXd+QILgeIwM0kLCGMePhxweMBTMcKm49P8vawHjF uIuRk0NCwESi4cpkFghbTOLCvfVsXYxcHEIC0xklui7fYoVwXjNKfNn9hBGkildAS+JU/1Qg m4ODRUBVYv1OMZAwm4C2xPf1i1lBbFGBCIn5x14zQ5QLSvyYfI8FpFxEQEXi3BtzkDCzwHdm iZabnCC2sICfRP+fpcwQqxYzSvyc1gp2EKeAhkT/nIPMEA1mEl9eHmaFsOUlNq95yzyBUWAW khWzkJTNQlK2gJF5FaNoakFyQXFSeq6hXnFibnFpXrpecn7uJkZw2ngmtYNxZYPFIUYBDkYl Ht6G3FchQqyJZcWVuYcYJTiYlUR4g92AQrwpiZVVqUX58UWlOanFhxhNgQEwkVlKNDkfmNLy SuINjU3MjCyNzA0tjIzNlcR5lezbQoQE0hNLUrNTUwtSi2D6mDg4pRoY2ZU3G2749dHxepXU neMZGcsPaNb5pyx7vt6750epwf3A9Fv7/CbvOuAjflBlndGnkLwDbkcna/3ku7Dd8m75gYCD IgJpayW6D9vl5x29sKZt2fbPUwokp2pMuvlo/e+WnTbmsfGdKw6V8QZxh24888LPiK1VU87c Tl646mu19d8rH/KEWb/GKLEUZyQaajEXFScCAKBqnJ0xAwAA 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 Hi Lee Jones, On 02/16/2015 10:51 PM, Lee Jones wrote: > On Wed, 04 Feb 2015, Jaewon Kim wrote: > >> This patch adds MAX77843 core/irq driver to support PMIC, >> MUIC(Micro USB Interface Controller), Charger, Fuel Gauge, >> LED and Haptic device. >> >> Cc: Lee Jones >> Signed-off-by: Jaewon Kim >> Signed-off-by: Beomho Seo >> --- >> drivers/mfd/Kconfig | 14 ++ >> drivers/mfd/Makefile | 1 + >> drivers/mfd/max77843.c | 245 +++++++++++++++++++ >> include/linux/mfd/max77843-private.h | 441 ++++++++++++++++++++++++++++++++++ >> 4 files changed, 701 insertions(+) >> create mode 100644 drivers/mfd/max77843.c >> create mode 100644 include/linux/mfd/max77843-private.h >> >> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig >> index 2e6b731..0c67c79 100644 >> --- a/drivers/mfd/Kconfig >> +++ b/drivers/mfd/Kconfig >> @@ -442,6 +442,20 @@ config MFD_MAX77693 >> additional drivers must be enabled in order to use the functionality >> of the device. >> >> +config MFD_MAX77843 >> + bool "Maxim Semiconductor MAX77843 PMIC Support" >> + depends on I2C=y >> + select MFD_CORE >> + select REGMAP_I2C >> + select REGMAP_IRQ >> + help >> + Say yes here to add support for Maxim Semiconductor MAX77843. >> + This is companion Power Management IC with LEDs, Haptic, Charger, >> + Fuel Gauge, MUIC(Micro USB Interface Controller) controls on chip. >> + This driver provides common support for accessing the device; >> + additional drivers must be enabled in order to use the functionality >> + of the device. >> + >> config MFD_MAX8907 >> tristate "Maxim Semiconductor MAX8907 PMIC Support" >> select MFD_CORE >> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile >> index 53467e2..fe4f75c 100644 >> --- a/drivers/mfd/Makefile >> +++ b/drivers/mfd/Makefile >> @@ -117,6 +117,7 @@ obj-$(CONFIG_MFD_DA9063) += da9063.o >> obj-$(CONFIG_MFD_MAX14577) += max14577.o >> obj-$(CONFIG_MFD_MAX77686) += max77686.o >> obj-$(CONFIG_MFD_MAX77693) += max77693.o >> +obj-$(CONFIG_MFD_MAX77843) += max77843.o > This is the 11th MAX driver. Can't they be supported using device > specific data structures instead of taking a 'one file per device' > approach? Kzysztof answered already, we try to merge with other files. But MAX77843 can`t merge with another MAX drivers. Because This driver has new features (e.g AFC charger, Reverse Boost, 4 channel LED driver, etc) so new registers extended and moved. > >> obj-$(CONFIG_MFD_MAX8907) += max8907.o >> max8925-objs := max8925-core.o max8925-i2c.o >> obj-$(CONFIG_MFD_MAX8925) += max8925.o >> diff --git a/drivers/mfd/max77843.c b/drivers/mfd/max77843.c >> new file mode 100644 >> index 0000000..191a557 >> --- /dev/null >> +++ b/drivers/mfd/max77843.c >> @@ -0,0 +1,245 @@ >> +/* >> + * max77843.c - MFD core driver for the Maxim MAX77843 >> + * >> + * Copyright (C) 2015 Samsung Electronics >> + * Author: Jaewon Kim >> + * Author: Beomho Seo >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include > [...] > >> +static int max77843_probe(struct i2c_client *i2c, >> + const struct i2c_device_id *id) > Strange tabbing here. I will fix it in next version. > >> +{ >> + struct max77843 *max77843; >> + unsigned int reg_data; >> + int ret; >> + >> + max77843 = devm_kzalloc(&i2c->dev, sizeof(*max77843), GFP_KERNEL); >> + if (!max77843) >> + return -ENOMEM; >> + >> + i2c_set_clientdata(i2c, max77843); >> + max77843->dev = &i2c->dev; >> + max77843->i2c = i2c; >> + max77843->irq = i2c->irq; >> + >> + max77843->regmap = devm_regmap_init_i2c(i2c, >> + &max77843_regmap_config); >> + if (IS_ERR(max77843->regmap)) { >> + dev_err(&i2c->dev, "Failed to allocate topsys register map\n"); >> + return PTR_ERR(max77843->regmap); >> + } >> + >> + ret = regmap_add_irq_chip(max77843->regmap, max77843->irq, >> + IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED, >> + 0, &max77843_irq_chip, &max77843->irq_data); >> + if (ret) { >> + dev_err(&i2c->dev, "Failed to add TOPSYS IRQ chip\n"); >> + return ret; >> + } >> + >> + ret = regmap_read(max77843->regmap, >> + MAX77843_SYS_REG_PMICID, ®_data); >> + if (ret < 0) { >> + dev_err(&i2c->dev, "Failed to read PMIC ID\n"); >> + goto err_pmic_id; >> + } >> + dev_info(&i2c->dev, "device ID: 0x%x\n", reg_data); >> + >> + ret = max77843_chg_init(max77843); >> + if (ret) { >> + dev_err(&i2c->dev, "Failed to init Charger\n"); >> + goto err_pmic_id; >> + } >> + >> + ret = regmap_update_bits(max77843->regmap, >> + MAX77843_SYS_REG_INTSRCMASK, >> + MAX77843_INTSRC_MASK_MASK, >> + (unsigned int)~MAX77843_INTSRC_MASK_MASK); > This stuff looks so much better/easier to read if you line up against > the '('. Okay, I will line up against '('. > >> + if (ret < 0) { >> + dev_err(&i2c->dev, "Failed to unmask interrupt source\n"); >> + goto err_pmic_id; >> + } >> + >> + ret = mfd_add_devices(max77843->dev, -1, max77843_devs, >> + ARRAY_SIZE(max77843_devs), NULL, 0, NULL); >> + if (ret < 0) { >> + dev_err(&i2c->dev, "Failed to add mfd device\n"); >> + goto err_pmic_id; >> + } >> + >> + device_init_wakeup(max77843->dev, 1); >> + >> + return 0; >> + >> +err_pmic_id: >> + regmap_del_irq_chip(max77843->irq, max77843->irq_data); >> + >> + return ret; >> +} >> + >> +static int max77843_remove(struct i2c_client *i2c) >> +{ >> + struct max77843 *max77843 = i2c_get_clientdata(i2c); >> + >> + mfd_remove_devices(max77843->dev); >> + >> + regmap_del_irq_chip(max77843->irq, max77843->irq_data); >> + >> + i2c_unregister_device(max77843->i2c_chg); >> + i2c_unregister_device(max77843->i2c); >> + >> + return 0; >> +} >> + >> +static const struct of_device_id max77843_dt_match[] = { >> + { .compatible = "maxim,max77843", }, >> + { /* sentinel */ }, >> +}; >> + >> +static const struct i2c_device_id max77843_id[] = { >> + { "max77843", }, >> + { /* sentinel */ }, >> +}; >> +MODULE_DEVICE_TABLE(i2c, max77843_id); >> + >> +static int __maybe_unused max77843_suspend(struct device *dev) >> +{ >> + struct i2c_client *i2c = container_of(dev, struct i2c_client, dev); >> + struct max77843 *max77843 = i2c_get_clientdata(i2c); >> + >> + if (device_may_wakeup(dev)) >> + enable_irq_wake(max77843->irq); >> + disable_irq(max77843->irq); >> + >> + return 0; >> +} >> + >> +static int __maybe_unused max77843_resume(struct device *dev) >> +{ >> + struct i2c_client *i2c = container_of(dev, struct i2c_client, dev); >> + struct max77843 *max77843 = i2c_get_clientdata(i2c); >> + >> + if (device_may_wakeup(dev)) >> + disable_irq_wake(max77843->irq); >> + enable_irq(max77843->irq); >> + >> + return 0; >> +} >> + >> +static SIMPLE_DEV_PM_OPS(max77843_pm, max77843_suspend, max77843_resume); >> + >> +static struct i2c_driver max77843_i2c_driver = { >> + .driver = { > More strange whitespace stuff. > > I'm gussing you haven't run checkpatch.pl. > > Please do so and fix-up the warnings. ".drivers" is seven letters. So, it seems like a white space but it is tab Anyway, I will not lining up driver structure in next version. > >> + .name = "max77843", >> + .pm = &max77843_pm, >> + .of_match_table = max77843_dt_match, >> + }, > Lining these up with the '=' doesn't make the code any better IMO. > > [...] > Thanks to review my patch. I will send version4 soon. Thanks, Jaewon Kim