[PATCH v4 00/10] mfd: add support for max77650 PMIC

[PATCH v4 00/10] mfd: add support for max77650 PMIC

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

This series adds support for max77650 ultra low-power PMIC. It provides
the core mfd driver and a set of five sub-drivers for the regulator,
power supply, gpio, leds and input subsystems.

Patches 1-4 add the DT binding documents. Patches 5-9 add all drivers.
Last patch adds a MAINTAINERS entry for this device.

The regulator part has already been picked up into the regulator tree.

v1 -> v2:

General:
- use C++ style comments for the SPDX license identifier and the
  copyright header
- s/MODULE_LICENSE("GPL")/MODULE_LICENSE("GPL v2")/
- lookup the virtual interrupt numbers in the MFD driver, setup
  resources for child devices and use platform_get_irq_byname()
  in sub-drivers
- picked up review tags
- use devm_request_any_context_irq() for interrupt requests

LEDs:
- changed the max77650_leds_ prefix to max77650_led_
- drop the max77650_leds structure as the only field it held was the
  regmap pointer, move said pointer to struct max77650_led
- change the driver name to "max77650-led"
- drop the last return value check and return the result of
  regmap_write() directly
- change the labeling scheme to one consistent with other LED drivers

ONKEY:
- drop the key reporting helper and call the input functions directly
  from interrupt handlers
- rename the rv local variable to error
- drop parent device asignment

Regulator:
- drop the unnecessary init_data lookup from the driver code
- drop unnecessary include

Charger:
- disable the charger on driver remove
- change the power supply type to POWER_SUPPLY_TYPE_USB

GPIO:
- drop interrupt support until we have correct implementation of hierarchical
  irqs in gpiolib

v2 -> v3:

General:
- dropped regulator patches as they're already in Mark Brown's branch

LED:
- fix the compatible string in the DT binding example
- use the max_brightness property
- use a common prefix ("MAX77650_LED") for all defines in the driver

MFD:
- add the MODULE_DEVICE_TABLE()
- add a sentinel to the of_device_id array
- constify the pointers to irq names
- use an enum instead of defines for interrupt indexes

v3 -> v4:

