[PATCH v2 0/2] i2c: Add new driver for Renesas RZ/V2M controller

[PATCH v2 0/2] i2c: Add new driver for Renesas RZ/V2M controller

[Phil Edworthy]

Hi,

The Renesas RZ/V2M SoC (r9a09g011) has a new i2c controller. This series
add the driver. One annoying problem is that the SoC uses a single reset
line for two i2c controllers, and unfortunately one of the controllers
is managed by some firmware, not by Linux. Therefore, the driver just
deasserts the reset.

v2:
  dt-binding:
  - Use an enum and set the default for clock-frequency
  - Add resets property
  driver:
  - Use the new NOIRQ_SYSTEM_SLEEP_PM_OPS() as suggested by Arnd
  - Lots of small fixes based on Geert's review

Phil Edworthy (2):
  dt-bindings: i2c: Document RZ/V2M I2C controller
  i2c: Add Renesas RZ/V2M controller

 .../bindings/i2c/renesas,rzv2m.yaml           |  80 +++
 drivers/i2c/busses/Kconfig                    |  10 +
 drivers/i2c/busses/Makefile                   |   1 +
 drivers/i2c/busses/i2c-rzv2m.c                | 530 ++++++++++++++++++
 4 files changed, 621 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/renesas,rzv2m.yaml
 create mode 100644 drivers/i2c/busses/i2c-rzv2m.c

-- 
2.34.1

[PATCH v2 1/2] dt-bindings: i2c: Document RZ/V2M I2C controller

[Phil Edworthy]

Document Renesas RZ/V2M (r9a09g011) I2C controller bindings.

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2:
 - Use an enum and set the default for clock-frequency
 - Add resets property
---
 .../bindings/i2c/renesas,rzv2m.yaml           | 80 +++++++++++++++++++
 1 file changed, 80 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/renesas,rzv2m.yaml

diff --git a/Documentation/devicetree/bindings/i2c/renesas,rzv2m.yaml b/Documentation/devicetree/bindings/i2c/renesas,rzv2m.yaml
new file mode 100644
index 000000000000..7f6d2bb4ecb3
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/renesas,rzv2m.yaml
@@ -0,0 +1,80 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i2c/renesas,rzv2m.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas RZ/V2M I2C Bus Interface
+
+maintainers:
+  - Phil Edworthy <phil.edworthy@renesas.com>
+
+allOf:
+  - $ref: /schemas/i2c/i2c-controller.yaml#
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - renesas,i2c-r9a09g011  # RZ/V2M
+      - const: renesas,rzv2m-i2c
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    items:
+      - description: Data transmission/reception interrupt
+      - description: Status interrupt
+
+  interrupt-names:
+    items:
+      - const: tia
+      - const: tis
+
+  clock-frequency:
+    default: 100000
+    enum: [ 100000, 400000 ]
+    description:
+      Desired I2C bus clock frequency in Hz.
+
+  clocks:
+    maxItems: 1
+
+  power-domains:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - interrupt-names
+  - clocks
+  - power-domains
+  - resets
+  - '#address-cells'
+  - '#size-cells'
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/r9a09g011-cpg.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    i2c0: i2c@a4030000 {
+            compatible = "renesas,i2c-r9a09g011", "renesas,rzv2m-i2c";
+            reg = <0xa4030000 0x80>;
+            interrupts = <GIC_SPI 232 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 236 IRQ_TYPE_EDGE_RISING>;
+            interrupt-names = "tia", "tis";
+            clocks = <&cpg CPG_MOD R9A09G011_IIC_PCLK0>;
+            resets = <&cpg R9A09G011_IIC_GPA_PRESETN>;
+            power-domains = <&cpg>;
+            clock-frequency = <100000>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+    };
-- 
2.34.1

[PATCH v2 2/2] i2c: Add Renesas RZ/V2M controller

[Phil Edworthy]

Yet another i2c controller from Renesas that is found on the RZ/V2M
(r9a09g011) SoC. It can support only 100kHz and 400KHz operation.

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2:
 - Use the new NOIRQ_SYSTEM_SLEEP_PM_OPS() as suggested by Arnd
 - Lots of small fixes based on Geert's review
---
 drivers/i2c/busses/Kconfig     |  10 +
 drivers/i2c/busses/Makefile    |   1 +
 drivers/i2c/busses/i2c-rzv2m.c | 530 +++++++++++++++++++++++++++++++++
 3 files changed, 541 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-rzv2m.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index b1d7069dd377..9e3f9eb1ea3c 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -984,6 +984,16 @@ config I2C_RK3X
 	  This driver can also be built as a module. If so, the module will
 	  be called i2c-rk3x.
 
+config I2C_RZV2M
+	tristate "Renesas RZ/V2M adapter"
+	depends on ARCH_RENESAS || COMPILE_TEST
+	help
+	  If you say yes to this option, support will be included for the
+	  Renesas RZ/V2M  I2C interface.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-rzv2m.
+
 config I2C_S3C2410
 	tristate "S3C/Exynos I2C Driver"
 	depends on ARCH_EXYNOS || ARCH_S3C24XX || ARCH_S3C64XX || \
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index b0a10e5d9ee9..7792ffc591f0 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -101,6 +101,7 @@ obj-$(CONFIG_I2C_QCOM_GENI)	+= i2c-qcom-geni.o
 obj-$(CONFIG_I2C_QUP)		+= i2c-qup.o
 obj-$(CONFIG_I2C_RIIC)		+= i2c-riic.o
 obj-$(CONFIG_I2C_RK3X)		+= i2c-rk3x.o
+obj-$(CONFIG_I2C_RZV2M)		+= i2c-rzv2m.o
 obj-$(CONFIG_I2C_S3C2410)	+= i2c-s3c2410.o
 obj-$(CONFIG_I2C_SH7760)	+= i2c-sh7760.o
 obj-$(CONFIG_I2C_SH_MOBILE)	+= i2c-sh_mobile.o
