| Message ID | cover.1581327762.git.matti.vaittinen@fi.rohmeurope.com |
|---|---|
| Headers | show |
| Series | Support ROHM BD99954 charger IC | expand |
On Mon, Feb 10, 2020 at 02:13:13PM +0200, Matti Vaittinen wrote: > Provide a linear_range helper which can do conversion from user value > to register value 'selector'. > Mark, this is loosely bound to register handling... Do you think > the regmap could host these helpers? There's no real tie to regmap here, something like this could quite happily be used by memory mapped devices where regmap has limited uses and would be a lot to pull in. A separate library would probably make more sense. Not sure how many users there would be outside of power related stuff, I don't recall seeing the pattern elsewhere. Note also that we already have quite extensive helpers for this sort of stuff in the regulator API which I sense may have been involved in this implementation and to an extent in ALSA which takes a different approach with TLVs since it baked selectors directly into the ABI.
Morning Mark, On Tue, 2020-02-11 at 19:06 +0000, Mark Brown wrote: > On Mon, Feb 10, 2020 at 02:13:13PM +0200, Matti Vaittinen wrote: > > > Provide a linear_range helper which can do conversion from user > > value > > to register value 'selector'. > > Mark, this is loosely bound to register handling... Do you think > > the regmap could host these helpers? > > There's no real tie to regmap here, something like this could quite > happily be used by memory mapped devices where regmap has limited > uses > and would be a lot to pull in. A separate library would probably > make > more sense. Ugh. I know you are right. I can admit that I just saw it easier to get this included in something that is already existing (besides, I have found co-operating with the regmap/regulator maintainer pretty straightforward - there has been no extra hassle required to get things done, just the necessary fixes). I'm a bit afraid that adding a new piece of library just for this might require a lot more social influencing ^_^; But you are correct. Devices with this kind of registers can easily be connected to memory mapped bus. > Not sure how many users there would be outside of power > related stuff, I don't recall seeing the pattern elsewhere. I think there must be other stuff too. Bunch of NCOs (numeric controller oscillators) from my clock/synchronization related career comes to mind first. AFAIR they had linear frequency response. In comparison tuning clock with DAC voltage was not linar - although voltage change was linear :) But I would expect that there is bunch of things (although I can not guarantee it). > Note also that we already have quite extensive helpers for this sort > of > stuff in the regulator API which I sense may have been involved in > this > implementation You sense well xD This has been inspired by REGULATOR_LINEAR_RANGES in regulator framework. I thought I did write that to somewhere so that the credits would go to regulator code :) But maybe I did only write that when I first introduced this code to BD70528 power-supply code. I thought that we should add generic helpers and that the regulator framework could switch to use them internally if it seemed like a good idea. But another option - which I thought only now - would be to see if current regulator implementation could be re-named to more generic and placed under some more generic component (I thought of regmap but as you pointed out this is equally usefull for devices connected to memory mapped buses - so maybe under lib - if static inline functions in a header are not a good option). I just have a feeling that the linear- ranges is currently kind of embedded in the code which is internal to regulator framework so it is probably not easily extracted from regulator code? > and to an extent in ALSA which takes a different approach > with TLVs since it baked selectors directly into the ABI. I've never played with any media subsystems. Sound, camera and video are completely unknown to me :/ So if we do not start pulling the range code out of regulator framework (for now at least) - and if we do not place this under regmap - then I can drop you out of the recipient list for this charger driver in order to not pollute your inbox ;) How do you feel Mark, do you want to be following this series? Best Regards, Matti Vaittinen
On Wed, Feb 12, 2020 at 06:56:37AM +0000, Vaittinen, Matti wrote: > On Tue, 2020-02-11 at 19:06 +0000, Mark Brown wrote: > > Note also that we already have quite extensive helpers for this sort > > of > > stuff in the regulator API which I sense may have been involved in > > this > > implementation > You sense well xD If you're factoring stuff out of an existing implementation it'd be good to explicitly do that - this both shows where things came from and also means that you can show that the existing user works with the new code which is good. > But another option - which I thought only now - would be to see if > current regulator implementation could be re-named to more generic and > placed under some more generic component (I thought of regmap but as > you pointed out this is equally usefull for devices connected to memory > mapped buses - so maybe under lib - if static inline functions in a > header are not a good option). I just have a feeling that the linear- > ranges is currently kind of embedded in the code which is internal to > regulator framework so it is probably not easily extracted from > regulator code? It is a bit but I think that's solvable with some refactoring in place (eg, pushing things into a smaller struct embedded in the main regulator one and then moving them out). I might look at it myself if nobody else gets to it first... > So if we do not start pulling the range code out of regulator framework > (for now at least) - and if we do not place this under regmap - then I > can drop you out of the recipient list for this charger driver in order > to not pollute your inbox ;) How do you feel Mark, do you want to be > following this series? Well, if there's a refactoring out of the regulator code going on I'll need to look at that anyway.
On Fri, 2020-02-14 at 11:47 +0000, Mark Brown wrote: > On Wed, Feb 12, 2020 at 06:56:37AM +0000, Vaittinen, Matti wrote: > > On Tue, 2020-02-11 at 19:06 +0000, Mark Brown wrote: > > > Note also that we already have quite extensive helpers for this > > > sort > > > of > > > stuff in the regulator API which I sense may have been involved > > > in > > > this > > > implementation > > You sense well xD > > If you're factoring stuff out of an existing implementation it'd be > good > to explicitly do that - this both shows where things came from and > also > means that you can show that the existing user works with the new > code > which is good. True. But I didn't refactor the regulator code - I stole the idea and wrote this thing in BD70528 power-supply driver. I didn't think of creating generic helper until Linus W mentioned we should create one. So now when I did the BD99954 driver and noticed I needed same helper in power-supply area again I decided to get the implementation I did in BD70528 driver and make it available for all. That was a low-hanging fruit for me as I authored the BD70528 and know the linear-range code was easy to pull out of the BD70528. Changing the regulator system was not as easy for me - although it is doable. > > But another option - which I thought only now - would be to see if > > current regulator implementation could be re-named to more generic > > and > > placed under some more generic component (I thought of regmap but > > as > > you pointed out this is equally usefull for devices connected to > > memory > > mapped buses - so maybe under lib - if static inline functions in a > > header are not a good option). I just have a feeling that the > > linear- > > ranges is currently kind of embedded in the code which is internal > > to > > regulator framework so it is probably not easily extracted from > > regulator code? > > It is a bit but I think that's solvable with some refactoring in > place > (eg, pushing things into a smaller struct embedded in the main > regulator > one and then moving them out). I might look at it myself if nobody > else > gets to it first... I need something like this in order to convert BD99954 current and voltage values to register values. I will happily use what-ever you do pull together - but if you don't feel like doing it now I might look at the regulator part while I am working with BD99954 anyways. Please just let me know if you want me to see if I can pull the range stuff out of regulator area. > > So if we do not start pulling the range code out of regulator > > framework > > (for now at least) - and if we do not place this under regmap - > > then I > > can drop you out of the recipient list for this charger driver in > > order > > to not pollute your inbox ;) How do you feel Mark, do you want to > > be > > following this series? > > Well, if there's a refactoring out of the regulator code going on > I'll > need to look at that anyway. My first idea was not to change the regulators now - hence I asked if I should drop you. But I definitely need your support if we decide to refactor the regulator code in this series and create these common helpers out of it. Br, Matti Vaittinen
Morning Mark, On Fri, 2020-02-14 at 14:32 +0200, Matti Vaittinen wrote: > On Fri, 2020-02-14 at 11:47 +0000, Mark Brown wrote: > > On Wed, Feb 12, 2020 at 06:56:37AM +0000, Vaittinen, Matti wrote: > > > On Tue, 2020-02-11 at 19:06 +0000, Mark Brown wrote: > > > > > > > > It is a bit but I think that's solvable with some refactoring in > > place > > (eg, pushing things into a smaller struct embedded in the main > > regulator > > one and then moving them out). I might look at it myself if nobody > > else > > gets to it first... > > I need something like this in order to convert BD99954 current and > voltage values to register values. I will happily use what-ever you > do > pull together - but if you don't feel like doing it now I might look > at > the regulator part while I am working with BD99954 anyways. Please > just > let me know if you want me to see if I can pull the range stuff out > of > regulator area. Just to avoid duplicate work - I started working with this. Please let me know if you are already working on it so I am not doing this in vain. By the way - do you have some nice test cases for regulators hidden somewhere? If so, do you think you could share them? I sure have some for BD718x7 but they are somewhat clumsy and require special HW. (I've never liked unit-tests but I must admit there are some specific cases where they would be pretty usable). Best Regards, Matti Vaittinen
On Tue, Feb 18, 2020 at 07:23:38AM +0000, Vaittinen, Matti wrote: > By the way - do you have some nice test cases for regulators hidden > somewhere? If so, do you think you could share them? I sure have some > for BD718x7 but they are somewhat clumsy and require special HW. (I've > never liked unit-tests but I must admit there are some specific cases > where they would be pretty usable). You can't really run tests on actual regulator drivers outside of test rigs as they're kind of important to the system they're running in.
Hello Mark, On Tue, 2020-02-18 at 13:49 +0000, Mark Brown wrote: > On Tue, Feb 18, 2020 at 07:23:38AM +0000, Vaittinen, Matti wrote: > > > By the way - do you have some nice test cases for regulators hidden > > somewhere? If so, do you think you could share them? I sure have > > some > > for BD718x7 but they are somewhat clumsy and require special HW. > > (I've > > never liked unit-tests but I must admit there are some specific > > cases > > where they would be pretty usable). > > You can't really run tests on actual regulator drivers outside of > test > rigs as they're kind of important to the system they're running in. Right. I was thinking of some tests which would only be testing the vsel logic by feeding test ranges to functions and verifying the resulting selectors/voltages were correct. Unit test style thing where part of the code is stubbed out and rest is compiled into a test executable. I know some people do such (and I usually don't see the effort/benefit ratio to be too great there - although there are cases where such tests can be beneficial). So I decided to ask if you are one of those who do such testing and happened to have such for regulator functions. But this is clear now. I'll try running some tests on BD71847 break-out board at the end of RFC phase - but I do appreciate all the testing on other systems as well! Touching the regulator core is a bit scary as it can result ... problems :/ Best Regards Matti Vaittinen