Re: [PATCH v3 3/7] power: supply: max77658: Add ADI MAX77654/58/59 Charger Support

[Krzysztof Kozlowski]

On 08/05/2023 15:10, Zeynep Arslanbenzer wrote:
> Charger driver for ADI MAX77654/58/59.
> 
> The MAX77654/58/59 charger is Smart Power Selector Li+/Li-Poly Charger.
> 
> Signed-off-by: Nurettin Bolucu <Nurettin.Bolucu@xxxxxxxxxx>
> Signed-off-by: Zeynep Arslanbenzer <Zeynep.Arslanbenzer@xxxxxxxxxx>
> ---
>  drivers/power/supply/Kconfig            |   7 +
>  drivers/power/supply/Makefile           |   1 +
>  drivers/power/supply/max77658-charger.c | 844 ++++++++++++++++++++++++
>  3 files changed, 852 insertions(+)
>  create mode 100644 drivers/power/supply/max77658-charger.c
> 


> +
> +static int max77658_charger_probe(struct platform_device *pdev)
> +{
> +	struct max77658_dev *max77658 = dev_get_drvdata(pdev->dev.parent);
> +	struct power_supply_config charger_cfg = {};
> +	struct power_supply_battery_info *info;
> +	struct max77658_charger *charger;
> +	struct device *dev = &pdev->dev;
> +	int i, n_irq, ret;
> +
> +	charger = devm_kzalloc(dev, sizeof(*charger), GFP_KERNEL);
> +	if (!charger)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, charger);
> +
> +	charger->dev = dev;
> +	charger->max77658 = max77658;
> +	charger->regmap = dev_get_regmap(charger->dev->parent, NULL);
> +
> +	charger->psy_chg_d.get_property	= max77658_charger_get_property;
> +	charger->psy_chg_d.set_property	= max77658_charger_set_property;
> +	charger->psy_chg_d.type = POWER_SUPPLY_TYPE_USB;
> +	charger_cfg.of_node = dev->of_node;

So here you have charger of_node...

> +	charger_cfg.drv_data = charger;
> +
> +	switch (max77658->id) {
> +	case ID_MAX77654:

This suggests your devices are not compatible...

> +		charger->psy_chg_d.properties = max77658_charger_props;
> +		charger->psy_chg_d.num_properties =
> +					ARRAY_SIZE(max77658_charger_props);
> +		charger->psy_chg_d.name = "max77654-charger";
> +		charger->psy_chg_d.property_is_writeable =
> +				max77658_property_is_writeable;
> +		n_irq = MAX77658_CHG_IRQ_MAX;
> +		break;
> +	case ID_MAX77658:
> +		charger->psy_chg_d.properties = max77658_charger_props;
> +		charger->psy_chg_d.num_properties =
> +					ARRAY_SIZE(max77658_charger_props);
> +		charger->psy_chg_d.name = "max77658-charger";
> +		charger->psy_chg_d.property_is_writeable =
> +				max77658_property_is_writeable;
> +		n_irq = MAX77658_CHG_IRQ_MAX;
> +		break;
> +	case ID_MAX77659:
> +		charger->psy_chg_d.properties = max77659_charger_props;
> +		charger->psy_chg_d.num_properties =
> +					ARRAY_SIZE(max77659_charger_props);
> +		charger->psy_chg_d.name = "max77659-charger";
> +		charger->psy_chg_d.property_is_writeable =
> +				max77659_property_is_writeable;
> +		n_irq = MAX77659_CHG_IRQ_MAX;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	charger->psy_chg = devm_power_supply_register(dev, &charger->psy_chg_d,
> +						      &charger_cfg);
> +	if (IS_ERR(charger->psy_chg))
> +		return dev_err_probe(dev, PTR_ERR(charger->psy_chg),
> +				     "Failed to register power supply\n");
> +
> +	charger->psy_chg->of_node = of_get_child_by_name(dev->parent->of_node,
> +							 "charger");

and here... this is confusing. Why do you get the child of a parent?
Isn't this exactly this node?


> +
> +	if (!charger->psy_chg->of_node)
> +		dev_err(charger->dev,
> +			"of_get_child_by_name\n");

??? Not helpful error message and actually not helpful case.
Can this even happen?
Maybe you should just drop platform_device_id?

> +
> +	ret = power_supply_get_battery_info(charger->psy_chg, &info);
> +	if (ret) {
> +		dev_err(charger->dev, "Unable to get charger info\n");
> +		charger->fast_charge_current_ua = 15000;
> +	} else {
> +		charger->fast_charge_current_ua =
> +			info->constant_charge_current_max_ua;
> +	}
> +
> +	if (charger->max77658->id != ID_MAX77659)
> +		max77658_charger_parse_dt(charger);
> +
> +	ret = max77658_charger_initialize(charger);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to initialize charger\n");
> +
> +	for (i = 0; i < n_irq; i++) {
> +		charger->irq_arr[i] =
> +				regmap_irq_get_virq(max77658->irqc_chg, i);
> +
> +		if (charger->irq_arr[i] < 0)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "Invalid IRQ for MAX77658_CHG_IRQ %d\n",
> +					     i);
> +
> +		ret = devm_request_threaded_irq(dev, charger->irq_arr[i],
> +						NULL, max77658_charger_isr,
> +						IRQF_TRIGGER_FALLING,
> +						max77658_irq_descs[i], charger);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +						 "Failed to request irq: %d\n",
> +						 charger->irq_arr[i]);
> +	}
> +
> +	ret = device_create_file(dev, &dev_attr_fast_charge_timer);

Where the ABI documentation?

> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to create fast charge timer sysfs entry\n");
> +
> +	ret = device_create_file(dev, &dev_attr_topoff_timer);

The same.

> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to create topoff timer sysfs entry\n");
> +
> +	return 0;
> +}
> +
> +static int max77658_charger_remove(struct platform_device *pdev)
> +{
> +	device_remove_file(&pdev->dev, &dev_attr_topoff_timer);
> +	device_remove_file(&pdev->dev, &dev_attr_fast_charge_timer);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id max77658_charger_of_id[] = {
> +	{ .compatible = "adi,max77654-charger" },
> +	{ .compatible = "adi,max77658-charger" },
> +	{ .compatible = "adi,max77659-charger" },

So they are compatible? If so, use one entry. If not, use driver data.


Best regards,
Krzysztof