GPIO:
- as discussed with Linus Walleij: the gpio-controller is now part of
  the core mfd module (we don't spawn a sub-node anymore), the binding
  document for GPIO has been dropped, the GPIO properties have been
  defined in the binding document for the mfd core, the interrupt
  functionality has been reintroduced with the irq directly passed from
  the mfd part
- due to the above changes the Reviewed-by tag from Linus was dropped

Bartosz Golaszewski (10):
  dt-bindings: mfd: add DT bindings for max77650
  dt-bindings: power: supply: add DT bindings for max77650
  dt-bindings: leds: add DT bindings for max77650
  dt-bindings: input: add DT bindings for max77650
  mfd: max77650: new core mfd driver
  power: supply: max77650: add support for battery charger
  gpio: max77650: add GPIO support
  leds: max77650: add LEDs support
  input: max77650: add onkey support
  MAINTAINERS: add an entry for max77650 mfd driver

 .../bindings/input/max77650-onkey.txt         |  26 ++
 .../bindings/leds/leds-max77650.txt           |  57 +++
 .../devicetree/bindings/mfd/max77650.txt      |  47 +++
 .../power/supply/max77650-charger.txt         |  27 ++
 MAINTAINERS                                   |  14 +
 drivers/gpio/Kconfig                          |   7 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-max77650.c                  | 190 ++++++++++
 drivers/input/misc/Kconfig                    |   9 +
 drivers/input/misc/Makefile                   |   1 +
 drivers/input/misc/max77650-onkey.c           | 127 +++++++
 drivers/leds/Kconfig                          |   6 +
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-max77650.c                  | 147 ++++++++
 drivers/mfd/Kconfig                           |  11 +
 drivers/mfd/Makefile                          |   1 +
 drivers/mfd/max77650.c                        | 342 +++++++++++++++++
 drivers/power/supply/Kconfig                  |   7 +
 drivers/power/supply/Makefile                 |   1 +
 drivers/power/supply/max77650-charger.c       | 355 ++++++++++++++++++
 include/linux/mfd/max77650.h                  |  59 +++
 21 files changed, 1436 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/max77650-onkey.txt
 create mode 100644 Documentation/devicetree/bindings/leds/leds-max77650.txt
 create mode 100644 Documentation/devicetree/bindings/mfd/max77650.txt
 create mode 100644 Documentation/devicetree/bindings/power/supply/max77650-charger.txt
 create mode 100644 drivers/gpio/gpio-max77650.c
 create mode 100644 drivers/input/misc/max77650-onkey.c
 create mode 100644 drivers/leds/leds-max77650.c
 create mode 100644 drivers/mfd/max77650.c
 create mode 100644 drivers/power/supply/max77650-charger.c
 create mode 100644 include/linux/mfd/max77650.h

-- 
2.20.1

[PATCH v4 01/10] dt-bindings: mfd: add DT bindings for max77650

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Add a DT binding document for max77650 ultra-low power PMIC. This
describes the core mfd device and the GPIO module.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 .../devicetree/bindings/mfd/max77650.txt      | 47 +++++++++++++++++++
 1 file changed, 47 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/max77650.txt

diff --git a/Documentation/devicetree/bindings/mfd/max77650.txt b/Documentation/devicetree/bindings/mfd/max77650.txt
new file mode 100644
index 000000000000..0a40e9c6aab8
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/max77650.txt
@@ -0,0 +1,47 @@
+MAX77650 ultra low-power PMIC from Maxim Integrated.
+
+Required properties:
+-------------------
+- compatible:		Must be "maxim,max77650"
+- reg:			I2C device address.
+- interrupts:		The interrupt on the parent the controller is
+			connected to.
+- interrupt-parent:	phandle of the parent interrupt controller.
+- interrupt-controller: Marks the device node as an interrupt controller.
+- #interrupt-cells:	Must be <2>.
+
+- gpio-controller : 	Marks the device node as a gpio controller.
+- #gpio-cells : 	Must be <2>. The first cell is the pin number and
+			the second cell is used to specify the gpio active
+			state.
+
+Optional properties:
+--------------------
+gpio-line-names:	Single string containing the name of the GPIO line.
+
+The GPIO-controller module is represented as part of the top-level PMIC
+node. The device exposes a single GPIO line.
+
+For device-tree bindings of other sub-modules (regulator, power supply,
+LEDs and onkey) refer to the binding documents under the respective
+sub-system directories.
+
+For more details on GPIO bindings, please refer to the generic GPIO DT
+binding document <devicetree/bindings/gpio/gpio.txt>.
+
+Example:
+--------
+
+	pmic: max77650@48 {
+		compatible = "maxim,max77650";
+		reg = <0x48>;
+
+		interrupt-controller;
+		interrupt-parent = <&gpio2>;
+		#interrupt-cells = <2>;
+		interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
+
+		gpio-controller;
+		#gpio-cells = <2>;
+		gpio-line-names = "max77650-charger";
+	};
-- 
2.20.1

[PATCH v4 02/10] dt-bindings: power: supply: add DT bindings for max77650

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Add the DT binding document for the battery charger module of max77650.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 .../power/supply/max77650-charger.txt         | 27 +++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/max77650-charger.txt

diff --git a/Documentation/devicetree/bindings/power/supply/max77650-charger.txt b/Documentation/devicetree/bindings/power/supply/max77650-charger.txt
new file mode 100644
index 000000000000..f3e00d41e299
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/max77650-charger.txt
@@ -0,0 +1,27 @@
+Battery charger driver for MAX77650 PMIC from Maxim Integrated.
+
+This module is part of the MAX77650 MFD device. For more details
+see Documentation/devicetree/bindings/mfd/max77650.txt.
+
+The charger is represented as a sub-node of the PMIC node on the device tree.
+
+Required properties:
+--------------------
+- compatible:		Must be "maxim,max77650-charger"
+
+Optional properties:
+--------------------
+- maxim,vchgin-min:	Minimum CHGIN regulation voltage (in microvolts). Must be
+			one of: 4000000, 4100000, 4200000, 4300000, 4400000,
+			4500000, 4600000, 4700000.
+- maxim,ichgin-lim:	CHGIN input current limit (in microamps). Must be one of:
+			95000, 190000, 285000, 380000, 475000.
+
+Example:
+--------
+
+	charger {
+		compatible = "maxim,max77650-charger";
+		maxim,vchgin-min = <4200000>;
+		maxim,ichgin-lim = <285000>;
+	};
-- 
2.20.1

[PATCH v4 03/10] dt-bindings: leds: add DT bindings for max77650

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Add the DT binding document for the LEDs module of max77650.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 .../bindings/leds/leds-max77650.txt           | 57 +++++++++++++++++++
 1 file changed, 57 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-max77650.txt

diff --git a/Documentation/devicetree/bindings/leds/leds-max77650.txt b/Documentation/devicetree/bindings/leds/leds-max77650.txt
new file mode 100644
index 000000000000..3a67115cc1da
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-max77650.txt
@@ -0,0 +1,57 @@
+LED driver for MAX77650 PMIC from Maxim Integrated.
+
+This module is part of the MAX77650 MFD device. For more details
+see Documentation/devicetree/bindings/mfd/max77650.txt.
+
+The LED controller is represented as a sub-node of the PMIC node on
+the device tree.
+
+This device has three current sinks.
+
+Required properties:
+--------------------
+- compatible:		Must be "maxim,max77650-led"
+- #address-cells:	Must be <1>.
+- #size-cells:		Must be <0>.
+
+Each LED is represented as a sub-node of the LED-controller node. Up to
+three sub-nodes can be defined.
+
+Required properties of the sub-node:
+------------------------------------
+
+- reg:			Must be <0>, <1> or <2>.
+
+Optional properties of the sub-node:
+------------------------------------
+
+- label:		See Documentation/devicetree/bindings/leds/common.txt
+- linux,default-trigger: See Documentation/devicetree/bindings/leds/common.txt
+
+For more details, please refer to the generic GPIO DT binding document
+<devicetree/bindings/gpio/gpio.txt>.
+
+Example:
+--------
+
+	leds {
+		compatible = "maxim,max77650-led";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		led@0 {
+			reg = <0>;
+			label = "blue:usr0";
+		};
+
+		led@1 {
+			reg = <1>;
+			label = "red:usr1";
+			linux,default-trigger = "heartbeat";
+		};
+
+		led@2 {
+			reg = <2>;
+			label = "green:usr2";
+		};
+	};
-- 
2.20.1

[PATCH v4 04/10] dt-bindings: input: add DT bindings for max77650

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Add the DT binding document for the onkey module of max77650.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 .../bindings/input/max77650-onkey.txt         | 26 +++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/max77650-onkey.txt

diff --git a/Documentation/devicetree/bindings/input/max77650-onkey.txt b/Documentation/devicetree/bindings/input/max77650-onkey.txt
new file mode 100644
index 000000000000..37c80898be4d
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/max77650-onkey.txt
@@ -0,0 +1,26 @@
+Onkey driver for MAX77650 PMIC from Maxim Integrated.
+
+This module is part of the MAX77650 MFD device. For more details
+see Documentation/devicetree/bindings/mfd/max77650.txt.
+
+The onkey controller is represented as a sub-node of the PMIC node on
+the device tree.
+
+Required properties:
+--------------------
+- compatible:		Must be "maxim,max77650-onkey".
+
+Optional properties:
+- linux,code:		The key-code to be reported when the key is pressed.
+			Defaults to KEY_POWER.
+- maxim,onkey-mode:	Must be "push" or "slide" depending on the type of
+			button used by the system. Defaults to "push".
+
+Example:
+--------
+
+	onkey {
+		compatible = "maxim,max77650-onkey";
+		linux,code = <KEY_END>;
+		maxim,onkey-mode = "slide";
+	};
-- 
2.20.1

[PATCH v4 05/10] mfd: max77650: new core mfd driver

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Add the core mfd driver for max77650 PMIC. We define five sub-devices
for which the drivers will be added in subsequent patches.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/mfd/Kconfig          |  11 ++
 drivers/mfd/Makefile         |   1 +
 drivers/mfd/max77650.c       | 342 +++++++++++++++++++++++++++++++++++
 include/linux/mfd/max77650.h |  59 ++++++
 4 files changed, 413 insertions(+)
 create mode 100644 drivers/mfd/max77650.c
 create mode 100644 include/linux/mfd/max77650.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 76f9909cf396..a80c3fe80fbe 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -734,6 +734,17 @@ config MFD_MAX77620
 	  provides common support for accessing the device; additional drivers
 	  must be enabled in order to use the functionality of the device.
 
+config MFD_MAX77650
+	tristate "Maxim MAX77650/77651 PMIC Support"
+	depends on I2C
+	depends on OF || COMPILE_TEST
+	select MFD_CORE
+	select REGMAP_I2C
+	help
+	  Say yes here to add support for Maxim Semiconductor MAX77650 and
+	  MAX77651 Power Management ICs. This is the core multifunction
+	  driver for interacting with the device.
+
 config MFD_MAX77686
 	tristate "Maxim Semiconductor MAX77686/802 PMIC Support"
 	depends on I2C
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 12980a4ad460..3b912a4015d1 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -151,6 +151,7 @@ obj-$(CONFIG_MFD_DA9150)	+= da9150-core.o
 
 obj-$(CONFIG_MFD_MAX14577)	+= max14577.o
 obj-$(CONFIG_MFD_MAX77620)	+= max77620.o
+obj-$(CONFIG_MFD_MAX77650)	+= max77650.o
 obj-$(CONFIG_MFD_MAX77686)	+= max77686.o
 obj-$(CONFIG_MFD_MAX77693)	+= max77693.o
 obj-$(CONFIG_MFD_MAX77843)	+= max77843.o
diff --git a/drivers/mfd/max77650.c b/drivers/mfd/max77650.c
new file mode 100644
index 000000000000..7c6164f1fde4
--- /dev/null
+++ b/drivers/mfd/max77650.c
@@ -0,0 +1,342 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (C) 2018 BayLibre SAS
+// Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
+//
+// Core MFD driver for MAXIM 77650/77651 charger/power-supply.
+
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/max77650.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#define MAX77650_INT_GPI_F_MSK		BIT(0)
+#define MAX77650_INT_GPI_R_MSK		BIT(1)
+#define MAX77650_INT_GPI_MSK \
+			(MAX77650_INT_GPI_F_MSK | MAX77650_INT_GPI_R_MSK)
+#define MAX77650_INT_nEN_F_MSK		BIT(2)
+#define MAX77650_INT_nEN_R_MSK		BIT(3)
+#define MAX77650_INT_TJAL1_R_MSK	BIT(4)
+#define MAX77650_INT_TJAL2_R_MSK	BIT(5)
+#define MAX77650_INT_DOD_R_MSK		BIT(6)
+
+#define MAX77650_INT_THM_MSK		BIT(0)
+#define MAX77650_INT_CHG_MSK		BIT(1)
+#define MAX77650_INT_CHGIN_MSK		BIT(2)
+#define MAX77650_INT_TJ_REG_MSK		BIT(3)
+#define MAX77650_INT_CHGIN_CTRL_MSK	BIT(4)
+#define MAX77650_INT_SYS_CTRL_MSK	BIT(5)
+#define MAX77650_INT_SYS_CNFG_MSK	BIT(6)
+
+#define MAX77650_INT_GLBL_OFFSET	0
+#define MAX77650_INT_CHG_OFFSET		1
+
+#define MAX77650_SBIA_LPM_MASK		BIT(5)
+#define MAX77650_SBIA_LPM_DISABLED	0x00
+
+enum {
+	MAX77650_INT_GPI = 0,
+	MAX77650_INT_nEN_F,
+	MAX77650_INT_nEN_R,
+	MAX77650_INT_TJAL1_R,
+	MAX77650_INT_TJAL2_R,
+	MAX77650_INT_DOD_R,
+	MAX77650_INT_THM,
+	MAX77650_INT_CHG,
+	MAX77650_INT_CHGIN,
+	MAX77650_INT_TJ_REG,
+	MAX77650_INT_CHGIN_CTRL,
+	MAX77650_INT_SYS_CTRL,
+	MAX77650_INT_SYS_CNFG,
+};
+
+enum {
+	MAX77650_CELL_REGULATOR = 0,
+	MAX77650_CELL_CHARGER,
+	MAX77650_CELL_GPIO,
+	MAX77650_CELL_LED,
+	MAX77650_CELL_ONKEY,
+	MAX77650_NUM_CELLS,
+};
+
+struct max77650_irq_mapping {
+	int cell_num;
+	const int *irqs;
+	const char *const *irq_names;
+	unsigned int num_irqs;
+};
+
+static const int max77650_charger_irqs[] = {
+	MAX77650_INT_CHG,
+	MAX77650_INT_CHGIN,
+};
+
+static const int max77650_gpio_irqs[] = {
+	MAX77650_INT_GPI,
+};
+
+static const int max77650_onkey_irqs[] = {
+	MAX77650_INT_nEN_F,
+	MAX77650_INT_nEN_R,
+};
+
+static const char *const max77650_charger_irq_names[] = {
+	"CHG",
+	"CHGIN",
+};
+
+static const char *const max77650_gpio_irq_names[] = {
+	"GPI",
+};
+
+static const char *const max77650_onkey_irq_names[] = {
+	"nEN_F",
+	"nEN_R",
+};
+
+static const struct max77650_irq_mapping max77650_irq_mapping_table[] = {
+	{
+		.cell_num	= MAX77650_CELL_CHARGER,
+		.irqs		= max77650_charger_irqs,
+		.irq_names	= max77650_charger_irq_names,
+		.num_irqs	= ARRAY_SIZE(max77650_charger_irqs),
+	},
+	{
+		.cell_num	= MAX77650_CELL_GPIO,
+		.irqs		= max77650_gpio_irqs,
+		.irq_names	= max77650_gpio_irq_names,
+		.num_irqs	= ARRAY_SIZE(max77650_gpio_irqs),
+	},
+	{
+		.cell_num	= MAX77650_CELL_ONKEY,
+		.irqs		= max77650_onkey_irqs,
+		.irq_names	= max77650_onkey_irq_names,
+		.num_irqs	= ARRAY_SIZE(max77650_onkey_irqs),
+	},
+};
+
+static const struct mfd_cell max77650_cells[] = {
+	[MAX77650_CELL_REGULATOR] = {
+		.name		= "max77650-regulator",
+		.of_compatible	= "maxim,max77650-regulator",
+	},
+	[MAX77650_CELL_CHARGER] = {
+		.name		= "max77650-charger",
+		.of_compatible	= "maxim,max77650-charger",
+	},
+	[MAX77650_CELL_GPIO] = {
+		.name		= "max77650-gpio",
+		.of_compatible	= "maxim,max77650-gpio",
+	},
+	[MAX77650_CELL_LED] = {
+		.name		= "max77650-led",
+		.of_compatible	= "maxim,max77650-led",
+	},
+	[MAX77650_CELL_ONKEY] = {
+		.name		= "max77650-onkey",
+		.of_compatible	= "maxim,max77650-onkey",
+	},
+};
+
+static const struct regmap_irq max77650_irqs[] = {
+	[MAX77650_INT_GPI] = {
+		.reg_offset		= MAX77650_INT_GLBL_OFFSET,
+		.mask			= MAX77650_INT_GPI_MSK,
+		.type = {
+			.type_falling_val	= MAX77650_INT_GPI_F_MSK,
+			.type_rising_val	= MAX77650_INT_GPI_R_MSK,
+			.types_supported	= IRQ_TYPE_EDGE_BOTH,
+		},
+	},
+	[MAX77650_INT_nEN_F] = {
+		.reg_offset		= MAX77650_INT_GLBL_OFFSET,
+		.mask			= MAX77650_INT_nEN_F_MSK,
+	},
+	[MAX77650_INT_nEN_R] = {
+		.reg_offset		= MAX77650_INT_GLBL_OFFSET,
+		.mask			= MAX77650_INT_nEN_R_MSK,
+	},
+	[MAX77650_INT_TJAL1_R] = {
+		.reg_offset		= MAX77650_INT_GLBL_OFFSET,
+		.mask			= MAX77650_INT_TJAL1_R_MSK,
+	},
+	[MAX77650_INT_TJAL2_R] = {
+		.reg_offset		= MAX77650_INT_GLBL_OFFSET,
+		.mask			= MAX77650_INT_TJAL2_R_MSK,
+	},
+	[MAX77650_INT_DOD_R] = {
+		.reg_offset		= MAX77650_INT_GLBL_OFFSET,
+		.mask			= MAX77650_INT_DOD_R_MSK,
+	},
+	[MAX77650_INT_THM] = {
+		.reg_offset		= MAX77650_INT_CHG_OFFSET,
+		.mask			= MAX77650_INT_THM_MSK,
+	},
+	[MAX77650_INT_CHG] = {
+		.reg_offset		= MAX77650_INT_CHG_OFFSET,
+		.mask			= MAX77650_INT_CHG_MSK,
+	},
+	[MAX77650_INT_CHGIN] = {
+		.reg_offset		= MAX77650_INT_CHG_OFFSET,
+		.mask			= MAX77650_INT_CHGIN_MSK,
+	},
+	[MAX77650_INT_TJ_REG] = {
+		.reg_offset		= MAX77650_INT_CHG_OFFSET,
+		.mask			= MAX77650_INT_TJ_REG_MSK,
+	},
+	[MAX77650_INT_CHGIN_CTRL] = {
+		.reg_offset		= MAX77650_INT_CHG_OFFSET,
+		.mask			= MAX77650_INT_CHGIN_CTRL_MSK,
+	},
+	[MAX77650_INT_SYS_CTRL] = {
+		.reg_offset		= MAX77650_INT_CHG_OFFSET,
+		.mask			= MAX77650_INT_SYS_CTRL_MSK,
+	},
+	[MAX77650_INT_SYS_CNFG] = {
+		.reg_offset		= MAX77650_INT_CHG_OFFSET,
+		.mask			= MAX77650_INT_SYS_CNFG_MSK,
+	},
+};
+
+static const struct regmap_irq_chip max77650_irq_chip = {
+	.name			= "max77650-irq",
+	.irqs			= max77650_irqs,
+	.num_irqs		= ARRAY_SIZE(max77650_irqs),
+	.num_regs		= 2,
+	.status_base		= MAX77650_REG_INT_GLBL,
+	.mask_base		= MAX77650_REG_INTM_GLBL,
+	.type_in_mask		= true,
+	.type_invert		= true,
+	.init_ack_masked	= true,
+	.clear_on_unmask	= true,
+};
+
+static const struct regmap_config max77650_regmap_config = {
+	.name		= "max77650",
+	.reg_bits	= 8,
+	.val_bits	= 8,
+};
+
+static int max77650_setup_irqs(struct device *dev, struct mfd_cell *cells)
+{
+	const struct max77650_irq_mapping *mapping;
+	struct regmap_irq_chip_data *irq_data;
+	struct i2c_client *i2c;
+	struct mfd_cell *cell;
+	struct resource *res;
+	struct regmap *map;
+	int i, j, irq, rv;
+
+	i2c = to_i2c_client(dev);
+
+	map = dev_get_regmap(dev, NULL);
+	if (!map)
+		return -ENODEV;
+
+	rv = devm_regmap_add_irq_chip(dev, map, i2c->irq,
+				      IRQF_ONESHOT | IRQF_SHARED, -1,
+				      &max77650_irq_chip, &irq_data);
+	if (rv)
+		return rv;
+
+	for (i = 0; i < ARRAY_SIZE(max77650_irq_mapping_table); i++) {
+		mapping = &max77650_irq_mapping_table[i];
+		cell = &cells[mapping->cell_num];
+
+		res = devm_kcalloc(dev, sizeof(*res),
+				   mapping->num_irqs, GFP_KERNEL);
+		if (!res)
+			return -ENOMEM;
+
+		cell->resources = res;
+		cell->num_resources = mapping->num_irqs;
+
+		for (j = 0; j < mapping->num_irqs; j++) {
+			irq = regmap_irq_get_virq(irq_data, mapping->irqs[j]);
+			if (irq < 0)
+				return irq;
+
+			res[j].start = res[j].end = irq;
+			res[j].flags = IORESOURCE_IRQ;
+			res[j].name = mapping->irq_names[j];
+		}
+	}
+
+	return 0;
+}
+
+static int max77650_i2c_probe(struct i2c_client *i2c)
+{
+	struct device *dev = &i2c->dev;
+	struct mfd_cell *cells;
+	struct regmap *map;
+	unsigned int val;
+	int rv;
+
+	map = devm_regmap_init_i2c(i2c, &max77650_regmap_config);
+	if (IS_ERR(map))
+		return PTR_ERR(map);
+
+	rv = regmap_read(map, MAX77650_REG_CID, &val);
+	if (rv)
+		return rv;
+
+	switch (MAX77650_CID_BITS(val)) {
+	case MAX77650_CID_77650A:
+	case MAX77650_CID_77650C:
+	case MAX77650_CID_77651A:
+	case MAX77650_CID_77651B:
+		break;
+	default:
+		return -ENODEV;
+	}
+
+	/*
+	 * This IC has a low-power mode which reduces the quiescent current
+	 * consumption to ~5.6uA but is only suitable for systems consuming
+	 * less than ~2mA. Since this is not likely the case even on
+	 * linux-based wearables - keep the chip in normal power mode.
+	 */
+	rv = regmap_update_bits(map,
+				MAX77650_REG_CNFG_GLBL,
+				MAX77650_SBIA_LPM_MASK,
+				MAX77650_SBIA_LPM_DISABLED);
+	if (rv)
+		return rv;
+
+	cells = devm_kmemdup(dev, max77650_cells,
+			     sizeof(max77650_cells), GFP_KERNEL);
+	if (!cells)
+		return -ENOMEM;
+
+	rv = max77650_setup_irqs(dev, cells);
+	if (rv)
+		return rv;
+
+	return devm_mfd_add_devices(dev, -1, cells,
+				    MAX77650_NUM_CELLS, NULL, 0, NULL);
+}
+
+static const struct of_device_id max77650_of_match[] = {
+	{ .compatible = "maxim,max77650" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, max77650_of_match);
+
+static struct i2c_driver max77650_i2c_driver = {
+	.driver = {
+		.name = "max77650",
+		.of_match_table = of_match_ptr(max77650_of_match),
+	},
+	.probe_new = max77650_i2c_probe,
+};
+module_i2c_driver(max77650_i2c_driver);
+
+MODULE_DESCRIPTION("MAXIM 77650/77651 multi-function core driver");
+MODULE_AUTHOR("Bartosz Golaszewski <bgolaszewski@baylibre.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/max77650.h b/include/linux/mfd/max77650.h
new file mode 100644
index 000000000000..c809e211a8cd
--- /dev/null
+++ b/include/linux/mfd/max77650.h
@@ -0,0 +1,59 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2018 BayLibre SAS
+ * Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
+ *
+ * Common definitions for MAXIM 77650/77651 charger/power-supply.
+ */
+
+#ifndef MAX77650_H
+#define MAX77650_H
+
+#include <linux/bits.h>
+
+#define MAX77650_REG_INT_GLBL		0x00
+#define MAX77650_REG_INT_CHG		0x01
+#define MAX77650_REG_STAT_CHG_A		0x02
+#define MAX77650_REG_STAT_CHG_B		0x03
+#define MAX77650_REG_ERCFLAG		0x04
+#define MAX77650_REG_STAT_GLBL		0x05
+#define MAX77650_REG_INTM_GLBL		0x06
+#define MAX77650_REG_INTM_CHG		0x07
+#define MAX77650_REG_CNFG_GLBL		0x10
+#define MAX77650_REG_CID		0x11
+#define MAX77650_REG_CNFG_GPIO		0x12
+#define MAX77650_REG_CNFG_CHG_A		0x18
+#define MAX77650_REG_CNFG_CHG_B		0x19
+#define MAX77650_REG_CNFG_CHG_C		0x1a
+#define MAX77650_REG_CNFG_CHG_D		0x1b
+#define MAX77650_REG_CNFG_CHG_E		0x1c
+#define MAX77650_REG_CNFG_CHG_F		0x1d
+#define MAX77650_REG_CNFG_CHG_G		0x1e
+#define MAX77650_REG_CNFG_CHG_H		0x1f
+#define MAX77650_REG_CNFG_CHG_I		0x20
+#define MAX77650_REG_CNFG_SBB_TOP	0x28
+#define MAX77650_REG_CNFG_SBB0_A	0x29
+#define MAX77650_REG_CNFG_SBB0_B	0x2a
+#define MAX77650_REG_CNFG_SBB1_A	0x2b
+#define MAX77650_REG_CNFG_SBB1_B	0x2c
+#define MAX77650_REG_CNFG_SBB2_A	0x2d
+#define MAX77650_REG_CNFG_SBB2_B	0x2e
+#define MAX77650_REG_CNFG_LDO_A		0x38
+#define MAX77650_REG_CNFG_LDO_B		0x39
+#define MAX77650_REG_CNFG_LED0_A	0x40
+#define MAX77650_REG_CNFG_LED1_A	0x41
+#define MAX77650_REG_CNFG_LED2_A	0x42
+#define MAX77650_REG_CNFG_LED0_B	0x43
+#define MAX77650_REG_CNFG_LED1_B	0x44
+#define MAX77650_REG_CNFG_LED2_B	0x45
+#define MAX77650_REG_CNFG_LED_TOP	0x46
+
+#define MAX77650_CID_MASK		GENMASK(3, 0)
+#define MAX77650_CID_BITS(_reg)		(_reg & MAX77650_CID_MASK)
+
+#define MAX77650_CID_77650A		0x03
+#define MAX77650_CID_77650C		0x0a
+#define MAX77650_CID_77651A		0x06
+#define MAX77650_CID_77651B		0x08
+
+#endif /* MAX77650_H */
-- 
2.20.1

[PATCH v4 06/10] power: supply: max77650: add support for battery charger

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Add basic support for the battery charger for max77650 PMIC.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/power/supply/Kconfig            |   7 +
 drivers/power/supply/Makefile           |   1 +
 drivers/power/supply/max77650-charger.c | 355 ++++++++++++++++++++++++
 3 files changed, 363 insertions(+)
 create mode 100644 drivers/power/supply/max77650-charger.c

diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index e901b9879e7e..0230c96fa94d 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -499,6 +499,13 @@ config CHARGER_DETECTOR_MAX14656
 	  Revision 1.2 and can be found e.g. in Kindle 4/5th generation
 	  readers and certain LG devices.
 
+config CHARGER_MAX77650
+	tristate "Maxim MAX77650 battery charger driver"
+	depends on MFD_MAX77650
+	help
+	  Say Y to enable support for the battery charger control of MAX77650
+	  PMICs.
+
 config CHARGER_MAX77693
 	tristate "Maxim MAX77693 battery charger driver"
 	depends on MFD_MAX77693
diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
index b731c2a9b695..b73eb8c5c1a9 100644
--- a/drivers/power/supply/Makefile
+++ b/drivers/power/supply/Makefile
@@ -70,6 +70,7 @@ obj-$(CONFIG_CHARGER_MANAGER)	+= charger-manager.o
 obj-$(CONFIG_CHARGER_LTC3651)	+= ltc3651-charger.o
 obj-$(CONFIG_CHARGER_MAX14577)	+= max14577_charger.o
 obj-$(CONFIG_CHARGER_DETECTOR_MAX14656)	+= max14656_charger_detector.o
+obj-$(CONFIG_CHARGER_MAX77650)	+= max77650-charger.o
 obj-$(CONFIG_CHARGER_MAX77693)	+= max77693_charger.o
 obj-$(CONFIG_CHARGER_MAX8997)	+= max8997_charger.o
 obj-$(CONFIG_CHARGER_MAX8998)	+= max8998_charger.o
diff --git a/drivers/power/supply/max77650-charger.c b/drivers/power/supply/max77650-charger.c
new file mode 100644
index 000000000000..7055c9b5ee24
--- /dev/null
+++ b/drivers/power/supply/max77650-charger.c
@@ -0,0 +1,355 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (C) 2018 BayLibre SAS
+// Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
+//
+// Battery charger driver for MAXIM 77650/77651 charger/power-supply.
+
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/max77650.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/power_supply.h>
+#include <linux/regmap.h>
+
+#define MAX77650_CHARGER_ENABLED		BIT(0)
+#define MAX77650_CHARGER_DISABLED		0x00
+#define MAX77650_CHARGER_CHG_EN_MASK		BIT(0)
+
+#define MAX77650_CHARGER_CHG_DTLS_MASK		GENMASK(7, 4)
+#define MAX77650_CHARGER_CHG_DTLS_BITS(_reg) \
+		(((_reg) & MAX77650_CHARGER_CHG_DTLS_MASK) >> 4)
+
+#define MAX77650_CHARGER_CHG_OFF		0x00
+#define MAX77650_CHARGER_CHG_PREQ		0x01
+#define MAX77650_CHARGER_CHG_ON_CURR		0x02
+#define MAX77650_CHARGER_CHG_ON_JCURR		0x03
+#define MAX77650_CHARGER_CHG_ON_VOLT		0x04
+#define MAX77650_CHARGER_CHG_ON_JVOLT		0x05
+#define MAX77650_CHARGER_CHG_ON_TOPOFF		0x06
+#define MAX77650_CHARGER_CHG_ON_JTOPOFF		0x07
+#define MAX77650_CHARGER_CHG_DONE		0x08
+#define MAX77650_CHARGER_CHG_JDONE		0x09
+#define MAX77650_CHARGER_CHG_SUSP_PF		0x0a
+#define MAX77650_CHARGER_CHG_SUSP_FCF		0x0b
+#define MAX77650_CHARGER_CHG_SUSP_BTF		0x0c
+
+#define MAX77650_CHARGER_CHGIN_DTLS_MASK	GENMASK(3, 2)
+#define MAX77650_CHARGER_CHGIN_DTLS_BITS(_reg) \
+		(((_reg) & MAX77650_CHARGER_CHGIN_DTLS_MASK) >> 2)
+
+#define MAX77650_CHARGER_CHGIN_UVL		0x00
+#define MAX77650_CHARGER_CHGIN_OVL		0x01
+#define MAX77650_CHARGER_CHGIN_OKAY		0x11
+
+#define MAX77650_CHARGER_CHG_MASK	BIT(1)
+#define MAX77650_CHARGER_CHG_CHARGING(_reg) \
+		(((_reg) & MAX77650_CHARGER_CHG_MASK) > 1)
+
+#define MAX77650_CHARGER_VCHGIN_MIN_MASK	0xc0
+#define MAX77650_CHARGER_VCHGIN_MIN_SHIFT(_val)	((_val) << 5)
+
+#define MAX77650_CHARGER_ICHGIN_LIM_MASK	0x1c
+#define MAX77650_CHARGER_ICHGIN_LIM_SHIFT(_val)	((_val) << 2)
+
+struct max77650_charger_data {
+	struct regmap *map;
+	struct device *dev;
+};
+
+static enum power_supply_property max77650_charger_properties[] = {
+	POWER_SUPPLY_PROP_STATUS,
+	POWER_SUPPLY_PROP_ONLINE,
+	POWER_SUPPLY_PROP_CHARGE_TYPE
+};
+
+static const unsigned int max77650_charger_vchgin_min_table[] = {
+	4000000, 4100000, 4200000, 4300000, 4400000, 4500000, 4600000, 4700000
+};
+
+static const unsigned int max77650_charger_ichgin_lim_table[] = {
+	95000, 190000, 285000, 380000, 475000
+};
+
+static int max77650_charger_set_vchgin_min(struct max77650_charger_data *chg,
+					   unsigned int val)
+{
+	int i, rv;
+
+	for (i = 0; i < ARRAY_SIZE(max77650_charger_vchgin_min_table); i++) {
+		if (val == max77650_charger_vchgin_min_table[i]) {
+			rv = regmap_update_bits(chg->map,
+					MAX77650_REG_CNFG_CHG_B,
+					MAX77650_CHARGER_VCHGIN_MIN_MASK,
+					MAX77650_CHARGER_VCHGIN_MIN_SHIFT(i));
+			if (rv)
+				return rv;
+
+			return 0;
+		}
+	}
+
+	return -EINVAL;
+}
+
+static int max77650_charger_set_ichgin_lim(struct max77650_charger_data *chg,
+					   unsigned int val)
+{
+	int i, rv;
+
+	for (i = 0; i < ARRAY_SIZE(max77650_charger_ichgin_lim_table); i++) {
+		if (val == max77650_charger_ichgin_lim_table[i]) {
+			rv = regmap_update_bits(chg->map,
+					MAX77650_REG_CNFG_CHG_B,
+					MAX77650_CHARGER_ICHGIN_LIM_MASK,
+					MAX77650_CHARGER_ICHGIN_LIM_SHIFT(i));
+			if (rv)
+				return rv;
+
+			return 0;
+		}
+	}
+
+	return -EINVAL;
+}
+
+static void max77650_charger_enable(struct max77650_charger_data *chg)
+{
+	int rv;
+
+	rv = regmap_update_bits(chg->map,
+				MAX77650_REG_CNFG_CHG_B,
+				MAX77650_CHARGER_CHG_EN_MASK,
+				MAX77650_CHARGER_ENABLED);
+	if (rv)
+		dev_err(chg->dev, "unable to enable the charger: %d\n", rv);
+}
+
+static void max77650_charger_disable(struct max77650_charger_data *chg)
+{
+	int rv;
+
+	rv = regmap_update_bits(chg->map,
+				MAX77650_REG_CNFG_CHG_B,
+				MAX77650_CHARGER_CHG_EN_MASK,
+				MAX77650_CHARGER_DISABLED);
+	if (rv)
+		dev_err(chg->dev, "unable to disable the charger: %d\n", rv);
+}
+
+static irqreturn_t max77650_charger_check_status(int irq, void *data)
+{
+	struct max77650_charger_data *chg = data;
+	int rv, reg;
+
+	rv = regmap_read(chg->map, MAX77650_REG_STAT_CHG_B, &reg);
+	if (rv) {
+		dev_err(chg->dev,
+			"unable to read the charger status: %d\n", rv);
+		return IRQ_HANDLED;
+	}
+
+	switch (MAX77650_CHARGER_CHGIN_DTLS_BITS(reg)) {
+	case MAX77650_CHARGER_CHGIN_UVL:
+		dev_err(chg->dev, "undervoltage lockout detected, disabling charger\n");
+		max77650_charger_disable(chg);
+		break;
+	case MAX77650_CHARGER_CHGIN_OVL:
+		dev_err(chg->dev, "overvoltage lockout detected, disabling charger\n");
+		max77650_charger_disable(chg);
+		break;
+	case MAX77650_CHARGER_CHGIN_OKAY:
+		max77650_charger_enable(chg);
+		break;
+	default:
+		/* May be 0x10 - debouncing */
+		break;
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int max77650_charger_get_property(struct power_supply *psy,
+					 enum power_supply_property psp,
+					 union power_supply_propval *val)
+{
+	struct max77650_charger_data *chg = power_supply_get_drvdata(psy);
+	int rv, reg;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_STATUS:
+		rv = regmap_read(chg->map, MAX77650_REG_STAT_CHG_B, &reg);
+		if (rv)
+			return rv;
+
+		if (MAX77650_CHARGER_CHG_CHARGING(reg)) {
+			val->intval = POWER_SUPPLY_STATUS_CHARGING;
+			break;
+		}
+
+		switch (MAX77650_CHARGER_CHG_DTLS_BITS(reg)) {
+		case MAX77650_CHARGER_CHG_OFF:
+		case MAX77650_CHARGER_CHG_SUSP_PF:
+		case MAX77650_CHARGER_CHG_SUSP_FCF:
+		case MAX77650_CHARGER_CHG_SUSP_BTF:
+			val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
+			break;
+		case MAX77650_CHARGER_CHG_PREQ:
+		case MAX77650_CHARGER_CHG_ON_CURR:
+		case MAX77650_CHARGER_CHG_ON_JCURR:
+		case MAX77650_CHARGER_CHG_ON_VOLT:
+		case MAX77650_CHARGER_CHG_ON_JVOLT:
+		case MAX77650_CHARGER_CHG_ON_TOPOFF:
+		case MAX77650_CHARGER_CHG_ON_JTOPOFF:
+			val->intval = POWER_SUPPLY_STATUS_CHARGING;
+			break;
+		case MAX77650_CHARGER_CHG_DONE:
+			val->intval = POWER_SUPPLY_STATUS_FULL;
+			break;
+		default:
+			val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
+		}
+		break;
+	case POWER_SUPPLY_PROP_ONLINE:
+		rv = regmap_read(chg->map, MAX77650_REG_STAT_CHG_B, &reg);
+		if (rv)
+			return rv;
+
+		val->intval = MAX77650_CHARGER_CHG_CHARGING(reg);
+		break;
+	case POWER_SUPPLY_PROP_CHARGE_TYPE:
+		rv = regmap_read(chg->map, MAX77650_REG_STAT_CHG_B, &reg);
+		if (rv)
+			return rv;
+
+		if (!MAX77650_CHARGER_CHG_CHARGING(reg)) {
+			val->intval = POWER_SUPPLY_CHARGE_TYPE_NONE;
+			break;
+		}
+
+		switch (MAX77650_CHARGER_CHG_DTLS_BITS(reg)) {
+		case MAX77650_CHARGER_CHG_PREQ:
+		case MAX77650_CHARGER_CHG_ON_CURR:
+		case MAX77650_CHARGER_CHG_ON_JCURR:
+		case MAX77650_CHARGER_CHG_ON_VOLT:
+		case MAX77650_CHARGER_CHG_ON_JVOLT:
+			val->intval = POWER_SUPPLY_CHARGE_TYPE_FAST;
+			break;
+		case MAX77650_CHARGER_CHG_ON_TOPOFF:
+		case MAX77650_CHARGER_CHG_ON_JTOPOFF:
+			val->intval = POWER_SUPPLY_CHARGE_TYPE_TRICKLE;
+			break;
+		default:
+			val->intval = POWER_SUPPLY_CHARGE_TYPE_UNKNOWN;
+		}
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static const struct power_supply_desc max77650_battery_desc = {
+	.name		= "max77650",
+	.type		= POWER_SUPPLY_TYPE_USB,
+	.get_property	= max77650_charger_get_property,
+	.properties	= max77650_charger_properties,
+	.num_properties	= ARRAY_SIZE(max77650_charger_properties),
+};
+
+static int max77650_charger_probe(struct platform_device *pdev)
+{
+	struct power_supply_config pscfg = {};
+	struct max77650_charger_data *chg;
+	struct power_supply *battery;
+	struct device *dev, *parent;
+	int rv, chg_irq, chgin_irq;
+	unsigned int prop;
+
+	dev = &pdev->dev;
+	parent = dev->parent;
+
+	chg = devm_kzalloc(dev, sizeof(*chg), GFP_KERNEL);
+	if (!chg)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, chg);
+
+	chg->map = dev_get_regmap(parent, NULL);
+	if (!chg->map)
+		return -ENODEV;
+
+	chg->dev = dev;
+
+	pscfg.of_node = dev->of_node;
+	pscfg.drv_data = chg;
+
+	chg_irq = platform_get_irq_byname(pdev, "CHG");
+	if (chg_irq < 0)
+		return chg_irq;
+
+	chgin_irq = platform_get_irq_byname(pdev, "CHGIN");
+	if (chgin_irq < 0)
+		return chgin_irq;
+
+	rv = devm_request_any_context_irq(dev, chg_irq,
+					  max77650_charger_check_status,
+					  IRQF_ONESHOT, "chg", chg);
+	if (rv < 0)
+		return rv;
+
+	rv = devm_request_any_context_irq(dev, chgin_irq,
+					  max77650_charger_check_status,
+					  IRQF_ONESHOT, "chgin", chg);
+	if (rv < 0)
+		return rv;
+
+	battery = devm_power_supply_register(dev,
+					     &max77650_battery_desc, &pscfg);
+	if (IS_ERR(battery))
+		return PTR_ERR(battery);
+
+	rv = of_property_read_u32(dev->of_node, "maxim,vchgin-min", &prop);
+	if (rv == 0) {
+		rv = max77650_charger_set_vchgin_min(chg, prop);
+		if (rv)
+			return rv;
+	}
+
+	rv = of_property_read_u32(dev->of_node, "maxim,ichgin-lim", &prop);
+	if (rv == 0) {
+		rv = max77650_charger_set_ichgin_lim(chg, prop);
+		if (rv)
+			return rv;
+	}
+
+	return regmap_update_bits(chg->map,
+				  MAX77650_REG_CNFG_CHG_B,
+				  MAX77650_CHARGER_CHG_EN_MASK,
+				  MAX77650_CHARGER_ENABLED);
+}
+
+static int max77650_charger_remove(struct platform_device *pdev)
+{
+	struct max77650_charger_data *chg = platform_get_drvdata(pdev);
+
+	return regmap_update_bits(chg->map,
+				  MAX77650_REG_CNFG_CHG_B,
+				  MAX77650_CHARGER_CHG_EN_MASK,
+				  MAX77650_CHARGER_DISABLED);
+}
+
+static struct platform_driver max77650_charger_driver = {
+	.driver = {
+		.name = "max77650-charger",
+	},
+	.probe = max77650_charger_probe,
+	.remove = max77650_charger_remove,
+};
+module_platform_driver(max77650_charger_driver);
+
+MODULE_DESCRIPTION("MAXIM 77650/77651 charger driver");
+MODULE_AUTHOR("Bartosz Golaszewski <bgolaszewski@baylibre.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.20.1

[PATCH v4 07/10] gpio: max77650: add GPIO support

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Add GPIO support for max77650 mfd device. This PMIC exposes a single
GPIO line.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/Kconfig         |   7 ++
 drivers/gpio/Makefile        |   1 +
 drivers/gpio/gpio-max77650.c | 190 +++++++++++++++++++++++++++++++++++
 3 files changed, 198 insertions(+)
 create mode 100644 drivers/gpio/gpio-max77650.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index b5a2845347ec..fb297fe5bfec 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1095,6 +1095,13 @@ config GPIO_MAX77620
 	  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_MAX77650
+	tristate "Maxim MAX77650/77651 GPIO support"
+	depends on MFD_MAX77650
+	help
+	  GPIO driver for MAX77650/77651 PMIC from Maxim Semiconductor.
+	  These chips have a single pin that can be configured as GPIO.
+
 config GPIO_MSIC
 	bool "Intel MSIC mixed signal gpio support"
 	depends on (X86 || COMPILE_TEST) && MFD_INTEL_MSIC
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 37628f8dbf70..8bdad50db822 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -78,6 +78,7 @@ 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_MAX77650)	+= gpio-max77650.o
 obj-$(CONFIG_GPIO_MB86S7X)	+= gpio-mb86s7x.o
 obj-$(CONFIG_GPIO_MENZ127)	+= gpio-menz127.o
 obj-$(CONFIG_GPIO_MERRIFIELD)	+= gpio-merrifield.o
diff --git a/drivers/gpio/gpio-max77650.c b/drivers/gpio/gpio-max77650.c
new file mode 100644
index 000000000000..3f03f4e8956c
--- /dev/null
+++ b/drivers/gpio/gpio-max77650.c
@@ -0,0 +1,190 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (C) 2018 BayLibre SAS
+// Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
+//
+// GPIO driver for MAXIM 77650/77651 charger/power-supply.
+
+#include <linux/gpio/driver.h>
+#include <linux/i2c.h>
+#include <linux/mfd/max77650.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define MAX77650_GPIO_DIR_MASK		BIT(0)
+#define MAX77650_GPIO_INVAL_MASK	BIT(1)
+#define MAX77650_GPIO_DRV_MASK		BIT(2)
+#define MAX77650_GPIO_OUTVAL_MASK	BIT(3)
+#define MAX77650_GPIO_DEBOUNCE_MASK	BIT(4)
+
+#define MAX77650_GPIO_DIR_OUT		0x00
+#define MAX77650_GPIO_DIR_IN		BIT(0)
+#define MAX77650_GPIO_OUT_LOW		0x00
+#define MAX77650_GPIO_OUT_HIGH		BIT(3)
+#define MAX77650_GPIO_DRV_OPEN_DRAIN	0x00
+#define MAX77650_GPIO_DRV_PUSH_PULL	BIT(2)
+#define MAX77650_GPIO_DEBOUNCE		BIT(4)
+
+#define MAX77650_GPIO_DIR_BITS(_reg) \
+		((_reg) & MAX77650_GPIO_DIR_MASK)
+#define MAX77650_GPIO_INVAL_BITS(_reg) \
+		(((_reg) & MAX77650_GPIO_INVAL_MASK) >> 1)
+
+struct max77650_gpio_chip {
+	struct regmap *map;
+	struct gpio_chip gc;
+	int irq;
+};
+
+static int max77650_gpio_direction_input(struct gpio_chip *gc,
+					 unsigned int offset)
+{
+	struct max77650_gpio_chip *chip = gpiochip_get_data(gc);
+
+	return regmap_update_bits(chip->map,
+				  MAX77650_REG_CNFG_GPIO,
+				  MAX77650_GPIO_DIR_MASK,
+				  MAX77650_GPIO_DIR_IN);
+}
+
+static int max77650_gpio_direction_output(struct gpio_chip *gc,
+					  unsigned int offset, int value)
+{
+	struct max77650_gpio_chip *chip = gpiochip_get_data(gc);
+	int mask, regval;
+
+	mask = MAX77650_GPIO_DIR_MASK | MAX77650_GPIO_OUTVAL_MASK;
+	regval = value ? MAX77650_GPIO_OUT_HIGH : MAX77650_GPIO_OUT_LOW;
+	regval |= MAX77650_GPIO_DIR_OUT;
+
+	return regmap_update_bits(chip->map,
+				  MAX77650_REG_CNFG_GPIO, mask, regval);
+}
+
+static void max77650_gpio_set_value(struct gpio_chip *gc,
+				    unsigned int offset, int value)
+{
+	struct max77650_gpio_chip *chip = gpiochip_get_data(gc);
+	int rv, regval;
+
+	regval = value ? MAX77650_GPIO_OUT_HIGH : MAX77650_GPIO_OUT_LOW;
+
+	rv = regmap_update_bits(chip->map, MAX77650_REG_CNFG_GPIO,
+				MAX77650_GPIO_OUTVAL_MASK, regval);
+	if (rv)
+		dev_err(gc->parent, "cannot set GPIO value: %d\n", rv);
+}
+
+static int max77650_gpio_get_value(struct gpio_chip *gc,
+				   unsigned int offset)
+{
+	struct max77650_gpio_chip *chip = gpiochip_get_data(gc);
+	unsigned int val;
+	int rv;
+
+	rv = regmap_read(chip->map, MAX77650_REG_CNFG_GPIO, &val);
+	if (rv)
+		return rv;
+
+	return MAX77650_GPIO_INVAL_BITS(val);
+}
+
+static int max77650_gpio_get_direction(struct gpio_chip *gc,
+				       unsigned int offset)
+{
+	struct max77650_gpio_chip *chip = gpiochip_get_data(gc);
+	unsigned int val;
+	int rv;
+
+	rv = regmap_read(chip->map, MAX77650_REG_CNFG_GPIO, &val);
+	if (rv)
+		return rv;
+
+	return MAX77650_GPIO_DIR_BITS(val);
+}
+
+static int max77650_gpio_set_config(struct gpio_chip *gc,
+				    unsigned int offset, unsigned long cfg)
+{
+	struct max77650_gpio_chip *chip = gpiochip_get_data(gc);
+
+	switch (pinconf_to_config_param(cfg)) {
+	case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+		return regmap_update_bits(chip->map,
+					  MAX77650_REG_CNFG_GPIO,
+					  MAX77650_GPIO_DRV_MASK,
+					  MAX77650_GPIO_DRV_OPEN_DRAIN);
+	case PIN_CONFIG_DRIVE_PUSH_PULL:
+		return regmap_update_bits(chip->map,
+					  MAX77650_REG_CNFG_GPIO,
+					  MAX77650_GPIO_DRV_MASK,
+					  MAX77650_GPIO_DRV_PUSH_PULL);
+	case PIN_CONFIG_INPUT_DEBOUNCE:
+		return regmap_update_bits(chip->map,
+					  MAX77650_REG_CNFG_GPIO,
+					  MAX77650_GPIO_DEBOUNCE_MASK,
+					  MAX77650_GPIO_DEBOUNCE);
+	default:
+		return -ENOTSUPP;
+	}
+}
+
+static int max77650_gpio_to_irq(struct gpio_chip *gc, unsigned int offset)
+{
+	struct max77650_gpio_chip *chip = gpiochip_get_data(gc);
+
+	return chip->irq;
+}
+
+static int max77650_gpio_probe(struct platform_device *pdev)
+{
+	struct max77650_gpio_chip *chip;
+	struct device *dev, *parent;
+	struct i2c_client *i2c;
+
+	dev = &pdev->dev;
+	parent = dev->parent;
+	i2c = to_i2c_client(parent);
+
+	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	chip->map = dev_get_regmap(parent, NULL);
+	if (!chip->map)
+		return -ENODEV;
+
+	chip->irq = platform_get_irq_byname(pdev, "GPI");
+	if (chip->irq < 0)
+		return chip->irq;
+
+	chip->gc.base = -1;
+	chip->gc.ngpio = 1;
+	chip->gc.label = i2c->name;
+	chip->gc.parent = dev;
+	chip->gc.owner = THIS_MODULE;
+	chip->gc.can_sleep = true;
+
+	chip->gc.direction_input = max77650_gpio_direction_input;
+	chip->gc.direction_output = max77650_gpio_direction_output;
+	chip->gc.set = max77650_gpio_set_value;
+	chip->gc.get = max77650_gpio_get_value;
+	chip->gc.get_direction = max77650_gpio_get_direction;
+	chip->gc.set_config = max77650_gpio_set_config;
+	chip->gc.to_irq = max77650_gpio_to_irq;
+
+	return devm_gpiochip_add_data(dev, &chip->gc, chip);
+}
+
+static struct platform_driver max77650_gpio_driver = {
+	.driver = {
+		.name = "max77650-gpio",
+	},
+	.probe = max77650_gpio_probe,
+};
+module_platform_driver(max77650_gpio_driver);
+
+MODULE_DESCRIPTION("MAXIM 77650/77651 GPIO driver");
+MODULE_AUTHOR("Bartosz Golaszewski <bgolaszewski@baylibre.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.20.1

[PATCH v4 08/10] leds: max77650: add LEDs support

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

This adds basic support for LEDs for the max77650 PMIC. The device has
three current sinks for driving LEDs.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
---
 drivers/leds/Kconfig         |   6 ++
 drivers/leds/Makefile        |   1 +
 drivers/leds/leds-max77650.c | 147 +++++++++++++++++++++++++++++++++++
 3 files changed, 154 insertions(+)
 create mode 100644 drivers/leds/leds-max77650.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index a72f97fca57b..6e7a8f51eccc 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -608,6 +608,12 @@ config LEDS_TLC591XX
 	  This option enables support for Texas Instruments TLC59108
 	  and TLC59116 LED controllers.
 
+config LEDS_MAX77650
+	tristate "LED support for Maxim MAX77650 PMIC"
+	depends on MFD_MAX77650
+	help
+	  LEDs driver for MAX77650 family of PMICs from Maxim Integrated."
+
 config LEDS_MAX77693
 	tristate "LED support for MAX77693 Flash"
 	depends on LEDS_CLASS_FLASH
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 4c1b0054f379..f48b2404dbb7 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -61,6 +61,7 @@ obj-$(CONFIG_LEDS_MC13783)		+= leds-mc13783.o
 obj-$(CONFIG_LEDS_NS2)			+= leds-ns2.o
 obj-$(CONFIG_LEDS_NETXBIG)		+= leds-netxbig.o
 obj-$(CONFIG_LEDS_ASIC3)		+= leds-asic3.o
+obj-$(CONFIG_LEDS_MAX77650)		+= leds-max77650.o
 obj-$(CONFIG_LEDS_MAX77693)		+= leds-max77693.o
 obj-$(CONFIG_LEDS_MAX8997)		+= leds-max8997.o
 obj-$(CONFIG_LEDS_LM355x)		+= leds-lm355x.o
diff --git a/drivers/leds/leds-max77650.c b/drivers/leds/leds-max77650.c
new file mode 100644
index 000000000000..6b74ce9cac12
--- /dev/null
+++ b/drivers/leds/leds-max77650.c
@@ -0,0 +1,147 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (C) 2018 BayLibre SAS
+// Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
+//
+// LED driver for MAXIM 77650/77651 charger/power-supply.
+
+#include <linux/i2c.h>
+#include <linux/leds.h>
+#include <linux/mfd/max77650.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define MAX77650_LED_NUM_LEDS		3
+
+#define MAX77650_LED_A_BASE		0x40
+#define MAX77650_LED_B_BASE		0x43
+
+#define MAX77650_LED_BR_MASK		GENMASK(4, 0)
+#define MAX77650_LED_EN_MASK		GENMASK(7, 6)
+
+#define MAX77650_LED_MAX_BRIGHTNESS	MAX77650_LED_BR_MASK
+
+/* Enable EN_LED_MSTR. */
+#define MAX77650_LED_TOP_DEFAULT	BIT(0)
+
+#define MAX77650_LED_ENABLE		GENMASK(7, 6)
+#define MAX77650_LED_DISABLE		0x00
+
+#define MAX77650_LED_A_DEFAULT		MAX77650_LED_DISABLE
+/* 100% on duty */
+#define MAX77650_LED_B_DEFAULT		GENMASK(3, 0)
+
+struct max77650_led {
+	struct led_classdev cdev;
+	struct regmap *map;
+	unsigned int regA;
+	unsigned int regB;
+};
+
+static struct max77650_led *max77650_to_led(struct led_classdev *cdev)
+{
+	return container_of(cdev, struct max77650_led, cdev);
+}
+
+static int max77650_led_brightness_set(struct led_classdev *cdev,
+				       enum led_brightness brightness)
+{
+	struct max77650_led *led = max77650_to_led(cdev);
+	int val, mask;
+
+	mask = MAX77650_LED_BR_MASK | MAX77650_LED_EN_MASK;
+
+	if (brightness == LED_OFF)
+		val = MAX77650_LED_DISABLE;
+	else
+		val = MAX77650_LED_ENABLE | brightness;
+
+	return regmap_update_bits(led->map, led->regA, mask, val);
+}
+
+static int max77650_led_probe(struct platform_device *pdev)
+{
+	struct device_node *of_node, *child;
+	struct max77650_led *leds, *led;
+	struct device *parent;
+	struct device *dev;
+	struct regmap *map;
+	const char *label;
+	int rv, num_leds;
+	u32 reg;
+
+	dev = &pdev->dev;
+	parent = dev->parent;
+	of_node = dev->of_node;
+
+	if (!of_node)
+		return -ENODEV;
+
+	leds = devm_kcalloc(dev, sizeof(*leds),
+			    MAX77650_LED_NUM_LEDS, GFP_KERNEL);
+	if (!leds)
+		return -ENOMEM;
+
+	map = dev_get_regmap(dev->parent, NULL);
+	if (!map)
+		return -ENODEV;
+
+	num_leds = of_get_child_count(of_node);
+	if (!num_leds || num_leds > MAX77650_LED_NUM_LEDS)
+		return -ENODEV;
+
+	for_each_child_of_node(of_node, child) {
+		rv = of_property_read_u32(child, "reg", &reg);
+		if (rv || reg >= MAX77650_LED_NUM_LEDS)
+			return -EINVAL;
+
+		led = &leds[reg];
+		led->map = map;
+		led->regA = MAX77650_LED_A_BASE + reg;
+		led->regB = MAX77650_LED_B_BASE + reg;
+		led->cdev.brightness_set_blocking = max77650_led_brightness_set;
+		led->cdev.max_brightness = MAX77650_LED_MAX_BRIGHTNESS;
+
+		label = of_get_property(child, "label", NULL);
+		if (!label) {
+			led->cdev.name = "max77650::";
+		} else {
+			led->cdev.name = devm_kasprintf(dev, GFP_KERNEL,
+							"max77650:%s", label);
+			if (!led->cdev.name)
+				return -ENOMEM;
+		}
+
+		of_property_read_string(child, "linux,default-trigger",
+					&led->cdev.default_trigger);
+
+		rv = devm_of_led_classdev_register(dev, child, &led->cdev);
+		if (rv)
+			return rv;
+
+		rv = regmap_write(map, led->regA, MAX77650_LED_A_DEFAULT);
+		if (rv)
+			return rv;
+
+		rv = regmap_write(map, led->regB, MAX77650_LED_B_DEFAULT);
+		if (rv)
+			return rv;
+	}
+
+	return regmap_write(map,
+			    MAX77650_REG_CNFG_LED_TOP,
+			    MAX77650_LED_TOP_DEFAULT);
+}
+
+static struct platform_driver max77650_led_driver = {
+	.driver = {
+		.name = "max77650-led",
+	},
+	.probe = max77650_led_probe,
+};
+module_platform_driver(max77650_led_driver);
+
+MODULE_DESCRIPTION("MAXIM 77650/77651 LED driver");
+MODULE_AUTHOR("Bartosz Golaszewski <bgolaszewski@baylibre.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.20.1

[PATCH v4 09/10] input: max77650: add onkey support

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Add support for the push- and slide-button events for max77650.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/Kconfig          |   9 ++
 drivers/input/misc/Makefile         |   1 +
 drivers/input/misc/max77650-onkey.c | 127 ++++++++++++++++++++++++++++
 3 files changed, 137 insertions(+)
 create mode 100644 drivers/input/misc/max77650-onkey.c

diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index ca59a2be9bc5..bb9c45c1269e 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -180,6 +180,15 @@ config INPUT_M68K_BEEP
 	tristate "M68k Beeper support"
 	depends on M68K
 
+config INPUT_MAX77650_ONKEY
+	tristate "Maxim MAX77650 ONKEY support"
+	depends on MFD_MAX77650
+	help
+	  Support the ONKEY of the MAX77650 PMIC as an input device.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called max77650-onkey.
+
 config INPUT_MAX77693_HAPTIC
 	tristate "MAXIM MAX77693/MAX77843 haptic controller support"
 	depends on (MFD_MAX77693 || MFD_MAX77843) && PWM
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index 9d0f9d1ff68f..5bd53590ce60 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -43,6 +43,7 @@ obj-$(CONFIG_INPUT_IXP4XX_BEEPER)	+= ixp4xx-beeper.o
 obj-$(CONFIG_INPUT_KEYSPAN_REMOTE)	+= keyspan_remote.o
 obj-$(CONFIG_INPUT_KXTJ9)		+= kxtj9.o
 obj-$(CONFIG_INPUT_M68K_BEEP)		+= m68kspkr.o
+obj-$(CONFIG_INPUT_MAX77650_ONKEY)	+= max77650-onkey.o
 obj-$(CONFIG_INPUT_MAX77693_HAPTIC)	+= max77693-haptic.o
 obj-$(CONFIG_INPUT_MAX8925_ONKEY)	+= max8925_onkey.o
 obj-$(CONFIG_INPUT_MAX8997_HAPTIC)	+= max8997_haptic.o
diff --git a/drivers/input/misc/max77650-onkey.c b/drivers/input/misc/max77650-onkey.c
new file mode 100644
index 000000000000..7fc3e9196abb
--- /dev/null
+++ b/drivers/input/misc/max77650-onkey.c
@@ -0,0 +1,127 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (C) 2018 BayLibre SAS
+// Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
+//
+// ONKEY driver for MAXIM 77650/77651 charger/power-supply.
+
+#include <linux/i2c.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/max77650.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define MAX77650_ONKEY_MODE_MASK	BIT(3)
+#define MAX77650_ONKEY_MODE_PUSH	0x00
+#define MAX77650_ONKEY_MODE_SLIDE	BIT(3)
+
+struct max77650_onkey {
+	struct input_dev *input;
+	unsigned int code;
+};
+
+static irqreturn_t max77650_onkey_falling(int irq, void *data)
+{
+	struct max77650_onkey *onkey = data;
+
+	input_report_key(onkey->input, onkey->code, 0);
+	input_sync(onkey->input);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t max77650_onkey_rising(int irq, void *data)
+{
+	struct max77650_onkey *onkey = data;
+
+	input_report_key(onkey->input, onkey->code, 1);
+	input_sync(onkey->input);
+
+	return IRQ_HANDLED;
+}
+
+static int max77650_onkey_probe(struct platform_device *pdev)
+{
+	int irq_r, irq_f, error, mode;
+	struct max77650_onkey *onkey;
+	struct device *dev, *parent;
+	const char *mode_prop;
+	struct regmap *map;
+
+	dev = &pdev->dev;
+	parent = dev->parent;
+
+	map = dev_get_regmap(parent, NULL);
+	if (!map)
+		return -ENODEV;
+
+	onkey = devm_kzalloc(dev, sizeof(*onkey), GFP_KERNEL);
+	if (!onkey)
+		return -ENOMEM;
+
+	error = device_property_read_u32(dev, "linux,code", &onkey->code);
+	if (error)
+		onkey->code = KEY_POWER;
+
+	error = device_property_read_string(dev,
+					    "maxim,onkey-mode", &mode_prop);
+	if (error)
+		mode_prop = "push";
+
+	if (strcmp(mode_prop, "push") == 0)
+		mode = MAX77650_ONKEY_MODE_PUSH;
+	else if (strcmp(mode_prop, "slide") == 0)
+		mode = MAX77650_ONKEY_MODE_SLIDE;
+	else
+		return -EINVAL;
+
+	error = regmap_update_bits(map, MAX77650_REG_CNFG_GLBL,
+				   MAX77650_ONKEY_MODE_MASK, mode);
+	if (error)
+		return error;
+
+	irq_f = platform_get_irq_byname(pdev, "nEN_F");
+	if (irq_f < 0)
+		return irq_f;
+
+	irq_r = platform_get_irq_byname(pdev, "nEN_R");
+	if (irq_r < 0)
+		return irq_r;
+
+	onkey->input = devm_input_allocate_device(dev);
+	if (!onkey->input)
+		return -ENOMEM;
+
+	onkey->input->name = "max77650_onkey";
+	onkey->input->phys = "max77650_onkey/input0";
+	onkey->input->id.bustype = BUS_I2C;
+	input_set_capability(onkey->input, EV_KEY, onkey->code);
+
+	error = devm_request_any_context_irq(dev, irq_f,
+					     max77650_onkey_falling,
+					     IRQF_ONESHOT, "onkey-down",
+					     onkey);
+	if (error < 0)
+		return error;
+
+	error = devm_request_any_context_irq(dev, irq_r, max77650_onkey_rising,
+					     IRQF_ONESHOT, "onkey-up", onkey);
+	if (error < 0)
+		return error;
+
+	return input_register_device(onkey->input);
+}
+
+static struct platform_driver max77650_onkey_driver = {
+	.driver = {
+		.name = "max77650-onkey",
+	},
+	.probe = max77650_onkey_probe,
+};
+module_platform_driver(max77650_onkey_driver);
+
+MODULE_DESCRIPTION("MAXIM 77650/77651 ONKEY driver");
+MODULE_AUTHOR("Bartosz Golaszewski <bgolaszewski@baylibre.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.20.1

[PATCH v4 10/10] MAINTAINERS: add an entry for max77650 mfd driver

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

I plan on extending this set of drivers so add myself as maintainer.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 MAINTAINERS | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 8c68de3cfd80..70106d30272b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9221,6 +9221,20 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/sound/max9860.txt
 F:	sound/soc/codecs/max9860.*
 
+MAXIM MAX77650 PMIC MFD DRIVER
+M:	Bartosz Golaszewski <bgolaszewski@baylibre.com>
+L:	linux-kernel@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/*/*max77650.txt
+F:	Documentation/devicetree/bindings/*/max77650*.txt
+F:	include/linux/mfd/max77650.h
+F:	drivers/mfd/max77650.c
+F:	drivers/regulator/max77650-regulator.c
+F:	drivers/power/supply/max77650-charger.c
+F:	drivers/input/misc/max77650-onkey.c
+F:	drivers/leds/leds-max77650.c
+F:	drivers/gpio/gpio-max77650.c
+
 MAXIM MAX77802 PMIC REGULATOR DEVICE DRIVER
 M:	Javier Martinez Canillas <javier@dowhile0.org>
 L:	linux-kernel@vger.kernel.org
-- 
2.20.1

Re: [PATCH v4 07/10] gpio: max77650: add GPIO support

[Linus Walleij]

On Tue, Feb 5, 2019 at 10:12 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Add GPIO support for max77650 mfd device. This PMIC exposes a single
> GPIO line.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Please merge this with the rest of the stuff through MFD!

Thanks,
Linus Walleij

Re: [PATCH v4 01/10] dt-bindings: mfd: add DT bindings for max77650

[Linus Walleij]

On Tue, Feb 5, 2019 at 10:12 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Add a DT binding document for max77650 ultra-low power PMIC. This
> describes the core mfd device and the GPIO module.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

This is a good solution!

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v4 06/10] power: supply: max77650: add support for battery charger

[Lee Jones]

Sebastian,

Just waiting on your Ack for the set.

Could you review this please?

> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Add basic support for the battery charger for max77650 PMIC.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/power/supply/Kconfig            |   7 +
>  drivers/power/supply/Makefile           |   1 +
>  drivers/power/supply/max77650-charger.c | 355 ++++++++++++++++++++++++
>  3 files changed, 363 insertions(+)
>  create mode 100644 drivers/power/supply/max77650-charger.c
> 
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index e901b9879e7e..0230c96fa94d 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -499,6 +499,13 @@ config CHARGER_DETECTOR_MAX14656
>  	  Revision 1.2 and can be found e.g. in Kindle 4/5th generation
>  	  readers and certain LG devices.
>  
> +config CHARGER_MAX77650
> +	tristate "Maxim MAX77650 battery charger driver"
> +	depends on MFD_MAX77650
> +	help
> +	  Say Y to enable support for the battery charger control of MAX77650
> +	  PMICs.
> +
>  config CHARGER_MAX77693
>  	tristate "Maxim MAX77693 battery charger driver"
>  	depends on MFD_MAX77693
> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
> index b731c2a9b695..b73eb8c5c1a9 100644
> --- a/drivers/power/supply/Makefile
> +++ b/drivers/power/supply/Makefile
> @@ -70,6 +70,7 @@ obj-$(CONFIG_CHARGER_MANAGER)	+= charger-manager.o
>  obj-$(CONFIG_CHARGER_LTC3651)	+= ltc3651-charger.o
>  obj-$(CONFIG_CHARGER_MAX14577)	+= max14577_charger.o
>  obj-$(CONFIG_CHARGER_DETECTOR_MAX14656)	+= max14656_charger_detector.o
> +obj-$(CONFIG_CHARGER_MAX77650)	+= max77650-charger.o
>  obj-$(CONFIG_CHARGER_MAX77693)	+= max77693_charger.o
>  obj-$(CONFIG_CHARGER_MAX8997)	+= max8997_charger.o
>  obj-$(CONFIG_CHARGER_MAX8998)	+= max8998_charger.o
> diff --git a/drivers/power/supply/max77650-charger.c b/drivers/power/supply/max77650-charger.c
> new file mode 100644
> index 000000000000..7055c9b5ee24
> --- /dev/null
> +++ b/drivers/power/supply/max77650-charger.c
> @@ -0,0 +1,355 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright (C) 2018 BayLibre SAS
> +// Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> +//
> +// Battery charger driver for MAXIM 77650/77651 charger/power-supply.
> +
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/max77650.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/power_supply.h>
> +#include <linux/regmap.h>
> +
> +#define MAX77650_CHARGER_ENABLED		BIT(0)
> +#define MAX77650_CHARGER_DISABLED		0x00
> +#define MAX77650_CHARGER_CHG_EN_MASK		BIT(0)
> +
> +#define MAX77650_CHARGER_CHG_DTLS_MASK		GENMASK(7, 4)
> +#define MAX77650_CHARGER_CHG_DTLS_BITS(_reg) \
> +		(((_reg) & MAX77650_CHARGER_CHG_DTLS_MASK) >> 4)
> +
> +#define MAX77650_CHARGER_CHG_OFF		0x00
> +#define MAX77650_CHARGER_CHG_PREQ		0x01
> +#define MAX77650_CHARGER_CHG_ON_CURR		0x02
> +#define MAX77650_CHARGER_CHG_ON_JCURR		0x03
> +#define MAX77650_CHARGER_CHG_ON_VOLT		0x04
> +#define MAX77650_CHARGER_CHG_ON_JVOLT		0x05
> +#define MAX77650_CHARGER_CHG_ON_TOPOFF		0x06
> +#define MAX77650_CHARGER_CHG_ON_JTOPOFF		0x07
> +#define MAX77650_CHARGER_CHG_DONE		0x08
> +#define MAX77650_CHARGER_CHG_JDONE		0x09
> +#define MAX77650_CHARGER_CHG_SUSP_PF		0x0a
> +#define MAX77650_CHARGER_CHG_SUSP_FCF		0x0b
> +#define MAX77650_CHARGER_CHG_SUSP_BTF		0x0c
> +
> +#define MAX77650_CHARGER_CHGIN_DTLS_MASK	GENMASK(3, 2)
> +#define MAX77650_CHARGER_CHGIN_DTLS_BITS(_reg) \
> +		(((_reg) & MAX77650_CHARGER_CHGIN_DTLS_MASK) >> 2)
> +
> +#define MAX77650_CHARGER_CHGIN_UVL		0x00
> +#define MAX77650_CHARGER_CHGIN_OVL		0x01
> +#define MAX77650_CHARGER_CHGIN_OKAY		0x11
> +
> +#define MAX77650_CHARGER_CHG_MASK	BIT(1)
> +#define MAX77650_CHARGER_CHG_CHARGING(_reg) \
> +		(((_reg) & MAX77650_CHARGER_CHG_MASK) > 1)
> +
> +#define MAX77650_CHARGER_VCHGIN_MIN_MASK	0xc0
> +#define MAX77650_CHARGER_VCHGIN_MIN_SHIFT(_val)	((_val) << 5)
> +
> +#define MAX77650_CHARGER_ICHGIN_LIM_MASK	0x1c
> +#define MAX77650_CHARGER_ICHGIN_LIM_SHIFT(_val)	((_val) << 2)
> +
> +struct max77650_charger_data {
> +	struct regmap *map;
> +	struct device *dev;
> +};
> +
> +static enum power_supply_property max77650_charger_properties[] = {
> +	POWER_SUPPLY_PROP_STATUS,
> +	POWER_SUPPLY_PROP_ONLINE,
> +	POWER_SUPPLY_PROP_CHARGE_TYPE
> +};
> +
> +static const unsigned int max77650_charger_vchgin_min_table[] = {
> +	4000000, 4100000, 4200000, 4300000, 4400000, 4500000, 4600000, 4700000
> +};
> +
> +static const unsigned int max77650_charger_ichgin_lim_table[] = {
> +	95000, 190000, 285000, 380000, 475000
> +};
> +
> +static int max77650_charger_set_vchgin_min(struct max77650_charger_data *chg,
> +					   unsigned int val)
> +{
> +	int i, rv;
> +
> +	for (i = 0; i < ARRAY_SIZE(max77650_charger_vchgin_min_table); i++) {
> +		if (val == max77650_charger_vchgin_min_table[i]) {
> +			rv = regmap_update_bits(chg->map,
> +					MAX77650_REG_CNFG_CHG_B,
> +					MAX77650_CHARGER_VCHGIN_MIN_MASK,
> +					MAX77650_CHARGER_VCHGIN_MIN_SHIFT(i));
> +			if (rv)
> +				return rv;
> +
> +			return 0;
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int max77650_charger_set_ichgin_lim(struct max77650_charger_data *chg,
> +					   unsigned int val)
> +{
> +	int i, rv;
> +
> +	for (i = 0; i < ARRAY_SIZE(max77650_charger_ichgin_lim_table); i++) {
> +		if (val == max77650_charger_ichgin_lim_table[i]) {
> +			rv = regmap_update_bits(chg->map,
> +					MAX77650_REG_CNFG_CHG_B,
> +					MAX77650_CHARGER_ICHGIN_LIM_MASK,
> +					MAX77650_CHARGER_ICHGIN_LIM_SHIFT(i));
> +			if (rv)
> +				return rv;
> +
> +			return 0;
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static void max77650_charger_enable(struct max77650_charger_data *chg)
> +{
> +	int rv;
> +
> +	rv = regmap_update_bits(chg->map,
> +				MAX77650_REG_CNFG_CHG_B,
> +				MAX77650_CHARGER_CHG_EN_MASK,
> +				MAX77650_CHARGER_ENABLED);
> +	if (rv)
> +		dev_err(chg->dev, "unable to enable the charger: %d\n", rv);
> +}
> +
> +static void max77650_charger_disable(struct max77650_charger_data *chg)
> +{
> +	int rv;
> +
> +	rv = regmap_update_bits(chg->map,
> +				MAX77650_REG_CNFG_CHG_B,
> +				MAX77650_CHARGER_CHG_EN_MASK,
> +				MAX77650_CHARGER_DISABLED);
> +	if (rv)
> +		dev_err(chg->dev, "unable to disable the charger: %d\n", rv);
> +}
> +
> +static irqreturn_t max77650_charger_check_status(int irq, void *data)
> +{
> +	struct max77650_charger_data *chg = data;
> +	int rv, reg;
> +
> +	rv = regmap_read(chg->map, MAX77650_REG_STAT_CHG_B, &reg);
> +	if (rv) {
> +		dev_err(chg->dev,
> +			"unable to read the charger status: %d\n", rv);
> +		return IRQ_HANDLED;
> +	}
> +
> +	switch (MAX77650_CHARGER_CHGIN_DTLS_BITS(reg)) {
> +	case MAX77650_CHARGER_CHGIN_UVL:
> +		dev_err(chg->dev, "undervoltage lockout detected, disabling charger\n");
> +		max77650_charger_disable(chg);
> +		break;
> +	case MAX77650_CHARGER_CHGIN_OVL:
> +		dev_err(chg->dev, "overvoltage lockout detected, disabling charger\n");
> +		max77650_charger_disable(chg);
> +		break;
> +	case MAX77650_CHARGER_CHGIN_OKAY:
> +		max77650_charger_enable(chg);
> +		break;
> +	default:
> +		/* May be 0x10 - debouncing */
> +		break;
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int max77650_charger_get_property(struct power_supply *psy,
> +					 enum power_supply_property psp,
> +					 union power_supply_propval *val)
> +{
> +	struct max77650_charger_data *chg = power_supply_get_drvdata(psy);
> +	int rv, reg;
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_STATUS:
> +		rv = regmap_read(chg->map, MAX77650_REG_STAT_CHG_B, &reg);
> +		if (rv)
> +			return rv;
> +
> +		if (MAX77650_CHARGER_CHG_CHARGING(reg)) {
> +			val->intval = POWER_SUPPLY_STATUS_CHARGING;
> +			break;
> +		}
> +
> +		switch (MAX77650_CHARGER_CHG_DTLS_BITS(reg)) {
> +		case MAX77650_CHARGER_CHG_OFF:
> +		case MAX77650_CHARGER_CHG_SUSP_PF:
> +		case MAX77650_CHARGER_CHG_SUSP_FCF:
> +		case MAX77650_CHARGER_CHG_SUSP_BTF:
> +			val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
> +			break;
> +		case MAX77650_CHARGER_CHG_PREQ:
> +		case MAX77650_CHARGER_CHG_ON_CURR:
> +		case MAX77650_CHARGER_CHG_ON_JCURR:
> +		case MAX77650_CHARGER_CHG_ON_VOLT:
> +		case MAX77650_CHARGER_CHG_ON_JVOLT:
> +		case MAX77650_CHARGER_CHG_ON_TOPOFF:
> +		case MAX77650_CHARGER_CHG_ON_JTOPOFF:
> +			val->intval = POWER_SUPPLY_STATUS_CHARGING;
> +			break;
> +		case MAX77650_CHARGER_CHG_DONE:
> +			val->intval = POWER_SUPPLY_STATUS_FULL;
> +			break;
> +		default:
> +			val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
> +		}
> +		break;
> +	case POWER_SUPPLY_PROP_ONLINE:
> +		rv = regmap_read(chg->map, MAX77650_REG_STAT_CHG_B, &reg);
> +		if (rv)
> +			return rv;
> +
> +		val->intval = MAX77650_CHARGER_CHG_CHARGING(reg);
> +		break;
> +	case POWER_SUPPLY_PROP_CHARGE_TYPE:
> +		rv = regmap_read(chg->map, MAX77650_REG_STAT_CHG_B, &reg);
> +		if (rv)
> +			return rv;
> +
> +		if (!MAX77650_CHARGER_CHG_CHARGING(reg)) {
> +			val->intval = POWER_SUPPLY_CHARGE_TYPE_NONE;
> +			break;
> +		}
> +
> +		switch (MAX77650_CHARGER_CHG_DTLS_BITS(reg)) {
> +		case MAX77650_CHARGER_CHG_PREQ:
> +		case MAX77650_CHARGER_CHG_ON_CURR:
> +		case MAX77650_CHARGER_CHG_ON_JCURR:
> +		case MAX77650_CHARGER_CHG_ON_VOLT:
> +		case MAX77650_CHARGER_CHG_ON_JVOLT:
> +			val->intval = POWER_SUPPLY_CHARGE_TYPE_FAST;
> +			break;
> +		case MAX77650_CHARGER_CHG_ON_TOPOFF:
> +		case MAX77650_CHARGER_CHG_ON_JTOPOFF:
> +			val->intval = POWER_SUPPLY_CHARGE_TYPE_TRICKLE;
> +			break;
> +		default:
> +			val->intval = POWER_SUPPLY_CHARGE_TYPE_UNKNOWN;
> +		}
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct power_supply_desc max77650_battery_desc = {
> +	.name		= "max77650",
> +	.type		= POWER_SUPPLY_TYPE_USB,
> +	.get_property	= max77650_charger_get_property,
> +	.properties	= max77650_charger_properties,
> +	.num_properties	= ARRAY_SIZE(max77650_charger_properties),
> +};
> +
> +static int max77650_charger_probe(struct platform_device *pdev)
> +{
> +	struct power_supply_config pscfg = {};
> +	struct max77650_charger_data *chg;
> +	struct power_supply *battery;
> +	struct device *dev, *parent;
> +	int rv, chg_irq, chgin_irq;
> +	unsigned int prop;
> +
> +	dev = &pdev->dev;
> +	parent = dev->parent;
> +
> +	chg = devm_kzalloc(dev, sizeof(*chg), GFP_KERNEL);
> +	if (!chg)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, chg);
> +
> +	chg->map = dev_get_regmap(parent, NULL);
> +	if (!chg->map)
> +		return -ENODEV;
> +
> +	chg->dev = dev;
> +
> +	pscfg.of_node = dev->of_node;
> +	pscfg.drv_data = chg;
> +
> +	chg_irq = platform_get_irq_byname(pdev, "CHG");
> +	if (chg_irq < 0)
> +		return chg_irq;
> +
> +	chgin_irq = platform_get_irq_byname(pdev, "CHGIN");
> +	if (chgin_irq < 0)
> +		return chgin_irq;
> +
> +	rv = devm_request_any_context_irq(dev, chg_irq,
> +					  max77650_charger_check_status,
> +					  IRQF_ONESHOT, "chg", chg);
> +	if (rv < 0)
> +		return rv;
> +
> +	rv = devm_request_any_context_irq(dev, chgin_irq,
> +					  max77650_charger_check_status,
> +					  IRQF_ONESHOT, "chgin", chg);
> +	if (rv < 0)
> +		return rv;
> +
> +	battery = devm_power_supply_register(dev,
> +					     &max77650_battery_desc, &pscfg);
> +	if (IS_ERR(battery))
> +		return PTR_ERR(battery);
> +
> +	rv = of_property_read_u32(dev->of_node, "maxim,vchgin-min", &prop);
> +	if (rv == 0) {
> +		rv = max77650_charger_set_vchgin_min(chg, prop);
> +		if (rv)
> +			return rv;
> +	}
> +
> +	rv = of_property_read_u32(dev->of_node, "maxim,ichgin-lim", &prop);
> +	if (rv == 0) {
> +		rv = max77650_charger_set_ichgin_lim(chg, prop);
> +		if (rv)
> +			return rv;
> +	}
> +
> +	return regmap_update_bits(chg->map,
> +				  MAX77650_REG_CNFG_CHG_B,
> +				  MAX77650_CHARGER_CHG_EN_MASK,
> +				  MAX77650_CHARGER_ENABLED);
> +}
> +
> +static int max77650_charger_remove(struct platform_device *pdev)
> +{
> +	struct max77650_charger_data *chg = platform_get_drvdata(pdev);
> +
> +	return regmap_update_bits(chg->map,
> +				  MAX77650_REG_CNFG_CHG_B,
> +				  MAX77650_CHARGER_CHG_EN_MASK,
> +				  MAX77650_CHARGER_DISABLED);
> +}
> +
> +static struct platform_driver max77650_charger_driver = {
> +	.driver = {
> +		.name = "max77650-charger",
> +	},
> +	.probe = max77650_charger_probe,
> +	.remove = max77650_charger_remove,
> +};
> +module_platform_driver(max77650_charger_driver);
> +
> +MODULE_DESCRIPTION("MAXIM 77650/77651 charger driver");
> +MODULE_AUTHOR("Bartosz Golaszewski <bgolaszewski@baylibre.com>");
> +MODULE_LICENSE("GPL v2");

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver

[Lee Jones]

On Tue, 05 Feb 2019, Bartosz Golaszewski wrote:

> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Add the core mfd driver for max77650 PMIC. We define five sub-devices
> for which the drivers will be added in subsequent patches.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/mfd/Kconfig          |  11 ++
>  drivers/mfd/Makefile         |   1 +
>  drivers/mfd/max77650.c       | 342 +++++++++++++++++++++++++++++++++++
>  include/linux/mfd/max77650.h |  59 ++++++
>  4 files changed, 413 insertions(+)
>  create mode 100644 drivers/mfd/max77650.c
>  create mode 100644 include/linux/mfd/max77650.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 76f9909cf396..a80c3fe80fbe 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -734,6 +734,17 @@ config MFD_MAX77620
>  	  provides common support for accessing the device; additional drivers
>  	  must be enabled in order to use the functionality of the device.
>  
> +config MFD_MAX77650
> +	tristate "Maxim MAX77650/77651 PMIC Support"
> +	depends on I2C
> +	depends on OF || COMPILE_TEST
> +	select MFD_CORE
> +	select REGMAP_I2C
> +	help
> +	  Say yes here to add support for Maxim Semiconductor MAX77650 and
> +	  MAX77651 Power Management ICs. This is the core multifunction
> +	  driver for interacting with the device.
> +
>  config MFD_MAX77686
>  	tristate "Maxim Semiconductor MAX77686/802 PMIC Support"
>  	depends on I2C
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 12980a4ad460..3b912a4015d1 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -151,6 +151,7 @@ obj-$(CONFIG_MFD_DA9150)	+= da9150-core.o
>  
>  obj-$(CONFIG_MFD_MAX14577)	+= max14577.o
>  obj-$(CONFIG_MFD_MAX77620)	+= max77620.o
> +obj-$(CONFIG_MFD_MAX77650)	+= max77650.o
>  obj-$(CONFIG_MFD_MAX77686)	+= max77686.o
>  obj-$(CONFIG_MFD_MAX77693)	+= max77693.o
>  obj-$(CONFIG_MFD_MAX77843)	+= max77843.o
> diff --git a/drivers/mfd/max77650.c b/drivers/mfd/max77650.c
> new file mode 100644
> index 000000000000..7c6164f1fde4
> --- /dev/null
> +++ b/drivers/mfd/max77650.c
> @@ -0,0 +1,342 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright (C) 2018 BayLibre SAS
> +// Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> +//
> +// Core MFD driver for MAXIM 77650/77651 charger/power-supply.
> +
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/max77650.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +#define MAX77650_INT_GPI_F_MSK		BIT(0)
> +#define MAX77650_INT_GPI_R_MSK		BIT(1)
> +#define MAX77650_INT_GPI_MSK \
> +			(MAX77650_INT_GPI_F_MSK | MAX77650_INT_GPI_R_MSK)
> +#define MAX77650_INT_nEN_F_MSK		BIT(2)
> +#define MAX77650_INT_nEN_R_MSK		BIT(3)
> +#define MAX77650_INT_TJAL1_R_MSK	BIT(4)
> +#define MAX77650_INT_TJAL2_R_MSK	BIT(5)
> +#define MAX77650_INT_DOD_R_MSK		BIT(6)
> +
> +#define MAX77650_INT_THM_MSK		BIT(0)
> +#define MAX77650_INT_CHG_MSK		BIT(1)
> +#define MAX77650_INT_CHGIN_MSK		BIT(2)
> +#define MAX77650_INT_TJ_REG_MSK		BIT(3)
> +#define MAX77650_INT_CHGIN_CTRL_MSK	BIT(4)
> +#define MAX77650_INT_SYS_CTRL_MSK	BIT(5)
> +#define MAX77650_INT_SYS_CNFG_MSK	BIT(6)
> +
> +#define MAX77650_INT_GLBL_OFFSET	0
> +#define MAX77650_INT_CHG_OFFSET		1
> +
> +#define MAX77650_SBIA_LPM_MASK		BIT(5)
> +#define MAX77650_SBIA_LPM_DISABLED	0x00
> +
> +enum {
> +	MAX77650_INT_GPI = 0,
> +	MAX77650_INT_nEN_F,
> +	MAX77650_INT_nEN_R,
> +	MAX77650_INT_TJAL1_R,
> +	MAX77650_INT_TJAL2_R,
> +	MAX77650_INT_DOD_R,
> +	MAX77650_INT_THM,
> +	MAX77650_INT_CHG,
> +	MAX77650_INT_CHGIN,
> +	MAX77650_INT_TJ_REG,
> +	MAX77650_INT_CHGIN_CTRL,
> +	MAX77650_INT_SYS_CTRL,
> +	MAX77650_INT_SYS_CNFG,
> +};
> +
> +enum {
> +	MAX77650_CELL_REGULATOR = 0,
> +	MAX77650_CELL_CHARGER,
> +	MAX77650_CELL_GPIO,
> +	MAX77650_CELL_LED,
> +	MAX77650_CELL_ONKEY,
> +	MAX77650_NUM_CELLS,
> +};
> +
> +struct max77650_irq_mapping {
> +	int cell_num;
> +	const int *irqs;
> +	const char *const *irq_names;
> +	unsigned int num_irqs;
> +};
> +
> +static const int max77650_charger_irqs[] = {
> +	MAX77650_INT_CHG,
> +	MAX77650_INT_CHGIN,
> +};
> +
> +static const int max77650_gpio_irqs[] = {
> +	MAX77650_INT_GPI,
> +};
> +
> +static const int max77650_onkey_irqs[] = {
> +	MAX77650_INT_nEN_F,
> +	MAX77650_INT_nEN_R,
> +};
> +
> +static const char *const max77650_charger_irq_names[] = {
> +	"CHG",
> +	"CHGIN",
> +};
> +
> +static const char *const max77650_gpio_irq_names[] = {
> +	"GPI",
> +};
> +
> +static const char *const max77650_onkey_irq_names[] = {
> +	"nEN_F",
> +	"nEN_R",
> +};
> +
> +static const struct max77650_irq_mapping max77650_irq_mapping_table[] = {
> +	{
> +		.cell_num	= MAX77650_CELL_CHARGER,
> +		.irqs		= max77650_charger_irqs,
> +		.irq_names	= max77650_charger_irq_names,
> +		.num_irqs	= ARRAY_SIZE(max77650_charger_irqs),
> +	},
> +	{
> +		.cell_num	= MAX77650_CELL_GPIO,
> +		.irqs		= max77650_gpio_irqs,
> +		.irq_names	= max77650_gpio_irq_names,
> +		.num_irqs	= ARRAY_SIZE(max77650_gpio_irqs),
> +	},
> +	{
> +		.cell_num	= MAX77650_CELL_ONKEY,
> +		.irqs		= max77650_onkey_irqs,
> +		.irq_names	= max77650_onkey_irq_names,
> +		.num_irqs	= ARRAY_SIZE(max77650_onkey_irqs),
> +	},
> +};

This is all a bit convoluted and nasty TBH.

> +static const struct mfd_cell max77650_cells[] = {
> +	[MAX77650_CELL_REGULATOR] = {
> +		.name		= "max77650-regulator",
> +		.of_compatible	= "maxim,max77650-regulator",
> +	},
> +	[MAX77650_CELL_CHARGER] = {
> +		.name		= "max77650-charger",
> +		.of_compatible	= "maxim,max77650-charger",
> +	},
> +	[MAX77650_CELL_GPIO] = {
> +		.name		= "max77650-gpio",
> +		.of_compatible	= "maxim,max77650-gpio",
> +	},
> +	[MAX77650_CELL_LED] = {
> +		.name		= "max77650-led",
> +		.of_compatible	= "maxim,max77650-led",
> +	},
> +	[MAX77650_CELL_ONKEY] = {
> +		.name		= "max77650-onkey",
> +		.of_compatible	= "maxim,max77650-onkey",
> +	},
> +};

Why are you numbering the cells?  There is no need to do this.

> +static const struct regmap_irq max77650_irqs[] = {
> +	[MAX77650_INT_GPI] = {
> +		.reg_offset		= MAX77650_INT_GLBL_OFFSET,
> +		.mask			= MAX77650_INT_GPI_MSK,
> +		.type = {
> +			.type_falling_val	= MAX77650_INT_GPI_F_MSK,
> +			.type_rising_val	= MAX77650_INT_GPI_R_MSK,
> +			.types_supported	= IRQ_TYPE_EDGE_BOTH,
> +		},
> +	},
> +	[MAX77650_INT_nEN_F] = {
> +		.reg_offset		= MAX77650_INT_GLBL_OFFSET,
> +		.mask			= MAX77650_INT_nEN_F_MSK,
> +	},
> +	[MAX77650_INT_nEN_R] = {
> +		.reg_offset		= MAX77650_INT_GLBL_OFFSET,
> +		.mask			= MAX77650_INT_nEN_R_MSK,
> +	},
> +	[MAX77650_INT_TJAL1_R] = {
> +		.reg_offset		= MAX77650_INT_GLBL_OFFSET,
> +		.mask			= MAX77650_INT_TJAL1_R_MSK,
> +	},
> +	[MAX77650_INT_TJAL2_R] = {
> +		.reg_offset		= MAX77650_INT_GLBL_OFFSET,
> +		.mask			= MAX77650_INT_TJAL2_R_MSK,
> +	},
> +	[MAX77650_INT_DOD_R] = {
> +		.reg_offset		= MAX77650_INT_GLBL_OFFSET,
> +		.mask			= MAX77650_INT_DOD_R_MSK,
> +	},
> +	[MAX77650_INT_THM] = {
> +		.reg_offset		= MAX77650_INT_CHG_OFFSET,
> +		.mask			= MAX77650_INT_THM_MSK,
> +	},
> +	[MAX77650_INT_CHG] = {
> +		.reg_offset		= MAX77650_INT_CHG_OFFSET,
> +		.mask			= MAX77650_INT_CHG_MSK,
> +	},
> +	[MAX77650_INT_CHGIN] = {
> +		.reg_offset		= MAX77650_INT_CHG_OFFSET,
> +		.mask			= MAX77650_INT_CHGIN_MSK,
> +	},
> +	[MAX77650_INT_TJ_REG] = {
> +		.reg_offset		= MAX77650_INT_CHG_OFFSET,
> +		.mask			= MAX77650_INT_TJ_REG_MSK,
> +	},
> +	[MAX77650_INT_CHGIN_CTRL] = {
> +		.reg_offset		= MAX77650_INT_CHG_OFFSET,
> +		.mask			= MAX77650_INT_CHGIN_CTRL_MSK,
> +	},
> +	[MAX77650_INT_SYS_CTRL] = {
> +		.reg_offset		= MAX77650_INT_CHG_OFFSET,
> +		.mask			= MAX77650_INT_SYS_CTRL_MSK,
> +	},
> +	[MAX77650_INT_SYS_CNFG] = {
> +		.reg_offset		= MAX77650_INT_CHG_OFFSET,
> +		.mask			= MAX77650_INT_SYS_CNFG_MSK,
> +	},
> +};

If you get rid of all of the horrible hoop jumping in *_setup_irqs(),
you can use REGMAP_IRQ_REG() like everyone else does.

> +static const struct regmap_irq_chip max77650_irq_chip = {
> +	.name			= "max77650-irq",
> +	.irqs			= max77650_irqs,
> +	.num_irqs		= ARRAY_SIZE(max77650_irqs),
> +	.num_regs		= 2,
> +	.status_base		= MAX77650_REG_INT_GLBL,
> +	.mask_base		= MAX77650_REG_INTM_GLBL,
> +	.type_in_mask		= true,
> +	.type_invert		= true,
> +	.init_ack_masked	= true,
> +	.clear_on_unmask	= true,
> +};
> +
> +static const struct regmap_config max77650_regmap_config = {
> +	.name		= "max77650",
> +	.reg_bits	= 8,
> +	.val_bits	= 8,
> +};
> +
> +static int max77650_setup_irqs(struct device *dev, struct mfd_cell *cells)
> +{
> +	const struct max77650_irq_mapping *mapping;
> +	struct regmap_irq_chip_data *irq_data;
> +	struct i2c_client *i2c;
> +	struct mfd_cell *cell;
> +	struct resource *res;
> +	struct regmap *map;
> +	int i, j, irq, rv;
> +
> +	i2c = to_i2c_client(dev);
> +
> +	map = dev_get_regmap(dev, NULL);
> +	if (!map)
> +		return -ENODEV;
> +
> +	rv = devm_regmap_add_irq_chip(dev, map, i2c->irq,
> +				      IRQF_ONESHOT | IRQF_SHARED, -1,

What is -1?  Are you sure this isn't defined somewhere?

> +				      &max77650_irq_chip, &irq_data);
> +	if (rv)
> +		return rv;
> +
> +	for (i = 0; i < ARRAY_SIZE(max77650_irq_mapping_table); i++) {
> +		mapping = &max77650_irq_mapping_table[i];
> +		cell = &cells[mapping->cell_num];
> +
> +		res = devm_kcalloc(dev, sizeof(*res),
> +				   mapping->num_irqs, GFP_KERNEL);
> +		if (!res)
> +			return -ENOMEM;
> +
> +		cell->resources = res;
> +		cell->num_resources = mapping->num_irqs;
> +
> +		for (j = 0; j < mapping->num_irqs; j++) {
> +			irq = regmap_irq_get_virq(irq_data, mapping->irqs[j]);
> +			if (irq < 0)
> +				return irq;
> +
> +			res[j].start = res[j].end = irq;
> +			res[j].flags = IORESOURCE_IRQ;
> +			res[j].name = mapping->irq_names[j];
> +		}
> +	}

This is the first time I've seen it done like this (and I hate it).

Why are you storing the virqs in resources?

I think this is highly irregular.

> +	return 0;
> +}
> +
> +static int max77650_i2c_probe(struct i2c_client *i2c)
> +{
> +	struct device *dev = &i2c->dev;
> +	struct mfd_cell *cells;
> +	struct regmap *map;
> +	unsigned int val;
> +	int rv;
> +
> +	map = devm_regmap_init_i2c(i2c, &max77650_regmap_config);
> +	if (IS_ERR(map))

What error messages does devm_regmap_init_i2c() report?  Does it print
out its own error messages internally?  If not it would be better to
provide a suitable error message here.

> +		return PTR_ERR(map);
> +
> +	rv = regmap_read(map, MAX77650_REG_CID, &val);
> +	if (rv)

Better to provide a suitable error message here.

> +		return rv;
> +
> +	switch (MAX77650_CID_BITS(val)) {
> +	case MAX77650_CID_77650A:
> +	case MAX77650_CID_77650C:
> +	case MAX77650_CID_77651A:
> +	case MAX77650_CID_77651B:
> +		break;
> +	default:

Better to provide a suitable error message here.

> +		return -ENODEV;
> +	}
> +
> +	/*
> +	 * This IC has a low-power mode which reduces the quiescent current
> +	 * consumption to ~5.6uA but is only suitable for systems consuming
> +	 * less than ~2mA. Since this is not likely the case even on
> +	 * linux-based wearables - keep the chip in normal power mode.
> +	 */
> +	rv = regmap_update_bits(map,
> +				MAX77650_REG_CNFG_GLBL,
> +				MAX77650_SBIA_LPM_MASK,
> +				MAX77650_SBIA_LPM_DISABLED);
> +	if (rv)

Better to provide a suitable error message here.

> +		return rv;
> +
> +	cells = devm_kmemdup(dev, max77650_cells,
> +			     sizeof(max77650_cells), GFP_KERNEL);
> +	if (!cells)
> +		return -ENOMEM;
> +
> +	rv = max77650_setup_irqs(dev, cells);
> +	if (rv)
> +		return rv;
> +
> +	return devm_mfd_add_devices(dev, -1, cells,

Use the correct defines instead of -1.

`git grep mfd_add_devices`

> +				    MAX77650_NUM_CELLS, NULL, 0, NULL);
> +}
> +
> +static const struct of_device_id max77650_of_match[] = {
> +	{ .compatible = "maxim,max77650" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, max77650_of_match);
> +
> +static struct i2c_driver max77650_i2c_driver = {
> +	.driver = {
> +		.name = "max77650",
> +		.of_match_table = of_match_ptr(max77650_of_match),
> +	},
> +	.probe_new = max77650_i2c_probe,
> +};
> +module_i2c_driver(max77650_i2c_driver);
> +
> +MODULE_DESCRIPTION("MAXIM 77650/77651 multi-function core driver");
> +MODULE_AUTHOR("Bartosz Golaszewski <bgolaszewski@baylibre.com>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/max77650.h b/include/linux/mfd/max77650.h
> new file mode 100644
> index 000000000000..c809e211a8cd
> --- /dev/null
> +++ b/include/linux/mfd/max77650.h
> @@ -0,0 +1,59 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2018 BayLibre SAS
> + * Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> + *
> + * Common definitions for MAXIM 77650/77651 charger/power-supply.
> + */
> +
> +#ifndef MAX77650_H
> +#define MAX77650_H
> +
> +#include <linux/bits.h>
> +
> +#define MAX77650_REG_INT_GLBL		0x00
> +#define MAX77650_REG_INT_CHG		0x01
> +#define MAX77650_REG_STAT_CHG_A		0x02
> +#define MAX77650_REG_STAT_CHG_B		0x03
> +#define MAX77650_REG_ERCFLAG		0x04
> +#define MAX77650_REG_STAT_GLBL		0x05
> +#define MAX77650_REG_INTM_GLBL		0x06
> +#define MAX77650_REG_INTM_CHG		0x07
> +#define MAX77650_REG_CNFG_GLBL		0x10
> +#define MAX77650_REG_CID		0x11
> +#define MAX77650_REG_CNFG_GPIO		0x12
> +#define MAX77650_REG_CNFG_CHG_A		0x18
> +#define MAX77650_REG_CNFG_CHG_B		0x19
> +#define MAX77650_REG_CNFG_CHG_C		0x1a
> +#define MAX77650_REG_CNFG_CHG_D		0x1b
> +#define MAX77650_REG_CNFG_CHG_E		0x1c
> +#define MAX77650_REG_CNFG_CHG_F		0x1d
> +#define MAX77650_REG_CNFG_CHG_G		0x1e
> +#define MAX77650_REG_CNFG_CHG_H		0x1f
> +#define MAX77650_REG_CNFG_CHG_I		0x20
> +#define MAX77650_REG_CNFG_SBB_TOP	0x28
> +#define MAX77650_REG_CNFG_SBB0_A	0x29
> +#define MAX77650_REG_CNFG_SBB0_B	0x2a
> +#define MAX77650_REG_CNFG_SBB1_A	0x2b
> +#define MAX77650_REG_CNFG_SBB1_B	0x2c
> +#define MAX77650_REG_CNFG_SBB2_A	0x2d
> +#define MAX77650_REG_CNFG_SBB2_B	0x2e
> +#define MAX77650_REG_CNFG_LDO_A		0x38
> +#define MAX77650_REG_CNFG_LDO_B		0x39
> +#define MAX77650_REG_CNFG_LED0_A	0x40
> +#define MAX77650_REG_CNFG_LED1_A	0x41
> +#define MAX77650_REG_CNFG_LED2_A	0x42
> +#define MAX77650_REG_CNFG_LED0_B	0x43
> +#define MAX77650_REG_CNFG_LED1_B	0x44
> +#define MAX77650_REG_CNFG_LED2_B	0x45
> +#define MAX77650_REG_CNFG_LED_TOP	0x46
> +
> +#define MAX77650_CID_MASK		GENMASK(3, 0)
> +#define MAX77650_CID_BITS(_reg)		(_reg & MAX77650_CID_MASK)
> +
> +#define MAX77650_CID_77650A		0x03
> +#define MAX77650_CID_77650C		0x0a
> +#define MAX77650_CID_77651A		0x06
> +#define MAX77650_CID_77651B		0x08
> +
> +#endif /* MAX77650_H */

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver

[Bartosz Golaszewski]

wt., 12 lut 2019 o 09:36 Lee Jones <lee.jones@linaro.org> napisał(a):
>
> On Tue, 05 Feb 2019, Bartosz Golaszewski wrote:
>
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > Add the core mfd driver for max77650 PMIC. We define five sub-devices
> > for which the drivers will be added in subsequent patches.
> >
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > ---
> >  drivers/mfd/Kconfig          |  11 ++
> >  drivers/mfd/Makefile         |   1 +
> >  drivers/mfd/max77650.c       | 342 +++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/max77650.h |  59 ++++++
> >  4 files changed, 413 insertions(+)
> >  create mode 100644 drivers/mfd/max77650.c
> >  create mode 100644 include/linux/mfd/max77650.h
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index 76f9909cf396..a80c3fe80fbe 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -734,6 +734,17 @@ config MFD_MAX77620
> >         provides common support for accessing the device; additional drivers
> >         must be enabled in order to use the functionality of the device.
> >
> > +config MFD_MAX77650
> > +     tristate "Maxim MAX77650/77651 PMIC Support"
> > +     depends on I2C
> > +     depends on OF || COMPILE_TEST
> > +     select MFD_CORE
> > +     select REGMAP_I2C
> > +     help
> > +       Say yes here to add support for Maxim Semiconductor MAX77650 and
> > +       MAX77651 Power Management ICs. This is the core multifunction
> > +       driver for interacting with the device.
> > +
> >  config MFD_MAX77686
> >       tristate "Maxim Semiconductor MAX77686/802 PMIC Support"
> >       depends on I2C
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 12980a4ad460..3b912a4015d1 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -151,6 +151,7 @@ obj-$(CONFIG_MFD_DA9150)  += da9150-core.o
> >
> >  obj-$(CONFIG_MFD_MAX14577)   += max14577.o
> >  obj-$(CONFIG_MFD_MAX77620)   += max77620.o
> > +obj-$(CONFIG_MFD_MAX77650)   += max77650.o
> >  obj-$(CONFIG_MFD_MAX77686)   += max77686.o
> >  obj-$(CONFIG_MFD_MAX77693)   += max77693.o
> >  obj-$(CONFIG_MFD_MAX77843)   += max77843.o
> > diff --git a/drivers/mfd/max77650.c b/drivers/mfd/max77650.c
> > new file mode 100644
> > index 000000000000..7c6164f1fde4
> > --- /dev/null
> > +++ b/drivers/mfd/max77650.c
> > @@ -0,0 +1,342 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +//
> > +// Copyright (C) 2018 BayLibre SAS
> > +// Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > +//
> > +// Core MFD driver for MAXIM 77650/77651 charger/power-supply.
> > +
> > +#include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/mfd/max77650.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
> > +
> > +#define MAX77650_INT_GPI_F_MSK               BIT(0)
> > +#define MAX77650_INT_GPI_R_MSK               BIT(1)
> > +#define MAX77650_INT_GPI_MSK \
> > +                     (MAX77650_INT_GPI_F_MSK | MAX77650_INT_GPI_R_MSK)
> > +#define MAX77650_INT_nEN_F_MSK               BIT(2)
> > +#define MAX77650_INT_nEN_R_MSK               BIT(3)
> > +#define MAX77650_INT_TJAL1_R_MSK     BIT(4)
> > +#define MAX77650_INT_TJAL2_R_MSK     BIT(5)
> > +#define MAX77650_INT_DOD_R_MSK               BIT(6)
> > +
> > +#define MAX77650_INT_THM_MSK         BIT(0)
> > +#define MAX77650_INT_CHG_MSK         BIT(1)
> > +#define MAX77650_INT_CHGIN_MSK               BIT(2)
> > +#define MAX77650_INT_TJ_REG_MSK              BIT(3)
> > +#define MAX77650_INT_CHGIN_CTRL_MSK  BIT(4)
> > +#define MAX77650_INT_SYS_CTRL_MSK    BIT(5)
> > +#define MAX77650_INT_SYS_CNFG_MSK    BIT(6)
> > +
> > +#define MAX77650_INT_GLBL_OFFSET     0
> > +#define MAX77650_INT_CHG_OFFSET              1
> > +
> > +#define MAX77650_SBIA_LPM_MASK               BIT(5)
> > +#define MAX77650_SBIA_LPM_DISABLED   0x00
> > +
> > +enum {
> > +     MAX77650_INT_GPI = 0,
> > +     MAX77650_INT_nEN_F,
> > +     MAX77650_INT_nEN_R,
> > +     MAX77650_INT_TJAL1_R,
> > +     MAX77650_INT_TJAL2_R,
> > +     MAX77650_INT_DOD_R,
> > +     MAX77650_INT_THM,
> > +     MAX77650_INT_CHG,
> > +     MAX77650_INT_CHGIN,
> > +     MAX77650_INT_TJ_REG,
> > +     MAX77650_INT_CHGIN_CTRL,
> > +     MAX77650_INT_SYS_CTRL,
> > +     MAX77650_INT_SYS_CNFG,
> > +};
> > +
> > +enum {
> > +     MAX77650_CELL_REGULATOR = 0,
> > +     MAX77650_CELL_CHARGER,
> > +     MAX77650_CELL_GPIO,
> > +     MAX77650_CELL_LED,
> > +     MAX77650_CELL_ONKEY,
> > +     MAX77650_NUM_CELLS,
> > +};
> > +
> > +struct max77650_irq_mapping {
> > +     int cell_num;
> > +     const int *irqs;
> > +     const char *const *irq_names;
> > +     unsigned int num_irqs;
> > +};
> > +
> > +static const int max77650_charger_irqs[] = {
> > +     MAX77650_INT_CHG,
> > +     MAX77650_INT_CHGIN,
> > +};
> > +
> > +static const int max77650_gpio_irqs[] = {
> > +     MAX77650_INT_GPI,
> > +};
> > +
> > +static const int max77650_onkey_irqs[] = {
> > +     MAX77650_INT_nEN_F,
> > +     MAX77650_INT_nEN_R,
> > +};
> > +
> > +static const char *const max77650_charger_irq_names[] = {
> > +     "CHG",
> > +     "CHGIN",
> > +};
> > +
> > +static const char *const max77650_gpio_irq_names[] = {
> > +     "GPI",
> > +};
> > +
> > +static const char *const max77650_onkey_irq_names[] = {
> > +     "nEN_F",
> > +     "nEN_R",
> > +};
> > +
> > +static const struct max77650_irq_mapping max77650_irq_mapping_table[] = {
> > +     {
> > +             .cell_num       = MAX77650_CELL_CHARGER,
> > +             .irqs           = max77650_charger_irqs,
> > +             .irq_names      = max77650_charger_irq_names,
> > +             .num_irqs       = ARRAY_SIZE(max77650_charger_irqs),
> > +     },
> > +     {
> > +             .cell_num       = MAX77650_CELL_GPIO,
> > +             .irqs           = max77650_gpio_irqs,
> > +             .irq_names      = max77650_gpio_irq_names,
> > +             .num_irqs       = ARRAY_SIZE(max77650_gpio_irqs),
> > +     },
> > +     {
> > +             .cell_num       = MAX77650_CELL_ONKEY,
> > +             .irqs           = max77650_onkey_irqs,
> > +             .irq_names      = max77650_onkey_irq_names,
> > +             .num_irqs       = ARRAY_SIZE(max77650_onkey_irqs),
> > +     },
> > +};
>
> This is all a bit convoluted and nasty TBH.
>
> > +static const struct mfd_cell max77650_cells[] = {
> > +     [MAX77650_CELL_REGULATOR] = {
> > +             .name           = "max77650-regulator",
> > +             .of_compatible  = "maxim,max77650-regulator",
> > +     },
> > +     [MAX77650_CELL_CHARGER] = {
> > +             .name           = "max77650-charger",
> > +             .of_compatible  = "maxim,max77650-charger",
> > +     },
> > +     [MAX77650_CELL_GPIO] = {
> > +             .name           = "max77650-gpio",
> > +             .of_compatible  = "maxim,max77650-gpio",
> > +     },
> > +     [MAX77650_CELL_LED] = {
> > +             .name           = "max77650-led",
> > +             .of_compatible  = "maxim,max77650-led",
> > +     },
> > +     [MAX77650_CELL_ONKEY] = {
> > +             .name           = "max77650-onkey",
> > +             .of_compatible  = "maxim,max77650-onkey",
> > +     },
> > +};
>
> Why are you numbering the cells?  There is no need to do this.
>

Just for better readability. It makes sense to me coupled with
MAX77650_NUM_CELLS.

> > +static const struct regmap_irq max77650_irqs[] = {
> > +     [MAX77650_INT_GPI] = {
> > +             .reg_offset             = MAX77650_INT_GLBL_OFFSET,
> > +             .mask                   = MAX77650_INT_GPI_MSK,
> > +             .type = {
> > +                     .type_falling_val       = MAX77650_INT_GPI_F_MSK,
> > +                     .type_rising_val        = MAX77650_INT_GPI_R_MSK,
> > +                     .types_supported        = IRQ_TYPE_EDGE_BOTH,
> > +             },
> > +     },
> > +     [MAX77650_INT_nEN_F] = {
> > +             .reg_offset             = MAX77650_INT_GLBL_OFFSET,
> > +             .mask                   = MAX77650_INT_nEN_F_MSK,
> > +     },
> > +     [MAX77650_INT_nEN_R] = {
> > +             .reg_offset             = MAX77650_INT_GLBL_OFFSET,
> > +             .mask                   = MAX77650_INT_nEN_R_MSK,
> > +     },
> > +     [MAX77650_INT_TJAL1_R] = {
> > +             .reg_offset             = MAX77650_INT_GLBL_OFFSET,
> > +             .mask                   = MAX77650_INT_TJAL1_R_MSK,
> > +     },
> > +     [MAX77650_INT_TJAL2_R] = {
> > +             .reg_offset             = MAX77650_INT_GLBL_OFFSET,
> > +             .mask                   = MAX77650_INT_TJAL2_R_MSK,
> > +     },
> > +     [MAX77650_INT_DOD_R] = {
> > +             .reg_offset             = MAX77650_INT_GLBL_OFFSET,
> > +             .mask                   = MAX77650_INT_DOD_R_MSK,
> > +     },
> > +     [MAX77650_INT_THM] = {
> > +             .reg_offset             = MAX77650_INT_CHG_OFFSET,
> > +             .mask                   = MAX77650_INT_THM_MSK,
> > +     },
> > +     [MAX77650_INT_CHG] = {
> > +             .reg_offset             = MAX77650_INT_CHG_OFFSET,
> > +             .mask                   = MAX77650_INT_CHG_MSK,
> > +     },
> > +     [MAX77650_INT_CHGIN] = {
> > +             .reg_offset             = MAX77650_INT_CHG_OFFSET,
> > +             .mask                   = MAX77650_INT_CHGIN_MSK,
> > +     },
> > +     [MAX77650_INT_TJ_REG] = {
> > +             .reg_offset             = MAX77650_INT_CHG_OFFSET,
> > +             .mask                   = MAX77650_INT_TJ_REG_MSK,
> > +     },
> > +     [MAX77650_INT_CHGIN_CTRL] = {
> > +             .reg_offset             = MAX77650_INT_CHG_OFFSET,
> > +             .mask                   = MAX77650_INT_CHGIN_CTRL_MSK,
> > +     },
> > +     [MAX77650_INT_SYS_CTRL] = {
> > +             .reg_offset             = MAX77650_INT_CHG_OFFSET,
> > +             .mask                   = MAX77650_INT_SYS_CTRL_MSK,
> > +     },
> > +     [MAX77650_INT_SYS_CNFG] = {
> > +             .reg_offset             = MAX77650_INT_CHG_OFFSET,
> > +             .mask                   = MAX77650_INT_SYS_CNFG_MSK,
> > +     },
> > +};
>
> If you get rid of all of the horrible hoop jumping in *_setup_irqs(),
> you can use REGMAP_IRQ_REG() like everyone else does.
>

I could even use it now - except for the first interrupt. I decided to
not use it everywhere as it looks much better that way than having
REGMAP_IRQ_REG() for all definitions and then the first one sticking
out like that. It just looks better.

> > +static const struct regmap_irq_chip max77650_irq_chip = {
> > +     .name                   = "max77650-irq",
> > +     .irqs                   = max77650_irqs,
> > +     .num_irqs               = ARRAY_SIZE(max77650_irqs),
> > +     .num_regs               = 2,
> > +     .status_base            = MAX77650_REG_INT_GLBL,
> > +     .mask_base              = MAX77650_REG_INTM_GLBL,
> > +     .type_in_mask           = true,
> > +     .type_invert            = true,
> > +     .init_ack_masked        = true,
> > +     .clear_on_unmask        = true,
> > +};
> > +
> > +static const struct regmap_config max77650_regmap_config = {
> > +     .name           = "max77650",
> > +     .reg_bits       = 8,
> > +     .val_bits       = 8,
> > +};
> > +
> > +static int max77650_setup_irqs(struct device *dev, struct mfd_cell *cells)
> > +{
> > +     const struct max77650_irq_mapping *mapping;
> > +     struct regmap_irq_chip_data *irq_data;
> > +     struct i2c_client *i2c;
> > +     struct mfd_cell *cell;
> > +     struct resource *res;
> > +     struct regmap *map;
> > +     int i, j, irq, rv;
> > +
> > +     i2c = to_i2c_client(dev);
> > +
> > +     map = dev_get_regmap(dev, NULL);
> > +     if (!map)
> > +             return -ENODEV;
> > +
> > +     rv = devm_regmap_add_irq_chip(dev, map, i2c->irq,
> > +                                   IRQF_ONESHOT | IRQF_SHARED, -1,
>
> What is -1?  Are you sure this isn't defined somewhere?
>

I don't see any define for negative irq_base argument. I can add that
in a separate series and convert the users, but for now I'd stick with
-1.

> > +                                   &max77650_irq_chip, &irq_data);
> > +     if (rv)
> > +             return rv;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(max77650_irq_mapping_table); i++) {
> > +             mapping = &max77650_irq_mapping_table[i];
> > +             cell = &cells[mapping->cell_num];
> > +
> > +             res = devm_kcalloc(dev, sizeof(*res),
> > +                                mapping->num_irqs, GFP_KERNEL);
> > +             if (!res)
> > +                     return -ENOMEM;
> > +
> > +             cell->resources = res;
> > +             cell->num_resources = mapping->num_irqs;
> > +
> > +             for (j = 0; j < mapping->num_irqs; j++) {
> > +                     irq = regmap_irq_get_virq(irq_data, mapping->irqs[j]);
> > +                     if (irq < 0)
> > +                             return irq;
> > +
> > +                     res[j].start = res[j].end = irq;
> > +                     res[j].flags = IORESOURCE_IRQ;
> > +                     res[j].name = mapping->irq_names[j];
> > +             }
> > +     }
>
> This is the first time I've seen it done like this (and I hate it).
>
> Why are you storing the virqs in resources?
>
> I think this is highly irregular.
>

I initially just passed the regmap_irq_chip_data over i2c clientdata
and sub-drivers would look up virq numbers from it but was advised by
Dmitry Torokhov to use resources instead. After implementing it this
way I too think it's more elegant in sub-drivers who can simply do
platform_get_irq_byname(). Do you have a different idea? What exactly
don't you like about this?

> > +     return 0;
> > +}
> > +
> > +static int max77650_i2c_probe(struct i2c_client *i2c)
> > +{
> > +     struct device *dev = &i2c->dev;
> > +     struct mfd_cell *cells;
> > +     struct regmap *map;
> > +     unsigned int val;
> > +     int rv;
> > +
> > +     map = devm_regmap_init_i2c(i2c, &max77650_regmap_config);
> > +     if (IS_ERR(map))
>
> What error messages does devm_regmap_init_i2c() report?  Does it print
> out its own error messages internally?  If not it would be better to
> provide a suitable error message here.
>
> > +             return PTR_ERR(map);
> > +
> > +     rv = regmap_read(map, MAX77650_REG_CID, &val);
> > +     if (rv)
>
> Better to provide a suitable error message here.
>
> > +             return rv;
> > +
> > +     switch (MAX77650_CID_BITS(val)) {
> > +     case MAX77650_CID_77650A:
> > +     case MAX77650_CID_77650C:
> > +     case MAX77650_CID_77651A:
> > +     case MAX77650_CID_77651B:
> > +             break;
> > +     default:
>
> Better to provide a suitable error message here.
>
> > +             return -ENODEV;
> > +     }
> > +
> > +     /*
> > +      * This IC has a low-power mode which reduces the quiescent current
> > +      * consumption to ~5.6uA but is only suitable for systems consuming
> > +      * less than ~2mA. Since this is not likely the case even on
> > +      * linux-based wearables - keep the chip in normal power mode.
> > +      */
> > +     rv = regmap_update_bits(map,
> > +                             MAX77650_REG_CNFG_GLBL,
> > +                             MAX77650_SBIA_LPM_MASK,
> > +                             MAX77650_SBIA_LPM_DISABLED);
> > +     if (rv)
>
> Better to provide a suitable error message here.
>
> > +             return rv;
> > +
> > +     cells = devm_kmemdup(dev, max77650_cells,
> > +                          sizeof(max77650_cells), GFP_KERNEL);
> > +     if (!cells)
> > +             return -ENOMEM;
> > +
> > +     rv = max77650_setup_irqs(dev, cells);
> > +     if (rv)
> > +             return rv;
> > +
> > +     return devm_mfd_add_devices(dev, -1, cells,
>
> Use the correct defines instead of -1.
>

Will do that and add the error messages.

Bart

> `git grep mfd_add_devices`
>
> > +                                 MAX77650_NUM_CELLS, NULL, 0, NULL);
> > +}
> > +
> > +static const struct of_device_id max77650_of_match[] = {
> > +     { .compatible = "maxim,max77650" },
> > +     { }
> > +};
> > +MODULE_DEVICE_TABLE(of, max77650_of_match);
> > +
> > +static struct i2c_driver max77650_i2c_driver = {
> > +     .driver = {
> > +             .name = "max77650",
> > +             .of_match_table = of_match_ptr(max77650_of_match),
> > +     },
> > +     .probe_new = max77650_i2c_probe,
> > +};
> > +module_i2c_driver(max77650_i2c_driver);
> > +
> > +MODULE_DESCRIPTION("MAXIM 77650/77651 multi-function core driver");
> > +MODULE_AUTHOR("Bartosz Golaszewski <bgolaszewski@baylibre.com>");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/include/linux/mfd/max77650.h b/include/linux/mfd/max77650.h
> > new file mode 100644
> > index 000000000000..c809e211a8cd
> > --- /dev/null
> > +++ b/include/linux/mfd/max77650.h
> > @@ -0,0 +1,59 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2018 BayLibre SAS
> > + * Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > + *
> > + * Common definitions for MAXIM 77650/77651 charger/power-supply.
> > + */
> > +
> > +#ifndef MAX77650_H
> > +#define MAX77650_H
> > +
> > +#include <linux/bits.h>
> > +
> > +#define MAX77650_REG_INT_GLBL                0x00
> > +#define MAX77650_REG_INT_CHG         0x01
> > +#define MAX77650_REG_STAT_CHG_A              0x02
> > +#define MAX77650_REG_STAT_CHG_B              0x03
> > +#define MAX77650_REG_ERCFLAG         0x04
> > +#define MAX77650_REG_STAT_GLBL               0x05
> > +#define MAX77650_REG_INTM_GLBL               0x06
> > +#define MAX77650_REG_INTM_CHG                0x07
> > +#define MAX77650_REG_CNFG_GLBL               0x10
> > +#define MAX77650_REG_CID             0x11
> > +#define MAX77650_REG_CNFG_GPIO               0x12
> > +#define MAX77650_REG_CNFG_CHG_A              0x18
> > +#define MAX77650_REG_CNFG_CHG_B              0x19
> > +#define MAX77650_REG_CNFG_CHG_C              0x1a
> > +#define MAX77650_REG_CNFG_CHG_D              0x1b
> > +#define MAX77650_REG_CNFG_CHG_E              0x1c
> > +#define MAX77650_REG_CNFG_CHG_F              0x1d
> > +#define MAX77650_REG_CNFG_CHG_G              0x1e
> > +#define MAX77650_REG_CNFG_CHG_H              0x1f
> > +#define MAX77650_REG_CNFG_CHG_I              0x20
> > +#define MAX77650_REG_CNFG_SBB_TOP    0x28
> > +#define MAX77650_REG_CNFG_SBB0_A     0x29
> > +#define MAX77650_REG_CNFG_SBB0_B     0x2a
> > +#define MAX77650_REG_CNFG_SBB1_A     0x2b
> > +#define MAX77650_REG_CNFG_SBB1_B     0x2c
> > +#define MAX77650_REG_CNFG_SBB2_A     0x2d
> > +#define MAX77650_REG_CNFG_SBB2_B     0x2e
> > +#define MAX77650_REG_CNFG_LDO_A              0x38
> > +#define MAX77650_REG_CNFG_LDO_B              0x39
> > +#define MAX77650_REG_CNFG_LED0_A     0x40
> > +#define MAX77650_REG_CNFG_LED1_A     0x41
> > +#define MAX77650_REG_CNFG_LED2_A     0x42
> > +#define MAX77650_REG_CNFG_LED0_B     0x43
> > +#define MAX77650_REG_CNFG_LED1_B     0x44
> > +#define MAX77650_REG_CNFG_LED2_B     0x45
> > +#define MAX77650_REG_CNFG_LED_TOP    0x46
> > +
> > +#define MAX77650_CID_MASK            GENMASK(3, 0)
> > +#define MAX77650_CID_BITS(_reg)              (_reg & MAX77650_CID_MASK)
> > +
> > +#define MAX77650_CID_77650A          0x03
> > +#define MAX77650_CID_77650C          0x0a
> > +#define MAX77650_CID_77651A          0x06
> > +#define MAX77650_CID_77651B          0x08
> > +
> > +#endif /* MAX77650_H */
>
> --
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver

[Lee Jones]

On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:

> wt., 12 lut 2019 o 09:36 Lee Jones <lee.jones@linaro.org> napisał(a):
> >
> > On Tue, 05 Feb 2019, Bartosz Golaszewski wrote:
> >
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > Add the core mfd driver for max77650 PMIC. We define five sub-devices
> > > for which the drivers will be added in subsequent patches.
> > >
> > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > ---
> > >  drivers/mfd/Kconfig          |  11 ++
> > >  drivers/mfd/Makefile         |   1 +
> > >  drivers/mfd/max77650.c       | 342 +++++++++++++++++++++++++++++++++++
> > >  include/linux/mfd/max77650.h |  59 ++++++
> > >  4 files changed, 413 insertions(+)
> > >  create mode 100644 drivers/mfd/max77650.c
> > >  create mode 100644 include/linux/mfd/max77650.h

[...]

> > > +static const struct max77650_irq_mapping max77650_irq_mapping_table[] = {
> > > +     {
> > > +             .cell_num       = MAX77650_CELL_CHARGER,
> > > +             .irqs           = max77650_charger_irqs,
> > > +             .irq_names      = max77650_charger_irq_names,
> > > +             .num_irqs       = ARRAY_SIZE(max77650_charger_irqs),
> > > +     },
> > > +     {
> > > +             .cell_num       = MAX77650_CELL_GPIO,
> > > +             .irqs           = max77650_gpio_irqs,
> > > +             .irq_names      = max77650_gpio_irq_names,
> > > +             .num_irqs       = ARRAY_SIZE(max77650_gpio_irqs),
> > > +     },
> > > +     {
> > > +             .cell_num       = MAX77650_CELL_ONKEY,
> > > +             .irqs           = max77650_onkey_irqs,
> > > +             .irq_names      = max77650_onkey_irq_names,
> > > +             .num_irqs       = ARRAY_SIZE(max77650_onkey_irqs),
> > > +     },
> > > +};
> >
> > This is all a bit convoluted and nasty TBH.
> >
> > > +static const struct mfd_cell max77650_cells[] = {
> > > +     [MAX77650_CELL_REGULATOR] = {
> > > +             .name           = "max77650-regulator",
> > > +             .of_compatible  = "maxim,max77650-regulator",
> > > +     },
> > > +     [MAX77650_CELL_CHARGER] = {
> > > +             .name           = "max77650-charger",
> > > +             .of_compatible  = "maxim,max77650-charger",
> > > +     },
> > > +     [MAX77650_CELL_GPIO] = {
> > > +             .name           = "max77650-gpio",
> > > +             .of_compatible  = "maxim,max77650-gpio",
> > > +     },
> > > +     [MAX77650_CELL_LED] = {
> > > +             .name           = "max77650-led",
> > > +             .of_compatible  = "maxim,max77650-led",
> > > +     },
> > > +     [MAX77650_CELL_ONKEY] = {
> > > +             .name           = "max77650-onkey",
> > > +             .of_compatible  = "maxim,max77650-onkey",
> > > +     },
> > > +};
> >
> > Why are you numbering the cells?  There is no need to do this.
> >
> 
> Just for better readability. It makes sense to me coupled with
> MAX77650_NUM_CELLS.

You have it the wrong way around.  You define the cell data, then
provide the number of them using ARRAY_SIZE().  The enum containing
MAX77650_NUM_CELLS should not exist.

> > > +static const struct regmap_irq max77650_irqs[] = {
> > > +     [MAX77650_INT_GPI] = {
> > > +             .reg_offset             = MAX77650_INT_GLBL_OFFSET,
> > > +             .mask                   = MAX77650_INT_GPI_MSK,
> > > +             .type = {
> > > +                     .type_falling_val       = MAX77650_INT_GPI_F_MSK,
> > > +                     .type_rising_val        = MAX77650_INT_GPI_R_MSK,
> > > +                     .types_supported        = IRQ_TYPE_EDGE_BOTH,
> > > +             },
> > > +     },
> > > +     [MAX77650_INT_nEN_F] = {
> > > +             .reg_offset             = MAX77650_INT_GLBL_OFFSET,
> > > +             .mask                   = MAX77650_INT_nEN_F_MSK,
> > > +     },
> > > +     [MAX77650_INT_nEN_R] = {
> > > +             .reg_offset             = MAX77650_INT_GLBL_OFFSET,
> > > +             .mask                   = MAX77650_INT_nEN_R_MSK,
> > > +     },
> > > +     [MAX77650_INT_TJAL1_R] = {
> > > +             .reg_offset             = MAX77650_INT_GLBL_OFFSET,
> > > +             .mask                   = MAX77650_INT_TJAL1_R_MSK,
> > > +     },
> > > +     [MAX77650_INT_TJAL2_R] = {
> > > +             .reg_offset             = MAX77650_INT_GLBL_OFFSET,
> > > +             .mask                   = MAX77650_INT_TJAL2_R_MSK,
> > > +     },
> > > +     [MAX77650_INT_DOD_R] = {
> > > +             .reg_offset             = MAX77650_INT_GLBL_OFFSET,
> > > +             .mask                   = MAX77650_INT_DOD_R_MSK,
> > > +     },
> > > +     [MAX77650_INT_THM] = {
> > > +             .reg_offset             = MAX77650_INT_CHG_OFFSET,
> > > +             .mask                   = MAX77650_INT_THM_MSK,
> > > +     },
> > > +     [MAX77650_INT_CHG] = {
> > > +             .reg_offset             = MAX77650_INT_CHG_OFFSET,
> > > +             .mask                   = MAX77650_INT_CHG_MSK,
> > > +     },
> > > +     [MAX77650_INT_CHGIN] = {
> > > +             .reg_offset             = MAX77650_INT_CHG_OFFSET,
> > > +             .mask                   = MAX77650_INT_CHGIN_MSK,
> > > +     },
> > > +     [MAX77650_INT_TJ_REG] = {
> > > +             .reg_offset             = MAX77650_INT_CHG_OFFSET,
> > > +             .mask                   = MAX77650_INT_TJ_REG_MSK,
> > > +     },
> > > +     [MAX77650_INT_CHGIN_CTRL] = {
> > > +             .reg_offset             = MAX77650_INT_CHG_OFFSET,
> > > +             .mask                   = MAX77650_INT_CHGIN_CTRL_MSK,
> > > +     },
> > > +     [MAX77650_INT_SYS_CTRL] = {
> > > +             .reg_offset             = MAX77650_INT_CHG_OFFSET,
> > > +             .mask                   = MAX77650_INT_SYS_CTRL_MSK,
> > > +     },
> > > +     [MAX77650_INT_SYS_CNFG] = {
> > > +             .reg_offset             = MAX77650_INT_CHG_OFFSET,
> > > +             .mask                   = MAX77650_INT_SYS_CNFG_MSK,
> > > +     },
> > > +};
> >
> > If you get rid of all of the horrible hoop jumping in *_setup_irqs(),
> > you can use REGMAP_IRQ_REG() like everyone else does.
> >
> 
> I could even use it now - except for the first interrupt. I decided to
> not use it everywhere as it looks much better that way than having
> REGMAP_IRQ_REG() for all definitions and then the first one sticking
> out like that. It just looks better.

The way it's done at the moment looks terrible.

Please use the MACROs to simplify as much of the code as possible.

> > > +static const struct regmap_irq_chip max77650_irq_chip = {
> > > +     .name                   = "max77650-irq",
> > > +     .irqs                   = max77650_irqs,
> > > +     .num_irqs               = ARRAY_SIZE(max77650_irqs),
> > > +     .num_regs               = 2,
> > > +     .status_base            = MAX77650_REG_INT_GLBL,
> > > +     .mask_base              = MAX77650_REG_INTM_GLBL,
> > > +     .type_in_mask           = true,
> > > +     .type_invert            = true,
> > > +     .init_ack_masked        = true,
> > > +     .clear_on_unmask        = true,
> > > +};
> > > +
> > > +static const struct regmap_config max77650_regmap_config = {
> > > +     .name           = "max77650",
> > > +     .reg_bits       = 8,
> > > +     .val_bits       = 8,
> > > +};
> > > +
> > > +static int max77650_setup_irqs(struct device *dev, struct mfd_cell *cells)
> > > +{
> > > +     const struct max77650_irq_mapping *mapping;
> > > +     struct regmap_irq_chip_data *irq_data;
> > > +     struct i2c_client *i2c;
> > > +     struct mfd_cell *cell;
> > > +     struct resource *res;
> > > +     struct regmap *map;
> > > +     int i, j, irq, rv;
> > > +
> > > +     i2c = to_i2c_client(dev);
> > > +
> > > +     map = dev_get_regmap(dev, NULL);
> > > +     if (!map)
> > > +             return -ENODEV;
> > > +
> > > +     rv = devm_regmap_add_irq_chip(dev, map, i2c->irq,
> > > +                                   IRQF_ONESHOT | IRQF_SHARED, -1,
> >
> > What is -1?  Are you sure this isn't defined somewhere?
> >
> 
> I don't see any define for negative irq_base argument. I can add that
> in a separate series and convert the users, but for now I'd stick with
> -1.

IMO it should be defined.  You can define it locally for now.

> > > +                                   &max77650_irq_chip, &irq_data);
> > > +     if (rv)
> > > +             return rv;
> > > +
> > > +     for (i = 0; i < ARRAY_SIZE(max77650_irq_mapping_table); i++) {
> > > +             mapping = &max77650_irq_mapping_table[i];
> > > +             cell = &cells[mapping->cell_num];
> > > +
> > > +             res = devm_kcalloc(dev, sizeof(*res),
> > > +                                mapping->num_irqs, GFP_KERNEL);
> > > +             if (!res)
> > > +                     return -ENOMEM;
> > > +
> > > +             cell->resources = res;
> > > +             cell->num_resources = mapping->num_irqs;
> > > +
> > > +             for (j = 0; j < mapping->num_irqs; j++) {
> > > +                     irq = regmap_irq_get_virq(irq_data, mapping->irqs[j]);
> > > +                     if (irq < 0)
> > > +                             return irq;
> > > +
> > > +                     res[j].start = res[j].end = irq;
> > > +                     res[j].flags = IORESOURCE_IRQ;
> > > +                     res[j].name = mapping->irq_names[j];
> > > +             }
> > > +     }
> >
> > This is the first time I've seen it done like this (and I hate it).
> >
> > Why are you storing the virqs in resources?
> >
> > I think this is highly irregular.
> >
> 
> I initially just passed the regmap_irq_chip_data over i2c clientdata
> and sub-drivers would look up virq numbers from it but was advised by
> Dmitry Torokhov to use resources instead. After implementing it this
> way I too think it's more elegant in sub-drivers who can simply do
> platform_get_irq_byname(). Do you have a different idea?

> What exactly don't you like about this?

 * The declaration of a superfluous struct
 * 100 lines of additional/avoidable code
 * Hacky hoop jumping trying to fudge VIRQs into resources
 * Resources were designed for HWIRQs (unless a domain is present)
 * Loads of additional/avoidable CPU cycles setting all this up

Need I go on? :)

Surely the fact that you are using both sides of an API
(devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must
set some alarm bells ringing?

This whole HWIRQ setting, VIRQ getting, resource hacking is a mess.

And for what?  To avoid passing IRQ data to a child driver?

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver

[Bartosz Golaszewski]

wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a):
>
> On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
>
> > wt., 12 lut 2019 o 09:36 Lee Jones <lee.jones@linaro.org> napisał(a):
> > >
> > > On Tue, 05 Feb 2019, Bartosz Golaszewski wrote:
> > >
> > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > >
> > > > Add the core mfd driver for max77650 PMIC. We define five sub-devices
> > > > for which the drivers will be added in subsequent patches.
> > > >
> > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > > ---
> > > >  drivers/mfd/Kconfig          |  11 ++
> > > >  drivers/mfd/Makefile         |   1 +
> > > >  drivers/mfd/max77650.c       | 342 +++++++++++++++++++++++++++++++++++
> > > >  include/linux/mfd/max77650.h |  59 ++++++
> > > >  4 files changed, 413 insertions(+)
> > > >  create mode 100644 drivers/mfd/max77650.c
> > > >  create mode 100644 include/linux/mfd/max77650.h
>
> [...]
>
> > > > +static const struct max77650_irq_mapping max77650_irq_mapping_table[] = {
> > > > +     {
> > > > +             .cell_num       = MAX77650_CELL_CHARGER,
> > > > +             .irqs           = max77650_charger_irqs,
> > > > +             .irq_names      = max77650_charger_irq_names,
> > > > +             .num_irqs       = ARRAY_SIZE(max77650_charger_irqs),
> > > > +     },
> > > > +     {
> > > > +             .cell_num       = MAX77650_CELL_GPIO,
> > > > +             .irqs           = max77650_gpio_irqs,
> > > > +             .irq_names      = max77650_gpio_irq_names,
> > > > +             .num_irqs       = ARRAY_SIZE(max77650_gpio_irqs),
> > > > +     },
> > > > +     {
> > > > +             .cell_num       = MAX77650_CELL_ONKEY,
> > > > +             .irqs           = max77650_onkey_irqs,
> > > > +             .irq_names      = max77650_onkey_irq_names,
> > > > +             .num_irqs       = ARRAY_SIZE(max77650_onkey_irqs),
> > > > +     },
> > > > +};
> > >
> > > This is all a bit convoluted and nasty TBH.
> > >
> > > > +static const struct mfd_cell max77650_cells[] = {
> > > > +     [MAX77650_CELL_REGULATOR] = {
> > > > +             .name           = "max77650-regulator",
> > > > +             .of_compatible  = "maxim,max77650-regulator",
> > > > +     },
> > > > +     [MAX77650_CELL_CHARGER] = {
> > > > +             .name           = "max77650-charger",
> > > > +             .of_compatible  = "maxim,max77650-charger",
> > > > +     },
> > > > +     [MAX77650_CELL_GPIO] = {
> > > > +             .name           = "max77650-gpio",
> > > > +             .of_compatible  = "maxim,max77650-gpio",
> > > > +     },
> > > > +     [MAX77650_CELL_LED] = {
> > > > +             .name           = "max77650-led",
> > > > +             .of_compatible  = "maxim,max77650-led",
> > > > +     },
> > > > +     [MAX77650_CELL_ONKEY] = {
> > > > +             .name           = "max77650-onkey",
> > > > +             .of_compatible  = "maxim,max77650-onkey",
> > > > +     },
> > > > +};
> > >
> > > Why are you numbering the cells?  There is no need to do this.
> > >
> >
> > Just for better readability. It makes sense to me coupled with
> > MAX77650_NUM_CELLS.
>
> You have it the wrong way around.  You define the cell data, then
> provide the number of them using ARRAY_SIZE().  The enum containing
> MAX77650_NUM_CELLS should not exist.
>
> > > > +static const struct regmap_irq max77650_irqs[] = {
> > > > +     [MAX77650_INT_GPI] = {
> > > > +             .reg_offset             = MAX77650_INT_GLBL_OFFSET,
> > > > +             .mask                   = MAX77650_INT_GPI_MSK,
> > > > +             .type = {
> > > > +                     .type_falling_val       = MAX77650_INT_GPI_F_MSK,
> > > > +                     .type_rising_val        = MAX77650_INT_GPI_R_MSK,
> > > > +                     .types_supported        = IRQ_TYPE_EDGE_BOTH,
> > > > +             },
> > > > +     },
> > > > +     [MAX77650_INT_nEN_F] = {
> > > > +             .reg_offset             = MAX77650_INT_GLBL_OFFSET,
> > > > +             .mask                   = MAX77650_INT_nEN_F_MSK,
> > > > +     },
> > > > +     [MAX77650_INT_nEN_R] = {
> > > > +             .reg_offset             = MAX77650_INT_GLBL_OFFSET,
> > > > +             .mask                   = MAX77650_INT_nEN_R_MSK,
> > > > +     },
> > > > +     [MAX77650_INT_TJAL1_R] = {
> > > > +             .reg_offset             = MAX77650_INT_GLBL_OFFSET,
> > > > +             .mask                   = MAX77650_INT_TJAL1_R_MSK,
> > > > +     },
> > > > +     [MAX77650_INT_TJAL2_R] = {
> > > > +             .reg_offset             = MAX77650_INT_GLBL_OFFSET,
> > > > +             .mask                   = MAX77650_INT_TJAL2_R_MSK,
> > > > +     },
> > > > +     [MAX77650_INT_DOD_R] = {
> > > > +             .reg_offset             = MAX77650_INT_GLBL_OFFSET,
> > > > +             .mask                   = MAX77650_INT_DOD_R_MSK,
> > > > +     },
> > > > +     [MAX77650_INT_THM] = {
> > > > +             .reg_offset             = MAX77650_INT_CHG_OFFSET,
> > > > +             .mask                   = MAX77650_INT_THM_MSK,
> > > > +     },
> > > > +     [MAX77650_INT_CHG] = {
> > > > +             .reg_offset             = MAX77650_INT_CHG_OFFSET,
> > > > +             .mask                   = MAX77650_INT_CHG_MSK,
> > > > +     },
> > > > +     [MAX77650_INT_CHGIN] = {
> > > > +             .reg_offset             = MAX77650_INT_CHG_OFFSET,
> > > > +             .mask                   = MAX77650_INT_CHGIN_MSK,
> > > > +     },
> > > > +     [MAX77650_INT_TJ_REG] = {
> > > > +             .reg_offset             = MAX77650_INT_CHG_OFFSET,
> > > > +             .mask                   = MAX77650_INT_TJ_REG_MSK,
> > > > +     },
> > > > +     [MAX77650_INT_CHGIN_CTRL] = {
> > > > +             .reg_offset             = MAX77650_INT_CHG_OFFSET,
> > > > +             .mask                   = MAX77650_INT_CHGIN_CTRL_MSK,
> > > > +     },
> > > > +     [MAX77650_INT_SYS_CTRL] = {
> > > > +             .reg_offset             = MAX77650_INT_CHG_OFFSET,
> > > > +             .mask                   = MAX77650_INT_SYS_CTRL_MSK,
> > > > +     },
> > > > +     [MAX77650_INT_SYS_CNFG] = {
> > > > +             .reg_offset             = MAX77650_INT_CHG_OFFSET,
> > > > +             .mask                   = MAX77650_INT_SYS_CNFG_MSK,
> > > > +     },
> > > > +};
> > >
> > > If you get rid of all of the horrible hoop jumping in *_setup_irqs(),
> > > you can use REGMAP_IRQ_REG() like everyone else does.
> > >
> >
> > I could even use it now - except for the first interrupt. I decided to
> > not use it everywhere as it looks much better that way than having
> > REGMAP_IRQ_REG() for all definitions and then the first one sticking
> > out like that. It just looks better.
>
> The way it's done at the moment looks terrible.
>
> Please use the MACROs to simplify as much of the code as possible.
>
> > > > +static const struct regmap_irq_chip max77650_irq_chip = {
> > > > +     .name                   = "max77650-irq",
> > > > +     .irqs                   = max77650_irqs,
> > > > +     .num_irqs               = ARRAY_SIZE(max77650_irqs),
> > > > +     .num_regs               = 2,
> > > > +     .status_base            = MAX77650_REG_INT_GLBL,
> > > > +     .mask_base              = MAX77650_REG_INTM_GLBL,
> > > > +     .type_in_mask           = true,
> > > > +     .type_invert            = true,
> > > > +     .init_ack_masked        = true,
> > > > +     .clear_on_unmask        = true,
> > > > +};
> > > > +
> > > > +static const struct regmap_config max77650_regmap_config = {
> > > > +     .name           = "max77650",
> > > > +     .reg_bits       = 8,
> > > > +     .val_bits       = 8,
> > > > +};
> > > > +
> > > > +static int max77650_setup_irqs(struct device *dev, struct mfd_cell *cells)
> > > > +{
> > > > +     const struct max77650_irq_mapping *mapping;
> > > > +     struct regmap_irq_chip_data *irq_data;
> > > > +     struct i2c_client *i2c;
> > > > +     struct mfd_cell *cell;
> > > > +     struct resource *res;
> > > > +     struct regmap *map;
> > > > +     int i, j, irq, rv;
> > > > +
> > > > +     i2c = to_i2c_client(dev);
> > > > +
> > > > +     map = dev_get_regmap(dev, NULL);
> > > > +     if (!map)
> > > > +             return -ENODEV;
> > > > +
> > > > +     rv = devm_regmap_add_irq_chip(dev, map, i2c->irq,
> > > > +                                   IRQF_ONESHOT | IRQF_SHARED, -1,
> > >
> > > What is -1?  Are you sure this isn't defined somewhere?
> > >
> >
> > I don't see any define for negative irq_base argument. I can add that
> > in a separate series and convert the users, but for now I'd stick with
> > -1.
>
> IMO it should be defined.  You can define it locally for now.
>
> > > > +                                   &max77650_irq_chip, &irq_data);
> > > > +     if (rv)
> > > > +             return rv;
> > > > +
> > > > +     for (i = 0; i < ARRAY_SIZE(max77650_irq_mapping_table); i++) {
> > > > +             mapping = &max77650_irq_mapping_table[i];
> > > > +             cell = &cells[mapping->cell_num];
> > > > +
> > > > +             res = devm_kcalloc(dev, sizeof(*res),
> > > > +                                mapping->num_irqs, GFP_KERNEL);
> > > > +             if (!res)
> > > > +                     return -ENOMEM;
> > > > +
> > > > +             cell->resources = res;
> > > > +             cell->num_resources = mapping->num_irqs;
> > > > +
> > > > +             for (j = 0; j < mapping->num_irqs; j++) {
> > > > +                     irq = regmap_irq_get_virq(irq_data, mapping->irqs[j]);
> > > > +                     if (irq < 0)
> > > > +                             return irq;
> > > > +
> > > > +                     res[j].start = res[j].end = irq;
> > > > +                     res[j].flags = IORESOURCE_IRQ;
> > > > +                     res[j].name = mapping->irq_names[j];
> > > > +             }
> > > > +     }
> > >
> > > This is the first time I've seen it done like this (and I hate it).
> > >
> > > Why are you storing the virqs in resources?
> > >
> > > I think this is highly irregular.
> > >
> >
> > I initially just passed the regmap_irq_chip_data over i2c clientdata
> > and sub-drivers would look up virq numbers from it but was advised by
> > Dmitry Torokhov to use resources instead. After implementing it this
> > way I too think it's more elegant in sub-drivers who can simply do
> > platform_get_irq_byname(). Do you have a different idea?
>
> > What exactly don't you like about this?
>
>  * The declaration of a superfluous struct
>  * 100 lines of additional/avoidable code
>  * Hacky hoop jumping trying to fudge VIRQs into resources
>  * Resources were designed for HWIRQs (unless a domain is present)
>  * Loads of additional/avoidable CPU cycles setting all this up

While the above may be right, this one is negligible and you know it. :)

>
> Need I go on? :)
>
> Surely the fact that you are using both sides of an API
> (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must
> set some alarm bells ringing?
>
> This whole HWIRQ setting, VIRQ getting, resource hacking is a mess.
>
> And for what?  To avoid passing IRQ data to a child driver?
>

What do you propose? Should I go back to the approach in v1 and pass
the regmap_irq_chip_data to child drivers?

@Dmitry: what do you think?

Bart

Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver

[Lee Jones]

On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:

> wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a):
> >
> > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> >
> > > wt., 12 lut 2019 o 09:36 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > >
> > > > On Tue, 05 Feb 2019, Bartosz Golaszewski wrote:
> > > >
> > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > > >
> > > > > Add the core mfd driver for max77650 PMIC. We define five sub-devices
> > > > > for which the drivers will be added in subsequent patches.
> > > > >
> > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > > > ---
> > > > >  drivers/mfd/Kconfig          |  11 ++
> > > > >  drivers/mfd/Makefile         |   1 +
> > > > >  drivers/mfd/max77650.c       | 342 +++++++++++++++++++++++++++++++++++
> > > > >  include/linux/mfd/max77650.h |  59 ++++++
> > > > >  4 files changed, 413 insertions(+)
> > > > >  create mode 100644 drivers/mfd/max77650.c
> > > > >  create mode 100644 include/linux/mfd/max77650.h
> >
> > [...]
> >
> > > > > +static const struct max77650_irq_mapping max77650_irq_mapping_table[] = {
> > > > > +     {
> > > > > +             .cell_num       = MAX77650_CELL_CHARGER,
> > > > > +             .irqs           = max77650_charger_irqs,
> > > > > +             .irq_names      = max77650_charger_irq_names,
> > > > > +             .num_irqs       = ARRAY_SIZE(max77650_charger_irqs),
> > > > > +     },
> > > > > +     {
> > > > > +             .cell_num       = MAX77650_CELL_GPIO,
> > > > > +             .irqs           = max77650_gpio_irqs,
> > > > > +             .irq_names      = max77650_gpio_irq_names,
> > > > > +             .num_irqs       = ARRAY_SIZE(max77650_gpio_irqs),
> > > > > +     },
> > > > > +     {
> > > > > +             .cell_num       = MAX77650_CELL_ONKEY,
> > > > > +             .irqs           = max77650_onkey_irqs,
> > > > > +             .irq_names      = max77650_onkey_irq_names,
> > > > > +             .num_irqs       = ARRAY_SIZE(max77650_onkey_irqs),
> > > > > +     },
> > > > > +};
> > > >
> > > > This is all a bit convoluted and nasty TBH.
> > > >
> > > > > +static const struct mfd_cell max77650_cells[] = {
> > > > > +     [MAX77650_CELL_REGULATOR] = {
> > > > > +             .name           = "max77650-regulator",
> > > > > +             .of_compatible  = "maxim,max77650-regulator",
> > > > > +     },
> > > > > +     [MAX77650_CELL_CHARGER] = {
> > > > > +             .name           = "max77650-charger",
> > > > > +             .of_compatible  = "maxim,max77650-charger",
> > > > > +     },
> > > > > +     [MAX77650_CELL_GPIO] = {
> > > > > +             .name           = "max77650-gpio",
> > > > > +             .of_compatible  = "maxim,max77650-gpio",
> > > > > +     },
> > > > > +     [MAX77650_CELL_LED] = {
> > > > > +             .name           = "max77650-led",
> > > > > +             .of_compatible  = "maxim,max77650-led",
> > > > > +     },
> > > > > +     [MAX77650_CELL_ONKEY] = {
> > > > > +             .name           = "max77650-onkey",
> > > > > +             .of_compatible  = "maxim,max77650-onkey",
> > > > > +     },
> > > > > +};
> > > >
> > > > Why are you numbering the cells?  There is no need to do this.
> > > >
> > >
> > > Just for better readability. It makes sense to me coupled with
> > > MAX77650_NUM_CELLS.
> >
> > You have it the wrong way around.  You define the cell data, then
> > provide the number of them using ARRAY_SIZE().  The enum containing
> > MAX77650_NUM_CELLS should not exist.
> >
> > > > > +static const struct regmap_irq max77650_irqs[] = {
> > > > > +     [MAX77650_INT_GPI] = {
> > > > > +             .reg_offset             = MAX77650_INT_GLBL_OFFSET,
> > > > > +             .mask                   = MAX77650_INT_GPI_MSK,
> > > > > +             .type = {
> > > > > +                     .type_falling_val       = MAX77650_INT_GPI_F_MSK,
> > > > > +                     .type_rising_val        = MAX77650_INT_GPI_R_MSK,
> > > > > +                     .types_supported        = IRQ_TYPE_EDGE_BOTH,
> > > > > +             },
> > > > > +     },
> > > > > +     [MAX77650_INT_nEN_F] = {
> > > > > +             .reg_offset             = MAX77650_INT_GLBL_OFFSET,
> > > > > +             .mask                   = MAX77650_INT_nEN_F_MSK,
> > > > > +     },
> > > > > +     [MAX77650_INT_nEN_R] = {
> > > > > +             .reg_offset             = MAX77650_INT_GLBL_OFFSET,
> > > > > +             .mask                   = MAX77650_INT_nEN_R_MSK,
> > > > > +     },
> > > > > +     [MAX77650_INT_TJAL1_R] = {
> > > > > +             .reg_offset             = MAX77650_INT_GLBL_OFFSET,
> > > > > +             .mask                   = MAX77650_INT_TJAL1_R_MSK,
> > > > > +     },
> > > > > +     [MAX77650_INT_TJAL2_R] = {
> > > > > +             .reg_offset             = MAX77650_INT_GLBL_OFFSET,
> > > > > +             .mask                   = MAX77650_INT_TJAL2_R_MSK,
> > > > > +     },
> > > > > +     [MAX77650_INT_DOD_R] = {
> > > > > +             .reg_offset             = MAX77650_INT_GLBL_OFFSET,
> > > > > +             .mask                   = MAX77650_INT_DOD_R_MSK,
> > > > > +     },
> > > > > +     [MAX77650_INT_THM] = {
> > > > > +             .reg_offset             = MAX77650_INT_CHG_OFFSET,
> > > > > +             .mask                   = MAX77650_INT_THM_MSK,
> > > > > +     },
> > > > > +     [MAX77650_INT_CHG] = {
> > > > > +             .reg_offset             = MAX77650_INT_CHG_OFFSET,
> > > > > +             .mask                   = MAX77650_INT_CHG_MSK,
> > > > > +     },
> > > > > +     [MAX77650_INT_CHGIN] = {
> > > > > +             .reg_offset             = MAX77650_INT_CHG_OFFSET,
> > > > > +             .mask                   = MAX77650_INT_CHGIN_MSK,
> > > > > +     },
> > > > > +     [MAX77650_INT_TJ_REG] = {
> > > > > +             .reg_offset             = MAX77650_INT_CHG_OFFSET,
> > > > > +             .mask                   = MAX77650_INT_TJ_REG_MSK,
> > > > > +     },
> > > > > +     [MAX77650_INT_CHGIN_CTRL] = {
> > > > > +             .reg_offset             = MAX77650_INT_CHG_OFFSET,
> > > > > +             .mask                   = MAX77650_INT_CHGIN_CTRL_MSK,
> > > > > +     },
> > > > > +     [MAX77650_INT_SYS_CTRL] = {
> > > > > +             .reg_offset             = MAX77650_INT_CHG_OFFSET,
> > > > > +             .mask                   = MAX77650_INT_SYS_CTRL_MSK,
> > > > > +     },
> > > > > +     [MAX77650_INT_SYS_CNFG] = {
> > > > > +             .reg_offset             = MAX77650_INT_CHG_OFFSET,
> > > > > +             .mask                   = MAX77650_INT_SYS_CNFG_MSK,
> > > > > +     },
> > > > > +};
> > > >
> > > > If you get rid of all of the horrible hoop jumping in *_setup_irqs(),
> > > > you can use REGMAP_IRQ_REG() like everyone else does.
> > > >
> > >
> > > I could even use it now - except for the first interrupt. I decided to
> > > not use it everywhere as it looks much better that way than having
> > > REGMAP_IRQ_REG() for all definitions and then the first one sticking
> > > out like that. It just looks better.
> >
> > The way it's done at the moment looks terrible.
> >
> > Please use the MACROs to simplify as much of the code as possible.
> >
> > > > > +static const struct regmap_irq_chip max77650_irq_chip = {
> > > > > +     .name                   = "max77650-irq",
> > > > > +     .irqs                   = max77650_irqs,
> > > > > +     .num_irqs               = ARRAY_SIZE(max77650_irqs),
> > > > > +     .num_regs               = 2,
> > > > > +     .status_base            = MAX77650_REG_INT_GLBL,
> > > > > +     .mask_base              = MAX77650_REG_INTM_GLBL,
> > > > > +     .type_in_mask           = true,
> > > > > +     .type_invert            = true,
> > > > > +     .init_ack_masked        = true,
> > > > > +     .clear_on_unmask        = true,
> > > > > +};
> > > > > +
> > > > > +static const struct regmap_config max77650_regmap_config = {
> > > > > +     .name           = "max77650",
> > > > > +     .reg_bits       = 8,
> > > > > +     .val_bits       = 8,
> > > > > +};
> > > > > +
> > > > > +static int max77650_setup_irqs(struct device *dev, struct mfd_cell *cells)
> > > > > +{
> > > > > +     const struct max77650_irq_mapping *mapping;
> > > > > +     struct regmap_irq_chip_data *irq_data;
> > > > > +     struct i2c_client *i2c;
> > > > > +     struct mfd_cell *cell;
> > > > > +     struct resource *res;
> > > > > +     struct regmap *map;
> > > > > +     int i, j, irq, rv;
> > > > > +
> > > > > +     i2c = to_i2c_client(dev);
> > > > > +
> > > > > +     map = dev_get_regmap(dev, NULL);
> > > > > +     if (!map)
> > > > > +             return -ENODEV;
> > > > > +
> > > > > +     rv = devm_regmap_add_irq_chip(dev, map, i2c->irq,
> > > > > +                                   IRQF_ONESHOT | IRQF_SHARED, -1,
> > > >
> > > > What is -1?  Are you sure this isn't defined somewhere?
> > > >
> > >
> > > I don't see any define for negative irq_base argument. I can add that
> > > in a separate series and convert the users, but for now I'd stick with
> > > -1.
> >
> > IMO it should be defined.  You can define it locally for now.
> >
> > > > > +                                   &max77650_irq_chip, &irq_data);
> > > > > +     if (rv)
> > > > > +             return rv;
> > > > > +
> > > > > +     for (i = 0; i < ARRAY_SIZE(max77650_irq_mapping_table); i++) {
> > > > > +             mapping = &max77650_irq_mapping_table[i];
> > > > > +             cell = &cells[mapping->cell_num];
> > > > > +
> > > > > +             res = devm_kcalloc(dev, sizeof(*res),
> > > > > +                                mapping->num_irqs, GFP_KERNEL);
> > > > > +             if (!res)
> > > > > +                     return -ENOMEM;
> > > > > +
> > > > > +             cell->resources = res;
> > > > > +             cell->num_resources = mapping->num_irqs;
> > > > > +
> > > > > +             for (j = 0; j < mapping->num_irqs; j++) {
> > > > > +                     irq = regmap_irq_get_virq(irq_data, mapping->irqs[j]);
> > > > > +                     if (irq < 0)
> > > > > +                             return irq;
> > > > > +
> > > > > +                     res[j].start = res[j].end = irq;
> > > > > +                     res[j].flags = IORESOURCE_IRQ;
> > > > > +                     res[j].name = mapping->irq_names[j];
> > > > > +             }
> > > > > +     }
> > > >
> > > > This is the first time I've seen it done like this (and I hate it).
> > > >
> > > > Why are you storing the virqs in resources?
> > > >
> > > > I think this is highly irregular.
> > > >
> > >
> > > I initially just passed the regmap_irq_chip_data over i2c clientdata
> > > and sub-drivers would look up virq numbers from it but was advised by
> > > Dmitry Torokhov to use resources instead. After implementing it this
> > > way I too think it's more elegant in sub-drivers who can simply do
> > > platform_get_irq_byname(). Do you have a different idea?
> >
> > > What exactly don't you like about this?
> >
> >  * The declaration of a superfluous struct
> >  * 100 lines of additional/avoidable code
> >  * Hacky hoop jumping trying to fudge VIRQs into resources
> >  * Resources were designed for HWIRQs (unless a domain is present)
> >  * Loads of additional/avoidable CPU cycles setting all this up
> 
> While the above may be right, this one is negligible and you know it. :)

You have nested for() loops.  You *are* wasting lots of cycles.

> > Need I go on? :)
> >
> > Surely the fact that you are using both sides of an API
> > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must
> > set some alarm bells ringing?
> >
> > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess.
> >
> > And for what?  To avoid passing IRQ data to a child driver?
> 
> What do you propose? Should I go back to the approach in v1 and pass
> the regmap_irq_chip_data to child drivers?

I'm saying you should remove all of this hackery and pass IRQs as they
are supposed to be passed (like everyone else does).

> @Dmitry: what do you think?

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver

[Bartosz Golaszewski]

wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a):
>
> On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
>
> > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a):
> > >
> > >  * The declaration of a superfluous struct
> > >  * 100 lines of additional/avoidable code
> > >  * Hacky hoop jumping trying to fudge VIRQs into resources
> > >  * Resources were designed for HWIRQs (unless a domain is present)
> > >  * Loads of additional/avoidable CPU cycles setting all this up
> >
> > While the above may be right, this one is negligible and you know it. :)
>
> You have nested for() loops.  You *are* wasting lots of cycles.
>
> > > Need I go on? :)
> > >
> > > Surely the fact that you are using both sides of an API
> > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must
> > > set some alarm bells ringing?
> > >
> > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess.
> > >
> > > And for what?  To avoid passing IRQ data to a child driver?
> >
> > What do you propose? Should I go back to the approach in v1 and pass
> > the regmap_irq_chip_data to child drivers?
>
> I'm saying you should remove all of this hackery and pass IRQs as they
> are supposed to be passed (like everyone else does).
>

I'm not sure what you mean by "like everyone else does" - different
mfd drivers seem to be doing different things. Is a simple struct
containing virtual irq numbers passed to sub-drivers fine?

Bart

Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver

[Lee Jones]

On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:

> wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a):
> >
> > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> >
> > > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > >
> > > >  * The declaration of a superfluous struct
> > > >  * 100 lines of additional/avoidable code
> > > >  * Hacky hoop jumping trying to fudge VIRQs into resources
> > > >  * Resources were designed for HWIRQs (unless a domain is present)
> > > >  * Loads of additional/avoidable CPU cycles setting all this up
> > >
> > > While the above may be right, this one is negligible and you know it. :)
> >
> > You have nested for() loops.  You *are* wasting lots of cycles.
> >
> > > > Need I go on? :)
> > > >
> > > > Surely the fact that you are using both sides of an API
> > > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must
> > > > set some alarm bells ringing?
> > > >
> > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess.
> > > >
> > > > And for what?  To avoid passing IRQ data to a child driver?
> > >
> > > What do you propose? Should I go back to the approach in v1 and pass
> > > the regmap_irq_chip_data to child drivers?
> >
> > I'm saying you should remove all of this hackery and pass IRQs as they
> > are supposed to be passed (like everyone else does).
> 
> I'm not sure what you mean by "like everyone else does" - different
> mfd drivers seem to be doing different things. Is a simple struct
> containing virtual irq numbers passed to sub-drivers fine?

How do you plan on deriving the VIRQs to place into the struct?

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver

[Bartosz Golaszewski]

wt., 12 lut 2019 o 12:14 Lee Jones <lee.jones@linaro.org> napisał(a):
>
> On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
>
> > wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a):
> > >
> > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > >
> > > > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > >
> > > > >  * The declaration of a superfluous struct
> > > > >  * 100 lines of additional/avoidable code
> > > > >  * Hacky hoop jumping trying to fudge VIRQs into resources
> > > > >  * Resources were designed for HWIRQs (unless a domain is present)
> > > > >  * Loads of additional/avoidable CPU cycles setting all this up
> > > >
> > > > While the above may be right, this one is negligible and you know it. :)
> > >
> > > You have nested for() loops.  You *are* wasting lots of cycles.
> > >
> > > > > Need I go on? :)
> > > > >
> > > > > Surely the fact that you are using both sides of an API
> > > > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must
> > > > > set some alarm bells ringing?
> > > > >
> > > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess.
> > > > >
> > > > > And for what?  To avoid passing IRQ data to a child driver?
> > > >
> > > > What do you propose? Should I go back to the approach in v1 and pass
> > > > the regmap_irq_chip_data to child drivers?
> > >
> > > I'm saying you should remove all of this hackery and pass IRQs as they
> > > are supposed to be passed (like everyone else does).
> >
> > I'm not sure what you mean by "like everyone else does" - different
> > mfd drivers seem to be doing different things. Is a simple struct
> > containing virtual irq numbers passed to sub-drivers fine?
>
> How do you plan on deriving the VIRQs to place into the struct?

Exampe:

struct max77650_gpio_pdata {
    int gpi_irq;
};

In MFD driver:

struct max77650_gpio_pdata *gpio_data = devm_kmalloc(dev, sizeof(*gpio_data));

gpio_data->gpi_irq = regmap_irq_get_virq(irqchip_data, GPI_NUM);

gpio_cell.platform_data = gpio_data;

In GPIO driver:

struct max77650_gpio_pdata *gpio_data = pdev->dev.platform_data;

int irq = gpio_data->gpi_irq;

Bart

Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver

[Lee Jones]

On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:

> wt., 12 lut 2019 o 12:14 Lee Jones <lee.jones@linaro.org> napisał(a):
> >
> > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> >
> > > wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > >
> > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > >
> > > > > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > >
> > > > > >  * The declaration of a superfluous struct
> > > > > >  * 100 lines of additional/avoidable code
> > > > > >  * Hacky hoop jumping trying to fudge VIRQs into resources
> > > > > >  * Resources were designed for HWIRQs (unless a domain is present)
> > > > > >  * Loads of additional/avoidable CPU cycles setting all this up
> > > > >
> > > > > While the above may be right, this one is negligible and you know it. :)
> > > >
> > > > You have nested for() loops.  You *are* wasting lots of cycles.
> > > >
> > > > > > Need I go on? :)
> > > > > >
> > > > > > Surely the fact that you are using both sides of an API
> > > > > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must
> > > > > > set some alarm bells ringing?
> > > > > >
> > > > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess.
> > > > > >
> > > > > > And for what?  To avoid passing IRQ data to a child driver?
> > > > >
> > > > > What do you propose? Should I go back to the approach in v1 and pass
> > > > > the regmap_irq_chip_data to child drivers?
> > > >
> > > > I'm saying you should remove all of this hackery and pass IRQs as they
> > > > are supposed to be passed (like everyone else does).
> > >
> > > I'm not sure what you mean by "like everyone else does" - different
> > > mfd drivers seem to be doing different things. Is a simple struct
> > > containing virtual irq numbers passed to sub-drivers fine?
> >
> > How do you plan on deriving the VIRQs to place into the struct?
> 
> Exampe:
> 
> struct max77650_gpio_pdata {
>     int gpi_irq;
> };
> 
> In MFD driver:
> 
> struct max77650_gpio_pdata *gpio_data = devm_kmalloc(dev, sizeof(*gpio_data));
> 
> gpio_data->gpi_irq = regmap_irq_get_virq(irqchip_data, GPI_NUM);
> 
> gpio_cell.platform_data = gpio_data;
> 
> In GPIO driver:
> 
> struct max77650_gpio_pdata *gpio_data = pdev->dev.platform_data;
> 
> int irq = gpio_data->gpi_irq;

Definitely not.  What you're trying to do is a hack.

If you're using Regmap to handle your IRQs, then you should use Regmap
in the client to pull them out.  Setting them via Regmap, then pulling
them out again in the *same driver*, only to store them in platform
data to be passed to a child device is bonkers.

*Either* use the MFD provided platform-data helpers *or* pass and
handle them via the Regmap APIs, *not* both.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver

[Lee Jones]

On Tue, 12 Feb 2019, Lee Jones wrote:

> On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> 
> > wt., 12 lut 2019 o 12:14 Lee Jones <lee.jones@linaro.org> napisał(a):
> > >
> > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > >
> > > > wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > >
> > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > >
> > > > > > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > >
> > > > > > >  * The declaration of a superfluous struct
> > > > > > >  * 100 lines of additional/avoidable code
> > > > > > >  * Hacky hoop jumping trying to fudge VIRQs into resources
> > > > > > >  * Resources were designed for HWIRQs (unless a domain is present)
> > > > > > >  * Loads of additional/avoidable CPU cycles setting all this up
> > > > > >
> > > > > > While the above may be right, this one is negligible and you know it. :)
> > > > >
> > > > > You have nested for() loops.  You *are* wasting lots of cycles.
> > > > >
> > > > > > > Need I go on? :)
> > > > > > >
> > > > > > > Surely the fact that you are using both sides of an API
> > > > > > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must
> > > > > > > set some alarm bells ringing?
> > > > > > >
> > > > > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess.
> > > > > > >
> > > > > > > And for what?  To avoid passing IRQ data to a child driver?
> > > > > >
> > > > > > What do you propose? Should I go back to the approach in v1 and pass
> > > > > > the regmap_irq_chip_data to child drivers?
> > > > >
> > > > > I'm saying you should remove all of this hackery and pass IRQs as they
> > > > > are supposed to be passed (like everyone else does).
> > > >
> > > > I'm not sure what you mean by "like everyone else does" - different
> > > > mfd drivers seem to be doing different things. Is a simple struct
> > > > containing virtual irq numbers passed to sub-drivers fine?
> > >
> > > How do you plan on deriving the VIRQs to place into the struct?
> > 
> > Exampe:
> > 
> > struct max77650_gpio_pdata {
> >     int gpi_irq;
> > };
> > 
> > In MFD driver:
> > 
> > struct max77650_gpio_pdata *gpio_data = devm_kmalloc(dev, sizeof(*gpio_data));
> > 
> > gpio_data->gpi_irq = regmap_irq_get_virq(irqchip_data, GPI_NUM);
> > 
> > gpio_cell.platform_data = gpio_data;
> > 
> > In GPIO driver:
> > 
> > struct max77650_gpio_pdata *gpio_data = pdev->dev.platform_data;
> > 
> > int irq = gpio_data->gpi_irq;
> 
> Definitely not.  What you're trying to do is a hack.
> 
> If you're using Regmap to handle your IRQs, then you should use Regmap
> in the client to pull them out.  Setting them via Regmap, then pulling
> them out again in the *same driver*, only to store them in platform
> data to be passed to a child device is bonkers.
> 
> *Either* use the MFD provided platform-data helpers *or* pass and
> handle them via the Regmap APIs, *not* both.

Right, a plan has been formed.

Hopefully this works and you can avoid all this dancing around.

Firstly, you need to make a small change to:

  drivers/base/regmap/regmap-irq.c

Add the following function:

  struct irq_domain *regmap_irq_get_domain(struct regmap *map)

As you can see, it will return the IRQ Domain for the chip.

You can then pass this IRQ domain to mfd_add_devices() and it will do
the HWIRQ => VIRQ mapping for you on the fly.  Meaning that you can
remove all the nastiness in max77650_setup_irqs() and have the Input
device use the standard (e.g. platform_get_irq()) APIs.

How does that Sound?

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver

[Bartosz Golaszewski]

śr., 13 lut 2019 o 10:25 Lee Jones <lee.jones@linaro.org> napisał(a):
>
> On Tue, 12 Feb 2019, Lee Jones wrote:
>
> > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> >
> > > wt., 12 lut 2019 o 12:14 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > >
> > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > >
> > > > > wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > >
> > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > > >
> > > > > > > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > > >
> > > > > > > >  * The declaration of a superfluous struct
> > > > > > > >  * 100 lines of additional/avoidable code
> > > > > > > >  * Hacky hoop jumping trying to fudge VIRQs into resources
> > > > > > > >  * Resources were designed for HWIRQs (unless a domain is present)
> > > > > > > >  * Loads of additional/avoidable CPU cycles setting all this up
> > > > > > >
> > > > > > > While the above may be right, this one is negligible and you know it. :)
> > > > > >
> > > > > > You have nested for() loops.  You *are* wasting lots of cycles.
> > > > > >
> > > > > > > > Need I go on? :)
> > > > > > > >
> > > > > > > > Surely the fact that you are using both sides of an API
> > > > > > > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must
> > > > > > > > set some alarm bells ringing?
> > > > > > > >
> > > > > > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess.
> > > > > > > >
> > > > > > > > And for what?  To avoid passing IRQ data to a child driver?
> > > > > > >
> > > > > > > What do you propose? Should I go back to the approach in v1 and pass
> > > > > > > the regmap_irq_chip_data to child drivers?
> > > > > >
> > > > > > I'm saying you should remove all of this hackery and pass IRQs as they
> > > > > > are supposed to be passed (like everyone else does).
> > > > >
> > > > > I'm not sure what you mean by "like everyone else does" - different
> > > > > mfd drivers seem to be doing different things. Is a simple struct
> > > > > containing virtual irq numbers passed to sub-drivers fine?
> > > >
> > > > How do you plan on deriving the VIRQs to place into the struct?
> > >
> > > Exampe:
> > >
> > > struct max77650_gpio_pdata {
> > >     int gpi_irq;
> > > };
> > >
> > > In MFD driver:
> > >
> > > struct max77650_gpio_pdata *gpio_data = devm_kmalloc(dev, sizeof(*gpio_data));
> > >
> > > gpio_data->gpi_irq = regmap_irq_get_virq(irqchip_data, GPI_NUM);
> > >
> > > gpio_cell.platform_data = gpio_data;
> > >
> > > In GPIO driver:
> > >
> > > struct max77650_gpio_pdata *gpio_data = pdev->dev.platform_data;
> > >
> > > int irq = gpio_data->gpi_irq;
> >
> > Definitely not.  What you're trying to do is a hack.
> >
> > If you're using Regmap to handle your IRQs, then you should use Regmap
> > in the client to pull them out.  Setting them via Regmap, then pulling
> > them out again in the *same driver*, only to store them in platform
> > data to be passed to a child device is bonkers.
> >
> > *Either* use the MFD provided platform-data helpers *or* pass and
> > handle them via the Regmap APIs, *not* both.
>
> Right, a plan has been formed.
>
> Hopefully this works and you can avoid all this dancing around.
>
> Firstly, you need to make a small change to:
>
>   drivers/base/regmap/regmap-irq.c
>
> Add the following function:
>
>   struct irq_domain *regmap_irq_get_domain(struct regmap *map)
>

We already do have such function and a lot of mfd drivers actually use it.

> As you can see, it will return the IRQ Domain for the chip.
>
> You can then pass this IRQ domain to mfd_add_devices() and it will do
> the HWIRQ => VIRQ mapping for you on the fly.  Meaning that you can
> remove all the nastiness in max77650_setup_irqs() and have the Input
> device use the standard (e.g. platform_get_irq()) APIs.
>
> How does that Sound?
>

This does sound better! Why didn't you lead with that in the first place?

It's a pity it's not documented, I had to look at the code to find out
irq resources would get translated in mfd_add_devices() if a domain is
present.

In that case - I really don't see a reason to create a superfluous
structure to only hold the main regmap - we can very well get it from
the parent device in sub-drivers as I do now.

Thanks,
Bartosz

Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver

[Lee Jones]

On Wed, 13 Feb 2019, Bartosz Golaszewski wrote:

> śr., 13 lut 2019 o 10:25 Lee Jones <lee.jones@linaro.org> napisał(a):
> >
> > On Tue, 12 Feb 2019, Lee Jones wrote:
> >
> > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > >
> > > > wt., 12 lut 2019 o 12:14 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > >
> > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > >
> > > > > > wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > >
> > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > > > >
> > > > > > > > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > > > >
> > > > > > > > >  * The declaration of a superfluous struct
> > > > > > > > >  * 100 lines of additional/avoidable code
> > > > > > > > >  * Hacky hoop jumping trying to fudge VIRQs into resources
> > > > > > > > >  * Resources were designed for HWIRQs (unless a domain is present)
> > > > > > > > >  * Loads of additional/avoidable CPU cycles setting all this up
> > > > > > > >
> > > > > > > > While the above may be right, this one is negligible and you know it. :)
> > > > > > >
> > > > > > > You have nested for() loops.  You *are* wasting lots of cycles.
> > > > > > >
> > > > > > > > > Need I go on? :)
> > > > > > > > >
> > > > > > > > > Surely the fact that you are using both sides of an API
> > > > > > > > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must
> > > > > > > > > set some alarm bells ringing?
> > > > > > > > >
> > > > > > > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess.
> > > > > > > > >
> > > > > > > > > And for what?  To avoid passing IRQ data to a child driver?
> > > > > > > >
> > > > > > > > What do you propose? Should I go back to the approach in v1 and pass
> > > > > > > > the regmap_irq_chip_data to child drivers?
> > > > > > >
> > > > > > > I'm saying you should remove all of this hackery and pass IRQs as they
> > > > > > > are supposed to be passed (like everyone else does).
> > > > > >
> > > > > > I'm not sure what you mean by "like everyone else does" - different
> > > > > > mfd drivers seem to be doing different things. Is a simple struct
> > > > > > containing virtual irq numbers passed to sub-drivers fine?
> > > > >
> > > > > How do you plan on deriving the VIRQs to place into the struct?
> > > >
> > > > Exampe:
> > > >
> > > > struct max77650_gpio_pdata {
> > > >     int gpi_irq;
> > > > };
> > > >
> > > > In MFD driver:
> > > >
> > > > struct max77650_gpio_pdata *gpio_data = devm_kmalloc(dev, sizeof(*gpio_data));
> > > >
> > > > gpio_data->gpi_irq = regmap_irq_get_virq(irqchip_data, GPI_NUM);
> > > >
> > > > gpio_cell.platform_data = gpio_data;
> > > >
> > > > In GPIO driver:
> > > >
> > > > struct max77650_gpio_pdata *gpio_data = pdev->dev.platform_data;
> > > >
> > > > int irq = gpio_data->gpi_irq;
> > >
> > > Definitely not.  What you're trying to do is a hack.
> > >
> > > If you're using Regmap to handle your IRQs, then you should use Regmap
> > > in the client to pull them out.  Setting them via Regmap, then pulling
> > > them out again in the *same driver*, only to store them in platform
> > > data to be passed to a child device is bonkers.
> > >
> > > *Either* use the MFD provided platform-data helpers *or* pass and
> > > handle them via the Regmap APIs, *not* both.
> >
> > Right, a plan has been formed.
> >
> > Hopefully this works and you can avoid all this dancing around.
> >
> > Firstly, you need to make a small change to:
> >
> >   drivers/base/regmap/regmap-irq.c
> >
> > Add the following function:
> >
> >   struct irq_domain *regmap_irq_get_domain(struct regmap *map)
> 
> We already do have such function and a lot of mfd drivers actually use it.

