diff mbox series

[3/5] i2c: riic: Introduce helper functions for I2C read/write operations

Message ID 20240308172726.225357-4-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series Add RIIC support for Renesas RZ/V2H SoC | expand

Commit Message

Lad, Prabhakar March 8, 2024, 5:27 p.m. UTC 
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

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 <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
---
 drivers/i2c/busses/i2c-riic.c | 56 +++++++++++++++++++++--------------
 1 file changed, 33 insertions(+), 23 deletions(-)

Comments

Geert Uytterhoeven March 8, 2024, 7:47 p.m. UTC | #1 
Hi Prabhakar,

On Fri, Mar 8, 2024 at 6:28 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> 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 <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>

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
Lad, Prabhakar March 8, 2024, 9 p.m. UTC | #2 
Hi Geert,

Thank you for the review.

On Fri, Mar 8, 2024 at 7:47 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Fri, Mar 8, 2024 at 6:28 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > 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 <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > Reviewed-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
>
> 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
diff mbox series

Patch

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);