| 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
| ||
|
Message-ID: <09369678-8a5e-46c7-97df-3783d48c65e5@altera.com>
Date: Thu, 18 Dec 2025 09:57:27 +0000
From: "Mohamad Jamian, Muhammad Amirul Asyraf"
<muhammad.amirul.asyraf.mohamad.jamian@...era.com>
To: Krzysztof Kozlowski <krzk@...nel.org>, "Mohamad Jamian, Muhammad Amirul
Asyraf" <muhammad.amirul.asyraf.mohamad.jamian@...era.com>
CC: Guenter Roeck <linux@...ck-us.net>, "linux-hwmon@...r.kernel.org"
<linux-hwmon@...r.kernel.org>, Dinh Nguyen <dinguyen@...nel.org>, Rob Herring
<robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, "devicetree@...r.kernel.org"
<devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "Ang, Tien Sung" <tien.sung.ang@...era.com>,
"Romli, Khairul Anuar" <khairul.anuar.romli@...era.com>
Subject: Re: [PATCH v1 3/5] hwmon: (altr-hwmon): Add initial support for
SoCFPGA
On 18/12/2025 4:29 pm, Krzysztof Kozlowski wrote:
> On Mon, Dec 15, 2025 at 10:49:24PM -0800, muhammadamirulasyraf.mohamadjamian@...era.com wrote:
>> From: Muhammad Amirul Asyraf Mohamad Jamian <muhammad.amirul.asyraf.mohamad.jamian@...era.com>
>>
>> This patch introduces a new hardware monitoring (hwmon) driver for the
>> Altera SoCFPGA platform, enabling kernel support for monitoring voltage and
>> temperature sensors critical for device health and system stability.
>>
>> Changes include:
>> - New driver implementation 'drivers/hwmon/altera-hwmon.c' providing sensor
>> reading and event handling capabilities tailored for SoCFPGA hardware.
>>
>> - Build system integration by adding Kconfig and Makefile entries, allowing
>> users to enable the driver in kernel configuration.
>>
>> - Documentation added in 'Documentation/hwmon/altera-hwmon.rst', detailing
>> driver features, usage instructions, device tree bindings, and
>> configuration options.
>>
>> - Update to 'Documentation/hwmon/index.rst' to reference the new driver
>> documentation, improving discoverability and user guidance.
>>
>> Signed-off-by: Khairul Anuar Romli <khairul.anuar.romli@...era.com>
>> Signed-off-by: Muhammad Amirul Asyraf Mohamad Jamian <muhammad.amirul.asyraf.mohamad.jamian@...era.com>
>> ---
>> Documentation/hwmon/altr-hwmon.rst | 32 +++
>> Documentation/hwmon/index.rst | 1 +
>> MAINTAINERS | 2 +
>> drivers/hwmon/Kconfig | 10 +
>> drivers/hwmon/Makefile | 1 +
>> drivers/hwmon/altr-hwmon.c | 427 +++++++++++++++++++++++++++++
>> 6 files changed, 473 insertions(+)
>> create mode 100644 Documentation/hwmon/altr-hwmon.rst
>> create mode 100644 drivers/hwmon/altr-hwmon.c
>>
>> diff --git a/Documentation/hwmon/altr-hwmon.rst b/Documentation/hwmon/altr-hwmon.rst
>> new file mode 100644
>> index 000000000000..3ef1ca0d1686
>> --- /dev/null
>> +++ b/Documentation/hwmon/altr-hwmon.rst
>> @@ -0,0 +1,32 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +Kernel driver altr-hwmon
>> +=========================
>> +
>> +Supported chips:
>> +
>> + * Intel N5X
>> + * Stratix10
>> + * Agilex
>> + * Agilex5
>> +
>> +Contributor: Kris Chaplin <kris.chaplin@...el.com>
>> + Khairul Anuar Romli <khairul.anuar.romli@...era.com>
>> + Muhammad Amirul Asyraf Mohamad Jamian <muhammad.amirul.asyraf.mohamad.jamian@...era.com>
>> +
>> +Description
>> +-----------
>> +
>> +This driver supports hardware monitoring for 64-Bit SoC FPGA and eASIC devices
>> +based around the Secure Device Manager and Stratix 10 Service layer.
>> +
>> +The following sensor types are supported
>> +
>> + * temperature
>> + * voltage
>> +
>> +
>> +Usage Notes
>> +-----------
>> +
>> +The driver relies on a device tree node to enumerate support present on the
>> +specific device. See Documentation/devicetree/bindings/hwmon/altr,socfpga-hwmon.yaml for details of the device-tree node.
>> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
>> index 85d7a686883e..d37d4cbbe8b5 100644
>> --- a/Documentation/hwmon/index.rst
>> +++ b/Documentation/hwmon/index.rst
>> @@ -40,6 +40,7 @@ Hardware Monitoring Kernel Drivers
>> adt7470
>> adt7475
>> aht10
>> + altr-hwmon
>> amc6821
>> aquacomputer_d5next
>> asb100
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 8ac7fef4563a..01f776fdbf6f 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -942,6 +942,8 @@ M: Muhammad Amirul Asyraf Mohamad Jamian <muhammad.amirul.asyraf.mohamad.jamian@
>> L: linux-hwmon@...r.kernel.org
>> S: Maintained
>> F: Documentation/devicetree/bindings/hwmon/altr,socfpga-hwmon.yaml
>> +F: Documentation/hwmon/altr-hwmon.rst
>> +F: drivers/hwmon/altr-hwmon.c
>>
>> ALTERA MAILBOX DRIVER
>> M: Tien Sung Ang <tiensung.ang@...era.com>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index d9bac1e3057b..4351725831d3 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -2122,6 +2122,16 @@ config SENSORS_SMSC47M192
>> This driver can also be built as a module. If so, the module
>> will be called smsc47m192.
>>
>> +config SENSORS_ALTERA_SOCFPGA
>> + tristate "Altera SoC FPGA Hardware monitoring features"
>> + depends on INTEL_STRATIX10_SERVICE
>
> Why this cannot be compile tested?
>
> ...
>
>
>> +static int altr_socfpga_probe_child_from_dt(struct device *dev,
>> + struct device_node *child,
>> + struct altr_socfpga_hwmon_priv *priv)
>> +{
>> + u32 val;
>> + int ret;
>> + struct device_node *grandchild;
>> + const char *label;
>> + const char *type;
>> +
>> + of_property_read_string(child, "name", &type);
>> + for_each_child_of_node(child, grandchild) {
>
> No, see my further comment.
>
>> + ret = of_property_read_u32(grandchild, "reg", &val);
>> + if (ret) {
>> + dev_err(dev, "missing reg property of %pOFn\n",
>> + grandchild);
>> + return ret;
>> + }
>> + ret = of_property_read_string(grandchild, "label", &label);
>> + if (ret) {
>> + dev_err(dev, "missing label propoerty of %pOFn\n",
>> + grandchild);
>> + return ret;
>> + }
>> +
>> + altr_socfpga_add_channel(dev, type, val, label, priv);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int altr_socfpga_probe_from_dt(struct device *dev,
>> + struct altr_socfpga_hwmon_priv *priv)
>> +{
>> + const struct device_node *np = dev->of_node;
>> + struct device_node *child;
>> + int ret;
>> +
>> + /* Compatible with non-DT platforms */
>> + if (!np)
>> + return 0;
>> +
>> + for_each_child_of_node(np, child) {
>> + ret = altr_socfpga_probe_child_from_dt(dev, child, priv);
>> + if (ret) {
>> + of_node_put(child);
>
> Just use scoped. Please don't upstream old code, but take new drivers
> and use them as your starting point. You just repeat issues we fixed or
> old style we changed loong time ago.
>
>
>> + return ret;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int altr_socfpga_hwmon_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct device *hwmon_dev;
>> + struct altr_socfpga_hwmon_priv *priv;
>> + int ret;
>> +
>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>> +
>> + priv->client.dev = dev;
>> + priv->client.receive_cb = NULL;
>> + priv->client.priv = priv;
>> + priv->temperature_channels = 0;
>> + priv->voltage_channels = 0;
>> +
>> + ret = altr_socfpga_probe_from_dt(dev, priv);
>> + if (ret) {
>> + dev_err(dev, "Unable to probe from device tree\n");
>> + return ret;
>
> No, syntax is return dev_err_probe
>> + }
>> +
>> + mutex_init(&priv->lock);
>> +
>> + priv->chan = stratix10_svc_request_channel_byname(&priv->client,
>> + SVC_CLIENT_HWMON);
>> + if (IS_ERR(priv->chan)) {
>> + dev_err(dev, "couldn't get service channel %s defering probe...\n",
>> + SVC_CLIENT_HWMON);
>
> No, you are now spamming the dmesg with useless deferrals. return
> dev_err_probe
>
>
>> + return -EPROBE_DEFER;
>
>
> Why ignoring actual error?
>
>> + }
>> +
>> + dev_info(dev, "Initialized %d temperature and %d voltage channels",
>> + priv->temperature_channels, priv->voltage_channels);
>
> Drop, pretty useless. Drivers are supposed to be silent on success.
>
>
>> +
>> + hwmon_dev = devm_hwmon_device_register_with_info(dev, "altr_hwmon", priv,
>> + &altr_socfpga_hwmon_chip_info,
>> + NULL);
>> +
>> + init_completion(&priv->completion);
>> + platform_set_drvdata(pdev, priv);
>> +
>> + return PTR_ERR_OR_ZERO(hwmon_dev);
>> +}
>> +
>> +static void altr_socfpga_hwmon_remove(struct platform_device *pdev)
>> +{
>> + struct altr_socfpga_hwmon_priv *priv = platform_get_drvdata(pdev);
>> +
>> + stratix10_svc_free_channel(priv->chan);
>> +}
>> +
>> +static const struct of_device_id altr_socfpga_of_match[] = {
>> + { .compatible = "altr,socfpga-hwmon" },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, altr_socfpga_of_match);
>> +
>> +static struct platform_driver altr_socfpga_hwmon_driver = {
>> + .driver = {
>> + .name = "altr-hwmon",
>> + .of_match_table = altr_socfpga_of_match,
>> + },
>> + .probe = altr_socfpga_hwmon_probe,
>> + .remove = altr_socfpga_hwmon_remove,
>> +};
>> +module_platform_driver(altr_socfpga_hwmon_driver);
>> +
>> +MODULE_AUTHOR("Altera Corporation");
>> +MODULE_DESCRIPTION("Altera SoC FPGA hardware monitoring features");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.43.7
>>
We will ovrehaul the patches based on new linux driver implementation as
starting point.
Powered by blists - more mailing lists