From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S967584AbdCXUib (ORCPT ); Fri, 24 Mar 2017 16:38:31 -0400 Received: from mail-pf0-f171.google.com ([209.85.192.171]:33018 "EHLO mail-pf0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756207AbdCXUiY (ORCPT ); Fri, 24 Mar 2017 16:38:24 -0400 Date: Fri, 24 Mar 2017 13:38:19 -0700 From: Brian Norris To: Matthias Kaehlcke Cc: Liam Girdwood , Mark Brown , Javier Martinez Canillas , linux-kernel@vger.kernel.org, Douglas Anderson Subject: Re: [PATCH] regulator: core: Limit propagation of parent voltage count and list Message-ID: <20170324203818.GA33073@google.com> References: <20170324200952.103303-1-mka@chromium.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170324200952.103303-1-mka@chromium.org> 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 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 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) > + return -EINVAL; > + > ret = _regulator_list_voltage(rdev->supply, selector, lock); > } else { > return -EINVAL; > @@ -2540,6 +2544,7 @@ EXPORT_SYMBOL_GPL(regulator_is_enabled); > int regulator_count_voltages(struct regulator *regulator) > { > struct regulator_dev *rdev = regulator->rdev; > + const struct regulator_ops *ops = rdev->desc->ops; > > if (rdev->desc->n_voltages) > return rdev->desc->n_voltages; > @@ -2547,6 +2552,10 @@ int regulator_count_voltages(struct regulator *regulator) > if (!rdev->supply) > return -EINVAL; > > + // Limit propagation of parent value to switch regulators Same here. > + if (ops->get_voltage || ops->get_voltage_sel || ops->list_voltage) > + return -EINVAL; > + > return regulator_count_voltages(rdev->supply); > } > EXPORT_SYMBOL_GPL(regulator_count_voltages); I'm not very familiar with this code, but judging by your problem description in previous threads and by comparing with the logic in _regulator_get_voltage() (for when to reference the ->supply), this seems resonable. So: Reviewed-by: Brian Norris It's probably worth verifying that this doesn't break whatever Javier was supporting in the first place, as a sanity check. Brian