| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <25c959bc-fb02-42d9-b973-4a74cebd7208@gmail.com>
Date: Fri, 12 Apr 2024 08:40:25 +0300
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Lee Jones <lee@...nel.org>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>, Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>, Wim Van Sebroeck <wim@...ux-watchdog.org>,
Guenter Roeck <linux@...ck-us.net>, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-watchdog@...r.kernel.org
Subject: Re: [RFC PATCH 3/6] mfd: support ROHM BD96801 PMIC core
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@...il.com>
>> ---
>> 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! ~~
Powered by blists - more mailing lists