Even better.

> > As you can see, it will return the IRQ Domain for the chip.
> >
> > You can then pass this IRQ domain to mfd_add_devices() and it will do
> > the HWIRQ => VIRQ mapping for you on the fly.  Meaning that you can
> > remove all the nastiness in max77650_setup_irqs() and have the Input
> > device use the standard (e.g. platform_get_irq()) APIs.
> >
> > How does that Sound?
> 
> This does sound better! Why didn't you lead with that in the first place?

I'm not even going to dignify that stupid question with a response.

> It's a pity it's not documented, I had to look at the code to find out
> irq resources would get translated in mfd_add_devices() if a domain is
> present.

Where is it likely to be documented?  MFD is pretty simple and seldom
needs explanation.  A 3 second look at the API you're trying to use
(instead of blind copy & paste) would have told you that it's possible
to take an IRQ domain as an argument.

It's only the craziness in this patch which forced me to look into how
Regmap handles IRQs and come up with a subsequent solution which fits
your use-case.

> In that case - I really don't see a reason to create a superfluous
> structure to only hold the main regmap - we can very well get it from
> the parent device in sub-drivers as I do now.

The whole point of this solution is that you don't need to pass
anything non-standard (i.e. not provided by the current APIs) to the
child device.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver

[Bartosz Golaszewski]

