[PATCH v2 0/4] serial: 8250_exar: Replace custom EEPROM code with eeprom_93cx6

[PATCH v2 0/4] serial: 8250_exar: Replace custom EEPROM code with eeprom_93cx6

[Parker Newman]

From: Parker Newman <pnewman@connecttech.com>

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/Ztr5u2wEt8VF1IdI@black.fi.intel.com/

Changes in v2:
- Dropped patch 3 "misc: eeprom: eeprom_93cx6: Replace printk(KERN_ERR...) with pr_err()".
- Moved Kconfig change into main patch.
- Moved quirk define into struct eeprom_93cx6.
- Moved quirk check function into eeprom_93cx6.h.
- Refactored cti_read_osc_freq() based on feedback.
- Minor commit message formatting fixes.

Parker Newman (4):
  misc: eeprom: eeprom_93cx6: Add quirk for extra read clock cycle
  misc: eeprom: eeprom_93cx6: Switch to BIT() macro
  serial: 8250_exar: Replace custom EEPROM read with eeprom_93cx6
  serial: 8250_exar: Remove old exar_ee_read() and other unneeded code

 drivers/misc/eeprom/eeprom_93cx6.c  |  15 +++-
 drivers/tty/serial/8250/8250_exar.c | 121 ++++++++--------------------
 drivers/tty/serial/8250/Kconfig     |   1 +
 include/linux/eeprom_93cx6.h        |  12 +++
 4 files changed, 58 insertions(+), 91 deletions(-)


base-commit: 5ed771f174726ae879945d4f148a9005ac909cb7
--
2.46.0

[PATCH v2 1/4] misc: eeprom: eeprom_93cx6: Add quirk for extra read clock cycle

[Parker Newman]

From: Parker Newman <pnewman@connecttech.com>

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 (linked below).
Similar notes are found in other 93xx6 datasheets.

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.

Link: https://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-5193-SEEPROM-AT93C46D-Datasheet.pdf
Signed-off-by: Parker Newman <pnewman@connecttech.com>
---
Changes in v2:
- Moved quirk define into struct eeprom_93cx6.
- Moved has_quirk_extra_read_cycle() from eeprom_93cx6.c into eeprom_93cx6.h.
- Fixed commit message formatting.

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

diff --git a/drivers/misc/eeprom/eeprom_93cx6.c b/drivers/misc/eeprom/eeprom_93cx6.c
index 9627294fe3e9..4c9827fe9217 100644
--- a/drivers/misc/eeprom/eeprom_93cx6.c
+++ b/drivers/misc/eeprom/eeprom_93cx6.c
@@ -186,6 +186,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 +257,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.
 	 */
diff --git a/include/linux/eeprom_93cx6.h b/include/linux/eeprom_93cx6.h
index c860c72a921d..4d141345f4d4 100644
--- a/include/linux/eeprom_93cx6.h
+++ b/include/linux/eeprom_93cx6.h
@@ -11,6 +11,8 @@
 	Supported chipsets: 93c46, 93c56 and 93c66.
  */

+#include <linux/bits.h>
+
 /*
  * EEPROM operation defines.
  */
