[U-Boot] i2c: Fix pca953x endianess issue, commit daa75b34828d45b7c1d63009188d45f4a32d06ba

[U-Boot] i2c: Fix pca953x endianess issue, commit daa75b34828d45b7c1d63009188d45f4a32d06ba

[Joakim Tjernlund]

This commit broke our pca953x usage(on ppc).

I wonder why gpio pins here has an endian, its not a number.
If there must be an endian connected with this, should it not
be a cpu_to_be16 instead, which will retain compatibility ?

[U-Boot] i2c: Fix pca953x endianess issue, commit daa75b34828d45b7c1d63009188d45f4a32d06ba

[Heiko Schocher]

Hello Joakim,

Am 10.10.2018 um 19:34 schrieb Joakim Tjernlund:
> This commit broke our pca953x usage(on ppc).
> 
> I wonder why gpio pins here has an endian, its not a number.
> If there must be an endian connected with this, should it not
> be a cpu_to_be16 instead, which will retain compatibility ?

Hmm.. good question, I think you are right. May dirk can do a test?
I have no pca953x with 16bit for doing a test.

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[U-Boot] i2c: Fix pca953x endianess issue, commit daa75b34828d45b7c1d63009188d45f4a32d06ba

[Dirk Eibach]

Hello,

we have a 16 bit value here, so we have to define whether bit0(containin
the information for IO0.0) is in the first or the second byte. Since the
PCA9555 does this encoding little endian, the conversion is allright.

Cheers
Dirk
Am Do., 11. Okt. 2018 um 07:42 Uhr schrieb Heiko Schocher <hs@denx.de>:
>
> Hello Joakim,
>
> Am 10.10.2018 um 19:34 schrieb Joakim Tjernlund:
> > This commit broke our pca953x usage(on ppc).
> >
> > I wonder why gpio pins here has an endian, its not a number.
> > If there must be an endian connected with this, should it not
> > be a cpu_to_be16 instead, which will retain compatibility ?
>
> Hmm.. good question, I think you are right. May dirk can do a test?
> I have no pca953x with 16bit for doing a test.
>
> bye,
> Heiko
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[U-Boot] i2c: Fix pca953x endianess issue, commit daa75b34828d45b7c1d63009188d45f4a32d06ba

[Joakim Tjernlund]

On Thu, 2018-10-11 at 16:11 +0200, Dirk Eibach wrote:
> 
> Hello,
> 
> we have a 16 bit value here, so we have to define whether bit0(containin the information for IO0.0) is in the first or the second byte. Since the PCA9555 does this encoding little endian, the conversion is allright. 
> 

You used it as some number but this is really just a bunch I/O pins. I haven't seen any
endian conversion in the kernel driver, have you? 
This is a bigger question than this driver really, so:

Does IO pins have an endian?

  Jocke

> Cheers
> Dirk
> Am Do., 11. Okt. 2018 um 07:42 Uhr schrieb Heiko Schocher <hs@denx.de>:
> >
> > Hello Joakim,
> >
> > Am 10.10.2018 um 19:34 schrieb Joakim Tjernlund:
> > > This commit broke our pca953x usage(on ppc).
> > >
> > > I wonder why gpio pins here has an endian, its not a number.
> > > If there must be an endian connected with this, should it not
> > > be a cpu_to_be16 instead, which will retain compatibility ?
> >
> > Hmm.. good question, I think you are right. May dirk can do a test?
> > I have no pca953x with 16bit for doing a test.
> >
> > bye,
> > Heiko
> > --
> > DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> > Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[U-Boot] i2c: Fix pca953x endianess issue, commit daa75b34828d45b7c1d63009188d45f4a32d06ba

[Mario Six]

Hi Jocke,
On Thu, Oct 11, 2018 at 5:48 PM Joakim Tjernlund
<Joakim.Tjernlund@infinera.com> wrote:
>
> On Thu, 2018-10-11 at 16:11 +0200, Dirk Eibach wrote:
> >
> > Hello,
> >
> > we have a 16 bit value here, so we have to define whether bit0(containin the information for IO0.0) is in the first or the second byte. Since the PCA9555 does this encoding little endian, the conversion is allright.
> >
>
> You used it as some number but this is really just a bunch I/O pins. I haven't seen any
> endian conversion in the kernel driver, have you?
Actually, there is. See [1]:

static int pca953x_write_regs_16(struct pca953x_chip *chip, int reg, u8 *val)
{
    u16 word = get_unaligned((u16 *)val);

    return i2c_smbus_write_word_data(chip->client, reg << 1, word);
}

And get_unaligned on powerpc (see [2]):

#ifdef __LITTLE_ENDIAN__
#define get_unaligned    __get_unaligned_le
#define put_unaligned    __put_unaligned_le
#else
#define get_unaligned    __get_unaligned_be
#define put_unaligned    __put_unaligned_be
#endif

So the endianness conversion happens in the get_unaligned.

> This is a bigger question than this driver really, so:
>
> Does IO pins have an endian?

The end effect really is where in the final 16-bit word the values of each pin
appear: If you read in little-endian order, the values of the first bank
(register 0) will end up in the lower byte, and the values of the second bank
(register 1) will end up in the higher byte (in big-endian order it's
reversed).

Since the bits in each bank register look like this (see [3]):

Register 0:
| 7    | 6    | 5    | 4    | 3    | 2    | 1    | 0    |
| I0.7 | I0.6 | I0.5 | I0.4 | I0.3 | I0.2 | I0.1 | I0.0 |

Register 1:
| 7    | 6    | 5    | 4    | 3    | 2    | 1    | 0    |
| I1.7 | I1.6 | I1.5 | I1.4 | I1.3 | I1.2 | I1.1 | I1.0 |

You will end up with this final word if you read little-endian (byte at lowest
address at lowest position in word):

| I1.7 | I1.6 | I1.5 | I1.4 | I1.3 | I1.2 | I1.1 | I1.0 | I0.7 | I0.6
| I0.5 | I0.4 | I0.3 | I0.2 | I0.1 | I0.0 |

Since you usually want to number the pins from 0 to 15 with bank 0 having 0-7
and bank 1 having 8-15, this is exactly what you want, since now

!!(word & (1 << NUM))

gives you the value of pin number NUM in this enumeration.

[1] https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpio-pca953x.c#L206
[2] https://elixir.bootlin.com/linux/latest/source/arch/powerpc/include/asm/unaligned.h
[3] https://www.nxp.com/docs/en/data-sheet/PCA9555.pdf
>
>   Jocke
>
> > Cheers
> > Dirk
> > Am Do., 11. Okt. 2018 um 07:42 Uhr schrieb Heiko Schocher <hs@denx.de>:
> > >
> > > Hello Joakim,
> > >
> > > Am 10.10.2018 um 19:34 schrieb Joakim Tjernlund:
> > > > This commit broke our pca953x usage(on ppc).
> > > >
> > > > I wonder why gpio pins here has an endian, its not a number.
> > > > If there must be an endian connected with this, should it not
> > > > be a cpu_to_be16 instead, which will retain compatibility ?
> > >
> > > Hmm.. good question, I think you are right. May dirk can do a test?
> > > I have no pca953x with 16bit for doing a test.
> > >
> > > bye,
> > > Heiko
>
Best regards,
Mario