Re: [PATCH 01/11] i2c: Add a DM_I2C wrapper for the sun6i_p2wi controller

Hi Heiko,

On 8/22/21 3:38 AM, Heiko Schocher wrote:

Hello Samuel,

On 22.08.21 01:05, Samuel Holland wrote:

...

This bus controller is used to communicate with an X-Powers AXP PMIC. Currently, various drivers access PMIC registers through a platform- specific non-DM "pmic_bus" interface, which depends on the legacy I2C framework. In order to convert those drivers to use DM_PMIC, this bus needs a DM_I2C driver.

Since the non-DM bus controller driver is still needed in SPL, the quick solution is to implement the DM_I2C ops using the existing functions.

The register for switching between I2C/P2WI/RSB mode is the same across all PMIC variants, so move that to the common header.

Signed-off-by: Samuel Holland samuel@sholland.org

arch/arm/mach-sunxi/Kconfig | 11 ------ arch/arm/mach-sunxi/pmic_bus.c | 7 ++-- drivers/i2c/Kconfig | 8 +++++ drivers/i2c/Makefile | 1 + drivers/i2c/sun6i_p2wi.c | 66 ++++++++++++++++++++++++++++++++++

I wonder, as this config symbol gets also removed, that there is no remove of driver code?

I did not remove any config symbol, only moved it to a different file. It sounds like you want me to use a new symbol for the DM_I2C driver (SYS_I2C_SUN6I_P2WI). So in that case, I would leave the old symbol alone, and have SYS_I2C_SUN6I_P2WI select it.

...

include/axp_pmic.h | 6 ++++ 6 files changed, 84 insertions(+), 15 deletions(-) create mode 100644 drivers/i2c/sun6i_p2wi.c

diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig index 79c669a4813..37076c2dfb3 100644 --- a/arch/arm/mach-sunxi/Kconfig +++ b/arch/arm/mach-sunxi/Kconfig @@ -88,17 +88,6 @@ config DRAM_SUN50I_H616_UNKNOWN_FEATURE feature. endif

-config SUN6I_P2WI

• bool "Allwinner sun6i internal P2WI controller"
• help

•  If you say yes to this option, support will be included for the
  

•  P2WI (Push/Pull 2 Wire Interface) controller embedded in some sunxi
  

•  SOCs.
  

•  The P2WI looks like an SMBus controller (which supports only byte
  

•  accesses), except that it only supports one slave device.
  

•  This interface is used to connect to specific PMIC devices (like the
  

•  AXP221).
  

config SUN6I_PRCM bool help diff --git a/arch/arm/mach-sunxi/pmic_bus.c b/arch/arm/mach-sunxi/pmic_bus.c index 0394ce85644..673a05fdd16 100644 --- a/arch/arm/mach-sunxi/pmic_bus.c +++ b/arch/arm/mach-sunxi/pmic_bus.c @@ -8,6 +8,7 @@

• axp223 uses the rsb bus, these functions abstract this.

*/

+#include <axp_pmic.h> #include <common.h> #include <asm/arch/p2wi.h> #include <asm/arch/rsb.h> @@ -21,8 +22,6 @@ #define AXP305_I2C_ADDR 0x36

#define AXP221_CHIP_ADDR 0x68 -#define AXP221_CTRL_ADDR 0x3e -#define AXP221_INIT_DATA 0x3e

/* AXP818 device and runtime addresses are same as AXP223 */ #define AXP223_DEVICE_ADDR 0x3a3 @@ -40,8 +39,8 @@ int pmic_bus_init(void) #if defined CONFIG_AXP221_POWER || defined CONFIG_AXP809_POWER || defined CONFIG_AXP818_POWER # ifdef CONFIG_MACH_SUN6I p2wi_init();

• ret = p2wi_change_to_p2wi_mode(AXP221_CHIP_ADDR, AXP221_CTRL_ADDR,

• 		       AXP221_INIT_DATA);
  

• ret = p2wi_change_to_p2wi_mode(AXP221_CHIP_ADDR, AXP_PMIC_MODE_REG,

• 		       AXP_PMIC_MODE_P2WI);
  

# elif defined CONFIG_MACH_SUN8I_R40 /* Nothing. R40 uses the AXP221s in I2C mode */ ret = 0; diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig index 5d27f503bfc..d082676c4b2 100644 --- a/drivers/i2c/Kconfig +++ b/drivers/i2c/Kconfig @@ -577,6 +577,14 @@ config SYS_I2C_STM32F7 _ Optional clock stretching _ Software reset

+config SUN6I_P2WI

Could you please use "SYS_I2C_" ?

...

• bool "Allwinner sun6i P2WI controller"
• depends on ARCH_SUNXI
• help

•  Support for the P2WI (Push/Pull 2 Wire Interface) controller embedded
  

•  in the Allwinner A31 and A31s SOCs. This interface is used to connect
  