śr., 13 lut 2019 o 10:53 Lee Jones <lee.jones@linaro.org> napisał(a):
>
> On Wed, 13 Feb 2019, Bartosz Golaszewski wrote:
>
> > śr., 13 lut 2019 o 10:25 Lee Jones <lee.jones@linaro.org> napisał(a):
> > >
> > > On Tue, 12 Feb 2019, Lee Jones wrote:
> > >
> > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > >
> > > > > wt., 12 lut 2019 o 12:14 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > >
> > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > > >
> > > > > > > wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > > >
> > > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > > > > >
> > > > > > > > > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > > > > >
> > > > > > > > > >  * The declaration of a superfluous struct
> > > > > > > > > >  * 100 lines of additional/avoidable code
> > > > > > > > > >  * Hacky hoop jumping trying to fudge VIRQs into resources
> > > > > > > > > >  * Resources were designed for HWIRQs (unless a domain is present)
> > > > > > > > > >  * Loads of additional/avoidable CPU cycles setting all this up
> > > > > > > > >
> > > > > > > > > While the above may be right, this one is negligible and you know it. :)
> > > > > > > >
> > > > > > > > You have nested for() loops.  You *are* wasting lots of cycles.
> > > > > > > >
> > > > > > > > > > Need I go on? :)
> > > > > > > > > >
> > > > > > > > > > Surely the fact that you are using both sides of an API
> > > > > > > > > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must
> > > > > > > > > > set some alarm bells ringing?
> > > > > > > > > >
> > > > > > > > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess.
> > > > > > > > > >
> > > > > > > > > > And for what?  To avoid passing IRQ data to a child driver?
> > > > > > > > >
> > > > > > > > > What do you propose? Should I go back to the approach in v1 and pass
> > > > > > > > > the regmap_irq_chip_data to child drivers?
> > > > > > > >
> > > > > > > > I'm saying you should remove all of this hackery and pass IRQs as they
> > > > > > > > are supposed to be passed (like everyone else does).
> > > > > > >
> > > > > > > I'm not sure what you mean by "like everyone else does" - different
> > > > > > > mfd drivers seem to be doing different things. Is a simple struct
> > > > > > > containing virtual irq numbers passed to sub-drivers fine?
> > > > > >
> > > > > > How do you plan on deriving the VIRQs to place into the struct?
> > > > >
> > > > > Exampe:
> > > > >
> > > > > struct max77650_gpio_pdata {
> > > > >     int gpi_irq;
> > > > > };
> > > > >
> > > > > In MFD driver:
> > > > >
> > > > > struct max77650_gpio_pdata *gpio_data = devm_kmalloc(dev, sizeof(*gpio_data));
> > > > >
> > > > > gpio_data->gpi_irq = regmap_irq_get_virq(irqchip_data, GPI_NUM);
> > > > >
> > > > > gpio_cell.platform_data = gpio_data;
> > > > >
> > > > > In GPIO driver:
> > > > >
> > > > > struct max77650_gpio_pdata *gpio_data = pdev->dev.platform_data;
> > > > >
> > > > > int irq = gpio_data->gpi_irq;
> > > >
> > > > Definitely not.  What you're trying to do is a hack.
> > > >
> > > > If you're using Regmap to handle your IRQs, then you should use Regmap
> > > > in the client to pull them out.  Setting them via Regmap, then pulling
> > > > them out again in the *same driver*, only to store them in platform
> > > > data to be passed to a child device is bonkers.
> > > >
> > > > *Either* use the MFD provided platform-data helpers *or* pass and
> > > > handle them via the Regmap APIs, *not* both.
> > >
> > > Right, a plan has been formed.
> > >
> > > Hopefully this works and you can avoid all this dancing around.
> > >
> > > Firstly, you need to make a small change to:
> > >
> > >   drivers/base/regmap/regmap-irq.c
> > >
> > > Add the following function:
> > >
> > >   struct irq_domain *regmap_irq_get_domain(struct regmap *map)
> >
> > We already do have such function and a lot of mfd drivers actually use it.
>
> Even better.
>
> > > As you can see, it will return the IRQ Domain for the chip.
> > >
> > > You can then pass this IRQ domain to mfd_add_devices() and it will do
> > > the HWIRQ => VIRQ mapping for you on the fly.  Meaning that you can
> > > remove all the nastiness in max77650_setup_irqs() and have the Input
> > > device use the standard (e.g. platform_get_irq()) APIs.
> > >
> > > How does that Sound?
> >
> > This does sound better! Why didn't you lead with that in the first place?
>
> I'm not even going to dignify that stupid question with a response.
>

