From: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
To: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Cc: Lee Jones <lee.jones@linaro.org>, Mark Brown <broonie@kernel.org>,
Mike Turquette <mturquette@linaro.org>,
Liam Girdwood <lgirdwood@gmail.com>,
Alessandro Zummo <a.zummo@towertech.it>,
Kukjin Kim <kgene.kim@samsung.com>,
Doug Anderson <dianders@chromium.org>,
Olof Johansson <olof@lixom.net>,
Tomeu Vizoso <tomeu.vizoso@collabora.com>,
Yadwinder Singh Brar <yadi.brar01@gmail.com>,
Tushar Behera <trblinux@gmail.com>,
Andreas Farber <afaerber@suse.de>,
linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v6 17/23] mfd: max77686: Add Maxim 77802 PMIC support
Date: Fri, 04 Jul 2014 13:35:56 +0200 [thread overview]
Message-ID: <53B6919C.7050502@collabora.co.uk> (raw)
In-Reply-To: <1404473434.14069.10.camel@AMDC1943>
Hello Krzysztof,
On 07/04/2014 01:30 PM, Krzysztof Kozlowski wrote:
> On pią, 2014-07-04 at 11:55 +0200, Javier Martinez Canillas wrote:
>> Maxim MAX77802 is a power management chip that contains 10 high
>> efficiency Buck regulators, 32 Low-dropout (LDO) regulators used
>> to power up application processors and peripherals, a 2-channel
>> 32kHz clock outputs, a Real-Time-Clock (RTC) and a I2C interface
>> to program the individual regulators, clocks outputs and the RTC.
>>
>> This patch adds support for MAX77802 to the MAX77686 driver and is
>> based on a driver added to the Chrome OS kernel 3.8 by Simon Glass.
>>
>> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>> ---
>>
>> NOTE: I didn't carry previous {Review,Acked,Tested}-by tags since
>> this patch extending MAX77686 is quite different than the old one
>> adding a new mfd driver. So review and test is highly appreciated.
>>
>> Changes since v5:
>> - Extend the 77686 driver to support 77802 instead of adding a new driver.
>> Suggested by Lee Jones.
>>
>> Changes since v4:
>> - Use consistent expressions when checking for NULL values.
>> Suggested by Krzysztof Kozlowski.
>> - Remove unused defines. Suggested by Krzysztof Kozlowski.
>> - Explain why IRQ is disabled on suspend. Suggested by Krzysztof Kozlowski.
>>
>> Changes since v3:
>> - Remove unnecessary OOM error message since the mm subsystem already logs it.
>>
>> Changes since v2:
>> - Split the DT binding docs in a separate patch and improve the documentation.
>> Suggested by Mark Brown.
>> - Add all the devices in the MFD driver instead of doing in separate patches.
>> Suggested by Mark Brown.
>>
>> Changes since v1:
>> - Convert max77{686,802} to regmap irq API and get rid of max77{686,802}-irq.c
>> Suggested by Krzysztof Kozlowski.
>> - Don't protect max77802 mfd_cells using Kconfig options since mfd core omits
>> devices that don't match. Suggested by Lee Jones.
>> - Change mfd driver to be tristate instead of boolean. Suggested by Mark Brown.
>> - Change binding "voltage-regulators" property to "regulators" to be consistent
>> with other PMIC drivers. Suggested by Mark Brown.
>> - Use regulators node names instead of the deprecated "regulator-compatible"
>> property. Suggested by Mark Brown.
>> - Use the new descriptor-based GPIO interface instead of the deprecated
>> ---
>> drivers/mfd/Kconfig | 6 +-
>> drivers/mfd/max77686.c | 187 ++++++++++++++++++++++++++-----
>> include/linux/mfd/max77686-private.h | 208 ++++++++++++++++++++++++++++++++++-
>> include/linux/mfd/max77686.h | 60 +++++++++-
>> 4 files changed, 428 insertions(+), 33 deletions(-)
>>
>
> Three comments below, after fixing:
> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>
> (...)
>
>> @@ -55,6 +123,17 @@ static struct regmap_config max77686_rtc_regmap_config = {
>> .val_bits = 8,
>> };
>>
>> +static struct regmap_config max77802_regmap_config = {
>> + .reg_bits = 8,
>> + .val_bits = 8,
>> + .writeable_reg = max77802_is_accessible_reg,
>> + .readable_reg = max77802_is_accessible_reg,
>> + .precious_reg = max77802_is_precious_reg,
>> + .volatile_reg = max77802_is_volatile_reg,
>> + .name = "max77802-pmic",
>> + .cache_type = REGCACHE_RBTREE,
>> +};
>> +
>> static const struct regmap_irq max77686_irqs[] = {
>> /* INT1 interrupts */
>> { .reg_offset = 0, .mask = MAX77686_INT1_PWRONF_MSK, },
>> @@ -98,9 +177,34 @@ static const struct regmap_irq_chip max77686_rtc_irq_chip = {
>> .num_irqs = ARRAY_SIZE(max77686_rtc_irqs),
>> };
>>
>> +static const struct regmap_irq_chip max77802_irq_chip = {
>> + .name = "max77802-pmic",
>> + .status_base = MAX77802_REG_INT1,
>> + .mask_base = MAX77802_REG_INT1MSK,
>> + .num_regs = 2,
>> + .irqs = max77686_irqs, /* same masks than 77686 */
>
> "same masks as 77686"
>
Ok
>> + .num_irqs = ARRAY_SIZE(max77686_irqs),
>> +};
>> +
>> +static const struct regmap_irq_chip max77802_rtc_irq_chip = {
>> + .name = "max77802-rtc",
>> + .status_base = MAX77802_RTC_INT,
>> + .mask_base = MAX77802_RTC_INTM,
>> + .num_regs = 1,
>> + .irqs = max77686_rtc_irqs, /* same masks than 77686 */
>
> Ditto
>
Ok
>> + .num_irqs = ARRAY_SIZE(max77686_rtc_irqs),
>> +};
>> +
>> static const struct of_device_id max77686_pmic_dt_match[] = {
>> - {.compatible = "maxim,max77686", .data = NULL},
>> - {},
>> + {
>> + .compatible = "maxim,max77686",
>> + .data = (void *)TYPE_MAX77686,
>> + },
>> + {
>> + .compatible = "maxim,max77802",
>> + .data = (void *)TYPE_MAX77802,
>> + },
>> + { },
>> };
>>
>> static void max77686_dt_parse_dvs_gpio(struct device *dev)
>> @@ -236,6 +340,12 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
>> struct max77686_platform_data *pdata = dev_get_platdata(&i2c->dev);
>> unsigned int data;
>> int ret = 0;
>> + const struct regmap_config *config;
>> + const struct regmap_irq_chip *irq_chip;
>> + const struct regmap_irq_chip *rtc_irq_chip;
>> + struct regmap **rtc_regmap;
>> + const struct mfd_cell *cells;
>> + int n_devs;
>>
>> if (IS_ENABLED(CONFIG_OF) && i2c->dev.of_node && !pdata)
>> pdata = max77686_i2c_parse_dt_pdata(&i2c->dev);
>> @@ -258,56 +368,77 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
>> max77686->wakeup = pdata->wakeup;
>> max77686->irq = i2c->irq;
>>
>> - max77686->regmap = devm_regmap_init_i2c(i2c, &max77686_regmap_config);
>> + if (max77686->type == TYPE_MAX77686) {
>> + config = &max77686_regmap_config;
>> + irq_chip = &max77686_irq_chip;
>> + rtc_irq_chip = &max77686_rtc_irq_chip;
>> + rtc_regmap = &max77686->rtc_regmap;
>> + cells = max77686_devs;
>> + n_devs = ARRAY_SIZE(max77686_devs);
>> + } else {
>> + config = &max77802_regmap_config;
>> + irq_chip = &max77802_irq_chip;
>> + rtc_irq_chip = &max77802_rtc_irq_chip;
>> + rtc_regmap = &max77686->regmap;
>> + cells = max77802_devs;
>> + n_devs = ARRAY_SIZE(max77802_devs);
>> + }
>> +
>> + max77686->regmap = devm_regmap_init_i2c(i2c, config);
>> if (IS_ERR(max77686->regmap)) {
>> ret = PTR_ERR(max77686->regmap);
>> dev_err(max77686->dev, "Failed to allocate register map: %d\n",
>> - ret);
>> + ret);
>> return ret;
>> }
>>
>> ret = regmap_read(max77686->regmap, MAX77686_REG_DEVICE_ID, &data);
>> if (ret < 0) {
>> dev_err(max77686->dev,
>> - "device not found on this channel (this is not an error)\n");
>> - return -ENODEV;
>> - }
>> -
>> - max77686->rtc = i2c_new_dummy(i2c->adapter, I2C_ADDR_RTC);
>> - if (!max77686->rtc) {
>> - dev_err(max77686->dev, "Failed to allocate I2C device for RTC\n");
>> + "device not found on this channel\n");
>> return -ENODEV;
>> }
>> - i2c_set_clientdata(max77686->rtc, max77686);
>>
>> - max77686->rtc_regmap = devm_regmap_init_i2c(max77686->rtc,
>> - &max77686_rtc_regmap_config);
>> - if (IS_ERR(max77686->rtc_regmap)) {
>> - ret = PTR_ERR(max77686->rtc_regmap);
>> - dev_err(max77686->dev, "failed to allocate RTC regmap: %d\n",
>> - ret);
>> - goto err_unregister_i2c;
>> + if (max77686->type == TYPE_MAX77686) {
>> + max77686->rtc = i2c_new_dummy(i2c->adapter, I2C_ADDR_RTC);
>> + if (!max77686->rtc) {
>> + dev_err(max77686->dev,
>> + "Failed to allocate I2C device for RTC\n");
>> + return -ENODEV;
>> + }
>> + i2c_set_clientdata(max77686->rtc, max77686);
>> +
>> + max77686->rtc_regmap =
>> + devm_regmap_init_i2c(max77686->rtc,
>> + &max77686_rtc_regmap_config);
>> + if (IS_ERR(max77686->rtc_regmap)) {
>> + ret = PTR_ERR(max77686->rtc_regmap);
>> + dev_err(max77686->dev,
>> + "failed to allocate RTC regmap: %d\n",
>> + ret);
>> + goto err_unregister_i2c;
>> + }
>> }
>>
>> ret = regmap_add_irq_chip(max77686->regmap, max77686->irq,
>> IRQF_TRIGGER_FALLING | IRQF_ONESHOT |
>> - IRQF_SHARED, 0, &max77686_irq_chip,
>> + IRQF_SHARED, 0, irq_chip,
>> &max77686->irq_data);
>> if (ret) {
>> dev_err(&i2c->dev, "failed to add PMIC irq chip: %d\n", ret);
>> goto err_unregister_i2c;
>> }
>> - ret = regmap_add_irq_chip(max77686->rtc_regmap, max77686->irq,
>> +
>> + ret = regmap_add_irq_chip(*rtc_regmap, max77686->irq,
>> IRQF_TRIGGER_FALLING | IRQF_ONESHOT |
>> - IRQF_SHARED, 0, &max77686_rtc_irq_chip,
>> + IRQF_SHARED, 0, rtc_irq_chip,
>> &max77686->rtc_irq_data);
>> if (ret) {
>> dev_err(&i2c->dev, "failed to add RTC irq chip: %d\n", ret);
>> goto err_del_irqc;
>> }
>>
>> - ret = mfd_add_devices(max77686->dev, -1, max77686_devs,
>> - ARRAY_SIZE(max77686_devs), NULL, 0, NULL);
>> + ret = mfd_add_devices(max77686->dev, -1, cells, n_devs, NULL, 0, NULL);
>> if (ret < 0) {
>> dev_err(&i2c->dev, "failed to add MFD devices: %d\n", ret);
>> goto err_del_rtc_irqc;
>> @@ -320,7 +451,8 @@ err_del_rtc_irqc:
>> err_del_irqc:
>> regmap_del_irq_chip(max77686->irq, max77686->irq_data);
>> err_unregister_i2c:
>> - i2c_unregister_device(max77686->rtc);
>> + if (max77686->type == TYPE_MAX77686)
>> + i2c_unregister_device(max77686->rtc);
>>
>> return ret;
>> }
>> @@ -341,6 +473,7 @@ static int max77686_i2c_remove(struct i2c_client *i2c)
>
> In remove() you should unregister dummy RTC I2C device only on MAX77686
> (77802 does not add it).
>
Yes, I added the check to the probe()'s error path but forgot about the remove()
function. Thanks a lot for pointing this out.
> Best regards,
> Krzysztof
>
>
Best regards,
Javier
next prev parent reply other threads:[~2014-07-04 11:35 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-07-04 9:54 [PATCH v6 00/23] Add Maxim 77802 PMIC support Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 01/23] mfd: max77686: Convert to use regmap_irq Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 02/23] mfd: max77686: Add power management support Javier Martinez Canillas
2014-07-04 10:51 ` Krzysztof Kozlowski
2014-07-04 9:55 ` [PATCH v6 03/23] mfd: max77686: Don't define dummy function if OF isn't enabled Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 04/23] mfd: max77686: Make platform data over-rule DT Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 05/23] mfd: max77686: Return correct error when pdata isn't found Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 06/23] mfd: max77686: Make error checking consistent Javier Martinez Canillas
2014-07-04 10:54 ` Krzysztof Kozlowski
2014-07-04 9:55 ` [PATCH v6 07/23] mfd: max77686: Remove unneeded OOM error message Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 08/23] mfd: max77686: Add Dynamic Voltage Scaling (DVS) support Javier Martinez Canillas
2014-07-04 11:15 ` Krzysztof Kozlowski
2014-07-04 11:32 ` Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 09/23] rtc: max77686: Allow the max77686 rtc to wakeup the system Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 10/23] clk: max77686: Add DT include for MAX77686 PMIC clock Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 11/23] clk: Add generic driver for Maxim PMIC clocks Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 12/23] clk: max77686: Convert to the generic max clock driver Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 13/23] clk: max77686: Improve Maxim 77686 PMIC clocks binding Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 14/23] regmap: Add regmap_reg_copy function Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 15/23] regulator: max77686: Setup DVS-related GPIOs on probe Javier Martinez Canillas
2014-07-10 10:08 ` amit daniel kachhap
[not found] ` <CADGdYn4EURaHinWVksQSPzVr=CkZYcrqvWsO7mKDz_gY9XB__Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2014-07-10 13:25 ` Lee Jones
2014-07-11 2:03 ` Javier Martinez Canillas
2014-07-11 9:31 ` amit daniel kachhap
2014-07-04 9:55 ` [PATCH v6 16/23] mfd: max77686: Add documentation for DVS bindings Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 17/23] mfd: max77686: Add Maxim 77802 PMIC support Javier Martinez Canillas
2014-07-04 11:30 ` Krzysztof Kozlowski
2014-07-04 11:35 ` Javier Martinez Canillas [this message]
2014-07-04 9:55 ` [PATCH v6 18/23] mfd: max77802: Add DT binding documentation Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 19/23] regulator: Add driver for Maxim 77802 PMIC regulators Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 20/23] clk: Add driver for Maxim 77802 PMIC clocks Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 21/23] clk: max77802: Add DT binding documentation Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 22/23] rtc: Add driver for Maxim 77802 PMIC Real-Time-Clock Javier Martinez Canillas
2014-07-04 11:56 ` Krzysztof Kozlowski
2014-07-04 12:52 ` Javier Martinez Canillas
2014-07-04 13:11 ` Krzysztof Kozlowski
2014-07-04 13:23 ` Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 23/23] ARM: dts: Add max77802 to exynos5420-peach-pit and exynos5800-peach-pi Javier Martinez Canillas
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=53B6919C.7050502@collabora.co.uk \
--to=javier.martinez@collabora.co.uk \
--cc=a.zummo@towertech.it \
--cc=afaerber@suse.de \
--cc=broonie@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=dianders@chromium.org \
--cc=k.kozlowski@samsung.com \
--cc=kgene.kim@samsung.com \
--cc=lee.jones@linaro.org \
--cc=lgirdwood@gmail.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-samsung-soc@vger.kernel.org \
--cc=mturquette@linaro.org \
--cc=olof@lixom.net \
--cc=tomeu.vizoso@collabora.com \
--cc=trblinux@gmail.com \
--cc=yadi.brar01@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).