diff mbox series

[v6,1/2] dt-bindings: hwmon: Add Sophgo SG2042 external hardware monitor support

Message ID IA1PR20MB4953F58B631D836F3863115ABBDD2@IA1PR20MB4953.namprd20.prod.outlook.com (mailing list archive)
State Changes Requested
Headers show
Series riscv: sophgo: Add SG2042 external hardware monitor support | expand

Commit Message

Inochi Amaoto July 3, 2024, 2:30 a.m. UTC
Due to the design, Sophgo SG2042 use an external MCU to provide
hardware information, thermal information and reset control.

Add bindings for this monitor device.

Signed-off-by: Inochi Amaoto <inochiama@outlook.com>
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../hwmon/sophgo,sg2042-hwmon-mcu.yaml        | 43 +++++++++++++++++++
 1 file changed, 43 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/sophgo,sg2042-hwmon-mcu.yaml

--
2.45.2

Comments

Guenter Roeck July 6, 2024, 4:48 p.m. UTC | #1
On Wed, Jul 03, 2024 at 10:30:43AM +0800, Inochi Amaoto wrote:
> Due to the design, Sophgo SG2042 use an external MCU to provide
> hardware information, thermal information and reset control.
> 
> Add bindings for this monitor device.
> 
> Signed-off-by: Inochi Amaoto <inochiama@outlook.com>
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  .../hwmon/sophgo,sg2042-hwmon-mcu.yaml        | 43 +++++++++++++++++++
>  1 file changed, 43 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/sophgo,sg2042-hwmon-mcu.yaml
> 
> --
> 2.45.2
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/sophgo,sg2042-hwmon-mcu.yaml b/Documentation/devicetree/bindings/hwmon/sophgo,sg2042-hwmon-mcu.yaml
> new file mode 100644
> index 000000000000..f0667ac41d75
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/sophgo,sg2042-hwmon-mcu.yaml
> @@ -0,0 +1,43 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/sophgo,sg2042-hwmon-mcu.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sophgo SG2042 onboard MCU support
> +
> +maintainers:
> +  - Inochi Amaoto <inochiama@outlook.com>
> +
> +properties:
> +  compatible:
> +    const: sophgo,sg2042-hwmon-mcu

According to the other patch, this actually covers four
distinct models/devices.

static const struct sg2042_mcu_board_data sg2042_boards_data[] = {
> +	{
> +		.id = 0x80,
> +		.name = "SG2042 evb x8",
> +	},
> +	{
> +		.id = 0x81,
> +		.name = "SG2042R evb",
> +	},
> +	{
> +		.id = 0x83,
> +		.name = "SG2042 evb x4",
> +	},
> +	{
> +		.id = 0x90,
> +		.name = "Milk-V Pioneer",
> +	},
> +};
> +

Is it really appropriate to use a single compatible property for all of those ?

Guenter
Inochi Amaoto July 6, 2024, 11:34 p.m. UTC | #2
On Sat, Jul 06, 2024 at 09:48:58AM GMT, Guenter Roeck wrote:
> On Wed, Jul 03, 2024 at 10:30:43AM +0800, Inochi Amaoto wrote:
> > Due to the design, Sophgo SG2042 use an external MCU to provide
> > hardware information, thermal information and reset control.
> > 
> > Add bindings for this monitor device.
> > 
> > Signed-off-by: Inochi Amaoto <inochiama@outlook.com>
> > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> > ---
> >  .../hwmon/sophgo,sg2042-hwmon-mcu.yaml        | 43 +++++++++++++++++++
> >  1 file changed, 43 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/hwmon/sophgo,sg2042-hwmon-mcu.yaml
> > 
> > --
> > 2.45.2
> > 
> > diff --git a/Documentation/devicetree/bindings/hwmon/sophgo,sg2042-hwmon-mcu.yaml b/Documentation/devicetree/bindings/hwmon/sophgo,sg2042-hwmon-mcu.yaml
> > new file mode 100644
> > index 000000000000..f0667ac41d75
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/sophgo,sg2042-hwmon-mcu.yaml
> > @@ -0,0 +1,43 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/hwmon/sophgo,sg2042-hwmon-mcu.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Sophgo SG2042 onboard MCU support
> > +
> > +maintainers:
> > +  - Inochi Amaoto <inochiama@outlook.com>
> > +
> > +properties:
> > +  compatible:
> > +    const: sophgo,sg2042-hwmon-mcu
> 
> According to the other patch, this actually covers four
> distinct models/devices.
> 
> static const struct sg2042_mcu_board_data sg2042_boards_data[] = {
> > +	{
> > +		.id = 0x80,
> > +		.name = "SG2042 evb x8",
> > +	},
> > +	{
> > +		.id = 0x81,
> > +		.name = "SG2042R evb",
> > +	},
> > +	{
> > +		.id = 0x83,
> > +		.name = "SG2042 evb x4",
> > +	},
> > +	{
> > +		.id = 0x90,
> > +		.name = "Milk-V Pioneer",
> > +	},
> > +};
> > +
> 
> Is it really appropriate to use a single compatible property for all of those ?
> 
> Guenter

