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 <sam...@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 <h...@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: h...@denx.de
