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

[Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>]

Hi Linus Walleij,

Thanks for your review comment.

> -----Original Message-----
> From: Linus Walleij <linus.walleij@linaro.org>
> Sent: Tuesday, January 17, 2023 7:40 PM
> To: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>
> Cc: patrick@stwcx.xyz; Derek Kiernan <derek.kiernan@xilinx.com>; Dragan
> Cvetic <dragan.cvetic@xilinx.com>; Arnd Bergmann <arnd@arndb.de>; Greg
> Kroah-Hartman <gregkh@linuxfoundation.org>; garnermic@fb.com; Rob
> Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Stanislav Jakubek
> <stano.jakubek@gmail.com>; Samuel Holland <samuel@sholland.org>;
> linux-i2c@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; Lee Jones <lee@kernel.org>; Sebastian Reichel
> <sre@kernel.org>
> 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.
> 
> Hi Delphine,
> 
> thanks for your patch!
> 
> On Tue, Jan 17, 2023 at 10:46 AM Delphine CC Chiu
> <Delphine_CC_Chiu@wiwynn.com> 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@Wiwynn.com>
> > Tested-by: Bonnie Lo <Bonnie_Lo@Wiwynn.com>
> 
> Why should this driver be in drivers/misc and not drivers/mfd?

The cld device is not a physical ASIC. It's a controller based on FPGA device and the FPGA may be Altera or Lattice.
So, we put the cld driver in misc folder. Is the cld driver suitable to put in mfd folder?

> MFS has support code for spawning child devices for the LED you are also
> creating for example, so please use that.

Could you please guide us which device driver we can refer?

> 
> > +#include <linux/sysfs.h>
> (...)
> > +#include <linux/kthread.h>
> (...)
> 
> > +static ssize_t cld_register_read(struct file *flip, struct kobject *kobj,
> > +                                struct bin_attribute *attr, char *buf,
> > +                                loff_t pos, size_t count) {
> (...)
> > +       snprintf(buf, sizeof(value), "%d\n", value);
> (...)
> > +static ssize_t cld_register_write(struct file *flip, struct kobject *kobj,
> > +                                 struct bin_attribute *attr, char
> *buf,
> > +                                 loff_t pos, size_t count) {
> > +       ret = kstrtoul(buf, 0, &val);
> (...)
> 
> Writing and reading some random regmap registers is something that the
> regmap debugfs already can do.
> 
> > +static int cld_bin_register(struct cld_register_info info,
> > +                           struct cld_client *cld) {
> 
> And this is for reading and writing binary blobs.
> 
> It looks like something that should be using the firmware API.
> 
> If the purpose of the driver is to open a hole from userspace down to the
> hardware, as Greg says why not just use userspace I2C then?
> 
> It seems a bit dangerous to relay whatever the ASIC is doing to userspace
> though.
> 
> Are you sure you can't use any of the existing kernel functionality for doing
> what these userspace "hole" is doing?
> 
> There is drivers/power etc for power control and I bet it can be extended if
> need be.

We will discuss with our customer.

> 
> Yours,
> Linus Walleij

Thanks,
Delphine