[PATCH v3 0/4] Add Ampere's Altra SMPro hwmon driver

[PATCH v3 0/4] Add Ampere's Altra SMPro hwmon driver

[Quan Nguyen]

This patch series adds support for Ampere SMpro hwmon driver. This
driver supports accessing various CPU sensors provided by the SMpro
co-processor including temperature, power, voltages, and current found
on Ampere Altra processor family.

v3:
  + Supported list of compatible string [Rob]
  + Introduced reg property in DT to specify reg offset [Rob]
  + Updated description and other minor changes in yaml file [Rob]
  + Handled negative temperature value [Guenter]
  + Returned -ENODEV if Manufacturer ID is wrong [Guenter]
  + Refactored smpro_read_string() and smpro_temp_read() [Guenter]
  + Removed smpro_write() function [Guenter]
  + Added minor refactor changes [Quan]

v2:
  + Used 'struct of_device_id's .data attribute [Lee Jones]
  + Removed "virtual" sensors [Guenter]
  + Fixed typo "mili" to "milli", "nanoWatt" to "microWatt" [Guenter]
  + Reported SOC_TDP as "Socket TDP" using max attributes [Guenter]
  + Clarified "highest" meaning in documentation [Guenter]
  + Corrected return error code when host is turn off [Guenter]
  + Reported MEM HOT Threshold for all DIMMs as temp*_crit [Guenter]
  + Removed license info as SPDX-License-Identifier existed [Guenter]
  + Added is_visible() support [Guenter]
  + Used HWMON_CHANNEL_INFO() macro and LABEL attributes [Guenter]
  + Made is_valid_id() return boolean [Guenter]
  + Returned -EPROBE_DEFER when smpro reg inaccessible [Guenter]
  + Removed unnecessary error message when dev register fail [Guenter]
  + Removed Socket TDP sensor [Quan]
  + Changed "ampere,ac01-smpro" to "ampere,smpro" [Quan]
  + Included sensor type and channel in labels [Quan]
  + Refactorized code to fix checkpatch.pl --strict complaint [Quan]

Quan Nguyen (4):
  dt-bindings: mfd: Add bindings for Ampere Altra SMPro drivers
  mfd: simple-mfd-i2c: Adds Ampere's Altra SMpro support
  hwmon: smpro: Add Ampere's Altra smpro-hwmon driver
  docs: hwmon: (smpro-hwmon) Add documentation

 .../bindings/hwmon/ampere,ac01-hwmon.yaml     |  28 +
 .../devicetree/bindings/mfd/ampere,smpro.yaml | 105 ++++
 Documentation/hwmon/index.rst                 |   1 +
 Documentation/hwmon/smpro-hwmon.rst           | 101 ++++
 drivers/hwmon/Kconfig                         |   8 +
 drivers/hwmon/Makefile                        |   1 +
 drivers/hwmon/smpro-hwmon.c                   | 491 ++++++++++++++++++
 drivers/mfd/Kconfig                           |  10 +
 drivers/mfd/simple-mfd-i2c.c                  |   6 +
 9 files changed, 751 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml
 create mode 100644 Documentation/devicetree/bindings/mfd/ampere,smpro.yaml
 create mode 100644 Documentation/hwmon/smpro-hwmon.rst
 create mode 100644 drivers/hwmon/smpro-hwmon.c

-- 
2.28.0

[PATCH v3 1/4] dt-bindings: mfd: Add bindings for Ampere Altra SMPro drivers

[Quan Nguyen]

Adds device tree bindings for SMPro drivers found on the Mt.Jade hardware
reference platform with Ampere's Altra Processor family.

Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
---
 .../bindings/hwmon/ampere,ac01-hwmon.yaml     |  28 +++++
 .../devicetree/bindings/mfd/ampere,smpro.yaml | 105 ++++++++++++++++++
 2 files changed, 133 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml
 create mode 100644 Documentation/devicetree/bindings/mfd/ampere,smpro.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml b/Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml
new file mode 100644
index 000000000000..fbf7ec754160
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml
@@ -0,0 +1,28 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/ampere,ac01-hwmon.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Hardware monitoring driver for the Ampere Altra SMPro
+
+maintainers:
+  - Quan Nguyen <quan@os.amperecomputing.com>
+
+description: |
+  This module is part of the Ampere Altra SMPro multi-function device. For more
+  details see ../mfd/ampere,smpro.yaml.
+
+properties:
+  compatible:
+    enum:
+      - ampere,ac01-hwmon
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
diff --git a/Documentation/devicetree/bindings/mfd/ampere,smpro.yaml b/Documentation/devicetree/bindings/mfd/ampere,smpro.yaml
new file mode 100644
index 000000000000..5613c420869e
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/ampere,smpro.yaml
@@ -0,0 +1,105 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/ampere,smpro.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Ampere Altra SMPro firmware driver
+
+maintainers:
+  - Quan Nguyen <quan@os.amperecomputing.com>
+
+description: |
+  Ampere Altra SMPro firmware may contain different blocks like hardware
+  monitoring, error monitoring and other miscellaneous features.
+
+properties:
+  compatible:
+    enum:
+      - ampere,smpro
+
+  reg:
+    description:
+      I2C device address.
+    maxItems: 1
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+patternProperties:
+  "^hwmon(@[0-9a-f]+)?$":
+    $ref: ../hwmon/ampere,ac01-hwmon.yaml
+
+  "^misc(@[0-9a-f]+)?$":
+    type: object
+    description: |
+      This module is part of the Ampere Altra SMPro multi-function device
+      to support miscellaneous features
+    properties:
+      compatible:
+        enum:
+          - ampere,ac01-misc
+      reg:
+        maxItems: 1
+
+    required:
+      - compatible
+      - reg
+
+  "^errmon(@[0-9a-f]+)?$":
+    type: object
+    description: |
+      This module is part of the Ampere Altra SMPro multi-function device
+      that supports error monitoring feature.
+
+    properties:
+      compatible:
+        enum:
+          - ampere,ac01-errmon
+      reg:
+        maxItems: 1
+
+    required:
+      - compatible
+      - reg
+
+required:
+  - "#address-cells"
+  - "#size-cells"
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        smpro@4f {
+            compatible = "ampere,smpro";
+            reg = <0x4f>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            hwmon@10 {
+                compatible = "ampere,ac01-hwmon";
+                reg = <0x10>;
+            };
+
+            misc@b0 {
+                compatible = "ampere,ac01-misc";
+                reg = <0xb0>;
+            };
+
+            errmon@80 {
+                compatible = "ampere,ac01-errmon";
+                reg = <0x80>;
+            };
+
+        };
+    };
-- 
2.28.0

Re: [PATCH v3 1/4] dt-bindings: mfd: Add bindings for Ampere Altra SMPro drivers

[Rob Herring]

On Fri, Apr 09, 2021 at 10:13:29AM +0700, Quan Nguyen wrote:
> Adds device tree bindings for SMPro drivers found on the Mt.Jade hardware
> reference platform with Ampere's Altra Processor family.
> 
> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
> ---
>  .../bindings/hwmon/ampere,ac01-hwmon.yaml     |  28 +++++
>  .../devicetree/bindings/mfd/ampere,smpro.yaml | 105 ++++++++++++++++++
>  2 files changed, 133 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml
>  create mode 100644 Documentation/devicetree/bindings/mfd/ampere,smpro.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml b/Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml
> new file mode 100644
> index 000000000000..fbf7ec754160
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml
> @@ -0,0 +1,28 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/ampere,ac01-hwmon.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Hardware monitoring driver for the Ampere Altra SMPro
> +
> +maintainers:
> +  - Quan Nguyen <quan@os.amperecomputing.com>
> +
> +description: |
> +  This module is part of the Ampere Altra SMPro multi-function device. For more
> +  details see ../mfd/ampere,smpro.yaml.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ampere,ac01-hwmon
> +
> +  reg:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> diff --git a/Documentation/devicetree/bindings/mfd/ampere,smpro.yaml b/Documentation/devicetree/bindings/mfd/ampere,smpro.yaml
> new file mode 100644
> index 000000000000..5613c420869e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/ampere,smpro.yaml
> @@ -0,0 +1,105 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/ampere,smpro.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Ampere Altra SMPro firmware driver
> +
> +maintainers:
> +  - Quan Nguyen <quan@os.amperecomputing.com>
> +
> +description: |
> +  Ampere Altra SMPro firmware may contain different blocks like hardware
> +  monitoring, error monitoring and other miscellaneous features.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ampere,smpro

Again, not very specific. There's only 1 version of 'smpro' h/w or 
firmware? Are the firmware version and features discoverable? If not, 
you need to be more specific (or better yet, make them discoverable).

> +
> +  reg:
> +    description:
> +      I2C device address.
> +    maxItems: 1
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +patternProperties:
> +  "^hwmon(@[0-9a-f]+)?$":
> +    $ref: ../hwmon/ampere,ac01-hwmon.yaml
> +
> +  "^misc(@[0-9a-f]+)?$":

You don't need these child nodes in DT if there are no resources 
associated with them. The parent driver can instantiate all the 
sub-functions.

> +    type: object
> +    description: |
> +      This module is part of the Ampere Altra SMPro multi-function device
> +      to support miscellaneous features
> +    properties:
> +      compatible:
> +        enum:
> +          - ampere,ac01-misc
> +      reg:
> +        maxItems: 1
> +
> +    required:
> +      - compatible
> +      - reg
> +
> +  "^errmon(@[0-9a-f]+)?$":
> +    type: object
> +    description: |
> +      This module is part of the Ampere Altra SMPro multi-function device
> +      that supports error monitoring feature.
> +
> +    properties:
> +      compatible:
> +        enum:
> +          - ampere,ac01-errmon
> +      reg:
> +        maxItems: 1
> +
> +    required:
> +      - compatible
> +      - reg
> +
> +required:
> +  - "#address-cells"
> +  - "#size-cells"
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        smpro@4f {
> +            compatible = "ampere,smpro";
> +            reg = <0x4f>;
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            hwmon@10 {
> +                compatible = "ampere,ac01-hwmon";
> +                reg = <0x10>;
> +            };
> +
> +            misc@b0 {
> +                compatible = "ampere,ac01-misc";
> +                reg = <0xb0>;
> +            };
> +
> +            errmon@80 {
> +                compatible = "ampere,ac01-errmon";
> +                reg = <0x80>;
> +            };
> +
> +        };
> +    };
> -- 
> 2.28.0
> 

Re: [PATCH v3 1/4] dt-bindings: mfd: Add bindings for Ampere Altra SMPro drivers

[Quan Nguyen]

