On Fri 20 Aug 01:10 PDT 2021, Dmitry Baryshkov wrote:
> Hi,
>
> On Fri, 20 Aug 2021 at 02:17, Bjorn Andersson
> <bjorn.andersson@xxxxxxxxxx> wrote:
> >
> > On Mon 16 Aug 17:55 PDT 2021, Dmitry Baryshkov wrote:
> > [..]
> > > diff --git a/drivers/power/pwrseq/pwrseq_qca.c b/drivers/power/pwrseq/pwrseq_qca.c
> > > new file mode 100644
> > > index 000000000000..3421a4821126
> > > --- /dev/null
> > > +++ b/drivers/power/pwrseq/pwrseq_qca.c
> > > @@ -0,0 +1,290 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Copyright (c) 2021, Linaro Ltd.
> > > + *
> > > + * Author: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > > + *
> > > + * Power Sequencer for Qualcomm WiFi + BT SoCs
> > > + */
> > > +
> > > +#include <linux/delay.h>
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/pwrseq/driver.h>
> > > +#include <linux/regulator/consumer.h>
> > > +
> > > +/*
> > > + * Voltage regulator information required for configuring the
> > > + * QCA WiFi+Bluetooth chipset
> > > + */
> > > +struct qca_vreg {
> > > + const char *name;
> > > + unsigned int load_uA;
> > > +};
> > > +
> > > +struct qca_device_data {
> > > + struct qca_vreg vddio;
> >
> > Any particular reason why this isn't just the first entry in vregs and
> > operated as part of the bulk API?
>
> Because VDDIO should be up before bringing the rest of the power
> sources (at least for wcn39xx). This is usually the case since VDDIO
> is S4A, but I'd still prefer to express this in the code.
> And register_bulk_enable powers up all the supplies asynchronously,
> thus it can not guarantee that the first entry would be powered up
> first.
>
Ahh, forgot about the async nature of bulk_enable. Make the code a
little ugly, but it needs to be done like that.
Thinking about it, isn't there a required minimum time between vddio and
the others in the wcn specification?
> >
> > > + struct qca_vreg *vregs;
> > > + size_t num_vregs;
> > > + bool has_bt_en;
> > > + bool has_wifi_en;
> > > +};
> > > +
> > > +struct pwrseq_qca;
> > > +struct pwrseq_qca_one {
> > > + struct pwrseq_qca *common;
> > > + struct gpio_desc *enable;
> > > +};
> > > +
> > > +#define PWRSEQ_QCA_WIFI 0
> > > +#define PWRSEQ_QCA_BT 1
> > > +
> > > +#define PWRSEQ_QCA_MAX 2
> > > +
> > > +struct pwrseq_qca {
> > > + struct regulator *vddio;
> > > + struct gpio_desc *sw_ctrl;
> > > + struct pwrseq_qca_one pwrseq_qcas[PWRSEQ_QCA_MAX];
> > > + int num_vregs;
> > > + struct regulator_bulk_data vregs[];
> > > +};
> > > +
> > > +static int pwrseq_qca_power_on(struct pwrseq *pwrseq)
> > > +{
> > > + struct pwrseq_qca_one *qca_one = pwrseq_get_data(pwrseq);
> > > + int ret;
> > > +
> > > + if (qca_one->common->vddio) {
> >
> > devm_regulator_get() doesn't return NULL, so this is always true.
>
> This is more of the safety guard for the cases when the qca doesn't
> have the special vddio supply.
>
If you think there's such a case coming up, then it makes sense.
On the flip side, debugging the resulting panic when someone adds a new
compatible without vddio is very minor...
I think this looks good then.
Regards,
Bjorn