@@ -34,6 +36,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 quirks
  * @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 +53,9 @@ struct eeprom_93cx6 {
 	void (*register_write)(struct eeprom_93cx6 *eeprom);

 	int width;
+	unsigned int quirks;
+/* Some EEPROMs require an extra clock cycle before reading */
+#define PCI_EEPROM_QUIRK_EXTRA_READ_CYCLE	BIT(0)

 	char drive_data;
 	char reg_data_in;
@@ -71,3 +77,9 @@ extern void eeprom_93cx6_wren(struct eeprom_93cx6 *eeprom, bool enable);

 extern void eeprom_93cx6_write(struct eeprom_93cx6 *eeprom,
 			       u8 addr, u16 data);
+
+static inline bool has_quirk_extra_read_cycle(struct eeprom_93cx6 *eeprom)
+{
+	return eeprom->quirks & PCI_EEPROM_QUIRK_EXTRA_READ_CYCLE;
+}
+
--
2.46.0

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

[Andy Shevchenko]

On Fri, Sep 20, 2024 at 10:03:21AM -0400, Parker Newman wrote:
> From: Parker Newman <pnewman@connecttech.com>
> 
> 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 (linked below).
> Similar notes are found in other 93xx6 datasheets.
> 
> 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.
> 
> Link: https://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-5193-SEEPROM-AT93C46D-Datasheet.pdf

JFYI: You may also convert this to Datasheet: tag (we have a history of it
mostly in IIO subsystem), basically replacing word Link by Datasheet in the
above line.

> Signed-off-by: Parker Newman <pnewman@connecttech.com>

Code wise LGTM now,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

(Do not forget to embed this tag into a new version. With this the `b4` tool
 is quite helpful, so the workflow is to checkout a new branch in your local
 Git tree, like `git checkout -b exar-93xx46 v6.12-rc1` then taking a message
 ID from email of this thread — any should succeed, but cover letter's one
 for sure — run the following `b4 am $<message ID>`. It will print the hints
 what to do next, something like `git am $<patch_title>.mbx`. Then you can
 continue with `git rebase --interactive v6.12-rc1` if the code or other
 stuff in the commit message needs to be updated.)

-- 
With Best Regards,
Andy Shevchenko

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

[Parker Newman]

On Fri, 20 Sep 2024 17:45:15 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Fri, Sep 20, 2024 at 10:03:21AM -0400, Parker Newman wrote:
> > From: Parker Newman <pnewman@connecttech.com>
> > 
> > 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 (linked below).
> > Similar notes are found in other 93xx6 datasheets.
> > 
> > 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.
> > 
> > Link: https://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-5193-SEEPROM-AT93C46D-Datasheet.pdf  
> 
> JFYI: You may also convert this to Datasheet: tag (we have a history of it
> mostly in IIO subsystem), basically replacing word Link by Datasheet in the
> above line.
> 

Sounds good, I will change in v3.

> > Signed-off-by: Parker Newman <pnewman@connecttech.com>  
> 
> Code wise LGTM now,
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> (Do not forget to embed this tag into a new version. With this the `b4` tool
>  is quite helpful, so the workflow is to checkout a new branch in your local
>  Git tree, like `git checkout -b exar-93xx46 v6.12-rc1` then taking a message
>  ID from email of this thread — any should succeed, but cover letter's one
>  for sure — run the following `b4 am $<message ID>`. It will print the hints
>  what to do next, something like `git am $<patch_title>.mbx`. Then you can
>  continue with `git rebase --interactive v6.12-rc1` if the code or other
>  stuff in the commit message needs to be updated.)
> 

Thanks! This is very helpful.  

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

[Andy Shevchenko]

On Fri, Sep 20, 2024 at 10:03:21AM -0400, Parker Newman wrote:
> From: Parker Newman <pnewman@connecttech.com>
> 
> This patch adds a quirk similar to eeprom_93xx46 to add an extra clock
> cycle before reading data from the EEPROM.

Ah, sorry, forgot to mention the Submitting Patches documentation.
It suggests to use imperative mood in the commit messages, hence
the above should be like

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

Also check other messages in this series.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 2/4] misc: eeprom: eeprom_93cx6: Switch to BIT() macro

[Parker Newman]

From: Parker Newman <pnewman@connecttech.com>

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

Signed-off-by: Parker Newman <pnewman@connecttech.com>
---
 drivers/misc/eeprom/eeprom_93cx6.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/eeprom/eeprom_93cx6.c b/drivers/misc/eeprom/eeprom_93cx6.c
index 4c9827fe9217..e6f0e0fc1ca2 100644
--- a/drivers/misc/eeprom/eeprom_93cx6.c
+++ b/drivers/misc/eeprom/eeprom_93cx6.c
@@ -8,6 +8,7 @@
  * Supported chipsets: 93c46 & 93c66.
  */

