2017-12-02 23:00 GMT+01:00 Heiner Kallweit <hkallweit1@xxxxxxxxx>:
> Am 01.12.2017 um 11:10 schrieb Bartosz Golaszewski:
>> 2017-11-30 7:49 GMT+01:00 Heiner Kallweit <hkallweit1@xxxxxxxxx>:
>>> 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@xxxxxxxxx>
>>> ---
>>> 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
>>
>
OK, this makes more sense, let's check if READONLY is set for MAC and
CS and if not: let's set it ourselves while also printing a warning.
Thanks,
Bartosz
