Re: [PATCH v6 2/2] drivers: hwmon: sophgo: Add SG2042 external hardware monitor support

[Guenter Roeck]

On 7/8/24 15:15, Inochi Amaoto wrote:
> On Mon, Jul 08, 2024 at 03:11:37PM GMT, Chen Wang wrote:
> > On 2024/7/8 8:53, Inochi Amaoto wrote:
> > > On Mon, Jul 08, 2024 at 08:25:55AM GMT, Chen Wang wrote:
> > > > On 2024/7/3 10:30, Inochi Amaoto wrote:
> > > > > SG2042 use an external MCU to provide basic hardware information
> > > > > and thermal sensors.
> > > > >
> > > > > Add driver support for the onboard MCU of SG2042.
> > > > >
> > > > > Signed-off-by: Inochi Amaoto <inochiama@xxxxxxxxxxx>
> > > > > ---
> > > > >     Documentation/hwmon/index.rst |   1 +
> > > > >     Documentation/hwmon/sgmcu.rst |  44 +++
> > > > >     drivers/hwmon/Kconfig         |  11 +
> > > > >     drivers/hwmon/Makefile        |   1 +
> > > > >     drivers/hwmon/sgmcu.c         | 585 ++++++++++++++++++++++++++++++++++
> > > > >     5 files changed, 642 insertions(+)
> > > > >     create mode 100644 Documentation/hwmon/sgmcu.rst
> > > > >     create mode 100644 drivers/hwmon/sgmcu.c
> > > > >
> > > > > diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> > > > > index 03d313af469a..189626b3a055 100644
> > > > > --- a/Documentation/hwmon/index.rst
> > > > > +++ b/Documentation/hwmon/index.rst
> > > > > @@ -203,6 +203,7 @@ Hardware Monitoring Kernel Drivers
> > > > >        sch5636
> > > > >        scpi-hwmon
> > > > >        sfctemp
> > > > > +   sgmcu
> > > > This driver is for sg2042 only, right? "sgmcu" looks be general for all
> > > > sophgo products.
> > > Yes, according to sophgo, it use this mechanism for multiple products,
> > > so I switch to a general name.
> >
> > But multiple != ALL.
> >
> > [......]
>
> We can add new driver when there is new mechanism.

Now you are contradicting yourself. Either sgmcu is the catch-all
driver, or it isn't. How are you going to call that new driver ? sgmcuv2 ?
Are we going to have sgmcuv[2-N] over time ?

All we know so far is that the driver and the mcu support sg2042. That is how the
driver should be named. It is easier to add support a new device with a different
name to the existing driver than to add a new driver if the name of an existing driver
is too generic.

Ultimately this is similar to wildcards in a file name, which are strongly discouraged.
One of the worst examples is drivers/hwmon/ina2xx.c, which does _not_ support all chips
from ina200 to ina299. Please don't let us go there.

An opposite example is the lm90 driver, which has not problem supporting more than 40
different chips with different names because they are all similar. The driver can be named
sg2042 and support as many similar variants if that mcu as feasible. It should not be named
sgmcu because we can not make the assumption that it will support all mcu variants from
sophgo.

Guenter