Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221008125924.1220203-1-j.neuschaefer@gmx.net>
In-Reply-To: <20221008125924.1220203-1-j.neuschaefer@gmx.net>
From:   Tali Perry <tali.perry1@gmail.com>
Date:   Tue, 11 Oct 2022 09:08:55 +0300
Message-ID: <CAHb3i=tzG0myrwHVvHCDNVf=yf8Ze2FhhX=7zdrhcaZAzqx9QQ@mail.gmail.com>
Subject: Re: [PATCH 1/2] i2c: npcm7xx: Group bank 0/1 registers together for readability
To:     =?UTF-8?Q?Jonathan_Neusch=C3=A4fer?= <j.neuschaefer@gmx.net>
Cc:     linux-i2c@vger.kernel.org, openbmc@lists.ozlabs.org,
        Avi Fishman <avifishman70@gmail.com>,
        Tomer Maimon <tmaimon77@gmail.com>,
        Patrick Venture <venture@google.com>,
        Nancy Yuen <yuenn@google.com>,
        Benjamin Fair <benjaminfair@google.com>,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Reviewed-by: Tali Perry <tali.perry1@gmail.com>

Thanks Jonathan!

On Sat, Oct 8, 2022 at 3:59 PM Jonathan Neusch=C3=A4fer
<j.neuschaefer@gmx.net> wrote:
>
> The unlabelled registers NPCM_I2CCTL4 to NPCM_I2CSCLHT overlap with the
> bank 1 registers below, and they are accessed after selecting bank 0, so
> they clearly belong to bank 0.
>
> Move them together with the other bank 0 registers, and move the
> unrelated definition of npcm_i2caddr down to keep the banked registers
> in one piece.
>
> Signed-off-by: Jonathan Neusch=C3=A4fer <j.neuschaefer@gmx.net>
> ---
>  drivers/i2c/busses/i2c-npcm7xx.c | 31 +++++++++++++++----------------
>  1 file changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-np=
cm7xx.c
> index 0c365b57d9572..9a7a2d0bf5765 100644
> --- a/drivers/i2c/busses/i2c-npcm7xx.c
> +++ b/drivers/i2c/busses/i2c-npcm7xx.c
> @@ -106,7 +106,7 @@ enum i2c_addr {
>  #define NPCM_I2CCST3                   0x19
>  #define I2C_VER                                0x1F
>
> -/*BANK0 regs*/
> +/* BANK 0 regs */
>  #define NPCM_I2CADDR3                  0x10
>  #define NPCM_I2CADDR7                  0x11
>  #define NPCM_I2CADDR4                  0x12
> @@ -115,6 +115,20 @@ enum i2c_addr {
>  #define NPCM_I2CADDR9                  0x15
>  #define NPCM_I2CADDR6                  0x16
>  #define NPCM_I2CADDR10                 0x17
> +#define NPCM_I2CCTL4                   0x1A
> +#define NPCM_I2CCTL5                   0x1B
> +#define NPCM_I2CSCLLT                  0x1C /* SCL Low Time */
> +#define NPCM_I2CFIF_CTL                        0x1D /* FIFO Control */
> +#define NPCM_I2CSCLHT                  0x1E /* SCL High Time */
> +
> +/* BANK 1 regs */
> +#define NPCM_I2CFIF_CTS                        0x10 /* Both FIFOs Contro=
l and Status */
> +#define NPCM_I2CTXF_CTL                        0x12 /* Tx-FIFO Control *=
/
> +#define NPCM_I2CT_OUT                  0x14 /* Bus T.O. */
> +#define NPCM_I2CPEC                    0x16 /* PEC Data */
> +#define NPCM_I2CTXF_STS                        0x1A /* Tx-FIFO Status */
> +#define NPCM_I2CRXF_STS                        0x1C /* Rx-FIFO Status */
> +#define NPCM_I2CRXF_CTL                        0x1E /* Rx-FIFO Control *=
/
>
>  #if IS_ENABLED(CONFIG_I2C_SLAVE)
>  /*
> @@ -131,21 +145,6 @@ static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] =3D =
{
>  };
>  #endif
>
> -#define NPCM_I2CCTL4                   0x1A
> -#define NPCM_I2CCTL5                   0x1B
> -#define NPCM_I2CSCLLT                  0x1C /* SCL Low Time */
> -#define NPCM_I2CFIF_CTL                        0x1D /* FIFO Control */
> -#define NPCM_I2CSCLHT                  0x1E /* SCL High Time */
> -
> -/* BANK 1 regs */
> -#define NPCM_I2CFIF_CTS                        0x10 /* Both FIFOs Contro=
l and Status */
> -#define NPCM_I2CTXF_CTL                        0x12 /* Tx-FIFO Control *=
/
> -#define NPCM_I2CT_OUT                  0x14 /* Bus T.O. */
> -#define NPCM_I2CPEC                    0x16 /* PEC Data */
> -#define NPCM_I2CTXF_STS                        0x1A /* Tx-FIFO Status */
> -#define NPCM_I2CRXF_STS                        0x1C /* Rx-FIFO Status */
> -#define NPCM_I2CRXF_CTL                        0x1E /* Rx-FIFO Control *=
/
> -
>  /* NPCM_I2CST reg fields */
>  #define NPCM_I2CST_XMIT                        BIT(0)
>  #define NPCM_I2CST_MASTER              BIT(1)
> --
> 2.35.1
>