On Tue, 22 Jun 2021 at 17:38, Mark Brown [off-list ref] wrote:

On Tue, Jun 22, 2021 at 05:17:28PM +0300, Dmitry Baryshkov wrote:

quoted

On Tue, 22 Jun 2021 at 14:29, Mark Brown [off-list ref] wrote:

quoted

On Tue, Jun 22, 2021 at 01:31:36AM +0300, Dmitry Baryshkov wrote:

quoted

quoted

quoted

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.

quoted

quoted

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.

quoted

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.

quoted

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

quoted

quoted

quoted

+static int qca6390_enable(struct regulator_dev *rdev)
+{
+     struct qca6390_data *data = rdev_get_drvdata(rdev);
+     int ret;

quoted

quoted

quoted

+     ret = regulator_bulk_enable(data->num_vregs, data->regulators);
+     if (ret) {
+             dev_err(data->dev, "Failed to enable regulators");
+             return ret;
+     }

quoted

quoted

The regulator API is *not* recursive, I am astonished this works.

quoted

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