•  to specific devices like the X-Powers AXP221 PMIC.
  

config SYNQUACER bool "Socionext SynQuacer I2C controller" depends on ARCH_SYNQUACER && DM_I2C diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile index 3a7ecd9274b..2461f0a2db8 100644 --- a/drivers/i2c/Makefile +++ b/drivers/i2c/Makefile @@ -43,6 +43,7 @@ obj-$(CONFIG_SYS_I2C_SANDBOX) += sandbox_i2c.o i2c-emul-uclass.o obj-$(CONFIG_SYS_I2C_SH) += sh_i2c.o obj-$(CONFIG_SYS_I2C_SOFT) += soft_i2c.o obj-$(CONFIG_SYS_I2C_STM32F7) += stm32f7_i2c.o +obj-$(CONFIG_SUN6I_P2WI) += sun6i_p2wi.o

please sort alphabetical.

I was sorting by file name -- this will be fixed by renaming the config symbol.

Regards, Samuel

...

obj-$(CONFIG_SYS_I2C_SYNQUACER) += synquacer_i2c.o obj-$(CONFIG_SYS_I2C_TEGRA) += tegra_i2c.o obj-$(CONFIG_SYS_I2C_UNIPHIER) += i2c-uniphier.o diff --git a/drivers/i2c/sun6i_p2wi.c b/drivers/i2c/sun6i_p2wi.c new file mode 100644 index 00000000000..f1e8e5ed107 --- /dev/null +++ b/drivers/i2c/sun6i_p2wi.c @@ -0,0 +1,66 @@ +// SPDX-License-Identifier: GPL-2.0+

+#include <axp_pmic.h> +#include <dm.h> +#include <i2c.h> +#include <asm/arch/p2wi.h>

+#if CONFIG_IS_ENABLED(DM_I2C)

+static int sun6i_p2wi_xfer(struct udevice *bus, struct i2c_msg *msg, int nmsgs) +{

• /* The hardware only supports SMBus-style transfers. */
• if (nmsgs == 2 && msg[1].flags == I2C_M_RD && msg[1].len == 1)

• return p2wi_read(msg[0].buf[0], msg[1].buf);
  

• if (nmsgs == 1 && msg[0].len == 2)

• return p2wi_write(msg[0].buf[0], msg[0].buf[1]);
  

• return -EINVAL;

+}

+static int sun6i_p2wi_probe_chip(struct udevice *bus, uint chip_addr,

• 		 uint chip_flags)
  

+{

• return p2wi_change_to_p2wi_mode(chip_addr,

• 			AXP_PMIC_MODE_REG,
  

• 			AXP_PMIC_MODE_P2WI);
  

+}

+static int sun6i_p2wi_probe(struct udevice *bus) +{

• p2wi_init();
• return 0;

+}

+static int sun6i_p2wi_child_pre_probe(struct udevice *child) +{

• struct dm_i2c_chip *chip = dev_get_parent_plat(child);
• /* Ensure each transfer is for a single register. */
• chip->flags |= DM_I2C_CHIP_RD_ADDRESS | DM_I2C_CHIP_WR_ADDRESS;
• return 0;

+}

+static const struct dm_i2c_ops sun6i_p2wi_ops = {

• .xfer = sun6i_p2wi_xfer,
• .probe_chip = sun6i_p2wi_probe_chip,

+};

+static const struct udevice_id sun6i_p2wi_ids[] = {

• { .compatible = "allwinner,sun6i-a31-p2wi" },
• { /* sentinel */ }

+};

+U_BOOT_DRIVER(sun6i_p2wi) = {

• .name = "sun6i_p2wi",
• .id = UCLASS_I2C,
• .of_match = sun6i_p2wi_ids,
• .probe = sun6i_p2wi_probe,
• .child_pre_probe = sun6i_p2wi_child_pre_probe,
• .ops = &sun6i_p2wi_ops,

+};

+#endif /* CONFIG_IS_ENABLED(DM_I2C) */ diff --git a/include/axp_pmic.h b/include/axp_pmic.h index 405044c3a32..0db3e143eda 100644 --- a/include/axp_pmic.h +++ b/include/axp_pmic.h @@ -6,6 +6,8 @@ */ #ifndef _AXP_PMIC_H_

+#include <stdbool.h>

#ifdef CONFIG_AXP152_POWER #include <axp152.h> #endif @@ -25,6 +27,10 @@ #include <axp818.h> #endif

+#define AXP_PMIC_MODE_REG 0x3e +#define AXP_PMIC_MODE_I2C 0x00 +#define AXP_PMIC_MODE_P2WI 0x3e

int axp_set_dcdc1(unsigned int mvolt); int axp_set_dcdc2(unsigned int mvolt); int axp_set_dcdc3(unsigned int mvolt);

Beside of the nitpicks: Reviewed-by: Heiko Schocher hs@denx.de

Thanks!

bye, Heiko