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

[PATCH 00/13] 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-6 add the DT binding documents. Patches 7-12 add all drivers.
Last patch adds a MAINTAINERS entry for this device.

Note that patch 8/13 depends on commit 03c87b95ac04 ("regulator: provide
rdev_get_regmap()") which was picked up by Mark Brown for v5.1.

Bartosz Golaszewski (13):
  dt-bindings: mfd: add DT bindings for max77650
  dt-bindings: regulator: add DT bindings for max77650
  dt-bindings: power: supply: add DT bindings for max77650
  dt-bindings: gpio: 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
  regulator: max77650: add regulator support
  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/gpio/gpio-max77650.txt           |  34 ++
 .../bindings/input/max77650-onkey.txt         |  26 +
 .../bindings/leds/leds-max77650.txt           |  57 ++
 .../devicetree/bindings/mfd/max77650.txt      |  28 +
 .../power/supply/max77650-charger.txt         |  27 +
 .../bindings/regulator/max77650-regulator.txt |  41 ++
 MAINTAINERS                                   |  14 +
 drivers/gpio/Kconfig                          |   7 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-max77650.c                  | 189 ++++++
 drivers/input/misc/Kconfig                    |   9 +
 drivers/input/misc/Makefile                   |   1 +
 drivers/input/misc/max77650-onkey.c           | 135 +++++
 drivers/leds/Kconfig                          |   6 +
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-max77650.c                  | 162 ++++++
 drivers/mfd/Kconfig                           |  11 +
 drivers/mfd/Makefile                          |   1 +
 drivers/mfd/max77650.c                        | 212 +++++++
 drivers/power/supply/Kconfig                  |   7 +
 drivers/power/supply/Makefile                 |   1 +
 drivers/power/supply/max77650-charger.c       | 347 +++++++++++
 drivers/regulator/Kconfig                     |   8 +
 drivers/regulator/Makefile                    |   1 +
 drivers/regulator/max77650-regulator.c        | 537 ++++++++++++++++++
 include/linux/mfd/max77650.h                  |  73 +++
 26 files changed, 1936 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-max77650.txt
 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 Documentation/devicetree/bindings/regulator/max77650-regulator.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 drivers/regulator/max77650-regulator.c
 create mode 100644 include/linux/mfd/max77650.h

-- 
2.19.1

[PATCH 01/13] 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.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 .../devicetree/bindings/mfd/max77650.txt      | 28 +++++++++++++++++++
 1 file changed, 28 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..84631d3b1e14
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/max77650.txt
@@ -0,0 +1,28 @@
+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>.
+
+For device-tree bindings of sub-modules (regulator, power supply, GPIO, LEDs
+and onkey) refer to the binding documents under the respective sub-system
+directories.
+
+Example:
+--------
+
+	pmic: max77650@48 {
+		compatible = "maxim,max77650";
+		reg = <0x48>;
+
+		interrupt-controller;
+		interrupt-parent = <&gpio2>;
+		#interrupt-cells = <2>;
+		interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
+	};
-- 
2.19.1

[PATCH 02/13] dt-bindings: regulator: add DT bindings for max77650

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Add the DT binding document for max77650 regulators.

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

diff --git a/Documentation/devicetree/bindings/regulator/max77650-regulator.txt b/Documentation/devicetree/bindings/regulator/max77650-regulator.txt
new file mode 100644
index 000000000000..f1cbe813c30f
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/max77650-regulator.txt
@@ -0,0 +1,41 @@
+Regulator 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 regulator controller is represented as a sub-node of the PMIC node
+on the device tree.
+
+The device has a single LDO regulator and a SIMO buck-boost regulator with
+three independent power rails.
+
+Required properties:
+--------------------
+- compatible:		Must be "maxim,max77650-regulator"
+
+Each rail must be instantiated under the regulators subnode of the top PMIC
+node. Up to four regulators can be defined. For standard regulator properties
+refer to Documentation/devicetree/bindings/regulator/regulator.txt.
+
+Available regulator compatible strings are: "ldo", "sbb0", "sbb1", "sbb2".
+
+Example:
+--------
+
+	regulators {
+		compatible = "maxim,max77650-regulator";
+
+		max77650_ldo: regulator@0 {
+			regulator-compatible = "ldo";
+			regulator-name = "max77650-ldo";
+			regulator-min-microvolt = <1350000>;
+			regulator-max-microvolt = <2937500>;
+		};
+
+		max77650_sbb0: regulator@1 {
+			regulator-compatible = "sbb0";
+			regulator-name = "max77650-sbb0";
+			regulator-min-microvolt = <800000>;
+			regulator-max-microvolt = <1587500>;
+		};
+	};
-- 
2.19.1

[PATCH 03/13] 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.19.1

[PATCH 04/13] dt-bindings: gpio: add DT bindings for max77650

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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

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

diff --git a/Documentation/devicetree/bindings/gpio/gpio-max77650.txt b/Documentation/devicetree/bindings/gpio/gpio-max77650.txt
new file mode 100644
index 000000000000..b5dbbe934deb
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-max77650.txt
@@ -0,0 +1,34 @@
+GPIO 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 GPIO controller is represented as a sub-node of the PMIC node on
+the device tree.
+
+This device has a single GPIO pin.
+
+Required properties:
+--------------------
+- compatible:		Must be "maxim,max77650-gpio"
+- 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.
+
+For more details, please refer to the generic GPIO DT binding document
+<devicetree/bindings/gpio/gpio.txt>.
+
+Example:
+--------
+
+	gpio {
+		compatible = "maxim,max77650-gpio";
+		gpio-line-names = "max77650-charger";
+		gpio-controller;
+		#gpio-cells = <2>;
+	};
-- 
2.19.1

Re: [PATCH 04/13] dt-bindings: gpio: add DT bindings for max77650

[Linus Walleij]

On Fri, Jan 18, 2019 at 2:43 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Add the DT binding document for the GPIO module of max77650.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Very simple so not much to complain about :)
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 04/13] dt-bindings: gpio: add DT bindings for max77650

[Linus Walleij]

On Fri, Jan 18, 2019 at 2:43 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Add the DT binding document for the GPIO module of max77650.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Very simple so not much to complain about :)
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 05/13] 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..822b8893bc20
--- /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-leds"
+- #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-leds";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		led0 {
+			reg = <0>;
+			label = "max77650:blue:usr0";
+		};
+
+		led1 {
+			reg = <1>;
+			label = "max77650:red:usr1";
+			linux,default-trigger = "heartbeat";
+		};
+
+		led2 {
+			reg = <2>;
+			label = "max77650:green:usr2";
+		};
+	};
-- 
2.19.1

Re: [PATCH 05/13] dt-bindings: leds: add DT bindings for max77650

[Jacek Anaszewski]

Hi Bartosz,

Thank you for the patch.

On 1/18/19 2:42 PM, Bartosz Golaszewski wrote:
> 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..822b8893bc20
> --- /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-leds"
> +- #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-leds";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		led0 {

s/led0/led@0/

> +			reg = <0>;
> +			label = "max77650:blue:usr0";
> +		};
> +
> +		led1 {

s/led1/led@1/

> +			reg = <1>;
> +			label = "max77650:red:usr1";
> +			linux,default-trigger = "heartbeat";
> +		};
> +
> +		led2 {

s/led2/led@2/

> +			reg = <2>;
> +			label = "max77650:green:usr2";
> +		};

Please remove "max77650:" from labels and add it in the driver.

> +	};
> 

-- 
Best regards,
Jacek Anaszewski

[PATCH 06/13] 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.19.1

[PATCH 07/13] 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       | 212 +++++++++++++++++++++++++++++++++++
 include/linux/mfd/max77650.h |  73 ++++++++++++
 4 files changed, 297 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 8c5dfdce4326..dd4dcd97fe21 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -733,6 +733,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..9c769570b491
