| 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 |
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
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
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
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 --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>; + }; + };