lkml.org 
[lkml]   [2019]   [Jul]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [RFC PATCH 1/2] Regulator: Core: Add clock-enable to fixed-regulator
Date
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@toradex.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@toradex.com>
> > Signed-off-by: Philippe Schenker <philippe.schenker@toradex.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
\
 
 \ /
  Last update: 2019-07-30 23:00    [W:2.181 / U:0.372 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site