diff --git a/drivers/i2c/busses/i2c-rzv2m.c b/drivers/i2c/busses/i2c-rzv2m.c
new file mode 100644
index 000000000000..ab326d557dc5
--- /dev/null
+++ b/drivers/i2c/busses/i2c-rzv2m.c
@@ -0,0 +1,530 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for the Renesas RZ/V2M I2C unit
+ *
+ * Copyright (C) 2016-2022 Renesas Electronics Corporation
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/i2c.h>
+#include <linux/jiffies.h>
+#include <linux/kernel.h>
+#include <linux/math64.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/reset.h>
+
+/* Register offsets */
+#define IICB0DAT	0x00		/* Data Register */
+#define IICB0CTL0	0x08		/* Control Register 0 */
+#define IICB0TRG	0x0C		/* Trigger Register */
+#define IICB0STR0	0x10		/* Status Register 0 */
+#define IICB0CTL1	0x20		/* Control Register 1 */
+#define IICB0WL		0x24		/* Low Level Width Setting Reg */
+#define IICB0WH		0x28		/* How Level Width Setting Reg */
+
+/* IICB0CTL0 */
+#define IICB0IICE	BIT(7)		/* I2C Enable */
+#define IICB0SLWT	BIT(1)		/* Interrupt Request Timing */
+#define IICB0SLAC	BIT(0)		/* Acknowledge */
+
+/* IICB0TRG */
+#define IICB0WRET	BIT(2)		/* Quit Wait Trigger */
+#define IICB0STT	BIT(1)		/* Create Start Condition Trigger */
+#define IICB0SPT	BIT(0)		/* Create Stop Condition Trigger */
+
+/* IICB0STR0 */
+#define IICB0SSAC	BIT(8)		/* Ack Flag */
+#define IICB0SSBS	BIT(6)		/* Bus Flag */
+#define IICB0SSSP	BIT(4)		/* Stop Condition Flag */
+
+/* IICB0CTL1 */
+#define IICB0MDSC	BIT(7)		/* Bus Mode */
+#define IICB0SLSE	BIT(1)		/* Start condition output */
+
+#define rzv2m_i2c_priv_to_dev(p)	((p)->adap.dev.parent)
+
+#define bit_setl(addr, val)		writel(readl(addr) | (val), (addr))
+#define bit_clrl(addr, val)		writel(readl(addr) & ~(val), (addr))
+
+struct rzv2m_i2c_priv {
+	void __iomem *base;
+	struct i2c_adapter adap;
+	struct clk *clk;
+	int bus_mode;
+	struct completion msg_tia_done;
+	u32 iicb0wl;
+	u32 iicb0wh;
+};
+
+enum bcr_index {
+	RZV2M_I2C_100K = 0,
+	RZV2M_I2C_400K,
+};
+
+struct bitrate_config {
+	unsigned int percent_low;
+	unsigned int min_hold_time_ns;
+};
+
+static const struct bitrate_config bitrate_configs[] = {
+	[RZV2M_I2C_100K] = { 47, 3450 },
+	[RZV2M_I2C_400K] = { 52, 900},
+};
+
+static irqreturn_t rzv2m_i2c_tia_irq_handler(int this_irq, void *dev_id)
+{
+	struct rzv2m_i2c_priv *priv = dev_id;
+
+	complete(&priv->msg_tia_done);
+
+	return IRQ_HANDLED;
+}
+
+/* Calculate IICB0WL and IICB0WH */
+static int rzv2m_i2c_clock_calculate(struct device *dev,
+				     struct rzv2m_i2c_priv *priv)
+{
+	const struct bitrate_config *config;
+	unsigned int hold_time_ns;
+	unsigned int total_pclks;
+	unsigned int trf_pclks;
+	unsigned long pclk_hz;
+	struct i2c_timings t;
+	u32 trf_ns;
+
+	i2c_parse_fw_timings(dev, &t, true);
+
+	pclk_hz = clk_get_rate(priv->clk);
+	total_pclks = pclk_hz / t.bus_freq_hz;
+
+	trf_ns = t.scl_rise_ns + t.scl_fall_ns;
+	trf_pclks = mul_u64_u32_div(pclk_hz, trf_ns, NSEC_PER_SEC);
+
+	/* Config setting */
+	switch (t.bus_freq_hz) {
+	case I2C_MAX_FAST_MODE_FREQ:
+		priv->bus_mode = RZV2M_I2C_400K;
+		break;
+	case I2C_MAX_STANDARD_MODE_FREQ:
+		priv->bus_mode = RZV2M_I2C_100K;
+		break;
+	default:
+		dev_err(dev, "transfer speed is invalid\n");
+		return -EINVAL;
+	}
+	config = &bitrate_configs[priv->bus_mode];
+
+	/* IICB0WL = (percent_low / Transfer clock) x PCLK */
+	priv->iicb0wl = total_pclks * config->percent_low / 100;
+	if (priv->iicb0wl > 0x3ff)
+		return -EINVAL;
+
+	/* IICB0WH = ((percent_high / Transfer clock) x PCLK) - (tR + tF) */
+	priv->iicb0wh = total_pclks - priv->iicb0wl - trf_pclks;
+	if (priv->iicb0wh > 0x3ff)
+		return -EINVAL;
+
+	/*
+	 * Data hold time must be less than 0.9us in fast mode and
+	 * 3.45us in standard mode.
+	 * Data hold time = IICB0WL[9:2] / PCLK
+	 */
+	hold_time_ns = div64_ul((u64)(priv->iicb0wl >> 2) * NSEC_PER_SEC, pclk_hz);
+	if (hold_time_ns > config->min_hold_time_ns) {
+		dev_err(dev, "data hold time %dns is over %dns\n",
+			hold_time_ns, config->min_hold_time_ns);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void rzv2m_i2c_init(struct rzv2m_i2c_priv *priv)
+{
+	u32 i2c_ctl0;
+	u32 i2c_ctl1;
+
+	/* i2c disable */
+	writel(0, priv->base + IICB0CTL0);
+
+	/* IICB0CTL1 setting */
+	i2c_ctl1 = IICB0SLSE;
+	if (priv->bus_mode == RZV2M_I2C_400K)
+		i2c_ctl1 |= IICB0MDSC;
+	writel(i2c_ctl1, priv->base + IICB0CTL1);
+
+	/* IICB0WL IICB0WH setting */
+	writel(priv->iicb0wl, priv->base + IICB0WL);
+	writel(priv->iicb0wh, priv->base + IICB0WH);
+
+	/* i2c enable after setting */
+	i2c_ctl0 = IICB0SLWT | IICB0SLAC | IICB0IICE;
+	writel(i2c_ctl0, priv->base + IICB0CTL0);
+}
+
+static int rzv2m_i2c_write_with_ack(struct rzv2m_i2c_priv *priv, u32 data)
+{
+	unsigned long time_left;
+
+	reinit_completion(&priv->msg_tia_done);
+
+	writel(data, priv->base + IICB0DAT);
+
+	time_left = wait_for_completion_timeout(&priv->msg_tia_done,
+						priv->adap.timeout);
+	if (!time_left)
+		return -ETIMEDOUT;
+
+	/* Confirm ACK */
+	if ((readl(priv->base + IICB0STR0) & IICB0SSAC) != IICB0SSAC)
+		return -ENXIO;
+
+	return 0;
+}
+
+static int rzv2m_i2c_read_with_ACK(struct rzv2m_i2c_priv *priv, u8 *data,
+				   bool last)
+{
+	unsigned long time_left;
+	u32 data_tmp;
+
+	reinit_completion(&priv->msg_tia_done);
+
+	/* Interrupt request timing : 8th clock */
+	bit_clrl(priv->base + IICB0CTL0, IICB0SLWT);
+
+	/* Exit the wait state */
+	writel(IICB0WRET, priv->base + IICB0TRG);
+
+	/* Wait for transaction */
+	time_left = wait_for_completion_timeout(&priv->msg_tia_done,
+						priv->adap.timeout);
+	if (!time_left)
+		return -ETIMEDOUT;
+
+	if (!last) {
+		/* Read data */
+		data_tmp = readl(priv->base + IICB0DAT);
+	} else {
+		/* Disable ACK */
+		bit_clrl(priv->base + IICB0CTL0, IICB0SLAC);
+
+		/* Read data*/
+		data_tmp = readl(priv->base + IICB0DAT);
+
+		/* Interrupt request timing : 9th clock */
+		bit_setl(priv->base + IICB0CTL0, IICB0SLWT);
+
+		/* Exit the wait state */
+		writel(IICB0WRET, priv->base + IICB0TRG);
+
+		/* Wait for transaction */
+		time_left = wait_for_completion_timeout(&priv->msg_tia_done,
+							priv->adap.timeout);
+		if (!time_left)
+			return -ETIMEDOUT;
+
+		/* Enable ACK */
+		bit_setl(priv->base + IICB0CTL0, IICB0SLAC);
+	}
+
+	*data = data_tmp;
+
+	return 0;
+}
+
+static int rzv2m_i2c_send(struct rzv2m_i2c_priv *priv, struct i2c_msg *msg,
+			  unsigned int *count)
+{
+	unsigned int i;
+	int ret = 0;
+
+	for (i = 0; i < msg->len; i++) {
+		ret = rzv2m_i2c_write_with_ack(priv, msg->buf[i]);
+		if (ret < 0)
+			return ret;
+	}
+	*count = i;
+
+	return ret;
+}
+
+static int rzv2m_i2c_receive(struct rzv2m_i2c_priv *priv, struct i2c_msg *msg,
+			     unsigned int *count)
+{
+	unsigned int i;
+	int ret = 0;
+
+	for (i = 0; i < msg->len; i++) {
+		ret = rzv2m_i2c_read_with_ACK(priv, &msg->buf[i],
+					      ((msg->len - 1) == i));
+		if (ret < 0)
+			return ret;
+	}
+	*count = i;
+
+	return ret;
+}
+
+static int rzv2m_i2c_send_address(struct rzv2m_i2c_priv *priv,
+				  struct i2c_msg *msg)
+{
+	u32 addr;
+	int ret;
+
+	if (msg->flags & I2C_M_TEN) {
+		/* 10-bit address
+		 *   addr_1: 5'b11110 | addr[9:8] | (R/nW)
+		 *   addr_2: addr[7:0]
+		 */
+		addr = 0xf0 | ((msg->addr >> 7) & 0x06);
+		addr |= !!(msg->flags & I2C_M_RD);
+		/* Send 1st address(extend code) */
+		ret = rzv2m_i2c_write_with_ack(priv, addr);
+		if (ret == 0) {
+			/* Send 2nd address */
+			ret = rzv2m_i2c_write_with_ack(priv, msg->addr & 0xff);
+		}
+	} else {
+		/* 7-bit address */
+		addr = i2c_8bit_addr_from_msg(msg);
+		ret = rzv2m_i2c_write_with_ack(priv, addr);
+	}
+
+	return ret;
+}
+
+static int rzv2m_i2c_stop_condition(struct rzv2m_i2c_priv *priv)
+{
+	u32 value;
+
+	/* Send stop condition */
+	writel(IICB0SPT, priv->base + IICB0TRG);
+	return readl_poll_timeout(priv->base + IICB0STR0,
+				  value, value & IICB0SSSP,
+				  100, jiffies_to_usecs(priv->adap.timeout));
+}
+
+static int rzv2m_i2c_master_xfer1(struct rzv2m_i2c_priv *priv,
+				  struct i2c_msg *msg, int stop)
+{
+	unsigned int count = 0;
+	int ret, read = !!(msg->flags & I2C_M_RD);
+
+	/* Send start condition */
+	writel(IICB0STT, priv->base + IICB0TRG);
+
+	ret = rzv2m_i2c_send_address(priv, msg);
+	if (ret == 0) {
+		if (read)
+			ret = rzv2m_i2c_receive(priv, msg, &count);
+		else
+			ret = rzv2m_i2c_send(priv, msg, &count);
+
+		if ((ret == 0) && stop)
+			ret = rzv2m_i2c_stop_condition(priv);
+	}
+
+	if (ret == -ENXIO)
+		rzv2m_i2c_stop_condition(priv);
+	else if (ret < 0)
+		rzv2m_i2c_init(priv);
+	else
+		ret = count;
+
+	return ret;
+}
+
+static int rzv2m_i2c_master_xfer(struct i2c_adapter *adap,
+				 struct i2c_msg *msgs, int num)
+{
+	struct rzv2m_i2c_priv *priv = i2c_get_adapdata(adap);
+	struct device *dev = rzv2m_i2c_priv_to_dev(priv);
+	unsigned int i;
+	int ret;
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret < 0)
+		return ret;
+
+	if (readl(priv->base + IICB0STR0) & IICB0SSBS) {
+		ret = -EAGAIN;
+		goto out;
+	}
+
+	/* I2C main transfer */
+	for (i = 0; i < num; i++) {
+		ret = rzv2m_i2c_master_xfer1(priv, &msgs[i], (i == (num - 1)));
+		if (ret < 0)
+			goto out;
+	}
+	ret = num;
+
+out:
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	return ret;
+}
+
+static u32 rzv2m_i2c_func(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK) |
+	       I2C_FUNC_10BIT_ADDR;
+}
+
+static const struct i2c_adapter_quirks rzv2m_i2c_quirks = {
+	.flags = I2C_AQ_NO_ZERO_LEN,
+};
+
+static struct i2c_algorithm rzv2m_i2c_algo = {
+	.master_xfer = rzv2m_i2c_master_xfer,
+	.functionality = rzv2m_i2c_func,
+};
+
+static const struct of_device_id rzv2m_i2c_ids[] = {
+	{ .compatible = "renesas,rzv2m-i2c", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, rzv2m_i2c_ids);
+
+static int rzv2m_i2c_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct rzv2m_i2c_priv *priv;
+	struct reset_control *rstc;
+	struct i2c_adapter *adap;
+	struct resource *res;
+	int irq, ret;
+
+	priv = devm_kzalloc(dev, sizeof(struct rzv2m_i2c_priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	priv->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(priv->clk)) {
+		dev_err_probe(dev, PTR_ERR(priv->clk), "Can't get clock\n");
+		return PTR_ERR(priv->clk);
+	}
+
+	rstc = devm_reset_control_get(dev, NULL);
+	if (IS_ERR(rstc)) {
+		dev_err_probe(dev, PTR_ERR(rstc), "Missing reset ctrl\n");
+		return PTR_ERR(rstc);
+	}
+	/*
+	 * The reset also affects other HW that is not under the control
+	 * of Linux. Therefore, all we can do is deassert the reset.
+	 */
+	reset_control_deassert(rstc);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	ret = devm_request_irq(dev, irq, rzv2m_i2c_tia_irq_handler, 0,
+			       dev_name(dev), priv);
+	if (ret < 0) {
+		dev_err_probe(dev, ret, "Unable to request irq %d\n", irq);
+		return ret;
+	}
+
+	adap = &priv->adap;
+	adap->nr = pdev->id;
+	adap->algo = &rzv2m_i2c_algo;
+	adap->quirks = &rzv2m_i2c_quirks;
+	adap->class = I2C_CLASS_DEPRECATED;
+	adap->dev.parent = dev;
+	adap->dev.of_node = dev->of_node;
+	adap->owner = THIS_MODULE;
+	i2c_set_adapdata(adap, priv);
+	strscpy(adap->name, pdev->name, sizeof(adap->name));
+	init_completion(&priv->msg_tia_done);
+
+	ret = rzv2m_i2c_clock_calculate(dev, priv);
+	if (ret < 0)
+		return ret;
+
+	pm_runtime_enable(dev);
+
+	pm_runtime_get_sync(dev);
+	rzv2m_i2c_init(priv);
+	pm_runtime_put(dev);
+
+	platform_set_drvdata(pdev, priv);
+
+	ret = i2c_add_numbered_adapter(adap);
+	if (ret < 0)
+		pm_runtime_disable(dev);
+
+	return ret;
+}
+
+static int rzv2m_i2c_remove(struct platform_device *pdev)
+{
+	struct rzv2m_i2c_priv *priv = platform_get_drvdata(pdev);
+	struct device *dev = rzv2m_i2c_priv_to_dev(priv);
+
+	i2c_del_adapter(&priv->adap);
+	bit_clrl(priv->base + IICB0CTL0, IICB0IICE);
+	pm_runtime_disable(dev);
+
+	return 0;
+}
+
+static int rzv2m_i2c_suspend(struct device *dev)
+{
+	struct rzv2m_i2c_priv *priv = dev_get_drvdata(dev);
+
+	pm_runtime_get_sync(dev);
+	bit_clrl(priv->base + IICB0CTL0, IICB0IICE);
+	pm_runtime_put(dev);
+
+	return 0;
+}
+
+static int rzv2m_i2c_resume(struct device *dev)
+{
+	struct rzv2m_i2c_priv *priv = dev_get_drvdata(dev);
+	int ret;
+
+	ret = rzv2m_i2c_clock_calculate(dev, priv);
+	if (ret < 0)
+		return ret;
+
+	pm_runtime_get_sync(dev);
+	rzv2m_i2c_init(priv);
+	pm_runtime_put(dev);
+
+	return 0;
+}
+
+static const struct dev_pm_ops rzv2m_i2c_pm_ops = {
+	NOIRQ_SYSTEM_SLEEP_PM_OPS(rzv2m_i2c_suspend, rzv2m_i2c_resume)
+};
+
+static struct platform_driver rzv2m_i2c_driver = {
+	.driver = {
+		.name = "rzv2m-i2c",
+		.of_match_table = rzv2m_i2c_ids,
+		.pm = pm_sleep_ptr(&rzv2m_i2c_pm_ops),
+	},
+	.probe	= rzv2m_i2c_probe,
+	.remove	= rzv2m_i2c_remove,
+};
+module_platform_driver(rzv2m_i2c_driver);
+
+MODULE_DESCRIPTION("RZ/V2M I2C bus driver");
+MODULE_AUTHOR("Renesas Electronics Corporation");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [PATCH v2 2/2] i2c: Add Renesas RZ/V2M controller

[Andy Shevchenko]

On Tue, Jun 28, 2022 at 08:45:26PM +0100, Phil Edworthy wrote:
> Yet another i2c controller from Renesas that is found on the RZ/V2M
> (r9a09g011) SoC. It can support only 100kHz and 400KHz operation.

...

> +#include <linux/of_device.h>

No user of this header.

But missed mod_devicetable.h (Okay, for I2C drivers we usually rely on i2c.h to
include it for us, but it's cleaner to include directly).

...

> +#define rzv2m_i2c_priv_to_dev(p)	((p)->adap.dev.parent)

It's longer than the actual expression. Why do you need this?!

...

> +static const struct bitrate_config bitrate_configs[] = {
> +	[RZV2M_I2C_100K] = { 47, 3450 },
> +	[RZV2M_I2C_400K] = { 52, 900},

Missed space.

> +};

...

> +	if (priv->iicb0wl > 0x3ff)

GENMASK() ?
Or (BIT(x) - 1) in case to tell that there is an HW limitation of x bits?

> +		return -EINVAL;

...

> +	if (priv->iicb0wh > 0x3ff)

Ditto.

> +		return -EINVAL;

...

> +	if (!last) {

Why not positive conditional?

> +	} else {

> +	}

...

> +static int rzv2m_i2c_send(struct rzv2m_i2c_priv *priv, struct i2c_msg *msg,
> +			  unsigned int *count)
> +{
> +	unsigned int i;
> +	int ret = 0;

Redundant assignment, you may return 0 directly.
Ditto for other similar cases in other functions.

> +	for (i = 0; i < msg->len; i++) {
> +		ret = rzv2m_i2c_write_with_ack(priv, msg->buf[i]);
> +		if (ret < 0)
> +			return ret;
> +	}
> +	*count = i;
> +
> +	return ret;
> +}

...

> +		ret = rzv2m_i2c_read_with_ACK(priv, &msg->buf[i],
> +					      ((msg->len - 1) == i));

Too many parentheses.

> +		if (ret < 0)
> +			return ret;

...

> +static int rzv2m_i2c_send_address(struct rzv2m_i2c_priv *priv,
> +				  struct i2c_msg *msg)
> +{
> +	u32 addr;
> +	int ret;
> +
> +	if (msg->flags & I2C_M_TEN) {
> +		/* 10-bit address
> +		 *   addr_1: 5'b11110 | addr[9:8] | (R/nW)
> +		 *   addr_2: addr[7:0]
> +		 */
> +		addr = 0xf0 | ((msg->addr >> 7) & 0x06);
> +		addr |= !!(msg->flags & I2C_M_RD);
> +		/* Send 1st address(extend code) */
> +		ret = rzv2m_i2c_write_with_ack(priv, addr);

	if (ret)
		return ret;

> +		if (ret == 0) {
> +			/* Send 2nd address */
> +			ret = rzv2m_i2c_write_with_ack(priv, msg->addr & 0xff);
> +		}
> +	} else {
> +		/* 7-bit address */
> +		addr = i2c_8bit_addr_from_msg(msg);
> +		ret = rzv2m_i2c_write_with_ack(priv, addr);
> +	}
> +
> +	return ret;
> +}

...

> +	ret = rzv2m_i2c_send_address(priv, msg);
> +	if (ret == 0) {

This is a bit confusing if it's only comparison with "no error code" condition.
Use if (!ret) if there is no meaning of positive value. Same applies to other
cases in the code.

> +		if (read)
> +			ret = rzv2m_i2c_receive(priv, msg, &count);
> +		else
> +			ret = rzv2m_i2c_send(priv, msg, &count);
> +
> +		if ((ret == 0) && stop)

Like here.

> +			ret = rzv2m_i2c_stop_condition(priv);
> +	}

...

> +static const struct of_device_id rzv2m_i2c_ids[] = {
> +	{ .compatible = "renesas,rzv2m-i2c", },

Inner comma is not needed.

> +	{ }
> +};

...

> +	priv->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(priv->clk)) {
> +		dev_err_probe(dev, PTR_ERR(priv->clk), "Can't get clock\n");
> +		return PTR_ERR(priv->clk);
> +	}

Why not

	return dev_err_probe(...);

?

Ditto for the rest cases like this.

...

> +	adap->dev.of_node = dev->of_node;

device_set_node()


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] dt-bindings: i2c: Document RZ/V2M I2C controller

