[PATCH v4 0/9] i2c: riic: driver cleanup and improvements

[PATCH v4 0/9] i2c: riic: driver cleanup and improvements

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Simplify and modernize the RIIC I2C driver with the following changes:

1. Refactor error handling in `riic_i2c_probe()` and `riic_init_hw()` by
   replacing `dev_err()` with `dev_err_probe()` and using a local `dev`
   pointer.
2. Use `BIT()` and `GENMASK()` macros for consistent and clear bit
   handling.
3. Manage reset lines with `devm_reset_control_get_exclusive()` to
   simplify resource handling.
4. Mark `riic_irqs` as `const` and simplify clock tick calculations with
   predefined macros.
5. Add `riic_bus_barrier()` to check bus availability and improve
   reliability.

v3->v4
-> Created new patch 1/9
-> Dropped RB/TB tags from patch 8/9
-> Dropped `unsigned long` cast and updated the format specifier while
   printing bus frequency
-> Included required headers
-> Propogated the error

Cheers,
Prabhakar

Lad Prabhakar (9):
  i2c: riic: Introduce a separate variable for IRQ
  i2c: riic: Use dev_err_probe in probe and riic_init_hw functions
  i2c: riic: Use local `dev` pointer in `dev_err_probe()`
  i2c: riic: Use BIT macro consistently
  i2c: riic: Use GENMASK() macro for bitmask definitions
  i2c: riic: Make use of devres helper to request deasserted reset line
  i2c: riic: Mark riic_irqs array as const
  i2c: riic: Use predefined macro and simplify clock tick calculation
  i2c: riic: Add `riic_bus_barrier()` to check bus availability

 drivers/i2c/busses/i2c-riic.c | 137 ++++++++++++++++++----------------
 1 file changed, 72 insertions(+), 65 deletions(-)

-- 
2.43.0

[PATCH v4 1/9] i2c: riic: Introduce a separate variable for IRQ

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Refactor the IRQ handling in riic_i2c_probe by introducing a local variable
`irq` to store IRQ numbers instead of assigning them to `ret`. This change
improves code readability and clarity.

