[PATCH v1 0/6] serial: 8250_exar: Replace custom EEPROM code with eeprom_93cx6

[PATCH v1 0/6] 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/

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

[PATCH v1 1/6] 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. 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 <pnewman@connecttech.com>
---
 drivers/misc/eeprom/eeprom_93cx6.c | 15 +++++++++++++++
 include/linux/eeprom_93cx6.h       |  7 +++++++
 2 files changed, 22 insertions(+)

diff --git a/drivers/misc/eeprom/eeprom_93cx6.c b/drivers/misc/eeprom/eeprom_93cx6.c
index 9627294fe3e9..0f52d3c4bc1d 100644
--- a/drivers/misc/eeprom/eeprom_93cx6.c
+++ b/drivers/misc/eeprom/eeprom_93cx6.c
@@ -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.
 	 */
diff --git a/include/linux/eeprom_93cx6.h b/include/linux/eeprom_93cx6.h
index c860c72a921d..5274bcc7ca39 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.
  */
@@ -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

[PATCH v1 2/6] 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 0f52d3c4bc1d..64da22edefa4 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>
@@ -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

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

[Parker Newman]

From: Parker Newman <pnewman@connecttech.com>

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 <pnewman@connecttech.com>
---
 drivers/misc/eeprom/eeprom_93cx6.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/eeprom/eeprom_93cx6.c b/drivers/misc/eeprom/eeprom_93cx6.c
index 64da22edefa4..755be9a4ffd4 100644
--- a/drivers/misc/eeprom/eeprom_93cx6.c
+++ b/drivers/misc/eeprom/eeprom_93cx6.c
@@ -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

[PATCH v1 4/6] 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().

Link to discussion with Andy Shevchenko:
Link: https://lore.kernel.org/linux-serial/Ztr5u2wEt8VF1IdI@black.fi.intel.com/

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

Signed-off-by: Parker Newman <pnewman@connecttech.com>
---
 drivers/tty/serial/8250/8250_exar.c | 54 +++++++++++++++++++++++------
 1 file changed, 43 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index b7a75db15249..0edb5aeba199 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -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

[PATCH v1 5/6] 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 | 92 -----------------------------
 1 file changed, 92 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 0edb5aeba199..3ef28429f1d1 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
@@ -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

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

[Parker Newman]

From: Parker Newman <pnewman@connecttech.com>

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 <pnewman@connecttech.com>
---
 drivers/tty/serial/8250/Kconfig | 1 +
 1 file changed, 1 insertion(+)

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 v1 1/6] misc: eeprom: eeprom_93cx6: Add quirk for extra read clock cycle

[Andy Shevchenko]

On Fri, Sep 13, 2024 at 10:55:38AM -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. 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)

> 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 :)

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

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

...

> +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.

...

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

No additional delay is needed?

> +		eeprom_93cx6_pulse_low(eeprom);
> +	}

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

Ditto.

> +		eeprom_93cx6_pulse_low(eeprom);
> +	}

...

