Re: [PATCH V9 3/4] power: supply: Add charger driver for Rockchip RK817

[Matti Vaittinen <mazziesaccount@gmail.com>]

On 8/23/22 22:30, Chris Morgan wrote:
> From: Chris Morgan <macromorgan@hotmail.com>
> 
> Add support for the Rockchip rk817 battery charger integrated into the
> rk817 PMIC.
> 
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> Signed-off-by: Maya Matuszczyk <maccraft123mc@gmail.com>
> ---
>   drivers/power/supply/Kconfig         |    6 +
>   drivers/power/supply/Makefile        |    1 +
>   drivers/power/supply/rk817_charger.c | 1157 ++++++++++++++++++++++++++
>   3 files changed, 1164 insertions(+)
>   create mode 100644 drivers/power/supply/rk817_charger.c
> 
> +
> +static void rk817_charging_monitor(struct work_struct *work)
> +{
> +	struct rk817_charger *charger;
> +
> +	charger = container_of(work, struct rk817_charger, work.work);
> +
> +	rk817_read_props(charger);
> +
> +	/* Run every 8 seconds like the BSP driver did. */
> +	queue_delayed_work(system_wq, &charger->work, msecs_to_jiffies(8000));
> +}

I really think we would benefit from some more framework code which 
could handle the periodic polling tasks and the coulomb counter drift 
corrections when battery is full/relaxed. I think I might revive the 
simple-gauge patch series...

> +
> +static int rk817_charger_probe(struct platform_device *pdev)
> +{
> +
> +	charger->sleep_filter_current_ua = of_value;
> +
> +	charger->bat_ps = devm_power_supply_register(&pdev->dev,
> +						     &rk817_bat_desc, &pscfg);
> +
> +	charger->chg_ps = devm_power_supply_register(&pdev->dev,
> +						     &rk817_chg_desc, &pscfg);

Hmm. I think I should respin the patch which added interface for getting 
the battery info w/o psy-device. Now we need to take into account the 
situation where the psy-core accesses the driver after the registration 
- and prior filling the battery details from the battery node (below) :/

> +
> +	if (IS_ERR(charger->chg_ps))
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Battery failed to probe\n");
> +
> +	if (IS_ERR(charger->chg_ps))
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Charger failed to probe\n");
> +
> +	ret = power_supply_get_battery_info(charger->bat_ps,
> +					    &bat_info);
> +	if (ret) {
> +		return dev_err_probe(dev, ret,
> +				     "Unable to get battery info: %d\n", ret);
> +	} > +
> +	if ((!bat_info->charge_full_design_uah) ||
> +	    (!bat_info->voltage_min_design_uv) ||
> +	    (!bat_info->voltage_max_design_uv) ||
> +	    (!bat_info->constant_charge_voltage_max_uv) ||
> +	    (!bat_info->constant_charge_current_max_ua) ||
> +	    (!bat_info->charge_term_current_ua)) {
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Required battery info missing.\n");
> +	}

Just a question - should the values be compared to -EINVAL (I think the 
power_supply_get_battery_info() did internally initialize many of the 
fields to -EINVAL and not to 0?). Maybe I am wrong...

> +
> +	charger->bat_charge_full_design_uah = bat_info->charge_full_design_uah;
> +	charger->bat_voltage_min_design_uv = bat_info->voltage_min_design_uv;
> +	charger->bat_voltage_max_design_uv = bat_info->voltage_max_design_uv;
> +

Generally, I did _really_ like the proper commenting/documenting of the 
driver. In my eyes this looked like one nice piece of a driver.

When the error checking of values returned by the 
power_supply_get_battery_info() is checked - FWIW:
Reviewed-By: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>

Yours
   -- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~