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

[Delphine_CC_Chiu/WYHQ/Wiwynn]

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?

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

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

> 
> thanks,
> 
> greg k-h

Thanks,
Delphine