Re: [RFC PATCH 06/13] mfd: bd71828: Support ROHM BD72720

[Matti Vaittinen <mazziesaccount@gmail.com>]

Hi deee Ho Lee,

And Thanks for the review!

On 09/10/2025 19:18, Lee Jones wrote:
> On Tue, 07 Oct 2025, Matti Vaittinen wrote:
> 
>> The ROHM BD72720 is a power management IC which continues the BD71828
>> family of PMICs. Similarly to the BD71815 and BD71828, the BD72720
>> integrates regulators, charger, RTC, clock gate and GPIOs.
>>
>> The main difference to the earlier PMICs is that the BD72720 has two
>> different I2C slave addresses. In addition to the registers behind the
>> 'main I2C address', most of the charger (and to some extent LED) control
>> is done via registers behind a 'secondary I2C slave address', 0x4c.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>
>> ---
>> Note: This patch depends on the series: "power: supply: add charger for
>> BD71828" by Andreas:
>> https://lore.kernel.org/all/20250918-bd71828-charger-v5-0-851164839c28@kemnade.info/
>>
>> There are some new variants being planned. Most notably, the BD73900
>> should be almost identical to the BD72720 - for everything else except
>> the charger block.
>> ---

// snip

>> +
>> +static struct regmap *bd72720_secondary_regmap;
> 
> Dynamically allocate this and add it to .platform_data once it's
> populated.
> 

This can be done but I suppose it's unnecessary churn. This driver does 
not (at the moment) support more than one instance of the PMIC anyways. 
(The button data is not alloacted).

This is not really a problem as typically there is only 1 of these PMICs 
to be controlled.

// snip

