From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751861AbdC0RjW (ORCPT ); Mon, 27 Mar 2017 13:39:22 -0400 Received: from mail-pg0-f54.google.com ([74.125.83.54]:34321 "EHLO mail-pg0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751522AbdC0RjM (ORCPT ); Mon, 27 Mar 2017 13:39:12 -0400 Date: Mon, 27 Mar 2017 10:39:02 -0700 From: Matthias Kaehlcke To: Javier Martinez Canillas Cc: Brian Norris , Liam Girdwood , Mark Brown , linux-kernel@vger.kernel.org, Douglas Anderson Subject: Re: [PATCH] regulator: core: Limit propagation of parent voltage count and list Message-ID: <20170327173902.GB84219@google.com> References: <20170324200952.103303-1-mka@chromium.org> <20170324203818.GA33073@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Thanks for the reviews and testing! El Sat, Mar 25, 2017 at 02:05:47AM -0300 Javier Martinez Canillas ha dit: On 03/24/2017 05:38 PM, Brian Norris wrote: > > On Fri, Mar 24, 2017 at 01:09:52PM -0700, Matthias Kaehlcke wrote: > >> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c > >> index 53d4fc70dbd0..121838e0125b 100644 > >> --- a/drivers/regulator/core.c > >> +++ b/drivers/regulator/core.c > >> @@ -2487,6 +2487,10 @@ static int _regulator_list_voltage(struct regulator *regulator, > >> if (lock) > >> mutex_unlock(&rdev->mutex); > >> } else if (rdev->supply) { > >> + // Limit propagation of parent values to switch regulators > > > > The kernel doesn't use C99 comments. Oddly enough, this isn't actually > > +1 Will fix > > in the coding style doc (Documentation/process/coding-style.rst), nor is > > it caught by scripts/checkpatch.pl (even though it clearly has a 'C99 > > comment' rule). > > > >> + if (ops->get_voltage || ops->get_voltage_sel) > > It's valid to have a .get_voltage_sel callback without a .list_voltage? > > At least it seems that _regulator_get_voltage() assumes that having a > .get_voltage_sel implies that a .list_voltage will also be available. > > static int _regulator_get_voltage(struct regulator_dev *rdev) > { > ... > if (rdev->desc->ops->get_voltage_sel) { > sel = rdev->desc->ops->get_voltage_sel(rdev); > if (sel < 0) > return sel; > ret = rdev->desc->ops->list_voltage(rdev, sel); > } else if (rdev->desc->ops->get_voltage) { > ... > } The same function (from which I derived the conditions) suggests that a regulator could have a .list_voltage op even if it doesn't have .get_voltage_sel: > ... > if (rdev->desc->ops->get_voltage_sel) { > ... > } else if (rdev->desc->ops->get_voltage) { > ... > } else if (rdev->desc->ops->list_voltage) { I don't know for sure if this condition is superfluous or if there are cases where it makes sense to have a .list_voltage but not .get_voltage_sel. > I wonder if instead of always checking if the regulator lacks operations, > it wouldn't be better to do it just once and store that the regulator is > a switch so that state can be used as explicit check for switch instead. > > Something like if (!rdev->supply || !rdev->switch) looks more clear > to me. I agree and we can even reduce it to if (!rdev_switch) since a switch implicitly has a supply. I'll send out a new version soon. Matthias