From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753566AbcFOMFV (ORCPT ); Wed, 15 Jun 2016 08:05:21 -0400 Received: from mail-pf0-f180.google.com ([209.85.192.180]:33871 "EHLO mail-pf0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750976AbcFOMFT (ORCPT ); Wed, 15 Jun 2016 08:05:19 -0400 Subject: Re: [RFC PATCH] regulator: introduce boot protection flag To: Mark Brown References: <1462777508-24934-1-git-send-email-pingbo.wen@linaro.org> <20160608171608.GM7510@sirena.org.uk> Cc: pingbo.wen@linaro.org, linux-kernel@vger.kernel.org, lgirdwood@gmail.com, vincent.guittot@linaro.org, stephen.boyd@linaro.org From: Pingbo Wen Message-ID: <57614479.1060708@linaro.org> Date: Wed, 15 Jun 2016 20:05:13 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 MIME-Version: 1.0 In-Reply-To: <20160608171608.GM7510@sirena.org.uk> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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