Re: [PATCH 4/5] hwmon: add Stratix 10 SoC FPGA hardware monitor driver

[Krzysztof Kozlowski]

On Fri, Jun 19, 2026 at 02:38:55AM -0700, tze.yee.ng@xxxxxxxxxx wrote:
> +	dev_warn(dev, "unsupported sensor type %s\n", type);
> +	return 0;
> +}
> +
> +static int stratix10_hwmon_probe_child_from_dt(struct device *dev,
> +					       struct device_node *child,

np

> +					      struct stratix10_hwmon_priv *priv)
> +{
> +	struct device_node *grandchild;

child

> +	const char *label;
> +	u32 val;
> +	int ret;
> +
> +	for_each_child_of_node(child, grandchild) {

child

> +		ret = of_property_read_u32(grandchild, "reg", &val);
> +		if (ret) {
> +			dev_err(dev, "missing reg property of %pOFn\n",
> +				grandchild);
> +			of_node_put(grandchild);
> +			return ret;
> +		}
> +
> +		ret = of_property_read_string(grandchild, "label", &label);
> +		if (ret)
> +			label = grandchild->name;
> +
> +		stratix10_hwmon_add_channel(dev, child->name, val, label, priv);
> +	}
> +
> +	return 0;
> +}
> +
> +static int stratix10_hwmon_probe_from_dt(struct device *dev,
> +					 struct stratix10_hwmon_priv *priv)
> +{
> +	struct device_node *child;
> +	int ret;
> +
> +	if (!dev->of_node)
> +		return 0;
> +
> +	for_each_child_of_node(dev->of_node, child) {

Just use scoped. And why not available child node? Why do you probe
disabled channels?

> +		ret = stratix10_hwmon_probe_child_from_dt(dev, child, priv);
> +		if (ret) {
> +			of_node_put(child);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int stratix10_hwmon_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct stratix10_hwmon_priv *priv;
> +	struct device *hwmon_dev;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->client.dev = dev;
> +	priv->client.priv = priv;
> +	init_completion(&priv->completion);
> +	mutex_init(&priv->lock);
> +
> +	ret = stratix10_hwmon_probe_from_dt(dev, priv);
> +	if (ret) {
> +		dev_err(dev, "Unable to probe from device tree\n");
> +		return ret;
> +	}
> +
> +	if (!priv->temperature_channels && !priv->voltage_channels) {
> +		dev_err(dev, "no temperature or voltage channels in device tree\n");
> +		return -ENODEV;
> +	}
> +
> +	priv->chan = stratix10_svc_request_channel_byname(&priv->client,
> +							  SVC_CLIENT_HWMON);
> +	if (IS_ERR(priv->chan)) {
> +		ret = PTR_ERR(priv->chan);
> +		if (ret == -EPROBE_DEFER)
> +			dev_dbg(dev, "service channel %s not ready, deferring probe\n",
> +				SVC_CLIENT_HWMON);
> +		else
> +			dev_err(dev, "couldn't get service channel %s: %d\n",
> +				SVC_CLIENT_HWMON, ret);
> +		return ret;
> +	}
> +
> +	ret = stratix10_svc_add_async_client(priv->chan, false);
> +	switch (ret) {
> +	case 0:
> +		priv->async = true;
> +		break;
> +	case -EINVAL:
> +		dev_dbg(dev, "async operations not supported, using sync mode\n");
> +		priv->async = false;
> +		break;
> +	default:
> +		dev_err(dev, "failed to add async client: %d\n", ret);
> +		stratix10_svc_free_channel(priv->chan);
> +		return ret;
> +	}
> +
> +	dev_info(dev, "Initialized %d temperature and %d voltage channels\n",
> +		 priv->temperature_channels, priv->voltage_channels);

Drop, driver should be silent on success. dev_dbg might be fine, but
honestly that's static information from DT so zero usefulness.

Best regards,
Krzysztof