Re: [PATCH] char: add driver for mps VR controller mp2891

["gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>]

On Tue, Mar 14, 2023 at 09:18:50AM +0000, Noah (Wensheng) Wang wrote:
> Hi Arnd, Grey:
> Thanks for the review.
> 
> This driver will be used by facebook. This driver provide a device node for userspace to get output voltage, input voltage, input current, input power, output power and temperature of mp2891 controller through I2C. This driver determine what kind of value the userspace wants through the mp2891_write interface and return the corresponding value when the interface mp2891_read is called.

Note, can you please take a look at the kernel documentation for how to
write a good changelog text when you resubmit this?

> 
> Signed-off-by: Noah Wang <Noah.Wang@monolithicpower.com>
> ---
>  drivers/char/mp2891.c | 403 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 403 insertions(+)
>  create mode 100644 drivers/char/mp2891.c

You didn't add the driver to the build, so it can not actually be used
at all.  How did you test this?

> 
> diff --git a/drivers/char/mp2891.c b/drivers/char/mp2891.c new file mode 100644 index 000000000000..84529b73f065
> --- /dev/null
> +++ b/drivers/char/mp2891.c
> @@ -0,0 +1,403 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later

Are you _sure_ you mean "or later"?  (I have to ask)

> +/*
> + * Driver for MPS Multi-phase Digital VR Controllers(MP2891)
> + *
> + * Copyright (C) 2023 MPS
> + */
> +
> +#include <linux/types.h>
> +#include <linux/kernel.h>
> +#include <linux/delay.h>
> +#include <linux/ide.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/errno.h>
> +#include <linux/gpio.h>
> +#include <linux/cdev.h>
> +#include <linux/device.h>
> +#include <linux/of_gpio.h>
> +#include <linux/semaphore.h>
> +#include <linux/timer.h>
> +#include <linux/i2c.h>
> +#include <linux/uaccess.h>
> +#include <linux/io.h>
> +#include <asm/mach/map.h>
> +
> +#define PMBUS_PAGE              0x00
> +#define MFR_VOUT_LOOP_CTRL_R1   0xBD
> +#define MFR_VOUT_LOOP_CTRL_R2   0xBD
> +
> +#define VID_STEP_POS            14
> +#define VID_STEP_MSK            (0x3 << VID_STEP_POS)
> +
> +#define READ_VIN                0x88
> +#define READ_VOUT               0x8B
> +#define READ_IOUT               0x8C
> +#define READ_TEMPERATURE        0x8D
> +#define READ_PIN_EST_PMBUS_R1   0x94
> +#define READ_PIN_EST_PMBUS_R2   0x94
> +#define READ_POUT_PMBUS_R1      0x96
> +#define READ_POUT_PMBUS_R2      0x96
> +
> +#define MP2891_PAGE_NUM			2
> +
> +#define MP2891_CNT 1
> +#define MP2891_NAME "mp2891"
> +
> +#define IOUT_PAGE0          "IOUT-0"
> +#define IOUT_PAGE1          "IOUT-1"
> +#define VOUT_PAGE0          "VOUT-0"
> +#define VOUT_PAGE1          "VOUT-1"
> +#define TEMPERATURE_PAGE0   "TEMPERATURE-0"
> +#define TEMPERATURE_PAGE1   "TEMPERATURE-1"
> +#define VIN_PAGE0           "VIN-0"
> +#define PIN_EST_PAGE0		"PIN_EST-0"
> +#define PIN_EST_PAGE1		"PIN_EST-1"
> +#define POUT_PAGE0          "POUT-0"
> +#define POUT_PAGE1          "POUT-1"
> +
> +struct mp2891_data {
> +	int vid_step[MP2891_PAGE_NUM];
> +};
> +
> +struct mp2891_dev {
> +	dev_t devid;
> +	struct cdev cdev;
> +	struct class *class;
> +	struct device *device;
> +	int major;
> +	char read_flag[20];
> +	struct i2c_client *client;
> +	struct mp2891_data *data;
> +};
> +
> +struct mp2891_dev mp2891cdev;

You really do not want to do this, the driver should be able to handle
any number of devices in the system, not just one.  Also, this is a
dynamic structure that you just made static, which is going to be
interesting when it comes to properly memory lifetime rules, right?

> +
> +static int read_word_data(struct i2c_client *client, int page, int reg) 

Always run scripts/checkpatch.pl on your code before submitting it so
you don't get grumpy reviewers asking why you didn't run
scripts/checkpatch.pl on your code before submitting it.

thanks,

greg k-h