Hi Simon,
On Thu, Aug 2, 2018 at 2:21 PM, Simon Glass <s...@chromium.org> wrote:
> On 31 July 2018 at 04:01, Mario Six <mario....@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....@gdsys.cc>
>> ---
>>
>> v2 -> v3:
>> New in v3
>>
>> ---
>> drivers/core/regmap.c | 68
>> ++++++++++++++++++++++++++++++++++++++++++---------
>> 1 file changed, 56 insertions(+), 12 deletions(-)
>
> Reviewed-by: Simon Glass <s...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