It's not a stupid question and you're being unnecessarily rude. As an
expert in the subsystem you maintain you could have answered simply
with a "this is wrong, retrieve the irq domain from the regmap
irq_chip and pass it over to mfd_add_devices, the mfd core will create
appropriate mappings".

> > It's a pity it's not documented, I had to look at the code to find out
> > irq resources would get translated in mfd_add_devices() if a domain is
> > present.
>
> Where is it likely to be documented?  MFD is pretty simple and seldom
> needs explanation.  A 3 second look at the API you're trying to use
> (instead of blind copy & paste) would have told you that it's possible
> to take an IRQ domain as an argument.
>
> It's only the craziness in this patch which forced me to look into how
> Regmap handles IRQs and come up with a subsequent solution which fits
> your use-case.
>
> > In that case - I really don't see a reason to create a superfluous
> > structure to only hold the main regmap - we can very well get it from
> > the parent device in sub-drivers as I do now.
>
> The whole point of this solution is that you don't need to pass
> anything non-standard (i.e. not provided by the current APIs) to the
> child device.
>

I don't understand what you're saying here.

Bartosz

Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver

[Lee Jones]

On Wed, 13 Feb 2019, Bartosz Golaszewski wrote:

> śr., 13 lut 2019 o 10:53 Lee Jones <lee.jones@linaro.org> napisał(a):
> >
> > On Wed, 13 Feb 2019, Bartosz Golaszewski wrote:
> >
> > > śr., 13 lut 2019 o 10:25 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > >
> > > > On Tue, 12 Feb 2019, Lee Jones wrote:
> > > >
> > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > >
> > > > > > wt., 12 lut 2019 o 12:14 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > >
> > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > > > >
> > > > > > > > wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > > > >
> > > > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > > > > > >
> > > > > > > > > > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > > > > > >
> > > > > > > > > > >  * The declaration of a superfluous struct
> > > > > > > > > > >  * 100 lines of additional/avoidable code
> > > > > > > > > > >  * Hacky hoop jumping trying to fudge VIRQs into resources
> > > > > > > > > > >  * Resources were designed for HWIRQs (unless a domain is present)
> > > > > > > > > > >  * Loads of additional/avoidable CPU cycles setting all this up
> > > > > > > > > >
> > > > > > > > > > While the above may be right, this one is negligible and you know it. :)
> > > > > > > > >
> > > > > > > > > You have nested for() loops.  You *are* wasting lots of cycles.
> > > > > > > > >
> > > > > > > > > > > Need I go on? :)
> > > > > > > > > > >
> > > > > > > > > > > Surely the fact that you are using both sides of an API
> > > > > > > > > > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must
> > > > > > > > > > > set some alarm bells ringing?
> > > > > > > > > > >
> > > > > > > > > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess.
> > > > > > > > > > >
> > > > > > > > > > > And for what?  To avoid passing IRQ data to a child driver?
> > > > > > > > > >
> > > > > > > > > > What do you propose? Should I go back to the approach in v1 and pass
> > > > > > > > > > the regmap_irq_chip_data to child drivers?
> > > > > > > > >
> > > > > > > > > I'm saying you should remove all of this hackery and pass IRQs as they
> > > > > > > > > are supposed to be passed (like everyone else does).
> > > > > > > >
> > > > > > > > I'm not sure what you mean by "like everyone else does" - different
> > > > > > > > mfd drivers seem to be doing different things. Is a simple struct
> > > > > > > > containing virtual irq numbers passed to sub-drivers fine?
> > > > > > >
> > > > > > > How do you plan on deriving the VIRQs to place into the struct?
> > > > > >
> > > > > > Exampe:
> > > > > >
> > > > > > struct max77650_gpio_pdata {
> > > > > >     int gpi_irq;
> > > > > > };
> > > > > >
> > > > > > In MFD driver:
> > > > > >
> > > > > > struct max77650_gpio_pdata *gpio_data = devm_kmalloc(dev, sizeof(*gpio_data));
> > > > > >
> > > > > > gpio_data->gpi_irq = regmap_irq_get_virq(irqchip_data, GPI_NUM);
> > > > > >
> > > > > > gpio_cell.platform_data = gpio_data;
> > > > > >
> > > > > > In GPIO driver:
> > > > > >
> > > > > > struct max77650_gpio_pdata *gpio_data = pdev->dev.platform_data;
> > > > > >
> > > > > > int irq = gpio_data->gpi_irq;
> > > > >
> > > > > Definitely not.  What you're trying to do is a hack.
> > > > >
> > > > > If you're using Regmap to handle your IRQs, then you should use Regmap
> > > > > in the client to pull them out.  Setting them via Regmap, then pulling
> > > > > them out again in the *same driver*, only to store them in platform
> > > > > data to be passed to a child device is bonkers.
> > > > >
> > > > > *Either* use the MFD provided platform-data helpers *or* pass and
> > > > > handle them via the Regmap APIs, *not* both.
> > > >
> > > > Right, a plan has been formed.
> > > >
> > > > Hopefully this works and you can avoid all this dancing around.
> > > >
> > > > Firstly, you need to make a small change to:
> > > >
> > > >   drivers/base/regmap/regmap-irq.c
> > > >
> > > > Add the following function:
> > > >
> > > >   struct irq_domain *regmap_irq_get_domain(struct regmap *map)
> > >
> > > We already do have such function and a lot of mfd drivers actually use it.
> >
> > Even better.
> >
> > > > As you can see, it will return the IRQ Domain for the chip.
> > > >
> > > > You can then pass this IRQ domain to mfd_add_devices() and it will do
> > > > the HWIRQ => VIRQ mapping for you on the fly.  Meaning that you can
> > > > remove all the nastiness in max77650_setup_irqs() and have the Input
> > > > device use the standard (e.g. platform_get_irq()) APIs.
> > > >
> > > > How does that Sound?
> > >
> > > This does sound better! Why didn't you lead with that in the first place?
> >
> > I'm not even going to dignify that stupid question with a response.
> 
> It's not a stupid question and you're being unnecessarily rude. As an
> expert in the subsystem you maintain you could have answered simply
> with a "this is wrong, retrieve the irq domain from the regmap
> irq_chip and pass it over to mfd_add_devices, the mfd core will create
> appropriate mappings".

