[PATCH v5 0/5] Add support for Silicon Mitus SM5703 MFD

[PATCH v5 0/5] Add support for Silicon Mitus SM5703 MFD

[Markuss Broks]

This series adds support for Silicon Mitus SM5703 MFD and the
appropriate device-tree bindings. This only adds support for the
regulator module, leaving room for other modules implemented in
future (code for other modules is really not ready for submission
right now). Silicon Mitus SM5703 is used on various mobile phones,
mostly Samsung Galaxy (J5 (2015, 2016), On7, J7 (2015, 2016) ...).

Cc: Matti Vaittinen <mazziesaccount@gmail.com>

v2:
- mfd bindings: add "SM5703" to the title
- mfd bindings: indent the example
- regulators/Makefile: sort alphabetically
v3:
- mfd bindings: fix an error in example: there should be no
  newline between #size-cells and #address-cells and the node
v4:
mfd bindings:
- drop the -mfd in the compatible
- change reference path to the regulator yaml to an absolute one
- use -gpios for the reset GPIO
- make reset-gpios a required property
- correct the example node name
- correct the example compatible
regulator bindings:
- add unevaluatedProperties: false to all four types of regulators
v5:
mfd driver:
- depend on OF for the SM5703 MFD (Kconfig)
- correct the compatible in the MFD driver
- remove the compatible of -regulators in the struct mfd_cell*
mfd bindings:
- adjust the example for regulators {} node
- include GPIO header in the example
- indent the example properly
- adjust the example for the voltage regulators being fixed
regulator bindings:
- remove the compatible
- refer to regulators.yaml
regulator driver:
- add a separate ops for fixed voltage regulators (to avoid kernel WARN)
- set the config.dev to dev->parent to probe from parent OF node
- (Kconfig) SM5703_MFD -> MFD_SM5703 fix typo

- Add a patch to avoid driver name conflicts in SM5502 extcon driver

Markuss Broks (4):
  dt-bindings: regulator: Add bindings for Silicon Mitus SM5703
    regulators
  dt-bindings: mfd: Add bindings for Silicon Mitus SM5703 MFD
  mfd: sm5703: Add support for SM5703 MFD
  regulator: sm5703-regulator: Add regulators support for SM5703 MFD

 .../bindings/mfd/siliconmitus,sm5703.yaml     |  91 ++++++++++
 .../siliconmitus,sm5703-regulator.yaml        |  48 ++++++
 MAINTAINERS                                   |   8 +
 drivers/mfd/Kconfig                           |  12 ++
 drivers/mfd/Makefile                          |   1 +
 drivers/mfd/sm5703.c                          |  81 +++++++++
 drivers/regulator/Kconfig                     |   7 +
 drivers/regulator/Makefile                    |   1 +
 drivers/regulator/sm5703-regulator.c          | 162 ++++++++++++++++++
 include/linux/mfd/sm5703.h                    | 105 ++++++++++++
 10 files changed, 516 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml
 create mode 100644 Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml
 create mode 100644 drivers/mfd/sm5703.c
 create mode 100644 drivers/regulator/sm5703-regulator.c
 create mode 100644 include/linux/mfd/sm5703.h

-- 
2.35.1

[PATCH v5 1/5] extcon: sm5502: Clarify SM5703's i2c device ID

[Markuss Broks]

While SM5502 and SM5504 are purely micro USB switching
circuits, SM5703 is a multi-function device which has multiple
modules in it. Change the i2c_device_id of it to avoid conflict
with MFD driver.

Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
---
 drivers/extcon/extcon-sm5502.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/extcon/extcon-sm5502.c b/drivers/extcon/extcon-sm5502.c
index 17a40c3782ee..f706f5288257 100644
--- a/drivers/extcon/extcon-sm5502.c
+++ b/drivers/extcon/extcon-sm5502.c
@@ -831,7 +831,7 @@ static SIMPLE_DEV_PM_OPS(sm5502_muic_pm_ops,
 static const struct i2c_device_id sm5502_i2c_id[] = {
 	{ "sm5502", (kernel_ulong_t)&sm5502_data },
 	{ "sm5504", (kernel_ulong_t)&sm5504_data },
-	{ "sm5703", (kernel_ulong_t)&sm5502_data },
+	{ "sm5703-muic", (kernel_ulong_t)&sm5502_data },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, sm5502_i2c_id);
-- 
2.35.1

[PATCH v5 2/5] dt-bindings: regulator: Add bindings for Silicon Mitus SM5703 regulators

[Markuss Broks]

This patch adds device-tree bindings for regulators on Silicon Mitus
SM5703 MFD.

Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
---
 .../siliconmitus,sm5703-regulator.yaml        | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml

diff --git a/Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml b/Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml
new file mode 100644
index 000000000000..75ff16b58000
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml
@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/regulator/siliconmitus,sm5703-regulator.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Silicon Mitus SM5703 multi function device regulators
+
+maintainers:
+  - Markuss Broks <markuss.broks@gmail.com>
+
+description: |
+  SM5703 regulators node should be a sub node of the SM5703 MFD node. See SM5703 MFD
+  bindings at Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml
+  Regulator nodes should be named as USBLDO_<number>, BUCK, VBUS, LDO_<number>.
+  The definition for each of these nodes is defined using the standard
+  binding for regulators at Documentation/devicetree/bindings/regulator/regulator.txt.
+
+properties:
+  buck:
+    type: object
+    $ref: regulators.yaml#
+    unevaluatedProperties: false
+    description:
+      Properties for the BUCK regulator.
+
+  vbus:
+    type: object
+    $ref: regulators.yaml#
+    unevaluatedProperties: false
+    description:
+      Properties for the VBUS regulator.
+
+patternProperties:
+  "^ldo[1-3]$":
+    type: object
+    $ref: regulators.yaml#
+    unevaluatedProperties: false
+    description:
+      Properties for single LDO regulator.
+
+  "^usbldo[1-2]$":
+    type: object
+    $ref: regulators.yaml#
+    unevaluatedProperties: false
+    description:
+      Properties for a single USBLDO regulator.
+
+additionalProperties: false
-- 
2.35.1

[PATCH v5 3/5] dt-bindings: mfd: Add bindings for Silicon Mitus SM5703 MFD

[Markuss Broks]

This patch adds device-tree bindings for the Silicon Mitus
SM5703 MFD.

Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
---
 .../bindings/mfd/siliconmitus,sm5703.yaml     | 84 +++++++++++++++++++
 1 file changed, 84 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml

diff --git a/Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml b/Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml
new file mode 100644
index 000000000000..32197be57cce
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml
@@ -0,0 +1,84 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/siliconmitus,sm5703.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Silicon Mitus SM5703 multi-function device bindings
+
+maintainers:
+  - Markuss Broks <markuss.broks@gmail.com>
+
+description: |
+  Silicon Mitus SM5703 is a multi-function device, that consists of several
+  modules, designed for a mobile phone use. It consists of
+  LDO, Buck, USBLDO and VBUS regulators, a flash LED driver, a MUIC unit,
+  a fuel gauge and a battery charger circuit. The MUIC and Fuel-Gauge units
+  are separate from the main MFD, having their own i2c lines, while the
+  LED driver, regulators and charger are sharing the main i2c bus of the MFD.
+
+properties:
+  compatible:
+    const: siliconmitus,sm5703
+
+  reg:
+    description:
+      I2C slave address.
+    maxItems: 1
+
+  regulators:
+    $ref: /schemas/regulator/siliconmitus,sm5703-regulator.yaml
+    description:
+      List of child nodes that specify the regulators.
+
+  reset-gpios:
+    description:
+      GPIO which is connected to the MRSTB pin.
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - regulators
+  - reset-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        pmic@49 {
+          compatible = "siliconmitus,sm5703";
+          reg = <0x49>;
+
+          reset-gpios = <&msmgpio 24 GPIO_ACTIVE_HIGH>;
+          pinctrl-0 = <&mrstb_default>;
+
+          regulators {
+            sm5703_ldo1: ldo1 {
+              regulator-min-microvolt = <2800000>;
+              regulator-max-microvolt = <2800000>;
+            };
+
+            sm5703_ldo2: ldo2 {
+              regulator-min-microvolt = <1500000>;
+              regulator-max-microvolt = <3300000>;
+            };
+
+            sm5703_ldo3: ldo3 {
+              regulator-min-microvolt = <3300000>;
+              regulator-max-microvolt = <3300000>;
+            };
+
+            sm5703_usbldo1: usbldo1 { };
+
+            sm5703_usbldo2: usbldo2 { };
+
+            sm5703_vbus: vbus { };
+          };
+        };
+      };
+...
-- 
2.35.1

[PATCH v5 4/5] mfd: sm5703: Add support for SM5703 MFD

[Markuss Broks]

Silicon Mitus SM5703 is a multi-function device, meant to be
used in mobile devices. It has several modules: LDO, BUCK regulators,
charger circuit, flash LED driver, a fuel gauge for monitoring the battery
and a MUIC USB switcher. The MUIC and fuel gauge parts are separate in that
they use separate i2c lines to communicate with the device, while charger
circuit, LED driver and regulators are on the main i2c line the device is
controlled with.

Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
---
 MAINTAINERS                |   8 +++
 drivers/mfd/Kconfig        |  13 +++++
 drivers/mfd/Makefile       |   1 +
 drivers/mfd/sm5703.c       |  78 +++++++++++++++++++++++++++
 include/linux/mfd/sm5703.h | 105 +++++++++++++++++++++++++++++++++++++
 5 files changed, 205 insertions(+)
 create mode 100644 drivers/mfd/sm5703.c
 create mode 100644 include/linux/mfd/sm5703.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 6157e706ed02..6125ed1a3be4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18172,6 +18172,14 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git dev
 F:	include/linux/srcu*.h
 F:	kernel/rcu/srcu*.c
 
+SM5703 MFD DRIVER
+M:	Markuss Broks <markuss.broks@gmail.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml
+F:	Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml
+F:	drivers/mfd/sm5703.c
+F:	drivers/regulator/sm5703-regulator.c
+
 SMACK SECURITY MODULE
 M:	Casey Schaufler <casey@schaufler-ca.com>
 L:	linux-security-module@vger.kernel.org
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 3b59456f5545..c13a99ceae99 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1237,6 +1237,19 @@ config MFD_SM501
 	  interface. The device may be connected by PCI or local bus with
 	  varying functions enabled.
 
+config MFD_SM5703
+	tristate "Silicon Mitus SM5703 MFD"
+	depends on I2C
+	depends on OF
+	select MFD_CORE
+	select REGMAP_I2C
+	help
+	  This is the core driver for the Silicon Mitus SM5703 multi-function
+	  device. This device is meant to be used in phones and it has numerous
+	  modules, including LED controller, regulators, fuel gauge, MUIC and
+	  charger circuit. It also support muxing a serial interface over USB
+	  data lines.
+
 config MFD_SM501_GPIO
 	bool "Export GPIO via GPIO layer"
 	depends on MFD_SM501 && GPIOLIB
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 858cacf659d6..ca8b86736a36 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -275,3 +275,4 @@ rsmu-i2c-objs			:= rsmu_core.o rsmu_i2c.o
 rsmu-spi-objs			:= rsmu_core.o rsmu_spi.o
 obj-$(CONFIG_MFD_RSMU_I2C)	+= rsmu-i2c.o
 obj-$(CONFIG_MFD_RSMU_SPI)	+= rsmu-spi.o
