Message-ID: <af076ff7e1df4c07ab659ff83efa0c85d5e5e3d6.camel@toradex.com>
Date:   Tue, 6 Aug 2019 12:57:32 +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 Mon, 2019-08-05 at 17:37 +0100, Mark Brown wrote:
> On Mon, Aug 05, 2019 at 11:07:58AM +0000, Philippe Schenker wrote:
> > On Wed, 2019-07-31 at 22:23 +0100, Mark Brown wrote:
> 
> Please fix your mail client to word wrap within paragraphs at something
> substantially less than 80 columns.  Doing this makes your messages much
> easier to read and reply to.

That was a mistake, sorry. It actually is and was set to 80. Not sure
what happened there.

So it's not switching with the clock, the circuit somehow keeps
the
> > > switch latched?
> > No, it doesn't keep it latched. To make things clear here a status table:
> 
> So the capacitor on the input of the p-FET is keeping the switch on?
> When I say it's not switching with the clock I mean it's not constantly
> bouncing on and off at whatever rate the clock is going at.

Ah, that's what you mean. Yes, the capacitor gets slowly charged with
the
resistor but nearly instantly discharged with the n-FET. So this
capacitor
is used as a Low-Pass filter to get the p-FET to be constantly switched.

It is not bouncing on and off with the clock but rather it is switched
constantly.
> 
> > > It does feel like it might be simpler to just handle this as a quirk in
> > > the PHY or ethernet driver, this feels like an awful lot of work to
> > > add a sleep on what's probably only going to ever be one system.
> > I thought of that too, but the problem with that approach is that I
> > can't reflect the actual switching behavior. What would happen is if
> > you turnethernet off with 'ip link set eth0 down', the clock would
> > stop and therefore no more supply voltage to the PHY. But the ethernet
> > driverwould in that case let the regulator enabled preventing,
> > switching off the clock.
> 
> You could include that in your quirk?

Yes, this could be done as a quirk in the ethernet driver. But there the
clock gets enabled/disabled in different places. So I would have to sort
out where I need it and where not and basically go through the whole
driver. Plus it gets intensive to maintain that solution.
> 
> > Anyway I feel that to solve this with a quirk would be a little
> > hackish, plus I'd anyway need to mess around with the Ethernet/PHY
> > drivers. So why not solve it properly with a regulator that supports
> > clocks?
> 
> I think you are going to end up with a hack no matter what.

That's exactly what I'm trying to prevent. To introduce a fixed
regulator that can have a clock is not a hack for me.
That the hardware solution is a hack is debatable yes, but why should I
not try to solve it properly in software?
> 
> > > Hopefully not a *lot* of duplication.  The GPIOs are handled in the core
> > > because they're really common and used by many regulator devices, the
> > > same will I hope not be true for clocks.
> > I agree that they are commonly and widely used. To add support for clocks in
> > regulator-core was really easy to do as I did it the same way as it is done
> > with
> > gpio's. If I don't need to touch regulator-core I don't want to. But as I
> > said
> > it was really easy for me to integrate it in there in a way without even
> > understanding the whole regulator API.
> > If it makes more sense to do it in a new file like clock-regulator.c and
> > creating a new compatible that is what I'm trying to find out here. I'd be
> > happy
> > to write also a new clock-regulator driver for that purpose.
> 
> It would be better if it wasn't in the core, that keeps everything
> partitioned off nicely.

I agree that our hardware design is somewhat special and is not widely
used. But I still think that this is valuable as a generic function and
could possibly be of benefit to someone else.
> 
> > > I guess my question here is what the trip through the regulator API buys
> > > us - it's a bit of a sledgehammer to crack a nut thing.
> > In my opinion this is not only about to solve my problem with startup-delay. 
> > I
> > think that this is really a behavior that can be generic. That's also why
> > I'm
> > asking here how we want to solve that so not only I solve my little problem
> > with
> > a board quirk but in a broader view for possible future usage by others.
> > It is possible that a regulator needs a clock. That exactly is, what we have
> > on
> > our board and works better than expected (at least by myself :-)).
> 
> The majority of regulators that need clocks are PWM devices which is a
> whole other thing that we already support.  This is a highly unusual
> hardware design, we don't have the regmap stuff in the core and that's a
> lot more common.

I agree with you, because of our unusual hardware design that this
shouldn't be in core. But I do not agree on solving that with quirks.

In the end I just want to represent our hardware in software. Would you
agree to create a new clock-regulator.c driver?
Or would it make more sense to extend fixed.c to support clocks-enable
without touching core?

Powered by blists - more mailing lists