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

Simon Glass
2016-05-01T18:56:54Z

Hi Stephen,

On 12 April 2016 at 14:56, Stephen Warren <swarren@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