Re: [PATCH] optoe: driver to read/write SFP/QSFP EEPROMs

[Arnd Bergmann]

On Mon, Jun 11, 2018 at 6:25 AM, Don Bollinger <don@xxxxxxxxxxxxxxxxx> wrote:
> optoe is an i2c based driver that supports read/write access to all
> the pages (tables) of MSA standard SFP and similar devices (conforming
> to the SFF-8472 spec) and MSA standard QSFP and similar devices
> (conforming to the SFF-8436 spec).
>
> These devices provide identification, operational status and control
> registers via an EEPROM model. These devices support one or 3 fixed
> pages (128 bytes) of data, and one page that is selected via a page
> register on the first fixed page.  Thus the driver's main task is
> to map these pages onto a simple linear address space for user space
> management applications.  See the driver code for a detailed layout.
>
> EEPROM data is accessible via a bin_attribute file called 'eeprom',
> e.g. /sys/bus/i2c/devices/24-0050/eeprom.
>
> Signed-off-by: Don Bollinger <don@xxxxxxxxxxxxxxxxx>

> +
> +#undef EEPROM_CLASS
> +#ifdef CONFIG_EEPROM_CLASS
> +#define EEPROM_CLASS
> +#endif
> +#ifdef CONFIG_EEPROM_CLASS_MODULE
> +#define EEPROM_CLASS
> +#endif

I don't understand this part: I see some older patches introducing an
EEPROM_CLASS, but nothing ever seems to have made it into the
mainline kernel.

If that class isn't there, this code shouldn't be either. You can always
add it back in case we decide to introduce that class later, but then
I wouldn't make it a compile-time option but just a hard dependency
instead.

> +struct optoe_platform_data {
> +       u32             byte_len;               /* size (sum of all addr) */
> +       u16             page_size;              /* for writes */
> +       u8              flags;
> +       void            *dummy1;                /* backward compatibility */
> +       void            *dummy2;                /* backward compatibility */
> +
> +#ifdef EEPROM_CLASS
> +       struct eeprom_platform_data *eeprom_data;
> +#endif
> +       char port_name[MAX_PORT_NAME_LEN];
> +};

What is the backward-compatibility for? Normally we don't do it
like that, we only keep compatibility with things that have already
been part of the kernel, especially when you also have an #ifdef
that makes it incompatible again ;-)

> +struct optoe_data {
> +       struct optoe_platform_data chip;

Maybe merge the two structures into one?

        Arnd