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

Geert Uytterhoeven
2024-03-08T19:47:32Z

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

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