These board can only be detected at running time (even this should on
a specific board). On real world, it can only sees a MCU onboard.
I don't think it is a good idea to add some bindings to cover these
model. It seems better to remove this array and let userspace to parse
these ids.

Regards,
Inochi
Guenter Roeck July 7, 2024, 1:35 a.m. UTC | #3
On 7/6/24 16:34, Inochi Amaoto wrote:
> On Sat, Jul 06, 2024 at 09:48:58AM GMT, Guenter Roeck wrote:
>> On Wed, Jul 03, 2024 at 10:30:43AM +0800, Inochi Amaoto wrote:
>>> Due to the design, Sophgo SG2042 use an external MCU to provide
>>> hardware information, thermal information and reset control.
>>>
>>> Add bindings for this monitor device.
>>>
>>> Signed-off-by: Inochi Amaoto <inochiama@outlook.com>
>>> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>>> ---
>>>   .../hwmon/sophgo,sg2042-hwmon-mcu.yaml        | 43 +++++++++++++++++++
>>>   1 file changed, 43 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/hwmon/sophgo,sg2042-hwmon-mcu.yaml
>>>
>>> --
>>> 2.45.2
>>>
>>> diff --git a/Documentation/devicetree/bindings/hwmon/sophgo,sg2042-hwmon-mcu.yaml b/Documentation/devicetree/bindings/hwmon/sophgo,sg2042-hwmon-mcu.yaml
>>> new file mode 100644
>>> index 000000000000..f0667ac41d75
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/hwmon/sophgo,sg2042-hwmon-mcu.yaml
>>> @@ -0,0 +1,43 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/hwmon/sophgo,sg2042-hwmon-mcu.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Sophgo SG2042 onboard MCU support
>>> +
>>> +maintainers:
>>> +  - Inochi Amaoto <inochiama@outlook.com>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: sophgo,sg2042-hwmon-mcu
>>
>> According to the other patch, this actually covers four
>> distinct models/devices.
>>
>> static const struct sg2042_mcu_board_data[] = {
>>> +	{
>>> +		.id = 0x80,
>>> +		.name = "SG2042 evb x8",
>>> +	},
>>> +	{
>>> +		.id = 0x81,
>>> +		.name = "SG2042R evb",
>>> +	},
>>> +	{
>>> +		.id = 0x83,
>>> +		.name = "SG2042 evb x4",
>>> +	},
>>> +	{
>>> +		.id = 0x90,
>>> +		.name = "Milk-V Pioneer",
>>> +	},
>>> +};
>>> +
>>
>> Is it really appropriate to use a single compatible property for all of those ?
>>
>> Guenter
> 
> These board can only be detected at running time (even this should on
> a specific board). On real world, it can only sees a MCU onboard.
> I don't think it is a good idea to add some bindings to cover these
> model. It seems better to remove this array and let userspace to parse
> these ids.
> 

Isn't that what devicetree is for ? It should either not be necessary
to distinguish the models because they all behave the same and all of them
are happy with the same compatible property, or there is some difference
which should be reflected in devicetree. In other words, either they
are all compatible with sophgo,sg2042-hwmon-mcu, and sg2042_mcu_board_data
as well as sg2042_mcu_check_board() are unnecessary, or they are not
compatible and there should be separate compatible property names.
struct of_device_data does have a .data pointer for a reason, after all.

In yet other words, it seems odd to have a devicetree file for Milk-V Pioneer
and then to check in the hwmon driver if this is _really_ a Milk-V Pioneer
and bail out if it isn't. And I _really_ don't want to deal with continuous
driver patches whenever one of the systems using one of those MCUs starts
shipping a new firmware version or is deployed on a new board variant.

