From: Lad Prabhakar <[email protected]>
Hi all,
This patch series aims to add RIIC support for Renesas RZ/V2H SoC.
Cheers,
Prabhakar
Lad Prabhakar (5):
dt-bindings: i2c: renesas,riic: Update comment for fallback string
dt-bindings: i2c: renesas,riic: Document R9A09G057 support
i2c: riic: Introduce helper functions for I2C read/write operations
i2c: riic: Pass register offsets and chip details as OF data
i2c: riic: Add support for R9A09G057 SoC
.../devicetree/bindings/i2c/renesas,riic.yaml | 21 +--
drivers/i2c/busses/i2c-riic.c | 132 +++++++++++++-----
2 files changed, 109 insertions(+), 44 deletions(-)
--
2.34.1
From: Lad Prabhakar <[email protected]>
Document support for the I2C Bus Interface (RIIC) available in the
Renesas RZ/V2H (R9A09G057) SoC.
The RIIC interface in the Renesas RZ/V2H differs from RZ/A in a
couple of ways:
- Register offsets for the RZ/V2H SoC differ from those of the RZ/A SoC.
- RZ/V2H register access is 8-bit, whereas RZ/A supports 8/16/32-bit.
- RZ/V2H has some bit differences in the slave address register.
To accommodate these differences in the existing driver, a new compatible
string "renesas,riic-r9a09g057" is added.
Signed-off-by: Lad Prabhakar <[email protected]>
Reviewed-by: Fabrizio Castro <[email protected]>
---
.../devicetree/bindings/i2c/renesas,riic.yaml | 21 ++++++++++++-------
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/Documentation/devicetree/bindings/i2c/renesas,riic.yaml b/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
index 63ac5fe3208d..2a7125688647 100644
--- a/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
+++ b/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
@@ -15,14 +15,19 @@ allOf:
properties:
compatible:
- items:
- - enum:
- - renesas,riic-r7s72100 # RZ/A1H
- - renesas,riic-r7s9210 # RZ/A2M
- - renesas,riic-r9a07g043 # RZ/G2UL and RZ/Five
- - renesas,riic-r9a07g044 # RZ/G2{L,LC}
- - renesas,riic-r9a07g054 # RZ/V2L
- - const: renesas,riic-rz # generic RIIC compatible
+ oneOf:
+ - items:
+ - enum:
+ - renesas,riic-r7s72100 # RZ/A1H
+ - renesas,riic-r7s9210 # RZ/A2M
+ - renesas,riic-r9a07g043 # RZ/G2UL and RZ/Five
+ - renesas,riic-r9a07g044 # RZ/G2{L,LC}
+ - renesas,riic-r9a07g054 # RZ/V2L
+ - const: renesas,riic-rz # generic RIIC compatible
+
+ - items:
+ - enum:
+ - renesas,riic-r9a09g057 # RZ/V2H(P)
reg:
maxItems: 1
--
2.34.1
From: Lad Prabhakar <[email protected]>
Introduce helper functions for performing I2C read and write operations
in the RIIC driver.
These helper functions lay the groundwork for adding support for the
RZ/V2H SoC. This is essential because the register offsets for the RZ/V2H
SoC differ from those of the RZ/A SoC. By abstracting the read and write
operations, we can seamlessly adapt the driver to support different SoC
variants without extensive modifications.
This patch is part of the preparation process for integrating support for
the RZ/V2H SoC into the RIIC driver.
Signed-off-by: Lad Prabhakar <[email protected]>
Reviewed-by: Fabrizio Castro <[email protected]>
---
drivers/i2c/busses/i2c-riic.c | 56 +++++++++++++++++++++--------------
1 file changed, 33 insertions(+), 23 deletions(-)
diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index e43ff483c56e..49a12f1ecdf9 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -105,9 +105,19 @@ struct riic_irq_desc {
char *name;
};
+static inline void riic_writeb_reg(u8 val, struct riic_dev *riic, u8 offset)
+{
+ writeb(val, riic->base + offset);
+}
+
+static inline u8 riic_readb_reg(struct riic_dev *riic, u8 offset)
+{
+ return readb(riic->base + offset);
+}
+
static inline void riic_clear_set_bit(struct riic_dev *riic, u8 clear, u8 set, u8 reg)
{
- writeb((readb(riic->base + reg) & ~clear) | set, riic->base + reg);
+ riic_writeb_reg((riic_readb_reg(riic, reg) & ~clear) | set, riic, reg);
}
static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
@@ -119,7 +129,7 @@ static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
pm_runtime_get_sync(adap->dev.parent);
- if (readb(riic->base + RIIC_ICCR2) & ICCR2_BBSY) {
+ if (riic_readb_reg(riic, RIIC_ICCR2) & ICCR2_BBSY) {
riic->err = -EBUSY;
goto out;
}
@@ -127,7 +137,7 @@ static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
reinit_completion(&riic->msg_done);
riic->err = 0;
- writeb(0, riic->base + RIIC_ICSR2);
+ riic_writeb_reg(0, riic, RIIC_ICSR2);
for (i = 0, start_bit = ICCR2_ST; i < num; i++) {
riic->bytes_left = RIIC_INIT_MSG;
@@ -135,9 +145,9 @@ static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
riic->msg = &msgs[i];
riic->is_last = (i == num - 1);
- writeb(ICIER_NAKIE | ICIER_TIE, riic->base + RIIC_ICIER);
+ riic_writeb_reg(ICIER_NAKIE | ICIER_TIE, riic, RIIC_ICIER);
- writeb(start_bit, riic->base + RIIC_ICCR2);
+ riic_writeb_reg(start_bit, riic, RIIC_ICCR2);
time_left = wait_for_completion_timeout(&riic->msg_done, riic->adapter.timeout);
if (time_left == 0)
@@ -191,7 +201,7 @@ static irqreturn_t riic_tdre_isr(int irq, void *data)
* value could be moved to the shadow shift register right away. So
* this must be after updates to ICIER (where we want to disable TIE)!
*/
- writeb(val, riic->base + RIIC_ICDRT);
+ riic_writeb_reg(val, riic, RIIC_ICDRT);
return IRQ_HANDLED;
}
@@ -200,9 +210,9 @@ static irqreturn_t riic_tend_isr(int irq, void *data)
{
struct riic_dev *riic = data;
- if (readb(riic->base + RIIC_ICSR2) & ICSR2_NACKF) {
+ if (riic_readb_reg(riic, RIIC_ICSR2) & ICSR2_NACKF) {
/* We got a NACKIE */
- readb(riic->base + RIIC_ICDRR); /* dummy read */
+ riic_readb_reg(riic, RIIC_ICDRR); /* dummy read */
riic_clear_set_bit(riic, ICSR2_NACKF, 0, RIIC_ICSR2);
riic->err = -ENXIO;
} else if (riic->bytes_left) {
@@ -211,7 +221,7 @@ static irqreturn_t riic_tend_isr(int irq, void *data)
if (riic->is_last || riic->err) {
riic_clear_set_bit(riic, ICIER_TEIE, ICIER_SPIE, RIIC_ICIER);
- writeb(ICCR2_SP, riic->base + RIIC_ICCR2);
+ riic_writeb_reg(ICCR2_SP, riic, RIIC_ICCR2);
} else {
/* Transfer is complete, but do not send STOP */
riic_clear_set_bit(riic, ICIER_TEIE, 0, RIIC_ICIER);
@@ -230,7 +240,7 @@ static irqreturn_t riic_rdrf_isr(int irq, void *data)
if (riic->bytes_left == RIIC_INIT_MSG) {
riic->bytes_left = riic->msg->len;
- readb(riic->base + RIIC_ICDRR); /* dummy read */
+ riic_readb_reg(riic, RIIC_ICDRR); /* dummy read */
return IRQ_HANDLED;
}
@@ -238,7 +248,7 @@ static irqreturn_t riic_rdrf_isr(int irq, void *data)
/* STOP must come before we set ACKBT! */
if (riic->is_last) {
riic_clear_set_bit(riic, 0, ICIER_SPIE, RIIC_ICIER);
- writeb(ICCR2_SP, riic->base + RIIC_ICCR2);
+ riic_writeb_reg(ICCR2_SP, riic, RIIC_ICCR2);
}
riic_clear_set_bit(riic, 0, ICMR3_ACKBT, RIIC_ICMR3);
@@ -248,7 +258,7 @@ static irqreturn_t riic_rdrf_isr(int irq, void *data)
}
/* Reading acks the RIE interrupt */
- *riic->buf = readb(riic->base + RIIC_ICDRR);
+ *riic->buf = riic_readb_reg(riic, RIIC_ICDRR);
riic->buf++;
riic->bytes_left--;
@@ -260,10 +270,10 @@ static irqreturn_t riic_stop_isr(int irq, void *data)
struct riic_dev *riic = data;
/* read back registers to confirm writes have fully propagated */
- writeb(0, riic->base + RIIC_ICSR2);
- readb(riic->base + RIIC_ICSR2);
- writeb(0, riic->base + RIIC_ICIER);
- readb(riic->base + RIIC_ICIER);
+ riic_writeb_reg(0, riic, RIIC_ICSR2);
+ riic_readb_reg(riic, RIIC_ICSR2);
+ riic_writeb_reg(0, riic, RIIC_ICIER);
+ riic_readb_reg(riic, RIIC_ICIER);
complete(&riic->msg_done);
@@ -365,15 +375,15 @@ static int riic_init_hw(struct riic_dev *riic, struct i2c_timings *t)
t->scl_rise_ns / (1000000000 / rate), cks, brl, brh);
/* Changing the order of accessing IICRST and ICE may break things! */
- writeb(ICCR1_IICRST | ICCR1_SOWP, riic->base + RIIC_ICCR1);
+ riic_writeb_reg(ICCR1_IICRST | ICCR1_SOWP, riic, RIIC_ICCR1);
riic_clear_set_bit(riic, 0, ICCR1_ICE, RIIC_ICCR1);
- writeb(ICMR1_CKS(cks), riic->base + RIIC_ICMR1);
- writeb(brh | ICBR_RESERVED, riic->base + RIIC_ICBRH);
- writeb(brl | ICBR_RESERVED, riic->base + RIIC_ICBRL);
+ riic_writeb_reg(ICMR1_CKS(cks), riic, RIIC_ICMR1);
+ riic_writeb_reg(brh | ICBR_RESERVED, riic, RIIC_ICBRH);
+ riic_writeb_reg(brl | ICBR_RESERVED, riic, RIIC_ICBRL);
- writeb(0, riic->base + RIIC_ICSER);
- writeb(ICMR3_ACKWP | ICMR3_RDRFS, riic->base + RIIC_ICMR3);
+ riic_writeb_reg(0, riic, RIIC_ICSER);
+ riic_writeb_reg(ICMR3_ACKWP | ICMR3_RDRFS, riic, RIIC_ICMR3);
riic_clear_set_bit(riic, ICCR1_IICRST, 0, RIIC_ICCR1);
@@ -481,7 +491,7 @@ static void riic_i2c_remove(struct platform_device *pdev)
struct riic_dev *riic = platform_get_drvdata(pdev);
pm_runtime_get_sync(&pdev->dev);
- writeb(0, riic->base + RIIC_ICIER);
+ riic_writeb_reg(0, riic, RIIC_ICIER);
pm_runtime_put(&pdev->dev);
i2c_del_adapter(&riic->adapter);
pm_runtime_disable(&pdev->dev);
--
2.34.1
From: Lad Prabhakar <[email protected]>
With an increasing number of SoCs reusing this driver, each with slight
variations in the RIIC IP, it becomes necessary to support passing these
details as OF data. This approach simplifies the extension of the driver
for other SoCs.
This patch lays the groundwork for adding support for the Renesas RZ/V2H
SoC.
Signed-off-by: Lad Prabhakar <[email protected]>
Reviewed-by: Fabrizio Castro <[email protected]>
---
drivers/i2c/busses/i2c-riic.c | 59 ++++++++++++++++++++++++++---------
1 file changed, 44 insertions(+), 15 deletions(-)
diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index 49a12f1ecdf9..3398d639b754 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -46,18 +46,6 @@
#include <linux/pm_runtime.h>
#include <linux/reset.h>
-#define RIIC_ICCR1 0x00
-#define RIIC_ICCR2 0x04
-#define RIIC_ICMR1 0x08
-#define RIIC_ICMR3 0x10
-#define RIIC_ICSER 0x18
-#define RIIC_ICIER 0x1c
-#define RIIC_ICSR2 0x24
-#define RIIC_ICBRL 0x34
-#define RIIC_ICBRH 0x38
-#define RIIC_ICDRT 0x3c
-#define RIIC_ICDRR 0x40
-
#define ICCR1_ICE 0x80
#define ICCR1_IICRST 0x40
#define ICCR1_SOWP 0x10
@@ -87,6 +75,13 @@
#define RIIC_INIT_MSG -1
+#define RIIC_RZ_A_TYPE 0
+
+struct riic_of_data {
+ u8 family;
+ u8 regs[];
+};
+
struct riic_dev {
void __iomem *base;
u8 *buf;
@@ -94,6 +89,7 @@ struct riic_dev {
int bytes_left;
int err;
int is_last;
+ const struct riic_of_data *info;
struct completion msg_done;
struct i2c_adapter adapter;
struct clk *clk;
@@ -105,14 +101,28 @@ struct riic_irq_desc {
char *name;
};
+enum riic_reg_list {
+ RIIC_ICCR1 = 0,
+ RIIC_ICCR2,
+ RIIC_ICMR1,
+ RIIC_ICMR3,
+ RIIC_ICSER,
+ RIIC_ICIER,
+ RIIC_ICSR2,
+ RIIC_ICBRL,
+ RIIC_ICBRH,
+ RIIC_ICDRT,
+ RIIC_ICDRR,
+};
+
static inline void riic_writeb_reg(u8 val, struct riic_dev *riic, u8 offset)
{
- writeb(val, riic->base + offset);
+ writeb(val, riic->base + riic->info->regs[offset]);
}
static inline u8 riic_readb_reg(struct riic_dev *riic, u8 offset)
{
- return readb(riic->base + offset);
+ return readb(riic->base + riic->info->regs[offset]);
}
static inline void riic_clear_set_bit(struct riic_dev *riic, u8 clear, u8 set, u8 reg)
@@ -453,6 +463,8 @@ static int riic_i2c_probe(struct platform_device *pdev)
}
}
+ riic->info = of_device_get_match_data(&pdev->dev);
+
adap = &riic->adapter;
i2c_set_adapdata(adap, riic);
strscpy(adap->name, "Renesas RIIC adapter", sizeof(adap->name));
@@ -497,8 +509,25 @@ static void riic_i2c_remove(struct platform_device *pdev)
pm_runtime_disable(&pdev->dev);
}
+static const struct riic_of_data riic_rz_a_info = {
+ .family = RIIC_RZ_A_TYPE,
+ .regs = {
+ [RIIC_ICCR1] = 0x00,
+ [RIIC_ICCR2] = 0x04,
+ [RIIC_ICMR1] = 0x08,
+ [RIIC_ICMR3] = 0x10,
+ [RIIC_ICSER] = 0x18,
+ [RIIC_ICIER] = 0x1c,
+ [RIIC_ICSR2] = 0x24,
+ [RIIC_ICBRL] = 0x34,
+ [RIIC_ICBRH] = 0x38,
+ [RIIC_ICDRT] = 0x3c,
+ [RIIC_ICDRR] = 0x40,
+ },
+};
+
static const struct of_device_id riic_i2c_dt_ids[] = {
- { .compatible = "renesas,riic-rz", },
+ { .compatible = "renesas,riic-rz", .data = &riic_rz_a_info },
{ /* Sentinel */ },
};
--
2.34.1
From: Lad Prabhakar <[email protected]>
Extend the RIIC driver to support the RZ/V2H ("R9A09G057") SoC. It
accomplishes this by appending the compatible string list and passing
the RZ/V2H-specific OF data.
Additionally, this patch introduces an riic_family enum for SoC variants,
replacing macro previously used.
Signed-off-by: Lad Prabhakar <[email protected]>
Reviewed-by: Fabrizio Castro <[email protected]>
---
drivers/i2c/busses/i2c-riic.c | 23 ++++++++++++++++++++++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index 3398d639b754..fc814a4f06a6 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -75,7 +75,10 @@
#define RIIC_INIT_MSG -1
-#define RIIC_RZ_A_TYPE 0
+enum riic_family {
+ RIIC_RZ_A_TYPE = 0,
+ RIIC_RZ_V2H_TYPE,
+};
struct riic_of_data {
u8 family;
@@ -526,8 +529,26 @@ static const struct riic_of_data riic_rz_a_info = {
},
};
+static const struct riic_of_data riic_rz_v2h_info = {
+ .family = RIIC_RZ_V2H_TYPE,
+ .regs = {
+ [RIIC_ICCR1] = 0x00,
+ [RIIC_ICCR2] = 0x01,
+ [RIIC_ICMR1] = 0x02,
+ [RIIC_ICMR3] = 0x04,
+ [RIIC_ICSER] = 0x06,
+ [RIIC_ICIER] = 0x07,
+ [RIIC_ICSR2] = 0x09,
+ [RIIC_ICBRL] = 0x10,
+ [RIIC_ICBRH] = 0x11,
+ [RIIC_ICDRT] = 0x12,
+ [RIIC_ICDRR] = 0x13,
+ },
+};
+
static const struct of_device_id riic_i2c_dt_ids[] = {
{ .compatible = "renesas,riic-rz", .data = &riic_rz_a_info },
+ { .compatible = "renesas,riic-r9a09g057", .data = &riic_rz_v2h_info },
{ /* Sentinel */ },
};
--
2.34.1
From: Lad Prabhakar <[email protected]>
With the fallback string being utilized by multiple other SoCs, this
patch updates the comment for the generic compatible string.
Signed-off-by: Lad Prabhakar <[email protected]>
Reviewed-by: Fabrizio Castro <[email protected]>
---
Documentation/devicetree/bindings/i2c/renesas,riic.yaml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/i2c/renesas,riic.yaml b/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
index 2291a7cd619b..63ac5fe3208d 100644
--- a/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
+++ b/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
@@ -22,7 +22,7 @@ properties:
- renesas,riic-r9a07g043 # RZ/G2UL and RZ/Five
- renesas,riic-r9a07g044 # RZ/G2{L,LC}
- renesas,riic-r9a07g054 # RZ/V2L
- - const: renesas,riic-rz # RZ/A or RZ/G2L
+ - const: renesas,riic-rz # generic RIIC compatible
reg:
maxItems: 1
--
2.34.1
Hi Prabhakar,
Thanks for the patch.
> -----Original Message-----
> From: Prabhakar <[email protected]>
> Sent: Friday, March 8, 2024 5:27 PM
> Subject: [PATCH 4/5] i2c: riic: Pass register offsets and chip details as OF data
>
> From: Lad Prabhakar <[email protected]>
>
> With an increasing number of SoCs reusing this driver, each with slight variations in the RIIC IP, it
> becomes necessary to support passing these details as OF data. This approach simplifies the extension
> of the driver for other SoCs.
>
> This patch lays the groundwork for adding support for the Renesas RZ/V2H SoC.
>
> Signed-off-by: Lad Prabhakar <[email protected]>
> Reviewed-by: Fabrizio Castro <[email protected]>
> ---
> drivers/i2c/busses/i2c-riic.c | 59 ++++++++++++++++++++++++++---------
> 1 file changed, 44 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c index
> 49a12f1ecdf9..3398d639b754 100644
> --- a/drivers/i2c/busses/i2c-riic.c
> +++ b/drivers/i2c/busses/i2c-riic.c
> @@ -46,18 +46,6 @@
> #include <linux/pm_runtime.h>
> #include <linux/reset.h>
>
> -#define RIIC_ICCR1 0x00
> -#define RIIC_ICCR2 0x04
> -#define RIIC_ICMR1 0x08
> -#define RIIC_ICMR3 0x10
> -#define RIIC_ICSER 0x18
> -#define RIIC_ICIER 0x1c
> -#define RIIC_ICSR2 0x24
> -#define RIIC_ICBRL 0x34
> -#define RIIC_ICBRH 0x38
> -#define RIIC_ICDRT 0x3c
> -#define RIIC_ICDRR 0x40
> -
> #define ICCR1_ICE 0x80
> #define ICCR1_IICRST 0x40
> #define ICCR1_SOWP 0x10
> @@ -87,6 +75,13 @@
>
> #define RIIC_INIT_MSG -1
>
> +#define RIIC_RZ_A_TYPE 0
> +
> +struct riic_of_data {
> + u8 family;
Do you need family as compatible have this information?
> + u8 regs[];
> +};
> +
> struct riic_dev {
> void __iomem *base;
> u8 *buf;
> @@ -94,6 +89,7 @@ struct riic_dev {
> int bytes_left;
> int err;
> int is_last;
> + const struct riic_of_data *info;
> struct completion msg_done;
> struct i2c_adapter adapter;
> struct clk *clk;
> @@ -105,14 +101,28 @@ struct riic_irq_desc {
> char *name;
> };
>
> +enum riic_reg_list {
> + RIIC_ICCR1 = 0,
> + RIIC_ICCR2,
> + RIIC_ICMR1,
> + RIIC_ICMR3,
> + RIIC_ICSER,
> + RIIC_ICIER,
> + RIIC_ICSR2,
> + RIIC_ICBRL,
> + RIIC_ICBRH,
> + RIIC_ICDRT,
> + RIIC_ICDRR,
> +};
> +
> static inline void riic_writeb_reg(u8 val, struct riic_dev *riic, u8 offset) {
> - writeb(val, riic->base + offset);
> + writeb(val, riic->base + riic->info->regs[offset]);
> }
>
> static inline u8 riic_readb_reg(struct riic_dev *riic, u8 offset) {
> - return readb(riic->base + offset);
> + return readb(riic->base + riic->info->regs[offset]);
> }
>
> static inline void riic_clear_set_bit(struct riic_dev *riic, u8 clear, u8 set, u8 reg) @@ -453,6
> +463,8 @@ static int riic_i2c_probe(struct platform_device *pdev)
> }
> }
>
> + riic->info = of_device_get_match_data(&pdev->dev);
> +
> adap = &riic->adapter;
> i2c_set_adapdata(adap, riic);
> strscpy(adap->name, "Renesas RIIC adapter", sizeof(adap->name)); @@ -497,8 +509,25 @@ static void
> riic_i2c_remove(struct platform_device *pdev)
> pm_runtime_disable(&pdev->dev);
> }
>
> +static const struct riic_of_data riic_rz_a_info = {
> + .family = RIIC_RZ_A_TYPE,
> + .regs = {
> + [RIIC_ICCR1] = 0x00,
> + [RIIC_ICCR2] = 0x04,
> + [RIIC_ICMR1] = 0x08,
> + [RIIC_ICMR3] = 0x10,
> + [RIIC_ICSER] = 0x18,
> + [RIIC_ICIER] = 0x1c,
> + [RIIC_ICSR2] = 0x24,
> + [RIIC_ICBRL] = 0x34,
> + [RIIC_ICBRH] = 0x38,
> + [RIIC_ICDRT] = 0x3c,
> + [RIIC_ICDRR] = 0x40,
> + },
> +};
> +
> static const struct of_device_id riic_i2c_dt_ids[] = {
> - { .compatible = "renesas,riic-rz", },
> + { .compatible = "renesas,riic-rz", .data = &riic_rz_a_info },
> { /* Sentinel */ },
> };
>
> --
> 2.34.1
>
Hi Biju,
On Fri, Mar 8, 2024 at 5:36 PM Biju Das <[email protected]> wrote:
>
> Hi Prabhakar,
>
> Thanks for the patch.
>
> > -----Original Message-----
> > From: Prabhakar <[email protected]>
> > Sent: Friday, March 8, 2024 5:27 PM
> > Subject: [PATCH 4/5] i2c: riic: Pass register offsets and chip details as OF data
> >
> > From: Lad Prabhakar <[email protected]>
> >
> > With an increasing number of SoCs reusing this driver, each with slight variations in the RIIC IP, it
> > becomes necessary to support passing these details as OF data. This approach simplifies the extension
> > of the driver for other SoCs.
> >
> > This patch lays the groundwork for adding support for the Renesas RZ/V2H SoC.
> >
> > Signed-off-by: Lad Prabhakar <[email protected]>
> > Reviewed-by: Fabrizio Castro <[email protected]>
> > ---
> > drivers/i2c/busses/i2c-riic.c | 59 ++++++++++++++++++++++++++---------
> > 1 file changed, 44 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c index
> > 49a12f1ecdf9..3398d639b754 100644
> > --- a/drivers/i2c/busses/i2c-riic.c
> > +++ b/drivers/i2c/busses/i2c-riic.c
> > @@ -46,18 +46,6 @@
> > #include <linux/pm_runtime.h>
> > #include <linux/reset.h>
> >
> > -#define RIIC_ICCR1 0x00
> > -#define RIIC_ICCR2 0x04
> > -#define RIIC_ICMR1 0x08
> > -#define RIIC_ICMR3 0x10
> > -#define RIIC_ICSER 0x18
> > -#define RIIC_ICIER 0x1c
> > -#define RIIC_ICSR2 0x24
> > -#define RIIC_ICBRL 0x34
> > -#define RIIC_ICBRH 0x38
> > -#define RIIC_ICDRT 0x3c
> > -#define RIIC_ICDRR 0x40
> > -
> > #define ICCR1_ICE 0x80
> > #define ICCR1_IICRST 0x40
> > #define ICCR1_SOWP 0x10
> > @@ -87,6 +75,13 @@
> >
> > #define RIIC_INIT_MSG -1
> >
> > +#define RIIC_RZ_A_TYPE 0
>
> > +
> > +struct riic_of_data {
> > + u8 family;
>
> Do you need family as compatible have this information?
>
Yes this is added to future proof it, as the RZ/V2H SoC has some bit
differences in the slave address register as compared to RZ/A and
RZ/G2L. Comparing the family outside probe is not always preferred
hence this is added as part of OF data.
Cheers,
Prabhakar
> > + u8 regs[];
> > +};
> > +
> > struct riic_dev {
> > void __iomem *base;
> > u8 *buf;
> > @@ -94,6 +89,7 @@ struct riic_dev {
> > int bytes_left;
> > int err;
> > int is_last;
> > + const struct riic_of_data *info;
> > struct completion msg_done;
> > struct i2c_adapter adapter;
> > struct clk *clk;
> > @@ -105,14 +101,28 @@ struct riic_irq_desc {
> > char *name;
> > };
> >
> > +enum riic_reg_list {
> > + RIIC_ICCR1 = 0,
> > + RIIC_ICCR2,
> > + RIIC_ICMR1,
> > + RIIC_ICMR3,
> > + RIIC_ICSER,
> > + RIIC_ICIER,
> > + RIIC_ICSR2,
> > + RIIC_ICBRL,
> > + RIIC_ICBRH,
> > + RIIC_ICDRT,
> > + RIIC_ICDRR,
> > +};
> > +
> > static inline void riic_writeb_reg(u8 val, struct riic_dev *riic, u8 offset) {
> > - writeb(val, riic->base + offset);
> > + writeb(val, riic->base + riic->info->regs[offset]);
> > }
> >
> > static inline u8 riic_readb_reg(struct riic_dev *riic, u8 offset) {
> > - return readb(riic->base + offset);
> > + return readb(riic->base + riic->info->regs[offset]);
> > }
> >
> > static inline void riic_clear_set_bit(struct riic_dev *riic, u8 clear, u8 set, u8 reg) @@ -453,6
> > +463,8 @@ static int riic_i2c_probe(struct platform_device *pdev)
> > }
> > }
> >
> > + riic->info = of_device_get_match_data(&pdev->dev);
> > +
> > adap = &riic->adapter;
> > i2c_set_adapdata(adap, riic);
> > strscpy(adap->name, "Renesas RIIC adapter", sizeof(adap->name)); @@ -497,8 +509,25 @@ static void
> > riic_i2c_remove(struct platform_device *pdev)
> > pm_runtime_disable(&pdev->dev);
> > }
> >
> > +static const struct riic_of_data riic_rz_a_info = {
> > + .family = RIIC_RZ_A_TYPE,
> > + .regs = {
> > + [RIIC_ICCR1] = 0x00,
> > + [RIIC_ICCR2] = 0x04,
> > + [RIIC_ICMR1] = 0x08,
> > + [RIIC_ICMR3] = 0x10,
> > + [RIIC_ICSER] = 0x18,
> > + [RIIC_ICIER] = 0x1c,
> > + [RIIC_ICSR2] = 0x24,
> > + [RIIC_ICBRL] = 0x34,
> > + [RIIC_ICBRH] = 0x38,
> > + [RIIC_ICDRT] = 0x3c,
> > + [RIIC_ICDRR] = 0x40,
> > + },
> > +};
> > +
> > static const struct of_device_id riic_i2c_dt_ids[] = {
> > - { .compatible = "renesas,riic-rz", },
> > + { .compatible = "renesas,riic-rz", .data = &riic_rz_a_info },
> > { /* Sentinel */ },
> > };
> >
> > --
> > 2.34.1
> >
>
> -----Original Message-----
> From: Lad, Prabhakar <[email protected]>
> Sent: Friday, March 8, 2024 6:04 PM
> To: Biju Das <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>; Chris Brandt <[email protected]>; Andi Shyti
> <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski
> <[email protected]>; Conor Dooley <[email protected]>; Magnus Damm
> <[email protected]>; Wolfram Sang <[email protected]>; linux-renesas-
> [email protected]; [email protected]; [email protected]; linux-
> [email protected]; Fabrizio Castro <[email protected]>; Prabhakar Mahadev Lad
> <[email protected]>
> Subject: Re: [PATCH 4/5] i2c: riic: Pass register offsets and chip details as OF data
>
> Hi Biju,
>
> On Fri, Mar 8, 2024 at 5:36 PM Biju Das <[email protected]> wrote:
> >
> > Hi Prabhakar,
> >
> > Thanks for the patch.
> >
> > > -----Original Message-----
> > > From: Prabhakar <[email protected]>
> > > Sent: Friday, March 8, 2024 5:27 PM
> > > Subject: [PATCH 4/5] i2c: riic: Pass register offsets and chip
> > > details as OF data
> > >
> > > From: Lad Prabhakar <[email protected]>
> > >
> > > With an increasing number of SoCs reusing this driver, each with
> > > slight variations in the RIIC IP, it becomes necessary to support
> > > passing these details as OF data. This approach simplifies the extension of the driver for other
> SoCs.
> > >
> > > This patch lays the groundwork for adding support for the Renesas RZ/V2H SoC.
> > >
> > > Signed-off-by: Lad Prabhakar
> > > <[email protected]>
> > > Reviewed-by: Fabrizio Castro <[email protected]>
> > > ---
> > > drivers/i2c/busses/i2c-riic.c | 59
> > > ++++++++++++++++++++++++++---------
> > > 1 file changed, 44 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-riic.c
> > > b/drivers/i2c/busses/i2c-riic.c index
> > > 49a12f1ecdf9..3398d639b754 100644
> > > --- a/drivers/i2c/busses/i2c-riic.c
> > > +++ b/drivers/i2c/busses/i2c-riic.c
> > > @@ -46,18 +46,6 @@
> > > #include <linux/pm_runtime.h>
> > > #include <linux/reset.h>
> > >
> > > -#define RIIC_ICCR1 0x00
> > > -#define RIIC_ICCR2 0x04
> > > -#define RIIC_ICMR1 0x08
> > > -#define RIIC_ICMR3 0x10
> > > -#define RIIC_ICSER 0x18
> > > -#define RIIC_ICIER 0x1c
> > > -#define RIIC_ICSR2 0x24
> > > -#define RIIC_ICBRL 0x34
> > > -#define RIIC_ICBRH 0x38
> > > -#define RIIC_ICDRT 0x3c
> > > -#define RIIC_ICDRR 0x40
> > > -
> > > #define ICCR1_ICE 0x80
> > > #define ICCR1_IICRST 0x40
> > > #define ICCR1_SOWP 0x10
> > > @@ -87,6 +75,13 @@
> > >
> > > #define RIIC_INIT_MSG -1
> > >
> > > +#define RIIC_RZ_A_TYPE 0
> >
> > > +
> > > +struct riic_of_data {
> > > + u8 family;
> >
> > Do you need family as compatible have this information?
> >
> Yes this is added to future proof it, as the RZ/V2H SoC has some bit differences in the slave address
> register as compared to RZ/A and RZ/G2L. Comparing the family outside probe is not always preferred
> hence this is added as part of OF data.
Ok. Got it.
But the family variable is unused and is wasting memory.
Simple reg layout is enough for device data??
Cheers,
Biju
>
> Cheers,
> Prabhakar
>
> > > + u8 regs[];
> > > +};
> > > +
> > > struct riic_dev {
> > > void __iomem *base;
> > > u8 *buf;
> > > @@ -94,6 +89,7 @@ struct riic_dev {
> > > int bytes_left;
> > > int err;
> > > int is_last;
> > > + const struct riic_of_data *info;
> > > struct completion msg_done;
> > > struct i2c_adapter adapter;
> > > struct clk *clk;
> > > @@ -105,14 +101,28 @@ struct riic_irq_desc {
> > > char *name;
> > > };
> > >
> > > +enum riic_reg_list {
> > > + RIIC_ICCR1 = 0,
> > > + RIIC_ICCR2,
> > > + RIIC_ICMR1,
> > > + RIIC_ICMR3,
> > > + RIIC_ICSER,
> > > + RIIC_ICIER,
> > > + RIIC_ICSR2,
> > > + RIIC_ICBRL,
> > > + RIIC_ICBRH,
> > > + RIIC_ICDRT,
> > > + RIIC_ICDRR,
> > > +};
> > > +
> > > static inline void riic_writeb_reg(u8 val, struct riic_dev *riic, u8 offset) {
> > > - writeb(val, riic->base + offset);
> > > + writeb(val, riic->base + riic->info->regs[offset]);
> > > }
> > >
> > > static inline u8 riic_readb_reg(struct riic_dev *riic, u8 offset) {
> > > - return readb(riic->base + offset);
> > > + return readb(riic->base + riic->info->regs[offset]);
> > > }
> > >
> > > static inline void riic_clear_set_bit(struct riic_dev *riic, u8
> > > clear, u8 set, u8 reg) @@ -453,6
> > > +463,8 @@ static int riic_i2c_probe(struct platform_device *pdev)
> > > }
> > > }
> > >
> > > + riic->info = of_device_get_match_data(&pdev->dev);
> > > +
> > > adap = &riic->adapter;
> > > i2c_set_adapdata(adap, riic);
> > > strscpy(adap->name, "Renesas RIIC adapter",
> > > sizeof(adap->name)); @@ -497,8 +509,25 @@ static void riic_i2c_remove(struct platform_device *pdev)
> > > pm_runtime_disable(&pdev->dev); }
> > >
> > > +static const struct riic_of_data riic_rz_a_info = {
> > > + .family = RIIC_RZ_A_TYPE,
> > > + .regs = {
> > > + [RIIC_ICCR1] = 0x00,
> > > + [RIIC_ICCR2] = 0x04,
> > > + [RIIC_ICMR1] = 0x08,
> > > + [RIIC_ICMR3] = 0x10,
> > > + [RIIC_ICSER] = 0x18,
> > > + [RIIC_ICIER] = 0x1c,
> > > + [RIIC_ICSR2] = 0x24,
> > > + [RIIC_ICBRL] = 0x34,
> > > + [RIIC_ICBRH] = 0x38,
> > > + [RIIC_ICDRT] = 0x3c,
> > > + [RIIC_ICDRR] = 0x40,
> > > + },
> > > +};
> > > +
> > > static const struct of_device_id riic_i2c_dt_ids[] = {
> > > - { .compatible = "renesas,riic-rz", },
> > > + { .compatible = "renesas,riic-rz", .data = &riic_rz_a_info },
> > > { /* Sentinel */ },
> > > };
> > >
> > > --
> > > 2.34.1
> > >
> >
Hi Prabhakar,
On Fri, Mar 8, 2024 at 6:28 PM Prabhakar <[email protected]> wrote:
> From: Lad Prabhakar <[email protected]>
>
> Introduce helper functions for performing I2C read and write operations
> in the RIIC driver.
>
> These helper functions lay the groundwork for adding support for the
> RZ/V2H SoC. This is essential because the register offsets for the RZ/V2H
> SoC differ from those of the RZ/A SoC. By abstracting the read and write
> operations, we can seamlessly adapt the driver to support different SoC
> variants without extensive modifications.
>
> This patch is part of the preparation process for integrating support for
> the RZ/V2H SoC into the RIIC driver.
>
> Signed-off-by: Lad Prabhakar <[email protected]>
> Reviewed-by: Fabrizio Castro <[email protected]>
Thanks for your patch!
> --- a/drivers/i2c/busses/i2c-riic.c
> +++ b/drivers/i2c/busses/i2c-riic.c
> @@ -105,9 +105,19 @@ struct riic_irq_desc {
> char *name;
> };
>
> +static inline void riic_writeb_reg(u8 val, struct riic_dev *riic, u8 offset)
Having "riic" in the middle is definitely the wrong order of parameters ;-)
Please make "riic" the first parameter.
> +{
> + writeb(val, riic->base + offset);
> +}
> +
> +static inline u8 riic_readb_reg(struct riic_dev *riic, u8 offset)
> +{
> + return readb(riic->base + offset);
> +}
> - writeb(0, riic->base + RIIC_ICSR2);
> + riic_writeb_reg(0, riic, RIIC_ICSR2);
This clearly shows that the new accessors involve more typing work.
Why not just call them riic_writeb() and riic_readb()?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg
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
>On Fri, Mar 8, 2024 at 6:15 PM Biju Das <[email protected]> wrote:
>
>
> > Subject: Re: [PATCH 4/5] i2c: riic: Pass register offsets and chip details as OF data
> >
> > Hi Biju,
> >
> > On Fri, Mar 8, 2024 at 5:36 PM Biju Das <[email protected]> wrote:
> > >
> > > Hi Prabhakar,
> > >
> > > Thanks for the patch.
> > >
> > > > -----Original Message-----
> > > > From: Prabhakar <[email protected]>
> > > > Sent: Friday, March 8, 2024 5:27 PM
> > > > Subject: [PATCH 4/5] i2c: riic: Pass register offsets and chip
> > > > details as OF data
> > > >
> > > > From: Lad Prabhakar <[email protected]>
> > > >
> > > > With an increasing number of SoCs reusing this driver, each with
> > > > slight variations in the RIIC IP, it becomes necessary to support
> > > > passing these details as OF data. This approach simplifies the extension of the driver for other
> > SoCs.
> > > >
> > > > This patch lays the groundwork for adding support for the Renesas RZ/V2H SoC.
> > > >
> > > > Signed-off-by: Lad Prabhakar
> > > > <[email protected]>
> > > > Reviewed-by: Fabrizio Castro <[email protected]>
> > > > ---
> > > > drivers/i2c/busses/i2c-riic.c | 59
> > > > ++++++++++++++++++++++++++---------
> > > > 1 file changed, 44 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/drivers/i2c/busses/i2c-riic.c
> > > > b/drivers/i2c/busses/i2c-riic.c index
> > > > 49a12f1ecdf9..3398d639b754 100644
> > > > --- a/drivers/i2c/busses/i2c-riic.c
> > > > +++ b/drivers/i2c/busses/i2c-riic.c
> > > > @@ -46,18 +46,6 @@
> > > > #include <linux/pm_runtime.h>
> > > > #include <linux/reset.h>
> > > >
> > > > -#define RIIC_ICCR1 0x00
> > > > -#define RIIC_ICCR2 0x04
> > > > -#define RIIC_ICMR1 0x08
> > > > -#define RIIC_ICMR3 0x10
> > > > -#define RIIC_ICSER 0x18
> > > > -#define RIIC_ICIER 0x1c
> > > > -#define RIIC_ICSR2 0x24
> > > > -#define RIIC_ICBRL 0x34
> > > > -#define RIIC_ICBRH 0x38
> > > > -#define RIIC_ICDRT 0x3c
> > > > -#define RIIC_ICDRR 0x40
> > > > -
> > > > #define ICCR1_ICE 0x80
> > > > #define ICCR1_IICRST 0x40
> > > > #define ICCR1_SOWP 0x10
> > > > @@ -87,6 +75,13 @@
> > > >
> > > > #define RIIC_INIT_MSG -1
> > > >
> > > > +#define RIIC_RZ_A_TYPE 0
> > >
> > > > +
> > > > +struct riic_of_data {
> > > > + u8 family;
> > >
> > > Do you need family as compatible have this information?
> > >
> > Yes this is added to future proof it, as the RZ/V2H SoC has some bit differences in the slave address
> > register as compared to RZ/A and RZ/G2L. Comparing the family outside probe is not always preferred
> > hence this is added as part of OF data.
>
> Ok. Got it.
> But the family variable is unused and is wasting memory.
> Simple reg layout is enough for device data??
>
Ok, I'll drop it for the current implementation.
Cheers,
Prabhakar
Hi Geert,
Thank you for the review.
On Fri, Mar 8, 2024 at 7:47 PM Geert Uytterhoeven <geert@linux-m68korg> wrote:
>
> Hi Prabhakar,
>
> On Fri, Mar 8, 2024 at 6:28 PM Prabhakar <[email protected]> wrote:
> > From: Lad Prabhakar <[email protected]>
> >
> > Introduce helper functions for performing I2C read and write operations
> > in the RIIC driver.
> >
> > These helper functions lay the groundwork for adding support for the
> > RZ/V2H SoC. This is essential because the register offsets for the RZ/V2H
> > SoC differ from those of the RZ/A SoC. By abstracting the read and write
> > operations, we can seamlessly adapt the driver to support different SoC
> > variants without extensive modifications.
> >
> > This patch is part of the preparation process for integrating support for
> > the RZ/V2H SoC into the RIIC driver.
> >
> > Signed-off-by: Lad Prabhakar <[email protected]>
> > Reviewed-by: Fabrizio Castro <[email protected]>
>
> Thanks for your patch!
>
> > --- a/drivers/i2c/busses/i2c-riic.c
> > +++ b/drivers/i2c/busses/i2c-riic.c
> > @@ -105,9 +105,19 @@ struct riic_irq_desc {
> > char *name;
> > };
> >
> > +static inline void riic_writeb_reg(u8 val, struct riic_dev *riic, u8 offset)
>
> Having "riic" in the middle is definitely the wrong order of parameters ;-)
> Please make "riic" the first parameter.
>
Agreed, will do.
> > +{
> > + writeb(val, riic->base + offset);
> > +}
> > +
> > +static inline u8 riic_readb_reg(struct riic_dev *riic, u8 offset)
> > +{
> > + return readb(riic->base + offset);
> > +}
>
>
> > - writeb(0, riic->base + RIIC_ICSR2);
> > + riic_writeb_reg(0, riic, RIIC_ICSR2);
>
> This clearly shows that the new accessors involve more typing work.
> Why not just call them riic_writeb() and riic_readb()?
>
Ok, I'll rename them to riic_writeb() and riic_readb().
Cheers,
Prabhakar
On 08/03/2024 18:27, Prabhakar wrote:
> From: Lad Prabhakar <[email protected]>
>
> With the fallback string being utilized by multiple other SoCs, this
> patch updates the comment for the generic compatible string.
>
> Signed-off-by: Lad Prabhakar <[email protected]>
> Reviewed-by: Fabrizio Castro <[email protected]>
Really, you review a comment change? Internally?
Is this some sort of company policy? Are these even true reviews?
> ---
> Documentation/devicetree/bindings/i2c/renesas,riic.yaml | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/i2c/renesas,riic.yaml b/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
> index 2291a7cd619b..63ac5fe3208d 100644
> --- a/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
> +++ b/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
> @@ -22,7 +22,7 @@ properties:
> - renesas,riic-r9a07g043 # RZ/G2UL and RZ/Five
> - renesas,riic-r9a07g044 # RZ/G2{L,LC}
> - renesas,riic-r9a07g054 # RZ/V2L
> - - const: renesas,riic-rz # RZ/A or RZ/G2L
> + - const: renesas,riic-rz # generic RIIC compatible
Just drop the comment instead.
Best regards,
Krzysztof
On 08/03/2024 18:27, Prabhakar wrote:
> From: Lad Prabhakar <[email protected]>
>
> Document support for the I2C Bus Interface (RIIC) available in the
> Renesas RZ/V2H (R9A09G057) SoC.
>
> The RIIC interface in the Renesas RZ/V2H differs from RZ/A in a
> couple of ways:
> - Register offsets for the RZ/V2H SoC differ from those of the RZ/A SoC.
> - RZ/V2H register access is 8-bit, whereas RZ/A supports 8/16/32-bit.
> - RZ/V2H has some bit differences in the slave address register.
>
> To accommodate these differences in the existing driver, a new compatible
> string "renesas,riic-r9a09g057" is added.
>
> Signed-off-by: Lad Prabhakar <[email protected]>
> Reviewed-by: Fabrizio Castro <[email protected]>
I have doubts this are true reviews. What did it even show? Why this
review did not point problem with generic compatible?
> ---
> .../devicetree/bindings/i2c/renesas,riic.yaml | 21 ++++++++++++-------
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/i2c/renesas,riic.yaml b/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
> index 63ac5fe3208d..2a7125688647 100644
> --- a/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
> +++ b/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
> @@ -15,14 +15,19 @@ allOf:
>
> properties:
> compatible:
> - items:
> - - enum:
> - - renesas,riic-r7s72100 # RZ/A1H
> - - renesas,riic-r7s9210 # RZ/A2M
> - - renesas,riic-r9a07g043 # RZ/G2UL and RZ/Five
> - - renesas,riic-r9a07g044 # RZ/G2{L,LC}
> - - renesas,riic-r9a07g054 # RZ/V2L
> - - const: renesas,riic-rz # generic RIIC compatible
> + oneOf:
> + - items:
> + - enum:
> + - renesas,riic-r7s72100 # RZ/A1H
> + - renesas,riic-r7s9210 # RZ/A2M
> + - renesas,riic-r9a07g043 # RZ/G2UL and RZ/Five
> + - renesas,riic-r9a07g044 # RZ/G2{L,LC}
> + - renesas,riic-r9a07g054 # RZ/V2L
> + - const: renesas,riic-rz # generic RIIC compatible
> +
> + - items:
> + - enum:
> + - renesas,riic-r9a09g057 # RZ/V2H(P)
No, that does not look right. If you added generic compatible for all
RIIC then how can you add a new RIIC compatible which does not follow
generic one?
This shows the ridiculousness of these generic compatibles. They are
generic till you figure out the truth: oh crap, it's not generic.
Stop adding generic compatibles when they are not generic.
Best regards,
Krzysztof
Hi Krzysztof,
Thank you for the review.
On Sat, Mar 9, 2024 at 11:58 AM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 08/03/2024 18:27, Prabhakar wrote:
> > From: Lad Prabhakar <[email protected]>
> >
> > With the fallback string being utilized by multiple other SoCs, this
> > patch updates the comment for the generic compatible string.
> >
> > Signed-off-by: Lad Prabhakar <[email protected]>
> > Reviewed-by: Fabrizio Castro <[email protected]>
>
> Really, you review a comment change? Internally?
>
> Is this some sort of company policy? Are these even true reviews?
>
Yes this patch was reviewed internally and it's "real". Unfortunately
I cannot share the repo externally where this review was done but I
can assure it was reviewed. As this is not a single patch all the
patches in this series were internally reviewed. Is it bad to review a
comment change?
BTW what makes you think I have added fake review tags?
Is there any guideline you can point me to that states what needs to
be done when the code has been internally reviewed please. I'll make
sure I'll follow it.
> > ---
> > Documentation/devicetree/bindings/i2c/renesas,riic.yaml | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/i2c/renesas,riic.yaml b/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
> > index 2291a7cd619b..63ac5fe3208d 100644
> > --- a/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
> > +++ b/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
> > @@ -22,7 +22,7 @@ properties:
> > - renesas,riic-r9a07g043 # RZ/G2UL and RZ/Five
> > - renesas,riic-r9a07g044 # RZ/G2{L,LC}
> > - renesas,riic-r9a07g054 # RZ/V2L
> > - - const: renesas,riic-rz # RZ/A or RZ/G2L
> > + - const: renesas,riic-rz # generic RIIC compatible
>
> Just drop the comment instead.
>
Ok, I will drop it.
Cheers,
Prabhakar
Hi Krzysztof,
Thank you for the review.
On Sat, Mar 9, 2024 at 12:00 PM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 08/03/2024 18:27, Prabhakar wrote:
> > From: Lad Prabhakar <[email protected]>
> >
> > Document support for the I2C Bus Interface (RIIC) available in the
> > Renesas RZ/V2H (R9A09G057) SoC.
> >
> > The RIIC interface in the Renesas RZ/V2H differs from RZ/A in a
> > couple of ways:
> > - Register offsets for the RZ/V2H SoC differ from those of the RZ/A SoC.
> > - RZ/V2H register access is 8-bit, whereas RZ/A supports 8/16/32-bit.
> > - RZ/V2H has some bit differences in the slave address register.
> >
> > To accommodate these differences in the existing driver, a new compatible
> > string "renesas,riic-r9a09g057" is added.
> >
> > Signed-off-by: Lad Prabhakar <[email protected]>
> > Reviewed-by: Fabrizio Castro <[email protected]>
>
> I have doubts this are true reviews. What did it even show? Why this
> review did not point problem with generic compatible?
>
As mentioned in path#1 these are "real"!
> > ---
> > .../devicetree/bindings/i2c/renesas,riic.yaml | 21 ++++++++++++-------
> > 1 file changed, 13 insertions(+), 8 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/i2c/renesas,riic.yaml b/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
> > index 63ac5fe3208d..2a7125688647 100644
> > --- a/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
> > +++ b/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
> > @@ -15,14 +15,19 @@ allOf:
> >
> > properties:
> > compatible:
> > - items:
> > - - enum:
> > - - renesas,riic-r7s72100 # RZ/A1H
> > - - renesas,riic-r7s9210 # RZ/A2M
> > - - renesas,riic-r9a07g043 # RZ/G2UL and RZ/Five
> > - - renesas,riic-r9a07g044 # RZ/G2{L,LC}
> > - - renesas,riic-r9a07g054 # RZ/V2L
> > - - const: renesas,riic-rz # generic RIIC compatible
> > + oneOf:
> > + - items:
> > + - enum:
> > + - renesas,riic-r7s72100 # RZ/A1H
> > + - renesas,riic-r7s9210 # RZ/A2M
> > + - renesas,riic-r9a07g043 # RZ/G2UL and RZ/Five
> > + - renesas,riic-r9a07g044 # RZ/G2{L,LC}
> > + - renesas,riic-r9a07g054 # RZ/V2L
> > + - const: renesas,riic-rz # generic RIIC compatible
> > +
> > + - items:
> > + - enum:
> > + - renesas,riic-r9a09g057 # RZ/V2H(P)
>
> No, that does not look right. If you added generic compatible for all
> RIIC then how can you add a new RIIC compatible which does not follow
> generic one?
>
The generic compatible above which was added previously was for the
RZ/(A) SoCs and not for all the RIICs. The RZ/G2L family was also
compatible hence they fallback to the generic RZ one.
> This shows the ridiculousness of these generic compatibles. They are
> generic till you figure out the truth: oh crap, it's not generic.
>
Sorry I lack skills to predict the future of upcoming IP blocks which
fit in the SoC.
> Stop adding generic compatibles when they are not generic.
>
BTW I am not adding a generic compatible string here and instead
adding a SoC specific string. Anyway DT maintainers "should not" have
been accepting the generic compatibles from day 1 for any binding at
all.
Is there a guideline where you can point me to please for when to add
generic and when not to.
Cheers,
Prabhakar
Hi Prabhakar,
> -----Original Message-----
> From: Prabhakar <[email protected]>
> Sent: Friday, March 8, 2024 5:27 PM
> Subject: [PATCH 2/5] dt-bindings: i2c: renesas,riic: Document R9A09G057 support
>
> Document support for the I2C Bus Interface (RIIC) available in the Renesas RZ/V2H (R9A09G057) SoC.
>
> The RIIC interface in the Renesas RZ/V2H differs from RZ/A in a couple of ways:
> - Register offsets for the RZ/V2H SoC differ from those of the RZ/A SoC.
> - RZ/V2H register access is 8-bit, whereas RZ/A supports 8/16/32-bit.
> - RZ/V2H has some bit differences in the slave address register.
>
> To accommodate these differences in the existing driver, a new compatible string "renesas,riic-
> r9a09g057" is added.
>
> Signed-off-by: Lad Prabhakar <[email protected]>
> Reviewed-by: Fabrizio Castro <[email protected]>
> ---
> .../devicetree/bindings/i2c/renesas,riic.yaml | 21 ++++++++++++-------
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
> b/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
> index 63ac5fe3208d..2a7125688647 100644
> --- a/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
> +++ b/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
> @@ -15,14 +15,19 @@ allOf:
>
> properties:
> compatible:
> - items:
> - - enum:
> - - renesas,riic-r7s72100 # RZ/A1H
> - - renesas,riic-r7s9210 # RZ/A2M
> - - renesas,riic-r9a07g043 # RZ/G2UL and RZ/Five
> - - renesas,riic-r9a07g044 # RZ/G2{L,LC}
> - - renesas,riic-r9a07g054 # RZ/V2L
> - - const: renesas,riic-rz # generic RIIC compatible
> + oneOf:
> + - items:
> + - enum:
> + - renesas,riic-r7s72100 # RZ/A1H
> + - renesas,riic-r7s9210 # RZ/A2M
> + - renesas,riic-r9a07g043 # RZ/G2UL and RZ/Five
> + - renesas,riic-r9a07g044 # RZ/G2{L,LC}
> + - renesas,riic-r9a07g054 # RZ/V2L
> + - const: renesas,riic-rz # generic RIIC compatible
> +
> + - items:
> + - enum:
> + - renesas,riic-r9a09g057 # RZ/V2H(P)
Based on [1], this will be
oneOf:
- const: renesas,riic-r9a09g057 # RZ/V2H(P)
- items:
- enum:
- renesas,riic-r7s72100 # RZ/A1H
- renesas,riic-r7s9210 # RZ/A2M
- renesas,riic-r9a07g043 # RZ/G2UL and RZ/Five
- renesas,riic-r9a07g044 # RZ/G2{L,LC}
- renesas,riic-r9a07g054 # RZ/V2L
- const: renesas,riic-rz
[]1
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20240308&id=fddee1e686de077c80ad9dd61f4a50fa1d8b6605
Cheers,
Biju
On 10/03/2024 00:28, Lad, Prabhakar wrote:
> Hi Krzysztof,
>
> Thank you for the review.
>
> On Sat, Mar 9, 2024 at 12:00 PM Krzysztof Kozlowski
> <[email protected]> wrote:
>>
>> On 08/03/2024 18:27, Prabhakar wrote:
>>> From: Lad Prabhakar <[email protected]>
>>>
>>> Document support for the I2C Bus Interface (RIIC) available in the
>>> Renesas RZ/V2H (R9A09G057) SoC.
>>>
>>> The RIIC interface in the Renesas RZ/V2H differs from RZ/A in a
>>> couple of ways:
>>> - Register offsets for the RZ/V2H SoC differ from those of the RZ/A SoC.
>>> - RZ/V2H register access is 8-bit, whereas RZ/A supports 8/16/32-bit.
>>> - RZ/V2H has some bit differences in the slave address register.
>>>
>>> To accommodate these differences in the existing driver, a new compatible
>>> string "renesas,riic-r9a09g057" is added.
>>>
>>> Signed-off-by: Lad Prabhakar <[email protected]>
>>> Reviewed-by: Fabrizio Castro <[email protected]>
>>
>> I have doubts this are true reviews. What did it even show? Why this
>> review did not point problem with generic compatible?
>>
> As mentioned in path#1 these are "real"!
>
>>> ---
>>> .../devicetree/bindings/i2c/renesas,riic.yaml | 21 ++++++++++++-------
>>> 1 file changed, 13 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/i2c/renesas,riic.yaml b/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
>>> index 63ac5fe3208d..2a7125688647 100644
>>> --- a/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
>>> +++ b/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
>>> @@ -15,14 +15,19 @@ allOf:
>>>
>>> properties:
>>> compatible:
>>> - items:
>>> - - enum:
>>> - - renesas,riic-r7s72100 # RZ/A1H
>>> - - renesas,riic-r7s9210 # RZ/A2M
>>> - - renesas,riic-r9a07g043 # RZ/G2UL and RZ/Five
>>> - - renesas,riic-r9a07g044 # RZ/G2{L,LC}
>>> - - renesas,riic-r9a07g054 # RZ/V2L
>>> - - const: renesas,riic-rz # generic RIIC compatible
>>> + oneOf:
>>> + - items:
>>> + - enum:
>>> + - renesas,riic-r7s72100 # RZ/A1H
>>> + - renesas,riic-r7s9210 # RZ/A2M
>>> + - renesas,riic-r9a07g043 # RZ/G2UL and RZ/Five
>>> + - renesas,riic-r9a07g044 # RZ/G2{L,LC}
>>> + - renesas,riic-r9a07g054 # RZ/V2L
>>> + - const: renesas,riic-rz # generic RIIC compatible
>>> +
>>> + - items:
>>> + - enum:
>>> + - renesas,riic-r9a09g057 # RZ/V2H(P)
>>
>> No, that does not look right. If you added generic compatible for all
>> RIIC then how can you add a new RIIC compatible which does not follow
>> generic one?
>>
> The generic compatible above which was added previously was for the
> RZ/(A) SoCs and not for all the RIICs. The RZ/G2L family was also
No, it said: "generic RIIC compatible". It did not say "RIIC RZ/A". It
said RIIC RZ
> compatible hence they fallback to the generic RZ one.
riic-r9a09g057 is also RIIC RZ, isn't it?
>
>> This shows the ridiculousness of these generic compatibles. They are
>> generic till you figure out the truth: oh crap, it's not generic.
>>
> Sorry I lack skills to predict the future of upcoming IP blocks which
> fit in the SoC.
So don't use generic compatibles as fallbacks. That's the point.
>
>> Stop adding generic compatibles when they are not generic.
>>
> BTW I am not adding a generic compatible string here and instead
> adding a SoC specific string. Anyway DT maintainers "should not" have
> been accepting the generic compatibles from day 1 for any binding at
> all.
How can we know that you do not understand the meaning of compatibles? I
assume you do, so we ack your patches. In the same time *MULTIPLE* times
Rob said, and later me as well, people should use SoC specific
compatibles mostly.
>
> Is there a guideline where you can point me to please for when to add
> generic and when not to.
Guideline is: Don't use generic compatible at all, because you cannot
give it any meaning, based on this patch.
Best regards,
Krzysztof
Hi Krzysztof,
On Sun, Mar 10, 2024 at 9:10 AM Krzysztof Kozlowski
<[email protected]> wrote:
> On 10/03/2024 00:28, Lad, Prabhakar wrote:
> > On Sat, Mar 9, 2024 at 12:00 PM Krzysztof Kozlowski
> > <[email protected]> wrote:
> >> On 08/03/2024 18:27, Prabhakar wrote:
> >>> From: Lad Prabhakar <[email protected]>
> >>>
> >>> Document support for the I2C Bus Interface (RIIC) available in the
> >>> Renesas RZ/V2H (R9A09G057) SoC.
> >>>
> >>> The RIIC interface in the Renesas RZ/V2H differs from RZ/A in a
> >>> couple of ways:
> >>> - Register offsets for the RZ/V2H SoC differ from those of the RZ/A SoC.
> >>> - RZ/V2H register access is 8-bit, whereas RZ/A supports 8/16/32-bit.
> >>> - RZ/V2H has some bit differences in the slave address register.
> >>>
> >>> To accommodate these differences in the existing driver, a new compatible
> >>> string "renesas,riic-r9a09g057" is added.
> >>>
> >>> Signed-off-by: Lad Prabhakar <[email protected]>
> >>> Reviewed-by: Fabrizio Castro <[email protected]>
> >>> --- a/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
> >>> +++ b/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
> >>> @@ -15,14 +15,19 @@ allOf:
> >>>
> >>> properties:
> >>> compatible:
> >>> - items:
> >>> - - enum:
> >>> - - renesas,riic-r7s72100 # RZ/A1H
> >>> - - renesas,riic-r7s9210 # RZ/A2M
> >>> - - renesas,riic-r9a07g043 # RZ/G2UL and RZ/Five
> >>> - - renesas,riic-r9a07g044 # RZ/G2{L,LC}
> >>> - - renesas,riic-r9a07g054 # RZ/V2L
> >>> - - const: renesas,riic-rz # generic RIIC compatible
> >>> + oneOf:
> >>> + - items:
> >>> + - enum:
> >>> + - renesas,riic-r7s72100 # RZ/A1H
> >>> + - renesas,riic-r7s9210 # RZ/A2M
> >>> + - renesas,riic-r9a07g043 # RZ/G2UL and RZ/Five
> >>> + - renesas,riic-r9a07g044 # RZ/G2{L,LC}
> >>> + - renesas,riic-r9a07g054 # RZ/V2L
> >>> + - const: renesas,riic-rz # generic RIIC compatible
> >>> +
> >>> + - items:
> >>> + - enum:
> >>> + - renesas,riic-r9a09g057 # RZ/V2H(P)
> >>
> >> No, that does not look right. If you added generic compatible for all
> >> RIIC then how can you add a new RIIC compatible which does not follow
> >> generic one?
> >>
> > The generic compatible above which was added previously was for the
> > RZ/(A) SoCs and not for all the RIICs. The RZ/G2L family was also
>
> No, it said: "generic RIIC compatible". It did not say "RIIC RZ/A". It
> said RIIC RZ
At the time the original bindings were written, only RZ/A1, RZ/T1,
and RZ/N1 existed, and all RIIC modules present in these SoCs were
identical. Later, we got RZ/A2, which also included a compatible
RIIC block.
Somewhere along the timeline, the marketing department became creative,
and we got RZ/G1 (RZ/G1[HMNEC]) and RZ/G2 (RZ/G2[HMNE]), which were
unrelated to earlier RZ series :-( When marketing started smoking
something different, we got RZ/G2L, which is unrelated to RZ/G2,
but reuses some parts from RZ/A. Recently, we got RZ/G3S, which is
similar to RZ/G2L...
> > compatible hence they fallback to the generic RZ one.
>
> riic-r9a09g057 is also RIIC RZ, isn't it?
Yes, as in "it comes from the division that calls its products
RZ/something". But...
> >> This shows the ridiculousness of these generic compatibles. They are
> >> generic till you figure out the truth: oh crap, it's not generic.
> >>
> > Sorry I lack skills to predict the future of upcoming IP blocks which
> > fit in the SoC.
>
> So don't use generic compatibles as fallbacks. That's the point.
It's indeed difficult to predict the future. So SoC-specific compatible
values are safer.
At the same time, we want to avoid having to add compatible values for
each and every SoC to each driver, so we try to group SoCs per family.
For R-Car that worked out reasonably well, however, for RZ...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg
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
On 11/03/2024 10:00, Geert Uytterhoeven wrote:
>>>>> - - renesas,riic-r9a07g054 # RZ/V2L
>>>>> - - const: renesas,riic-rz # generic RIIC compatible
>>>>> + oneOf:
>>>>> + - items:
>>>>> + - enum:
>>>>> + - renesas,riic-r7s72100 # RZ/A1H
>>>>> + - renesas,riic-r7s9210 # RZ/A2M
>>>>> + - renesas,riic-r9a07g043 # RZ/G2UL and RZ/Five
>>>>> + - renesas,riic-r9a07g044 # RZ/G2{L,LC}
>>>>> + - renesas,riic-r9a07g054 # RZ/V2L
>>>>> + - const: renesas,riic-rz # generic RIIC compatible
>>>>> +
>>>>> + - items:
>>>>> + - enum:
>>>>> + - renesas,riic-r9a09g057 # RZ/V2H(P)
>>>>
>>>> No, that does not look right. If you added generic compatible for all
>>>> RIIC then how can you add a new RIIC compatible which does not follow
>>>> generic one?
>>>>
>>> The generic compatible above which was added previously was for the
>>> RZ/(A) SoCs and not for all the RIICs. The RZ/G2L family was also
>>
>> No, it said: "generic RIIC compatible". It did not say "RIIC RZ/A". It
>> said RIIC RZ
>
> At the time the original bindings were written, only RZ/A1, RZ/T1,
> and RZ/N1 existed, and all RIIC modules present in these SoCs were
> identical. Later, we got RZ/A2, which also included a compatible
> RIIC block.
>
> Somewhere along the timeline, the marketing department became creative,
> and we got RZ/G1 (RZ/G1[HMNEC]) and RZ/G2 (RZ/G2[HMNE]), which were
> unrelated to earlier RZ series :-( When marketing started smoking
> something different, we got RZ/G2L, which is unrelated to RZ/G2,
> but reuses some parts from RZ/A. Recently, we got RZ/G3S, which is
> similar to RZ/G2L...
That's fine, but then the comment "generic RIIC compatible" is confusing
for anyone not knowing this. Commit msg could also mention why the
generic compatible covers actually entirely different hardware. The
commit msg so far focused on the differences between these hardwares,
thus my questions - why do you create generic compatibles which are not
generic?
>
>>> compatible hence they fallback to the generic RZ one.
>>
>> riic-r9a09g057 is also RIIC RZ, isn't it?
>
> Yes, as in "it comes from the division that calls its products
> RZ/something". But...
>
>>>> This shows the ridiculousness of these generic compatibles. They are
>>>> generic till you figure out the truth: oh crap, it's not generic.
>>>>
>>> Sorry I lack skills to predict the future of upcoming IP blocks which
>>> fit in the SoC.
>>
>> So don't use generic compatibles as fallbacks. That's the point.
>
> It's indeed difficult to predict the future. So SoC-specific compatible
> values are safer.
> At the same time, we want to avoid having to add compatible values for
> each and every SoC to each driver, so we try to group SoCs per family.
> For R-Car that worked out reasonably well, however, for RZ...
I did not propose that. Nothing changes in your driver with my proposal.
Use SoC-compatibles only: for fallbacks and for specific(frontbacks?) parts.
To give you some sort of guidance for any future submission:
1. Use SoC-like fallback compatible, prepended with SoC-specific compatible.
2. If you insist on generic fallback compatible, its usage should be
limited to the cases where you can guarantee for 99.9% that future
devices will be compatible with this. I doubt anyone can guarantee that,
thus we keep repeating on mailing lists the same: go to point (1).
Best regards,
Krzysztof
Hi Krzysztof,
On Tue, Mar 12, 2024 at 12:04 PM Krzysztof Kozlowski
<[email protected]> wrote:
> On 11/03/2024 10:00, Geert Uytterhoeven wrote:
> >>>>> - - renesas,riic-r9a07g054 # RZ/V2L
> >>>>> - - const: renesas,riic-rz # generic RIIC compatible
> >>>>> + oneOf:
> >>>>> + - items:
> >>>>> + - enum:
> >>>>> + - renesas,riic-r7s72100 # RZ/A1H
> >>>>> + - renesas,riic-r7s9210 # RZ/A2M
> >>>>> + - renesas,riic-r9a07g043 # RZ/G2UL and RZ/Five
> >>>>> + - renesas,riic-r9a07g044 # RZ/G2{L,LC}
> >>>>> + - renesas,riic-r9a07g054 # RZ/V2L
> >>>>> + - const: renesas,riic-rz # generic RIIC compatible
> >>>>> +
> >>>>> + - items:
> >>>>> + - enum:
> >>>>> + - renesas,riic-r9a09g057 # RZ/V2H(P)
> >>>>
> >>>> No, that does not look right. If you added generic compatible for all
> >>>> RIIC then how can you add a new RIIC compatible which does not follow
> >>>> generic one?
> >>>>
> >>> The generic compatible above which was added previously was for the
> >>> RZ/(A) SoCs and not for all the RIICs. The RZ/G2L family was also
> >>
> >> No, it said: "generic RIIC compatible". It did not say "RIIC RZ/A". It
> >> said RIIC RZ
> >
> > At the time the original bindings were written, only RZ/A1, RZ/T1,
> > and RZ/N1 existed, and all RIIC modules present in these SoCs were
> > identical. Later, we got RZ/A2, which also included a compatible
> > RIIC block.
> >
> > Somewhere along the timeline, the marketing department became creative,
> > and we got RZ/G1 (RZ/G1[HMNEC]) and RZ/G2 (RZ/G2[HMNE]), which were
> > unrelated to earlier RZ series :-( When marketing started smoking
> > something different, we got RZ/G2L, which is unrelated to RZ/G2,
> > but reuses some parts from RZ/A. Recently, we got RZ/G3S, which is
> > similar to RZ/G2L...
>
> That's fine, but then the comment "generic RIIC compatible" is confusing
> for anyone not knowing this. Commit msg could also mention why the
> generic compatible covers actually entirely different hardware. The
> commit msg so far focused on the differences between these hardwares,
> thus my questions - why do you create generic compatibles which are not
> generic?
I agree the comment should be updated when adding a new variant which
is not compatible with the old generic variant (i.e. in this patch).
> >> So don't use generic compatibles as fallbacks. That's the point.
> >
> > It's indeed difficult to predict the future. So SoC-specific compatible
> > values are safer.
> > At the same time, we want to avoid having to add compatible values for
> > each and every SoC to each driver, so we try to group SoCs per family.
> > For R-Car that worked out reasonably well, however, for RZ...
>
> I did not propose that. Nothing changes in your driver with my proposal.
> Use SoC-compatibles only: for fallbacks and for specific(frontbacks?) parts.
>
> To give you some sort of guidance for any future submission:
> 1. Use SoC-like fallback compatible, prepended with SoC-specific compatible.
> 2. If you insist on generic fallback compatible, its usage should be
> limited to the cases where you can guarantee for 99.9% that future
> devices will be compatible with this. I doubt anyone can guarantee that,
> thus we keep repeating on mailing lists the same: go to point (1).
Personally, I am not such a big fan of method 1, for several reasons:
- Support for new SoCs is not always added in chronological SoC
release date order. So you could end up with:
compatible = "vendor,socB-foo", "vendor,socA-foo";
with socA being (much) newer than socB.
- Worse, adding support for different modules in different SoCs
can be unordered, too, leading to
compatible = "vendor,socB-foo", "vendor,socA-foo";
but
compatible = "vendor,socA-bar", "vendor,socB-bar";
Which is inconsistent. Fortunately we now have "make dtbs_check"
to catch mistakes there.
- When a third SoC arrives, which one do you pick?
compatible = "vendor,socC-foo", "vendor,socA-foo";
or
compatible = "vendor,socC-foo", "vendor,socB-foo";
Obviously you pick the former (unless you detected the issues
below first ;-)
- When socA-foo turns out to be slightly different from socB-foo,
socC-foo, ... you have to add of_device_id entries for all
socX-foo to the driver. With a family-specific fallback, you'd
be limited to one entry for the family-specific callback and
a second entry for the misbehaving socA.
So far my 5€c....
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg
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
On 12/03/2024 15:06, Geert Uytterhoeven wrote:
> Hi Krzysztof,
>
> On Tue, Mar 12, 2024 at 12:04 PM Krzysztof Kozlowski
> <[email protected]> wrote:
>> On 11/03/2024 10:00, Geert Uytterhoeven wrote:
>>>>>>> - - renesas,riic-r9a07g054 # RZ/V2L
>>>>>>> - - const: renesas,riic-rz # generic RIIC compatible
>>>>>>> + oneOf:
>>>>>>> + - items:
>>>>>>> + - enum:
>>>>>>> + - renesas,riic-r7s72100 # RZ/A1H
>>>>>>> + - renesas,riic-r7s9210 # RZ/A2M
>>>>>>> + - renesas,riic-r9a07g043 # RZ/G2UL and RZ/Five
>>>>>>> + - renesas,riic-r9a07g044 # RZ/G2{L,LC}
>>>>>>> + - renesas,riic-r9a07g054 # RZ/V2L
>>>>>>> + - const: renesas,riic-rz # generic RIIC compatible
>>>>>>> +
>>>>>>> + - items:
>>>>>>> + - enum:
>>>>>>> + - renesas,riic-r9a09g057 # RZ/V2H(P)
>>>>>>
>>>>>> No, that does not look right. If you added generic compatible for all
>>>>>> RIIC then how can you add a new RIIC compatible which does not follow
>>>>>> generic one?
>>>>>>
>>>>> The generic compatible above which was added previously was for the
>>>>> RZ/(A) SoCs and not for all the RIICs. The RZ/G2L family was also
>>>>
>>>> No, it said: "generic RIIC compatible". It did not say "RIIC RZ/A". It
>>>> said RIIC RZ
>>>
>>> At the time the original bindings were written, only RZ/A1, RZ/T1,
>>> and RZ/N1 existed, and all RIIC modules present in these SoCs were
>>> identical. Later, we got RZ/A2, which also included a compatible
>>> RIIC block.
>>>
>>> Somewhere along the timeline, the marketing department became creative,
>>> and we got RZ/G1 (RZ/G1[HMNEC]) and RZ/G2 (RZ/G2[HMNE]), which were
>>> unrelated to earlier RZ series :-( When marketing started smoking
>>> something different, we got RZ/G2L, which is unrelated to RZ/G2,
>>> but reuses some parts from RZ/A. Recently, we got RZ/G3S, which is
>>> similar to RZ/G2L...
>>
>> That's fine, but then the comment "generic RIIC compatible" is confusing
>> for anyone not knowing this. Commit msg could also mention why the
>> generic compatible covers actually entirely different hardware. The
>> commit msg so far focused on the differences between these hardwares,
>> thus my questions - why do you create generic compatibles which are not
>> generic?
>
> I agree the comment should be updated when adding a new variant which
> is not compatible with the old generic variant (i.e. in this patch).
>
>>>> So don't use generic compatibles as fallbacks. That's the point.
>>>
>>> It's indeed difficult to predict the future. So SoC-specific compatible
>>> values are safer.
>>> At the same time, we want to avoid having to add compatible values for
>>> each and every SoC to each driver, so we try to group SoCs per family.
>>> For R-Car that worked out reasonably well, however, for RZ...
>>
>> I did not propose that. Nothing changes in your driver with my proposal.
>> Use SoC-compatibles only: for fallbacks and for specific(frontbacks?) parts.
>>
>> To give you some sort of guidance for any future submission:
>> 1. Use SoC-like fallback compatible, prepended with SoC-specific compatible.
>> 2. If you insist on generic fallback compatible, its usage should be
>> limited to the cases where you can guarantee for 99.9% that future
>> devices will be compatible with this. I doubt anyone can guarantee that,
>> thus we keep repeating on mailing lists the same: go to point (1).
>
> Personally, I am not such a big fan of method 1, for several reasons:
>
> - Support for new SoCs is not always added in chronological SoC
> release date order. So you could end up with:
>
> compatible = "vendor,socB-foo", "vendor,socA-foo";
>
> with socA being (much) newer than socB.
Which is not a problem. We already have such examples in Qualcomm and
once you get used to seeing it, no one wonders. Of course it's not like
we target such reversed compatibility, but if it happens - does not
matter. compatible expresses compatibility, not timeframe.
>
> - Worse, adding support for different modules in different SoCs
> can be unordered, too, leading to
>
> compatible = "vendor,socB-foo", "vendor,socA-foo";
>
> but
>
> compatible = "vendor,socA-bar", "vendor,socB-bar";
Yeah, that looks not nice, indeed, but it's a rare case and still does
not matter for actual meaning. compatible does not express timeframe.
>
> Which is inconsistent. Fortunately we now have "make dtbs_check"
> to catch mistakes there.
>
> - When a third SoC arrives, which one do you pick?
>
> compatible = "vendor,socC-foo", "vendor,socA-foo";
>
> or
>
> compatible = "vendor,socC-foo", "vendor,socB-foo";
compatibility defines this.
>
> Obviously you pick the former (unless you detected the issues
> below first ;-)
>
> - When socA-foo turns out to be slightly different from socB-foo,
> socC-foo, ... you have to add of_device_id entries for all
> socX-foo to the driver. With a family-specific fallback, you'd
> be limited to one entry for the family-specific callback and
> a second entry for the misbehaving socA.
Yes and we have exactly that kind of argument from Bjorn A. for
Qualcomm. I repeat: this means your generic family-fallback changes
meaning, which is not really correct. Bindings (and DTS) are used for
outside projects, so if you need to change the driver for generic
fallback, all outside projects might be affected. You basically changed
ABI without telling it to anyone.
Best regards,
Krzysztof
Hi Prabhakar,
On Fri, Mar 8, 2024 at 6:28 PM Prabhakar <[email protected]> wrote:
> From: Lad Prabhakar <[email protected]>
>
> With the fallback string being utilized by multiple other SoCs, this
> patch updates the comment for the generic compatible string.
>
> Signed-off-by: Lad Prabhakar <[email protected]>
> Reviewed-by: Fabrizio Castro <[email protected]>
Thanks for your patch!
> --- a/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
> +++ b/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
> @@ -22,7 +22,7 @@ properties:
> - renesas,riic-r9a07g043 # RZ/G2UL and RZ/Five
> - renesas,riic-r9a07g044 # RZ/G2{L,LC}
> - renesas,riic-r9a07g054 # RZ/V2L
> - - const: renesas,riic-rz # RZ/A or RZ/G2L
> + - const: renesas,riic-rz # generic RIIC compatible
Please drop this patch, as this is not a truly generic RIIC compatible,
but applies to a subset of the RZ series only.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg
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
Hi Prabhakar,
On Sat, Mar 09, 2024 at 11:05:40PM +0000, Lad, Prabhakar wrote:
> On Sat, Mar 9, 2024 at 11:58 AM Krzysztof Kozlowski
> > On 08/03/2024 18:27, Prabhakar wrote:
> > > With the fallback string being utilized by multiple other SoCs, this
> > > patch updates the comment for the generic compatible string.
> > >
> > > Signed-off-by: Lad Prabhakar <[email protected]>
> > > Reviewed-by: Fabrizio Castro <[email protected]>
> >
> > Really, you review a comment change? Internally?
> >
> > Is this some sort of company policy? Are these even true reviews?
> >
> Yes this patch was reviewed internally and it's "real". Unfortunately
> I cannot share the repo externally where this review was done but I
> can assure it was reviewed. As this is not a single patch all the
> patches in this series were internally reviewed. Is it bad to review a
> comment change?
> BTW what makes you think I have added fake review tags?
I don't believe Krzysztof is questioning the validity of your
offline reviews, but the community is unaware of what happens
in your closed environment.
If you submit a patch with the r-b tag, it holds little
significance for me since I haven't witnessed the review process
myself. However, you are, of course, free to include it; I have
no objections to that.
My suggestion is for Fabrizio to publicly express his review on
this mailing list, which would add more value to the time he
spent reviewing your patch.
By the way, there are other companies that do this.
Andi
On 19/03/2024 22:19, Andi Shyti wrote:
> Hi Prabhakar,
>
> On Sat, Mar 09, 2024 at 11:05:40PM +0000, Lad, Prabhakar wrote:
>> On Sat, Mar 9, 2024 at 11:58 AM Krzysztof Kozlowski
>>> On 08/03/2024 18:27, Prabhakar wrote:
>>>> With the fallback string being utilized by multiple other SoCs, this
>>>> patch updates the comment for the generic compatible string.
>>>>
>>>> Signed-off-by: Lad Prabhakar <[email protected]>
>>>> Reviewed-by: Fabrizio Castro <[email protected]>
>>>
>>> Really, you review a comment change? Internally?
>>>
>>> Is this some sort of company policy? Are these even true reviews?
>>>
>> Yes this patch was reviewed internally and it's "real". Unfortunately
>> I cannot share the repo externally where this review was done but I
>> can assure it was reviewed. As this is not a single patch all the
>> patches in this series were internally reviewed. Is it bad to review a
>> comment change?
>> BTW what makes you think I have added fake review tags?
>
> I don't believe Krzysztof is questioning the validity of your
> offline reviews, but the community is unaware of what happens
> in your closed environment.
>
> If you submit a patch with the r-b tag, it holds little
> significance for me since I haven't witnessed the review process
> myself. However, you are, of course, free to include it; I have
> no objections to that.
>
> My suggestion is for Fabrizio to publicly express his review on
> this mailing list, which would add more value to the time he
> spent reviewing your patch.
>
> By the way, there are other companies that do this.
>
To me seeing such reviews of a trivial comment patch means reviews are
fake, just to fulfill the process. Especially done internally. Kind of
"patchset looks good, so +1 in Gerrit" (it does not matter if you use
Gerrit or not...). I don't consider them reviews, but useless company
policies. Provide real review or do not provide one at all. And provide
it public, so work with the community, not your inside systems.
Best regards,
Krzysztof