Re: [resent RFC 08/22] dm: blk: add UCLASS_PARTITION

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

On Mon, Oct 04, 2021 at 09:40:21PM +0300, Ilias Apalodimas wrote:
> Hi Akashi-san,
> 
> >  
> 
> [...]
> 
> > +int blk_create_partitions(struct udevice *parent)
> > +{
> > +	int part, count;
> > +	struct blk_desc *desc = dev_get_uclass_plat(parent);
> > +	struct disk_partition info;
> > +	struct disk_part *part_data;
> > +	char devname[32];
> > +	struct udevice *dev;
> > +	int ret;
> > +
> > +	if (!CONFIG_IS_ENABLED(PARTITIONS) ||
> > +	    !CONFIG_IS_ENABLED(HAVE_BLOCK_DEVICE))
> > +		return 0;
> 
> Would it make more sense to return an error here?

!CONFIG_IS_ENABLED(PARTITIONS) means that a user doesn't want to
use partitions on their system. So simply returning 0 would be fine, I think.
This is kinda equivalence at the caller site:

	if (CONFIG_IS_ENABLED(PARTITIONS) &&
	    CONFIG_IS_ENABLED(HAVE_BLOCK_DEVICE))
                ret = blk_create_partitions(dev);
        else
                debug("We don't care about partitions.");

> > +
> > +	/* Add devices for each partition */
> > +	for (count = 0, part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
> > +		if (part_get_info(desc, part, &info))
> > +			continue;
> > +		snprintf(devname, sizeof(devname), "%s:%d", parent->name,
> > +			 part);
> > +
> > +		ret = device_bind_driver(parent, "blk_partition",
> > +					 strdup(devname), &dev);
> > +		if (ret)
> > +			return ret;
> > +
> > +		part_data = dev_get_uclass_plat(dev);
> > +		part_data->partnum = part;
> > +		part_data->gpt_part_info = info;
> > +		count++;
> > +
> > +		device_probe(dev);
> 
> Probe can fail. 

Theoretically, yes. But as a matter of fact, device_probe() does almost nothing
for UCLASS_PARTITION devices under the proposed implementation here and
I don't expect it will ever fail.
Please note that, as I commented in blk_part_post_probe(), we may want to
call blk_create_partitions() in the future so that we will support
"nested" partitioning in a partition :)

-Takahiro Akashi


> > +	}
> > +	debug("%s: %d partitions found in %s\n", __func__, count, parent->name);
> > +
> > +	return 0;
> > +}
> > +
> >  static int blk_post_probe(struct udevice *dev)
> >  {
> >  	if (IS_ENABLED(CONFIG_PARTITIONS) &&
> > @@ -713,3 +752,75 @@ UCLASS_DRIVER(blk) = {
> >  	.post_probe	= blk_post_probe,
> >  	.per_device_plat_auto	= sizeof(struct blk_desc),
> >  };
> [...]
> 
> Regards
> /Ilias