Could be culture clash, but I found the question offensive which is
why I chose not to answer it.  The reason is actually explained below:

 "It's only the craziness in this patch which forced me to look into how
  Regmap handles IRQs and come up with a subsequent solution which fits
  your use-case."

Thus the fact that a) Regmap used IRQ domains and b) the IRQ domain
could be fetched and reused here didn't enter my thought process until
I delved into the inner workings of Regmap.

Yes, I know MFD pretty well, but I only tend to deep-dive into other
subsystems, particularly ones as complicated as Regmap, when it's
necessary to do so.  Now I know a little more about it, I can provide
the feedback you suggest going forward.

> > > It's a pity it's not documented, I had to look at the code to find out
> > > irq resources would get translated in mfd_add_devices() if a domain is
> > > present.
> >
> > Where is it likely to be documented?  MFD is pretty simple and seldom
> > needs explanation.  A 3 second look at the API you're trying to use
> > (instead of blind copy & paste) would have told you that it's possible
> > to take an IRQ domain as an argument.
> >
> > It's only the craziness in this patch which forced me to look into how
> > Regmap handles IRQs and come up with a subsequent solution which fits
> > your use-case.
> >
> > > In that case - I really don't see a reason to create a superfluous
> > > structure to only hold the main regmap - we can very well get it from
> > > the parent device in sub-drivers as I do now.
> >
> > The whole point of this solution is that you don't need to pass
> > anything non-standard (i.e. not provided by the current APIs) to the
> > child device.
> 
> I don't understand what you're saying here.

