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

[Inochi Amaoto]

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 at outlook.com>
> > > > Reviewed-by: Conor Dooley <conor.dooley at 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 at 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