On 9/24/26 11:02 AM, Arnaud Ferraris wrote: > This driver supports basic operations (get/set voltage, enable/disable) > for regulators found in the MediaTek MT6373 PMIC.
I saw in [1] that VBUCK4 gets special handling. Do we need that in U-Boot? [1]: https://lore.kernel.org/all/[email protected]/ > > Signed-off-by: Arnaud Ferraris <[email protected]> > --- > drivers/power/regulator/Kconfig | 9 + > drivers/power/regulator/Makefile | 1 + > drivers/power/regulator/mt6373_regulator.c | 797 > +++++++++++++++++++++++++++++ > 3 files changed, 807 insertions(+) > > diff --git a/drivers/power/regulator/Kconfig b/drivers/power/regulator/Kconfig > index 428689702c8..b529285cfd7 100644 > --- a/drivers/power/regulator/Kconfig > +++ b/drivers/power/regulator/Kconfig > @@ -581,6 +581,15 @@ config DM_REGULATOR_MT6359 > This driver supports the control of different power rails of device > through regulator interface. > > +config DM_REGULATOR_MT6373 > + bool "Enable driver for MediaTek MT6373 PMIC regulators" > + depends on DM_REGULATOR && PMIC_MTK Did you mean DM_PMIC_MTK_SPMI instead of PMIC_MTK? > + help > + Say y here to select this option to enable the power regulator of > + MediaTek MT6373 PMIC. > + This driver supports the control of different power rails of device > + through regulator interface. > + > config DM_REGULATOR_PMBUS_HELPER > bool "Shared regulator helpers for PMBus chip drivers" > depends on DM_REGULATOR && PMBUS && DM_I2C > diff --git a/drivers/power/regulator/Makefile > b/drivers/power/regulator/Makefile > index e17937f4c26..ce8b465afc4 100644 > --- a/drivers/power/regulator/Makefile > +++ b/drivers/power/regulator/Makefile > @@ -13,6 +13,7 @@ obj-$(CONFIG_$(PHASE_)REGULATOR_AXP_USB_POWER) += > axp_usb_power.o > obj-$(CONFIG_$(PHASE_)DM_REGULATOR_DA9063) += da9063.o > obj-$(CONFIG_$(PHASE_)DM_REGULATOR_MAX77663) += max77663_regulator.o > obj-$(CONFIG_$(PHASE_)DM_REGULATOR_MAX8907) += max8907_regulator.o > +obj-$(CONFIG_$(PHASE_)DM_REGULATOR_MT6373) += mt6373_regulator.o > obj-$(CONFIG_DM_REGULATOR_MAX77686) += max77686.o > obj-$(CONFIG_DM_REGULATOR_NPCM8XX) += npcm8xx_regulator.o > obj-$(CONFIG_$(PHASE_)DM_REGULATOR_PFUZE100) += pfuze100.o > diff --git a/drivers/power/regulator/mt6373_regulator.c > b/drivers/power/regulator/mt6373_regulator.c > new file mode 100644 > index 00000000000..7927a620fb6 > --- /dev/null > +++ b/drivers/power/regulator/mt6373_regulator.c > @@ -0,0 +1,797 @@ > +// SPDX-License-Identifier: GPL-2.0 Prefer GPL-2.0-only or GPL-2.0-or-later > +/* > + * Copyright (c) 2024 MediaTek Inc. > + * Copyright (c) 2025-2026 Collabora Ltd > + * Authors: AngeloGioacchino Del Regno > <[email protected]> > + * Arnaud Ferraris <[email protected]> > + */ > + > +#include <dm/device_compat.h> > +#include <dm/device-internal.h> > +#include <dm/device.h> > +#include <dm/devres.h> > +#include <dm/lists.h> > +#include <linux/bitfield.h> > +#include <linux/delay.h> > +#include <linux/err.h> > +#include <log.h> > +#include <power/pmic.h> > +#include <power/regulator.h> Headers could use some work here to include only what we use (and directly rather than indirectly). > + > +/** > + * struct linear_range - table of selector - value pairs > + * > + * Define a lookup-table for range of values. Intended to help when looking > + * for a register value matching certaing physical measure (like voltage). > + * Usable when increment of one in register always results a constant > increment > + * of the physical measure (like voltage). > + * > + * @min: Lowest value in range > + * @min_sel: Lowest selector for range > + * @max_sel: Highest selector for range > + * @step: Value step size > + */ > +struct linear_range { > + unsigned int min; > + unsigned int min_sel; > + unsigned int max_sel; It looks like this is not being used as much as it should. min_sel is never read and max_sel is only read in one case. > + unsigned int step; > +}; > + > +/* Initialize struct linear_range for regulators */ > +#define REGULATOR_LINEAR_RANGE(_min_uV, _min_sel, _max_sel, _step_uV) > \ > +{ \ > + .min = _min_uV, \ > + .min_sel = _min_sel, \ > + .max_sel = _max_sel, \ > + .step = _step_uV, \ > +} > + ... > +#define MT6373_PMIC_RG_LDO_VAUD18_ADDR 0x1b57 > +#define MT6373_PMIC_RG_LDO_VUSB_ADDR 0x1b65 > +#define MT6373_PMIC_RG_LDO_VAUX18_ADDR 0x1b73 > +#define MT6373_PMIC_RG_LDO_VRF13_AIF_ADDR 0x1b81 > +#define MT6373_PMIC_RG_LDO_VRF18_AIF_ADDR 0x1b8f > +#define MT6373_PMIC_RG_LDO_VRFIO18_AIF_ADDR 0x1b9d > +#define MT6373_PMIC_RG_LDO_VCN33_1_ADDR 0x1bd7 > +#define MT6373_PMIC_RG_LDO_VCN33_2_ADDR 0x1be5 > +#define MT6373_PMIC_RG_LDO_VCN33_3_ADDR 0x1bf3 > +#define MT6373_PMIC_RG_LDO_VCN18IO_ADDR 0x1c01 > +#define MT6373_PMIC_RG_LDO_VRF09_AIF_ADDR 0x1c0f > +#define MT6373_PMIC_RG_LDO_VRF12_AIF_ADDR 0x1c1d > +#define MT6373_PMIC_RG_LDO_VANT18_ADDR 0x1c57 > +#define MT6373_PMIC_RG_LDO_VEFUSE_ADDR 0x1c73 > +#define MT6373_PMIC_RG_LDO_VMCH_ADDR 0x1c81 > +#define MT6373_PMIC_RG_LDO_VMCH_EINT_ADDR 0x1c8f > +#define MT6373_PMIC_RG_LDO_VMCH_EINT_HIGH_ADDR > MT6373_PMIC_RG_LDO_VMCH_EINT_ADDR > +#define MT6373_PMIC_RG_LDO_VMCH_EINT_LOW_ADDR > MT6373_PMIC_RG_LDO_VMCH_EINT_ADDR > +#define MT6373_PMIC_RG_LDO_VMCH_EINT_POL_BIT BIT(2) > +#define MT6373_PMIC_RG_LDO_VMC_ADDR 0x1c90 > +#define MT6373_PMIC_RG_LDO_VIBR_ADDR 0x1c9e > +#define MT6373_PMIC_RG_LDO_VIO28_ADDR 0x1cd7 > +#define MT6373_PMIC_RG_LDO_VFP_ADDR 0x1ce5 > +#define MT6373_PMIC_RG_LDO_VTP_ADDR 0x1cf3 > +#define MT6373_PMIC_RG_LDO_VSIM1_ADDR 0x1d01 > +#define MT6373_PMIC_RG_LDO_VSIM2_ADDR 0x1d10 > +#define MT6373_PMIC_RG_LDO_VSIM2_LP_ADDR 0x1d10 > +#define MT6373_PMIC_RG_LDO_VSRAM_DIGRF_AIF_ADDR 0x1d57 > +#define MT6373_PMIC_RG_VAUX18_VOCAL_ADDR 0x1dd8 > +#define MT6373_PMIC_RG_VAUX18_VOSEL_ADDR 0x1dd9 > +#define MT6373_PMIC_RG_VUSB_VOCAL_ADDR 0x1ddc > +#define MT6373_PMIC_RG_VUSB_VOSEL_ADDR 0x1ddd > +#define MT6373_PMIC_RG_VCN33_1_VOCAL_ADDR 0x1de0 > +#define MT6373_PMIC_RG_VCN33_1_VOSEL_ADDR 0x1de1 > +#define MT6373_PMIC_RG_VCN33_2_VOCAL_ADDR 0x1de4 > +#define MT6373_PMIC_RG_VCN33_2_VOSEL_ADDR 0x1de5 > +#define MT6373_PMIC_RG_VCN33_3_VOCAL_ADDR 0x1de8 > +#define MT6373_PMIC_RG_VCN33_3_VOSEL_ADDR 0x1de9 > +#define MT6373_PMIC_RG_VMCH_VOCAL_ADDR 0x1dec > +#define MT6373_PMIC_RG_VMCH_VOSEL_ADDR 0x1ded > +#define MT6373_PMIC_RG_VMCH_EINT_HIGH_VOSEL_ADDR > MT6373_PMIC_RG_VMCH_VOSEL_ADDR > +#define MT6373_PMIC_RG_VMCH_EINT_LOW_VOSEL_ADDR > MT6373_PMIC_RG_VMCH_VOSEL_ADDR > +#define MT6373_PMIC_RG_VEFUSE_VOCAL_ADDR 0x1df0 > +#define MT6373_PMIC_RG_VEFUSE_VOSEL_ADDR 0x1df1 > +#define MT6373_PMIC_RG_VMC_VOCAL_ADDR 0x1df4 > +#define MT6373_PMIC_RG_VMCH_EINT_HIGH_VOCAL_ADDR > MT6373_PMIC_RG_VMC_VOCAL_ADDR > +#define MT6373_PMIC_RG_VMCH_EINT_LOW_VOCAL_ADDR > MT6373_PMIC_RG_VMC_VOCAL_ADDR Is there a typo here? Aren't VMC and VMCH separate entities? > +#define MT6373_PMIC_RG_VMC_VOSEL_ADDR 0x1df5 > +#define MT6373_PMIC_RG_VIBR_VOCAL_ADDR 0x1df8 > +#define MT6373_PMIC_RG_VIBR_VOSEL_ADDR 0x1df9 > +#define MT6373_PMIC_RG_VIO28_VOCAL_ADDR 0x1dfc > +#define MT6373_PMIC_RG_VIO28_VOSEL_ADDR 0x1dfd > +#define MT6373_PMIC_RG_VFP_VOCAL_ADDR 0x1e00 > +#define MT6373_PMIC_RG_VFP_VOSEL_ADDR 0x1e01 > +#define MT6373_PMIC_RG_VTP_VOCAL_ADDR 0x1e04 > +#define MT6373_PMIC_RG_VTP_VOSEL_ADDR 0x1e05 > +#define MT6373_PMIC_RG_VSIM1_VOCAL_ADDR 0x1e08 > +#define MT6373_PMIC_RG_VSIM1_VOSEL_ADDR 0x1e09 > +#define MT6373_PMIC_RG_VSIM2_VOCAL_ADDR 0x1e0c > +#define MT6373_PMIC_RG_VSIM2_VOSEL_ADDR 0x1e0d > +#define MT6373_PMIC_RG_VAUD18_VOCAL_ADDR 0x1e58 > +#define MT6373_PMIC_RG_VAUD18_VOSEL_ADDR 0x1e59 > +#define MT6373_PMIC_RG_VRF18_AIF_VOCAL_ADDR 0x1e5c > +#define MT6373_PMIC_RG_VRF18_AIF_VOSEL_ADDR 0x1e5d > +#define MT6373_PMIC_RG_VCN18IO_VOCAL_ADDR 0x1e60 > +#define MT6373_PMIC_RG_VCN18IO_VOSEL_ADDR 0x1e61 > +#define MT6373_PMIC_RG_VRFIO18_AIF_VOCAL_ADDR 0x1e64 > +#define MT6373_PMIC_RG_VRFIO18_AIF_VOSEL_ADDR 0x1e65 > +#define MT6373_PMIC_RG_VANT18_VOCAL_ADDR 0x1e68 > +#define MT6373_PMIC_RG_VANT18_VOSEL_ADDR 0x1e69 > +#define MT6373_PMIC_RG_VRF13_AIF_VOCAL_ADDR 0x1ed8 > +#define MT6373_PMIC_RG_VRF13_AIF_VOSEL_ADDR 0x1ed9 > +#define MT6373_PMIC_RG_VRF12_AIF_VOCAL_ADDR 0x1edc > +#define MT6373_PMIC_RG_VRF12_AIF_VOSEL_ADDR 0x1edd > +#define MT6373_PMIC_RG_VRF09_AIF_VOCAL_ADDR 0x1f58 > +#define MT6373_PMIC_RG_VRF09_AIF_VOSEL_ADDR 0x1f59 > +#define MT6373_PMIC_RG_LDO_VT_VOCALSEL_MASK GENMASK(7, 0) > + ... > +static u8 mt6373_read(struct mt6373_regulator *reg, u16 addr, u8 *value) Needs to return int instead of u8 to pass error along. > +{ > + return pmic_read(reg->data->pmic, reg->data->base + addr, value, 1); > +} > + ... > +static int mt6373_regulator_set_value(struct udevice *dev, int uV) > +{ > + struct mt6373_regulator *plat = dev_get_plat(dev); > + const struct linear_range *range = NULL; > + int i, max, ret, nranges = plat->info->n_linear_ranges; > + unsigned int vsel; > + > + if (nranges > 1) { > + for (i = 0; i < nranges; i++) { > + if (uV < plat->info->linear_ranges[i].min) { > + if (i) { > + i--; > + range = &plat->info->linear_ranges[i]; > + break; > + } > + > + dev_err(dev, > + "Requested voltage (%d) is lower than" > + " minimum regulator voltage (%d)\n", > + uV, plat->info->linear_ranges[0].min); > + return -EINVAL; > + } > + } > + > + if (!range) { > + i = nranges - 1; > + max = plat->info->linear_ranges[i].min > + + (plat->info->linear_ranges[i].max_sel > + * plat->info->linear_ranges[i].step); > + > + if (uV > max) > + dev_err(dev, > + "Requested voltage (%d) is higher than" > + " maximum regulator voltage (%d)\n", > + uV, max); > + else > + range = &plat->info->linear_ranges[i]; > + } > + > + if (!range) { > + dev_err(dev, > + "Unable to determine proper range for value > (%d)\n", > + uV); > + return -EINVAL; > + } > + > + ret = mt6373_write(plat, plat->info->vsel_range_reg, > + (u8)i & plat->info->vsel_range_mask); > + if (ret) > + return ret; > + } else { > + range = plat->info->linear_ranges; > + } > + > + vsel = (uV - range->min) / range->step; Should this round up? (I think that is what Linux does.) > + return mt6373_write(plat, plat->info->vsel_reg, > + (u8)vsel & plat->info->vsel_mask); > +} > + ... > +static int mt6373_regulators_bind(struct udevice *dev) > +{ > + struct mt6373_regulator_data *data = dev_get_plat(dev); > + struct driver *drv; > + char *name; > + ofnode child, node = dev_ofnode(dev); > + int ret; > + size_t namelen; > + u32 addr; > + > + ret = ofnode_read_u32_index(node, "reg", 0, &addr); > + if (ret < 0) > + return -EINVAL; Why ignoring the actual error return? > + > + data->base = (u16)addr; Cast is not necessary. > + data->pmic = dev->parent; > + > + ofnode_for_each_subnode(child, node) { > + struct mt6373_regulator reg; > + > + reg.data = data; > + reg.info = mt6373_get_regulator_info(child); > + if (!reg.info) > + continue; > + > + namelen = strlen("reg-mt6373-") + strlen(reg.info->name) + 1; > + name = devm_kzalloc(dev, namelen, GFP_KERNEL); Should check !name before using it. > + ret = snprintf(name, namelen, "reg-mt6373-%s", reg.info->name); > + if (ret < 0 || ret >= namelen) { Doesn't snprintf() return number of bytes actually written, so could not be >= namelen? Also, it doesn't return negative error. > + dev_err(dev, "Failed to create MT6373 regulator > name\n"); > + return -ENOMEM; > + } > + > + if (reg.info->id <= MT6373_ID_VBUCK9) > + drv = DM_DRIVER_GET(mt6373_regulator_buck); > + else > + drv = DM_DRIVER_GET(mt6373_regulator_ldo); > + > + ret = device_bind_with_driver_data(dev, drv, name, (ulong)®, > + child, NULL); This is dangling pointer. reg is on the stack and the device will keep a pointer to it. Even if it doesn't get used now, someone might accidentally use it in the future. It would make more sense to just past the static per-regulator info here. And since the data is the same for each regulator, it can be fetched with dev_get_plat(dev->parent) without having to make a per-regulator pointer to it. > + if (ret < 0) { > + dev_err(dev, "Failed to bind MT6373 regulator %s: %d\n", > + name, ret); > + return ret; > + } > + } > + > + return 0; > +} > + > +static const struct udevice_id mt6373_regulators_ids[] = { > + { .compatible = "mediatek,mt6373-regulator" }, > + { /* sentinel */ } > +}; > + > +/* > + * Driver for a 'bank' of regulators. This creates devices for each > + * individual regulator > + */ > +U_BOOT_DRIVER(mt6373_regulators) = { > + .name = "mt6373_regulators", > + .id = UCLASS_MISC, Should be UCLASS_REGULATOR? UCLASS_MISC requires ops to be struct misc_ops. Or I guess this is because this is binding to the parent node rather than individual regulators. In other MediaTek drivers, we have the PMIC driver register the regulators with pmic_bind_children() instead. That seems simpler than re-implementing the same logic here. > + .bind = mt6373_regulators_bind, > + .probe = mt6373_regulators_probe, > + .of_match = mt6373_regulators_ids, > + .ops = &mt6373_regulator_ldo_ops, > + .plat_auto = sizeof(struct mt6373_regulator_data), > +}; >
