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

[Matti Vaittinen <mazziesaccount@gmail.com>]

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 :)

>> +	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");
>>   
>>   	intb_domain = regmap_irq_get_domain(intb_irq_data);
>>   
>> -	ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO,
>> -				   bd96801_cells,
>> -				   ARRAY_SIZE(bd96801_cells), NULL, 0,
>> -				   intb_domain);
>> +	/*
>> +	 * MFD core code is built to handle only one IRQ domain. BD96801
>> +	 * has two domains so we do IRQ mapping here and provide the
>> +	 * already mapped IRQ numbers to sub-devices.
>> +	 */
> 
> Do one or more of the subdevices consume both domains?

I believe the regulators consume both.

> If not, why not call devm_mfd_add_devices() twice?

Thanks for this suggestion :) It didn't occur to me I could do that. 
Well, here I need both domains for regulators so it probably wouldn't 
work - but maybe I will remember this is a viable option for future 
designs! Thanks!

> 
>> +	for (i = 0; i < num_intb; i++) {
>> +		struct resource *res = &regulator_res[i];
>> +
>> +		*res = regulator_intb_irqs[i];
>> +		res->start = res->end = irq_create_mapping(intb_domain,
>> +							    res->start);
>> +	}
>> +
>> +	wdg_irq_no = irq_create_mapping(intb_domain, BD96801_WDT_ERR_STAT);
>> +	wdg_irq = DEFINE_RES_IRQ_NAMED(wdg_irq_no, "bd96801-wdg");
>> +	bd96801_cells[WDG_CELL].resources = &wdg_irq;
>> +	bd96801_cells[WDG_CELL].num_resources = 1;
>> +
>> +	if (!num_errb)
>> +		goto skip_errb;
>> +
>> +	ret = devm_regmap_add_irq_chip(&i2c->dev, regmap, errb_irq, IRQF_ONESHOT,
>> +				       0, &bd96801_irq_chip_errb, &errb_irq_data);
>> +	if (ret)
>> +		return dev_err_probe(&i2c->dev, ret,
>> +				     "Failed to add ERRB (%d) irq_chip\n", errb_irq);
>> +
>> +	errb_domain = regmap_irq_get_domain(errb_irq_data);
>> +
>> +	for (i = 0; i < num_errb; i++) {
>> +		struct resource *res = &regulator_res[num_intb + i];
>> +
>> +		*res = regulator_errb_irqs[i];
>> +		res->start = res->end = irq_create_mapping(errb_domain, res->start);
>> +	}
>> +
>> +skip_errb:
>> +	bd96801_cells[REGULATOR_CELL].resources = regulator_res;
>> +	bd96801_cells[REGULATOR_CELL].num_resources = num_regu_irqs;
>> +
>> +	ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, bd96801_cells,
>> +				   ARRAY_SIZE(bd96801_cells), NULL, 0, NULL);
>>   	if (ret)
>> -		dev_err(&i2c->dev, "Failed to create subdevices\n");
>> +		dev_err_probe(&i2c->dev, ret, "Failed to create subdevices\n");
>>   
>>   	return ret;
>>   }

Yours,
	-- Matti