Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1307159 > unrolled thread
| Started by | Laxman Dewangan <ldewangan@nvidia.com> |
|---|---|
| First post | 2016-01-12 10:40 +0100 |
| Last post | 2016-01-13 11:20 +0100 |
| Articles | 16 — 5 participants |
Back to article view | Back to linux.kernel
[PATCH V2 0/6] Add support for MAXIM MAX77620/MAX20024 PMIC Laxman Dewangan <ldewangan@nvidia.com> - 2016-01-12 10:40 +0100
[PATCH V2 4/6] gpio: max77620: add gpio driver for MAX77620/MAX20024 Laxman Dewangan <ldewangan@nvidia.com> - 2016-01-12 10:40 +0100
[PATCH V2 5/6] rtc: max77xxx: add RTC driver for Maxim MAX77xxx series RTC IP Laxman Dewangan <ldewangan@nvidia.com> - 2016-01-12 10:40 +0100
Re: [PATCH V2 5/6] rtc: max77xxx: add RTC driver for Maxim MAX77xxx series RTC IP Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-01-13 01:10 +0100
Re: [PATCH V2 5/6] rtc: max77xxx: add RTC driver for Maxim MAX77xxx series RTC IP Laxman Dewangan <ldewangan@nvidia.com> - 2016-01-13 05:20 +0100
Re: [PATCH V2 5/6] rtc: max77xxx: add RTC driver for Maxim MAX77xxx series RTC IP Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-01-13 05:30 +0100
Re: [PATCH V2 5/6] rtc: max77xxx: add RTC driver for Maxim MAX77xxx series RTC IP Laxman Dewangan <ldewangan@nvidia.com> - 2016-01-13 05:40 +0100
Re: [PATCH V2 5/6] rtc: max77xxx: add RTC driver for Maxim MAX77xxx series RTC IP Laxman Dewangan <ldewangan@nvidia.com> - 2016-01-13 16:20 +0100
Re: [PATCH V2 5/6] rtc: max77xxx: add RTC driver for Maxim MAX77xxx series RTC IP Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-01-14 02:00 +0100
Re: [PATCH V2 5/6] rtc: max77xxx: add RTC driver for Maxim MAX77xxx series RTC IP Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-01-14 02:30 +0100
Re: [PATCH V2 5/6] rtc: max77xxx: add RTC driver for Maxim MAX77xxx series RTC IP Laxman Dewangan <ldewangan@nvidia.com> - 2016-01-14 13:10 +0100
Re: [PATCH V2 5/6] rtc: max77xxx: add RTC driver for Maxim MAX77xxx series RTC IP Javier Martinez Canillas <javier@dowhile0.org> - 2016-01-15 03:00 +0100
Re: [PATCH V2 5/6] rtc: max77xxx: add RTC driver for Maxim MAX77xxx series RTC IP Mark Brown <broonie@kernel.org> - 2016-01-14 15:30 +0100
Re: [PATCH V2 2/6] mfd: max77620: add core driver for MAX77620/MAX20024 Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-01-13 01:50 +0100
Re: [PATCH V2 2/6] mfd: max77620: add core driver for MAX77620/MAX20024 Laxman Dewangan <ldewangan@nvidia.com> - 2016-01-13 10:20 +0100
Re: [PATCH V2 2/6] mfd: max77620: add core driver for MAX77620/MAX20024 Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2016-01-13 11:20 +0100
| From | Laxman Dewangan <ldewangan@nvidia.com> |
|---|---|
| Date | 2016-01-12 10:40 +0100 |
| Subject | [PATCH V2 0/6] Add support for MAXIM MAX77620/MAX20024 PMIC |
| Message-ID | <qQ3r4-7nB-9@gated-at.bofh.it> |
Add SW support for MAXIM Semiconductor's Power Management IC (PMIC) MAX77620/MAX20024. This PMIC supports DC-DC/LDOS, GPIOs, RTC, watchdog, clocks etc. This series add respective driver for each of sub-modules. --- Changes from V1: DT DOC: - Added units in some of properties. - Change the boolean property to tristate type and detail some of properties. RTC: - Rename the file to rtc-max77xxx.c and make the generic implementation. - Direct regmap apis are used for the register access. - Decouped from max77620 driver. - Taken care of cleanup comments form V1 version. MFD: - Code cleanups per review from V1. - Move register acccess APIs from header to c file. - Remove some of non required variable, remove duplication in error message and simplify some of function implementation. GPIO: - Use the gpiochip_add_data and get the chip data from core APIs. - Cleanups based on comment received on mfd/rtc. - Avoid duplication on error message. Pinctrl: - Cleanup code based on comment received on mfd/rtc. - Avoid duplication on error message. Regulators: - Cleanup code based on comment received on mfd/rtc. - Avoid duplication on error message. - Taken care of review comment from Mark. Laxman Dewangan (6): DT: mfd: add device-tree binding doc fro PMIC max77620/max20024 mfd: max77620: add core driver for MAX77620/MAX20024 pinctrl: max77620: add pincontrol driver for MAX77620/MAX20024 gpio: max77620: add gpio driver for MAX77620/MAX20024 rtc: max77xxx: add RTC driver for Maxim MAX77xxx series RTC IP regulator: max77620: add regulator driver for max77620/max20024 Documentation/devicetree/bindings/mfd/max77620.txt | 397 ++++++++ drivers/gpio/Kconfig | 9 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-max77620.c | 306 +++++++ drivers/mfd/Kconfig | 15 + drivers/mfd/Makefile | 1 + drivers/mfd/max77620.c | 997 +++++++++++++++++++++ drivers/pinctrl/Kconfig | 10 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-max77620.c | 705 +++++++++++++++ drivers/regulator/Kconfig | 9 + drivers/regulator/Makefile | 1 + drivers/regulator/max77620-regulator.c | 991 ++++++++++++++++++++ drivers/rtc/Kconfig | 10 + drivers/rtc/Makefile | 1 + drivers/rtc/rtc-max77xxx.c | 500 +++++++++++ include/dt-bindings/mfd/max77620.h | 38 + include/linux/mfd/max77620.h | 407 +++++++++ 18 files changed, 4399 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/max77620.txt create mode 100644 drivers/gpio/gpio-max77620.c create mode 100644 drivers/mfd/max77620.c create mode 100644 drivers/pinctrl/pinctrl-max77620.c create mode 100644 drivers/regulator/max77620-regulator.c create mode 100644 drivers/rtc/rtc-max77xxx.c create mode 100644 include/dt-bindings/mfd/max77620.h create mode 100644 include/linux/mfd/max77620.h -- 2.1.4
[toc] | [next] | [standalone]
| From | Laxman Dewangan <ldewangan@nvidia.com> |
|---|---|
| Date | 2016-01-12 10:40 +0100 |
| Subject | [PATCH V2 4/6] gpio: max77620: add gpio driver for MAX77620/MAX20024 |
| Message-ID | <qQ3AL-7rn-33@gated-at.bofh.it> |
| In reply to | #1307159 |
MAXIM Semiconductor's PMIC, MAX77620/MAX20024 has 8 GPIO
pins. It also supports interrupts from these pins.
Add GPIO driver for these pins to control via GPIO APIs.
Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
Signed-off-by: Chaitanya Bandi <bandik@nvidia.com>
---
- Use the gpiochip_add_data and get the chip data from core APIs.
- Cleanups based on comment received on mfd/rtc.
- Avoid duplication on error message.
drivers/gpio/Kconfig | 9 ++
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-max77620.c | 306 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 316 insertions(+)
create mode 100644 drivers/gpio/gpio-max77620.c
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index f2b7160..b96a80c 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -797,6 +797,15 @@ config GPIO_LP3943
LP3943 can be used as a GPIO expander which provides up to 16 GPIOs.
Open drain outputs are required for this usage.
+config GPIO_MAX77620
+ bool "GPIO support for PMIC MAX77620 and MAX20024"
+ depends on MFD_MAX77620
+ help
+ GPIO driver for MAX77620 and MAX20024 PMIC from Maxim Semiconductor.
+ MAX77620 PMIC has 8 pins that can be configured as GPIOs. The
+ driver also provides interrupt support for each of the gpios.
+ Say yes here to enable the max77620 to be used as gpio controller.
+
config GPIO_MSIC
bool "Intel MSIC mixed signal gpio support"
depends on MFD_INTEL_MSIC
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index ece7d7c..f676a2d 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -57,6 +57,7 @@ obj-$(CONFIG_GPIO_MAX730X) += gpio-max730x.o
obj-$(CONFIG_GPIO_MAX7300) += gpio-max7300.o
obj-$(CONFIG_GPIO_MAX7301) += gpio-max7301.o
obj-$(CONFIG_GPIO_MAX732X) += gpio-max732x.o
+obj-$(CONFIG_GPIO_MAX77620) += gpio-max77620.o
obj-$(CONFIG_GPIO_MB86S7X) += gpio-mb86s7x.o
obj-$(CONFIG_GPIO_MC33880) += gpio-mc33880.o
obj-$(CONFIG_GPIO_MC9S08DZ60) += gpio-mc9s08dz60.o
diff --git a/drivers/gpio/gpio-max77620.c b/drivers/gpio/gpio-max77620.c
new file mode 100644
index 0000000..3185e6a
--- /dev/null
+++ b/drivers/gpio/gpio-max77620.c
@@ -0,0 +1,306 @@
+/*
+ * MAXIM MAX77620 GPIO driver
+ *
+ * Copyright (c) 2016, NVIDIA CORPORATION. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ */
+
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/gpio.h>
+#include <linux/platform_device.h>
+#include <linux/mfd/max77620.h>
+
+#define GPIO_REG_ADDR(offset) (MAX77620_REG_GPIO0 + offset)
+
+struct max77620_gpio {
+ struct gpio_chip gpio_chip;
+ struct device *parent;
+ struct device *dev;
+ int gpio_irq;
+ int irq_base;
+ int gpio_base;
+};
+
+static const struct regmap_irq max77620_gpio_irqs[] = {
+ [MAX77620_IRQ_GPIO0 - MAX77620_IRQ_GPIO0] = {
+ .mask = MAX77620_IRQ_LVL2_GPIO_EDGE0,
+ .reg_offset = 0,
+ },
+ [MAX77620_IRQ_GPIO1 - MAX77620_IRQ_GPIO0] = {
+ .mask = MAX77620_IRQ_LVL2_GPIO_EDGE1,
+ .reg_offset = 0,
+ },
+ [MAX77620_IRQ_GPIO2 - MAX77620_IRQ_GPIO0] = {
+ .mask = MAX77620_IRQ_LVL2_GPIO_EDGE2,
+ .reg_offset = 0,
+ },
+ [MAX77620_IRQ_GPIO3 - MAX77620_IRQ_GPIO0] = {
+ .mask = MAX77620_IRQ_LVL2_GPIO_EDGE3,
+ .reg_offset = 0,
+ },
+ [MAX77620_IRQ_GPIO4 - MAX77620_IRQ_GPIO0] = {
+ .mask = MAX77620_IRQ_LVL2_GPIO_EDGE4,
+ .reg_offset = 0,
+ },
+ [MAX77620_IRQ_GPIO5 - MAX77620_IRQ_GPIO0] = {
+ .mask = MAX77620_IRQ_LVL2_GPIO_EDGE5,
+ .reg_offset = 0,
+ },
+ [MAX77620_IRQ_GPIO6 - MAX77620_IRQ_GPIO0] = {
+ .mask = MAX77620_IRQ_LVL2_GPIO_EDGE6,
+ .reg_offset = 0,
+ },
+ [MAX77620_IRQ_GPIO7 - MAX77620_IRQ_GPIO0] = {
+ .mask = MAX77620_IRQ_LVL2_GPIO_EDGE7,
+ .reg_offset = 0,
+ },
+};
+
+static struct regmap_irq_chip max77620_gpio_irq_chip = {
+ .name = "max77620-gpio",
+ .irqs = max77620_gpio_irqs,
+ .num_irqs = ARRAY_SIZE(max77620_gpio_irqs),
+ .num_regs = 1,
+ .irq_reg_stride = 1,
+ .status_base = MAX77620_REG_IRQ_LVL2_GPIO,
+};
+
+static int max77620_gpio_dir_input(struct gpio_chip *gc, unsigned offset)
+{
+ struct max77620_gpio *mgpio = gpiochip_get_data(gc);
+ int ret;
+
+ ret = max77620_reg_update(mgpio->parent, MAX77620_PWR_SLAVE,
+ GPIO_REG_ADDR(offset), MAX77620_CNFG_GPIO_DIR_MASK,
+ MAX77620_CNFG_GPIO_DIR_INPUT);
+ if (ret < 0)
+ dev_err(mgpio->dev, "CNFG_GPIOx dir update failed: %d\n", ret);
+
+ return ret;
+}
+
+static int max77620_gpio_get(struct gpio_chip *gc, unsigned offset)
+{
+ struct max77620_gpio *mgpio = gpiochip_get_data(gc);
+ u8 val;
+ int ret;
+
+ ret = max77620_reg_read(mgpio->parent, MAX77620_PWR_SLAVE,
+ GPIO_REG_ADDR(offset), &val);
+ if (ret < 0) {
+ dev_err(mgpio->dev, "CNFG_GPIOx read failed: %d\n", ret);
+ return ret;
+ }
+
+ return !!(val & MAX77620_CNFG_GPIO_INPUT_VAL_MASK);
+}
+
+static int max77620_gpio_dir_output(struct gpio_chip *gc, unsigned offset,
+ int value)
+{
+ struct max77620_gpio *mgpio = gpiochip_get_data(gc);
+ u8 val;
+ int ret;
+
+ val = (value) ? MAX77620_CNFG_GPIO_OUTPUT_VAL_HIGH :
+ MAX77620_CNFG_GPIO_OUTPUT_VAL_LOW;
+
+ ret = max77620_reg_update(mgpio->parent, MAX77620_PWR_SLAVE,
+ GPIO_REG_ADDR(offset),
+ MAX77620_CNFG_GPIO_OUTPUT_VAL_MASK, val);
+ if (ret < 0) {
+ dev_err(mgpio->dev, "CNFG_GPIOx val update failed: %d\n", ret);
+ return ret;
+ }
+
+ ret = max77620_reg_update(mgpio->parent, MAX77620_PWR_SLAVE,
+ GPIO_REG_ADDR(offset), MAX77620_CNFG_GPIO_DIR_MASK,
+ MAX77620_CNFG_GPIO_DIR_OUTPUT);
+ if (ret < 0)
+ dev_err(mgpio->dev, "CNFG_GPIOx dir update failed: %d\n", ret);
+
+ return ret;
+}
+
+static int max77620_gpio_set_debounce(struct gpio_chip *gc,
+ unsigned offset, unsigned debounce)
+{
+ struct max77620_gpio *mgpio = gpiochip_get_data(gc);
+ u8 val;
+ int ret;
+
+ switch (debounce) {
+ case 0:
+ val = MAX77620_CNFG_GPIO_DBNC_None;
+ break;
+ case 1 ... 8:
+ val = MAX77620_CNFG_GPIO_DBNC_8ms;
+ break;
+ case 9 ... 16:
+ val = MAX77620_CNFG_GPIO_DBNC_16ms;
+ break;
+ case 17 ... 32:
+ val = MAX77620_CNFG_GPIO_DBNC_32ms;
+ break;
+ default:
+ dev_err(mgpio->dev, "Illegal value %u\n", debounce);
+ return -EINVAL;
+ }
+
+ ret = max77620_reg_update(mgpio->parent, MAX77620_PWR_SLAVE,
+ GPIO_REG_ADDR(offset), MAX77620_CNFG_GPIO_DBNC_MASK, val);
+ if (ret < 0)
+ dev_err(mgpio->dev, "CNFG_GPIOx_DBNC update failed: %d\n", ret);
+
+ return ret;
+}
+
+static void max77620_gpio_set(struct gpio_chip *gc, unsigned offset,
+ int value)
+{
+ struct max77620_gpio *mgpio = gpiochip_get_data(gc);
+ u8 val;
+ int ret;
+
+ val = (value) ? MAX77620_CNFG_GPIO_OUTPUT_VAL_HIGH :
+ MAX77620_CNFG_GPIO_OUTPUT_VAL_LOW;
+
+ ret = max77620_reg_update(mgpio->parent, MAX77620_PWR_SLAVE,
+ GPIO_REG_ADDR(offset),
+ MAX77620_CNFG_GPIO_OUTPUT_VAL_MASK, val);
+ if (ret < 0)
+ dev_err(mgpio->dev, "CNFG_GPIO_OUT update failed: %d\n", ret);
+}
+
+static int max77620_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
+{
+ struct max77620_gpio *mgpio = gpiochip_get_data(gc);
+ struct max77620_chip *chip = dev_get_drvdata(mgpio->dev->parent);
+
+ return regmap_irq_get_virq(chip->gpio_irq_data, offset);
+}
+
+static void max77620_gpio_irq_remove(struct max77620_gpio *mgpio)
+{
+ struct max77620_chip *chip = dev_get_drvdata(mgpio->dev->parent);
+
+ regmap_del_irq_chip(mgpio->gpio_irq, chip->gpio_irq_data);
+ chip->gpio_irq_data = NULL;
+}
+
+static int max77620_gpio_probe(struct platform_device *pdev)
+{
+ struct max77620_gpio *mgpio;
+ struct max77620_chip *chip = dev_get_drvdata(pdev->dev.parent);
+ int ret;
+ int gpio_irq;
+
+ gpio_irq = platform_get_irq(pdev, 0);
+ if (gpio_irq <= 0) {
+ dev_err(&pdev->dev, "Gpio irq not available %d\n", gpio_irq);
+ return -ENODEV;
+ }
+
+ mgpio = devm_kzalloc(&pdev->dev, sizeof(*mgpio), GFP_KERNEL);
+ if (!mgpio)
+ return -ENOMEM;
+
+ mgpio->parent = pdev->dev.parent;
+ mgpio->dev = &pdev->dev;
+ mgpio->gpio_irq = gpio_irq;
+
+ mgpio->gpio_chip.label = pdev->name;
+ mgpio->gpio_chip.parent = &pdev->dev;
+ mgpio->gpio_chip.direction_input = max77620_gpio_dir_input;
+ mgpio->gpio_chip.get = max77620_gpio_get;
+ mgpio->gpio_chip.direction_output = max77620_gpio_dir_output;
+ mgpio->gpio_chip.set_debounce = max77620_gpio_set_debounce;
+ mgpio->gpio_chip.set = max77620_gpio_set;
+ mgpio->gpio_chip.to_irq = max77620_gpio_to_irq;
+ mgpio->gpio_chip.ngpio = MAX77620_GPIO_NR;
+ mgpio->gpio_chip.can_sleep = 1;
+ mgpio->gpio_chip.base = -1;
+ mgpio->irq_base = -1;
+#ifdef CONFIG_OF_GPIO
+ mgpio->gpio_chip.of_node = pdev->dev.parent->of_node;
+#endif
+
+ platform_set_drvdata(pdev, mgpio);
+
+ ret = gpiochip_add_data(&mgpio->gpio_chip, mgpio);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "gpio_init: Failed to add max77620_gpio\n");
+ return ret;
+ }
+ mgpio->gpio_base = mgpio->gpio_chip.base;
+
+ ret = regmap_add_irq_chip(chip->rmap[MAX77620_PWR_SLAVE],
+ mgpio->gpio_irq, IRQF_ONESHOT | IRQF_EARLY_RESUME,
+ mgpio->irq_base,
+ &max77620_gpio_irq_chip, &chip->gpio_irq_data);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "Failed to add gpio irq_chip %d\n", ret);
+ goto fail;
+ }
+
+ return 0;
+
+fail:
+ gpiochip_remove(&mgpio->gpio_chip);
+
+ return ret;
+}
+
+static int max77620_gpio_remove(struct platform_device *pdev)
+{
+ struct max77620_gpio *mgpio = platform_get_drvdata(pdev);
+
+ max77620_gpio_irq_remove(mgpio);
+ gpiochip_remove(&mgpio->gpio_chip);
+
+ return 0;
+}
+
+static const struct platform_device_id max77620_gpio_devtype[] = {
+ {
+ .name = "max77620-gpio",
+ },
+ {
+ .name = "max20024-gpio",
+ },
+};
+
+static struct platform_driver max77620_gpio_driver = {
+ .driver.name = "max77620-gpio",
+ .driver.owner = THIS_MODULE,
+ .probe = max77620_gpio_probe,
+ .remove = max77620_gpio_remove,
+ .id_table = max77620_gpio_devtype,
+};
+
+static int __init max77620_gpio_init(void)
+{
+ return platform_driver_register(&max77620_gpio_driver);
+}
+subsys_initcall(max77620_gpio_init);
+
+static void __exit max77620_gpio_exit(void)
+{
+ platform_driver_unregister(&max77620_gpio_driver);
+}
+module_exit(max77620_gpio_exit);
+
+MODULE_DESCRIPTION("GPIO interface for MAX77620 and MAX20024 PMIC");
+MODULE_AUTHOR("Laxman Dewangan <ldewangan@nvidia.com>");
+MODULE_AUTHOR("Chaitanya Bandi <bandik@nvidia.com>");
+MODULE_ALIAS("platform:max77620-gpio");
+MODULE_LICENSE("GPL v2");
--
2.1.4
[toc] | [prev] | [next] | [standalone]
| From | Laxman Dewangan <ldewangan@nvidia.com> |
|---|---|
| Date | 2016-01-12 10:40 +0100 |
| Subject | [PATCH V2 5/6] rtc: max77xxx: add RTC driver for Maxim MAX77xxx series RTC IP |
| Message-ID | <qQ3AM-7rn-41@gated-at.bofh.it> |
| In reply to | #1307159 |
Maxim Semiconductor's PMIC MAX77620, MAX77686, MAX20024 have
same RTC IP on these PMICs.
Add generic MAX77xxxx series RTC driver which can be used as
RTC driver for these PMIC and avoids duplication of RTC driver
for each PMICs. Their MFD driver can be different here.
Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
Changes from V1:
- Rename the file to rtc-max77xxx.c and make the generic implementation.
- Direct regmap apis are used for the register access.
- Decouped from max77620 driver.
- Taken care of cleanup comments form V1 version.
drivers/rtc/Kconfig | 10 +
drivers/rtc/Makefile | 1 +
drivers/rtc/rtc-max77xxx.c | 500 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 511 insertions(+)
create mode 100644 drivers/rtc/rtc-max77xxx.c
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 376322f..4972dd5 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -315,6 +315,16 @@ config RTC_DRV_MAX8997
This driver can also be built as a module. If so, the module
will be called rtc-max8997.
+config RTC_DRV_MAX77XXX
+ tristate "Maxim MAX77XXX series generic RTC driver"
+ help
+ If you say yes here you will get support for the generic RTC driver
+ for Maxim Semiconductor MAX77XXX series of PMIC like MAX77620.
+ This also supports the RTC driver for Maxim PMIC MaX20024 which
+ is almost same as MAX77620.
+ This driver can also be built as a module. If so, the module
+ will be called rtc-max77xxx.
+
config RTC_DRV_MAX77686
tristate "Maxim MAX77686"
depends on MFD_MAX77686
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 62d61b2..e5aba30 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -85,6 +85,7 @@ obj-$(CONFIG_RTC_DRV_M48T59) += rtc-m48t59.o
obj-$(CONFIG_RTC_DRV_M48T86) += rtc-m48t86.o
obj-$(CONFIG_RTC_DRV_MAX6900) += rtc-max6900.o
obj-$(CONFIG_RTC_DRV_MAX6902) += rtc-max6902.o
+obj-$(CONFIG_RTC_DRV_MAX77XXX) += rtc-max77xxx.o
obj-$(CONFIG_RTC_DRV_MAX77686) += rtc-max77686.o
obj-$(CONFIG_RTC_DRV_MAX77802) += rtc-max77802.o
obj-$(CONFIG_RTC_DRV_MAX8907) += rtc-max8907.o
diff --git a/drivers/rtc/rtc-max77xxx.c b/drivers/rtc/rtc-max77xxx.c
new file mode 100644
index 0000000..c5ada63
--- /dev/null
+++ b/drivers/rtc/rtc-max77xxx.c
@@ -0,0 +1,500 @@
+/*
+ * Max77xxx(MAX77620, MAX77686 etc) RTC driver
+ *
+ * Copyright (c) 2016, NVIDIA CORPORATION. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/regmap.h>
+#include <linux/rtc.h>
+
+/* RTC Registers */
+#define MAX77XXX_REG_RTCINT 0x00
+#define MAX77XXX_REG_RTCINTM 0x01
+#define MAX77XXX_REG_RTCCNTLM 0x02
+#define MAX77XXX_REG_RTCCNTL 0x03
+#define MAX77XXX_REG_RTCUPDATE0 0x04
+#define MAX77XXX_REG_RTCUPDATE1 0x05
+#define MAX77XXX_REG_RTCSMPL 0x06
+#define MAX77XXX_REG_RTCSEC 0x07
+#define MAX77XXX_REG_RTCMIN 0x08
+#define MAX77XXX_REG_RTCHOUR 0x09
+#define MAX77XXX_REG_RTCDOW 0x0A
+#define MAX77XXX_REG_RTCMONTH 0x0B
+#define MAX77XXX_REG_RTCYEAR 0x0C
+#define MAX77XXX_REG_RTCDOM 0x0D
+#define MAX77XXX_REG_RTCSECA1 0x0E
+#define MAX77XXX_REG_RTCMINA1 0x0F
+#define MAX77XXX_REG_RTCHOURA1 0x10
+#define MAX77XXX_REG_RTCDOWA1 0x11
+#define MAX77XXX_REG_RTCMONTHA1 0x12
+#define MAX77XXX_REG_RTCYEARA1 0x13
+#define MAX77XXX_REG_RTCDOMA1 0x14
+#define MAX77XXX_REG_RTCSECA2 0x15
+#define MAX77XXX_REG_RTCMINA2 0x16
+#define MAX77XXX_REG_RTCHOURA2 0x17
+#define MAX77XXX_REG_RTCDOWA2 0x18
+#define MAX77XXX_REG_RTCMONTHA2 0x19
+#define MAX77XXX_REG_RTCYEARA2 0x1A
+#define MAX77XXX_REG_RTCDOMA2 0x1B
+
+#define MAX77XXX_RTC60S_MASK BIT(0)
+#define MAX77XXX_RTCA1_MASK BIT(1)
+#define MAX77XXX_RTCA2_MASK BIT(2)
+#define MAX77XXX_RTC_SMPL_MASK BIT(3)
+#define MAX77XXX_RTC_RTC1S_MASK BIT(4)
+#define MAX77XXX_RTC_ALL_IRQ_MASK 0x1F
+
+#define MAX77XXX_BCDM_MASK BIT(0)
+#define MAX77XXX_HRMODEM_MASK BIT(1)
+
+#define WB_UPDATE_MASK BIT(0)
+#define FLAG_AUTO_CLEAR_MASK BIT(1)
+#define FREEZE_SEC_MASK BIT(2)
+#define RTC_WAKE_MASK BIT(3)
+#define RB_UPDATE_MASK BIT(4)
+
+#define MAX77XXX_UDF_MASK BIT(0)
+#define MAX77XXX_RBUDF_MASK BIT(1)
+
+#define SEC_MASK 0x7F
+#define MIN_MASK 0x7F
+#define HOUR_MASK 0x3F
+#define WEEKDAY_MASK 0x7F
+#define MONTH_MASK 0x1F
+#define YEAR_MASK 0xFF
+#define MONTHDAY_MASK 0x3F
+
+#define ALARM_EN_MASK 0x80
+#define ALARM_EN_SHIFT 7
+
+#define RTC_YEAR_BASE 100
+#define RTC_YEAR_MAX 99
+
+#define ONOFF_WK_ALARM1_MASK BIT(2)
+
+enum {
+ RTC_SEC,
+ RTC_MIN,
+ RTC_HOUR,
+ RTC_WEEKDAY,
+ RTC_MONTH,
+ RTC_YEAR,
+ RTC_MONTHDAY,
+ RTC_NR
+};
+
+struct max77xxx_rtc_info {
+ struct rtc_device *rtc;
+ struct device *dev;
+ struct regmap *rmap;
+ struct mutex io_lock;
+ int irq;
+ u8 irq_mask;
+};
+
+static int max77xxx_rtc_update_buffer(struct max77xxx_rtc_info *rinfo,
+ bool write)
+{
+ u8 val = FLAG_AUTO_CLEAR_MASK | RTC_WAKE_MASK;
+ int ret;
+
+ if (write)
+ val |= WB_UPDATE_MASK;
+ else
+ val |= RB_UPDATE_MASK;
+
+ ret = regmap_write(rinfo->rmap, MAX77XXX_REG_RTCUPDATE0, val);
+ if (ret < 0) {
+ dev_err(rinfo->dev, "Reg RTCUPDATE0 write failed: %d\n", ret);
+ return ret;
+ }
+
+ /* Must wait 16ms for buffer update */
+ usleep_range(16000, 17000);
+
+ return 0;
+}
+
+static int max77xxx_rtc_write(struct max77xxx_rtc_info *rinfo, u8 addr,
+ void *vals, u32 len)
+{
+ int ret;
+ int i;
+ u8 *src = vals;
+
+ mutex_lock(&rinfo->io_lock);
+ for (i = 0; i < len; ++i) {
+ ret = regmap_write(rinfo->rmap, addr + i, *src++);
+ if (ret < 0) {
+ dev_err(rinfo->dev, "Reg 0x%02x write failed: %d\n",
+ addr + i, ret);
+ goto out;
+ }
+ }
+ ret = max77xxx_rtc_update_buffer(rinfo, true);
+out:
+ mutex_unlock(&rinfo->io_lock);
+
+ return ret;
+}
+
+static int max77xxx_rtc_read(struct max77xxx_rtc_info *rinfo, u8 addr,
+ void *vals, u32 len, bool update_buffer)
+{
+ int ret;
+
+ mutex_lock(&rinfo->io_lock);
+ if (update_buffer) {
+ ret = max77xxx_rtc_update_buffer(rinfo, false);
+ if (ret < 0)
+ goto out;
+ }
+
+ ret = regmap_bulk_read(rinfo->rmap, addr, vals, len);
+ if (ret < 0)
+ dev_err(rinfo->dev, "Reg 0x%02x read failed: %d\n", addr, ret);
+out:
+ mutex_unlock(&rinfo->io_lock);
+
+ return ret;
+}
+
+static int max77xxx_rtc_reg_to_tm(struct max77xxx_rtc_info *rinfo, u8 *buf,
+ struct rtc_time *tm)
+{
+ int wday = buf[RTC_WEEKDAY] & WEEKDAY_MASK;
+
+ if (!wday) {
+ dev_err(rinfo->dev, "Invalid day of week, %d\n", wday);
+ return -EINVAL;
+ }
+
+ tm->tm_sec = (int)(buf[RTC_SEC] & SEC_MASK);
+ tm->tm_min = (int)(buf[RTC_MIN] & MIN_MASK);
+ tm->tm_hour = (int)(buf[RTC_HOUR] & HOUR_MASK);
+ tm->tm_mday = (int)(buf[RTC_MONTHDAY] & MONTHDAY_MASK);
+ tm->tm_mon = (int)(buf[RTC_MONTH] & MONTH_MASK) - 1;
+ tm->tm_year = (int)(buf[RTC_YEAR] & YEAR_MASK) + RTC_YEAR_BASE;
+ tm->tm_wday = ffs(wday) - 1;
+
+ return 0;
+}
+
+static int max77xxx_rtc_tm_to_reg(struct max77xxx_rtc_info *rinfo, u8 *buf,
+ struct rtc_time *tm, int alarm)
+{
+ u8 alarm_mask = alarm ? ALARM_EN_MASK : 0;
+
+ if ((tm->tm_year < RTC_YEAR_BASE) ||
+ (tm->tm_year > (RTC_YEAR_BASE + RTC_YEAR_MAX))) {
+ dev_err(rinfo->dev, "Invalid year, %d\n", tm->tm_year);
+ return -EINVAL;
+ }
+
+ buf[RTC_SEC] = tm->tm_sec | alarm_mask;
+ buf[RTC_MIN] = tm->tm_min | alarm_mask;
+ buf[RTC_HOUR] = tm->tm_hour | alarm_mask;
+ buf[RTC_MONTHDAY] = tm->tm_mday | alarm_mask;
+ buf[RTC_MONTH] = (tm->tm_mon + 1) | alarm_mask;
+ buf[RTC_YEAR] = (tm->tm_year - RTC_YEAR_BASE) | alarm_mask;
+
+ /* The wday is configured only when disabled alarm. */
+ buf[RTC_WEEKDAY] = (alarm) ? 0x01 : (1 << tm->tm_wday);
+
+ return 0;
+}
+
+static int max77xxx_rtc_irq_mask(struct max77xxx_rtc_info *rinfo, u8 irq)
+{
+ u8 irq_mask = rinfo->irq_mask | irq;
+ int ret;
+
+ ret = max77xxx_rtc_write(rinfo, MAX77XXX_REG_RTCINTM, &irq_mask, 1);
+ if (ret < 0)
+ return ret;
+ rinfo->irq_mask = irq_mask;
+
+ return ret;
+}
+
+static int max77xxx_rtc_irq_unmask(struct max77xxx_rtc_info *rinfo, u8 irq)
+{
+ u8 irq_mask = rinfo->irq_mask & ~irq;
+ int ret;
+
+ ret = max77xxx_rtc_write(rinfo, MAX77XXX_REG_RTCINTM, &irq_mask, 1);
+ if (ret < 0)
+ return ret;
+ rinfo->irq_mask = irq_mask;
+
+ return ret;
+}
+
+static int max77xxx_rtc_do_irq(struct max77xxx_rtc_info *rinfo)
+{
+ unsigned int irq_status;
+ int ret;
+
+ ret = regmap_read(rinfo->rmap, MAX77XXX_REG_RTCINT, &irq_status);
+ if (ret < 0) {
+ dev_err(rinfo->dev, "RTCINT read failed: %d\n", ret);
+ return ret;
+ }
+
+ if (!(rinfo->irq_mask & MAX77XXX_RTCA1_MASK) &&
+ (irq_status & MAX77XXX_RTCA1_MASK))
+ rtc_update_irq(rinfo->rtc, 1, RTC_IRQF | RTC_AF);
+
+ if (!(rinfo->irq_mask & MAX77XXX_RTC_RTC1S_MASK) &&
+ (irq_status & MAX77XXX_RTC_RTC1S_MASK))
+ rtc_update_irq(rinfo->rtc, 1, RTC_IRQF | RTC_UF);
+
+ return ret;
+}
+
+static irqreturn_t max77xxx_rtc_irq(int irq, void *data)
+{
+ struct max77xxx_rtc_info *rinfo = (struct max77xxx_rtc_info *)data;
+
+ max77xxx_rtc_do_irq(rinfo);
+
+ return IRQ_HANDLED;
+}
+
+static int max77xxx_rtc_alarm_irq_enable(struct device *dev,
+ unsigned int enabled)
+{
+ struct max77xxx_rtc_info *rinfo = dev_get_drvdata(dev);
+ int ret;
+
+ if (rinfo->irq < 0)
+ return -ENXIO;
+
+ /* Handle pending interrupt */
+ ret = max77xxx_rtc_do_irq(rinfo);
+ if (ret < 0)
+ return ret;
+
+ /* Configure alarm interrupt */
+ if (enabled)
+ ret = max77xxx_rtc_irq_unmask(rinfo, MAX77XXX_RTCA1_MASK);
+ else
+ ret = max77xxx_rtc_irq_mask(rinfo, MAX77XXX_RTCA1_MASK);
+
+ return ret;
+}
+
+static int max77xxx_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+ struct max77xxx_rtc_info *rinfo = dev_get_drvdata(dev);
+ u8 buf[RTC_NR];
+ int ret;
+
+ ret = max77xxx_rtc_read(rinfo, MAX77XXX_REG_RTCSEC, buf, RTC_NR, true);
+ if (ret < 0)
+ return ret;
+
+ return max77xxx_rtc_reg_to_tm(rinfo, buf, tm);
+}
+
+static int max77xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+ struct max77xxx_rtc_info *rinfo = dev_get_drvdata(dev);
+ u8 buf[RTC_NR];
+ int ret;
+
+ ret = max77xxx_rtc_tm_to_reg(rinfo, buf, tm, 0);
+ if (ret < 0)
+ return ret;
+
+ return max77xxx_rtc_write(rinfo, MAX77XXX_REG_RTCSEC, buf, RTC_NR);
+}
+
+static int max77xxx_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+ struct max77xxx_rtc_info *rinfo = dev_get_drvdata(dev);
+ u8 buf[RTC_NR];
+ int ret;
+
+ ret = max77xxx_rtc_read(rinfo, MAX77XXX_REG_RTCSECA1, buf, RTC_NR, 1);
+ if (ret < 0)
+ return ret;
+
+ buf[RTC_YEAR] &= ~ALARM_EN_MASK;
+ ret = max77xxx_rtc_reg_to_tm(rinfo, buf, &alrm->time);
+ if (ret < 0)
+ return ret;
+
+ alrm->enabled = (rinfo->irq_mask & MAX77XXX_RTCA1_MASK) ? 0 : 1;
+
+ return 0;
+}
+
+static int max77xxx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+ struct max77xxx_rtc_info *rinfo = dev_get_drvdata(dev);
+ u8 buf[RTC_NR];
+ int ret;
+
+ ret = max77xxx_rtc_tm_to_reg(rinfo, buf, &alrm->time, 1);
+ if (ret < 0)
+ return ret;
+
+ ret = max77xxx_rtc_write(rinfo, MAX77XXX_REG_RTCSECA1, buf, RTC_NR);
+ if (ret < 0)
+ return ret;
+
+ ret = max77xxx_rtc_alarm_irq_enable(dev, alrm->enabled);
+ if (ret < 0)
+ return ret;
+
+ return ret;
+}
+
+static const struct rtc_class_ops max77xxx_rtc_ops = {
+ .read_time = max77xxx_rtc_read_time,
+ .set_time = max77xxx_rtc_set_time,
+ .read_alarm = max77xxx_rtc_read_alarm,
+ .set_alarm = max77xxx_rtc_set_alarm,
+ .alarm_irq_enable = max77xxx_rtc_alarm_irq_enable,
+};
+
+static int max77xxx_rtc_preinit(struct max77xxx_rtc_info *rinfo)
+{
+ u8 val;
+ int ret;
+
+ /* Mask all interrupts */
+ rinfo->irq_mask = 0xFF;
+ ret = max77xxx_rtc_write(rinfo, MAX77XXX_REG_RTCINTM,
+ &rinfo->irq_mask, 1);
+ if (ret < 0)
+ return ret;
+
+ max77xxx_rtc_read(rinfo, MAX77XXX_REG_RTCINT, &val, 1, false);
+
+ /* Configure Binary mode and 24hour mode */
+ val = MAX77XXX_HRMODEM_MASK;
+ return max77xxx_rtc_write(rinfo, MAX77XXX_REG_RTCCNTL, &val, 1);
+}
+
+static int max77xxx_rtc_probe(struct platform_device *pdev)
+{
+ static struct max77xxx_rtc_info *rinfo;
+ int ret;
+
+ rinfo = devm_kzalloc(&pdev->dev, sizeof(*rinfo), GFP_KERNEL);
+ if (!rinfo)
+ return -ENOMEM;
+
+ dev_set_drvdata(&pdev->dev, rinfo);
+ rinfo->dev = &pdev->dev;
+ mutex_init(&rinfo->io_lock);
+ rinfo->rmap = dev_get_regmap(pdev->dev.parent, "rtc-slave");
+ if (!rinfo->rmap) {
+ dev_err(&pdev->dev, "Regmap for RTC device not found\n");
+ return -ENODEV;
+ }
+
+ ret = max77xxx_rtc_preinit(rinfo);
+ if (ret < 0)
+ goto fail_preinit;
+
+ device_init_wakeup(&pdev->dev, 1);
+
+ rinfo->rtc = devm_rtc_device_register(&pdev->dev, "max77xxx-rtc",
+ &max77xxx_rtc_ops, THIS_MODULE);
+ if (IS_ERR(rinfo->rtc)) {
+ ret = PTR_ERR(rinfo->rtc);
+ dev_err(&pdev->dev, "RTC registration failed: %d\n", ret);
+ goto fail_preinit;
+ }
+
+ rinfo->irq = platform_get_irq(pdev, 0);
+ ret = devm_request_threaded_irq(&pdev->dev, rinfo->irq, NULL,
+ max77xxx_rtc_irq, IRQF_ONESHOT, "max77xxx-rtc", rinfo);
+ if (ret < 0) {
+ dev_err(rinfo->dev, "Failed to request irq %d\n", rinfo->irq);
+ goto fail_preinit;
+ }
+
+ enable_irq_wake(rinfo->irq);
+
+ return 0;
+
+fail_preinit:
+ mutex_destroy(&rinfo->io_lock);
+
+ return ret;
+}
+
+static int max77xxx_rtc_remove(struct platform_device *pdev)
+{
+ struct max77xxx_rtc_info *rinfo = dev_get_drvdata(&pdev->dev);
+
+ mutex_destroy(&rinfo->io_lock);
+
+ return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int max77xxx_rtc_suspend(struct device *dev)
+{
+ struct max77xxx_rtc_info *rinfo = dev_get_drvdata(dev);
+
+ if (device_may_wakeup(dev))
+ enable_irq_wake(rinfo->irq);
+
+ return 0;
+}
+
+static int max77xxx_rtc_resume(struct device *dev)
+{
+ struct max77xxx_rtc_info *rinfo = dev_get_drvdata(dev);
+
+ if (device_may_wakeup(dev))
+ disable_irq_wake(rinfo->irq);
+
+ return 0;
+}
+#endif
+
+static const struct dev_pm_ops max77xxx_rtc_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(max77xxx_rtc_suspend, max77xxx_rtc_resume)
+};
+
+static const struct platform_device_id max77xxx_rtc_devtype[] = {
+ { .name = "max77xxx-rtc", },
+ { .name = "max77620-rtc", },
+ { .name = "max20024-rtc", },
+};
+
+static struct platform_driver max77xxx_rtc_driver = {
+ .probe = max77xxx_rtc_probe,
+ .remove = max77xxx_rtc_remove,
+ .id_table = max77xxx_rtc_devtype,
+ .driver = {
+ .name = "max77xxx-rtc",
+ .pm = &max77xxx_rtc_pm_ops,
+ },
+};
+
+module_platform_driver(max77xxx_rtc_driver);
+
+MODULE_DESCRIPTION("max77xxx RTC driver");
+MODULE_AUTHOR("Laxman Dewangan <ldewangan@nvidia.com>");
+MODULE_ALIAS("platform:max77xxx-rtc");
+MODULE_LICENSE("GPL v2");
--
2.1.4
[toc] | [prev] | [next] | [standalone]
| From | Krzysztof Kozlowski <k.kozlowski@samsung.com> |
|---|---|
| Date | 2016-01-13 01:10 +0100 |
| Subject | Re: [PATCH V2 5/6] rtc: max77xxx: add RTC driver for Maxim MAX77xxx series RTC IP |
| Message-ID | <qQhaG-8s7-17@gated-at.bofh.it> |
| In reply to | #1307164 |
On 12.01.2016 18:17, Laxman Dewangan wrote: > Maxim Semiconductor's PMIC MAX77620, MAX77686, MAX20024 have > same RTC IP on these PMICs. > > Add generic MAX77xxxx series RTC driver which can be used as > RTC driver for these PMIC and avoids duplication of RTC driver > for each PMICs. Their MFD driver can be different here. > > Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com> > --- > Changes from V1: > - Rename the file to rtc-max77xxx.c and make the generic implementation. > - Direct regmap apis are used for the register access. > - Decouped from max77620 driver. > - Taken care of cleanup comments form V1 version. > > drivers/rtc/Kconfig | 10 + > drivers/rtc/Makefile | 1 + > drivers/rtc/rtc-max77xxx.c | 500 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 511 insertions(+) > create mode 100644 drivers/rtc/rtc-max77xxx.c > > diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig > index 376322f..4972dd5 100644 > --- a/drivers/rtc/Kconfig > +++ b/drivers/rtc/Kconfig > @@ -315,6 +315,16 @@ config RTC_DRV_MAX8997 > This driver can also be built as a module. If so, the module > will be called rtc-max8997. > > +config RTC_DRV_MAX77XXX > + tristate "Maxim MAX77XXX series generic RTC driver" > + help > + If you say yes here you will get support for the generic RTC driver > + for Maxim Semiconductor MAX77XXX series of PMIC like MAX77620. > + This also supports the RTC driver for Maxim PMIC MaX20024 which > + is almost same as MAX77620. > + This driver can also be built as a module. If so, the module > + will be called rtc-max77xxx. > + That was not the consensus... You still added a new driver - but now with different name. That is useless duplication Please work with existing code. Use existing maxim RTC drivers: either max77686 or max77802. There is no need for new one. Best regards, Krzysztof
[toc] | [prev] | [next] | [standalone]
| From | Laxman Dewangan <ldewangan@nvidia.com> |
|---|---|
| Date | 2016-01-13 05:20 +0100 |
| Subject | Re: [PATCH V2 5/6] rtc: max77xxx: add RTC driver for Maxim MAX77xxx series RTC IP |
| Message-ID | <qQl4C-2Je-3@gated-at.bofh.it> |
| In reply to | #1307933 |
On Wednesday 13 January 2016 05:36 AM, Krzysztof Kozlowski wrote: > On 12.01.2016 18:17, Laxman Dewangan wrote: >> Maxim Semiconductor's PMIC MAX77620, MAX77686, MAX20024 have >> same RTC IP on these PMICs. >> >> Add generic MAX77xxxx series RTC driver which can be used as >> RTC driver for these PMIC and avoids duplication of RTC driver >> for each PMICs. Their MFD driver can be different here. >> >> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com> >> --- >> Changes from V1: >> - Rename the file to rtc-max77xxx.c and make the generic implementation. >> - Direct regmap apis are used for the register access. >> - Decouped from max77620 driver. >> - Taken care of cleanup comments form V1 version. >> >> drivers/rtc/Kconfig | 10 + >> drivers/rtc/Makefile | 1 + >> drivers/rtc/rtc-max77xxx.c | 500 +++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 511 insertions(+) >> create mode 100644 drivers/rtc/rtc-max77xxx.c >> >> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig >> index 376322f..4972dd5 100644 >> --- a/drivers/rtc/Kconfig >> +++ b/drivers/rtc/Kconfig >> @@ -315,6 +315,16 @@ config RTC_DRV_MAX8997 >> This driver can also be built as a module. If so, the module >> will be called rtc-max8997. >> >> +config RTC_DRV_MAX77XXX >> + tristate "Maxim MAX77XXX series generic RTC driver" >> + help >> + If you say yes here you will get support for the generic RTC driver >> + for Maxim Semiconductor MAX77XXX series of PMIC like MAX77620. >> + This also supports the RTC driver for Maxim PMIC MaX20024 which >> + is almost same as MAX77620. >> + This driver can also be built as a module. If so, the module >> + will be called rtc-max77xxx. >> + > That was not the consensus... You still added a new driver - but now > with different name. > > That is useless duplication > > Please work with existing code. Use existing maxim RTC drivers: either > max77686 or max77802. > > There is no need for new one. If we modify the existing one then that work will be outside of this series to make it independent. However, the file name does not suggest common in older file. Also this will require mfd and rtc driver changes to decouple it. Here is my approach: - Let's have common driver in implementation and file name. This will be independent of the mfd driver on all sense. - Once it is merged, move the max77686 and max77802 to use this driver, this will need the modification on the mfd driver, related defconfig file and if specific stuff needed in the rtc then addition of that. Per your approach: - Modify rtc max77686 and mfd driver max77686 to decouple and proper registration. - Add support of max77620 on the max77686 driver if any specific is required. That is also fine to me but still I am not comfortable with the config name and driver file name as this does not suggest the common.
[toc] | [prev] | [next] | [standalone]
| From | Krzysztof Kozlowski <k.kozlowski@samsung.com> |
|---|---|
| Date | 2016-01-13 05:30 +0100 |
| Subject | Re: [PATCH V2 5/6] rtc: max77xxx: add RTC driver for Maxim MAX77xxx series RTC IP |
| Message-ID | <qQleh-2NQ-5@gated-at.bofh.it> |
| In reply to | #1308029 |
On 13.01.2016 13:07, Laxman Dewangan wrote: > > On Wednesday 13 January 2016 05:36 AM, Krzysztof Kozlowski wrote: >> On 12.01.2016 18:17, Laxman Dewangan wrote: >>> Maxim Semiconductor's PMIC MAX77620, MAX77686, MAX20024 have >>> same RTC IP on these PMICs. >>> >>> Add generic MAX77xxxx series RTC driver which can be used as >>> RTC driver for these PMIC and avoids duplication of RTC driver >>> for each PMICs. Their MFD driver can be different here. >>> >>> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com> >>> --- >>> Changes from V1: >>> - Rename the file to rtc-max77xxx.c and make the generic implementation. >>> - Direct regmap apis are used for the register access. >>> - Decouped from max77620 driver. >>> - Taken care of cleanup comments form V1 version. >>> >>> drivers/rtc/Kconfig | 10 + >>> drivers/rtc/Makefile | 1 + >>> drivers/rtc/rtc-max77xxx.c | 500 >>> +++++++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 511 insertions(+) >>> create mode 100644 drivers/rtc/rtc-max77xxx.c >>> >>> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig >>> index 376322f..4972dd5 100644 >>> --- a/drivers/rtc/Kconfig >>> +++ b/drivers/rtc/Kconfig >>> @@ -315,6 +315,16 @@ config RTC_DRV_MAX8997 >>> This driver can also be built as a module. If so, the module >>> will be called rtc-max8997. >>> +config RTC_DRV_MAX77XXX >>> + tristate "Maxim MAX77XXX series generic RTC driver" >>> + help >>> + If you say yes here you will get support for the generic RTC >>> driver >>> + for Maxim Semiconductor MAX77XXX series of PMIC like MAX77620. >>> + This also supports the RTC driver for Maxim PMIC MaX20024 which >>> + is almost same as MAX77620. >>> + This driver can also be built as a module. If so, the module >>> + will be called rtc-max77xxx. >>> + >> That was not the consensus... You still added a new driver - but now >> with different name. >> >> That is useless duplication >> >> Please work with existing code. Use existing maxim RTC drivers: either >> max77686 or max77802. >> >> There is no need for new one. > > If we modify the existing one then that work will be outside of this > series to make it independent. And that is the problem? The series evolve. The ultimate goal is to support max77686, max77802, max77620 and max20024. > > However, the file name does not suggest common in older file. This is not a sensible argument. The name does not matter. But if really needed we can rename it... > Also this > will require mfd and rtc driver changes to decouple it. Yes, decouple everything! I like it! :) Make it robust, generic, readable, fix bugs etc. :) > > Here is my approach: > - Let's have common driver in implementation and file name. This will be > independent of the mfd driver on all sense. > - Once it is merged, move the max77686 and max77802 to use this driver, > this will need the modification on the mfd driver, related defconfig > file and if specific stuff needed in the rtc then addition of that. Nope, because *the second part won't happen*. Never. After merging you will be happy and another duplicated stuff ends in the kernel. Fix things before merging. Not after. > > Per your approach: > - Modify rtc max77686 and mfd driver max77686 to decouple and proper > registration. > - Add support of max77620 on the max77686 driver if any specific is > required. That is the way we usually extend the drivers for new devices. > > That is also fine to me but still I am not comfortable with the config > name and driver file name as this does not suggest the common. The name does not matter. Really. We have a lot of drivers with a specific device-like name and supporting different devices. To point that your argument is invalid - your initial name of driver "rtc-max77620.c" supported totally different "names": the max77620 and max20024. It also wasn't suggesting something "common"... With my approach we are not developing common think neither. We just want to extend/re-use existing max77686 (or max77802) driver for new devices. Just like everywhere else. Best regards, Krzysztof
[toc] | [prev] | [next] | [standalone]
| From | Laxman Dewangan <ldewangan@nvidia.com> |
|---|---|
| Date | 2016-01-13 05:40 +0100 |
| Subject | Re: [PATCH V2 5/6] rtc: max77xxx: add RTC driver for Maxim MAX77xxx series RTC IP |
| Message-ID | <qQlnY-2S1-27@gated-at.bofh.it> |
| In reply to | #1308034 |
On Wednesday 13 January 2016 09:58 AM, Krzysztof Kozlowski wrote: > On 13.01.2016 13:07, Laxman Dewangan wrote: >> On Wednesday 13 January 2016 05:36 AM, Krzysztof Kozlowski wrote: > >> That is also fine to me but still I am not comfortable with the config >> name and driver file name as this does not suggest the common. > The name does not matter. Really. We have a lot of drivers with a > specific device-like name and supporting different devices. To point > that your argument is invalid - your initial name of driver > "rtc-max77620.c" supported totally different "names": the max77620 and > max20024. It also wasn't suggesting something "common"... In all config string, I have mentioned the MAX20024. > With my approach we are not developing common think neither. We just > want to extend/re-use existing max77686 (or max77802) driver for new > devices. Just like everywhere else. OK, fine to me if this is acceptable. I will drop this rtc patch form this series and work in max77686 driver to modify first and once merged, use this config on my defconfig.
[toc] | [prev] | [next] | [standalone]
| From | Laxman Dewangan <ldewangan@nvidia.com> |
|---|---|
| Date | 2016-01-13 16:20 +0100 |
| Subject | Re: [PATCH V2 5/6] rtc: max77xxx: add RTC driver for Maxim MAX77xxx series RTC IP |
| Message-ID | <qQvnk-1xE-23@gated-at.bofh.it> |
| In reply to | #1308043 |
On Wednesday 13 January 2016 09:55 AM, Laxman Dewangan wrote:
>
> On Wednesday 13 January 2016 09:58 AM, Krzysztof Kozlowski wrote:
>> On 13.01.2016 13:07, Laxman Dewangan wrote:
>>> On Wednesday 13 January 2016 05:36 AM, Krzysztof Kozlowski wrote:
>>
>>> That is also fine to me but still I am not comfortable with the config
>>> name and driver file name as this does not suggest the common.
>> The name does not matter. Really. We have a lot of drivers with a
>> specific device-like name and supporting different devices. To point
>> that your argument is invalid - your initial name of driver
>> "rtc-max77620.c" supported totally different "names": the max77620 and
>> max20024. It also wasn't suggesting something "common"...
> In all config string, I have mentioned the MAX20024.
>
>
>> With my approach we are not developing common think neither. We just
>> want to extend/re-use existing max77686 (or max77802) driver for new
>> devices. Just like everywhere else.
>
> OK, fine to me if this is acceptable.
> I will drop this rtc patch form this series and work in max77686
> driver to modify first and once merged, use this config on my defconfig.
>
Here, MAX686 RTC driver needs two regmap handle, one for the rtc_regmap
and other for STATUS2 register access.
ret = regmap_read(info->max77686->regmap, MAX77686_REG_STATUS2,
&val);
if (ret < 0) {
dev_err(info->dev, "%s:%d fail to read status2 reg(%d)\n",
__func__, __LINE__, ret);
goto out;
}
We can not have two regmap handle on rtc driver as both regmap (pmic and
rtc) registered with different i2c device.
Also this register should not be accessed by RTC driver if we want to
decouple as this is very much MAX77686 register set.
Do we need this code?
static int max77686_rtc_read_alarm(struct device *dev, struct rtc_wkalrm
*alrm)
{
struct max77686_rtc_info *info = dev_get_drvdata(dev);
u8 data[RTC_NR_TIME];
unsigned int val;
int i, ret;
::::::::
alrm->pending = 0;
ret = regmap_read(info->max77686->regmap, MAX77686_REG_STATUS2,
&val);
if (ret < 0) {
dev_err(info->dev, "%s:%d fail to read status2 reg(%d)\n",
__func__, __LINE__, ret);
goto out;
}
if (val & (1 << 4)) /* RTCA1 */
alrm->pending = 1;
}
[toc] | [prev] | [next] | [standalone]
| From | Krzysztof Kozlowski <k.kozlowski@samsung.com> |
|---|---|
| Date | 2016-01-14 02:00 +0100 |
| Subject | Re: [PATCH V2 5/6] rtc: max77xxx: add RTC driver for Maxim MAX77xxx series RTC IP |
| Message-ID | <qQEqC-7BF-5@gated-at.bofh.it> |
| In reply to | #1308497 |
On 13.01.2016 23:59, Laxman Dewangan wrote:
>
> On Wednesday 13 January 2016 09:55 AM, Laxman Dewangan wrote:
>>
>> On Wednesday 13 January 2016 09:58 AM, Krzysztof Kozlowski wrote:
>>> On 13.01.2016 13:07, Laxman Dewangan wrote:
>>>> On Wednesday 13 January 2016 05:36 AM, Krzysztof Kozlowski wrote:
>>>
>>>> That is also fine to me but still I am not comfortable with the config
>>>> name and driver file name as this does not suggest the common.
>>> The name does not matter. Really. We have a lot of drivers with a
>>> specific device-like name and supporting different devices. To point
>>> that your argument is invalid - your initial name of driver
>>> "rtc-max77620.c" supported totally different "names": the max77620 and
>>> max20024. It also wasn't suggesting something "common"...
>> In all config string, I have mentioned the MAX20024.
>>
>>
>>> With my approach we are not developing common think neither. We just
>>> want to extend/re-use existing max77686 (or max77802) driver for new
>>> devices. Just like everywhere else.
>>
>> OK, fine to me if this is acceptable.
>> I will drop this rtc patch form this series and work in max77686
>> driver to modify first and once merged, use this config on my defconfig.
>>
>
> Here, MAX686 RTC driver needs two regmap handle, one for the rtc_regmap
> and other for STATUS2 register access.
>
> ret = regmap_read(info->max77686->regmap, MAX77686_REG_STATUS2,
> &val);
> if (ret < 0) {
> dev_err(info->dev, "%s:%d fail to read status2 reg(%d)\n",
> __func__, __LINE__, ret);
> goto out;
> }
>
>
> We can not have two regmap handle on rtc driver as both regmap (pmic and
> rtc) registered with different i2c device.
>
> Also this register should not be accessed by RTC driver if we want to
> decouple as this is very much MAX77686 register set.
> Do we need this code?
> static int max77686_rtc_read_alarm(struct device *dev, struct rtc_wkalrm
> *alrm)
> {
> struct max77686_rtc_info *info = dev_get_drvdata(dev);
> u8 data[RTC_NR_TIME];
> unsigned int val;
> int i, ret;
>
> ::::::::
> alrm->pending = 0;
> ret = regmap_read(info->max77686->regmap, MAX77686_REG_STATUS2,
> &val);
> if (ret < 0) {
> dev_err(info->dev, "%s:%d fail to read status2 reg(%d)\n",
> __func__, __LINE__, ret);
> goto out;
> }
>
> if (val & (1 << 4)) /* RTCA1 */
> alrm->pending = 1;
>
> }
Most of the drivers set the 'pending' field when reading alarm. Your
original driver did not.
The max77802 does exactly the same (BTW, these should be merged as
well... I'll add this to the TODO list) so I think this is necessary.
BR,
Krzysztof
[toc] | [prev] | [next] | [standalone]
| From | Krzysztof Kozlowski <k.kozlowski@samsung.com> |
|---|---|
| Date | 2016-01-14 02:30 +0100 |
| Subject | Re: [PATCH V2 5/6] rtc: max77xxx: add RTC driver for Maxim MAX77xxx series RTC IP |
| Message-ID | <qQETE-83s-17@gated-at.bofh.it> |
| In reply to | #1308920 |
On 14.01.2016 09:50, Krzysztof Kozlowski wrote:
> On 13.01.2016 23:59, Laxman Dewangan wrote:
>>
>> On Wednesday 13 January 2016 09:55 AM, Laxman Dewangan wrote:
>>>
>>> On Wednesday 13 January 2016 09:58 AM, Krzysztof Kozlowski wrote:
>>>> On 13.01.2016 13:07, Laxman Dewangan wrote:
>>>>> On Wednesday 13 January 2016 05:36 AM, Krzysztof Kozlowski wrote:
>>>>
>>>>> That is also fine to me but still I am not comfortable with the config
>>>>> name and driver file name as this does not suggest the common.
>>>> The name does not matter. Really. We have a lot of drivers with a
>>>> specific device-like name and supporting different devices. To point
>>>> that your argument is invalid - your initial name of driver
>>>> "rtc-max77620.c" supported totally different "names": the max77620 and
>>>> max20024. It also wasn't suggesting something "common"...
>>> In all config string, I have mentioned the MAX20024.
>>>
>>>
>>>> With my approach we are not developing common think neither. We just
>>>> want to extend/re-use existing max77686 (or max77802) driver for new
>>>> devices. Just like everywhere else.
>>>
>>> OK, fine to me if this is acceptable.
>>> I will drop this rtc patch form this series and work in max77686
>>> driver to modify first and once merged, use this config on my defconfig.
>>>
>>
>> Here, MAX686 RTC driver needs two regmap handle, one for the rtc_regmap
>> and other for STATUS2 register access.
>>
>> ret = regmap_read(info->max77686->regmap, MAX77686_REG_STATUS2,
>> &val);
>> if (ret < 0) {
>> dev_err(info->dev, "%s:%d fail to read status2 reg(%d)\n",
>> __func__, __LINE__, ret);
>> goto out;
>> }
>>
>>
>> We can not have two regmap handle on rtc driver as both regmap (pmic and
>> rtc) registered with different i2c device.
>>
>> Also this register should not be accessed by RTC driver if we want to
>> decouple as this is very much MAX77686 register set.
>> Do we need this code?
>> static int max77686_rtc_read_alarm(struct device *dev, struct rtc_wkalrm
>> *alrm)
>> {
>> struct max77686_rtc_info *info = dev_get_drvdata(dev);
>> u8 data[RTC_NR_TIME];
>> unsigned int val;
>> int i, ret;
>>
>> ::::::::
>> alrm->pending = 0;
>> ret = regmap_read(info->max77686->regmap, MAX77686_REG_STATUS2,
>> &val);
>> if (ret < 0) {
>> dev_err(info->dev, "%s:%d fail to read status2 reg(%d)\n",
>> __func__, __LINE__, ret);
>> goto out;
>> }
>>
>> if (val & (1 << 4)) /* RTCA1 */
>> alrm->pending = 1;
>>
>> }
>
> Most of the drivers set the 'pending' field when reading alarm. Your
> original driver did not.
>
> The max77802 does exactly the same (BTW, these should be merged as
> well... I'll add this to the TODO list) so I think this is necessary.
How about merging max77802 to max77686 first? The only differences I
found are:
1. It uses main MFD/PMIC regmap.
This can be solved as part of decoupling code. The driver will get
MFD's regmap and set up its own (only on max77686). The max77802 will
only use parent's regmap.
2. It has different register address.
We need a register-layout/configuration structure. The logic is the
same except few differences (e.g. presence of MAX77802_RTC_AE1).
It may be easier to merge them now, before adding support for max77620?
I could handle this probably next week or in the following week
(assuming someone would test max77802 because I don't have the hardware).
Anyway I think we should develop these RTC patches having this in mind:
merge all of them.
Best regards,
Krzysztof
[toc] | [prev] | [next] | [standalone]
| From | Laxman Dewangan <ldewangan@nvidia.com> |
|---|---|
| Date | 2016-01-14 13:10 +0100 |
| Subject | Re: [PATCH V2 5/6] rtc: max77xxx: add RTC driver for Maxim MAX77xxx series RTC IP |
| Message-ID | <qQOT0-6Lb-21@gated-at.bofh.it> |
| In reply to | #1308946 |
On Thursday 14 January 2016 06:50 AM, Krzysztof Kozlowski wrote:
> On 14.01.2016 09:50, Krzysztof Kozlowski wrote:
>> On 13.01.2016 23:59, Laxman Dewangan wrote:
>>> On Wednesday 13 January 2016 09:55 AM, Laxman Dewangan wrote:
>>>> On Wednesday 13 January 2016 09:58 AM, Krzysztof Kozlowski wrote:
>>>>> On 13.01.2016 13:07, Laxman Dewangan wrote:
>>>>>> On Wednesday 13 January 2016 05:36 AM, Krzysztof Kozlowski wrote:
>>>>>> That is also fine to me but still I am not comfortable with the config
>>>>>> name and driver file name as this does not suggest the common.
>>>>> The name does not matter. Really. We have a lot of drivers with a
>>>>> specific device-like name and supporting different devices. To point
>>>>> that your argument is invalid - your initial name of driver
>>>>> "rtc-max77620.c" supported totally different "names": the max77620 and
>>>>> max20024. It also wasn't suggesting something "common"...
>>>> In all config string, I have mentioned the MAX20024.
>>>>
>>>>
>>>>> With my approach we are not developing common think neither. We just
>>>>> want to extend/re-use existing max77686 (or max77802) driver for new
>>>>> devices. Just like everywhere else.
>>>> OK, fine to me if this is acceptable.
>>>> I will drop this rtc patch form this series and work in max77686
>>>> driver to modify first and once merged, use this config on my defconfig.
>>>>
>>> Here, MAX686 RTC driver needs two regmap handle, one for the rtc_regmap
>>> and other for STATUS2 register access.
>>>
>>> ret = regmap_read(info->max77686->regmap, MAX77686_REG_STATUS2,
>>> &val);
>>> if (ret < 0) {
>>> dev_err(info->dev, "%s:%d fail to read status2 reg(%d)\n",
>>> __func__, __LINE__, ret);
>>> goto out;
>>> }
>>>
>>>
>>> We can not have two regmap handle on rtc driver as both regmap (pmic and
>>> rtc) registered with different i2c device.
>>>
>>> Also this register should not be accessed by RTC driver if we want to
>>> decouple as this is very much MAX77686 register set.
>>> Do we need this code?
>>> static int max77686_rtc_read_alarm(struct device *dev, struct rtc_wkalrm
>>> *alrm)
>>> {
>>> struct max77686_rtc_info *info = dev_get_drvdata(dev);
>>> u8 data[RTC_NR_TIME];
>>> unsigned int val;
>>> int i, ret;
>>>
>>> ::::::::
>>> alrm->pending = 0;
>>> ret = regmap_read(info->max77686->regmap, MAX77686_REG_STATUS2,
>>> &val);
>>> if (ret < 0) {
>>> dev_err(info->dev, "%s:%d fail to read status2 reg(%d)\n",
>>> __func__, __LINE__, ret);
>>> goto out;
>>> }
>>>
>>> if (val & (1 << 4)) /* RTCA1 */
>>> alrm->pending = 1;
>>>
>>> }
>> Most of the drivers set the 'pending' field when reading alarm. Your
>> original driver did not.
>>
>> The max77802 does exactly the same (BTW, these should be merged as
>> well... I'll add this to the TODO list) so I think this is necessary.
> How about merging max77802 to max77686 first? The only differences I
> found are:
> 1. It uses main MFD/PMIC regmap.
> This can be solved as part of decoupling code. The driver will get
> MFD's regmap and set up its own (only on max77686). The max77802 will
> only use parent's regmap.
>
> 2. It has different register address.
> We need a register-layout/configuration structure. The logic is the
> same except few differences (e.g. presence of MAX77802_RTC_AE1).
>
> It may be easier to merge them now, before adding support for max77620?
> I could handle this probably next week or in the following week
> (assuming someone would test max77802 because I don't have the hardware).
>
> Anyway I think we should develop these RTC patches having this in mind:
> merge all of them.
>
I think we can do the merging of max77802 and max77686 at second step.
At first, lets decouple the rtc-max77686.c with its mfd driver. This
will give better picture how will it be done.
Once this is there then max77620 can use this and in parallel, can be
work for max77802 to use the same driver.
Let's first conclude and get accpepted for max77686 and max77620 as both
of us have related HW to verify the changes.
[toc] | [prev] | [next] | [standalone]
| From | Javier Martinez Canillas <javier@dowhile0.org> |
|---|---|
| Date | 2016-01-15 03:00 +0100 |
| Subject | Re: [PATCH V2 5/6] rtc: max77xxx: add RTC driver for Maxim MAX77xxx series RTC IP |
| Message-ID | <qR1Qe-7rW-5@gated-at.bofh.it> |
| In reply to | #1309213 |
Hello, On Thu, Jan 14, 2016 at 8:50 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote: > On Thursday 14 January 2016 06:50 AM, Krzysztof Kozlowski wrote: >> On 14.01.2016 09:50, Krzysztof Kozlowski wrote: [snip] >>> >>> The max77802 does exactly the same (BTW, these should be merged as >>> well... I'll add this to the TODO list) so I think this is necessary. >> >> How about merging max77802 to max77686 first? The only differences I >> found are: >> 1. It uses main MFD/PMIC regmap. >> This can be solved as part of decoupling code. The driver will get >> MFD's regmap and set up its own (only on max77686). The max77802 will >> only use parent's regmap. >> >> 2. It has different register address. >> We need a register-layout/configuration structure. The logic is the >> same except few differences (e.g. presence of MAX77802_RTC_AE1). >> >> It may be easier to merge them now, before adding support for max77620? Agreed. When I originally posted the max77802 support, I had separate MFD, RTC, regulator and clock drivers and the feedback (IIRC) was that the MFD and clock blocks were too similar to the max77686 so I extended those drivers instead of adding new ones. But that the RTC and regulator blocks were different so a separate driver was justified... but it's true that are not that different and rtc-max77686 could be extended and rtc-max77802 removed. In fact, the ChromiumOS vendor tree has a single RTC driver for both max77686 and max77802: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.8/drivers/rtc/rtc-max77xxx.c >> I could handle this probably next week or in the following week >> (assuming someone would test max77802 because I don't have the hardware). >> I could also work on this next week if you want. After all I feel guilty for the code duplication :-) >> Anyway I think we should develop these RTC patches having this in mind: >> merge all of them. >> > > I think we can do the merging of max77802 and max77686 at second step. > > At first, lets decouple the rtc-max77686.c with its mfd driver. This will > give better picture how will it be done. I don't quite understand what you mean by decoupling here, the max77686 MFD driver today is not highly coupled to their cells devices drivers since it supports both max77802 and max77686 already. Yes, some changes will be needed but I think those should be small. > Once this is there then max77620 can use this and in parallel, can be work > for max77802 to use the same driver. > > > Let's first conclude and get accpepted for max77686 and max77620 as both of > us have related HW to verify the changes. > I agree with Krzysztof that merging first before extending makes more sense but I don't have a strong opinion on this and can be done as a followup as well. Best regards, Javier
[toc] | [prev] | [next] | [standalone]
| From | Mark Brown <broonie@kernel.org> |
|---|---|
| Date | 2016-01-14 15:30 +0100 |
| Subject | Re: [PATCH V2 5/6] rtc: max77xxx: add RTC driver for Maxim MAX77xxx series RTC IP |
| Message-ID | <qQR4t-8kt-7@gated-at.bofh.it> |
| In reply to | #1308946 |
[Multipart message — attachments visible in raw view] — view raw
On Thu, Jan 14, 2016 at 10:20:56AM +0900, Krzysztof Kozlowski wrote: > 2. It has different register address. > We need a register-layout/configuration structure. The logic is the > same except few differences (e.g. presence of MAX77802_RTC_AE1). Depending on what the differences are either just some variables in the driver data that get set on probe with the different registers (if it's just the register) or using regmap_field (if things get shifted as well, or if it happens to make more sense) should work.
[toc] | [prev] | [next] | [standalone]
| From | Krzysztof Kozlowski <k.kozlowski@samsung.com> |
|---|---|
| Date | 2016-01-13 01:50 +0100 |
| Subject | Re: [PATCH V2 2/6] mfd: max77620: add core driver for MAX77620/MAX20024 |
| Message-ID | <qQhNn-gY-1@gated-at.bofh.it> |
| In reply to | #1307159 |
On 12.01.2016 18:17, Laxman Dewangan wrote:
> MAX77620/MAX20024 are Power Management IC from the MAXIM.
> It supports RTC, multiple GPIOs, multiple DCDC and LDOs,
> watchdog, clock etc.
>
> Add MFD drier to provides common support for accessing the
> device; additional drivers is developed on respected subsystem
> in order to use the functionality of the device.
>
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> Signed-off-by: Chaitanya Bandi <bandik@nvidia.com>
> Signed-off-by: Mallikarjun Kasoju <mkasoju@nvidia.com>
> ---
> - Code cleanups per review from V1.
> - Move register acccess APIs from header to c file.
> - Remove some of non required variable, remove duplication in error message
> and simplify some of function implementation.
> - Register RTC driver such that it can get the regmap handle form parent device
>
> drivers/mfd/Kconfig | 15 +
> drivers/mfd/Makefile | 1 +
> drivers/mfd/max77620.c | 997 +++++++++++++++++++++++++++++++++++++++++++
> include/linux/mfd/max77620.h | 407 ++++++++++++++++++
> 4 files changed, 1420 insertions(+)
> create mode 100644 drivers/mfd/max77620.c
> create mode 100644 include/linux/mfd/max77620.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 9581ebb..edeb85c 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -492,6 +492,21 @@ config MFD_MAX14577
> additional drivers must be enabled in order to use the functionality
> of the device.
>
> +config MFD_MAX77620
> + bool "Maxim Semiconductor MAX77620 and MAX20024 PMIC Support"
> + depends on I2C=y
> + depends on OF
> + select MFD_CORE
> + select REGMAP_I2C
> + select REGMAP_IRQ
> + select IRQ_DOMAIN
> + help
> + Say yes here to add support for Maxim Semiconductor MAX77620 and
> + MAX20024 which are Power Management IC with General purpose pins,
> + RTC, regulators, clock generator, watchdog etc. 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_MAX77686
> bool "Maxim Semiconductor MAX77686/802 PMIC Support"
> depends on I2C=y
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 0f230a6..97910ed 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -123,6 +123,7 @@ obj-$(CONFIG_MFD_DA9063) += da9063.o
> obj-$(CONFIG_MFD_DA9150) += da9150-core.o
>
> obj-$(CONFIG_MFD_MAX14577) += max14577.o
> +obj-$(CONFIG_MFD_MAX77620) += max77620.o
> obj-$(CONFIG_MFD_MAX77686) += max77686.o
> obj-$(CONFIG_MFD_MAX77693) += max77693.o
> obj-$(CONFIG_MFD_MAX77843) += max77843.o
> diff --git a/drivers/mfd/max77620.c b/drivers/mfd/max77620.c
> new file mode 100644
> index 0000000..c0cd400
> --- /dev/null
> +++ b/drivers/mfd/max77620.c
> @@ -0,0 +1,997 @@
> +/*
> + * Maxim MAX77620 MFD Driver
> + *
> + * Copyright (C) 2016 NVIDIA CORPORATION. All rights reserved.
> + *
> + * Author:
> + * Laxman Dewangan <ldewangan@nvidia.com>
> + * Chaitanya Bandi <bandik@nvidia.com>
> + * Mallikarjun Kasoju <mkasoju@nvidia.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/slab.h>
> +#include <linux/mfd/core.h>
> +#include <linux/interrupt.h>
> +#include <linux/regmap.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/mfd/max77620.h>
> +
> +static struct resource gpio_resources[] = {
> + {
> + .start = MAX77620_IRQ_TOP_GPIO,
> + .end = MAX77620_IRQ_TOP_GPIO,
> + .flags = IORESOURCE_IRQ,
> + }
> +};
> +
> +static struct resource rtc_resources[] = {
> + {
> + .start = MAX77620_IRQ_TOP_RTC,
> + .end = MAX77620_IRQ_TOP_RTC,
> + .flags = IORESOURCE_IRQ,
> + }
> +};
> +
> +static struct resource thermal_resources[] = {
> + {
> + .start = MAX77620_IRQ_LBT_TJALRM1,
> + .end = MAX77620_IRQ_LBT_TJALRM1,
> + .flags = IORESOURCE_IRQ,
> + },
> + {
> + .start = MAX77620_IRQ_LBT_TJALRM2,
> + .end = MAX77620_IRQ_LBT_TJALRM2,
> + .flags = IORESOURCE_IRQ,
> + }
> +};
> +
> +static const struct regmap_irq max77620_top_irqs[] = {
> + [MAX77620_IRQ_TOP_GLBL] = {
> + .mask = MAX77620_IRQ_TOP_GLBL_MASK,
> + .reg_offset = 0,
> + },
> + [MAX77620_IRQ_TOP_SD] = {
> + .mask = MAX77620_IRQ_TOP_SD_MASK,
> + .reg_offset = 0,
> + },
> + [MAX77620_IRQ_TOP_LDO] = {
> + .mask = MAX77620_IRQ_TOP_LDO_MASK,
> + .reg_offset = 0,
> + },
> + [MAX77620_IRQ_TOP_GPIO] = {
> + .mask = MAX77620_IRQ_TOP_GPIO_MASK,
> + .reg_offset = 0,
> + },
> + [MAX77620_IRQ_TOP_RTC] = {
> + .mask = MAX77620_IRQ_TOP_RTC_MASK,
> + .reg_offset = 0,
> + },
> + [MAX77620_IRQ_TOP_32K] = {
> + .mask = MAX77620_IRQ_TOP_32K_MASK,
> + .reg_offset = 0,
> + },
> + [MAX77620_IRQ_TOP_ONOFF] = {
> + .mask = MAX77620_IRQ_TOP_ONOFF_MASK,
> + .reg_offset = 0,
> + },
> +
> + [MAX77620_IRQ_LBT_MBATLOW] = {
> + .mask = MAX77620_IRQ_LBM_MASK,
> + .reg_offset = 1,
> + },
> + [MAX77620_IRQ_LBT_TJALRM1] = {
> + .mask = MAX77620_IRQ_TJALRM1_MASK,
> + .reg_offset = 1,
> + },
> + [MAX77620_IRQ_LBT_TJALRM2] = {
> + .mask = MAX77620_IRQ_TJALRM2_MASK,
> + .reg_offset = 1,
> + },
> +
> +};
> +
> +static const char * const max77620_nverc[] = {
> + "Shutdown-pin",
> + "System WatchDog Timer",
> + "Hard Reset",
> + "Junction Temp Overload",
> + "Main-Battery Low",
> + "Main-Battery overvoltage Lockout",
> + "Main-Battery undervoltage Lockout",
> + "Reset input",
> +};
> +
> +enum max77660_ids {
> + MAX77620_PMIC_ID,
> + MAX77620_GPIO_ID,
> + MAX77620_PINCTRL_ID,
> + MAX77620_CLK_ID,
> + MAX77620_POWER_OFF_ID,
> + MAX77620_WDT_ID,
> + MAX77620_THERMAL_ID,
> + MAX77620_RTC_ID,
> +};
> +
> +#define MAX77620_SUB_MODULE_RES(_name, _id) \
> + [MAX77620_##_id##_ID] = { \
> + .name = "max77620-"#_name, \
> + .num_resources = ARRAY_SIZE(_name##_resources), \
> + .resources = &_name##_resources[0], \
> + .id = MAX77620_##_id##_ID, \
> + }
> +
> +#define MAX20024_SUB_MODULE_RES(_name, _id) \
> + [MAX77620_##_id##_ID] = { \
> + .name = "max20024-"#_name, \
> + .num_resources = ARRAY_SIZE(_name##_resources), \
> + .resources = &_name##_resources[0], \
> + .id = MAX77620_##_id##_ID, \
> + }
> +
> +#define MAX77620_SUB_MODULE_NO_RES(_name, _id) \
> + [MAX77620_##_id##_ID] = { \
> + .name = "max77620-"#_name, \
> + .id = MAX77620_##_id##_ID, \
> + }
> +
> +#define MAX20024_SUB_MODULE_NO_RES(_name, _id) \
> + [MAX77620_##_id##_ID] = { \
> + .name = "max20024-"#_name, \
> + .id = MAX77620_##_id##_ID, \
> + }
> +
> +static struct mfd_cell max77620_children[] = {
> + MAX77620_SUB_MODULE_RES(gpio, GPIO),
> + MAX77620_SUB_MODULE_NO_RES(pmic, PMIC),
> + MAX77620_SUB_MODULE_NO_RES(pinctrl, PINCTRL),
> + MAX77620_SUB_MODULE_NO_RES(clk, CLK),
> + MAX77620_SUB_MODULE_NO_RES(power-off, POWER_OFF),
> + MAX77620_SUB_MODULE_NO_RES(wdt, WDT),
> + MAX77620_SUB_MODULE_RES(thermal, THERMAL),
> +};
> +
> +static struct mfd_cell max20024_children[] = {
> + MAX20024_SUB_MODULE_RES(gpio, GPIO),
> + MAX20024_SUB_MODULE_NO_RES(pmic, PMIC),
> + MAX20024_SUB_MODULE_NO_RES(pinctrl, PINCTRL),
> + MAX20024_SUB_MODULE_NO_RES(clk, CLK),
> + MAX20024_SUB_MODULE_NO_RES(power-off, POWER_OFF),
> + MAX20024_SUB_MODULE_NO_RES(wdt, WDT),
> + MAX20024_SUB_MODULE_RES(thermal, THERMAL),
> +};
> +
> +static struct mfd_cell max77620_20024_rtc = {
> + .name = "max77620-rtc",
> + .num_resources = ARRAY_SIZE(rtc_resources),
> + .resources = rtc_resources,
> + .id = MAX77620_RTC_ID,
> +};
> +
> +struct max77620_sub_modules {
> + struct mfd_cell *cells;
> + int ncells;
> + u32 id;
> +};
> +
> +static const struct max77620_sub_modules max77620_cells = {
> + .cells = max77620_children,
> + .ncells = ARRAY_SIZE(max77620_children),
> + .id = MAX77620,
> +};
> +
> +static const struct max77620_sub_modules max20024_cells = {
> + .cells = max20024_children,
> + .ncells = ARRAY_SIZE(max20024_children),
> + .id = MAX20024,
> +};
> +
> +
> +static const struct of_device_id max77620_of_match[] = {
> + {
> + .compatible = "maxim,max77620",
> + .data = &max77620_cells,
> + }, {
> + .compatible = "maxim,max20024",
> + .data = &max20024_cells,
> + }, {
> + },
> +};
> +MODULE_DEVICE_TABLE(of, max77620_of_match);
> +
> +static struct regmap_irq_chip max77620_top_irq_chip = {
> + .name = "max77620-top",
> + .irqs = max77620_top_irqs,
> + .num_irqs = ARRAY_SIZE(max77620_top_irqs),
> + .num_regs = 2,
> + .status_base = MAX77620_REG_IRQTOP,
> + .mask_base = MAX77620_REG_IRQTOPM,
> +};
> +
> +static const struct regmap_range max77620_readable_ranges[] = {
> + regmap_reg_range(MAX77620_REG_CNFGGLBL1, MAX77620_REG_DVSSD4),
> +};
> +
> +static const struct regmap_access_table max77620_readable_table = {
> + .yes_ranges = max77620_readable_ranges,
> + .n_yes_ranges = ARRAY_SIZE(max77620_readable_ranges),
> +};
> +
> +static const struct regmap_range max20024_readable_ranges[] = {
> + regmap_reg_range(MAX77620_REG_CNFGGLBL1, MAX77620_REG_DVSSD4),
> + regmap_reg_range(MAX20024_REG_MAX_ADD, MAX20024_REG_MAX_ADD),
> +};
> +
> +static const struct regmap_access_table max20024_readable_table = {
> + .yes_ranges = max20024_readable_ranges,
> + .n_yes_ranges = ARRAY_SIZE(max20024_readable_ranges),
> +};
> +
> +static const struct regmap_range max77620_writable_ranges[] = {
> + regmap_reg_range(MAX77620_REG_CNFGGLBL1, MAX77620_REG_DVSSD4),
> +};
> +
> +static const struct regmap_access_table max77620_writable_table = {
> + .yes_ranges = max77620_writable_ranges,
> + .n_yes_ranges = ARRAY_SIZE(max77620_writable_ranges),
> +};
> +
> +static const struct regmap_range max77620_cacheable_ranges[] = {
> + regmap_reg_range(MAX77620_REG_SD0_CFG, MAX77620_REG_LDO_CFG3),
> + regmap_reg_range(MAX77620_REG_FPS_CFG0, MAX77620_REG_FPS_SD3),
> +};
> +
> +static const struct regmap_access_table max77620_volatile_table = {
> + .no_ranges = max77620_cacheable_ranges,
> + .n_no_ranges = ARRAY_SIZE(max77620_cacheable_ranges),
> +};
> +
> +static const struct regmap_config max77620_regmap_config[] = {
> + [MAX77620_PWR_SLAVE] = {
> + .name = "power-slave",
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = MAX77620_REG_DVSSD4 + 1,
> + .cache_type = REGCACHE_RBTREE,
> + .rd_table = &max77620_readable_table,
> + .wr_table = &max77620_writable_table,
> + .volatile_table = &max77620_volatile_table,
> + },
> + [MAX77620_RTC_SLAVE] = {
> + .name = "rtc-slave",
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = 0x1b,
> + },
> +};
> +
> +static const struct regmap_config max20024_regmap_config[] = {
> + [MAX77620_PWR_SLAVE] = {
> + .name = "power-slave",
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = MAX20024_REG_MAX_ADD + 1,
> + .cache_type = REGCACHE_RBTREE,
> + .rd_table = &max20024_readable_table,
> + .wr_table = &max77620_writable_table,
> + .volatile_table = &max77620_volatile_table,
> + },
> + [MAX77620_RTC_SLAVE] = {
> + .name = "rtc-slave",
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = 0x1b,
> + },
> +};
> +
> +static int max77620_slave_address[MAX77620_NUM_SLAVES] = {
> + MAX77620_PWR_I2C_ADDR,
> + MAX77620_RTC_I2C_ADDR,
> +};
> +
> +int max77620_irq_get_virq(struct device *dev, int irq)
> +{
> + struct max77620_chip *chip = dev_get_drvdata(dev);
> +
> + return regmap_irq_get_virq(chip->top_irq_data, irq);
> +}
> +EXPORT_SYMBOL_GPL(max77620_irq_get_virq);
> +
> +int max77620_reg_write(struct device *dev, int sid,
> + unsigned int reg, u8 val)
> +{
> + struct max77620_chip *chip = dev_get_drvdata(dev);
> +
> + return regmap_write(chip->rmap[sid], reg, val);
> +}
> +EXPORT_SYMBOL_GPL(max77620_reg_write);
> +
> +int max77620_reg_read(struct device *dev, int sid,
> + unsigned int reg, u8 *val)
> +{
> + struct max77620_chip *chip = dev_get_drvdata(dev);
> + unsigned int ival;
> + int ret;
> +
> + ret = regmap_read(chip->rmap[sid], reg, &ival);
> + if (ret < 0)
> + return ret;
> + *val = ival;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(max77620_reg_read);
> +
> +int max77620_reg_update(struct device *dev, int sid,
> + unsigned int reg, unsigned int mask, unsigned int val)
> +{
> + struct max77620_chip *chip = dev_get_drvdata(dev);
> +
> + return regmap_update_bits(chip->rmap[sid], reg, mask, val);
> +}
> +EXPORT_SYMBOL_GPL(max77620_reg_update);
> +
> +static int max77620_get_fps_period_reg_value(struct max77620_chip *chip,
> + int tperiod)
> +{
> + int base_fps_time = (chip->id == MAX20024) ? 20 : 40;
> + int x, i;
> +
> + for (i = 0; i < 0x7; ++i) {
> + x = base_fps_time * BIT(i);
> + if (x >= tperiod)
> + return i;
> + }
> +
> + return i;
> +}
> +
> +static int max77620_config_fps(struct max77620_chip *chip,
> + struct device_node *fps_np)
> +{
> + struct device *dev = chip->dev;
> + unsigned int mask = 0, config = 0;
> + int input_enable = 2;
> + u32 pval;
> + int tperiod, fps_id;
> + int ret;
> +
> + ret = of_property_read_u32(fps_np, "reg", &pval);
> + if (ret < 0) {
> + dev_err(dev, "reg prop missing from node %s\n", fps_np->name);
> + return 0;
> + }
> + if (pval > 2) {
> + dev_err(dev, "FPS%u is not supported\n", pval);
> + return 0;
> + }
> + fps_id = pval;
> +
> + ret = of_property_read_u32(fps_np,
> + "maxim,active-fps-time-period", &pval);
> + if (!ret) {
> + mask |= MAX77620_FPS_TIME_PERIOD_MASK;
> + chip->active_fps_period[fps_id] = min(pval, 5120U);
> + tperiod = max77620_get_fps_period_reg_value(chip,
> + chip->active_fps_period[fps_id]);
> + config |= tperiod << MAX77620_FPS_TIME_PERIOD_SHIFT;
> + }
> +
> + ret = of_property_read_u32(fps_np,
> + "maxim,suspend-fps-time-period", &pval);
> + if (!ret)
> + chip->suspend_fps_period[fps_id] = min(pval, 5120U);
> +
> + ret = of_property_read_u32(fps_np, "maxim,fps-enable-input", &pval);
> + if (!ret) {
> + if (pval > 2) {
> + dev_err(dev, "FPS %d enable-input invalid\n", fps_id);
> + } else {
> + input_enable = pval;
> + mask |= MAX77620_FPS_EN_SRC_MASK;
> + }
> + }
> + config |= (input_enable & 0x3) << MAX77620_FPS_EN_SRC_SHIFT;
> +
> + if (input_enable == 2) {
> + bool enable_fps = of_property_read_bool(fps_np,
> + "maxim,fps-sw-enable");
> + mask |= MAX77620_FPS_ENFPS_MASK;
> + if (enable_fps)
> + config |= 1;
> + }
> +
> + if (!chip->sleep_enable)
> + chip->sleep_enable = of_property_read_bool(fps_np,
> + "maxim,enable-sleep");
> + if (!chip->enable_global_lpm)
> + chip->enable_global_lpm = of_property_read_bool(fps_np,
> + "maxim,enable-global-lpm");
> +
> + ret = max77620_reg_update(dev, MAX77620_PWR_SLAVE,
> + MAX77620_REG_FPS_CFG0 + fps_id, mask, config);
> + if (ret < 0) {
> + dev_err(dev, "Reg 0x%02x write failed: %d\n",
> + MAX77620_REG_FPS_CFG0 + fps_id, ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int max77620_initialise_fps(struct max77620_chip *chip,
> + struct device *dev)
> +{
> + struct device_node *fps_np, *fps_child;
> + u8 config;
> + int fps_id;
> + int ret;
> +
> + for (fps_id = 0; fps_id < 3; ++fps_id) {
> + chip->active_fps_period[fps_id] = -1;
> + chip->suspend_fps_period[fps_id] = -1;
> + }
> +
> + fps_np = of_get_child_by_name(dev->of_node, "fps");
> + if (!fps_np)
> + goto skip_fps;
> +
> + for_each_child_of_node(fps_np, fps_child) {
> + ret = max77620_config_fps(chip, fps_child);
> + if (ret < 0)
> + return ret;
> + }
> +
> + config = chip->enable_global_lpm ? MAX77620_ONOFFCNFG2_SLP_LPM_MSK : 0;
> + ret = max77620_reg_update(chip->dev, MAX77620_PWR_SLAVE,
> + MAX77620_REG_ONOFFCNFG2,
> + MAX77620_ONOFFCNFG2_SLP_LPM_MSK, config);
> + if (ret < 0) {
> + dev_err(dev, "Reg ONOFFCNFG2 update failed: %d\n", ret);
> + return ret;
> + }
> +
> +skip_fps:
> + /* Enable wake on EN0 pin */
> + ret = max77620_reg_update(chip->dev, MAX77620_PWR_SLAVE,
> + MAX77620_REG_ONOFFCNFG2, MAX77620_ONOFFCNFG2_WK_EN0,
> + MAX77620_ONOFFCNFG2_WK_EN0);
> + if (ret < 0) {
> + dev_err(dev, "Reg ONOFFCNFG2 WK_EN0 update failed: %d\n", ret);
> + return ret;
> + }
> +
> + if (!chip->sleep_enable)
> + chip->sleep_enable = of_property_read_bool(dev->of_node,
> + "maxim,enable-sleep");
> +
> + /* For MAX20024, SLPEN will be POR reset if CLRSE is b11 */
> + if ((chip->id == MAX20024) && chip->sleep_enable) {
> + config = MAX77620_ONOFFCNFG1_SLPEN | MAX20024_ONOFFCNFG1_CLRSE;
> + ret = max77620_reg_update(chip->dev, MAX77620_PWR_SLAVE,
> + MAX77620_REG_ONOFFCNFG1, config, config);
> + if (ret < 0) {
> + dev_err(dev, "Reg ONOFFCNFG1 update failed: %d\n", ret);
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int max77620_init_backup_battery_charging(struct max77620_chip *chip,
> + struct device *dev)
> +{
> + struct device_node *bb_node;
> + u32 pval;
> + u8 config;
> + int ret;
> +
> + bb_node = of_get_child_by_name(dev->of_node, "backup-battery");
> + if (!bb_node) {
> + dev_info(dev, "Backup battery charging support disabled\n");
> + max77620_reg_update(chip->dev, MAX77620_PWR_SLAVE,
> + MAX77620_REG_CNFGBBC, MAX77620_CNFGBBC_ENABLE, 0);
> + return 0;
> + }
> +
> + config = MAX77620_CNFGBBC_ENABLE;
> + ret = of_property_read_u32(bb_node,
> + "maxim,bb-charging-current-microamp", &pval);
> + if (ret < 0)
> + pval = 50;
> + if (pval <= 50)
> + config |= 0 << MAX77620_CNFGBBC_CURRENT_SHIFT;
> + else if (pval <= 100)
> + config |= 3 << MAX77620_CNFGBBC_CURRENT_SHIFT;
> + else if (pval <= 200)
> + config |= 0 << MAX77620_CNFGBBC_CURRENT_SHIFT;
> + else if (pval <= 400)
> + config |= 3 << MAX77620_CNFGBBC_CURRENT_SHIFT;
> + else if (pval <= 600)
> + config |= 1 << MAX77620_CNFGBBC_CURRENT_SHIFT;
> + else
> + config |= 2 << MAX77620_CNFGBBC_CURRENT_SHIFT;
> +
> + if (pval > 100)
> + config |= MAX77620_CNFGBBC_LOW_CURRENT_DISABLE;
> +
> + ret = of_property_read_u32(bb_node,
> + "maxim,bb-charging-voltage-microvolt", &pval);
> + if (ret < 0)
> + pval = 2500000;
> + pval /= 1000;
> + if (pval <= 2500)
> + config |= 0 << MAX77620_CNFGBBC_VOLTAGE_SHIFT;
> + else if (pval <= 3000)
> + config |= 1 << MAX77620_CNFGBBC_VOLTAGE_SHIFT;
> + else if (pval <= 3300)
> + config |= 2 << MAX77620_CNFGBBC_VOLTAGE_SHIFT;
> + else
> + config |= 3 << MAX77620_CNFGBBC_VOLTAGE_SHIFT;
> +
> + ret = of_property_read_u32(bb_node,
> + "maxim,bb-output-resister-ohm", &pval);
> + if (ret < 0)
> + pval = 1000;
> +
> + if (pval <= 100)
> + config |= 0 << MAX77620_CNFGBBC_RESISTOR_SHIFT;
> + else if (pval <= 1000)
> + config |= 1 << MAX77620_CNFGBBC_RESISTOR_SHIFT;
> + else if (pval <= 3000)
> + config |= 2 << MAX77620_CNFGBBC_RESISTOR_SHIFT;
> + else if (pval <= 6000)
> + config |= 3 << MAX77620_CNFGBBC_RESISTOR_SHIFT;
> +
> + ret = max77620_reg_write(dev, MAX77620_PWR_SLAVE,
> + MAX77620_REG_CNFGBBC, config);
> + if (ret < 0) {
> + dev_err(dev, "Reg 0x%02x write failed: %d\n",
> + MAX77620_REG_CNFGBBC, ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int max77620_init_low_battery_monitor(struct max77620_chip *chip,
> + struct device *dev)
> +{
> + struct device_node *np;
> + u8 mask = 0, val = 0;
> + u32 pval;
> + int ret;
> +
> + np = of_get_child_by_name(dev->of_node, "low-battery-monitor");
> + if (!np)
> + return 0;
> +
> + ret = of_property_read_u32(np, "maxim,low-battery-dac", &pval);
> + if (!ret) {
> + mask |= MAX77620_CNFGGLBL1_LBDAC_EN;
> + if (pval)
> + val |= MAX77620_CNFGGLBL1_LBDAC_EN;
> + }
> +
> + ret = of_property_read_u32(np, "maxim,low-battery-shutdown", &pval);
> + if (!ret) {
> + mask |= MAX77620_CNFGGLBL1_MPPLD;
> + if (pval)
> + val |= MAX77620_CNFGGLBL1_MPPLD;
> + }
> +
> + ret = of_property_read_u32(np, "maxim,low-battery-reset", &pval);
> + if (!ret) {
> + mask |= MAX77620_CNFGGLBL1_LBRSTEN;
> + if (pval)
> + val |= MAX77620_CNFGGLBL1_LBRSTEN;
> + }
> +
> + if (mask) {
> + ret = max77620_reg_update(dev, MAX77620_PWR_SLAVE,
> + MAX77620_REG_CNFGGLBL1, mask, val);
> + if (ret < 0)
> + dev_err(dev, "Reg CNFGGLBL1 update failed: %d\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int max77620_initialise_chip(struct max77620_chip *chip,
> + struct device *dev)
> +{
> + struct device_node *np = dev->of_node;
> + u32 mrt_time = 0;
> + u8 reg_val;
> + int ret;
> +
> + ret = of_property_read_u32(np, "maxim,hard-power-off-time", &mrt_time);
> + if (ret < 0)
> + return 0;
> +
> + mrt_time = (mrt_time > 12) ? 12 : mrt_time;
> + if (mrt_time <= 6)
> + reg_val = mrt_time - 2;
> + else
> + reg_val = (mrt_time - 6) / 2 + 4;
> +
> + reg_val <<= MAX77620_ONOFFCNFG1_MRT_SHIFT;
> +
> + ret = max77620_reg_update(dev, MAX77620_PWR_SLAVE,
> + MAX77620_REG_ONOFFCNFG1, MAX77620_ONOFFCNFG1_MRT_MASK,
> + reg_val);
> + if (ret < 0) {
> + dev_err(dev, "REG ONOFFCNFG1 update failed: %d\n", ret);
> + return ret;
> + }
> +
> + /* Disable alarm wake to enable sleep from EN input signal */
> + ret = max77620_reg_update(dev, MAX77620_PWR_SLAVE,
> + MAX77620_REG_ONOFFCNFG2, MAX77620_ONOFFCNFG2_WK_ALARM1, 0);
> + if (ret < 0) {
> + dev_err(dev, "REG ONOFFCNFG2 update failed: %d\n", ret);
> + return ret;
> + }
> +
> + return ret;
> +}
> +
> +static int max77620_read_es_version(struct max77620_chip *chip)
> +{
> + u8 val, cid_val[6];
> + int i;
> + int ret;
> +
> + for (i = MAX77620_REG_CID0; i <= MAX77620_REG_CID5; ++i) {
> + ret = max77620_reg_read(chip->dev, MAX77620_PWR_SLAVE, i, &val);
> + if (ret < 0) {
> + dev_err(chip->dev, "CID%d register read failed: %d\n",
> + i - MAX77620_REG_CID0, ret);
> + return ret;
> + }
> + dev_dbg(chip->dev, "CID%d: 0x%02x\n",
> + i - MAX77620_REG_CID0, val);
> + cid_val[i - MAX77620_REG_CID0] = val;
> + }
> +
> + /* CID4 is OTP Version */
> + dev_info(chip->dev, "MAX77620 PMIC OTP Version: 0x%02X\n", cid_val[4]);
> +
> + /* CID5 is ES version */
> + dev_info(chip->dev, "MAX77620 PMIC ES version: 1.%d\n",
> + MAX77620_CID5_DIDM(cid_val[5]));
> +
> + /* Read NVERC register */
> + ret = max77620_reg_read(chip->dev, MAX77620_PWR_SLAVE,
> + MAX77620_REG_NVERC, &val);
> + if (ret < 0) {
> + dev_err(chip->dev, "NVERC read failed: %d\n", ret);
> + return ret;
> + }
> + dev_dbg(chip->dev, "NVERC = 0x%02x\n", val);
> + for (i = 0; i < 8; ++i) {
> + if (val & BIT(i))
> + dev_info(chip->dev, "NVERC: %s\n", max77620_nverc[i]);
You are still printing two dev_info (OTP, ES) and here NVERC (probably
one?). This will be printed on each boot, over and over, till the user
will learn it and will remember it forever :).
From my point of view: one dev_info for one probed device.
I don't know if others agree with that, though. What's your opinion Lee?
> + }
> +
> + return ret;
> +}
> +
> +static irqreturn_t max77620_mbattlow_irq(int irq, void *data)
> +{
> + struct max77620_chip *max77620 = data;
> +
> + dev_dbg(max77620->dev, "MBATTLOW interrupt occurred\n");
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int max77620_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct device_node *node = client->dev.of_node;
> + const struct max77620_sub_modules *children;
> + const struct of_device_id *match;
> + const struct regmap_config *rmap_config = max77620_regmap_config;
> + struct max77620_chip *chip;
> + int i = 0;
> + int ret = 0;
> +
> + if (!node) {
> + dev_err(&client->dev, "Device is not from DT\n");
> + return -ENODEV;
> + }
> +
> + match = of_match_device(max77620_of_match, &client->dev);
> + children = match->data;
You can just use of_device_get_match_data() thus removing "match" variable.
> + if (!children)
> + return -ENODEV;
> +
> + chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> + if (!chip)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(client, chip);
> + chip->dev = &client->dev;
> + chip->irq_base = -1;
> + chip->chip_irq = client->irq;
> + chip->id = children->id;
> +
> + if (chip->id == MAX20024)
> + rmap_config = max20024_regmap_config;
> +
> + mutex_init(&chip->mutex_config);
> +
> + for (i = 0; i < MAX77620_NUM_SLAVES; i++) {
> + if (max77620_slave_address[i] == client->addr)
> + chip->clients[i] = client;
> + else
> + chip->clients[i] = i2c_new_dummy(client->adapter,
> + max77620_slave_address[i]);
> + if (!chip->clients[i]) {
> + dev_err(&client->dev, "can't attach client %d\n", i);
> + ret = -ENOMEM;
> + goto fail_client_reg;
> + }
> +
> + chip->clients[i]->dev.of_node = node;
> + i2c_set_clientdata(chip->clients[i], chip);
> + chip->rmap[i] = devm_regmap_init_i2c(chip->clients[i],
> + &rmap_config[i]);
> + if (IS_ERR(chip->rmap[i])) {
> + ret = PTR_ERR(chip->rmap[i]);
> + dev_err(&client->dev,
> + "regmap %d init failed, err %d\n", i, ret);
> + goto fail_client_reg;
> + }
> + }
> +
> + ret = max77620_read_es_version(chip);
> + if (ret < 0)
> + goto fail_client_reg;
> +
> + ret = max77620_initialise_chip(chip, &client->dev);
> + if (ret < 0)
> + goto fail_client_reg;
> +
> + ret = regmap_add_irq_chip(chip->rmap[MAX77620_PWR_SLAVE],
> + chip->chip_irq, IRQF_ONESHOT | IRQF_SHARED, chip->irq_base,
Why do you need IRQF_SHARED?
> + &max77620_top_irq_chip, &chip->top_irq_data);
More tabs needed for indentation of arguments.
Actually the alignment of arguments here is mixed. Sometimes arguments
are aligned with opening parenthesis, mostly not. Can you make it
consistent - always aligned?
Thank you for implementing the comments pointed for v1. Overall the MFD
part here looks good for me.
Best regards,
Krzysztof
[toc] | [prev] | [next] | [standalone]
| From | Laxman Dewangan <ldewangan@nvidia.com> |
|---|---|
| Date | 2016-01-13 10:20 +0100 |
| Subject | Re: [PATCH V2 2/6] mfd: max77620: add core driver for MAX77620/MAX20024 |
| Message-ID | <qQpKX-60h-29@gated-at.bofh.it> |
| In reply to | #1307942 |
On Wednesday 13 January 2016 06:17 AM, Krzysztof Kozlowski wrote:
> On 12.01.2016 18:17, Laxman Dewangan wrote:
>> + }
>> + dev_dbg(chip->dev, "NVERC = 0x%02x\n", val);
>> + for (i = 0; i < 8; ++i) {
>> + if (val & BIT(i))
>> + dev_info(chip->dev, "NVERC: %s\n", max77620_nverc[i]);
> You are still printing two dev_info (OTP, ES) and here NVERC (probably
> one?). This will be printed on each boot, over and over, till the user
> will learn it and will remember it forever :).
>
> From my point of view: one dev_info for one probed device.
>
> I don't know if others agree with that, though. What's your opinion Lee?
OK, I make single line print for OTP and ES version.
I remove the NVREC reading and printing as this is read on clear and
better to move on uboot for this prints.
>
>
> +
> + ret = regmap_add_irq_chip(chip->rmap[MAX77620_PWR_SLAVE],
> + chip->chip_irq, IRQF_ONESHOT | IRQF_SHARED, chip->irq_base,
> Why do you need IRQF_SHARED?
In one of my design, I have three PMICs, one MAX77620, two MAX77621.
MAX77621 alert an MAX77620 interrupt line is tied and going to single
interrupt of SoC.
To register same interrupt from all driver, I made it SHARED.
This is per discussion on the other patch
regulator: max8973: add support for junction thermal warning
>> + &max77620_top_irq_chip, &chip->top_irq_data);
> More tabs needed for indentation of arguments.
>
> Actually the alignment of arguments here is mixed. Sometimes arguments
> are aligned with opening parenthesis, mostly not. Can you make it
> consistent - always aligned?
In my 3rd patch, I tried to align it little more where is possible. But
I feel that all these indenting is more over individual choice. Example,
I added one more indent in below example to look better.
ret = max77620_reg_update(chip->dev, MAX77620_PWR_SLAVE,
- MAX77620_REG_ONOFFCNFG2, MAX77620_ONOFFCNFG2_WK_EN0,
- MAX77620_ONOFFCNFG2_WK_EN0);
+ MAX77620_REG_ONOFFCNFG2, MAX77620_ONOFFCNFG2_WK_EN0,
+ MAX77620_ONOFFCNFG2_WK_EN0);
[toc] | [prev] | [next] | [standalone]
| From | Alexandre Belloni <alexandre.belloni@free-electrons.com> |
|---|---|
| Date | 2016-01-13 11:20 +0100 |
| Subject | Re: [PATCH V2 2/6] mfd: max77620: add core driver for MAX77620/MAX20024 |
| Message-ID | <qQqGZ-6FS-9@gated-at.bofh.it> |
| In reply to | #1308194 |
On 13/01/2016 at 14:30:26 +0530, Laxman Dewangan wrote : > In my 3rd patch, I tried to align it little more where is possible. But I > feel that all these indenting is more over individual choice. Example, I > added one more indent in below example to look better. > > > ret = max77620_reg_update(chip->dev, MAX77620_PWR_SLAVE, > - MAX77620_REG_ONOFFCNFG2, MAX77620_ONOFFCNFG2_WK_EN0, > - MAX77620_ONOFFCNFG2_WK_EN0); > + MAX77620_REG_ONOFFCNFG2, MAX77620_ONOFFCNFG2_WK_EN0, > + MAX77620_ONOFFCNFG2_WK_EN0); > > You should probably use checkpatch.pl --strict, it requires that you align on the open parenthesis. -- Alexandre Belloni, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web