Re: [RFC PATCH 3/6] mfd: support ROHM BD96801 PMIC core

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi deee Ho Lee!

Thanks a ton for taking a look at this :) I already sent the V2 yesterday, briefly before receiving your comments. I think all of the comments are relevant for the V2 as well, I will fix them for the V3 when I get to that. If you find the time to take a look at V2, then the major things are addition of a watchdog IRQ + a work-around for the debugFS name collision for IRQ domains.

On 4/11/24 17:38, Lee Jones wrote:
On Tue, 02 Apr 2024, Matti Vaittinen wrote:

The ROHM BD96801 PMIC is highly customizable automotive grade PMIC
which integrates regulator and watchdog funtionalities.

Provide IRQ and register accesses for regulator/watchdog drivers.

Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
---
  drivers/mfd/Kconfig              |  13 +
  drivers/mfd/Makefile             |   1 +
  drivers/mfd/rohm-bd96801.c       | 454 +++++++++++++++++++++++++++++++
  include/linux/mfd/rohm-bd96801.h | 212 +++++++++++++++
  include/linux/mfd/rohm-generic.h |   1 +
  5 files changed, 681 insertions(+)
  create mode 100644 drivers/mfd/rohm-bd96801.c
  create mode 100644 include/linux/mfd/rohm-bd96801.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 4b023ee229cf..947045eb3a8e 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -2089,6 +2089,19 @@ config MFD_ROHM_BD957XMUF
  	  BD9573MUF Power Management ICs. BD9576 and BD9573 are primarily
  	  designed to be used to power R-Car series processors.
+config MFD_ROHM_BD96801
+	tristate "ROHM BD96801 Power Management IC"
+	depends on I2C=y
+	depends on OF
+	select REGMAP_I2C
+	select REGMAP_IRQ
+	select MFD_CORE
+	help
+	  Select this option to get support for the ROHM BD96801 Power
+	  Management IC. The ROHM BD96801 is a highly scalable power management

Power Management

Out of the curiosity, why is the "Power Management IC" written with capitals, when speaking of a class of devices instead of a model? (I am 100% fine with the change, just curious).


+	  IC for industrial and automotive use. The BD96801 can be used as a
+	  master PMIC in a chained PMIC solutions with suitable companion PMICs
...

+// SPDX-License-Identifier: GPL-2.0-only
+//
+// Copyright (C) 2022 ROHM Semiconductors

No updates for 2 years?

The year should be updated - thanks. But, now that you asked... Almost no updates. The patches have rotten in my outbox, waiting for the permisson to be sent out... But yeah, I've sure added some changes before sending the series - I'll update the copyright :)

+
+static int bd96801_i2c_probe(struct i2c_client *i2c)
+{
+	int i, ret, intb_irq, errb_irq, num_regu_irqs, num_intb, num_errb = 0;
+	struct regmap_irq_chip_data *intb_irq_data, *errb_irq_data;
+	struct irq_domain *intb_domain, *errb_domain;
+	const struct fwnode_handle *fwnode;
+	struct resource *regulator_res;
+	struct regmap *regmap;
+
+	fwnode = dev_fwnode(&i2c->dev);
+	if (!fwnode) {
+		dev_err(&i2c->dev, "no fwnode\n");
+		return -EINVAL;

Why not dev_err_probe() here for uniformity?

I can change it to dev_err_probe() if it's strongly preferred. It just feels silly to use dev_err_probe() when the return value is hardcoded. Intentionally writing code like

err = -EINVAL;
if (err == ...)

just makes me feel a bit sick.

+	}
+
+	intb_irq = fwnode_irq_get_byname(fwnode, "intb");
+	if (intb_irq < 0)
+		return dev_err_probe(&i2c->dev, intb_irq,
+				     "No INTB IRQ configured\n");

This function would look nicer if you expanded to 100-chars.

The reason why I still prefer the good old 80-chars for files I work with, is that I am often having 3 terminal windows parallel on my laptop screen. (Or, when I have my wide mofnitor connected it is 3 editor windows + minicom). I need to keep the terminals small enough. Besides... I hate to admit this, but the time is finally taking it's toll. My eyes aren't quite the same they were 2 years ago...

So, same old story, I can change this if it is important enough for others, but personally I rather work with the short lines.

...

diff --git a/include/linux/mfd/rohm-bd96801.h b/include/linux/mfd/rohm-bd96801.h
new file mode 100644
index 000000000000..47b07171dcb2
--- /dev/null
+++ b/include/linux/mfd/rohm-bd96801.h
@@ -0,0 +1,212 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/* Copyright (C) 2020 ROHM Semiconductors */
+

...

+/* IRQ register area */
+#define BD96801_REG_INT_MAIN		0x51
+
+/*
+ * The BD96801 has two physical IRQ lines, INTB and ERRB.
+ * For now we just handle the INTB.

Note to self, this comment is no longer true.

Thanks for the review!

Yours,
	-- Matti


--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux