serial: 8250_exar: Replace custom EEPROM code with eeprom_93cx6

PATCH Superseded Parker Newman 4 participants 29 messages Sep 13, 2024 – Sep 16, 2024

Subsystems (2)

TTY layer and serial drivers Char and misc drivers

lore

Share & tools

lore

Parker Newman

author

2024-09-13 15:01 UTC #0 lore raw

cover serial: 8250_exar: Replace custom EEPROM code with eeprom_93cx6

From: Parker Newman <[email protected]>

This series of patches replaces the custom 93cx6 EEPROM read functions in the 8250_exar driver with the eeprom_93cx6 driver. This removes duplicate code and improves code readability.

In order to use the eeprom_93cx6 driver a quirk needed to be added to add an extra clock cycle before reading from the EEPROM. This is similar to the quirk in the eeprom_93xx46 driver.

More details in associated patch and mailing list discussion with Andy Shevchenko about these changes:

Link https://lore.kernel.org/linux-serial/[email protected]/ open here

Parker Newman (6):
misc: eeprom: eeprom_93cx6: Add quirk for extra read clock cycle
misc: eeprom: eeprom_93cx6: Switch to BIT() macro
misc: eeprom: eeprom_93cx6: Replace printk(KERN_ERR ...) with pr_err()
serial: 8250_exar: Replace custom EEPROM read with eeprom_93cx6
serial: 8250_exar: Remove old exar_ee_read() and other unneeded code
serial: 8250_exar: Add select EEPROM_93CX6 in Kconfig

 drivers/misc/eeprom/eeprom_93cx6.c  |  22 ++++-
 drivers/tty/serial/8250/8250_exar.c | 122 +++++++---------------------
 drivers/tty/serial/8250/Kconfig     |   1 +
 include/linux/eeprom_93cx6.h        |   7 ++
 4 files changed, 58 insertions(+), 94 deletions(-)

base-commit: 5ed771f174726ae879945d4f148a9005ac909cb7 --
2.46.0

Parker Newman

author

2024-09-13 14:56 UTC #1 lore raw

PATCH 1/6 misc: eeprom: eeprom_93cx6: Add quirk for extra read clock cycle

From: Parker Newman <[email protected]>

This patch adds a quirk similar to eeprom_93xx46 to add an extra clock cycle before reading data from the EEPROM.

The 93Cx6 family of EEPROMs output a "dummy 0 bit" between the writing of the op-code/address from the host to the EEPROM and the reading of the actual data from the EEPROM.

More info can be found on page 6 of the AT93C46 datasheet. Similar notes are found in other 93xx6 datasheets.

Link https://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-5193-SEEPROM-AT93C46D-Datasheet.pdf

In summary the read operation for a 93Cx6 EEPROM is:
Write to EEPROM : 110[A5-A0] (9 bits)
Read from EEPROM: 0[D15-D0] (17 bits)

Where:
110 is the start bit and READ OpCode [A5-A0] is the address to read from 0 is a "dummy bit" preceding the actual data [D15-D0] is the actual data.

Looking at the READ timing diagrams in the 93Cx6 datasheets the dummy bit should be clocked out on the last address bit clock cycle meaning it should be discarded naturally.

However, depending on the hardware configuration sometimes this dummy bit is not discarded. This is the case with Exar PCI UARTs which require an extra clock cycle between sending the address and reading the data.

Signed-off-by Parker Newman <[email protected]>

---
 drivers/misc/eeprom/eeprom_93cx6.c | 15 +++++++++++++++
 include/linux/eeprom_93cx6.h       |  7 +++++++
 2 files changed, 22 insertions(+)

drivers/misc/eeprom/eeprom_93cx6.c +15 -0

index 9627294fe3e9..0f52d3c4bc1d 100644
@@ -18,6 +18,11 @@ MODULE_VERSION("1.0");
 MODULE_DESCRIPTION("EEPROM 93cx6 chip driver");
 MODULE_LICENSE("GPL");
 