Remove explicit error handling after `platform_get_irq()` since
`devm_request_irq()` already handles such errors.

Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v3->v4
- New patch
---
 drivers/i2c/busses/i2c-riic.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index 9264adc97ca9..c9b2ceaf4000 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -464,11 +464,9 @@ static int riic_i2c_probe(struct platform_device *pdev)
 		return ret;
 
 	for (i = 0; i < ARRAY_SIZE(riic_irqs); i++) {
-		ret = platform_get_irq(pdev, riic_irqs[i].res_num);
-		if (ret < 0)
-			return ret;
+		int irq = platform_get_irq(pdev, riic_irqs[i].res_num);
 
-		ret = devm_request_irq(dev, ret, riic_irqs[i].isr,
+		ret = devm_request_irq(dev, irq, riic_irqs[i].isr,
 				       0, riic_irqs[i].name, riic);
 		if (ret) {
 			dev_err(dev, "failed to request irq %s\n", riic_irqs[i].name);
-- 
2.43.0

[PATCH v4 2/9] i2c: riic: Use dev_err_probe in probe and riic_init_hw functions

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Refactor error handling in the riic_i2c_probe() and riic_init_hw()
functions by replacing multiple dev_err() calls with dev_err_probe().

Additionally, update the riic_init_hw() function to use a local `dev`
pointer instead of `riic->adapter.dev` for dev_err_probe(), as the I2C
adapter is not initialized at this stage. Drop the cast to (unsigned long)
in the riic_init_hw() function when printing the bus frequency, and update
the error message to display the frequency in Hz, improving clarity.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Reviewed-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
v3->v4
- Dropped `unsigned long` cast and updated the format specifier while
  printing bus frequency
- Since the changes were small, I've kept the RB/TB tags.

v2->v3
- Squashed dev_err_probe() change from patch #2 into patch #1
- Updated commit message
- Collected RB and tested tags

v1->v2
- Collected RB tag from Geert
---
 drivers/i2c/busses/i2c-riic.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index c9b2ceaf4000..9041936fde04 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -356,11 +356,9 @@ static int riic_init_hw(struct riic_dev *riic)
 		rate /= 2;
 	}
 
-	if (brl > (0x1F + 3)) {
-		dev_err(&riic->adapter.dev, "invalid speed (%lu). Too slow.\n",
-			(unsigned long)t->bus_freq_hz);
-		return -EINVAL;
-	}
+	if (brl > (0x1F + 3))
+		return dev_err_probe(dev, -EINVAL, "invalid speed (%uHz). Too slow.\n",
+				     t->bus_freq_hz);
 
 	brh = total_ticks - brl;
 
@@ -445,10 +443,9 @@ static int riic_i2c_probe(struct platform_device *pdev)
 		return PTR_ERR(riic->base);
 
 	riic->clk = devm_clk_get(dev, NULL);
-	if (IS_ERR(riic->clk)) {
-		dev_err(dev, "missing controller clock");
-		return PTR_ERR(riic->clk);
-	}
+	if (IS_ERR(riic->clk))
+		return dev_err_probe(dev, PTR_ERR(riic->clk),
+				     "missing controller clock");
 
 	riic->rstc = devm_reset_control_get_optional_exclusive(dev, NULL);
 	if (IS_ERR(riic->rstc))
@@ -468,10 +465,9 @@ static int riic_i2c_probe(struct platform_device *pdev)
 
 		ret = devm_request_irq(dev, irq, riic_irqs[i].isr,
 				       0, riic_irqs[i].name, riic);
-		if (ret) {
-			dev_err(dev, "failed to request irq %s\n", riic_irqs[i].name);
-			return ret;
-		}
+		if (ret)
+			return dev_err_probe(dev, ret, "failed to request irq %s\n",
+					     riic_irqs[i].name);
 	}
 
 	riic->info = of_device_get_match_data(dev);
-- 
2.43.0

[PATCH v4 3/9] i2c: riic: Use local `dev` pointer in `dev_err_probe()`

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Update the `riic_init_hw()` function to use the local `dev` pointer in
calls to `dev_err_probe()`. Previously, `riic_init_hw()` used
`riic->adapter.dev` in error reporting. Since this function is invoked
during the probe phase, the I2C adapter is not yet initialized, leading to
`(null) ...` being printed in error messages. This patch fixes the issue
by consistently using the local `dev` pointer, which points to
`riic->adapter.dev.parent`.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Reviewed-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
v3->v4
- No change

v2->v3
- Moved replacing dev_err -> dev_err_probe into patch#1
- Dropped fixes tags
- Updated commit message
- Collected RB and tested tags

v1->v2
- Collected RB tag from Geert
---
 drivers/i2c/busses/i2c-riic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index 9041936fde04..459f7158ed11 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -320,7 +320,7 @@ static int riic_init_hw(struct riic_dev *riic)
 				      : I2C_MAX_FAST_MODE_FREQ;
 
 	if (t->bus_freq_hz > max_freq)
-		return dev_err_probe(&riic->adapter.dev, -EINVAL,
+		return dev_err_probe(dev, -EINVAL,
 				     "unsupported bus speed %uHz (%u max)\n",
 				     t->bus_freq_hz, max_freq);
 
-- 
2.43.0

[PATCH v4 4/9] i2c: riic: Use BIT macro consistently

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Easier to read and ensures proper types.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Reviewed-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
v3->v4
- Included bits.h
- Since the changes were small, I've kept the RB/TB tags.

v2->v3
- Collected RB and tested tags

v1->v2
- Collected RB tag from Geert
---
 drivers/i2c/busses/i2c-riic.c | 37 ++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index 459f7158ed11..5551964c3971 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -45,33 +45,34 @@
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/reset.h>
+#include <vdso/bits.h>
 
-#define ICCR1_ICE	0x80
-#define ICCR1_IICRST	0x40
-#define ICCR1_SOWP	0x10
+#define ICCR1_ICE	BIT(7)
+#define ICCR1_IICRST	BIT(6)
+#define ICCR1_SOWP	BIT(4)
 
-#define ICCR2_BBSY	0x80
-#define ICCR2_SP	0x08
-#define ICCR2_RS	0x04
-#define ICCR2_ST	0x02
+#define ICCR2_BBSY	BIT(7)
+#define ICCR2_SP	BIT(3)
+#define ICCR2_RS	BIT(2)
+#define ICCR2_ST	BIT(1)
 
 #define ICMR1_CKS_MASK	0x70
-#define ICMR1_BCWP	0x08
+#define ICMR1_BCWP	BIT(3)
 #define ICMR1_CKS(_x)	((((_x) << 4) & ICMR1_CKS_MASK) | ICMR1_BCWP)
 
-#define ICMR3_RDRFS	0x20
-#define ICMR3_ACKWP	0x10
-#define ICMR3_ACKBT	0x08
+#define ICMR3_RDRFS	BIT(5)
+#define ICMR3_ACKWP	BIT(4)
+#define ICMR3_ACKBT	BIT(3)
 
-#define ICFER_FMPE	0x80
+#define ICFER_FMPE	BIT(7)
 
-#define ICIER_TIE	0x80
-#define ICIER_TEIE	0x40
-#define ICIER_RIE	0x20
-#define ICIER_NAKIE	0x10
-#define ICIER_SPIE	0x08
+#define ICIER_TIE	BIT(7)
+#define ICIER_TEIE	BIT(6)
+#define ICIER_RIE	BIT(5)
+#define ICIER_NAKIE	BIT(4)
+#define ICIER_SPIE	BIT(3)
 
-#define ICSR2_NACKF	0x10
+#define ICSR2_NACKF	BIT(4)
 
 #define ICBR_RESERVED	0xe0 /* Should be 1 on writes */
 
-- 
2.43.0

[PATCH v4 5/9] i2c: riic: Use GENMASK() macro for bitmask definitions

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Replace raw bitmask values with the `GENMASK()` macro in the `i2c-riic`
driver to improve readability and maintain consistency.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Reviewed-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
v3->v4
- Included bits.h
- Since the changes were small, I've kept the RB/TB tags.

v2->v3
- Collected RB and tested tags

v1->v2
- Collected RB tag from Geert
---
 drivers/i2c/busses/i2c-riic.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index 5551964c3971..ab9ec7f12032 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -34,6 +34,7 @@
  * Also check the comments in the interrupt routines for some gory details.
  */
 
+#include <linux/bits.h>
 #include <linux/clk.h>
 #include <linux/completion.h>
 #include <linux/err.h>
@@ -56,7 +57,7 @@
 #define ICCR2_RS	BIT(2)
 #define ICCR2_ST	BIT(1)
 
-#define ICMR1_CKS_MASK	0x70
+#define ICMR1_CKS_MASK	GENMASK(6, 4)
 #define ICMR1_BCWP	BIT(3)
 #define ICMR1_CKS(_x)	((((_x) << 4) & ICMR1_CKS_MASK) | ICMR1_BCWP)
 
@@ -74,7 +75,7 @@
 
 #define ICSR2_NACKF	BIT(4)
 
-#define ICBR_RESERVED	0xe0 /* Should be 1 on writes */
+#define ICBR_RESERVED	GENMASK(7, 5) /* Should be 1 on writes */
 
 #define RIIC_INIT_MSG	-1
 
-- 
2.43.0

[PATCH v4 6/9] i2c: riic: Make use of devres helper to request deasserted reset line

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Simplify the `riic_i2c_probe()` function by using the
`devm_reset_control_get_optional_exclusive_deasserted()` API to request a
deasserted reset line. This eliminates the need to manually deassert the
reset control and the additional cleanup.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Reviewed-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
v2->v3
- No change

v2->v3
- Collected RB and tested tags

v1->v2
- Updated error message
---
 drivers/i2c/busses/i2c-riic.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index ab9ec7f12032..0953fedac786 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -424,11 +424,6 @@ static struct riic_irq_desc riic_irqs[] = {
 	{ .res_num = 5, .isr = riic_tend_isr, .name = "riic-nack" },
 };
 
-static void riic_reset_control_assert(void *data)
-{
-	reset_control_assert(data);
-}
-
 static int riic_i2c_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -449,18 +444,10 @@ static int riic_i2c_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, PTR_ERR(riic->clk),
 				     "missing controller clock");
 
-	riic->rstc = devm_reset_control_get_optional_exclusive(dev, NULL);
+	riic->rstc = devm_reset_control_get_optional_exclusive_deasserted(dev, NULL);
 	if (IS_ERR(riic->rstc))
 		return dev_err_probe(dev, PTR_ERR(riic->rstc),
-				     "Error: missing reset ctrl\n");
-
-	ret = reset_control_deassert(riic->rstc);
-	if (ret)
-		return ret;
-
-	ret = devm_add_action_or_reset(dev, riic_reset_control_assert, riic->rstc);
-	if (ret)
-		return ret;
+				     "failed to acquire deasserted reset\n");
 
 	for (i = 0; i < ARRAY_SIZE(riic_irqs); i++) {
 		int irq = platform_get_irq(pdev, riic_irqs[i].res_num);
-- 
2.43.0

[PATCH v4 7/9] i2c: riic: Mark riic_irqs array as const

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

The riic_irqs array describes the supported IRQs by the RIIC driver and
does not change at runtime.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Reviewed-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
v2->v3
- No change

v2->v3
- Collected RB and tested tags

v1->v2
- Collected RB tag from Geert
---
 drivers/i2c/busses/i2c-riic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index 0953fedac786..eff1efd381dd 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -416,7 +416,7 @@ static int riic_init_hw(struct riic_dev *riic)
 	return 0;
 }
 
