Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1320936 > unrolled thread

Re: [PATCH V4 2/2] regulator: mt6323: Add support for MT6323 regulator

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH V4 2/2] regulator: mt6323: Add support for MT6323  regulator John Crispin <blogic@openwrt.org> - 2016-01-28 19:20 +0100
    Re: [PATCH V4 2/2] regulator: mt6323: Add support for MT6323  regulator Mark Brown <broonie@kernel.org> - 2016-01-29 00:20 +0100
      Re: [PATCH V4 2/2] regulator: mt6323: Add support for MT6323  regulator menghui lin <menghui.lin@mediatek.com> - 2016-01-29 11:00 +0100
        Re: [PATCH V4 2/2] regulator: mt6323: Add support for MT6323  regulator Mark Brown <broonie@kernel.org> - 2016-01-29 12:30 +0100
          Re: [PATCH V4 2/2] regulator: mt6323: Add support for MT6323  regulator menghui lin <menghui.lin@mediatek.com> - 2016-01-29 13:20 +0100
            Re: [PATCH V4 2/2] regulator: mt6323: Add support for MT6323  regulator Mark Brown <broonie@kernel.org> - 2016-02-02 20:40 +0100
            Re: [PATCH V4 2/2] regulator: mt6323: Add support for MT6323  regulator Mark Brown <broonie@kernel.org> - 2016-02-02 20:40 +0100
              Re: [PATCH V4 2/2] regulator: mt6323: Add support for MT6323  regulator menghui lin <menghui.lin@mediatek.com> - 2016-02-03 06:40 +0100
                Re: [PATCH V4 2/2] regulator: mt6323: Add support for MT6323  regulator Mark Brown <broonie@kernel.org> - 2016-02-03 13:40 +0100
                  Re: [PATCH V4 2/2] regulator: mt6323: Add support for MT6323  regulator menghui lin <menghui.lin@mediatek.com> - 2016-02-04 03:50 +0100

#1320936 — Re: [PATCH V4 2/2] regulator: mt6323: Add support for MT6323 regulator

On 27/01/2016 15:41, Mark Brown wrote:
> On Wed, Jan 27, 2016 at 01:00:59PM +0100, John Crispin wrote:
> 
>> +		/* Constrain board-specific capabilities according to what
>> +		 * this driver and the chip itself can actually do.
>> +		 */
>> +		c = rdev->constraints;
>> +		c->valid_modes_mask |= REGULATOR_MODE_NORMAL |
>> +				       REGULATOR_MODE_STANDBY;
>> +		c->valid_ops_mask |= REGULATOR_CHANGE_MODE;
> 
> No, drivers should *never* enable things that weren't explictly enabled
> by the machine constraints.  This misses the whole point of having
> constraints.  They are there so that the system integrator can enable
> the functionality that is safe on a given board.  
> 
> The comment is also inaccurate, it claims it's imposing constraints but
> in fact it's adding additional permissions.
> 

Hi Mark

would the following two bindings be ok ? I would create patches to add them.

* regulator-allow-mode; or regulator-allow-change-mode;
* regulator-modes = <REGULATOR_MODE_NORMAL REGULATOR_MODE_STANDBY>;

	John

[toc] | [next] | [standalone]

#1321134

[Multipart message — attachments visible in raw view] — view raw

On Thu, Jan 28, 2016 at 07:13:48PM +0100, John Crispin wrote:

> would the following two bindings be ok ? I would create patches to add them.

> * regulator-allow-mode; or regulator-allow-change-mode;

This seems redundant, if we have a list of valid modes presumably they
can be used - same idea as with voltage setting.

> * regulator-modes = <REGULATOR_MODE_NORMAL REGULATOR_MODE_STANDBY>;

I'm not convinced this binding makes sense, how would a user of the API
(currently there are none in tree) know what the modes mean?  It's a bit
different when the user is supplying configuration for a specific
regulator but this needs to be something that can be used by consumers.

What are you actually trying to do with this?

[toc] | [prev] | [next] | [standalone]

