mbox series

[v3,00/10] Support ROHM BD96801 Scalable PMIC

Message ID cover.1717486682.git.mazziesaccount@gmail.com
Headers show
Series Support ROHM BD96801 Scalable PMIC | expand

Message

Matti Vaittinen June 4, 2024, 7:53 a.m. UTC
Support ROHM BD96801 Scalable PMIC

The ROHM BD96801 is automotive grade PMIC, intended to be usable in
multiple solutions. The BD96801 can be used as a stand-alone, or together
with separate 'companion PMICs'. This modular approach aims to make this
PMIC suitable for various use-cases.

This series brings only limited support. The more complete set of
features was sent in the RFC:
https://lore.kernel.org/lkml/cover.1712058690.git.mazziesaccount@gmail.com/

The BD96801 provides two physical IRQ lines called "intb" and "errb" in
the data-sheet. These are handled using own regmap-IRQ controller for
both of the IRQ lines. This causes a debugFS naming conflict for IRQ
domains created by the regmap-IRQ. This series adds support for setting
a name suffix to IRQ domains. Some prior discussion can be seen here:
https://lore.kernel.org/all/Zjzt8mOW6dO_7XNV@finisterre.sirena.org.uk/

As writing of this there is no known system doing configurations which
require the PMIC to be in STBY state using Linux driver. Furthermore,
ensuring the PMIC is and stays in the STBY state when configurations
are done may not be trivial. Especially, not in a generic way in a
regulator driver. This is likely to be system specific.

Hence it felt natural to upstream only partial support for
now, while leaving a note about the RFC series with more complete
support for those who may need it later.

The patches from 1 to 6 are just typical "add support for device X"
stuff. They should provide very much usable driver for BD96801 and I
hope they don't cause too many questions and can be merged when
quality seems high enough :)

Supporting the ERRB IRQ (patches 9 and 10) requires the regmap IRQ change
(patch 8) which further requires the IRQ domain change (patch 7).

Patches 7 and 8 may need more careful thinking. Thus, the ERRB IRQ
support is added as a separate step, which can be merged later or even
dropped if the irqdomain changes prove to be unacceptable.

Revision history still tries to summarize changes from the RFC for the
reviewers.

Revision history:
v2 => v3: Mostly based on feedback from Thomas Gleixner
	- Added acks from Krzysztof and Mark
	- Rebased on v6.10-rc2
	- Drop name suffix support for legacy IRQ domains (both
	  irqdomain and regmap)
	- Improve the commit message for patch 7/10

v1 => v2:
	- Add support for setting a name suffix for fwnode backed IRQ domains.
	- Add support for setting a domain name suffix for regmap-IRQ.
	- Add handling of ERRB IRQs.
	- Small fixes based on feedback.

RFCv2 => v1:
	- Drop ERRB IRQ from drivers (but not DT bindings).
	- Drop configuration which requires STBY - state.
	- Fix the register lock race by moving it from the regulator
	  driver to the MFD driver.

RFCv1 => RFCv2:
	- Tidying code based on feedback form Krzysztof Kozlowski and
	  Lee Jones.
	- Documented undocumented watchdog related DT properties.
	- Added usage of the watchdog IRQ.
	- Use irq_domain_update_bus_token() to work-around debugFS name
	  collision for IRQ domains.

---

Matti Vaittinen (10):
  dt-bindings: ROHM BD96801 PMIC regulators
  dt-bindings: mfd: bd96801 PMIC core
  mfd: support ROHM BD96801 PMIC core
  regulator: bd96801: ROHM BD96801 PMIC regulators
  watchdog: ROHM BD96801 PMIC WDG driver
  MAINTAINERS: Add ROHM BD96801 'scalable PMIC' entries
  irqdomain: Allow giving name suffix for domain
  regmap: Allow setting IRQ domain name suffix
  mfd: bd96801: Add ERRB IRQ
  regulator: bd96801: Add ERRB IRQ

 .../bindings/mfd/rohm,bd96801-pmic.yaml       |  173 +++
 .../regulator/rohm,bd96801-regulator.yaml     |   63 ++
 MAINTAINERS                                   |    4 +
 drivers/base/regmap/regmap-irq.c              |   15 +-
 drivers/mfd/Kconfig                           |   13 +
 drivers/mfd/Makefile                          |    1 +
 drivers/mfd/rohm-bd96801.c                    |  488 ++++++++
 drivers/regulator/Kconfig                     |   12 +
 drivers/regulator/Makefile                    |    2 +
 drivers/regulator/bd96801-regulator.c         | 1008 +++++++++++++++++
 drivers/watchdog/Kconfig                      |   13 +
 drivers/watchdog/Makefile                     |    1 +
 drivers/watchdog/bd96801_wdt.c                |  416 +++++++
 include/linux/irqdomain.h                     |   22 +-
 include/linux/mfd/rohm-bd96801.h              |  215 ++++
 include/linux/mfd/rohm-generic.h              |    1 +
 include/linux/regmap.h                        |    4 +
 kernel/irq/irqdomain.c                        |   37 +-
 18 files changed, 2470 insertions(+), 18 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/rohm,bd96801-pmic.yaml
 create mode 100644 Documentation/devicetree/bindings/regulator/rohm,bd96801-regulator.yaml
 create mode 100644 drivers/mfd/rohm-bd96801.c
 create mode 100644 drivers/regulator/bd96801-regulator.c
 create mode 100644 drivers/watchdog/bd96801_wdt.c
 create mode 100644 include/linux/mfd/rohm-bd96801.h


