On Wed, Sep 28, 2022 at 08:35:58PM -0600, Simon Glass wrote:
> Hi Takahiro,
>
> On Wed, 28 Sept 2022 at 18:51, AKASHI Takahiro
> <takahiro.aka...@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > On Wed, Sep 28, 2022 at 04:20:56AM -0600, Simon Glass wrote:
> > > Hi Takahiro,
> > >
> > > On Sun, 25 Sept 2022 at 18:17, AKASHI Takahiro
> > > <takahiro.aka...@linaro.org> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Sun, Sep 25, 2022 at 09:02:17AM -0600, Simon Glass wrote:
> > > > > At present we have functions called blk_dread(), etc., which take a
> > > > > struct blk_desc * to refer to the block device. Add some functions
> > > > > which
> > > > > use udevice instead, since this is more in keeping with how driver
> > > > > model
> > > > > is supposed to work.
> > > >
> > > > Unfortunately, NAK.
> > > > I have already added similar functions in disk/disk-uclass.c
> > > > with my commit 59da9d4782cd ("dm: disk: add read/write interfaces with
> > > > udevice"). dev_read()/dev_write() works well with UCLASS_BLK (as
> > > > intended).
> > > >
> > > > I remember that you also ack'ed that patch.
> > >
> > > You have a better memory than me!
> > >
> > > How about we make those functions call my new ones?
> >
> > So do you want to have two distinct API's for BLK and PARTITION?
> > You seem to have a policy that each DM class should have its own
> > API's.
>
> Yes that's right.
>
> >
> > I don't have a strong opinion here, but form user's point of view,
> > block-level accessing has no difference between BLK and PARTITION.
> > Most common users of such accessors are file systems.
> > In stead of using the following code at every corner,
> > cid = device_get_uclass_id(udev);
> > if (cid == UCLASS_BLK)
> > blk_read(udev, ...);
> > else if (cid == UCLASS_PARTITION)
> > disk_part(udev, ...);
-> I meant disk_read(...)
> > it may be more convenient to have a single API.
>
> Yes I agree, but you already have that with your dev_read() API. I
> just want to rename it a bit.
If so, that's fine to me.
-Takahiro Akashi
> >
> > > Also I think we should rename your functions to avoid using the
> > > dev_read prefix, since this is for reading from the device tree.
> >
> > Okay.
> >
> > > Perhaps disk_read()? Also it seems that we could rationalise the code
> > > between disk_read() and part_read() ?
> >
> > I'm not sure what you mean by "rationalise".
>
> The code seems to be almost the same between the two so I think it may
> be possible to move the common code into a shared function.
>
> >
> > > Also should have comments in the
> > > header file about what the functions do (and what type of device they
> > > accept).
> >
> > Yeah.
>
> Regards,
> Simon
