[PATCH-v2 0/2] regulator: 88pm800: Add dual phase mode support on BUCK1

[PATCH-v2 0/2] regulator: 88pm800: Add dual phase mode support on BUCK1

[Vaibhav Hiremath]

88PM860 device supports dual phase mode on BUCK1 output.
In normal usecase, BUCK1A and BUCK1B operates independently with 3A
capacity. And they both can work as a dual phase providing 6A capacity.

This patch series is subset of earlier patch-series
Link to earlier series - https://lkml.org/lkml/2015/7/16/722

Except PATCH[5/5], all other patches in the series are accepted and
queued up for next merge window.
And based on discussion on the list, creating DT property to enable
dual-phase mode on BUCK1.

Testing:
 - Tested on 88PM860 based platform
 - Boot tested 
 - Tested with & without DT property being set
 - Read register value before and after probe to make sure that
   value has been set.

V1 => V2:
========
 - This is new patch-series, where, all accepted patches dropped.
   Upgraded Patch version, to ease review.
 - Based on Mark Brown's comment, we should use DT proeprty of its own.
   using set_current_limit() is not right way here.
   So, created DT property for Dual phase mode enable.
 - Updated binding for new DT property


Vaibhav Hiremath (2):
  mfd: devicetree: bindings: 88pm800: Add DT property for dual phase
    enable
  regulator: 88pm800: Add support for configuration of dual phase on
    BUCK1

 Documentation/devicetree/bindings/mfd/88pm800.txt |  6 +++++
 drivers/regulator/88pm800.c                       | 31 +++++++++++++++++++++++
 include/linux/mfd/88pm80x.h                       |  3 +++
 3 files changed, 40 insertions(+)

-- 
1.9.1

[PATCH-v2 1/2] mfd: devicetree: bindings: 88pm800: Add DT property for dual phase enable

[Vaibhav Hiremath]

88PM860 family of device supports dual phase mode on BUCK1 supply
providing total 6A capacity.
Note that by default they operate independently with 3A capacity.

This patch updates the devicetree binding with DT property
to enable dual-phase mode on BUCK1.

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 Documentation/devicetree/bindings/mfd/88pm800.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/88pm800.txt b/Documentation/devicetree/bindings/mfd/88pm800.txt
index dec842f..2c82fcb 100644
--- a/Documentation/devicetree/bindings/mfd/88pm800.txt
+++ b/Documentation/devicetree/bindings/mfd/88pm800.txt
@@ -9,6 +9,12 @@ Required parent device properties:
 - #interrupt-cells 	: should be 1.
 			  The cell is the 88pm80x local IRQ number
 
+Optional properties :
+- marvell,88pm860-buck1-dualphase-en  : If set, enable dual phase on BUCK1,
+  providing 6A capacity.
+  Without this both BUCK1A and BUCK1B operates independently with 3A capacity.
+  (This property is only applicable to 88PM860)
+
 88pm80x family of devices consists of varied group of sub-devices:
 
 Device		 	Supply Names	 Description
-- 
1.9.1

Re: [PATCH-v2 1/2] mfd: devicetree: bindings: 88pm800: Add DT property for dual phase enable

[Lee Jones]

On Tue, 21 Jul 2015, Vaibhav Hiremath wrote:

> 88PM860 family of device supports dual phase mode on BUCK1 supply
> providing total 6A capacity.
> Note that by default they operate independently with 3A capacity.
> 
> This patch updates the devicetree binding with DT property
> to enable dual-phase mode on BUCK1.
> 
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> ---
>  Documentation/devicetree/bindings/mfd/88pm800.txt | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/88pm800.txt b/Documentation/devicetree/bindings/mfd/88pm800.txt
> index dec842f..2c82fcb 100644
> --- a/Documentation/devicetree/bindings/mfd/88pm800.txt
> +++ b/Documentation/devicetree/bindings/mfd/88pm800.txt
> @@ -9,6 +9,12 @@ Required parent device properties:
>  - #interrupt-cells 	: should be 1.
>  			  The cell is the 88pm80x local IRQ number
>  
> +Optional properties :
> +- marvell,88pm860-buck1-dualphase-en  : If set, enable dual phase on BUCK1,
> +  providing 6A capacity.
> +  Without this both BUCK1A and BUCK1B operates independently with 3A capacity.
> +  (This property is only applicable to 88PM860)

