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

[Guenter Roeck]

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@xxxxxxxxxxx>
> > > Reviewed-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
> > > ---
> > >   .../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@xxxxxxxxxxx>
> > > +
> > > +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