--- /dev/null
+++ b/drivers/mfd/max77650.c
@@ -0,0 +1,212 @@
+// 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
+
+static const struct mfd_cell max77650_devs[] = {
+	{
+		.name		= "max77650-regulator",
+		.of_compatible	= "maxim,max77650-regulator",
+	},
+	{
+		.name		= "max77650-charger",
+		.of_compatible	= "maxim,max77650-charger",
+	},
+	{
+		.name		= "max77650-gpio",
+		.of_compatible	= "maxim,max77650-gpio",
+	},
+	{
+		.name		= "max77650-leds",
+		.of_compatible	= "maxim,max77650-leds",
+	},
+	{
+		.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 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_i2c_probe(struct i2c_client *i2c)
+{
+	struct regmap_irq_chip_data *irq_data;
+	struct device *dev = &i2c->dev;
+	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;
+
+	max77650_irq_chip.irq_drv_data = map;
+	rv = devm_regmap_add_irq_chip(dev, map, i2c->irq,
+				      IRQF_ONESHOT | IRQF_SHARED,
+				      -1, &max77650_irq_chip, &irq_data);
+	if (rv)
+		return rv;
+
+	i2c_set_clientdata(i2c, irq_data);
+
+	return devm_mfd_add_devices(dev, -1, max77650_devs,
+				    ARRAY_SIZE(max77650_devs), NULL, 0, NULL);
+}
+
+static const struct of_device_id max77650_of_match[] = {
+	{ .compatible = "maxim,max77650", },
+};
+
+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");
diff --git a/include/linux/mfd/max77650.h b/include/linux/mfd/max77650.h
new file mode 100644
index 000000000000..841bbccdd5ad
--- /dev/null
+++ b/include/linux/mfd/max77650.h
@@ -0,0 +1,73 @@
+/* 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
+
+#define MAX77650_INT_GPI		0
+#define MAX77650_INT_nEN_F		1
+#define MAX77650_INT_nEN_R		2
+#define MAX77650_INT_TJAL1_R		3
+#define MAX77650_INT_TJAL2_R		4
+#define MAX77650_INT_DOD_R		5
+#define MAX77650_INT_THM		6
+#define MAX77650_INT_CHG		7
+#define MAX77650_INT_CHGIN		8
+#define MAX77650_INT_TJ_REG		9
+#define MAX77650_INT_CHGIN_CTRL		10
+#define MAX77650_INT_SYS_CTRL		11
+#define MAX77650_INT_SYS_CNFG		12
+
+#endif /* MAX77650_H */
-- 
2.19.1

[PATCH 08/13] regulator: max77650: add regulator support

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Add regulator support for max77650. We support all four variants of this
PMIC including non-linear voltage table for max77651 SBB1 rail.

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

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index ee60a222f5eb..514f094f9444 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -457,6 +457,14 @@ config REGULATOR_MAX77620
 	  chip to control Step-Down DC-DC and LDOs. Say Y here to
 	  enable the regulator driver.
 
+config REGULATOR_MAX77650
+	tristate "Maxim MAX77650/77651 regulator support"
+	depends on MFD_MAX77650
+	help
+	  Regulator driver for MAX77650/77651 PMIC from Maxim
+	  Semiconductor. This device has a SIMO with three independent
+	  power rails and an LDO.
+
 config REGULATOR_MAX8649
 	tristate "Maxim 8649 voltage regulator"
 	depends on I2C
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index b12e1c9b2118..7de79a12b0b7 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -60,6 +60,7 @@ obj-$(CONFIG_REGULATOR_LTC3676) += ltc3676.o
 obj-$(CONFIG_REGULATOR_MAX14577) += max14577-regulator.o
 obj-$(CONFIG_REGULATOR_MAX1586) += max1586.o
 obj-$(CONFIG_REGULATOR_MAX77620) += max77620-regulator.o
+obj-$(CONFIG_REGULATOR_MAX77650) += max77650-regulator.o
 obj-$(CONFIG_REGULATOR_MAX8649)	+= max8649.o
 obj-$(CONFIG_REGULATOR_MAX8660) += max8660.o
 obj-$(CONFIG_REGULATOR_MAX8907) += max8907-regulator.o
diff --git a/drivers/regulator/max77650-regulator.c b/drivers/regulator/max77650-regulator.c
new file mode 100644
index 000000000000..036022566907
--- /dev/null
+++ b/drivers/regulator/max77650-regulator.c
@@ -0,0 +1,537 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 BayLibre SAS
+ * Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
+ *
+ * Regulator driver for MAXIM 77650/77651 charger/power-supply.
+ */
+
+#include <linux/i2c.h>
+#include <linux/mfd/max77650.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
+
+#define MAX77650_REGULATOR_EN_CTRL_MASK		GENMASK(3, 0)
+#define MAX77650_REGULATOR_EN_CTRL_BITS(_reg) \
+		((_reg) & MAX77650_REGULATOR_EN_CTRL_MASK)
+#define MAX77650_REGULATOR_ENABLED		GENMASK(2, 1)
+#define MAX77650_REGULATOR_DISABLED		BIT(2)
+
+#define MAX77650_REGULATOR_V_LDO_MASK		GENMASK(6, 0)
+#define MAX77650_REGULATOR_V_SBB_MASK		GENMASK(5, 0)
+
+#define MAX77650_REGULATOR_AD_MASK		BIT(3)
+#define MAX77650_REGULATOR_AD_DISABLED		0x00
+#define MAX77650_REGULATOR_AD_ENABLED		BIT(3)
+
+#define MAX77650_REGULATOR_CURR_LIM_MASK	GENMASK(7, 6)
+#define MAX77650_REGULATOR_CURR_LIM_BITS(_reg) \
+		(((_reg) & MAX77650_REGULATOR_CURR_LIM_MASK) >> 6)
+#define MAX77650_REGULATOR_CURR_LIM_SHIFT(_val)	((_val) << 6)
+
+enum {
+	MAX77650_REGULATOR_ID_LDO = 0,
+	MAX77650_REGULATOR_ID_SBB0,
+	MAX77650_REGULATOR_ID_SBB1,
+	MAX77650_REGULATOR_ID_SBB2,
+	MAX77650_REGULATOR_NUM_REGULATORS,
+};
+
+struct max77650_regulator_desc {
+	struct regulator_desc desc;
+	unsigned int regA;
+	unsigned int regB;
+};
+
+static const u32 max77651_sbb1_regulator_volt_table[] = {
+	2400000, 3200000, 4000000, 4800000,
+	2450000, 3250000, 4050000, 4850000,
+	2500000, 3300000, 4100000, 4900000,
+	2550000, 3350000, 4150000, 4950000,
+	2600000, 3400000, 4200000, 5000000,
+	2650000, 3450000, 4250000, 5050000,
+	2700000, 3500000, 4300000, 5100000,
+	2750000, 3550000, 4350000, 5150000,
+	2800000, 3600000, 4400000, 5200000,
+	2850000, 3650000, 4450000, 5250000,
+	2900000, 3700000, 4500000,       0,
+	2950000, 3750000, 4550000,       0,
+	3000000, 3800000, 4600000,       0,
+	3050000, 3850000, 4650000,       0,
+	3100000, 3900000, 4700000,       0,
+	3150000, 3950000, 4750000,       0,
+};
+
+#define MAX77651_REGULATOR_SBB1_SEL_DEC(_val) \
+		(((_val & 0x3c) >> 2) | ((_val & 0x03) << 4))
+#define MAX77651_REGULATOR_SBB1_SEL_ENC(_val) \
+		(((_val & 0x30) >> 4) | ((_val & 0x0f) << 2))
+
+#define MAX77650_REGULATOR_SBB1_SEL_DECR(_val)				\
+	do {								\
+		_val = MAX77651_REGULATOR_SBB1_SEL_DEC(_val);		\
+		_val--;							\
+		_val = MAX77651_REGULATOR_SBB1_SEL_ENC(_val);		\
+	} while (0)
+
+#define MAX77650_REGULATOR_SBB1_SEL_INCR(_val)				\
+	do {								\
+		_val = MAX77651_REGULATOR_SBB1_SEL_DEC(_val);		\
+		_val++;							\
+		_val = MAX77651_REGULATOR_SBB1_SEL_ENC(_val);		\
+	} while (0)
+
+static const int max77650_current_limit_table[] = {
+	1000000, 866000, 707000, 500000,
+};
+
+static int max77650_regulator_is_enabled(struct regulator_dev *rdev)
+{
+	struct max77650_regulator_desc *rdesc;
+	struct regmap *map;
+	int val, rv, en;
+
+	rdesc = rdev_get_drvdata(rdev);
+	map = rdev_get_regmap(rdev);
+
+	rv = regmap_read(map, rdesc->regB, &val);
+	if (rv)
+		return rv;
+
+	en = MAX77650_REGULATOR_EN_CTRL_BITS(val);
+
+	return en != MAX77650_REGULATOR_DISABLED;
+}
+
+static int max77650_regulator_enable(struct regulator_dev *rdev)
+{
+	struct max77650_regulator_desc *rdesc;
+	struct regmap *map;
+
+	rdesc = rdev_get_drvdata(rdev);
+	map = rdev_get_regmap(rdev);
+
+	return regmap_update_bits(map, rdesc->regB,
+				  MAX77650_REGULATOR_EN_CTRL_MASK,
+				  MAX77650_REGULATOR_ENABLED);
+}
+
+static int max77650_regulator_disable(struct regulator_dev *rdev)
+{
+	struct max77650_regulator_desc *rdesc;
+	struct regmap *map;
+
+	rdesc = rdev_get_drvdata(rdev);
+	map = rdev_get_regmap(rdev);
+
+	return regmap_update_bits(map, rdesc->regB,
+				  MAX77650_REGULATOR_EN_CTRL_MASK,
+				  MAX77650_REGULATOR_DISABLED);
+}
+
+static int max77650_regulator_set_voltage_sel(struct regulator_dev *rdev,
+					      unsigned int sel)
+{
+	int rv = 0, curr, diff;
+	bool ascending;
+
+	/*
+	 * If the regulator is disabled, we can program the desired
+	 * voltage right away.
+	 */
+	if (!max77650_regulator_is_enabled(rdev))
+		return regulator_set_voltage_sel_regmap(rdev, sel);
+
+	/*
+	 * Otherwise we need to manually ramp the output voltage up/down
+	 * one step at a time.
+	 */
+
+	curr = regulator_get_voltage_sel_regmap(rdev);
+	if (curr < 0)
+		return curr;
+
+	diff = curr - sel;
+	if (diff == 0)
+		return 0; /* Already there. */
+	else if (diff > 0)
+		ascending = false;
+	else
+		ascending = true;
+
+	/*
+	 * Make sure we'll get to the right voltage and break the loop even if
+	 * the selector equals 0.
+	 */
+	for (ascending ? curr++ : curr--;; ascending ? curr++ : curr--) {
+		rv = regulator_set_voltage_sel_regmap(rdev, curr);
+		if (rv)
+			return rv;
+
+		if (curr == sel)
+			break;
+	}
+
+	return 0;
+}
+
+/*
+ * Special case: non-linear voltage table for max77651 SBB1 - software
+ * must ensure the voltage is ramped in 50mV increments.
+ */
+static int max77651_regulator_sbb1_set_voltage_sel(struct regulator_dev *rdev,
+						   unsigned int sel)
+{
+	int rv = 0, curr, vcurr, vdest, vdiff;
+
+	/*
+	 * If the regulator is disabled, we can program the desired
+	 * voltage right away.
+	 */
+	if (!max77650_regulator_is_enabled(rdev))
+		return regulator_set_voltage_sel_regmap(rdev, sel);
+
+	curr = regulator_get_voltage_sel_regmap(rdev);
+	if (curr < 0)
+		return curr;
+
+	if (curr == sel)
+		return 0; /* Already there. */
+
+	vcurr = max77651_sbb1_regulator_volt_table[curr];
+	vdest = max77651_sbb1_regulator_volt_table[sel];
+	vdiff = vcurr - vdest;
+
+	for (;;) {
+		if (vdiff > 0)
+			MAX77650_REGULATOR_SBB1_SEL_DECR(curr);
+		else
+			MAX77650_REGULATOR_SBB1_SEL_INCR(curr);
+
+		rv = regulator_set_voltage_sel_regmap(rdev, curr);
+		if (rv)
+			return rv;
+
+		if (curr == sel)
+			break;
+	};
+
+	return 0;
+}
+
+static int max77650_regulator_get_current_limit(struct regulator_dev *rdev)
+{
+	struct max77650_regulator_desc *rdesc;
+	struct regmap *map;
+	int val, rv, limit;
+
+	rdesc = rdev_get_drvdata(rdev);
+	map = rdev_get_regmap(rdev);
+
+	rv = regmap_read(map, rdesc->regA, &val);
+	if (rv)
+		return rv;
+
+	limit = MAX77650_REGULATOR_CURR_LIM_BITS(val);
+
+	return max77650_current_limit_table[limit];
+}
+
+static int max77650_regulator_set_current_limit(struct regulator_dev *rdev,
+						int min_uA, int max_uA)
+{
+	struct max77650_regulator_desc *rdesc;
+	struct regmap *map;
+	int rv, i, limit;
+
+	rdesc = rdev_get_drvdata(rdev);
+	map = rdev_get_regmap(rdev);
+
+	for (i = 0; i < ARRAY_SIZE(max77650_current_limit_table); i++) {
+		limit = max77650_current_limit_table[i];
+
+		if (limit >= min_uA && limit <= max_uA) {
+			rv = regmap_update_bits(map, rdesc->regA,
+					MAX77650_REGULATOR_CURR_LIM_MASK,
+					MAX77650_REGULATOR_CURR_LIM_SHIFT(i));
+			if (rv)
+				return rv;
+		}
+	}
+
+	return -EINVAL;
+}
+
+static const struct regulator_ops max77650_regulator_LDO_ops = {
+	.is_enabled		= max77650_regulator_is_enabled,
+	.enable			= max77650_regulator_enable,
+	.disable		= max77650_regulator_disable,
+	.list_voltage		= regulator_list_voltage_linear,
+	.map_voltage		= regulator_map_voltage_linear,
+	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
+	.set_voltage_sel	= max77650_regulator_set_voltage_sel,
+	.set_active_discharge	= regulator_set_active_discharge_regmap,
+};
+
+static const struct regulator_ops max77650_regulator_SBB_ops = {
+	.is_enabled		= max77650_regulator_is_enabled,
+	.enable			= max77650_regulator_enable,
+	.disable		= max77650_regulator_disable,
+	.list_voltage		= regulator_list_voltage_linear,
+	.map_voltage		= regulator_map_voltage_linear,
+	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
+	.set_voltage_sel	= max77650_regulator_set_voltage_sel,
+	.get_current_limit	= max77650_regulator_get_current_limit,
+	.set_current_limit	= max77650_regulator_set_current_limit,
+	.set_active_discharge	= regulator_set_active_discharge_regmap,
+};
+
+/* Special case for max77651 SBB1 - non-linear voltage mapping. */
+static const struct regulator_ops max77651_SBB1_regulator_ops = {
+	.is_enabled		= max77650_regulator_is_enabled,
+	.enable			= max77650_regulator_enable,
+	.disable		= max77650_regulator_disable,
+	.list_voltage		= regulator_list_voltage_table,
+	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
+	.set_voltage_sel	= max77651_regulator_sbb1_set_voltage_sel,
+	.get_current_limit	= max77650_regulator_get_current_limit,
+	.set_current_limit	= max77650_regulator_set_current_limit,
+	.set_active_discharge	= regulator_set_active_discharge_regmap,
+};
+
+static struct max77650_regulator_desc max77650_LDO_desc = {
+	.desc = {
+		.name			= "ldo",
+		.of_match		= of_match_ptr("ldo"),
+		.regulators_node	= of_match_ptr("regulators"),
+		.supply_name		= "in-ldo",
+		.id			= MAX77650_REGULATOR_ID_LDO,
+		.ops			= &max77650_regulator_LDO_ops,
+		.min_uV			= 1350000,
+		.uV_step		= 12500,
+		.n_voltages		= 128,
+		.vsel_mask		= MAX77650_REGULATOR_V_LDO_MASK,
+		.vsel_reg		= MAX77650_REG_CNFG_LDO_A,
+		.active_discharge_off	= MAX77650_REGULATOR_AD_DISABLED,
+		.active_discharge_on	= MAX77650_REGULATOR_AD_ENABLED,
+		.active_discharge_mask	= MAX77650_REGULATOR_AD_MASK,
+		.active_discharge_reg	= MAX77650_REG_CNFG_LDO_B,
+		.enable_time		= 100,
+		.type			= REGULATOR_VOLTAGE,
+	},
+	.regA		= MAX77650_REG_CNFG_LDO_A,
+	.regB		= MAX77650_REG_CNFG_LDO_B,
+};
+
+static struct max77650_regulator_desc max77650_SBB0_desc = {
+	.desc = {
+		.name			= "sbb0",
+		.of_match		= of_match_ptr("sbb0"),
+		.regulators_node	= of_match_ptr("regulators"),
+		.supply_name		= "in-sbb0",
+		.id			= MAX77650_REGULATOR_ID_SBB0,
+		.ops			= &max77650_regulator_SBB_ops,
+		.min_uV			= 800000,
+		.uV_step		= 25000,
+		.n_voltages		= 64,
+		.vsel_mask		= MAX77650_REGULATOR_V_SBB_MASK,
+		.vsel_reg		= MAX77650_REG_CNFG_SBB0_A,
+		.active_discharge_off	= MAX77650_REGULATOR_AD_DISABLED,
+		.active_discharge_on	= MAX77650_REGULATOR_AD_ENABLED,
+		.active_discharge_mask	= MAX77650_REGULATOR_AD_MASK,
+		.active_discharge_reg	= MAX77650_REG_CNFG_SBB0_B,
+		.enable_time		= 100,
+		.type			= REGULATOR_VOLTAGE,
+	},
+	.regA		= MAX77650_REG_CNFG_SBB0_A,
+	.regB		= MAX77650_REG_CNFG_SBB0_B,
+};
+
+static struct max77650_regulator_desc max77650_SBB1_desc = {
+	.desc = {
+		.name			= "sbb1",
+		.of_match		= of_match_ptr("sbb1"),
+		.regulators_node	= of_match_ptr("regulators"),
+		.supply_name		= "in-sbb1",
+		.id			= MAX77650_REGULATOR_ID_SBB1,
+		.ops			= &max77650_regulator_SBB_ops,
+		.min_uV			= 800000,
+		.uV_step		= 12500,
+		.n_voltages		= 64,
+		.vsel_mask		= MAX77650_REGULATOR_V_SBB_MASK,
+		.vsel_reg		= MAX77650_REG_CNFG_SBB1_A,
+		.active_discharge_off	= MAX77650_REGULATOR_AD_DISABLED,
+		.active_discharge_on	= MAX77650_REGULATOR_AD_ENABLED,
+		.active_discharge_mask	= MAX77650_REGULATOR_AD_MASK,
+		.active_discharge_reg	= MAX77650_REG_CNFG_SBB1_B,
+		.enable_time		= 100,
+		.type			= REGULATOR_VOLTAGE,
+	},
+	.regA		= MAX77650_REG_CNFG_SBB1_A,
+	.regB		= MAX77650_REG_CNFG_SBB1_B,
+};
+
+static struct max77650_regulator_desc max77651_SBB1_desc = {
+	.desc = {
+		.name			= "sbb1",
+		.of_match		= of_match_ptr("sbb1"),
+		.regulators_node	= of_match_ptr("regulators"),
+		.supply_name		= "in-sbb1",
+		.id			= MAX77650_REGULATOR_ID_SBB1,
+		.ops			= &max77651_SBB1_regulator_ops,
+		.volt_table		= max77651_sbb1_regulator_volt_table,
+		.n_voltages = ARRAY_SIZE(max77651_sbb1_regulator_volt_table),
+		.vsel_mask		= MAX77650_REGULATOR_V_SBB_MASK,
+		.vsel_reg		= MAX77650_REG_CNFG_SBB1_A,
+		.active_discharge_off	= MAX77650_REGULATOR_AD_DISABLED,
+		.active_discharge_on	= MAX77650_REGULATOR_AD_ENABLED,
+		.active_discharge_mask	= MAX77650_REGULATOR_AD_MASK,
+		.active_discharge_reg	= MAX77650_REG_CNFG_SBB1_B,
+		.enable_time		= 100,
+		.type			= REGULATOR_VOLTAGE,
+	},
+	.regA		= MAX77650_REG_CNFG_SBB1_A,
+	.regB		= MAX77650_REG_CNFG_SBB1_B,
+};
+
+static struct max77650_regulator_desc max77650_SBB2_desc = {
+	.desc = {
+		.name			= "sbb2",
+		.of_match		= of_match_ptr("sbb2"),
+		.regulators_node	= of_match_ptr("regulators"),
+		.supply_name		= "in-sbb0",
+		.id			= MAX77650_REGULATOR_ID_SBB2,
+		.ops			= &max77650_regulator_SBB_ops,
+		.min_uV			= 800000,
+		.uV_step		= 50000,
+		.n_voltages		= 64,
+		.vsel_mask		= MAX77650_REGULATOR_V_SBB_MASK,
+		.vsel_reg		= MAX77650_REG_CNFG_SBB2_A,
+		.active_discharge_off	= MAX77650_REGULATOR_AD_DISABLED,
+		.active_discharge_on	= MAX77650_REGULATOR_AD_ENABLED,
+		.active_discharge_mask	= MAX77650_REGULATOR_AD_MASK,
+		.active_discharge_reg	= MAX77650_REG_CNFG_SBB2_B,
+		.enable_time		= 100,
+		.type			= REGULATOR_VOLTAGE,
+	},
+	.regA		= MAX77650_REG_CNFG_SBB2_A,
+	.regB		= MAX77650_REG_CNFG_SBB2_B,
+};
+
+static struct max77650_regulator_desc max77651_SBB2_desc = {
+	.desc = {
+		.name			= "sbb2",
+		.of_match		= of_match_ptr("sbb2"),
+		.regulators_node	= of_match_ptr("regulators"),
+		.supply_name		= "in-sbb0",
+		.id			= MAX77650_REGULATOR_ID_SBB2,
+		.ops			= &max77650_regulator_SBB_ops,
+		.min_uV			= 2400000,
+		.uV_step		= 50000,
+		.n_voltages		= 64,
+		.vsel_mask		= MAX77650_REGULATOR_V_SBB_MASK,
+		.vsel_reg		= MAX77650_REG_CNFG_SBB2_A,
+		.active_discharge_off	= MAX77650_REGULATOR_AD_DISABLED,
+		.active_discharge_on	= MAX77650_REGULATOR_AD_ENABLED,
+		.active_discharge_mask	= MAX77650_REGULATOR_AD_MASK,
+		.active_discharge_reg	= MAX77650_REG_CNFG_SBB2_B,
+		.enable_time		= 100,
+		.type			= REGULATOR_VOLTAGE,
+	},
+	.regA		= MAX77650_REG_CNFG_SBB2_A,
+	.regB		= MAX77650_REG_CNFG_SBB2_B,
+};
+
+static int max77650_regulator_probe(struct platform_device *pdev)
+{
+	struct max77650_regulator_desc **rdescs;
+	struct max77650_regulator_desc *rdesc;
+	struct regulator_init_data *init_data;
+	struct regulator_config config = { };
+	struct device *dev, *parent;
+	struct regulator_dev *rdev;
+	struct device_node *child;
+	struct regmap *map;
+	unsigned int val;
+	int i, rv;
+
+	dev = &pdev->dev;
+	parent = dev->parent;
+
+	if (!dev->of_node)
+		dev->of_node = parent->of_node;
+
+	rdescs = devm_kcalloc(dev, MAX77650_REGULATOR_NUM_REGULATORS,
+			      sizeof(*rdescs), GFP_KERNEL);
+	if (!rdescs)
+		return -ENOMEM;
+
+	map = dev_get_regmap(parent, NULL);
+	if (!map)
+		return -ENODEV;
+
+	rv = regmap_read(map, MAX77650_REG_CID, &val);
+	if (rv)
+		return rv;
+
+	rdescs[MAX77650_REGULATOR_ID_LDO] = &max77650_LDO_desc;
+	rdescs[MAX77650_REGULATOR_ID_SBB0] = &max77650_SBB0_desc;
+
+	switch (MAX77650_CID_BITS(val)) {
+	case MAX77650_CID_77650A:
+	case MAX77650_CID_77650C:
+		rdescs[MAX77650_REGULATOR_ID_SBB1] = &max77650_SBB1_desc;
+		rdescs[MAX77650_REGULATOR_ID_SBB2] = &max77650_SBB2_desc;
+		break;
+	case MAX77650_CID_77651A:
+	case MAX77650_CID_77651B:
+		rdescs[MAX77650_REGULATOR_ID_SBB1] = &max77651_SBB1_desc;
+		rdescs[MAX77650_REGULATOR_ID_SBB2] = &max77651_SBB2_desc;
+		break;
+	default:
+		return -ENODEV;
+	}
+
+	config.dev = dev;
+
+	for (i = 0; i < MAX77650_REGULATOR_NUM_REGULATORS; i++) {
+		rdesc = rdescs[i];
+		config.driver_data = rdesc;
+		config.of_node = NULL;
+		config.init_data = NULL;
+
+		for_each_child_of_node(dev->of_node, child) {
+			if (!of_node_name_eq(child, rdesc->desc.name))
+				continue;
+
+			init_data = of_get_regulator_init_data(dev, child,
+							       &rdesc->desc);
+			if (!init_data)
+				return -ENOMEM;
+
+			config.of_node = child;
+			config.init_data = init_data;
+		}
+
+		rdev = devm_regulator_register(dev, &rdesc->desc, &config);
+		if (IS_ERR(rdev))
+			return PTR_ERR(rdev);
+	}
+
+	return 0;
+}
+
+static struct platform_driver max77650_regulator_driver = {
+	.driver = {
+		.name = "max77650-regulator",
+	},
+	.probe = max77650_regulator_probe,
+};
+module_platform_driver(max77650_regulator_driver);
+
+MODULE_DESCRIPTION("MAXIM 77650/77651 regulator driver");
+MODULE_AUTHOR("Bartosz Golaszewski <bgolaszewski@baylibre.com>");
+MODULE_LICENSE("GPL");
-- 
2.19.1

Re: [PATCH 08/13] regulator: max77650: add regulator support

[Mark Brown]

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

On Fri, Jan 18, 2019 at 02:42:39PM +0100, Bartosz Golaszewski wrote:

> Add regulator support for max77650. We support all four variants of this
> PMIC including non-linear voltage table for max77651 SBB1 rail.

Looks good, the ramping stuff might be a candidate for core (TBH I was
sure we'd got that implemented already but we don't seem to) but that
can be done later and the more complex one with non-linear steps does
feel like it might have to stay in the driver anyway.

A couple of small nits:

> +++ b/drivers/regulator/max77650-regulator.c
> @@ -0,0 +1,537 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2018 BayLibre SAS
> + * Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Please make the entire header C++ style so it looks more intentional.

> +		for_each_child_of_node(dev->of_node, child) {
> +			if (!of_node_name_eq(child, rdesc->desc.name))
> +				continue;
> +
> +			init_data = of_get_regulator_init_data(dev, child,
> +							       &rdesc->desc);
> +			if (!init_data)
> +				return -ENOMEM;
> +
> +			config.of_node = child;
> +			config.init_data = init_data;
> +		}

You don't need to do this, the core will do it for you (it will actually
still do it even with the above, it'll only fall back to using
config->init_data if it's own lookup fails).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 08/13] regulator: max77650: add regulator support

[Bartosz Golaszewski]

pt., 18 sty 2019 o 19:01 Mark Brown <broonie@kernel.org> napisał(a):
>
> On Fri, Jan 18, 2019 at 02:42:39PM +0100, Bartosz Golaszewski wrote:
>
> > Add regulator support for max77650. We support all four variants of this
> > PMIC including non-linear voltage table for max77651 SBB1 rail.
>
> Looks good, the ramping stuff might be a candidate for core (TBH I was
> sure we'd got that implemented already but we don't seem to) but that
> can be done later and the more complex one with non-linear steps does
> feel like it might have to stay in the driver anyway.
>
> A couple of small nits:
>
> > +++ b/drivers/regulator/max77650-regulator.c
> > @@ -0,0 +1,537 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2018 BayLibre SAS
> > + * Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Please make the entire header C++ style so it looks more intentional.
>

Seems like there are more files in the kernel source using the mixed
comment style for the SPDX identifier and I also prefer it over C++
only. Would you mind if it stayed that way?

> > +             for_each_child_of_node(dev->of_node, child) {
> > +                     if (!of_node_name_eq(child, rdesc->desc.name))
> > +                             continue;
> > +
> > +                     init_data = of_get_regulator_init_data(dev, child,
> > +                                                            &rdesc->desc);
> > +                     if (!init_data)
> > +                             return -ENOMEM;
> > +
> > +                     config.of_node = child;
> > +                     config.init_data = init_data;
> > +             }
>
> You don't need to do this, the core will do it for you (it will actually
> still do it even with the above, it'll only fall back to using
> config->init_data if it's own lookup fails).

I added this loop specifically because the core would not pick up the
init data from DT. What did I miss (some specific variable to assign)?
I just noticed some other drivers do the same and thought it's the
right thing to do.

Thanks,
Bart

Re: [PATCH 08/13] regulator: max77650: add regulator support

[Mark Brown]

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

On Fri, Jan 18, 2019 at 07:13:38PM +0100, Bartosz Golaszewski wrote:
> pt., 18 sty 2019 o 19:01 Mark Brown <broonie@kernel.org> napisał(a):

> > > +++ b/drivers/regulator/max77650-regulator.c
> > > @@ -0,0 +1,537 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (C) 2018 BayLibre SAS
> > > + * Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>

> > Please make the entire header C++ style so it looks more intentional.

> Seems like there are more files in the kernel source using the mixed
> comment style for the SPDX identifier and I also prefer it over C++
> only. Would you mind if it stayed that way?

I'd prefer to change.

> > You don't need to do this, the core will do it for you (it will actually
> > still do it even with the above, it'll only fall back to using
> > config->init_data if it's own lookup fails).

> I added this loop specifically because the core would not pick up the
> init data from DT. What did I miss (some specific variable to assign)?
> I just noticed some other drivers do the same and thought it's the
> right thing to do.

You should set regulators_node and of_match, see regulator_of_get_init_node().

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 08/13] regulator: max77650: add regulator support

[Bartosz Golaszewski]

pt., 18 sty 2019 o 19:01 Mark Brown <broonie@kernel.org> napisał(a):
>
> On Fri, Jan 18, 2019 at 02:42:39PM +0100, Bartosz Golaszewski wrote:
>
> > Add regulator support for max77650. We support all four variants of this
> > PMIC including non-linear voltage table for max77651 SBB1 rail.
>

Sorry for spamming, I missed it the last time.

> Looks good, the ramping stuff might be a candidate for core (TBH I was
> sure we'd got that implemented already but we don't seem to) but that
> can be done later and the more complex one with non-linear steps does
> feel like it might have to stay in the driver anyway.
>

I'm thinking about implementing that in the core for the next release
cycle and then making this driver the first user.

Bart

> A couple of small nits:
>
> > +++ b/drivers/regulator/max77650-regulator.c
> > @@ -0,0 +1,537 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2018 BayLibre SAS
> > + * Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Please make the entire header C++ style so it looks more intentional.
>
> > +             for_each_child_of_node(dev->of_node, child) {
> > +                     if (!of_node_name_eq(child, rdesc->desc.name))
> > +                             continue;
> > +
> > +                     init_data = of_get_regulator_init_data(dev, child,
> > +                                                            &rdesc->desc);
> > +                     if (!init_data)
> > +                             return -ENOMEM;
> > +
> > +                     config.of_node = child;
> > +                     config.init_data = init_data;
> > +             }
>
> You don't need to do this, the core will do it for you (it will actually
> still do it even with the above, it'll only fall back to using
> config->init_data if it's own lookup fails).

[PATCH 09/13] 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 | 347 ++++++++++++++++++++++++
 3 files changed, 355 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..fb9d8f933174
--- /dev/null
+++ b/drivers/power/supply/max77650-charger.c
@@ -0,0 +1,347 @@
+// 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_BATTERY,
+	.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 regmap_irq_chip_data *irq_data;
+	struct power_supply_config pscfg = {};
+	struct max77650_charger_data *chg;
+	struct power_supply *battery;
+	struct device *dev, *parent;
+	int rv, chg_irq, chgin_irq;
+	struct i2c_client *i2c;
+	unsigned int prop;
+
+	dev = &pdev->dev;
+	parent = dev->parent;
+	i2c = to_i2c_client(parent);
+	irq_data = i2c_get_clientdata(i2c);
+
+	chg = devm_kzalloc(dev, sizeof(*chg), GFP_KERNEL);
+	if (!chg)
+		return -ENOMEM;
+
+	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 = regmap_irq_get_virq(irq_data, MAX77650_INT_CHG);
+	if (chg_irq <= 0)
+		return -EINVAL;
+
+	chgin_irq = regmap_irq_get_virq(irq_data, MAX77650_INT_CHGIN);
+	if (chgin_irq <= 0)
+		return -EINVAL;
+
+	rv = devm_request_threaded_irq(dev, chg_irq, NULL,
+				       max77650_charger_check_status,
+				       IRQF_ONESHOT, "chg", chg);
+	if (rv)
+		return rv;
+
+	rv = devm_request_threaded_irq(dev, chgin_irq, NULL,
+				       max77650_charger_check_status,
+				       IRQF_ONESHOT, "chgin", chg);
+	if (rv)
+		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 struct platform_driver max77650_charger_driver = {
+	.driver = {
+		.name = "max77650-charger",
+	},
+	.probe = max77650_charger_probe,
+};
+module_platform_driver(max77650_charger_driver);
+
+MODULE_DESCRIPTION("MAXIM 77650/77651 charger driver");
+MODULE_AUTHOR("Bartosz Golaszewski <bgolaszewski@baylibre.com>");
+MODULE_LICENSE("GPL");
-- 
2.19.1

Re: [PATCH 09/13] power: supply: max77650: add support for battery charger

[Sebastian Reichel]

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

Hi Bartosz,

Looks mostly ok, I have a few comments inline towards the end.

On Fri, Jan 18, 2019 at 02:42:40PM +0100, Bartosz Golaszewski wrote:
> 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 | 347 ++++++++++++++++++++++++
>  3 files changed, 355 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..fb9d8f933174
> --- /dev/null
> +++ b/drivers/power/supply/max77650-charger.c
> @@ -0,0 +1,347 @@
> +// 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_BATTERY,

This is a charger, not a battery. BATTERY type is for the fuel
gauges. You should be using POWER_SUPPLY_TYPE_USB for USB based
battery chargers and POWER_SUPPLY_TYPE_MAINS otherwise.

> +	.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 regmap_irq_chip_data *irq_data;
> +	struct power_supply_config pscfg = {};
> +	struct max77650_charger_data *chg;
> +	struct power_supply *battery;
> +	struct device *dev, *parent;
> +	int rv, chg_irq, chgin_irq;
> +	struct i2c_client *i2c;
> +	unsigned int prop;
> +
> +	dev = &pdev->dev;
> +	parent = dev->parent;
> +	i2c = to_i2c_client(parent);
> +	irq_data = i2c_get_clientdata(i2c);
> +
> +	chg = devm_kzalloc(dev, sizeof(*chg), GFP_KERNEL);
> +	if (!chg)
> +		return -ENOMEM;
> +
> +	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 = regmap_irq_get_virq(irq_data, MAX77650_INT_CHG);
> +	if (chg_irq <= 0)
> +		return -EINVAL;
> +
> +	chgin_irq = regmap_irq_get_virq(irq_data, MAX77650_INT_CHGIN);
> +	if (chgin_irq <= 0)
> +		return -EINVAL;
> +
> +	rv = devm_request_threaded_irq(dev, chg_irq, NULL,
> +				       max77650_charger_check_status,
> +				       IRQF_ONESHOT, "chg", chg);
> +	if (rv)
> +		return rv;
> +
> +	rv = devm_request_threaded_irq(dev, chgin_irq, NULL,
> +				       max77650_charger_check_status,
> +				       IRQF_ONESHOT, "chgin", chg);
> +	if (rv)
> +		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);

