[U-Boot] [PATCH 04/11] dm: test: Add tests for the generic PHY uclass

[Simon Glass <sjg@chromium.org>]

Hi Jean-Jacques,

On 14 April 2017 at 05:08, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
> Those tests check:
> - the ability for a phy-user to get a phy device a reference in the
>   device-tree
> - the ability to perform operations on the phy (init,deinit,on,off)
> - the behavior of the uclass when optional operations are not implemented
>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> ---
>  arch/sandbox/dts/test.dts       |  9 +++++
>  configs/sandbox_defconfig       |  2 +
>  configs/sandbox_noblk_defconfig |  2 +
>  configs/sandbox_spl_defconfig   |  3 ++
>  drivers/phy/Kconfig             |  9 +++++
>  drivers/phy/Makefile            |  1 +
>  drivers/phy/sandbox-phy.c       | 78 ++++++++++++++++++++++++++++++++++++
>  test/dm/Makefile                |  1 +
>  test/dm/generic_phy.c           | 89 +++++++++++++++++++++++++++++++++++++++++
>  9 files changed, 194 insertions(+)
>  create mode 100644 drivers/phy/sandbox-phy.c
>  create mode 100644 test/dm/generic_phy.c

This looks really good, just some nits.

>
> diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
> index fff175d..918c899 100644
> --- a/arch/sandbox/dts/test.dts
> +++ b/arch/sandbox/dts/test.dts
> @@ -59,6 +59,15 @@
>                 ping-add = <3>;
>         };
>
> +       gen_phy: gen_phy {
> +               compatible = "sandbox,phy";
> +       };
> +
> +       gen_phy_user: gen_phy_user {
> +               compatible = "simple-bus";
> +               phy = <&gen_phy>;
> +       };
> +
>         some-bus {
>                 #address-cells = <1>;
>                 #size-cells = <0>;
> diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
> index 7f3f5ac..42116cf 100644
> --- a/configs/sandbox_defconfig
> +++ b/configs/sandbox_defconfig
> @@ -164,6 +164,8 @@ CONFIG_CONSOLE_ROTATION=y
>  CONFIG_CONSOLE_TRUETYPE=y
>  CONFIG_CONSOLE_TRUETYPE_CANTORAONE=y
>  CONFIG_VIDEO_SANDBOX_SDL=y
> +CONFIG_GENERIC_PHY=y
> +CONFIG_PHY_SANDBOX=y
>  CONFIG_CMD_DHRYSTONE=y
>  CONFIG_TPM=y
>  CONFIG_LZ4=y
> diff --git a/configs/sandbox_noblk_defconfig b/configs/sandbox_noblk_defconfig
> index 3f8e70d..7a5cd4b 100644
> --- a/configs/sandbox_noblk_defconfig
> +++ b/configs/sandbox_noblk_defconfig
> @@ -166,6 +166,8 @@ CONFIG_CONSOLE_ROTATION=y
>  CONFIG_CONSOLE_TRUETYPE=y
>  CONFIG_CONSOLE_TRUETYPE_CANTORAONE=y
>  CONFIG_VIDEO_SANDBOX_SDL=y
> +CONFIG_GENERIC_PHY=y
> +CONFIG_PHY_SANDBOX=y
>  CONFIG_CMD_DHRYSTONE=y
>  CONFIG_TPM=y
>  CONFIG_LZ4=y
> diff --git a/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig
> index ade6714..9b4cf39 100644
> --- a/configs/sandbox_spl_defconfig
> +++ b/configs/sandbox_spl_defconfig
> @@ -170,6 +170,9 @@ CONFIG_CONSOLE_ROTATION=y
>  CONFIG_CONSOLE_TRUETYPE=y
>  CONFIG_CONSOLE_TRUETYPE_CANTORAONE=y
>  CONFIG_VIDEO_SANDBOX_SDL=y
> +CONFIG_GENERIC_PHY=y
> +CONFIG_SPL_GENERIC_PHY=y
> +CONFIG_PHY_SANDBOX=y
>  CONFIG_CMD_DHRYSTONE=y
>  CONFIG_TPM=y
>  CONFIG_LZ4=y
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index d66c9e3..032b932 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -29,4 +29,13 @@ config SPL_GENERIC_PHY
>           compatible as possible with the equivalent framework found in the
>           linux kernel.
>
> +config PHY_SANDBOX
> +       bool "Sandbox PHY support"
> +       depends on SANDBOX
> +       depends on GENERIC_PHY
> +       help
> +         This select a dummy sandbox PHY driver. It used only to implement
> +         unit tests for the generic phy framework
> +
> +
>  endmenu
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index b29a8b9..0125844 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -1,2 +1,3 @@
>  obj-$(CONFIG_$(SPL_)GENERIC_PHY) += phy-uclass.o
> +obj-$(CONFIG_PHY_SANDBOX) += sandbox-phy.o
>
> diff --git a/drivers/phy/sandbox-phy.c b/drivers/phy/sandbox-phy.c
> new file mode 100644
> index 0000000..1c60308
> --- /dev/null
> +++ b/drivers/phy/sandbox-phy.c
> @@ -0,0 +1,78 @@
> +/*
> + * Copyright (C) 2017 Texas Instruments Incorporated - http://www.ti.com/
> + * Written by Jean-Jacques Hiblot  <jjhiblot@ti.com>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <generic-phy.h>
> +
> +struct sandbox_phy_priv {
> +       int initialized;
> +       int on;

bool?

> +};
> +
> +static int sandbox_phy_power_on(struct udevice *dev)
> +{
> +       struct sandbox_phy_priv *priv = dev_get_priv(dev);

blank line between decls and rest of code
> +       if (!priv->initialized)
> +               return -EIO;
> +       priv->on = 1;

blank line before return
true/false better than 0/1 I think

> +       return 0;
> +}
> +
> +static int sandbox_phy_power_off(struct udevice *dev)
> +{
> +       struct sandbox_phy_priv *priv = dev_get_priv(dev);
> +       if (!priv->initialized)
> +               return -EIO;
> +       priv->on = 0;
> +       return 0;
> +}
> +
> +static int sandbox_phy_init(struct udevice *dev)
> +{
> +       struct sandbox_phy_priv *priv = dev_get_priv(dev);
> +       priv->initialized = 1;
> +       priv->on = 0;
> +       return 0;
> +}
> +
> +static int sandbox_phy_exit(struct udevice *dev)
> +{
> +       struct sandbox_phy_priv *priv = dev_get_priv(dev);
> +       priv->initialized = 0;
> +       priv->on = 0;
> +       return 0;
> +}
> +
> +static int sandbox_phy_probe(struct udevice *dev)
> +{
> +       struct sandbox_phy_priv *priv = dev_get_priv(dev);
> +       priv->initialized = 0;
> +       priv->on = 0;
> +       return 0;
> +}
> +
> +static struct generic_phy_ops sandbox_phy_ops = {
> +       .power_on = sandbox_phy_power_on,
> +       .power_off = sandbox_phy_power_off,
> +       .init = sandbox_phy_init,
> +       .exit = sandbox_phy_exit,
> +};
> +
> +static const struct udevice_id sandbox_phy_ids[] = {
> +       { .compatible = "sandbox,phy" },
> +       { }
> +};
> +
> +U_BOOT_DRIVER(phy_sandbox) = {
> +       .name           = "phy_sandbox",
> +       .id             = UCLASS_PHY,
> +       .of_match       = sandbox_phy_ids,
> +       .ops            = &sandbox_phy_ops,
> +       .probe          = sandbox_phy_probe,
> +       .priv_auto_alloc_size = sizeof(struct sandbox_phy_priv),
> +};
> diff --git a/test/dm/Makefile b/test/dm/Makefile
> index 1885e17..e2b7d43 100644
> --- a/test/dm/Makefile
> +++ b/test/dm/Makefile
> @@ -41,4 +41,5 @@ obj-$(CONFIG_TIMER) += timer.o
>  obj-$(CONFIG_DM_VIDEO) += video.o
>  obj-$(CONFIG_ADC) += adc.o
>  obj-$(CONFIG_SPMI) += spmi.o
> +obj-$(CONFIG_GENERIC_PHY) += generic_phy.o

check ordering

>  endif
> diff --git a/test/dm/generic_phy.c b/test/dm/generic_phy.c
> new file mode 100644
> index 0000000..08b862a
> --- /dev/null
> +++ b/test/dm/generic_phy.c
> @@ -0,0 +1,89 @@
> +/*
> + * Copyright (C) 2017 Texas Instruments Incorporated - http://www.ti.com/
> + * Written by Jean-Jacques Hiblot  <jjhiblot@ti.com>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <dm/test.h>
> +#include <test/ut.h>
> +#include <generic-phy.h>

This should go below dm.h

> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +/* Base test of the phy uclass */
> +static int dm_test_phy_base(struct unit_test_state *uts)
> +{
> +       struct udevice *dev_method1;
> +       struct udevice *dev_method2;
> +       struct udevice *parent;
> +
> +

drop extra blank ilne

> +       /* Get the device using the phy device*/
> +       ut_assertok(uclass_get_device_by_name(UCLASS_SIMPLE_BUS,
> +                                             "gen_phy_user", &parent));
> +       /* Get the phy device with std uclass function */
> +       ut_assertok(uclass_get_device_by_name(UCLASS_PHY, "gen_phy",
> +                                             &dev_method1));
> +
> +       /*
> +        * Get the phy device from user device and compare with the one
> +        * obtained with the previous method.
> +        */
> +       dev_method2 = dm_generic_phy_get(parent, "phy");

This should be able to use ut_assertok() once the function returns an
error code.

> +       ut_assertnonnull(dev_method2);
> +       ut_assertok_ptr(dev_method2);
> +       ut_asserteq_ptr(dev_method1, dev_method2);
> +
> +       /* Try to get a non-existing phy */
> +       ut_asserteq(-ENODEV, uclass_get_device(UCLASS_PHY, 1, &dev_method2));
> +       dev_method2 = dm_generic_phy_get(parent, "phy_not_existing");
> +       ut_assert(IS_ERR_OR_NULL(dev_method2));
> +
> +       return 0;
> +}
> +DM_TEST(dm_test_phy_base, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
> +
> +/* Test of the phy uclass using the sandbox phy driver operations */
> +static int dm_test_phy_ops(struct unit_test_state *uts)
> +{
> +       struct udevice *dev;
> +       struct udevice *parent;
> +
> +       ut_assertok(uclass_get_device_by_name(UCLASS_SIMPLE_BUS,
> +                                             "gen_phy_user", &parent));
> +       dev = dm_generic_phy_get(parent, "phy");
> +       ut_assertnonnull(dev);
> +       ut_assertok_ptr(dev);
> +
> +       /* test normal operations */
> +       ut_assertok(generic_phy_init(dev));
> +       ut_assertok(generic_phy_power_on(dev));
> +       ut_assertok(generic_phy_power_off(dev));
> +
> +       /*
> +        * test operations after exit().
> +        * The sandbox phy driver does not allow it.
> +        */
> +       ut_assertok(generic_phy_exit(dev));
> +       ut_assert(generic_phy_power_on(dev) != 0);
> +       ut_assert(generic_phy_power_off(dev) != 0);
> +
> +       /*
> +        * test normal operations again (after re-init)
> +        */
> +       ut_assertok(generic_phy_init(dev));
> +       ut_assertok(generic_phy_power_on(dev));
> +       ut_assertok(generic_phy_power_off(dev));
> +
> +       /*
> +        * test calling unimplemented feature.
> +        * The call is expected to succeed
> +        */
> +       ut_assertok(generic_phy_reset(dev));
> +
> +       return 0;
> +}
> +DM_TEST(dm_test_phy_ops, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
> --
> 1.9.1
>

Great test, thanks.

Regards,
Simon