On 13/04/2021 20:49, Rob Herring wrote:
> On Fri, Apr 09, 2021 at 10:13:29AM +0700, Quan Nguyen wrote:
>> Adds device tree bindings for SMPro drivers found on the Mt.Jade hardware
>> reference platform with Ampere's Altra Processor family.
>>
>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
>> ---
>>   .../bindings/hwmon/ampere,ac01-hwmon.yaml     |  28 +++++
>>   .../devicetree/bindings/mfd/ampere,smpro.yaml | 105 ++++++++++++++++++
>>   2 files changed, 133 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml
>>   create mode 100644 Documentation/devicetree/bindings/mfd/ampere,smpro.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml b/Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml
>> new file mode 100644
>> index 000000000000..fbf7ec754160
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml
>> @@ -0,0 +1,28 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/hwmon/ampere,ac01-hwmon.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Hardware monitoring driver for the Ampere Altra SMPro
>> +
>> +maintainers:
>> +  - Quan Nguyen <quan@os.amperecomputing.com>
>> +
>> +description: |
>> +  This module is part of the Ampere Altra SMPro multi-function device. For more
>> +  details see ../mfd/ampere,smpro.yaml.
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - ampere,ac01-hwmon
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +additionalProperties: false
>> diff --git a/Documentation/devicetree/bindings/mfd/ampere,smpro.yaml b/Documentation/devicetree/bindings/mfd/ampere,smpro.yaml
>> new file mode 100644
>> index 000000000000..5613c420869e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/ampere,smpro.yaml
>> @@ -0,0 +1,105 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/mfd/ampere,smpro.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Ampere Altra SMPro firmware driver
>> +
>> +maintainers:
>> +  - Quan Nguyen <quan@os.amperecomputing.com>
>> +
>> +description: |
>> +  Ampere Altra SMPro firmware may contain different blocks like hardware
>> +  monitoring, error monitoring and other miscellaneous features.
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - ampere,smpro
> 
> Again, not very specific. There's only 1 version of 'smpro' h/w or
> firmware? Are the firmware version and features discoverable? If not,
> you need to be more specific (or better yet, make them discoverable).
> 
Hi Rob,

So far, there's nothing to guarantee this is the only version of SMPro 
and neither firmware version nor features that are discoverable.

In fact, it was "ampere,ac01-smpro" specifically in my v1. But this is 
the "ampere,smpro" in arch/arm/boot/dts/nuvoton-npcm730-kudo.dts, that 
is why it got changed to "ampere,smpro" to avoid changes in that dts file.

I'm thinking about change it back to "ampere,ac01-smpro" in next version 
to make this compatible string more specific.

>> +
>> +  reg:
>> +    description:
>> +      I2C device address.
>> +    maxItems: 1
>> +
>> +  "#address-cells":
>> +    const: 1
>> +
>> +  "#size-cells":
>> +    const: 0
>> +
>> +patternProperties:
>> +  "^hwmon(@[0-9a-f]+)?$":
>> +    $ref: ../hwmon/ampere,ac01-hwmon.yaml
>> +
>> +  "^misc(@[0-9a-f]+)?$":
> 
> You don't need these child nodes in DT if there are no resources
> associated with them. The parent driver can instantiate all the
> sub-functions.
> 

 From v3, there is a "reg" property introduced for the child driver, 
especially for the misc driver. This is unavoidable because other 
properties might be introduced in future for other misc features.

>> +    type: object
>> +    description: |
>> +      This module is part of the Ampere Altra SMPro multi-function device
>> +      to support miscellaneous features
>> +    properties:
>> +      compatible:
>> +        enum:
>> +          - ampere,ac01-misc
>> +      reg:
>> +        maxItems: 1
>> +
>> +    required:
>> +      - compatible
>> +      - reg
>> +
>> +  "^errmon(@[0-9a-f]+)?$":
>> +    type: object
>> +    description: |
>> +      This module is part of the Ampere Altra SMPro multi-function device
>> +      that supports error monitoring feature.
>> +
>> +    properties:
>> +      compatible:
>> +        enum:
>> +          - ampere,ac01-errmon
>> +      reg:
>> +        maxItems: 1
>> +
>> +    required:
>> +      - compatible
>> +      - reg
>> +
>> +required:
>> +  - "#address-cells"
>> +  - "#size-cells"
>> +  - compatible
>> +  - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    i2c {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        smpro@4f {
>> +            compatible = "ampere,smpro";
>> +            reg = <0x4f>;
>> +            #address-cells = <1>;
>> +            #size-cells = <0>;
>> +
>> +            hwmon@10 {
>> +                compatible = "ampere,ac01-hwmon";
>> +                reg = <0x10>;
>> +            };
>> +
>> +            misc@b0 {
>> +                compatible = "ampere,ac01-misc";
>> +                reg = <0xb0>;
>> +            };
>> +
>> +            errmon@80 {
>> +                compatible = "ampere,ac01-errmon";
>> +                reg = <0x80>;
>> +            };
>> +
>> +        };
>> +    };
>> -- 
>> 2.28.0
>>

Re: [PATCH v3 1/4] dt-bindings: mfd: Add bindings for Ampere Altra SMPro drivers

[Quan Nguyen]

On 14/04/2021 15:28, Quan Nguyen wrote:
> On 13/04/2021 20:49, Rob Herring wrote:
>> On Fri, Apr 09, 2021 at 10:13:29AM +0700, Quan Nguyen wrote:
>>> Adds device tree bindings for SMPro drivers found on the Mt.Jade 
>>> hardware
>>> reference platform with Ampere's Altra Processor family.
>>>
>>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
>>> ---
>>>   .../bindings/hwmon/ampere,ac01-hwmon.yaml     |  28 +++++
>>>   .../devicetree/bindings/mfd/ampere,smpro.yaml | 105 ++++++++++++++++++
>>>   2 files changed, 133 insertions(+)
>>>   create mode 100644 
>>> Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml
>>>   create mode 100644 
>>> Documentation/devicetree/bindings/mfd/ampere,smpro.yaml
>>>
>>> diff --git 
>>> a/Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml 
>>> b/Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml
>>> new file mode 100644
>>> index 000000000000..fbf7ec754160
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml
>>> @@ -0,0 +1,28 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/hwmon/ampere,ac01-hwmon.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Hardware monitoring driver for the Ampere Altra SMPro
>>> +
>>> +maintainers:
>>> +  - Quan Nguyen <quan@os.amperecomputing.com>
>>> +
>>> +description: |
>>> +  This module is part of the Ampere Altra SMPro multi-function 
>>> device. For more
>>> +  details see ../mfd/ampere,smpro.yaml.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - ampere,ac01-hwmon
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +
>>> +additionalProperties: false
>>> diff --git a/Documentation/devicetree/bindings/mfd/ampere,smpro.yaml 
>>> b/Documentation/devicetree/bindings/mfd/ampere,smpro.yaml
>>> new file mode 100644
>>> index 000000000000..5613c420869e
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/mfd/ampere,smpro.yaml
>>> @@ -0,0 +1,105 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/mfd/ampere,smpro.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Ampere Altra SMPro firmware driver
>>> +
>>> +maintainers:
>>> +  - Quan Nguyen <quan@os.amperecomputing.com>
>>> +
>>> +description: |
>>> +  Ampere Altra SMPro firmware may contain different blocks like 
>>> hardware
>>> +  monitoring, error monitoring and other miscellaneous features.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - ampere,smpro
>>
>> Again, not very specific. There's only 1 version of 'smpro' h/w or
>> firmware? Are the firmware version and features discoverable? If not,
>> you need to be more specific (or better yet, make them discoverable).
>>
> Hi Rob,
> 
> So far, there's nothing to guarantee this is the only version of SMPro 
> and neither firmware version nor features that are discoverable.
> 
> In fact, it was "ampere,ac01-smpro" specifically in my v1. But this is 
> the "ampere,smpro" in arch/arm/boot/dts/nuvoton-npcm730-kudo.dts, that 
> is why it got changed to "ampere,smpro" to avoid changes in that dts file.
> 
> I'm thinking about change it back to "ampere,ac01-smpro" in next version 
> to make this compatible string more specific.
> 

Hi Rob, I have a second thought on this.

What MFD SMPro driver does is only to expose the register map for child 
drivers to use. Child drivers are capable to handle the specific details 
among SMPro version if necessary. Hence, even though we address the 
SMPro specific here (ie: by using specific compatible string), 
eventually, regardless of any version the SMPro might be, the MFD SMPro 
driver still just to expose the register map for its child drivers.

So, if that makes sense, I'd like to keep the "ampere,smpro" as 
compatible string.

- Quan

>>> +
>>> +  reg:
>>> +    description:
>>> +      I2C device address.
>>> +    maxItems: 1
>>> +
>>> +  "#address-cells":
>>> +    const: 1
>>> +
>>> +  "#size-cells":
>>> +    const: 0
>>> +
>>> +patternProperties:
>>> +  "^hwmon(@[0-9a-f]+)?$":
>>> +    $ref: ../hwmon/ampere,ac01-hwmon.yaml
>>> +
>>> +  "^misc(@[0-9a-f]+)?$":
>>
>> You don't need these child nodes in DT if there are no resources
>> associated with them. The parent driver can instantiate all the
>> sub-functions.
>>
> 
>  From v3, there is a "reg" property introduced for the child driver, 
> especially for the misc driver. This is unavoidable because other 
> properties might be introduced in future for other misc features.
> 
>>> +    type: object
>>> +    description: |
>>> +      This module is part of the Ampere Altra SMPro multi-function 
>>> device
>>> +      to support miscellaneous features
>>> +    properties:
>>> +      compatible:
>>> +        enum:
>>> +          - ampere,ac01-misc
>>> +      reg:
>>> +        maxItems: 1
>>> +
>>> +    required:
>>> +      - compatible
>>> +      - reg
>>> +
>>> +  "^errmon(@[0-9a-f]+)?$":
>>> +    type: object
>>> +    description: |
>>> +      This module is part of the Ampere Altra SMPro multi-function 
>>> device
>>> +      that supports error monitoring feature.
>>> +
>>> +    properties:
>>> +      compatible:
>>> +        enum:
>>> +          - ampere,ac01-errmon
>>> +      reg:
>>> +        maxItems: 1
>>> +
>>> +    required:
>>> +      - compatible
>>> +      - reg
>>> +
>>> +required:
>>> +  - "#address-cells"
>>> +  - "#size-cells"
>>> +  - compatible
>>> +  - reg
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    i2c {
>>> +        #address-cells = <1>;
>>> +        #size-cells = <0>;
>>> +
>>> +        smpro@4f {
>>> +            compatible = "ampere,smpro";
>>> +            reg = <0x4f>;
>>> +            #address-cells = <1>;
>>> +            #size-cells = <0>;
>>> +
>>> +            hwmon@10 {
>>> +                compatible = "ampere,ac01-hwmon";
>>> +                reg = <0x10>;
>>> +            };
>>> +
>>> +            misc@b0 {
>>> +                compatible = "ampere,ac01-misc";
>>> +                reg = <0xb0>;
>>> +            };
>>> +
>>> +            errmon@80 {
>>> +                compatible = "ampere,ac01-errmon";
>>> +                reg = <0x80>;
>>> +            };
>>> +
>>> +        };
>>> +    };
>>> -- 
>>> 2.28.0
>>>
> 

[PATCH v3 2/4] mfd: simple-mfd-i2c: Adds Ampere's Altra SMpro support

[Quan Nguyen]

Adds an MFD driver for SMpro found on the Mt.Jade hardware reference
platform with Ampere's Altra processor family.

Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
---
 drivers/mfd/Kconfig          | 10 ++++++++++
 drivers/mfd/simple-mfd-i2c.c |  6 ++++++
 2 files changed, 16 insertions(+)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index d07e8cf93286..f7a6460f7aa0 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -77,6 +77,16 @@ config MFD_AS3711
 	help
 	  Support for the AS3711 PMIC from AMS
 
+config MFD_SMPRO
+	tristate "Ampere Computing MFD SMpro core driver"
+	select MFD_SIMPLE_MFD_I2C
+	help
+	  Say yes here to enable SMpro driver support for Ampere's Altra
+	  processor family.
+
+	  Ampere's Altra SMpro exposes an I2C regmap interface that can
+	  be accessed by child devices.
+
 config MFD_AS3722
 	tristate "ams AS3722 Power Management IC"
 	select MFD_CORE