[Rob Herring]

On Tue, 28 Jun 2022 20:45:25 +0100, Phil Edworthy wrote:
> Document Renesas RZ/V2M (r9a09g011) I2C controller bindings.
> 
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v2:
>  - Use an enum and set the default for clock-frequency
>  - Add resets property
> ---
>  .../bindings/i2c/renesas,rzv2m.yaml           | 80 +++++++++++++++++++
>  1 file changed, 80 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/renesas,rzv2m.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml: patternProperties:^thermistor@:properties:adi,excitation-current-nanoamp: '$ref' should not be valid under {'const': '$ref'}
	hint: Standard unit suffix properties don't need a type $ref
	from schema $id: http://devicetree.org/meta-schemas/core.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml: ignoring, error in schema: patternProperties: ^thermistor@: properties: adi,excitation-current-nanoamp
Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.example.dtb:0:0: /example-0/spi/ltc2983@0: failed to match any schema with compatible: ['adi,ltc2983']

doc reference errors (make refcheckdocs):

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

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 v2 2/2] i2c: Add Renesas RZ/V2M controller

[Geert Uytterhoeven]

Hi Andy,

On Tue, Jun 28, 2022 at 11:08 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Tue, Jun 28, 2022 at 08:45:26PM +0100, Phil Edworthy wrote:
> > Yet another i2c controller from Renesas that is found on the RZ/V2M
> > (r9a09g011) SoC. It can support only 100kHz and 400KHz operation.

