[v1] serial: 8250_exar: Clean up the driver

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

Posted by Andy Shevchenko 1 year, 7 months ago

Use BIT() in exar_ee_read() like other functions do.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_exar.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 306bc6d7c141..bf3730f4231d 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -340,13 +340,13 @@ static u16 exar_ee_read(struct exar8250 *priv, u8 ee_addr)
 	exar_ee_write_bit(priv, 0);
 
 	// 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);
 	}
 
 	exar_ee_deselect(priv);
-- 
2.43.0.rc1.1336.g36b5255a03ac

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

Posted by Ilpo Järvinen 1 year, 7 months ago

On Thu, 2 May 2024, Andy Shevchenko wrote:

> Use BIT() in exar_ee_read() like other functions do.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/tty/serial/8250/8250_exar.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> index 306bc6d7c141..bf3730f4231d 100644
> --- a/drivers/tty/serial/8250/8250_exar.c
> +++ b/drivers/tty/serial/8250/8250_exar.c
> @@ -340,13 +340,13 @@ static u16 exar_ee_read(struct exar8250 *priv, u8 ee_addr)
>  	exar_ee_write_bit(priv, 0);
>  
>  	// 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?

-- 
 i.

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

Posted by Andy Shevchenko 1 year, 7 months ago

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.

-- 
With Best Regards,
Andy Shevchenko

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

Posted by Parker Newman 1 year, 7 months ago

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ä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,
-Parker

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

Posted by Andy Shevchenko 1 year, 7 months ago

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@linux.intel.com> 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

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

Posted by Parker Newman 1 year, 7 months ago

On Fri, 3 May 2024 18:35:45 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> 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@linux.intel.com> 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?

The address portion is 6 bits, nothing extra, so what you have is correct.

The original code was legacy, I cleaned it up but didn't change those loops. 

Your method works out the the same number of bits as the legacy method
which sets bit 5 and has to shift right 6 times to get i = 0 which ends the loop.

Parker

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

Posted by Andy Shevchenko 1 year, 7 months ago

On Fri, May 03, 2024 at 02:56:33PM -0400, Parker Newman wrote:
> On Fri, 3 May 2024 18:35:45 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > 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@linux.intel.com> 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?
> 
> The address portion is 6 bits, nothing extra, so what you have is correct.
> 
> The original code was legacy, I cleaned it up but didn't change those loops. 
> 
> Your method works out the the same number of bits as the legacy method
> which sets bit 5 and has to shift right 6 times to get i = 0 which ends the loop.

Okay, thank your for confirming the correctness of a new code!

-- 
With Best Regards,
Andy Shevchenko