Re: [PATCH v2] disk: Use a helper function to reduce duplication

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

Hi Simon,

Patch looks good, but isn't the new function name a bit misleading?
Something like blk_part_find_start() sounds a bit more descriptive, or am I
missing something?

Cheers
/Ilias

On Mon, Mar 20, 2023 at 08:29:57AM +1300, Simon Glass wrote:
> Reduce the duplicated code slightly by using a helper function to handle
> the common code.
>
> This reduces the code size very slightly.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v2:
> - Rebase to -next
>
>  disk/disk-uclass.c | 46 +++++++++++++++++++++++++---------------------
>  1 file changed, 25 insertions(+), 21 deletions(-)
>
> diff --git a/disk/disk-uclass.c b/disk/disk-uclass.c
> index d32747e2242d..7f1fd80b2248 100644
> --- a/disk/disk-uclass.c
> +++ b/disk/disk-uclass.c
> @@ -65,26 +65,38 @@ int part_create_block_devices(struct udevice *blk_dev)
>  	return 0;
>  }
>
> +static int blk_part_setup(struct udevice *dev, lbaint_t *startp,
> +			  lbaint_t blkcnt)
> +{
> +	struct disk_part *part;
> +
> +	part = dev_get_uclass_plat(dev);
> +	if (*startp >= part->gpt_part_info.size)
> +		return -E2BIG;
> +
> +	if (*startp + blkcnt > part->gpt_part_info.size)
> +		blkcnt = part->gpt_part_info.size - *startp;
> +	*startp += part->gpt_part_info.start;
> +
> +	return 0;
> +}
> +
>  static ulong part_blk_read(struct udevice *dev, lbaint_t start,
>  			   lbaint_t blkcnt, void *buffer)
>  {
>  	struct udevice *parent;
> -	struct disk_part *part;
>  	const struct blk_ops *ops;
> +	int ret;
>
>  	parent = dev_get_parent(dev);
>  	ops = blk_get_ops(parent);
>  	if (!ops->read)
>  		return -ENOSYS;
>
> -	part = dev_get_uclass_plat(dev);
> -	if (start >= part->gpt_part_info.size)
> +	ret = blk_part_setup(dev, &start, blkcnt);
> +	if (ret)
>  		return 0;
>
> -	if ((start + blkcnt) > part->gpt_part_info.size)
> -		blkcnt = part->gpt_part_info.size - start;
> -	start += part->gpt_part_info.start;
> -
>  	return ops->read(parent, start, blkcnt, buffer);
>  }
>
> @@ -92,22 +104,18 @@ static ulong part_blk_write(struct udevice *dev, lbaint_t start,
>  			    lbaint_t blkcnt, const void *buffer)
>  {
>  	struct udevice *parent;
> -	struct disk_part *part;
>  	const struct blk_ops *ops;
> +	int ret;
>
>  	parent = dev_get_parent(dev);
>  	ops = blk_get_ops(parent);
>  	if (!ops->write)
>  		return -ENOSYS;
>
> -	part = dev_get_uclass_plat(dev);
> -	if (start >= part->gpt_part_info.size)
> +	ret = blk_part_setup(dev, &start, blkcnt);
> +	if (ret)
>  		return 0;
>
> -	if ((start + blkcnt) > part->gpt_part_info.size)
> -		blkcnt = part->gpt_part_info.size - start;
> -	start += part->gpt_part_info.start;
> -
>  	return ops->write(parent, start, blkcnt, buffer);
>  }
>
> @@ -115,22 +123,18 @@ static ulong part_blk_erase(struct udevice *dev, lbaint_t start,
>  			    lbaint_t blkcnt)
>  {
>  	struct udevice *parent;
> -	struct disk_part *part;
>  	const struct blk_ops *ops;
> +	int ret;
>
>  	parent = dev_get_parent(dev);
>  	ops = blk_get_ops(parent);
>  	if (!ops->erase)
>  		return -ENOSYS;
>
> -	part = dev_get_uclass_plat(dev);
> -	if (start >= part->gpt_part_info.size)
> +	ret = blk_part_setup(dev, &start, blkcnt);
> +	if (ret)
>  		return 0;
>
> -	if ((start + blkcnt) > part->gpt_part_info.size)
> -		blkcnt = part->gpt_part_info.size - start;
> -	start += part->gpt_part_info.start;
> -
>  	return ops->erase(parent, start, blkcnt);
>  }
>
> --
> 2.40.0.rc1.284.g88254d51c5-goog
>