[PATCH 1/6] devicetree/bindings: Initial commit of silergy,sy7636a.yaml

[PATCH 1/6] devicetree/bindings: Initial commit of silergy,sy7636a.yaml

[Alistair Francis]

Initial support for the Silergy SY7636A Power Management chip
driver.

Signed-off-by: Alistair Francis <alistair@alistair23.me>
---
 .../bindings/mfd/silergy,sy7636a.yaml         | 37 +++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml

diff --git a/Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml b/Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml
new file mode 100644
index 000000000000..37541a7fcc5d
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml
@@ -0,0 +1,37 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/regulator/silergy,sy7636a.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: silergy sy7636a PMIC
+
+maintainers:
+  - Alistair Francis <alistair@alistair23.me>
+
+properties:
+  compatible:
+    enum:
+      - silergy,sy7636a
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        regulator@60 {
+          compatible = "silergy,sy7636a";
+          reg = <0x60>;
+        };
+    };
+
+...
-- 
2.29.2

[PATCH 2/6] mfd: Initial commit of sy7636a

[Alistair Francis]

Initial support for the Silergy SY7636A Power Management chip
driver.

Signed-off-by: Alistair Francis <alistair@alistair23.me>
---
 drivers/mfd/Kconfig         |  10 ++
 drivers/mfd/Makefile        |   2 +
 drivers/mfd/sy7636a.c       | 252 ++++++++++++++++++++++++++++++++++++
 include/linux/mfd/sy7636a.h |  50 +++++++
 4 files changed, 314 insertions(+)
 create mode 100644 drivers/mfd/sy7636a.c
 create mode 100644 include/linux/mfd/sy7636a.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index bdfce7b15621..c8c62d92433c 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1360,6 +1360,16 @@ config MFD_SYSCON
 	  Select this option to enable accessing system control registers
 	  via regmap.
 
+config MFD_SY7636A
+	tristate "Silergy SY7636A Power Management chip driver"
+	select MFD_CORE
+	select REGMAP_I2C
+	select REGMAP_IRQ
+	depends on I2C=y
+	help
+	  Select this option to enable support for the Silergy SY7636A
+	  Power Management chip driver.
+
 config MFD_DAVINCI_VOICECODEC
 	tristate
 	select MFD_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 14fdb188af02..1fa1e635f506 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -265,6 +265,8 @@ obj-$(CONFIG_MFD_ROHM_BD718XX)	+= rohm-bd718x7.o
 obj-$(CONFIG_MFD_STMFX) 	+= stmfx.o
 obj-$(CONFIG_MFD_KHADAS_MCU) 	+= khadas-mcu.o
 
+obj-$(CONFIG_MFD_SY7636A)	+= sy7636a.o
+
 obj-$(CONFIG_SGI_MFD_IOC3)	+= ioc3.o
 obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)	+= simple-mfd-i2c.o
 obj-$(CONFIG_MFD_INTEL_M10_BMC)   += intel-m10-bmc.o
