On 08/05/2023 23:28, dinh.nguyen@xxxxxxxxxxxxxxx wrote:
> From: Dinh Nguyen <dinh.nguyen@xxxxxxxxxxxxxxx>
>
> The driver supports 64-bit SoCFPGA platforms for temperature and voltage
> reading using the platform's SDM(Secure Device Manager). The driver
> also uses the Stratix10 Service layer driver.
>
> This driver only supports OF SoCFPGA 64-bit platforms.
>
(...)
> +static int socfpga_probe_child_from_dt(struct device *dev,
> + struct device_node *child,
> + struct socfpga_hwmon_priv *priv)
> +{
> + struct device_node *grandchild;
> + const char *label;
> + const char *type;
> + u32 val;
> + int ret;
> +
> + if (of_property_read_string(child, "name", &type))
> + return dev_err_probe(dev, -EINVAL, "No type for %pOF\n", child);
> +
> + for_each_child_of_node(child, grandchild) {
> + ret = of_property_read_u32(grandchild, "reg", &val);
> + if (ret)
> + return dev_err_probe(dev, ret, "missing reg property of %pOF\n",
> + grandchild);
Where do you drop child reference?
> +
> + ret = of_property_read_string(grandchild, "label", &label);
> + if (ret)
> + return dev_err_probe(dev, ret, "missing label propoerty of %pOF\n",
> + grandchild);
> + ret = socfpga_add_channel(dev, type, val, label, priv);
> + if (ret == -ENOSPC)
> + return dev_err_probe(dev, ret, "too many channels, only %d supported\n",
> + SOCFPGA_HWMON_MAXSENSORS);
> + }
> + return 0;
> +}
> +
> +static int socfpga_probe_from_dt(struct device *dev,
> + struct socfpga_hwmon_priv *priv)
> +{
> + const struct device_node *np = dev->of_node;
> + struct device_node *child;
> + int ret = 0;
> +
> + for_each_child_of_node(np, child) {
> + ret = socfpga_probe_child_from_dt(dev, child, priv);
> + if (ret)
> + break;
> + }
> + of_node_put(child);
Hm, and if the loop does not break, is this still correct?
> +
> + return ret;
> +}
> +
> +static int socfpga_hwmon_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct socfpga_hwmon_priv *priv;
> + int ret;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->client.dev = dev;
> + priv->client.priv = priv;
> +
> + ret = socfpga_probe_from_dt(dev, priv);
> + if (ret)
> + return dev_err_probe(dev, ret, "Unable to probe from device tree\n");
> +
> + mutex_init(&priv->lock);
> + init_completion(&priv->completion);
> + priv->chan = stratix10_svc_request_channel_byname(&priv->client,
> + SVC_CLIENT_HWMON);
> + if (IS_ERR(priv->chan))
> + return dev_err_probe(dev, PTR_ERR(priv->chan),
> + "couldn't get service channel %s\n",
> + SVC_CLIENT_RSU);
> +
> + priv->hwmon_dev = devm_hwmon_device_register_with_info(dev, "socfpgahwmon",
> + priv,
> + &socfpga_chip_info,
> + NULL);
> + if (IS_ERR(priv->hwmon_dev))
> + return PTR_ERR(priv->hwmon_dev);
> +
> + platform_set_drvdata(pdev, priv);
> +
> + return 0;
> +}
> +
> +static int socfpga_hwmon_remove(struct platform_device *pdev)
> +{
> + struct socfpga_hwmon_priv *priv = platform_get_drvdata(pdev);
> +
> + hwmon_device_unregister(priv->hwmon_dev);
Please test it. I am pretty sure you will have double free here.
> + stratix10_svc_free_channel(priv->chan);
> + return 0;
> +}
> +
> +static const struct of_device_id socfpga_of_match[] = {
> + { .compatible = "intel,socfpga-hwmon" },
> + { .compatible = "intel,socfpga-agilex-hwmon" },
> + { .compatible = "intel,socfpga-n5x-hwmon" },
> + { .compatible = "intel,socfpga-stratix10-hwmon" },
These are all compatible, so why having 4 entries?
Best regards,
Krzysztof