+#include <linux/bits.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/delay.h>
@@ -102,7 +103,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.
@@ -152,7 +153,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

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

[Andy Shevchenko]

On Fri, Sep 20, 2024 at 10:03:22AM -0400, Parker Newman wrote:
> From: Parker Newman <pnewman@connecttech.com>
> 
> Use the BIT() macro rather than (1 << (i - 1)).

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 3/4] serial: 8250_exar: Replace custom EEPROM read with eeprom_93cx6

[Parker Newman]

From: Parker Newman <pnewman@connecttech.com>

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().

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

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

Link to mailing list discussion with Andy Shevchenko for reference.

Link: https://lore.kernel.org/linux-serial/Ztr5u2wEt8VF1IdI@black.fi.intel.com/
Signed-off-by: Parker Newman <pnewman@connecttech.com>
---
Changes in v2:
- Refactored cti_read_osc_freq() based on feedback.
- Moved Kconfig change into this patch.
- Sorted headers.
- Fixed comment.

 drivers/tty/serial/8250/8250_exar.c | 55 +++++++++++++++++++++++------
 drivers/tty/serial/8250/Kconfig     |  1 +
 2 files changed, 45 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index b7a75db15249..c40e86920110 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -11,6 +11,7 @@
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/dmi.h>
+#include <linux/eeprom_93cx6.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/math.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 = 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 = 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 = 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,16 @@ 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;
+	__le16 ee_words[2];
+	u32 osc_freq;

-	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, ee_words, ARRAY_SIZE(ee_words));

-	upper_word = exar_ee_read(priv, (eeprom_offset + 1));
-	if (upper_word == 0xFFFF)
+	osc_freq = le16_to_cpu(ee_words[0]) | (le16_to_cpu(ee_words[1]) << 16);
+	if (osc_freq == GENMASK(31, 0))
 		return -EIO;

