linux-kernel - Re: [PATCH v3 2/7] regulator: qca6390: add support for QCA639x powerup sequence

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 22 Jun 2021 17:17:28 +0300
From:   Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To:     Mark Brown <broonie@...nel.org>
Cc:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Marcel Holtmann <marcel@...tmann.org>,
        Johan Hedberg <johan.hedberg@...il.com>,
        Luiz Augusto von Dentz <luiz.dentz@...il.com>,
        "open list:DRM DRIVER FOR MSM ADRENO GPU" 
        <linux-arm-msm@...r.kernel.org>,
        Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>,
        linux-bluetooth@...r.kernel.org
Subject: Re: [PATCH v3 2/7] regulator: qca6390: add support for QCA639x
 powerup sequence

On Tue, 22 Jun 2021 at 14:29, Mark Brown <broonie@...nel.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]

I've attempted implementing that as a genpd (in previous iterations),
but it results in worse design. PCIe controllers are not expected to
handle power domains for EP devices, especially in cases when the PD
must come up before the controller does link training and bus probe.
I've tried following Rob's suggestions on implementing things clearly,
but doing so results in too big restructure just for a single device.

> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2021, Linaro Limited
> > + */
>
> Please make the entire comment a C++ one so things look more
> intentional.

Ack.

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

>
> > +     /* Wait for 1ms before toggling enable pins. */
> > +     usleep_range(1000, 2000);
>
> There's core support for delays after power on, better to use it.

Ack.

>
> > +     data->enable_counter++;
>
> You shouldn't assume that enable and disable calls are matched.

Ack.

--
With best wishes
Dmitry