+obj-$(CONFIG_MFD_SM5703)	+= sm5703.o
diff --git a/drivers/mfd/sm5703.c b/drivers/mfd/sm5703.c
new file mode 100644
index 000000000000..7f9838149051
--- /dev/null
+++ b/drivers/mfd/sm5703.c
@@ -0,0 +1,78 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/err.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/sm5703.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+
+static const struct mfd_cell sm5703_devs[] = {
+	{ .name = "sm5703-regulator", },
+};
+
+static const struct regmap_config sm5703_regmap_config = {
+	.reg_bits	= 8,
+	.val_bits	= 8,
+};
+
+static int sm5703_i2c_probe(struct i2c_client *i2c,
+			    const struct i2c_device_id *id)
+{
+	struct sm5703_dev *sm5703;
+	struct device *dev = &i2c->dev;
+	unsigned int dev_id;
+	int ret;
+
+	sm5703 = devm_kzalloc(dev, sizeof(*sm5703), GFP_KERNEL);
+	if (!sm5703)
+		return -ENOMEM;
+
+	i2c_set_clientdata(i2c, sm5703);
+	sm5703->dev = dev;
+
+	sm5703->regmap = devm_regmap_init_i2c(i2c, &sm5703_regmap_config);
+	if (IS_ERR(sm5703->regmap))
+		return dev_err_probe(dev, PTR_ERR(sm5703->regmap),
+				     "Failed to allocate the register map\n");
+
+	sm5703->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(sm5703->reset_gpio))
+		return dev_err_probe(dev, PTR_ERR(sm5703->reset_gpio), "Cannot get reset GPIO\n");
+
+	gpiod_set_value_cansleep(sm5703->reset_gpio, 1);
+	msleep(20);
+
+	ret = regmap_read(sm5703->regmap, SM5703_DEVICE_ID, &dev_id);
+	if (ret)
+		return dev_err_probe(dev, -ENODEV, "Device not found\n");
+
+	ret = devm_mfd_add_devices(sm5703->dev, -1, sm5703_devs,
+				   ARRAY_SIZE(sm5703_devs), NULL, 0, NULL);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to add child devices\n");
+
+	return 0;
+}
+
+static const struct of_device_id sm5703_of_match[] = {
+	{ .compatible = "siliconmitus,sm5703", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, sm5703_of_match);
+
+static struct i2c_driver sm5703_driver = {
+	.driver = {
+		.name = "sm5703",
+		.of_match_table = sm5703_of_match,
+	},
+	.probe = sm5703_i2c_probe,
+};
+module_i2c_driver(sm5703_driver);
+
+MODULE_DESCRIPTION("Silicon Mitus SM5703 multi-function device driver");
+MODULE_AUTHOR("Markuss Broks <markuss.broks@gmail.com>");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/sm5703.h b/include/linux/mfd/sm5703.h
new file mode 100644
index 000000000000..c62affa0d3f1
--- /dev/null
+++ b/include/linux/mfd/sm5703.h
@@ -0,0 +1,105 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef _SM5703_H
+#define _SM5703_H
+
+struct sm5703_dev {
+	struct device *dev;
+	struct regmap *regmap;
+	struct gpio_desc *reset_gpio;
+};
+
+// Regulator-related defines
+
+#define SM5703_REG_LDO1				0x1A
+#define SM5703_REG_LDO2				0x1B
+#define SM5703_REG_LDO3				0x1C
+#define SM5703_LDO_EN				BIT(3)
+#define SM5703_LDO_VOLT_MASK			0x7
+#define SM5703_BUCK_VOLT_MASK			0x1F
+#define SM5703_REG_USBLDO12			0x1C
+#define SM5703_REG_EN_USBLDO1			BIT(6)
+#define SM5703_REG_EN_USBLDO2			BIT(7)
+#define SM5703_REG_BUCK				0x1D
+#define SM5703_REG_EN_BUCK			BIT(6)
+#define SM5703_USBLDO_MICROVOLT			4800000
+#define SM5703_VBUS_MICROVOLT			5000000
+
+// Fuel-Gauge-related defines
+
+#define SM5703_FG_REG_DEVICE_ID			0x00
+#define SM5703_FG_REG_CNTL			0x01
+#define SM5703_FG_REG_INTFG			0x02
+#define SM5703_FG_REG_INTFG_MASK		0x03
+#define SM5703_FG_REG_STATUS			0x04
+#define SM5703_FG_REG_SOC			0x05
+#define SM5703_FG_REG_OCV			0x06
+#define SM5703_FG_REG_VOLTAGE			0x07
+#define SM5703_FG_REG_CURRENT			0x08
+#define SM5703_FG_REG_TEMPERATURE		0x09
+#define SM5703_FG_REG_CURRENT_EST		0x85
+#define SM5703_FG_REG_FG_OP_STATUS		0x10
+
+// Flash LED driver-related defines
+
+#define SM5703_FLEDCNTL1			0x14
+#define SM5703_FLEDCNTL2			0x15
+#define SM5703_FLEDCNTL3			0x16
+#define SM5703_FLEDCNTL4			0x17
+#define SM5703_FLEDCNTL5			0x18
+#define SM5703_FLEDCNTL6			0x19
+
+#define SM5703_FLEDEN_MASK			0x03
+#define SM5703_FLEDEN_DISABLE			0x00
+#define SM5703_FLEDEN_MOVIE_MODE		0x01
+#define SM5703_FLEDEN_FLASH_MODE		0x02
+#define SM5703_FLEDEN_EXTERNAL			0x03
+
+#define SM5703_IFLED_MASK			0x1F
+#define SM5703_IMLED_MASK			0x1F
+
+// Charger-related, IRQ and device ID defines
+
+#define SM5703_REG_CNTL				0x0C
+#define SM5703_VBUSCNTL				0x0D
+#define SM5703_CHGCNTL1				0x0E
+#define SM5703_CHGCNTL2				0x0F
+#define SM5703_CHGCNTL3				0x10
+#define SM5703_CHGCNTL4				0x11
+#define SM5703_CHGCNTL5				0x12
+#define SM5703_CHGCNTL6				0x13
+#define SM5703_OTGCURRENTCNTL			0x60
+#define SM5703_Q3LIMITCNTL			0x66
+#define SM5703_DEVICE_ID			0x1E
+#define SM5703_OPERATION_MODE			0x07
+#define SM5703_OPERATION_MODE_MASK		0x07
+
+#define SM5703_OPERATION_MODE_SUSPEND		0x00
+#define SM5703_OPERATION_MODE_CHARGING_OFF	0x04
+#define SM5703_OPERATION_MODE_CHARGING_ON	0x05
+#define SM5703_OPERATION_MODE_FLASH_BOOST_MODE	0x06
+#define SM5703_OPERATION_MODE_USB_OTG_MODE	0x07
+
+#define SM5703_BSTOUT				0x0F
+#define SM5703_BSTOUT_MASK			0x0F
+#define SM5703_BSTOUT_SHIFT			0
+
+#define SM5703_BSTOUT_4P5			0x05
+#define SM5703_BSTOUT_5P0			0x0A
+#define SM5703_BSTOUT_5P1			0x0B
+
+#define SM5703_IRQ_STATUS1			0x08
+#define SM5703_IRQ_STATUS2			0x09
+#define SM5703_IRQ_STATUS3			0x0A
+#define SM5703_IRQ_STATUS4			0x0B
+#define SM5703_IRQ_STATUS5			0x6B
+
+#define SM5703_STATUS1_OTGFAIL			0x80
+#define SM5703_STATUS3_DONE			0x08
+#define SM5703_STATUS3_TOPOFF			0x04
+#define SM5703_STATUS3_CHGON			0x01
+#define SM5703_STATUS5_VBUSOVP			0x80
+#define SM5703_STATUS5_VBUSUVLO			0x40
+#define SM5703_STATUS5_VBUSOK			0x20
+
+#endif
-- 
2.35.1

[PATCH v5 5/5] regulator: sm5703-regulator: Add regulators support for SM5703 MFD

[Markuss Broks]

Regulators block of SM5703 controls several voltage regulators which
are used to power various components. There are 3 LDO outputs ranging
from 1.5 to 3.3V, a buck regulator ranging from 1V to 3V, two fixed
voltage LDO regulators for powering the USB devices and one high-power
fixed voltage LDO line (actually two lines) meant to power high-power
USB devices.

Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
---
 drivers/regulator/Kconfig            |   7 ++
 drivers/regulator/Makefile           |   1 +
 drivers/regulator/sm5703-regulator.c | 167 +++++++++++++++++++++++++++
 3 files changed, 175 insertions(+)
 create mode 100644 drivers/regulator/sm5703-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index a7effc57bbdb..cbe0f96ca342 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -1167,6 +1167,13 @@ config REGULATOR_SLG51000
 	  The SLG51000 is seven compact and customizable low dropout
 	  regulators.
 
+config REGULATOR_SM5703
+	tristate "Silicon Mitus SM5703 regulators"
+	depends on MFD_SM5703
+	help
+	  This driver provides support for voltage regulators of SM5703
+	  multi-function device.
+
 config REGULATOR_STM32_BOOSTER
 	tristate "STMicroelectronics STM32 BOOSTER"
 	depends on ARCH_STM32 || COMPILE_TEST
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 69b5a196acd2..8d3ee8b6d41d 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -139,6 +139,7 @@ obj-$(CONFIG_REGULATOR_S5M8767) += s5m8767.o
 obj-$(CONFIG_REGULATOR_SC2731) += sc2731-regulator.o
 obj-$(CONFIG_REGULATOR_SKY81452) += sky81452-regulator.o
 obj-$(CONFIG_REGULATOR_SLG51000) += slg51000-regulator.o
+obj-$(CONFIG_REGULATOR_SM5703) += sm5703-regulator.o
 obj-$(CONFIG_REGULATOR_STM32_BOOSTER) += stm32-booster.o
 obj-$(CONFIG_REGULATOR_STM32_VREFBUF) += stm32-vrefbuf.o
 obj-$(CONFIG_REGULATOR_STM32_PWR) += stm32-pwr.o
diff --git a/drivers/regulator/sm5703-regulator.c b/drivers/regulator/sm5703-regulator.c
new file mode 100644
index 000000000000..05ad28fc4da8
--- /dev/null
+++ b/drivers/regulator/sm5703-regulator.c
@@ -0,0 +1,167 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/mfd/sm5703.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
+
+enum sm5703_regulators {
+	SM5703_BUCK,
+	SM5703_LDO1,
+	SM5703_LDO2,
+	SM5703_LDO3,
+	SM5703_USBLDO1,
+	SM5703_USBLDO2,
+	SM5703_VBUS,
+	SM5703_MAX_REGULATORS,
+};
+
+static const int sm5703_ldo_voltagemap[] = {
+	1500000, 1800000, 2600000, 2800000, 3000000, 3300000,
+};
+
+static const int sm5703_buck_voltagemap[] = {
+	1000000, 1000000, 1000000, 1000000,
+	1000000, 1000000, 1000000, 1000000,
+	1000000, 1000000, 1000000, 1100000,
+	1200000, 1300000, 1400000, 1500000,
+	1600000, 1700000, 1800000, 1900000,
+	2000000, 2100000, 2200000, 2300000,
+	2400000, 2500000, 2600000, 2700000,
+	2800000, 2900000, 3000000, 3000000,
+};
+
+#define SM5703USBLDO(_name, _id)					\
+	[SM5703_USBLDO ## _id] = {					\
+		.name = _name,						\
+		.of_match = _name,					\
+		.regulators_node = "regulators",			\
+		.type = REGULATOR_VOLTAGE,				\
+		.id = SM5703_USBLDO ## _id,				\
+		.ops = &sm5703_regulator_ops_fixed,			\
+		.fixed_uV = SM5703_USBLDO_MICROVOLT,			\
+		.enable_reg = SM5703_REG_USBLDO12,			\
+		.enable_mask = SM5703_REG_EN_USBLDO ##_id,		\
+		.owner			= THIS_MODULE,			\
+	}
+
+#define SM5703VBUS(_name)						\
+	[SM5703_VBUS] = {						\
+		.name = _name,						\
+		.of_match = _name,					\
+		.regulators_node = "regulators",			\
+		.type = REGULATOR_VOLTAGE,				\
+		.id = SM5703_VBUS,					\
+		.ops = &sm5703_regulator_ops_fixed,			\
+		.fixed_uV = SM5703_VBUS_MICROVOLT,			\
+		.enable_reg = SM5703_REG_CNTL,				\
+		.enable_mask = SM5703_OPERATION_MODE_MASK,		\
+		.enable_val = SM5703_OPERATION_MODE_USB_OTG_MODE,	\
+		.disable_val = SM5703_OPERATION_MODE_CHARGING_ON,	\
+		.owner			= THIS_MODULE,			\
+	}
+
+#define SM5703BUCK(_name)						\
+	[SM5703_BUCK] = {						\
+		.name = _name,						\
+		.of_match = _name,					\
+		.regulators_node = "regulators",			\
+		.type = REGULATOR_VOLTAGE,				\
+		.id = SM5703_BUCK,					\
+		.ops = &sm5703_regulator_ops,				\
+		.n_voltages = ARRAY_SIZE(sm5703_buck_voltagemap),	\
+		.volt_table = sm5703_buck_voltagemap,			\
+		.vsel_reg = SM5703_REG_BUCK,				\
+		.vsel_mask = SM5703_BUCK_VOLT_MASK,			\
+		.enable_reg = SM5703_REG_BUCK,				\
+		.enable_mask = SM5703_REG_EN_BUCK,			\
+		.owner			= THIS_MODULE,			\
+	}
+
+#define SM5703LDO(_name, _id)						\
+	[SM5703_LDO ## _id] = {						\
+		.name = _name,						\
+		.of_match = _name,					\
+		.regulators_node = "regulators",			\
+		.type = REGULATOR_VOLTAGE,				\
+		.id = SM5703_LDO ## _id,				\
+		.ops = &sm5703_regulator_ops,				\
+		.n_voltages = ARRAY_SIZE(sm5703_ldo_voltagemap),	\
+		.volt_table = sm5703_ldo_voltagemap,			\
+		.vsel_reg = SM5703_REG_LDO ##_id,			\
+		.vsel_mask = SM5703_LDO_VOLT_MASK,			\
+		.enable_reg = SM5703_REG_LDO ##_id,			\
+		.enable_mask = SM5703_LDO_EN,				\
+		.owner			= THIS_MODULE,			\
+	}
+
+static const struct regulator_ops sm5703_regulator_ops = {
+	.enable			= regulator_enable_regmap,
+	.disable		= regulator_disable_regmap,
+	.is_enabled		= regulator_is_enabled_regmap,
+	.list_voltage		= regulator_list_voltage_table,
+	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
+	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
+};
+
+static const struct regulator_ops sm5703_regulator_ops_fixed = {
+	.enable			= regulator_enable_regmap,
+	.disable		= regulator_disable_regmap,
+	.is_enabled		= regulator_is_enabled_regmap,
+};
+
+static struct regulator_desc sm5703_regulators_desc[SM5703_MAX_REGULATORS] = {
+	SM5703BUCK("buck"),
+	SM5703LDO("ldo1", 1),
+	SM5703LDO("ldo2", 2),
+	SM5703LDO("ldo3", 3),
+	SM5703USBLDO("usbldo1", 1),
+	SM5703USBLDO("usbldo2", 2),
+	SM5703VBUS("vbus"),
+};
+
+static int sm5703_regulator_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct regulator_config config = { NULL, };
+	struct regulator_dev *rdev;
+	struct sm5703_dev *sm5703 = dev_get_drvdata(pdev->dev.parent);
+	int i;
+
+	config.dev = dev->parent;
+	config.regmap = sm5703->regmap;
+
+	for (i = 0; i < SM5703_MAX_REGULATORS; i++) {
+		rdev = devm_regulator_register(dev,
+					       &sm5703_regulators_desc[i],
+					       &config);
+		if (IS_ERR(rdev))
+			return dev_err_probe(dev, PTR_ERR(rdev),
+					     "Failed to register a regulator\n");
+	}
+
+	return 0;
+}
+
+static const struct platform_device_id sm5703_regulator_id[] = {
+	{ "sm5703-regulator", 0 },
+	{}
+};
+MODULE_DEVICE_TABLE(platform, sm5703_regulator_id);
+
+static struct platform_driver sm5703_regulator_driver = {
+	.driver = {
+		.name = "sm5703-regulator",
+	},
+	.probe	= sm5703_regulator_probe,
+	.id_table	= sm5703_regulator_id,
+};
+
+module_platform_driver(sm5703_regulator_driver);
+
+MODULE_DESCRIPTION("Silicon Mitus SM5703 LDO/Buck/USB regulator driver");
+MODULE_AUTHOR("Markuss Broks <markuss.broks@gmail.com>");
+MODULE_LICENSE("GPL");
-- 
2.35.1

Re: [PATCH v5 3/5] dt-bindings: mfd: Add bindings for Silicon Mitus SM5703 MFD

[Rob Herring]

On Sat, Apr 23, 2022 at 11:53:16AM +0300, Markuss Broks wrote:
> This patch adds device-tree bindings for the Silicon Mitus
> SM5703 MFD.
> 
> Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
> ---
>  .../bindings/mfd/siliconmitus,sm5703.yaml     | 84 +++++++++++++++++++
>  1 file changed, 84 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v5 2/5] dt-bindings: regulator: Add bindings for Silicon Mitus SM5703 regulators

[Rob Herring]

On Sat, 23 Apr 2022 11:53:15 +0300, Markuss Broks wrote:
> This patch adds device-tree bindings for regulators on Silicon Mitus
> SM5703 MFD.
> 
> Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
> ---
>  .../siliconmitus,sm5703-regulator.yaml        | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v5 1/5] extcon: sm5502: Clarify SM5703's i2c device ID

[Chanwoo Choi]

On 22. 4. 23. 17:53, Markuss Broks wrote:
> While SM5502 and SM5504 are purely micro USB switching
> circuits, SM5703 is a multi-function device which has multiple
> modules in it. Change the i2c_device_id of it to avoid conflict
> with MFD driver.
> 
> Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
> ---
>   drivers/extcon/extcon-sm5502.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/extcon/extcon-sm5502.c b/drivers/extcon/extcon-sm5502.c
> index 17a40c3782ee..f706f5288257 100644
> --- a/drivers/extcon/extcon-sm5502.c
> +++ b/drivers/extcon/extcon-sm5502.c
> @@ -831,7 +831,7 @@ static SIMPLE_DEV_PM_OPS(sm5502_muic_pm_ops,
>   static const struct i2c_device_id sm5502_i2c_id[] = {
>   	{ "sm5502", (kernel_ulong_t)&sm5502_data },
>   	{ "sm5504", (kernel_ulong_t)&sm5504_data },
> -	{ "sm5703", (kernel_ulong_t)&sm5502_data },
> +	{ "sm5703-muic", (kernel_ulong_t)&sm5502_data },
>   	{ }
>   };
>   MODULE_DEVICE_TABLE(i2c, sm5502_i2c_id);

Applied it because this patch depends on already merged patch
on extcon-next branch.

-- 
Best Regards,
Samsung Electronics
Chanwoo Choi

Re: (subset) [PATCH v5 0/5] Add support for Silicon Mitus SM5703 MFD

[Mark Brown]

On Sat, 23 Apr 2022 11:53:13 +0300, Markuss Broks wrote:
> This series adds support for Silicon Mitus SM5703 MFD and the
> appropriate device-tree bindings. This only adds support for the
> regulator module, leaving room for other modules implemented in
> future (code for other modules is really not ready for submission
> right now). Silicon Mitus SM5703 is used on various mobile phones,
> mostly Samsung Galaxy (J5 (2015, 2016), On7, J7 (2015, 2016) ...).
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next

Thanks!

[2/5] dt-bindings: regulator: Add bindings for Silicon Mitus SM5703 regulators
      commit: d496d68d6ba6debcc135794edb5fdc5a5b4531f1
[5/5] regulator: sm5703-regulator: Add regulators support for SM5703 MFD
      commit: e8858ba89ca377064da130d09648c99683f8bd90

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH v5 2/5] dt-bindings: regulator: Add bindings for Silicon Mitus SM5703 regulators

[Rob Herring]

