Re: [PATCH 05/11] power: pmic: Add a driver for X-Powers AXP PMICs

[Samuel Holland <samuel@sholland.org>]

On 9/2/21 6:17 PM, Jaehoon Chung wrote:
> On 8/22/21 8:05 AM, Samuel Holland wrote:
>> These PMICs provide some combination of battery charger, fuel gauge,
>> GPIOs, regulators, and VBUS routing. These functions are represented
>> as child nodes in the device tree. Add the minimal driver needed to
>> probe these child devices and provide the DM_PMIC ops.
>>
>> Enable the driver by default for SoCs that normally pair with a PMIC.
>>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>
>>  drivers/power/pmic/Kconfig  | 14 +++++++++++
>>  drivers/power/pmic/Makefile |  1 +
>>  drivers/power/pmic/axp.c    | 49 +++++++++++++++++++++++++++++++++++++
>>  3 files changed, 64 insertions(+)
>>  create mode 100644 drivers/power/pmic/axp.c
>>
>> diff --git a/drivers/power/pmic/Kconfig b/drivers/power/pmic/Kconfig
>> index 0c4da9c5536..6a3e82e1be7 100644
>> --- a/drivers/power/pmic/Kconfig
>> +++ b/drivers/power/pmic/Kconfig
>> @@ -57,6 +57,20 @@ config PMIC_ACT8846
>>  	functions. It uses an I2C interface and is designed for use with
>>  	tablets and smartphones.
>>  
>> +config PMIC_AXP
>> +	bool "Enable Driver Model for X-Powers AXP PMICs"
>> +	depends on DM_I2C
>> +	help
>> +	  This config enables driver-model PMIC uclass features for
>> +	  X-Powers AXP152, AXP2xx, and AXP8xx PMICs.
>> +
>> +config SPL_PMIC_AXP
>> +	bool "Enable Driver Model for X-Powers AXP PMICs in SPL"
>> +	depends on SPL_DM_I2C && SPL_DM_PMIC
>> +	help
>> +	  This config enables driver-model PMIC uclass features in the SPL for
>> +	  X-Powers AXP152, AXP2xx, and AXP8xx PMICs.
>> +
>>  config DM_PMIC_DA9063
>>  	bool "Enable Driver Model for the Dialog DA9063 PMIC"
>>  	help
>> diff --git a/drivers/power/pmic/Makefile b/drivers/power/pmic/Makefile
>> index 6e40c0971fb..4021be8e801 100644
>> --- a/drivers/power/pmic/Makefile
>> +++ b/drivers/power/pmic/Makefile
>> @@ -18,6 +18,7 @@ obj-$(CONFIG_DM_PMIC_SANDBOX) += sandbox.o i2c_pmic_emul.o
>>  obj-$(CONFIG_PMIC_AB8500) += ab8500.o
>>  obj-$(CONFIG_PMIC_ACT8846) += act8846.o
>>  obj-$(CONFIG_PMIC_AS3722) += as3722.o as3722_gpio.o
>> +obj-$(CONFIG_$(SPL_)PMIC_AXP) += axp.o
>>  obj-$(CONFIG_PMIC_MAX8997) += max8997.o
>>  obj-$(CONFIG_PMIC_PM8916) += pm8916.o
>>  obj-$(CONFIG_PMIC_RK8XX) += rk8xx.o
>> diff --git a/drivers/power/pmic/axp.c b/drivers/power/pmic/axp.c
>> new file mode 100644
>> index 00000000000..7720e1afd9b
>> --- /dev/null
>> +++ b/drivers/power/pmic/axp.c
>> @@ -0,0 +1,49 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +
>> +#include <dm.h>
>> +#include <i2c.h>
>> +#include <power/pmic.h>
>> +
>> +static int axp_pmic_reg_count(struct udevice *dev)
>> +{
>> +	/* TODO: Get the specific value from driver data. */
>> +	return 0x100;
>> +}
>> +
>> +static struct dm_pmic_ops axp_pmic_ops = {
>> +	.reg_count	= axp_pmic_reg_count,
>> +	.read		= dm_i2c_read,
>> +	.write		= dm_i2c_write,
>> +};
>> +
>> +static int axp_pmic_bind(struct udevice *dev)
>> +{
>> +	int ret;
>> +
>> +	ret = dm_scan_fdt_dev(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
> 
> return dm_scan_fdt_dev(); ?
> 
> or { .bind = dm_scan_fdt_dev, } ?

The PMIC will eventually need to do more in this function, including
binding a sysreset device[1] and regulators, so this minimized the diff.
I do like the idea of .bind = dm_scan_fdt_dev, so I will plan to do that
for v2.

Regards,
Samuel

[1]:
https://patchwork.ozlabs.org/project/uboot/patch/20210822231807.3330-5-samuel@sholland.org/

> Best Regards,
> Jaehoon Chung
> 
>> +}
>> +
>> +static const struct udevice_id axp_pmic_ids[] = {
>> +	{ .compatible = "x-powers,axp152" },
>> +	{ .compatible = "x-powers,axp202" },
>> +	{ .compatible = "x-powers,axp209" },
>> +	{ .compatible = "x-powers,axp221" },
>> +	{ .compatible = "x-powers,axp223" },
>> +	{ .compatible = "x-powers,axp803" },
>> +	{ .compatible = "x-powers,axp806" },
>> +	{ .compatible = "x-powers,axp809" },
>> +	{ .compatible = "x-powers,axp813" },
>> +	{ }
>> +};
>> +
>> +U_BOOT_DRIVER(axp_pmic) = {
>> +	.name		= "axp_pmic",
>> +	.id		= UCLASS_PMIC,
>> +	.of_match	= axp_pmic_ids,
>> +	.bind		= axp_pmic_bind,
>> +	.ops		= &axp_pmic_ops,
>> +};
>>
>