Prepare for adding support for BD96802 which is very similar to BD96801.
Separate chip specific data into own structure which can be picked to be
used by the device-tree.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 drivers/mfd/rohm-bd96801.c | 83 ++++++++++++++++++++++++++------------
 1 file changed, 57 insertions(+), 26 deletions(-)

diff --git a/drivers/mfd/rohm-bd96801.c b/drivers/mfd/rohm-bd96801.c
index 60ec8db790a7..1232f571e4b1 100644
--- a/drivers/mfd/rohm-bd96801.c
+++ b/drivers/mfd/rohm-bd96801.c
@@ -40,7 +40,21 @@
 #include <linux/mfd/rohm-bd96801.h>
 #include <linux/mfd/rohm-generic.h>
 
-static const struct resource regulator_errb_irqs[] = {
+struct bd968xx_chip_data {
+	const struct resource *errb_irqs;
+	const struct resource *intb_irqs;
+	int num_errb_irqs;
+	int num_intb_irqs;
+	const struct regmap_irq_chip *errb_irq_chip;
+	const struct regmap_irq_chip *intb_irq_chip;
+	const struct regmap_config *regmap_config;
+	struct mfd_cell *cells;
+	int num_cells;
+	int unlock_reg;
+	int unlock_val;
+};
+
+static const struct resource bd96801_reg_errb_irqs[] = {
 	DEFINE_RES_IRQ_NAMED(BD96801_OTP_ERR_STAT, "bd96801-otp-err"),
 	DEFINE_RES_IRQ_NAMED(BD96801_DBIST_ERR_STAT, "bd96801-dbist-err"),
 	DEFINE_RES_IRQ_NAMED(BD96801_EEP_ERR_STAT, "bd96801-eep-err"),
@@ -98,7 +112,7 @@ static const struct resource regulator_errb_irqs[] = {
 	DEFINE_RES_IRQ_NAMED(BD96801_LDO7_SHDN_ERR_STAT, "bd96801-ldo7-shdn-err"),
 };
 
-static const struct resource regulator_intb_irqs[] = {
+static const struct resource bd96801_reg_intb_irqs[] = {
 	DEFINE_RES_IRQ_NAMED(BD96801_TW_STAT, "bd96801-core-thermal"),
 
 	DEFINE_RES_IRQ_NAMED(BD96801_BUCK1_OCPH_STAT, "bd96801-buck1-overcurr-h"),
@@ -345,18 +359,37 @@ static const struct regmap_config bd96801_regmap_config = {
 	.cache_type = REGCACHE_MAPLE,
 };
 
+static const struct bd968xx_chip_data bd96801_chip_data = {
+	.errb_irqs = bd96801_reg_errb_irqs,
+	.intb_irqs = bd96801_reg_intb_irqs,
+	.num_errb_irqs = ARRAY_SIZE(bd96801_reg_errb_irqs),
+	.num_intb_irqs = ARRAY_SIZE(bd96801_reg_intb_irqs),
+	.errb_irq_chip = &bd96801_irq_chip_errb,
+	.intb_irq_chip = &bd96801_irq_chip_intb,
+	.regmap_config = &bd96801_regmap_config,
+	.cells = bd96801_cells,
+	.num_cells = ARRAY_SIZE(bd96801_cells),
+	.unlock_reg = BD96801_LOCK_REG,
+	.unlock_val = BD96801_UNLOCK,
+};
+
 static int bd96801_i2c_probe(struct i2c_client *i2c)
 {
 	struct regmap_irq_chip_data *intb_irq_data, *errb_irq_data;
 	struct irq_domain *intb_domain, *errb_domain;
+	static const struct bd968xx_chip_data *cd;
 	const struct fwnode_handle *fwnode;
 	struct resource *regulator_res;
 	struct resource wdg_irq;
 	struct regmap *regmap;
-	int intb_irq, errb_irq, num_intb, num_errb = 0;
+	int intb_irq, errb_irq, num_errb = 0;
 	int num_regu_irqs, wdg_irq_no;
 	int i, ret;
 
+	cd = device_get_match_data(&i2c->dev);
+	if (!cd)
+		return -ENODEV;
+
 	fwnode = dev_fwnode(&i2c->dev);
 	if (!fwnode)
 		return dev_err_probe(&i2c->dev, -EINVAL, "Failed to find fwnode\n");
@@ -365,34 +398,32 @@ 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 == -EPROBE_DEFER)
+		return errb_irq;
 
-	if (errb_irq)
-		num_errb = ARRAY_SIZE(regulator_errb_irqs);
+	if (errb_irq > 0)
+		num_errb = cd->num_errb_irqs;
 
-	num_regu_irqs = num_intb + num_errb;
+	num_regu_irqs = cd->num_intb_irqs + 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);
+	regmap = devm_regmap_init_i2c(i2c, cd->regmap_config);
 	if (IS_ERR(regmap))
 		return dev_err_probe(&i2c->dev, PTR_ERR(regmap),
 				    "Regmap initialization failed\n");
 
-	ret = regmap_write(regmap, BD96801_LOCK_REG, BD96801_UNLOCK);
+	ret = regmap_write(regmap, cd->unlock_reg, cd->unlock_val);
 	if (ret)
 		return dev_err_probe(&i2c->dev, ret, "Failed to unlock PMIC\n");
 
 	ret = devm_regmap_add_irq_chip(&i2c->dev, regmap, intb_irq,
-				       IRQF_ONESHOT, 0, &bd96801_irq_chip_intb,
+				       IRQF_ONESHOT, 0, cd->intb_irq_chip,
 				       &intb_irq_data);
 	if (ret)
 		return dev_err_probe(&i2c->dev, ret, "Failed to add INTB IRQ chip\n");
@@ -404,24 +435,25 @@ static int bd96801_i2c_probe(struct i2c_client *i2c)
 	 * has two domains so we do IRQ mapping here and provide the
 	 * already mapped IRQ numbers to sub-devices.
 	 */
-	for (i = 0; i < num_intb; i++) {
+	for (i = 0; i < cd->num_intb_irqs; i++) {
 		struct resource *res = &regulator_res[i];
 
-		*res = regulator_intb_irqs[i];
+		*res = cd->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;
+
+	cd->cells[WDG_CELL].resources = &wdg_irq;
+	cd->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);
+				       0, cd->errb_irq_chip, &errb_irq_data);
 	if (ret)
 		return dev_err_probe(&i2c->dev, ret,
 				     "Failed to add ERRB IRQ chip\n");
@@ -429,18 +461,17 @@ static int bd96801_i2c_probe(struct i2c_client *i2c)
 	errb_domain = regmap_irq_get_domain(errb_irq_data);
 
 	for (i = 0; i < num_errb; i++) {
-		struct resource *res = &regulator_res[num_intb + i];
+		struct resource *res = &regulator_res[cd->num_intb_irqs + i];
 
-		*res = regulator_errb_irqs[i];
+		*res = cd->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);
+	cd->cells[REGULATOR_CELL].resources = regulator_res;
+	cd->cells[REGULATOR_CELL].num_resources = num_regu_irqs;
+	ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO,
+				   cd->cells, cd->num_cells, NULL, 0, NULL);
 	if (ret)
 		dev_err_probe(&i2c->dev, ret, "Failed to create subdevices\n");
 
@@ -448,7 +479,7 @@ static int bd96801_i2c_probe(struct i2c_client *i2c)
 }
 
 static const struct of_device_id bd96801_of_match[] = {
-	{ .compatible = "rohm,bd96801",	},
+	{ .compatible = "rohm,bd96801", .data = &bd96801_chip_data, },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, bd96801_of_match);
-- 
2.48.1

On Thu, 13 Mar 2025, Matti Vaittinen wrote:

> Prepare for adding support for BD96802 which is very similar to BD96801.
> Separate chip specific data into own structure which can be picked to be
> used by the device-tree.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> ---
>  drivers/mfd/rohm-bd96801.c | 83 ++++++++++++++++++++++++++------------
>  1 file changed, 57 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/mfd/rohm-bd96801.c b/drivers/mfd/rohm-bd96801.c
> index 60ec8db790a7..1232f571e4b1 100644
> --- a/drivers/mfd/rohm-bd96801.c
> +++ b/drivers/mfd/rohm-bd96801.c
> @@ -40,7 +40,21 @@
>  #include <linux/mfd/rohm-bd96801.h>
>  #include <linux/mfd/rohm-generic.h>
>  
> -static const struct resource regulator_errb_irqs[] = {
> +struct bd968xx_chip_data {
> +	const struct resource *errb_irqs;
> +	const struct resource *intb_irqs;
> +	int num_errb_irqs;
> +	int num_intb_irqs;
> +	const struct regmap_irq_chip *errb_irq_chip;
> +	const struct regmap_irq_chip *intb_irq_chip;
> +	const struct regmap_config *regmap_config;
> +	struct mfd_cell *cells;

We're not passing MFD data through OF to be fed back through MFD APIs.

It's generally considered better to device_get_match_data() on an enum,
then populate MFD cells using that as a differentiator.

  git grep compatible -- drivers/mfd | grep data

> +	int num_cells;
> +	int unlock_reg;
> +	int unlock_val;
> +};
> +
> +static const struct resource bd96801_reg_errb_irqs[] = {
>  	DEFINE_RES_IRQ_NAMED(BD96801_OTP_ERR_STAT, "bd96801-otp-err"),
>  	DEFINE_RES_IRQ_NAMED(BD96801_DBIST_ERR_STAT, "bd96801-dbist-err"),
>  	DEFINE_RES_IRQ_NAMED(BD96801_EEP_ERR_STAT, "bd96801-eep-err"),
> @@ -98,7 +112,7 @@ static const struct resource regulator_errb_irqs[] = {
>  	DEFINE_RES_IRQ_NAMED(BD96801_LDO7_SHDN_ERR_STAT, "bd96801-ldo7-shdn-err"),
>  };
>  
> -static const struct resource regulator_intb_irqs[] = {
> +static const struct resource bd96801_reg_intb_irqs[] = {
>  	DEFINE_RES_IRQ_NAMED(BD96801_TW_STAT, "bd96801-core-thermal"),
>  
>  	DEFINE_RES_IRQ_NAMED(BD96801_BUCK1_OCPH_STAT, "bd96801-buck1-overcurr-h"),
> @@ -345,18 +359,37 @@ static const struct regmap_config bd96801_regmap_config = {
>  	.cache_type = REGCACHE_MAPLE,
>  };
>  
> +static const struct bd968xx_chip_data bd96801_chip_data = {

Just call it 'struct bd968xx' then below instead of cd, use ddata.

  git grep "cc =" -- drivers/mfd

VS

  git grep "ddata =" -- drivers/mfd

Conforrrrrrmmm ...    =;-)

> +	.errb_irqs = bd96801_reg_errb_irqs,
> +	.intb_irqs = bd96801_reg_intb_irqs,
> +	.num_errb_irqs = ARRAY_SIZE(bd96801_reg_errb_irqs),
> +	.num_intb_irqs = ARRAY_SIZE(bd96801_reg_intb_irqs),
> +	.errb_irq_chip = &bd96801_irq_chip_errb,
> +	.intb_irq_chip = &bd96801_irq_chip_intb,
> +	.regmap_config = &bd96801_regmap_config,
> +	.cells = bd96801_cells,
> +	.num_cells = ARRAY_SIZE(bd96801_cells),
> +	.unlock_reg = BD96801_LOCK_REG,
> +	.unlock_val = BD96801_UNLOCK,
> +};
> +
>  static int bd96801_i2c_probe(struct i2c_client *i2c)
>  {
>  	struct regmap_irq_chip_data *intb_irq_data, *errb_irq_data;
>  	struct irq_domain *intb_domain, *errb_domain;
> +	static const struct bd968xx_chip_data *cd;
>  	const struct fwnode_handle *fwnode;
>  	struct resource *regulator_res;
>  	struct resource wdg_irq;
>  	struct regmap *regmap;
> -	int intb_irq, errb_irq, num_intb, num_errb = 0;
> +	int intb_irq, errb_irq, num_errb = 0;
>  	int num_regu_irqs, wdg_irq_no;
>  	int i, ret;
>  
> +	cd = device_get_match_data(&i2c->dev);
> +	if (!cd)
> +		return -ENODEV;
> +
>  	fwnode = dev_fwnode(&i2c->dev);
>  	if (!fwnode)
>  		return dev_err_probe(&i2c->dev, -EINVAL, "Failed to find fwnode\n");
> @@ -365,34 +398,32 @@ 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 == -EPROBE_DEFER)
> +		return errb_irq;
>  
> -	if (errb_irq)
> -		num_errb = ARRAY_SIZE(regulator_errb_irqs);
> +	if (errb_irq > 0)
> +		num_errb = cd->num_errb_irqs;
>  
> -	num_regu_irqs = num_intb + num_errb;
> +	num_regu_irqs = cd->num_intb_irqs + 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);
> +	regmap = devm_regmap_init_i2c(i2c, cd->regmap_config);
>  	if (IS_ERR(regmap))
>  		return dev_err_probe(&i2c->dev, PTR_ERR(regmap),
>  				    "Regmap initialization failed\n");
>  
> -	ret = regmap_write(regmap, BD96801_LOCK_REG, BD96801_UNLOCK);
> +	ret = regmap_write(regmap, cd->unlock_reg, cd->unlock_val);
>  	if (ret)
>  		return dev_err_probe(&i2c->dev, ret, "Failed to unlock PMIC\n");
>  
>  	ret = devm_regmap_add_irq_chip(&i2c->dev, regmap, intb_irq,
> -				       IRQF_ONESHOT, 0, &bd96801_irq_chip_intb,
> +				       IRQF_ONESHOT, 0, cd->intb_irq_chip,
>  				       &intb_irq_data);
>  	if (ret)
>  		return dev_err_probe(&i2c->dev, ret, "Failed to add INTB IRQ chip\n");
> @@ -404,24 +435,25 @@ static int bd96801_i2c_probe(struct i2c_client *i2c)
>  	 * has two domains so we do IRQ mapping here and provide the
>  	 * already mapped IRQ numbers to sub-devices.
>  	 */
> -	for (i = 0; i < num_intb; i++) {
> +	for (i = 0; i < cd->num_intb_irqs; i++) {
>  		struct resource *res = &regulator_res[i];
>  
> -		*res = regulator_intb_irqs[i];
> +		*res = cd->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;
> +
> +	cd->cells[WDG_CELL].resources = &wdg_irq;
> +	cd->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);
> +				       0, cd->errb_irq_chip, &errb_irq_data);
>  	if (ret)
>  		return dev_err_probe(&i2c->dev, ret,
>  				     "Failed to add ERRB IRQ chip\n");
> @@ -429,18 +461,17 @@ static int bd96801_i2c_probe(struct i2c_client *i2c)
>  	errb_domain = regmap_irq_get_domain(errb_irq_data);
>  
>  	for (i = 0; i < num_errb; i++) {
> -		struct resource *res = &regulator_res[num_intb + i];
> +		struct resource *res = &regulator_res[cd->num_intb_irqs + i];
>  
> -		*res = regulator_errb_irqs[i];
> +		*res = cd->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);
> +	cd->cells[REGULATOR_CELL].resources = regulator_res;
> +	cd->cells[REGULATOR_CELL].num_resources = num_regu_irqs;
> +	ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO,
> +				   cd->cells, cd->num_cells, NULL, 0, NULL);
>  	if (ret)
>  		dev_err_probe(&i2c->dev, ret, "Failed to create subdevices\n");
>  
> @@ -448,7 +479,7 @@ static int bd96801_i2c_probe(struct i2c_client *i2c)
>  }
>  
>  static const struct of_device_id bd96801_of_match[] = {
> -	{ .compatible = "rohm,bd96801",	},
> +	{ .compatible = "rohm,bd96801", .data = &bd96801_chip_data, },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, bd96801_of_match);
> -- 
> 2.48.1
> 



-- 
Lee Jones [李琼斯]

On 20/03/2025 18:52, Lee Jones wrote:
> On Thu, 13 Mar 2025, Matti Vaittinen wrote:
> 
>> Prepare for adding support for BD96802 which is very similar to BD96801.
>> Separate chip specific data into own structure which can be picked to be
>> used by the device-tree.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>> ---
>>   drivers/mfd/rohm-bd96801.c | 83 ++++++++++++++++++++++++++------------
>>   1 file changed, 57 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/mfd/rohm-bd96801.c b/drivers/mfd/rohm-bd96801.c
>> index 60ec8db790a7..1232f571e4b1 100644
>> --- a/drivers/mfd/rohm-bd96801.c
>> +++ b/drivers/mfd/rohm-bd96801.c
>> @@ -40,7 +40,21 @@
>>   #include <linux/mfd/rohm-bd96801.h>
>>   #include <linux/mfd/rohm-generic.h>
>>   
>> -static const struct resource regulator_errb_irqs[] = {
>> +struct bd968xx_chip_data {
>> +	const struct resource *errb_irqs;
>> +	const struct resource *intb_irqs;
>> +	int num_errb_irqs;
>> +	int num_intb_irqs;
>> +	const struct regmap_irq_chip *errb_irq_chip;
>> +	const struct regmap_irq_chip *intb_irq_chip;
>> +	const struct regmap_config *regmap_config;
>> +	struct mfd_cell *cells;
> 
> We're not passing MFD data through OF to be fed back through MFD APIs.
> 
> It's generally considered better to device_get_match_data() on an enum,
> then populate MFD cells using that as a differentiator.

Or, at least someone has done this at the beginning and it got copied 
all over the place, right? ;) Sometimes we just need to challenge the 
status quo to develop ;)

I can go back to enum + switch - case in probe, and pick the correct 
data there. Done that before as well. It's just that during my journey 
to some other subsystems, I've realized people can often just skip the 
enum and switch - case, making things a tad simpler :)