>>   
>> +/*
>> + * The BD72720 is an odd beast in that it contains two separate sets of
>> + * registers, both starting from address 0x0. The twist is that these "pages"
>> + * are behind different I2C slave addresses. Most of the registers are behind
>> + * a slave address 0x4b, which will be used as the "main" address for this
>> + * device.
>> + * Most of the charger related registers are located behind slave address 0x4c.
>> + * It is tempting to push the dealing with the charger registers and the extra
>> + * 0x4c device in power-supply driver - but perhaps it's better for the sake of
>> + * the cleaner re-use to deal with setting up all of the regmaps here.
>> + * Furthermore, the LED stuff may need access to both of these devices.
>> + */
>> +#define BD72720_SECONDARY_I2C_SLAVE 0x4c
>> +static const struct regmap_range bd72720_volatile_ranges_4b[] = {
>> +	{
>> +		/* RESETSRC1 and 2 are write '1' to clear */
>> +		.range_min = BD72720_REG_RESETSRC_1,
>> +		.range_max = BD72720_REG_RESETSRC_2,
> 
> regmap_reg_range()?

Ah, thanks. Out of the curiosity - do you know why this macro is written 
on lowercase?

// snip
>> +static int bd72720_set_type_config(unsigned int **buf, unsigned int type,
>> +				   const struct regmap_irq *irq_data,
>> +				   int idx, void *irq_drv_data)
>> +{
>> +	const struct regmap_irq_type *t = &irq_data->type;
>> +
>> +	/*
>> +	 * The regmap IRQ ecpects IRQ_TYPE_EDGE_BOTH to be written to register
>> +	 * as logical OR of the type_falling_val and type_rising_val. This is
>> +	 * not how the BD72720 implements this configuration, hence we need
>> +	 * to handle this specific case separately.
>> +	 */
>> +	if (type == IRQ_TYPE_EDGE_BOTH) {
>> +		buf[0][idx] &= ~t->type_reg_mask;
>> +		buf[0][idx] |= BD72720_GPIO_IRQ_TYPE_BOTH;
>> +
>> +		return 0;
>> +	}
>> +
>> +	return regmap_irq_set_type_config_simple(buf, type, irq_data, idx,
>> +						 irq_drv_data);
> 
> Use 100-chars to avoid these pointless wraps please.

gnarl. I think we have discussed this before :)
I would love to keep the lines short - closer to 80 chars - because that 
way I can fit 3 terminals on my screen. All the years spent staring at 
the monitor are taking their toll, and my vision isn't as good as it 
used to be. Frightening thing being that it seems I will only need to 
increase the font in the future :/

Well, sure the lines can be split if you feel strongly about it - but I 
have a real reason (other than the usual - "they have always been like 
that") to try keep them short...

>> +}
>> +
>>   static const struct regmap_irq_chip bd71828_irq_chip = {
>>   	.name = "bd71828_irq",
>>   	.main_status = BD71828_REG_INT_MAIN,
>> @@ -465,6 +814,28 @@ static const struct regmap_irq_chip bd71815_irq_chip = {
>>   	.irq_reg_stride = 1,
>>   };
>>   
>> +static const unsigned int bd72720_irq_type_base = BD72720_REG_GPIO1_CTRL;
> 
> This makes it look like a global variable, which I am allergic to.
> 
> Perhaps make it clear that this is a single element static array instead.

Ok. Just a comment will do?

>> +static const struct regmap_irq_chip bd72720_irq_chip = {
>> +	.name = "bd72720_irq",
>> +	.main_status = BD72720_REG_INT_LVL1_STAT,
>> +	.irqs = &bd72720_irqs[0],
>> +	.num_irqs = ARRAY_SIZE(bd72720_irqs),
>> +	.status_base = BD72720_REG_INT_PS1_STAT,
>> +	.unmask_base = BD72720_REG_INT_PS1_EN,
>> +	.config_base = &bd72720_irq_type_base,
>> +	.num_config_bases = 1,
>> +	.num_config_regs = 2,
>> +	.set_type_config = bd72720_set_type_config,
>> +	.ack_base = BD72720_REG_INT_PS1_STAT,
>> +	.init_ack_masked = true,
>> +	.num_regs = 12,
>> +	.num_main_regs = 1,
>> +	.sub_reg_offsets = &bd72720_sub_irq_offsets[0],
>> +	.num_main_status_bits = 8,
>> +	.irq_reg_stride = 1,
>> +};
>> +
>>   static int set_clk_mode(struct device *dev, struct regmap *regmap,
>>   			int clkmode_reg)
>>   {
>> @@ -511,6 +882,25 @@ static void bd71828_remove_poweroff(void *data)
>>   	pm_power_off = NULL;
>>   }
>>   
>> +static int bd72720_get_secondary_regmap(struct i2c_client *i2c,
> 
> Does this 'secondary' have a specific purpose or a better name?

I am not entirely sure. When I asked this from the designers they just 
told me that they needed more than 255 registers so they added another 
slave address... (I'm not sure what would have been wrong with using a 
page register). So, I assume they just placed stuff that didn't fit in 
first 255 register there. But yeah, it looks like most of the registers 
there are related to the charger. So, perhaps it isn't completely 
misleading to use "charger regmap"? The data-sheet seems to be just 
using "Register map 1" and "Register map 2" in the tables listing these 
registers. I kind of like using something which maps easily to the 
data-sheet, but I really have no strong opinion on this.

>> +					const struct mfd_cell *mfd, int cells)
>> +{

// snip

>> diff --git a/include/linux/mfd/rohm-bd72720.h b/include/linux/mfd/rohm-bd72720.h
>> new file mode 100644
>> index 000000000000..856a6962b1b2
>> --- /dev/null
>> +++ b/include/linux/mfd/rohm-bd72720.h
>> @@ -0,0 +1,632 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright 2024 ROHM Semiconductors.
> 
> Seems odd to introduce a new file with an old date.

I originally wrote this last year :) I can it update though. Thanks.

> 
>> + *
>> + * Author: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
>> + */
>> +
>> +#ifndef _MFD_BD72720_H
>> +#define _MFD_BD72720_H
>> +
>> +#include <linux/regmap.h>
>> +
>> +enum {
>> +	BD72720_BUCK1,
>> +	BD72720_BUCK2,
>> +	BD72720_BUCK3,
>> +	BD72720_BUCK4,
>> +	BD72720_BUCK5,
>> +	BD72720_BUCK6,
>> +	BD72720_BUCK7,
>> +	BD72720_BUCK8,
>> +	BD72720_BUCK9,
>> +	BD72720_BUCK10,
>> +	BD72720_BUCK11,
>> +	BD72720_LDO1,
>> +	BD72720_LDO2,
>> +	BD72720_LDO3,
>> +	BD72720_LDO4,
>> +	BD72720_LDO5,
>> +	BD72720_LDO6,
>> +	BD72720_LDO7,
>> +	BD72720_LDO8,
>> +	BD72720_LDO9,
>> +	BD72720_LDO10,
>> +	BD72720_LDO11,
>> +	BD72720_REGULATOR_AMOUNT,
>> +};
>> +
>> +/* BD72720 interrupts */
>> +#define BD72720_INT_LONGPUSH_MASK BIT(0)
> 
> Tab out the values please.

Ok, sure.

Ps.
I do really appreciate you going through RFCs :) Kudos!

Yours,
	-- Matti