From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753551AbaCKO7U (ORCPT ); Tue, 11 Mar 2014 10:59:20 -0400 Received: from mailout3.w1.samsung.com ([210.118.77.13]:25977 "EHLO mailout3.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752791AbaCKO7R (ORCPT ); Tue, 11 Mar 2014 10:59:17 -0400 X-AuditID: cbfec7f5-b7fc96d000004885-ba-531f24c2bcfa Message-id: <531F24C0.3070603@samsung.com> Date: Tue, 11 Mar 2014 15:59:12 +0100 From: Robert Baldyga User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.3.0 MIME-version: 1.0 To: Krzysztof Kozlowski Cc: sameo@linux.intel.com, lee.jones@linaro.org, myungjoo.ham@samsung.com, cw00.choi@samsung.com, dmitry.torokhov@gmail.com, cooloney@gmail.com, rpurdie@rpsys.net, dbaryshkov@gmail.com, dwmw2@infradead.org, lgirdwood@gmail.com, broonie@kernel.org, a.zummo@towertech.it, paul.gortmaker@windriver.com, linux-kernel@vger.kernel.org, linux-input@vger.kernel.org, linux-leds@vger.kernel.org, rtc-linux@googlegroups.com, m.szyprowski@samsung.com Subject: Re: [PATCH 1/3] mfd: max8997: use regmap to access registers References: <1394546304-15191-1-git-send-email-r.baldyga@samsung.com> <1394546304-15191-2-git-send-email-r.baldyga@samsung.com> <1394548336.24263.7.camel@AMDC1943> In-reply-to: <1394548336.24263.7.camel@AMDC1943> Content-type: text/plain; charset=UTF-8 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprGIsWRmVeSWpSXmKPExsVy+t/xa7qHVOSDDf68YLdYcvEqu8XUh0/Y LI7unMhkcf3Lc1aLSU/eM1scXvSC0WLiysnMFq9fGFrc/3qU0eLblQ4mi5ufvrFaXN41h81i 65t1jBZrj9xlt7jduILN4tre48wWu3c9ZbXY39nBaHG6m9VB2GPnrLvsHnsmnmTz2LxCy2PT qk42jzvX9rB5zDsZ6LFn/g9Wj74tqxg9ps/7yeTxeZOcx/otW5kCuKO4bFJSczLLUov07RK4 MmbffcNcsDKnoq/1C1sD47/ILkZODgkBE4mtG9tZIGwxiQv31rN1MXJxCAksZZR4sH4qK4Tz kVFi/dsdrCBVvAJaEld/XWcGsVkEVCVmvDnOBGKzCehIbPk+gRHEFhWIkJg7cTMbRL2gxI/J 98A2iAgYShzcvZ0JZCizwGFmiX0POsCGCgu4Skza1A21bTmjxJkPr8A2cAoYSGxd/BZsA7OA usSkeYuYIWx5ic1r3jJPYBSYhWTJLCRls5CULWBkXsUomlqaXFCclJ5rpFecmFtcmpeul5yf u4kREpdfdzAuPWZ1iFGAg1GJh3emknywEGtiWXFl7iFGCQ5mJRHeZHGgEG9KYmVValF+fFFp TmrxIUYmDk6pBsYt3QtSb61bbcffbreiIv//kew1V1WPtJxYv1mvt/OW+BWvlee5/5VXhd+5 9WnK+l6Z1RulfwdM1g5SjD566Xx60h3NGcfn2S6ecH7fnH0O3eV3VTIyePeH7y+Wv60w0UrY r0BuG8fSRdk7Xse9M3hgWP5iycXAdXq5v4xmLko+kzeXQbCvgWujEktxRqKhFnNRcSIAlHYl 26kCAAA= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 03/11/2014 03:32 PM, Krzysztof Kozlowski wrote: > On Tue, 2014-03-11 at 14:58 +0100, Robert Baldyga wrote: >> This patch modifies max8997 driver and each associated function >> driver, to use regmap instead of operating directly on i2c bus. It >> will allow to simplify IRQ handling using regmap-irq. >> >> Signed-off-by: Robert Baldyga --- >> drivers/extcon/extcon-max8997.c | 31 ++++---- >> drivers/input/misc/max8997_haptic.c | 34 +++++---- >> drivers/leds/leds-max8997.c | 13 ++-- >> drivers/mfd/max8997-irq.c | 64 ++++++---------- >> drivers/mfd/max8997.c | 143 >> ++++++++++++++++------------------- >> drivers/power/max8997_charger.c | 33 ++++---- >> drivers/regulator/max8997.c | 87 ++++++++++----------- >> drivers/rtc/rtc-max8997.c | 56 ++++++++------ >> include/linux/mfd/max8997-private.h | 17 ++--- 9 files changed, >> 229 insertions(+), 249 deletions(-) > > I think you should also add "select REGMAP_I2C" to the main driver > config secion (drivers/mfd/Kconfig). Will be fixed. > > (...) > >> diff --git a/drivers/mfd/max8997-irq.c b/drivers/mfd/max8997-irq.c >> index 43fa614..656d03b 100644 --- a/drivers/mfd/max8997-irq.c +++ >> b/drivers/mfd/max8997-irq.c @@ -26,6 +26,7 @@ #include >> #include #include >> +#include >> >> static const u8 max8997_mask_reg[] = { [PMIC_INT1] = >> MAX8997_REG_INT1MSK, @@ -41,25 +42,6 @@ static const u8 >> max8997_mask_reg[] = { [FLASH_STATUS] = MAX8997_REG_INVALID, }; >> >> -static struct i2c_client *get_i2c(struct max8997_dev *max8997, - >> enum max8997_irq_source src) -{ - switch (src) { - case PMIC_INT1 >> ... PMIC_INT4: - return max8997->i2c; - case FUEL_GAUGE: - return >> NULL; - case MUIC_INT1 ... MUIC_INT3: - return max8997->muic; - >> case GPIO_LOW ... GPIO_HI: - return max8997->i2c; - case >> FLASH_STATUS: - return max8997->i2c; - default: - return >> ERR_PTR(-EINVAL); - } -} - struct max8997_irq_data { int mask; >> enum max8997_irq_source group; @@ -124,15 +106,20 @@ static void >> max8997_irq_sync_unlock(struct irq_data *data) int i; >> >> for (i = 0; i < MAX8997_IRQ_GROUP_NR; i++) { + struct regmap *map; >> u8 mask_reg = max8997_mask_reg[i]; - struct i2c_client *i2c = >> get_i2c(max8997, i); + + if (i >= MUIC_INT1 && i <= MUIC_INT3) + >> map = max8997->regmap_muic; + else + map = max8997->regmap; >> >> if (mask_reg == MAX8997_REG_INVALID || - IS_ERR_OR_NULL(i2c)) + >> IS_ERR_OR_NULL(map)) continue; max8997->irq_masks_cache[i] = >> max8997->irq_masks_cur[i]; >> >> - max8997_write_reg(i2c, max8997_mask_reg[i], + regmap_write(map, >> max8997_mask_reg[i], max8997->irq_masks_cur[i]); } >> >> @@ -180,12 +167,12 @@ static struct irq_chip max8997_irq_chip = { >> static irqreturn_t max8997_irq_thread(int irq, void *data) { >> struct max8997_dev *max8997 = data; - u8 >> irq_reg[MAX8997_IRQ_GROUP_NR] = {}; - u8 irq_src; + unsigned int >> irq_reg[MAX8997_IRQ_GROUP_NR] = {}; + unsigned int irq_src; int >> ret; int i, cur_irq; >> >> - ret = max8997_read_reg(max8997->i2c, MAX8997_REG_INTSRC, >> &irq_src); + ret = regmap_read(max8997->regmap, >> MAX8997_REG_INTSRC, &irq_src); if (ret < 0) { dev_err(max8997->dev, >> "Failed to read interrupt source: %d\n", ret); @@ -194,8 +181,9 @@ >> static irqreturn_t max8997_irq_thread(int irq, void *data) >> >> if (irq_src & MAX8997_IRQSRC_PMIC) { /* PMIC INT1 ~ INT4 */ - >> max8997_bulk_read(max8997->i2c, MAX8997_REG_INT1, 4, - >> &irq_reg[PMIC_INT1]); + for (i = 0; i < 4; ++i) + >> regmap_read(max8997->regmap, + MAX8997_REG_INT1+i, >> &irq_reg[PMIC_INT1+i]); > > Can't you use here one bulk read instead of 4xregmap_read()? Mixing regmap_read and regmap_bulk_read is not good idea, because the first function returns register value as unsigned int, but the second returns reg value to each single byte. So it would need to do some additional operations, and makes things more complicated. > > >> } if (irq_src & MAX8997_IRQSRC_FUELGAUGE) { /* @@ -215,8 +203,9 @@ >> static irqreturn_t max8997_irq_thread(int irq, void *data) } if >> (irq_src & MAX8997_IRQSRC_MUIC) { /* MUIC INT1 ~ INT3 */ - >> max8997_bulk_read(max8997->muic, MAX8997_MUIC_REG_INT1, 3, - >> &irq_reg[MUIC_INT1]); + for (i = 0; i < 3; ++i) + >> regmap_read(max8997->regmap_muic, + MAX8997_MUIC_REG_INT1+i, >> &irq_reg[MUIC_INT1+i]); > > Same as above - bulk. > >> } if (irq_src & MAX8997_IRQSRC_GPIO) { /* GPIO Interrupt */ @@ >> -225,8 +214,8 @@ static irqreturn_t max8997_irq_thread(int irq, >> void *data) irq_reg[GPIO_LOW] = 0; irq_reg[GPIO_HI] = 0; >> >> - max8997_bulk_read(max8997->i2c, MAX8997_REG_GPIOCNTL1, - >> MAX8997_NUM_GPIO, gpio_info); + regmap_bulk_read(max8997->regmap, >> MAX8997_REG_GPIOCNTL1, + gpio_info, MAX8997_NUM_GPIO); for (i = >> 0; i < MAX8997_NUM_GPIO; i++) { bool interrupt = false; >> >> @@ -260,7 +249,7 @@ static irqreturn_t max8997_irq_thread(int irq, >> void *data) } if (irq_src & MAX8997_IRQSRC_FLASH) { /* Flash Status >> Interrupt */ - ret = max8997_read_reg(max8997->i2c, >> MAX8997_REG_FLASHSTATUS, + ret = regmap_read(max8997->regmap, >> MAX8997_REG_FLASHSTATUS, &irq_reg[FLASH_STATUS]); } >> >> @@ -312,7 +301,7 @@ int max8997_irq_init(struct max8997_dev >> *max8997) struct irq_domain *domain; int i; int ret; - u8 val; + >> unsigned int val; >> >> if (!max8997->irq) { dev_warn(max8997->dev, "No interrupt >> specified.\n"); @@ -323,22 +312,19 @@ int max8997_irq_init(struct >> max8997_dev *max8997) >> >> /* Mask individual interrupt sources */ for (i = 0; i < >> MAX8997_IRQ_GROUP_NR; i++) { - struct i2c_client *i2c; - >> max8997->irq_masks_cur[i] = 0xff; max8997->irq_masks_cache[i] = >> 0xff; - i2c = get_i2c(max8997, i); >> >> - if (IS_ERR_OR_NULL(i2c)) + if (IS_ERR_OR_NULL(max8997->regmap)) >> continue; if (max8997_mask_reg[i] == MAX8997_REG_INVALID) >> continue; >> >> - max8997_write_reg(i2c, max8997_mask_reg[i], 0xff); + >> regmap_write(max8997->regmap, max8997_mask_reg[i], 0xff); } >> >> for (i = 0; i < MAX8997_NUM_GPIO; i++) { - max8997->gpio_status[i] >> = (max8997_read_reg(max8997->i2c, + max8997->gpio_status[i] = >> (regmap_read(max8997->regmap, MAX8997_REG_GPIOCNTL1 + i, &val) & >> MAX8997_GPIO_DATA_MASK) ? diff --git a/drivers/mfd/max8997.c >> b/drivers/mfd/max8997.c index 5adede0..ca6f310 100644 --- >> a/drivers/mfd/max8997.c +++ b/drivers/mfd/max8997.c @@ -33,6 +33,7 >> @@ #include #include >> #include +#include >> >> #define I2C_ADDR_PMIC (0xCC >> 1) #define I2C_ADDR_MUIC (0x4A >> 1) >> @@ -57,81 +58,29 @@ static struct of_device_id >> max8997_pmic_dt_match[] = { }; #endif >> >> -int max8997_read_reg(struct i2c_client *i2c, u8 reg, u8 *dest) -{ >> - struct max8997_dev *max8997 = i2c_get_clientdata(i2c); - int >> ret; - - mutex_lock(&max8997->iolock); - ret = >> i2c_smbus_read_byte_data(i2c, reg); - >> mutex_unlock(&max8997->iolock); - if (ret < 0) - return ret; - - >> ret &= 0xff; - *dest = ret; - return 0; -} >> -EXPORT_SYMBOL_GPL(max8997_read_reg); - -int >> max8997_bulk_read(struct i2c_client *i2c, u8 reg, int count, u8 >> *buf) -{ - struct max8997_dev *max8997 = i2c_get_clientdata(i2c); >> - int ret; - - mutex_lock(&max8997->iolock); - ret = >> i2c_smbus_read_i2c_block_data(i2c, reg, count, buf); - >> mutex_unlock(&max8997->iolock); - if (ret < 0) - return ret; - - >> return 0; -} -EXPORT_SYMBOL_GPL(max8997_bulk_read); - -int >> max8997_write_reg(struct i2c_client *i2c, u8 reg, u8 value) -{ - >> struct max8997_dev *max8997 = i2c_get_clientdata(i2c); - int ret; - >> - mutex_lock(&max8997->iolock); - ret = >> i2c_smbus_write_byte_data(i2c, reg, value); - >> mutex_unlock(&max8997->iolock); - return ret; -} >> -EXPORT_SYMBOL_GPL(max8997_write_reg); - -int >> max8997_bulk_write(struct i2c_client *i2c, u8 reg, int count, u8 >> *buf) -{ - struct max8997_dev *max8997 = i2c_get_clientdata(i2c); >> - int ret; +static const struct regmap_config max8997_regmap_config >> = { + .reg_bits = 8, + .val_bits = 8, + .max_register = >> MAX8997_REG_PMIC_END, +}; >> >> - mutex_lock(&max8997->iolock); - ret = >> i2c_smbus_write_i2c_block_data(i2c, reg, count, buf); - >> mutex_unlock(&max8997->iolock); - if (ret < 0) - return ret; >> +static const struct regmap_config max8997_regmap_rtc_config = { + >> .reg_bits = 8, + .val_bits = 8, + .max_register = >> MAX8997_RTC_REG_END, +}; >> >> - return 0; -} -EXPORT_SYMBOL_GPL(max8997_bulk_write); +static >> const struct regmap_config max8997_regmap_haptic_config = { + >> .reg_bits = 8, + .val_bits = 8, + .max_register = >> MAX8997_HAPTIC_REG_END, +}; >> >> -int max8997_update_reg(struct i2c_client *i2c, u8 reg, u8 val, u8 >> mask) -{ - struct max8997_dev *max8997 = i2c_get_clientdata(i2c); - >> int ret; - - mutex_lock(&max8997->iolock); - ret = >> i2c_smbus_read_byte_data(i2c, reg); - if (ret >= 0) { - u8 >> old_val = ret & 0xff; - u8 new_val = (val & mask) | (old_val & >> (~mask)); - ret = i2c_smbus_write_byte_data(i2c, reg, new_val); - } >> - mutex_unlock(&max8997->iolock); - return ret; -} >> -EXPORT_SYMBOL_GPL(max8997_update_reg); +static const struct >> regmap_config max8997_regmap_muic_config = { + .reg_bits = 8, + >> .val_bits = 8, + .max_register = MAX8997_MUIC_REG_END, +}; >> >> /* * Only the common platform data elements for max8997 are parsed >> here from the @@ -209,11 +158,48 @@ static int >> max8997_i2c_probe(struct i2c_client *i2c, >> >> max8997->rtc = i2c_new_dummy(i2c->adapter, I2C_ADDR_RTC); >> i2c_set_clientdata(max8997->rtc, max8997); + max8997->haptic = >> i2c_new_dummy(i2c->adapter, I2C_ADDR_HAPTIC); >> i2c_set_clientdata(max8997->haptic, max8997); + max8997->muic = >> i2c_new_dummy(i2c->adapter, I2C_ADDR_MUIC); >> i2c_set_clientdata(max8997->muic, max8997); >> >> + max8997->regmap = devm_regmap_init_i2c(i2c, >> &max8997_regmap_config); + if (IS_ERR(max8997->regmap)) { + ret = >> PTR_ERR(max8997->regmap); + dev_err(max8997->dev, + "failed to >> allocate register map: %d\n", ret); + return ret; + } + + >> max8997->regmap_rtc = devm_regmap_init_i2c(max8997->rtc, + >> &max8997_regmap_rtc_config); + if (IS_ERR(max8997->regmap_rtc)) { >> + ret = PTR_ERR(max8997->regmap_rtc); + dev_err(max8997->dev, + >> "failed to allocate register map: %d\n", ret); + goto err_regmap; >> + } + + max8997->regmap_haptic = >> devm_regmap_init_i2c(max8997->haptic, + >> &max8997_regmap_haptic_config); + if >> (IS_ERR(max8997->regmap_haptic)) { + ret = >> PTR_ERR(max8997->regmap_haptic); + dev_err(max8997->dev, + "failed >> to allocate register map: %d\n", ret); + goto err_regmap; + } + + >> max8997->regmap_muic = devm_regmap_init_i2c(max8997->muic, + >> &max8997_regmap_muic_config); + if (IS_ERR(max8997->regmap_muic)) { >> + ret = PTR_ERR(max8997->regmap_muic); + dev_err(max8997->dev, + >> "failed to allocate register map: %d\n", ret); + goto err_regmap; >> + } + pm_runtime_set_active(max8997->dev); >> >> max8997_irq_init(max8997); @@ -238,6 +224,7 @@ static int >> max8997_i2c_probe(struct i2c_client *i2c, >> >> err_mfd: mfd_remove_devices(max8997->dev); +err_regmap: >> i2c_unregister_device(max8997->muic); >> i2c_unregister_device(max8997->haptic); >> i2c_unregister_device(max8997->rtc); @@ -423,15 +410,15 @@ static >> int max8997_freeze(struct device *dev) int i; >> >> for (i = 0; i < ARRAY_SIZE(max8997_dumpaddr_pmic); i++) - >> max8997_read_reg(i2c, max8997_dumpaddr_pmic[i], + >> regmap_read(max8997->regmap, max8997_dumpaddr_pmic[i], >> &max8997->reg_dump[i]); >> >> for (i = 0; i < ARRAY_SIZE(max8997_dumpaddr_muic); i++) - >> max8997_read_reg(i2c, max8997_dumpaddr_muic[i], + >> regmap_read(max8997->regmap_muic, max8997_dumpaddr_muic[i], >> &max8997->reg_dump[i + MAX8997_REG_PMIC_END]); >> >> for (i = 0; i < ARRAY_SIZE(max8997_dumpaddr_haptic); i++) - >> max8997_read_reg(i2c, max8997_dumpaddr_haptic[i], + >> regmap_read(max8997->regmap_haptic, max8997_dumpaddr_haptic[i], >> &max8997->reg_dump[i + MAX8997_REG_PMIC_END + >> MAX8997_MUIC_REG_END]); > > Code looks good. Idea for another patch: could bulk read be used > here? At least some of registers have continuous addresses so maybe > its worth to read them at once? It's problematic, because address ranges of dumped registers are not continuous. So it would need to call regmap_bulk_read in loop, which gives a small gain and complicates the code. I think there is no purpose to do it. It's better and keep it simple. >> >> @@ -445,15 +432,15 @@ static int max8997_restore(struct device >> *dev) int i; >> >> for (i = 0; i < ARRAY_SIZE(max8997_dumpaddr_pmic); i++) - >> max8997_write_reg(i2c, max8997_dumpaddr_pmic[i], + >> regmap_write(max8997->regmap, max8997_dumpaddr_pmic[i], >> max8997->reg_dump[i]); >> >> for (i = 0; i < ARRAY_SIZE(max8997_dumpaddr_muic); i++) - >> max8997_write_reg(i2c, max8997_dumpaddr_muic[i], + >> regmap_write(max8997->regmap_muic, max8997_dumpaddr_muic[i], >> max8997->reg_dump[i + MAX8997_REG_PMIC_END]); >> >> for (i = 0; i < ARRAY_SIZE(max8997_dumpaddr_haptic); i++) - >> max8997_write_reg(i2c, max8997_dumpaddr_haptic[i], + >> regmap_write(max8997->regmap_haptic, max8997_dumpaddr_haptic[i], >> max8997->reg_dump[i + MAX8997_REG_PMIC_END + >> MAX8997_MUIC_REG_END]); > > As above - bulk write (if you still would like to improve things in > this driver)? > Thanks, Robert