diff mbox

[v9,1/2] regulator: Add driver for max77802 PMIC PMIC regulators

Message ID 53FAFCBC.2050407@collabora.co.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Javier Martinez Canillas Aug. 25, 2014, 9:07 a.m. UTC
Hello Yuvaraj,

On 08/25/2014 10:22 AM, Yuvaraj Cd wrote:
>>> Good question. I'm not that familiar with the dw_mmc host controller nor
>>> its driver implementation so I'll let Yuvaraj or Doug to answer that.
> Well,here it goes!
> 1. Power ON the board LDO4CTRL1[7:6] 11b
> 2. dw_mmc driver enable the vqmmc.
> 3. checks for UHS support, complete the voltage switching t0 1.8V
> 4. Does warm reset by reboot command.
> 5. mmc core calls mmc_set_ios() with MMC_POWER_OFF.
> 6. dw_mmc driver cut-off the regulator with LDO4CTRL1[7:6] is 00b
> 7.dw_mmc driver enable the vqmmc.
>  But after step 7 also, LD4CTRL[7:6] is 00b.

Ok, so the dw_mmc driver is enabling vqmmc, that's good.

>>
>> I haven't seen the issue that Yuvaraj is reporting (but I also haven't
>> picked up all of the relevant patches and tried to reproduce), so I'm
>> going to have to leave it to Yuvaraj to answer.
>     static int max77802_enable(struct regulator_dev *rdev)
>     {
>       struct max77802_regulator_prv *max77802 = rdev_get_drvdata(rdev);
>      int id = rdev_get_id(rdev);
>      int shift = max77802_get_opmode_shift(id);
>      return regmap_update_bits(rdev->regmap,
> rdev->desc->enable_reg,rdev->desc->enable_mask,max77802->opmode[id] <<
> shift);
>   }
> I think in the above code snippet, the "val" is what we got it during
> the probe.We always write that value for enabling this regulator(which
> is LDO4CTRL1[7:6] 00b after warm reset) which is not correct according
> the MAX77802 manual.
>>

I see, so probably until we have a way to define the operating mode for
each regulator using DT we should set the opmode to normal when enabling a
regulator independently of the value the hardware register reported on probe.

Can you please test the following change [0] so I can post as a proper
patch? Doug, Mark do you think that forcing the regulator to opmode normal
when enabling is the right solution here?

Best regards,
Javier

[0]
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Comments

Yuvaraj CD Aug. 25, 2014, 10:46 a.m. UTC | #1
On Mon, Aug 25, 2014 at 2:37 PM, Javier Martinez Canillas
<javier.martinez@collabora.co.uk> wrote:
> Hello Yuvaraj,
>
> On 08/25/2014 10:22 AM, Yuvaraj Cd wrote:
>>>> Good question. I'm not that familiar with the dw_mmc host controller nor
>>>> its driver implementation so I'll let Yuvaraj or Doug to answer that.
>> Well,here it goes!
>> 1. Power ON the board LDO4CTRL1[7:6] 11b
>> 2. dw_mmc driver enable the vqmmc.
>> 3. checks for UHS support, complete the voltage switching t0 1.8V
>> 4. Does warm reset by reboot command.
>> 5. mmc core calls mmc_set_ios() with MMC_POWER_OFF.
>> 6. dw_mmc driver cut-off the regulator with LDO4CTRL1[7:6] is 00b
>> 7.dw_mmc driver enable the vqmmc.
>>  But after step 7 also, LD4CTRL[7:6] is 00b.
>
> Ok, so the dw_mmc driver is enabling vqmmc, that's good.
>
>>>
>>> I haven't seen the issue that Yuvaraj is reporting (but I also haven't
>>> picked up all of the relevant patches and tried to reproduce), so I'm
>>> going to have to leave it to Yuvaraj to answer.
>>     static int max77802_enable(struct regulator_dev *rdev)
>>     {
>>       struct max77802_regulator_prv *max77802 = rdev_get_drvdata(rdev);
>>      int id = rdev_get_id(rdev);
>>      int shift = max77802_get_opmode_shift(id);
>>      return regmap_update_bits(rdev->regmap,
>> rdev->desc->enable_reg,rdev->desc->enable_mask,max77802->opmode[id] <<
>> shift);
>>   }
>> I think in the above code snippet, the "val" is what we got it during
>> the probe.We always write that value for enabling this regulator(which
>> is LDO4CTRL1[7:6] 00b after warm reset) which is not correct according
>> the MAX77802 manual.
>>>
>
> I see, so probably until we have a way to define the operating mode for
> each regulator using DT we should set the opmode to normal when enabling a
> regulator independently of the value the hardware register reported on probe.
>
> Can you please test the following change [0] so I can post as a proper
> patch? Doug, Mark do you think that forcing the regulator to opmode normal
> when enabling is the right solution here?
>
> Best regards,
> Javier
>
> [0]
> diff --git a/drivers/regulator/max77802.c b/drivers/regulator/max77802.c
> index ad1caa9..917b5ab 100644
> --- a/drivers/regulator/max77802.c
> +++ b/drivers/regulator/max77802.c
> @@ -180,7 +180,7 @@ static int max77802_enable(struct regulator_dev *rdev)
>
>         return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
>                                   rdev->desc->enable_mask,
> -                                 max77802->opmode[id] << shift);
> +                                 MAX77802_OPMODE_NORMAL << shift);
>  }
It works.
Doug Anderson Aug. 25, 2014, 3:40 p.m. UTC | #2
Javier,

