Re: [PATCH v1 11/13] serial: 8250_exar: Use BIT() in exar_ee_read()

[Andy Shevchenko]

On Fri, May 03, 2024 at 10:26:32AM -0400, Parker Newman wrote:
> On Thu, 2 May 2024 20:20:01 +0300
> Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> > On Thu, May 02, 2024 at 07:08:21PM +0300, Ilpo Järvinen wrote:
> > > On Thu, 2 May 2024, Andy Shevchenko wrote:  

..

> > > >  	// Send address to read from
> > > > -	for (i = 1 << (UART_EXAR_REGB_EE_ADDR_SIZE - 1); i; i >>= 1)
> > > > -		exar_ee_write_bit(priv, (ee_addr & i));
> > > > +	for (i = UART_EXAR_REGB_EE_ADDR_SIZE - 1; i >= 0; i--)
> > > > +		exar_ee_write_bit(priv, ee_addr & BIT(i));
> > > >  
> > > >  	// Read data 1 bit at a time
> > > >  	for (i = 0; i <= UART_EXAR_REGB_EE_DATA_SIZE; i++) {
> > > > -		data <<= 1;
> > > > -		data |= exar_ee_read_bit(priv);
> > > > +		if (exar_ee_read_bit(priv))
> > > > +			data |= BIT(i);  
> > > 
> > > 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 replacement
> > > adds highest bit on each iteration?  
> > 
> > Oh, seems a good catch!
> > 
> > I was also wondering, but missed this somehow. Seems the EEPROM is in BE mode,
> > so two loops has to be aligned.
> > 
> 
> I just tested this and Ilpo is correct, the read loop portion is backwards as 
> expected. This is the corrected loop:
> 
>     // Read data 1 bit at a time
>     for (i = UART_EXAR_REGB_EE_DATA_SIZE; i >= 0; i--) {
>         if (exar_ee_read_bit(priv))
>             data |= BIT(i);
>     }
> 
> I know this looks wrong because its looping from 16->0 rather than the 
> more intuitive 15->0 for a 16bit value. This is actually correct however 
> because according to the AT93C46D datasheet there is always dummy 0 bit
> before the actual 16 bits of data.
> 
> I hope that helps,

Yes, it helps and means that we need that comment to be added to the code. Is
it the same applicable to the write part above (for address)? Because AFAIU
mine is one bit longer than yours. Maybe in the original code is a bug? Have
you tried to read high addresses?


-- 
With Best Regards,
Andy Shevchenko