-static struct riic_irq_desc riic_irqs[] = {
+static const struct riic_irq_desc riic_irqs[] = {
 	{ .res_num = 0, .isr = riic_tend_isr, .name = "riic-tend" },
 	{ .res_num = 1, .isr = riic_rdrf_isr, .name = "riic-rdrf" },
 	{ .res_num = 2, .isr = riic_tdre_isr, .name = "riic-tdre" },
-- 
2.43.0

[PATCH v4 8/9] i2c: riic: Use predefined macro and simplify clock tick calculation

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Replace the hardcoded `1000000000` with the predefined `NSEC_PER_SEC`
macro for clarity. Simplify the code by introducing a `ns_per_tick`
variable to store `NSEC_PER_SEC / rate`, reducing redundancy and
improving readability.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v3->v4
- Switched to use NSEC_PER_SEC instead of NANO
- Updated the commit message
- Dropped the RB/TB tags

v2->v3
- Collected RB and tested tags

v1->v2
- Collected RB tag from Geert
---
 drivers/i2c/busses/i2c-riic.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index eff1efd381dd..042933a2a985 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -47,6 +47,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/reset.h>
 #include <vdso/bits.h>
+#include <vdso/time64.h>
 
 #define ICCR1_ICE	BIT(7)
 #define ICCR1_IICRST	BIT(6)
@@ -314,6 +315,7 @@ static int riic_init_hw(struct riic_dev *riic)
 {
 	int ret;
 	unsigned long rate;
+	unsigned long ns_per_tick;
 	int total_ticks, cks, brl, brh;
 	struct i2c_timings *t = &riic->i2c_t;
 	struct device *dev = riic->adapter.dev.parent;
@@ -377,8 +379,9 @@ static int riic_init_hw(struct riic_dev *riic)
 	 * Remove clock ticks for rise and fall times. Convert ns to clock
 	 * ticks.
 	 */
-	brl -= t->scl_fall_ns / (1000000000 / rate);
-	brh -= t->scl_rise_ns / (1000000000 / rate);
+	ns_per_tick = NSEC_PER_SEC / rate;
+	brl -= t->scl_fall_ns / ns_per_tick;
+	brh -= t->scl_rise_ns / ns_per_tick;
 
 	/* Adjust for min register values for when SCLE=1 and NFE=1 */
 	if (brl < 1)
@@ -388,8 +391,7 @@ static int riic_init_hw(struct riic_dev *riic)
 
 	pr_debug("i2c-riic: freq=%lu, duty=%d, fall=%lu, rise=%lu, cks=%d, brl=%d, brh=%d\n",
 		 rate / total_ticks, ((brl + 3) * 100) / (brl + brh + 6),
-		 t->scl_fall_ns / (1000000000 / rate),
-		 t->scl_rise_ns / (1000000000 / rate), cks, brl, brh);
+		 t->scl_fall_ns / ns_per_tick, t->scl_rise_ns / ns_per_tick, cks, brl, brh);
 
 	ret = pm_runtime_resume_and_get(dev);
 	if (ret)
-- 
2.43.0

[PATCH v4 9/9] i2c: riic: Add `riic_bus_barrier()` to check bus availability

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Introduce a new `riic_bus_barrier()` function to verify bus availability
before initiating an I2C transfer. This function enhances the bus
arbitration check by ensuring that the SDA and SCL lines are not held low,
in addition to checking the BBSY flag using `readb_poll_timeout()`.

Previously, only the BBSY flag was checked to determine bus availability.
However, it is possible for the SDA line to remain low even when BBSY = 0.
This new implementation performs an additional check on the SDA and SCL
lines to avoid potential bus contention issues.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Reviewed-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
v3->v4
- Propogated error code when readb_poll_timeout() failed
- I've kept the RB/TB tags as the changes were minimal.

v2->v3
- Collected RB and tested tags

v1->v2
- Used single register read to check SDA/SCL lines
---
 drivers/i2c/busses/i2c-riic.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index 042933a2a985..bb8383d53b52 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -41,6 +41,7 @@
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/iopoll.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
@@ -52,6 +53,8 @@
 #define ICCR1_ICE	BIT(7)
 #define ICCR1_IICRST	BIT(6)
 #define ICCR1_SOWP	BIT(4)
+#define ICCR1_SCLI	BIT(1)
+#define ICCR1_SDAI	BIT(0)
 
 #define ICCR2_BBSY	BIT(7)
 #define ICCR2_SP	BIT(3)
@@ -137,6 +140,27 @@ static inline void riic_clear_set_bit(struct riic_dev *riic, u8 clear, u8 set, u
 	riic_writeb(riic, (riic_readb(riic, reg) & ~clear) | set, reg);
 }
 
+static int riic_bus_barrier(struct riic_dev *riic)
+{
+	int ret;
+	u8 val;
+
+	/*
+	 * The SDA line can still be low even when BBSY = 0. Therefore, after checking
+	 * the BBSY flag, also verify that the SDA and SCL lines are not being held low.
+	 */
+	ret = readb_poll_timeout(riic->base + riic->info->regs[RIIC_ICCR2], val,
+				 !(val & ICCR2_BBSY), 10, riic->adapter.timeout);
+	if (ret)
+		return ret;
+
+	if ((riic_readb(riic, RIIC_ICCR1) & (ICCR1_SDAI | ICCR1_SCLI)) !=
+	     (ICCR1_SDAI | ICCR1_SCLI))
+		return -EBUSY;
+
+	return 0;
+}
+
 static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 {
 	struct riic_dev *riic = i2c_get_adapdata(adap);
@@ -149,13 +173,11 @@ static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	if (ret)
 		return ret;
 
-	if (riic_readb(riic, RIIC_ICCR2) & ICCR2_BBSY) {
-		riic->err = -EBUSY;
+	riic->err = riic_bus_barrier(riic);
+	if (riic->err)
 		goto out;
-	}
 
 	reinit_completion(&riic->msg_done);
-	riic->err = 0;
 
 	riic_writeb(riic, 0, RIIC_ICSR2);
 
-- 
2.43.0

Re: [PATCH v4 1/9] i2c: riic: Introduce a separate variable for IRQ

[Geert Uytterhoeven]

Hi Prabhakar,

Thanks for your patch!

On Fri, Jan 3, 2025 at 10:19 AM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Refactor the IRQ handling in riic_i2c_probe by introducing a local variable
> `irq` to store IRQ numbers instead of assigning them to `ret`. This change
> improves code readability and clarity.
>
> Remove explicit error handling after `platform_get_irq()` since
> `devm_request_irq()` already handles such errors.

Where does it handle such errors?
I only found the following check in request_threaded_irq():

        desc = irq_to_desc(irq);
        if (!desc)
                return -EINVAL;

Although irq_to_desc() takes an unsigned int, it should indeed catch
invalid (negative) interrupt numbers, but the code above would not
propagate the correct error code (e.g. -EPROBE_DEFER).

> Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>


> --- a/drivers/i2c/busses/i2c-riic.c
> +++ b/drivers/i2c/busses/i2c-riic.c
> @@ -464,11 +464,9 @@ static int riic_i2c_probe(struct platform_device *pdev)
>                 return ret;
>
>         for (i = 0; i < ARRAY_SIZE(riic_irqs); i++) {
> -               ret = platform_get_irq(pdev, riic_irqs[i].res_num);
> -               if (ret < 0)
> -                       return ret;
> +               int irq = platform_get_irq(pdev, riic_irqs[i].res_num);

So I think you need to keep a check for irq < 0.

>
> -               ret = devm_request_irq(dev, ret, riic_irqs[i].isr,
> +               ret = devm_request_irq(dev, irq, riic_irqs[i].isr,
>                                        0, riic_irqs[i].name, riic);
>                 if (ret) {
>                         dev_err(dev, "failed to request irq %s\n", riic_irqs[i].name);

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 v4 1/9] i2c: riic: Introduce a separate variable for IRQ

[Lad, Prabhakar]

Hi Geert,

Thank you for the review.

On Fri, Jan 3, 2025 at 10:48 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> Thanks for your patch!
>
> On Fri, Jan 3, 2025 at 10:19 AM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Refactor the IRQ handling in riic_i2c_probe by introducing a local variable
> > `irq` to store IRQ numbers instead of assigning them to `ret`. This change
> > improves code readability and clarity.
> >
> > Remove explicit error handling after `platform_get_irq()` since
> > `devm_request_irq()` already handles such errors.
>
> Where does it handle such errors?
> I only found the following check in request_threaded_irq():
>
>         desc = irq_to_desc(irq);
>         if (!desc)
>                 return -EINVAL;
>
> Although irq_to_desc() takes an unsigned int, it should indeed catch
> invalid (negative) interrupt numbers, but the code above would not
> propagate the correct error code (e.g. -EPROBE_DEFER).
>
Agreed, I had missed that. I will restore the check.

Cheers,
Prabhakar

Re: [PATCH v4 8/9] i2c: riic: Use predefined macro and simplify clock tick calculation

[Geert Uytterhoeven]

On Fri, Jan 3, 2025 at 10:19 AM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Replace the hardcoded `1000000000` with the predefined `NSEC_PER_SEC`
> macro for clarity. Simplify the code by introducing a `ns_per_tick`
> variable to store `NSEC_PER_SEC / rate`, reducing redundancy and
> improving readability.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> v3->v4
> - Switched to use NSEC_PER_SEC instead of NANO
> - Updated the commit message
> - Dropped the RB/TB tags

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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 v4 9/9] i2c: riic: Add `riic_bus_barrier()` to check bus availability

[Geert Uytterhoeven]

Hi Prabhakar,

On Fri, Jan 3, 2025 at 10:19 AM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Introduce a new `riic_bus_barrier()` function to verify bus availability
> before initiating an I2C transfer. This function enhances the bus
> arbitration check by ensuring that the SDA and SCL lines are not held low,
> in addition to checking the BBSY flag using `readb_poll_timeout()`.
>
> Previously, only the BBSY flag was checked to determine bus availability.
> However, it is possible for the SDA line to remain low even when BBSY = 0.
> This new implementation performs an additional check on the SDA and SCL
> lines to avoid potential bus contention issues.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> Reviewed-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
> v3->v4
> - Propogated error code when readb_poll_timeout() failed
> - I've kept the RB/TB tags as the changes were minimal.

OK.

+ you removed a superfluous initialization of riic->err (last change below).

> --- a/drivers/i2c/busses/i2c-riic.c
> +++ b/drivers/i2c/busses/i2c-riic.c
> @@ -149,13 +173,11 @@ static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>         if (ret)
>                 return ret;
>
> -       if (riic_readb(riic, RIIC_ICCR2) & ICCR2_BBSY) {
> -               riic->err = -EBUSY;
> +       riic->err = riic_bus_barrier(riic);
> +       if (riic->err)
>                 goto out;
> -       }
>
>         reinit_completion(&riic->msg_done);
> -       riic->err = 0;
>
>         riic_writeb(riic, 0, RIIC_ICSR2);
>

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 v4 9/9] i2c: riic: Add `riic_bus_barrier()` to check bus availability

[Lad, Prabhakar]

Hi Geert,

On Fri, Jan 3, 2025 at 4:09 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Fri, Jan 3, 2025 at 10:19 AM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Introduce a new `riic_bus_barrier()` function to verify bus availability
> > before initiating an I2C transfer. This function enhances the bus
> > arbitration check by ensuring that the SDA and SCL lines are not held low,
> > in addition to checking the BBSY flag using `readb_poll_timeout()`.
> >
> > Previously, only the BBSY flag was checked to determine bus availability.
> > However, it is possible for the SDA line to remain low even when BBSY = 0.
> > This new implementation performs an additional check on the SDA and SCL
> > lines to avoid potential bus contention issues.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > Reviewed-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > ---
> > v3->v4
> > - Propogated error code when readb_poll_timeout() failed
> > - I've kept the RB/TB tags as the changes were minimal.
>
> OK.
>
> + you removed a superfluous initialization of riic->err (last change below).
>
This change  was requested by Wolfram on v2.

[0] https://lore.kernel.org/all/Z2XhI4L9nzUqa22Z@ninjato/

Cheers,
Prabhakar

> > --- a/drivers/i2c/busses/i2c-riic.c
> > +++ b/drivers/i2c/busses/i2c-riic.c
> > @@ -149,13 +173,11 @@ static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> >         if (ret)
> >                 return ret;
> >
> > -       if (riic_readb(riic, RIIC_ICCR2) & ICCR2_BBSY) {
> > -               riic->err = -EBUSY;
> > +       riic->err = riic_bus_barrier(riic);
> > +       if (riic->err)
> >                 goto out;
> > -       }
> >
> >         reinit_completion(&riic->msg_done);
> > -       riic->err = 0;
> >
> >         riic_writeb(riic, 0, RIIC_ICSR2);
> >
>
> 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 v4 0/9] i2c: riic: driver cleanup and improvements

[Andi Shyti]

Hi,

I will need for Andy's ack here because he had some comments.

Thanks,
Andi

On Fri, Jan 03, 2025 at 09:18:51AM +0000, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Simplify and modernize the RIIC I2C driver with the following changes:
> 
> 1. Refactor error handling in `riic_i2c_probe()` and `riic_init_hw()` by
>    replacing `dev_err()` with `dev_err_probe()` and using a local `dev`
>    pointer.
> 2. Use `BIT()` and `GENMASK()` macros for consistent and clear bit
>    handling.
> 3. Manage reset lines with `devm_reset_control_get_exclusive()` to
>    simplify resource handling.
> 4. Mark `riic_irqs` as `const` and simplify clock tick calculations with
>    predefined macros.
> 5. Add `riic_bus_barrier()` to check bus availability and improve
>    reliability.
> 
> v3->v4
> -> Created new patch 1/9
> -> Dropped RB/TB tags from patch 8/9
> -> Dropped `unsigned long` cast and updated the format specifier while
>    printing bus frequency
> -> Included required headers
> -> Propogated the error
> 
> Cheers,
> Prabhakar
> 
> Lad Prabhakar (9):
>   i2c: riic: Introduce a separate variable for IRQ
>   i2c: riic: Use dev_err_probe in probe and riic_init_hw functions
>   i2c: riic: Use local `dev` pointer in `dev_err_probe()`
>   i2c: riic: Use BIT macro consistently
>   i2c: riic: Use GENMASK() macro for bitmask definitions
>   i2c: riic: Make use of devres helper to request deasserted reset line
>   i2c: riic: Mark riic_irqs array as const
>   i2c: riic: Use predefined macro and simplify clock tick calculation
>   i2c: riic: Add `riic_bus_barrier()` to check bus availability
> 
>  drivers/i2c/busses/i2c-riic.c | 137 ++++++++++++++++++----------------
>  1 file changed, 72 insertions(+), 65 deletions(-)
> 
> -- 
> 2.43.0
> 

Re: [PATCH v4 0/9] i2c: riic: driver cleanup and improvements

[Lad, Prabhakar]

Hi Andi,

On Fri, Jan 3, 2025 at 11:54 PM Andi Shyti <andi.shyti@kernel.org> wrote:
>
> Hi,
>
> I will need for Andy's ack here because he had some comments.
>
I'll have to respin the series fixing patch 1/9.

Cheers,
Prabhakar

> Thanks,
> Andi
>
> On Fri, Jan 03, 2025 at 09:18:51AM +0000, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Simplify and modernize the RIIC I2C driver with the following changes:
> >
> > 1. Refactor error handling in `riic_i2c_probe()` and `riic_init_hw()` by
> >    replacing `dev_err()` with `dev_err_probe()` and using a local `dev`
> >    pointer.
> > 2. Use `BIT()` and `GENMASK()` macros for consistent and clear bit
> >    handling.
> > 3. Manage reset lines with `devm_reset_control_get_exclusive()` to
> >    simplify resource handling.
> > 4. Mark `riic_irqs` as `const` and simplify clock tick calculations with
> >    predefined macros.
> > 5. Add `riic_bus_barrier()` to check bus availability and improve
> >    reliability.
> >
> > v3->v4
> > -> Created new patch 1/9
> > -> Dropped RB/TB tags from patch 8/9
> > -> Dropped `unsigned long` cast and updated the format specifier while
> >    printing bus frequency
> > -> Included required headers
> > -> Propogated the error
> >
> > Cheers,
> > Prabhakar
> >
> > Lad Prabhakar (9):
> >   i2c: riic: Introduce a separate variable for IRQ
> >   i2c: riic: Use dev_err_probe in probe and riic_init_hw functions
> >   i2c: riic: Use local `dev` pointer in `dev_err_probe()`
> >   i2c: riic: Use BIT macro consistently
> >   i2c: riic: Use GENMASK() macro for bitmask definitions
> >   i2c: riic: Make use of devres helper to request deasserted reset line
> >   i2c: riic: Mark riic_irqs array as const
> >   i2c: riic: Use predefined macro and simplify clock tick calculation
> >   i2c: riic: Add `riic_bus_barrier()` to check bus availability
> >
> >  drivers/i2c/busses/i2c-riic.c | 137 ++++++++++++++++++----------------
> >  1 file changed, 72 insertions(+), 65 deletions(-)
> >
> > --
> > 2.43.0
> >

Re: [PATCH v4 6/9] i2c: riic: Make use of devres helper to request deasserted reset line

[Wolfram Sang]

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

On Fri, Jan 03, 2025 at 09:18:57AM +0000, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Simplify the `riic_i2c_probe()` function by using the
> `devm_reset_control_get_optional_exclusive_deasserted()` API to request a
> deasserted reset line. This eliminates the need to manually deassert the
> reset control and the additional cleanup.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> Reviewed-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Missed to give my tag here:

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


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

Re: [PATCH v4 7/9] i2c: riic: Mark riic_irqs array as const

[Wolfram Sang]

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

On Fri, Jan 03, 2025 at 09:18:58AM +0000, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> The riic_irqs array describes the supported IRQs by the RIIC driver and
> does not change at runtime.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> Reviewed-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


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

Re: [PATCH v4 8/9] i2c: riic: Use predefined macro and simplify clock tick calculation

[Wolfram Sang]

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


> +#include <vdso/time64.h>

Hmmm, what about using 'linux/time.h' instead and relying that it does
the right thing?

Nice cleanup otherwise!


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

Re: [PATCH v4 8/9] i2c: riic: Use predefined macro and simplify clock tick calculation

[Wolfram Sang]

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

On Thu, Jan 09, 2025 at 10:41:04AM +0100, Wolfram Sang wrote:
> 
> > +#include <vdso/time64.h>
> 
> Hmmm, what about using 'linux/time.h' instead and relying that it does
> the right thing?
> 
> Nice cleanup otherwise!

Besides:

Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


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

Re: [PATCH v4 0/9] i2c: riic: driver cleanup and improvements

[Andi Shyti]

Hi,

On Sat, Jan 04, 2025 at 08:15:56AM +0000, Lad, Prabhakar wrote:
> On Fri, Jan 3, 2025 at 11:54 PM Andi Shyti <andi.shyti@kernel.org> wrote:
> > I will need for Andy's ack here because he had some comments.
> >
> I'll have to respin the series fixing patch 1/9.

if you want you can only send patch 1. It's OK as far as I'm
aware of it ;-)

I pinged Andy privately to give an ack here, we still have a few
days to the merge window.

Andi

Re: [PATCH v4 0/9] i2c: riic: driver cleanup and improvements

[Lad, Prabhakar]

Hi Andi,

On Thu, Jan 9, 2025 at 2:44 PM Andi Shyti <andi.shyti@kernel.org> wrote:
>
> Hi,
>
> On Sat, Jan 04, 2025 at 08:15:56AM +0000, Lad, Prabhakar wrote:
> > On Fri, Jan 3, 2025 at 11:54 PM Andi Shyti <andi.shyti@kernel.org> wrote:
> > > I will need for Andy's ack here because he had some comments.
> > >
> > I'll have to respin the series fixing patch 1/9.
>
> if you want you can only send patch 1. It's OK as far as I'm
> aware of it ;-)
>
> I pinged Andy privately to give an ack here, we still have a few
> days to the merge window.
>
My initial plan was to send a v5 including the recovery patch too, I
switched to using the generic i2c recovery algorithm and saw issues
which I reported [0] and waiting for Wolfram's reply.

