* [PATCH 1/1] clk: clk-cdce925: Add regulator support
@ 2019-06-14 3:52 Phil Reid
2019-06-25 22:54 ` Stephen Boyd
0 siblings, 1 reply; 6+ messages in thread
From: Phil Reid @ 2019-06-14 3:52 UTC (permalink / raw)
To: mturquette, sboyd, preid, linux-clk
The cdce925 power supplies could be controllable on some platforms.
Enable them before communicating with the cdce925.
Signed-off-by: Phil Reid <preid@electromag.com.au>
---
Notes:
We see a kernel panic later in the boot if the regulator is not
enabled. Unsure what in the driver is causing that. Something
to do with regmap perhaps?
drivers/clk/clk-cdce925.c | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/drivers/clk/clk-cdce925.c b/drivers/clk/clk-cdce925.c
index a98b3f19..2678ee6 100644
--- a/drivers/clk/clk-cdce925.c
+++ b/drivers/clk/clk-cdce925.c
@@ -16,6 +16,7 @@
#include <linux/module.h>
#include <linux/i2c.h>
#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
#include <linux/slab.h>
#include <linux/gcd.h>
@@ -602,6 +603,30 @@ static int cdce925_regmap_i2c_read(void *context,
return &data->clk[idx].hw;
}
+static void cdce925_regulator_disable(void *regulator)
+{
+ regulator_disable(regulator);
+}
+
+static int cdce925_regulator_enable(struct device *dev, const char *name)
+{
+ struct regulator *regulator;
+ int err;
+
+ regulator = devm_regulator_get(dev, name);
+ if (IS_ERR(regulator))
+ return PTR_ERR(regulator);
+
+ err = regulator_enable(regulator);
+ if (err) {
+ dev_err(dev, "Failed to enable %s: %d\n", name, err);
+ return err;
+ }
+
+ return devm_add_action_or_reset(dev, cdce925_regulator_disable,
+ regulator);
+}
+
/* The CDCE925 uses a funky way to read/write registers. Bulk mode is
* just weird, so just use the single byte mode exclusively. */
static struct regmap_bus regmap_cdce925_bus = {
@@ -630,6 +655,15 @@ static int cdce925_probe(struct i2c_client *client,
};
dev_dbg(&client->dev, "%s\n", __func__);
+
+ err = cdce925_regulator_enable(&client->dev, "vdd");
+ if (err)
+ return err;
+
+ err = cdce925_regulator_enable(&client->dev, "vddout");
+ if (err)
+ return err;
+
data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
if (!data)
return -ENOMEM;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/1] clk: clk-cdce925: Add regulator support
2019-06-14 3:52 [PATCH 1/1] clk: clk-cdce925: Add regulator support Phil Reid
@ 2019-06-25 22:54 ` Stephen Boyd
2019-06-26 1:06 ` Phil Reid
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Boyd @ 2019-06-25 22:54 UTC (permalink / raw)
To: linux-clk, mturquette, preid
Quoting Phil Reid (2019-06-13 20:52:43)
> The cdce925 power supplies could be controllable on some platforms.
> Enable them before communicating with the cdce925.
>
> Signed-off-by: Phil Reid <preid@electromag.com.au>
> ---
>
> Notes:
> We see a kernel panic later in the boot if the regulator is not
> enabled. Unsure what in the driver is causing that. Something
> to do with regmap perhaps?
>
> drivers/clk/clk-cdce925.c | 34 ++++++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> diff --git a/drivers/clk/clk-cdce925.c b/drivers/clk/clk-cdce925.c
> index a98b3f19..2678ee6 100644
> --- a/drivers/clk/clk-cdce925.c
> +++ b/drivers/clk/clk-cdce925.c
> @@ -16,6 +16,7 @@
> #include <linux/module.h>
> #include <linux/i2c.h>
> #include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> #include <linux/slab.h>
> #include <linux/gcd.h>
>
> @@ -602,6 +603,30 @@ static int cdce925_regmap_i2c_read(void *context,
> return &data->clk[idx].hw;
> }
>
> +static void cdce925_regulator_disable(void *regulator)
> +{
> + regulator_disable(regulator);
> +}
> +
> +static int cdce925_regulator_enable(struct device *dev, const char *name)
> +{
> + struct regulator *regulator;
> + int err;
> +
> + regulator = devm_regulator_get(dev, name);
> + if (IS_ERR(regulator))
> + return PTR_ERR(regulator);
> +
> + err = regulator_enable(regulator);
The regulator is never turned off though. Are these regulators really
just always on regulators that don't need to be managed by this driver?
Also, is there an update to the DT binding somewhere?
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/1] clk: clk-cdce925: Add regulator support
2019-06-25 22:54 ` Stephen Boyd
@ 2019-06-26 1:06 ` Phil Reid
2019-06-26 3:46 ` Stephen Boyd
0 siblings, 1 reply; 6+ messages in thread
From: Phil Reid @ 2019-06-26 1:06 UTC (permalink / raw)
To: Stephen Boyd, linux-clk, mturquette
On 26/06/2019 06:54, Stephen Boyd wrote:
> Quoting Phil Reid (2019-06-13 20:52:43)
>> The cdce925 power supplies could be controllable on some platforms.
>> Enable them before communicating with the cdce925.
>>
>> Signed-off-by: Phil Reid <preid@electromag.com.au>
>> ---
>>
>> Notes:
>> We see a kernel panic later in the boot if the regulator is not
>> enabled. Unsure what in the driver is causing that. Something
>> to do with regmap perhaps?
>>
>> drivers/clk/clk-cdce925.c | 34 ++++++++++++++++++++++++++++++++++
>> 1 file changed, 34 insertions(+)
>>
>> diff --git a/drivers/clk/clk-cdce925.c b/drivers/clk/clk-cdce925.c
>> index a98b3f19..2678ee6 100644
>> --- a/drivers/clk/clk-cdce925.c
>> +++ b/drivers/clk/clk-cdce925.c
>> @@ -16,6 +16,7 @@
>> #include <linux/module.h>
>> #include <linux/i2c.h>
>> #include <linux/regmap.h>
>> +#include <linux/regulator/consumer.h>
>> #include <linux/slab.h>
>> #include <linux/gcd.h>
>>
>> @@ -602,6 +603,30 @@ static int cdce925_regmap_i2c_read(void *context,
>> return &data->clk[idx].hw;
>> }
>>
>> +static void cdce925_regulator_disable(void *regulator)
>> +{
>> + regulator_disable(regulator);
>> +}
>> +
>> +static int cdce925_regulator_enable(struct device *dev, const char *name)
>> +{
>> + struct regulator *regulator;
>> + int err;
>> +
>> + regulator = devm_regulator_get(dev, name);
>> + if (IS_ERR(regulator))
>> + return PTR_ERR(regulator);
>> +
>> + err = regulator_enable(regulator);
>
G'day Stephen,
Thanks for looking at this.
> The regulator is never turned off though. Are these regulators really
> just always on regulators that don't need to be managed by this driver?
>
For our system the regulator needs to be enabled before we try talking to the chip.
Funny that.
Unloading the driver will disable the regulator thru the devm call to
cdce925_regulator_disable
> + return devm_add_action_or_reset(dev, cdce925_regulator_disable,
> + regulator);
> +}
In the future suspend/resume support could be added to power the device down.
The system I have doesn't support suspending thou.
> Also, is there an update to the DT binding somewhere?
>
No I didn't update that.
It seems a bit adhoc if supply reference are including in the DT docs.
I can add something if your happy with the pathc in general.
--
Regards
Phil Reid
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/1] clk: clk-cdce925: Add regulator support
2019-06-26 1:06 ` Phil Reid
@ 2019-06-26 3:46 ` Stephen Boyd
2019-06-26 4:03 ` Phil Reid
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Boyd @ 2019-06-26 3:46 UTC (permalink / raw)
To: Phil Reid, linux-clk, mturquette
Quoting Phil Reid (2019-06-25 18:06:29)
> On 26/06/2019 06:54, Stephen Boyd wrote:
> > Quoting Phil Reid (2019-06-13 20:52:43)
> >> + return PTR_ERR(regulator);
> >> +
> >> + err = regulator_enable(regulator);
> >
> G'day Stephen,
>
> Thanks for looking at this.
> > The regulator is never turned off though. Are these regulators really
> > just always on regulators that don't need to be managed by this driver?
> >
> For our system the regulator needs to be enabled before we try talking to the chip.
> Funny that.
> Unloading the driver will disable the regulator thru the devm call to
> cdce925_regulator_disable
Ok. Is it a regulator that is expected to just always be on though? Or
does the datasheet for this device indicate that these supplies can be
turned on and off when the device isn't in use?
>
> > + return devm_add_action_or_reset(dev, cdce925_regulator_disable,
> > + regulator);
> > +}
>
> In the future suspend/resume support could be added to power the device down.
> The system I have doesn't support suspending thou.
>
> > Also, is there an update to the DT binding somewhere?
> >
> No I didn't update that.
> It seems a bit adhoc if supply reference are including in the DT docs.
> I can add something if your happy with the pathc in general.
>
Yes, the binding needs an update to list out the supplies. If they're
not always going to be enabled because they're controlled supplies in
the design then it makes sense to me to add them to the binding and use
them from this driver so that things operate properly.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] clk: clk-cdce925: Add regulator support
2019-06-26 3:46 ` Stephen Boyd
@ 2019-06-26 4:03 ` Phil Reid
2019-06-26 4:10 ` Stephen Boyd
0 siblings, 1 reply; 6+ messages in thread
From: Phil Reid @ 2019-06-26 4:03 UTC (permalink / raw)
To: Stephen Boyd, linux-clk, mturquette
On 26/06/2019 11:46, Stephen Boyd wrote:
> Quoting Phil Reid (2019-06-25 18:06:29)
>> On 26/06/2019 06:54, Stephen Boyd wrote:
>>> Quoting Phil Reid (2019-06-13 20:52:43)
>>>> + return PTR_ERR(regulator);
>>>> +
>>>> + err = regulator_enable(regulator);
>>>
>> G'day Stephen,
>>
>> Thanks for looking at this.
>>> The regulator is never turned off though. Are these regulators really
>>> just always on regulators that don't need to be managed by this driver?
>>>
>> For our system the regulator needs to be enabled before we try talking to the chip.
>> Funny that.
>> Unloading the driver will disable the regulator thru the devm call to
>> cdce925_regulator_disable
>
> Ok. Is it a regulator that is expected to just always be on though? Or
> does the datasheet for this device indicate that these supplies can be
> turned on and off when the device isn't in use?
Sorry I'm not understanding what your asking.
Should we be using some other method to ensure there is power to this device
before probingHow do I then ensure the dependency to that power supply?
I can't see why that part of the system can't be shut down, provided that inputs
are also isolated correctly when the regulator is disabled etc.
The data sheet says that input can execeed VDD / VDDOUT provide
clamp current ratings are observed.
It is also seems permissible to disable Vddout independently of Vdd (for some variants).
>
>>
>>> + return devm_add_action_or_reset(dev, cdce925_regulator_disable,
>>> + regulator);
>>> +}
>>
>> In the future suspend/resume support could be added to power the device down.
>> The system I have doesn't support suspending thou.
>>
>>> Also, is there an update to the DT binding somewhere?
>>>
>> No I didn't update that.
>> It seems a bit adhoc if supply reference are including in the DT docs.
>> I can add something if your happy with the pathc in general.
>>
>
> Yes, the binding needs an update to list out the supplies. If they're
> not always going to be enabled because they're controlled supplies in
> the design then it makes sense to me to add them to the binding and use
> them from this driver so that things operate properly.
>
>
>
--
Regards
Phil Reid
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] clk: clk-cdce925: Add regulator support
2019-06-26 4:03 ` Phil Reid
@ 2019-06-26 4:10 ` Stephen Boyd
0 siblings, 0 replies; 6+ messages in thread
From: Stephen Boyd @ 2019-06-26 4:10 UTC (permalink / raw)
To: Phil Reid, linux-clk, mturquette
Quoting Phil Reid (2019-06-25 21:03:20)
> On 26/06/2019 11:46, Stephen Boyd wrote:
> > Quoting Phil Reid (2019-06-25 18:06:29)
> >> On 26/06/2019 06:54, Stephen Boyd wrote:
> >>> Quoting Phil Reid (2019-06-13 20:52:43)
> >>>> + return PTR_ERR(regulator);
> >>>> +
> >>>> + err = regulator_enable(regulator);
> >>>
> >> G'day Stephen,
> >>
> >> Thanks for looking at this.
> >>> The regulator is never turned off though. Are these regulators really
> >>> just always on regulators that don't need to be managed by this driver?
> >>>
> >> For our system the regulator needs to be enabled before we try talking to the chip.
> >> Funny that.
> >> Unloading the driver will disable the regulator thru the devm call to
> >> cdce925_regulator_disable
> >
> > Ok. Is it a regulator that is expected to just always be on though? Or
> > does the datasheet for this device indicate that these supplies can be
> > turned on and off when the device isn't in use?
>
> Sorry I'm not understanding what your asking.
> Should we be using some other method to ensure there is power to this device
> before probingHow do I then ensure the dependency to that power supply?
No, I'm not an expert in this clk driver so I have no idea if this is
some sort of supply that is typically always on or if software is
expected to manage the regulator at runtime. I'm just asking to make
sure that your use case requires this be managed at runtime instead of
turned on immediately when the regulator driver probes and left on
forever.
>
> I can't see why that part of the system can't be shut down, provided that inputs
> are also isolated correctly when the regulator is disabled etc.
> The data sheet says that input can execeed VDD / VDDOUT provide
> clamp current ratings are observed.
>
> It is also seems permissible to disable Vddout independently of Vdd (for some variants).
Sounds like it's all fine then. Just document it in the binding and it
should be fine to add.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-06-26 4:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-14 3:52 [PATCH 1/1] clk: clk-cdce925: Add regulator support Phil Reid
2019-06-25 22:54 ` Stephen Boyd
2019-06-26 1:06 ` Phil Reid
2019-06-26 3:46 ` Stephen Boyd
2019-06-26 4:03 ` Phil Reid
2019-06-26 4:10 ` Stephen Boyd
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).