diff --git a/drivers/mfd/sy7636a.c b/drivers/mfd/sy7636a.c
new file mode 100644
index 000000000000..39aac965d854
--- /dev/null
+++ b/drivers/mfd/sy7636a.c
@@ -0,0 +1,252 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * MFD driver for SY7636A chip
+ *
+ * Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/
+ *
+ * Author: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * Based on the lp87565 driver by Keerthy <j-keerthy@ti.com>
+ */
+
+#include <linux/interrupt.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+#include <linux/sysfs.h>
+
+#include <linux/mfd/sy7636a.h>
+
+static const struct regmap_config sy7636a_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+static const struct mfd_cell sy7636a_cells[] = {
+	{ .name = "sy7636a-regulator", },
+	{ .name = "sy7636a-temperature", },
+	{ .name = "sy7636a-thermal", },
+};
+
+static const struct of_device_id of_sy7636a_match_table[] = {
+	{ .compatible = "silergy,sy7636a", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, of_sy7636a_match_table);
+
+static const char * const states[] = {
+	"no fault event",
+	"UVP at VP rail",
+	"UVP at VN rail",
+	"UVP at VPOS rail",
+	"UVP at VNEG rail",
+	"UVP at VDDH rail",
+	"UVP at VEE rail",
+	"SCP at VP rail",
+	"SCP at VN rail",
+	"SCP at VPOS rail",
+	"SCP at VNEG rail",
+	"SCP at VDDH rail",
+	"SCP at VEE rail",
+	"SCP at V COM rail",
+	"UVLO",
+	"Thermal shutdown",
+};
+
+int get_vcom_voltage_mv(struct regmap *regmap)
+{
+	int ret;
+	unsigned int val, val_h;
+
+	ret = regmap_read(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_L, &val);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_H, &val_h);
+	if (ret)
+		return ret;
+
+	val |= (val_h << 8);
+
+	return (val & 0x1FF) * 10;
+}
+
+int set_vcom_voltage_mv(struct regmap *regmap, unsigned int vcom)
+{
+	int ret;
+	unsigned int val;
+
+	if (vcom < 0 || vcom > 5000)
+		return -EINVAL;
+
+	val = (unsigned int)(vcom / 10) & 0x1ff;
+
+	ret = regmap_write(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_L, val);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_H, val >> 8);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static ssize_t state_show(struct device *dev, struct device_attribute *attr,
+		char *buf)
+{
+	int ret;
+	unsigned int val;
+	struct sy7636a *sy7636a = dev_get_drvdata(dev);
+
+	ret = regmap_read(sy7636a->regmap, SY7636A_REG_FAULT_FLAG, &val);
+	if (ret) {
+		dev_err(sy7636a->dev, "Failed to read from device\n");
+		return ret;
+	}
+
+	val = val >> 1;
+
+	if (val >= ARRAY_SIZE(states)) {
+		dev_err(sy7636a->dev, "Unexpected value read from device: %u\n", val);
+		return -EINVAL;
+	}
+
+	return snprintf(buf, PAGE_SIZE, "%s\n", states[val]);
+}
+static DEVICE_ATTR(state, 0444, state_show, NULL);
+
+static ssize_t power_good_show(struct device *dev, struct device_attribute *attr,
+		char *buf)
+{
+	int ret;
+	unsigned int val;
+	struct sy7636a *sy7636a = dev_get_drvdata(dev);
+
+	ret = regmap_read(sy7636a->regmap, SY7636A_REG_FAULT_FLAG, &val);
+	if (ret) {
+		dev_err(sy7636a->dev, "Failed to read from device\n");
+		return ret;
+	}
+
+	val &= 0x01;
+
+	return snprintf(buf, PAGE_SIZE, "%s\n", val ? "ON" : "OFF");
+}
+static DEVICE_ATTR(power_good, 0444, power_good_show, NULL);
+
+static ssize_t vcom_show(struct device *dev, struct device_attribute *attr,
+		char *buf)
+{
+	int ret;
+	struct sy7636a *sy7636a = dev_get_drvdata(dev);
+
+	ret = get_vcom_voltage_mv(sy7636a->regmap);
+	if (ret < 0)
+		return ret;
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", -ret);
+}
+
+static ssize_t vcom_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	int ret;
+	int vcom;
+	struct sy7636a *sy7636a = dev_get_drvdata(dev);
+
+	ret = kstrtoint(buf, 0, &vcom);
+	if (ret)
+		return ret;
+
+	if (vcom > 0 || vcom < -5000)
+		return -EINVAL;
+
+	ret = set_vcom_voltage_mv(sy7636a->regmap, (unsigned int)(-vcom));
+	if (ret)
+		return ret;
+
+	return count;
+}
+static DEVICE_ATTR(vcom, 0644, vcom_show, vcom_store);
+
+static struct attribute *sy7636a_sysfs_attrs[] = {
+	&dev_attr_state.attr,
+	&dev_attr_power_good.attr,
+	&dev_attr_vcom.attr,
+	NULL,
+};
+
+static const struct attribute_group sy7636a_sysfs_attr_group = {
+	.attrs = sy7636a_sysfs_attrs,
+};
+
+static int sy7636a_probe(struct i2c_client *client,
+			 const struct i2c_device_id *ids)
+{
+	struct sy7636a *sy7636a;
+	int ret;
+
+	sy7636a = devm_kzalloc(&client->dev, sizeof(struct sy7636a), GFP_KERNEL);
+	if (sy7636a == NULL)
+		return -ENOMEM;
+
+	sy7636a->dev = &client->dev;
+
+	sy7636a->regmap = devm_regmap_init_i2c(client, &sy7636a_regmap_config);
+	if (IS_ERR(sy7636a->regmap)) {
+		ret = PTR_ERR(sy7636a->regmap);
+		dev_err(sy7636a->dev,
+			"Failed to initialize register map: %d\n", ret);
+		return ret;
+	}
+
+	i2c_set_clientdata(client, sy7636a);
+
+	ret = sysfs_create_group(&client->dev.kobj, &sy7636a_sysfs_attr_group);
+	if (ret) {
+		dev_err(sy7636a->dev, "Failed to create sysfs attributes\n");
+		return ret;
+	}
+
+	ret = devm_mfd_add_devices(sy7636a->dev, PLATFORM_DEVID_AUTO,
+					sy7636a_cells, ARRAY_SIZE(sy7636a_cells),
+					NULL, 0, NULL);
+	if (ret) {
+		dev_err(sy7636a->dev, "Failed to add mfd devices\n");
+		sysfs_remove_group(&client->dev.kobj, &sy7636a_sysfs_attr_group);
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct i2c_device_id sy7636a_id_table[] = {
+	{ "sy7636a", 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(i2c, sy7636a_id_table);
+
+static struct i2c_driver sy7636a_driver = {
+	.driver	= {
+		.name	= "sy7636a",
+		.of_match_table = of_sy7636a_match_table,
+	},
+	.probe = sy7636a_probe,
+	.id_table = sy7636a_id_table,
+};
+module_i2c_driver(sy7636a_driver);
+
+MODULE_AUTHOR("Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>");
+MODULE_DESCRIPTION("Silergy SY7636A Multi-Function Device Driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/sy7636a.h b/include/linux/mfd/sy7636a.h
new file mode 100644
index 000000000000..642789c4d0a9
--- /dev/null
+++ b/include/linux/mfd/sy7636a.h
@@ -0,0 +1,50 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Functions to access SY3686A power management chip.
+ *
+ * Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __LINUX_MFD_SY7636A_H
+#define __LINUX_MFD_SY7636A_H
+
+#include <linux/i2c.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/regmap.h>
+
+#define SY7636A_REG_OPERATION_MODE_CRL 0x00
+#define SY7636A_OPERATION_MODE_CRL_VCOMCTL (1 << 6)
+#define SY7636A_OPERATION_MODE_CRL_ONOFF (1 << 7)
+#define SY7636A_REG_VCOM_ADJUST_CTRL_L 0x01
+#define SY7636A_REG_VCOM_ADJUST_CTRL_H 0x02
+#define SY7636A_REG_VCOM_ADJUST_CTRL_MASK 0x01ff
+#define SY7636A_REG_VLDO_VOLTAGE_ADJULST_CTRL 0x03
+#define SY7636A_REG_POWER_ON_DELAY_TIME 0x06
+#define SY7636A_REG_FAULT_FLAG 0x07
+#define SY7636A_FAULT_FLAG_PG (1 << 0)
+#define SY7636A_REG_TERMISTOR_READOUT 0x08
+
+#define SY7636A_REG_MAX 0x08
+
+struct sy7636a {
+	struct device *dev;
+	struct regmap *regmap;
+	unsigned int vcom;
+	struct gpio_desc *pgood_gpio;
+	struct mutex reglock;
+};
+
+int get_vcom_voltage_mv(struct regmap *regmap);
+int set_vcom_voltage_mv(struct regmap *regmap, unsigned int vcom);
+
+#endif /* __LINUX_MFD_SY7636A_H */
-- 
2.29.2

[PATCH 3/6] devicetree/bindings: Initial commit of silergy,sy7636a-regulator.yaml

[Alistair Francis]

Initial support for the Silergy SY7636A-regulator Power Management chip
driver.

Signed-off-by: Alistair Francis <alistair@alistair23.me>
---
 .../bindings/regulator/silergy,sy7636a.yaml   | 39 +++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/silergy,sy7636a.yaml

diff --git a/Documentation/devicetree/bindings/regulator/silergy,sy7636a.yaml b/Documentation/devicetree/bindings/regulator/silergy,sy7636a.yaml
new file mode 100644
index 000000000000..d88c4ab1be02
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/silergy,sy7636a.yaml
@@ -0,0 +1,39 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/regulator/silergy,sy7636a-regulator.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: silergy sy7636a PMIC
+
+maintainers:
+  - Alistair Francis <alistair@alistair23.me>
+
+allOf:
+  - $ref: regulator.yaml#
+
+properties:
+  compatible:
+    enum:
+      - sy7636a-regulator
+
+required:
+  - compatible
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        regulator@60 {
+          compatible = "sy7636a-regulator";
+          reg_epdpmic: vcom {
+            regulator-name = "vcom";
+            regulator-boot-on;
+          };
+        };
+    };
+
+...
-- 
2.29.2

[PATCH 4/6] regulator: Initial commit of sy7636a

[Alistair Francis]

Initial support for the Silergy SY7636A-regulator Power Management chip
driver.

Signed-off-by: Alistair Francis <alistair@alistair23.me>
---
 drivers/regulator/Kconfig             |   6 +
 drivers/regulator/Makefile            |   1 +
 drivers/regulator/sy7636a-regulator.c | 233 ++++++++++++++++++++++++++
 3 files changed, 240 insertions(+)
 create mode 100644 drivers/regulator/sy7636a-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 5abdd29fb9f3..76510a0db4f9 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -1097,6 +1097,12 @@ config REGULATOR_STW481X_VMMC
 	  This driver supports the internal VMMC regulator in the STw481x
 	  PMIC chips.
 
+config REGULATOR_SY7636A
+	tristate "Silergy SY7636A voltage regulator"
+	depends on MFD_SY7636A
+	help
+	  This driver supports Silergy SY3686A voltage regulator.
+
 config REGULATOR_SY8106A
 	tristate "Silergy SY8106A regulator"
 	depends on I2C && (OF || COMPILE_TEST)
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 680e539f6579..fe05347cbf84 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -131,6 +131,7 @@ obj-$(CONFIG_REGULATOR_STM32_VREFBUF) += stm32-vrefbuf.o
 obj-$(CONFIG_REGULATOR_STM32_PWR) += stm32-pwr.o
 obj-$(CONFIG_REGULATOR_STPMIC1) += stpmic1_regulator.o
 obj-$(CONFIG_REGULATOR_STW481X_VMMC) += stw481x-vmmc.o
+obj-$(CONFIG_REGULATOR_SY7636A) += sy7636a-regulator.o
 obj-$(CONFIG_REGULATOR_SY8106A) += sy8106a-regulator.o
 obj-$(CONFIG_REGULATOR_SY8824X) += sy8824x.o
 obj-$(CONFIG_REGULATOR_SY8827N) += sy8827n.o
diff --git a/drivers/regulator/sy7636a-regulator.c b/drivers/regulator/sy7636a-regulator.c
new file mode 100644
index 000000000000..a639830298d6
--- /dev/null
+++ b/drivers/regulator/sy7636a-regulator.c
@@ -0,0 +1,233 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Functions to access SY3686A power management chip voltages
+ *
+ * Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/
+ *
+ * Author: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/gpio/consumer.h>
+
+#include <linux/mfd/sy7636a.h>
+
+static int get_vcom_voltage_op(struct regulator_dev *rdev)
+{
+	int ret = get_vcom_voltage_mv(rdev->regmap);
+
+	if (ret < 0)
+		return ret;
+
+	return ret * 1000;
+}
+
+static int disable_regulator(struct regulator_dev *rdev)
+{
+	struct sy7636a *sy7636a = dev_get_drvdata(rdev->dev.parent);
+	int ret = 0;
+
+	mutex_lock(&sy7636a->reglock);
+	ret = regulator_disable_regmap(rdev);
+	usleep_range(30000, 35000);
+	mutex_unlock(&sy7636a->reglock);
+
+	return ret;
+}
+
+static int sy7636a_regulator_is_enabled(struct regulator_dev *rdev)
+{
+	struct sy7636a *sy7636a = dev_get_drvdata(rdev->dev.parent);
+	int ret;
+
+	mutex_lock(&sy7636a->reglock);
+	ret = regulator_is_enabled_regmap(rdev);
+	mutex_unlock(&sy7636a->reglock);
+
+	return ret;
+}
+
+static int enable_regulator_pgood(struct regulator_dev *rdev)
+{
+	struct sy7636a *sy7636a = dev_get_drvdata(rdev->dev.parent);
+	int pwr_good = 0;
+	int ret = 0;
+	unsigned long t0, t1;
+	const unsigned int wait_time = 500;
+	unsigned int wait_cnt;
+
+	t0 = jiffies;
+
+	mutex_lock(&sy7636a->reglock);
+
+	ret = regulator_enable_regmap(rdev);
+	if (ret)
+		goto finish;
+
+	for (wait_cnt = 0; wait_cnt < wait_time; wait_cnt++) {
+		pwr_good = gpiod_get_value_cansleep(sy7636a->pgood_gpio);
+		if (pwr_good < 0) {
+			dev_err(&rdev->dev, "Failed to read pgood gpio: %d\n", pwr_good);
+			ret = pwr_good;
+			goto finish;
+		} else if (pwr_good)
+			break;
+
+		usleep_range(1000, 1500);
+	}
+
+	t1 = jiffies;
+
+	if (!pwr_good) {
+		dev_err(&rdev->dev, "Power good signal timeout after %u ms\n",
+				jiffies_to_msecs(t1 - t0));
+		ret = -ETIME;
+		goto finish;
+	}
+
+	dev_dbg(&rdev->dev, "Power good OK (took %u ms, %u waits)\n",
+		jiffies_to_msecs(t1 - t0),
+		wait_cnt);
+
+finish:
+	mutex_unlock(&sy7636a->reglock);
+	return ret;
+}
+
+static const struct regulator_ops sy7636a_vcom_volt_ops = {
+	.get_voltage = get_vcom_voltage_op,
+	.enable = enable_regulator_pgood,
+	.disable = disable_regulator,
+	.is_enabled = sy7636a_regulator_is_enabled,
+};
+
+struct regulator_desc desc = {
+	.name = "vcom",
+	.id = 0,
+	.ops = &sy7636a_vcom_volt_ops,
+	.type = REGULATOR_VOLTAGE,
+	.owner = THIS_MODULE,
+	.enable_reg = SY7636A_REG_OPERATION_MODE_CRL,
+	.enable_mask = SY7636A_OPERATION_MODE_CRL_ONOFF,
+	.regulators_node = of_match_ptr("regulators"),
+	.of_match = of_match_ptr("vcom"),
+};
+
+static int sy7636a_regulator_init(struct sy7636a *sy7636a)
+{
+	return regmap_write(sy7636a->regmap,
+				SY7636A_REG_POWER_ON_DELAY_TIME,
+				0x0);
+}
+
+static int sy7636a_regulator_suspend(struct device *dev)
+{
+	int ret;
+	struct sy7636a *sy7636a = dev_get_drvdata(dev->parent);
+
+	ret = get_vcom_voltage_mv(sy7636a->regmap);
+
+	if (ret > 0)
+		sy7636a->vcom = (unsigned int)ret;
+
+	return 0;
+}
+
+static int sy7636a_regulator_resume(struct device *dev)
+{
+	int ret;
+
+	struct sy7636a *sy7636a = dev_get_drvdata(dev->parent);
+
+	if (!sy7636a->vcom || sy7636a->vcom > 5000) {
+		dev_warn(dev, "Vcom value invalid, and thus not restored\n");
+	} else {
+		ret = set_vcom_voltage_mv(sy7636a->regmap, sy7636a->vcom);
+		if (ret)
+			return ret;
+	}
+
+	return sy7636a_regulator_init(sy7636a);
+}
+
+static int sy7636a_regulator_probe(struct platform_device *pdev)
+{
+	struct sy7636a *sy7636a = dev_get_drvdata(pdev->dev.parent);
+	struct regulator_config config = { };
+	struct regulator_dev *rdev;
+	struct gpio_desc *gdp;
+	int ret;
+
+	if (!sy7636a)
+		return -EPROBE_DEFER;
+
+	platform_set_drvdata(pdev, sy7636a);
+
+	mutex_init(&sy7636a->reglock);
+
+	gdp = devm_gpiod_get(sy7636a->dev, "epd-pwr-good", GPIOD_IN);
+	if (IS_ERR(gdp)) {
+		dev_err(sy7636a->dev, "Power good GPIO fault %ld\n", PTR_ERR(gdp));
+		return PTR_ERR(gdp);
+	}
+
+	sy7636a->pgood_gpio = gdp;
+	dev_info(sy7636a->dev,
+		"Power good GPIO registered (gpio# %d)\n",
+		desc_to_gpio(sy7636a->pgood_gpio));
+
+	ret = sy7636a_regulator_init(sy7636a);
+	if (ret) {
+		dev_err(sy7636a->dev, "Failed to initialize regulator: %d\n", ret);
+		return ret;
+	}
+
+	config.dev = &pdev->dev;
+	config.dev->of_node = sy7636a->dev->of_node;
+	config.driver_data = sy7636a;
+	config.regmap = sy7636a->regmap;
+
+	rdev = devm_regulator_register(&pdev->dev, &desc, &config);
+	if (IS_ERR(rdev)) {
+		dev_err(sy7636a->dev, "Failed to register %s regulator\n",
+			pdev->name);
+		return PTR_ERR(rdev);
+	}
+
+	return 0;
+}
+
+static const struct platform_device_id sy7636a_regulator_id_table[] = {
+	{ "sy7636a-regulator", },
+};
+MODULE_DEVICE_TABLE(platform, sy7636a_regulator_id_table);
+
+static const struct dev_pm_ops sy7636a_pm_ops = {
+	.suspend = sy7636a_regulator_suspend,
+	.resume = sy7636a_regulator_resume,
+};
+
+static struct platform_driver sy7636a_regulator_driver = {
+	.driver = {
+		.name = "sy7636a-regulator",
+		.pm = &sy7636a_pm_ops,
+	},
+	.probe = sy7636a_regulator_probe,
+	.id_table = sy7636a_regulator_id_table,
+};
+module_platform_driver(sy7636a_regulator_driver);
+
+MODULE_AUTHOR("Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>");
+MODULE_DESCRIPTION("SY7636A voltage regulator driver");
+MODULE_LICENSE("GPL v2");
-- 
2.29.2

[PATCH 5/6] arch/arm: reMarkable2: Enable silergy,sy7636a

[Alistair Francis]

Enable the silergy,sy7636a and silergy,sy7636a-regulator on the
reMarkable2.

Signed-off-by: Alistair Francis <alistair@alistair23.me>
---
 arch/arm/boot/dts/imx7d-remarkable2.dts | 61 +++++++++++++++++++++++++
 arch/arm/configs/remarkable2_defconfig  |  2 +
 2 files changed, 63 insertions(+)

diff --git a/arch/arm/boot/dts/imx7d-remarkable2.dts b/arch/arm/boot/dts/imx7d-remarkable2.dts
index 8052d884a5e5..f419ab704f06 100644
--- a/arch/arm/boot/dts/imx7d-remarkable2.dts
+++ b/arch/arm/boot/dts/imx7d-remarkable2.dts
@@ -58,6 +58,27 @@ memory {
 		reg = <0x80000000 0x40000000>;
 	};
 
+	thermal-zones {
+		epd-thermal {
+			thermal-sensors = <&epd_pmic>;
+			polling-delay-passive = <30000>;
+			polling-delay = <30000>;
+			trips {
+				trip0 {
+					temperature = <49000>;
+					hysteresis = <2000>;
+					type = "passive";
+				};
+
+				trip1 {
+					temperature = <50000>;
+					hysteresis = <2000>;
+					type = "critical";
+				};
+			};
+		};
+	};
+
 	reg_vref_1v8: regulator-vref-1v8 {
 		compatible = "regulator-fixed";
 		regulator-name = "vref-1v8";
@@ -174,6 +195,32 @@ digitizer: wacom-i2c@9 {
 	};
 };
 
+&i2c4 {
+	clock-frequency = <100000>;
+	pinctrl-names = "default", "sleep";
+	pinctrl-0 = <&pinctrl_i2c4>;
+	pinctrl-1 = <&pinctrl_i2c4>;
+	status = "okay";
+
+	epd_pmic: sy7636a@62 {
+		compatible = "silergy,sy7636a";
+		reg = <0x62>;
+		status = "okay";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_epdpmic>;
+		#thermal-sensor-cells = <0>;
+
+		epd-pwr-good-gpios = <&gpio6 21 GPIO_ACTIVE_HIGH>;
+		regulators {
+			compatible = "silergy,sy7636a-regulator";
+			reg_epdpmic: vcom {
+				regulator-name = "vcom";
+				regulator-boot-on;
+			};
+		};
+	};
+};
+
 &sdma {
 	status = "okay";
 };
@@ -277,6 +324,13 @@ MX7D_PAD_I2C1_SCL__I2C1_SCL		0x4000007f
 		>;
 	};
 
+	pinctrl_i2c4: i2c4grp {
+		fsl,pins = <
+			MX7D_PAD_I2C4_SDA__I2C4_SDA		0x4000007f
+			MX7D_PAD_I2C4_SCL__I2C4_SCL		0x4000007f
+		>;
+	};
+
 	pinctrl_uart1: uart1grp {
 		fsl,pins = <
 			MX7D_PAD_UART1_TX_DATA__UART1_DCE_TX	0x79
@@ -389,6 +443,13 @@ MX7D_PAD_SD1_WP__CCM_CLKO2		0x00000014
 		>;
 	};
 
+	pinctrl_epdpmic: epdpmicgrp {
+		fsl,pins = <
+			MX7D_PAD_SAI2_RX_DATA__GPIO6_IO21 0x00000074
+			MX7D_PAD_ENET1_RGMII_TXC__GPIO7_IO11 0x00000014
+		>;
+	};
+
 	pinctrl_wdog: wdoggrp {
 		fsl,pins = <
 			MX7D_PAD_ENET1_COL__WDOG1_WDOG_ANY	0x74
diff --git a/arch/arm/configs/remarkable2_defconfig b/arch/arm/configs/remarkable2_defconfig
index 8c9785555cda..6306568772c3 100644
--- a/arch/arm/configs/remarkable2_defconfig
+++ b/arch/arm/configs/remarkable2_defconfig
@@ -193,6 +193,7 @@ CONFIG_MFD_MC13XXX_SPI=y
 CONFIG_MFD_MC13XXX_I2C=y
 CONFIG_MFD_SI476X_CORE=y
 CONFIG_MFD_STMPE=y
+CONFIG_MFD_SY7636A=y
 CONFIG_REGULATOR=y
 CONFIG_REGULATOR_FIXED_VOLTAGE=y
 CONFIG_REGULATOR_ANATOP=y
@@ -201,6 +202,7 @@ CONFIG_REGULATOR_GPIO=y
 CONFIG_REGULATOR_MC13783=y
 CONFIG_REGULATOR_MC13892=y
 CONFIG_REGULATOR_PFUZE100=y
+CONFIG_REGULATOR_SY7636A=y
 CONFIG_MEDIA_SUPPORT=y
 CONFIG_MEDIA_USB_SUPPORT=y
 CONFIG_USB_VIDEO_CLASS=m
-- 
2.29.2

[PATCH 6/6] arch/arm: reMarkable2: Enable lcdif

[Alistair Francis]

Connect the dispaly on the reMarkable2.

Signed-off-by: Alistair Francis <alistair@alistair23.me>
---
 arch/arm/boot/dts/imx7d-remarkable2.dts | 70 +++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/arch/arm/boot/dts/imx7d-remarkable2.dts b/arch/arm/boot/dts/imx7d-remarkable2.dts
index f419ab704f06..d4e93f7ca7ea 100644
--- a/arch/arm/boot/dts/imx7d-remarkable2.dts
+++ b/arch/arm/boot/dts/imx7d-remarkable2.dts
@@ -221,6 +221,42 @@ reg_epdpmic: vcom {
 	};
 };
 
+&lcdif {
+	pinctrl-names = "default", "sleep";
+	pinctrl-0 = <&pinctrl_lcdif>;
+	pinctrl-1 = <&pinctrl_lcdif>;
+	lcd-supply = <&reg_epdpmic>;
+	lcd2-supply = <&reg_sdoe>;
+	display = <&display0>;
+	prevent-frying-pan;
+	status = "okay";
+
+	display0: display {
+		bits-per-pixel = <24>;
+		bus-width = <24>;
+
+		display-timings {
+			native-mode = <&timing0>;
+
+			timing0: timing0 {
+				clock-frequency = <40000000>;
+				hactive = <334>;
+				vactive = <1405>;
+				hfront-porch = <1>;
+				hback-porch = <1>;
+				hsync-len = <1>;
+				vback-porch = <1>;
+				vfront-porch = <1>;
+				vsync-len = <1>;
+				hsync-active = <0>;
+				vsync-active = <0>;
+				de-active = <1>;
+				pixelclk-active = <1>;
+			};
+		};
+	};
+};
+
 &sdma {
 	status = "okay";
 };
@@ -331,6 +367,40 @@ MX7D_PAD_I2C4_SCL__I2C4_SCL		0x4000007f
 		>;
 	};
 
+	pinctrl_lcdif: lcdifgrp {
+		fsl,pins = <
+			MX7D_PAD_LCD_DATA00__LCD_DATA0		0x79
+			MX7D_PAD_LCD_DATA01__LCD_DATA1		0x79
+			MX7D_PAD_LCD_DATA02__LCD_DATA2		0x79
+			MX7D_PAD_LCD_DATA03__LCD_DATA3		0x79
+			MX7D_PAD_LCD_DATA04__LCD_DATA4		0x79
+			MX7D_PAD_LCD_DATA05__LCD_DATA5		0x79
+			MX7D_PAD_LCD_DATA06__LCD_DATA6		0x79
+			MX7D_PAD_LCD_DATA07__LCD_DATA7		0x79
+			MX7D_PAD_LCD_DATA08__LCD_DATA8		0x79
+			MX7D_PAD_LCD_DATA09__LCD_DATA9		0x79
+			MX7D_PAD_LCD_DATA10__LCD_DATA10		0x79
+			MX7D_PAD_LCD_DATA11__LCD_DATA11		0x79
+			MX7D_PAD_LCD_DATA12__LCD_DATA12		0x79
+			MX7D_PAD_LCD_DATA13__LCD_DATA13		0x79
+			MX7D_PAD_LCD_DATA14__LCD_DATA14		0x79
+			MX7D_PAD_LCD_DATA15__LCD_DATA15		0x79
+
+			MX7D_PAD_LCD_DATA17__LCD_DATA17		0x79
+			MX7D_PAD_LCD_DATA18__LCD_DATA18		0x79
+			MX7D_PAD_LCD_DATA19__LCD_DATA19		0x79
+			MX7D_PAD_LCD_DATA20__LCD_DATA20		0x79
+			MX7D_PAD_LCD_DATA21__LCD_DATA21		0x79
+
+			MX7D_PAD_LCD_DATA23__LCD_DATA23		0x79
+			MX7D_PAD_LCD_CLK__LCD_CLK		0x79
+			MX7D_PAD_LCD_ENABLE__LCD_ENABLE		0x79
+			MX7D_PAD_LCD_VSYNC__LCD_VSYNC		0x79
+			MX7D_PAD_LCD_HSYNC__LCD_HSYNC		0x79
+			MX7D_PAD_LCD_RESET__LCD_RESET		0x79
+		>;
+	};
+
 	pinctrl_uart1: uart1grp {
 		fsl,pins = <
 			MX7D_PAD_UART1_TX_DATA__UART1_DCE_TX	0x79
-- 
2.29.2

Re: [PATCH 4/6] regulator: Initial commit of sy7636a

[Mark Brown]

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

On Sat, Jan 16, 2021 at 08:25:37PM -0800, Alistair Francis wrote:

> --- /dev/null
> +++ b/drivers/regulator/sy7636a-regulator.c
> @@ -0,0 +1,233 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Functions to access SY3686A power management chip voltages
> + *

Please make the entire comment a C++ one so things look more
intentional.

> + * Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/
> + *
> + * Author: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>

This probably needs an update.

> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.

This boilerplate is redundant and should be removed.

> +static int get_vcom_voltage_op(struct regulator_dev *rdev)
> +{
> +	int ret = get_vcom_voltage_mv(rdev->regmap);
> +

Why is this get_vcom_voltage_mv() function not in the regulator driver,
and why is it not just inline here?  It also needs namespacing.

> +static int disable_regulator(struct regulator_dev *rdev)
> +{
> +	struct sy7636a *sy7636a = dev_get_drvdata(rdev->dev.parent);
> +	int ret = 0;
> +
> +	mutex_lock(&sy7636a->reglock);
> +	ret = regulator_disable_regmap(rdev);
> +	usleep_range(30000, 35000);
> +	mutex_unlock(&sy7636a->reglock);

Why do you need this delay here, and what purpose is this lock intended
to serve?  I can't understand what it's intended to protect.

> +	mutex_lock(&sy7636a->reglock);
> +	ret = regulator_is_enabled_regmap(rdev);
> +	mutex_unlock(&sy7636a->reglock);

This lock usage in particular looks confused.

> +	ret = regulator_enable_regmap(rdev);
> +	if (ret)
> +		goto finish;

> +	if (!pwr_good) {
> +		dev_err(&rdev->dev, "Power good signal timeout after %u ms\n",
> +				jiffies_to_msecs(t1 - t0));
> +		ret = -ETIME;
> +		goto finish;
> +	}

This doesn't undo the underlying enable, leaving the regulator in a
partially enabled state.

> +static const struct regulator_ops sy7636a_vcom_volt_ops = {
> +	.get_voltage = get_vcom_voltage_op,
> +	.enable = enable_regulator_pgood,
> +	.disable = disable_regulator,
> +	.is_enabled = sy7636a_regulator_is_enabled,
> +};

The namespacing for functions is very random and prone to clashes.
Given the power good signal I'd also expect a get_status() operation.

> +static int sy7636a_regulator_suspend(struct device *dev)
> +{
> +	int ret;
> +	struct sy7636a *sy7636a = dev_get_drvdata(dev->parent);
> +
> +	ret = get_vcom_voltage_mv(sy7636a->regmap);
> +
> +	if (ret > 0)
> +		sy7636a->vcom = (unsigned int)ret;
> +
> +	return 0;
> +}

What's going on here, and if you are going to store this value over
suspend why not store it in a variable of the correct type?  In general
it's surprising to need a suspend operation for a regulator.

> +	sy7636a->pgood_gpio = gdp;
> +	dev_info(sy7636a->dev,
> +		"Power good GPIO registered (gpio# %d)\n",
> +		desc_to_gpio(sy7636a->pgood_gpio));

This print is just adding noise to the boot process.

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

Re: [PATCH 3/6] devicetree/bindings: Initial commit of silergy,sy7636a-regulator.yaml

[Mark Brown]

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

On Sat, Jan 16, 2021 at 08:25:36PM -0800, Alistair Francis wrote:
> Initial support for the Silergy SY7636A-regulator Power Management chip
> driver.

Please submit patches using subject lines reflecting the style for the
subsystem, this makes it easier for people to identify relevant patches.
Look at what existing commits in the area you're changing are doing and
make sure your subject lines visually resemble what they're doing.
There's no need to resubmit to fix this alone.

> +properties:
> +  compatible:
> +    enum:
> +      - sy7636a-regulator

Compatible strings should be in the form vendor,device.  

> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        regulator@60 {
> +          compatible = "sy7636a-regulator";
> +          reg_epdpmic: vcom {
> +            regulator-name = "vcom";
> +            regulator-boot-on;
> +          };
> +        };
> +    };

There's no documentation of VCOM as a valid regulator in the binding.

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

Re: [PATCH 3/6] devicetree/bindings: Initial commit of silergy,sy7636a-regulator.yaml

[Mark Brown]

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

On Mon, Jan 18, 2021 at 12:35:19PM +0000, Mark Brown wrote:
> On Sat, Jan 16, 2021 at 08:25:36PM -0800, Alistair Francis wrote:

> > +properties:
> > +  compatible:
> > +    enum:
> > +      - sy7636a-regulator

> Compatible strings should be in the form vendor,device.  

You should not have separate binding documents for MFD subfunctions that
don't have separate compatible strings in DT, include the documentation
for the properties used by those subfunctions in the main MFD binding
document.

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

Re: [PATCH 3/6] devicetree/bindings: Initial commit of silergy,sy7636a-regulator.yaml

[Rob Herring]

On Sat, 16 Jan 2021 20:25:36 -0800, Alistair Francis wrote:
> Initial support for the Silergy SY7636A-regulator Power Management chip
> driver.
> 
> Signed-off-by: Alistair Francis <alistair@alistair23.me>
> ---
>  .../bindings/regulator/silergy,sy7636a.yaml   | 39 +++++++++++++++++++
>  1 file changed, 39 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/regulator/silergy,sy7636a.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
./Documentation/devicetree/bindings/regulator/silergy,sy7636a.yaml: $id: relative path/filename doesn't match actual path or filename
	expected: http://devicetree.org/schemas/regulator/silergy,sy7636a.yaml#
Documentation/devicetree/bindings/regulator/silergy,sy7636a.example.dts:22.26-28.15: Warning (unit_address_vs_reg): /example-0/i2c/regulator@60: node has a unit name, but no reg or ranges property
Documentation/devicetree/bindings/regulator/silergy,sy7636a.example.dts:22.26-28.15: Warning (i2c_bus_reg): /example-0/i2c/regulator@60: missing or empty reg property

See https://patchwork.ozlabs.org/patch/1427907

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH 1/6] devicetree/bindings: Initial commit of silergy,sy7636a.yaml

[Rob Herring]

On Sat, 16 Jan 2021 20:25:34 -0800, Alistair Francis wrote:
> Initial support for the Silergy SY7636A Power Management chip
> driver.
> 
> Signed-off-by: Alistair Francis <alistair@alistair23.me>
> ---
>  .../bindings/mfd/silergy,sy7636a.yaml         | 37 +++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
./Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml: $id: relative path/filename doesn't match actual path or filename
	expected: http://devicetree.org/schemas/mfd/silergy,sy7636a.yaml#

See https://patchwork.ozlabs.org/patch/1427906

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH 3/6] devicetree/bindings: Initial commit of silergy,sy7636a-regulator.yaml

[Alistair Francis]

On Mon, Jan 18, 2021 at 4:42 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Jan 18, 2021 at 12:35:19PM +0000, Mark Brown wrote:
> > On Sat, Jan 16, 2021 at 08:25:36PM -0800, Alistair Francis wrote:
>
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - sy7636a-regulator
>
> > Compatible strings should be in the form vendor,device.
>
> You should not have separate binding documents for MFD subfunctions that
> don't have separate compatible strings in DT, include the documentation
> for the properties used by those subfunctions in the main MFD binding
> document.

Thanks for the review. I have updated the patch and will send a v2.

Alistair

Re: [PATCH 4/6] regulator: Initial commit of sy7636a

[Alistair Francis]

On Mon, Jan 18, 2021 at 4:32 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Sat, Jan 16, 2021 at 08:25:37PM -0800, Alistair Francis wrote:
>
> > --- /dev/null
> > +++ b/drivers/regulator/sy7636a-regulator.c
> > @@ -0,0 +1,233 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Functions to access SY3686A power management chip voltages
> > + *
>
> Please make the entire comment a C++ one so things look more
> intentional.

Fixed.

>
> > + * Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/
> > + *
> > + * Author: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>
>
> This probably needs an update.
>
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation version 2.
> > + *
> > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> > + * kind, whether express or implied; without even the implied warranty
> > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
>
> This boilerplate is redundant and should be removed.

Fixed.

>
> > +static int get_vcom_voltage_op(struct regulator_dev *rdev)
> > +{
> > +     int ret = get_vcom_voltage_mv(rdev->regmap);
> > +
>
> Why is this get_vcom_voltage_mv() function not in the regulator driver,
> and why is it not just inline here?  It also needs namespacing.

I'm not sure what you mean, can you please explain?

>
> > +static int disable_regulator(struct regulator_dev *rdev)
> > +{
> > +     struct sy7636a *sy7636a = dev_get_drvdata(rdev->dev.parent);
> > +     int ret = 0;
> > +
> > +     mutex_lock(&sy7636a->reglock);
> > +     ret = regulator_disable_regmap(rdev);
> > +     usleep_range(30000, 35000);
> > +     mutex_unlock(&sy7636a->reglock);
>
> Why do you need this delay here, and what purpose is this lock intended

The delay is to allow a power ramp up, I have added a comment.

> to serve?  I can't understand what it's intended to protect.

Apparently the mutex is to protect enable/disable, I don't think it's
required and I will remove it.

>
> > +     mutex_lock(&sy7636a->reglock);
> > +     ret = regulator_is_enabled_regmap(rdev);
> > +     mutex_unlock(&sy7636a->reglock);
>
> This lock usage in particular looks confused.
>
> > +     ret = regulator_enable_regmap(rdev);
> > +     if (ret)
> > +             goto finish;
>
> > +     if (!pwr_good) {
> > +             dev_err(&rdev->dev, "Power good signal timeout after %u ms\n",
> > +                             jiffies_to_msecs(t1 - t0));
> > +             ret = -ETIME;
> > +             goto finish;
> > +     }
>
> This doesn't undo the underlying enable, leaving the regulator in a
> partially enabled state.

Thanks, fixed.

>
> > +static const struct regulator_ops sy7636a_vcom_volt_ops = {
> > +     .get_voltage = get_vcom_voltage_op,
> > +     .enable = enable_regulator_pgood,
> > +     .disable = disable_regulator,
> > +     .is_enabled = sy7636a_regulator_is_enabled,
> > +};
>
> The namespacing for functions is very random and prone to clashes.

Fixed.

> Given the power good signal I'd also expect a get_status() operation.

Added.

>
> > +static int sy7636a_regulator_suspend(struct device *dev)
> > +{
> > +     int ret;
> > +     struct sy7636a *sy7636a = dev_get_drvdata(dev->parent);
> > +
> > +     ret = get_vcom_voltage_mv(sy7636a->regmap);
> > +
> > +     if (ret > 0)
> > +             sy7636a->vcom = (unsigned int)ret;
> > +
> > +     return 0;
> > +}
>
> What's going on here, and if you are going to store this value over
> suspend why not store it in a variable of the correct type?  In general

It is part of the vendor's kernel, they specifically added it to
ensure vcom is set on resume.

I have fixed the variable type.

> it's surprising to need a suspend operation for a regulator.
>
> > +     sy7636a->pgood_gpio = gdp;
> > +     dev_info(sy7636a->dev,
> > +             "Power good GPIO registered (gpio# %d)\n",
> > +             desc_to_gpio(sy7636a->pgood_gpio));
>
> This print is just adding noise to the boot process.

Removed.


Alistair

Re: [PATCH 4/6] regulator: Initial commit of sy7636a

[Mark Brown]

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

On Thu, Jan 21, 2021 at 10:24:10PM -0800, Alistair Francis wrote:
> On Mon, Jan 18, 2021 at 4:32 AM Mark Brown <broonie@kernel.org> wrote:
> > On Sat, Jan 16, 2021 at 08:25:37PM -0800, Alistair Francis wrote:

> > > +static int get_vcom_voltage_op(struct regulator_dev *rdev)
> > > +{
> > > +     int ret = get_vcom_voltage_mv(rdev->regmap);
> > > +

> > Why is this get_vcom_voltage_mv() function not in the regulator driver,
> > and why is it not just inline here?  It also needs namespacing.

> I'm not sure what you mean, can you please explain?

This is a wrapper for a function that has exactly one caller but is not
only a separate function but also in the MFD header, part of a separate
driver.  This seems at best pointless.

> > Why do you need this delay here, and what purpose is this lock intended

> The delay is to allow a power ramp up, I have added a comment.

Use the standard ramp_delay, don't open code it.

> > > +static int sy7636a_regulator_suspend(struct device *dev)
> > > +{
> > > +     int ret;
> > > +     struct sy7636a *sy7636a = dev_get_drvdata(dev->parent);
> > > +
> > > +     ret = get_vcom_voltage_mv(sy7636a->regmap);
> > > +
> > > +     if (ret > 0)
> > > +             sy7636a->vcom = (unsigned int)ret;
> > > +
> > > +     return 0;
> > > +}

> > What's going on here, and if you are going to store this value over
> > suspend why not store it in a variable of the correct type?  In general

> It is part of the vendor's kernel, they specifically added it to
> ensure vcom is set on resume.

"I copied this from the vendor" isn't really a great explanation...  If
the device is likely to get completely powered off and loosing settings
then presumably the entire register map, not just this one value, needs
to be saved and restored instead of just this one value.  If that is the
case it's probably best to use a register cache and just resync it on
resume.

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

Re: [PATCH 4/6] regulator: Initial commit of sy7636a

[Alistair Francis]

On Fri, Jan 22, 2021 at 5:37 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Jan 21, 2021 at 10:24:10PM -0800, Alistair Francis wrote:
> > On Mon, Jan 18, 2021 at 4:32 AM Mark Brown <broonie@kernel.org> wrote:
> > > On Sat, Jan 16, 2021 at 08:25:37PM -0800, Alistair Francis wrote:
>
> > > > +static int get_vcom_voltage_op(struct regulator_dev *rdev)
> > > > +{
> > > > +     int ret = get_vcom_voltage_mv(rdev->regmap);
> > > > +
>
> > > Why is this get_vcom_voltage_mv() function not in the regulator driver,
> > > and why is it not just inline here?  It also needs namespacing.
>
> > I'm not sure what you mean, can you please explain?
>
> This is a wrapper for a function that has exactly one caller but is not
> only a separate function but also in the MFD header, part of a separate
> driver.  This seems at best pointless.

Ah I see. I think I have fixed this.

>
> > > Why do you need this delay here, and what purpose is this lock intended
>
> > The delay is to allow a power ramp up, I have added a comment.
>
> Use the standard ramp_delay, don't open code it.
>
> > > > +static int sy7636a_regulator_suspend(struct device *dev)
> > > > +{
> > > > +     int ret;
> > > > +     struct sy7636a *sy7636a = dev_get_drvdata(dev->parent);
> > > > +
> > > > +     ret = get_vcom_voltage_mv(sy7636a->regmap);
> > > > +
> > > > +     if (ret > 0)
> > > > +             sy7636a->vcom = (unsigned int)ret;
> > > > +
> > > > +     return 0;
> > > > +}
>
> > > What's going on here, and if you are going to store this value over
> > > suspend why not store it in a variable of the correct type?  In general
>
> > It is part of the vendor's kernel, they specifically added it to
> > ensure vcom is set on resume.
>
> "I copied this from the vendor" isn't really a great explanation...  If
> the device is likely to get completely powered off and loosing settings
> then presumably the entire register map, not just this one value, needs
> to be saved and restored instead of just this one value.  If that is the
> case it's probably best to use a register cache and just resync it on
> resume.

Good point.

I don't have a good answer so I have removed the suspend/resume part.
I'll have to investigate in the future if/why this is required.

Alistair

Re: [PATCH 2/6] mfd: Initial commit of sy7636a

[Lee Jones]

On Sat, 16 Jan 2021, Alistair Francis wrote:

> Initial support for the Silergy SY7636A Power Management chip
> driver.

Please remove "driver", as this is not support for the driver, it *is*
the driver which supports the chip.

> Signed-off-by: Alistair Francis <alistair@alistair23.me>
> ---
>  drivers/mfd/Kconfig         |  10 ++
>  drivers/mfd/Makefile        |   2 +
>  drivers/mfd/sy7636a.c       | 252 ++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/sy7636a.h |  50 +++++++
>  4 files changed, 314 insertions(+)
>  create mode 100644 drivers/mfd/sy7636a.c
>  create mode 100644 include/linux/mfd/sy7636a.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index bdfce7b15621..c8c62d92433c 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1360,6 +1360,16 @@ config MFD_SYSCON
>  	  Select this option to enable accessing system control registers
>  	  via regmap.
>  
> +config MFD_SY7636A
> +	tristate "Silergy SY7636A Power Management chip driver"

Again, please remove the word "driver" here.

> +	select MFD_CORE
> +	select REGMAP_I2C
> +	select REGMAP_IRQ
> +	depends on I2C=y
> +	help
> +	  Select this option to enable support for the Silergy SY7636A
> +	  Power Management chip driver.

And again.

>  config MFD_DAVINCI_VOICECODEC
>  	tristate
>  	select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 14fdb188af02..1fa1e635f506 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -265,6 +265,8 @@ obj-$(CONFIG_MFD_ROHM_BD718XX)	+= rohm-bd718x7.o
>  obj-$(CONFIG_MFD_STMFX) 	+= stmfx.o
>  obj-$(CONFIG_MFD_KHADAS_MCU) 	+= khadas-mcu.o
>  
> +obj-$(CONFIG_MFD_SY7636A)	+= sy7636a.o
> +

Why does this have to be segregated?

>  obj-$(CONFIG_SGI_MFD_IOC3)	+= ioc3.o
>  obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)	+= simple-mfd-i2c.o
>  obj-$(CONFIG_MFD_INTEL_M10_BMC)   += intel-m10-bmc.o
> diff --git a/drivers/mfd/sy7636a.c b/drivers/mfd/sy7636a.c
> new file mode 100644
> index 000000000000..39aac965d854
> --- /dev/null
> +++ b/drivers/mfd/sy7636a.c
> @@ -0,0 +1,252 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * MFD driver for SY7636A chip

"Parent driver".

> + * Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/

This is quite out of date.  Please update.

> + * Author: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.

This test is replaced by the SPDX header above.

> + * Based on the lp87565 driver by Keerthy <j-keerthy@ti.com>
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
> +#include <linux/sysfs.h>
> +
> +#include <linux/mfd/sy7636a.h>
> +
> +static const struct regmap_config sy7636a_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +};
> +
> +static const struct mfd_cell sy7636a_cells[] = {
> +	{ .name = "sy7636a-regulator", },
> +	{ .name = "sy7636a-temperature", },
> +	{ .name = "sy7636a-thermal", },
> +};
> +
> +static const struct of_device_id of_sy7636a_match_table[] = {
> +	{ .compatible = "silergy,sy7636a", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, of_sy7636a_match_table);

Please move this to where it is used i.e. at the bottom of the file.

> +static const char * const states[] = {
> +	"no fault event",
> +	"UVP at VP rail",
> +	"UVP at VN rail",
> +	"UVP at VPOS rail",
> +	"UVP at VNEG rail",
> +	"UVP at VDDH rail",
> +	"UVP at VEE rail",
> +	"SCP at VP rail",
> +	"SCP at VN rail",
> +	"SCP at VPOS rail",
> +	"SCP at VNEG rail",
> +	"SCP at VDDH rail",
> +	"SCP at VEE rail",
> +	"SCP at V COM rail",
> +	"UVLO",
> +	"Thermal shutdown",
> +};
> +
> +int get_vcom_voltage_mv(struct regmap *regmap)
> +{
> +	int ret;
> +	unsigned int val, val_h;
> +
> +	ret = regmap_read(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_L, &val);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_H, &val_h);
> +	if (ret)
> +		return ret;
> +
> +	val |= (val_h << 8);

Please define the shifts and masks.

> +	return (val & 0x1FF) * 10;

What's 10?

> +}
> +
> +int set_vcom_voltage_mv(struct regmap *regmap, unsigned int vcom)
> +{
> +	int ret;
> +	unsigned int val;
> +
> +	if (vcom < 0 || vcom > 5000)

Please define min/max values.

> +		return -EINVAL;
> +
> +	val = (unsigned int)(vcom / 10) & 0x1ff;

As above.

> +	ret = regmap_write(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_L, val);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_H, val >> 8);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

Who calls these?

> +static ssize_t state_show(struct device *dev, struct device_attribute *attr,
> +		char *buf)
> +{
> +	int ret;
> +	unsigned int val;
> +	struct sy7636a *sy7636a = dev_get_drvdata(dev);
> +
> +	ret = regmap_read(sy7636a->regmap, SY7636A_REG_FAULT_FLAG, &val);
> +	if (ret) {
> +		dev_err(sy7636a->dev, "Failed to read from device\n");
> +		return ret;
> +	}
> +
> +	val = val >> 1;

Why 1?

> +	if (val >= ARRAY_SIZE(states)) {
> +		dev_err(sy7636a->dev, "Unexpected value read from device: %u\n", val);
> +		return -EINVAL;
> +	}
> +
> +	return snprintf(buf, PAGE_SIZE, "%s\n", states[val]);
> +}
> +static DEVICE_ATTR(state, 0444, state_show, NULL);

You need to document new sysfs entries.

> +static ssize_t power_good_show(struct device *dev, struct device_attribute *attr,
> +		char *buf)
> +{
> +	int ret;
> +	unsigned int val;
> +	struct sy7636a *sy7636a = dev_get_drvdata(dev);
> +
> +	ret = regmap_read(sy7636a->regmap, SY7636A_REG_FAULT_FLAG, &val);
> +	if (ret) {
> +		dev_err(sy7636a->dev, "Failed to read from device\n");
> +		return ret;
> +	}
> +
> +	val &= 0x01;
> +
> +	return snprintf(buf, PAGE_SIZE, "%s\n", val ? "ON" : "OFF");

Doesn't 0 just mean "no fault event"?

> +}
> +static DEVICE_ATTR(power_good, 0444, power_good_show, NULL);
> +
> +static ssize_t vcom_show(struct device *dev, struct device_attribute *attr,
> +		char *buf)
> +{
> +	int ret;
> +	struct sy7636a *sy7636a = dev_get_drvdata(dev);
> +
> +	ret = get_vcom_voltage_mv(sy7636a->regmap);
> +	if (ret < 0)
> +		return ret;
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", -ret);
> +}
> +

Remove this line please.

> +static ssize_t vcom_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	int ret;
> +	int vcom;
> +	struct sy7636a *sy7636a = dev_get_drvdata(dev);
> +
> +	ret = kstrtoint(buf, 0, &vcom);
> +	if (ret)
> +		return ret;
> +
> +	if (vcom > 0 || vcom < -5000)
> +		return -EINVAL;
> +
> +	ret = set_vcom_voltage_mv(sy7636a->regmap, (unsigned int)(-vcom));
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +static DEVICE_ATTR(vcom, 0644, vcom_show, vcom_store);
> +
> +static struct attribute *sy7636a_sysfs_attrs[] = {
> +	&dev_attr_state.attr,
> +	&dev_attr_power_good.attr,
> +	&dev_attr_vcom.attr,
> +	NULL,
> +};

These all look like power options?  Do they really belong here?

> +static const struct attribute_group sy7636a_sysfs_attr_group = {
> +	.attrs = sy7636a_sysfs_attrs,
> +};
> +
> +static int sy7636a_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *ids)
> +{
> +	struct sy7636a *sy7636a;
> +	int ret;
> +
> +	sy7636a = devm_kzalloc(&client->dev, sizeof(struct sy7636a), GFP_KERNEL);

sizeof(*sy7636a)

> +	if (sy7636a == NULL)

if (!sy7636a)

> +		return -ENOMEM;
> +
> +	sy7636a->dev = &client->dev;
> +
> +	sy7636a->regmap = devm_regmap_init_i2c(client, &sy7636a_regmap_config);
> +	if (IS_ERR(sy7636a->regmap)) {
> +		ret = PTR_ERR(sy7636a->regmap);
> +		dev_err(sy7636a->dev,
> +			"Failed to initialize register map: %d\n", ret);
> +		return ret;
> +	}
> +
> +	i2c_set_clientdata(client, sy7636a);
> +
> +	ret = sysfs_create_group(&client->dev.kobj, &sy7636a_sysfs_attr_group);
> +	if (ret) {
> +		dev_err(sy7636a->dev, "Failed to create sysfs attributes\n");
> +		return ret;
> +	}
> +
> +	ret = devm_mfd_add_devices(sy7636a->dev, PLATFORM_DEVID_AUTO,
> +					sy7636a_cells, ARRAY_SIZE(sy7636a_cells),
> +					NULL, 0, NULL);
> +	if (ret) {
> +		dev_err(sy7636a->dev, "Failed to add mfd devices\n");

"Failed to add child devices"

> +		sysfs_remove_group(&client->dev.kobj, &sy7636a_sysfs_attr_group);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id sy7636a_id_table[] = {
> +	{ "sy7636a", 0 },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(i2c, sy7636a_id_table);

If you use .probe2, you can omit this table.

> +static struct i2c_driver sy7636a_driver = {
> +	.driver	= {
> +		.name	= "sy7636a",
> +		.of_match_table = of_sy7636a_match_table,
> +	},
> +	.probe = sy7636a_probe,
> +	.id_table = sy7636a_id_table,
> +};
> +module_i2c_driver(sy7636a_driver);
> +
> +MODULE_AUTHOR("Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>");
> +MODULE_DESCRIPTION("Silergy SY7636A Multi-Function Device Driver");

s/Multi-Function Device Driver/Power Management Chip/

> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/sy7636a.h b/include/linux/mfd/sy7636a.h
> new file mode 100644
> index 000000000000..642789c4d0a9
> --- /dev/null
> +++ b/include/linux/mfd/sy7636a.h
> @@ -0,0 +1,50 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Functions to access SY3686A power management chip.
> + *
> + * Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */

Same issues as mentioned above.

> +#ifndef __LINUX_MFD_SY7636A_H
> +#define __LINUX_MFD_SY7636A_H

Just MFD is fine.

> +#include <linux/i2c.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/machine.h>
> +#include <linux/regmap.h>

Alphabetical.

> +#define SY7636A_REG_OPERATION_MODE_CRL 0x00
> +#define SY7636A_OPERATION_MODE_CRL_VCOMCTL (1 << 6)
> +#define SY7636A_OPERATION_MODE_CRL_ONOFF (1 << 7)
> +#define SY7636A_REG_VCOM_ADJUST_CTRL_L 0x01
> +#define SY7636A_REG_VCOM_ADJUST_CTRL_H 0x02
> +#define SY7636A_REG_VCOM_ADJUST_CTRL_MASK 0x01ff
> +#define SY7636A_REG_VLDO_VOLTAGE_ADJULST_CTRL 0x03
> +#define SY7636A_REG_POWER_ON_DELAY_TIME 0x06
> +#define SY7636A_REG_FAULT_FLAG 0x07
> +#define SY7636A_FAULT_FLAG_PG (1 << 0)
> +#define SY7636A_REG_TERMISTOR_READOUT 0x08

Tab out the values please.

Use BIT()

> +#define SY7636A_REG_MAX 0x08
> +
> +struct sy7636a {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	unsigned int vcom;

Where is this used?

> +	struct gpio_desc *pgood_gpio;

Where is this used?

> +	struct mutex reglock;

Where is this used?

> +};
> +
> +int get_vcom_voltage_mv(struct regmap *regmap);
> +int set_vcom_voltage_mv(struct regmap *regmap, unsigned int vcom);

What calls these?

> +#endif /* __LINUX_MFD_SY7636A_H */

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

Re: [PATCH 2/6] mfd: Initial commit of sy7636a

[Alistair Francis]

On Thu, Feb 4, 2021 at 5:31 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Sat, 16 Jan 2021, Alistair Francis wrote:
>
> > Initial support for the Silergy SY7636A Power Management chip
> > driver.
>
> Please remove "driver", as this is not support for the driver, it *is*
> the driver which supports the chip.

Sorry for the long delay here.

I have addressed your comments.

>
> > Signed-off-by: Alistair Francis <alistair@alistair23.me>
> > ---
> >  drivers/mfd/Kconfig         |  10 ++
> >  drivers/mfd/Makefile        |   2 +
> >  drivers/mfd/sy7636a.c       | 252 ++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/sy7636a.h |  50 +++++++
> >  4 files changed, 314 insertions(+)
> >  create mode 100644 drivers/mfd/sy7636a.c
> >  create mode 100644 include/linux/mfd/sy7636a.h
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index bdfce7b15621..c8c62d92433c 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -1360,6 +1360,16 @@ config MFD_SYSCON
> >         Select this option to enable accessing system control registers
> >         via regmap.
> >
> > +config MFD_SY7636A
> > +     tristate "Silergy SY7636A Power Management chip driver"
>
> Again, please remove the word "driver" here.
>
> > +     select MFD_CORE
> > +     select REGMAP_I2C
> > +     select REGMAP_IRQ
> > +     depends on I2C=y
> > +     help
> > +       Select this option to enable support for the Silergy SY7636A
> > +       Power Management chip driver.
>
> And again.

I removed "driver" from all of these.

>
> >  config MFD_DAVINCI_VOICECODEC
> >       tristate
> >       select MFD_CORE
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 14fdb188af02..1fa1e635f506 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -265,6 +265,8 @@ obj-$(CONFIG_MFD_ROHM_BD718XX)    += rohm-bd718x7.o
> >  obj-$(CONFIG_MFD_STMFX)      += stmfx.o
> >  obj-$(CONFIG_MFD_KHADAS_MCU)         += khadas-mcu.o
> >
> > +obj-$(CONFIG_MFD_SY7636A)    += sy7636a.o
> > +
>
> Why does this have to be segregated?

Fixed

>
> >  obj-$(CONFIG_SGI_MFD_IOC3)   += ioc3.o
> >  obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)     += simple-mfd-i2c.o
> >  obj-$(CONFIG_MFD_INTEL_M10_BMC)   += intel-m10-bmc.o
> > diff --git a/drivers/mfd/sy7636a.c b/drivers/mfd/sy7636a.c
> > new file mode 100644
> > index 000000000000..39aac965d854
> > --- /dev/null
> > +++ b/drivers/mfd/sy7636a.c
> > @@ -0,0 +1,252 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * MFD driver for SY7636A chip
>
> "Parent driver".
>
> > + * Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/
>
> This is quite out of date.  Please update.

I don't own this copyright, so I would rather not change it.

>
> > + * Author: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation version 2.
> > + *
> > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> > + * kind, whether express or implied; without even the implied warranty
> > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
>
> This test is replaced by the SPDX header above.

Removed.

>
> > + * Based on the lp87565 driver by Keerthy <j-keerthy@ti.com>
> > + */
> > +
> > +#include <linux/interrupt.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/sysfs.h>
> > +
> > +#include <linux/mfd/sy7636a.h>
> > +
> > +static const struct regmap_config sy7636a_regmap_config = {
> > +     .reg_bits = 8,
> > +     .val_bits = 8,
> > +};
> > +
> > +static const struct mfd_cell sy7636a_cells[] = {
> > +     { .name = "sy7636a-regulator", },
> > +     { .name = "sy7636a-temperature", },
> > +     { .name = "sy7636a-thermal", },
> > +};
> > +
> > +static const struct of_device_id of_sy7636a_match_table[] = {
> > +     { .compatible = "silergy,sy7636a", },
> > +     {}
> > +};
> > +MODULE_DEVICE_TABLE(of, of_sy7636a_match_table);
>
> Please move this to where it is used i.e. at the bottom of the file.

Done.

>
> > +static const char * const states[] = {
> > +     "no fault event",
> > +     "UVP at VP rail",
> > +     "UVP at VN rail",
> > +     "UVP at VPOS rail",
> > +     "UVP at VNEG rail",
> > +     "UVP at VDDH rail",
> > +     "UVP at VEE rail",
> > +     "SCP at VP rail",
> > +     "SCP at VN rail",
> > +     "SCP at VPOS rail",
> > +     "SCP at VNEG rail",
> > +     "SCP at VDDH rail",
> > +     "SCP at VEE rail",
> > +     "SCP at V COM rail",
> > +     "UVLO",
> > +     "Thermal shutdown",
> > +};
> > +
> > +int get_vcom_voltage_mv(struct regmap *regmap)
> > +{
> > +     int ret;
> > +     unsigned int val, val_h;
> > +
> > +     ret = regmap_read(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_L, &val);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = regmap_read(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_H, &val_h);
> > +     if (ret)
> > +             return ret;
> > +
> > +     val |= (val_h << 8);
>
> Please define the shifts and masks.
>
> > +     return (val & 0x1FF) * 10;
>
> What's 10?
>
> > +}
> > +
> > +int set_vcom_voltage_mv(struct regmap *regmap, unsigned int vcom)
> > +{
> > +     int ret;
> > +     unsigned int val;
> > +
> > +     if (vcom < 0 || vcom > 5000)
>
> Please define min/max values.
>
> > +             return -EINVAL;
> > +
> > +     val = (unsigned int)(vcom / 10) & 0x1ff;
>
> As above.

I have used defines for all of these.

>
> > +     ret = regmap_write(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_L, val);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = regmap_write(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_H, val >> 8);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return 0;
> > +}
>
> Who calls these?

They sysfs store and show functions.

>
> > +static ssize_t state_show(struct device *dev, struct device_attribute *attr,
> > +             char *buf)
> > +{
> > +     int ret;
> > +     unsigned int val;
> > +     struct sy7636a *sy7636a = dev_get_drvdata(dev);
> > +
> > +     ret = regmap_read(sy7636a->regmap, SY7636A_REG_FAULT_FLAG, &val);
> > +     if (ret) {
> > +             dev_err(sy7636a->dev, "Failed to read from device\n");
> > +             return ret;
> > +     }
> > +
> > +     val = val >> 1;
>
> Why 1?

I used a define here to make it clearer

>
> > +     if (val >= ARRAY_SIZE(states)) {
> > +             dev_err(sy7636a->dev, "Unexpected value read from device: %u\n", val);
> > +             return -EINVAL;
> > +     }
> > +
> > +     return snprintf(buf, PAGE_SIZE, "%s\n", states[val]);
> > +}
> > +static DEVICE_ATTR(state, 0444, state_show, NULL);
>
> You need to document new sysfs entries.

I'm not sure how to document this. Do you mind pointing out an example
I can use?

>
> > +static ssize_t power_good_show(struct device *dev, struct device_attribute *attr,
> > +             char *buf)
> > +{
> > +     int ret;
> > +     unsigned int val;
> > +     struct sy7636a *sy7636a = dev_get_drvdata(dev);
> > +
> > +     ret = regmap_read(sy7636a->regmap, SY7636A_REG_FAULT_FLAG, &val);
> > +     if (ret) {
> > +             dev_err(sy7636a->dev, "Failed to read from device\n");
> > +             return ret;
> > +     }
> > +
> > +     val &= 0x01;
> > +
> > +     return snprintf(buf, PAGE_SIZE, "%s\n", val ? "ON" : "OFF");
>
> Doesn't 0 just mean "no fault event"?

The LSB is reserved, so we don't clear that.

>
> > +}
> > +static DEVICE_ATTR(power_good, 0444, power_good_show, NULL);
> > +
> > +static ssize_t vcom_show(struct device *dev, struct device_attribute *attr,
> > +             char *buf)
> > +{
> > +     int ret;
> > +     struct sy7636a *sy7636a = dev_get_drvdata(dev);
> > +
> > +     ret = get_vcom_voltage_mv(sy7636a->regmap);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     return snprintf(buf, PAGE_SIZE, "%d\n", -ret);
> > +}
> > +
>
> Remove this line please.
>
> > +static ssize_t vcom_store(struct device *dev,
> > +             struct device_attribute *attr, const char *buf, size_t count)
> > +{
> > +     int ret;
> > +     int vcom;
> > +     struct sy7636a *sy7636a = dev_get_drvdata(dev);
> > +
> > +     ret = kstrtoint(buf, 0, &vcom);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (vcom > 0 || vcom < -5000)
> > +             return -EINVAL;
> > +
> > +     ret = set_vcom_voltage_mv(sy7636a->regmap, (unsigned int)(-vcom));
> > +     if (ret)
> > +             return ret;
> > +
> > +     return count;
> > +}
> > +static DEVICE_ATTR(vcom, 0644, vcom_show, vcom_store);
> > +
> > +static struct attribute *sy7636a_sysfs_attrs[] = {
> > +     &dev_attr_state.attr,
> > +     &dev_attr_power_good.attr,
> > +     &dev_attr_vcom.attr,
> > +     NULL,
> > +};
>
> These all look like power options?  Do they really belong here?

From what I can tell I think they do. Let me know if you don't think so.

>
> > +static const struct attribute_group sy7636a_sysfs_attr_group = {
> > +     .attrs = sy7636a_sysfs_attrs,
> > +};
> > +
> > +static int sy7636a_probe(struct i2c_client *client,
> > +                      const struct i2c_device_id *ids)
> > +{
> > +     struct sy7636a *sy7636a;
> > +     int ret;
> > +
> > +     sy7636a = devm_kzalloc(&client->dev, sizeof(struct sy7636a), GFP_KERNEL);
>
> sizeof(*sy7636a)
>
> > +     if (sy7636a == NULL)
>
> if (!sy7636a)
>
> > +             return -ENOMEM;
> > +
> > +     sy7636a->dev = &client->dev;
> > +
> > +     sy7636a->regmap = devm_regmap_init_i2c(client, &sy7636a_regmap_config);
> > +     if (IS_ERR(sy7636a->regmap)) {
> > +             ret = PTR_ERR(sy7636a->regmap);
> > +             dev_err(sy7636a->dev,
> > +                     "Failed to initialize register map: %d\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     i2c_set_clientdata(client, sy7636a);
> > +
> > +     ret = sysfs_create_group(&client->dev.kobj, &sy7636a_sysfs_attr_group);
> > +     if (ret) {
> > +             dev_err(sy7636a->dev, "Failed to create sysfs attributes\n");
> > +             return ret;
> > +     }
> > +
> > +     ret = devm_mfd_add_devices(sy7636a->dev, PLATFORM_DEVID_AUTO,
> > +                                     sy7636a_cells, ARRAY_SIZE(sy7636a_cells),
> > +                                     NULL, 0, NULL);
> > +     if (ret) {
> > +             dev_err(sy7636a->dev, "Failed to add mfd devices\n");
>
> "Failed to add child devices"
>
> > +             sysfs_remove_group(&client->dev.kobj, &sy7636a_sysfs_attr_group);
> > +             return ret;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct i2c_device_id sy7636a_id_table[] = {
> > +     { "sy7636a", 0 },
> > +     { },
> > +};
> > +MODULE_DEVICE_TABLE(i2c, sy7636a_id_table);
>
> If you use .probe2, you can omit this table.
>
> > +static struct i2c_driver sy7636a_driver = {
> > +     .driver = {
> > +             .name   = "sy7636a",
> > +             .of_match_table = of_sy7636a_match_table,
> > +     },
> > +     .probe = sy7636a_probe,
> > +     .id_table = sy7636a_id_table,
> > +};
> > +module_i2c_driver(sy7636a_driver);
> > +
> > +MODULE_AUTHOR("Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>");
> > +MODULE_DESCRIPTION("Silergy SY7636A Multi-Function Device Driver");
>
> s/Multi-Function Device Driver/Power Management Chip/
>
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/include/linux/mfd/sy7636a.h b/include/linux/mfd/sy7636a.h
> > new file mode 100644
> > index 000000000000..642789c4d0a9
> > --- /dev/null
> > +++ b/include/linux/mfd/sy7636a.h
> > @@ -0,0 +1,50 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Functions to access SY3686A power management chip.
> > + *
> > + * Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation version 2.
> > + *
> > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> > + * kind, whether express or implied; without even the implied warranty
> > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
>
> Same issues as mentioned above.
>
> > +#ifndef __LINUX_MFD_SY7636A_H
> > +#define __LINUX_MFD_SY7636A_H
>
> Just MFD is fine.
>
> > +#include <linux/i2c.h>
> > +#include <linux/regulator/driver.h>
> > +#include <linux/regulator/machine.h>
> > +#include <linux/regmap.h>
>
> Alphabetical.
>
> > +#define SY7636A_REG_OPERATION_MODE_CRL 0x00
> > +#define SY7636A_OPERATION_MODE_CRL_VCOMCTL (1 << 6)
> > +#define SY7636A_OPERATION_MODE_CRL_ONOFF (1 << 7)
> > +#define SY7636A_REG_VCOM_ADJUST_CTRL_L 0x01
> > +#define SY7636A_REG_VCOM_ADJUST_CTRL_H 0x02
> > +#define SY7636A_REG_VCOM_ADJUST_CTRL_MASK 0x01ff
> > +#define SY7636A_REG_VLDO_VOLTAGE_ADJULST_CTRL 0x03
> > +#define SY7636A_REG_POWER_ON_DELAY_TIME 0x06
> > +#define SY7636A_REG_FAULT_FLAG 0x07
> > +#define SY7636A_FAULT_FLAG_PG (1 << 0)
> > +#define SY7636A_REG_TERMISTOR_READOUT 0x08
>
> Tab out the values please.
>
> Use BIT()
>
> > +#define SY7636A_REG_MAX 0x08
> > +
> > +struct sy7636a {
> > +     struct device *dev;
> > +     struct regmap *regmap;
> > +     unsigned int vcom;
>
> Where is this used?
>
> > +     struct gpio_desc *pgood_gpio;
>
> Where is this used?
>
> > +     struct mutex reglock;
>
> Where is this used?

I have removed these.

>
> > +};
> > +
> > +int get_vcom_voltage_mv(struct regmap *regmap);
> > +int set_vcom_voltage_mv(struct regmap *regmap, unsigned int vcom);
>
> What calls these?

I have just made these internal to the driver.


Alistair

>
> > +#endif /* __LINUX_MFD_SY7636A_H */
>
> --
> Lee Jones [李琼斯]
> Senior Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/6] mfd: Initial commit of sy7636a

[Lee Jones]

On Sat, 20 Mar 2021, Alistair Francis wrote:

> On Thu, Feb 4, 2021 at 5:31 AM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > On Sat, 16 Jan 2021, Alistair Francis wrote:
> >
> > > Initial support for the Silergy SY7636A Power Management chip
> > > driver.
> >
> > Please remove "driver", as this is not support for the driver, it *is*
> > the driver which supports the chip.
> 
> Sorry for the long delay here.
> 
> I have addressed your comments.

[...]

> > > diff --git a/drivers/mfd/sy7636a.c b/drivers/mfd/sy7636a.c
> > > new file mode 100644
> > > index 000000000000..39aac965d854
> > > --- /dev/null
> > > +++ b/drivers/mfd/sy7636a.c
> > > @@ -0,0 +1,252 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * MFD driver for SY7636A chip
> >
> > "Parent driver".
> >
> > > + * Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/
> >
> > This is quite out of date.  Please update.
> 
> I don't own this copyright, so I would rather not change it.

I'm not comfortable taking a new driver with an old Copyright.

Maybe ask reMarkable if it's okay to bump it.

> > > + * Author: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>

Or ping this guy.

[...]

> > > +int set_vcom_voltage_mv(struct regmap *regmap, unsigned int vcom)
> > > +{
> > > +     int ret;
> > > +     unsigned int val;
> > > +
> > > +     if (vcom < 0 || vcom > 5000)
> >
> > Please define min/max values.
> >
> > > +             return -EINVAL;
> > > +
> > > +     val = (unsigned int)(vcom / 10) & 0x1ff;
> >
> > As above.
> 
> I have used defines for all of these.
> 
> >
> > > +     ret = regmap_write(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_L, val);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     ret = regmap_write(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_H, val >> 8);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     return 0;
> > > +}
> >
> > Who calls these?
> 
> They sysfs store and show functions.

They should be in a power/regulator driver really.

[...]

> > > +     if (val >= ARRAY_SIZE(states)) {
> > > +             dev_err(sy7636a->dev, "Unexpected value read from device: %u\n", val);
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     return snprintf(buf, PAGE_SIZE, "%s\n", states[val]);
> > > +}
> > > +static DEVICE_ATTR(state, 0444, state_show, NULL);
> >
> > You need to document new sysfs entries.
> 
> I'm not sure how to document this. Do you mind pointing out an example
> I can use?

See the final paragraph in:

  Documentation/filesystems/sysfs.rst

[...]

> > > +static struct attribute *sy7636a_sysfs_attrs[] = {
> > > +     &dev_attr_state.attr,
> > > +     &dev_attr_power_good.attr,
> > > +     &dev_attr_vcom.attr,
> > > +     NULL,
> > > +};
> >
> > These all look like power options?  Do they really belong here?
> 
> From what I can tell I think they do. Let me know if you don't think so.

As above, I think they should be in power or regulator.

[...]

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

Re: [PATCH 2/6] mfd: Initial commit of sy7636a

[Alistair Francis]

On Tue, Mar 23, 2021 at 5:35 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Sat, 20 Mar 2021, Alistair Francis wrote:
>
> > On Thu, Feb 4, 2021 at 5:31 AM Lee Jones <lee.jones@linaro.org> wrote:
> > >
> > > On Sat, 16 Jan 2021, Alistair Francis wrote:
> > >
> > > > Initial support for the Silergy SY7636A Power Management chip
> > > > driver.
> > >
> > > Please remove "driver", as this is not support for the driver, it *is*
> > > the driver which supports the chip.
> >
> > Sorry for the long delay here.
> >
> > I have addressed your comments.
>
> [...]
>
> > > > diff --git a/drivers/mfd/sy7636a.c b/drivers/mfd/sy7636a.c
> > > > new file mode 100644
> > > > index 000000000000..39aac965d854
> > > > --- /dev/null
> > > > +++ b/drivers/mfd/sy7636a.c
> > > > @@ -0,0 +1,252 @@
> > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > +/*
> > > > + * MFD driver for SY7636A chip
> > >
> > > "Parent driver".
> > >
> > > > + * Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/
> > >
> > > This is quite out of date.  Please update.
> >
> > I don't own this copyright, so I would rather not change it.
>
> I'm not comfortable taking a new driver with an old Copyright.
>
> Maybe ask reMarkable if it's okay to bump it.
>
> > > > + * Author: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>
>
> Or ping this guy.

I reached out to him and have permission to bump the year.

>
> [...]
>
> > > > +int set_vcom_voltage_mv(struct regmap *regmap, unsigned int vcom)
> > > > +{
> > > > +     int ret;
> > > > +     unsigned int val;
> > > > +
> > > > +     if (vcom < 0 || vcom > 5000)
> > >
> > > Please define min/max values.
> > >
> > > > +             return -EINVAL;
> > > > +
> > > > +     val = (unsigned int)(vcom / 10) & 0x1ff;
> > >
> > > As above.
> >
> > I have used defines for all of these.
> >
> > >
> > > > +     ret = regmap_write(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_L, val);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     ret = regmap_write(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_H, val >> 8);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     return 0;
> > > > +}
> > >
> > > Who calls these?
> >
> > They sysfs store and show functions.
>
> They should be in a power/regulator driver really.

Ok, I have moved these to the regulator.

>
> [...]
>
> > > > +     if (val >= ARRAY_SIZE(states)) {
> > > > +             dev_err(sy7636a->dev, "Unexpected value read from device: %u\n", val);
> > > > +             return -EINVAL;
> > > > +     }
> > > > +
> > > > +     return snprintf(buf, PAGE_SIZE, "%s\n", states[val]);
> > > > +}
> > > > +static DEVICE_ATTR(state, 0444, state_show, NULL);
> > >
> > > You need to document new sysfs entries.
> >
> > I'm not sure how to document this. Do you mind pointing out an example
> > I can use?
>
> See the final paragraph in:
>
>   Documentation/filesystems/sysfs.rst

Thanks!

>
> [...]
>
> > > > +static struct attribute *sy7636a_sysfs_attrs[] = {
> > > > +     &dev_attr_state.attr,
> > > > +     &dev_attr_power_good.attr,
> > > > +     &dev_attr_vcom.attr,
> > > > +     NULL,
> > > > +};
> > >
> > > These all look like power options?  Do they really belong here?
> >
> > From what I can tell I think they do. Let me know if you don't think so.
>
> As above, I think they should be in power or regulator.

Done.

Alistair

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