Maybe for now, I'll send patch 1/9 only, thanks.

[0] https://lore.kernel.org/all/CA+V-a8s4-g9vxyfYMgnKMK=Oej9kDBwWsWehWLYTkxw-06w-2g@mail.gmail.com/

Cheers,
Prabhakar

Re: [PATCH v4 4/9] i2c: riic: Use BIT macro consistently

[Andy Shevchenko]

On Fri, Jan 3, 2025 at 11:19 AM Prabhakar <prabhakar.csengg@gmail.com> wrote:
>
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Easier to read and ensures proper types.

ensure

...

>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/reset.h>

> +#include <vdso/bits.h>

Please, don't include vdso/* directly in the code that has no
relations with VDSO.
Use linux/bits.h.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 5/9] i2c: riic: Use GENMASK() macro for bitmask definitions

[Andy Shevchenko]

On Fri, Jan 3, 2025 at 11:19 AM Prabhakar <prabhakar.csengg@gmail.com> wrote:
>
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Replace raw bitmask values with the `GENMASK()` macro in the `i2c-riic`
> driver to improve readability and maintain consistency.

...

> +#include <linux/bits.h>

This will be part of the previous patch.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 0/9] i2c: riic: driver cleanup and improvements

[Andy Shevchenko]

On Thu, Jan 9, 2025 at 4:44 PM Andi Shyti <andi.shyti@kernel.org> wrote:
>
> Hi,
>
> On Sat, Jan 04, 2025 at 08:15:56AM +0000, Lad, Prabhakar wrote:
> > On Fri, Jan 3, 2025 at 11:54 PM Andi Shyti <andi.shyti@kernel.org> wrote:
> > > I will need for Andy's ack here because he had some comments.
> > >
> > I'll have to respin the series fixing patch 1/9.
>
> if you want you can only send patch 1. It's OK as far as I'm
> aware of it ;-)
>
> I pinged Andy privately to give an ack here, we still have a few
> days to the merge window.

