| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 8 May 2023 21:51:42 +0200
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Zeynep Arslanbenzer <Zeynep.Arslanbenzer@...log.com>,
lee@...nel.org, robh+dt@...nel.org,
krzysztof.kozlowski+dt@...aro.org, sre@...nel.org,
lgirdwood@...il.com, broonie@...nel.org
Cc: linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
linux-pm@...r.kernel.org,
Nurettin Bolucu <Nurettin.Bolucu@...log.com>
Subject: Re: [PATCH v3 3/7] power: supply: max77658: Add ADI MAX77654/58/59
Charger Support
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@...log.com>
> Signed-off-by: Zeynep Arslanbenzer <Zeynep.Arslanbenzer@...log.com>
> ---
> 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
Powered by blists - more mailing lists