should this be disabled on module remove?

> +}
> +
> +static struct platform_driver max77650_charger_driver = {
> +	.driver = {
> +		.name = "max77650-charger",
> +	},
> +	.probe = max77650_charger_probe,
> +};
> +module_platform_driver(max77650_charger_driver);
> +
> +MODULE_DESCRIPTION("MAXIM 77650/77651 charger driver");
> +MODULE_AUTHOR("Bartosz Golaszewski <bgolaszewski@baylibre.com>");
> +MODULE_LICENSE("GPL");

This is GPL2 or later, but "SPDX-License-Identifier: GPL-2.0" is not.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 10/13] 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 | 189 +++++++++++++++++++++++++++++++++++
 3 files changed, 197 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..f8fe2b083e4d
--- /dev/null
+++ b/drivers/gpio/gpio-max77650.c
@@ -0,0 +1,189 @@
+// 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 regmap_irq_chip_data *irq_chip_data;
+	struct gpio_chip gc;
+};
+
+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 regmap_irq_get_virq(chip->irq_chip_data, MAX77650_INT_GPI);
+}
+
+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_chip_data = i2c_get_clientdata(i2c);
+
+	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");
-- 
2.19.1

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

[Linus Walleij]

Hi Bartosz,

thanks for the patch!

On Fri, Jan 18, 2019 at 2:43 PM 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>