On Mon, Aug 25, 2014 at 2:07 AM, Javier Martinez Canillas
<javier.martinez@collabora.co.uk> wrote:
> Hello Yuvaraj,
>
> On 08/25/2014 10:22 AM, Yuvaraj Cd wrote:
>>>> Good question. I'm not that familiar with the dw_mmc host controller nor
>>>> its driver implementation so I'll let Yuvaraj or Doug to answer that.
>> Well,here it goes!
>> 1. Power ON the board LDO4CTRL1[7:6] 11b
>> 2. dw_mmc driver enable the vqmmc.
>> 3. checks for UHS support, complete the voltage switching t0 1.8V
>> 4. Does warm reset by reboot command.
>> 5. mmc core calls mmc_set_ios() with MMC_POWER_OFF.
>> 6. dw_mmc driver cut-off the regulator with LDO4CTRL1[7:6] is 00b
>> 7.dw_mmc driver enable the vqmmc.
>>  But after step 7 also, LD4CTRL[7:6] is 00b.
>
> Ok, so the dw_mmc driver is enabling vqmmc, that's good.
>
>>>
>>> I haven't seen the issue that Yuvaraj is reporting (but I also haven't
>>> picked up all of the relevant patches and tried to reproduce), so I'm
>>> going to have to leave it to Yuvaraj to answer.
>>     static int max77802_enable(struct regulator_dev *rdev)
>>     {
>>       struct max77802_regulator_prv *max77802 = rdev_get_drvdata(rdev);
>>      int id = rdev_get_id(rdev);
>>      int shift = max77802_get_opmode_shift(id);
>>      return regmap_update_bits(rdev->regmap,
>> rdev->desc->enable_reg,rdev->desc->enable_mask,max77802->opmode[id] <<
>> shift);
>>   }
>> I think in the above code snippet, the "val" is what we got it during
>> the probe.We always write that value for enabling this regulator(which
>> is LDO4CTRL1[7:6] 00b after warm reset) which is not correct according
>> the MAX77802 manual.
>>>
>
> I see, so probably until we have a way to define the operating mode for
> each regulator using DT we should set the opmode to normal when enabling a
> regulator independently of the value the hardware register reported on probe.
>
> Can you please test the following change [0] so I can post as a proper
> patch? Doug, Mark do you think that forcing the regulator to opmode normal
> when enabling is the right solution here?

