From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751978AbdC0SVA (ORCPT ); Mon, 27 Mar 2017 14:21:00 -0400 Received: from mail-pg0-f53.google.com ([74.125.83.53]:33938 "EHLO mail-pg0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751533AbdC0SUn (ORCPT ); Mon, 27 Mar 2017 14:20:43 -0400 Date: Mon, 27 Mar 2017 11:20:40 -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: <20170327182040.GC84219@google.com> References: <20170324200952.103303-1-mka@chromium.org> <20170324203818.GA33073@google.com> <20170327173902.GB84219@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 El Mon, Mar 27, 2017 at 01:54:50PM -0400 Javier Martinez Canillas ha dit: > On 03/27/2017 01:39 PM, Matthias Kaehlcke wrote: > > >>>> + 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 don't think is the same condition. Unless I'm misreading the code > what it's checking is if there's a .list_voltage even when there is > no .get_voltage_sel. > > IOW, it's valid to have a .list_voltage even when there's no callback > for .get_voltage_sel, but the opposite isn't true. I see, thanks for the clarification. > >> 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 wonder if that's always true. What happens if you have a switch but > its -supply parent isn't defined in the Device Tree? My idea was to only set rdev->switch after having resolved the parent supply, though I concede this is not semantically. Maybe we still want this logic but give the flag a different name? Matthias