Re: [RFC PATCH] regulator: introduce boot protection flag

[Pingbo Wen]

Hi, Mark

On Thursday, June 09, 2016 01:16 AM, Mark Brown wrote:
> On Mon, May 09, 2016 at 03:05:08PM +0800, WEN Pingbo wrote:
> 
>> And regulator core will postpone all operations until all consumers
>> have taked their place.
> 
> It doesn't, it postpones them until late_initacall().  This is both
> after the consumers have loaded if they are built in and before any
> consumers built as modules come up.

Yes, this patch only protects a regulator from regulator
registration(built in) to late_initcall(). But, IMO, if a regulator is
critical, it's weird to build as a module. Maybe I was thoughtless here.

If we take modules under consideration, and to make this patch more
universal, I think what we really need is adding a flag to protect a
regulator from registration to a specific consumer(not the first
consumer). The regulator driver gives the initial state, and the
specific consumer need to clear this flag while finishing regulator
setting(by calling a function like regulator_clear_protect()). And what
the regulator core need to do is staging all operations during
protection. And that will cover all consumers probing order, whenever
the regulator is registered.

Any idea?

> 
>> The boot_protection flag only work before late_initicall. And as other
>> constraints liked, you can specify this flag in a board file, or in
>> dts file.
> 
> Anything added to the DT ABI needs a binding.
> 

I will add bindings in next version.

>> +	/* constraints check has already done */
>> +	if (rdev->boot_mode)
>> +		rdev->desc->ops->set_mode(rdev, rdev->boot_mode);
> 
> This whole sequence of code ignores errors - that's not great.  We
> should at least log them.
> 

OK.

>> +	mutex_unlock(&rdev->mutex);
>> +
>> +	if (regulator)
>> +		regulator_set_voltage(regulator, regulator->min_uV,
>> +				regulator->max_uV);
> 
> That's...  exciting.  There's a couple of issues here.  One is that
> this is not operating on the rdev but rather on a consumer regulator
> device, the other is that we drop out of the lock before doing the
> update which tends to be a warning sign that something fun is going on
> and at least an internal function should be used.  These two most likely
> come down to the same issue.
> 

OK, some bugs here. I will use a unlock version.

Pingbo