Re: [PATCH] regulator: anatop: add is_enabled interface

[Mark Brown]

On Tue, Dec 17, 2013 at 12:38:33PM +0000, Anson.Huang@xxxxxxxxxxxxx wrote:

> > better to add the error checking there wouldn't it?

> Okay, than what about other functions? there is such condition check
> there is other functions too, that is why I add it here. if you think
> it is no necessary, I will remove this check in my patch?

It shouldn't be in the other functions either.

> > This sounds like you need to have some higher level synchronisation
> > between whatever is managing this supply and your cpufreq driver - if
> > you rely on reading back the current status from the hardware there will
> > always be races between reading the state and the other thing doing the
> > enable or disable.

> yes, you are right. but I think we have handled that, all the
> operations of this LDO will via regulator interface, and regulator
> framework already has mutex lock. so there should be no such race in
> kernel as long as we all use regulator interface to access anatop LDO.

No, that's not going to work.  Consider this sequence:

	cpufreq		other driver
			disable()
	is_enabled()
			enable()

The locking the regulator core does won't help you here, nothing stops
the state of the regulator changing after it's read.  Your cpufreq
driver should just change the voltage and not worry if the regulator is
enabled, the voltage change won't have any effect while the regulator is
off and presumably if it does get enabled then it needs to be enabled at
whatever voltage cpufreq set anyway.

In any case I'd be much happier with this patch if it implemented the
enable and disable operations as well.

Attachment: signature.asc
Description: Digital signature