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

[Linus Walleij]

Hi Delphine,

thanks for your patch!

On Tue, Jan 17, 2023 at 10:46 AM Delphine CC Chiu
<Delphine_CC_Chiu@xxxxxxxxxx> 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>

Why should this driver be in drivers/misc and not drivers/mfd?
MFS has support code for spawning child devices for the LED
you are also creating for example, so please use that.

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

Yours,
Linus Walleij