+static inline bool has_quirk_extra_read_cycle(struct eeprom_93cx6 *eeprom)
+{
+	return eeprom->quirks & PCI_EEPROM_QUIRK_EXTRA_READ_CYCLE;
+}
+
 static inline void eeprom_93cx6_pulse_high(struct eeprom_93cx6 *eeprom)
 {
 	eeprom->reg_data_clock = 1;

@@ -186,6 +191,11 @@ void eeprom_93cx6_read(struct eeprom_93cx6 *eeprom, const u8 word,
 	eeprom_93cx6_write_bits(eeprom, command,
 		PCI_EEPROM_WIDTH_OPCODE + eeprom->width);
 
+	if (has_quirk_extra_read_cycle(eeprom)) {
+		eeprom_93cx6_pulse_high(eeprom);
+		eeprom_93cx6_pulse_low(eeprom);
+	}
+
 	/*
 	 * Read the requested 16 bits.
 	 */

@@ -252,6 +262,11 @@ void eeprom_93cx6_readb(struct eeprom_93cx6 *eeprom, const u8 byte,
 	eeprom_93cx6_write_bits(eeprom, command,
 		PCI_EEPROM_WIDTH_OPCODE + eeprom->width + 1);
 
+	if (has_quirk_extra_read_cycle(eeprom)) {
+		eeprom_93cx6_pulse_high(eeprom);
+		eeprom_93cx6_pulse_low(eeprom);
+	}
+
 	/*
 	 * Read the requested 8 bits.
 	 */

include/linux/eeprom_93cx6.h +7 -0

index c860c72a921d..5274bcc7ca39 100644
@@ -11,6 +11,8 @@
 	Supported chipsets: 93c46, 93c56 and 93c66.
  */
 
+#include <linux/bits.h>
+
 /*
  * EEPROM operation defines.
  */

@@ -25,6 +27,9 @@
 #define PCI_EEPROM_EWDS_OPCODE	0x10
 #define PCI_EEPROM_EWEN_OPCODE	0x13
 
+/* Some EEPROMs require an extra clock cycle before reading */
+#define PCI_EEPROM_QUIRK_EXTRA_READ_CYCLE	BIT(0)
+
 /**
  * struct eeprom_93cx6 - control structure for setting the commands
  * for reading the eeprom data.

@@ -34,6 +39,7 @@
  * @register_write(struct eeprom_93cx6 *eeprom): handler to
  * write to the eeprom register by using all reg_* fields.
  * @width: eeprom width, should be one of the PCI_EEPROM_WIDTH_* defines
+ * @quirks: eeprom or controller quirk bitfield
  * @drive_data: Set if we're driving the data line.
  * @reg_data_in: register field to indicate data input
  * @reg_data_out: register field to indicate data output

@@ -50,6 +56,7 @@ struct eeprom_93cx6 {
 	void (*register_write)(struct eeprom_93cx6 *eeprom);
 
 	int width;
+	unsigned int quirks;
 
 	char drive_data;
 	char reg_data_in;

--
2.46.0

Andy Shevchenko

2024-09-13 17:48 UTC #2 lore raw

Re: [PATCH v1 1/6] misc: eeprom: eeprom_93cx6: Add quirk for extra read clock cycle

Quoted 5 lines · Parker Newman

From: Parker Newman <[email protected]>
This patch adds a quirk similar to eeprom_93xx46 to add an extra clock cycle before reading data from the EEPROM.

Quoted 8 lines

The 93Cx6 family of EEPROMs output a "dummy 0 bit" between the writing of the op-code/address from the host to the EEPROM and the reading of the actual data from the EEPROM.
More info can be found on page 6 of the AT93C46 datasheet. Similar notes are found in other 93xx6 datasheets.
Link https://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-5193-SEEPROM-AT93C46D-Datasheet.pdf

Make it a tag (i.e. locate just above your SoB tag)

Quoted 4 lines

In summary the read operation for a 93Cx6 EEPROM is:
Write to EEPROM : 110[A5-A0] (9 bits)
Read from EEPROM: 0[D15-D0] (17 bits)

The mixed TABs/space here (one extra space after :)

Quoted 5 lines

Where:
110 is the start bit and READ OpCode [A5-A0] is the address to read from 0 is a "dummy bit" preceding the actual data [D15-D0] is the actual data.

Also leading spaces, please remove them and use TAB, or use spaces only.

Quoted 7 lines

Looking at the READ timing diagrams in the 93Cx6 datasheets the dummy bit should be clocked out on the last address bit clock cycle meaning it should be discarded naturally.
However, depending on the hardware configuration sometimes this dummy bit is not discarded. This is the case with Exar PCI UARTs which require an extra clock cycle between sending the address and reading the data.

...

drivers/misc/eeprom/eeprom_93cx6.c MODULE_VERSION("1.0"); · line 24

Quoted 4 lines

drivers/misc/eeprom/eeprom_93cx6.c +4 -0

+static inline bool has_quirk_extra_read_cycle(struct eeprom_93cx6 *eeprom)
+{
+	return eeprom->quirks & PCI_EEPROM_QUIRK_EXTRA_READ_CYCLE;
+}

So, this makes sense to be in a header since everything else related to that also in the header already.

...

Review

Quoted 2 lines

(quoted diff) +2 -0

+	if (has_quirk_extra_read_cycle(eeprom)) {
+		eeprom_93cx6_pulse_high(eeprom);

No additional delay is needed?

drivers/misc/eeprom/eeprom_93cx6.c void eeprom_93cx6_read(struct eeprom_93cx6 *eeprom, const u8 word, · line 195

Quoted 5 lines

drivers/misc/eeprom/eeprom_93cx6.c +4 -0

+		eeprom_93cx6_pulse_low(eeprom);
+	}
 
+	if (has_quirk_extra_read_cycle(eeprom)) {
+		eeprom_93cx6_pulse_high(eeprom);

Ditto.

Quoted 2 lines

(quoted diff) +2 -0

+		eeprom_93cx6_pulse_low(eeprom);
+	}

...

include/linux/eeprom_93cx6.h line 31

Quoted 2 lines

include/linux/eeprom_93cx6.h +2 -0

+/* Some EEPROMs require an extra clock cycle before reading */
+#define PCI_EEPROM_QUIRK_EXTRA_READ_CYCLE	BIT(0)

I would move it directly into the structure definitions, just after quirk field (the same way it's done in the other driver)...

...

include/linux/eeprom_93cx6.h struct eeprom_93cx6 { · line 59

Quoted 2 lines

include/linux/eeprom_93cx6.h +1 -0
 	int width;
+	unsigned int quirks;

...somewhere here.

Quoted 2 lines

include/linux/eeprom_93cx6.h +0 -0
 	char drive_data;
 	char reg_data_in;

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Parker Newman

author

2024-09-13 18:24 UTC #3 lore raw

Re: [PATCH v1 1/6] misc: eeprom: eeprom_93cx6: Add quirk for extra read clock cycle

Quoted 10 lines · On Fri, 13 Sep 2024 20:48:03 +0300 Andy Shevchenko…

Quoted 9 lines · Parker Newman
From: Parker Newman <[email protected]>
This patch adds a quirk similar to eeprom_93xx46 to add an extra clock cycle before reading data from the EEPROM.
The 93Cx6 family of EEPROMs output a "dummy 0 bit" between the writing of the op-code/address from the host to the EEPROM and the reading of the actual data from the EEPROM.

Quoted 7 lines

Quoted 4 lines
More info can be found on page 6 of the AT93C46 datasheet. Similar notes are found in other 93xx6 datasheets.
Link https://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-5193-SEEPROM-AT93C46D-Datasheet.pdf
Make it a tag (i.e. locate just above your SoB tag)

Sorry, not 100% sure what you mean by tag? Do I just need to move the Link: entry to be above my Sign-off? Or is there something else? Thanks!

Quoted 7 lines

Quoted 4 lines
In summary the read operation for a 93Cx6 EEPROM is:
Write to EEPROM : 110[A5-A0] (9 bits)
Read from EEPROM: 0[D15-D0] (17 bits)
The mixed TABs/space here (one extra space after :)

Quoted 8 lines

Quoted 5 lines
Where:
110 is the start bit and READ OpCode [A5-A0] is the address to read from 0 is a "dummy bit" preceding the actual data [D15-D0] is the actual data.
Also leading spaces, please remove them and use TAB, or use spaces only.

Ugh, copy/paste is hard! I will fix :).

Quoted 10 lines

Quoted 7 lines
Looking at the READ timing diagrams in the 93Cx6 datasheets the dummy bit should be clocked out on the last address bit clock cycle meaning it should be discarded naturally.
However, depending on the hardware configuration sometimes this dummy bit is not discarded. This is the case with Exar PCI UARTs which require an extra clock cycle between sending the address and reading the data.
...

Quoted 7 lines

drivers/misc/eeprom/eeprom_93cx6.c MODULE_VERSION("1.0"); · line 24
Quoted 4 lines
drivers/misc/eeprom/eeprom_93cx6.c +4 -0
+static inline bool has_quirk_extra_read_cycle(struct eeprom_93cx6 *eeprom)
+{
+	return eeprom->quirks & PCI_EEPROM_QUIRK_EXTRA_READ_CYCLE;
+}
So, this makes sense to be in a header since everything else related to that also in the header already.

Makes sense, will do.

Quoted 7 lines

...
Review
Quoted 2 lines
(quoted diff) +2 -0
+	if (has_quirk_extra_read_cycle(eeprom)) {
+		eeprom_93cx6_pulse_high(eeprom);
No additional delay is needed?

Should not need any extra delay as both pulse high/low functions have the worst case 450ns delay after the register write. It was working well on my test cards.

Quoted 13 lines

drivers/misc/eeprom/eeprom_93cx6.c void eeprom_93cx6_read(struct eeprom_93cx6 *eeprom, const u8 word, · line 195
Quoted 5 lines
drivers/misc/eeprom/eeprom_93cx6.c +4 -0
+		eeprom_93cx6_pulse_low(eeprom);
+	}
 
+	if (has_quirk_extra_read_cycle(eeprom)) {
+		eeprom_93cx6_pulse_high(eeprom);
Ditto.
Quoted 2 lines
(quoted diff) +2 -0
+		eeprom_93cx6_pulse_low(eeprom);
+	}
...

Quoted 6 lines

include/linux/eeprom_93cx6.h line 31
Quoted 2 lines
include/linux/eeprom_93cx6.h +2 -0
+/* Some EEPROMs require an extra clock cycle before reading */
+#define PCI_EEPROM_QUIRK_EXTRA_READ_CYCLE	BIT(0)
I would move it directly into the structure definitions, just after quirk field (the same way it's done in the other driver)...

Will do, thanks!

Quoted 10 lines

...
include/linux/eeprom_93cx6.h struct eeprom_93cx6 { · line 59
Quoted 2 lines
include/linux/eeprom_93cx6.h +1 -0
 	int width;
+	unsigned int quirks;
...somewhere here.
Quoted 2 lines
include/linux/eeprom_93cx6.h +0 -0
 	char drive_data;
 	char reg_data_in;

Andy Shevchenko

2024-09-13 18:32 UTC #4 lore raw

Re: [PATCH v1 1/6] misc: eeprom: eeprom_93cx6: Add quirk for extra read clock cycle

Quoted 3 lines · Parker Newman

Quoted 1 line · On Fri, 13 Sep 2024 20:48:03 +0300 Andy Shevchenko…
On Fri, Sep 13, 2024 at 10:55:38AM -0400, Parker Newman wrote:

...

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
Link https://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-5193-SEEPROM-AT93C46D-Datasheet.pdf
Make it a tag (i.e. locate just above your SoB tag)
Sorry, not 100% sure what you mean by tag? Do I just need to move the Link: entry to be above my Sign-off? Or is there something else? Thanks!

Make it like

...Summary...
<blank line>
...commit message text...
<blank line>
Link: ...
Signed-off-by: ...

...

Quoted 7 lines

Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+	if (has_quirk_extra_read_cycle(eeprom)) {
+		eeprom_93cx6_pulse_high(eeprom);
No additional delay is needed?
Should not need any extra delay as both pulse high/low functions have the worst case 450ns delay after the register write. It was working well on my test cards.

OK!

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
(quoted diff) +2 -0
+		eeprom_93cx6_pulse_low(eeprom);
+	}

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Parker Newman

author

2024-09-13 14:56 UTC #5 lore raw

PATCH 2/6 misc: eeprom: eeprom_93cx6: Switch to BIT() macro

In reply to Parker Newman

From: Parker Newman <[email protected]>

Use the BIT() macro rather than (1 << (i - 1))

Signed-off-by Parker Newman <[email protected]>

---
 drivers/misc/eeprom/eeprom_93cx6.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/misc/eeprom/eeprom_93cx6.c +3 -2

index 0f52d3c4bc1d..64da22edefa4 100644
@@ -8,6 +8,7 @@
  * Supported chipsets: 93c46 & 93c66.
  */
 
+#include <linux/bits.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/delay.h>

@@ -107,7 +108,7 @@ static void eeprom_93cx6_write_bits(struct eeprom_93cx6 *eeprom,
 		/*
 		 * Check if this bit needs to be set.
 		 */
-		eeprom->reg_data_in = !!(data & (1 << (i - 1)));
+		eeprom->reg_data_in = !!(data & BIT(i - 1));
 
 		/*
 		 * Write the bit to the eeprom register.

@@ -157,7 +158,7 @@ static void eeprom_93cx6_read_bits(struct eeprom_93cx6 *eeprom,
 		 * Read if the bit has been set.
 		 */
 		if (eeprom->reg_data_out)
-			buf |= (1 << (i - 1));
+			buf |= BIT(i - 1);
 
 		eeprom_93cx6_pulse_low(eeprom);
 	}

--
2.46.0

Andy Shevchenko

2024-09-13 17:48 UTC #6 lore raw

Re: [PATCH v1 2/6] misc: eeprom: eeprom_93cx6: Switch to BIT() macro

Quoted 3 lines · Parker Newman

From: Parker Newman <[email protected]>
Use the BIT() macro rather than (1 << (i - 1))

Missed period at the end.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Parker Newman

author

2024-09-13 15:01 UTC #7 lore raw

PATCH 3/6 misc: eeprom: eeprom_93cx6: Replace printk(KERN_ERR ...) with pr_err()

In reply to Parker Newman

From: Parker Newman <[email protected]>

Replace printk(KERN_ERR ...) with pr_err() to improve readability.

Fixes checkpatch warning:

WARNING: Prefer [subsystem eg: netdev]_err([subsystem]dev, ... then dev_err(dev, ... then pr_err(... to printk(KERN_ERR ...
+ printk(KERN_ERR "%s: timeout\n", __func__);

Signed-off-by Parker Newman <[email protected]>

---
 drivers/misc/eeprom/eeprom_93cx6.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/misc/eeprom/eeprom_93cx6.c +1 -1

index 64da22edefa4..755be9a4ffd4 100644
@@ -378,7 +378,7 @@ void eeprom_93cx6_write(struct eeprom_93cx6 *eeprom, u8 addr, u16 data)
 		usleep_range(1000, 2000);
 
 		if (--timeout <= 0) {
-			printk(KERN_ERR "%s: timeout\n", __func__);
+			pr_err("%s: timeout\n", __func__);
 			break;
 		}
 	}

--
2.46.0

Andy Shevchenko

2024-09-13 17:54 UTC #8 lore raw

Re: [PATCH v1 3/6] misc: eeprom: eeprom_93cx6: Replace printk(KERN_ERR ...) with pr_err()

Review

Quoted 9 lines · Parker Newman

From: Parker Newman <[email protected]>
Replace printk(KERN_ERR ...) with pr_err() to improve readability.
Fixes checkpatch warning:
WARNING: Prefer [subsystem eg: netdev]_err([subsystem]dev, ... then dev_err(dev, ... then pr_err(... to printk(KERN_ERR ...
(quoted diff) +1 -0
+			printk(KERN_ERR "%s: timeout\n", __func__);

First of all, you probably want pr_fmt() to be defined.
Second, I would replace the entire while loop by the read_poll_timeout() macro from iopoll.h (note to use "true" for sleep before check and drop that one from before the loop in the driver).
Naturally the pr_err() change can be combined with read_poll_timeout(), but if you go with pr_fmt() perhaps it still makes sense to have separate changes. I dunno which one is better, up to you.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Parker Newman

author

2024-09-13 19:12 UTC #9 lore raw

Re: [PATCH v1 3/6] misc: eeprom: eeprom_93cx6: Replace printk(KERN_ERR ...) with pr_err()

Quoted 5 lines · On Fri, 13 Sep 2024 20:54:05 +0300 Andy Shevchenko…

Quoted 4 lines · Parker Newman
From: Parker Newman <[email protected]>
Replace printk(KERN_ERR ...) with pr_err() to improve readability.

Quoted 7 lines

Review
Quoted 5 lines
Fixes checkpatch warning:
WARNING: Prefer [subsystem eg: netdev]_err([subsystem]dev, ... then dev_err(dev, ... then pr_err(... to printk(KERN_ERR ...
(quoted diff) +1 -0
+			printk(KERN_ERR "%s: timeout\n", __func__);
First of all, you probably want pr_fmt() to be defined.

Good point, I will add.

Quoted 3 lines

Second, I would replace the entire while loop by the read_poll_timeout() macro from iopoll.h (note to use "true" for sleep before check and drop that one from before the loop in the driver).

Good idea.

Quoted 4 lines

Naturally the pr_err() change can be combined with read_poll_timeout(), but if you go with pr_fmt() perhaps it still makes sense to have separate changes. I dunno which one is better, up to you.

Sorry if I am miss-reading but do you mean the pr_err() and pr_fmt() can be combined and the read_poll_timeout() change should be made in a separate patch after?

Or should I be adding the pr_fmt() define in its own patch, followed by the pr_err() and read_poll_timeout() in a patch?

Thanks,
Parker

Andy Shevchenko

2024-09-13 19:26 UTC #10 lore raw

Re: [PATCH v1 3/6] misc: eeprom: eeprom_93cx6: Replace printk(KERN_ERR ...) with pr_err()

Quoted 3 lines · Parker Newman

Quoted 1 line · On Fri, 13 Sep 2024 20:54:05 +0300 Andy Shevchenko…
On Fri, Sep 13, 2024 at 10:55:40AM -0400, Parker Newman wrote:

...

Quoted 5 lines

Quoted 2 lines
if you go with pr_fmt() perhaps it still makes sense to have separate changes. I dunno which one is better, up to you.
Sorry if I am miss-reading but do you mean the pr_err() and pr_fmt() can be combined and the read_poll_timeout() change should be made in a separate patch after?

Possibly, I dunno.

Quoted 2 lines

Or should I be adding the pr_fmt() define in its own patch, followed by the pr_err() and read_poll_timeout() in a patch?

No, either altogether, or one patch for pr_err() + pr_fmt() and one for read_poll_timeout().

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Greg Kroah-Hartman

maintainer

2024-09-14 18:58 UTC #11 lore raw

Re: [PATCH v1 3/6] misc: eeprom: eeprom_93cx6: Replace printk(KERN_ERR ...) with pr_err()

In reply to Parker Newman

Quoted 20 lines · Parker Newman

From: Parker Newman <[email protected]>
Replace printk(KERN_ERR ...) with pr_err() to improve readability.
Fixes checkpatch warning:
WARNING: Prefer [subsystem eg: netdev]_err([subsystem]dev, ... then dev_err(dev, ... then pr_err(... to printk(KERN_ERR ...
(quoted diff) +1 -0
+			printk(KERN_ERR "%s: timeout\n", __func__);
Signed-off-by Parker Newman <[email protected]>
---
 drivers/misc/eeprom/eeprom_93cx6.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/misc/eeprom/eeprom_93cx6.c +0 -0
index 64da22edefa4..755be9a4ffd4 100644

drivers/misc/eeprom/eeprom_93cx6.c void eeprom_93cx6_write(struct eeprom_93cx6 *eeprom, u8 addr, u16 data) · line 381

Quoted 5 lines

drivers/misc/eeprom/eeprom_93cx6.c +1 -1

@@ -378,7 +378,7 @@ void eeprom_93cx6_write(struct eeprom_93cx6 *eeprom, u8 addr, u16 data)
 		usleep_range(1000, 2000);
 
 		if (--timeout <= 0) {
-			printk(KERN_ERR "%s: timeout\n", __func__);
+			pr_err("%s: timeout\n", __func__);

It's a device, please use dev_err().

thanks,

greg k-h

Andy Shevchenko

2024-09-16 09:55 UTC #12 lore raw

Re: [PATCH v1 3/6] misc: eeprom: eeprom_93cx6: Replace printk(KERN_ERR ...) with pr_err()

Quoted 1 line · Greg Kroah-Hartman

On Fri, Sep 13, 2024 at 10:55:40AM -0400, Parker Newman wrote:

...

Quoted 4 lines

drivers/misc/eeprom/eeprom_93cx6.c void eeprom_93cx6_write(struct eeprom_93cx6 *eeprom, u8 addr, u16 data) · line 381
Quoted 2 lines
drivers/misc/eeprom/eeprom_93cx6.c +1 -1
-			printk(KERN_ERR "%s: timeout\n", __func__);
+			pr_err("%s: timeout\n", __func__);
It's a device, please use dev_err().

The problem is that this library doesn't know about this fact. I.e. it would need a new member just for this message. Instead, maybe drop the message as we anyway get a unique enough error code?

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Greg Kroah-Hartman

maintainer

2024-09-16 10:26 UTC #13 lore raw

Re: [PATCH v1 3/6] misc: eeprom: eeprom_93cx6: Replace printk(KERN_ERR ...) with pr_err()

Quoted 5 lines · Andy Shevchenko

Quoted 1 line · Greg Kroah-Hartman
On Fri, Sep 13, 2024 at 10:55:40AM -0400, Parker Newman wrote:
...

Quoted 8 lines

Quoted 4 lines
drivers/misc/eeprom/eeprom_93cx6.c void eeprom_93cx6_write(struct eeprom_93cx6 *eeprom, u8 addr, u16 data) · line 381
Quoted 2 lines
drivers/misc/eeprom/eeprom_93cx6.c +1 -1
-			printk(KERN_ERR "%s: timeout\n", __func__);
+			pr_err("%s: timeout\n", __func__);
It's a device, please use dev_err().
The problem is that this library doesn't know about this fact. I.e. it would need a new member just for this message. Instead, maybe drop the message as we anyway get a unique enough error code?

Fair enough, although adding real device pointers would be good to do in the future...

thanks,

greg k-h

Andy Shevchenko

2024-09-16 10:32 UTC #14 lore raw

Re: [PATCH v1 3/6] misc: eeprom: eeprom_93cx6: Replace printk(KERN_ERR ...) with pr_err()

Quoted 3 lines · Greg Kroah-Hartman

Quoted 2 lines · Andy Shevchenko
Quoted 1 line · Greg Kroah-Hartman
On Fri, Sep 13, 2024 at 10:55:40AM -0400, Parker Newman wrote:

...

Quoted 11 lines

Quoted 8 lines
Quoted 4 lines
drivers/misc/eeprom/eeprom_93cx6.c void eeprom_93cx6_write(struct eeprom_93cx6 *eeprom, u8 addr, u16 data) · line 381
Quoted 2 lines
drivers/misc/eeprom/eeprom_93cx6.c +1 -1
-			printk(KERN_ERR "%s: timeout\n", __func__);
+			pr_err("%s: timeout\n", __func__);
It's a device, please use dev_err().
The problem is that this library doesn't know about this fact. I.e. it would need a new member just for this message. Instead, maybe drop the message as we anyway get a unique enough error code?
Fair enough, although adding real device pointers would be good to do in the future...

Let's then do it when it will be the real need? Because I don't think this message is _so_ important. I believe one of the upper layers (whichever calls this function) should propagate the error code up to the user space. If it's not the case _that_ has to be fixed.

TL;DR: Let's remove the message for now.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Parker Newman

author

2024-09-16 12:04 UTC #15 lore raw

Re: [PATCH v1 3/6] misc: eeprom: eeprom_93cx6: Replace printk(KERN_ERR ...) with pr_err()

Quoted 19 lines · On Mon, 16 Sep 2024 13:32:47 +0300 Andy Shevchenko…

Quoted 3 lines · Greg Kroah-Hartman
Quoted 2 lines · Andy Shevchenko
Quoted 1 line · Greg Kroah-Hartman
On Fri, Sep 13, 2024 at 10:55:40AM -0400, Parker Newman wrote:
...
Quoted 11 lines
Quoted 8 lines
Quoted 4 lines
drivers/misc/eeprom/eeprom_93cx6.c void eeprom_93cx6_write(struct eeprom_93cx6 *eeprom, u8 addr, u16 data) · line 381
Quoted 2 lines
drivers/misc/eeprom/eeprom_93cx6.c +1 -1
-			printk(KERN_ERR "%s: timeout\n", __func__);
+			pr_err("%s: timeout\n", __func__);
It's a device, please use dev_err().
The problem is that this library doesn't know about this fact. I.e. it would need a new member just for this message. Instead, maybe drop the message as we anyway get a unique enough error code?
Fair enough, although adding real device pointers would be good to do in the future...

Quoted 7 lines

Let's then do it when it will be the real need? Because I don't think this message is _so_ important. I believe one of the upper layers (whichever calls this function) should propagate the error code up to the user space. If it's not the case _that_ has to be fixed.
TL;DR: Let's remove the message for now.

I can remove the message or leave it as is and drop this patch from the series. One could make the argument that any error indication it is better than none in this case.

Andy Shevchenko

2024-09-16 12:18 UTC #16 lore raw

Re: [PATCH v1 3/6] misc: eeprom: eeprom_93cx6: Replace printk(KERN_ERR ...) with pr_err()

Quoted 6 lines · Parker Newman

Quoted 4 lines · On Mon, 16 Sep 2024 13:32:47 +0300 Andy Shevchenko…
Quoted 3 lines · Greg Kroah-Hartman
Quoted 2 lines · Andy Shevchenko
Quoted 1 line · Greg Kroah-Hartman
On Fri, Sep 13, 2024 at 10:55:40AM -0400, Parker Newman wrote:

...

Quoted 17 lines

Quoted 17 lines
Quoted 11 lines
Quoted 8 lines
Quoted 4 lines
drivers/misc/eeprom/eeprom_93cx6.c void eeprom_93cx6_write(struct eeprom_93cx6 *eeprom, u8 addr, u16 data) · line 381
Quoted 2 lines
drivers/misc/eeprom/eeprom_93cx6.c +1 -1
-			printk(KERN_ERR "%s: timeout\n", __func__);
+			pr_err("%s: timeout\n", __func__);
It's a device, please use dev_err().
The problem is that this library doesn't know about this fact. I.e. it would need a new member just for this message. Instead, maybe drop the message as we anyway get a unique enough error code?
Fair enough, although adding real device pointers would be good to do in the future...
Let's then do it when it will be the real need? Because I don't think this message is _so_ important. I believe one of the upper layers (whichever calls this function) should propagate the error code up to the user space. If it's not the case _that_ has to be fixed.

Quoted 5 lines

Quoted 1 line
TL;DR: Let's remove the message for now.
I can remove the message or leave it as is and drop this patch from the series. One could make the argument that any error indication it is better than none in this case.

I think you can drop the message and make the patch to be last in the series, so it can be easily abandoned (in case that decision will be made) without throttling the rest. At the same time in the commit message explain that with move to read_poll_timeout() we drop the seems redundant message. I'm fine with that approach. But at the end of the day it's not that critical to the main purpose, i.e. cleaning up the Exar serial driver.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Parker Newman

author

2024-09-16 15:21 UTC #17 lore raw

Re: [PATCH v1 3/6] misc: eeprom: eeprom_93cx6: Replace printk(KERN_ERR ...) with pr_err()

Quoted 33 lines · On Mon, 16 Sep 2024 15:18:15 +0300 Andy Shevchenko…

Quoted 6 lines · Parker Newman
Quoted 4 lines · On Mon, 16 Sep 2024 13:32:47 +0300 Andy Shevchenko…
Quoted 3 lines · Greg Kroah-Hartman
Quoted 2 lines · Andy Shevchenko
Quoted 1 line · Greg Kroah-Hartman
On Fri, Sep 13, 2024 at 10:55:40AM -0400, Parker Newman wrote:
...
Quoted 17 lines
Quoted 17 lines
Quoted 11 lines
Quoted 8 lines
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +1 -1
-			printk(KERN_ERR "%s: timeout\n", __func__);
+			pr_err("%s: timeout\n", __func__);
It's a device, please use dev_err().
The problem is that this library doesn't know about this fact. I.e. it would need a new member just for this message. Instead, maybe drop the message as we anyway get a unique enough error code?
Fair enough, although adding real device pointers would be good to do in the future...
Let's then do it when it will be the real need? Because I don't think this message is _so_ important. I believe one of the upper layers (whichever calls this function) should propagate the error code up to the user space. If it's not the case _that_ has to be fixed.
Quoted 5 lines
Quoted 1 line
TL;DR: Let's remove the message for now.
I can remove the message or leave it as is and drop this patch from the series. One could make the argument that any error indication it is better than none in this case.

Quoted 7 lines

I think you can drop the message and make the patch to be last in the series, so it can be easily abandoned (in case that decision will be made) without throttling the rest. At the same time in the commit message explain that with move to read_poll_timeout() we drop the seems redundant message. I'm fine with that approach. But at the end of the day it's not that critical to the main purpose, i.e. cleaning up the Exar serial driver.

I don't think read_poll_timeout() will work directly because eeprom->register_read() does not return a value. I could add a "is write complete" wrapper function to work around that I guess. However, I think I will just drop this patch from the series as fixing it properly will be a big change and like you said its not critical to the main patch.

Andy Shevchenko

2024-09-16 15:27 UTC #18 lore raw

Re: [PATCH v1 3/6] misc: eeprom: eeprom_93cx6: Replace printk(KERN_ERR ...) with pr_err()

Quoted 9 lines · Parker Newman

Quoted 7 lines · On Mon, 16 Sep 2024 15:18:15 +0300 Andy Shevchenko…
Quoted 6 lines · Parker Newman
Quoted 4 lines · On Mon, 16 Sep 2024 13:32:47 +0300 Andy Shevchenko…
Quoted 3 lines · Greg Kroah-Hartman
Quoted 2 lines · Andy Shevchenko
Quoted 1 line · Greg Kroah-Hartman
On Fri, Sep 13, 2024 at 10:55:40AM -0400, Parker Newman wrote:

...

Quoted 30 lines

Quoted 23 lines
Quoted 17 lines
Quoted 17 lines
Quoted 11 lines
Quoted 8 lines
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +1 -1
-			printk(KERN_ERR "%s: timeout\n", __func__);
+			pr_err("%s: timeout\n", __func__);
It's a device, please use dev_err().
The problem is that this library doesn't know about this fact. I.e. it would need a new member just for this message. Instead, maybe drop the message as we anyway get a unique enough error code?
Fair enough, although adding real device pointers would be good to do in the future...
Let's then do it when it will be the real need? Because I don't think this message is _so_ important. I believe one of the upper layers (whichever calls this function) should propagate the error code up to the user space. If it's not the case _that_ has to be fixed.
Quoted 5 lines
Quoted 1 line
TL;DR: Let's remove the message for now.
I can remove the message or leave it as is and drop this patch from the series. One could make the argument that any error indication it is better than none in this case.
Quoted 6 lines
I think you can drop the message and make the patch to be last in the series, so it can be easily abandoned (in case that decision will be made) without throttling the rest. At the same time in the commit message explain that with move to read_poll_timeout() we drop the seems redundant message. I'm fine with that approach. But at the end of the day it's not that critical to the main purpose, i.e. cleaning up the Exar serial driver.

Quoted 5 lines

I don't think read_poll_timeout() will work directly because eeprom->register_read() does not return a value. I could add a "is write complete" wrapper function to work around that I guess. However, I think I will just drop this patch from the series as fixing it properly will be a big change and like you said its not critical to the main patch.

Sure, fine with me.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Parker Newman

author

2024-09-13 14:56 UTC #19 lore raw

PATCH 4/6 serial: 8250_exar: Replace custom EEPROM read with eeprom_93cx6

In reply to Parker Newman

From: Parker Newman <[email protected]>

Replace the custom 93cx6 EEPROM read functions with the eeprom_93cx6 driver. This removes duplicate code and improves code readability.

exar_ee_read() calls are replaced with eeprom_93cx6_read() or eeprom_93cx6_multiread().

Link to discussion with Andy Shevchenko:

Link https://lore.kernel.org/linux-serial/[email protected]/ open here

Note: Old exar_ee_read() and associated functions are removed in next patch in this series.

Signed-off-by Parker Newman <[email protected]>

---
 drivers/tty/serial/8250/8250_exar.c | 54 +++++++++++++++++++++++------
 1 file changed, 43 insertions(+), 11 deletions(-)

drivers/tty/serial/8250/8250_exar.c +43 ~~-11~~

index b7a75db15249..0edb5aeba199 100644
@@ -21,6 +21,7 @@
 #include <linux/property.h>
 #include <linux/string.h>
 #include <linux/types.h>
+#include <linux/eeprom_93cx6.h>
 
 #include <linux/serial_8250.h>
 #include <linux/serial_core.h>

@@ -252,6 +253,7 @@ struct exar8250 {
 	unsigned int		nr;
 	unsigned int		osc_freq;
 	struct exar8250_board	*board;
+	struct eeprom_93cx6	eeprom;
 	void __iomem		*virt;
 	int			line[];
 };

@@ -355,6 +357,39 @@ static u16 exar_ee_read(struct exar8250 *priv, u8 ee_addr)
 	return data;
 }
 
+static void exar_eeprom_93cx6_reg_read(struct eeprom_93cx6 *eeprom)
+{
+	struct exar8250 *priv = (struct exar8250 *)eeprom->data;
+	u8 regb = exar_read_reg(priv, UART_EXAR_REGB);
+
+	// EECK and EECS always read 0 from REGB so only set EEDO
+	eeprom->reg_data_out = regb & UART_EXAR_REGB_EEDO;
+}
+
+static void exar_eeprom_93cx6_reg_write(struct eeprom_93cx6 *eeprom)
+{
+	struct exar8250 *priv = (struct exar8250 *)eeprom->data;
+	u8 regb = 0;
+
+	if (eeprom->reg_data_in)
+		regb |= UART_EXAR_REGB_EEDI;
+	if (eeprom->reg_data_clock)
+		regb |= UART_EXAR_REGB_EECK;
+	if (eeprom->reg_chip_select)
+		regb |= UART_EXAR_REGB_EECS;
+
+	exar_write_reg(priv, UART_EXAR_REGB, regb);
+}
+
+static void exar_eeprom_init(struct exar8250 *priv)
+{
+	priv->eeprom.data = (void *)priv;
+	priv->eeprom.register_read = exar_eeprom_93cx6_reg_read;
+	priv->eeprom.register_write = exar_eeprom_93cx6_reg_write;
+	priv->eeprom.width = PCI_EEPROM_WIDTH_93C46;
+	priv->eeprom.quirks |= PCI_EEPROM_QUIRK_EXTRA_READ_CYCLE;
+}
+
 /**
  * exar_mpio_config_output() - Configure an Exar MPIO as an output
  * @priv: Device's private structure

@@ -696,20 +731,15 @@ static int cti_plx_int_enable(struct exar8250 *priv)
  */
 static int cti_read_osc_freq(struct exar8250 *priv, u8 eeprom_offset)
 {
-	u16 lower_word;
-	u16 upper_word;
+	__le32 osc_freq_le;
 
-	lower_word = exar_ee_read(priv, eeprom_offset);
-	// Check if EEPROM word was blank
-	if (lower_word == 0xFFFF)
-		return -EIO;
+	eeprom_93cx6_multiread(&priv->eeprom, eeprom_offset,
+				(__le16 *)&osc_freq_le, 2);
 
-	upper_word = exar_ee_read(priv, (eeprom_offset + 1));
-	if (upper_word == 0xFFFF)
+	if (osc_freq_le == 0xFFFFFFFF)
 		return -EIO;
 
-	return FIELD_PREP(CTI_EE_MASK_OSC_FREQ_LOWER, lower_word) |
-	       FIELD_PREP(CTI_EE_MASK_OSC_FREQ_UPPER, upper_word);
+	return le32_to_cpu(osc_freq_le);
 }
 
 /**

@@ -833,7 +863,7 @@ static enum cti_port_type cti_get_port_type_xr17v35x(struct exar8250 *priv,
 	u8 offset;
 
 	offset = CTI_EE_OFF_XR17V35X_PORT_FLAGS + port_num;
-	port_flags = exar_ee_read(priv, offset);
+	eeprom_93cx6_read(&priv->eeprom, offset, &port_flags);
 
 	port_type = FIELD_GET(CTI_EE_MASK_PORT_FLAGS_TYPE, port_flags);
 	if (CTI_PORT_TYPE_VALID(port_type))

@@ -1551,6 +1581,8 @@ exar_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *ent)
 	if (rc)
 		return rc;
 
+	exar_eeprom_init(priv);
+
 	for (i = 0; i < nr_ports && i < maxnr; i++) {
 		rc = board->setup(priv, pcidev, &uart, i);
 		if (rc) {

--
2.46.0

Andy Shevchenko

2024-09-13 18:06 UTC #20 lore raw

Re: [PATCH v1 4/6] serial: 8250_exar: Replace custom EEPROM read with eeprom_93cx6

Quoted 9 lines · Parker Newman

From: Parker Newman <[email protected]>
Replace the custom 93cx6 EEPROM read functions with the eeprom_93cx6 driver. This removes duplicate code and improves code readability.
exar_ee_read() calls are replaced with eeprom_93cx6_read() or eeprom_93cx6_multiread().
Link to discussion with Andy Shevchenko:

(the above might need to be rephrased a bit, see below why)

Quoted 1 line

Link https://lore.kernel.org/linux-serial/[email protected]/ open here

Make it real tag by moving...

Quoted 3 lines

Note: Old exar_ee_read() and associated functions are removed in next patch in this series.

...somewhere here.

Quoted 1 line

Signed-off-by Parker Newman <[email protected]>

...

Quoted 4 lines

#include <linux/property.h>
#include <linux/string.h>
#include <linux/types.h>
+#include <linux/eeprom_93cx6.h>

Keep it sorted.

...

drivers/tty/serial/8250/8250_exar.c static u16 exar_ee_read(struct exar8250 *priv, u8 ee_addr) · line 362

Quoted 3 lines

drivers/tty/serial/8250/8250_exar.c +3 -0

+static void exar_eeprom_93cx6_reg_read(struct eeprom_93cx6 *eeprom)
+{
+	struct exar8250 *priv = (struct exar8250 *)eeprom->data;

Unneeded explicit cast.

drivers/tty/serial/8250/8250_exar.c static u16 exar_ee_read(struct exar8250 *priv, u8 ee_addr) · line 365

Quoted 3 lines

drivers/tty/serial/8250/8250_exar.c +3 -0

+	u8 regb = exar_read_reg(priv, UART_EXAR_REGB);
+
+	// EECK and EECS always read 0 from REGB so only set EEDO

Please, use C comment style as everywhere else in the file.

Quoted 2 lines

(quoted diff) +2 -0

+	eeprom->reg_data_out = regb & UART_EXAR_REGB_EEDO;
+}

...

drivers/tty/serial/8250/8250_exar.c static u16 exar_ee_read(struct exar8250 *priv, u8 ee_addr) · line 371

Quoted 3 lines

drivers/tty/serial/8250/8250_exar.c +3 -0

+static void exar_eeprom_93cx6_reg_write(struct eeprom_93cx6 *eeprom)
+{
+	struct exar8250 *priv = (struct exar8250 *)eeprom->data;

Unneeded cast from void *.

Quoted 11 lines

drivers/tty/serial/8250/8250_exar.c +11 -0

+	u8 regb = 0;
+
+	if (eeprom->reg_data_in)
+		regb |= UART_EXAR_REGB_EEDI;
+	if (eeprom->reg_data_clock)
+		regb |= UART_EXAR_REGB_EECK;
+	if (eeprom->reg_chip_select)
+		regb |= UART_EXAR_REGB_EECS;
+
+	exar_write_reg(priv, UART_EXAR_REGB, regb);
+}

...

Quoted 1 line

+ priv->eeprom.data = (void *)priv;

Unneeded cast.

...

drivers/tty/serial/8250/8250_exar.c static int cti_plx_int_enable(struct exar8250 *priv) · line 737

Quoted 2 lines

drivers/tty/serial/8250/8250_exar.c +2 -0

+	eeprom_93cx6_multiread(&priv->eeprom, eeprom_offset,
+				(__le16 *)&osc_freq_le, 2);

Okay, this should be done better I believe:

	/* ...Find better names for variables... */
	__le16 f[2];
	u32 osc_freq;

	eeprom_93cx6_multiread(&priv->eeprom, eeprom_offset, &freq, ARRAY_SIZE(freq));

	osc_freq = le16_to_cpu(f[0]) | (le16_to_cpu(f[1]) << 16);
	if (osc_freq == GENMASK(31, 0))
		...

	return osc_freq;

(Also no need to break on 80 characters)

Quoted 4 lines

drivers/tty/serial/8250/8250_exar.c +2 -0

+	if (osc_freq_le == 0xFFFFFFFF)
 		return -EIO;
 
+	return le32_to_cpu(osc_freq_le);

Signature · 2 lines

With Best Regards,
Andy Shevchenko

kernel test robot

build bot

2024-09-14 13:27 UTC #21 lore raw

Re: [PATCH v1 4/6] serial: 8250_exar: Replace custom EEPROM read with eeprom_93cx6

In reply to Parker Newman

kernel test robot noticed the following build warnings:

Hi Parker,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 5ed771f174726ae879945d4f148a9005ac909cb7]

url:    https://github.com/intel-lab-lkp/linux/commits/Parker-Newman/misc-eeprom-eeprom_93cx6-Add-quirk-for-extra-read-clock-cycle/20240913-230345
base:   5ed771f174726ae879945d4f148a9005ac909cb7
patch link:    https://lore.kernel.org/r/78dead78311ea619e0be99cc32ee0df1610a480d.1726237379.git.pnewman%40connecttech.com
patch subject: [PATCH v1 4/6] serial: 8250_exar: Replace custom EEPROM read with eeprom_93cx6
config: x86_64-randconfig-122-20240914 (https://download.01.org/0day-ci/archive/20240914/[email protected]/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240914/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

sparse warnings: (new ones prefixed by >>)

>> drivers/tty/serial/8250/8250_exar.c:739:13: sparse: sparse: restricted __le32 degrades to integer

vim +739 drivers/tty/serial/8250/8250_exar.c

721
722 /**
723 * cti_read_osc_freq() - Read the UART oscillator frequency from EEPROM
724 * @priv: Device's private structure
725 * @eeprom_offset: Offset where the oscillator frequency is stored
726 *
727 * CTI XR17x15X and XR17V25X cards have the serial boards oscillator frequency
728 * stored in the EEPROM. FPGA and XR17V35X based cards use the PCI/PCIe clock.
729 *
730 * Return: frequency on success, negative error code on failure
731 */
732 static int cti_read_osc_freq(struct exar8250 *priv, u8 eeprom_offset)
733 {
734 __le32 osc_freq_le;
735
736 eeprom_93cx6_multiread(&priv->eeprom, eeprom_offset,
737 (__le16 *)&osc_freq_le, 2);
738

 > 739		if (osc_freq_le == 0xFFFFFFFF)

740 return -EIO;
741
742 return le32_to_cpu(osc_freq_le);
743 }
744

Signature · 2 lines

0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Andy Shevchenko

2024-09-16 10:09 UTC #22 lore raw

Re: [PATCH v1 4/6] serial: 8250_exar: Replace custom EEPROM read with eeprom_93cx6

Quoted 14 lines · kernel test robot

Hi Parker,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 5ed771f174726ae879945d4f148a9005ac909cb7]

url:    https://github.com/intel-lab-lkp/linux/commits/Parker-Newman/misc-eeprom-eeprom_93cx6-Add-quirk-for-extra-read-clock-cycle/20240913-230345
base:   5ed771f174726ae879945d4f148a9005ac909cb7
patch link:    https://lore.kernel.org/r/78dead78311ea619e0be99cc32ee0df1610a480d.1726237379.git.pnewman%40connecttech.com
patch subject: [PATCH v1 4/6] serial: 8250_exar: Replace custom EEPROM read with eeprom_93cx6
config: x86_64-randconfig-122-20240914 (https://download.01.org/0day-ci/archive/20240914/[email protected]/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240914/[email protected]/reproduce)

Quoted 7 lines

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
sparse warnings: (new ones prefixed by >>)
Quoted 1 line
Quoted 1 line
drivers/tty/serial/8250/8250_exar.c:739:13: sparse: sparse: restricted __le32 degrades to integer

Yes, and this should gone if Parker follows my suggestion on how to handle this.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Parker Newman

author

2024-09-13 14:56 UTC #23 lore raw

PATCH 5/6 serial: 8250_exar: Remove old exar_ee_read() and other unneeded code

In reply to Parker Newman

From: Parker Newman <[email protected]>

Remove the old exar_ee_read() and associated helper functions as well as some defines that are no longer needed after the switch to using the eeprom_93cx6 driver.

Signed-off-by Parker Newman <[email protected]>

---
 drivers/tty/serial/8250/8250_exar.c | 92 -----------------------------
 1 file changed, 92 deletions(-)

drivers/tty/serial/8250/8250_exar.c +0 ~~-92~~

index 0edb5aeba199..3ef28429f1d1 100644
@@ -136,8 +136,6 @@
 #define UART_EXAR_REGB_EECS		BIT(5)
 #define UART_EXAR_REGB_EEDI		BIT(6)
 #define UART_EXAR_REGB_EEDO		BIT(7)
-#define UART_EXAR_REGB_EE_ADDR_SIZE	6
-#define UART_EXAR_REGB_EE_DATA_SIZE	16
 
 #define UART_EXAR_XR17C15X_PORT_OFFSET	0x200
 #define UART_EXAR_XR17V25X_PORT_OFFSET	0x200

@@ -190,8 +188,6 @@
 #define CTI_EE_OFF_XR17V35X_PORT_FLAGS	0x14  /* 1 word */
 
 #define CTI_EE_MASK_PORT_FLAGS_TYPE	GENMASK(7, 0)
-#define CTI_EE_MASK_OSC_FREQ_LOWER	GENMASK(15, 0)
-#define CTI_EE_MASK_OSC_FREQ_UPPER	GENMASK(31, 16)
 
 #define CTI_FPGA_RS485_IO_REG		0x2008
 #define CTI_FPGA_CFG_INT_EN_REG		0x48

@@ -269,94 +265,6 @@ static inline u8 exar_read_reg(struct exar8250 *priv, unsigned int reg)
 	return readb(priv->virt + reg);
 }
 
-static inline void exar_ee_select(struct exar8250 *priv)
-{
-	// Set chip select pin high to enable EEPROM reads/writes
-	exar_write_reg(priv, UART_EXAR_REGB, UART_EXAR_REGB_EECS);
-	// Min ~500ns delay needed between CS assert and EEPROM access
-	udelay(1);
-}
-
-static inline void exar_ee_deselect(struct exar8250 *priv)
-{
-	exar_write_reg(priv, UART_EXAR_REGB, 0x00);
-}
-
-static inline void exar_ee_write_bit(struct exar8250 *priv, u8 bit)
-{
-	u8 value = UART_EXAR_REGB_EECS;
-
-	if (bit)
-		value |= UART_EXAR_REGB_EEDI;
-
-	// Clock out the bit on the EEPROM interface
-	exar_write_reg(priv, UART_EXAR_REGB, value);
-	// 2us delay = ~500khz clock speed
-	udelay(2);
-
-	value |= UART_EXAR_REGB_EECK;
-
-	exar_write_reg(priv, UART_EXAR_REGB, value);
-	udelay(2);
-}
-
-static inline u8 exar_ee_read_bit(struct exar8250 *priv)
-{
-	u8 regb;
-	u8 value = UART_EXAR_REGB_EECS;
-
-	// Clock in the bit on the EEPROM interface
-	exar_write_reg(priv, UART_EXAR_REGB, value);
-	// 2us delay = ~500khz clock speed
-	udelay(2);
-
-	value |= UART_EXAR_REGB_EECK;
-
-	exar_write_reg(priv, UART_EXAR_REGB, value);
-	udelay(2);
-
-	regb = exar_read_reg(priv, UART_EXAR_REGB);
-
-	return (regb & UART_EXAR_REGB_EEDO ? 1 : 0);
-}
-
-/**
- * exar_ee_read() - Read a word from the EEPROM
- * @priv: Device's private structure
- * @ee_addr: Offset of EEPROM to read word from
- *
- * Read a single 16bit word from an Exar UART's EEPROM.
- * The type of the EEPROM is AT93C46D.
- *
- * Return: EEPROM word
- */
-static u16 exar_ee_read(struct exar8250 *priv, u8 ee_addr)
-{
-	int i;
-	u16 data = 0;
-
-	exar_ee_select(priv);
-
-	// Send read command (opcode 110)
-	exar_ee_write_bit(priv, 1);
-	exar_ee_write_bit(priv, 1);
-	exar_ee_write_bit(priv, 0);
-
-	// Send address to read from
-	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 starting with a dummy bit
-	for (i = UART_EXAR_REGB_EE_DATA_SIZE; i >= 0; i--) {
-		if (exar_ee_read_bit(priv))
-			data |= BIT(i);
-	}
-
-	exar_ee_deselect(priv);
-
-	return data;
-}
-
 static void exar_eeprom_93cx6_reg_read(struct eeprom_93cx6 *eeprom)
 {
 	struct exar8250 *priv = (struct exar8250 *)eeprom->data;

--
2.46.0

Andy Shevchenko

Reviewed-by on v1 5/6

2024-09-13 18:07 UTC #24 lore raw

Re: [PATCH v1 5/6] serial: 8250_exar: Remove old exar_ee_read() and other unneeded code

Quoted 5 lines · Parker Newman

From: Parker Newman <[email protected]>
Remove the old exar_ee_read() and associated helper functions as well as some defines that are no longer needed after the switch to using the eeprom_93cx6 driver.

Nice!

Reviewed-by Andy Shevchenko <[email protected]>

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Parker Newman

author

2024-09-13 15:02 UTC #25 lore raw

PATCH 6/6 serial: 8250_exar: Add select EEPROM_93CX6 in Kconfig

In reply to Parker Newman

From: Parker Newman <[email protected]>

Add "select EEPROM_93CX6" to config SERIAL_8250_EXAR to ensure eeprom_93cx6 driver is also compiled when 8250_exar driver is selected.

Signed-off-by Parker Newman <[email protected]>

---
 drivers/tty/serial/8250/Kconfig | 1 +
 1 file changed, 1 insertion(+)

drivers/tty/serial/8250/Kconfig +1 -0

index 47ff50763c04..94910ced8238 100644
@@ -150,6 +150,7 @@ config SERIAL_8250_EXAR
 	tristate "8250/16550 Exar/Commtech PCI/PCIe device support"
 	depends on SERIAL_8250 && PCI
 	select SERIAL_8250_PCILIB
+	select EEPROM_93CX6
 	default SERIAL_8250
 	help
 	  This builds support for XR17C1xx, XR17V3xx and some Commtech

--
2.46.0

Andy Shevchenko

2024-09-13 18:08 UTC #26 lore raw

Re: [PATCH v1 6/6] serial: 8250_exar: Add select EEPROM_93CX6 in Kconfig

Quoted 4 lines · Parker Newman

From: Parker Newman <[email protected]>
Add "select EEPROM_93CX6" to config SERIAL_8250_EXAR to ensure eeprom_93cx6 driver is also compiled when 8250_exar driver is selected.

This should be done in the patch 4 when you started using the respective APIs.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

2024-09-13 18:10 UTC #27 lore raw

In reply to Parker Newman

Quoted 6 lines · Parker Newman

From: Parker Newman <[email protected]>
This series of patches replaces the custom 93cx6 EEPROM read functions in the 8250_exar driver with the eeprom_93cx6 driver. This removes duplicate code and improves code readability.

Quoted 7 lines

In order to use the eeprom_93cx6 driver a quirk needed to be added to add an extra clock cycle before reading from the EEPROM. This is similar to the quirk in the eeprom_93xx46 driver.
More details in associated patch and mailing list discussion with Andy Shevchenko about these changes:
Link https://lore.kernel.org/linux-serial/[email protected]/ open here

Thanks for the prompt update!

However we are close to the merge window, I think Greg won't accept this until v6.12-rc1 is out. So, we have a two or three weeks of time.

Meanwhile I have some (small) comments, I just sent in individual replies. Overall, LGTM.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Parker Newman

author

2024-09-13 18:51 UTC #28 lore raw

Quoted 15 lines · On Fri, 13 Sep 2024 21:10:01 +0300 Andy Shevchenko…

Quoted 6 lines · Parker Newman
From: Parker Newman <[email protected]>
This series of patches replaces the custom 93cx6 EEPROM read functions in the 8250_exar driver with the eeprom_93cx6 driver. This removes duplicate code and improves code readability.
Quoted 7 lines
In order to use the eeprom_93cx6 driver a quirk needed to be added to add an extra clock cycle before reading from the EEPROM. This is similar to the quirk in the eeprom_93xx46 driver.
More details in associated patch and mailing list discussion with Andy Shevchenko about these changes:
Link https://lore.kernel.org/linux-serial/[email protected]/ open here

Quoted 8 lines

Thanks for the prompt update!
However we are close to the merge window, I think Greg won't accept this until v6.12-rc1 is out. So, we have a two or three weeks of time.
Meanwhile I have some (small) comments, I just sent in individual replies. Overall, LGTM.

Thanks for the review! I will create a v2 with your feedback some time next week. Parker