Overall you know for sure what you're doing so not much to
say about the GPIO chip etc. The .set_config() is nice and
helpful (maybe you will be able to also add pull up/down
using Thomas Petazzoni's new config interfaces!)

However enlighten me on this:

> +static int max77650_gpio_to_irq(struct gpio_chip *gc, unsigned int offset)
> +{
> +       struct max77650_gpio_chip *chip = gpiochip_get_data(gc);
> +
> +       return regmap_irq_get_virq(chip->irq_chip_data, MAX77650_INT_GPI);
> +}

I know this may be opening the gates to a lot of coding, but
isn't this IRQ hierarchical? I.e. that irqchip is not in the
node of the GPIO chip but in the node of the MFD top
device, with a 1:1 mapping between some of the IRQs
and a certain GPIO line.

Using regmap IRQ makes it confusing for me so I do not
know for sure if that is the case.

I am worried that you are recreating a problem (present in many
drivers, including some written by me, mea culpa) that Brian Masney
has been trying to solve for the gpiochip inside the SPMI
GPIO (drivers/pinctrl/qcom/pinctrl-spmi-gpio.c).

I have queued Brians refactoring and solution here:
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/log/?h=ib-qcom-spmi

And the overall description on what he's trying to achieve is
here:
https://marc.info/?l=linux-gpio&m=154793071511130&w=2

My problem description (which I fear will apply also to this
driver):
https://www.spinics.net/lists/linux-gpio/msg34655.html

I plan to merge Brians patches soon-ish to devel and then from
there try to construct more helpers in the gpiolib core,
and if possible fix some of the old drivers who rely on
.to_irq().

We will certainly fix ssbi-gpio as well, and that is a good start
since the Qualdcomm platforms are so pervasive in the
market.

But maybe this doesn't apply here? I am not the smartest...
Just want to make sure that it is possible to refer an
interrupt directly to this DT node, as it is indeed marked
as interrupt-controller.

Yours,
Linus Walleij

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

[Linus Walleij]

Hi Bartosz,

thanks for the patch!

On Fri, Jan 18, 2019 at 2:43 PM 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>

Overall you know for sure what you're doing so not much to
say about the GPIO chip etc. The .set_config() is nice and
helpful (maybe you will be able to also add pull up/down
using Thomas Petazzoni's new config interfaces!)

However enlighten me on this:

> +static int max77650_gpio_to_irq(struct gpio_chip *gc, unsigned int offset)
> +{
> +       struct max77650_gpio_chip *chip = gpiochip_get_data(gc);
> +
> +       return regmap_irq_get_virq(chip->irq_chip_data, MAX77650_INT_GPI);
> +}

I know this may be opening the gates to a lot of coding, but
isn't this IRQ hierarchical? I.e. that irqchip is not in the
node of the GPIO chip but in the node of the MFD top
device, with a 1:1 mapping between some of the IRQs
and a certain GPIO line.

Using regmap IRQ makes it confusing for me so I do not
know for sure if that is the case.

I am worried that you are recreating a problem (present in many
drivers, including some written by me, mea culpa) that Brian Masney
has been trying to solve for the gpiochip inside the SPMI
GPIO (drivers/pinctrl/qcom/pinctrl-spmi-gpio.c).

I have queued Brians refactoring and solution here:
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/log/?h=ib-qcom-spmi

And the overall description on what he's trying to achieve is
here:
https://marc.info/?l=linux-gpio&m=154793071511130&w=2

My problem description (which I fear will apply also to this
driver):
https://www.spinics.net/lists/linux-gpio/msg34655.html

I plan to merge Brians patches soon-ish to devel and then from
there try to construct more helpers in the gpiolib core,
and if possible fix some of the old drivers who rely on
.to_irq().

We will certainly fix ssbi-gpio as well, and that is a good start
since the Qualdcomm platforms are so pervasive in the
market.

But maybe this doesn't apply here? I am not the smartest...
Just want to make sure that it is possible to refer an
interrupt directly to this DT node, as it is indeed marked
as interrupt-controller.

Yours,
Linus Walleij

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

[Bartosz Golaszewski]

pon., 21 sty 2019 o 15:20 Linus Walleij <linus.walleij@linaro.org> napisał(a):
>
> Hi Bartosz,
>
> thanks for the patch!
>
> On Fri, Jan 18, 2019 at 2:43 PM 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>
>
> Overall you know for sure what you're doing so not much to
> say about the GPIO chip etc. The .set_config() is nice and
> helpful (maybe you will be able to also add pull up/down
> using Thomas Petazzoni's new config interfaces!)
>
> However enlighten me on this:
>
> > +static int max77650_gpio_to_irq(struct gpio_chip *gc, unsigned int offset)
> > +{
> > +       struct max77650_gpio_chip *chip = gpiochip_get_data(gc);
> > +
> > +       return regmap_irq_get_virq(chip->irq_chip_data, MAX77650_INT_GPI);
> > +}
>
> I know this may be opening the gates to a lot of coding, but
> isn't this IRQ hierarchical? I.e. that irqchip is not in the
> node of the GPIO chip but in the node of the MFD top
> device, with a 1:1 mapping between some of the IRQs
> and a certain GPIO line.
>
> Using regmap IRQ makes it confusing for me so I do not
> know for sure if that is the case.
>
> I am worried that you are recreating a problem (present in many
> drivers, including some written by me, mea culpa) that Brian Masney
> has been trying to solve for the gpiochip inside the SPMI
> GPIO (drivers/pinctrl/qcom/pinctrl-spmi-gpio.c).
>
> I have queued Brians refactoring and solution here:
> https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/log/?h=ib-qcom-spmi
>
> And the overall description on what he's trying to achieve is
> here:
> https://marc.info/?l=linux-gpio&m=154793071511130&w=2
>
> My problem description (which I fear will apply also to this
> driver):
> https://www.spinics.net/lists/linux-gpio/msg34655.html
>
> I plan to merge Brians patches soon-ish to devel and then from
> there try to construct more helpers in the gpiolib core,
> and if possible fix some of the old drivers who rely on
> .to_irq().
>
> We will certainly fix ssbi-gpio as well, and that is a good start
> since the Qualdcomm platforms are so pervasive in the
> market.
>
> But maybe this doesn't apply here? I am not the smartest...
> Just want to make sure that it is possible to refer an
> interrupt directly to this DT node, as it is indeed marked
> as interrupt-controller.
>

Hi Linus,

Thank you for your review. While I think you're right about the issue
being present in this driver, I'm not sure it's really a problem. Do
we actually require every gpio-controller to also be a stand-alone
interrupt-controller? The binding document for the GPIO module doesn't
mention this - it only requires the gpio-controller property. Without
the "interrupt-controller" property dtc will bail-out if anyone uses
this node as the interrupt parent.

If I'm wrong and we do require it, then I think we need to update
Documentation/devicetree/bindings/gpio/gpio.txt.

Best regards,
Bartosz Golaszewski

PS: FYI since this submission I dropped the virtual irq number lookup
in sub-drivers in favor of resources setup by the parent driver[1] as
suggested by Dmitry in his review of the input module driver.

[1] https://github.com/brgl/linux/blob/topic/max77650_mfd_driver/drivers/gpio/gpio-max77650.c#L158

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

[Bartosz Golaszewski]

pon., 21 sty 2019 o 15:20 Linus Walleij <linus.walleij@linaro.org> napisał(a):
>
> Hi Bartosz,
>
> thanks for the patch!
>
> On Fri, Jan 18, 2019 at 2:43 PM 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>
>
> Overall you know for sure what you're doing so not much to
> say about the GPIO chip etc. The .set_config() is nice and
> helpful (maybe you will be able to also add pull up/down
> using Thomas Petazzoni's new config interfaces!)
>
> However enlighten me on this:
>
> > +static int max77650_gpio_to_irq(struct gpio_chip *gc, unsigned int offset)
> > +{
> > +       struct max77650_gpio_chip *chip = gpiochip_get_data(gc);
> > +
> > +       return regmap_irq_get_virq(chip->irq_chip_data, MAX77650_INT_GPI);
> > +}
>
> I know this may be opening the gates to a lot of coding, but
> isn't this IRQ hierarchical? I.e. that irqchip is not in the
> node of the GPIO chip but in the node of the MFD top
> device, with a 1:1 mapping between some of the IRQs
> and a certain GPIO line.
>
> Using regmap IRQ makes it confusing for me so I do not
> know for sure if that is the case.
>
> I am worried that you are recreating a problem (present in many
> drivers, including some written by me, mea culpa) that Brian Masney
> has been trying to solve for the gpiochip inside the SPMI
> GPIO (drivers/pinctrl/qcom/pinctrl-spmi-gpio.c).
>
> I have queued Brians refactoring and solution here:
> https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/log/?h=ib-qcom-spmi
>
> And the overall description on what he's trying to achieve is
> here:
> https://marc.info/?l=linux-gpio&m=154793071511130&w=2
>
> My problem description (which I fear will apply also to this
> driver):
> https://www.spinics.net/lists/linux-gpio/msg34655.html
>
> I plan to merge Brians patches soon-ish to devel and then from
> there try to construct more helpers in the gpiolib core,
> and if possible fix some of the old drivers who rely on
> .to_irq().
>
> We will certainly fix ssbi-gpio as well, and that is a good start
> since the Qualdcomm platforms are so pervasive in the
> market.
>
> But maybe this doesn't apply here? I am not the smartest...
> Just want to make sure that it is possible to refer an
> interrupt directly to this DT node, as it is indeed marked
> as interrupt-controller.
>

Hi Linus,

Thank you for your review. While I think you're right about the issue
being present in this driver, I'm not sure it's really a problem. Do
we actually require every gpio-controller to also be a stand-alone
interrupt-controller? The binding document for the GPIO module doesn't
mention this - it only requires the gpio-controller property. Without
the "interrupt-controller" property dtc will bail-out if anyone uses
this node as the interrupt parent.

If I'm wrong and we do require it, then I think we need to update
Documentation/devicetree/bindings/gpio/gpio.txt.

Best regards,
Bartosz Golaszewski

PS: FYI since this submission I dropped the virtual irq number lookup
in sub-drivers in favor of resources setup by the parent driver[1] as
suggested by Dmitry in his review of the input module driver.

[1] https://github.com/brgl/linux/blob/topic/max77650_mfd_driver/drivers/gpio/gpio-max77650.c#L158

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

[Linus Walleij]

On Mon, Jan 21, 2019 at 6:07 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> Thank you for your review. While I think you're right about the issue
> being present in this driver, I'm not sure it's really a problem. Do
> we actually require every gpio-controller to also be a stand-alone
> interrupt-controller?

Absolutely not :D

Just GPIO is fine.

> The binding document for the GPIO module doesn't
> mention this - it only requires the gpio-controller property. Without
> the "interrupt-controller" property dtc will bail-out if anyone uses
> this node as the interrupt parent.
>
> If I'm wrong and we do require it, then I think we need to update
> Documentation/devicetree/bindings/gpio/gpio.txt.

What is weird is if a driver with DT bindings not mentioning IRQ
and only probing from DT start implementing IRQ support, that
becomes quite inconsistent. So then max77650_gpio_to_irq()
should just return -ENOTSUPP
or something for now, then it's fine.

We can add the (complicated) IRQ handling later.

I am trying to eat my own dogfood here, I was sweating all
last night trying to implement a hierarchical IRQ controller.
There is no running away from that now. :/

Apparently doing hierarchical IRQs demand that all irq
controllers up to the top-level SoC IRQ controller support
hierarchical interrupts using the v2 version of the irqdomain
API, and currently it seems like the ARM
GIC seems like the only top level IRQ controller that can
do that.

Yours,
Linus Walleij

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

[Linus Walleij]

On Mon, Jan 21, 2019 at 6:07 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> Thank you for your review. While I think you're right about the issue
> being present in this driver, I'm not sure it's really a problem. Do
> we actually require every gpio-controller to also be a stand-alone
> interrupt-controller?

Absolutely not :D

Just GPIO is fine.

> The binding document for the GPIO module doesn't
> mention this - it only requires the gpio-controller property. Without
> the "interrupt-controller" property dtc will bail-out if anyone uses
> this node as the interrupt parent.
>
> If I'm wrong and we do require it, then I think we need to update
> Documentation/devicetree/bindings/gpio/gpio.txt.

What is weird is if a driver with DT bindings not mentioning IRQ
and only probing from DT start implementing IRQ support, that
becomes quite inconsistent. So then max77650_gpio_to_irq()
should just return -ENOTSUPP
or something for now, then it's fine.

We can add the (complicated) IRQ handling later.

I am trying to eat my own dogfood here, I was sweating all
last night trying to implement a hierarchical IRQ controller.
There is no running away from that now. :/

Apparently doing hierarchical IRQs demand that all irq
controllers up to the top-level SoC IRQ controller support
hierarchical interrupts using the v2 version of the irqdomain
API, and currently it seems like the ARM
GIC seems like the only top level IRQ controller that can
do that.

Yours,
Linus Walleij

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

[Bartosz Golaszewski]

czw., 24 sty 2019 o 11:30 Linus Walleij <linus.walleij@linaro.org> napisał(a):
>
> On Mon, Jan 21, 2019 at 6:07 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> > Thank you for your review. While I think you're right about the issue
> > being present in this driver, I'm not sure it's really a problem. Do
> > we actually require every gpio-controller to also be a stand-alone
> > interrupt-controller?
>
> Absolutely not :D
>
> Just GPIO is fine.
>
> > The binding document for the GPIO module doesn't
> > mention this - it only requires the gpio-controller property. Without
> > the "interrupt-controller" property dtc will bail-out if anyone uses
> > this node as the interrupt parent.
> >
> > If I'm wrong and we do require it, then I think we need to update
> > Documentation/devicetree/bindings/gpio/gpio.txt.
>
> What is weird is if a driver with DT bindings not mentioning IRQ
> and only probing from DT start implementing IRQ support, that
> becomes quite inconsistent. So then max77650_gpio_to_irq()
> should just return -ENOTSUPP
> or something for now, then it's fine.
>

I don't see it as weird at all. I see the need to define the register
and interrupt resources in DT for SoC peripherals becaue SoCs often
reuse IPs. But in the case of a self-contained i2c PMIC - the modules
such as GPIO are tightly coupled with the core functionality. In the
case of this device for example: there isn't even a separate set of
mask/status registers for GPIO interrupts.

Most mfd devices setup the resources in a hard-coded manner.

> We can add the (complicated) IRQ handling later.
>
> I am trying to eat my own dogfood here, I was sweating all
> last night trying to implement a hierarchical IRQ controller.
> There is no running away from that now. :/
>
> Apparently doing hierarchical IRQs demand that all irq
> controllers up to the top-level SoC IRQ controller support
> hierarchical interrupts using the v2 version of the irqdomain
> API, and currently it seems like the ARM
> GIC seems like the only top level IRQ controller that can
> do that.
>

Yep, and for that reason I can't use the regmap irq_chip abstraction
for now because it doesn't implement support for hierarchical
interrupts either.

How about the cascaded gpiochip irq_chip?

Best regards,
Bartosz

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

[Bartosz Golaszewski]

czw., 24 sty 2019 o 11:30 Linus Walleij <linus.walleij@linaro.org> napisał(a):
>
> On Mon, Jan 21, 2019 at 6:07 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> > Thank you for your review. While I think you're right about the issue
> > being present in this driver, I'm not sure it's really a problem. Do
> > we actually require every gpio-controller to also be a stand-alone
> > interrupt-controller?
>
> Absolutely not :D
>
> Just GPIO is fine.
>
> > The binding document for the GPIO module doesn't
> > mention this - it only requires the gpio-controller property. Without
> > the "interrupt-controller" property dtc will bail-out if anyone uses
> > this node as the interrupt parent.
> >
> > If I'm wrong and we do require it, then I think we need to update
> > Documentation/devicetree/bindings/gpio/gpio.txt.
>
> What is weird is if a driver with DT bindings not mentioning IRQ
> and only probing from DT start implementing IRQ support, that
> becomes quite inconsistent. So then max77650_gpio_to_irq()
> should just return -ENOTSUPP
> or something for now, then it's fine.
>

I don't see it as weird at all. I see the need to define the register
and interrupt resources in DT for SoC peripherals becaue SoCs often
reuse IPs. But in the case of a self-contained i2c PMIC - the modules
such as GPIO are tightly coupled with the core functionality. In the
case of this device for example: there isn't even a separate set of
mask/status registers for GPIO interrupts.

Most mfd devices setup the resources in a hard-coded manner.

> We can add the (complicated) IRQ handling later.
>
> I am trying to eat my own dogfood here, I was sweating all
> last night trying to implement a hierarchical IRQ controller.
> There is no running away from that now. :/
>
> Apparently doing hierarchical IRQs demand that all irq
> controllers up to the top-level SoC IRQ controller support
> hierarchical interrupts using the v2 version of the irqdomain
> API, and currently it seems like the ARM
> GIC seems like the only top level IRQ controller that can
> do that.
>

Yep, and for that reason I can't use the regmap irq_chip abstraction
for now because it doesn't implement support for hierarchical
interrupts either.

How about the cascaded gpiochip irq_chip?

Best regards,
Bartosz

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

[Bartosz Golaszewski]

wt., 29 sty 2019 o 12:00 Bartosz Golaszewski <brgl@bgdev.pl> napisał(a):
>
> czw., 24 sty 2019 o 11:30 Linus Walleij <linus.walleij@linaro.org> napisał(a):
> >
> > On Mon, Jan 21, 2019 at 6:07 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > > Thank you for your review. While I think you're right about the issue
> > > being present in this driver, I'm not sure it's really a problem. Do
> > > we actually require every gpio-controller to also be a stand-alone
> > > interrupt-controller?
> >
> > Absolutely not :D
> >
> > Just GPIO is fine.
> >
> > > The binding document for the GPIO module doesn't
> > > mention this - it only requires the gpio-controller property. Without
> > > the "interrupt-controller" property dtc will bail-out if anyone uses
> > > this node as the interrupt parent.
> > >
> > > If I'm wrong and we do require it, then I think we need to update
> > > Documentation/devicetree/bindings/gpio/gpio.txt.
> >
> > What is weird is if a driver with DT bindings not mentioning IRQ
> > and only probing from DT start implementing IRQ support, that
> > becomes quite inconsistent. So then max77650_gpio_to_irq()
> > should just return -ENOTSUPP
> > or something for now, then it's fine.
> >
>
> I don't see it as weird at all. I see the need to define the register
> and interrupt resources in DT for SoC peripherals becaue SoCs often
> reuse IPs. But in the case of a self-contained i2c PMIC - the modules
> such as GPIO are tightly coupled with the core functionality. In the
> case of this device for example: there isn't even a separate set of
> mask/status registers for GPIO interrupts.
>
> Most mfd devices setup the resources in a hard-coded manner.
>
> > We can add the (complicated) IRQ handling later.
> >
> > I am trying to eat my own dogfood here, I was sweating all
> > last night trying to implement a hierarchical IRQ controller.
> > There is no running away from that now. :/
> >
> > Apparently doing hierarchical IRQs demand that all irq
> > controllers up to the top-level SoC IRQ controller support
> > hierarchical interrupts using the v2 version of the irqdomain
> > API, and currently it seems like the ARM
> > GIC seems like the only top level IRQ controller that can
> > do that.
> >
>
> Yep, and for that reason I can't use the regmap irq_chip abstraction
> for now because it doesn't implement support for hierarchical
> interrupts either.
>
> How about the cascaded gpiochip irq_chip?
>
> Best regards,
> Bartosz

Nah that won't work either without a proper hierarchy...

In that case, let's leave out the irq support for now. I'll send v2.

Bart

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

[Bartosz Golaszewski]

wt., 29 sty 2019 o 12:00 Bartosz Golaszewski <brgl@bgdev.pl> napisał(a):
>
> czw., 24 sty 2019 o 11:30 Linus Walleij <linus.walleij@linaro.org> napisał(a):
> >
> > On Mon, Jan 21, 2019 at 6:07 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > > Thank you for your review. While I think you're right about the issue
> > > being present in this driver, I'm not sure it's really a problem. Do
> > > we actually require every gpio-controller to also be a stand-alone
> > > interrupt-controller?
> >
> > Absolutely not :D
> >
> > Just GPIO is fine.
> >
> > > The binding document for the GPIO module doesn't
> > > mention this - it only requires the gpio-controller property. Without
> > > the "interrupt-controller" property dtc will bail-out if anyone uses
> > > this node as the interrupt parent.
> > >
> > > If I'm wrong and we do require it, then I think we need to update
> > > Documentation/devicetree/bindings/gpio/gpio.txt.
> >
> > What is weird is if a driver with DT bindings not mentioning IRQ
> > and only probing from DT start implementing IRQ support, that
> > becomes quite inconsistent. So then max77650_gpio_to_irq()
> > should just return -ENOTSUPP
> > or something for now, then it's fine.
> >
>
> I don't see it as weird at all. I see the need to define the register
> and interrupt resources in DT for SoC peripherals becaue SoCs often
> reuse IPs. But in the case of a self-contained i2c PMIC - the modules
> such as GPIO are tightly coupled with the core functionality. In the
> case of this device for example: there isn't even a separate set of
> mask/status registers for GPIO interrupts.
>
> Most mfd devices setup the resources in a hard-coded manner.
>
> > We can add the (complicated) IRQ handling later.
> >
> > I am trying to eat my own dogfood here, I was sweating all
> > last night trying to implement a hierarchical IRQ controller.
> > There is no running away from that now. :/
> >
> > Apparently doing hierarchical IRQs demand that all irq
> > controllers up to the top-level SoC IRQ controller support
> > hierarchical interrupts using the v2 version of the irqdomain
> > API, and currently it seems like the ARM
> > GIC seems like the only top level IRQ controller that can
> > do that.
> >
>
> Yep, and for that reason I can't use the regmap irq_chip abstraction
> for now because it doesn't implement support for hierarchical
> interrupts either.
>
> How about the cascaded gpiochip irq_chip?
>
> Best regards,
> Bartosz

Nah that won't work either without a proper hierarchy...

In that case, let's leave out the irq support for now. I'll send v2.

Bart

[PATCH 11/13] 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>
---
 drivers/leds/Kconfig         |   6 ++
 drivers/leds/Makefile        |   1 +
 drivers/leds/leds-max77650.c | 162 +++++++++++++++++++++++++++++++++++
 3 files changed, 169 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..be2bb1c60448
--- /dev/null
+++ b/drivers/leds/leds-max77650.c
@@ -0,0 +1,162 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 BayLibre SAS
+ * Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
+ *
+ * LEDS 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_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)
+
+/* 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_leds;
+
+struct max77650_led {
+	struct led_classdev cdev;
+	struct max77650_leds *parent;
+	unsigned int regA;
+	unsigned int regB;
+};
+
+struct max77650_leds {
+	struct regmap *map;
+	struct max77650_led leds[MAX77650_NUM_LEDS];
+};
+
+static struct max77650_led *max77650_to_led(struct led_classdev *cdev)
+{
+	return container_of(cdev, struct max77650_led, cdev);
+}
+
+static int max77650_leds_brightness_set(struct led_classdev *cdev,
+					enum led_brightness brightness)
+{
+	struct max77650_led *led = max77650_to_led(cdev);
+	struct regmap *regmap = led->parent->map;
+	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;
+		/*
+		 * We can set the brightness with 5-bit resolution.
+		 *
+		 * For brightness == 1, the bits we're writing will be 0, but
+		 * since we keep LED_FS0 set to 12.8mA full-scale range, the
+		 * LED will be lit slightly.
+		 */
+		val |= brightness / 8;
+	}
+
+	return regmap_update_bits(regmap, led->regA, mask, val);
+}
+
+static int max77650_leds_probe(struct platform_device *pdev)
+{
+	struct device_node *of_node, *child;
+	struct max77650_leds *leds;
+	struct max77650_led *led;
+	struct device *parent;
+	struct device *dev;
+	int rv, num_leds;
+	u32 reg;
+
+	dev = &pdev->dev;
+	parent = dev->parent;
+	of_node = dev->of_node;
+
+	if (!of_node)
+		return -ENODEV;
+
+	leds = devm_kzalloc(dev, sizeof(*leds), GFP_KERNEL);
+	if (!leds)
+		return -ENOMEM;
+
+	leds->map = dev_get_regmap(dev->parent, NULL);
+	if (!leds->map)
+		return -ENODEV;
+
+	num_leds = of_get_child_count(of_node);
+	if (!num_leds || num_leds > MAX77650_NUM_LEDS)
+		return -ENODEV;
+
+	for_each_child_of_node(of_node, child) {
+		rv = of_property_read_u32(child, "reg", &reg);
+		if (rv || reg >= MAX77650_NUM_LEDS)
+			return -EINVAL;
+
+		led = &leds->leds[reg];
+
+		led->parent = leds;
+		led->regA = MAX77650_LED_A_BASE + reg;
+		led->regB = MAX77650_LED_B_BASE + reg;
+		led->cdev.brightness_set_blocking
+					= max77650_leds_brightness_set;
+
+		led->cdev.name = of_get_property(child, "label", NULL);
+		if (!led->cdev.name)
+			led->cdev.name = child->name;
+
+		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(leds->map, led->regA,
+				  MAX77650_LED_A_DEFAULT);
+		if (rv)
+			return rv;
+
+		rv = regmap_write(leds->map, led->regB,
+				  MAX77650_LED_B_DEFAULT);
+		if (rv)
+			return rv;
+	}
+
+	rv = regmap_write(leds->map,
+			  MAX77650_REG_CNFG_LED_TOP,
+			  MAX77650_LED_TOP_DEFAULT);
+	if (rv)
+		return rv;
+
+	return 0;
+}
+
+static struct platform_driver max77650_leds_driver = {
+	.driver = {
+		.name = "max77650-leds",
+	},
+	.probe = max77650_leds_probe,
+};
+module_platform_driver(max77650_leds_driver);
+
+MODULE_DESCRIPTION("MAXIM 77650/77651 LED driver");
+MODULE_AUTHOR("Bartosz Golaszewski <bgolaszewski@baylibre.com>");
+MODULE_LICENSE("GPL");
-- 
2.19.1

Re: [PATCH 11/13] leds: max77650: add LEDs support

[Jacek Anaszewski]

Hi Bartosz,

Thank you for the patch.

I have few minor issues below.

On 1/18/19 2:42 PM, Bartosz Golaszewski wrote:
> 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>
> ---
>   drivers/leds/Kconfig         |   6 ++
>   drivers/leds/Makefile        |   1 +
>   drivers/leds/leds-max77650.c | 162 +++++++++++++++++++++++++++++++++++
>   3 files changed, 169 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..be2bb1c60448
> --- /dev/null
> +++ b/drivers/leds/leds-max77650.c
> @@ -0,0 +1,162 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2018 BayLibre SAS
> + * Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> + *
> + * LEDS driver for MAXIM 77650/77651 charger/power-supply.
> + */

Please use uniform C++ comment style.

If in doubt, please refer to the following Linus statement
from [0]:

"And yes, feel free to replace block comments with // while at it."

> +#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_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)
> +
> +/* 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_leds;
> +
> +struct max77650_led {
> +	struct led_classdev cdev;
> +	struct max77650_leds *parent;
> +	unsigned int regA;
> +	unsigned int regB;
> +};
> +
> +struct max77650_leds {
> +	struct regmap *map;
> +	struct max77650_led leds[MAX77650_NUM_LEDS];
> +};
> +
> +static struct max77650_led *max77650_to_led(struct led_classdev *cdev)
> +{
> +	return container_of(cdev, struct max77650_led, cdev);
> +}
> +
> +static int max77650_leds_brightness_set(struct led_classdev *cdev,
> +					enum led_brightness brightness)

I would change function names to max77650_led* (singular).
It would be consistent with most of the other LED class drivers.
LED driver for max77693 flash cell also uses this scheme
(drivers/leds/leds-max77693.c).

> +{
> +	struct max77650_led *led = max77650_to_led(cdev);
> +	struct regmap *regmap = led->parent->map;
> +	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;
> +		/*
> +		 * We can set the brightness with 5-bit resolution.
> +		 *
> +		 * For brightness == 1, the bits we're writing will be 0, but
> +		 * since we keep LED_FS0 set to 12.8mA full-scale range, the
> +		 * LED will be lit slightly.
> +		 */
> +		val |= brightness / 8;
> +	}
> +
> +	return regmap_update_bits(regmap, led->regA, mask, val);
> +}
> +
> +static int max77650_leds_probe(struct platform_device *pdev)
> +{
> +	struct device_node *of_node, *child;
> +	struct max77650_leds *leds;
> +	struct max77650_led *led;
> +	struct device *parent;
> +	struct device *dev;
> +	int rv, num_leds;
> +	u32 reg;
> +
> +	dev = &pdev->dev;
> +	parent = dev->parent;
> +	of_node = dev->of_node;
> +
> +	if (!of_node)
> +		return -ENODEV;
> +
> +	leds = devm_kzalloc(dev, sizeof(*leds), GFP_KERNEL);
> +	if (!leds)
> +		return -ENOMEM;
> +
> +	leds->map = dev_get_regmap(dev->parent, NULL);
> +	if (!leds->map)
> +		return -ENODEV;
> +
> +	num_leds = of_get_child_count(of_node);
> +	if (!num_leds || num_leds > MAX77650_NUM_LEDS)
> +		return -ENODEV;
> +
> +	for_each_child_of_node(of_node, child) {
> +		rv = of_property_read_u32(child, "reg", &reg);
> +		if (rv || reg >= MAX77650_NUM_LEDS)
> +			return -EINVAL;
> +
> +		led = &leds->leds[reg];
> +
> +		led->parent = leds;
> +		led->regA = MAX77650_LED_A_BASE + reg;
> +		led->regB = MAX77650_LED_B_BASE + reg;
> +		led->cdev.brightness_set_blocking
> +					= max77650_leds_brightness_set;
> +
> +		led->cdev.name = of_get_property(child, "label", NULL);
> +		if (!led->cdev.name)
> +			led->cdev.name = child->name;

Please follow how other recent LED class drivers construct LED names,
e.g. drivers/leds/leds-cr0014114.c. We are in the course of creating
generic support for creating LED names, but until it is agreed upon
we've got to use other drivers as a reference.

> +
> +		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(leds->map, led->regA,
> +				  MAX77650_LED_A_DEFAULT);
> +		if (rv)
> +			return rv;
> +
> +		rv = regmap_write(leds->map, led->regB,
> +				  MAX77650_LED_B_DEFAULT);
> +		if (rv)
> +			return rv;
> +	}
> +
> +	rv = regmap_write(leds->map,
> +			  MAX77650_REG_CNFG_LED_TOP,
> +			  MAX77650_LED_TOP_DEFAULT);
> +	if (rv)
> +		return rv;

No need to check rv here:

	return regmap_write(leds->map,
			  MAX77650_REG_CNFG_LED_TOP,
			  MAX77650_LED_TOP_DEFAULT)

> +	return 0;
> +}
> +
> +static struct platform_driver max77650_leds_driver = {
> +	.driver = {
> +		.name = "max77650-leds",

s/leds/led/

> +	},
> +	.probe = max77650_leds_probe,
> +};
> +module_platform_driver(max77650_leds_driver);
> +
> +MODULE_DESCRIPTION("MAXIM 77650/77651 LED driver");
> +MODULE_AUTHOR("Bartosz Golaszewski <bgolaszewski@baylibre.com>");
> +MODULE_LICENSE("GPL");

