| Message ID | 20221005-mt6357-support-v5-0-8210d955dd3d@baylibre.com |
|---|---|
| Headers | show |
| Series | Add MediaTek MT6357 PMIC support | expand |
Hi Alexandre, All Please, treat my review more as initiation for discussion than 'hard requirements' for this driver. I am in no point or no "confidence level" to give you any requirements ;) On 11/16/22 14:33, Alexandre Mergnat wrote: > From: Fabien Parent <fparent@baylibre.com> > > Add regulator driver for the MT6357 PMIC. > > Signed-off-by: Fabien Parent <fparent@baylibre.com> > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> > --- //snip > +/* > + * MT6357 regulators' information > + * > + * @desc: standard fields of regulator description. > + * @da_vsel_reg: Monitor register for query buck's voltage. > + * @da_vsel_mask: Mask for query buck's voltage. > + */ > +struct mt6357_regulator_info { > + struct regulator_desc desc; > + u32 da_vsel_reg; > + u32 da_vsel_mask; > +}; > + //snip > +/** > + * mt6357_get_buck_voltage_sel - get_voltage_sel for regmap users > + * > + * @rdev: regulator to operate on > + * > + * Regulators that use regmap for their register I/O can set the > + * da_vsel_reg and da_vsel_mask fields in the info structure and > + * then use this as their get_voltage_vsel operation. > + */ > +static int mt6357_get_buck_voltage_sel(struct regulator_dev *rdev) > +{ > + int ret, regval; > + struct mt6357_regulator_info *info = rdev_get_drvdata(rdev); > + > + ret = regmap_read(rdev->regmap, info->da_vsel_reg, ®val); > + if (ret != 0) { > + dev_err(&rdev->dev, > + "Failed to get mt6357 Buck %s vsel reg: %d\n", > + info->desc.name, ret); > + return ret; > + } > + > + regval &= info->da_vsel_mask; > + regval >>= ffs(info->da_vsel_mask) - 1; > + > + return regval; > +} If I read this right, the device has separate register(s) for writing and reading the voltage? I wonder if this is a completely unique setup? If this is not unique, then it might be worth adding another field for 'vsel_get' register and a flag in regulator desc - and modify the generic regmap helpers to handle this in common code if the special register? Not sure if this HW design is common enough to warrant the added confusion though. You and Mark may have more insight. > + > +static const struct linear_range buck_volt_range1[] = { > + REGULATOR_LINEAR_RANGE(518750, 0, 0x7f, 6250), > +}; > + > +static const struct linear_range buck_volt_range2[] = { > + REGULATOR_LINEAR_RANGE(500000, 0, 0x7f, 6250), > +}; > + > +static const struct linear_range buck_volt_range3[] = { > + REGULATOR_LINEAR_RANGE(500000, 0, 0x3f, 50000), > +}; > + > +static const struct linear_range buck_volt_range4[] = { > + REGULATOR_LINEAR_RANGE(1200000, 0, 0x7f, 12500), > +}; I am unsure if we should aim for dropping the REGULATOR_LINEAR_RANGE() and using the LINEAR_RANGE(). If yes, then it might simplify things if new drivers used LINEAR_RANGE() from the day 1. If we don't, then it makes sense to keep consistently using REGULATOR_LINEAR_RANGE() for all of the drivers. I am not sure which way is the right way. > +static int mt6357_regulator_probe(struct platform_device *pdev) > +{ > + struct mt6397_chip *mt6357 = dev_get_drvdata(pdev->dev.parent); I am unsure what data do you need from the parent. If it is just the regmap / device-tree node / device, then it does not (in my opinion) really warrant using parent's drvdata. One can often get away with the dev_get_regmap(pdev->dev.parent, NULL). Anyways, the driver looks good to me. Yours, -- Matti Vaittinen
On Wed, 16 Nov 2022 13:32:59 +0100, Alexandre Mergnat wrote: > Currently, almost all MT63XX PMIC are documented mfd/mt6397.txt. > Unfortunately, the PMICs haven't always similar HW sub-features. > To have a better human readable schema, I chose to make one PMIC schema > to match the exact HW capabilities instead of convert mt6397.txt to > mediatek,mt63xx.yaml and put a bunch of properties behind > "if contain ... then ..." > > - add interrupt property > - change property refs to match with new yaml documentation > > Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> > --- > .../devicetree/bindings/mfd/mediatek,mt6357.yaml | 105 +++++++++++++++++++++ > 1 file changed, 105 insertions(+) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: doc reference errors (make refcheckdocs): Warning: Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml references a file that doesn't exist: Documentation/devicetree/bindings/soc/mediatek/mediatek,pwrap.yaml Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml: Documentation/devicetree/bindings/soc/mediatek/mediatek,pwrap.yaml See https://patchwork.ozlabs.org/patch/ This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
On 16/11/2022 13:33, Alexandre Mergnat wrote: > - Change the node name from "mt6358" to "pmic" to be consistent > with mediatek,pwrap.yaml documentation. > > - Change the node name from "mt6358rtc" to "rtc" to be consistent > with mediatek,mt6397-rtc.yaml documentation. > > - Change the node name from "mt6358keys" to "keys" to be consistent > with mediatek,pmic-keys.yaml documentation. > I think the node names should be changed anyway. So I'd advise to update the commit message and I can take some right away. Regards, Matthias > Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> > --- > arch/arm64/boot/dts/mediatek/mt6358.dtsi | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/boot/dts/mediatek/mt6358.dtsi b/arch/arm64/boot/dts/mediatek/mt6358.dtsi > index 98f3b0e0c9f6..b605313bed99 100644 > --- a/arch/arm64/boot/dts/mediatek/mt6358.dtsi > +++ b/arch/arm64/boot/dts/mediatek/mt6358.dtsi > @@ -5,7 +5,7 @@ > #include <dt-bindings/input/input.h> > > &pwrap { > - pmic: mt6358 { > + pmic: pmic { > compatible = "mediatek,mt6358"; > interrupt-controller; > interrupt-parent = <&pio>; > @@ -355,11 +355,11 @@ mt6358_vsim2_reg: ldo_vsim2 { > }; > }; > > - mt6358rtc: mt6358rtc { > + mt6358rtc: rtc { > compatible = "mediatek,mt6358-rtc"; > }; > > - mt6358keys: mt6358keys { > + mt6358keys: keys { > compatible = "mediatek,mt6358-keys"; > power { > linux,keycodes = <KEY_POWER>; >
Le mer. 16 nov. 2022 à 15:17, Matti Vaittinen <mazziesaccount@gmail.com> a écrit : > > Hi Alexandre, All > > Please, treat my review more as initiation for discussion than 'hard > requirements' for this driver. I am in no point or no "confidence level" > to give you any requirements ;) Hi Matti, Understood, thanks for clarifying this. > > If I read this right, the device has separate register(s) for writing > and reading the voltage? I wonder if this is a completely unique setup? > > If this is not unique, then it might be worth adding another field for > 'vsel_get' register and a flag in regulator desc - and modify the > generic regmap helpers to handle this in common code if the special > register? Not sure if this HW design is common enough to warrant the > added confusion though. You and Mark may have more insight. > I didn't write this driver and when I handled it, I found this weird. In the datasheet, registers access are read and write. After some read/write tests in the registers, I understood that read vosel_reg always returns the wrong value. That's why the debug register is used to get the value. I'm not sure I understand your proposal, but it seems to add more custom stuff and modify generic regmap instead of using the generic regmap which already allows us to customize get and set functions properly. IMHO, modifying the generic regmap isn't a good solution because I think this HW design isn't common. > > + > > +static const struct linear_range buck_volt_range1[] = { > > + REGULATOR_LINEAR_RANGE(518750, 0, 0x7f, 6250), > > +}; > > + > > +static const struct linear_range buck_volt_range2[] = { > > + REGULATOR_LINEAR_RANGE(500000, 0, 0x7f, 6250), > > +}; > > + > > +static const struct linear_range buck_volt_range3[] = { > > + REGULATOR_LINEAR_RANGE(500000, 0, 0x3f, 50000), > > +}; > > + > > +static const struct linear_range buck_volt_range4[] = { > > + REGULATOR_LINEAR_RANGE(1200000, 0, 0x7f, 12500), > > +}; > > I am unsure if we should aim for dropping the REGULATOR_LINEAR_RANGE() > and using the LINEAR_RANGE(). If yes, then it might simplify things if > new drivers used LINEAR_RANGE() from the day 1. If we don't, then it > makes sense to keep consistently using REGULATOR_LINEAR_RANGE() for all > of the drivers. I am not sure which way is the right way. Good catch. LINEAR_RANGE() is defined in "linear_range.h" REGULATOR_LINEAR_RANGE() is defined in "regulator/driver.h" "linear_range.h" is included in "regulator/driver.h" Then, I would like to say that regulator drivers should use REGULATOR_LINEAR_RANGE(). But duplicating the definition is weird, this is probably something which needs to be fixed or clarified. Also, that means mt6357-regulator.c no longer needs "#include <linux/linear_range.h>". Then I will remove it. > > > +static int mt6357_regulator_probe(struct platform_device *pdev) > > +{ > > + struct mt6397_chip *mt6357 = dev_get_drvdata(pdev->dev.parent); > > I am unsure what data do you need from the parent. If it is just the > regmap / device-tree node / device, then it does not (in my opinion) > really warrant using parent's drvdata. One can often get away with the > dev_get_regmap(pdev->dev.parent, NULL). Ok thanks, I wasn't aware of that. I tried to apply this change but I've got a kernel panic at boot because "mt6357_get_buck_voltage_sel" needs to retrieve the regmap.
Hi, This patch series adds MFD, PMIC keys, and regulator support for MT6357. MT6357 is a MediaTek PMIC very similar to MT6358. Currently, MTK bindings related to the PMICs are not converted yet (still .txt): soc/mediatek/pwrap.txt (all PMIC parent) | V mfd/mt6397.txt (support lot of mt63XX PMIC) +---------------+----------------+---... V V V regulator/... rtc/... codec/... 1) Convert pwrap to yaml is ok. 2) For the PMIC bindings, there are two option: - Convert mt6397.txt to mediatek,mt6397.yaml and continue to support multiple PMIC with only one file. IMO, the file will be hard to read because the supported features aren't the same for each PMIC. - Make a binding file for each PMIC ref: - mfd/mediatek,mt6357.yaml - mfd/mediatek,mt6358.yaml - ... 3) All PMIC daughter bindings (regulator, rtc, codec, led, ...) aren't fully converted yet. Refering to the two PMIC convertion options above: - To be clean, all daughter bindings should be converted. This is hard because a good understanding of each device is requiered to write efficient bindings. - Only daughter bindings supported by the added PMIC should be converted, that allows to do the task conversion step by step. In the V4 of this serie, I chose the second option. Regards, Alex Changes in v5: - Add missing maintainers - Improve RTC binding by adding rtc.yaml ref and start-year property - Split the txt->yaml conversion in one commit and the addition of the new mt6357-rtc compatible in another commit. - Improve PWRAP binding: - clocks and clock-name have been refactored. - reset-names is now properly dependent to resets. - additionalProperties change from true to false. - change example for a most recent and popular SoC. - "allOf" part has been simplified. - Pass binding tests with the updated tools. Here the command: "make DT_CHECKER_FLAGS=-m dt_binding_check" - Link to v4: https://lore.kernel.org/r/20221005-mt6357-support-v4-0-5d2bb58e6087@baylibre.com Changes in v4: - "dt-bindings: mfd: mt6397: add binding for MT6357" has been applied by Lee Jones - All fixed regulator are now refering to fixed-regulator.yaml - vfe28 and vcamio18 regulators have been added - pwrap binding has been converted and mt8365 support has been added - Change node names for mt8173 and mt6358 SoC to be consistent with pwrap documentation. - mt6357 PMIC binding has been created - mt6397 RTC binding has been converted and mt6357 support has been added - Link to v3: https://lore.kernel.org/r/20221005-mt6357-support-v3-0-7e0bd7c315b2@baylibre.com Changes in v3: - To be consistent with regulator/driver.h and helper.c, shift variables have been removed and the mask values have been directly shifted. - Remove index tables and rework volt tables to use set/get helper functions. - Add comment to structure and function. - Fix Fabien Parent mail address. - Link to v2: https://lore.kernel.org/r/20221005-mt6357-support-v2-0-f17ba2d2d0a9@baylibre.com Changes in v2: - Rebase - Fix typo - Remove dependencies with https://lore.kernel.org/all/20220415153629.1817202-1-fparent@baylibre.com/ which is no longer relevant. Previous versions: v1 - https://lore.kernel.org/all/20220531124959.202787-1-fparent@baylibre.com/ To: Lee Jones <lee@kernel.org> To: Rob Herring <robh+dt@kernel.org> To: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> To: Matthias Brugger <matthias.bgg@gmail.com> To: Dmitry Torokhov <dmitry.torokhov@gmail.com> To: Chen Zhong <chen.zhong@mediatek.com> To: Liam Girdwood <lgirdwood@gmail.com> To: Mark Brown <broonie@kernel.org> To: Fabien Parent <fabien.parent@linaro.org> To: Alessandro Zummo <a.zummo@towertech.it> To: Alexandre Belloni <alexandre.belloni@bootlin.com> To: Sean Wang <sean.wang@mediatek.com> To: Pavel Machek <pavel@ucw.cz> To: Tianping Fang <tianping.fang@mediatek.com> To: Flora Fu <flora.fu@mediatek.com> Cc: devicetree@vger.kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-mediatek@lists.infradead.org Cc: linux-kernel@vger.kernel.org Cc: linux-input@vger.kernel.org Cc: Fabien Parent <fparent@baylibre.com> Cc: Rob Herring <robh@kernel.org> Cc: linux-rtc@vger.kernel.org Cc: linux-leds@vger.kernel.org Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> --- Alexandre Mergnat (6): dt-bindings: rtc: mediatek: convert MT6397 rtc documentation dt-bindings: rtc: mediatek: add MT6357 support dt-bindings: mfd: mediatek: Add bindings for MT6357 PMIC dt-bindings: soc: mediatek: convert pwrap documentation arm64: dts: mt6358: change node names arm64: dts: mt8173: change node name Fabien Parent (4): dt-bindings: input: mtk-pmic-keys: add binding for MT6357 PMIC regulator: dt-bindings: Add binding schema for mt6357 regulators regulator: add mt6357 regulator Input: mtk-pmic-keys: add MT6357 support .../bindings/input/mediatek,pmic-keys.yaml | 1 + .../devicetree/bindings/leds/leds-mt6323.txt | 2 +- .../devicetree/bindings/mfd/mediatek,mt6357.yaml | 105 +++++ Documentation/devicetree/bindings/mfd/mt6397.txt | 4 +- .../regulator/mediatek,mt6357-regulator.yaml | 293 +++++++++++++ .../bindings/rtc/mediatek,mt6397-rtc.yaml | 44 ++ .../devicetree/bindings/rtc/rtc-mt6397.txt | 31 -- .../bindings/soc/mediatek/mediatek,pwrap.yaml | 145 +++++++ .../devicetree/bindings/soc/mediatek/pwrap.txt | 75 ---- arch/arm64/boot/dts/mediatek/mt6358.dtsi | 6 +- arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi | 2 +- arch/arm64/boot/dts/mediatek/mt8173-evb.dts | 2 +- drivers/input/keyboard/mtk-pmic-keys.c | 17 + drivers/regulator/Kconfig | 9 + drivers/regulator/Makefile | 1 + drivers/regulator/mt6357-regulator.c | 454 +++++++++++++++++++++ include/linux/regulator/mt6357-regulator.h | 51 +++ 17 files changed, 1128 insertions(+), 114 deletions(-) --- base-commit: e7f535c0775b896befb4f6765c02bc065fd26156 change-id: 20221005-mt6357-support-55308b82e33f Best regards,