> > +static int rzv2m_i2c_send(struct rzv2m_i2c_priv *priv, struct i2c_msg *msg,
> > +                       unsigned int *count)
> > +{
> > +     unsigned int i;
> > +     int ret = 0;
>
> Redundant assignment, you may return 0 directly.

Can you prove msg->len is never zero, and the loop below is always
executed at least once?
The driver does set I2C_AQ_NO_ZERO_LEN, but I don't think the static
checkers know ;-)

>
> > +     for (i = 0; i < msg->len; i++) {
> > +             ret = rzv2m_i2c_write_with_ack(priv, msg->buf[i]);
> > +             if (ret < 0)
> > +                     return ret;
> > +     }
> > +     *count = i;
> > +
> > +     return ret;
> > +}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 1/2] dt-bindings: i2c: Document RZ/V2M I2C controller

[Geert Uytterhoeven]

Hi Rob,

On Wed, Jun 29, 2022 at 4:09 AM Rob Herring <robh@kernel.org> wrote:
> On Tue, 28 Jun 2022 20:45:25 +0100, Phil Edworthy wrote:
> > Document Renesas RZ/V2M (r9a09g011) I2C controller bindings.
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > v2:
> >  - Use an enum and set the default for clock-frequency
> >  - Add resets property
> > ---
> >  .../bindings/i2c/renesas,rzv2m.yaml           | 80 +++++++++++++++++++
> >  1 file changed, 80 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/i2c/renesas,rzv2m.yaml
> >
>
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>
> yamllint warnings/errors:
>
> dtschema/dtc warnings/errors:
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml: patternProperties:^thermistor@:properties:adi,excitation-current-nanoamp: '$ref' should not be valid under {'const': '$ref'}
>         hint: Standard unit suffix properties don't need a type $ref
>         from schema $id: http://devicetree.org/meta-schemas/core.yaml#
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml: ignoring, error in schema: patternProperties: ^thermistor@: properties: adi,excitation-current-nanoamp
> Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.example.dtb:0:0: /example-0/spi/ltc2983@0: failed to match any schema with compatible: ['adi,ltc2983']

All of these look like false-positives, i.e. not related to this patch?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 1/2] dt-bindings: i2c: Document RZ/V2M I2C controller

[Krzysztof Kozlowski]

On 29/06/2022 08:53, Geert Uytterhoeven wrote:
> Hi Rob,
> 
> On Wed, Jun 29, 2022 at 4:09 AM Rob Herring <robh@kernel.org> wrote:
>> On Tue, 28 Jun 2022 20:45:25 +0100, Phil Edworthy wrote:
>>> Document Renesas RZ/V2M (r9a09g011) I2C controller bindings.
>>>
>>> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
>>> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
>>> ---
>>> v2:
>>>  - Use an enum and set the default for clock-frequency
>>>  - Add resets property
>>> ---
>>>  .../bindings/i2c/renesas,rzv2m.yaml           | 80 +++++++++++++++++++
>>>  1 file changed, 80 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/i2c/renesas,rzv2m.yaml
>>>
>>
>> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
>> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>>
>> yamllint warnings/errors:
>>
>> dtschema/dtc warnings/errors:
>> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml: patternProperties:^thermistor@:properties:adi,excitation-current-nanoamp: '$ref' should not be valid under {'const': '$ref'}
>>         hint: Standard unit suffix properties don't need a type $ref
>>         from schema $id: http://devicetree.org/meta-schemas/core.yaml#
>> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml: ignoring, error in schema: patternProperties: ^thermistor@: properties: adi,excitation-current-nanoamp
>> Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.example.dtb:0:0: /example-0/spi/ltc2983@0: failed to match any schema with compatible: ['adi,ltc2983']
> 
> All of these look like false-positives, i.e. not related to this patch?

