Message-ID: <b5e1cc3fb5838d9ea4160078402bff95903ba0da.camel@toradex.com>
Date:   Tue, 30 Jul 2019 21:00:01 +0000
From:   Philippe Schenker <philippe.schenker@...adex.com>
To:     "broonie@...nel.org" <broonie@...nel.org>
CC:     "lgirdwood@...il.com" <lgirdwood@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 1/2] Regulator: Core: Add clock-enable to
 fixed-regulator

On Tue, 2019-07-30 at 19:10 +0100, Mark Brown wrote:
> On Tue, Jul 30, 2019 at 07:30:04PM +0200, Philippe Schenker wrote:
> > From: Philippe Schenker <philippe.schenker@...adex.com>
> > 
> > This adds the possibility to enable a fixed-regulator with a clock.
> 
> Why?  What does the hardware which makes this make sense look like?

Tomorrow I can provide some schematics if needed. But its just a simple
switch that is switched by a clock (on when clock is on and off when
clock is off). This clock is the RGMII 50MHz clock for the ethernet
PHY.

That switch switches power rail of a KSZ8041 ethernet PHY. So the power
rail of the KSZ8041 PHY is switched by its own clock.

> Your cover letter didn't explain at all clearly, it just said that
> there's a circuit that is connected to a clock which somehow switches
> something but it's not clear.  It's certainly not clear that this
> should
> be in the core, the circuit doesn't sound like a good idea at all.

Sorry if I didn't explain it clear enough. I hope the hardware part is
clear now from the explanation above. Otherwise let me know I will
provide further explanations/schematics.

To your other questions, I will split those for better understanding:

Why is a regulator even needed?
- On power up of the PHY there is a huge time I have to wait for
voltage rail to settle. In the range of 100ms.
- Because there is a switch in the circuit I abstract it with a
regulator-fixed in devicetree to make use of the startup-delay.
- This regulator/switch is enabled with a clock. So to be able to use
the startup delay I need an enable-by-clock on regulator-fixed.

Why do I think this should be in core?
- Normally this task is done with gpio that is already in regulator-
core.
- Because that is already there I added the functionality for enabled-
by-clock-functionality.
- I thought of creating a new regulator-clock driver but that would
hold a lot of code duplication from regulator-fixed.

Why is this a good Idea at all?
- Well I'm here for the software part and should just support our
hardware. If that is a good Idea at all I don't know, for sure it is
not a solution that is from some school-book. But I tried it and
measured it out and it seems to work pretty fine.
- The reason behind all of that is limited GPIO availability from the
iMX6ULL.

> 
> > Signed-off-by: <philippe.schenker@...adex.com>
> > Signed-off-by: Philippe Schenker <philippe.schenker@...adex.com>
> 
> This needs a cleanup.

Of course, sorry I didn't saw that beforehand. Some mess created with
cherry-picking...

> 
> >  
> >  	/* cares about last_off_jiffy only if off_on_delay is required
> > by
> > @@ -2796,6 +2805,9 @@ static int _regulator_is_enabled(struct
> > regulator_dev *rdev)
> >  	if (rdev->ena_pin)
> >  		return rdev->ena_gpio_state;
> >  
> > +	if (rdev->ena_clk)
> > +		return (rdev->ena_clk_state > 0) ? 1 : 0;
> > +
> 
> Please write normal conditional statements, this isn't helping
> legibility.  Though in this case the ternery operator is totally
> redundant anyway...

Yeah now that I look at it you're right. I have in mind that I copied
that from somewhere to get the same coding style. I developed that in
an old kernel so could be that it's from there.
Anyway, this is just a concept for now and would need some more
thinking...
With this patch I want to put off a discussion, how we can support our
hardware in mainline Linux. This is my first proposal for that.

Philippe

Powered by blists - more mailing lists