> +/* 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)...

...

>  	int width;
> +	unsigned int quirks;

...somewhere here.

>  	char drive_data;
>  	char reg_data_in;

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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

Missed period at the end.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Sep 13, 2024 at 10:55:40AM -0400, Parker Newman wrote:
> From: Parker Newman <pnewman@connecttech.com>
> 
> 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__);

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.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Sep 13, 2024 at 10:55:41AM -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().
> 
> Link to discussion with Andy Shevchenko:

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

> Link: https://lore.kernel.org/linux-serial/Ztr5u2wEt8VF1IdI@black.fi.intel.com/

Make it real tag by moving...

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

...somewhere here.

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

...

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

Keep it sorted.

...

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

Unneeded explicit cast.

> +	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.

> +	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;

Unneeded cast from void *.

> +	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);
> +}

...

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

Unneeded cast.

...

> +	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)

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

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Sep 13, 2024 at 10:55:42AM -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.

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

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Sep 13, 2024 at 10:55:37AM -0400, Parker Newman wrote:
> 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/

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.

-- 
With Best Regards,
Andy Shevchenko

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

[Parker Newman]

On Fri, 13 Sep 2024 20:48:03 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Fri, Sep 13, 2024 at 10:55:38AM -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. 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!

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

> > 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.
>
> ...
>
> > +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.

> ...
>
> > +	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.

> > +		eeprom_93cx6_pulse_low(eeprom);
> > +	}
>
> > +	if (has_quirk_extra_read_cycle(eeprom)) {
> > +		eeprom_93cx6_pulse_high(eeprom);
>
> Ditto.
>
> > +		eeprom_93cx6_pulse_low(eeprom);
> > +	}
>
> ...
>
> > +/* 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!

> ...
>
> >  	int width;
> > +	unsigned int quirks;
>
> ...somewhere here.
>
> >  	char drive_data;
> >  	char reg_data_in;
>

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

[Andy Shevchenko]

On Fri, Sep 13, 2024 at 02:24:20PM -0400, Parker Newman wrote:
> On Fri, 13 Sep 2024 20:48:03 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, Sep 13, 2024 at 10:55:38AM -0400, Parker Newman wrote:

...

> > > 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: ...

...

> > > +	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!

> > > +		eeprom_93cx6_pulse_low(eeprom);
> > > +	}

-- 
With Best Regards,
Andy Shevchenko

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

[Parker Newman]

On Fri, 13 Sep 2024 21:10:01 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Fri, Sep 13, 2024 at 10:55:37AM -0400, Parker Newman wrote:
> > 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/
>
> 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

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

[Parker Newman]

On Fri, 13 Sep 2024 20:54:05 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Fri, Sep 13, 2024 at 10:55:40AM -0400, Parker Newman wrote:
> > From: Parker Newman <pnewman@connecttech.com>
> >
> > 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__);
>
> First of all, you probably want pr_fmt() to be defined.

Good point, I will add.

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

> 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

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

[Andy Shevchenko]

On Fri, Sep 13, 2024 at 03:12:28PM -0400, Parker Newman wrote:
> On Fri, 13 Sep 2024 20:54:05 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, Sep 13, 2024 at 10:55:40AM -0400, Parker Newman wrote:

...

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

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

-- 
With Best Regards,
Andy Shevchenko

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

[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/202409142138.yCOHBlL1-lkp@intel.com/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/202409142138.yCOHBlL1-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409142138.yCOHBlL1-lkp@intel.com/

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	

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

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

[Greg Kroah-Hartman]

On Fri, Sep 13, 2024 at 10:55:40AM -0400, Parker Newman wrote:
> From: Parker Newman <pnewman@connecttech.com>
> 
> 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 <pnewman@connecttech.com>
> ---
>  drivers/misc/eeprom/eeprom_93cx6.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/eeprom/eeprom_93cx6.c b/drivers/misc/eeprom/eeprom_93cx6.c
> index 64da22edefa4..755be9a4ffd4 100644
> --- a/drivers/misc/eeprom/eeprom_93cx6.c
> +++ b/drivers/misc/eeprom/eeprom_93cx6.c
> @@ -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

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

[Andy Shevchenko]

On Sat, Sep 14, 2024 at 08:58:50PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Sep 13, 2024 at 10:55:40AM -0400, Parker Newman wrote:

...

> > -			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?

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Sat, Sep 14, 2024 at 09:26:27PM +0800, kernel test robot wrote:
> 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/202409142138.yCOHBlL1-lkp@intel.com/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/202409142138.yCOHBlL1-lkp@intel.com/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 <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202409142138.yCOHBlL1-lkp@intel.com/
> 
> sparse warnings: (new ones prefixed by >>)
> >> 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.

-- 
With Best Regards,
Andy Shevchenko

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

[Greg Kroah-Hartman]

On Mon, Sep 16, 2024 at 12:55:05PM +0300, Andy Shevchenko wrote:
> On Sat, Sep 14, 2024 at 08:58:50PM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Sep 13, 2024 at 10:55:40AM -0400, Parker Newman wrote:
> 
> ...
> 
> > > -			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

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

[Andy Shevchenko]

On Mon, Sep 16, 2024 at 12:25:52PM +0200, Greg Kroah-Hartman wrote:
> On Mon, Sep 16, 2024 at 12:55:05PM +0300, Andy Shevchenko wrote:
> > On Sat, Sep 14, 2024 at 08:58:50PM +0200, Greg Kroah-Hartman wrote:
> > > On Fri, Sep 13, 2024 at 10:55:40AM -0400, Parker Newman wrote:

...

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

-- 
With Best Regards,
Andy Shevchenko

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

[Parker Newman]

On Mon, 16 Sep 2024 13:32:47 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Mon, Sep 16, 2024 at 12:25:52PM +0200, Greg Kroah-Hartman wrote:
> > On Mon, Sep 16, 2024 at 12:55:05PM +0300, Andy Shevchenko wrote:
> > > On Sat, Sep 14, 2024 at 08:58:50PM +0200, Greg Kroah-Hartman wrote:
> > > > On Fri, Sep 13, 2024 at 10:55:40AM -0400, Parker Newman wrote:
>
> ...
>
> > > > > -			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.
>

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.

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

[Andy Shevchenko]

On Mon, Sep 16, 2024 at 08:04:10AM -0400, Parker Newman wrote:
> On Mon, 16 Sep 2024 13:32:47 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Sep 16, 2024 at 12:25:52PM +0200, Greg Kroah-Hartman wrote:
> > > On Mon, Sep 16, 2024 at 12:55:05PM +0300, Andy Shevchenko wrote:
> > > > On Sat, Sep 14, 2024 at 08:58:50PM +0200, Greg Kroah-Hartman wrote:
> > > > > On Fri, Sep 13, 2024 at 10:55:40AM -0400, Parker Newman wrote:

...

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

-- 
With Best Regards,
Andy Shevchenko

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

[Parker Newman]

On Mon, 16 Sep 2024 15:18:15 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Mon, Sep 16, 2024 at 08:04:10AM -0400, Parker Newman wrote:
> > On Mon, 16 Sep 2024 13:32:47 +0300
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > On Mon, Sep 16, 2024 at 12:25:52PM +0200, Greg Kroah-Hartman wrote:
> > > > On Mon, Sep 16, 2024 at 12:55:05PM +0300, Andy Shevchenko wrote:
> > > > > On Sat, Sep 14, 2024 at 08:58:50PM +0200, Greg Kroah-Hartman wrote:
> > > > > > On Fri, Sep 13, 2024 at 10:55:40AM -0400, Parker Newman wrote:
>
> ...
>
> > > > > > > -			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.
> >
> > 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.
>

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.

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

[Andy Shevchenko]

On Mon, Sep 16, 2024 at 11:20:56AM -0400, Parker Newman wrote:
> On Mon, 16 Sep 2024 15:18:15 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Sep 16, 2024 at 08:04:10AM -0400, Parker Newman wrote:
> > > On Mon, 16 Sep 2024 13:32:47 +0300
> > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Mon, Sep 16, 2024 at 12:25:52PM +0200, Greg Kroah-Hartman wrote:
> > > > > On Mon, Sep 16, 2024 at 12:55:05PM +0300, Andy Shevchenko wrote:
> > > > > > On Sat, Sep 14, 2024 at 08:58:50PM +0200, Greg Kroah-Hartman wrote:
> > > > > > > On Fri, Sep 13, 2024 at 10:55:40AM -0400, Parker Newman wrote:

...

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

-- 
With Best Regards,
Andy Shevchenko