Few other patches also got it, I think the bot got some problem.


Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: i2c: Document RZ/V2M I2C controller

[Krzysztof Kozlowski]

On 28/06/2022 21:45, Phil Edworthy wrote:
> Document Renesas RZ/V2M (r9a09g011) I2C controller bindings.
> 
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v2:
>  - Use an enum and set the default for clock-frequency
>  - Add resets property
> ---
>  .../bindings/i2c/renesas,rzv2m.yaml           | 80 +++++++++++++++++++
>  1 file changed, 80 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/renesas,rzv2m.yaml
> 
> diff --git a/Documentation/devicetree/bindings/i2c/renesas,rzv2m.yaml b/Documentation/devicetree/bindings/i2c/renesas,rzv2m.yaml
> new file mode 100644
> index 000000000000..7f6d2bb4ecb3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/renesas,rzv2m.yaml
> @@ -0,0 +1,80 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i2c/renesas,rzv2m.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas RZ/V2M I2C Bus Interface
> +
> +maintainers:
> +  - Phil Edworthy <phil.edworthy@renesas.com>
> +
> +allOf:
> +  - $ref: /schemas/i2c/i2c-controller.yaml#
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - renesas,i2c-r9a09g011  # RZ/V2M
> +      - const: renesas,rzv2m-i2c
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    items:
> +      - description: Data transmission/reception interrupt
> +      - description: Status interrupt
> +
> +  interrupt-names:
> +    items:
> +      - const: tia
> +      - const: tis
> +
> +  clock-frequency:
> +    default: 100000
> +    enum: [ 100000, 400000 ]
> +    description:
> +      Desired I2C bus clock frequency in Hz.
> +
> +  clocks:
> +    maxItems: 1
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - interrupt-names
> +  - clocks
> +  - power-domains
> +  - resets
> +  - '#address-cells'
> +  - '#size-cells'
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/r9a09g011-cpg.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    i2c0: i2c@a4030000 {
> +            compatible = "renesas,i2c-r9a09g011", "renesas,rzv2m-i2c";

I missed that part in last version - you have some weird indentation
here. Use 4 spaces for DTS example.

Best regards,
Krzysztof

Re: [PATCH v2 2/2] i2c: Add Renesas RZ/V2M controller

[Andy Shevchenko]

On Wed, Jun 29, 2022 at 08:52:27AM +0200, Geert Uytterhoeven wrote:
> On Tue, Jun 28, 2022 at 11:08 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Tue, Jun 28, 2022 at 08:45:26PM +0100, Phil Edworthy wrote:

...

> > > +static int rzv2m_i2c_send(struct rzv2m_i2c_priv *priv, struct i2c_msg *msg,
> > > +                       unsigned int *count)
> > > +{
> > > +     unsigned int i;
> > > +     int ret = 0;
> >
> > Redundant assignment, you may return 0 directly.
> 
> Can you prove msg->len is never zero, and the loop below is always
> executed at least once?

I don't see how this is related. The ret is used only in the loop body,

	return 0;

outside will suffice. No?

> The driver does set I2C_AQ_NO_ZERO_LEN, but I don't think the static
> checkers know ;-)
> 
> >
> > > +     for (i = 0; i < msg->len; i++) {
> > > +             ret = rzv2m_i2c_write_with_ack(priv, msg->buf[i]);
> > > +             if (ret < 0)
> > > +                     return ret;
> > > +     }
> > > +     *count = i;
> > > +
> > > +     return ret;
> > > +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] dt-bindings: i2c: Document RZ/V2M I2C controller

[Rob Herring]

On Wed, Jun 29, 2022 at 2:15 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 29/06/2022 08:53, Geert Uytterhoeven wrote:
> > Hi Rob,
> >
> > On Wed, Jun 29, 2022 at 4:09 AM Rob Herring <robh@kernel.org> wrote:
> >> On Tue, 28 Jun 2022 20:45:25 +0100, Phil Edworthy wrote:
> >>> Document Renesas RZ/V2M (r9a09g011) I2C controller bindings.
> >>>
> >>> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> >>> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> >>> ---
> >>> v2:
> >>>  - Use an enum and set the default for clock-frequency
> >>>  - Add resets property
> >>> ---
> >>>  .../bindings/i2c/renesas,rzv2m.yaml           | 80 +++++++++++++++++++
> >>>  1 file changed, 80 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/i2c/renesas,rzv2m.yaml
> >>>
> >>
> >> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> >> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> >>
> >> yamllint warnings/errors:
> >>
> >> dtschema/dtc warnings/errors:
> >> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml: patternProperties:^thermistor@:properties:adi,excitation-current-nanoamp: '$ref' should not be valid under {'const': '$ref'}
> >>         hint: Standard unit suffix properties don't need a type $ref
> >>         from schema $id: http://devicetree.org/meta-schemas/core.yaml#
> >> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml: ignoring, error in schema: patternProperties: ^thermistor@: properties: adi,excitation-current-nanoamp
> >> Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.example.dtb:0:0: /example-0/spi/ltc2983@0: failed to match any schema with compatible: ['adi,ltc2983']
> >
> > All of these look like false-positives, i.e. not related to this patch?
>
> Few other patches also got it, I think the bot got some problem.

Yes, and the bot's overlord failed to see that too. A change yesterday
in dtschema main branch introduced a new warning and that requires
clearing the CI cache which I didn't do til now.

Rob

Re: [PATCH v2 2/2] i2c: Add Renesas RZ/V2M controller

[Geert Uytterhoeven]

Hi Andy,

On Wed, Jun 29, 2022 at 12:46 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Wed, Jun 29, 2022 at 08:52:27AM +0200, Geert Uytterhoeven wrote:
> > On Tue, Jun 28, 2022 at 11:08 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Tue, Jun 28, 2022 at 08:45:26PM +0100, Phil Edworthy wrote:
>
> ...
>
> > > > +static int rzv2m_i2c_send(struct rzv2m_i2c_priv *priv, struct i2c_msg *msg,
> > > > +                       unsigned int *count)
> > > > +{
> > > > +     unsigned int i;
> > > > +     int ret = 0;
> > >
> > > Redundant assignment, you may return 0 directly.
> >
> > Can you prove msg->len is never zero, and the loop below is always
> > executed at least once?
>
> I don't see how this is related. The ret is used only in the loop body,
>
>         return 0;
>
> outside will suffice. No?

Right, with the "return ret" in the end replaced.

> > The driver does set I2C_AQ_NO_ZERO_LEN, but I don't think the static
> > checkers know ;-)
> >
> > >
> > > > +     for (i = 0; i < msg->len; i++) {
> > > > +             ret = rzv2m_i2c_write_with_ack(priv, msg->buf[i]);
> > > > +             if (ret < 0)
> > > > +                     return ret;
> > > > +     }
> > > > +     *count = i;
> > > > +
> > > > +     return ret;
> > > > +}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 2/2] i2c: Add Renesas RZ/V2M controller

[Andy Shevchenko]

On Wed, Jun 29, 2022 at 05:58:39PM +0200, Geert Uytterhoeven wrote:
> On Wed, Jun 29, 2022 at 12:46 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Wed, Jun 29, 2022 at 08:52:27AM +0200, Geert Uytterhoeven wrote:
> > > On Tue, Jun 28, 2022 at 11:08 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Tue, Jun 28, 2022 at 08:45:26PM +0100, Phil Edworthy wrote:

...

