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.0On 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
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. >
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
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
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
© 2016 - 2024 Red Hat, Inc.