Sorry for the delay, I have got sick just at the end of my vacation
(and I still have a couple of days left), but I looked at the v4 and
most of the patches LGTM, the respective comments are in the
individual replies.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 8/9] i2c: riic: Use predefined macro and simplify clock tick calculation

[Andy Shevchenko]

On Thu, Jan 9, 2025 at 11:41 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> > +#include <vdso/time64.h>
>
> Hmmm, what about using 'linux/time.h' instead and relying that it does
> the right thing?

+1.

> Nice cleanup otherwise!

+1.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 0/9] i2c: riic: driver cleanup and improvements

[Andy Shevchenko]

On Sun, Jan 12, 2025 at 4:27 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Jan 9, 2025 at 4:44 PM Andi Shyti <andi.shyti@kernel.org> wrote:
> > On Sat, Jan 04, 2025 at 08:15:56AM +0000, Lad, Prabhakar wrote:
> > > On Fri, Jan 3, 2025 at 11:54 PM Andi Shyti <andi.shyti@kernel.org> wrote:
> > > > I will need for Andy's ack here because he had some comments.
> > > >
> > > I'll have to respin the series fixing patch 1/9.
> >
> > if you want you can only send patch 1. It's OK as far as I'm
> > aware of it ;-)
> >
> > I pinged Andy privately to give an ack here, we still have a few
> > days to the merge window.
>
> Sorry for the delay, I have got sick just at the end of my vacation
> (and I still have a couple of days left), but I looked at the v4 and
> most of the patches LGTM, the respective comments are in the
> individual replies.

Okay, seems the main concern over v4/v5 is that vdso/* inclusions
while they should be linux/* ones. Otherwise LGTM. Since there are a
few changes required I would expect a v6 soon.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 4/9] i2c: riic: Use BIT macro consistently

[Geert Uytterhoeven]

Hi Andy,

On Sun, Jan 12, 2025 at 3:25 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Jan 3, 2025 at 11:19 AM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> >
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Easier to read and ensures proper types.
>
> ensure

I think the original wording is fine, as it really means
"(it is) easier to read and (it) ensures proper types".

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