[U-Boot] [PATCH v2 1/6] dm: SMEM (Shared memory) uclass

[Ramon Fried]

On Wed, Jun 27, 2018 at 2:18 AM Simon Glass <sjg at chromium.org> wrote:

> HI Ramon,
>
> On 26 June 2018 at 04:15, Ramon Fried <ramon.fried at gmail.com> wrote:
> >
> >
> > On Tue, Jun 26, 2018 at 6:59 AM Simon Glass <sjg at chromium.org> wrote:
> >>
> >> Hi Ramon,
> >>
> >> On 21 June 2018 at 20:11, Ramon Fried <ramon.fried at gmail.com> wrote:
> >> > This is a uclass for Shared memory manager drivers.
> >> >
> >> > A Shared Memory Manager driver implements an interface for allocating
> >> > and accessing items in the memory area shared among all of the
> >> > processors.
> >> >
> >> > Signed-off-by: Ramon Fried <ramon.fried at gmail.com>
> >> >
> >> > ---
> >> >
> >> > Changes in v2:
> >> > (As suggested by Simon Glass)
> >> > - Introduced a new dm class (CLASS_SMEM) instead of CLASS_SOC.
> >> > - Added sandbox driver
> >> > - Added testing for DM class.
> >> >
> >> >  drivers/Makefile           |  1 +
> >> >  drivers/smem/Kconfig       |  2 +
> >> >  drivers/smem/Makefile      |  5 +++
> >> >  drivers/smem/smem-uclass.c | 53 ++++++++++++++++++++++++
> >> >  include/dm/uclass-id.h     |  1 +
> >> >  include/smem.h             | 84
> ++++++++++++++++++++++++++++++++++++++
> >> >  6 files changed, 146 insertions(+)
> >> >  create mode 100644 drivers/smem/Kconfig
> >> >  create mode 100644 drivers/smem/Makefile
> >> >  create mode 100644 drivers/smem/smem-uclass.c
> >> >  create mode 100644 include/smem.h
> >> >
> >>
> >> Reviewed-by: Simon Glass <sjg at chromium.org>
> >>
> >> A few nits below.
> >>
> [..]
>
> >> > +/* struct smem_ops: Operations for the SMEM uclass */
> >> > +struct smem_ops {
> >> > +       /**
> >> > +        * alloc() - allocate space for a smem item
> >> > +        *
> >> > +        * @host:       remote processor id, or -1
> >>
> >> What does -1 mean? Please expand commment
> >>
> >> > +        * @item:       smem item handle
> >>
> >> How does one choose this? What does it mean? Can you expand this
> comment a
> >> bit?
> >
> > It can be an index (like in QCOM implementation) but honestly, it can be
> a
> > hash as well. it's up to the implementation
> > to decide how to access the items.
>
> OK that's fine. The key thing is to document this. Your API at present
> is pretty vague which is why I am asking for more comments showing
> what is going on. Even a comment at the top of your header file with
> an example of usage would be useful.
>
> I just saw your latest v3 patch and it still seems to me that it needs
> more details. I had to read the qualcomm driver to try to understand
> how the uclass API is supposed to work.
>
> Thanks for the feedback. I'll look in to it soon.
Thanks,
Ramon.

> Regards,
> Simon
>