Re: [PATCH 8/8] disk: Make blk_get_ops() internal to blk uclass

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

On Mon, Aug 14, 2023 at 04:42:57PM -0600, Simon Glass wrote:
> On Sun, 13 Aug 2023 at 17:47, Marek Vasut
> <marek.vasut+renesas@mailbox.org> wrote:
> >
> > Move the macro into blk-uclass.c , since it is only used there.
> >
> > Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> > ---
> > Cc: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > Cc: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> > Cc: Bin Meng <bmeng.cn@gmail.com>
> > Cc: Heinrich Schuchardt <xypron.glplk@gmx.de>
> > Cc: Joshua Watt <jpewhacker@gmail.com>
> > Cc: Michal Suchanek <msuchanek@suse.de>
> > Cc: Simon Glass <sjg@chromium.org>
> > Cc: Tobias Waldekranz <tobias@waldekranz.com>
> > ---
> >  drivers/block/blk-uclass.c | 2 ++
> >  include/blk.h              | 2 --
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Unfortunately this does not stop people using the ops member directly.
> 
> For this series, I tried a patch myself [1] but I think I stuffed it
> up. So I will let Takahiro-San figure it out. I would very much like
> to see this clean-up go in.

At that time I thought that the necessary change was small and trivial:)

As for Marek's patch, let me first explain why I implement that way,
i.e. separating disk_blk_*() from part_disk_*():
- Initially I tried to implement disk_blk_*() work for both UCLASS_BLOCK
  and UCLASS_PARTITION, while this idea was rejected by Simon.
- Then, I implemented part_disk_*() with direct access to the devices,
  and part_disk_*(), as helper functions, with block caching.
  I thought that this approach was aligned with the implementation of
  block devices (blk_[read|wirte]).

If you don't think the second point makes sense, I can agree to Marek's
approach.

Thanks,
-Takahiro Akashi


> Regards,
> Simon
> 
> [1] https://patchwork.ozlabs.org/project/uboot/patch/20230319192957.1084530-1-sjg@chromium.org/