| Message ID | 20241018081050.23592-1-y.oudjana@protonmail.com |
|---|---|
| Headers | show |
| Series | MediaTek MT6735+MT6328 SoC/PMIC pair base support | expand |
Il 18/10/24 10:10, Yassine Oudjana ha scritto: > From: Yassine Oudjana <y.oudjana@protonmail.com> > > Add a driver for the regulators on the MT6328 PMIC. > > Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com> > --- > drivers/regulator/Kconfig | 9 + > drivers/regulator/Makefile | 1 + > drivers/regulator/mt6328-regulator.c | 479 +++++++++++++++++++++ > include/linux/regulator/mt6328-regulator.h | 49 +++ > 4 files changed, 538 insertions(+) > create mode 100644 drivers/regulator/mt6328-regulator.c > create mode 100644 include/linux/regulator/mt6328-regulator.h > > diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig > index 249933d6388dd..e9b9faff67f3a 100644 > --- a/drivers/regulator/Kconfig > +++ b/drivers/regulator/Kconfig > @@ -862,6 +862,15 @@ config REGULATOR_MT6323 > This driver supports the control of different power rails of device > through regulator interface. > > +config REGULATOR_MT6328 > + tristate "MediaTek MT6328 PMIC" > + depends on MFD_MT6397 > + help > + Say y here to select this option to enable the power regulator of > + MediaTek MT6328 PMIC. > + This driver supports the control of different power rails of device > + through regulator interface. > + > config REGULATOR_MT6331 > tristate "MediaTek MT6331 PMIC" > depends on MFD_MT6397 > diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile > index 9b69546fb3f65..c1a5a44413198 100644 > --- a/drivers/regulator/Makefile > +++ b/drivers/regulator/Makefile > @@ -103,6 +103,7 @@ obj-$(CONFIG_REGULATOR_MPQ7920) += mpq7920.o > obj-$(CONFIG_REGULATOR_MT6311) += mt6311-regulator.o > obj-$(CONFIG_REGULATOR_MT6315) += mt6315-regulator.o > obj-$(CONFIG_REGULATOR_MT6323) += mt6323-regulator.o > +obj-$(CONFIG_REGULATOR_MT6328) += mt6328-regulator.o > obj-$(CONFIG_REGULATOR_MT6331) += mt6331-regulator.o > obj-$(CONFIG_REGULATOR_MT6332) += mt6332-regulator.o > obj-$(CONFIG_REGULATOR_MT6357) += mt6357-regulator.o > diff --git a/drivers/regulator/mt6328-regulator.c b/drivers/regulator/mt6328-regulator.c > new file mode 100644 > index 0000000000000..e15a64404f494 > --- /dev/null > +++ b/drivers/regulator/mt6328-regulator.c > @@ -0,0 +1,479 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * MediaTek MT6328 regulator driver > + * Based on MT6323 driver. > + * > + * Copyright (c) 2016 MediaTek Inc. > + * Copyright (c) 2022 Yassine Oudjana <y.oudjana@protonmail.com> > + */ > + ..snip.. > +/* The array is indexed by id(MT6328_ID_XXX) */ > +static struct mt6328_regulator_info mt6328_regulators[] = { > + MT6328_BUCK("buck_vpa", VPA, 500000, 3650000, 50000, > + buck_volt_range1, MT6328_VPA_CON9, MT6328_VPA_CON11, 0x3f, > + MT6328_VPA_CON12, MT6328_VPA_CON7), Can you please fix the indentation? Also, all of those entries do fit in two lines, I checked a couple of those and always ended up with less than 90 columns anyway. After which, Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Il 18/10/24 10:10, Yassine Oudjana ha scritto: > From: Yassine Oudjana <y.oudjana@protonmail.com> > > Add register definitions and configuration for the MT6735 SoC and the > MT6328 PMIC which are commonly paired and communicate through the PMIC > wrapper. > > Note that the PMIC wrapper on MT6735M has a slightly different register > map and is therefore NOT compatible with MT6735. > > Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com> > --- > drivers/soc/mediatek/mtk-pmic-wrap.c | 251 ++++++++++++++++++++++++++- > 1 file changed, 248 insertions(+), 3 deletions(-) > > diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c > index 9fdc0ef792026..b9e8dd2a5999d 100644 > --- a/drivers/soc/mediatek/mtk-pmic-wrap.c > +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c > @@ -3,6 +3,7 @@ > * Copyright (c) 2014 MediaTek Inc. > * Author: Flora Fu, MediaTek > */ > + > #include <linux/clk.h> > #include <linux/interrupt.h> > #include <linux/io.h> > @@ -100,7 +101,7 @@ enum dew_regs { > PWRAP_DEW_CIPHER_MODE, > PWRAP_DEW_CIPHER_SWRST, > > - /* MT6323 only regs */ > + /* MT6323 and MT6328 only regs */ > PWRAP_DEW_CIPHER_EN, > PWRAP_DEW_RDDMY_NO, > > @@ -121,8 +122,10 @@ enum dew_regs { > PWRAP_RG_SPI_CON13, > PWRAP_SPISLV_KEY, > > - /* MT6359 only regs */ > + /* MT6359 and MT6328 only regs */ > PWRAP_DEW_CRC_SWRST, > + > + /* MT6359 only regs */ > PWRAP_DEW_RG_EN_RECORD, > PWRAP_DEW_RECORD_CMD0, > PWRAP_DEW_RECORD_CMD1, > @@ -171,6 +174,23 @@ static const u32 mt6323_regs[] = { > [PWRAP_DEW_RDDMY_NO] = 0x01a4, > }; > > +static const u32 mt6328_regs[] = { > + [PWRAP_DEW_DIO_EN] = 0x02d4, > + [PWRAP_DEW_READ_TEST] = 0x02d6, > + [PWRAP_DEW_WRITE_TEST] = 0x02d8, > + [PWRAP_DEW_CRC_SWRST] = 0x02da, > + [PWRAP_DEW_CRC_EN] = 0x02dc, > + [PWRAP_DEW_CRC_VAL] = 0x02de, > + [PWRAP_DEW_MON_GRP_SEL] = 0x02e0, > + [PWRAP_DEW_CIPHER_KEY_SEL] = 0x02e2, > + [PWRAP_DEW_CIPHER_IV_SEL] = 0x02e4, > + [PWRAP_DEW_CIPHER_EN] = 0x02e6, > + [PWRAP_DEW_CIPHER_RDY] = 0x02e8, > + [PWRAP_DEW_CIPHER_MODE] = 0x02ea, > + [PWRAP_DEW_CIPHER_SWRST] = 0x02ec, > + [PWRAP_DEW_RDDMY_NO] = 0x02ee, > +}; > + > static const u32 mt6331_regs[] = { > [PWRAP_DEW_DIO_EN] = 0x018c, > [PWRAP_DEW_READ_TEST] = 0x018e, > @@ -394,7 +414,7 @@ enum pwrap_regs { > PWRAP_ADC_RDATA_ADDR1, > PWRAP_ADC_RDATA_ADDR2, > > - /* MT7622 only regs */ > + /* MT7622 and MT6735 only regs */ > PWRAP_STA, > PWRAP_CLR, > PWRAP_DVFS_ADR8, > @@ -417,6 +437,8 @@ enum pwrap_regs { > PWRAP_ADC_RDATA_ADDR, > PWRAP_GPS_STA, > PWRAP_SW_RST, > + > + /* MT7622 only regs */ > PWRAP_DVFS_STEP_CTRL0, > PWRAP_DVFS_STEP_CTRL1, > PWRAP_DVFS_STEP_CTRL2, > @@ -481,6 +503,50 @@ enum pwrap_regs { > /* MT8516 only regs */ > PWRAP_OP_TYPE, > PWRAP_MSB_FIRST, > + > + /* MT6735 only regs */ > + PWRAP_WACS3_EN, > + PWRAP_INIT_DONE3, > + PWRAP_WACS3_CMD, > + PWRAP_WACS3_RDATA, > + PWRAP_WACS3_VLDCLR, Are you sure that you need the PWRAP_MD_ADC_xxxx registers in here? Since MD is relatively big on its own, I'm not sure how to proceed here.. it may make sense to split the MD part to a different array, or it may not... I do need to understand what's going on. Can you please point me at some reference code to look at, so that I can understand the situation a bit? Besides, I'm noticing that the "MD_ADC_RDATA_ADDR_R(x)" are sequential registers and that's on more than just MT6735: instead of having 32 more entries, it might make more sense to set only the first and declare the number (or size) of regs... Something like: enum pwrap_regs { ..... PWRAP_MD_ADC_RDATA_ADDR_LATEST, PWRAP_MD_ADC_RDATA_ADDR_WP, PWRAP_MD_ADC_RDATA_ADDR_R0, PWRAP_MD_ADC_STA0, PWRAP_MD_ADC_STA1, PWRAP_MD_ADC_STA2 }; static const struct pmic_wrapper_type pwrap_mt6735 = { .regs = mt6735_regs, .num_md_addr = 32, [other stuff] }; ...but again, please, if you can point me at an implementation that actually uses the R(x) registers, that'd be better ... so that we can choose the best option to add those in there. Everything else is great: good job :-) Cheers, Angelo
On Mon, Oct 21 2024 at 15:24:51 +02:00:00, AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote: > Il 18/10/24 10:10, Yassine Oudjana ha scritto: >> From: Yassine Oudjana <y.oudjana@protonmail.com> >> >> Add a driver for the regulators on the MT6328 PMIC. >> >> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com> >> --- >> drivers/regulator/Kconfig | 9 + >> drivers/regulator/Makefile | 1 + >> drivers/regulator/mt6328-regulator.c | 479 >> +++++++++++++++++++++ >> include/linux/regulator/mt6328-regulator.h | 49 +++ >> 4 files changed, 538 insertions(+) >> create mode 100644 drivers/regulator/mt6328-regulator.c >> create mode 100644 include/linux/regulator/mt6328-regulator.h >> >> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig >> index 249933d6388dd..e9b9faff67f3a 100644 >> --- a/drivers/regulator/Kconfig >> +++ b/drivers/regulator/Kconfig >> @@ -862,6 +862,15 @@ config REGULATOR_MT6323 >> This driver supports the control of different power rails of >> device >> through regulator interface. >> +config REGULATOR_MT6328 >> + tristate "MediaTek MT6328 PMIC" >> + depends on MFD_MT6397 >> + help >> + Say y here to select this option to enable the power regulator of >> + MediaTek MT6328 PMIC. >> + This driver supports the control of different power rails of >> device >> + through regulator interface. >> + >> config REGULATOR_MT6331 >> tristate "MediaTek MT6331 PMIC" >> depends on MFD_MT6397 >> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile >> index 9b69546fb3f65..c1a5a44413198 100644 >> --- a/drivers/regulator/Makefile >> +++ b/drivers/regulator/Makefile >> @@ -103,6 +103,7 @@ obj-$(CONFIG_REGULATOR_MPQ7920) += mpq7920.o >> obj-$(CONFIG_REGULATOR_MT6311) += mt6311-regulator.o >> obj-$(CONFIG_REGULATOR_MT6315) += mt6315-regulator.o >> obj-$(CONFIG_REGULATOR_MT6323) += mt6323-regulator.o >> +obj-$(CONFIG_REGULATOR_MT6328) += mt6328-regulator.o >> obj-$(CONFIG_REGULATOR_MT6331) += mt6331-regulator.o >> obj-$(CONFIG_REGULATOR_MT6332) += mt6332-regulator.o >> obj-$(CONFIG_REGULATOR_MT6357) += mt6357-regulator.o >> diff --git a/drivers/regulator/mt6328-regulator.c >> b/drivers/regulator/mt6328-regulator.c >> new file mode 100644 >> index 0000000000000..e15a64404f494 >> --- /dev/null >> +++ b/drivers/regulator/mt6328-regulator.c >> @@ -0,0 +1,479 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * MediaTek MT6328 regulator driver >> + * Based on MT6323 driver. >> + * >> + * Copyright (c) 2016 MediaTek Inc. >> + * Copyright (c) 2022 Yassine Oudjana <y.oudjana@protonmail.com> >> + */ >> + > > ..snip.. > >> +/* The array is indexed by id(MT6328_ID_XXX) */ >> +static struct mt6328_regulator_info mt6328_regulators[] = { >> + MT6328_BUCK("buck_vpa", VPA, 500000, 3650000, 50000, >> + buck_volt_range1, MT6328_VPA_CON9, MT6328_VPA_CON11, 0x3f, >> + MT6328_VPA_CON12, MT6328_VPA_CON7), > > Can you please fix the indentation? > > Also, all of those entries do fit in two lines, I checked a couple of > those > and always ended up with less than 90 columns anyway. I can't seem to fit even the first one in 2 lines in under 90 columns :/ That is unless I don't indent the second line: MT6328_BUCK("buck_vpa", VPA, 500000, 3650000, 50000, buck_volt_range1, MT6328_VPA_CON9, MT6328_VPA_CON11, 0x3f, MT6328_VPA_CON12, MT6328_VPA_CON7), Which I don't think is what you meant by fixing the indentation. Can you show me an example? With 100 columns on the other hand it seems like they should fit. >
On Tue, Oct 22 2024 at 11:44:46 +02:00:00, AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote: > Il 21/10/24 16:48, Yassine Oudjana ha scritto: >> >> On Mon, Oct 21 2024 at 15:25:00 +02:00:00, AngeloGioacchino Del >> Regno <angelogioacchino.delregno@collabora.com> wrote: >>> Il 18/10/24 10:10, Yassine Oudjana ha scritto: >>>> From: Yassine Oudjana <y.oudjana@protonmail.com> >>>> >>>> Add register definitions and configuration for the MT6735 SoC and >>>> the >>>> MT6328 PMIC which are commonly paired and communicate through the >>>> PMIC >>>> wrapper. >>>> >>>> Note that the PMIC wrapper on MT6735M has a slightly different >>>> register >>>> map and is therefore NOT compatible with MT6735. >>>> >>>> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com> >>>> --- >>>> drivers/soc/mediatek/mtk-pmic-wrap.c | 251 >>>> ++++++++++++++++++++++++++- >>>> 1 file changed, 248 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c >>>> b/drivers/soc/mediatek/mtk- pmic-wrap.c >>>> index 9fdc0ef792026..b9e8dd2a5999d 100644 >>>> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c >>>> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c >>>> @@ -3,6 +3,7 @@ >>>> * Copyright (c) 2014 MediaTek Inc. >>>> * Author: Flora Fu, MediaTek >>>> */ >>>> + >>>> #include <linux/clk.h> >>>> #include <linux/interrupt.h> >>>> #include <linux/io.h> >>>> @@ -100,7 +101,7 @@ enum dew_regs { >>>> PWRAP_DEW_CIPHER_MODE, >>>> PWRAP_DEW_CIPHER_SWRST, >>>> - /* MT6323 only regs */ >>>> + /* MT6323 and MT6328 only regs */ >>>> PWRAP_DEW_CIPHER_EN, >>>> PWRAP_DEW_RDDMY_NO, >>>> @@ -121,8 +122,10 @@ enum dew_regs { >>>> PWRAP_RG_SPI_CON13, >>>> PWRAP_SPISLV_KEY, >>>> - /* MT6359 only regs */ >>>> + /* MT6359 and MT6328 only regs */ >>>> PWRAP_DEW_CRC_SWRST, >>>> + >>>> + /* MT6359 only regs */ >>>> PWRAP_DEW_RG_EN_RECORD, >>>> PWRAP_DEW_RECORD_CMD0, >>>> PWRAP_DEW_RECORD_CMD1, >>>> @@ -171,6 +174,23 @@ static const u32 mt6323_regs[] = { >>>> [PWRAP_DEW_RDDMY_NO] = 0x01a4, >>>> }; >>>> +static const u32 mt6328_regs[] = { >>>> + [PWRAP_DEW_DIO_EN] = 0x02d4, >>>> + [PWRAP_DEW_READ_TEST] = 0x02d6, >>>> + [PWRAP_DEW_WRITE_TEST] = 0x02d8, >>>> + [PWRAP_DEW_CRC_SWRST] = 0x02da, >>>> + [PWRAP_DEW_CRC_EN] = 0x02dc, >>>> + [PWRAP_DEW_CRC_VAL] = 0x02de, >>>> + [PWRAP_DEW_MON_GRP_SEL] = 0x02e0, >>>> + [PWRAP_DEW_CIPHER_KEY_SEL] = 0x02e2, >>>> + [PWRAP_DEW_CIPHER_IV_SEL] = 0x02e4, >>>> + [PWRAP_DEW_CIPHER_EN] = 0x02e6, >>>> + [PWRAP_DEW_CIPHER_RDY] = 0x02e8, >>>> + [PWRAP_DEW_CIPHER_MODE] = 0x02ea, >>>> + [PWRAP_DEW_CIPHER_SWRST] = 0x02ec, >>>> + [PWRAP_DEW_RDDMY_NO] = 0x02ee, >>>> +}; >>>> + >>>> static const u32 mt6331_regs[] = { >>>> [PWRAP_DEW_DIO_EN] = 0x018c, >>>> [PWRAP_DEW_READ_TEST] = 0x018e, >>>> @@ -394,7 +414,7 @@ enum pwrap_regs { >>>> PWRAP_ADC_RDATA_ADDR1, >>>> PWRAP_ADC_RDATA_ADDR2, >>>> - /* MT7622 only regs */ >>>> + /* MT7622 and MT6735 only regs */ >>>> PWRAP_STA, >>>> PWRAP_CLR, >>>> PWRAP_DVFS_ADR8, >>>> @@ -417,6 +437,8 @@ enum pwrap_regs { >>>> PWRAP_ADC_RDATA_ADDR, >>>> PWRAP_GPS_STA, >>>> PWRAP_SW_RST, >>>> + >>>> + /* MT7622 only regs */ >>>> PWRAP_DVFS_STEP_CTRL0, >>>> PWRAP_DVFS_STEP_CTRL1, >>>> PWRAP_DVFS_STEP_CTRL2, >>>> @@ -481,6 +503,50 @@ enum pwrap_regs { >>>> /* MT8516 only regs */ >>>> PWRAP_OP_TYPE, >>>> PWRAP_MSB_FIRST, >>>> + >>>> + /* MT6735 only regs */ >>>> + PWRAP_WACS3_EN, >>>> + PWRAP_INIT_DONE3, >>>> + PWRAP_WACS3_CMD, >>>> + PWRAP_WACS3_RDATA, >>>> + PWRAP_WACS3_VLDCLR, >>> >>> Are you sure that you need the PWRAP_MD_ADC_xxxx registers in here? >>> >>> Since MD is relatively big on its own, I'm not sure how to proceed >>> here.. it may >>> make sense to split the MD part to a different array, or it may >>> not... I do need >>> to understand what's going on. >>> >>> Can you please point me at some reference code to look at, so that >>> I can understand >>> the situation a bit? >>> >>> Besides, I'm noticing that the "MD_ADC_RDATA_ADDR_R(x)" are >>> sequential registers >>> and that's on more than just MT6735: instead of having 32 more >>> entries, it might >>> make more sense to set only the first and declare the number (or >>> size) of regs... >>> >>> Something like: >>> >>> enum pwrap_regs { >>> ..... >>> PWRAP_MD_ADC_RDATA_ADDR_LATEST, >>> PWRAP_MD_ADC_RDATA_ADDR_WP, >>> PWRAP_MD_ADC_RDATA_ADDR_R0, >>> PWRAP_MD_ADC_STA0, >>> PWRAP_MD_ADC_STA1, >>> PWRAP_MD_ADC_STA2 >>> }; >>> >>> static const struct pmic_wrapper_type pwrap_mt6735 = { >>> .regs = mt6735_regs, >>> .num_md_addr = 32, >>> [other stuff] >>> }; >>> >>> ...but again, please, if you can point me at an implementation that >>> actually >>> uses the R(x) registers, that'd be better ... so that we can choose >>> the best >>> option to add those in there. >>> >>> Everything else is great: good job :-) >>> >>> Cheers, >>> Angelo >> >> I just defined all the registers I could find. We aren't using them >> for anything yet so it's also fine to keep them out for now. >> >> It seems that in the downstream kernel they are only initialized >> once and never accessed again. This is the only place I could find >> where they are accessed: >> https://gitlab.com/Tooniis/linux-samsung-grandpplte/-/blob/master/drivers/misc/ >> mediatek/pmic_wrap/mt6735/pwrap_hal.c#L1254-1290 >> >> > > Thanks for pointing me at that reference. > > Yeah, looks like they're getting statically initialized to some > value, and then > nothing else... I wonder if the values that they're writing are just > the register > defaults, or if they're actually overriding anything for real... > > ...that's because, in case those are the defaults, we may get them > set by just > resetting the MD, catching two birds with one stone. > That'd be easy to check, anyway, as you can just read them out and > see if the > values are these... The values aren't defaults. The registers all default to 0 and these values are PMIC registers. Anyway, I'll just remove the registers since I won't need them any time soon.
From: Yassine Oudjana <y.oudjana@protonmail.com> These patches are part of a larger effort to support the MT6735 SoC family in mainline Linux. More patches (unsent or sent and pending review or revision) can be found here[1]. This series adds base support for the MediaTek MT6735 SoC and MT6328 PMIC pair. This includes PMIC wrapper support on the SoC side and regulators and keys on the PMIC side. The PMIC has other blocks such as an audio codec and battery charger which can be supported in the future. [1] https://gitlab.com/mt6735-mainline/linux/-/commits/mt6735-staging Yassine Oudjana (6): dt-bindings: mediatek: pwrap: Add MT6735 compatible dt-bindings: mfd: mediatek: mt6397: Add bindings for MT6328 soc: mediatek: pwrap: Add support for MT6735 and MT6328 SoC/PMIC pair mfd: mt6397: Add initial support for MT6328 regulator: Add driver for MediaTek MT6328 PMIC regulators Input: mtk-pmic-keys - Add support for MT6328 .../bindings/input/mediatek,pmic-keys.yaml | 1 + .../bindings/mfd/mediatek,mt6397.yaml | 2 + .../bindings/soc/mediatek/mediatek,pwrap.yaml | 1 + drivers/input/keyboard/mtk-pmic-keys.c | 15 + drivers/mfd/mt6397-core.c | 32 + drivers/mfd/mt6397-irq.c | 23 + drivers/regulator/Kconfig | 9 + drivers/regulator/Makefile | 1 + drivers/regulator/mt6328-regulator.c | 479 ++++++++++ drivers/soc/mediatek/mtk-pmic-wrap.c | 251 +++++- include/linux/mfd/mt6328/core.h | 53 ++ include/linux/mfd/mt6328/registers.h | 822 ++++++++++++++++++ include/linux/mfd/mt6397/core.h | 11 +- include/linux/regulator/mt6328-regulator.h | 49 ++ 14 files changed, 1741 insertions(+), 8 deletions(-) create mode 100644 drivers/regulator/mt6328-regulator.c create mode 100644 include/linux/mfd/mt6328/core.h create mode 100644 include/linux/mfd/mt6328/registers.h create mode 100644 include/linux/regulator/mt6328-regulator.h