Received-SPF: pass (google.com: domain of linux-kernel+bounces-167772-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 3 May 2024 10:26:32 -0400
From: Parker Newman <parker@finest.io>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Ilpo =?UTF-8?B?SsOkcnZpbmVu?= <ilpo.jarvinen@linux.intel.com>, Parker
 Newman <pnewman@connecttech.com>, LKML <linux-kernel@vger.kernel.org>,
 linux-serial <linux-serial@vger.kernel.org>, Greg Kroah-Hartman
 <gregkh@linuxfoundation.org>, Jiri Slaby <jirislaby@kernel.org>
Subject: Re: [PATCH v1 11/13] serial: 8250_exar: Use BIT() in exar_ee_read()
Message-ID: <20240503102632.112a34bd@SWDEV2.connecttech.local>
In-Reply-To: <ZjPLQeTEKvt7B3mj@smile.fi.intel.com>
References: <20240502144626.2716994-1-andriy.shevchenko@linux.intel.com>
	<20240502144626.2716994-12-andriy.shevchenko@linux.intel.com>
	<702a9145-5bc1-c765-a1fa-278702741637@linux.intel.com>
	<ZjPLQeTEKvt7B3mj@smile.fi.intel.com>
Organization: Connect Tech Inc.
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
UI-OutboundReport: notjunk:1;M01:P0:hw/IaLlh8sI=;BvyzJB1ag/J7juyaFuRSiqM2srX
 1Fvy9ykiwlh0bHBqoxdRlkaVJsuVVBI2VIcZ+ist/JeCBXH2YkmAC2pM/5x12HGboxi5r46dp
 XZcSexsbCbY+5TPmE5UPzJSn41DEOYddYwm9UPNK9OWg2Dprpd0GGCjjJAg/wE7RsR16WKWt8
 NkhhDvHz8GYgn5RqBpSHiQ0b9zwRSt1RA3oiDvBEWmIKXbrszKddaGY1kHoFCWpFOK/Rzmjak
 dMGzEjbC7LISFY/xTyC5m+zIxrwp6NYMe0fzw/Y6Or4aceVndQZo1h2MOKhvy+Je+IF/wvfd4
 eCQIEeBBaU4rJCOPGW9cislu9syUDeEDzATxnoiNarqGoY6YiLcRtn+tD3PMy9umrhhsIdcQx
 HgC1SyZZhyK1t2xYx+bomuFSAK3q3YfeuNVwRmvYi4xvwT7bqNrgmodS8aTthKoEBSr5ucTzw
 wKV9AJOcdlV59BaSR75B85J6eePeXqrmApxDQrAJpp+FC5txB4v88NFgUomFiEGHu4RJ1rNEI
 LNG6hx4NiEi9GKbjWH9yas23UE1aDHw93wyFw/BYgQ4Rgt9oQs/uxCCuptYiH4wCh8NQ4jxC3
 2xYADNW6faz3uLZEtq3mk1knFElVd7GknnXegYmDqQMIAMEdh+JheqOUDe9jagsRhS3qTSeQR
 pEDE8hQtC/sp5rJOSgsys2l61hWtP92150EuhBRtpb1hA4V4rokOj9qOzK5xKP/mWbuEyM1Ol
 b2O1tfvhV86nZ05oq9rzmIDJ5+VN8UaeIi/sMuAD2zMqP08V2s/9do=

On Thu, 2 May 2024 20:20:01 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Thu, May 02, 2024 at 07:08:21PM +0300, Ilpo J=C3=A4rvinen wrote:
> > On Thu, 2 May 2024, Andy Shevchenko wrote: =20
>=20
> ...
>=20
> > >  	// Send address to read from
> > > -	for (i =3D 1 << (UART_EXAR_REGB_EE_ADDR_SIZE - 1); i; i >>=3D 1)
> > > -		exar_ee_write_bit(priv, (ee_addr & i));
> > > +	for (i =3D UART_EXAR_REGB_EE_ADDR_SIZE - 1; i >=3D 0; i--)
> > > +		exar_ee_write_bit(priv, ee_addr & BIT(i));
> > > =20
> > >  	// Read data 1 bit at a time
> > >  	for (i =3D 0; i <=3D UART_EXAR_REGB_EE_DATA_SIZE; i++) {
> > > -		data <<=3D 1;
> > > -		data |=3D exar_ee_read_bit(priv);
> > > +		if (exar_ee_read_bit(priv))
> > > +			data |=3D BIT(i); =20
> >=20
> > Does this end up reversing the order of bits? In the original, data was=
 left
> > shifted which moved the existing bits and added the lsb but the replace=
ment
> > adds highest bit on each iteration? =20
>=20
> Oh, seems a good catch!
>=20
> I was also wondering, but missed this somehow. Seems the EEPROM is in BE =
mode,
> so two loops has to be aligned.
>=20

I just tested this and Ilpo is correct, the read loop portion is backwards =
as=20
expected. This is the corrected loop:

    // Read data 1 bit at a time
    for (i =3D UART_EXAR_REGB_EE_DATA_SIZE; i >=3D 0; i--) {
        if (exar_ee_read_bit(priv))
            data |=3D BIT(i);
    }

I know this looks wrong because its looping from 16->0 rather than the=20
more intuitive 15->0 for a 16bit value. This is actually correct however=20
because according to the AT93C46D datasheet there is always dummy 0 bit
before the actual 16 bits of data.

I hope that helps,
-Parker