Re: [PATCH 2/2] power: supply: Add bd718(15/28/78) charger driver

[Krzysztof Kozlowski <krzk@kernel.org>]

On 16/08/2025 21:19, Andreas Kemnade wrote:
> Add charger driver for ROHM BD718(15/28/78) PMIC charger block.
> It is a stripped down version of the driver here:
> https://lore.kernel.org/lkml/dbd97c1b0d715aa35a8b4d79741e433d97c562aa.1637061794.git.matti.vaittinen@fi.rohmeurope.com/

Why are you duplicating the driver? Why original cannot be used?


...

> +
> +#define RSENS_DEFAULT_30MOHM 30000 /* 30 mOhm in uOhms*/
> +
> +static int bd7182x_get_rsens(struct bd71828_power *pwr)
> +{
> +	u64 tmp = RSENS_CURR;
> +	int rsens_ohm = RSENS_DEFAULT_30MOHM;
> +	struct fwnode_handle *node = NULL;
> +
> +	if (pwr->dev->parent)
> +		node = dev_fwnode(pwr->dev->parent);
> +
> +	if (node) {
> +		int ret;
> +		uint32_t rs;
> +
> +		ret = fwnode_property_read_u32(node,
> +					       "rohm,charger-sense-resistor-micro-ohms",

Hm? Are you writing ACPI or DT driver?

> +					       &rs);
> +		if (ret) {
> +			if (ret == -EINVAL) {
> +				rs = RSENS_DEFAULT_30MOHM;
> +			} else {
> +				dev_err(pwr->dev, "Bad RSENS dt property\n");
> +				return ret;
> +			}
> +		}
> +		if (!rs) {
> +			dev_err(pwr->dev, "Bad RSENS value\n");
> +			return -EINVAL;
> +		}
> +
> +		rsens_ohm = (int)rs;
> +	}
> +
> +	/* Reg val to uA */
> +	do_div(tmp, rsens_ohm);
> +
> +	pwr->curr_factor = tmp;
> +	pwr->rsens = rsens_ohm;
> +	dev_dbg(pwr->dev, "Setting rsens to %u micro ohm\n", pwr->rsens);
> +	dev_dbg(pwr->dev, "Setting curr-factor to %u\n", pwr->curr_factor);
> +	return 0;
> +}
> +
> +static int bd71828_power_probe(struct platform_device *pdev)
> +{
> +	struct bd71828_power *pwr;
> +	struct power_supply_config ac_cfg = {};
> +	struct power_supply_config bat_cfg = {};
> +	int ret;
> +	struct regmap *regmap;
> +
> +	regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +	if (!regmap) {
> +		dev_err(&pdev->dev, "No parent regmap\n");
> +		return -EINVAL;
> +	}
> +
> +	pwr = devm_kzalloc(&pdev->dev, sizeof(*pwr), GFP_KERNEL);
> +	if (!pwr)
> +		return -ENOMEM;
> +
> +	pwr->regmap = regmap;
> +	pwr->dev = &pdev->dev;
> +	pwr->chip_type = platform_get_device_id(pdev)->driver_data;
> +
> +	switch (pwr->chip_type) {
> +	case ROHM_CHIP_TYPE_BD71828:
> +		pwr->bat_inserted = bd71828_bat_inserted;
> +		pwr->get_temp = bd71828_get_temp;
> +		pwr->regs = &pwr_regs_bd71828;
> +		dev_dbg(pwr->dev, "Found ROHM BD71828\n");

This is pretty useless debug. You do not use here autodetection, so
there is no "found" case. It's straightforward bind.

> +		break;
> +	case ROHM_CHIP_TYPE_BD71815:
> +		pwr->bat_inserted = bd71815_bat_inserted;
> +		pwr->get_temp = bd71815_get_temp;
> +		pwr->regs = &pwr_regs_bd71815;
> +		dev_dbg(pwr->dev, "Found ROHM BD71815\n");

Same here, drop.

> +	break;
> +	default:
> +		dev_err(pwr->dev, "Unknown PMIC\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = bd7182x_get_rsens(pwr);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "sense resistor missing\n");
> +
> +	dev_set_drvdata(&pdev->dev, pwr);
> +	bd71828_init_hardware(pwr);
> +
> +	bat_cfg.drv_data	= pwr;
> +	bat_cfg.fwnode		= dev_fwnode(&pdev->dev);
> +
> +	ac_cfg.supplied_to	= bd71828_ac_supplied_to;
> +	ac_cfg.num_supplicants	= ARRAY_SIZE(bd71828_ac_supplied_to);
> +	ac_cfg.drv_data		= pwr;
> +
> +	pwr->ac = devm_power_supply_register(&pdev->dev, &bd71828_ac_desc,
> +					     &ac_cfg);
> +	if (IS_ERR(pwr->ac)) {
> +		return dev_err_probe(&pdev->dev, PTR_ERR(pwr->ac),
> +				     "failed to register ac\n");
> +	}
> +
> +	pwr->bat = devm_power_supply_register(&pdev->dev, &bd71828_bat_desc,
> +					      &bat_cfg);
> +	if (IS_ERR(pwr->bat)) {
> +		return dev_err_probe(&pdev->dev, PTR_ERR(pwr->bat),
> +				     "failed to register bat\n");
> +	}
> +
> +	ret = bd7182x_get_irqs(pdev, pwr);
> +	if (ret) {

Please run scripts/checkpatch.pl on the patches and fix reported
warnings. After that, run also 'scripts/checkpatch.pl --strict' on the
patches and (probably) fix more warnings. Some warnings can be ignored,
especially from --strict run, but the code here looks like it needs a
fix. Feel free to get in touch if the warning is not clear.

Drop {}

This applies to other places as well.

> +		return dev_err_probe(&pdev->dev, ret, "failed to request IRQs");
> +	};
> +
> +	/* Configure wakeup capable */
> +	device_set_wakeup_capable(pwr->dev, 1);
> +	device_set_wakeup_enable(pwr->dev, 1);
> +
> +	return 0;
> +}
> +
> +static const struct platform_device_id bd71828_charger_id[] = {
> +	{ "bd71815-power", ROHM_CHIP_TYPE_BD71815 },
> +	{ "bd71828-power", ROHM_CHIP_TYPE_BD71828 },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(platform, bd71828_charger_id);
> +
> +static struct platform_driver bd71828_power_driver = {
> +	.driver = {
> +		.name = "bd718xx-power",
> +	},
> +	.probe = bd71828_power_probe,
> +	.id_table = bd71828_charger_id,
> +};
> +
> +module_platform_driver(bd71828_power_driver);
> +MODULE_ALIAS("platform:bd718xx-power");

Drop module alias, incorrect name anyway and you already have proper
aliases from table.


Best regards,
Krzysztof