Thanks,
Guenter
Inochi Amaoto July 7, 2024, 5:45 a.m. UTC | #4
On Sat, Jul 06, 2024 at 06:35:35PM GMT, Guenter Roeck wrote:
> On 7/6/24 16:34, Inochi Amaoto wrote:
> > On Sat, Jul 06, 2024 at 09:48:58AM GMT, Guenter Roeck wrote:
> > > On Wed, Jul 03, 2024 at 10:30:43AM +0800, Inochi Amaoto wrote:
> > > > Due to the design, Sophgo SG2042 use an external MCU to provide
> > > > hardware information, thermal information and reset control.
> > > > 
> > > > Add bindings for this monitor device.
> > > > 
> > > > Signed-off-by: Inochi Amaoto <inochiama@outlook.com>
> > > > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> > > > ---
> > > >   .../hwmon/sophgo,sg2042-hwmon-mcu.yaml        | 43 +++++++++++++++++++
> > > >   1 file changed, 43 insertions(+)
> > > >   create mode 100644 Documentation/devicetree/bindings/hwmon/sophgo,sg2042-hwmon-mcu.yaml
> > > > 
> > > > --
> > > > 2.45.2
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/hwmon/sophgo,sg2042-hwmon-mcu.yaml b/Documentation/devicetree/bindings/hwmon/sophgo,sg2042-hwmon-mcu.yaml
> > > > new file mode 100644
> > > > index 000000000000..f0667ac41d75
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/hwmon/sophgo,sg2042-hwmon-mcu.yaml
> > > > @@ -0,0 +1,43 @@
> > > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/hwmon/sophgo,sg2042-hwmon-mcu.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Sophgo SG2042 onboard MCU support
> > > > +
> > > > +maintainers:
> > > > +  - Inochi Amaoto <inochiama@outlook.com>
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    const: sophgo,sg2042-hwmon-mcu
> > > 
> > > According to the other patch, this actually covers four
> > > distinct models/devices.
> > > 
> > > static const struct sg2042_mcu_board_data[] = {
> > > > +	{
> > > > +		.id = 0x80,
> > > > +		.name = "SG2042 evb x8",
> > > > +	},
> > > > +	{
> > > > +		.id = 0x81,
> > > > +		.name = "SG2042R evb",
> > > > +	},
> > > > +	{
> > > > +		.id = 0x83,
> > > > +		.name = "SG2042 evb x4",
> > > > +	},
> > > > +	{
> > > > +		.id = 0x90,
> > > > +		.name = "Milk-V Pioneer",
> > > > +	},
> > > > +};
> > > > +
> > > 
> > > Is it really appropriate to use a single compatible property for all of those ?
> > > 
> > > Guenter
> > 
> > These board can only be detected at running time (even this should on
> > a specific board). On real world, it can only sees a MCU onboard.
> > I don't think it is a good idea to add some bindings to cover these
> > model. It seems better to remove this array and let userspace to parse
> > these ids.
> > 
> 
> Isn't that what devicetree is for ? It should either not be necessary
> to distinguish the models because they all behave the same and all of them
> are happy with the same compatible property, or there is some difference
> which should be reflected in devicetree. In other words, either they
> are all compatible with sophgo,sg2042-hwmon-mcu, and sg2042_mcu_board_data
> as well as sg2042_mcu_check_board() are unnecessary, or they are not
> compatible and there should be separate compatible property names.
> struct of_device_data does have a .data pointer for a reason, after all.
> 
> In yet other words, it seems odd to have a devicetree file for Milk-V Pioneer
> and then to check in the hwmon driver if this is _really_ a Milk-V Pioneer
> and bail out if it isn't. And I _really_ don't want to deal with continuous
> driver patches whenever one of the systems using one of those MCUs starts
> shipping a new firmware version or is deployed on a new board variant.
> 
> Thanks,
> Guenter
> 

When I firstly added this compatible, I want to match all boards with this
SoC. This is why I add compatiable like "sg2042-xxx". In driver, I tested
the id of the MCU to ensure it is sg2042 (although the id can used to detect
board). And this is why I said that I was not sure it is better to add board
specific compatible.

I know you concern about continuous patches about new board or new firmware
version. I was told the MCU use almost the same register layout to provide
necessary information about its SoC. So is it better to remove the id check
and let the dts write to decide?

Regards,
Inochi
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/hwmon/sophgo,sg2042-hwmon-mcu.yaml b/Documentation/devicetree/bindings/hwmon/sophgo,sg2042-hwmon-mcu.yaml
new file mode 100644
index 000000000000..f0667ac41d75
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/sophgo,sg2042-hwmon-mcu.yaml
@@ -0,0 +1,43 @@ 
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/sophgo,sg2042-hwmon-mcu.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sophgo SG2042 onboard MCU support
+
+maintainers:
+  - Inochi Amaoto <inochiama@outlook.com>
+
+properties:
+  compatible:
+    const: sophgo,sg2042-hwmon-mcu
+
+  reg:
+    maxItems: 1
+
+  "#thermal-sensor-cells":
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - "#thermal-sensor-cells"
+
+allOf:
+  - $ref: /schemas/thermal/thermal-sensor.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        hwmon@17 {
+            compatible = "sophgo,sg2042-hwmon-mcu";
+            reg = <0x17>;
+            #thermal-sensor-cells = <1>;
+        };
+    };