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.schen...@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.schen...@toradex.com> > > Signed-off-by: Philippe Schenker <philippe.schen...@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
