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

[Heiko Schocher]

Hello Samuel,

On 22.08.21 19:19, Samuel Holland wrote:
> 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 at 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.

Yes, and you add "drivers/i2c/sun6i_p2wi.c", and nowhere code is removed,
which is enabled by this config symbol... was the config symbol not used
before?

Ah, I see, you use code from arch/arm/mach-sunxi/p2wi.c...

Shouldn;t this be moved than to drivers/i2c too? Only i2c driver use
this functions ... or?

(Hope it is not to dummy question...)

> 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.

Hmm.. better to move the whole code to dirvers/i2c ?

> 
>>>  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.

we sort by config symbol.

bye,
Heiko
> 
> 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 at denx.de>
>>
>> Thanks!
>>
>> bye,
>> Heiko
>>

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de