s/GPL/GPL v2/


[0] https://lkml.org/lkml/2017/11/2/715

-- 
Best regards,
Jacek Anaszewski

[PATCH 12/13] 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>
---
 drivers/input/misc/Kconfig          |   9 ++
 drivers/input/misc/Makefile         |   1 +
 drivers/input/misc/max77650-onkey.c | 135 ++++++++++++++++++++++++++++
 3 files changed, 145 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..cc7e83f589cd
--- /dev/null
+++ b/drivers/input/misc/max77650-onkey.c
@@ -0,0 +1,135 @@
+// 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_report(struct max77650_onkey *onkey, int value)
+{
+	input_report_key(onkey->input, onkey->code, value);
+	input_sync(onkey->input);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t max77650_onkey_falling(int irq, void *data)
+{
+	struct max77650_onkey *onkey = data;
+
+	return max77650_onkey_report(onkey, 0);
+}
+
+static irqreturn_t max77650_onkey_rising(int irq, void *data)
+{
+	struct max77650_onkey *onkey = data;
+
+	return max77650_onkey_report(onkey, 1);
+}
+
+static int max77650_onkey_probe(struct platform_device *pdev)
+{
+	struct regmap_irq_chip_data *irq_data;
+	struct max77650_onkey *onkey;
+	struct device *dev, *parent;
+	int irq_r, irq_f, rv, mode;
+	struct i2c_client *i2c;
+	const char *mode_prop;
+	struct regmap *map;
+
+	dev = &pdev->dev;
+	parent = dev->parent;
+	i2c = to_i2c_client(parent);
+	irq_data = i2c_get_clientdata(i2c);
+
+	map = dev_get_regmap(parent, NULL);
+	if (!map)
+		return -ENODEV;
+
+	onkey = devm_kzalloc(dev, sizeof(*onkey), GFP_KERNEL);
+	if (!onkey)
+		return -ENOMEM;
+
+	rv = device_property_read_u32(dev, "linux,code", &onkey->code);
+	if (rv)
+		onkey->code = KEY_POWER;
+
+	rv = device_property_read_string(dev, "maxim,onkey-mode", &mode_prop);
+	if (rv)
+		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;
+
+	rv = regmap_update_bits(map, MAX77650_REG_CNFG_GLBL,
+				MAX77650_ONKEY_MODE_MASK, mode);
+	if (rv)
+		return rv;
+
+	irq_f = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_F);
+	if (irq_f <= 0)
+		return -EINVAL;
+
+	irq_r = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_R);
+	if (irq_r <= 0)
+		return -EINVAL;
+
+	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;
+	onkey->input->dev.parent = dev;
+	input_set_capability(onkey->input, EV_KEY, onkey->code);
+
+	rv = devm_request_threaded_irq(dev, irq_f, NULL,
+				       max77650_onkey_falling,
+				       IRQF_ONESHOT, "onkey-down", onkey);
+	if (rv)
+		return rv;
+
+	rv = devm_request_threaded_irq(dev, irq_r, NULL,
+				       max77650_onkey_rising,
+				       IRQF_ONESHOT, "onkey-up", onkey);
+	if (rv)
+		return rv;
+
+	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");
-- 
2.19.1

