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

Andy Shevchenko
2024-09-13T17:48:09Z

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