Re: [PATCH 2/7] eeprom: at24: consider that SERIAL and MAC flags imply read-only

[Heiner Kallweit <hkallweit1@gmail.com>]

Am 01.12.2017 um 11:10 schrieb Bartosz Golaszewski:
> 2017-11-30 7:49 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
>> Flags AT24_FLAG_SERIAL and AT24_FLAG_MAC imply read-only.
>> Therefore handle this in the code instead of specifying
>> AT24_FLAG_READONLY in the config data in these cases.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/misc/eeprom/at24.c | 31 +++++++++++++------------------
>>  1 file changed, 13 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
>> index c75bb9b45..90fefd1cf 100644
>> --- a/drivers/misc/eeprom/at24.c
>> +++ b/drivers/misc/eeprom/at24.c
>> @@ -131,38 +131,29 @@ static const struct i2c_device_id at24_ids[] = {
>>         { "24c00",      AT24_DEVICE_MAGIC(128 / 8,      AT24_FLAG_TAKE8ADDR) },
>>         /* old variants can't be handled with this generic entry! */
>>         { "24c01",      AT24_DEVICE_MAGIC(1024 / 8,     0) },
>> -       { "24cs01",     AT24_DEVICE_MAGIC(16,
>> -                               AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
>> +       { "24cs01",     AT24_DEVICE_MAGIC(16,           AT24_FLAG_SERIAL) },
>>         { "24c02",      AT24_DEVICE_MAGIC(2048 / 8,     0) },
>> -       { "24cs02",     AT24_DEVICE_MAGIC(16,
>> -                               AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
>> -       { "24mac402",   AT24_DEVICE_MAGIC(48 / 8,
>> -                               AT24_FLAG_MAC | AT24_FLAG_READONLY) },
>> -       { "24mac602",   AT24_DEVICE_MAGIC(64 / 8,
>> -                               AT24_FLAG_MAC | AT24_FLAG_READONLY) },
>> +       { "24cs02",     AT24_DEVICE_MAGIC(16,           AT24_FLAG_SERIAL) },
>> +       { "24mac402",   AT24_DEVICE_MAGIC(48 / 8,       AT24_FLAG_MAC) },
>> +       { "24mac602",   AT24_DEVICE_MAGIC(64 / 8,       AT24_FLAG_MAC) },
>>         /* spd is a 24c02 in memory DIMMs */
>>         { "spd",        AT24_DEVICE_MAGIC(2048 / 8,
>>                                 AT24_FLAG_READONLY | AT24_FLAG_IRUGO) },
>>         { "24c04",      AT24_DEVICE_MAGIC(4096 / 8,     0) },
>> -       { "24cs04",     AT24_DEVICE_MAGIC(16,
>> -                               AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
>> +       { "24cs04",     AT24_DEVICE_MAGIC(16,   AT24_FLAG_SERIAL) },
>>         /* 24rf08 quirk is handled at i2c-core */
>>         { "24c08",      AT24_DEVICE_MAGIC(8192 / 8,     0) },
>> -       { "24cs08",     AT24_DEVICE_MAGIC(16,
>> -                               AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
>> +       { "24cs08",     AT24_DEVICE_MAGIC(16,   AT24_FLAG_SERIAL) },
>>         { "24c16",      AT24_DEVICE_MAGIC(16384 / 8,    0) },
>> -       { "24cs16",     AT24_DEVICE_MAGIC(16,
>> -                               AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
>> +       { "24cs16",     AT24_DEVICE_MAGIC(16,           AT24_FLAG_SERIAL) },
>>         { "24c32",      AT24_DEVICE_MAGIC(32768 / 8,    AT24_FLAG_ADDR16) },
>>         { "24cs32",     AT24_DEVICE_MAGIC(16,
>>                                 AT24_FLAG_ADDR16 |
>> -                               AT24_FLAG_SERIAL |
>> -                               AT24_FLAG_READONLY) },
>> +                               AT24_FLAG_SERIAL) },
>>         { "24c64",      AT24_DEVICE_MAGIC(65536 / 8,    AT24_FLAG_ADDR16) },
>>         { "24cs64",     AT24_DEVICE_MAGIC(16,
>>                                 AT24_FLAG_ADDR16 |
>> -                               AT24_FLAG_SERIAL |
>> -                               AT24_FLAG_READONLY) },
>> +                               AT24_FLAG_SERIAL) },
>>         { "24c128",     AT24_DEVICE_MAGIC(131072 / 8,   AT24_FLAG_ADDR16) },
>>         { "24c256",     AT24_DEVICE_MAGIC(262144 / 8,   AT24_FLAG_ADDR16) },
>>         { "24c512",     AT24_DEVICE_MAGIC(524288 / 8,   AT24_FLAG_ADDR16) },
>> @@ -556,6 +547,10 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
>>                 chip.context = NULL;
>>         }
>>
>> +       /* both flags imply read-only */
>> +       if (chip.flags & AT24_FLAG_SERIAL || chip.flags & AT24_FLAG_MAC)
>> +               chip.flags |= AT24_FLAG_READONLY;
>> +
>>         if (!is_power_of_2(chip.byte_len))
>>                 dev_warn(&client->dev,
>>                         "byte_len looks suspicious (no power of 2)!\n");
>> --
>> 2.15.0
>>
>>
> 
> Just as with the header patch: this doesn't save us any code other
> than visual. We want the definition to scream that this chip is
> read-only. Nacked.
> 
I see your point. My concern is that so far we may have platform-data-
configured devices setting flag SERIAL or MAC and not READONLY and we
have no control over such (IMO) misconfigurations.

So my primary question would be whether we agree on SERIAL and MAC
implying READONLY. If yes then I think we should reflect this
relationship in the code.
Instead of silently setting flag READONLY we could also reject such
invalid flag configurations. Or print at least a message/warning
when setting READONLY.

Rgds, Heiner

> Thanks,
> Bartosz
>