> > > > > +static int rzv2m_i2c_send(struct rzv2m_i2c_priv *priv, struct i2c_msg *msg,
> > > > > +                       unsigned int *count)
> > > > > +{
> > > > > +     unsigned int i;
> > > > > +     int ret = 0;
> > > >
> > > > Redundant assignment, you may return 0 directly.

^^^

> > > Can you prove msg->len is never zero, and the loop below is always
> > > executed at least once?
> >
> > I don't see how this is related. The ret is used only in the loop body,
> >
> >         return 0;
> >
> > outside will suffice. No?
> 
> Right, with the "return ret" in the end replaced.

Exactly what I meant above with "you may return 0 directly".

> > > The driver does set I2C_AQ_NO_ZERO_LEN, but I don't think the static
> > > checkers know ;-)
> > >
> > > > > +     for (i = 0; i < msg->len; i++) {
> > > > > +             ret = rzv2m_i2c_write_with_ack(priv, msg->buf[i]);
> > > > > +             if (ret < 0)
> > > > > +                     return ret;
> > > > > +     }
> > > > > +     *count = i;
> > > > > +
> > > > > +     return ret;
> > > > > +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/2] i2c: Add new driver for Renesas RZ/V2M controller

[Philipp Zabel]

Hi Phil,

On Di, 2022-06-28 at 20:45 +0100, Phil Edworthy wrote:
> Hi,
> 
> The Renesas RZ/V2M SoC (r9a09g011) has a new i2c controller. This series
> add the driver. One annoying problem is that the SoC uses a single reset
> line for two i2c controllers, and unfortunately one of the controllers
> is managed by some firmware, not by Linux. Therefore, the driver just
> deasserts the reset.

This sounds scary. If the driver is never loaded, and the reset is
never deasserted, what happens to the firmware trying to access the
other i2c controller? Does it hang? Or write to the reset controller
registers to deassert the reset? If so, is there any protection against
concurrent access from firmware and reset controller driver?

regards
Philipp

Re: [PATCH v2 2/2] i2c: Add Renesas RZ/V2M controller

[Philipp Zabel]

On Di, 2022-06-28 at 20:45 +0100, Phil Edworthy wrote:
> Yet another i2c controller from Renesas that is found on the RZ/V2M
> (r9a09g011) SoC. It can support only 100kHz and 400KHz operation.
> 
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v2:
>  - Use the new NOIRQ_SYSTEM_SLEEP_PM_OPS() as suggested by Arnd
>  - Lots of small fixes based on Geert's review
> ---
>  drivers/i2c/busses/Kconfig     |  10 +
>  drivers/i2c/busses/Makefile    |   1 +
>  drivers/i2c/busses/i2c-rzv2m.c | 530 +++++++++++++++++++++++++++++++++
>  3 files changed, 541 insertions(+)
>  create mode 100644 drivers/i2c/busses/i2c-rzv2m.c
> 
[...]
> diff --git a/drivers/i2c/busses/i2c-rzv2m.c b/drivers/i2c/busses/i2c-rzv2m.c
> new file mode 100644
> index 000000000000..ab326d557dc5
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-rzv2m.c
> @@ -0,0 +1,530 @@
[...]
> +static int rzv2m_i2c_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct rzv2m_i2c_priv *priv;
> +	struct reset_control *rstc;
> +	struct i2c_adapter *adap;
> +	struct resource *res;
> +	int irq, ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(struct rzv2m_i2c_priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	priv->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(priv->clk)) {
> +		dev_err_probe(dev, PTR_ERR(priv->clk), "Can't get clock\n");
> +		return PTR_ERR(priv->clk);
> +	}
> +
> +	rstc = devm_reset_control_get(dev, NULL);

Please don't use devm_reset_control_get. This should probably be
devm_reset_control_get_shared().

regards
Philipp

Re: [PATCH v2 0/2] i2c: Add new driver for Renesas RZ/V2M controller

[Geert Uytterhoeven]

Hi Philipp,

On Wed, Jun 29, 2022 at 6:21 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> On Di, 2022-06-28 at 20:45 +0100, Phil Edworthy wrote:
> > The Renesas RZ/V2M SoC (r9a09g011) has a new i2c controller. This series
> > add the driver. One annoying problem is that the SoC uses a single reset
> > line for two i2c controllers, and unfortunately one of the controllers
> > is managed by some firmware, not by Linux. Therefore, the driver just
> > deasserts the reset.
>
> This sounds scary. If the driver is never loaded, and the reset is
> never deasserted, what happens to the firmware trying to access the
> other i2c controller? Does it hang? Or write to the reset controller
> registers to deassert the reset? If so, is there any protection against
> concurrent access from firmware and reset controller driver?


In response to v1, I wrote

| That is actually an integration issue, not an i2c controller issue.
|
| Perhaps we need a RESET_IS_CRITICAL flag, cfr. CLK_IS_CRITICAL,
| to be set by the reset provider?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v2 2/2] i2c: Add Renesas RZ/V2M controller

[Phil Edworthy]

Hi Andy,

Thanks for your review!

