Hi,
On Mon, 3 Jul 2023 at 01:57, AKASHI Takahiro <takahiro.aka...@linaro.org> wrote:
>
> On Thu, Jun 29, 2023 at 08:09:58PM +0100, Simon Glass wrote:
> > Hi AKASHI,
> >
> > On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
> > <takahiro.aka...@linaro.org> wrote:
> > >
> > > Added is a new unit test for SCMI base protocol, which will exercise all
> > > the commands provided by the protocol, except SCMI_BASE_NOTIFY_ERRORS.
> > > $ ut dm scmi_base
> > > It is assumed that test.dtb is used as sandbox's device tree.
> > >
> > > Signed-off-by: AKASHI Takahiro <takahiro.aka...@linaro.org>
> > > ---
> > > test/dm/scmi.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 112 insertions(+)
> > >
> > > diff --git a/test/dm/scmi.c b/test/dm/scmi.c
> > > index 881be3171b7c..563017bb63e0 100644
> > > --- a/test/dm/scmi.c
> > > +++ b/test/dm/scmi.c
> > > @@ -16,6 +16,9 @@
> > > #include <clk.h>
> > > #include <dm.h>
> > > #include <reset.h>
> > > +#include <scmi_agent.h>
> > > +#include <scmi_agent-uclass.h>
> > > +#include <scmi_protocols.h>
> > > #include <asm/scmi_test.h>
> > > #include <dm/device-internal.h>
> > > #include <dm/test.h>
> > > @@ -95,6 +98,115 @@ static int dm_test_scmi_sandbox_agent(struct
> > > unit_test_state *uts)
> > > }
> > > DM_TEST(dm_test_scmi_sandbox_agent, UT_TESTF_SCAN_FDT);
> > >
> > > +static int dm_test_scmi_base(struct unit_test_state *uts)
> > > +{
> > > + struct udevice *agent_dev, *base;
> > > + struct scmi_agent_priv *priv;
> > > + const struct scmi_base_ops *ops;
> > > + u32 version, num_agents, num_protocols, impl_version;
> > > + u32 attributes, agent_id;
> > > + char vendor[SCMI_BASE_NAME_LENGTH_MAX],
> > > + agent_name[SCMI_BASE_NAME_LENGTH_MAX];
> > > + u8 *protocols;
> > > + int ret;
> > > +
> > > + /* preparation */
> > > + ut_assertok(uclass_get_device_by_name(UCLASS_SCMI_AGENT, "scmi",
> > > + &agent_dev));
> > > + ut_assertnonnull(agent_dev);
> > > + ut_assertnonnull(priv = dev_get_uclass_plat(agent_dev));
> > > + ut_assertnonnull(base = scmi_get_protocol(agent_dev,
> > > + SCMI_PROTOCOL_ID_BASE));
> > > + ut_assertnonnull(ops = dev_get_driver_ops(base));
> > > +
> > > + /* version */
> > > + ret = (*ops->protocol_version)(base, &version);
> >
> > Can you add uclass helpers to call each of the methods? That is how it
> > is commonly done. You should not be calling ops->xxx directly here.
>
> Yes, I will add inline functions instead.
I don't mean inline...see all the other uclasses which define a
function which is implemented in the uclass. It is confusing when one
uclass does something different. People might copy this style and then
the code base diverges. Did you not notice this when looking around
the source tree?
Regards,
Simon