You need to get Mark Brown to Ack this.

>  88pm80x family of devices consists of varied group of sub-devices:
>  
>  Device		 	Supply Names	 Description

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH-v2 1/2] mfd: devicetree: bindings: 88pm800: Add DT property for dual phase enable

[Vaibhav Hiremath]

On Thursday 23 July 2015 09:23 PM, Lee Jones wrote:
> On Tue, 21 Jul 2015, Vaibhav Hiremath wrote:
>
>> 88PM860 family of device supports dual phase mode on BUCK1 supply
>> providing total 6A capacity.
>> Note that by default they operate independently with 3A capacity.
>>
>> This patch updates the devicetree binding with DT property
>> to enable dual-phase mode on BUCK1.
>>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>> ---
>>   Documentation/devicetree/bindings/mfd/88pm800.txt | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/88pm800.txt b/Documentation/devicetree/bindings/mfd/88pm800.txt
>> index dec842f..2c82fcb 100644
>> --- a/Documentation/devicetree/bindings/mfd/88pm800.txt
>> +++ b/Documentation/devicetree/bindings/mfd/88pm800.txt
>> @@ -9,6 +9,12 @@ Required parent device properties:
>>   - #interrupt-cells 	: should be 1.
>>   			  The cell is the 88pm80x local IRQ number
>>
>> +Optional properties :
>> +- marvell,88pm860-buck1-dualphase-en  : If set, enable dual phase on BUCK1,
>> +  providing 6A capacity.
>> +  Without this both BUCK1A and BUCK1B operates independently with 3A capacity.
>> +  (This property is only applicable to 88PM860)
>
> You need to get Mark Brown to Ack this.
>

Mark,

Ay update on this patch?

Thanks,
Vaibhav

[PATCH-v2 2/2] regulator: 88pm800: Add support for configuration of dual phase on BUCK1

[Vaibhav Hiremath]

88PM860 device supports dual phase mode on BUCK1 output.
In normal usecase, BUCK1A and BUCK1B operates independently with 3A
capacity. And they both can work as a dual phase providing 6A capacity.

This patch updates the regulator driver to read the respective
DT property and enable dual-phase mode on BUCK1.

Note that, this is init time (one time) initialization.

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 drivers/regulator/88pm800.c | 31 +++++++++++++++++++++++++++++++
 include/linux/mfd/88pm80x.h |  3 +++
 2 files changed, 34 insertions(+)

diff --git a/drivers/regulator/88pm800.c b/drivers/regulator/88pm800.c
index e846e4c..1bf2b35 100644
--- a/drivers/regulator/88pm800.c
+++ b/drivers/regulator/88pm800.c
@@ -267,6 +267,31 @@ static struct pm800_regulator_info pm860_regulator_info[] = {
 	PM800_LDO(ldo20, LDO20, LDO_ENA1_3, 3, 10000, ldo_volt_table2),
 };
 
