[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <71414d3b-dc0c-4b33-a4df-16ef9cea1380@gmail.com>
Date: Wed, 4 Sep 2024 08:07:44 +0300
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Lee Jones <lee@...nel.org>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] mfd: bd96801: Add ERRB IRQ
On 9/3/24 17:37, Lee Jones wrote:
> On Fri, 30 Aug 2024, Matti Vaittinen wrote:
>
>> On 8/30/24 10:28, Lee Jones wrote:
>>> On Mon, 26 Aug 2024, Matti Vaittinen wrote:
>>>
>>>> The ROHM BD96801 "scalable PMIC" provides two physical IRQs. The ERRB
>>>> handling can in many cases be omitted because it is used to inform fatal
>>>> IRQs, which usually kill the power from the SOC.
>>>>
>>>> There may however be use-cases where the SOC has a 'back-up' emergency
>>>> power source which allows some very short time of operation to try to
>>>> gracefully shut down sensitive hardware. Furthermore, it is possible the
>>>> processor controlling the PMIC is not powered by the PMIC. In such cases
>>>> handling the ERRB IRQs may be beneficial.
>>>>
>>>> Add support for ERRB IRQs.
>>
>> Thanks for the review Lee! :)
>>
>>>>
>>>> Signed-off-by: Matti Vaittinen <mazziesaccount@...il.com>
>>>> ---
>>>> Revision history:
>>>> New series (only ERRB addition)
>>>> v1:
>>>> - use devm allocation for regulator_res
>>>> - use goto skip_errb instead of an if (errb)
>>>> - constify immutable structs
>>>>
>>>> Old series (All BD96801 functionality + irqdomain and regmap changes)
>>>> v2 => v3:
>>>> - No changes
>>>> v1 => v2:
>>>> - New patch
>>>>
>>>> mfd: constify structs
>>>> ---
>>>> static int bd96801_i2c_probe(struct i2c_client *i2c)
>>>> {
>>>> - struct regmap_irq_chip_data *intb_irq_data;
>>>> + int i, ret, intb_irq, errb_irq, num_regu_irqs, num_intb, num_errb = 0;
>>>> + int wdg_irq_no;
>>>
>>> Nit: Not sure why the smaller data elements have been placed at the top.
>>
>> Because some people have told me it's easier for them to read the local
>> variable declarations when the code is formatted to "reverse xmas-tree"
>> -style. I suppose I've tried to follow that here.
>>
>>> They were better down where they were.
>>
>> My old personal preference has just been to have 'simple' integer types
>> first, then structs, and the pointers last. I don't think having xmas-tree
>> (reversed or not) plays a role in my code-reading ability...
>>
>> I won't re-spin the series just for this, if this is just a 'nit'. I will
>> try to remember the comment if I need to rebase / respin this later though
>> :)
>
> Please leave them were they are.
I suppose this is a request to leave them where they are in-tree code
now - not to leave them where they are in this patch :) I'll re-spin
this anyways for the print below. So, I might as well revert the local
variable formatting here :)
>>>> + struct regmap_irq_chip_data *intb_irq_data, *errb_irq_data;
>>>> + struct irq_domain *intb_domain, *errb_domain;
>>>> + struct resource wdg_irq;
>>>> const struct fwnode_handle *fwnode;
>>>> - struct irq_domain *intb_domain;
>>>> + struct resource *regulator_res;
>>>> struct regmap *regmap;
>>>> - int ret, intb_irq;
>>>> fwnode = dev_fwnode(&i2c->dev);
>>>> if (!fwnode)
>>>> @@ -213,6 +364,23 @@ static int bd96801_i2c_probe(struct i2c_client *i2c)
>>>> if (intb_irq < 0)
>>>> return dev_err_probe(&i2c->dev, intb_irq, "INTB IRQ not configured\n");
>>>> + num_intb = ARRAY_SIZE(regulator_intb_irqs);
>>>> +
>>>> + /* ERRB may be omitted if processor is powered by the PMIC */
>>>> + errb_irq = fwnode_irq_get_byname(fwnode, "errb");
>>>> + if (errb_irq < 0)
>>>> + errb_irq = 0;
>>>> +
>>>> + if (errb_irq)
>>>> + num_errb = ARRAY_SIZE(regulator_errb_irqs);
>>>> +
>>>> + num_regu_irqs = num_intb + num_errb;
>>>> +
>>>> + regulator_res = devm_kcalloc(&i2c->dev, num_regu_irqs,
>>>> + sizeof(*regulator_res), GFP_KERNEL);
>>>> + if (!regulator_res)
>>>> + return -ENOMEM;
>>>> +
>>>> regmap = devm_regmap_init_i2c(i2c, &bd96801_regmap_config);
>>>> if (IS_ERR(regmap))
>>>> return dev_err_probe(&i2c->dev, PTR_ERR(regmap),
>>>> @@ -226,16 +394,54 @@ static int bd96801_i2c_probe(struct i2c_client *i2c)
>>>> IRQF_ONESHOT, 0, &bd96801_irq_chip_intb,
>>>> &intb_irq_data);
>>>> if (ret)
>>>> - return dev_err_probe(&i2c->dev, ret, "Failed to add INTB IRQ chip\n");
>>>> + return dev_err_probe(&i2c->dev, ret, "Failed to add INTB irq_chip\n");
>
> Kernel logs are user facing. The previous message was better.
>
> Please drop this change.
Good catch. This has been an unintended change. Not sure where this
originated from.
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