On Fri, Sep 13, 2024 at 10:55:41AM -0400, Parker Newman wrote: > From: Parker Newman <pnewman@xxxxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxxxxxxx/ 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@xxxxxxxxxxxxxxx> ... > #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