+static int pm800_regulator_init(struct platform_device *pdev)
+{
+	struct pm800_regulators *pm800_data = platform_get_drvdata(pdev);
+	struct pm80x_chip *chip = pm800_data->chip;
+	int ret;
+
+	/* Currently only supported on 88pm860 device */
+	if (chip->type != CHIP_PM860)
+		return 0;
+
+	if (of_property_read_bool(pdev->dev.of_node,
+				"marvell,88pm860-buck1-dualphase-en")) {
+		ret = regmap_update_bits(chip->subchip->regmap_power,
+					PM860_BUCK1_MISC,
+					BUCK1_DUAL_PHASE_SEL,
+					BUCK1_DUAL_PHASE_SEL);
+		if (ret) {
+			dev_err(chip->dev, "failed to set dual-pase mode %d\n", ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
 static int pm800_regulator_probe(struct platform_device *pdev)
 {
 	struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
@@ -336,6 +361,12 @@ static int pm800_regulator_probe(struct platform_device *pdev)
 		}
 	}
 
+	ret = pm800_regulator_init(pdev);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to init 88pm800 regulator device\n");
+		return ret;
+	}
+
 	return 0;
 }
 
diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
index a92d173..05d9bad 100644
--- a/include/linux/mfd/88pm80x.h
+++ b/include/linux/mfd/88pm80x.h
@@ -295,6 +295,9 @@ enum {
 #define PM860_BUCK4_MISC2		(0x82)
 #define PM860_BUCK4_FULL_DRV		BIT(2)
 
+#define PM860_BUCK1_MISC		(0x8E)
+#define BUCK1_DUAL_PHASE_SEL		BIT(2)
+
 struct pm80x_rtc_pdata {
 	int		vrtc;
 	int		rtc_wakeup;
-- 
1.9.1

Re: [PATCH-v2 2/2] regulator: 88pm800: Add support for configuration of dual phase on BUCK1

[Krzysztof Kozlowski]

2015-07-22 1:23 GMT+09:00 Vaibhav Hiremath <vaibhav.hiremath@linaro.org>:
> 88PM860 device supports dual phase mode on BUCK1 output.
> In normal usecase, BUCK1A and BUCK1B operates independently with 3A
> capacity. And they both can work as a dual phase providing 6A capacity.
>
> This patch updates the regulator driver to read the respective
> DT property and enable dual-phase mode on BUCK1.
>
> Note that, this is init time (one time) initialization.
>
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> ---
>  drivers/regulator/88pm800.c | 31 +++++++++++++++++++++++++++++++
>  include/linux/mfd/88pm80x.h |  3 +++
>  2 files changed, 34 insertions(+)

Don't you need to update the constraints also? I think the BUCK1
regulator has fixed constraint of 3 A:
  PM800_BUCK(buck1, BUCK1, BUCK_ENA, 0, 3000000, buck1_volt_range, 0x55),
and now it can handle 6 A.

>
> diff --git a/drivers/regulator/88pm800.c b/drivers/regulator/88pm800.c
> index e846e4c..1bf2b35 100644
> --- a/drivers/regulator/88pm800.c
> +++ b/drivers/regulator/88pm800.c
> @@ -267,6 +267,31 @@ static struct pm800_regulator_info pm860_regulator_info[] = {
>         PM800_LDO(ldo20, LDO20, LDO_ENA1_3, 3, 10000, ldo_volt_table2),
>  };
>
> +static int pm800_regulator_init(struct platform_device *pdev)
> +{
> +       struct pm800_regulators *pm800_data = platform_get_drvdata(pdev);
> +       struct pm80x_chip *chip = pm800_data->chip;
> +       int ret;

'ret' is used only in if statement below. I don't have strong feelings
but can you move it there to limit its scope or always return 'ret'
(after initializing to '0'). To me this would be more readable.

Best regards,
Krzysztof

> +
> +       /* Currently only supported on 88pm860 device */
> +       if (chip->type != CHIP_PM860)
> +               return 0;
> +
> +       if (of_property_read_bool(pdev->dev.of_node,
> +                               "marvell,88pm860-buck1-dualphase-en")) {
> +               ret = regmap_update_bits(chip->subchip->regmap_power,
> +                                       PM860_BUCK1_MISC,
> +                                       BUCK1_DUAL_PHASE_SEL,
> +                                       BUCK1_DUAL_PHASE_SEL);
> +               if (ret) {
> +                       dev_err(chip->dev, "failed to set dual-pase mode %d\n", ret);
> +                       return ret;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
>  static int pm800_regulator_probe(struct platform_device *pdev)
>  {
>         struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
> @@ -336,6 +361,12 @@ static int pm800_regulator_probe(struct platform_device *pdev)
>                 }
>         }
>
> +       ret = pm800_regulator_init(pdev);
> +       if (ret) {
> +               dev_err(&pdev->dev, "failed to init 88pm800 regulator device\n");
> +               return ret;
> +       }
> +
>         return 0;
>  }
>
> diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
> index a92d173..05d9bad 100644
> --- a/include/linux/mfd/88pm80x.h
> +++ b/include/linux/mfd/88pm80x.h
> @@ -295,6 +295,9 @@ enum {
>  #define PM860_BUCK4_MISC2              (0x82)
>  #define PM860_BUCK4_FULL_DRV           BIT(2)
>
> +#define PM860_BUCK1_MISC               (0x8E)
> +#define BUCK1_DUAL_PHASE_SEL           BIT(2)
> +
>  struct pm80x_rtc_pdata {
>         int             vrtc;
>         int             rtc_wakeup;
> --
> 1.9.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH-v2 2/2] regulator: 88pm800: Add support for configuration of dual phase on BUCK1

[Vaibhav Hiremath]

On Thursday 23 July 2015 10:21 AM, Krzysztof Kozlowski wrote:
> 2015-07-22 1:23 GMT+09:00 Vaibhav Hiremath <vaibhav.hiremath@linaro.org>:
>> 88PM860 device supports dual phase mode on BUCK1 output.
>> In normal usecase, BUCK1A and BUCK1B operates independently with 3A
>> capacity. And they both can work as a dual phase providing 6A capacity.
>>
>> This patch updates the regulator driver to read the respective
>> DT property and enable dual-phase mode on BUCK1.
>>
>> Note that, this is init time (one time) initialization.
>>

Sorry for delayed response, was on bed rest almost for week.

>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>> ---
>>   drivers/regulator/88pm800.c | 31 +++++++++++++++++++++++++++++++
>>   include/linux/mfd/88pm80x.h |  3 +++
>>   2 files changed, 34 insertions(+)
>
> Don't you need to update the constraints also? I think the BUCK1
> regulator has fixed constraint of 3 A:
>    PM800_BUCK(buck1, BUCK1, BUCK_ENA, 0, 3000000, buck1_volt_range, 0x55),
> and now it can handle 6 A.
>

Actually, BUCK1A and BUCK1B both combined together provide 6A capacity.
And as discussed earlier, we need board change for this.

I am quite not sure.

Should I read the property and update the constraint runtime during
probe?


>>
>> diff --git a/drivers/regulator/88pm800.c b/drivers/regulator/88pm800.c
>> index e846e4c..1bf2b35 100644
>> --- a/drivers/regulator/88pm800.c
>> +++ b/drivers/regulator/88pm800.c
>> @@ -267,6 +267,31 @@ static struct pm800_regulator_info pm860_regulator_info[] = {
>>          PM800_LDO(ldo20, LDO20, LDO_ENA1_3, 3, 10000, ldo_volt_table2),
>>   };
>>
>> +static int pm800_regulator_init(struct platform_device *pdev)
>> +{
>> +       struct pm800_regulators *pm800_data = platform_get_drvdata(pdev);
>> +       struct pm80x_chip *chip = pm800_data->chip;
>> +       int ret;
>
> 'ret' is used only in if statement below. I don't have strong feelings
> but can you move it there to limit its scope or always return 'ret'
> (after initializing to '0'). To me this would be more readable.
>

OK, will fix in V3.

I will wait to close on constraint discussion above.

Thanks,
Vaibhav

> Best regards,
> Krzysztof
>
>> +
>> +       /* Currently only supported on 88pm860 device */
>> +       if (chip->type != CHIP_PM860)
>> +               return 0;
>> +
>> +       if (of_property_read_bool(pdev->dev.of_node,
>> +                               "marvell,88pm860-buck1-dualphase-en")) {
>> +               ret = regmap_update_bits(chip->subchip->regmap_power,
>> +                                       PM860_BUCK1_MISC,
>> +                                       BUCK1_DUAL_PHASE_SEL,
>> +                                       BUCK1_DUAL_PHASE_SEL);
>> +               if (ret) {
>> +                       dev_err(chip->dev, "failed to set dual-pase mode %d\n", ret);
>> +                       return ret;
>> +               }
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>>   static int pm800_regulator_probe(struct platform_device *pdev)
>>   {
>>          struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
>> @@ -336,6 +361,12 @@ static int pm800_regulator_probe(struct platform_device *pdev)
>>                  }
>>          }
>>
>> +       ret = pm800_regulator_init(pdev);
>> +       if (ret) {
>> +               dev_err(&pdev->dev, "failed to init 88pm800 regulator device\n");
>> +               return ret;
>> +       }
>> +
>>          return 0;
>>   }
>>
>> diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
>> index a92d173..05d9bad 100644
>> --- a/include/linux/mfd/88pm80x.h
>> +++ b/include/linux/mfd/88pm80x.h
>> @@ -295,6 +295,9 @@ enum {
>>   #define PM860_BUCK4_MISC2              (0x82)
>>   #define PM860_BUCK4_FULL_DRV           BIT(2)
>>
>> +#define PM860_BUCK1_MISC               (0x8E)
>> +#define BUCK1_DUAL_PHASE_SEL           BIT(2)
>> +
>>   struct pm80x_rtc_pdata {
>>          int             vrtc;
>>          int             rtc_wakeup;
>> --
>> 1.9.1
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH-v2 2/2] regulator: 88pm800: Add support for configuration of dual phase on BUCK1

[Krzysztof Kozlowski]

On 05.08.2015 17:45, Vaibhav Hiremath wrote:
> 
> 
> On Thursday 23 July 2015 10:21 AM, Krzysztof Kozlowski wrote:
>> 2015-07-22 1:23 GMT+09:00 Vaibhav Hiremath <vaibhav.hiremath@linaro.org>:
>>> 88PM860 device supports dual phase mode on BUCK1 output.
>>> In normal usecase, BUCK1A and BUCK1B operates independently with 3A
>>> capacity. And they both can work as a dual phase providing 6A capacity.
>>>
>>> This patch updates the regulator driver to read the respective
>>> DT property and enable dual-phase mode on BUCK1.
>>>
>>> Note that, this is init time (one time) initialization.
>>>
> 
> Sorry for delayed response, was on bed rest almost for week.
> 
>>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>>> ---
>>>   drivers/regulator/88pm800.c | 31 +++++++++++++++++++++++++++++++
>>>   include/linux/mfd/88pm80x.h |  3 +++
>>>   2 files changed, 34 insertions(+)
>>
>> Don't you need to update the constraints also? I think the BUCK1
>> regulator has fixed constraint of 3 A:
>>    PM800_BUCK(buck1, BUCK1, BUCK_ENA, 0, 3000000, buck1_volt_range,
>> 0x55),
>> and now it can handle 6 A.
>>
> 
> Actually, BUCK1A and BUCK1B both combined together provide 6A capacity.
> And as discussed earlier, we need board change for this.
> 
> I am quite not sure.

AFAIU the regulator driver creates one BUCK1 regulator with constraints
3 A. However after your change the regulator will handle up to 6 A.

This means that constraints set by driver are wrong.

Additionally I can't find BUCK1A and BUCK1B regulators. Driver provides
only BUCK1.

> 
> Should I read the property and update the constraint runtime during
> probe?

Driver should provide real constraints. Find the proper way to do this.

The pm800_regulator_info[] array is not const so you can change it in
whatever way you want (although it should be const for existing driver
because regulator core accepts const and passing it to driver_data is
not necessary).

Best regards,
Krzysztof

> 
> 
>>>
>>> diff --git a/drivers/regulator/88pm800.c b/drivers/regulator/88pm800.c
>>> index e846e4c..1bf2b35 100644
>>> --- a/drivers/regulator/88pm800.c
>>> +++ b/drivers/regulator/88pm800.c
>>> @@ -267,6 +267,31 @@ static struct pm800_regulator_info
>>> pm860_regulator_info[] = {
>>>          PM800_LDO(ldo20, LDO20, LDO_ENA1_3, 3, 10000, ldo_volt_table2),
>>>   };
>>>
>>> +static int pm800_regulator_init(struct platform_device *pdev)
>>> +{
>>> +       struct pm800_regulators *pm800_data =
>>> platform_get_drvdata(pdev);
>>> +       struct pm80x_chip *chip = pm800_data->chip;
>>> +       int ret;
>>
>> 'ret' is used only in if statement below. I don't have strong feelings
>> but can you move it there to limit its scope or always return 'ret'
>> (after initializing to '0'). To me this would be more readable.
>>
> 
> OK, will fix in V3.
> 
> I will wait to close on constraint discussion above.
> 
> Thanks,
> Vaibhav
> 
>> Best regards,
>> Krzysztof
>>
>>> +
>>> +       /* Currently only supported on 88pm860 device */
>>> +       if (chip->type != CHIP_PM860)
>>> +               return 0;
>>> +
>>> +       if (of_property_read_bool(pdev->dev.of_node,
>>> +                               "marvell,88pm860-buck1-dualphase-en")) {
>>> +               ret = regmap_update_bits(chip->subchip->regmap_power,
>>> +                                       PM860_BUCK1_MISC,
>>> +                                       BUCK1_DUAL_PHASE_SEL,
>>> +                                       BUCK1_DUAL_PHASE_SEL);
>>> +               if (ret) {
>>> +                       dev_err(chip->dev, "failed to set dual-pase
>>> mode %d\n", ret);
>>> +                       return ret;
>>> +               }
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>> +
>>>   static int pm800_regulator_probe(struct platform_device *pdev)
>>>   {
>>>          struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
>>> @@ -336,6 +361,12 @@ static int pm800_regulator_probe(struct
>>> platform_device *pdev)
>>>                  }
>>>          }
>>>
>>> +       ret = pm800_regulator_init(pdev);
>>> +       if (ret) {
>>> +               dev_err(&pdev->dev, "failed to init 88pm800 regulator
>>> device\n");
>>> +               return ret;
>>> +       }
>>> +
>>>          return 0;
>>>   }
>>>
>>> diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
>>> index a92d173..05d9bad 100644
>>> --- a/include/linux/mfd/88pm80x.h
>>> +++ b/include/linux/mfd/88pm80x.h
>>> @@ -295,6 +295,9 @@ enum {
>>>   #define PM860_BUCK4_MISC2              (0x82)
>>>   #define PM860_BUCK4_FULL_DRV           BIT(2)
>>>
>>> +#define PM860_BUCK1_MISC               (0x8E)
>>> +#define BUCK1_DUAL_PHASE_SEL           BIT(2)
>>> +
>>>   struct pm80x_rtc_pdata {
>>>          int             vrtc;
>>>          int             rtc_wakeup;
>>> -- 
>>> 1.9.1
>>>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH-v2 2/2] regulator: 88pm800: Add support for configuration of dual phase on BUCK1

[Vaibhav Hiremath]

On Thursday 06 August 2015 05:28 AM, Krzysztof Kozlowski wrote:
> On 05.08.2015 17:45, Vaibhav Hiremath wrote:
>>
>>
>> On Thursday 23 July 2015 10:21 AM, Krzysztof Kozlowski wrote:
>>> 2015-07-22 1:23 GMT+09:00 Vaibhav Hiremath <vaibhav.hiremath@linaro.org>:
>>>> 88PM860 device supports dual phase mode on BUCK1 output.
>>>> In normal usecase, BUCK1A and BUCK1B operates independently with 3A
>>>> capacity. And they both can work as a dual phase providing 6A capacity.
>>>>
>>>> This patch updates the regulator driver to read the respective
>>>> DT property and enable dual-phase mode on BUCK1.
>>>>
>>>> Note that, this is init time (one time) initialization.
>>>>
>>
>> Sorry for delayed response, was on bed rest almost for week.
>>
>>>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>>>> ---
>>>>    drivers/regulator/88pm800.c | 31 +++++++++++++++++++++++++++++++
>>>>    include/linux/mfd/88pm80x.h |  3 +++
>>>>    2 files changed, 34 insertions(+)
>>>
>>> Don't you need to update the constraints also? I think the BUCK1
>>> regulator has fixed constraint of 3 A:
>>>     PM800_BUCK(buck1, BUCK1, BUCK_ENA, 0, 3000000, buck1_volt_range,
>>> 0x55),
>>> and now it can handle 6 A.
>>>
>>
>> Actually, BUCK1A and BUCK1B both combined together provide 6A capacity.
>> And as discussed earlier, we need board change for this.
>>
>> I am quite not sure.
>
> AFAIU the regulator driver creates one BUCK1 regulator with constraints
> 3 A. However after your change the regulator will handle up to 6 A.
>
> This means that constraints set by driver are wrong.
>
> Additionally I can't find BUCK1A and BUCK1B regulators. Driver provides
> only BUCK1.
>

My patch does add BUCK1A and BUCK1B, please refer to the PATCH[4/5] of
earlier series, which is accepted.

https://patchwork.kernel.org/patch/6810461/


>>
>> Should I read the property and update the constraint runtime during
>> probe?
>
> Driver should provide real constraints. Find the proper way to do this.
>
> The pm800_regulator_info[] array is not const so you can change it in
> whatever way you want (although it should be const for existing driver
> because regulator core accepts const and passing it to driver_data is
> not necessary).
>


Probably that is the only way to handle this.

how about,

As you mentioned, pm800_regulator_info[] is not constant, so I can 
update the constraint before regulator_register() and also do not
register BUCK1B, if dual phase is enabled.

So in summary,

if (dual phase is enabled)
{
	Update constraint of BUCK1 to 6A
	and do not register BUCK1B
} else {
	register both BUCK1A and BUCK1B with default constraint of 3A.
}

Thanks,
Vaibhav

Re: [PATCH-v2 2/2] regulator: 88pm800: Add support for configuration of dual phase on BUCK1

[Krzysztof Kozlowski]

On 06.08.2015 15:03, Vaibhav Hiremath wrote:
> 
> 
> On Thursday 06 August 2015 05:28 AM, Krzysztof Kozlowski wrote:
>> On 05.08.2015 17:45, Vaibhav Hiremath wrote:
>>>
>>>
>>> On Thursday 23 July 2015 10:21 AM, Krzysztof Kozlowski wrote:
>>>> 2015-07-22 1:23 GMT+09:00 Vaibhav Hiremath
>>>> <vaibhav.hiremath@linaro.org>:
>>>>> 88PM860 device supports dual phase mode on BUCK1 output.
>>>>> In normal usecase, BUCK1A and BUCK1B operates independently with 3A
>>>>> capacity. And they both can work as a dual phase providing 6A
>>>>> capacity.
>>>>>
>>>>> This patch updates the regulator driver to read the respective
>>>>> DT property and enable dual-phase mode on BUCK1.
>>>>>
>>>>> Note that, this is init time (one time) initialization.
>>>>>
>>>
>>> Sorry for delayed response, was on bed rest almost for week.
>>>
>>>>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>>>>> ---
>>>>>    drivers/regulator/88pm800.c | 31 +++++++++++++++++++++++++++++++
>>>>>    include/linux/mfd/88pm80x.h |  3 +++
>>>>>    2 files changed, 34 insertions(+)
>>>>
>>>> Don't you need to update the constraints also? I think the BUCK1
>>>> regulator has fixed constraint of 3 A:
>>>>     PM800_BUCK(buck1, BUCK1, BUCK_ENA, 0, 3000000, buck1_volt_range,
>>>> 0x55),
>>>> and now it can handle 6 A.
>>>>
>>>
>>> Actually, BUCK1A and BUCK1B both combined together provide 6A capacity.
>>> And as discussed earlier, we need board change for this.
>>>
>>> I am quite not sure.
>>
>> AFAIU the regulator driver creates one BUCK1 regulator with constraints
>> 3 A. However after your change the regulator will handle up to 6 A.
>>
>> This means that constraints set by driver are wrong.
>>
>> Additionally I can't find BUCK1A and BUCK1B regulators. Driver provides
>> only BUCK1.
>>
> 
> My patch does add BUCK1A and BUCK1B, please refer to the PATCH[4/5] of
> earlier series, which is accepted.
> 
> https://patchwork.kernel.org/patch/6810461/

Indeed.

> 
> 
>>>
>>> Should I read the property and update the constraint runtime during
>>> probe?
>>
>> Driver should provide real constraints. Find the proper way to do this.
>>
>> The pm800_regulator_info[] array is not const so you can change it in
>> whatever way you want (although it should be const for existing driver
>> because regulator core accepts const and passing it to driver_data is
>> not necessary).
>>
> 
> 
> Probably that is the only way to handle this.
> 
> how about,
> 
> As you mentioned, pm800_regulator_info[] is not constant, so I can
> update the constraint before regulator_register() and also do not
> register BUCK1B, if dual phase is enabled.
> 
> So in summary,
> 
> if (dual phase is enabled)
> {
>     Update constraint of BUCK1 to 6A
>     and do not register BUCK1B
> } else {
>     register both BUCK1A and BUCK1B with default constraint of 3A.
> }

This idea looks good to me.

Best regards,
Krzysztof

Re: [PATCH-v2 2/2] regulator: 88pm800: Add support for configuration of dual phase on BUCK1

[Lee Jones]

On Tue, 21 Jul 2015, Vaibhav Hiremath wrote:

> 88PM860 device supports dual phase mode on BUCK1 output.
> In normal usecase, BUCK1A and BUCK1B operates independently with 3A
> capacity. And they both can work as a dual phase providing 6A capacity.
> 
> This patch updates the regulator driver to read the respective
> DT property and enable dual-phase mode on BUCK1.
> 
> Note that, this is init time (one time) initialization.
> 
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> ---
>  drivers/regulator/88pm800.c | 31 +++++++++++++++++++++++++++++++
>  include/linux/mfd/88pm80x.h |  3 +++
>  2 files changed, 34 insertions(+)

[...]

> diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
> index a92d173..05d9bad 100644
> --- a/include/linux/mfd/88pm80x.h
> +++ b/include/linux/mfd/88pm80x.h
> @@ -295,6 +295,9 @@ enum {
>  #define PM860_BUCK4_MISC2		(0x82)
>  #define PM860_BUCK4_FULL_DRV		BIT(2)
>  
> +#define PM860_BUCK1_MISC		(0x8E)

Why the over-bracketing?

> +#define BUCK1_DUAL_PHASE_SEL		BIT(2)
> +
>  struct pm80x_rtc_pdata {
>  	int		vrtc;
>  	int		rtc_wakeup;

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH-v2 2/2] regulator: 88pm800: Add support for configuration of dual phase on BUCK1

[Vaibhav Hiremath]

On Thursday 23 July 2015 09:24 PM, Lee Jones wrote:
> On Tue, 21 Jul 2015, Vaibhav Hiremath wrote:
>
>> 88PM860 device supports dual phase mode on BUCK1 output.
>> In normal usecase, BUCK1A and BUCK1B operates independently with 3A
>> capacity. And they both can work as a dual phase providing 6A capacity.
>>
>> This patch updates the regulator driver to read the respective
>> DT property and enable dual-phase mode on BUCK1.
>>
>> Note that, this is init time (one time) initialization.
>>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>> ---
>>   drivers/regulator/88pm800.c | 31 +++++++++++++++++++++++++++++++
>>   include/linux/mfd/88pm80x.h |  3 +++
>>   2 files changed, 34 insertions(+)
>
> [...]
>
>> diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
>> index a92d173..05d9bad 100644
>> --- a/include/linux/mfd/88pm80x.h
>> +++ b/include/linux/mfd/88pm80x.h
>> @@ -295,6 +295,9 @@ enum {
>>   #define PM860_BUCK4_MISC2		(0x82)
>>   #define PM860_BUCK4_FULL_DRV		BIT(2)
>>
>> +#define PM860_BUCK1_MISC		(0x8E)
>
> Why the over-bracketing?
>

No specific reason, just wanted to be consistent with other definitions
in the file.

Anyway, will remove it in V3.

Thanks,
Vaibhav