On Sat, Apr 23, 2022 at 3:54 AM Markuss Broks <markuss.broks@gmail.com> wrote:
>
> This patch adds device-tree bindings for regulators on Silicon Mitus
> SM5703 MFD.
>
> Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
> ---
>  .../siliconmitus,sm5703-regulator.yaml        | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml
>
> diff --git a/Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml b/Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml
> new file mode 100644
> index 000000000000..75ff16b58000
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml
> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/regulator/siliconmitus,sm5703-regulator.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Silicon Mitus SM5703 multi function device regulators
> +
> +maintainers:
> +  - Markuss Broks <markuss.broks@gmail.com>
> +
> +description: |
> +  SM5703 regulators node should be a sub node of the SM5703 MFD node. See SM5703 MFD
> +  bindings at Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml
> +  Regulator nodes should be named as USBLDO_<number>, BUCK, VBUS, LDO_<number>.
> +  The definition for each of these nodes is defined using the standard
> +  binding for regulators at Documentation/devicetree/bindings/regulator/regulator.txt.
> +
> +properties:
> +  buck:
> +    type: object
> +    $ref: regulators.yaml#

The correct file is regulator.yaml.

That's indicated by this warning:

schemas/regulator/regulators.yaml: ignoring, error parsing file

It will fail worse than that once the example in the MFD file is added.

Lee, Mark, I've said it before, but MFD plus child schemas need to be
applied together. Maybe no one cares if dt_binding_check is broken on
the MFD tree. The primary issue for me is transient failures during
the merge window.

Rob

Re: [PATCH v5 2/5] dt-bindings: regulator: Add bindings for Silicon Mitus SM5703 regulators

[Markuss Broks]

Hi Rob,

On 4/28/22 19:36, Rob Herring wrote:
> On Sat, Apr 23, 2022 at 3:54 AM Markuss Broks <markuss.broks@gmail.com> wrote:
>> This patch adds device-tree bindings for regulators on Silicon Mitus
>> SM5703 MFD.
>>
>> Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
>> ---
>>   .../siliconmitus,sm5703-regulator.yaml        | 49 +++++++++++++++++++
>>   1 file changed, 49 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml b/Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml
>> new file mode 100644
>> index 000000000000..75ff16b58000
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml
>> @@ -0,0 +1,49 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/regulator/siliconmitus,sm5703-regulator.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Silicon Mitus SM5703 multi function device regulators
>> +
>> +maintainers:
>> +  - Markuss Broks <markuss.broks@gmail.com>
>> +
>> +description: |
>> +  SM5703 regulators node should be a sub node of the SM5703 MFD node. See SM5703 MFD
>> +  bindings at Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml
>> +  Regulator nodes should be named as USBLDO_<number>, BUCK, VBUS, LDO_<number>.
>> +  The definition for each of these nodes is defined using the standard
>> +  binding for regulators at Documentation/devicetree/bindings/regulator/regulator.txt.
>> +
>> +properties:
>> +  buck:
>> +    type: object
>> +    $ref: regulators.yaml#
> The correct file is regulator.yaml.
I was applying all the suggestions, and I thought I had somehow made a 
typo referring to a wrong file.

Do I re-send the whole series with just the s/regulators/regulator/g? A 
part of series has already been merged, do I not re-send the already 
merged patches then?

>
> That's indicated by this warning:
>
> schemas/regulator/regulators.yaml: ignoring, error parsing file
>
> It will fail worse than that once the example in the MFD file is added.
>
> Lee, Mark, I've said it before, but MFD plus child schemas need to be
> applied together. Maybe no one cares if dt_binding_check is broken on
> the MFD tree. The primary issue for me is transient failures during
> the merge window.
>
> Rob
- Markuss

Re: [PATCH v5 2/5] dt-bindings: regulator: Add bindings for Silicon Mitus SM5703 regulators

[Rob Herring]

On Thu, Apr 28, 2022 at 12:04 PM Markuss Broks <markuss.broks@gmail.com> wrote:
>
> Hi Rob,
>
> On 4/28/22 19:36, Rob Herring wrote:
> > On Sat, Apr 23, 2022 at 3:54 AM Markuss Broks <markuss.broks@gmail.com> wrote:
> >> This patch adds device-tree bindings for regulators on Silicon Mitus
> >> SM5703 MFD.
> >>
> >> Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
> >> ---
> >>   .../siliconmitus,sm5703-regulator.yaml        | 49 +++++++++++++++++++
> >>   1 file changed, 49 insertions(+)
> >>   create mode 100644 Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml b/Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml
> >> new file mode 100644
> >> index 000000000000..75ff16b58000
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml
> >> @@ -0,0 +1,49 @@
> >> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/regulator/siliconmitus,sm5703-regulator.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Silicon Mitus SM5703 multi function device regulators
> >> +
> >> +maintainers:
> >> +  - Markuss Broks <markuss.broks@gmail.com>
> >> +
> >> +description: |
> >> +  SM5703 regulators node should be a sub node of the SM5703 MFD node. See SM5703 MFD
> >> +  bindings at Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml
> >> +  Regulator nodes should be named as USBLDO_<number>, BUCK, VBUS, LDO_<number>.
> >> +  The definition for each of these nodes is defined using the standard
> >> +  binding for regulators at Documentation/devicetree/bindings/regulator/regulator.txt.
> >> +
> >> +properties:
> >> +  buck:
> >> +    type: object
> >> +    $ref: regulators.yaml#
> > The correct file is regulator.yaml.
> I was applying all the suggestions, and I thought I had somehow made a
> typo referring to a wrong file.
>
> Do I re-send the whole series with just the s/regulators/regulator/g? A
> part of series has already been merged, do I not re-send the already
> merged patches then?

Just send an incremental patch fixing the file name. If there are then
errors in the example, then you need to resend the MFD binding with
the errors fixed. Unless Lee applies v5 before you do that, then you
need an incremental fix for example.

Rob

Re: [PATCH v5 4/5] mfd: sm5703: Add support for SM5703 MFD

[Lee Jones]

On Sat, 23 Apr 2022, Markuss Broks wrote:

> Silicon Mitus SM5703 is a multi-function device, meant to be

Please avoid using the term MFD.

How is the device described in the data-sheet?

What do you mean by "meant to be"?

> used in mobile devices. It has several modules: LDO, BUCK regulators,

Modules or functions?

> charger circuit, flash LED driver, a fuel gauge for monitoring the battery
> and a MUIC USB switcher. The MUIC and fuel gauge parts are separate in that
> they use separate i2c lines to communicate with the device, while charger

"I2C"

> circuit, LED driver and regulators are on the main i2c line the device is
> controlled with.
> 
> Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
> ---
>  MAINTAINERS                |   8 +++
>  drivers/mfd/Kconfig        |  13 +++++
>  drivers/mfd/Makefile       |   1 +
>  drivers/mfd/sm5703.c       |  78 +++++++++++++++++++++++++++
>  include/linux/mfd/sm5703.h | 105 +++++++++++++++++++++++++++++++++++++
>  5 files changed, 205 insertions(+)
>  create mode 100644 drivers/mfd/sm5703.c
>  create mode 100644 include/linux/mfd/sm5703.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6157e706ed02..6125ed1a3be4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18172,6 +18172,14 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git dev
>  F:	include/linux/srcu*.h
>  F:	kernel/rcu/srcu*.c
>  
> +SM5703 MFD DRIVER
> +M:	Markuss Broks <markuss.broks@gmail.com>
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml
> +F:	Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml
> +F:	drivers/mfd/sm5703.c
> +F:	drivers/regulator/sm5703-regulator.c
> +
>  SMACK SECURITY MODULE
>  M:	Casey Schaufler <casey@schaufler-ca.com>
>  L:	linux-security-module@vger.kernel.org
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 3b59456f5545..c13a99ceae99 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1237,6 +1237,19 @@ config MFD_SM501
>  	  interface. The device may be connected by PCI or local bus with
>  	  varying functions enabled.
>  
> +config MFD_SM5703
> +	tristate "Silicon Mitus SM5703 MFD"
> +	depends on I2C
> +	depends on OF
> +	select MFD_CORE
> +	select REGMAP_I2C
> +	help
> +	  This is the core driver for the Silicon Mitus SM5703 multi-function

Please drop the MFD term, as above.

> +	  device. This device is meant to be used in phones and it has numerous

"meant to be"?

> +	  modules, including LED controller, regulators, fuel gauge, MUIC and

Either "an LED controller" or "LED controllers"

Same with "charger circuit" below.

> +	  charger circuit. It also support muxing a serial interface over USB

"supports"

What kind of serial?

> +	  data lines.
> +
>  config MFD_SM501_GPIO
>  	bool "Export GPIO via GPIO layer"
>  	depends on MFD_SM501 && GPIOLIB
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 858cacf659d6..ca8b86736a36 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -275,3 +275,4 @@ rsmu-i2c-objs			:= rsmu_core.o rsmu_i2c.o
>  rsmu-spi-objs			:= rsmu_core.o rsmu_spi.o
>  obj-$(CONFIG_MFD_RSMU_I2C)	+= rsmu-i2c.o
>  obj-$(CONFIG_MFD_RSMU_SPI)	+= rsmu-spi.o
> +obj-$(CONFIG_MFD_SM5703)	+= sm5703.o
> diff --git a/drivers/mfd/sm5703.c b/drivers/mfd/sm5703.c
> new file mode 100644
> index 000000000000..7f9838149051
> --- /dev/null
> +++ b/drivers/mfd/sm5703.c
> @@ -0,0 +1,78 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/err.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/sm5703.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
> +
> +static const struct mfd_cell sm5703_devs[] = {
> +	{ .name = "sm5703-regulator", },
> +};

Where are the rest of the child drivers?