Re: [PATCH 12/13] input: max77650: add onkey support

[Dmitry Torokhov]

Hi Bartosz,

On Fri, Jan 18, 2019 at 02:42:43PM +0100, Bartosz Golaszewski wrote:
> 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>
> ---
>  drivers/input/misc/Kconfig          |   9 ++
>  drivers/input/misc/Makefile         |   1 +
>  drivers/input/misc/max77650-onkey.c | 135 ++++++++++++++++++++++++++++
>  3 files changed, 145 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..cc7e83f589cd
> --- /dev/null
> +++ b/drivers/input/misc/max77650-onkey.c
> @@ -0,0 +1,135 @@
> +// 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_report(struct max77650_onkey *onkey, int value)
> +{
> +	input_report_key(onkey->input, onkey->code, value);
> +	input_sync(onkey->input);
> +
> +	return IRQ_HANDLED;

It is weird that report function returns irqreturn_t. I'd simply moved
input_report_key()/input_sync() into real IRQ handlers.

> +}
> +
> +static irqreturn_t max77650_onkey_falling(int irq, void *data)
> +{
> +	struct max77650_onkey *onkey = data;
> +
> +	return max77650_onkey_report(onkey, 0);
> +}
> +
> +static irqreturn_t max77650_onkey_rising(int irq, void *data)
> +{
> +	struct max77650_onkey *onkey = data;
> +
> +	return max77650_onkey_report(onkey, 1);
> +}
> +
> +static int max77650_onkey_probe(struct platform_device *pdev)
> +{
> +	struct regmap_irq_chip_data *irq_data;
> +	struct max77650_onkey *onkey;
> +	struct device *dev, *parent;
> +	int irq_r, irq_f, rv, mode;

Please call "rv" "error".

> +	struct i2c_client *i2c;
> +	const char *mode_prop;
> +	struct regmap *map;
> +
> +	dev = &pdev->dev;
> +	parent = dev->parent;
> +	i2c = to_i2c_client(parent);
> +	irq_data = i2c_get_clientdata(i2c);
> +
> +	map = dev_get_regmap(parent, NULL);
> +	if (!map)
> +		return -ENODEV;
> +
> +	onkey = devm_kzalloc(dev, sizeof(*onkey), GFP_KERNEL);
> +	if (!onkey)
> +		return -ENOMEM;
> +
> +	rv = device_property_read_u32(dev, "linux,code", &onkey->code);
> +	if (rv)
> +		onkey->code = KEY_POWER;
> +
> +	rv = device_property_read_string(dev, "maxim,onkey-mode", &mode_prop);
> +	if (rv)
> +		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;
> +
> +	rv = regmap_update_bits(map, MAX77650_REG_CNFG_GLBL,
> +				MAX77650_ONKEY_MODE_MASK, mode);
> +	if (rv)
> +		return rv;
> +
> +	irq_f = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_F);
> +	if (irq_f <= 0)
> +		return -EINVAL;
> +
> +	irq_r = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_R);
> +	if (irq_r <= 0)
> +		return -EINVAL;