#1321619

On Fri, 2016-01-29 at 00:13 +0100, Mark Brown wrote:
> On Thu, Jan 28, 2016 at 07:13:48PM +0100, John Crispin wrote:
> 
> > would the following two bindings be ok ? I would create patches to add them.
> 
> > * regulator-allow-mode; or regulator-allow-change-mode;
> 
> This seems redundant, if we have a list of valid modes presumably they
> can be used - same idea as with voltage setting.
> 
> > * regulator-modes = <REGULATOR_MODE_NORMAL REGULATOR_MODE_STANDBY>;
> 
> I'm not convinced this binding makes sense, how would a user of the API
> (currently there are none in tree) know what the modes mean?  It's a bit
> different when the user is supplying configuration for a specific
> regulator but this needs to be something that can be used by consumers.
> 
> What are you actually trying to do with this?

Hi Mark,

Per documentation in regulator/consumer.h, I think user should be able
to know the behavior of each mode(fast, normal, idle, standby).

In this patch, we want to support both normal/standby modes for mt6323
regulators due to mt6323 regulators support low power mode which
provides better power efficiency for standby case.

We expect user of mt6323 regulators could dynamically change power mode
by regulator_set_mode(). -EINVAL is returned if the given mode is not
supported.

The regulator_set_mode() API looks very straightforward and possible
modes are already defined in consumer.h. It looks like we don't have to
list possible modes for mt6323 additionally in binding document.

MengHui

[toc] | [prev] | [next] | [standalone]

#1321669

[Multipart message — attachments visible in raw view] — view raw

On Fri, Jan 29, 2016 at 05:52:14PM +0800, menghui lin wrote:
> On Fri, 2016-01-29 at 00:13 +0100, Mark Brown wrote:

> > I'm not convinced this binding makes sense, how would a user of the API
> > (currently there are none in tree) know what the modes mean?  It's a bit
> > different when the user is supplying configuration for a specific
> > regulator but this needs to be something that can be used by consumers.

> > What are you actually trying to do with this?

> In this patch, we want to support both normal/standby modes for mt6323
> regulators due to mt6323 regulators support low power mode which
> provides better power efficiency for standby case.

> We expect user of mt6323 regulators could dynamically change power mode
> by regulator_set_mode(). -EINVAL is returned if the given mode is not
> supported.

> The regulator_set_mode() API looks very straightforward and possible
> modes are already defined in consumer.h. It looks like we don't have to
> list possible modes for mt6323 additionally in binding document.

None of this is answering my question - I know what the current API is,
describing it doesn't tell me about actual users or how they are able to
sensibly use the interface.  Bear in mind that the definitions of the
various modes are all relative and what one device thinks is high usage
may be low usage for another device.

[toc] | [prev] | [next] | [standalone]

#1321719

On Fri, 2016-01-29 at 12:27 +0100, Mark Brown wrote:
> On Fri, Jan 29, 2016 at 05:52:14PM +0800, menghui lin wrote:
> > On Fri, 2016-01-29 at 00:13 +0100, Mark Brown wrote:
> 
> > > I'm not convinced this binding makes sense, how would a user of the API
> > > (currently there are none in tree) know what the modes mean?  It's a bit
> > > different when the user is supplying configuration for a specific
> > > regulator but this needs to be something that can be used by consumers.
> 
> > > What are you actually trying to do with this?
> 
> > In this patch, we want to support both normal/standby modes for mt6323
> > regulators due to mt6323 regulators support low power mode which
> > provides better power efficiency for standby case.
> 
> > We expect user of mt6323 regulators could dynamically change power mode
> > by regulator_set_mode(). -EINVAL is returned if the given mode is not
> > supported.
> 
> > The regulator_set_mode() API looks very straightforward and possible
> > modes are already defined in consumer.h. It looks like we don't have to
> > list possible modes for mt6323 additionally in binding document.
> 
> None of this is answering my question - I know what the current API is,
> describing it doesn't tell me about actual users or how they are able to
> sensibly use the interface.  Bear in mind that the definitions of the
> various modes are all relative and what one device thinks is high usage
> may be low usage for another device.

