linux-bluetooth.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Mark Brown <broonie@kernel.org>
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc: Andy Gross <agross@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Rob Herring <robh+dt@kernel.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Marcel Holtmann <marcel@holtmann.org>,
	Johan Hedberg <johan.hedberg@gmail.com>,
	Luiz Augusto von Dentz <luiz.dentz@gmail.com>,
	"open list:DRM DRIVER FOR MSM ADRENO GPU" 
	<linux-arm-msm@vger.kernel.org>,
	Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@vger.kernel.org>,
	open list <linux-kernel@vger.kernel.org>,
	linux-bluetooth@vger.kernel.org
Subject: Re: [PATCH v3 2/7] regulator: qca6390: add support for QCA639x powerup sequence
Date: Tue, 22 Jun 2021 15:38:12 +0100	[thread overview]
Message-ID: <20210622143812.GE4574@sirena.org.uk> (raw)
In-Reply-To: <CAA8EJpoTdg3O6dzpTaNS5fJRbtb1Fndv0mEuO+e4b6XCmuvzhQ@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 2549 bytes --]

On Tue, Jun 22, 2021 at 05:17:28PM +0300, Dmitry Baryshkov wrote:
> On Tue, 22 Jun 2021 at 14:29, Mark Brown <broonie@kernel.org> 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?  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.

> 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?

> > > +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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

  reply	other threads:[~2021-06-22 14:38 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-21 22:31 [PATCH v3 0/7] Add support for Qualcomm QCA639x chips family Dmitry Baryshkov
2021-06-21 22:31 ` [PATCH v3 1/7] dt-bindings: regulator: qcom,qca6390: add binding for QCA6390 device Dmitry Baryshkov
2021-06-21 23:11   ` Add support for Qualcomm QCA639x chips family bluez.test.bot
2021-06-21 22:31 ` [PATCH v3 2/7] regulator: qca6390: add support for QCA639x powerup sequence Dmitry Baryshkov
2021-06-22 11:28   ` Mark Brown
2021-06-22 14:17     ` Dmitry Baryshkov
2021-06-22 14:38       ` Mark Brown [this message]
2021-06-22 16:46         ` Dmitry Baryshkov
2021-06-22 17:08           ` Mark Brown
2021-07-06  7:54   ` Ulf Hansson
2021-07-06 11:55     ` Mark Brown
2021-07-08 10:09       ` Ulf Hansson
2021-07-08 11:37         ` Dmitry Baryshkov
2021-07-14 16:47           ` Rob Herring
2021-07-14 17:10             ` Bjorn Andersson
2021-08-10 11:55             ` Ulf Hansson
2021-08-10 16:03               ` Bjorn Andersson
2021-08-12  9:48                 ` Ulf Hansson
2021-08-12 11:51                   ` Dmitry Baryshkov
2021-07-14 17:23         ` Bjorn Andersson
2021-06-21 22:31 ` [PATCH v3 3/7] Bluetooth: hci_qca: provide default device data Dmitry Baryshkov
2021-07-14 17:27   ` Bjorn Andersson
2021-06-21 22:31 ` [PATCH v3 4/7] Bluetooth: hci_qca: merge qca_power into qca_serdev Dmitry Baryshkov
2021-07-14 17:25   ` Bjorn Andersson
2021-06-21 22:31 ` [PATCH v3 5/7] Bluetooth: hci_qca: merge wcn & non-wcn code paths Dmitry Baryshkov
2021-06-21 22:31 ` [PATCH v3 6/7] Bluetooth: hci_qca: add power sequencer support to qca6390 Dmitry Baryshkov
2021-06-21 22:31 ` [PATCH v3 7/7] arm64: dts: qcom: qrb5165-rb5: add QCA6391 WiFi+BT SoC Dmitry Baryshkov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210622143812.GE4574@sirena.org.uk \
    --to=broonie@kernel.org \
    --cc=agross@kernel.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=dmitry.baryshkov@linaro.org \
    --cc=johan.hedberg@gmail.com \
    --cc=lgirdwood@gmail.com \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-bluetooth@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luiz.dentz@gmail.com \
    --cc=manivannan.sadhasivam@linaro.org \
    --cc=marcel@holtmann.org \
    --cc=robh+dt@kernel.org \
    --subject='Re: [PATCH v3 2/7] regulator: qca6390: add support for QCA639x powerup sequence' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).