On 28 June 2022 22:08 Andy Shevchenko wrote:
> On Tue, Jun 28, 2022 at 08:45:26PM +0100, Phil Edworthy wrote:
> > Yet another i2c controller from Renesas that is found on the RZ/V2M
> > (r9a09g011) SoC. It can support only 100kHz and 400KHz operation.
> 
> ...
> 
> > +#include <linux/of_device.h>
> 
> No user of this header.
> 
> But missed mod_devicetable.h (Okay, for I2C drivers we usually rely on
> i2c.h to
> include it for us, but it's cleaner to include directly).
Ok

> > +#define rzv2m_i2c_priv_to_dev(p)	((p)->adap.dev.parent)
> 
> It's longer than the actual expression. Why do you need this?!
Ok, no need for it

> > +static const struct bitrate_config bitrate_configs[] = {
> > +	[RZV2M_I2C_100K] = { 47, 3450 },
> > +	[RZV2M_I2C_400K] = { 52, 900},
> 
> Missed space.
Ok

> > +	if (priv->iicb0wl > 0x3ff)
> 
> GENMASK() ?
> Or (BIT(x) - 1) in case to tell that there is an HW limitation of x bits?
Ok, BIT makes sense here

> > +	if (priv->iicb0wh > 0x3ff)
> 
> Ditto.
Ok

> > +	if (!last) {
> 
> Why not positive conditional?
Ok

> > +static int rzv2m_i2c_send(struct rzv2m_i2c_priv *priv, struct i2c_msg
> *msg,
> > +			  unsigned int *count)
> > +{
> > +	unsigned int i;
> > +	int ret = 0;
> 
> Redundant assignment, you may return 0 directly.
> Ditto for other similar cases in other functions.
Ok

> > +		ret = rzv2m_i2c_read_with_ACK(priv, &msg->buf[i],
> > +					      ((msg->len - 1) == i));
> 
> Too many parentheses.
Ok

> > +static int rzv2m_i2c_send_address(struct rzv2m_i2c_priv *priv,
> > +				  struct i2c_msg *msg)
> > +{
> > +	u32 addr;
> > +	int ret;
> > +
> > +	if (msg->flags & I2C_M_TEN) {
> > +		/* 10-bit address
> > +		 *   addr_1: 5'b11110 | addr[9:8] | (R/nW)
> > +		 *   addr_2: addr[7:0]
> > +		 */
> > +		addr = 0xf0 | ((msg->addr >> 7) & 0x06);
> > +		addr |= !!(msg->flags & I2C_M_RD);
> > +		/* Send 1st address(extend code) */
> > +		ret = rzv2m_i2c_write_with_ack(priv, addr);
> 
> 	if (ret)
> 		return ret;
Ok


> > +	ret = rzv2m_i2c_send_address(priv, msg);
> > +	if (ret == 0) {
> 
> This is a bit confusing if it's only comparison with "no error code"
> condition.
> Use if (!ret) if there is no meaning of positive value. Same applies to
> other
> cases in the code.
> 
> > +		if (read)
> > +			ret = rzv2m_i2c_receive(priv, msg, &count);
> > +		else
> > +			ret = rzv2m_i2c_send(priv, msg, &count);
> > +
> > +		if ((ret == 0) && stop)
> 
> Like here.
Sorry, I don't follow you. How is (ret == 0) any different to (!ret) ?



> > +static const struct of_device_id rzv2m_i2c_ids[] = {
> > +	{ .compatible = "renesas,rzv2m-i2c", },
> 
> Inner comma is not needed.
Ok

> > +	priv->clk = devm_clk_get(dev, NULL);
> > +	if (IS_ERR(priv->clk)) {
> > +		dev_err_probe(dev, PTR_ERR(priv->clk), "Can't get clock\n");
> > +		return PTR_ERR(priv->clk);
> > +	}
> 
> Why not
> 
> 	return dev_err_probe(...);
> 
> ?
> 
> Ditto for the rest cases like this.
Ok

> > +	adap->dev.of_node = dev->of_node;
> 
> device_set_node()
Since we don't need to set dev->fwnode, why is device_set_node() any
better?


Thanks
Phil

Re: [PATCH v2 2/2] i2c: Add Renesas RZ/V2M controller

[Andy Shevchenko]

On Thu, Jun 30, 2022 at 09:41:37AM +0000, Phil Edworthy wrote:
> On 28 June 2022 22:08 Andy Shevchenko wrote:
> > On Tue, Jun 28, 2022 at 08:45:26PM +0100, Phil Edworthy wrote:

...

> > > +	if (ret == 0) {
> > 
> > This is a bit confusing if it's only comparison with "no error code"
> > condition. Use if (!ret) if there is no meaning of positive value.

> > Same applies to other cases in the code.

^^^

> > > +		if (read)
> > > +			ret = rzv2m_i2c_receive(priv, msg, &count);
> > > +		else
> > > +			ret = rzv2m_i2c_send(priv, msg, &count);
> > > +
> > > +		if ((ret == 0) && stop)
> > 
> > Like here.
> Sorry, I don't follow you. How is (ret == 0) any different to (!ret) ?

See above.

...

> > > +	adap->dev.of_node = dev->of_node;
> > 
> > device_set_node()
> Since we don't need to set dev->fwnode, why is device_set_node() any
> better?

We may go for I2C subsystem that supports only fwnode (as others already or
almost about to do). This will reduce a lot of unneeded churn in the future.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v2 1/2] dt-bindings: i2c: Document RZ/V2M I2C controller

[Phil Edworthy]

Hi Krzysztof,

On 29 June 2022 09:23, Krzysztof Kozlowski wrote:
> On 28/06/2022 21:45, Phil Edworthy wrote:
> > Document Renesas RZ/V2M (r9a09g011) I2C controller bindings.
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > v2:
> >  - Use an enum and set the default for clock-frequency
> >  - Add resets property
> > ---
> >  .../bindings/i2c/renesas,rzv2m.yaml           | 80 +++++++++++++++++++
> >  1 file changed, 80 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/i2c/renesas,rzv2m.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/i2c/renesas,rzv2m.yaml
...

> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/r9a09g011-cpg.h>
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    i2c0: i2c@a4030000 {
> > +            compatible = "renesas,i2c-r9a09g011",
> > + "renesas,rzv2m-i2c";
> 
> I missed that part in last version - you have some weird indentation here.
> Use 4 spaces for DTS example.
Will do!

Thanks
Phil

RE: [PATCH v2 0/2] i2c: Add new driver for Renesas RZ/V2M controller

[Phil Edworthy]

Hi Philipp, Geert,

On 29 June 2022 18:18 Geert Uytterhoeven wrote:
> On Wed, Jun 29, 2022 at 6:21 PM Philipp Zabel wrote:
> > On Di, 2022-06-28 at 20:45 +0100, Phil Edworthy wrote:
> > > The Renesas RZ/V2M SoC (r9a09g011) has a new i2c controller. This
> series
> > > add the driver. One annoying problem is that the SoC uses a single
> reset
> > > line for two i2c controllers, and unfortunately one of the controllers
> > > is managed by some firmware, not by Linux. Therefore, the driver just
> > > deasserts the reset.
> >
> > This sounds scary. If the driver is never loaded, and the reset is
> > never deasserted, what happens to the firmware trying to access the
> > other i2c controller? Does it hang? Or write to the reset controller
> > registers to deassert the reset? If so, is there any protection against
> > concurrent access from firmware and reset controller driver?
Where a common reset is used by Linux and some firmware, I think we have to
ensure/assume that both only ever de-assert it.
In this particular SoC, the register used to assert/de-assert the reset
has write enable bits in the upper half of the reg. There shouldn't be any
issues with both trying to de-assert the reset at the same time.


> In response to v1, I wrote
> 
> | That is actually an integration issue, not an i2c controller issue.
> |
> | Perhaps we need a RESET_IS_CRITICAL flag, cfr. CLK_IS_CRITICAL,
> | to be set by the reset provider?

From what I understand, there are two main use cases for resets:
1. Often reset lines may be asserted at power on and so a driver needs to
   de-assert them so that the module can be used.
2. A driver may need to reset the module for some reason. I have only
   seen this with watchdog timers with no way out.

So if a driver does not need to reset the module, shouldn't the driver
only ever be de-asserting the reset line?
If so, it also doesn’t matter whether the reset is shared with other
modules or not.
If a driver needs to reset the module, then the reset cannot be shared
with other modules used by firmware or Linux, or we cannot use any
other modules that share the reset line.

Have I missed something?

Thanks
Phil

Re: [PATCH v2 0/2] i2c: Add new driver for Renesas RZ/V2M controller

[Philipp Zabel]

Hi Phil,

On Do, 2022-06-30 at 13:43 +0000, Phil Edworthy wrote:
> Hi Philipp, Geert,
> 
> On 29 June 2022 18:18 Geert Uytterhoeven wrote:
> > On Wed, Jun 29, 2022 at 6:21 PM Philipp Zabel wrote:
> > > On Di, 2022-06-28 at 20:45 +0100, Phil Edworthy wrote:
> > > > The Renesas RZ/V2M SoC (r9a09g011) has a new i2c controller. This
> > series
> > > > add the driver. One annoying problem is that the SoC uses a single
> > reset
> > > > line for two i2c controllers, and unfortunately one of the controllers
> > > > is managed by some firmware, not by Linux. Therefore, the driver just
> > > > deasserts the reset.
> > > 
> > > This sounds scary. If the driver is never loaded, and the reset is
> > > never deasserted, what happens to the firmware trying to access the
> > > other i2c controller? Does it hang? Or write to the reset controller
> > > registers to deassert the reset? If so, is there any protection against
> > > concurrent access from firmware and reset controller driver?
> Where a common reset is used by Linux and some firmware, I think we have to
> ensure/assume that both only ever de-assert it.

We also have to make sure that no read-modify-write cycles are required
to deassert the resets if we can't lock between firmware and kernel.
Otherwise concurrent access could cause a deassert to be reverted.

> In this particular SoC, the register used to assert/de-assert the reset
> has write enable bits in the upper half of the reg. There shouldn't be any
> issues with both trying to de-assert the reset at the same time.

Which reset driver is handling the reset for this i2c module?

> > In response to v1, I wrote
> > 
> > > That is actually an integration issue, not an i2c controller issue.
> > > 
> > > Perhaps we need a RESET_IS_CRITICAL flag, cfr. CLK_IS_CRITICAL,
> > > to be set by the reset provider?
> 
> From what I understand, there are two main use cases for resets:
> 1. Often reset lines may be asserted at power on and so a driver needs to
>    de-assert them so that the module can be used.

There are resets that are not initially asserted (among them the self-
deasserting resets) that are required to be asserted some time during
boot, to put some hardware into a well defined state.
I don't think those should be shared, but they sometimes are.

> 2. A driver may need to reset the module for some reason. I have only
>    seen this with watchdog timers with no way out.

Grep for device_reset() or reset_control_reset() for some examples.
Also there are quite a few assert/udelay/deassert calls in drivers.

Also many drivers assert the reset again during remove(). Whether that
is always necessary or useful, I can't say.

It's sometimes nice during development, to be able to reload a kernel
module or rebind a driver to reset some locked up hardware.

> So if a driver does not need to reset the module, shouldn't the driver
> only ever be de-asserting the reset line?

I'm not sure the driver can always know this if it is used on different
platforms.

> If so, it also doesn’t matter whether the reset is shared with other
> modules or not.
> If a driver needs to reset the module, then the reset cannot be shared
> with other modules used by firmware or Linux, or we cannot use any
> other modules that share the reset line.

It can be shared for the special case of multiple modules requiring a
shared reset line to be asserted once, at some time before the modules
are used. The reset controller API supports this for the
reset_control_reset() call.

regards
Philipp

RE: [PATCH v2 0/2] i2c: Add new driver for Renesas RZ/V2M controller

[Phil Edworthy]

Hi Philipp,

On 30 June 2022 15:45 Philipp Zabel wrote:
> On Do, 2022-06-30 at 13:43 +0000, Phil Edworthy wrote:
> > On 29 June 2022 18:18 Geert Uytterhoeven wrote:
> > > On Wed, Jun 29, 2022 at 6:21 PM Philipp Zabel wrote:
> > > > On Di, 2022-06-28 at 20:45 +0100, Phil Edworthy wrote:
> > > > > The Renesas RZ/V2M SoC (r9a09g011) has a new i2c controller. This
> > > series
> > > > > add the driver. One annoying problem is that the SoC uses a single
> > > reset
> > > > > line for two i2c controllers, and unfortunately one of the
> controllers
> > > > > is managed by some firmware, not by Linux. Therefore, the driver
> just
> > > > > deasserts the reset.
> > > >
> > > > This sounds scary. If the driver is never loaded, and the reset is
> > > > never deasserted, what happens to the firmware trying to access the
> > > > other i2c controller? Does it hang? Or write to the reset controller
> > > > registers to deassert the reset? If so, is there any protection
> against
> > > > concurrent access from firmware and reset controller driver?
> > Where a common reset is used by Linux and some firmware, I think we have
> to
> > ensure/assume that both only ever de-assert it.
> 
> We also have to make sure that no read-modify-write cycles are required
> to deassert the resets if we can't lock between firmware and kernel.
> Otherwise concurrent access could cause a deassert to be reverted.
Agreed


> > In this particular SoC, the register used to assert/de-assert the reset
> > has write enable bits in the upper half of the reg. There shouldn't be
> any
> > issues with both trying to de-assert the reset at the same time.
> 
> Which reset driver is handling the reset for this i2c module?
drivers/clk/renesas/rzg2l-cpg.c 
See rzg2l_cpg_assert() and rzg2l_cpg_deassert()
Note this driver handles a few different SoCs, the SoC using this i2c
driver is specified in drivers/clk/renesas/r9a09g011-cpg.c


> > > In response to v1, I wrote
> > >
> > > > That is actually an integration issue, not an i2c controller issue.
> > > >
> > > > Perhaps we need a RESET_IS_CRITICAL flag, cfr. CLK_IS_CRITICAL,
> > > > to be set by the reset provider?
> >
> > From what I understand, there are two main use cases for resets:
> > 1. Often reset lines may be asserted at power on and so a driver needs
> to
> >    de-assert them so that the module can be used.
> 
> There are resets that are not initially asserted (among them the self-
> deasserting resets) that are required to be asserted some time during
> boot, to put some hardware into a well defined state.
> I don't think those should be shared, but they sometimes are.
> 
> > 2. A driver may need to reset the module for some reason. I have only
> >    seen this with watchdog timers with no way out.
> 
> Grep for device_reset() or reset_control_reset() for some examples.
> Also there are quite a few assert/udelay/deassert calls in drivers.
Ok, though I'm not convinced that the driver specifying the reset
period is the right way to support lots of different platforms.


> Also many drivers assert the reset again during remove(). Whether that
> is always necessary or useful, I can't say.
Quite. It's difficult to know if the module requires a reset or that's
just what the driver developer used.


> It's sometimes nice during development, to be able to reload a kernel
> module or rebind a driver to reset some locked up hardware.
Ok, that's a good use case!


> > So if a driver does not need to reset the module, shouldn't the driver
> > only ever be de-asserting the reset line?
> 
> I'm not sure the driver can always know this if it is used on different
> platforms.
> 
> > If so, it also doesn’t matter whether the reset is shared with other
> > modules or not.
> > If a driver needs to reset the module, then the reset cannot be shared
> > with other modules used by firmware or Linux, or we cannot use any
> > other modules that share the reset line.
> 
> It can be shared for the special case of multiple modules requiring a
> shared reset line to be asserted once, at some time before the modules
> are used. The reset controller API supports this for the
> reset_control_reset() call.
In order for drivers to work on lots of platforms, should all drivers
use devm_reset_control_get_shared() instead of devm_reset_control_get(),
unless there is a need to reset the hardware at a specific time after
boot (e.g. watchdog with no way out)?

So where do we go with this for this i2c driver?

Thanks
Phil

Re: [PATCH v2 0/2] i2c: Add new driver for Renesas RZ/V2M controller

[Philipp Zabel]

Hi Geert,

On Mi, 2022-06-29 at 19:18 +0200, Geert Uytterhoeven wrote:
> Hi Philipp,
> 
> On Wed, Jun 29, 2022 at 6:21 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > On Di, 2022-06-28 at 20:45 +0100, Phil Edworthy wrote:
> > > The Renesas RZ/V2M SoC (r9a09g011) has a new i2c controller. This series
> > > add the driver. One annoying problem is that the SoC uses a single reset
> > > line for two i2c controllers, and unfortunately one of the controllers
> > > is managed by some firmware, not by Linux. Therefore, the driver just
> > > deasserts the reset.
> > 
> > This sounds scary. If the driver is never loaded, and the reset is
> > never deasserted, what happens to the firmware trying to access the
> > other i2c controller? Does it hang? Or write to the reset controller
> > registers to deassert the reset? If so, is there any protection against
> > concurrent access from firmware and reset controller driver?
> 
> In response to v1, I wrote
> 
> > That is actually an integration issue, not an i2c controller issue.
> > 
> > Perhaps we need a RESET_IS_CRITICAL flag, cfr. CLK_IS_CRITICAL,
> > to be set by the reset provider?

I would just let the reset controller driver implement this by
disabling _assert and _reset for those firmware-shared resets.

regards
Philipp

Re: [PATCH v2 0/2] i2c: Add new driver for Renesas RZ/V2M controller

[Philipp Zabel]

On Do, 2022-06-30 at 15:16 +0000, Phil Edworthy wrote:
[...]
> > Which reset driver is handling the reset for this i2c module?
> drivers/clk/renesas/rzg2l-cpg.c 
> See rzg2l_cpg_assert() and rzg2l_cpg_deassert()
> Note this driver handles a few different SoCs, the SoC using this i2c
> driver is specified in drivers/clk/renesas/r9a09g011-cpg.c

Thank you.

[...]
> 
> 
> In order for drivers to work on lots of platforms, should all drivers
> use devm_reset_control_get_shared() instead of devm_reset_control_get(),
> unless there is a need to reset the hardware at a specific time after
> boot (e.g. watchdog with no way out)?

Nobody should use devm_reset_control_get(). Those drivers that require
direct control should use devm_reset_control_get_exclusive(). All
others probably should use the _shared() variant, if it works for them.

> So where do we go with this for this i2c driver?

In this specific case letting the driver deassert the reset seems to be
safe, so I'm fine with the way it is.

You could also let the i2c driver call reset_control_assert() during
remove() and modify the rzg2l-cpg.c driver to ignore it. That doesn't
seem very useful on its own, but it would have the positive effect of
documenting the shared-with-firmware reset in the reset controller
driver.

regards
Philipp

RE: [PATCH v2 0/2] i2c: Add new driver for Renesas RZ/V2M controller

[Phil Edworthy]

Hi Philipp,

On 01 July 2022 16:40 Philipp Zabel wrote:
> On Do, 2022-06-30 at 15:16 +0000, Phil Edworthy wrote:
> > In order for drivers to work on lots of platforms, should all drivers
> > use devm_reset_control_get_shared() instead of devm_reset_control_get(),
> > unless there is a need to reset the hardware at a specific time after
> > boot (e.g. watchdog with no way out)?
> 
> Nobody should use devm_reset_control_get(). Those drivers that require
> direct control should use devm_reset_control_get_exclusive(). All
> others probably should use the _shared() variant, if it works for them.
Ok, got it!

> > So where do we go with this for this i2c driver?
> 
> In this specific case letting the driver deassert the reset seems to be
> safe, so I'm fine with the way it is.
> 
> You could also let the i2c driver call reset_control_assert() during
> remove() and modify the rzg2l-cpg.c driver to ignore it. That doesn't
> seem very useful on its own, but it would have the positive effect of
> documenting the shared-with-firmware reset in the reset controller
> driver.
Ok, I'll skip the assert for the time being and when we get round to
making the reset controller mask out shared resets, we can then modify
the i2c driver.

Thanks for your advice,
Phil