Re: [PATCH 1/2] mfd: bd96801: Add ERRB IRQ

[Matti Vaittinen <mazziesaccount@gmail.com>]

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@gmail.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! ~~