IMHO that makes sense.
Javier Martinez Canillas Aug. 25, 2014, 5:20 p.m. UTC | #3
Hello Doug,

On 08/25/2014 05:40 PM, Doug Anderson wrote:
>>
>> I see, so probably until we have a way to define the operating mode for
>> each regulator using DT we should set the opmode to normal when enabling a
>> regulator independently of the value the hardware register reported on probe.
>>
>> Can you please test the following change [0] so I can post as a proper
>> patch? Doug, Mark do you think that forcing the regulator to opmode normal
>> when enabling is the right solution here?
> 
> IMHO that makes sense.
> 

Thanks, I'll post it as a proper patch tomorrow then.

Best regards,
Javier
Mark Brown Aug. 26, 2014, 7:17 a.m. UTC | #4
On Mon, Aug 25, 2014 at 08:40:40AM -0700, Doug Anderson wrote:
> On Mon, Aug 25, 2014 at 2:07 AM, Javier Martinez Canillas

> > I see, so probably until we have a way to define the operating mode for
> > each regulator using DT we should set the opmode to normal when enabling a
> > regulator independently of the value the hardware register reported on probe.

> > Can you please test the following change [0] so I can post as a proper
> > patch? Doug, Mark do you think that forcing the regulator to opmode normal
> > when enabling is the right solution here?

> IMHO that makes sense.

No, this doesn't make any obvious sense to me at all.  Picking normal as
a default if the hardware reads back off due to overlapping
impelementation or something *might* make sense but not overwriting the
hardware state without explicit permission from the machine integration
is a key goal for the regulator API.
Javier Martinez Canillas Aug. 26, 2014, 9:08 a.m. UTC | #5
Hello Mark,

On 08/26/2014 09:17 AM, Mark Brown wrote:
> On Mon, Aug 25, 2014 at 08:40:40AM -0700, Doug Anderson wrote:
>> > Can you please test the following change [0] so I can post as a proper
>> > patch? Doug, Mark do you think that forcing the regulator to opmode normal
>> > when enabling is the right solution here?
> 
>> IMHO that makes sense.
> 
> No, this doesn't make any obvious sense to me at all.  Picking normal as
> a default if the hardware reads back off due to overlapping
> impelementation or something *might* make sense but not overwriting the
> hardware state without explicit permission from the machine integration
> is a key goal for the regulator API.
> 

Just to be sure I understood you correctly, what might makes sense to you
then is to set the opmode to normal as default on probe only if off is
read back from the hardware register but leaving the enable function as it
is now using the opmode set on probe?

That seems like a safer solution indeed since enable won't overwrite other
values different from off read from the hardware register, I'll prepare a
patch.

Thanks a lot for your help and best regards,
Javier
Mark Brown Aug. 26, 2014, 9:12 a.m. UTC | #6
On Tue, Aug 26, 2014 at 11:08:07AM +0200, Javier Martinez Canillas wrote:
> On 08/26/2014 09:17 AM, Mark Brown wrote:

> > No, this doesn't make any obvious sense to me at all.  Picking normal as
> > a default if the hardware reads back off due to overlapping
> > impelementation or something *might* make sense but not overwriting the
> > hardware state without explicit permission from the machine integration
> > is a key goal for the regulator API.

> Just to be sure I understood you correctly, what might makes sense to you
> then is to set the opmode to normal as default on probe only if off is
> read back from the hardware register but leaving the enable function as it
> is now using the opmode set on probe?

Yes.
diff mbox

Patch

diff --git a/drivers/regulator/max77802.c b/drivers/regulator/max77802.c
index ad1caa9..917b5ab 100644
--- a/drivers/regulator/max77802.c
+++ b/drivers/regulator/max77802.c
@@ -180,7 +180,7 @@  static int max77802_enable(struct regulator_dev *rdev)

        return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
                                  rdev->desc->enable_mask,
-                                 max77802->opmode[id] << shift);
+                                 MAX77802_OPMODE_NORMAL << shift);
 }

_______________________________________________