base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0

Comments

Thomas Gleixner June 6, 2024, 6:59 p.m. UTC | #1
Matti!

On Tue, Jun 04 2024 at 10:55, Matti Vaittinen wrote:
>  struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode, unsigned int size,
>  				    irq_hw_number_t hwirq_max, int direct_max,
>  				    const struct irq_domain_ops *ops,
> -				    void *host_data);
> +				    void *host_data, const char *name_suffix);
>  struct irq_domain *irq_domain_create_simple(struct fwnode_handle *fwnode,
>  					    unsigned int size,
>  					    unsigned int first_irq,
> @@ -350,7 +350,8 @@ static inline struct irq_domain *irq_domain_add_linear(struct device_node *of_no
>  					 const struct irq_domain_ops *ops,
>  					 void *host_data)
>  {
> -	return __irq_domain_add(of_node_to_fwnode(of_node), size, size, 0, ops, host_data);
> +	return __irq_domain_add(of_node_to_fwnode(of_node), size, size, 0, ops,
> +				host_data, NULL);

....

Looking at the resulting amount of churn to add that argument, I'm not
really enthused. There is some other unrelated change required in this
area:

  https://lore.kernel.org/all/8734pr5yq1.ffs@tglx

My suggestion to convert all of this mess into a template based
mechanism would nicely solve your problem too.

Can you please have a look and eventually team up with Herve (CC'ed) to
sort this out? I'm happy to help and give guidance.

Thanks,

        tglx
Matti Vaittinen June 7, 2024, 6:38 a.m. UTC | #2
Hello Thomas, Herve.

On 6/6/24 21:59, Thomas Gleixner wrote:
> Matti!
> 
> On Tue, Jun 04 2024 at 10:55, Matti Vaittinen wrote:
>>   struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode, unsigned int size,
>>   				    irq_hw_number_t hwirq_max, int direct_max,
>>   				    const struct irq_domain_ops *ops,
>> -				    void *host_data);
>> +				    void *host_data, const char *name_suffix);
>>   struct irq_domain *irq_domain_create_simple(struct fwnode_handle *fwnode,
>>   					    unsigned int size,
>>   					    unsigned int first_irq,
>> @@ -350,7 +350,8 @@ static inline struct irq_domain *irq_domain_add_linear(struct device_node *of_no
>>   					 const struct irq_domain_ops *ops,
>>   					 void *host_data)
>>   {
>> -	return __irq_domain_add(of_node_to_fwnode(of_node), size, size, 0, ops, host_data);
>> +	return __irq_domain_add(of_node_to_fwnode(of_node), size, size, 0, ops,
>> +				host_data, NULL);
> 
> ....
> 
> Looking at the resulting amount of churn to add that argument, I'm not
> really enthused. There is some other unrelated change required in this
> area:
> 
>    https://lore.kernel.org/all/8734pr5yq1.ffs@tglx
> 
> My suggestion to convert all of this mess into a template based
> mechanism would nicely solve your problem too.

I am not entirely sure what you mean by template based in this context. 
My brains are somehow fixed to start thinking of C++ templates, or C 
macro magic with typeof() and I just can't get past that.

Anyways, what I picked from discussion between you and Herve, is using 
an initialization structure (struct irq_domain_info) for the new domain 
creation function (irq_domain_instantiate()) instead of adding bunch of 
functions with quite a few separate arguments. So, I assume you're 
referring to a possibility to add the name-suffix in this initialization 
structure? I hope I got this right.

I assume there is no intention to change the existing public 
irq_domain_creat_foo() APIs to use the new irq_domain_info - and change 
all the callers(?) But I think changing the internal 
__irq_domain_create() to use this new info struct should be very much 
doable - although, in my opinion, making existing callers of the 
__irq_domain_create() to assign their parameters to this struct so they 
can pass it to __irq_domain_create() does not seem so nice.

So, even though I am not really happy about the delay (I secretly hoped 
to get the series merged before my summer vacations ;) ) - I admit your 
suggested change looks cleaner (again, at least to me).

Herve, do you have any idea when you plan to do further sketching on 
this? Do you want me to try seeing if I can add the struct 
irq_domain_info and maybe use that in the __irq_domain_add() to get the 
name-suffix added? I might be able to send one version out during next 
week - but then I plan to be offline for couple of weeks ... so it may 
be I am not much of a help here.

> Can you please have a look and eventually team up with Herve (CC'ed) to
> sort this out? I'm happy to help and give guidance.

I appreciate the guidance! Thanks Thomas.

Yours,
	-- Matti
Herve Codina June 7, 2024, 8:13 a.m. UTC | #3
Hi Matti,

On Fri, 7 Jun 2024 09:38:31 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

...

> Herve, do you have any idea when you plan to do further sketching on 
> this? Do you want me to try seeing if I can add the struct 
> irq_domain_info and maybe use that in the __irq_domain_add() to get the 
> name-suffix added? I might be able to send one version out during next 
> week - but then I plan to be offline for couple of weeks ... so it may 
> be I am not much of a help here.
> 

On my side, I plan to work on it next week too.
If you are off a couple of weeks after, I think I can start and move forward
on this topic.

Best regards,
Hervé
Matti Vaittinen June 7, 2024, 8:49 a.m. UTC | #4
On 6/7/24 11:13, Herve Codina wrote:
> Hi Matti,
> 
> On Fri, 7 Jun 2024 09:38:31 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
> ...
> 
>> Herve, do you have any idea when you plan to do further sketching on
>> this? Do you want me to try seeing if I can add the struct
>> irq_domain_info and maybe use that in the __irq_domain_add() to get the
>> name-suffix added? I might be able to send one version out during next
>> week - but then I plan to be offline for couple of weeks ... so it may
>> be I am not much of a help here.
>>
> 
> On my side, I plan to work on it next week too.
> If you are off a couple of weeks after, I think I can start and move forward
> on this topic.

Thanks for the prompt reply and thanks for working with this :) I'll 
leave it to you for now then, as I don't think it makes much sense to 
intentionally do conflicting changes. I'll see what you've been up to 
when I return to the keyboard :) I'd appreciated if you added me to CC 
when sending the irqdomain refactoring patches (but I can dig them up if 
you don't).

Have fun!

Yours,
	-- Matti
Herve Codina June 7, 2024, 9:23 a.m. UTC | #5
On Fri, 7 Jun 2024 11:49:07 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 6/7/24 11:13, Herve Codina wrote:
> > Hi Matti,
> > 
> > On Fri, 7 Jun 2024 09:38:31 +0300
> > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> > 
> > ...
> >   
> >> Herve, do you have any idea when you plan to do further sketching on
> >> this? Do you want me to try seeing if I can add the struct
> >> irq_domain_info and maybe use that in the __irq_domain_add() to get the
> >> name-suffix added? I might be able to send one version out during next
> >> week - but then I plan to be offline for couple of weeks ... so it may
> >> be I am not much of a help here.
> >>  
> > 
> > On my side, I plan to work on it next week too.
> > If you are off a couple of weeks after, I think I can start and move forward
> > on this topic.  
> 
> Thanks for the prompt reply and thanks for working with this :) I'll 
> leave it to you for now then, as I don't think it makes much sense to 
> intentionally do conflicting changes. I'll see what you've been up to 
> when I return to the keyboard :) I'd appreciated if you added me to CC 
> when sending the irqdomain refactoring patches (but I can dig them up if 
> you don't).

Sure, you will CC you.

Also, I am not sure that I will perfectly take into account your use-case
but it should not be a big deal to add it on top of my commits afterwards.

> 
> Have fun!
>

Cheers,
Hervé
Matti Vaittinen June 7, 2024, 9:25 a.m. UTC | #6
On 6/7/24 12:23, Herve Codina wrote:
> On Fri, 7 Jun 2024 11:49:07 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> On 6/7/24 11:13, Herve Codina wrote:
>>> Hi Matti,
>>>
>>> On Fri, 7 Jun 2024 09:38:31 +0300
>>> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>>>
>>> ...
>>>    
>>>> Herve, do you have any idea when you plan to do further sketching on
>>>> this? Do you want me to try seeing if I can add the struct
>>>> irq_domain_info and maybe use that in the __irq_domain_add() to get the
>>>> name-suffix added? I might be able to send one version out during next
>>>> week - but then I plan to be offline for couple of weeks ... so it may
>>>> be I am not much of a help here.
>>>>   
>>>
>>> On my side, I plan to work on it next week too.
>>> If you are off a couple of weeks after, I think I can start and move forward
>>> on this topic.
>>
>> Thanks for the prompt reply and thanks for working with this :) I'll
>> leave it to you for now then, as I don't think it makes much sense to
>> intentionally do conflicting changes. I'll see what you've been up to
>> when I return to the keyboard :) I'd appreciated if you added me to CC
>> when sending the irqdomain refactoring patches (but I can dig them up if
>> you don't).
> 
> Sure, you will CC you.

Thanks!

> Also, I am not sure that I will perfectly take into account your use-case
> but it should not be a big deal to add it on top of my commits afterwards.

No problem! That's just fine :)