-	return FIELD_PREP(CTI_EE_MASK_OSC_FREQ_LOWER, lower_word) |
-	       FIELD_PREP(CTI_EE_MASK_OSC_FREQ_UPPER, upper_word);
+	return osc_freq;
 }

 /**
@@ -833,7 +864,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 +1582,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) {
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 47ff50763c04..94910ced8238 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -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

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

[Andy Shevchenko]

On Fri, Sep 20, 2024 at 10:03:23AM -0400, Parker Newman wrote:
> From: Parker Newman <pnewman@connecttech.com>
> 
> 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().
> 
> Add "select EEPROM_93CX6" to config SERIAL_8250_EXAR to ensure
> eeprom_93cx6 driver is also compiled when 8250_exar driver is selected.
> 
> Note: Old exar_ee_read() and associated functions are removed in next
> patch in this series.

Looks perfect!
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Let's wait for others and CIs to test all this and then after v6.12-rc1
is out (presumably within ~ten days) rebase your series (as I suggested
in another reply) and send a v3.

Good work!

-- 
With Best Regards,
Andy Shevchenko

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

[Parker Newman]

On Fri, 20 Sep 2024 17:55:33 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Fri, Sep 20, 2024 at 10:03:23AM -0400, Parker Newman wrote:
> > From: Parker Newman <pnewman@connecttech.com>
> >
> > 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().
> >
> > Add "select EEPROM_93CX6" to config SERIAL_8250_EXAR to ensure
> > eeprom_93cx6 driver is also compiled when 8250_exar driver is selected.
> >
> > Note: Old exar_ee_read() and associated functions are removed in next
> > patch in this series.
>
> Looks perfect!
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> Let's wait for others and CIs to test all this and then after v6.12-rc1
> is out (presumably within ~ten days) rebase your series (as I suggested
> in another reply) and send a v3.
>

Sounds good, I will wait for v6.12-rc1, rebase and send v3 with the
changes you (or others) suggested. I promise I won't forget the Reviewed-by
tags this time :).

> Good work!
>

Thanks and thanks for the help!
-Parker

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

[Andy Shevchenko]

On Fri, Sep 20, 2024 at 10:03:23AM -0400, Parker Newman wrote:
> From: Parker Newman <pnewman@connecttech.com>

...

> +	osc_freq = le16_to_cpu(ee_words[0]) | (le16_to_cpu(ee_words[1]) << 16);
> +	if (osc_freq == GENMASK(31, 0))
>  		return -EIO;

Just noticed that you have
#define CTI_EE_MASK_OSC_FREQ_LOWER      GENMASK(15, 0)
#define CTI_EE_MASK_OSC_FREQ_UPPER      GENMASK(31, 16)

So, please modify them and the above check using these.
Something like

#define CTI_EE_MASK_OSC_FREQ		GENMASK(31, 0)

	osc_freq = le16_to_cpu(ee_words[0]) | (le16_to_cpu(ee_words[1]) << 16);
	if (osc_freq == CTI_EE_MASK_OSC_FREQ)
		return -EIO;

P.S> If I am not mistaken the definitions were only used in this function.

-- 
With Best Regards,
Andy Shevchenko

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

[Parker Newman]

On Fri, 20 Sep 2024 18:26:08 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Fri, Sep 20, 2024 at 10:03:23AM -0400, Parker Newman wrote:
> > From: Parker Newman <pnewman@connecttech.com>
>
> ...
>
> > +	osc_freq = le16_to_cpu(ee_words[0]) | (le16_to_cpu(ee_words[1]) << 16);
> > +	if (osc_freq == GENMASK(31, 0))
> >  		return -EIO;
>
> Just noticed that you have
> #define CTI_EE_MASK_OSC_FREQ_LOWER      GENMASK(15, 0)
> #define CTI_EE_MASK_OSC_FREQ_UPPER      GENMASK(31, 16)
>
> So, please modify them and the above check using these.
> Something like
>

Good catch, I debated using them again with FIELD_PREP() like the
old code but it looked pretty ugly. I guess I missed removing them.

I will fix in v3. In this case should I drop your Reviewed-by tag?
Or is this change small enough to keep it?

> #define CTI_EE_MASK_OSC_FREQ		GENMASK(31, 0)
>
> 	osc_freq = le16_to_cpu(ee_words[0]) | (le16_to_cpu(ee_words[1]) << 16);
> 	if (osc_freq == CTI_EE_MASK_OSC_FREQ)
> 		return -EIO;
>
> P.S> If I am not mistaken the definitions were only used in this function.
>

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

[Andy Shevchenko]

On Fri, Sep 20, 2024 at 11:42:21AM -0400, Parker Newman wrote:
> On Fri, 20 Sep 2024 18:26:08 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

...

> I will fix in v3. In this case should I drop your Reviewed-by tag?
> Or is this change small enough to keep it?

I'm glad you asked. As you rightfully noted, this one is small, no need
to drop tag.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 4/4] serial: 8250_exar: Remove old exar_ee_read() and other unneeded code

[Parker Newman]

From: Parker Newman <pnewman@connecttech.com>

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 <pnewman@connecttech.com>
---
 drivers/tty/serial/8250/8250_exar.c | 90 -----------------------------
 1 file changed, 90 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index c40e86920110..659a44b52f8d 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -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
@@ -269,94 +267,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 = eeprom->data;
--
2.46.0

Re: [PATCH v2 4/4] serial: 8250_exar: Remove old exar_ee_read() and other unneeded code

[Andy Shevchenko]

On Fri, Sep 20, 2024 at 10:03:24AM -0400, Parker Newman wrote:
> From: Parker Newman <pnewman@connecttech.com>
> 
> 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.

You missed my tag. If one sends a new version of the series, they are
responsible to take care about previously given tags.

I'm going to comment on the rest, but something tells me that you might
need a v3 of the series.

-- 
With Best Regards,
Andy Shevchenko