Assuming valid_modes_mask and initial_mode are specified, a possible
way to modify regulator_set_mode() is to allow mode change only if the
regulator is controlled exclusively by a certain consumer or the
requested mode provides stronger power capability than current mode.
Here I assume that power capability fast > normal > idle > standby.

[toc] | [prev] | [next] | [standalone]

#1324554

[Multipart message — attachments visible in raw view] — view raw

On Fri, Jan 29, 2016 at 08:11:19PM +0800, menghui lin wrote:
> On Fri, 2016-01-29 at 12:27 +0100, Mark Brown wrote:

> > None of this is answering my question - I know what the current API is,
> > describing it doesn't tell me about actual users or how they are able to
> > sensibly use the interface.  Bear in mind that the definitions of the
> > various modes are all relative and what one device thinks is high usage
> > may be low usage for another device.

> Assuming valid_modes_mask and initial_mode are specified, a possible
> way to modify regulator_set_mode() is to allow mode change only if the
> regulator is controlled exclusively by a certain consumer or the
> requested mode provides stronger power capability than current mode.
> Here I assume that power capability fast > normal > idle > standby.

...and I also note that the above *still* doesn't answer my questions
above.

[toc] | [prev] | [next] | [standalone]

#1324561

[Multipart message — attachments visible in raw view] — view raw

On Fri, Jan 29, 2016 at 08:11:19PM +0800, menghui lin wrote:
> On Fri, 2016-01-29 at 12:27 +0100, Mark Brown wrote:

> > None of this is answering my question - I know what the current API is,
> > describing it doesn't tell me about actual users or how they are able to
> > sensibly use the interface.  Bear in mind that the definitions of the
> > various modes are all relative and what one device thinks is high usage
> > may be low usage for another device.

> Assuming valid_modes_mask and initial_mode are specified, a possible
> way to modify regulator_set_mode() is to allow mode change only if the
> regulator is controlled exclusively by a certain consumer or the
> requested mode provides stronger power capability than current mode.
> Here I assume that power capability fast > normal > idle > standby.

How does the driver know if it needs to change the mode (ie, how can it
tell if the current mode is inadequate) and surely if we can only change
in one direction this isn't terribly useful?

[toc] | [prev] | [next] | [standalone]

#1324921

On Tue, 2016-02-02 at 19:38 +0000, Mark Brown wrote:
> On Fri, Jan 29, 2016 at 08:11:19PM +0800, menghui lin wrote:
> > On Fri, 2016-01-29 at 12:27 +0100, Mark Brown wrote:
> 
> > > None of this is answering my question - I know what the current API is,
> > > describing it doesn't tell me about actual users or how they are able to
> > > sensibly use the interface.  Bear in mind that the definitions of the
> > > various modes are all relative and what one device thinks is high usage
> > > may be low usage for another device.
> 
> > Assuming valid_modes_mask and initial_mode are specified, a possible
> > way to modify regulator_set_mode() is to allow mode change only if the
> > regulator is controlled exclusively by a certain consumer or the
> > requested mode provides stronger power capability than current mode.
> > Here I assume that power capability fast > normal > idle > standby.
> 
> How does the driver know if it needs to change the mode (ie, how can it
> tell if the current mode is inadequate) and surely if we can only change
> in one direction this isn't terribly useful?

Hi Mark,

I think the datasheet of buck/ldo could provide information about power
capability of each mode. The driver should adjust regulator mode per its
device's power requirement.

Below are some actual scenarios we have now. I provide them for your
reference. If unfortunately below cases are not the actual users you
ask, please kindly let me know more about your suggestion.
Thank you. :)

case 1:

We have a USB typeC micro-controller, which has two modes - standby and
normal. It requires 1.8V and 3.3V to operate (both powers are always
on). The device stays in standby mode when there is no cable in. When
cable in, we got an interrupt and change device into normal mode.

