| Message ID | cc46e329efa30c66f000ab7c97f9bbf0bc31f0f7.1605882179.git.matti.vaittinen@fi.rohmeurope.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support ROHM BD9576MUF and BD9573MUF PMICs | expand |
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@fi.rohmeurope.com> > --- > 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?
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@fi.rohmeurope.com> > > --- > > 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? 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. 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. 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. Best Regards Matti
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@fi.rohmeurope.com> > > > --- > > > 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. > 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?
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@fi.rohmeurope.com> > > > > --- > > > > 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. Best Regards, Matti Vaittinen
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@fi.rohmeurope.com> > > > > --- > > > > 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. After re-reading this - do you mean one should describe for example the register ranges in DT? I don't see code which parses the volatile ranges or other regmap configs here. I assume no. I guess you replied to the question whether each sub device would need own node and compatible. Best Regards Matti Vaittinen
Hi deee Ho Lee, 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@fi.rohmeurope.com> > > > > > --- > > > > > 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). Best Regards Matti Vaittinen
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@fi.rohmeurope.com> > > > > > > --- > > > > > > 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. 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) Best Regards Matti Vaittinen
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@fi.rohmeurope.com> > > > > > > > --- > > > > > > > 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.
Hello Lee, Nice to see you are back :) On Thu, 2021-01-14 at 10:00 +0000, Lee Jones wrote: > 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@fi.rohmeurope.com> > > > > > > > > --- > > > > > > > > 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. My understanding is that this is exactly why we have MFD? To bring all the functions under one multifunctional device and to handle things which are common to all blocks (like IRQs). Besides, like you know (better than me) we don't need additional compatibles or dummy dt-nodes when MFD instantiates the sub-devices. > > 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. Yes. The BD71815 is in my opinion something we can and should handle with same MFD driver as BD71828. The BD9576/BD9573 I'd still go with own MFD (as was in this series). But if you as the maintainer absolutely require me to rework it in some direction (regardless of my reasoning here) - please let me know. I would like to get this in a shape that rest of the drivers can also be accepted in-tree. I think this has been waiting for the decision what to do with the MFD for a while now. > There will either be superflouous files or inflexible code. > > You have to make the right decision for the driver and the subsystem. I think I have explained what I think of but I need you to decide what is best for the subsystem. Best regards Matti Vaittinen
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index 8b99a13669bf..dcb2b14a570e 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -2010,6 +2010,17 @@ config MFD_ROHM_BD71828 Also included is a Coulomb counter, a real-time clock (RTC), and a 32.768 kHz clock gate. +config MFD_ROHM_BD957XMUF + tristate "ROHM BD9576MUF and BD9573MUF Power Management ICs" + depends on I2C=y + depends on OF + select REGMAP_I2C + select MFD_CORE + help + Select this option to get support for the ROHM BD9576MUF and + BD9573MUF Power Management ICs. BD9576 and BD9573 are primarily + designed to be used to power R-Car series processors. + config MFD_STM32_LPTIMER tristate "Support for STM32 Low-Power Timer" depends on (ARCH_STM32 && OF) || COMPILE_TEST diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index 1780019d2474..837f68c9f336 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -261,6 +261,7 @@ obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o obj-$(CONFIG_MFD_ROHM_BD70528) += rohm-bd70528.o obj-$(CONFIG_MFD_ROHM_BD71828) += rohm-bd71828.o obj-$(CONFIG_MFD_ROHM_BD718XX) += rohm-bd718x7.o +obj-$(CONFIG_MFD_ROHM_BD957XMUF) += rohm-bd9576.o obj-$(CONFIG_MFD_STMFX) += stmfx.o obj-$(CONFIG_MFD_KHADAS_MCU) += khadas-mcu.o diff --git a/drivers/mfd/rohm-bd9576.c b/drivers/mfd/rohm-bd9576.c new file mode 100644 index 000000000000..f4dd9e438427 --- /dev/null +++ b/drivers/mfd/rohm-bd9576.c @@ -0,0 +1,108 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +// +// Copyright (C) 2020 ROHM Semiconductors +// +// ROHM BD9576MUF and BD9573MUF PMIC driver + +#include <linux/i2c.h> +#include <linux/interrupt.h> +#include <linux/ioport.h> +#include <linux/irq.h> +#include <linux/mfd/core.h> +#include <linux/mfd/rohm-bd957x.h> +#include <linux/mfd/rohm-generic.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/regmap.h> +#include <linux/types.h> + +static struct mfd_cell bd9573_mfd_cells[] = { + { .name = "bd9573-pmic", }, + { .name = "bd9576-wdt", }, +}; + +static struct mfd_cell bd9576_mfd_cells[] = { + { .name = "bd9576-pmic", }, + { .name = "bd9576-wdt", }, +}; + +static const struct regmap_range volatile_ranges[] = { + regmap_reg_range(BD957X_REG_SMRB_ASSERT, BD957X_REG_SMRB_ASSERT), + regmap_reg_range(BD957X_REG_PMIC_INTERNAL_STAT, + BD957X_REG_PMIC_INTERNAL_STAT), + regmap_reg_range(BD957X_REG_INT_THERM_STAT, BD957X_REG_INT_THERM_STAT), + regmap_reg_range(BD957X_REG_INT_OVP_STAT, BD957X_REG_INT_SYS_STAT), + regmap_reg_range(BD957X_REG_INT_MAIN_STAT, BD957X_REG_INT_MAIN_STAT), +}; + +static const struct regmap_access_table volatile_regs = { + .yes_ranges = &volatile_ranges[0], + .n_yes_ranges = ARRAY_SIZE(volatile_ranges), +}; + +static struct regmap_config bd957x_regmap = { + .reg_bits = 8, + .val_bits = 8, + .volatile_table = &volatile_regs, + .max_register = BD957X_MAX_REGISTER, + .cache_type = REGCACHE_RBTREE, +}; + +static int bd957x_i2c_probe(struct i2c_client *i2c, + const struct i2c_device_id *id) +{ + int ret; + struct regmap *regmap; + struct mfd_cell *mfd; + int cells; + unsigned long chip_type; + + chip_type = (unsigned long)of_device_get_match_data(&i2c->dev); + + switch (chip_type) { + case ROHM_CHIP_TYPE_BD9576: + mfd = bd9576_mfd_cells; + cells = ARRAY_SIZE(bd9576_mfd_cells); + break; + case ROHM_CHIP_TYPE_BD9573: + mfd = bd9573_mfd_cells; + cells = ARRAY_SIZE(bd9573_mfd_cells); + break; + default: + dev_err(&i2c->dev, "Unknown device type"); + return -EINVAL; + } + + regmap = devm_regmap_init_i2c(i2c, &bd957x_regmap); + if (IS_ERR(regmap)) { + dev_err(&i2c->dev, "Failed to initialize Regmap\n"); + return PTR_ERR(regmap); + } + + ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, mfd, cells, + NULL, 0, NULL); + if (ret) + dev_err(&i2c->dev, "Failed to create subdevices\n"); + + return ret; +} + +static const struct of_device_id bd957x_of_match[] = { + { .compatible = "rohm,bd9576", .data = (void *)ROHM_CHIP_TYPE_BD9576, }, + { .compatible = "rohm,bd9573", .data = (void *)ROHM_CHIP_TYPE_BD9573, }, + { }, +}; +MODULE_DEVICE_TABLE(of, bd957x_of_match); + +static struct i2c_driver bd957x_drv = { + .driver = { + .name = "rohm-bd957x", + .of_match_table = bd957x_of_match, + }, + .probe = &bd957x_i2c_probe, +}; +module_i2c_driver(bd957x_drv); + +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>"); +MODULE_DESCRIPTION("ROHM BD9576MUF and BD9573MUF Power Management IC driver"); +MODULE_LICENSE("GPL"); diff --git a/include/linux/mfd/rohm-bd957x.h b/include/linux/mfd/rohm-bd957x.h new file mode 100644 index 000000000000..3e7ca6fe5d4f --- /dev/null +++ b/include/linux/mfd/rohm-bd957x.h @@ -0,0 +1,59 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* Copyright (C) 2020 ROHM Semiconductors */ + +#ifndef __LINUX_MFD_BD957X_H__ +#define __LINUX_MFD_BD957X_H__ + +enum { + BD957X_VD50, + BD957X_VD18, + BD957X_VDDDR, + BD957X_VD10, + BD957X_VOUTL1, + BD957X_VOUTS1, +}; + +#define BD957X_REG_SMRB_ASSERT 0x15 +#define BD957X_REG_PMIC_INTERNAL_STAT 0x20 +#define BD957X_REG_INT_THERM_STAT 0x23 +#define BD957X_REG_INT_THERM_MASK 0x24 +#define BD957X_REG_INT_OVP_STAT 0x25 +#define BD957X_REG_INT_SCP_STAT 0x26 +#define BD957X_REG_INT_OCP_STAT 0x27 +#define BD957X_REG_INT_OVD_STAT 0x28 +#define BD957X_REG_INT_UVD_STAT 0x29 +#define BD957X_REG_INT_UVP_STAT 0x2a +#define BD957X_REG_INT_SYS_STAT 0x2b +#define BD957X_REG_INT_SYS_MASK 0x2c +#define BD957X_REG_INT_MAIN_STAT 0x30 +#define BD957X_REG_INT_MAIN_MASK 0x31 + +#define BD957X_REG_WDT_CONF 0x16 + +#define BD957X_REG_POW_TRIGGER1 0x41 +#define BD957X_REG_POW_TRIGGER2 0x42 +#define BD957X_REG_POW_TRIGGER3 0x43 +#define BD957X_REG_POW_TRIGGER4 0x44 +#define BD957X_REG_POW_TRIGGERL1 0x45 +#define BD957X_REG_POW_TRIGGERS1 0x46 + +#define BD957X_REGULATOR_EN_MASK 0xff +#define BD957X_REGULATOR_DIS_VAL 0xff + +#define BD957X_VSEL_REG_MASK 0xff + +#define BD957X_MASK_VOUT1_TUNE 0x87 +#define BD957X_MASK_VOUT2_TUNE 0x87 +#define BD957X_MASK_VOUT3_TUNE 0x1f +#define BD957X_MASK_VOUT4_TUNE 0x1f +#define BD957X_MASK_VOUTL1_TUNE 0x87 + +#define BD957X_REG_VOUT1_TUNE 0x50 +#define BD957X_REG_VOUT2_TUNE 0x53 +#define BD957X_REG_VOUT3_TUNE 0x56 +#define BD957X_REG_VOUT4_TUNE 0x59 +#define BD957X_REG_VOUTL1_TUNE 0x5c + +#define BD957X_MAX_REGISTER 0x61 + +#endif diff --git a/include/linux/mfd/rohm-generic.h b/include/linux/mfd/rohm-generic.h index 4283b5b33e04..58b4f1a0f4af 100644 --- a/include/linux/mfd/rohm-generic.h +++ b/include/linux/mfd/rohm-generic.h @@ -12,6 +12,8 @@ enum rohm_chip_type { ROHM_CHIP_TYPE_BD71847, ROHM_CHIP_TYPE_BD70528, ROHM_CHIP_TYPE_BD71828, + ROHM_CHIP_TYPE_BD9576, + ROHM_CHIP_TYPE_BD9573, ROHM_CHIP_TYPE_AMOUNT };
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@fi.rohmeurope.com> --- 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