From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id CD3F9C48BDF for ; Tue, 22 Jun 2021 16:46:24 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id A368D6128C for ; Tue, 22 Jun 2021 16:46:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231272AbhFVQsj (ORCPT ); Tue, 22 Jun 2021 12:48:39 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49410 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229501AbhFVQsg (ORCPT ); Tue, 22 Jun 2021 12:48:36 -0400 Received: from mail-qv1-xf2d.google.com (mail-qv1-xf2d.google.com [IPv6:2607:f8b0:4864:20::f2d]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A2825C06175F for ; Tue, 22 Jun 2021 09:46:20 -0700 (PDT) Received: by mail-qv1-xf2d.google.com with SMTP id h10so1498819qvz.2 for ; Tue, 22 Jun 2021 09:46:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=cX1+N3znKheAWMZ/uIaQ9Wu1I4c8xfSCdEid5XXgYUU=; b=YOPMkgmgYEaJx78lKXGLKjEtNH7hC+lgi844pNliJkG4ti5vrqjMc+oYAqOQD7H2jS 2qqrRzjl9eiZPl3/CUfCvP3bVN4outKo6a1WsV8JSRjLD6PAeSIZDztM7Khj3cxdCfUR p4PY5N5SmT98GRvCrjoWP2IhcR9X54oRBzFtZo7yU7KpcGXTDGBCgT8U5Q8nDCvkcL4f 6GbkWOcfKD4uaLMwk1sYQhUHbquWuh76GcYII+D/ew4UZBKFK7T3YXQcj7bP5hirphRr woN0x87w5h9RppF3feOaRx2qlmXznUkTQwOi6hGFBXBK1pENgf1bTy7VU2Y8rcRZSYMs TeVg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=cX1+N3znKheAWMZ/uIaQ9Wu1I4c8xfSCdEid5XXgYUU=; b=C8hQnF6KVJ5YqyKcK43maurtE28mKWwaoimwM9Jxfo3BcnUz1N3WUHwFh+LTY5gMPo 7OeuvjiaW7Y29SYnBOM8w2fOp1cpofaqrfvNrt/YKx8de4euwMy9JM0B18ey72OksKoy 9oU+tsEP84heb/C6DKA2KCeFEWLuvcaFb4hrjUbGfEPnT8seOXgBUcimzsrXU8l1RQ7h 1a77qTbYyd/6DzLmr/gUuZqJV4CEFIXzWzkf1TegaiiAFpf9xUITNRI8ZXc2QEnkz5l0 QyWosYj0Pb4Fx4LTjrBITr/y2jwU9wOVZYSNTgzUnqX6khOGvMvalGBS1KeCfkj9BgGN BPNQ== X-Gm-Message-State: AOAM530/J6WEg34+f6DF/WWl1CeOKfbkkeOH8uGMUsBTVt0G0dToLx3U 1kg+iSX6YaNrqwMdy7Xf3HsmX8U0vk6Nti4apiKbWg== X-Google-Smtp-Source: ABdhPJzISR/1moqz9T2VFK0No/teqAwV9c5ubfc0VsheSSZ5WjW1a7WkGDxsHNNpv4HDu3/B1wo2UFlinPyOenYwBYo= X-Received: by 2002:ad4:55eb:: with SMTP id bu11mr26378298qvb.2.1624380379836; Tue, 22 Jun 2021 09:46:19 -0700 (PDT) MIME-Version: 1.0 References: <20210621223141.1638189-1-dmitry.baryshkov@linaro.org> <20210621223141.1638189-3-dmitry.baryshkov@linaro.org> <20210622112843.GB4574@sirena.org.uk> <20210622143812.GE4574@sirena.org.uk> In-Reply-To: <20210622143812.GE4574@sirena.org.uk> From: Dmitry Baryshkov Date: Tue, 22 Jun 2021 19:46:08 +0300 Message-ID: Subject: Re: [PATCH v3 2/7] regulator: qca6390: add support for QCA639x powerup sequence To: Mark Brown Cc: Andy Gross , Bjorn Andersson , Rob Herring , Liam Girdwood , Marcel Holtmann , Johan Hedberg , Luiz Augusto von Dentz , "open list:DRM DRIVER FOR MSM ADRENO GPU" , Manivannan Sadhasivam , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , open list , linux-bluetooth@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 22 Jun 2021 at 17:38, Mark Brown wrote: > > On Tue, Jun 22, 2021 at 05:17:28PM +0300, Dmitry Baryshkov wrote: > > On Tue, 22 Jun 2021 at 14:29, Mark Brown wrote: > > > On Tue, Jun 22, 2021 at 01:31:36AM +0300, Dmitry Baryshkov wrote: > > > > > Qualcomm QCA6390/1 is a family of WiFi + Bluetooth SoCs, with BT part > > > > being controlled through the UART and WiFi being present on PCIe > > > > bus. Both blocks share common power sources. Add device driver handling > > > > power sequencing of QCA6390/1. > > > > Are you sure this is a regulator and not a MFD? It appears to be a > > > consumer driver that turns on and off a bunch of regulators en masse > > > which for some reason exposes that on/off control as a single supply. > > > This looks like it'd be much more appropriate to implement as a MFD or > > > possibly power domain with the subdevices using runtime PM, it's clearly > > > not a regulator. > > > First attempt was designed to be an MFD. And Lee clearly stated that > > this is wrong: > > "This is not an MFD, since it utilised neither the MFD API nor > > of_platform_populate() to register child devices." [1] > > Well, perhaps it should do one of those things then? I don't think so. BT part is just a serdev sitting on top of UART, WiFi is PCIe device (for qca6390). So using MFD API (which primarily targets platform devices) does not seem logical and feasible. > Like I say this is > very clearly not a regulator, it looks like a consumer of some kind. > The regulator API isn't there just to absorb things that need reference > counting, it's there to represent things providing supplies. This seems > to be very clearly not a supply given that it's grouping together a > bunch of other supplies and switching them on and off together without > providing a clear output supply. Ack. > > I've tried following Rob's suggestions on implementing things clearly, > > but doing so results in too big restructure just for a single device. > > I don't know what that suggestion was? If there's only one device that > uses this why is it not implemented as part of that device? One device = qca6390 (or 91). Because it is still a device sitting on a PCI bus which is typically discoverable, while adding power sequencer to the device driver would demand making the bus fully descibiable (like PCI bus is on Spark). > > > > +static int qca6390_enable(struct regulator_dev *rdev) > > > > +{ > > > > + struct qca6390_data *data = rdev_get_drvdata(rdev); > > > > + int ret; > > > > > + ret = regulator_bulk_enable(data->num_vregs, data->regulators); > > > > + if (ret) { > > > > + dev_err(data->dev, "Failed to enable regulators"); > > > > + return ret; > > > > + } > > > > The regulator API is *not* recursive, I am astonished this works. > > > It does, even with lockdep enabled. Moreover BT regularly does disable > > and enable this regulator, so both enable and disable paths were well > > tested. > > Should I change this into some internal call to remove API recursiveness? > > You should not be implementing this as a regulator at all. Ack -- With best wishes Dmitry