Re: [PATCH v3 5/8] regulator: mt6359: Add support for MT6359 regulator

2020-12-15 Thread Hsin-hsiung Wang 
Hi,
On Tue, 2020-12-15 at 11:56 +, Mark Brown wrote:
> On Tue, Dec 15, 2020 at 05:23:08PM +0800, Hsin-hsiung Wang wrote:
> > On Tue, 2020-11-24 at 17:07 +, Mark Brown wrote:
> 
> > > This looks like it could just be regmap_get_voltage_sel_regmap()?
> > > Otherwise the driver looks good.
> 
> > Thanks for the review.
> > MT6359 regulator has sel_reg and status_reg, so we use
> > mt6359_get_linear_voltage_sel for status_reg instead of
> > regmap_get_voltage_sel_regmap() which uses sel_reg.
> 
> Is the selector register not readable?  In general the rule is that the
> get should be reporting what was configured, the actual status should be
> reported separately if it can be read separately.  We don't currently
> have a mechanism for doing that with voltage but one could be added.

Thanks for your comments. The select register is readable, and I will
update it in next patch.

Re: [PATCH v3 5/8] regulator: mt6359: Add support for MT6359 regulator

2020-12-15 Thread Mark Brown 
On Tue, Dec 15, 2020 at 05:23:08PM +0800, Hsin-hsiung Wang wrote:
> On Tue, 2020-11-24 at 17:07 +, Mark Brown wrote:

> > This looks like it could just be regmap_get_voltage_sel_regmap()?
> > Otherwise the driver looks good.

> Thanks for the review.
> MT6359 regulator has sel_reg and status_reg, so we use
> mt6359_get_linear_voltage_sel for status_reg instead of
> regmap_get_voltage_sel_regmap() which uses sel_reg.

Is the selector register not readable?  In general the rule is that the
get should be reporting what was configured, the actual status should be
reported separately if it can be read separately.  We don't currently
have a mechanism for doing that with voltage but one could be added.


signature.asc
Description: PGP signature

Re: [PATCH v3 5/8] regulator: mt6359: Add support for MT6359 regulator

2020-12-15 Thread Hsin-hsiung Wang 
Hi,

On Tue, 2020-11-24 at 17:07 +, Mark Brown wrote:
> On Mon, Nov 23, 2020 at 11:48:07AM +0800, Hsin-Hsiung Wang wrote:
> 
> > +static int mt6359_get_linear_voltage_sel(struct regulator_dev *rdev)
> > +{
> > +   struct mt6359_regulator_info *info = rdev_get_drvdata(rdev);
> > +   int ret, regval;
> > +
> > +   ret = regmap_read(rdev->regmap, info->da_vsel_reg, ®val);
> > +   if (ret != 0) {
> > +   dev_err(&rdev->dev,
> > +   "Failed to get mt6359 Buck %s vsel reg: %d\n",
> > +   info->desc.name, ret);
> > +   return ret;
> > +   }
> > +
> > +   ret = (regval >> info->da_vsel_shift) & info->da_vsel_mask;
> > +
> > +   return ret;
> > +}
> 
> This looks like it could just be regmap_get_voltage_sel_regmap()?
> Otherwise the driver looks good.

Thanks for the review.
MT6359 regulator has sel_reg and status_reg, so we use
mt6359_get_linear_voltage_sel for status_reg instead of
regmap_get_voltage_sel_regmap() which uses sel_reg.

Thanks.

Re: [PATCH v3 5/8] regulator: mt6359: Add support for MT6359 regulator

2020-11-24 Thread Mark Brown 
On Mon, Nov 23, 2020 at 11:48:07AM +0800, Hsin-Hsiung Wang wrote:

> +static int mt6359_get_linear_voltage_sel(struct regulator_dev *rdev)
> +{
> + struct mt6359_regulator_info *info = rdev_get_drvdata(rdev);
> + int ret, regval;
> +
> + ret = regmap_read(rdev->regmap, info->da_vsel_reg, ®val);
> + if (ret != 0) {
> + dev_err(&rdev->dev,
> + "Failed to get mt6359 Buck %s vsel reg: %d\n",
> + info->desc.name, ret);
> + return ret;
> + }
> +
> + ret = (regval >> info->da_vsel_shift) & info->da_vsel_mask;
> +
> + return ret;
> +}

This looks like it could just be regmap_get_voltage_sel_regmap()?
Otherwise the driver looks good.


signature.asc
Description: PGP signature

[PATCH v3 5/8] regulator: mt6359: Add support for MT6359 regulator

2020-11-22 Thread Hsin-Hsiung Wang 
From: Wen Su 

The MT6359 is a regulator found on boards based on MediaTek MT6779 and
probably other SoCs. It is a so called pmic and connects as a slave to
SoC using SPI, wrapped inside the pmic-wrapper.