Ugh, it would be better if you handled IRQ mapping in the MFD piece and
passed it as resources of platform device. Then you'd simply call
platform_get_irq() here and did not have to reach into parent device for
"irq_dara".

> +
> +	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;
> +	onkey->input->dev.parent = dev;

Not needed since devm_input_allocate_device sets parent for you.

> +	input_set_capability(onkey->input, EV_KEY, onkey->code);
> +
> +	rv = devm_request_threaded_irq(dev, irq_f, NULL,

Why threaded interrupt with only hard interrupt handler? If parent
interrupt is threaded use "any_context_irq" here.

> +				       max77650_onkey_falling,
> +				       IRQF_ONESHOT, "onkey-down", onkey);

Why do you need oneshot with interrupt that is essentially not threaded?

> +	if (rv)
> +		return rv;
> +
> +	rv = devm_request_threaded_irq(dev, irq_r, NULL,
> +				       max77650_onkey_rising,
> +				       IRQF_ONESHOT, "onkey-up", onkey);
> +	if (rv)
> +		return rv;
> +
> +	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");

SPDX header say GPL-2.0 so please "GPL v2" here as "GPL" means v2 and
later.

Thanks.

-- 
Dmitry

Re: [PATCH 12/13] input: max77650: add onkey support

[Bartosz Golaszewski]

