Re: [PATCH v3 5/7] eeprom: at24: add regmap-based read function

[Heiner Kallweit <hkallweit1@gmail.com>]

Am 22.11.2017 um 12:20 schrieb Bartosz Golaszewski:
> 2017-11-22 8:05 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
>> Add regmap-based read function and instead of using three different
>> read functions (standard, mac, serial) use just one and factor out the
>> read offset adjustment for mac and serial to at24_adjust_read_offset.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> v2:
>> - rebased
>> v3:
>> - improve readability
>> - re-introduce debug message
>> - introduce at24_adjust_read_offset
>> ---
>>  drivers/misc/eeprom/at24.c | 53 +++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 52 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
>> index 493e2b646..589e6d855 100644
>> --- a/drivers/misc/eeprom/at24.c
>> +++ b/drivers/misc/eeprom/at24.c
>> @@ -312,6 +312,57 @@ static ssize_t at24_eeprom_read_smbus(struct at24_data *at24, char *buf,
>>         return -ETIMEDOUT;
>>  }
>>
>> +static void at24_adjust_read_offset(struct at24_data *at24,
>> +                                   unsigned int *offset)
>> +{
>> +       u8 flags = at24->chip.flags;
>> +
>> +       if (flags & AT24_FLAG_MAC)
>> +               *offset += 0x90;
> 
> Ok, so I missed this last time. On the other hand we could avoid this
> function and all these if elses if we added an unsigned int called for
> example: offset_adj to struct at24_data. It could be initialized in
> probe() to whatever value is needed according to the flags and then
> or'ed right before reading from the regmap in at24_regmap_read(). How
> about that?
> 
Indeed, this would be a little better. Will prepare a v4 with this change.

>> +       else if (flags & AT24_FLAG_SERIAL && flags & AT24_FLAG_ADDR16)
>> +               /*
>> +                * For 16 bit address pointers, the word address must contain
>> +                * a '10' sequence in bits 11 and 10 regardless of the
>> +                * intended position of the address pointer.
>> +                */
>> +               *offset |= BIT(11);
>> +       else if (flags & AT24_FLAG_SERIAL)
>> +               /*
>> +                * Otherwise the word address must begin with a '10' sequence,
>> +                * regardless of the intended address.
>> +                */
>> +               *offset |= BIT(7);
>> +}
>> +
>> +static ssize_t at24_regmap_read(struct at24_data *at24, char *buf,
>> +                               unsigned int offset, size_t count)
>> +{
>> +       unsigned long timeout, read_time;
>> +       struct at24_client *at24_client;
>> +       struct i2c_client *client;
>> +       struct regmap *regmap;
>> +       int ret;
>> +
>> +       at24_client = at24_translate_offset(at24, &offset);
>> +       regmap = at24_client->regmap;
>> +       client = at24_client->client;
>> +
>> +       if (count > io_limit)
>> +               count = io_limit;
>> +
>> +       at24_adjust_read_offset(at24, &offset);
>> +
>> +       loop_until_timeout(timeout, read_time) {
>> +               ret = regmap_bulk_read(regmap, offset, buf, count);
>> +               dev_dbg(&client->dev, "read %zu@%d --> %d (%ld)\n",
>> +                       count, offset, ret, jiffies);
>> +               if (!ret)
>> +                       return count;
>> +       }
>> +
>> +       return -ETIMEDOUT;
>> +}
>> +
>>  static ssize_t at24_eeprom_read_i2c(struct at24_data *at24, char *buf,
>>                                     unsigned int offset, size_t count)
>>  {
>> @@ -531,7 +582,7 @@ static int at24_read(void *priv, unsigned int off, void *val, size_t count)
>>         while (count) {
>>                 int     status;
>>
>> -               status = at24->read_func(at24, buf, off, count);
>> +               status = at24_regmap_read(at24, buf, off, count);
>>                 if (status < 0) {
>>                         mutex_unlock(&at24->lock);
>>                         pm_runtime_put(&client->dev);
>> --
>> 2.15.0
>>
>>
>