Re: [PATCH v1 3/3] misc: Add meta cld driver

[Greg Kroah-Hartman]

On Mon, Mar 13, 2023 at 08:47:45AM +0000, Delphine_CC_Chiu/WYHQ/Wiwynn wrote:
> Hi Greg,
> 
> Thanks for your comment!
> 
> > -----Original Message-----
> > From: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Sent: Tuesday, January 17, 2023 6:15 PM
> > To: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@xxxxxxxxxx>
> > Cc: patrick@xxxxxxxxx; Derek Kiernan <derek.kiernan@xxxxxxxxxx>; Dragan
> > Cvetic <dragan.cvetic@xxxxxxxxxx>; Arnd Bergmann <arnd@xxxxxxxx>;
> > garnermic@xxxxxx; Rob Herring <robh+dt@xxxxxxxxxx>; Krzysztof Kozlowski
> > <krzysztof.kozlowski+dt@xxxxxxxxxx>; Stanislav Jakubek
> > <stano.jakubek@xxxxxxxxx>; Linus Walleij <linus.walleij@xxxxxxxxxx>; Samuel
> > Holland <samuel@xxxxxxxxxxxx>; linux-i2c@xxxxxxxxxxxxxxx;
> > devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> > Subject: Re: [PATCH v1 3/3] misc: Add meta cld driver
> > 
> >   Security Reminder: Please be aware that this email is sent by an external
> > sender.
> > 
> > On Tue, Jan 17, 2023 at 05:44:22PM +0800, Delphine CC Chiu wrote:
> > > Add support for meta control-logic-device driver. The CLD manages the
> > > server system power squence and other state such as host-power-state,
> > > uart-selection and presense-slots. The baseboard management controller
> > > (BMC) can access the CLD through I2C.
> > >
> > > The version 1 of CLD driver is supported. The registers number, name
> > > and mode of CLD can be defined in dts file for version 1. The driver
> > > exports the filesystem following the dts setting.
> > >
> > > Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@xxxxxxxxxx>
> > > Tested-by: Bonnie Lo <Bonnie_Lo@xxxxxxxxxx>
> > > ---
> > >  MAINTAINERS                         |   6 +
> > >  drivers/misc/Kconfig                |   9 +
> > >  drivers/misc/Makefile               |   1 +
> > >  drivers/misc/control-logic-device.c | 443
> > > ++++++++++++++++++++++++++++
> > 
> > That is a very generic name for a very specific driver.  Please make it more
> > hardware-specific.
> 
> In server project, there is a component (control-logic-device). This component manages the server status including whole system power sequence, status and other devices presence status. And baseboard management controller (BMC) on server can acquire the information from CLD device through I2C.
> Currently, our customer plan to follow the spec to design the computing server. 
> We would like to change the naming from CLD to "compute CPLD".
> Do you have any suggestion?

Make it something hardware/vendor specific please.  As is, this is very
generic.  Remember, this is a name you will be using to refer to for the
next 20+ years.

> > Also, you add a bunch of undocumented sysfs files here, which require a
> > Documentation/ABI/ entries so that we can review the code to verify it does
> > what you all think it does.
> 
> We will add the document in Documentation/ABI/testing folder.
> 
> > 
> > And finally, why is this needed to be a kernel driver at all?  Why can't you
> > control this all through the userspace i2c api?
> 
> After discussing with our customer, they prefer the userspace access the physical device through driver not I2C API.
> There is an example on the OpenBMC Gerrit.
> https://gerrit.openbmc.org/c/openbmc/phosphor-buttons/+/60807

I do not understand, if functionalty can be done in userspace, it should
be done there, UNLESS you have a generic way of handling multiple
hardware devices as the same type (i.e. keyboard, sensor, etc.)  There
does not seem to be any generic interface here, so again, why can't you
just do it all in userspace?  What is forcing a kernel driver for this?

> > One review comment:
> > 
> > > +static int cld_remove(struct i2c_client *client) {
> > > +     struct device *dev = &client->dev;
> > > +     struct cld_client *cld = dev_get_drvdata(dev);
> > > +
> > > +     if (cld->task) {
> > > +             kthread_stop(cld->task);
> > > +             cld->task = NULL;
> > > +     }
> > > +
> > > +     devm_kfree(dev, cld);
> > 
> > Whenever you see this line in code, it's almost always a huge red flag that
> > someone is not using the devm_* api properly.  I think that is most certainly
> > the case here.
> 
> Do you mean the dev_free function is no need in this remove function?

Why do you think it is needed here?  If it is needed, please document it
with a comment explaining why this is required as it is not how the api
is designed to be used at all.

thanks,

greg k-h