[U-Boot] [PATCH v3 05/13] regmap: Introduce init_range

[Mario Six]

Hi Simon,

On Thu, Aug 2, 2018 at 2:21 PM, Simon Glass <sjg at chromium.org> wrote:
> On 31 July 2018 at 04:01, Mario Six <mario.six at gdsys.cc> wrote:
>> Both fdtdec_get_addr_size_fixed and of_address_to_resource can fail with
>> an error, which is not currently checked during regmap initialization.
>>
>> Since the indentation depth is already quite deep, extract a new
>> 'init_range' method to do the initialization.
>>
>> Signed-off-by: Mario Six <mario.six at gdsys.cc>
>> ---
>>
>> v2 -> v3:
>> New in v3
>>
>> ---
>>  drivers/core/regmap.c | 68 ++++++++++++++++++++++++++++++++++++++++++---------
>>  1 file changed, 56 insertions(+), 12 deletions(-)
>
> Reviewed-by: Simon Glass <sjg at chromium.org>
>
> nit below
>
>>
>> diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c
>> index 4ebab233490..51d9cadc510 100644
>> --- a/drivers/core/regmap.c
>> +++ b/drivers/core/regmap.c
>> @@ -56,6 +56,58 @@ int regmap_init_mem_platdata(struct udevice *dev, fdt_val_t *reg, int count,
>>         return 0;
>>  }
>>  #else
>> +/**
>> + * init_range() - Initialize a single range of a regmap
>> + * @node:     Device node that will use the map in question
>> + * @range:    Pointer to a regmap_range structure that will be initialized
>> + * @addr_len: The length of the addr parts of the reg property
>> + * @size_len: The length of the size parts of the reg property
>> + * @index:    The index of the range to initialize
>> + *
>> + * This function will read the necessary 'reg' information from the device tree
>> + * (the 'addr' part, and the 'length' part), and initialize the range in
>> + * quesion.
>> + *
>> + * Return: 0 if OK, -ve on error
>> + */
>> +static int init_range(ofnode node, struct regmap_range *range, int addr_len,
>> +                     int size_len, int index)
>> +{
>> +       fdt_size_t sz;
>> +       struct resource r;
>> +
>> +       if (of_live_active()) {
>> +               int ret;
>> +
>> +               ret = of_address_to_resource(ofnode_to_np(node),
>> +                                            index, &r);
>> +               if (ret) {
>> +                       debug("%s: Could not read resource of range %d (ret = %d)\n",
>> +                             ofnode_get_name(node), index, ret);
>> +                       return ret;
>> +               }
>> +
>> +               range->start = r.start;
>> +               range->size = r.end - r.start + 1;
>> +
>> +               return 0;
>> +       }
>
> I wonder about having an else here? That makes it clear that this
> function has two implementations.
>

OK, shouldn't be a problem. I originally didn't put the else in because of the
length of the fdtdec_get_add_size_fixed call, but I can extract the
ofnode_to_offset into a "offset" variable, which should make the line more
easily indentable.

>> +
>> +       range->start = fdtdec_get_addr_size_fixed(gd->fdt_blob,
>> +                                                 ofnode_to_offset(node),
>> +                                                 "reg", index, addr_len,
>> +                                                 size_len, &sz, true);
>> +       if (range->start == FDT_ADDR_T_NONE) {
>> +               debug("%s: Could not read start of range %d\n",
>> +                     ofnode_get_name(node), index);
>> +               return -EINVAL;
>> +       }
>> +
>> +       range->size = sz;
>> +
>> +       return 0;
>> +}
>> +
>>  int regmap_init_mem(ofnode node, struct regmap **mapp)
>>  {
>>         struct regmap_range *range;
>> @@ -64,7 +116,6 @@ int regmap_init_mem(ofnode node, struct regmap **mapp)
>>         int addr_len, size_len, both_len;
>>         int len;
>>         int index;
>> -       struct resource r;
>>
>>         addr_len = ofnode_read_simple_addr_cells(ofnode_get_parent(node));
>>         if (addr_len < 0) {
>> @@ -101,17 +152,10 @@ int regmap_init_mem(ofnode node, struct regmap **mapp)
>>
>>         for (range = map->ranges, index = 0; count > 0;
>>              count--, range++, index++) {
>> -               fdt_size_t sz;
>> -               if (of_live_active()) {
>> -                       of_address_to_resource(ofnode_to_np(node), index, &r);
>> -                       range->start = r.start;
>> -                       range->size = r.end - r.start + 1;
>> -               } else {
>> -                       range->start = fdtdec_get_addr_size_fixed(gd->fdt_blob,
>> -                                       ofnode_to_offset(node), "reg", index,
>> -                                       addr_len, size_len, &sz, true);
>> -                       range->size = sz;
>> -               }
>> +               int ret = init_range(node, range, addr_len, size_len, index);
>> +
>> +               if (ret)
>> +                       return ret;
>>         }
>>
>>         *mapp = map;
>> --
>> 2.11.0
>>

Best regards,
Mario