The VCN33_BT and VCN33_WIFI regulators are actually the same regulator,
having the same voltage setting and output pin. There are simply two
enable bits that are ORed together to enable the regulator.
Having two regulators representing the same output pin is misleading
from a design matching standpoint, and also error-prone in driver
implementations. If consumers try to set different voltages on either
regulator, the one set later would override the one set before. There
are ways around this, such as chaining them together and having the
downstream one act as a switch. But given there's only one output pin,
such a workaround doesn't match reality.
Remove the VCN33_WIFI regulator. During the probe phase, have the driver
sync the enable status of VCN33_WIFI to VCN33_BT. Also drop the suffix
so that the regulator name matches the pin name in the datasheet.
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
drivers/regulator/mt6358-regulator.c | 65 +++++++++++++++++-----
include/linux/regulator/mt6358-regulator.h | 6 +-
2 files changed, 52 insertions(+), 19 deletions(-)
diff --git a/drivers/regulator/mt6358-regulator.c b/drivers/regulator/mt6358-regulator.c
index c9e16bd092f6..faf6b0757019 100644
--- a/drivers/regulator/mt6358-regulator.c
+++ b/drivers/regulator/mt6358-regulator.c
@@ -277,7 +277,7 @@ static const unsigned int vcama_voltages[] = {
2800000, 2900000, 3000000,
};
-static const unsigned int vcn33_bt_wifi_voltages[] = {
+static const unsigned int vcn33_voltages[] = {
3300000, 3400000, 3500000,
};
@@ -321,7 +321,7 @@ static const u32 vcama_idx[] = {
0, 7, 9, 10, 11, 12,
};
-static const u32 vcn33_bt_wifi_idx[] = {
+static const u32 vcn33_idx[] = {
1, 2, 3,
};
@@ -566,12 +566,8 @@ static struct mt6358_regulator_info mt6358_regulators[] = {
MT6358_LDO_VCAMA1_CON0, 0, MT6358_VCAMA1_ANA_CON0, 0xf00),
MT6358_LDO("ldo_vemc", VEMC, vmch_vemc_voltages, vmch_vemc_idx,
MT6358_LDO_VEMC_CON0, 0, MT6358_VEMC_ANA_CON0, 0x700),
- MT6358_LDO("ldo_vcn33_bt", VCN33_BT, vcn33_bt_wifi_voltages,
- vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_0,
- 0, MT6358_VCN33_ANA_CON0, 0x300),
- MT6358_LDO("ldo_vcn33_wifi", VCN33_WIFI, vcn33_bt_wifi_voltages,
- vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_1,
- 0, MT6358_VCN33_ANA_CON0, 0x300),
+ MT6358_LDO("ldo_vcn33", VCN33, vcn33_voltages, vcn33_idx,
+ MT6358_LDO_VCN33_CON0_0, 0, MT6358_VCN33_ANA_CON0, 0x300),
MT6358_LDO("ldo_vcama2", VCAMA2, vcama_voltages, vcama_idx,
MT6358_LDO_VCAMA2_CON0, 0, MT6358_VCAMA2_ANA_CON0, 0xf00),
MT6358_LDO("ldo_vmc", VMC, vmc_voltages, vmc_idx,
@@ -662,12 +658,8 @@ static struct mt6358_regulator_info mt6366_regulators[] = {
MT6358_LDO_VMCH_CON0, 0, MT6358_VMCH_ANA_CON0, 0x700),
MT6366_LDO("ldo_vemc", VEMC, vmch_vemc_voltages, vmch_vemc_idx,
MT6358_LDO_VEMC_CON0, 0, MT6358_VEMC_ANA_CON0, 0x700),
- MT6366_LDO("ldo_vcn33_bt", VCN33_BT, vcn33_bt_wifi_voltages,
- vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_0,
- 0, MT6358_VCN33_ANA_CON0, 0x300),
- MT6366_LDO("ldo_vcn33_wifi", VCN33_WIFI, vcn33_bt_wifi_voltages,
- vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_1,
- 0, MT6358_VCN33_ANA_CON0, 0x300),
+ MT6366_LDO("ldo_vcn33", VCN33, vcn33_voltages, vcn33_idx,
+ MT6358_LDO_VCN33_CON0_0, 0, MT6358_VCN33_ANA_CON0, 0x300),
MT6366_LDO("ldo_vmc", VMC, vmc_voltages, vmc_idx,
MT6358_LDO_VMC_CON0, 0, MT6358_VMC_ANA_CON0, 0xf00),
MT6366_LDO("ldo_vsim2", VSIM2, vsim_voltages, vsim_idx,
@@ -690,13 +682,56 @@ static struct mt6358_regulator_info mt6366_regulators[] = {
MT6358_LDO_VSRAM_CON1, 0x7f),
};
+static int mt6358_sync_vcn33_setting(struct device *dev)
+{
+ struct mt6397_chip *mt6397 = dev_get_drvdata(dev->parent);
+ unsigned int val;
+ int ret;
+
+ /*
+ * VCN33_WIFI and VCN33_BT are two separate enable bits for the same
+ * regulator. They share the same voltage setting and output pin.
+ * Instead of having two potentially conflicting regulators, just have
+ * one VCN33 regulator. Sync the two enable bits and only use one in
+ * the regulator device.
+ */
+ ret = regmap_read(mt6397->regmap, MT6358_LDO_VCN33_CON0_1, &val);
+ if (ret) {
+ dev_err(dev, "Failed to read VCN33_WIFI setting\n");
+ return ret;
+ }
+
+ if (!(val & BIT(0)))
+ return 0;
+
+ /* Sync VCN33_WIFI enable status to VCN33_BT */
+ ret = regmap_update_bits(mt6397->regmap, MT6358_LDO_VCN33_CON0_0, BIT(0), BIT(0));
+ if (ret) {
+ dev_err(dev, "Failed to sync VCN33_WIFI setting to VCN33_BT\n");
+ return ret;
+ }
+
+ /* Disable VCN33_WIFI */
+ ret = regmap_update_bits(mt6397->regmap, MT6358_LDO_VCN33_CON0_1, BIT(0), 0);
+ if (ret) {
+ dev_err(dev, "Failed to disable VCN33_BT\n");
+ return ret;
+ }
+
+ return 0;
+}
+
static int mt6358_regulator_probe(struct platform_device *pdev)
{
struct mt6397_chip *mt6397 = dev_get_drvdata(pdev->dev.parent);
struct regulator_config config = {};
struct regulator_dev *rdev;
struct mt6358_regulator_info *mt6358_info;
- int i, max_regulator;
+ int i, max_regulator, ret;
+
+ ret = mt6358_sync_vcn33_setting(&pdev->dev);
+ if (ret)
+ return ret;
if (mt6397->chip_id == MT6366_CHIP_ID) {
max_regulator = MT6366_MAX_REGULATOR;
diff --git a/include/linux/regulator/mt6358-regulator.h b/include/linux/regulator/mt6358-regulator.h
index bdcf83cd719e..a4307cd9edd6 100644
--- a/include/linux/regulator/mt6358-regulator.h
+++ b/include/linux/regulator/mt6358-regulator.h
@@ -41,8 +41,7 @@ enum {
MT6358_ID_VIO28,
MT6358_ID_VA12,
MT6358_ID_VRF18,
- MT6358_ID_VCN33_BT,
- MT6358_ID_VCN33_WIFI,
+ MT6358_ID_VCN33,
MT6358_ID_VCAMA2,
MT6358_ID_VMC,
MT6358_ID_VLDO28,
@@ -85,8 +84,7 @@ enum {
MT6366_ID_VIO28,
MT6366_ID_VA12,
MT6366_ID_VRF18,
- MT6366_ID_VCN33_BT,
- MT6366_ID_VCN33_WIFI,
+ MT6366_ID_VCN33,
MT6366_ID_VMC,
MT6366_ID_VAUD28,
MT6366_ID_VSIM2,
--
2.41.0.162.gfafddb0af9-googOn Fri, Jun 9, 2023 at 4:30 PM Chen-Yu Tsai <wenst@chromium.org> wrote:
>
> The VCN33_BT and VCN33_WIFI regulators are actually the same regulator,
> having the same voltage setting and output pin. There are simply two
> enable bits that are ORed together to enable the regulator.
>
> Having two regulators representing the same output pin is misleading
> from a design matching standpoint, and also error-prone in driver
> implementations. If consumers try to set different voltages on either
> regulator, the one set later would override the one set before. There
> are ways around this, such as chaining them together and having the
> downstream one act as a switch. But given there's only one output pin,
> such a workaround doesn't match reality.
>
> Remove the VCN33_WIFI regulator. During the probe phase, have the driver
> sync the enable status of VCN33_WIFI to VCN33_BT. Also drop the suffix
> so that the regulator name matches the pin name in the datasheet.
>
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
> drivers/regulator/mt6358-regulator.c | 65 +++++++++++++++++-----
> include/linux/regulator/mt6358-regulator.h | 6 +-
> 2 files changed, 52 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/regulator/mt6358-regulator.c b/drivers/regulator/mt6358-regulator.c
> index c9e16bd092f6..faf6b0757019 100644
> --- a/drivers/regulator/mt6358-regulator.c
> +++ b/drivers/regulator/mt6358-regulator.c
> @@ -277,7 +277,7 @@ static const unsigned int vcama_voltages[] = {
> 2800000, 2900000, 3000000,
> };
>
> -static const unsigned int vcn33_bt_wifi_voltages[] = {
> +static const unsigned int vcn33_voltages[] = {
> 3300000, 3400000, 3500000,
> };
>
> @@ -321,7 +321,7 @@ static const u32 vcama_idx[] = {
> 0, 7, 9, 10, 11, 12,
> };
>
> -static const u32 vcn33_bt_wifi_idx[] = {
> +static const u32 vcn33_idx[] = {
> 1, 2, 3,
> };
>
> @@ -566,12 +566,8 @@ static struct mt6358_regulator_info mt6358_regulators[] = {
> MT6358_LDO_VCAMA1_CON0, 0, MT6358_VCAMA1_ANA_CON0, 0xf00),
> MT6358_LDO("ldo_vemc", VEMC, vmch_vemc_voltages, vmch_vemc_idx,
> MT6358_LDO_VEMC_CON0, 0, MT6358_VEMC_ANA_CON0, 0x700),
> - MT6358_LDO("ldo_vcn33_bt", VCN33_BT, vcn33_bt_wifi_voltages,
> - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_0,
> - 0, MT6358_VCN33_ANA_CON0, 0x300),
> - MT6358_LDO("ldo_vcn33_wifi", VCN33_WIFI, vcn33_bt_wifi_voltages,
> - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_1,
> - 0, MT6358_VCN33_ANA_CON0, 0x300),
> + MT6358_LDO("ldo_vcn33", VCN33, vcn33_voltages, vcn33_idx,
> + MT6358_LDO_VCN33_CON0_0, 0, MT6358_VCN33_ANA_CON0, 0x300),
> MT6358_LDO("ldo_vcama2", VCAMA2, vcama_voltages, vcama_idx,
> MT6358_LDO_VCAMA2_CON0, 0, MT6358_VCAMA2_ANA_CON0, 0xf00),
> MT6358_LDO("ldo_vmc", VMC, vmc_voltages, vmc_idx,
> @@ -662,12 +658,8 @@ static struct mt6358_regulator_info mt6366_regulators[] = {
> MT6358_LDO_VMCH_CON0, 0, MT6358_VMCH_ANA_CON0, 0x700),
> MT6366_LDO("ldo_vemc", VEMC, vmch_vemc_voltages, vmch_vemc_idx,
> MT6358_LDO_VEMC_CON0, 0, MT6358_VEMC_ANA_CON0, 0x700),
> - MT6366_LDO("ldo_vcn33_bt", VCN33_BT, vcn33_bt_wifi_voltages,
> - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_0,
> - 0, MT6358_VCN33_ANA_CON0, 0x300),
> - MT6366_LDO("ldo_vcn33_wifi", VCN33_WIFI, vcn33_bt_wifi_voltages,
> - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_1,
> - 0, MT6358_VCN33_ANA_CON0, 0x300),
> + MT6366_LDO("ldo_vcn33", VCN33, vcn33_voltages, vcn33_idx,
> + MT6358_LDO_VCN33_CON0_0, 0, MT6358_VCN33_ANA_CON0, 0x300),
> MT6366_LDO("ldo_vmc", VMC, vmc_voltages, vmc_idx,
> MT6358_LDO_VMC_CON0, 0, MT6358_VMC_ANA_CON0, 0xf00),
> MT6366_LDO("ldo_vsim2", VSIM2, vsim_voltages, vsim_idx,
> @@ -690,13 +682,56 @@ static struct mt6358_regulator_info mt6366_regulators[] = {
> MT6358_LDO_VSRAM_CON1, 0x7f),
> };
>
> +static int mt6358_sync_vcn33_setting(struct device *dev)
> +{
> + struct mt6397_chip *mt6397 = dev_get_drvdata(dev->parent);
> + unsigned int val;
> + int ret;
> +
> + /*
> + * VCN33_WIFI and VCN33_BT are two separate enable bits for the same
> + * regulator. They share the same voltage setting and output pin.
> + * Instead of having two potentially conflicting regulators, just have
> + * one VCN33 regulator. Sync the two enable bits and only use one in
> + * the regulator device.
> + */
> + ret = regmap_read(mt6397->regmap, MT6358_LDO_VCN33_CON0_1, &val);
> + if (ret) {
> + dev_err(dev, "Failed to read VCN33_WIFI setting\n");
> + return ret;
> + }
> +
> + if (!(val & BIT(0)))
> + return 0;
> +
> + /* Sync VCN33_WIFI enable status to VCN33_BT */
> + ret = regmap_update_bits(mt6397->regmap, MT6358_LDO_VCN33_CON0_0, BIT(0), BIT(0));
> + if (ret) {
> + dev_err(dev, "Failed to sync VCN33_WIFI setting to VCN33_BT\n");
> + return ret;
> + }
> +
> + /* Disable VCN33_WIFI */
> + ret = regmap_update_bits(mt6397->regmap, MT6358_LDO_VCN33_CON0_1, BIT(0), 0);
> + if (ret) {
> + dev_err(dev, "Failed to disable VCN33_BT\n");
I think it should be "VCN33_WIFI" in the error message?
Regards,
Fei
On Mon, Jun 12, 2023 at 6:57 PM Fei Shao <fshao@chromium.org> wrote:
>
> On Fri, Jun 9, 2023 at 4:30 PM Chen-Yu Tsai <wenst@chromium.org> wrote:
> >
> > The VCN33_BT and VCN33_WIFI regulators are actually the same regulator,
> > having the same voltage setting and output pin. There are simply two
> > enable bits that are ORed together to enable the regulator.
> >
> > Having two regulators representing the same output pin is misleading
> > from a design matching standpoint, and also error-prone in driver
> > implementations. If consumers try to set different voltages on either
> > regulator, the one set later would override the one set before. There
> > are ways around this, such as chaining them together and having the
> > downstream one act as a switch. But given there's only one output pin,
> > such a workaround doesn't match reality.
> >
> > Remove the VCN33_WIFI regulator. During the probe phase, have the driver
> > sync the enable status of VCN33_WIFI to VCN33_BT. Also drop the suffix
> > so that the regulator name matches the pin name in the datasheet.
> >
> > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > ---
> > drivers/regulator/mt6358-regulator.c | 65 +++++++++++++++++-----
> > include/linux/regulator/mt6358-regulator.h | 6 +-
> > 2 files changed, 52 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/regulator/mt6358-regulator.c b/drivers/regulator/mt6358-regulator.c
> > index c9e16bd092f6..faf6b0757019 100644
> > --- a/drivers/regulator/mt6358-regulator.c
> > +++ b/drivers/regulator/mt6358-regulator.c
> > @@ -277,7 +277,7 @@ static const unsigned int vcama_voltages[] = {
> > 2800000, 2900000, 3000000,
> > };
> >
> > -static const unsigned int vcn33_bt_wifi_voltages[] = {
> > +static const unsigned int vcn33_voltages[] = {
> > 3300000, 3400000, 3500000,
> > };
> >
> > @@ -321,7 +321,7 @@ static const u32 vcama_idx[] = {
> > 0, 7, 9, 10, 11, 12,
> > };
> >
> > -static const u32 vcn33_bt_wifi_idx[] = {
> > +static const u32 vcn33_idx[] = {
> > 1, 2, 3,
> > };
> >
> > @@ -566,12 +566,8 @@ static struct mt6358_regulator_info mt6358_regulators[] = {
> > MT6358_LDO_VCAMA1_CON0, 0, MT6358_VCAMA1_ANA_CON0, 0xf00),
> > MT6358_LDO("ldo_vemc", VEMC, vmch_vemc_voltages, vmch_vemc_idx,
> > MT6358_LDO_VEMC_CON0, 0, MT6358_VEMC_ANA_CON0, 0x700),
> > - MT6358_LDO("ldo_vcn33_bt", VCN33_BT, vcn33_bt_wifi_voltages,
> > - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_0,
> > - 0, MT6358_VCN33_ANA_CON0, 0x300),
> > - MT6358_LDO("ldo_vcn33_wifi", VCN33_WIFI, vcn33_bt_wifi_voltages,
> > - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_1,
> > - 0, MT6358_VCN33_ANA_CON0, 0x300),
> > + MT6358_LDO("ldo_vcn33", VCN33, vcn33_voltages, vcn33_idx,
> > + MT6358_LDO_VCN33_CON0_0, 0, MT6358_VCN33_ANA_CON0, 0x300),
> > MT6358_LDO("ldo_vcama2", VCAMA2, vcama_voltages, vcama_idx,
> > MT6358_LDO_VCAMA2_CON0, 0, MT6358_VCAMA2_ANA_CON0, 0xf00),
> > MT6358_LDO("ldo_vmc", VMC, vmc_voltages, vmc_idx,
> > @@ -662,12 +658,8 @@ static struct mt6358_regulator_info mt6366_regulators[] = {
> > MT6358_LDO_VMCH_CON0, 0, MT6358_VMCH_ANA_CON0, 0x700),
> > MT6366_LDO("ldo_vemc", VEMC, vmch_vemc_voltages, vmch_vemc_idx,
> > MT6358_LDO_VEMC_CON0, 0, MT6358_VEMC_ANA_CON0, 0x700),
> > - MT6366_LDO("ldo_vcn33_bt", VCN33_BT, vcn33_bt_wifi_voltages,
> > - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_0,
> > - 0, MT6358_VCN33_ANA_CON0, 0x300),
> > - MT6366_LDO("ldo_vcn33_wifi", VCN33_WIFI, vcn33_bt_wifi_voltages,
> > - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_1,
> > - 0, MT6358_VCN33_ANA_CON0, 0x300),
> > + MT6366_LDO("ldo_vcn33", VCN33, vcn33_voltages, vcn33_idx,
> > + MT6358_LDO_VCN33_CON0_0, 0, MT6358_VCN33_ANA_CON0, 0x300),
> > MT6366_LDO("ldo_vmc", VMC, vmc_voltages, vmc_idx,
> > MT6358_LDO_VMC_CON0, 0, MT6358_VMC_ANA_CON0, 0xf00),
> > MT6366_LDO("ldo_vsim2", VSIM2, vsim_voltages, vsim_idx,
> > @@ -690,13 +682,56 @@ static struct mt6358_regulator_info mt6366_regulators[] = {
> > MT6358_LDO_VSRAM_CON1, 0x7f),
> > };
> >
> > +static int mt6358_sync_vcn33_setting(struct device *dev)
> > +{
> > + struct mt6397_chip *mt6397 = dev_get_drvdata(dev->parent);
> > + unsigned int val;
> > + int ret;
> > +
> > + /*
> > + * VCN33_WIFI and VCN33_BT are two separate enable bits for the same
> > + * regulator. They share the same voltage setting and output pin.
> > + * Instead of having two potentially conflicting regulators, just have
> > + * one VCN33 regulator. Sync the two enable bits and only use one in
> > + * the regulator device.
> > + */
> > + ret = regmap_read(mt6397->regmap, MT6358_LDO_VCN33_CON0_1, &val);
> > + if (ret) {
> > + dev_err(dev, "Failed to read VCN33_WIFI setting\n");
> > + return ret;
> > + }
> > +
> > + if (!(val & BIT(0)))
> > + return 0;
> > +
> > + /* Sync VCN33_WIFI enable status to VCN33_BT */
> > + ret = regmap_update_bits(mt6397->regmap, MT6358_LDO_VCN33_CON0_0, BIT(0), BIT(0));
> > + if (ret) {
> > + dev_err(dev, "Failed to sync VCN33_WIFI setting to VCN33_BT\n");
> > + return ret;
> > + }
> > +
> > + /* Disable VCN33_WIFI */
> > + ret = regmap_update_bits(mt6397->regmap, MT6358_LDO_VCN33_CON0_1, BIT(0), 0);
> > + if (ret) {
> > + dev_err(dev, "Failed to disable VCN33_BT\n");
>
> I think it should be "VCN33_WIFI" in the error message?
Mark already merged the patch. I send followup fixes for this and the
call sequence.
ChenYu
On Fri, Jun 09, 2023 at 04:30:00PM +0800, Chen-Yu Tsai wrote:
> The VCN33_BT and VCN33_WIFI regulators are actually the same regulator,
> having the same voltage setting and output pin. There are simply two
> enable bits that are ORed together to enable the regulator.
>
> Having two regulators representing the same output pin is misleading
> from a design matching standpoint, and also error-prone in driver
> implementations. If consumers try to set different voltages on either
> regulator, the one set later would override the one set before. There
> are ways around this, such as chaining them together and having the
> downstream one act as a switch. But given there's only one output pin,
> such a workaround doesn't match reality.
>
> Remove the VCN33_WIFI regulator. During the probe phase, have the driver
> sync the enable status of VCN33_WIFI to VCN33_BT. Also drop the suffix
> so that the regulator name matches the pin name in the datasheet.
>
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
> drivers/regulator/mt6358-regulator.c | 65 +++++++++++++++++-----
> include/linux/regulator/mt6358-regulator.h | 6 +-
> 2 files changed, 52 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/regulator/mt6358-regulator.c b/drivers/regulator/mt6358-regulator.c
> index c9e16bd092f6..faf6b0757019 100644
> --- a/drivers/regulator/mt6358-regulator.c
> +++ b/drivers/regulator/mt6358-regulator.c
> @@ -277,7 +277,7 @@ static const unsigned int vcama_voltages[] = {
> 2800000, 2900000, 3000000,
> };
>
> -static const unsigned int vcn33_bt_wifi_voltages[] = {
> +static const unsigned int vcn33_voltages[] = {
> 3300000, 3400000, 3500000,
> };
>
> @@ -321,7 +321,7 @@ static const u32 vcama_idx[] = {
> 0, 7, 9, 10, 11, 12,
> };
>
> -static const u32 vcn33_bt_wifi_idx[] = {
> +static const u32 vcn33_idx[] = {
> 1, 2, 3,
> };
>
> @@ -566,12 +566,8 @@ static struct mt6358_regulator_info mt6358_regulators[] = {
> MT6358_LDO_VCAMA1_CON0, 0, MT6358_VCAMA1_ANA_CON0, 0xf00),
> MT6358_LDO("ldo_vemc", VEMC, vmch_vemc_voltages, vmch_vemc_idx,
> MT6358_LDO_VEMC_CON0, 0, MT6358_VEMC_ANA_CON0, 0x700),
> - MT6358_LDO("ldo_vcn33_bt", VCN33_BT, vcn33_bt_wifi_voltages,
> - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_0,
> - 0, MT6358_VCN33_ANA_CON0, 0x300),
> - MT6358_LDO("ldo_vcn33_wifi", VCN33_WIFI, vcn33_bt_wifi_voltages,
> - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_1,
> - 0, MT6358_VCN33_ANA_CON0, 0x300),
> + MT6358_LDO("ldo_vcn33", VCN33, vcn33_voltages, vcn33_idx,
> + MT6358_LDO_VCN33_CON0_0, 0, MT6358_VCN33_ANA_CON0, 0x300),
Excuse me if I am being daft here, but could you explain how this change
is compatible with existing devicetrees?
Thanks,
Conor.
On Fri, Jun 09, 2023 at 04:56:05PM +0100, Conor Dooley wrote:
> On Fri, Jun 09, 2023 at 04:30:00PM +0800, Chen-Yu Tsai wrote:
> > The VCN33_BT and VCN33_WIFI regulators are actually the same regulator,
> > having the same voltage setting and output pin. There are simply two
> > enable bits that are ORed together to enable the regulator.
> >
> > Having two regulators representing the same output pin is misleading
> > from a design matching standpoint, and also error-prone in driver
> > implementations. If consumers try to set different voltages on either
> > regulator, the one set later would override the one set before. There
> > are ways around this, such as chaining them together and having the
> > downstream one act as a switch. But given there's only one output pin,
> > such a workaround doesn't match reality.
> >
> > Remove the VCN33_WIFI regulator. During the probe phase, have the driver
> > sync the enable status of VCN33_WIFI to VCN33_BT. Also drop the suffix
> > so that the regulator name matches the pin name in the datasheet.
> >
> > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > ---
> > drivers/regulator/mt6358-regulator.c | 65 +++++++++++++++++-----
> > include/linux/regulator/mt6358-regulator.h | 6 +-
> > 2 files changed, 52 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/regulator/mt6358-regulator.c b/drivers/regulator/mt6358-regulator.c
> > index c9e16bd092f6..faf6b0757019 100644
> > --- a/drivers/regulator/mt6358-regulator.c
> > +++ b/drivers/regulator/mt6358-regulator.c
> > @@ -277,7 +277,7 @@ static const unsigned int vcama_voltages[] = {
> > 2800000, 2900000, 3000000,
> > };
> >
> > -static const unsigned int vcn33_bt_wifi_voltages[] = {
> > +static const unsigned int vcn33_voltages[] = {
> > 3300000, 3400000, 3500000,
> > };
> >
> > @@ -321,7 +321,7 @@ static const u32 vcama_idx[] = {
> > 0, 7, 9, 10, 11, 12,
> > };
> >
> > -static const u32 vcn33_bt_wifi_idx[] = {
> > +static const u32 vcn33_idx[] = {
> > 1, 2, 3,
> > };
> >
> > @@ -566,12 +566,8 @@ static struct mt6358_regulator_info mt6358_regulators[] = {
> > MT6358_LDO_VCAMA1_CON0, 0, MT6358_VCAMA1_ANA_CON0, 0xf00),
> > MT6358_LDO("ldo_vemc", VEMC, vmch_vemc_voltages, vmch_vemc_idx,
> > MT6358_LDO_VEMC_CON0, 0, MT6358_VEMC_ANA_CON0, 0x700),
> > - MT6358_LDO("ldo_vcn33_bt", VCN33_BT, vcn33_bt_wifi_voltages,
> > - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_0,
> > - 0, MT6358_VCN33_ANA_CON0, 0x300),
> > - MT6358_LDO("ldo_vcn33_wifi", VCN33_WIFI, vcn33_bt_wifi_voltages,
> > - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_1,
> > - 0, MT6358_VCN33_ANA_CON0, 0x300),
> > + MT6358_LDO("ldo_vcn33", VCN33, vcn33_voltages, vcn33_idx,
> > + MT6358_LDO_VCN33_CON0_0, 0, MT6358_VCN33_ANA_CON0, 0x300),
>
> Excuse me if I am being daft here, but could you explain how this change
> is compatible with existing devicetrees?
Ah, I see in the binding commit there's a "Luckily no device tree actually
uses them." Does that just cover the kernel, or does it consider other
operating systems/bootloaders?
Cheers,
Conor.
On Sat, Jun 10, 2023 at 11:28 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Fri, Jun 09, 2023 at 04:56:05PM +0100, Conor Dooley wrote:
> > On Fri, Jun 09, 2023 at 04:30:00PM +0800, Chen-Yu Tsai wrote:
> > > The VCN33_BT and VCN33_WIFI regulators are actually the same regulator,
> > > having the same voltage setting and output pin. There are simply two
> > > enable bits that are ORed together to enable the regulator.
> > >
> > > Having two regulators representing the same output pin is misleading
> > > from a design matching standpoint, and also error-prone in driver
> > > implementations. If consumers try to set different voltages on either
> > > regulator, the one set later would override the one set before. There
> > > are ways around this, such as chaining them together and having the
> > > downstream one act as a switch. But given there's only one output pin,
> > > such a workaround doesn't match reality.
> > >
> > > Remove the VCN33_WIFI regulator. During the probe phase, have the driver
> > > sync the enable status of VCN33_WIFI to VCN33_BT. Also drop the suffix
> > > so that the regulator name matches the pin name in the datasheet.
> > >
> > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > > ---
> > > drivers/regulator/mt6358-regulator.c | 65 +++++++++++++++++-----
> > > include/linux/regulator/mt6358-regulator.h | 6 +-
> > > 2 files changed, 52 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/drivers/regulator/mt6358-regulator.c b/drivers/regulator/mt6358-regulator.c
> > > index c9e16bd092f6..faf6b0757019 100644
> > > --- a/drivers/regulator/mt6358-regulator.c
> > > +++ b/drivers/regulator/mt6358-regulator.c
> > > @@ -277,7 +277,7 @@ static const unsigned int vcama_voltages[] = {
> > > 2800000, 2900000, 3000000,
> > > };
> > >
> > > -static const unsigned int vcn33_bt_wifi_voltages[] = {
> > > +static const unsigned int vcn33_voltages[] = {
> > > 3300000, 3400000, 3500000,
> > > };
> > >
> > > @@ -321,7 +321,7 @@ static const u32 vcama_idx[] = {
> > > 0, 7, 9, 10, 11, 12,
> > > };
> > >
> > > -static const u32 vcn33_bt_wifi_idx[] = {
> > > +static const u32 vcn33_idx[] = {
> > > 1, 2, 3,
> > > };
> > >
> > > @@ -566,12 +566,8 @@ static struct mt6358_regulator_info mt6358_regulators[] = {
> > > MT6358_LDO_VCAMA1_CON0, 0, MT6358_VCAMA1_ANA_CON0, 0xf00),
> > > MT6358_LDO("ldo_vemc", VEMC, vmch_vemc_voltages, vmch_vemc_idx,
> > > MT6358_LDO_VEMC_CON0, 0, MT6358_VEMC_ANA_CON0, 0x700),
> > > - MT6358_LDO("ldo_vcn33_bt", VCN33_BT, vcn33_bt_wifi_voltages,
> > > - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_0,
> > > - 0, MT6358_VCN33_ANA_CON0, 0x300),
> > > - MT6358_LDO("ldo_vcn33_wifi", VCN33_WIFI, vcn33_bt_wifi_voltages,
> > > - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_1,
> > > - 0, MT6358_VCN33_ANA_CON0, 0x300),
> > > + MT6358_LDO("ldo_vcn33", VCN33, vcn33_voltages, vcn33_idx,
> > > + MT6358_LDO_VCN33_CON0_0, 0, MT6358_VCN33_ANA_CON0, 0x300),
> >
> > Excuse me if I am being daft here, but could you explain how this change
> > is compatible with existing devicetrees?
>
> Ah, I see in the binding commit there's a "Luckily no device tree actually
> uses them." Does that just cover the kernel, or does it consider other
> operating systems/bootloaders?
That comment covers the upstream kernel and the downstream ChromeOS kernel
specifically. The bootloader that ChromeOS uses (coreboot) doesn't use
device trees. I don't know what MediaTek uses for their phones though.
AFAIK MediaTek only supports the Linux kernel, be it for Android or ChromeOS.
There's not a large community around it, unlike some of the other ARM SoCs.
I did find an old v4.4 Android kernel [1] for the MediaTek Helio P60
(MT6771) that is also paired with MT6358. There are no device tree
references to the VCN33 regulator either. Only the definition exists
in the mt6358.dtsi file, much like what we have upstream.
As far as the regulator driver goes, if it can't find a matching regulator
node, it's the same as if the node doesn't exist, and therefore the given
constraints are not ingested. If no constraints are ingested that can
turn it on, and no consumer references to enable it either, we can say
that the regulator is effectively unused.
ChenYu
[1] https://github.com/nokia-dev/android_kernel_nokia_mt6771
On Mon, Jun 12, 2023 at 12:19:01PM +0800, Chen-Yu Tsai wrote: > On Sat, Jun 10, 2023 at 11:28 PM Conor Dooley <conor@kernel.org> wrote: > > Ah, I see in the binding commit there's a "Luckily no device tree actually > > uses them." Does that just cover the kernel, or does it consider other > > operating systems/bootloaders? > > That comment covers the upstream kernel and the downstream ChromeOS kernel > specifically. The bootloader that ChromeOS uses (coreboot) doesn't use > device trees. I don't know what MediaTek uses for their phones though. > > AFAIK MediaTek only supports the Linux kernel, be it for Android or ChromeOS. > There's not a large community around it, unlike some of the other ARM SoCs. > > I did find an old v4.4 Android kernel [1] for the MediaTek Helio P60 > (MT6771) that is also paired with MT6358. There are no device tree > references to the VCN33 regulator either. Only the definition exists > in the mt6358.dtsi file, much like what we have upstream. > > As far as the regulator driver goes, if it can't find a matching regulator > node, it's the same as if the node doesn't exist, and therefore the given > constraints are not ingested. If no constraints are ingested that can > turn it on, and no consumer references to enable it either, we can say > that the regulator is effectively unused. Okay, that sounds reasonable. Seems like you've done your research, so thanks for that!
Il 09/06/23 10:30, Chen-Yu Tsai ha scritto:
> The VCN33_BT and VCN33_WIFI regulators are actually the same regulator,
> having the same voltage setting and output pin. There are simply two
> enable bits that are ORed together to enable the regulator.
>
> Having two regulators representing the same output pin is misleading
> from a design matching standpoint, and also error-prone in driver
> implementations. If consumers try to set different voltages on either
> regulator, the one set later would override the one set before. There
> are ways around this, such as chaining them together and having the
> downstream one act as a switch. But given there's only one output pin,
> such a workaround doesn't match reality.
>
> Remove the VCN33_WIFI regulator. During the probe phase, have the driver
> sync the enable status of VCN33_WIFI to VCN33_BT. Also drop the suffix
> so that the regulator name matches the pin name in the datasheet.
>
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
> drivers/regulator/mt6358-regulator.c | 65 +++++++++++++++++-----
> include/linux/regulator/mt6358-regulator.h | 6 +-
> 2 files changed, 52 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/regulator/mt6358-regulator.c b/drivers/regulator/mt6358-regulator.c
> index c9e16bd092f6..faf6b0757019 100644
> --- a/drivers/regulator/mt6358-regulator.c
> +++ b/drivers/regulator/mt6358-regulator.c
> @@ -277,7 +277,7 @@ static const unsigned int vcama_voltages[] = {
> 2800000, 2900000, 3000000,
> };
>
> -static const unsigned int vcn33_bt_wifi_voltages[] = {
> +static const unsigned int vcn33_voltages[] = {
> 3300000, 3400000, 3500000,
> };
>
> @@ -321,7 +321,7 @@ static const u32 vcama_idx[] = {
> 0, 7, 9, 10, 11, 12,
> };
>
> -static const u32 vcn33_bt_wifi_idx[] = {
> +static const u32 vcn33_idx[] = {
> 1, 2, 3,
> };
>
> @@ -566,12 +566,8 @@ static struct mt6358_regulator_info mt6358_regulators[] = {
> MT6358_LDO_VCAMA1_CON0, 0, MT6358_VCAMA1_ANA_CON0, 0xf00),
> MT6358_LDO("ldo_vemc", VEMC, vmch_vemc_voltages, vmch_vemc_idx,
> MT6358_LDO_VEMC_CON0, 0, MT6358_VEMC_ANA_CON0, 0x700),
> - MT6358_LDO("ldo_vcn33_bt", VCN33_BT, vcn33_bt_wifi_voltages,
> - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_0,
> - 0, MT6358_VCN33_ANA_CON0, 0x300),
> - MT6358_LDO("ldo_vcn33_wifi", VCN33_WIFI, vcn33_bt_wifi_voltages,
> - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_1,
> - 0, MT6358_VCN33_ANA_CON0, 0x300),
> + MT6358_LDO("ldo_vcn33", VCN33, vcn33_voltages, vcn33_idx,
> + MT6358_LDO_VCN33_CON0_0, 0, MT6358_VCN33_ANA_CON0, 0x300),
> MT6358_LDO("ldo_vcama2", VCAMA2, vcama_voltages, vcama_idx,
> MT6358_LDO_VCAMA2_CON0, 0, MT6358_VCAMA2_ANA_CON0, 0xf00),
> MT6358_LDO("ldo_vmc", VMC, vmc_voltages, vmc_idx,
> @@ -662,12 +658,8 @@ static struct mt6358_regulator_info mt6366_regulators[] = {
> MT6358_LDO_VMCH_CON0, 0, MT6358_VMCH_ANA_CON0, 0x700),
> MT6366_LDO("ldo_vemc", VEMC, vmch_vemc_voltages, vmch_vemc_idx,
> MT6358_LDO_VEMC_CON0, 0, MT6358_VEMC_ANA_CON0, 0x700),
> - MT6366_LDO("ldo_vcn33_bt", VCN33_BT, vcn33_bt_wifi_voltages,
> - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_0,
> - 0, MT6358_VCN33_ANA_CON0, 0x300),
> - MT6366_LDO("ldo_vcn33_wifi", VCN33_WIFI, vcn33_bt_wifi_voltages,
> - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_1,
> - 0, MT6358_VCN33_ANA_CON0, 0x300),
> + MT6366_LDO("ldo_vcn33", VCN33, vcn33_voltages, vcn33_idx,
> + MT6358_LDO_VCN33_CON0_0, 0, MT6358_VCN33_ANA_CON0, 0x300),
> MT6366_LDO("ldo_vmc", VMC, vmc_voltages, vmc_idx,
> MT6358_LDO_VMC_CON0, 0, MT6358_VMC_ANA_CON0, 0xf00),
> MT6366_LDO("ldo_vsim2", VSIM2, vsim_voltages, vsim_idx,
> @@ -690,13 +682,56 @@ static struct mt6358_regulator_info mt6366_regulators[] = {
> MT6358_LDO_VSRAM_CON1, 0x7f),
> };
>
> +static int mt6358_sync_vcn33_setting(struct device *dev)
> +{
> + struct mt6397_chip *mt6397 = dev_get_drvdata(dev->parent);
> + unsigned int val;
> + int ret;
> +
> + /*
> + * VCN33_WIFI and VCN33_BT are two separate enable bits for the same
> + * regulator. They share the same voltage setting and output pin.
> + * Instead of having two potentially conflicting regulators, just have
> + * one VCN33 regulator. Sync the two enable bits and only use one in
> + * the regulator device.
> + */
> + ret = regmap_read(mt6397->regmap, MT6358_LDO_VCN33_CON0_1, &val);
> + if (ret) {
> + dev_err(dev, "Failed to read VCN33_WIFI setting\n");
> + return ret;
> + }
> +
> + if (!(val & BIT(0)))
> + return 0;
> +
> + /* Sync VCN33_WIFI enable status to VCN33_BT */
> + ret = regmap_update_bits(mt6397->regmap, MT6358_LDO_VCN33_CON0_0, BIT(0), BIT(0));
> + if (ret) {
> + dev_err(dev, "Failed to sync VCN33_WIFI setting to VCN33_BT\n");
> + return ret;
> + }
> +
> + /* Disable VCN33_WIFI */
> + ret = regmap_update_bits(mt6397->regmap, MT6358_LDO_VCN33_CON0_1, BIT(0), 0);
> + if (ret) {
> + dev_err(dev, "Failed to disable VCN33_BT\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> static int mt6358_regulator_probe(struct platform_device *pdev)
> {
> struct mt6397_chip *mt6397 = dev_get_drvdata(pdev->dev.parent);
> struct regulator_config config = {};
> struct regulator_dev *rdev;
> struct mt6358_regulator_info *mt6358_info;
> - int i, max_regulator;
> + int i, max_regulator, ret;
> +
> + ret = mt6358_sync_vcn33_setting(&pdev->dev);
> + if (ret)
> + return ret;
I'd put this after the chip_id check, and I would also add a safety check for
that...
switch (mt6397->chip_id) {
case MT6366_CHIP_ID:
max_regulator = MT6366_MAX_REGULATOR;
mt6358_info = mt6366_regulators;
break;
case MT6358_CHIP_ID:
max_regulator = MT6358_MAX_REGULATOR;
mt6358_info = mt6358_regulators;
break;
default:
return -EINVAL;
}
ret = mt6358_sync_vcn33_setting(....)
...but I agree with your point here about this being a strange design and
also with your way of fixing the driver.
Regards,
Angelo
On Fri, Jun 9, 2023 at 4:58 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 09/06/23 10:30, Chen-Yu Tsai ha scritto:
> > The VCN33_BT and VCN33_WIFI regulators are actually the same regulator,
> > having the same voltage setting and output pin. There are simply two
> > enable bits that are ORed together to enable the regulator.
> >
> > Having two regulators representing the same output pin is misleading
> > from a design matching standpoint, and also error-prone in driver
> > implementations. If consumers try to set different voltages on either
> > regulator, the one set later would override the one set before. There
> > are ways around this, such as chaining them together and having the
> > downstream one act as a switch. But given there's only one output pin,
> > such a workaround doesn't match reality.
> >
> > Remove the VCN33_WIFI regulator. During the probe phase, have the driver
> > sync the enable status of VCN33_WIFI to VCN33_BT. Also drop the suffix
> > so that the regulator name matches the pin name in the datasheet.
> >
> > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > ---
> > drivers/regulator/mt6358-regulator.c | 65 +++++++++++++++++-----
> > include/linux/regulator/mt6358-regulator.h | 6 +-
> > 2 files changed, 52 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/regulator/mt6358-regulator.c b/drivers/regulator/mt6358-regulator.c
> > index c9e16bd092f6..faf6b0757019 100644
> > --- a/drivers/regulator/mt6358-regulator.c
> > +++ b/drivers/regulator/mt6358-regulator.c
> > @@ -277,7 +277,7 @@ static const unsigned int vcama_voltages[] = {
> > 2800000, 2900000, 3000000,
> > };
> >
> > -static const unsigned int vcn33_bt_wifi_voltages[] = {
> > +static const unsigned int vcn33_voltages[] = {
> > 3300000, 3400000, 3500000,
> > };
> >
> > @@ -321,7 +321,7 @@ static const u32 vcama_idx[] = {
> > 0, 7, 9, 10, 11, 12,
> > };
> >
> > -static const u32 vcn33_bt_wifi_idx[] = {
> > +static const u32 vcn33_idx[] = {
> > 1, 2, 3,
> > };
> >
> > @@ -566,12 +566,8 @@ static struct mt6358_regulator_info mt6358_regulators[] = {
> > MT6358_LDO_VCAMA1_CON0, 0, MT6358_VCAMA1_ANA_CON0, 0xf00),
> > MT6358_LDO("ldo_vemc", VEMC, vmch_vemc_voltages, vmch_vemc_idx,
> > MT6358_LDO_VEMC_CON0, 0, MT6358_VEMC_ANA_CON0, 0x700),
> > - MT6358_LDO("ldo_vcn33_bt", VCN33_BT, vcn33_bt_wifi_voltages,
> > - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_0,
> > - 0, MT6358_VCN33_ANA_CON0, 0x300),
> > - MT6358_LDO("ldo_vcn33_wifi", VCN33_WIFI, vcn33_bt_wifi_voltages,
> > - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_1,
> > - 0, MT6358_VCN33_ANA_CON0, 0x300),
> > + MT6358_LDO("ldo_vcn33", VCN33, vcn33_voltages, vcn33_idx,
> > + MT6358_LDO_VCN33_CON0_0, 0, MT6358_VCN33_ANA_CON0, 0x300),
> > MT6358_LDO("ldo_vcama2", VCAMA2, vcama_voltages, vcama_idx,
> > MT6358_LDO_VCAMA2_CON0, 0, MT6358_VCAMA2_ANA_CON0, 0xf00),
> > MT6358_LDO("ldo_vmc", VMC, vmc_voltages, vmc_idx,
> > @@ -662,12 +658,8 @@ static struct mt6358_regulator_info mt6366_regulators[] = {
> > MT6358_LDO_VMCH_CON0, 0, MT6358_VMCH_ANA_CON0, 0x700),
> > MT6366_LDO("ldo_vemc", VEMC, vmch_vemc_voltages, vmch_vemc_idx,
> > MT6358_LDO_VEMC_CON0, 0, MT6358_VEMC_ANA_CON0, 0x700),
> > - MT6366_LDO("ldo_vcn33_bt", VCN33_BT, vcn33_bt_wifi_voltages,
> > - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_0,
> > - 0, MT6358_VCN33_ANA_CON0, 0x300),
> > - MT6366_LDO("ldo_vcn33_wifi", VCN33_WIFI, vcn33_bt_wifi_voltages,
> > - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_1,
> > - 0, MT6358_VCN33_ANA_CON0, 0x300),
> > + MT6366_LDO("ldo_vcn33", VCN33, vcn33_voltages, vcn33_idx,
> > + MT6358_LDO_VCN33_CON0_0, 0, MT6358_VCN33_ANA_CON0, 0x300),
> > MT6366_LDO("ldo_vmc", VMC, vmc_voltages, vmc_idx,
> > MT6358_LDO_VMC_CON0, 0, MT6358_VMC_ANA_CON0, 0xf00),
> > MT6366_LDO("ldo_vsim2", VSIM2, vsim_voltages, vsim_idx,
> > @@ -690,13 +682,56 @@ static struct mt6358_regulator_info mt6366_regulators[] = {
> > MT6358_LDO_VSRAM_CON1, 0x7f),
> > };
> >
> > +static int mt6358_sync_vcn33_setting(struct device *dev)
> > +{
> > + struct mt6397_chip *mt6397 = dev_get_drvdata(dev->parent);
> > + unsigned int val;
> > + int ret;
> > +
> > + /*
> > + * VCN33_WIFI and VCN33_BT are two separate enable bits for the same
> > + * regulator. They share the same voltage setting and output pin.
> > + * Instead of having two potentially conflicting regulators, just have
> > + * one VCN33 regulator. Sync the two enable bits and only use one in
> > + * the regulator device.
> > + */
> > + ret = regmap_read(mt6397->regmap, MT6358_LDO_VCN33_CON0_1, &val);
> > + if (ret) {
> > + dev_err(dev, "Failed to read VCN33_WIFI setting\n");
> > + return ret;
> > + }
> > +
> > + if (!(val & BIT(0)))
> > + return 0;
> > +
> > + /* Sync VCN33_WIFI enable status to VCN33_BT */
> > + ret = regmap_update_bits(mt6397->regmap, MT6358_LDO_VCN33_CON0_0, BIT(0), BIT(0));
> > + if (ret) {
> > + dev_err(dev, "Failed to sync VCN33_WIFI setting to VCN33_BT\n");
> > + return ret;
> > + }
> > +
> > + /* Disable VCN33_WIFI */
> > + ret = regmap_update_bits(mt6397->regmap, MT6358_LDO_VCN33_CON0_1, BIT(0), 0);
> > + if (ret) {
> > + dev_err(dev, "Failed to disable VCN33_BT\n");
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int mt6358_regulator_probe(struct platform_device *pdev)
> > {
> > struct mt6397_chip *mt6397 = dev_get_drvdata(pdev->dev.parent);
> > struct regulator_config config = {};
> > struct regulator_dev *rdev;
> > struct mt6358_regulator_info *mt6358_info;
> > - int i, max_regulator;
> > + int i, max_regulator, ret;
> > +
> > + ret = mt6358_sync_vcn33_setting(&pdev->dev);
> > + if (ret)
> > + return ret;
>
> I'd put this after the chip_id check, and I would also add a safety check for
> that...
>
> switch (mt6397->chip_id) {
> case MT6366_CHIP_ID:
> max_regulator = MT6366_MAX_REGULATOR;
> mt6358_info = mt6366_regulators;
> break;
> case MT6358_CHIP_ID:
> max_regulator = MT6358_MAX_REGULATOR;
> mt6358_info = mt6358_regulators;
> break;
> default:
> return -EINVAL;
> }
>
> ret = mt6358_sync_vcn33_setting(....)
Sounds good. We wouldn't want to be poking random bits in some other PMIC.
> ...but I agree with your point here about this being a strange design and
> also with your way of fixing the driver.
What I heard was that they support separate Bluetooth and WiFi drivers that
don't have a common reference counting framework for their regulator
supplies using this scheme. Maybe they are doing the power sequencing in
some small firmware.
ChenYu
© 2016 - 2024 Red Hat, Inc.