I'm saying that the structure you speak of is no longer required.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver

[Bartosz Golaszewski]

śr., 13 lut 2019 o 11:39 Lee Jones <lee.jones@linaro.org> napisał(a):
>
> On Wed, 13 Feb 2019, Bartosz Golaszewski wrote:
>
> > śr., 13 lut 2019 o 10:53 Lee Jones <lee.jones@linaro.org> napisał(a):
> > >
> > > On Wed, 13 Feb 2019, Bartosz Golaszewski wrote:
> > >
> > > > śr., 13 lut 2019 o 10:25 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > >
> > > > > On Tue, 12 Feb 2019, Lee Jones wrote:
> > > > >
> > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > > >
> > > > > > > wt., 12 lut 2019 o 12:14 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > > >
> > > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > > > > >
> > > > > > > > > wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > > > > >
> > > > > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > > > > > > >
> > > > > > > > > > > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > > > > > > >
> > > > > > > > > > > >  * The declaration of a superfluous struct
> > > > > > > > > > > >  * 100 lines of additional/avoidable code
> > > > > > > > > > > >  * Hacky hoop jumping trying to fudge VIRQs into resources
> > > > > > > > > > > >  * Resources were designed for HWIRQs (unless a domain is present)
> > > > > > > > > > > >  * Loads of additional/avoidable CPU cycles setting all this up
> > > > > > > > > > >
> > > > > > > > > > > While the above may be right, this one is negligible and you know it. :)
> > > > > > > > > >
> > > > > > > > > > You have nested for() loops.  You *are* wasting lots of cycles.
> > > > > > > > > >
> > > > > > > > > > > > Need I go on? :)
> > > > > > > > > > > >
> > > > > > > > > > > > Surely the fact that you are using both sides of an API
> > > > > > > > > > > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must
> > > > > > > > > > > > set some alarm bells ringing?
> > > > > > > > > > > >
> > > > > > > > > > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess.
> > > > > > > > > > > >
> > > > > > > > > > > > And for what?  To avoid passing IRQ data to a child driver?
> > > > > > > > > > >
> > > > > > > > > > > What do you propose? Should I go back to the approach in v1 and pass
> > > > > > > > > > > the regmap_irq_chip_data to child drivers?
> > > > > > > > > >
> > > > > > > > > > I'm saying you should remove all of this hackery and pass IRQs as they
> > > > > > > > > > are supposed to be passed (like everyone else does).
> > > > > > > > >
> > > > > > > > > I'm not sure what you mean by "like everyone else does" - different
> > > > > > > > > mfd drivers seem to be doing different things. Is a simple struct
> > > > > > > > > containing virtual irq numbers passed to sub-drivers fine?
> > > > > > > >
> > > > > > > > How do you plan on deriving the VIRQs to place into the struct?
> > > > > > >
> > > > > > > Exampe:
> > > > > > >
> > > > > > > struct max77650_gpio_pdata {
> > > > > > >     int gpi_irq;
> > > > > > > };
> > > > > > >
> > > > > > > In MFD driver:
> > > > > > >
> > > > > > > struct max77650_gpio_pdata *gpio_data = devm_kmalloc(dev, sizeof(*gpio_data));
> > > > > > >
> > > > > > > gpio_data->gpi_irq = regmap_irq_get_virq(irqchip_data, GPI_NUM);
> > > > > > >
> > > > > > > gpio_cell.platform_data = gpio_data;
> > > > > > >
> > > > > > > In GPIO driver:
> > > > > > >
> > > > > > > struct max77650_gpio_pdata *gpio_data = pdev->dev.platform_data;
> > > > > > >
> > > > > > > int irq = gpio_data->gpi_irq;
> > > > > >
> > > > > > Definitely not.  What you're trying to do is a hack.
> > > > > >
> > > > > > If you're using Regmap to handle your IRQs, then you should use Regmap
> > > > > > in the client to pull them out.  Setting them via Regmap, then pulling
> > > > > > them out again in the *same driver*, only to store them in platform
> > > > > > data to be passed to a child device is bonkers.
> > > > > >
> > > > > > *Either* use the MFD provided platform-data helpers *or* pass and
> > > > > > handle them via the Regmap APIs, *not* both.
> > > > >
> > > > > Right, a plan has been formed.
> > > > >
> > > > > Hopefully this works and you can avoid all this dancing around.
> > > > >
> > > > > Firstly, you need to make a small change to:
> > > > >
> > > > >   drivers/base/regmap/regmap-irq.c
> > > > >
> > > > > Add the following function:
> > > > >
> > > > >   struct irq_domain *regmap_irq_get_domain(struct regmap *map)
> > > >
> > > > We already do have such function and a lot of mfd drivers actually use it.
> > >
> > > Even better.
> > >
> > > > > As you can see, it will return the IRQ Domain for the chip.
> > > > >
> > > > > You can then pass this IRQ domain to mfd_add_devices() and it will do
> > > > > the HWIRQ => VIRQ mapping for you on the fly.  Meaning that you can
> > > > > remove all the nastiness in max77650_setup_irqs() and have the Input
> > > > > device use the standard (e.g. platform_get_irq()) APIs.
> > > > >
> > > > > How does that Sound?
> > > >
> > > > This does sound better! Why didn't you lead with that in the first place?
> > >
> > > I'm not even going to dignify that stupid question with a response.
> >
> > It's not a stupid question and you're being unnecessarily rude. As an
> > expert in the subsystem you maintain you could have answered simply
> > with a "this is wrong, retrieve the irq domain from the regmap
> > irq_chip and pass it over to mfd_add_devices, the mfd core will create
> > appropriate mappings".
>
> Could be culture clash, but I found the question offensive which is
> why I chose not to answer it.  The reason is actually explained below:
>

It wasn't meant to be offensive. I guess when dealing with non-native
English speakers over e-mail it's best to assume good faith.

>  "It's only the craziness in this patch which forced me to look into how
>   Regmap handles IRQs and come up with a subsequent solution which fits
>   your use-case."
>
> Thus the fact that a) Regmap used IRQ domains and b) the IRQ domain
> could be fetched and reused here didn't enter my thought process until
> I delved into the inner workings of Regmap.
>
> Yes, I know MFD pretty well, but I only tend to deep-dive into other
> subsystems, particularly ones as complicated as Regmap, when it's
> necessary to do so.  Now I know a little more about it, I can provide
> the feedback you suggest going forward.
>
> > > > It's a pity it's not documented, I had to look at the code to find out
> > > > irq resources would get translated in mfd_add_devices() if a domain is
> > > > present.
> > >
> > > Where is it likely to be documented?  MFD is pretty simple and seldom
> > > needs explanation.  A 3 second look at the API you're trying to use
> > > (instead of blind copy & paste) would have told you that it's possible
> > > to take an IRQ domain as an argument.
> > >
> > > It's only the craziness in this patch which forced me to look into how
> > > Regmap handles IRQs and come up with a subsequent solution which fits
> > > your use-case.
> > >
> > > > In that case - I really don't see a reason to create a superfluous
> > > > structure to only hold the main regmap - we can very well get it from
> > > > the parent device in sub-drivers as I do now.
> > >
> > > The whole point of this solution is that you don't need to pass
> > > anything non-standard (i.e. not provided by the current APIs) to the
> > > child device.
> >
> > I don't understand what you're saying here.
>
> I'm saying that the structure you speak of is no longer required.
>

Thanks for clarifying.

Bartosz

Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 948 bytes --]

Hi!

> > As you can see, it will return the IRQ Domain for the chip.
> >
> > You can then pass this IRQ domain to mfd_add_devices() and it will do
> > the HWIRQ => VIRQ mapping for you on the fly.  Meaning that you can
> > remove all the nastiness in max77650_setup_irqs() and have the Input
> > device use the standard (e.g. platform_get_irq()) APIs.
> >
> > How does that Sound?
> 
> This does sound better! Why didn't you lead with that in the first
>place?

TBH, this can be considered rude and I'm not surprised Lee reacted the
way he did.

> It's a pity it's not documented, I had to look at the code to find out
> irq resources would get translated in mfd_add_devices() if a domain is
> present.

I guess documentation patch would be welcome.

Best regards,
									Pavel


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]