sob., 19 sty 2019 o 10:03 Dmitry Torokhov <dmitry.torokhov@gmail.com>
napisał(a):
>
> Hi Bartosz,
>
> On Fri, Jan 18, 2019 at 02:42:43PM +0100, Bartosz Golaszewski wrote:
> > 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>
> > ---
> >  drivers/input/misc/Kconfig          |   9 ++
> >  drivers/input/misc/Makefile         |   1 +
> >  drivers/input/misc/max77650-onkey.c | 135 ++++++++++++++++++++++++++++
> >  3 files changed, 145 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..cc7e83f589cd
> > --- /dev/null
> > +++ b/drivers/input/misc/max77650-onkey.c
> > @@ -0,0 +1,135 @@
> > +// 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_report(struct max77650_onkey *onkey, int value)
> > +{
> > +     input_report_key(onkey->input, onkey->code, value);
> > +     input_sync(onkey->input);
> > +
> > +     return IRQ_HANDLED;
>
> It is weird that report function returns irqreturn_t. I'd simply moved
> input_report_key()/input_sync() into real IRQ handlers.
>
> > +}
> > +
> > +static irqreturn_t max77650_onkey_falling(int irq, void *data)
> > +{
> > +     struct max77650_onkey *onkey = data;
> > +
> > +     return max77650_onkey_report(onkey, 0);
> > +}
> > +
> > +static irqreturn_t max77650_onkey_rising(int irq, void *data)
> > +{
> > +     struct max77650_onkey *onkey = data;
> > +
> > +     return max77650_onkey_report(onkey, 1);
> > +}
> > +
> > +static int max77650_onkey_probe(struct platform_device *pdev)
> > +{
> > +     struct regmap_irq_chip_data *irq_data;
> > +     struct max77650_onkey *onkey;
> > +     struct device *dev, *parent;
> > +     int irq_r, irq_f, rv, mode;
>
> Please call "rv" "error".
>
> > +     struct i2c_client *i2c;
> > +     const char *mode_prop;
> > +     struct regmap *map;
> > +
> > +     dev = &pdev->dev;
> > +     parent = dev->parent;
> > +     i2c = to_i2c_client(parent);
> > +     irq_data = i2c_get_clientdata(i2c);
> > +
> > +     map = dev_get_regmap(parent, NULL);
> > +     if (!map)
> > +             return -ENODEV;
> > +
> > +     onkey = devm_kzalloc(dev, sizeof(*onkey), GFP_KERNEL);
> > +     if (!onkey)
> > +             return -ENOMEM;
> > +
> > +     rv = device_property_read_u32(dev, "linux,code", &onkey->code);
> > +     if (rv)
> > +             onkey->code = KEY_POWER;
> > +
> > +     rv = device_property_read_string(dev, "maxim,onkey-mode", &mode_prop);
> > +     if (rv)
> > +             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;
> > +
> > +     rv = regmap_update_bits(map, MAX77650_REG_CNFG_GLBL,
> > +                             MAX77650_ONKEY_MODE_MASK, mode);
> > +     if (rv)
> > +             return rv;
> > +
> > +     irq_f = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_F);
> > +     if (irq_f <= 0)
> > +             return -EINVAL;
> > +
> > +     irq_r = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_R);
> > +     if (irq_r <= 0)
> > +             return -EINVAL;
>
> Ugh, it would be better if you handled IRQ mapping in the MFD piece and
> passed it as resources of platform device. Then you'd simply call
> platform_get_irq() here and did not have to reach into parent device for
> "irq_dara".
>
> > +
> > +     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;
> > +     onkey->input->dev.parent = dev;
>
> Not needed since devm_input_allocate_device sets parent for you.
>
> > +     input_set_capability(onkey->input, EV_KEY, onkey->code);
> > +
> > +     rv = devm_request_threaded_irq(dev, irq_f, NULL,
>
> Why threaded interrupt with only hard interrupt handler? If parent
> interrupt is threaded use "any_context_irq" here.
>

Hi Dmitry,

actually it's the other way around. Take a look at the function
prototype for  devm_request_threaded_irq()[1].

The third parameter is the hard-irq handler (NULL in my patch), the
fourth is the thread function. Actually even if I did what you're
saying - it would never work as this is a nested irq for which the
hard-irq handler is never called.

For the rest: I'll fix it in v2.

Best regards,
Bartosz Golaszewski

[1] https://elixir.bootlin.com/linux/latest/source/kernel/irq/devres.c#L52

> > +                                    max77650_onkey_falling,
> > +                                    IRQF_ONESHOT, "onkey-down", onkey);
>
> Why do you need oneshot with interrupt that is essentially not threaded?
>
> > +     if (rv)
> > +             return rv;
> > +
> > +     rv = devm_request_threaded_irq(dev, irq_r, NULL,
> > +                                    max77650_onkey_rising,
> > +                                    IRQF_ONESHOT, "onkey-up", onkey);
> > +     if (rv)
> > +             return rv;
> > +
> > +     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");
>
> SPDX header say GPL-2.0 so please "GPL v2" here as "GPL" means v2 and
> later.
>
> Thanks.
>
> --
> Dmitry

Re: [PATCH 12/13] input: max77650: add onkey support

[Dmitry Torokhov]

On Mon, Jan 21, 2019 at 11:52:50AM +0100, Bartosz Golaszewski wrote:
> sob., 19 sty 2019 o 10:03 Dmitry Torokhov <dmitry.torokhov@gmail.com>
> napisał(a):
> >
> > Hi Bartosz,
> >
> > On Fri, Jan 18, 2019 at 02:42:43PM +0100, Bartosz Golaszewski wrote:
> > > +     input_set_capability(onkey->input, EV_KEY, onkey->code);
> > > +
> > > +     rv = devm_request_threaded_irq(dev, irq_f, NULL,
> >
> > Why threaded interrupt with only hard interrupt handler? If parent
> > interrupt is threaded use "any_context_irq" here.
> >
> 
> Hi Dmitry,
> 
> actually it's the other way around. Take a look at the function
> prototype for  devm_request_threaded_irq()[1].
> 
> The third parameter is the hard-irq handler (NULL in my patch), the
> fourth is the thread function. Actually even if I did what you're
> saying - it would never work as this is a nested irq for which the
> hard-irq handler is never called.

Sorry, my eyes must have crossed. Still, from the driver POV the
interrupt does not have to be threaded, this is dictated by the
constraints beyond the driver control. For these cases we have
devm_request_any_context_irq() that takes essentially only "hard" IRQ
handler, but internally either does request_irq() or
request_threaded_irq(), depending on the context (whether the interrupt
is nested or not). Using devm_request_any_context_irq() should not have
any behavioral changes, but documents the logic better.

Thanks.

-- 
Dmitry

Re: [PATCH 12/13] input: max77650: add onkey support

[Lee Jones]

> > 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>
> > ---
> >  drivers/input/misc/Kconfig          |   9 ++
> >  drivers/input/misc/Makefile         |   1 +
> >  drivers/input/misc/max77650-onkey.c | 135 ++++++++++++++++++++++++++++
> >  3 files changed, 145 insertions(+)
> >  create mode 100644 drivers/input/misc/max77650-onkey.c

[...]

Moving things around a bit:

> > +static int max77650_onkey_probe(struct platform_device *pdev)
> > +{

> > +	irq_f = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_F);
> > +	if (irq_f <= 0)
> > +		return -EINVAL;
> > +
> > +	irq_r = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_R);
> > +	if (irq_r <= 0)
> > +		return -EINVAL;
> 
> Ugh, it would be better if you handled IRQ mapping in the MFD piece and
> passed it as resources of platform device. Then you'd simply call
> platform_get_irq() here and did not have to reach into parent device for
> "irq_dara".

These device IRQs were defined and registered with the Regmap *set*
(actually init()) APIs and thus should be pulled out using the
appropriate reverse Regmap *get* APIs here in the device.

Registering them with Regmap *and* pulling them back out again in the
same (MFD in this case) driver, only to register them as platform data
is certainly not how I see the API being designed/used.

> > +	struct regmap_irq_chip_data *irq_data;
> > +	struct device *dev, *parent;

> > +	dev = &pdev->dev;
> > +	parent = dev->parent;
> > +	i2c = to_i2c_client(parent);
> > +	irq_data = i2c_get_clientdata(i2c);
> > +
> > +	map = dev_get_regmap(parent, NULL);
> > +	if (!map)
> > +		return -ENODEV;

However, this hoop jumping is a bit crazy and for the most part
superfluous.  Instead, create a struct of attributes you wish to share
with the child devices (containing both regmap (which you should call
regmap and not map by the way) and irq_data) and pass it as either
platform data (preferred) or as device data.

If you choose the latter, you do not need to convert from 'device' to
'i2c' to do the look-up.  Since this function (probe()) is provided
with a platform_device, you can just use platform_get_drvdata() to
achieve the same as above.

If you go the preferred (platform data) route, then you should use
dev_get_platdata() instead.

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

Re: [PATCH 12/13] input: max77650: add onkey support

[Dmitry Torokhov]

Hi Lee,

On Tue, Feb 12, 2019 at 12:34 PM Lee Jones <lee.jones@linaro.org> wrote:
>
> > > 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>
> > > ---
> > >  drivers/input/misc/Kconfig          |   9 ++
> > >  drivers/input/misc/Makefile         |   1 +
> > >  drivers/input/misc/max77650-onkey.c | 135 ++++++++++++++++++++++++++++
> > >  3 files changed, 145 insertions(+)
> > >  create mode 100644 drivers/input/misc/max77650-onkey.c
>
> [...]
>
> Moving things around a bit:
>
> > > +static int max77650_onkey_probe(struct platform_device *pdev)
> > > +{
>
> > > +   irq_f = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_F);
> > > +   if (irq_f <= 0)
> > > +           return -EINVAL;
> > > +
> > > +   irq_r = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_R);
> > > +   if (irq_r <= 0)
> > > +           return -EINVAL;
> >
> > Ugh, it would be better if you handled IRQ mapping in the MFD piece and
> > passed it as resources of platform device. Then you'd simply call
> > platform_get_irq() here and did not have to reach into parent device for
> > "irq_dara".
>
> These device IRQs were defined and registered with the Regmap *set*
> (actually init()) APIs and thus should be pulled out using the
> appropriate reverse Regmap *get* APIs here in the device.
>
> Registering them with Regmap *and* pulling them back out again in the
> same (MFD in this case) driver, only to register them as platform data
> is certainly not how I see the API being designed/used.
>
> > > +   struct regmap_irq_chip_data *irq_data;
> > > +   struct device *dev, *parent;
>
> > > +   dev = &pdev->dev;
> > > +   parent = dev->parent;
> > > +   i2c = to_i2c_client(parent);
> > > +   irq_data = i2c_get_clientdata(i2c);
> > > +
> > > +   map = dev_get_regmap(parent, NULL);
> > > +   if (!map)
> > > +           return -ENODEV;
>
> However, this hoop jumping is a bit crazy and for the most part
> superfluous.  Instead, create a struct of attributes you wish to share
> with the child devices (containing both regmap (which you should call
> regmap and not map by the way) and irq_data) and pass it as either
> platform data (preferred) or as device data.
>
> If you choose the latter, you do not need to convert from 'device' to
> 'i2c' to do the look-up.  Since this function (probe()) is provided
> with a platform_device, you can just use platform_get_drvdata() to
> achieve the same as above.
>
> If you go the preferred (platform data) route, then you should use
> dev_get_platdata() instead.

By doing what you are suggesting (introducing platform data) you
introducing strong dependency between MFD and input piece for no
different reason. With the current implementation the parent can be
reworked completely without involving onkey driver.

I find such clean separation desirable. We are trying to move away
form platform data where it makes sense.

Thanks.

-- 
Dmitry

Re: [PATCH 12/13] input: max77650: add onkey support

[Lee Jones]

On Tue, 12 Feb 2019, Dmitry Torokhov wrote:

> Hi Lee,
> 
> On Tue, Feb 12, 2019 at 12:34 PM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > > > 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>
> > > > ---
> > > >  drivers/input/misc/Kconfig          |   9 ++
> > > >  drivers/input/misc/Makefile         |   1 +
> > > >  drivers/input/misc/max77650-onkey.c | 135 ++++++++++++++++++++++++++++
> > > >  3 files changed, 145 insertions(+)
> > > >  create mode 100644 drivers/input/misc/max77650-onkey.c
> >
> > [...]
> >
> > Moving things around a bit:
> >
> > > > +static int max77650_onkey_probe(struct platform_device *pdev)
> > > > +{
> >
> > > > +   irq_f = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_F);
> > > > +   if (irq_f <= 0)
> > > > +           return -EINVAL;
> > > > +
> > > > +   irq_r = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_R);
> > > > +   if (irq_r <= 0)
> > > > +           return -EINVAL;
> > >
> > > Ugh, it would be better if you handled IRQ mapping in the MFD piece and
> > > passed it as resources of platform device. Then you'd simply call
> > > platform_get_irq() here and did not have to reach into parent device for
> > > "irq_dara".
> >
> > These device IRQs were defined and registered with the Regmap *set*
> > (actually init()) APIs and thus should be pulled out using the
> > appropriate reverse Regmap *get* APIs here in the device.
> >
> > Registering them with Regmap *and* pulling them back out again in the
> > same (MFD in this case) driver, only to register them as platform data
> > is certainly not how I see the API being designed/used.
> >
> > > > +   struct regmap_irq_chip_data *irq_data;
> > > > +   struct device *dev, *parent;
> >
> > > > +   dev = &pdev->dev;
> > > > +   parent = dev->parent;
> > > > +   i2c = to_i2c_client(parent);
> > > > +   irq_data = i2c_get_clientdata(i2c);
> > > > +
> > > > +   map = dev_get_regmap(parent, NULL);
> > > > +   if (!map)
> > > > +           return -ENODEV;
> >
> > However, this hoop jumping is a bit crazy and for the most part
> > superfluous.  Instead, create a struct of attributes you wish to share
> > with the child devices (containing both regmap (which you should call
> > regmap and not map by the way) and irq_data) and pass it as either
> > platform data (preferred) or as device data.
> >
> > If you choose the latter, you do not need to convert from 'device' to
> > 'i2c' to do the look-up.  Since this function (probe()) is provided
> > with a platform_device, you can just use platform_get_drvdata() to
> > achieve the same as above.
> >
> > If you go the preferred (platform data) route, then you should use
> > dev_get_platdata() instead.
> 
> By doing what you are suggesting (introducing platform data) you
> introducing strong dependency between MFD and input piece for no
> different reason. With the current implementation the parent can be
> reworked completely without involving onkey driver.
> 
> I find such clean separation desirable. We are trying to move away
> form platform data where it makes sense.

Never mind.  We found a way forward where everyone wins!

Thanks for your time.

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

[PATCH 13/13] 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 4d04cebb4a71..bb29fe250796 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9220,6 +9220,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.19.1