mbox series

[0/3] Add new binding regulator-fixed-clock to regulator-fixed

Message ID 20190903080336.32288-1-philippe.schenker@toradex.com
Headers show
Series Add new binding regulator-fixed-clock to regulator-fixed | expand

Message

Philippe Schenker Sept. 3, 2019, 8:03 a.m. UTC 
Our hardware has a FET that is switching power rail of the ethernet PHY
on and off. This switching enable signal is a clock from the SoC.

There is no possibility in regulator subsystem to have this hardware
reflected in software.

I already discussed with Mark Brown about possible solutions and he
suggested to create at least a new compatible. [1]
This discussion includes also a better explanation of our circuit as
well as schematics. So please refer to that link if you have questions
about that.

In this first attempt I created a new binding "regulator-fixed-clock"
that can take a clock from devicetree. This is a simple addition to
regulator-fixed. If the binding regulator-fixed-clock is given, the
clock is simply enabled on regulator enable and disabled on regulator
disable.
To be able to have multiple consumers a counter variable is also given
that tells how many consumers need power from this regulator.

Best regards,
Philippe

[1] https://lkml.org/lkml/2019/8/7/78



Philippe Schenker (3):
  regulator: fixed: add possibility to enable by clock
  ARM: dts: imx6ull-colibri: add phy-supply and respective regulator
  dt-bindings: regulator: add regulator-fixed-clock binding

 .../bindings/regulator/fixed-regulator.yaml   | 18 +++-
 arch/arm/boot/dts/imx6ull-colibri.dtsi        | 12 +++
 drivers/regulator/fixed.c                     | 86 ++++++++++++++++++-
 3 files changed, 112 insertions(+), 4 deletions(-)

Comments

Mark Brown Sept. 5, 2019, 6:06 p.m. UTC | #1 
On Tue, Sep 03, 2019 at 08:03:46AM +0000, Philippe Schenker wrote:
> This commit adds the possibility to choose the compatible
> "regulator-fixed-clock" in devicetree.
> 
> This is a special regulator-fixed that has to have a clock, from which
> the regulator gets switched on and off.

This seems conceptually fine.  Minor issues though:

> +static int reg_clock_is_enabled(struct regulator_dev *rdev)
> +{
> +	struct fixed_voltage_data *priv = rdev_get_drvdata(rdev);
> +
> +	if (priv->clk_enable_counter > 0)
> +		return 1;
> +
> +	return 0;
> +}

This could just be return priv->clk_enable_counter > 0 - ideally the
clock API would let us query if the clock is enabled but that might be a
bit confused anyway given that it's possibly shared.
Philippe Schenker Sept. 10, 2019, 6:08 a.m. UTC | #2 
On Thu, 2019-09-05 at 19:06 +0100, Mark Brown wrote:
> On Tue, Sep 03, 2019 at 08:03:46AM +0000, Philippe Schenker wrote:
> > This commit adds the possibility to choose the compatible
> > "regulator-fixed-clock" in devicetree.
> > 
> > This is a special regulator-fixed that has to have a clock, from
> > which
> > the regulator gets switched on and off.
> 
> This seems conceptually fine.  Minor issues though:

Thanks for your comments and I'm glad you like it! I will send a v2
shortly, also with Rob's fixes in. Can I expect it to be pulled for 5.4?

Best regards,
Philippe

> 
> > +static int reg_clock_is_enabled(struct regulator_dev *rdev)
> > +{
> > +	struct fixed_voltage_data *priv = rdev_get_drvdata(rdev);
> > +
> > +	if (priv->clk_enable_counter > 0)
> > +		return 1;
> > +
> > +	return 0;
> > +}
> 
> This could just be return priv->clk_enable_counter > 0 - ideally the
> clock API would let us query if the clock is enabled but that might be
> a
> bit confused anyway given that it's possibly shared.
Philippe Schenker Sept. 10, 2019, 6:14 a.m. UTC | #3 
On Tue, 2019-09-10 at 06:08 +0000, Philippe Schenker wrote:
> On Thu, 2019-09-05 at 19:06 +0100, Mark Brown wrote:
> > On Tue, Sep 03, 2019 at 08:03:46AM +0000, Philippe Schenker wrote:
> > > This commit adds the possibility to choose the compatible
> > > "regulator-fixed-clock" in devicetree.
> > > 
> > > This is a special regulator-fixed that has to have a clock, from
> > > which
> > > the regulator gets switched on and off.
> > 
> > This seems conceptually fine.  Minor issues though:
> 
> Thanks for your comments and I'm glad you like it! I will send a v2
> shortly, also with Rob's fixes in. Can I expect it to be pulled for
> 5.4?

I meant 5.5 of course.

> 
> Best regards,
> Philippe
> 
> > > +static int reg_clock_is_enabled(struct regulator_dev *rdev)
> > > +{
> > > +	struct fixed_voltage_data *priv = rdev_get_drvdata(rdev);
> > > +
> > > +	if (priv->clk_enable_counter > 0)
> > > +		return 1;
> > > +
> > > +	return 0;
> > > +}
> > 
> > This could just be return priv->clk_enable_counter > 0 - ideally the
> > clock API would let us query if the clock is enabled but that might
> > be
> > a
> > bit confused anyway given that it's possibly shared.