On Fri, 13 Sep 2024 20:48:03 +0300 Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote: > On Fri, Sep 13, 2024 at 10:55:38AM -0400, Parker Newman wrote: > > From: Parker Newman <pnewman@xxxxxxxxxxxxxxx> > > > > 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; >