Well, not a big deal to me. I suppose it has some value to keep things 
consistent inside a subsystem - and I'm not offering to drop the switch 
cases from all of the drivers :p

TL; DR - Ok.

>    git grep compatible -- drivers/mfd | grep data
> 
>> +	int num_cells;
>> +	int unlock_reg;
>> +	int unlock_val;
>> +};
>> +
>> +static const struct resource bd96801_reg_errb_irqs[] = {
>>   	DEFINE_RES_IRQ_NAMED(BD96801_OTP_ERR_STAT, "bd96801-otp-err"),
>>   	DEFINE_RES_IRQ_NAMED(BD96801_DBIST_ERR_STAT, "bd96801-dbist-err"),
>>   	DEFINE_RES_IRQ_NAMED(BD96801_EEP_ERR_STAT, "bd96801-eep-err"),
>> @@ -98,7 +112,7 @@ static const struct resource regulator_errb_irqs[] = {
>>   	DEFINE_RES_IRQ_NAMED(BD96801_LDO7_SHDN_ERR_STAT, "bd96801-ldo7-shdn-err"),
>>   };
>>   
>> -static const struct resource regulator_intb_irqs[] = {
>> +static const struct resource bd96801_reg_intb_irqs[] = {
>>   	DEFINE_RES_IRQ_NAMED(BD96801_TW_STAT, "bd96801-core-thermal"),
>>   
>>   	DEFINE_RES_IRQ_NAMED(BD96801_BUCK1_OCPH_STAT, "bd96801-buck1-overcurr-h"),
>> @@ -345,18 +359,37 @@ static const struct regmap_config bd96801_regmap_config = {
>>   	.cache_type = REGCACHE_MAPLE,
>>   };
>>   
>> +static const struct bd968xx_chip_data bd96801_chip_data = {
> 
> Just call it 'struct bd968xx' then below instead of cd, use ddata.
> 
>    git grep "cc =" -- drivers/mfd
> 
> VS
> 
>    git grep "ddata =" -- drivers/mfd
> 
> Conforrrrrrmmm ...    =;-)

I've lived through the depression of the early 90's in Finland. Learned 
how to avoid wasting things - especially letters. Wouldn't guess when 
reading my review replies, right?

...Ok.

Thanks for the review :) Much appreciated.

Yours,
	-- Matti

On Fri, 21 Mar 2025, Matti Vaittinen wrote:

> On 20/03/2025 18:52, Lee Jones wrote:
> > On Thu, 13 Mar 2025, Matti Vaittinen wrote:
> > 
> > > Prepare for adding support for BD96802 which is very similar to BD96801.
> > > Separate chip specific data into own structure which can be picked to be
> > > used by the device-tree.
> > > 
> > > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> > > ---
> > >   drivers/mfd/rohm-bd96801.c | 83 ++++++++++++++++++++++++++------------
> > >   1 file changed, 57 insertions(+), 26 deletions(-)
> > > 
> > > diff --git a/drivers/mfd/rohm-bd96801.c b/drivers/mfd/rohm-bd96801.c
> > > index 60ec8db790a7..1232f571e4b1 100644
> > > --- a/drivers/mfd/rohm-bd96801.c
> > > +++ b/drivers/mfd/rohm-bd96801.c
> > > @@ -40,7 +40,21 @@
> > >   #include <linux/mfd/rohm-bd96801.h>
> > >   #include <linux/mfd/rohm-generic.h>
> > > -static const struct resource regulator_errb_irqs[] = {
> > > +struct bd968xx_chip_data {
> > > +	const struct resource *errb_irqs;
> > > +	const struct resource *intb_irqs;
> > > +	int num_errb_irqs;
> > > +	int num_intb_irqs;
> > > +	const struct regmap_irq_chip *errb_irq_chip;
> > > +	const struct regmap_irq_chip *intb_irq_chip;
> > > +	const struct regmap_config *regmap_config;
> > > +	struct mfd_cell *cells;
> > 
> > We're not passing MFD data through OF to be fed back through MFD APIs.
> > 
> > It's generally considered better to device_get_match_data() on an enum,
> > then populate MFD cells using that as a differentiator.
> 
> Or, at least someone has done this at the beginning and it got copied all
> over the place, right? ;) Sometimes we just need to challenge the status quo
> to develop ;)

This is not one of those times.

I've never allowed mixing platform init strategies (arch-plat, OF, ACPI, MFD).

> I can go back to enum + switch - case in probe, and pick the correct data
> there. Done that before as well. It's just that during my journey to some
> other subsystems, I've realized people can often just skip the enum and
> switch - case, making things a tad simpler :)
> 
> Well, not a big deal to me. I suppose it has some value to keep things
> consistent inside a subsystem - and I'm not offering to drop the switch
> cases from all of the drivers :p
> 
> TL; DR - Ok.
> 
> >    git grep compatible -- drivers/mfd | grep data
> > 
> > > +	int num_cells;
> > > +	int unlock_reg;
> > > +	int unlock_val;
> > > +};
> > > +
> > > +static const struct resource bd96801_reg_errb_irqs[] = {
> > >   	DEFINE_RES_IRQ_NAMED(BD96801_OTP_ERR_STAT, "bd96801-otp-err"),
> > >   	DEFINE_RES_IRQ_NAMED(BD96801_DBIST_ERR_STAT, "bd96801-dbist-err"),
> > >   	DEFINE_RES_IRQ_NAMED(BD96801_EEP_ERR_STAT, "bd96801-eep-err"),
> > > @@ -98,7 +112,7 @@ static const struct resource regulator_errb_irqs[] = {
> > >   	DEFINE_RES_IRQ_NAMED(BD96801_LDO7_SHDN_ERR_STAT, "bd96801-ldo7-shdn-err"),
> > >   };
> > > -static const struct resource regulator_intb_irqs[] = {
> > > +static const struct resource bd96801_reg_intb_irqs[] = {
> > >   	DEFINE_RES_IRQ_NAMED(BD96801_TW_STAT, "bd96801-core-thermal"),
> > >   	DEFINE_RES_IRQ_NAMED(BD96801_BUCK1_OCPH_STAT, "bd96801-buck1-overcurr-h"),
> > > @@ -345,18 +359,37 @@ static const struct regmap_config bd96801_regmap_config = {
> > >   	.cache_type = REGCACHE_MAPLE,
> > >   };
> > > +static const struct bd968xx_chip_data bd96801_chip_data = {
> > 
> > Just call it 'struct bd968xx' then below instead of cd, use ddata.
> > 
> >    git grep "cc =" -- drivers/mfd
> > 
> > VS
> > 
> >    git grep "ddata =" -- drivers/mfd
> > 
> > Conforrrrrrmmm ...    =;-)
> 
> I've lived through the depression of the early 90's in Finland. Learned how
> to avoid wasting things - especially letters. Wouldn't guess when reading my
> review replies, right?
> 
> ...Ok.
> 
> Thanks for the review :) Much appreciated.

NP

-- 
Lee Jones [李琼斯]