The standby mode power consumption is quite small, so we would like the
change mode of regulator into STANDBY to save more power. And we change
into NORMAL when we receive cable-in interrupt.

case 2:

About buck regulator for CPU, it usually provides PWM mode, PWM/PFM Auto
mode, PFM mode. I think it could map to FAST, NORMAL, IDLE mode
respectively. Most of time we would use just normal mode. However, we
would change regulator into PWM mode time to time to test buck output
performance on the tested board.

[toc] | [prev] | [next] | [standalone]

#1325336

[Multipart message — attachments visible in raw view] — view raw

On Wed, Feb 03, 2016 at 01:39:02PM +0800, menghui lin wrote:
> On Tue, 2016-02-02 at 19:38 +0000, Mark Brown wrote:

> > How does the driver know if it needs to change the mode (ie, how can it
> > tell if the current mode is inadequate) and surely if we can only change
> > in one direction this isn't terribly useful?

> I think the datasheet of buck/ldo could provide information about power
> capability of each mode. The driver should adjust regulator mode per its
> device's power requirement.

That's of no help for a consumer driver which doesn't know what
regulator is supplying it.

> case 1:

> We have a USB typeC micro-controller, which has two modes - standby and
> normal. It requires 1.8V and 3.3V to operate (both powers are always
> on). The device stays in standby mode when there is no cable in. When
> cable in, we got an interrupt and change device into normal mode.

> The standby mode power consumption is quite small, so we would like the
> change mode of regulator into STANDBY to save more power. And we change
> into NORMAL when we receive cable-in interrupt.

This seems like something that we ought to be doing via runtime PM
anyway which should be going through the suspend mode bindings, though
that would need some plumbing in.

> case 2:

> About buck regulator for CPU, it usually provides PWM mode, PWM/PFM Auto
> mode, PFM mode. I think it could map to FAST, NORMAL, IDLE mode
> respectively. Most of time we would use just normal mode. However, we
> would change regulator into PWM mode time to time to test buck output
> performance on the tested board.

That's a test use that doesn't seem a good fit for upstream at all.

[toc] | [prev] | [next] | [standalone]

#1326352

On Wed, 2016-02-03 at 12:29 +0000, Mark Brown wrote:
> On Wed, Feb 03, 2016 at 01:39:02PM +0800, menghui lin wrote:
> > On Tue, 2016-02-02 at 19:38 +0000, Mark Brown wrote:
> 
> > > How does the driver know if it needs to change the mode (ie, how can it
> > > tell if the current mode is inadequate) and surely if we can only change
> > > in one direction this isn't terribly useful?
> 
> > I think the datasheet of buck/ldo could provide information about power
> > capability of each mode. The driver should adjust regulator mode per its
> > device's power requirement.
> 
> That's of no help for a consumer driver which doesn't know what
> regulator is supplying it.
> 
> > case 1:
> 
> > We have a USB typeC micro-controller, which has two modes - standby and
> > normal. It requires 1.8V and 3.3V to operate (both powers are always
> > on). The device stays in standby mode when there is no cable in. When
> > cable in, we got an interrupt and change device into normal mode.
> 
> > The standby mode power consumption is quite small, so we would like the
> > change mode of regulator into STANDBY to save more power. And we change
> > into NORMAL when we receive cable-in interrupt.
> 
> This seems like something that we ought to be doing via runtime PM
> anyway which should be going through the suspend mode bindings, though
> that would need some plumbing in.
> 
> > case 2:
> 
> > About buck regulator for CPU, it usually provides PWM mode, PWM/PFM Auto
> > mode, PFM mode. I think it could map to FAST, NORMAL, IDLE mode
> > respectively. Most of time we would use just normal mode. However, we
> > would change regulator into PWM mode time to time to test buck output
> > performance on the tested board.
> 
> That's a test use that doesn't seem a good fit for upstream at all.

Hi Mark,

Thanks for your response.
It seems more work is required for dynamic mode switch.
For MT6323 regulator patch, we would remove mode-switch related code for
now to provide basic regulator support first.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web