Signed-off-by: Wen Su 
Signed-off-by: Hsin-Hsiung Wang 
Acked-by: Mark Brown 
---
 drivers/regulator/Kconfig  |   9 +
 drivers/regulator/Makefile |   1 +
 drivers/regulator/mt6359-regulator.c   | 714 +
 include/linux/regulator/mt6359-regulator.h |  58 ++
 4 files changed, 782 insertions(+)
 create mode 100644 drivers/regulator/mt6359-regulator.c
 create mode 100644 include/linux/regulator/mt6359-regulator.h

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 020a00d6696b..7572efc38bd5 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -721,6 +721,15 @@ config REGULATOR_MT6358
  This driver supports the control of different power rails of device
  through regulator interface.
 
+config REGULATOR_MT6359
+   tristate "MediaTek MT6359 PMIC"
+   depends on MFD_MT6397
+   help
+ Say y here to select this option to enable the power regulator of
+ MediaTek MT6359 PMIC.
+ This driver supports the control of different power rails of device
+ through regulator interface.
+
 config REGULATOR_MT6360
tristate "MT6360 SubPMIC Regulator"
depends on MFD_MT6360
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 6ebae516258e..56c28ab191e4 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -89,6 +89,7 @@ obj-$(CONFIG_REGULATOR_MPQ7920) += mpq7920.o
 obj-$(CONFIG_REGULATOR_MT6311) += mt6311-regulator.o
 obj-$(CONFIG_REGULATOR_MT6323) += mt6323-regulator.o
 obj-$(CONFIG_REGULATOR_MT6358) += mt6358-regulator.o
+obj-$(CONFIG_REGULATOR_MT6359) += mt6359-regulator.o
 obj-$(CONFIG_REGULATOR_MT6360) += mt6360-regulator.o
 obj-$(CONFIG_REGULATOR_MT6380) += mt6380-regulator.o
 obj-$(CONFIG_REGULATOR_MT6397) += mt6397-regulator.o
diff --git a/drivers/regulator/mt6359-regulator.c 
b/drivers/regulator/mt6359-regulator.c
new file mode 100644
index ..9746e2c281ae
--- /dev/null
+++ b/drivers/regulator/mt6359-regulator.c
@@ -0,0 +1,714 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (c) 2020 MediaTek Inc.
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define MT6359_BUCK_MODE_AUTO  0
+#define MT6359_BUCK_MODE_FORCE_PWM 1
+#define MT6359_BUCK_MODE_NORMAL0
+#define MT6359_BUCK_MODE_LP2
+
+/*
+ * MT6359 regulators' information
+ *
+ * @desc: standard fields of regulator description.
+ * @status_reg: for query status of regulators.
+ * @qi: Mask for query enable signal status of regulators.
+ * @modeset_reg: for operating AUTO/PWM mode register.
+ * @modeset_mask: MASK for operating modeset register.
+ * @modeset_shift: SHIFT for operating modeset register.
+ */
+struct mt6359_regulator_info {
+   struct regulator_desc desc;
+   u32 status_reg;
+   u32 qi;
+   u32 da_vsel_reg;
+   u32 da_vsel_mask;
+   u32 da_vsel_shift;
+   u32 modeset_reg;
+   u32 modeset_mask;
+   u32 modeset_shift;
+   u32 lp_mode_reg;
+   u32 lp_mode_mask;
+   u32 lp_mode_shift;
+};
+
+#define MT6359_BUCK(match, _name, min, max, step, min_sel, \
+   volt_ranges, _enable_reg, _status_reg,  \
+   _da_vsel_reg, _da_vsel_mask, _da_vsel_shift,\
+   _vsel_reg, _vsel_mask,  \
+   _lp_mode_reg, _lp_mode_shift,   \
+   _modeset_reg, _modeset_shift)   \
+[MT6359_ID_##_name] = {\
+   .desc = {   \
+   .name = #_name, \
+   .of_match = of_match_ptr(match),\
+   .ops = &mt6359_volt_range_ops,  \
+   .type = REGULATOR_VOLTAGE,  \
+   .id = MT6359_ID_##_name,\
+   .owner = THIS_MODULE,   \
+   .uV_step = (step),  \
+   .linear_min_sel = (min_sel),\
+   .n_voltages = ((max) - (min)) / (step) + 1, \
+   .min_uV = (min),\
+   .linear_ranges = volt_ranges,   \
+   .n_linear_ranges = ARRAY_SIZE(volt_ranges), \
+   .vsel_reg = _vsel_reg,  \
+   .vsel_mask = _vsel_mask,\
+   .enable_reg = _enable_reg,  \
+   .enable_mask = BIT(0),  \
+   .of_map_mode =