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

[Inochi Amaoto]

On Mon, Jul 08, 2024 at 06:42:14PM GMT, Guenter Roeck wrote:
> 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 ?
> 

No, I mean new mechanism that does not use MCU. For example, the chip 
cv1800b and the incoming sg2380, these chip have different mechanism. 
For all chip with the MCU, I want to keep only one driver.

> 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.

Thanks, this is hole I don't consider. I have made an assumption that 
this driver only serves for sophgo products (especially products with
this MCU mechanism), which is too limited. Now let me restrict it as
sg2042 specific and evolve it in the future.

Regards,
Inochi

> 
> Guenter
>