[U-Boot] [PATCH 40/44] dm: blk: Allow blk_create_device() to allocate the device number

[Simon Glass]

Hi Stephen,

On 12 April 2016 at 14:56, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 04/09/2016 08:45 PM, Simon Glass wrote:
>>
>> Allow a devnum parameter of -1 to indicate that the device number should
>> be
>> alocated automatically. The next highest available device number for that
>> interface type is used.
>
>
>> diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
>
>
>> +int blk_find_max_devnum(enum if_type if_type)
>> +{
>> +       struct udevice *dev;
>> +       int max_devnum = -ENODEV;
>> +       struct uclass *uc;
>> +       int ret;
>> +
>> +       ret = uclass_get(UCLASS_BLK, &uc);
>> +       if (ret)
>> +               return ret;
>
>
> I'm tempted to suggest that the -ENODEV special case be place here, and
> return 0 instead. Of course, that would require the devnum to be a
> pointer/output parameter rather than encoded into the return value, so that
> other errors could still be returned. Perhaps not worth it.
>
> My reasoning is that clients of this function should just be able to ask
> "what's the next device number" and not care about implementing the special
> cases, but rather rely on this function encoding everything. This doesn't
> matter so much if there's only one call-site though. If there is only one
> call-site, should this be static?
>
> Hmm. This could all be solved by having the function return the next free
> devnum (0..n) (or -ve error) rather than the highest used devnum. That way,
> the -ENODEV special case would return 0, and clients could just do:
>
> devnum = blk_find_next_free_devnum(...);
> if (devnum < 0)
>     return devnum;
>
> and nothing else.

Yes that was my first implementation. But I had to change it because
MMC needs to find the maximum device number at present (in the next
series). It's a little painful but some future work may centralise
things better.

>
>> @@ -428,6 +448,15 @@ int blk_create_device(struct udevice *parent, const
>> char *drv_name,
>>         desc->lba = size / blksz;
>>         desc->part_type = PART_TYPE_UNKNOWN;
>>         desc->bdev = dev;
>> +       if (devnum == -1) {
>> +               ret = blk_find_max_devnum(if_type);
>> +               if (ret == -ENODEV)
>> +                       devnum = 0;
>
>
> I'm thinking that clients should have to care about implementing that ever
> time they use the function.

Right, but there really are only two callers - one in this code and one in MMC.

>
>
>> +               else if (ret < 0)
>> +                       return ret;
>> +               else
>> +                       devnum = ret + 1;
>> +       }
>>         desc->devnum = devnum;
>
>

Regards,
Simon