diff --git a/drivers/mfd/simple-mfd-i2c.c b/drivers/mfd/simple-mfd-i2c.c
index 87f684cff9a1..9a44655f5592 100644
--- a/drivers/mfd/simple-mfd-i2c.c
+++ b/drivers/mfd/simple-mfd-i2c.c
@@ -21,6 +21,11 @@ static const struct regmap_config simple_regmap_config = {
 	.val_bits = 8,
 };
 
+static const struct regmap_config simple_word_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+};
+
 static int simple_mfd_i2c_probe(struct i2c_client *i2c)
 {
 	const struct regmap_config *config;
@@ -39,6 +44,7 @@ static int simple_mfd_i2c_probe(struct i2c_client *i2c)
 
 static const struct of_device_id simple_mfd_i2c_of_match[] = {
 	{ .compatible = "kontron,sl28cpld" },
+	{ .compatible = "ampere,smpro", .data = &simple_word_regmap_config },
 	{}
 };
 MODULE_DEVICE_TABLE(of, simple_mfd_i2c_of_match);
-- 
2.28.0

Re: [PATCH v3 2/4] mfd: simple-mfd-i2c: Adds Ampere's Altra SMpro support

[Guenter Roeck]

On Fri, Apr 09, 2021 at 10:13:30AM +0700, Quan Nguyen wrote:
> Adds an MFD driver for SMpro found on the Mt.Jade hardware reference
> platform with Ampere's Altra processor family.
> 
> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
> Reported-by: kernel test robot <lkp@intel.com>
> ---
>  drivers/mfd/Kconfig          | 10 ++++++++++
>  drivers/mfd/simple-mfd-i2c.c |  6 ++++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index d07e8cf93286..f7a6460f7aa0 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -77,6 +77,16 @@ config MFD_AS3711
>  	help
>  	  Support for the AS3711 PMIC from AMS
>  
> +config MFD_SMPRO
> +	tristate "Ampere Computing MFD SMpro core driver"
> +	select MFD_SIMPLE_MFD_I2C

This is missing "depends on I2C".

> +	help
> +	  Say yes here to enable SMpro driver support for Ampere's Altra
> +	  processor family.
> +
> +	  Ampere's Altra SMpro exposes an I2C regmap interface that can
> +	  be accessed by child devices.
> +
>  config MFD_AS3722
>  	tristate "ams AS3722 Power Management IC"
>  	select MFD_CORE
> diff --git a/drivers/mfd/simple-mfd-i2c.c b/drivers/mfd/simple-mfd-i2c.c
> index 87f684cff9a1..9a44655f5592 100644
> --- a/drivers/mfd/simple-mfd-i2c.c
> +++ b/drivers/mfd/simple-mfd-i2c.c
> @@ -21,6 +21,11 @@ static const struct regmap_config simple_regmap_config = {
>  	.val_bits = 8,
>  };
>  
> +static const struct regmap_config simple_word_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +};
> +
>  static int simple_mfd_i2c_probe(struct i2c_client *i2c)
>  {
>  	const struct regmap_config *config;
> @@ -39,6 +44,7 @@ static int simple_mfd_i2c_probe(struct i2c_client *i2c)
>  
>  static const struct of_device_id simple_mfd_i2c_of_match[] = {
>  	{ .compatible = "kontron,sl28cpld" },
> +	{ .compatible = "ampere,smpro", .data = &simple_word_regmap_config },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, simple_mfd_i2c_of_match);

Re: [PATCH v3 2/4] mfd: simple-mfd-i2c: Adds Ampere's Altra SMpro support

[Quan Nguyen]

On 14/04/2021 19:21, Guenter Roeck wrote:
> On Fri, Apr 09, 2021 at 10:13:30AM +0700, Quan Nguyen wrote:
>> Adds an MFD driver for SMpro found on the Mt.Jade hardware reference
>> platform with Ampere's Altra processor family.
>>
>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
>> Reported-by: kernel test robot <lkp@intel.com>
>> ---
>>   drivers/mfd/Kconfig          | 10 ++++++++++
>>   drivers/mfd/simple-mfd-i2c.c |  6 ++++++
>>   2 files changed, 16 insertions(+)
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index d07e8cf93286..f7a6460f7aa0 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -77,6 +77,16 @@ config MFD_AS3711
>>   	help
>>   	  Support for the AS3711 PMIC from AMS
>>   
>> +config MFD_SMPRO
>> +	tristate "Ampere Computing MFD SMpro core driver"
>> +	select MFD_SIMPLE_MFD_I2C
> 
> This is missing "depends on I2C".
> 
Thanks Guenter to help pointing this out.
This fixes the issue reported by kernel test robot. Will include this in 
next version.

>> +	help
>> +	  Say yes here to enable SMpro driver support for Ampere's Altra
>> +	  processor family.
>> +
>> +	  Ampere's Altra SMpro exposes an I2C regmap interface that can
>> +	  be accessed by child devices.
>> +
>>   config MFD_AS3722
>>   	tristate "ams AS3722 Power Management IC"
>>   	select MFD_CORE
>> diff --git a/drivers/mfd/simple-mfd-i2c.c b/drivers/mfd/simple-mfd-i2c.c
>> index 87f684cff9a1..9a44655f5592 100644
>> --- a/drivers/mfd/simple-mfd-i2c.c
>> +++ b/drivers/mfd/simple-mfd-i2c.c
>> @@ -21,6 +21,11 @@ static const struct regmap_config simple_regmap_config = {
>>   	.val_bits = 8,
>>   };
>>   
>> +static const struct regmap_config simple_word_regmap_config = {
>> +	.reg_bits = 8,
>> +	.val_bits = 16,
>> +};
>> +
>>   static int simple_mfd_i2c_probe(struct i2c_client *i2c)
>>   {
>>   	const struct regmap_config *config;
>> @@ -39,6 +44,7 @@ static int simple_mfd_i2c_probe(struct i2c_client *i2c)
>>   
>>   static const struct of_device_id simple_mfd_i2c_of_match[] = {
>>   	{ .compatible = "kontron,sl28cpld" },
>> +	{ .compatible = "ampere,smpro", .data = &simple_word_regmap_config },
>>   	{}
>>   };
>>   MODULE_DEVICE_TABLE(of, simple_mfd_i2c_of_match);

[PATCH v3 3/4] hwmon: smpro: Add Ampere's Altra smpro-hwmon driver

[Quan Nguyen]

This commit adds support for Ampere SMpro hwmon driver. This driver
supports accessing various CPU sensors provided by the SMpro co-processor
including temperature, power, voltages, and current.

Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
---
 drivers/hwmon/Kconfig       |   8 +
 drivers/hwmon/Makefile      |   1 +
 drivers/hwmon/smpro-hwmon.c | 491 ++++++++++++++++++++++++++++++++++++
 3 files changed, 500 insertions(+)
 create mode 100644 drivers/hwmon/smpro-hwmon.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 0ddc974b102e..ba4b5a911baf 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -67,6 +67,14 @@ config SENSORS_ABITUGURU3
 	  This driver can also be built as a module. If so, the module
 	  will be called abituguru3.
 
+config SENSORS_SMPRO
+	tristate "Ampere's Altra SMpro hardware monitoring driver"
+	depends on MFD_SMPRO
+	help
+	  If you say yes here you get support for the thermal, voltage,
+	  current and power sensors of Ampere's Altra processor family SoC
+	  with SMpro co-processor.
+
 config SENSORS_AD7314
 	tristate "Analog Devices AD7314 and compatibles"
 	depends on SPI
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 59e78bc212cf..b25391f9c651 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -174,6 +174,7 @@ obj-$(CONFIG_SENSORS_SHT3x)	+= sht3x.o
 obj-$(CONFIG_SENSORS_SHTC1)	+= shtc1.o
 obj-$(CONFIG_SENSORS_SIS5595)	+= sis5595.o
 obj-$(CONFIG_SENSORS_SMM665)	+= smm665.o
+obj-$(CONFIG_SENSORS_SMPRO)	+= smpro-hwmon.o
 obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
 obj-$(CONFIG_SENSORS_SMSC47M1)	+= smsc47m1.o
 obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
diff --git a/drivers/hwmon/smpro-hwmon.c b/drivers/hwmon/smpro-hwmon.c
new file mode 100644
index 000000000000..a3389fcbad82
--- /dev/null
+++ b/drivers/hwmon/smpro-hwmon.c
@@ -0,0 +1,491 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Ampere Computing SoC's SMPro Hardware Monitoring Driver
+ *
+ * Copyright (c) 2021, Ampere Computing LLC
+ */
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+
+/* Identification Registers */
+#define MANUFACTURER_ID_REG	0x02
+#define AMPERE_MANUFACTURER_ID	0xCD3A
+
+/* Logical Power Sensor Registers */
+#define SOC_TEMP		0x00
+#define SOC_VRD_TEMP		0x01
+#define DIMM_VRD_TEMP		0x02
+#define CORE_VRD_TEMP		0x03
+#define CH0_DIMM_TEMP		0x04
+#define CH1_DIMM_TEMP		0x05
+#define CH2_DIMM_TEMP		0x06
+#define CH3_DIMM_TEMP		0x07
+#define CH4_DIMM_TEMP		0x08
+#define CH5_DIMM_TEMP		0x09
+#define CH6_DIMM_TEMP		0x0A
+#define CH7_DIMM_TEMP		0x0B
+#define RCA_VRD_TEMP		0x0C
+
+#define CORE_VRD_PWR		0x10
+#define SOC_PWR			0x11
+#define DIMM_VRD1_PWR		0x12
+#define DIMM_VRD2_PWR		0x13
+#define CORE_VRD_PWR_MW		0x16
+#define SOC_PWR_MW		0x17
+#define DIMM_VRD1_PWR_MW	0x18
+#define DIMM_VRD2_PWR_MW	0x19
+#define RCA_VRD_PWR		0x1A
+#define RCA_VRD_PWR_MW		0x1B
+
+#define MEM_HOT_THRESHOLD	0x22
+#define SOC_VR_HOT_THRESHOLD	0x23
+#define CORE_VRD_VOLT		0x24
+#define SOC_VRD_VOLT		0x25
+#define DIMM_VRD1_VOLT		0x26
+#define DIMM_VRD2_VOLT		0x27
+#define RCA_VRD_VOLT		0x28
+
+#define CORE_VRD_CURR		0x29
+#define SOC_VRD_CURR		0x2A
+#define DIMM_VRD1_CURR		0x2B
+#define DIMM_VRD2_CURR		0x2C
+#define RCA_VRD_CURR		0x2D
+
+struct smpro_hwmon {
+	struct regmap *regmap;
+	u32 offset;
+};
+
+struct smpro_sensor {
+	const u8 reg;
+	const u8 reg_ext;
+	const char *label;
+};
+
+static const struct smpro_sensor temperature[] = {
+	{
+		.reg = SOC_TEMP,
+		.label = "temp1 SoC"
+	},
+	{
+		.reg = SOC_VRD_TEMP,
+		.reg_ext = SOC_VR_HOT_THRESHOLD,
+		.label = "temp2 SoC VRD"
+	},
+	{
+		.reg = DIMM_VRD_TEMP,
+		.label = "temp3 DIMM VRD"
+	},
+	{
+		.reg = CORE_VRD_TEMP,
+		.label = "temp4 CORE VRD"
+	},
+	{
+		.reg = CH0_DIMM_TEMP,
+		.reg_ext = MEM_HOT_THRESHOLD,
+		.label = "temp5 CH0 DIMM"
+	},
+	{
+		.reg = CH1_DIMM_TEMP,
+		.reg_ext = MEM_HOT_THRESHOLD,
+		.label = "temp6 CH1 DIMM"
+	},
+	{
+		.reg = CH2_DIMM_TEMP,
+		.reg_ext = MEM_HOT_THRESHOLD,
+		.label = "temp7 CH2 DIMM"
+	},
+	{
+		.reg = CH3_DIMM_TEMP,
+		.reg_ext = MEM_HOT_THRESHOLD,
+		.label = "temp8 CH3 DIMM"
+	},
+	{
+		.reg = CH4_DIMM_TEMP,
+		.reg_ext = MEM_HOT_THRESHOLD,
+		.label = "temp9 CH4 DIMM"
+	},
+	{
+		.reg = CH5_DIMM_TEMP,
+		.reg_ext = MEM_HOT_THRESHOLD,
+		.label = "temp10 CH5 DIMM"
+	},
+	{
+		.reg = CH6_DIMM_TEMP,
+		.reg_ext = MEM_HOT_THRESHOLD,
+		.label = "temp11 CH6 DIMM"
+	},
+	{
+		.reg = CH7_DIMM_TEMP,
+		.reg_ext = MEM_HOT_THRESHOLD,
+		.label = "temp12 CH7 DIMM"
+	},
+	{
+		.reg = RCA_VRD_TEMP,
+		.label = "temp13 RCA VRD"
+	},
+};
+
+static const struct smpro_sensor voltage[] = {
+	{
+		.reg = CORE_VRD_VOLT,
+		.label = "vout0 CORE VRD"
+	},
+	{
+		.reg = SOC_VRD_VOLT,
+		.label = "vout1 SoC VRD"
+	},
+	{
+		.reg = DIMM_VRD1_VOLT,
+		.label = "vout2 DIMM VRD1"
+	},
+	{
+		.reg = DIMM_VRD2_VOLT,
+		.label = "vout3 DIMM VRD2"
+	},
+	{
+		.reg = RCA_VRD_VOLT,
+		.label = "vout4 RCA VRD"
+	},
+};
+
+static const struct smpro_sensor curr_sensor[] = {
+	{
+		.reg = CORE_VRD_CURR,
+		.label = "iout1 CORE VRD"
+	},
+	{
+		.reg = SOC_VRD_CURR,
+		.label = "iout2 SoC VRD"
+	},
+	{
+		.reg = DIMM_VRD1_CURR,
+		.label = "iout3 DIMM VRD1"
+	},
+	{
+		.reg = DIMM_VRD2_CURR,
+		.label = "iout4 DIMM VRD2"
+	},
+	{
+		.reg = RCA_VRD_CURR,
+		.label = "iout5 RCA VRD"
+	},
+};
+
+static const struct smpro_sensor power[] = {
+	{
+		.reg = CORE_VRD_PWR,
+		.reg_ext = CORE_VRD_PWR_MW,
+		.label = "power1 CORE VRD"
+	},
+	{
+		.reg = SOC_PWR,
+		.reg_ext = SOC_PWR_MW,
+		.label = "power2 SoC"
+	},
+	{
+		.reg = DIMM_VRD1_PWR,
+		.reg_ext = DIMM_VRD1_PWR_MW,
+		.label = "power3 DIMM VRD1"
+	},
+	{
+		.reg = DIMM_VRD2_PWR,
+		.reg_ext = DIMM_VRD2_PWR_MW,
+		.label = "power4 DIMM VRD2"
+	},
+	{
+		.reg = RCA_VRD_PWR,
+		.reg_ext = RCA_VRD_PWR_MW,
+		.label = "power5 RCA VRD"
+	},
+};
+
+static int smpro_read_temp(struct device *dev, u32 attr, int channel, long *val)
+{
+	struct smpro_hwmon *hwmon = dev_get_drvdata(dev);
+	unsigned int value;
+	int ret;
+
+	switch (attr) {
+	case hwmon_temp_input:
+		ret = regmap_read(hwmon->regmap, hwmon->offset + temperature[channel].reg, &value);
+		if (ret)
+			return ret;
+		break;
+	case hwmon_temp_crit:
+		ret = regmap_read(hwmon->regmap,
+				  hwmon->offset + temperature[channel].reg_ext, &value);
+		if (ret)
+			return ret;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	*val = sign_extend32(value, 8) * 1000;
+	return 0;
+}
+
+static int smpro_read_in(struct device *dev, u32 attr, int channel, long *val)
+{
+	struct smpro_hwmon *hwmon = dev_get_drvdata(dev);
+	unsigned int value;
+	int ret;
+
+	switch (attr) {
+	case hwmon_in_input:
+		ret = regmap_read(hwmon->regmap, hwmon->offset + voltage[channel].reg, &value);
+		if (ret < 0)
+			return ret;
+		/* 15-bit value in 1mV */
+		*val = value & 0x7fff;
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int smpro_read_curr(struct device *dev, u32 attr, int channel, long *val)
+{
+	struct smpro_hwmon *hwmon = dev_get_drvdata(dev);
+	unsigned int value;
+	int ret;
+
+	switch (attr) {
+	case hwmon_curr_input:
+		ret = regmap_read(hwmon->regmap, hwmon->offset + curr_sensor[channel].reg, &value);
+		if (ret < 0)
+			return ret;
+		/* Scale reported by the hardware is 1mA */
+		*val = value & 0x7fff;
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int smpro_read_power(struct device *dev, u32 attr, int channel, long *val_pwr)
+{
+	struct smpro_hwmon *hwmon = dev_get_drvdata(dev);
+	unsigned int val = 0, val_mw = 0;
+	int ret;
+
+	switch (attr) {
+	case hwmon_power_input:
+		ret = regmap_read(hwmon->regmap, hwmon->offset + power[channel].reg, &val);
+		if (ret)
+			return ret;
+
+		ret = regmap_read(hwmon->regmap, hwmon->offset + power[channel].reg_ext, &val_mw);
+		if (ret)
+			return ret;
+
+		*val_pwr = val * 1000000 + val_mw * 1000;
+		return 0;
+
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int smpro_read(struct device *dev, enum hwmon_sensor_types type,
+		      u32 attr, int channel, long *val)
+{
+	switch (type) {
+	case hwmon_temp:
+		return smpro_read_temp(dev, attr, channel, val);
+	case hwmon_in:
+		return smpro_read_in(dev, attr, channel, val);
+	case hwmon_power:
+		return smpro_read_power(dev, attr, channel, val);
+	case hwmon_curr:
+		return smpro_read_curr(dev, attr, channel, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int smpro_read_string(struct device *dev, enum hwmon_sensor_types type,
+			     u32 attr, int channel, const char **str)
+{
+	switch (type) {
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_label:
+			*str = temperature[channel].label;
+			return 0;
+		default:
+			break;
+		}
+		break;
+
+	case hwmon_in:
+		switch (attr) {
+		case hwmon_in_label:
+			*str = voltage[channel].label;
+			return 0;
+		default:
+			break;
+		}
+		break;
+
+	case hwmon_curr:
+		switch (attr) {
+		case hwmon_curr_label:
+			*str = curr_sensor[channel].label;
+			return 0;
+		default:
+			break;
+		}
+		break;
+
+	case hwmon_power:
+		switch (attr) {
+		case hwmon_power_label:
+			*str = power[channel].label;
+			return 0;
+		default:
+			break;
+		}
+		break;
+	default:
+		break;
+	}
+
+	return -EOPNOTSUPP;
+}
+
+static umode_t smpro_is_visible(const void *data, enum hwmon_sensor_types type,
+				u32 attr, int channel)
+{
+	const struct smpro_hwmon *hwmon = data;
+	unsigned int value;
+	int ret;
+
+	switch (type) {
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_input:
+		case hwmon_temp_label:
+		case hwmon_temp_crit:
+			ret = regmap_read(hwmon->regmap,
+					  hwmon->offset + temperature[channel].reg, &value);
+			if (ret || value == 0xFFFF)
+				return 0;
+		break;
+		}
+	default:
+		break;
+	}
+
+	return 0444;
+}
+
+static const struct hwmon_channel_info *smpro_info[] = {
+	HWMON_CHANNEL_INFO(temp,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_CRIT,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_CRIT,
+			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_CRIT,
+			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_CRIT,
+			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_CRIT,
+			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_CRIT,
+			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_CRIT,
+			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_CRIT,
+			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_CRIT,
+			   HWMON_T_INPUT | HWMON_T_LABEL),
+	HWMON_CHANNEL_INFO(in,
+			   HWMON_I_INPUT | HWMON_I_LABEL,
+			   HWMON_I_INPUT | HWMON_I_LABEL,
+			   HWMON_I_INPUT | HWMON_I_LABEL,
+			   HWMON_I_INPUT | HWMON_I_LABEL,
+			   HWMON_I_INPUT | HWMON_I_LABEL),
+	HWMON_CHANNEL_INFO(power,
+			   HWMON_P_INPUT | HWMON_P_LABEL,
+			   HWMON_P_INPUT | HWMON_P_LABEL,
+			   HWMON_P_INPUT | HWMON_P_LABEL,
+			   HWMON_P_INPUT | HWMON_P_LABEL,
+			   HWMON_P_INPUT | HWMON_P_LABEL),
+	HWMON_CHANNEL_INFO(curr,
+			   HWMON_C_INPUT | HWMON_C_LABEL,
+			   HWMON_C_INPUT | HWMON_C_LABEL,
+			   HWMON_C_INPUT | HWMON_C_LABEL,
+			   HWMON_C_INPUT | HWMON_C_LABEL,
+			   HWMON_C_INPUT | HWMON_C_LABEL),
+	NULL
+};
+
+static const struct hwmon_ops smpro_hwmon_ops = {
+	.is_visible = smpro_is_visible,
+	.read = smpro_read,
+	.read_string = smpro_read_string,
+};
+
+static const struct hwmon_chip_info smpro_chip_info = {
+	.ops = &smpro_hwmon_ops,
+	.info = smpro_info,
+};
+
+static int smpro_hwmon_probe(struct platform_device *pdev)
+{
+	struct smpro_hwmon *hwmon;
+	struct device *hwmon_dev;
+	unsigned int val;
+	int ret;
+
+	hwmon = devm_kzalloc(&pdev->dev, sizeof(struct smpro_hwmon), GFP_KERNEL);
+	if (!hwmon)
+		return -ENOMEM;
+
+	hwmon->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!hwmon->regmap)
+		return -ENODEV;
+
+	ret = device_property_read_u32(&pdev->dev, "reg", &hwmon->offset);
+	if (ret)
+		return -EINVAL;
+
+	/* Check for valid ID */
+	ret = regmap_read(hwmon->regmap, MANUFACTURER_ID_REG, &val);
+	if (ret)
+		return -EPROBE_DEFER;
+
+	if (val != AMPERE_MANUFACTURER_ID)
+		return -ENODEV;
+
+	hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev, "smpro_hwmon",
+							 hwmon, &smpro_chip_info, NULL);
+
+	return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static const struct of_device_id smpro_hwmon_of_match[] = {
+	{ .compatible = "ampere,ac01-hwmon" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, smpro_hwmon_of_match);
+
+static struct platform_driver smpro_hwmon_driver = {
+	.probe		= smpro_hwmon_probe,
+	.driver = {
+		.name	= "smpro-hwmon",
+		.of_match_table = smpro_hwmon_of_match,
+	},
+};
+
+module_platform_driver(smpro_hwmon_driver);
+
+MODULE_AUTHOR("Thu Nguyen <thu@os.amperecomputing.com>");
+MODULE_AUTHOR("Quan Nguyen <quan@os.amperecomputing.com>");
+MODULE_DESCRIPTION("Ampere Altra SMPro hwmon driver");
+MODULE_LICENSE("GPL v2");
-- 
2.28.0

Re: [PATCH v3 3/4] hwmon: smpro: Add Ampere's Altra smpro-hwmon driver

[Guenter Roeck]

On Fri, Apr 09, 2021 at 10:13:31AM +0700, Quan Nguyen wrote:
> This commit adds support for Ampere SMpro hwmon driver. This driver
> supports accessing various CPU sensors provided by the SMpro co-processor
> including temperature, power, voltages, and current.
> 
> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
> ---

Change log goes here. You are making it difficult to review your patches.

>  drivers/hwmon/Kconfig       |   8 +
>  drivers/hwmon/Makefile      |   1 +
>  drivers/hwmon/smpro-hwmon.c | 491 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 500 insertions(+)
>  create mode 100644 drivers/hwmon/smpro-hwmon.c
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 0ddc974b102e..ba4b5a911baf 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -67,6 +67,14 @@ config SENSORS_ABITUGURU3
>  	  This driver can also be built as a module. If so, the module
>  	  will be called abituguru3.
>  
> +config SENSORS_SMPRO
> +	tristate "Ampere's Altra SMpro hardware monitoring driver"
> +	depends on MFD_SMPRO
> +	help
> +	  If you say yes here you get support for the thermal, voltage,
> +	  current and power sensors of Ampere's Altra processor family SoC
> +	  with SMpro co-processor.
> +
>  config SENSORS_AD7314
>  	tristate "Analog Devices AD7314 and compatibles"
>  	depends on SPI
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 59e78bc212cf..b25391f9c651 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -174,6 +174,7 @@ obj-$(CONFIG_SENSORS_SHT3x)	+= sht3x.o
>  obj-$(CONFIG_SENSORS_SHTC1)	+= shtc1.o
>  obj-$(CONFIG_SENSORS_SIS5595)	+= sis5595.o
>  obj-$(CONFIG_SENSORS_SMM665)	+= smm665.o
> +obj-$(CONFIG_SENSORS_SMPRO)	+= smpro-hwmon.o
>  obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
>  obj-$(CONFIG_SENSORS_SMSC47M1)	+= smsc47m1.o
>  obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
> diff --git a/drivers/hwmon/smpro-hwmon.c b/drivers/hwmon/smpro-hwmon.c
> new file mode 100644
> index 000000000000..a3389fcbad82
> --- /dev/null
> +++ b/drivers/hwmon/smpro-hwmon.c
> @@ -0,0 +1,491 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Ampere Computing SoC's SMPro Hardware Monitoring Driver
> + *
> + * Copyright (c) 2021, Ampere Computing LLC
> + */
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/kernel.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +
> +/* Identification Registers */
> +#define MANUFACTURER_ID_REG	0x02
> +#define AMPERE_MANUFACTURER_ID	0xCD3A
> +
> +/* Logical Power Sensor Registers */
> +#define SOC_TEMP		0x00
> +#define SOC_VRD_TEMP		0x01
> +#define DIMM_VRD_TEMP		0x02
> +#define CORE_VRD_TEMP		0x03
> +#define CH0_DIMM_TEMP		0x04
> +#define CH1_DIMM_TEMP		0x05
> +#define CH2_DIMM_TEMP		0x06
> +#define CH3_DIMM_TEMP		0x07
> +#define CH4_DIMM_TEMP		0x08
> +#define CH5_DIMM_TEMP		0x09
> +#define CH6_DIMM_TEMP		0x0A
> +#define CH7_DIMM_TEMP		0x0B
> +#define RCA_VRD_TEMP		0x0C
> +
> +#define CORE_VRD_PWR		0x10
> +#define SOC_PWR			0x11
> +#define DIMM_VRD1_PWR		0x12
> +#define DIMM_VRD2_PWR		0x13
> +#define CORE_VRD_PWR_MW		0x16
> +#define SOC_PWR_MW		0x17
> +#define DIMM_VRD1_PWR_MW	0x18
> +#define DIMM_VRD2_PWR_MW	0x19
> +#define RCA_VRD_PWR		0x1A
> +#define RCA_VRD_PWR_MW		0x1B
> +
> +#define MEM_HOT_THRESHOLD	0x22
> +#define SOC_VR_HOT_THRESHOLD	0x23
> +#define CORE_VRD_VOLT		0x24
> +#define SOC_VRD_VOLT		0x25
> +#define DIMM_VRD1_VOLT		0x26
> +#define DIMM_VRD2_VOLT		0x27
> +#define RCA_VRD_VOLT		0x28
> +
> +#define CORE_VRD_CURR		0x29
> +#define SOC_VRD_CURR		0x2A
> +#define DIMM_VRD1_CURR		0x2B
> +#define DIMM_VRD2_CURR		0x2C
> +#define RCA_VRD_CURR		0x2D
> +
> +struct smpro_hwmon {
> +	struct regmap *regmap;
> +	u32 offset;
> +};
> +
> +struct smpro_sensor {
> +	const u8 reg;
> +	const u8 reg_ext;
> +	const char *label;
> +};
> +
> +static const struct smpro_sensor temperature[] = {
> +	{
> +		.reg = SOC_TEMP,
> +		.label = "temp1 SoC"
> +	},
> +	{
> +		.reg = SOC_VRD_TEMP,
> +		.reg_ext = SOC_VR_HOT_THRESHOLD,
> +		.label = "temp2 SoC VRD"
> +	},
> +	{
> +		.reg = DIMM_VRD_TEMP,
> +		.label = "temp3 DIMM VRD"
> +	},
> +	{
> +		.reg = CORE_VRD_TEMP,
> +		.label = "temp4 CORE VRD"
> +	},
> +	{
> +		.reg = CH0_DIMM_TEMP,
> +		.reg_ext = MEM_HOT_THRESHOLD,
> +		.label = "temp5 CH0 DIMM"
> +	},
> +	{
> +		.reg = CH1_DIMM_TEMP,
> +		.reg_ext = MEM_HOT_THRESHOLD,
> +		.label = "temp6 CH1 DIMM"
> +	},
> +	{
> +		.reg = CH2_DIMM_TEMP,
> +		.reg_ext = MEM_HOT_THRESHOLD,
> +		.label = "temp7 CH2 DIMM"
> +	},
> +	{
> +		.reg = CH3_DIMM_TEMP,
> +		.reg_ext = MEM_HOT_THRESHOLD,
> +		.label = "temp8 CH3 DIMM"
> +	},
> +	{
> +		.reg = CH4_DIMM_TEMP,
> +		.reg_ext = MEM_HOT_THRESHOLD,
> +		.label = "temp9 CH4 DIMM"
> +	},
> +	{
> +		.reg = CH5_DIMM_TEMP,
> +		.reg_ext = MEM_HOT_THRESHOLD,
> +		.label = "temp10 CH5 DIMM"
> +	},
> +	{
> +		.reg = CH6_DIMM_TEMP,
> +		.reg_ext = MEM_HOT_THRESHOLD,
> +		.label = "temp11 CH6 DIMM"
> +	},
> +	{
> +		.reg = CH7_DIMM_TEMP,
> +		.reg_ext = MEM_HOT_THRESHOLD,
> +		.label = "temp12 CH7 DIMM"
> +	},
> +	{
> +		.reg = RCA_VRD_TEMP,
> +		.label = "temp13 RCA VRD"
> +	},
> +};
> +
> +static const struct smpro_sensor voltage[] = {
> +	{
> +		.reg = CORE_VRD_VOLT,
> +		.label = "vout0 CORE VRD"
> +	},
> +	{
> +		.reg = SOC_VRD_VOLT,
> +		.label = "vout1 SoC VRD"
> +	},
> +	{
> +		.reg = DIMM_VRD1_VOLT,
> +		.label = "vout2 DIMM VRD1"
> +	},
> +	{
> +		.reg = DIMM_VRD2_VOLT,
> +		.label = "vout3 DIMM VRD2"
> +	},
> +	{
> +		.reg = RCA_VRD_VOLT,
> +		.label = "vout4 RCA VRD"
> +	},
> +};
> +
> +static const struct smpro_sensor curr_sensor[] = {
> +	{
> +		.reg = CORE_VRD_CURR,
> +		.label = "iout1 CORE VRD"
> +	},
> +	{
> +		.reg = SOC_VRD_CURR,
> +		.label = "iout2 SoC VRD"
> +	},
> +	{
> +		.reg = DIMM_VRD1_CURR,
> +		.label = "iout3 DIMM VRD1"
> +	},
> +	{
> +		.reg = DIMM_VRD2_CURR,
> +		.label = "iout4 DIMM VRD2"
> +	},
> +	{
> +		.reg = RCA_VRD_CURR,
> +		.label = "iout5 RCA VRD"
> +	},
> +};
> +
> +static const struct smpro_sensor power[] = {
> +	{
> +		.reg = CORE_VRD_PWR,
> +		.reg_ext = CORE_VRD_PWR_MW,
> +		.label = "power1 CORE VRD"
> +	},
> +	{
> +		.reg = SOC_PWR,
> +		.reg_ext = SOC_PWR_MW,
> +		.label = "power2 SoC"
> +	},
> +	{
> +		.reg = DIMM_VRD1_PWR,
> +		.reg_ext = DIMM_VRD1_PWR_MW,
> +		.label = "power3 DIMM VRD1"
> +	},
> +	{
> +		.reg = DIMM_VRD2_PWR,
> +		.reg_ext = DIMM_VRD2_PWR_MW,
> +		.label = "power4 DIMM VRD2"
> +	},
> +	{
> +		.reg = RCA_VRD_PWR,
> +		.reg_ext = RCA_VRD_PWR_MW,
> +		.label = "power5 RCA VRD"
> +	},
> +};
> +
> +static int smpro_read_temp(struct device *dev, u32 attr, int channel, long *val)
> +{
> +	struct smpro_hwmon *hwmon = dev_get_drvdata(dev);
> +	unsigned int value;
> +	int ret;
> +
> +	switch (attr) {
> +	case hwmon_temp_input:
> +		ret = regmap_read(hwmon->regmap, hwmon->offset + temperature[channel].reg, &value);
> +		if (ret)
> +			return ret;
> +		break;
> +	case hwmon_temp_crit:
> +		ret = regmap_read(hwmon->regmap,
> +				  hwmon->offset + temperature[channel].reg_ext, &value);
> +		if (ret)
> +			return ret;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	*val = sign_extend32(value, 8) * 1000;
> +	return 0;
> +}
> +
> +static int smpro_read_in(struct device *dev, u32 attr, int channel, long *val)
> +{
> +	struct smpro_hwmon *hwmon = dev_get_drvdata(dev);
> +	unsigned int value;
> +	int ret;
> +
> +	switch (attr) {
> +	case hwmon_in_input:
> +		ret = regmap_read(hwmon->regmap, hwmon->offset + voltage[channel].reg, &value);
> +		if (ret < 0)
> +			return ret;
> +		/* 15-bit value in 1mV */
> +		*val = value & 0x7fff;
> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int smpro_read_curr(struct device *dev, u32 attr, int channel, long *val)
> +{
> +	struct smpro_hwmon *hwmon = dev_get_drvdata(dev);
> +	unsigned int value;
> +	int ret;
> +
> +	switch (attr) {
> +	case hwmon_curr_input:
> +		ret = regmap_read(hwmon->regmap, hwmon->offset + curr_sensor[channel].reg, &value);
> +		if (ret < 0)
> +			return ret;
> +		/* Scale reported by the hardware is 1mA */
> +		*val = value & 0x7fff;
> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int smpro_read_power(struct device *dev, u32 attr, int channel, long *val_pwr)
> +{
> +	struct smpro_hwmon *hwmon = dev_get_drvdata(dev);
> +	unsigned int val = 0, val_mw = 0;
> +	int ret;
> +
> +	switch (attr) {
> +	case hwmon_power_input:
> +		ret = regmap_read(hwmon->regmap, hwmon->offset + power[channel].reg, &val);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_read(hwmon->regmap, hwmon->offset + power[channel].reg_ext, &val_mw);
> +		if (ret)
> +			return ret;
> +
> +		*val_pwr = val * 1000000 + val_mw * 1000;
> +		return 0;
> +
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int smpro_read(struct device *dev, enum hwmon_sensor_types type,
> +		      u32 attr, int channel, long *val)
> +{
> +	switch (type) {
> +	case hwmon_temp:
> +		return smpro_read_temp(dev, attr, channel, val);
> +	case hwmon_in:
> +		return smpro_read_in(dev, attr, channel, val);
> +	case hwmon_power:
> +		return smpro_read_power(dev, attr, channel, val);
> +	case hwmon_curr:
> +		return smpro_read_curr(dev, attr, channel, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int smpro_read_string(struct device *dev, enum hwmon_sensor_types type,
> +			     u32 attr, int channel, const char **str)
> +{
> +	switch (type) {
> +	case hwmon_temp:
> +		switch (attr) {
> +		case hwmon_temp_label:
> +			*str = temperature[channel].label;
> +			return 0;
> +		default:
> +			break;
> +		}
> +		break;
> +
> +	case hwmon_in:
> +		switch (attr) {
> +		case hwmon_in_label:
> +			*str = voltage[channel].label;
> +			return 0;
> +		default:
> +			break;
> +		}
> +		break;
> +
> +	case hwmon_curr:
> +		switch (attr) {
> +		case hwmon_curr_label:
> +			*str = curr_sensor[channel].label;
> +			return 0;
> +		default:
> +			break;
> +		}
> +		break;
> +
> +	case hwmon_power:
> +		switch (attr) {
> +		case hwmon_power_label:
> +			*str = power[channel].label;
> +			return 0;
> +		default:
> +			break;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static umode_t smpro_is_visible(const void *data, enum hwmon_sensor_types type,
> +				u32 attr, int channel)
> +{
> +	const struct smpro_hwmon *hwmon = data;
> +	unsigned int value;
> +	int ret;
> +
> +	switch (type) {
> +	case hwmon_temp:
> +		switch (attr) {
> +		case hwmon_temp_input:
> +		case hwmon_temp_label:
> +		case hwmon_temp_crit:
> +			ret = regmap_read(hwmon->regmap,
> +					  hwmon->offset + temperature[channel].reg, &value);
> +			if (ret || value == 0xFFFF)
> +				return 0;
> +		break;
> +		}
> +	default:
> +		break;
> +	}
> +
> +	return 0444;
> +}
> +
> +static const struct hwmon_channel_info *smpro_info[] = {
> +	HWMON_CHANNEL_INFO(temp,
> +			   HWMON_T_INPUT | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_CRIT,
> +			   HWMON_T_INPUT | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_CRIT,
> +			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_CRIT,
> +			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_CRIT,
> +			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_CRIT,
> +			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_CRIT,
> +			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_CRIT,
> +			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_CRIT,
> +			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_CRIT,
> +			   HWMON_T_INPUT | HWMON_T_LABEL),
> +	HWMON_CHANNEL_INFO(in,
> +			   HWMON_I_INPUT | HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_LABEL),
> +	HWMON_CHANNEL_INFO(power,
> +			   HWMON_P_INPUT | HWMON_P_LABEL,
> +			   HWMON_P_INPUT | HWMON_P_LABEL,
> +			   HWMON_P_INPUT | HWMON_P_LABEL,
> +			   HWMON_P_INPUT | HWMON_P_LABEL,
> +			   HWMON_P_INPUT | HWMON_P_LABEL),
> +	HWMON_CHANNEL_INFO(curr,
> +			   HWMON_C_INPUT | HWMON_C_LABEL,
> +			   HWMON_C_INPUT | HWMON_C_LABEL,
> +			   HWMON_C_INPUT | HWMON_C_LABEL,
> +			   HWMON_C_INPUT | HWMON_C_LABEL,
> +			   HWMON_C_INPUT | HWMON_C_LABEL),
> +	NULL
> +};
> +
> +static const struct hwmon_ops smpro_hwmon_ops = {
> +	.is_visible = smpro_is_visible,
> +	.read = smpro_read,
> +	.read_string = smpro_read_string,
> +};
> +
> +static const struct hwmon_chip_info smpro_chip_info = {
> +	.ops = &smpro_hwmon_ops,
> +	.info = smpro_info,
> +};
> +
> +static int smpro_hwmon_probe(struct platform_device *pdev)
> +{
> +	struct smpro_hwmon *hwmon;
> +	struct device *hwmon_dev;
> +	unsigned int val;
> +	int ret;
> +
> +	hwmon = devm_kzalloc(&pdev->dev, sizeof(struct smpro_hwmon), GFP_KERNEL);
> +	if (!hwmon)
> +		return -ENOMEM;
> +
> +	hwmon->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +	if (!hwmon->regmap)
> +		return -ENODEV;
> +
> +	ret = device_property_read_u32(&pdev->dev, "reg", &hwmon->offset);
> +	if (ret)
> +		return -EINVAL;
> +
> +	/* Check for valid ID */
> +	ret = regmap_read(hwmon->regmap, MANUFACTURER_ID_REG, &val);
> +	if (ret)
> +		return -EPROBE_DEFER;

This is unacceptable. regmap_read() could return various errors.
An error here might mean that the chip is simply not there, or that
there is some other problem with, say, the i2c controller driver.
The function might, for all we know, even return -ENOMEM.

If this is supposed to mean that the chip is not yet ready, some other
and more specific solution will be needed. -EPROBE_DEFER means that
some other driver needs to be loaded first, not "chip is not ready,
try again later".

Thanks,
Guenter

> +
> +	if (val != AMPERE_MANUFACTURER_ID)
> +		return -ENODEV;
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev, "smpro_hwmon",
> +							 hwmon, &smpro_chip_info, NULL);
> +
> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static const struct of_device_id smpro_hwmon_of_match[] = {
> +	{ .compatible = "ampere,ac01-hwmon" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, smpro_hwmon_of_match);
> +
> +static struct platform_driver smpro_hwmon_driver = {
> +	.probe		= smpro_hwmon_probe,
> +	.driver = {
> +		.name	= "smpro-hwmon",
> +		.of_match_table = smpro_hwmon_of_match,
> +	},
> +};
> +
> +module_platform_driver(smpro_hwmon_driver);
> +
> +MODULE_AUTHOR("Thu Nguyen <thu@os.amperecomputing.com>");
> +MODULE_AUTHOR("Quan Nguyen <quan@os.amperecomputing.com>");
> +MODULE_DESCRIPTION("Ampere Altra SMPro hwmon driver");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH v3 3/4] hwmon: smpro: Add Ampere's Altra smpro-hwmon driver

[Quan Nguyen]

On 14/04/2021 19:33, Guenter Roeck wrote:
> On Fri, Apr 09, 2021 at 10:13:31AM +0700, Quan Nguyen wrote:
>> This commit adds support for Ampere SMpro hwmon driver. This driver
>> supports accessing various CPU sensors provided by the SMpro co-processor
>> including temperature, power, voltages, and current.
>>
>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
>> ---
> 
> Change log goes here. You are making it difficult to review your patches.
> 
Will add changelog here in next version.

>>   drivers/hwmon/Kconfig       |   8 +
>>   drivers/hwmon/Makefile      |   1 +
>>   drivers/hwmon/smpro-hwmon.c | 491 ++++++++++++++++++++++++++++++++++++
>>   3 files changed, 500 insertions(+)
>>   create mode 100644 drivers/hwmon/smpro-hwmon.c
>>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 0ddc974b102e..ba4b5a911baf 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -67,6 +67,14 @@ config SENSORS_ABITUGURU3
>>   	  This driver can also be built as a module. If so, the module
>>   	  will be called abituguru3.
>>   
>> +config SENSORS_SMPRO
>> +	tristate "Ampere's Altra SMpro hardware monitoring driver"
>> +	depends on MFD_SMPRO
>> +	help
>> +	  If you say yes here you get support for the thermal, voltage,
>> +	  current and power sensors of Ampere's Altra processor family SoC
>> +	  with SMpro co-processor.
>> +
>>   config SENSORS_AD7314
>>   	tristate "Analog Devices AD7314 and compatibles"
>>   	depends on SPI
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index 59e78bc212cf..b25391f9c651 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -174,6 +174,7 @@ obj-$(CONFIG_SENSORS_SHT3x)	+= sht3x.o
>>   obj-$(CONFIG_SENSORS_SHTC1)	+= shtc1.o
>>   obj-$(CONFIG_SENSORS_SIS5595)	+= sis5595.o
>>   obj-$(CONFIG_SENSORS_SMM665)	+= smm665.o
>> +obj-$(CONFIG_SENSORS_SMPRO)	+= smpro-hwmon.o
>>   obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
>>   obj-$(CONFIG_SENSORS_SMSC47M1)	+= smsc47m1.o
>>   obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
>> diff --git a/drivers/hwmon/smpro-hwmon.c b/drivers/hwmon/smpro-hwmon.c
>> new file mode 100644
>> index 000000000000..a3389fcbad82
>> --- /dev/null
>> +++ b/drivers/hwmon/smpro-hwmon.c
>> @@ -0,0 +1,491 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Ampere Computing SoC's SMPro Hardware Monitoring Driver
>> + *
>> + * Copyright (c) 2021, Ampere Computing LLC
>> + */
>> +#include <linux/bitfield.h>
>> +#include <linux/bitops.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/hwmon-sysfs.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/property.h>
>> +#include <linux/regmap.h>
>> +
>> +/* Identification Registers */
>> +#define MANUFACTURER_ID_REG	0x02
>> +#define AMPERE_MANUFACTURER_ID	0xCD3A
>> +
>> +/* Logical Power Sensor Registers */
>> +#define SOC_TEMP		0x00
>> +#define SOC_VRD_TEMP		0x01
>> +#define DIMM_VRD_TEMP		0x02
>> +#define CORE_VRD_TEMP		0x03
>> +#define CH0_DIMM_TEMP		0x04
>> +#define CH1_DIMM_TEMP		0x05
>> +#define CH2_DIMM_TEMP		0x06
>> +#define CH3_DIMM_TEMP		0x07
>> +#define CH4_DIMM_TEMP		0x08
>> +#define CH5_DIMM_TEMP		0x09
>> +#define CH6_DIMM_TEMP		0x0A
>> +#define CH7_DIMM_TEMP		0x0B
>> +#define RCA_VRD_TEMP		0x0C
>> +
>> +#define CORE_VRD_PWR		0x10
>> +#define SOC_PWR			0x11
>> +#define DIMM_VRD1_PWR		0x12
>> +#define DIMM_VRD2_PWR		0x13
>> +#define CORE_VRD_PWR_MW		0x16
>> +#define SOC_PWR_MW		0x17
>> +#define DIMM_VRD1_PWR_MW	0x18
>> +#define DIMM_VRD2_PWR_MW	0x19
>> +#define RCA_VRD_PWR		0x1A
>> +#define RCA_VRD_PWR_MW		0x1B
>> +
>> +#define MEM_HOT_THRESHOLD	0x22
>> +#define SOC_VR_HOT_THRESHOLD	0x23
>> +#define CORE_VRD_VOLT		0x24
>> +#define SOC_VRD_VOLT		0x25
>> +#define DIMM_VRD1_VOLT		0x26
>> +#define DIMM_VRD2_VOLT		0x27
>> +#define RCA_VRD_VOLT		0x28
>> +
>> +#define CORE_VRD_CURR		0x29
>> +#define SOC_VRD_CURR		0x2A
>> +#define DIMM_VRD1_CURR		0x2B
>> +#define DIMM_VRD2_CURR		0x2C
>> +#define RCA_VRD_CURR		0x2D
>> +
>> +struct smpro_hwmon {
>> +	struct regmap *regmap;
>> +	u32 offset;
>> +};
>> +
>> +struct smpro_sensor {
>> +	const u8 reg;
>> +	const u8 reg_ext;
>> +	const char *label;
>> +};
>> +
>> +static const struct smpro_sensor temperature[] = {
>> +	{
>> +		.reg = SOC_TEMP,
>> +		.label = "temp1 SoC"
>> +	},
>> +	{
>> +		.reg = SOC_VRD_TEMP,
>> +		.reg_ext = SOC_VR_HOT_THRESHOLD,
>> +		.label = "temp2 SoC VRD"
>> +	},
>> +	{
>> +		.reg = DIMM_VRD_TEMP,
>> +		.label = "temp3 DIMM VRD"
>> +	},
>> +	{
>> +		.reg = CORE_VRD_TEMP,
>> +		.label = "temp4 CORE VRD"
>> +	},
>> +	{
>> +		.reg = CH0_DIMM_TEMP,
>> +		.reg_ext = MEM_HOT_THRESHOLD,
>> +		.label = "temp5 CH0 DIMM"
>> +	},
>> +	{
>> +		.reg = CH1_DIMM_TEMP,
>> +		.reg_ext = MEM_HOT_THRESHOLD,
>> +		.label = "temp6 CH1 DIMM"
>> +	},
>> +	{
>> +		.reg = CH2_DIMM_TEMP,
>> +		.reg_ext = MEM_HOT_THRESHOLD,
>> +		.label = "temp7 CH2 DIMM"
>> +	},
>> +	{
>> +		.reg = CH3_DIMM_TEMP,
>> +		.reg_ext = MEM_HOT_THRESHOLD,
>> +		.label = "temp8 CH3 DIMM"
>> +	},
>> +	{
>> +		.reg = CH4_DIMM_TEMP,
>> +		.reg_ext = MEM_HOT_THRESHOLD,
>> +		.label = "temp9 CH4 DIMM"
>> +	},
>> +	{
>> +		.reg = CH5_DIMM_TEMP,
>> +		.reg_ext = MEM_HOT_THRESHOLD,
>> +		.label = "temp10 CH5 DIMM"
>> +	},
>> +	{
>> +		.reg = CH6_DIMM_TEMP,
>> +		.reg_ext = MEM_HOT_THRESHOLD,
>> +		.label = "temp11 CH6 DIMM"
>> +	},
>> +	{
>> +		.reg = CH7_DIMM_TEMP,
>> +		.reg_ext = MEM_HOT_THRESHOLD,
>> +		.label = "temp12 CH7 DIMM"
>> +	},
>> +	{
>> +		.reg = RCA_VRD_TEMP,
>> +		.label = "temp13 RCA VRD"
>> +	},
>> +};
>> +
>> +static const struct smpro_sensor voltage[] = {
>> +	{
>> +		.reg = CORE_VRD_VOLT,
>> +		.label = "vout0 CORE VRD"
>> +	},
>> +	{
>> +		.reg = SOC_VRD_VOLT,
>> +		.label = "vout1 SoC VRD"
>> +	},
>> +	{
>> +		.reg = DIMM_VRD1_VOLT,
>> +		.label = "vout2 DIMM VRD1"
>> +	},
>> +	{
>> +		.reg = DIMM_VRD2_VOLT,
>> +		.label = "vout3 DIMM VRD2"
>> +	},
>> +	{
>> +		.reg = RCA_VRD_VOLT,
>> +		.label = "vout4 RCA VRD"
>> +	},
>> +};
>> +
>> +static const struct smpro_sensor curr_sensor[] = {
>> +	{
>> +		.reg = CORE_VRD_CURR,
>> +		.label = "iout1 CORE VRD"
>> +	},
>> +	{
>> +		.reg = SOC_VRD_CURR,
>> +		.label = "iout2 SoC VRD"
>> +	},
>> +	{
>> +		.reg = DIMM_VRD1_CURR,
>> +		.label = "iout3 DIMM VRD1"
>> +	},
>> +	{
>> +		.reg = DIMM_VRD2_CURR,
>> +		.label = "iout4 DIMM VRD2"
>> +	},
>> +	{
>> +		.reg = RCA_VRD_CURR,
>> +		.label = "iout5 RCA VRD"
>> +	},
>> +};
>> +
>> +static const struct smpro_sensor power[] = {
>> +	{
>> +		.reg = CORE_VRD_PWR,
>> +		.reg_ext = CORE_VRD_PWR_MW,
>> +		.label = "power1 CORE VRD"
>> +	},
>> +	{
>> +		.reg = SOC_PWR,
>> +		.reg_ext = SOC_PWR_MW,
>> +		.label = "power2 SoC"
>> +	},
>> +	{
>> +		.reg = DIMM_VRD1_PWR,
>> +		.reg_ext = DIMM_VRD1_PWR_MW,
>> +		.label = "power3 DIMM VRD1"
>> +	},
>> +	{
>> +		.reg = DIMM_VRD2_PWR,
>> +		.reg_ext = DIMM_VRD2_PWR_MW,
>> +		.label = "power4 DIMM VRD2"
>> +	},
>> +	{
>> +		.reg = RCA_VRD_PWR,
>> +		.reg_ext = RCA_VRD_PWR_MW,
>> +		.label = "power5 RCA VRD"
>> +	},
>> +};
>> +
>> +static int smpro_read_temp(struct device *dev, u32 attr, int channel, long *val)
>> +{
>> +	struct smpro_hwmon *hwmon = dev_get_drvdata(dev);
>> +	unsigned int value;
>> +	int ret;
>> +
>> +	switch (attr) {
>> +	case hwmon_temp_input:
>> +		ret = regmap_read(hwmon->regmap, hwmon->offset + temperature[channel].reg, &value);
>> +		if (ret)
>> +			return ret;
>> +		break;
>> +	case hwmon_temp_crit:
>> +		ret = regmap_read(hwmon->regmap,
>> +				  hwmon->offset + temperature[channel].reg_ext, &value);
>> +		if (ret)
>> +			return ret;
>> +		break;
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	*val = sign_extend32(value, 8) * 1000;
>> +	return 0;
>> +}
>> +
>> +static int smpro_read_in(struct device *dev, u32 attr, int channel, long *val)
>> +{
>> +	struct smpro_hwmon *hwmon = dev_get_drvdata(dev);
>> +	unsigned int value;
>> +	int ret;
>> +
>> +	switch (attr) {
>> +	case hwmon_in_input:
>> +		ret = regmap_read(hwmon->regmap, hwmon->offset + voltage[channel].reg, &value);
>> +		if (ret < 0)
>> +			return ret;
>> +		/* 15-bit value in 1mV */
>> +		*val = value & 0x7fff;
>> +		return 0;
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +}
>> +
>> +static int smpro_read_curr(struct device *dev, u32 attr, int channel, long *val)
>> +{
>> +	struct smpro_hwmon *hwmon = dev_get_drvdata(dev);
>> +	unsigned int value;
>> +	int ret;
>> +
>> +	switch (attr) {
>> +	case hwmon_curr_input:
>> +		ret = regmap_read(hwmon->regmap, hwmon->offset + curr_sensor[channel].reg, &value);
>> +		if (ret < 0)
>> +			return ret;
>> +		/* Scale reported by the hardware is 1mA */
>> +		*val = value & 0x7fff;
>> +		return 0;
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +}
>> +
>> +static int smpro_read_power(struct device *dev, u32 attr, int channel, long *val_pwr)
>> +{
>> +	struct smpro_hwmon *hwmon = dev_get_drvdata(dev);
>> +	unsigned int val = 0, val_mw = 0;
>> +	int ret;
>> +
>> +	switch (attr) {
>> +	case hwmon_power_input:
>> +		ret = regmap_read(hwmon->regmap, hwmon->offset + power[channel].reg, &val);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = regmap_read(hwmon->regmap, hwmon->offset + power[channel].reg_ext, &val_mw);
>> +		if (ret)
>> +			return ret;
>> +
>> +		*val_pwr = val * 1000000 + val_mw * 1000;
>> +		return 0;
>> +
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +}
>> +
>> +static int smpro_read(struct device *dev, enum hwmon_sensor_types type,
>> +		      u32 attr, int channel, long *val)
>> +{
>> +	switch (type) {
>> +	case hwmon_temp:
>> +		return smpro_read_temp(dev, attr, channel, val);
>> +	case hwmon_in:
>> +		return smpro_read_in(dev, attr, channel, val);
>> +	case hwmon_power:
>> +		return smpro_read_power(dev, attr, channel, val);
>> +	case hwmon_curr:
>> +		return smpro_read_curr(dev, attr, channel, val);
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +}
>> +
>> +static int smpro_read_string(struct device *dev, enum hwmon_sensor_types type,
>> +			     u32 attr, int channel, const char **str)
>> +{
>> +	switch (type) {
>> +	case hwmon_temp:
>> +		switch (attr) {
>> +		case hwmon_temp_label:
>> +			*str = temperature[channel].label;
>> +			return 0;
>> +		default:
>> +			break;
>> +		}
>> +		break;
>> +
>> +	case hwmon_in:
>> +		switch (attr) {
>> +		case hwmon_in_label:
>> +			*str = voltage[channel].label;
>> +			return 0;
>> +		default:
>> +			break;
>> +		}
>> +		break;
>> +
>> +	case hwmon_curr:
>> +		switch (attr) {
>> +		case hwmon_curr_label:
>> +			*str = curr_sensor[channel].label;
>> +			return 0;
>> +		default:
>> +			break;
>> +		}
>> +		break;
>> +
>> +	case hwmon_power:
>> +		switch (attr) {
>> +		case hwmon_power_label:
>> +			*str = power[channel].label;
>> +			return 0;
>> +		default:
>> +			break;
>> +		}
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return -EOPNOTSUPP;
>> +}
>> +
>> +static umode_t smpro_is_visible(const void *data, enum hwmon_sensor_types type,
>> +				u32 attr, int channel)
>> +{
>> +	const struct smpro_hwmon *hwmon = data;
>> +	unsigned int value;
>> +	int ret;
>> +
>> +	switch (type) {
>> +	case hwmon_temp:
>> +		switch (attr) {
>> +		case hwmon_temp_input:
>> +		case hwmon_temp_label:
>> +		case hwmon_temp_crit:
>> +			ret = regmap_read(hwmon->regmap,
>> +					  hwmon->offset + temperature[channel].reg, &value);
>> +			if (ret || value == 0xFFFF)
>> +				return 0;
>> +		break;
>> +		}
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return 0444;
>> +}
>> +
>> +static const struct hwmon_channel_info *smpro_info[] = {
>> +	HWMON_CHANNEL_INFO(temp,
>> +			   HWMON_T_INPUT | HWMON_T_LABEL,
>> +			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_CRIT,
>> +			   HWMON_T_INPUT | HWMON_T_LABEL,
>> +			   HWMON_T_INPUT | HWMON_T_LABEL,
>> +			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_CRIT,
>> +			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_CRIT,
>> +			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_CRIT,
>> +			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_CRIT,
>> +			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_CRIT,
>> +			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_CRIT,
>> +			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_CRIT,
>> +			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_CRIT,
>> +			   HWMON_T_INPUT | HWMON_T_LABEL),
>> +	HWMON_CHANNEL_INFO(in,
>> +			   HWMON_I_INPUT | HWMON_I_LABEL,
>> +			   HWMON_I_INPUT | HWMON_I_LABEL,
>> +			   HWMON_I_INPUT | HWMON_I_LABEL,
>> +			   HWMON_I_INPUT | HWMON_I_LABEL,
>> +			   HWMON_I_INPUT | HWMON_I_LABEL),
>> +	HWMON_CHANNEL_INFO(power,
>> +			   HWMON_P_INPUT | HWMON_P_LABEL,
>> +			   HWMON_P_INPUT | HWMON_P_LABEL,
>> +			   HWMON_P_INPUT | HWMON_P_LABEL,
>> +			   HWMON_P_INPUT | HWMON_P_LABEL,
>> +			   HWMON_P_INPUT | HWMON_P_LABEL),
>> +	HWMON_CHANNEL_INFO(curr,
>> +			   HWMON_C_INPUT | HWMON_C_LABEL,
>> +			   HWMON_C_INPUT | HWMON_C_LABEL,
>> +			   HWMON_C_INPUT | HWMON_C_LABEL,
>> +			   HWMON_C_INPUT | HWMON_C_LABEL,
>> +			   HWMON_C_INPUT | HWMON_C_LABEL),
>> +	NULL
>> +};
>> +
>> +static const struct hwmon_ops smpro_hwmon_ops = {
>> +	.is_visible = smpro_is_visible,
>> +	.read = smpro_read,
>> +	.read_string = smpro_read_string,
>> +};
>> +
>> +static const struct hwmon_chip_info smpro_chip_info = {
>> +	.ops = &smpro_hwmon_ops,
>> +	.info = smpro_info,
>> +};
>> +
>> +static int smpro_hwmon_probe(struct platform_device *pdev)
>> +{
>> +	struct smpro_hwmon *hwmon;
>> +	struct device *hwmon_dev;
>> +	unsigned int val;
>> +	int ret;
>> +
>> +	hwmon = devm_kzalloc(&pdev->dev, sizeof(struct smpro_hwmon), GFP_KERNEL);
>> +	if (!hwmon)
>> +		return -ENOMEM;
>> +
>> +	hwmon->regmap = dev_get_regmap(pdev->dev.parent, NULL);
>> +	if (!hwmon->regmap)
>> +		return -ENODEV;
>> +
>> +	ret = device_property_read_u32(&pdev->dev, "reg", &hwmon->offset);
>> +	if (ret)
>> +		return -EINVAL;
>> +
>> +	/* Check for valid ID */
>> +	ret = regmap_read(hwmon->regmap, MANUFACTURER_ID_REG, &val);
>> +	if (ret)
>> +		return -EPROBE_DEFER;
> 
> This is unacceptable. regmap_read() could return various errors.
> An error here might mean that the chip is simply not there, or that
> there is some other problem with, say, the i2c controller driver.
> The function might, for all we know, even return -ENOMEM.
> 
> If this is supposed to mean that the chip is not yet ready, some other
> and more specific solution will be needed. -EPROBE_DEFER means that
> some other driver needs to be loaded first, not "chip is not ready,
> try again later".
> 
> Thanks,
> Guenter
> 

Thanks to this clarification and will return regmap_read() return value, 
ie: "return ret" in next version.

>> +
>> +	if (val != AMPERE_MANUFACTURER_ID)
>> +		return -ENODEV;
>> +
>> +	hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev, "smpro_hwmon",
>> +							 hwmon, &smpro_chip_info, NULL);
>> +
>> +	return PTR_ERR_OR_ZERO(hwmon_dev);
>> +}
>> +
>> +static const struct of_device_id smpro_hwmon_of_match[] = {
>> +	{ .compatible = "ampere,ac01-hwmon" },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, smpro_hwmon_of_match);
>> +
>> +static struct platform_driver smpro_hwmon_driver = {
>> +	.probe		= smpro_hwmon_probe,
>> +	.driver = {
>> +		.name	= "smpro-hwmon",
>> +		.of_match_table = smpro_hwmon_of_match,
>> +	},
>> +};
>> +
>> +module_platform_driver(smpro_hwmon_driver);
>> +
>> +MODULE_AUTHOR("Thu Nguyen <thu@os.amperecomputing.com>");
>> +MODULE_AUTHOR("Quan Nguyen <quan@os.amperecomputing.com>");
>> +MODULE_DESCRIPTION("Ampere Altra SMPro hwmon driver");
>> +MODULE_LICENSE("GPL v2");

[PATCH v3 4/4] docs: hwmon: (smpro-hwmon) Add documentation

[Quan Nguyen]

Add documentation for the Ampere(R)'s Altra(R) SMpro hwmon driver.

Signed-off-by: Thu Nguyen <thu@os.amperecomputing.com>
Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
---
 Documentation/hwmon/index.rst       |   1 +
 Documentation/hwmon/smpro-hwmon.rst | 101 ++++++++++++++++++++++++++++
 2 files changed, 102 insertions(+)
 create mode 100644 Documentation/hwmon/smpro-hwmon.rst

diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 48bfa7887dd4..3e3631b253b6 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -166,6 +166,7 @@ Hardware Monitoring Kernel Drivers
    sis5595
    sl28cpld
    smm665
+   smpro-hwmon
    smsc47b397
    smsc47m192
    smsc47m1
diff --git a/Documentation/hwmon/smpro-hwmon.rst b/Documentation/hwmon/smpro-hwmon.rst
new file mode 100644
index 000000000000..f978b1370e16
--- /dev/null
+++ b/Documentation/hwmon/smpro-hwmon.rst
@@ -0,0 +1,101 @@
+.. SPDX-License-Identifier: GPL-2.0-or-later
+
+Kernel driver Ampere(R)'s Altra(R) SMpro hwmon
+==============================================
+
+Supported chips:
+
+  * Ampere(R) Altra(R)
+
+    Prefix: 'smpro'
+
+    Reference: Altra SoC BMC Interface Specification
+
+Author: Thu Nguyen <thu@os.amperecomputing.com>
+
+Description
+-----------
+This driver supports hardware monitoring for Ampere(R) Altra(R) SoC's based on the
+SMpro co-processor (SMpro).
+The following sensor types are supported by the driver:
+
+  * temperature
+  * voltage
+  * current
+  * power
+
+The SMpro interface provides the registers to query the various sensors and
+their values which are then exported to userspace by this driver.
+
+Usage Notes
+-----------
+
+SMpro hwmon driver creates at least two sysfs files for each sensor.
+
+* File ``<sensor_type><idx>_label`` reports the sensor label.
+* File ``<sensor_type><idx>_input`` returns the sensor value.
+
+The sysfs files are allocated in the SMpro root fs folder.
+There is one root folder for each SMpro instance.
+
+When the SoC is turned off, the driver will fail to read registers
+and return -ENXIO.
+
+Sysfs entries
+-------------
+
+The following sysfs files are supported:
+
+* Ampere(R) Altra(R):
+
+============    =============   ======  ===============================================
+Name            Unit            Perm    Description
+temp1_input     milli Celsius   RO      SoC temperature
+temp2_input     milli Celsius   RO      Max temperature reported among SoC VRDs
+temp2_crit      milli Celsius   RO      SoC VRD HOT Threshold temperature
+temp3_input     milli Celsius   RO      Max temperature reported among DIMM VRDs
+temp4_input     milli Celsius   RO      Max temperature reported among Core VRDs
+temp5_input     milli Celsius   RO      Temperature of DIMM0 on CH0
+temp5_crit      milli Celsius   RO      MEM HOT Threshold for all DIMMs
+temp6_input     milli Celsius   RO      Temperature of DIMM0 on CH1
+temp6_crit      milli Celsius   RO      MEM HOT Threshold for all DIMMs
+temp7_input     milli Celsius   RO      Temperature of DIMM0 on CH2
+temp7_crit      milli Celsius   RO      MEM HOT Threshold for all DIMMs
+temp8_input     milli Celsius   RO      Temperature of DIMM0 on CH3
+temp8_crit      milli Celsius   RO      MEM HOT Threshold for all DIMMs
+temp9_input     milli Celsius   RO      Temperature of DIMM0 on CH4
+temp9_crit      milli Celsius   RO      MEM HOT Threshold for all DIMMs
+temp10_input    milli Celsius   RO      Temperature of DIMM0 on CH5
+temp10_crit     milli Celsius   RO      MEM HOT Threshold for all DIMMs
+temp11_input    milli Celsius   RO      Temperature of DIMM0 on CH6
+temp11_crit     milli Celsius   RO      MEM HOT Threshold for all DIMMs
+temp12_input    milli Celsius   RO      Temperature of DIMM0 on CH7
+temp12_crit     milli Celsius   RO      MEM HOT Threshold for all DIMMs
+temp13_input    milli Celsius   RO      Max temperature reported among RCA VRDs
+in0_input       milli Volts     RO      Core voltage
+in1_input       milli Volts     RO      SoC voltage
+in2_input       milli Volts     RO      DIMM VRD1 voltage
+in3_input       milli Volts     RO      DIMM VRD2 voltage
+in4_input       milli Volts     RO      RCA VRD voltage
+cur1_input      milli Amperes   RO      Core VRD current
+cur2_input      milli Amperes   RO      SoC VRD current
+cur3_input      milli Amperes   RO      DIMM VRD1 current
+cur4_input      milli Amperes   RO      DIMM VRD2 current
+cur5_input      milli Amperes   RO      RCA VRD current
+power1_input    micro Watts     RO      Core VRD power
+power2_input    micro Watts     RO      SoC VRD power
+power3_input    micro Watts     RO      DIMM VRD1 power
+power4_input    micro Watts     RO      DIMM VRD2 power
+power5_input    micro Watts     RO      RCA VRD power
+============    =============   ======  ===============================================
+
+Example::
+
+    # cat in0_input
+    830
+    # cat temp1_input
+    37000
+    # cat curr1_input
+    9000
+    # cat power5_input
+    19500000
-- 
2.28.0