> +static const struct regmap_config sm5703_regmap_config = {
> +	.reg_bits	= 8,
> +	.val_bits	= 8,

Tabbing looks odd.  Just use a space.

> +};
> +
> +static int sm5703_i2c_probe(struct i2c_client *i2c,
> +			    const struct i2c_device_id *id)
> +{
> +	struct sm5703_dev *sm5703;
> +	struct device *dev = &i2c->dev;
> +	unsigned int dev_id;
> +	int ret;
> +
> +	sm5703 = devm_kzalloc(dev, sizeof(*sm5703), GFP_KERNEL);
> +	if (!sm5703)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(i2c, sm5703);
> +	sm5703->dev = dev;
> +
> +	sm5703->regmap = devm_regmap_init_i2c(i2c, &sm5703_regmap_config);
> +	if (IS_ERR(sm5703->regmap))
> +		return dev_err_probe(dev, PTR_ERR(sm5703->regmap),
> +				     "Failed to allocate the register map\n");
> +
> +	sm5703->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(sm5703->reset_gpio))
> +		return dev_err_probe(dev, PTR_ERR(sm5703->reset_gpio), "Cannot get reset GPIO\n");
> +
> +	gpiod_set_value_cansleep(sm5703->reset_gpio, 1);
> +	msleep(20);
> +
> +	ret = regmap_read(sm5703->regmap, SM5703_DEVICE_ID, &dev_id);
> +	if (ret)
> +		return dev_err_probe(dev, -ENODEV, "Device not found\n");
> +
> +	ret = devm_mfd_add_devices(sm5703->dev, -1, sm5703_devs,

Not -1.  Please use the defines provided.

> +				   ARRAY_SIZE(sm5703_devs), NULL, 0, NULL);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to add child devices\n");
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id sm5703_of_match[] = {
> +	{ .compatible = "siliconmitus,sm5703", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, sm5703_of_match);
> +
> +static struct i2c_driver sm5703_driver = {
> +	.driver = {
> +		.name = "sm5703",
> +		.of_match_table = sm5703_of_match,
> +	},
> +	.probe = sm5703_i2c_probe,
> +};
> +module_i2c_driver(sm5703_driver);
> +
> +MODULE_DESCRIPTION("Silicon Mitus SM5703 multi-function device driver");

There is no such thing as an MFD.

> +MODULE_AUTHOR("Markuss Broks <markuss.broks@gmail.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/sm5703.h b/include/linux/mfd/sm5703.h
> new file mode 100644
> index 000000000000..c62affa0d3f1
> --- /dev/null
> +++ b/include/linux/mfd/sm5703.h
> @@ -0,0 +1,105 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef _SM5703_H
> +#define _SM5703_H
> +
> +struct sm5703_dev {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct gpio_desc *reset_gpio;
> +};
> +
> +// Regulator-related defines

No C++ style comments.

> +#define SM5703_REG_LDO1				0x1A

I'd drop the REG parts from these.

What are these registers called in the data-sheet?

> +#define SM5703_REG_LDO2				0x1B
> +#define SM5703_REG_LDO3				0x1C
> +#define SM5703_LDO_EN				BIT(3)

Some people like to space out the bits to differentiate them from the
register addresses.

> +#define SM5703_LDO_VOLT_MASK			0x7

Please keep the bit length consistent, so 0x07 here if these are Byte
length registers.

> +#define SM5703_BUCK_VOLT_MASK			0x1F
> +#define SM5703_REG_USBLDO12			0x1C
> +#define SM5703_REG_EN_USBLDO1			BIT(6)
> +#define SM5703_REG_EN_USBLDO2			BIT(7)
> +#define SM5703_REG_BUCK				0x1D

Please place these in order.

> +#define SM5703_REG_EN_BUCK			BIT(6)
> +#define SM5703_USBLDO_MICROVOLT			4800000
> +#define SM5703_VBUS_MICROVOLT			5000000
> +
> +// Fuel-Gauge-related defines
> +
> +#define SM5703_FG_REG_DEVICE_ID			0x00
> +#define SM5703_FG_REG_CNTL			0x01
> +#define SM5703_FG_REG_INTFG			0x02
> +#define SM5703_FG_REG_INTFG_MASK		0x03
> +#define SM5703_FG_REG_STATUS			0x04
> +#define SM5703_FG_REG_SOC			0x05
> +#define SM5703_FG_REG_OCV			0x06
> +#define SM5703_FG_REG_VOLTAGE			0x07
> +#define SM5703_FG_REG_CURRENT			0x08
> +#define SM5703_FG_REG_TEMPERATURE		0x09
> +#define SM5703_FG_REG_CURRENT_EST		0x85
> +#define SM5703_FG_REG_FG_OP_STATUS		0x10
> +
> +// Flash LED driver-related defines
> +
> +#define SM5703_FLEDCNTL1			0x14
> +#define SM5703_FLEDCNTL2			0x15
> +#define SM5703_FLEDCNTL3			0x16
> +#define SM5703_FLEDCNTL4			0x17
> +#define SM5703_FLEDCNTL5			0x18
> +#define SM5703_FLEDCNTL6			0x19
> +
> +#define SM5703_FLEDEN_MASK			0x03
> +#define SM5703_FLEDEN_DISABLE			0x00
> +#define SM5703_FLEDEN_MOVIE_MODE		0x01
> +#define SM5703_FLEDEN_FLASH_MODE		0x02
> +#define SM5703_FLEDEN_EXTERNAL			0x03
> +
> +#define SM5703_IFLED_MASK			0x1F
> +#define SM5703_IMLED_MASK			0x1F
> +
> +// Charger-related, IRQ and device ID defines
> +
> +#define SM5703_REG_CNTL				0x0C
> +#define SM5703_VBUSCNTL				0x0D
> +#define SM5703_CHGCNTL1				0x0E
> +#define SM5703_CHGCNTL2				0x0F
> +#define SM5703_CHGCNTL3				0x10
> +#define SM5703_CHGCNTL4				0x11
> +#define SM5703_CHGCNTL5				0x12
> +#define SM5703_CHGCNTL6				0x13
> +#define SM5703_OTGCURRENTCNTL			0x60
> +#define SM5703_Q3LIMITCNTL			0x66
> +#define SM5703_DEVICE_ID			0x1E
> +#define SM5703_OPERATION_MODE			0x07
> +#define SM5703_OPERATION_MODE_MASK		0x07
> +
> +#define SM5703_OPERATION_MODE_SUSPEND		0x00
> +#define SM5703_OPERATION_MODE_CHARGING_OFF	0x04
> +#define SM5703_OPERATION_MODE_CHARGING_ON	0x05
> +#define SM5703_OPERATION_MODE_FLASH_BOOST_MODE	0x06
> +#define SM5703_OPERATION_MODE_USB_OTG_MODE	0x07
> +
> +#define SM5703_BSTOUT				0x0F
> +#define SM5703_BSTOUT_MASK			0x0F
> +#define SM5703_BSTOUT_SHIFT			0
> +
> +#define SM5703_BSTOUT_4P5			0x05
> +#define SM5703_BSTOUT_5P0			0x0A
> +#define SM5703_BSTOUT_5P1			0x0B
> +
> +#define SM5703_IRQ_STATUS1			0x08
> +#define SM5703_IRQ_STATUS2			0x09
> +#define SM5703_IRQ_STATUS3			0x0A
> +#define SM5703_IRQ_STATUS4			0x0B
> +#define SM5703_IRQ_STATUS5			0x6B
> +
> +#define SM5703_STATUS1_OTGFAIL			0x80
> +#define SM5703_STATUS3_DONE			0x08
> +#define SM5703_STATUS3_TOPOFF			0x04
> +#define SM5703_STATUS3_CHGON			0x01
> +#define SM5703_STATUS5_VBUSOVP			0x80
> +#define SM5703_STATUS5_VBUSUVLO			0x40
> +#define SM5703_STATUS5_VBUSOK			0x20
> +
> +#endif

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

Re: [PATCH v5 4/5] mfd: sm5703: Add support for SM5703 MFD

[Markuss Broks]

Hi Lee,

On 6/15/22 00:32, Lee Jones wrote:
> On Sat, 23 Apr 2022, Markuss Broks wrote:
>
>> Silicon Mitus SM5703 is a multi-function device, meant to be
> Please avoid using the term MFD.
>
> How is the device described in the data-sheet?
I don't have a data-sheet for this device. The only reference I have is 
downstream Linux driver. Maybe a better way to call it would be PMIC?
>
> What do you mean by "meant to be"?
"designed to be", it appears that this part is almost exclusively used 
by Samsung in its mobile phones.
>
>> used in mobile devices. It has several modules: LDO, BUCK regulators,
> Modules or functions?

Some of "modules" are quite separate, so I decided to call it that way. 
How should it be called?

>
>> charger circuit, flash LED driver, a fuel gauge for monitoring the battery
>> and a MUIC USB switcher. The MUIC and fuel gauge parts are separate in that
>> they use separate i2c lines to communicate with the device, while charger
> "I2C"
>
>> circuit, LED driver and regulators are on the main i2c line the device is
>> controlled with.
>>
>> Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
>> ---
>>   MAINTAINERS                |   8 +++
>>   drivers/mfd/Kconfig        |  13 +++++
>>   drivers/mfd/Makefile       |   1 +
>>   drivers/mfd/sm5703.c       |  78 +++++++++++++++++++++++++++
>>   include/linux/mfd/sm5703.h | 105 +++++++++++++++++++++++++++++++++++++
>>   5 files changed, 205 insertions(+)
>>   create mode 100644 drivers/mfd/sm5703.c
>>   create mode 100644 include/linux/mfd/sm5703.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 6157e706ed02..6125ed1a3be4 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -18172,6 +18172,14 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git dev
>>   F:	include/linux/srcu*.h
>>   F:	kernel/rcu/srcu*.c
>>   
>> +SM5703 MFD DRIVER
>> +M:	Markuss Broks <markuss.broks@gmail.com>
>> +S:	Maintained
>> +F:	Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml
>> +F:	Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml
>> +F:	drivers/mfd/sm5703.c
>> +F:	drivers/regulator/sm5703-regulator.c
>> +
>>   SMACK SECURITY MODULE
>>   M:	Casey Schaufler <casey@schaufler-ca.com>
>>   L:	linux-security-module@vger.kernel.org
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index 3b59456f5545..c13a99ceae99 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -1237,6 +1237,19 @@ config MFD_SM501
>>   	  interface. The device may be connected by PCI or local bus with
>>   	  varying functions enabled.
>>   
>> +config MFD_SM5703
>> +	tristate "Silicon Mitus SM5703 MFD"
>> +	depends on I2C
>> +	depends on OF
>> +	select MFD_CORE
>> +	select REGMAP_I2C
>> +	help
>> +	  This is the core driver for the Silicon Mitus SM5703 multi-function
> Please drop the MFD term, as above.
>
>> +	  device. This device is meant to be used in phones and it has numerous
> "meant to be"?
>
>> +	  modules, including LED controller, regulators, fuel gauge, MUIC and
> Either "an LED controller" or "LED controllers"
>
> Same with "charger circuit" below.
>
>> +	  charger circuit. It also support muxing a serial interface over USB
> "supports"
>
> What kind of serial?
UART as a standard Samsung debug port interface (619K Ohm resistor 
between data lines is Samsung's debug cable).
>
>> +	  data lines.
>> +
>>   config MFD_SM501_GPIO
>>   	bool "Export GPIO via GPIO layer"
>>   	depends on MFD_SM501 && GPIOLIB
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index 858cacf659d6..ca8b86736a36 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -275,3 +275,4 @@ rsmu-i2c-objs			:= rsmu_core.o rsmu_i2c.o
>>   rsmu-spi-objs			:= rsmu_core.o rsmu_spi.o
>>   obj-$(CONFIG_MFD_RSMU_I2C)	+= rsmu-i2c.o
>>   obj-$(CONFIG_MFD_RSMU_SPI)	+= rsmu-spi.o
>> +obj-$(CONFIG_MFD_SM5703)	+= sm5703.o
>> diff --git a/drivers/mfd/sm5703.c b/drivers/mfd/sm5703.c
>> new file mode 100644
>> index 000000000000..7f9838149051
>> --- /dev/null
>> +++ b/drivers/mfd/sm5703.c
>> @@ -0,0 +1,78 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +
>> +#include <linux/err.h>
>> +#include <linux/delay.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/i2c.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/mfd/sm5703.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/regmap.h>
>> +
>> +static const struct mfd_cell sm5703_devs[] = {
>> +	{ .name = "sm5703-regulator", },
>> +};
> Where are the rest of the child drivers?
>
>> +static const struct regmap_config sm5703_regmap_config = {
>> +	.reg_bits	= 8,
>> +	.val_bits	= 8,
> Tabbing looks odd.  Just use a space.
>
>> +};
>> +
>> +static int sm5703_i2c_probe(struct i2c_client *i2c,
>> +			    const struct i2c_device_id *id)
>> +{
>> +	struct sm5703_dev *sm5703;
>> +	struct device *dev = &i2c->dev;
>> +	unsigned int dev_id;
>> +	int ret;
>> +
>> +	sm5703 = devm_kzalloc(dev, sizeof(*sm5703), GFP_KERNEL);
>> +	if (!sm5703)
>> +		return -ENOMEM;
>> +
>> +	i2c_set_clientdata(i2c, sm5703);
>> +	sm5703->dev = dev;
>> +
>> +	sm5703->regmap = devm_regmap_init_i2c(i2c, &sm5703_regmap_config);
>> +	if (IS_ERR(sm5703->regmap))
>> +		return dev_err_probe(dev, PTR_ERR(sm5703->regmap),
>> +				     "Failed to allocate the register map\n");
>> +
>> +	sm5703->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
>> +	if (IS_ERR(sm5703->reset_gpio))
>> +		return dev_err_probe(dev, PTR_ERR(sm5703->reset_gpio), "Cannot get reset GPIO\n");
>> +
>> +	gpiod_set_value_cansleep(sm5703->reset_gpio, 1);
>> +	msleep(20);
>> +
>> +	ret = regmap_read(sm5703->regmap, SM5703_DEVICE_ID, &dev_id);
>> +	if (ret)
>> +		return dev_err_probe(dev, -ENODEV, "Device not found\n");
>> +
>> +	ret = devm_mfd_add_devices(sm5703->dev, -1, sm5703_devs,
> Not -1.  Please use the defines provided.
>
>> +				   ARRAY_SIZE(sm5703_devs), NULL, 0, NULL);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to add child devices\n");
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id sm5703_of_match[] = {
>> +	{ .compatible = "siliconmitus,sm5703", },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, sm5703_of_match);
>> +
>> +static struct i2c_driver sm5703_driver = {
>> +	.driver = {
>> +		.name = "sm5703",
>> +		.of_match_table = sm5703_of_match,
>> +	},
>> +	.probe = sm5703_i2c_probe,
>> +};
>> +module_i2c_driver(sm5703_driver);
>> +
>> +MODULE_DESCRIPTION("Silicon Mitus SM5703 multi-function device driver");
> There is no such thing as an MFD.
>
>> +MODULE_AUTHOR("Markuss Broks <markuss.broks@gmail.com>");
>> +MODULE_LICENSE("GPL");
>> diff --git a/include/linux/mfd/sm5703.h b/include/linux/mfd/sm5703.h
>> new file mode 100644
>> index 000000000000..c62affa0d3f1
>> --- /dev/null
>> +++ b/include/linux/mfd/sm5703.h
>> @@ -0,0 +1,105 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +
>> +#ifndef _SM5703_H
>> +#define _SM5703_H
>> +
>> +struct sm5703_dev {
>> +	struct device *dev;
>> +	struct regmap *regmap;
>> +	struct gpio_desc *reset_gpio;
>> +};
>> +
>> +// Regulator-related defines
> No C++ style comments.
>
>> +#define SM5703_REG_LDO1				0x1A
> I'd drop the REG parts from these.
>
> What are these registers called in the data-sheet?
No data-sheet, sadly.
>
>> +#define SM5703_REG_LDO2				0x1B
>> +#define SM5703_REG_LDO3				0x1C
>> +#define SM5703_LDO_EN				BIT(3)
> Some people like to space out the bits to differentiate them from the
> register addresses.
>
>> +#define SM5703_LDO_VOLT_MASK			0x7
> Please keep the bit length consistent, so 0x07 here if these are Byte
> length registers.
>
>> +#define SM5703_BUCK_VOLT_MASK			0x1F
>> +#define SM5703_REG_USBLDO12			0x1C
>> +#define SM5703_REG_EN_USBLDO1			BIT(6)
>> +#define SM5703_REG_EN_USBLDO2			BIT(7)
>> +#define SM5703_REG_BUCK				0x1D
> Please place these in order.
>
>> +#define SM5703_REG_EN_BUCK			BIT(6)
>> +#define SM5703_USBLDO_MICROVOLT			4800000
>> +#define SM5703_VBUS_MICROVOLT			5000000
>> +
>> +// Fuel-Gauge-related defines
>> +
>> +#define SM5703_FG_REG_DEVICE_ID			0x00
>> +#define SM5703_FG_REG_CNTL			0x01
>> +#define SM5703_FG_REG_INTFG			0x02
>> +#define SM5703_FG_REG_INTFG_MASK		0x03
>> +#define SM5703_FG_REG_STATUS			0x04
>> +#define SM5703_FG_REG_SOC			0x05
>> +#define SM5703_FG_REG_OCV			0x06
>> +#define SM5703_FG_REG_VOLTAGE			0x07
>> +#define SM5703_FG_REG_CURRENT			0x08
>> +#define SM5703_FG_REG_TEMPERATURE		0x09
>> +#define SM5703_FG_REG_CURRENT_EST		0x85
>> +#define SM5703_FG_REG_FG_OP_STATUS		0x10
>> +
>> +// Flash LED driver-related defines
>> +
>> +#define SM5703_FLEDCNTL1			0x14
>> +#define SM5703_FLEDCNTL2			0x15
>> +#define SM5703_FLEDCNTL3			0x16
>> +#define SM5703_FLEDCNTL4			0x17
>> +#define SM5703_FLEDCNTL5			0x18
>> +#define SM5703_FLEDCNTL6			0x19
>> +
>> +#define SM5703_FLEDEN_MASK			0x03
>> +#define SM5703_FLEDEN_DISABLE			0x00
>> +#define SM5703_FLEDEN_MOVIE_MODE		0x01
>> +#define SM5703_FLEDEN_FLASH_MODE		0x02
>> +#define SM5703_FLEDEN_EXTERNAL			0x03
>> +
>> +#define SM5703_IFLED_MASK			0x1F
>> +#define SM5703_IMLED_MASK			0x1F
>> +
>> +// Charger-related, IRQ and device ID defines
>> +
>> +#define SM5703_REG_CNTL				0x0C
>> +#define SM5703_VBUSCNTL				0x0D
>> +#define SM5703_CHGCNTL1				0x0E
>> +#define SM5703_CHGCNTL2				0x0F
>> +#define SM5703_CHGCNTL3				0x10
>> +#define SM5703_CHGCNTL4				0x11
>> +#define SM5703_CHGCNTL5				0x12
>> +#define SM5703_CHGCNTL6				0x13
>> +#define SM5703_OTGCURRENTCNTL			0x60
>> +#define SM5703_Q3LIMITCNTL			0x66
>> +#define SM5703_DEVICE_ID			0x1E
>> +#define SM5703_OPERATION_MODE			0x07
>> +#define SM5703_OPERATION_MODE_MASK		0x07
>> +
>> +#define SM5703_OPERATION_MODE_SUSPEND		0x00
>> +#define SM5703_OPERATION_MODE_CHARGING_OFF	0x04
>> +#define SM5703_OPERATION_MODE_CHARGING_ON	0x05
>> +#define SM5703_OPERATION_MODE_FLASH_BOOST_MODE	0x06
>> +#define SM5703_OPERATION_MODE_USB_OTG_MODE	0x07
>> +
>> +#define SM5703_BSTOUT				0x0F
>> +#define SM5703_BSTOUT_MASK			0x0F
>> +#define SM5703_BSTOUT_SHIFT			0
>> +
>> +#define SM5703_BSTOUT_4P5			0x05
>> +#define SM5703_BSTOUT_5P0			0x0A
>> +#define SM5703_BSTOUT_5P1			0x0B
>> +
>> +#define SM5703_IRQ_STATUS1			0x08
>> +#define SM5703_IRQ_STATUS2			0x09
>> +#define SM5703_IRQ_STATUS3			0x0A
>> +#define SM5703_IRQ_STATUS4			0x0B
>> +#define SM5703_IRQ_STATUS5			0x6B
>> +
>> +#define SM5703_STATUS1_OTGFAIL			0x80
>> +#define SM5703_STATUS3_DONE			0x08
>> +#define SM5703_STATUS3_TOPOFF			0x04
>> +#define SM5703_STATUS3_CHGON			0x01
>> +#define SM5703_STATUS5_VBUSOVP			0x80
>> +#define SM5703_STATUS5_VBUSUVLO			0x40
>> +#define SM5703_STATUS5_VBUSOK			0x20
>> +
>> +#endif
- Markuss

Re: [PATCH v5 4/5] mfd: sm5703: Add support for SM5703 MFD

[Lee Jones]

On Sat, 09 Jul 2022, Markuss Broks wrote:

> Hi Lee,
> 
> On 6/15/22 00:32, Lee Jones wrote:
> > On Sat, 23 Apr 2022, Markuss Broks wrote:
> > 
> > > Silicon Mitus SM5703 is a multi-function device, meant to be
> > Please avoid using the term MFD.
> > 
> > How is the device described in the data-sheet?
> I don't have a data-sheet for this device. The only reference I have is
> downstream Linux driver. Maybe a better way to call it would be PMIC?

Yes, if that's what this is, PMIC sounds better.

> > What do you mean by "meant to be"?
> "designed to be", it appears that this part is almost exclusively used by
> Samsung in its mobile phones.

"designed to be" sounds better.

> > > used in mobile devices. It has several modules: LDO, BUCK regulators,
> > Modules or functions?
> 
> Some of "modules" are quite separate, so I decided to call it that way. How
> should it be called?

Functions.

> > > charger circuit, flash LED driver, a fuel gauge for monitoring the battery
> > > and a MUIC USB switcher. The MUIC and fuel gauge parts are separate in that
> > > they use separate i2c lines to communicate with the device, while charger
> > "I2C"
> > 
> > > circuit, LED driver and regulators are on the main i2c line the device is
> > > controlled with.
> > > 
> > > Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
> > > ---
> > >   MAINTAINERS                |   8 +++
> > >   drivers/mfd/Kconfig        |  13 +++++
> > >   drivers/mfd/Makefile       |   1 +
> > >   drivers/mfd/sm5703.c       |  78 +++++++++++++++++++++++++++
> > >   include/linux/mfd/sm5703.h | 105 +++++++++++++++++++++++++++++++++++++
> > >   5 files changed, 205 insertions(+)
> > >   create mode 100644 drivers/mfd/sm5703.c
> > >   create mode 100644 include/linux/mfd/sm5703.h
> > > 
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 6157e706ed02..6125ed1a3be4 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -18172,6 +18172,14 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git dev
> > >   F:	include/linux/srcu*.h
> > >   F:	kernel/rcu/srcu*.c
> > > +SM5703 MFD DRIVER
> > > +M:	Markuss Broks <markuss.broks@gmail.com>
> > > +S:	Maintained
> > > +F:	Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml
> > > +F:	Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml
> > > +F:	drivers/mfd/sm5703.c
> > > +F:	drivers/regulator/sm5703-regulator.c
> > > +
> > >   SMACK SECURITY MODULE
> > >   M:	Casey Schaufler <casey@schaufler-ca.com>
> > >   L:	linux-security-module@vger.kernel.org
> > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > index 3b59456f5545..c13a99ceae99 100644
> > > --- a/drivers/mfd/Kconfig
> > > +++ b/drivers/mfd/Kconfig
> > > @@ -1237,6 +1237,19 @@ config MFD_SM501
> > >   	  interface. The device may be connected by PCI or local bus with
> > >   	  varying functions enabled.
> > > +config MFD_SM5703
> > > +	tristate "Silicon Mitus SM5703 MFD"
> > > +	depends on I2C
> > > +	depends on OF
> > > +	select MFD_CORE
> > > +	select REGMAP_I2C
> > > +	help
> > > +	  This is the core driver for the Silicon Mitus SM5703 multi-function
> > Please drop the MFD term, as above.
> > 
> > > +	  device. This device is meant to be used in phones and it has numerous
> > "meant to be"?
> > 
> > > +	  modules, including LED controller, regulators, fuel gauge, MUIC and
> > Either "an LED controller" or "LED controllers"
> > 
> > Same with "charger circuit" below.
> > 
> > > +	  charger circuit. It also support muxing a serial interface over USB
> > "supports"
> > 
> > What kind of serial?
> UART as a standard Samsung debug port interface (619K Ohm resistor between
> data lines is Samsung's debug cable).

Probably best to state that then.

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

Re: [PATCH v5 4/5] mfd: sm5703: Add support for SM5703 MFD

[Markuss Broks]

Hi Lee,

Sorry to bother you again, but I've got additional questions while I was 
preparing the next version of the series:

On 6/15/22 00:32, Lee Jones wrote:
> On Sat, 23 Apr 2022, Markuss Broks wrote:
>
>> Silicon Mitus SM5703 is a multi-function device, meant to be
> Please avoid using the term MFD.
>
> How is the device described in the data-sheet?
>
> What do you mean by "meant to be"?
>
>> used in mobile devices. It has several modules: LDO, BUCK regulators,
> Modules or functions?
>
>> charger circuit, flash LED driver, a fuel gauge for monitoring the battery
>> and a MUIC USB switcher. The MUIC and fuel gauge parts are separate in that
>> they use separate i2c lines to communicate with the device, while charger
> "I2C"
>
>> circuit, LED driver and regulators are on the main i2c line the device is
>> controlled with.
>>
>> Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
>> ---
>>   MAINTAINERS                |   8 +++
>>   drivers/mfd/Kconfig        |  13 +++++
>>   drivers/mfd/Makefile       |   1 +
>>   drivers/mfd/sm5703.c       |  78 +++++++++++++++++++++++++++
>>   include/linux/mfd/sm5703.h | 105 +++++++++++++++++++++++++++++++++++++
>>   5 files changed, 205 insertions(+)
>>   create mode 100644 drivers/mfd/sm5703.c
>>   create mode 100644 include/linux/mfd/sm5703.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 6157e706ed02..6125ed1a3be4 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -18172,6 +18172,14 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git dev
>>   F:	include/linux/srcu*.h
>>   F:	kernel/rcu/srcu*.c
>>   
>> +SM5703 MFD DRIVER
>> +M:	Markuss Broks <markuss.broks@gmail.com>
>> +S:	Maintained
>> +F:	Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml
>> +F:	Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml
>> +F:	drivers/mfd/sm5703.c
>> +F:	drivers/regulator/sm5703-regulator.c
>> +
>>   SMACK SECURITY MODULE
>>   M:	Casey Schaufler <casey@schaufler-ca.com>
>>   L:	linux-security-module@vger.kernel.org
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index 3b59456f5545..c13a99ceae99 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -1237,6 +1237,19 @@ config MFD_SM501
>>   	  interface. The device may be connected by PCI or local bus with
>>   	  varying functions enabled.
>>   
>> +config MFD_SM5703
>> +	tristate "Silicon Mitus SM5703 MFD"
>> +	depends on I2C
>> +	depends on OF
>> +	select MFD_CORE
>> +	select REGMAP_I2C
>> +	help
>> +	  This is the core driver for the Silicon Mitus SM5703 multi-function
> Please drop the MFD term, as above.
>
>> +	  device. This device is meant to be used in phones and it has numerous
> "meant to be"?
>
>> +	  modules, including LED controller, regulators, fuel gauge, MUIC and
> Either "an LED controller" or "LED controllers"
>
> Same with "charger circuit" below.
>
>> +	  charger circuit. It also support muxing a serial interface over USB
> "supports"
>
> What kind of serial?
>
>> +	  data lines.
>> +
>>   config MFD_SM501_GPIO
>>   	bool "Export GPIO via GPIO layer"
>>   	depends on MFD_SM501 && GPIOLIB
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index 858cacf659d6..ca8b86736a36 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -275,3 +275,4 @@ rsmu-i2c-objs			:= rsmu_core.o rsmu_i2c.o
>>   rsmu-spi-objs			:= rsmu_core.o rsmu_spi.o
>>   obj-$(CONFIG_MFD_RSMU_I2C)	+= rsmu-i2c.o
>>   obj-$(CONFIG_MFD_RSMU_SPI)	+= rsmu-spi.o
>> +obj-$(CONFIG_MFD_SM5703)	+= sm5703.o
>> diff --git a/drivers/mfd/sm5703.c b/drivers/mfd/sm5703.c
>> new file mode 100644
>> index 000000000000..7f9838149051
>> --- /dev/null
>> +++ b/drivers/mfd/sm5703.c
>> @@ -0,0 +1,78 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +
>> +#include <linux/err.h>
>> +#include <linux/delay.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/i2c.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/mfd/sm5703.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/regmap.h>
>> +
>> +static const struct mfd_cell sm5703_devs[] = {
>> +	{ .name = "sm5703-regulator", },
>> +};
> Where are the rest of the child drivers?
Should those devices still be present even though there's no driver for 
them (yet) ? I have a WIP version of driver for almost every function, 
but I currently lack time to get them done.
>
>> +static const struct regmap_config sm5703_regmap_config = {
>> +	.reg_bits	= 8,
>> +	.val_bits	= 8,
> Tabbing looks odd.  Just use a space.
>
>> +};
>> +
>> +static int sm5703_i2c_probe(struct i2c_client *i2c,
>> +			    const struct i2c_device_id *id)
>> +{
>> +	struct sm5703_dev *sm5703;
>> +	struct device *dev = &i2c->dev;
>> +	unsigned int dev_id;
>> +	int ret;
>> +
>> +	sm5703 = devm_kzalloc(dev, sizeof(*sm5703), GFP_KERNEL);
>> +	if (!sm5703)
>> +		return -ENOMEM;
>> +
>> +	i2c_set_clientdata(i2c, sm5703);
>> +	sm5703->dev = dev;
>> +
>> +	sm5703->regmap = devm_regmap_init_i2c(i2c, &sm5703_regmap_config);
>> +	if (IS_ERR(sm5703->regmap))
>> +		return dev_err_probe(dev, PTR_ERR(sm5703->regmap),
>> +				     "Failed to allocate the register map\n");
>> +
>> +	sm5703->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
>> +	if (IS_ERR(sm5703->reset_gpio))
>> +		return dev_err_probe(dev, PTR_ERR(sm5703->reset_gpio), "Cannot get reset GPIO\n");
>> +
>> +	gpiod_set_value_cansleep(sm5703->reset_gpio, 1);
>> +	msleep(20);
>> +
>> +	ret = regmap_read(sm5703->regmap, SM5703_DEVICE_ID, &dev_id);
>> +	if (ret)
>> +		return dev_err_probe(dev, -ENODEV, "Device not found\n");
>> +
>> +	ret = devm_mfd_add_devices(sm5703->dev, -1, sm5703_devs,
> Not -1.  Please use the defines provided.
>
>> +				   ARRAY_SIZE(sm5703_devs), NULL, 0, NULL);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to add child devices\n");
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id sm5703_of_match[] = {
>> +	{ .compatible = "siliconmitus,sm5703", },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, sm5703_of_match);
>> +
>> +static struct i2c_driver sm5703_driver = {
>> +	.driver = {
>> +		.name = "sm5703",
>> +		.of_match_table = sm5703_of_match,
>> +	},
>> +	.probe = sm5703_i2c_probe,
>> +};
>> +module_i2c_driver(sm5703_driver);
>> +
>> +MODULE_DESCRIPTION("Silicon Mitus SM5703 multi-function device driver");
> There is no such thing as an MFD.
>
>> +MODULE_AUTHOR("Markuss Broks <markuss.broks@gmail.com>");
>> +MODULE_LICENSE("GPL");
>> diff --git a/include/linux/mfd/sm5703.h b/include/linux/mfd/sm5703.h
>> new file mode 100644
>> index 000000000000..c62affa0d3f1
>> --- /dev/null
>> +++ b/include/linux/mfd/sm5703.h
>> @@ -0,0 +1,105 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +
>> +#ifndef _SM5703_H
>> +#define _SM5703_H
>> +
>> +struct sm5703_dev {
>> +	struct device *dev;
>> +	struct regmap *regmap;
>> +	struct gpio_desc *reset_gpio;
>> +};
>> +
>> +// Regulator-related defines
> No C++ style comments.
>
>> +#define SM5703_REG_LDO1				0x1A
> I'd drop the REG parts from these.
I have no issues with that, however the already upstreamed 
sm5703-regulator driver uses those defines. If I change the define name, 
how should I make changes in that driver, would it be reasonable to send 
an additional patch together with the new MFD series?
>
> What are these registers called in the data-sheet?
>
>> +#define SM5703_REG_LDO2				0x1B
>> +#define SM5703_REG_LDO3				0x1C
>> +#define SM5703_LDO_EN				BIT(3)
> Some people like to space out the bits to differentiate them from the
> register addresses.
>
>> +#define SM5703_LDO_VOLT_MASK			0x7
> Please keep the bit length consistent, so 0x07 here if these are Byte
> length registers.
>
>> +#define SM5703_BUCK_VOLT_MASK			0x1F
>> +#define SM5703_REG_USBLDO12			0x1C
>> +#define SM5703_REG_EN_USBLDO1			BIT(6)
>> +#define SM5703_REG_EN_USBLDO2			BIT(7)
>> +#define SM5703_REG_BUCK				0x1D
> Please place these in order.
>
>> +#define SM5703_REG_EN_BUCK			BIT(6)
>> +#define SM5703_USBLDO_MICROVOLT			4800000
>> +#define SM5703_VBUS_MICROVOLT			5000000
>> +
>> +// Fuel-Gauge-related defines
>> +
>> +#define SM5703_FG_REG_DEVICE_ID			0x00
>> +#define SM5703_FG_REG_CNTL			0x01
>> +#define SM5703_FG_REG_INTFG			0x02
>> +#define SM5703_FG_REG_INTFG_MASK		0x03
>> +#define SM5703_FG_REG_STATUS			0x04
>> +#define SM5703_FG_REG_SOC			0x05
>> +#define SM5703_FG_REG_OCV			0x06
>> +#define SM5703_FG_REG_VOLTAGE			0x07
>> +#define SM5703_FG_REG_CURRENT			0x08
>> +#define SM5703_FG_REG_TEMPERATURE		0x09
>> +#define SM5703_FG_REG_CURRENT_EST		0x85
>> +#define SM5703_FG_REG_FG_OP_STATUS		0x10
>> +
>> +// Flash LED driver-related defines
>> +
>> +#define SM5703_FLEDCNTL1			0x14
>> +#define SM5703_FLEDCNTL2			0x15
>> +#define SM5703_FLEDCNTL3			0x16
>> +#define SM5703_FLEDCNTL4			0x17
>> +#define SM5703_FLEDCNTL5			0x18
>> +#define SM5703_FLEDCNTL6			0x19
>> +
>> +#define SM5703_FLEDEN_MASK			0x03
>> +#define SM5703_FLEDEN_DISABLE			0x00
>> +#define SM5703_FLEDEN_MOVIE_MODE		0x01
>> +#define SM5703_FLEDEN_FLASH_MODE		0x02
>> +#define SM5703_FLEDEN_EXTERNAL			0x03
>> +
>> +#define SM5703_IFLED_MASK			0x1F
>> +#define SM5703_IMLED_MASK			0x1F
>> +
>> +// Charger-related, IRQ and device ID defines
>> +
>> +#define SM5703_REG_CNTL				0x0C
>> +#define SM5703_VBUSCNTL				0x0D
>> +#define SM5703_CHGCNTL1				0x0E
>> +#define SM5703_CHGCNTL2				0x0F
>> +#define SM5703_CHGCNTL3				0x10
>> +#define SM5703_CHGCNTL4				0x11
>> +#define SM5703_CHGCNTL5				0x12
>> +#define SM5703_CHGCNTL6				0x13
>> +#define SM5703_OTGCURRENTCNTL			0x60
>> +#define SM5703_Q3LIMITCNTL			0x66
>> +#define SM5703_DEVICE_ID			0x1E
>> +#define SM5703_OPERATION_MODE			0x07
>> +#define SM5703_OPERATION_MODE_MASK		0x07
>> +
>> +#define SM5703_OPERATION_MODE_SUSPEND		0x00
>> +#define SM5703_OPERATION_MODE_CHARGING_OFF	0x04
>> +#define SM5703_OPERATION_MODE_CHARGING_ON	0x05
>> +#define SM5703_OPERATION_MODE_FLASH_BOOST_MODE	0x06
>> +#define SM5703_OPERATION_MODE_USB_OTG_MODE	0x07
>> +
>> +#define SM5703_BSTOUT				0x0F
>> +#define SM5703_BSTOUT_MASK			0x0F
>> +#define SM5703_BSTOUT_SHIFT			0
>> +
>> +#define SM5703_BSTOUT_4P5			0x05
>> +#define SM5703_BSTOUT_5P0			0x0A
>> +#define SM5703_BSTOUT_5P1			0x0B
>> +
>> +#define SM5703_IRQ_STATUS1			0x08
>> +#define SM5703_IRQ_STATUS2			0x09
>> +#define SM5703_IRQ_STATUS3			0x0A
>> +#define SM5703_IRQ_STATUS4			0x0B
>> +#define SM5703_IRQ_STATUS5			0x6B
>> +
>> +#define SM5703_STATUS1_OTGFAIL			0x80
>> +#define SM5703_STATUS3_DONE			0x08
>> +#define SM5703_STATUS3_TOPOFF			0x04
>> +#define SM5703_STATUS3_CHGON			0x01
>> +#define SM5703_STATUS5_VBUSOVP			0x80
>> +#define SM5703_STATUS5_VBUSUVLO			0x40
>> +#define SM5703_STATUS5_VBUSOK			0x20
>> +
>> +#endif
- Markuss

Re: [PATCH v5 4/5] mfd: sm5703: Add support for SM5703 MFD

[Lee Jones]

On Fri, 15 Jul 2022, Markuss Broks wrote:

> Hi Lee,
> 
> Sorry to bother you again, but I've got additional questions while I was
> preparing the next version of the series:
> 
> On 6/15/22 00:32, Lee Jones wrote:
> > On Sat, 23 Apr 2022, Markuss Broks wrote:
> > 
> > > Silicon Mitus SM5703 is a multi-function device, meant to be
> > Please avoid using the term MFD.
> > 
> > How is the device described in the data-sheet?
> > 
> > What do you mean by "meant to be"?
> > 
> > > used in mobile devices. It has several modules: LDO, BUCK regulators,
> > Modules or functions?
> > 
> > > charger circuit, flash LED driver, a fuel gauge for monitoring the battery
> > > and a MUIC USB switcher. The MUIC and fuel gauge parts are separate in that
> > > they use separate i2c lines to communicate with the device, while charger
> > "I2C"
> > 
> > > circuit, LED driver and regulators are on the main i2c line the device is
> > > controlled with.
> > > 
> > > Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
> > > ---
> > >   MAINTAINERS                |   8 +++
> > >   drivers/mfd/Kconfig        |  13 +++++
> > >   drivers/mfd/Makefile       |   1 +
> > >   drivers/mfd/sm5703.c       |  78 +++++++++++++++++++++++++++
> > >   include/linux/mfd/sm5703.h | 105 +++++++++++++++++++++++++++++++++++++
> > >   5 files changed, 205 insertions(+)
> > >   create mode 100644 drivers/mfd/sm5703.c
> > >   create mode 100644 include/linux/mfd/sm5703.h
> > > 
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 6157e706ed02..6125ed1a3be4 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -18172,6 +18172,14 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git dev
> > >   F:	include/linux/srcu*.h
> > >   F:	kernel/rcu/srcu*.c
> > > +SM5703 MFD DRIVER
> > > +M:	Markuss Broks <markuss.broks@gmail.com>
> > > +S:	Maintained
> > > +F:	Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml
> > > +F:	Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml
> > > +F:	drivers/mfd/sm5703.c
> > > +F:	drivers/regulator/sm5703-regulator.c
> > > +
> > >   SMACK SECURITY MODULE
> > >   M:	Casey Schaufler <casey@schaufler-ca.com>
> > >   L:	linux-security-module@vger.kernel.org
> > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > index 3b59456f5545..c13a99ceae99 100644
> > > --- a/drivers/mfd/Kconfig
> > > +++ b/drivers/mfd/Kconfig
> > > @@ -1237,6 +1237,19 @@ config MFD_SM501
> > >   	  interface. The device may be connected by PCI or local bus with
> > >   	  varying functions enabled.
> > > +config MFD_SM5703
> > > +	tristate "Silicon Mitus SM5703 MFD"
> > > +	depends on I2C
> > > +	depends on OF
> > > +	select MFD_CORE
> > > +	select REGMAP_I2C
> > > +	help
> > > +	  This is the core driver for the Silicon Mitus SM5703 multi-function
> > Please drop the MFD term, as above.
> > 
> > > +	  device. This device is meant to be used in phones and it has numerous
> > "meant to be"?
> > 
> > > +	  modules, including LED controller, regulators, fuel gauge, MUIC and
> > Either "an LED controller" or "LED controllers"
> > 
> > Same with "charger circuit" below.
> > 
> > > +	  charger circuit. It also support muxing a serial interface over USB
> > "supports"
> > 
> > What kind of serial?
> > 
> > > +	  data lines.
> > > +
> > >   config MFD_SM501_GPIO
> > >   	bool "Export GPIO via GPIO layer"
> > >   	depends on MFD_SM501 && GPIOLIB
> > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > > index 858cacf659d6..ca8b86736a36 100644
> > > --- a/drivers/mfd/Makefile
> > > +++ b/drivers/mfd/Makefile
> > > @@ -275,3 +275,4 @@ rsmu-i2c-objs			:= rsmu_core.o rsmu_i2c.o
> > >   rsmu-spi-objs			:= rsmu_core.o rsmu_spi.o
> > >   obj-$(CONFIG_MFD_RSMU_I2C)	+= rsmu-i2c.o
> > >   obj-$(CONFIG_MFD_RSMU_SPI)	+= rsmu-spi.o
> > > +obj-$(CONFIG_MFD_SM5703)	+= sm5703.o
> > > diff --git a/drivers/mfd/sm5703.c b/drivers/mfd/sm5703.c
> > > new file mode 100644
> > > index 000000000000..7f9838149051
> > > --- /dev/null
> > > +++ b/drivers/mfd/sm5703.c
> > > @@ -0,0 +1,78 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +
> > > +#include <linux/err.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/mfd/core.h>
> > > +#include <linux/mfd/sm5703.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/regmap.h>
> > > +
> > > +static const struct mfd_cell sm5703_devs[] = {
> > > +	{ .name = "sm5703-regulator", },
> > > +};
> > Where are the rest of the child drivers?
> Should those devices still be present even though there's no driver for them
> (yet) ? I have a WIP version of driver for almost every function, but I
> currently lack time to get them done.

Without them the driver-set is useless, no?

We try to refrain from applying dead code.

A lot of it has a tendency to stay that way.

[...]

> > > +++ b/include/linux/mfd/sm5703.h
> > > @@ -0,0 +1,105 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > +
> > > +#ifndef _SM5703_H
> > > +#define _SM5703_H
> > > +
> > > +struct sm5703_dev {
> > > +	struct device *dev;
> > > +	struct regmap *regmap;
> > > +	struct gpio_desc *reset_gpio;
> > > +};
> > > +
> > > +// Regulator-related defines
> > No C++ style comments.
> > 
> > > +#define SM5703_REG_LDO1				0x1A
> > I'd drop the REG parts from these.
> I have no issues with that, however the already upstreamed sm5703-regulator
> driver uses those defines. If I change the define name, how should I make
> changes in that driver, would it be reasonable to send an additional patch
> together with the new MFD series?

It would.  You could also keep them in for now.

They're not a major blocker.

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

Re: [PATCH v5 4/5] mfd: sm5703: Add support for SM5703 MFD

[Markuss Broks]

Hi Lee,

On 7/18/22 11:17, Lee Jones wrote:
> On Fri, 15 Jul 2022, Markuss Broks wrote:
>
>> Hi Lee,
>>
>> Sorry to bother you again, but I've got additional questions while I was
>> preparing the next version of the series:
>>
>> On 6/15/22 00:32, Lee Jones wrote:
>>> On Sat, 23 Apr 2022, Markuss Broks wrote:
>>>
>>>> Silicon Mitus SM5703 is a multi-function device, meant to be
>>> Please avoid using the term MFD.
>>>
>>> How is the device described in the data-sheet?
>>>
>>> What do you mean by "meant to be"?
>>>
>>>> used in mobile devices. It has several modules: LDO, BUCK regulators,
>>> Modules or functions?
>>>
>>>> charger circuit, flash LED driver, a fuel gauge for monitoring the battery
>>>> and a MUIC USB switcher. The MUIC and fuel gauge parts are separate in that
>>>> they use separate i2c lines to communicate with the device, while charger
>>> "I2C"
>>>
>>>> circuit, LED driver and regulators are on the main i2c line the device is
>>>> controlled with.
>>>>
>>>> Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
>>>> ---
>>>>    MAINTAINERS                |   8 +++
>>>>    drivers/mfd/Kconfig        |  13 +++++
>>>>    drivers/mfd/Makefile       |   1 +
>>>>    drivers/mfd/sm5703.c       |  78 +++++++++++++++++++++++++++
>>>>    include/linux/mfd/sm5703.h | 105 +++++++++++++++++++++++++++++++++++++
>>>>    5 files changed, 205 insertions(+)
>>>>    create mode 100644 drivers/mfd/sm5703.c
>>>>    create mode 100644 include/linux/mfd/sm5703.h
>>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index 6157e706ed02..6125ed1a3be4 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -18172,6 +18172,14 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git dev
>>>>    F:	include/linux/srcu*.h
>>>>    F:	kernel/rcu/srcu*.c
>>>> +SM5703 MFD DRIVER
>>>> +M:	Markuss Broks <markuss.broks@gmail.com>
>>>> +S:	Maintained
>>>> +F:	Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml
>>>> +F:	Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml
>>>> +F:	drivers/mfd/sm5703.c
>>>> +F:	drivers/regulator/sm5703-regulator.c
>>>> +
>>>>    SMACK SECURITY MODULE
>>>>    M:	Casey Schaufler <casey@schaufler-ca.com>
>>>>    L:	linux-security-module@vger.kernel.org
>>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>>>> index 3b59456f5545..c13a99ceae99 100644
>>>> --- a/drivers/mfd/Kconfig
>>>> +++ b/drivers/mfd/Kconfig
>>>> @@ -1237,6 +1237,19 @@ config MFD_SM501
>>>>    	  interface. The device may be connected by PCI or local bus with
>>>>    	  varying functions enabled.
>>>> +config MFD_SM5703
>>>> +	tristate "Silicon Mitus SM5703 MFD"
>>>> +	depends on I2C
>>>> +	depends on OF
>>>> +	select MFD_CORE
>>>> +	select REGMAP_I2C
>>>> +	help
>>>> +	  This is the core driver for the Silicon Mitus SM5703 multi-function
>>> Please drop the MFD term, as above.
>>>
>>>> +	  device. This device is meant to be used in phones and it has numerous
>>> "meant to be"?
>>>
>>>> +	  modules, including LED controller, regulators, fuel gauge, MUIC and
>>> Either "an LED controller" or "LED controllers"
>>>
>>> Same with "charger circuit" below.
>>>
>>>> +	  charger circuit. It also support muxing a serial interface over USB
>>> "supports"
>>>
>>> What kind of serial?
>>>
>>>> +	  data lines.
>>>> +
>>>>    config MFD_SM501_GPIO
>>>>    	bool "Export GPIO via GPIO layer"
>>>>    	depends on MFD_SM501 && GPIOLIB
>>>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>>>> index 858cacf659d6..ca8b86736a36 100644
>>>> --- a/drivers/mfd/Makefile
>>>> +++ b/drivers/mfd/Makefile
>>>> @@ -275,3 +275,4 @@ rsmu-i2c-objs			:= rsmu_core.o rsmu_i2c.o
>>>>    rsmu-spi-objs			:= rsmu_core.o rsmu_spi.o
>>>>    obj-$(CONFIG_MFD_RSMU_I2C)	+= rsmu-i2c.o
>>>>    obj-$(CONFIG_MFD_RSMU_SPI)	+= rsmu-spi.o
>>>> +obj-$(CONFIG_MFD_SM5703)	+= sm5703.o
>>>> diff --git a/drivers/mfd/sm5703.c b/drivers/mfd/sm5703.c
>>>> new file mode 100644
>>>> index 000000000000..7f9838149051
>>>> --- /dev/null
>>>> +++ b/drivers/mfd/sm5703.c
>>>> @@ -0,0 +1,78 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +
>>>> +#include <linux/err.h>
>>>> +#include <linux/delay.h>
>>>> +#include <linux/gpio/consumer.h>
>>>> +#include <linux/i2c.h>
>>>> +#include <linux/mfd/core.h>
>>>> +#include <linux/mfd/sm5703.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/of_device.h>
>>>> +#include <linux/regmap.h>
>>>> +
>>>> +static const struct mfd_cell sm5703_devs[] = {
>>>> +	{ .name = "sm5703-regulator", },
>>>> +};
>>> Where are the rest of the child drivers?
>> Should those devices still be present even though there's no driver for them
>> (yet) ? I have a WIP version of driver for almost every function, but I
>> currently lack time to get them done.
> Without them the driver-set is useless, no?
>
> We try to refrain from applying dead code.
>
> A lot of it has a tendency to stay that way.
Well, in my opinion, having just the regulator driver is already useful 
enough: my board (Samsung Galaxy J5 (2015) ) uses one of LDO outputs for 
powering the touchscreen. Touchscreen is kind of vital functionality for 
a phone so I decided to upstream parts that are necessary for it first 
and finish up other stuff later. It's not the only board that uses 
SM5703's regulators for supplying all different kinds of hardware, either.
>
> [...]
>
>>>> +++ b/include/linux/mfd/sm5703.h
>>>> @@ -0,0 +1,105 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>>> +
>>>> +#ifndef _SM5703_H
>>>> +#define _SM5703_H
>>>> +
>>>> +struct sm5703_dev {
>>>> +	struct device *dev;
>>>> +	struct regmap *regmap;
>>>> +	struct gpio_desc *reset_gpio;
>>>> +};
>>>> +
>>>> +// Regulator-related defines
>>> No C++ style comments.
>>>
>>>> +#define SM5703_REG_LDO1				0x1A
>>> I'd drop the REG parts from these.
>> I have no issues with that, however the already upstreamed sm5703-regulator
>> driver uses those defines. If I change the define name, how should I make
>> changes in that driver, would it be reasonable to send an additional patch
>> together with the new MFD series?
> It would.  You could also keep them in for now.
>
> They're not a major blocker.
>
- Markuss

Re: [PATCH v5 4/5] mfd: sm5703: Add support for SM5703 MFD

[Lee Jones]

On Tue, 19 Jul 2022, Markuss Broks wrote:

> Hi Lee,
> 
> On 7/18/22 11:17, Lee Jones wrote:
> > On Fri, 15 Jul 2022, Markuss Broks wrote:
> > 
> > > Hi Lee,
> > > 
> > > Sorry to bother you again, but I've got additional questions while I was
> > > preparing the next version of the series:
> > > 
> > > On 6/15/22 00:32, Lee Jones wrote:
> > > > On Sat, 23 Apr 2022, Markuss Broks wrote:
> > > > 
> > > > > Silicon Mitus SM5703 is a multi-function device, meant to be
> > > > Please avoid using the term MFD.
> > > > 
> > > > How is the device described in the data-sheet?
> > > > 
> > > > What do you mean by "meant to be"?
> > > > 
> > > > > used in mobile devices. It has several modules: LDO, BUCK regulators,
> > > > Modules or functions?
> > > > 
> > > > > charger circuit, flash LED driver, a fuel gauge for monitoring the battery
> > > > > and a MUIC USB switcher. The MUIC and fuel gauge parts are separate in that
> > > > > they use separate i2c lines to communicate with the device, while charger
> > > > "I2C"
> > > > 
> > > > > circuit, LED driver and regulators are on the main i2c line the device is
> > > > > controlled with.
> > > > > 
> > > > > Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
> > > > > ---
> > > > >    MAINTAINERS                |   8 +++
> > > > >    drivers/mfd/Kconfig        |  13 +++++
> > > > >    drivers/mfd/Makefile       |   1 +
> > > > >    drivers/mfd/sm5703.c       |  78 +++++++++++++++++++++++++++
> > > > >    include/linux/mfd/sm5703.h | 105 +++++++++++++++++++++++++++++++++++++
> > > > >    5 files changed, 205 insertions(+)
> > > > >    create mode 100644 drivers/mfd/sm5703.c
> > > > >    create mode 100644 include/linux/mfd/sm5703.h
> > > > > 
> > > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > > index 6157e706ed02..6125ed1a3be4 100644
> > > > > --- a/MAINTAINERS
> > > > > +++ b/MAINTAINERS
> > > > > @@ -18172,6 +18172,14 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git dev
> > > > >    F:	include/linux/srcu*.h
> > > > >    F:	kernel/rcu/srcu*.c
> > > > > +SM5703 MFD DRIVER
> > > > > +M:	Markuss Broks <markuss.broks@gmail.com>
> > > > > +S:	Maintained
> > > > > +F:	Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml
> > > > > +F:	Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml
> > > > > +F:	drivers/mfd/sm5703.c
> > > > > +F:	drivers/regulator/sm5703-regulator.c
> > > > > +
> > > > >    SMACK SECURITY MODULE
> > > > >    M:	Casey Schaufler <casey@schaufler-ca.com>
> > > > >    L:	linux-security-module@vger.kernel.org
> > > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > > > index 3b59456f5545..c13a99ceae99 100644
> > > > > --- a/drivers/mfd/Kconfig
> > > > > +++ b/drivers/mfd/Kconfig
> > > > > @@ -1237,6 +1237,19 @@ config MFD_SM501
> > > > >    	  interface. The device may be connected by PCI or local bus with
> > > > >    	  varying functions enabled.
> > > > > +config MFD_SM5703
> > > > > +	tristate "Silicon Mitus SM5703 MFD"
> > > > > +	depends on I2C
> > > > > +	depends on OF
> > > > > +	select MFD_CORE
> > > > > +	select REGMAP_I2C
> > > > > +	help
> > > > > +	  This is the core driver for the Silicon Mitus SM5703 multi-function
> > > > Please drop the MFD term, as above.
> > > > 
> > > > > +	  device. This device is meant to be used in phones and it has numerous
> > > > "meant to be"?
> > > > 
> > > > > +	  modules, including LED controller, regulators, fuel gauge, MUIC and
> > > > Either "an LED controller" or "LED controllers"
> > > > 
> > > > Same with "charger circuit" below.
> > > > 
> > > > > +	  charger circuit. It also support muxing a serial interface over USB
> > > > "supports"
> > > > 
> > > > What kind of serial?
> > > > 
> > > > > +	  data lines.
> > > > > +
> > > > >    config MFD_SM501_GPIO
> > > > >    	bool "Export GPIO via GPIO layer"
> > > > >    	depends on MFD_SM501 && GPIOLIB
> > > > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > > > > index 858cacf659d6..ca8b86736a36 100644
> > > > > --- a/drivers/mfd/Makefile
> > > > > +++ b/drivers/mfd/Makefile
> > > > > @@ -275,3 +275,4 @@ rsmu-i2c-objs			:= rsmu_core.o rsmu_i2c.o
> > > > >    rsmu-spi-objs			:= rsmu_core.o rsmu_spi.o
> > > > >    obj-$(CONFIG_MFD_RSMU_I2C)	+= rsmu-i2c.o
> > > > >    obj-$(CONFIG_MFD_RSMU_SPI)	+= rsmu-spi.o
> > > > > +obj-$(CONFIG_MFD_SM5703)	+= sm5703.o
> > > > > diff --git a/drivers/mfd/sm5703.c b/drivers/mfd/sm5703.c
> > > > > new file mode 100644
> > > > > index 000000000000..7f9838149051
> > > > > --- /dev/null
> > > > > +++ b/drivers/mfd/sm5703.c
> > > > > @@ -0,0 +1,78 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > > +
> > > > > +#include <linux/err.h>
> > > > > +#include <linux/delay.h>
> > > > > +#include <linux/gpio/consumer.h>
> > > > > +#include <linux/i2c.h>
> > > > > +#include <linux/mfd/core.h>
> > > > > +#include <linux/mfd/sm5703.h>
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/of_device.h>
> > > > > +#include <linux/regmap.h>
> > > > > +
> > > > > +static const struct mfd_cell sm5703_devs[] = {
> > > > > +	{ .name = "sm5703-regulator", },
> > > > > +};
> > > > Where are the rest of the child drivers?
> > > Should those devices still be present even though there's no driver for them
> > > (yet) ? I have a WIP version of driver for almost every function, but I
> > > currently lack time to get them done.
> > Without them the driver-set is useless, no?
> > 
> > We try to refrain from applying dead code.
> > 
> > A lot of it has a tendency to stay that way.
> Well, in my opinion, having just the regulator driver is already useful
> enough: my board (Samsung Galaxy J5 (2015) ) uses one of LDO outputs for
> powering the touchscreen. Touchscreen is kind of vital functionality for a
> phone so I decided to upstream parts that are necessary for it first and
> finish up other stuff later. It's not the only board that uses SM5703's
> regulators for supplying all different kinds of hardware, either.

Upstreaming functionality which is useful on its own is fine, but that 
doesn't tick all of the boxes to justify an MFD.  This is a lot of
code which is hard to justify if it only registers a Regulator driver.

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

Re: [PATCH v5 4/5] mfd: sm5703: Add support for SM5703 MFD

[Markuss Broks]

Hi Lee,

On 7/20/22 11:22, Lee Jones wrote:
> On Tue, 19 Jul 2022, Markuss Broks wrote:
>
>> Hi Lee,
>>
>> On 7/18/22 11:17, Lee Jones wrote:
>>> On Fri, 15 Jul 2022, Markuss Broks wrote:
>>>
>>>> Hi Lee,
>>>>
>>>> Sorry to bother you again, but I've got additional questions while I was
>>>> preparing the next version of the series:
>>>>
>>>> On 6/15/22 00:32, Lee Jones wrote:
>>>>> On Sat, 23 Apr 2022, Markuss Broks wrote:
>>>>>
>>>>>> Silicon Mitus SM5703 is a multi-function device, meant to be
>>>>> Please avoid using the term MFD.
>>>>>
>>>>> How is the device described in the data-sheet?
>>>>>
>>>>> What do you mean by "meant to be"?
>>>>>
>>>>>> used in mobile devices. It has several modules: LDO, BUCK regulators,
>>>>> Modules or functions?
>>>>>
>>>>>> charger circuit, flash LED driver, a fuel gauge for monitoring the battery
>>>>>> and a MUIC USB switcher. The MUIC and fuel gauge parts are separate in that
>>>>>> they use separate i2c lines to communicate with the device, while charger
>>>>> "I2C"
>>>>>
>>>>>> circuit, LED driver and regulators are on the main i2c line the device is
>>>>>> controlled with.
>>>>>>
>>>>>> Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
>>>>>> ---
>>>>>>     MAINTAINERS                |   8 +++
>>>>>>     drivers/mfd/Kconfig        |  13 +++++
>>>>>>     drivers/mfd/Makefile       |   1 +
>>>>>>     drivers/mfd/sm5703.c       |  78 +++++++++++++++++++++++++++
>>>>>>     include/linux/mfd/sm5703.h | 105 +++++++++++++++++++++++++++++++++++++
>>>>>>     5 files changed, 205 insertions(+)
>>>>>>     create mode 100644 drivers/mfd/sm5703.c
>>>>>>     create mode 100644 include/linux/mfd/sm5703.h
>>>>>>
>>>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>>>> index 6157e706ed02..6125ed1a3be4 100644
>>>>>> --- a/MAINTAINERS
>>>>>> +++ b/MAINTAINERS
>>>>>> @@ -18172,6 +18172,14 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git dev
>>>>>>     F:	include/linux/srcu*.h
>>>>>>     F:	kernel/rcu/srcu*.c
>>>>>> +SM5703 MFD DRIVER
>>>>>> +M:	Markuss Broks <markuss.broks@gmail.com>
>>>>>> +S:	Maintained
>>>>>> +F:	Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml
>>>>>> +F:	Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml
>>>>>> +F:	drivers/mfd/sm5703.c
>>>>>> +F:	drivers/regulator/sm5703-regulator.c
>>>>>> +
>>>>>>     SMACK SECURITY MODULE
>>>>>>     M:	Casey Schaufler <casey@schaufler-ca.com>
>>>>>>     L:	linux-security-module@vger.kernel.org
>>>>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>>>>>> index 3b59456f5545..c13a99ceae99 100644
>>>>>> --- a/drivers/mfd/Kconfig
>>>>>> +++ b/drivers/mfd/Kconfig
>>>>>> @@ -1237,6 +1237,19 @@ config MFD_SM501
>>>>>>     	  interface. The device may be connected by PCI or local bus with
>>>>>>     	  varying functions enabled.
>>>>>> +config MFD_SM5703
>>>>>> +	tristate "Silicon Mitus SM5703 MFD"
>>>>>> +	depends on I2C
>>>>>> +	depends on OF
>>>>>> +	select MFD_CORE
>>>>>> +	select REGMAP_I2C
>>>>>> +	help
>>>>>> +	  This is the core driver for the Silicon Mitus SM5703 multi-function
>>>>> Please drop the MFD term, as above.
>>>>>
>>>>>> +	  device. This device is meant to be used in phones and it has numerous
>>>>> "meant to be"?
>>>>>
>>>>>> +	  modules, including LED controller, regulators, fuel gauge, MUIC and
>>>>> Either "an LED controller" or "LED controllers"
>>>>>
>>>>> Same with "charger circuit" below.
>>>>>
>>>>>> +	  charger circuit. It also support muxing a serial interface over USB
>>>>> "supports"
>>>>>
>>>>> What kind of serial?
>>>>>
>>>>>> +	  data lines.
>>>>>> +
>>>>>>     config MFD_SM501_GPIO
>>>>>>     	bool "Export GPIO via GPIO layer"
>>>>>>     	depends on MFD_SM501 && GPIOLIB
>>>>>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>>>>>> index 858cacf659d6..ca8b86736a36 100644
>>>>>> --- a/drivers/mfd/Makefile
>>>>>> +++ b/drivers/mfd/Makefile
>>>>>> @@ -275,3 +275,4 @@ rsmu-i2c-objs			:= rsmu_core.o rsmu_i2c.o
>>>>>>     rsmu-spi-objs			:= rsmu_core.o rsmu_spi.o
>>>>>>     obj-$(CONFIG_MFD_RSMU_I2C)	+= rsmu-i2c.o
>>>>>>     obj-$(CONFIG_MFD_RSMU_SPI)	+= rsmu-spi.o
>>>>>> +obj-$(CONFIG_MFD_SM5703)	+= sm5703.o
>>>>>> diff --git a/drivers/mfd/sm5703.c b/drivers/mfd/sm5703.c
>>>>>> new file mode 100644
>>>>>> index 000000000000..7f9838149051
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/mfd/sm5703.c
>>>>>> @@ -0,0 +1,78 @@
>>>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>>>> +
>>>>>> +#include <linux/err.h>
>>>>>> +#include <linux/delay.h>
>>>>>> +#include <linux/gpio/consumer.h>
>>>>>> +#include <linux/i2c.h>
>>>>>> +#include <linux/mfd/core.h>
>>>>>> +#include <linux/mfd/sm5703.h>
>>>>>> +#include <linux/module.h>
>>>>>> +#include <linux/of_device.h>
>>>>>> +#include <linux/regmap.h>
>>>>>> +
>>>>>> +static const struct mfd_cell sm5703_devs[] = {
>>>>>> +	{ .name = "sm5703-regulator", },
>>>>>> +};
>>>>> Where are the rest of the child drivers?
>>>> Should those devices still be present even though there's no driver for them
>>>> (yet) ? I have a WIP version of driver for almost every function, but I
>>>> currently lack time to get them done.
>>> Without them the driver-set is useless, no?
>>>
>>> We try to refrain from applying dead code.
>>>
>>> A lot of it has a tendency to stay that way.
>> Well, in my opinion, having just the regulator driver is already useful
>> enough: my board (Samsung Galaxy J5 (2015) ) uses one of LDO outputs for
>> powering the touchscreen. Touchscreen is kind of vital functionality for a
>> phone so I decided to upstream parts that are necessary for it first and
>> finish up other stuff later. It's not the only board that uses SM5703's
>> regulators for supplying all different kinds of hardware, either.
> Upstreaming functionality which is useful on its own is fine, but that
> doesn't tick all of the boxes to justify an MFD.  This is a lot of
> code which is hard to justify if it only registers a Regulator driver.
Do you think I should hold on this series until I have other things 
done? Alternatively, I could make the regulator driver standalone, 
dedicated, but then when I'd add other functionality I'd have to redo it 
and add the MFD driver back, that I believe would be quite annoying from 
maintainers' and sanity perspective. The other functions left on the 
main control I2C are also not really "vital" to device's functionality 
(Flash LED and charger), so the regulator function makes the most sense 
to be available first, which was my motivation behind upstreaming that 
first.
>
- Markuss

Re: [PATCH v5 4/5] mfd: sm5703: Add support for SM5703 MFD

[Lee Jones]

On Wed, 20 Jul 2022, Markuss Broks wrote:

> Hi Lee,
> 
> On 7/20/22 11:22, Lee Jones wrote:
> > On Tue, 19 Jul 2022, Markuss Broks wrote:
> > 
> > > Hi Lee,
> > > 
> > > On 7/18/22 11:17, Lee Jones wrote:
> > > > On Fri, 15 Jul 2022, Markuss Broks wrote:
> > > > 
> > > > > Hi Lee,
> > > > > 
> > > > > Sorry to bother you again, but I've got additional questions while I was
> > > > > preparing the next version of the series:
> > > > > 
> > > > > On 6/15/22 00:32, Lee Jones wrote:
> > > > > > On Sat, 23 Apr 2022, Markuss Broks wrote:
> > > > > > 
> > > > > > > Silicon Mitus SM5703 is a multi-function device, meant to be
> > > > > > Please avoid using the term MFD.
> > > > > > 
> > > > > > How is the device described in the data-sheet?
> > > > > > 
> > > > > > What do you mean by "meant to be"?
> > > > > > 
> > > > > > > used in mobile devices. It has several modules: LDO, BUCK regulators,
> > > > > > Modules or functions?
> > > > > > 
> > > > > > > charger circuit, flash LED driver, a fuel gauge for monitoring the battery
> > > > > > > and a MUIC USB switcher. The MUIC and fuel gauge parts are separate in that
> > > > > > > they use separate i2c lines to communicate with the device, while charger
> > > > > > "I2C"
> > > > > > 
> > > > > > > circuit, LED driver and regulators are on the main i2c line the device is
> > > > > > > controlled with.
> > > > > > > 
> > > > > > > Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
> > > > > > > ---
> > > > > > >     MAINTAINERS                |   8 +++
> > > > > > >     drivers/mfd/Kconfig        |  13 +++++
> > > > > > >     drivers/mfd/Makefile       |   1 +
> > > > > > >     drivers/mfd/sm5703.c       |  78 +++++++++++++++++++++++++++
> > > > > > >     include/linux/mfd/sm5703.h | 105 +++++++++++++++++++++++++++++++++++++
> > > > > > >     5 files changed, 205 insertions(+)
> > > > > > >     create mode 100644 drivers/mfd/sm5703.c
> > > > > > >     create mode 100644 include/linux/mfd/sm5703.h
> > > > > > > 
> > > > > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > > > > index 6157e706ed02..6125ed1a3be4 100644
> > > > > > > --- a/MAINTAINERS
> > > > > > > +++ b/MAINTAINERS
> > > > > > > @@ -18172,6 +18172,14 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git dev
> > > > > > >     F:	include/linux/srcu*.h
> > > > > > >     F:	kernel/rcu/srcu*.c
> > > > > > > +SM5703 MFD DRIVER
> > > > > > > +M:	Markuss Broks <markuss.broks@gmail.com>
> > > > > > > +S:	Maintained
> > > > > > > +F:	Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml
> > > > > > > +F:	Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml
> > > > > > > +F:	drivers/mfd/sm5703.c
> > > > > > > +F:	drivers/regulator/sm5703-regulator.c
> > > > > > > +
> > > > > > >     SMACK SECURITY MODULE
> > > > > > >     M:	Casey Schaufler <casey@schaufler-ca.com>
> > > > > > >     L:	linux-security-module@vger.kernel.org
> > > > > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > > > > > index 3b59456f5545..c13a99ceae99 100644
> > > > > > > --- a/drivers/mfd/Kconfig
> > > > > > > +++ b/drivers/mfd/Kconfig
> > > > > > > @@ -1237,6 +1237,19 @@ config MFD_SM501
> > > > > > >     	  interface. The device may be connected by PCI or local bus with
> > > > > > >     	  varying functions enabled.
> > > > > > > +config MFD_SM5703
> > > > > > > +	tristate "Silicon Mitus SM5703 MFD"
> > > > > > > +	depends on I2C
> > > > > > > +	depends on OF
> > > > > > > +	select MFD_CORE
> > > > > > > +	select REGMAP_I2C
> > > > > > > +	help
> > > > > > > +	  This is the core driver for the Silicon Mitus SM5703 multi-function
> > > > > > Please drop the MFD term, as above.
> > > > > > 
> > > > > > > +	  device. This device is meant to be used in phones and it has numerous
> > > > > > "meant to be"?
> > > > > > 
> > > > > > > +	  modules, including LED controller, regulators, fuel gauge, MUIC and
> > > > > > Either "an LED controller" or "LED controllers"
> > > > > > 
> > > > > > Same with "charger circuit" below.
> > > > > > 
> > > > > > > +	  charger circuit. It also support muxing a serial interface over USB
> > > > > > "supports"
> > > > > > 
> > > > > > What kind of serial?
> > > > > > 
> > > > > > > +	  data lines.
> > > > > > > +
> > > > > > >     config MFD_SM501_GPIO
> > > > > > >     	bool "Export GPIO via GPIO layer"
> > > > > > >     	depends on MFD_SM501 && GPIOLIB
> > > > > > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > > > > > > index 858cacf659d6..ca8b86736a36 100644
> > > > > > > --- a/drivers/mfd/Makefile
> > > > > > > +++ b/drivers/mfd/Makefile
> > > > > > > @@ -275,3 +275,4 @@ rsmu-i2c-objs			:= rsmu_core.o rsmu_i2c.o
> > > > > > >     rsmu-spi-objs			:= rsmu_core.o rsmu_spi.o
> > > > > > >     obj-$(CONFIG_MFD_RSMU_I2C)	+= rsmu-i2c.o
> > > > > > >     obj-$(CONFIG_MFD_RSMU_SPI)	+= rsmu-spi.o
> > > > > > > +obj-$(CONFIG_MFD_SM5703)	+= sm5703.o
> > > > > > > diff --git a/drivers/mfd/sm5703.c b/drivers/mfd/sm5703.c
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..7f9838149051
> > > > > > > --- /dev/null
> > > > > > > +++ b/drivers/mfd/sm5703.c
> > > > > > > @@ -0,0 +1,78 @@
> > > > > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > > > > +
> > > > > > > +#include <linux/err.h>
> > > > > > > +#include <linux/delay.h>
> > > > > > > +#include <linux/gpio/consumer.h>
> > > > > > > +#include <linux/i2c.h>
> > > > > > > +#include <linux/mfd/core.h>
> > > > > > > +#include <linux/mfd/sm5703.h>
> > > > > > > +#include <linux/module.h>
> > > > > > > +#include <linux/of_device.h>
> > > > > > > +#include <linux/regmap.h>
> > > > > > > +
> > > > > > > +static const struct mfd_cell sm5703_devs[] = {
> > > > > > > +	{ .name = "sm5703-regulator", },
> > > > > > > +};
> > > > > > Where are the rest of the child drivers?
> > > > > Should those devices still be present even though there's no driver for them
> > > > > (yet) ? I have a WIP version of driver for almost every function, but I
> > > > > currently lack time to get them done.
> > > > Without them the driver-set is useless, no?
> > > > 
> > > > We try to refrain from applying dead code.
> > > > 
> > > > A lot of it has a tendency to stay that way.
> > > Well, in my opinion, having just the regulator driver is already useful
> > > enough: my board (Samsung Galaxy J5 (2015) ) uses one of LDO outputs for
> > > powering the touchscreen. Touchscreen is kind of vital functionality for a
> > > phone so I decided to upstream parts that are necessary for it first and
> > > finish up other stuff later. It's not the only board that uses SM5703's
> > > regulators for supplying all different kinds of hardware, either.

> > Upstreaming functionality which is useful on its own is fine, but that
> > doesn't tick all of the boxes to justify an MFD.  This is a lot of
> > code which is hard to justify if it only registers a Regulator driver.

> Do you think I should hold on this series until I have other things done?
> Alternatively, I could make the regulator driver standalone, dedicated, but
> then when I'd add other functionality I'd have to redo it and add the MFD
> driver back, that I believe would be quite annoying from maintainers' and
> sanity perspective. The other functions left on the main control I2C are
> also not really "vital" to device's functionality (Flash LED and charger),
> so the regulator function makes the most sense to be available first, which
> was my motivation behind upstreaming that first.

What's the reason for this being an MFD in the first place?

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

Re: [PATCH v5 4/5] mfd: sm5703: Add support for SM5703 MFD

[Markuss Broks]

Hi Lee,

On 7/20/22 16:29, Lee Jones wrote:
> On Wed, 20 Jul 2022, Markuss Broks wrote:
>
>> Hi Lee,
>>
>> On 7/20/22 11:22, Lee Jones wrote:
>>> On Tue, 19 Jul 2022, Markuss Broks wrote:
>>>
>>>> Hi Lee,
>>>>
>>>> On 7/18/22 11:17, Lee Jones wrote:
>>>>> On Fri, 15 Jul 2022, Markuss Broks wrote:
>>>>>
>>>>>> Hi Lee,
>>>>>>
>>>>>> Sorry to bother you again, but I've got additional questions while I was
>>>>>> preparing the next version of the series:
>>>>>>
>>>>>> On 6/15/22 00:32, Lee Jones wrote:
>>>>>>> On Sat, 23 Apr 2022, Markuss Broks wrote:
>>>>>>>
>>>>>>>> Silicon Mitus SM5703 is a multi-function device, meant to be
>>>>>>> Please avoid using the term MFD.
>>>>>>>
>>>>>>> How is the device described in the data-sheet?
>>>>>>>
>>>>>>> What do you mean by "meant to be"?
>>>>>>>
>>>>>>>> used in mobile devices. It has several modules: LDO, BUCK regulators,
>>>>>>> Modules or functions?
>>>>>>>
>>>>>>>> charger circuit, flash LED driver, a fuel gauge for monitoring the battery
>>>>>>>> and a MUIC USB switcher. The MUIC and fuel gauge parts are separate in that
>>>>>>>> they use separate i2c lines to communicate with the device, while charger
>>>>>>> "I2C"
>>>>>>>
>>>>>>>> circuit, LED driver and regulators are on the main i2c line the device is
>>>>>>>> controlled with.
>>>>>>>>
>>>>>>>> Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
>>>>>>>> ---
>>>>>>>>      MAINTAINERS                |   8 +++
>>>>>>>>      drivers/mfd/Kconfig        |  13 +++++
>>>>>>>>      drivers/mfd/Makefile       |   1 +
>>>>>>>>      drivers/mfd/sm5703.c       |  78 +++++++++++++++++++++++++++
>>>>>>>>      include/linux/mfd/sm5703.h | 105 +++++++++++++++++++++++++++++++++++++
>>>>>>>>      5 files changed, 205 insertions(+)
>>>>>>>>      create mode 100644 drivers/mfd/sm5703.c
>>>>>>>>      create mode 100644 include/linux/mfd/sm5703.h
>>>>>>>>
>>>>>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>>>>>> index 6157e706ed02..6125ed1a3be4 100644
>>>>>>>> --- a/MAINTAINERS
>>>>>>>> +++ b/MAINTAINERS
>>>>>>>> @@ -18172,6 +18172,14 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git dev
>>>>>>>>      F:	include/linux/srcu*.h
>>>>>>>>      F:	kernel/rcu/srcu*.c
>>>>>>>> +SM5703 MFD DRIVER
>>>>>>>> +M:	Markuss Broks <markuss.broks@gmail.com>
>>>>>>>> +S:	Maintained
>>>>>>>> +F:	Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml
>>>>>>>> +F:	Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml
>>>>>>>> +F:	drivers/mfd/sm5703.c
>>>>>>>> +F:	drivers/regulator/sm5703-regulator.c
>>>>>>>> +
>>>>>>>>      SMACK SECURITY MODULE
>>>>>>>>      M:	Casey Schaufler <casey@schaufler-ca.com>
>>>>>>>>      L:	linux-security-module@vger.kernel.org
>>>>>>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>>>>>>>> index 3b59456f5545..c13a99ceae99 100644
>>>>>>>> --- a/drivers/mfd/Kconfig
>>>>>>>> +++ b/drivers/mfd/Kconfig
>>>>>>>> @@ -1237,6 +1237,19 @@ config MFD_SM501
>>>>>>>>      	  interface. The device may be connected by PCI or local bus with
>>>>>>>>      	  varying functions enabled.
>>>>>>>> +config MFD_SM5703
>>>>>>>> +	tristate "Silicon Mitus SM5703 MFD"
>>>>>>>> +	depends on I2C
>>>>>>>> +	depends on OF
>>>>>>>> +	select MFD_CORE
>>>>>>>> +	select REGMAP_I2C
>>>>>>>> +	help
>>>>>>>> +	  This is the core driver for the Silicon Mitus SM5703 multi-function
>>>>>>> Please drop the MFD term, as above.
>>>>>>>
>>>>>>>> +	  device. This device is meant to be used in phones and it has numerous
>>>>>>> "meant to be"?
>>>>>>>
>>>>>>>> +	  modules, including LED controller, regulators, fuel gauge, MUIC and
>>>>>>> Either "an LED controller" or "LED controllers"
>>>>>>>
>>>>>>> Same with "charger circuit" below.
>>>>>>>
>>>>>>>> +	  charger circuit. It also support muxing a serial interface over USB
>>>>>>> "supports"
>>>>>>>
>>>>>>> What kind of serial?
>>>>>>>
>>>>>>>> +	  data lines.
>>>>>>>> +
>>>>>>>>      config MFD_SM501_GPIO
>>>>>>>>      	bool "Export GPIO via GPIO layer"
>>>>>>>>      	depends on MFD_SM501 && GPIOLIB
>>>>>>>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>>>>>>>> index 858cacf659d6..ca8b86736a36 100644
>>>>>>>> --- a/drivers/mfd/Makefile
>>>>>>>> +++ b/drivers/mfd/Makefile
>>>>>>>> @@ -275,3 +275,4 @@ rsmu-i2c-objs			:= rsmu_core.o rsmu_i2c.o
>>>>>>>>      rsmu-spi-objs			:= rsmu_core.o rsmu_spi.o
>>>>>>>>      obj-$(CONFIG_MFD_RSMU_I2C)	+= rsmu-i2c.o
>>>>>>>>      obj-$(CONFIG_MFD_RSMU_SPI)	+= rsmu-spi.o
>>>>>>>> +obj-$(CONFIG_MFD_SM5703)	+= sm5703.o
>>>>>>>> diff --git a/drivers/mfd/sm5703.c b/drivers/mfd/sm5703.c
>>>>>>>> new file mode 100644
>>>>>>>> index 000000000000..7f9838149051
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/drivers/mfd/sm5703.c
>>>>>>>> @@ -0,0 +1,78 @@
>>>>>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>>>>>> +
>>>>>>>> +#include <linux/err.h>
>>>>>>>> +#include <linux/delay.h>
>>>>>>>> +#include <linux/gpio/consumer.h>
>>>>>>>> +#include <linux/i2c.h>
>>>>>>>> +#include <linux/mfd/core.h>
>>>>>>>> +#include <linux/mfd/sm5703.h>
>>>>>>>> +#include <linux/module.h>
>>>>>>>> +#include <linux/of_device.h>
>>>>>>>> +#include <linux/regmap.h>
>>>>>>>> +
>>>>>>>> +static const struct mfd_cell sm5703_devs[] = {
>>>>>>>> +	{ .name = "sm5703-regulator", },
>>>>>>>> +};
>>>>>>> Where are the rest of the child drivers?
>>>>>> Should those devices still be present even though there's no driver for them
>>>>>> (yet) ? I have a WIP version of driver for almost every function, but I
>>>>>> currently lack time to get them done.
>>>>> Without them the driver-set is useless, no?
>>>>>
>>>>> We try to refrain from applying dead code.
>>>>>
>>>>> A lot of it has a tendency to stay that way.
>>>> Well, in my opinion, having just the regulator driver is already useful
>>>> enough: my board (Samsung Galaxy J5 (2015) ) uses one of LDO outputs for
>>>> powering the touchscreen. Touchscreen is kind of vital functionality for a
>>>> phone so I decided to upstream parts that are necessary for it first and
>>>> finish up other stuff later. It's not the only board that uses SM5703's
>>>> regulators for supplying all different kinds of hardware, either.
>>> Upstreaming functionality which is useful on its own is fine, but that
>>> doesn't tick all of the boxes to justify an MFD.  This is a lot of
>>> code which is hard to justify if it only registers a Regulator driver.
>> Do you think I should hold on this series until I have other things done?
>> Alternatively, I could make the regulator driver standalone, dedicated, but
>> then when I'd add other functionality I'd have to redo it and add the MFD
>> driver back, that I believe would be quite annoying from maintainers' and
>> sanity perspective. The other functions left on the main control I2C are
>> also not really "vital" to device's functionality (Flash LED and charger),
>> so the regulator function makes the most sense to be available first, which
>> was my motivation behind upstreaming that first.
> What's the reason for this being an MFD in the first place?
Well, the "main" I2C interface is shared between three functions: 
regulators, Flash LED and charger. I call this one the "main" one 
because the device is controlled with it: you can select internal clock 
rate, enable or disable the PMIC and do other things that more of apply 
to the whole PMIC, not just the separate functions (as is the case with 
fuel gauge and MUIC I2C) . That's the reason for this being an MFD: 
those three functions share one I2C interface. While they might not be 
implemented at the moment, this places a foundation for adding future 
support.
- Markuss

Re: [PATCH v5 4/5] mfd: sm5703: Add support for SM5703 MFD

[Lee Jones]

On Wed, 20 Jul 2022, Markuss Broks wrote:

> On 7/20/22 16:29, Lee Jones wrote:
> > On Wed, 20 Jul 2022, Markuss Broks wrote:
> > > > > > > > > +static const struct mfd_cell sm5703_devs[] = {
> > > > > > > > > +	{ .name = "sm5703-regulator", },
> > > > > > > > > +};
> > > > > > > > Where are the rest of the child drivers?
> > > > > > > Should those devices still be present even though there's no driver for them
> > > > > > > (yet) ? I have a WIP version of driver for almost every function, but I
> > > > > > > currently lack time to get them done.
> > > > > > Without them the driver-set is useless, no?
> > > > > > 
> > > > > > We try to refrain from applying dead code.
> > > > > > 
> > > > > > A lot of it has a tendency to stay that way.
> > > > > Well, in my opinion, having just the regulator driver is already useful
> > > > > enough: my board (Samsung Galaxy J5 (2015) ) uses one of LDO outputs for
> > > > > powering the touchscreen. Touchscreen is kind of vital functionality for a
> > > > > phone so I decided to upstream parts that are necessary for it first and
> > > > > finish up other stuff later. It's not the only board that uses SM5703's
> > > > > regulators for supplying all different kinds of hardware, either.
> > > > Upstreaming functionality which is useful on its own is fine, but that
> > > > doesn't tick all of the boxes to justify an MFD.  This is a lot of
> > > > code which is hard to justify if it only registers a Regulator driver.
> > > Do you think I should hold on this series until I have other things done?
> > > Alternatively, I could make the regulator driver standalone, dedicated, but
> > > then when I'd add other functionality I'd have to redo it and add the MFD
> > > driver back, that I believe would be quite annoying from maintainers' and
> > > sanity perspective. The other functions left on the main control I2C are
> > > also not really "vital" to device's functionality (Flash LED and charger),
> > > so the regulator function makes the most sense to be available first, which
> > > was my motivation behind upstreaming that first.

Can you configure your mailer to stop removing the space between
messages please?

> > What's the reason for this being an MFD in the first place?

> Well, the "main" I2C interface is shared between three functions:
> regulators, Flash LED and charger. I call this one the "main" one because
> the device is controlled with it: you can select internal clock rate, enable
> or disable the PMIC and do other things that more of apply to the whole
> PMIC, not just the separate functions (as is the case with fuel gauge and
> MUIC I2C) . That's the reason for this being an MFD: those three functions
> share one I2C interface. While they might not be implemented at the moment,
> this places a foundation for adding future support.

Okay, so the functions cannot be controlled separately?  That's fine.

For acceptance into MFD, you need to enable more than one function.

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