On Tue, 29 Dec 2020, Vaittinen, Matti wrote: > Hello Again peeps, > > On Thu, 2020-12-17 at 12:04 +0200, Matti Vaittinen wrote: > > On Wed, 2020-12-02 at 15:32 +0200, Matti Vaittinen wrote: > > > Hello Lee, > > > > > > On Wed, 2020-12-02 at 12:57 +0000, Lee Jones wrote: > > > > On Fri, 27 Nov 2020, Vaittinen, Matti wrote: > > > > > > > > > Hello Lee, > > > > > > > > > > On Fri, 2020-11-27 at 08:32 +0000, Lee Jones wrote: > > > > > > On Mon, 23 Nov 2020, Matti Vaittinen wrote: > > > > > > > > > > > > > Add core support for ROHM BD9576MUF and BD9573MUF PMICs > > > > > > > which > > > > > > > are > > > > > > > mainly used to power the R-Car series processors. > > > > > > > > > > > > > > Signed-off-by: Matti Vaittinen < > > > > > > > matti.vaittinen@xxxxxxxxxxxxxxxxx> > > > > > > > --- > > > > > > > drivers/mfd/Kconfig | 11 ++++ > > > > > > > drivers/mfd/Makefile | 1 + > > > > > > > drivers/mfd/rohm-bd9576.c | 108 > > > > > > > +++++++++++++++++++++++++++++++ > > > > > > > include/linux/mfd/rohm-bd957x.h | 59 +++++++++++++++++ > > > > > > > include/linux/mfd/rohm-generic.h | 2 + > > > > > > > 5 files changed, 181 insertions(+) > > > > > > > create mode 100644 drivers/mfd/rohm-bd9576.c > > > > > > > create mode 100644 include/linux/mfd/rohm-bd957x.h > > > > > > > > > > > > Looks like a possible candidate for "simple-mfd-i2c". > > > > > > > > > > > > Could you look into that please? > > > > > > > > > > > I must admit I didn't know about "simple-mfd-i2c". Good thing > > > > > to > > > > > know > > > > > when working with simple devices :) Is this a new thing? > > > > > > > > Yes, it's new. > > > > > > > > > I am unsure I understand the idea fully. Should users put all > > > > > the > > > > > different regamp configs in this file and just add the device > > > > > IDs > > > > > with > > > > > pointer to correct config? (BD9576 and BD9573 need volatile > > > > > ranges). > > > > > Also, does this mean each sub-device should have own node and > > > > > own > > > > > compatible in DT to get correctly load and probed? I guess this > > > > > would > > > > > need a buy-in from Rob too then. > > > > > > > > You should describe the H/W in DT. > > > > > > Yes. And it is described. But I've occasionally received request > > > from > > > DT guys to add some properties directly to MFD node and not to add > > > own > > > sub-node. This is what is done for example with the BD71837/47 > > > clocks > > > - > > > there is no own node for clk - the clk properties are placed > > > directly > > > in MFD node (as was requested by Stephen and Rob back then - I > > > originally had own node for clk). I really have no clear view on > > > when > > > things warrant for own subnode and when they don't - but as far as > > > I > > > can see using simple-mfd-i2c forces one to always have a sub-node / > > > device. Even just a empty node with nothing but the compatible even > > > if > > > device does not need stuff from DT? Anyways, I think this is nice > > > addition for simple drivers. > > > > > > > > By the way - for uneducated eyes like mine this does not look > > > > > like > > > > > it > > > > > has much to do with MFD as a device - here MFD reminds me of a > > > > > simple- > > > > > bus on top of I2C. > > > > > > > > This is for MFD devices where the parent does little more than > > > > create > > > > a shared address space for child devices to operate on - like > > > > yours. > > > > > > > > > Anyways, the BD9576 and BD9573 both have a few interrupts for > > > > > OVD/UVD > > > > > conditions and I am expecting that I will be asked to provide > > > > > the > > > > > regulator notifiers for those. Reason why I omitted the IRQs > > > > > for > > > > > now is > > > > > that the HW is designed to keep the IRQ asserted for whole > > > > > error > > > > > duration so some delayed ack mechanism would be needed. I would > > > > > like to > > > > > keep the door open for adding IRQs to MFD core. > > > > > > > > You mean to add an IRQ Domain? > > > > > > Yes. I planned to use regmap-irq and create irq chip in MFD when > > > the > > > over / under voltage / temperature - notifications or watchdog IRQs > > > are > > > needed. > > > > I am sorry if I have missed your reply. The ROHM email had redirected > > almost all patch emails to spam + I am not sure if some mails are > > dropping :( > > > > (I am considering moving to gmail - but I'd rather keep all mails in > > one system and I can't transfer work mail traffic to gmail... I > > wonder > > how others are managing the mails - which mail system you are using?) > > > > I think this series is now pending the decision how to proceed with > > MFD > > part. If you still want me to start with "simple-mfd-i2c", then I > > would > > appreciate if you pointed me how you would like to see the regmap > > configs added. Although I am quite positive this (eventually) ends up > > being more than what simple-mfd-i2c is intended for (because at some > > point people want to add the use of the interrupts). > > Looking at this topic again. I kind of understand the idea of combining > bunch of MFD drivers into one file. Many of the ROHM PMIC MFD drivers > do provide same functionality. Regmap configs, regmap IRQ and MFD > cells. Some do also probe the device. So having own file for each IC is > likely to not scale well when more devices are supported (and I do hope > this will be the case also with the ROHM ICs). > > What bugs me with the simple-mfd-i2c here is: > 1. Requiring to have own compatibles for sub-devices (regulator and > WDG) to get them properly probed. (3 compatibles for 1 IC). > 2. Requiring to have own DT node for WDG. > 3. Supporting differences between BD9576 and BD9573 by having 6 > compatibles for 2 ICs. > 4. Adding interrupt support. Linux sees each of these functions as separate devices which are handled in different ways by isolated subsystems. So yes, they each require their own compatible string regardless of whether they share the same physical piece of silicon or not. > So ... How do you see adding BD9576/BD9573 MFD stuff in BD9571/(BD9574) > MFD driver? The data structures (regmap configs, MFD cells, regmap IRQ > portion when added) will be different but the functions and maybe > engineers looking at these may be common. > > Is it just plain confusing to add core structures for technically > different ICs in same file - or is it way to avoid duplicating same > code in many files? I can try adding the BD9576/BD9573 to the BD9571 > core - or I can do resend this as is (rebased on 5.11). I can also hack > this to be kicked by simple-mfd-i2c (although I have these strong > objections) - but I bet it will in the long run just lead to a sub- > optimal solution. When the BD9576/BD9573 logic blocks are re-used in > some "non simple" designs and re-using the sub-drivers is needed and/or > when IRQs are needed. > > (BTW - I am currently working with BD71815/BD71817 - and after this > discussion I will add these in BD71828/BD71878 MFD core. I had created > new MFD file for them but this discussion has been a nice kick to the > better direction for me) Everything will be a trade-off. There will either be superflouous files or inflexible code. You have to make the right decision for the driver and the subsystem. -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
