On Wed, Feb 15, 2023 at 08:26:12PM +0300, andybeg wrote: Subject should be something like: "Add driver for MAX34409" checkpatch reports: total: 0 errors, 8 warnings, 12 checks, 256 lines checked Please fix. > diff --git a/Documentation/hwmon/max34409.rst b/Documentation/hwmon/max34409.rst > new file mode 100644 > index 000000000000..91779c6a9163 > --- /dev/null > +++ b/Documentation/hwmon/max34409.rst > @@ -0,0 +1,23 @@ > +Kernel driver max34409 > +===================== > + > +Supported chips: > + * Analog Devices MAX34409 > + Prefix: 'max34409' > + Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/MAX34408-MAX34409.pdf > + > +Author: Andrey Kononov <a.kononov@xxxxxxxxxx> > + > + > +Description > +----------- > + > +This driver for SMBus Dual/Quad Current Monitor MaximIntegrated MAX34409 > + > + > +Usage Notes > +----------- > + > +This driver does not auto-detect devices. You will have to instantiate the > +devices explicitly. Please see Documentation/i2c/instantiating-devices.rst > +for details. > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index 3176c33af6c6..de412f7dcad8 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -1088,6 +1088,13 @@ config SENSORS_MAX31760 > This driver can also be built as a module. If so, the module > will be called max31760. > > +config SENSORS_MAX3440X X -> 9 This driver for sure does not and never will cover all of MAX3440[0-9]. Same for the code itself. > + tristate "Maxim max3440x SMBus Dual/Quad Current Monitor" > + depends on I2C > + help > + Say yes here to build support for Maxim family of SMBus Dual/Quad Current Monitors. > + This driver is mutually exclusive with the HWMON version. > + > config SENSORS_MAX6620 > tristate "Maxim MAX6620 fan controller" > depends on I2C > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > index e2e4e87b282f..a4e24d2b03c1 100644 > --- a/drivers/hwmon/Makefile > +++ b/drivers/hwmon/Makefile > @@ -141,6 +141,7 @@ obj-$(CONFIG_SENSORS_MAX197) += max197.o > obj-$(CONFIG_SENSORS_MAX31722) += max31722.o > obj-$(CONFIG_SENSORS_MAX31730) += max31730.o > obj-$(CONFIG_SENSORS_MAX31760) += max31760.o > +obj-$(CONFIG_SENSORS_MAX3440X) += max3440x.o > obj-$(CONFIG_SENSORS_MAX6620) += max6620.o > obj-$(CONFIG_SENSORS_MAX6621) += max6621.o > obj-$(CONFIG_SENSORS_MAX6639) += max6639.o > diff --git a/drivers/hwmon/max3440x.c b/drivers/hwmon/max3440x.c > new file mode 100644 > index 000000000000..b62c34f9425c > --- /dev/null > +++ b/drivers/hwmon/max3440x.c > @@ -0,0 +1,213 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > +* > +*/ Completely useless comment. > + > +#include <linux/module.h> > +#include <linux/init.h> > +#include <linux/slab.h> > +#include <linux/jiffies.h> > +#include <linux/i2c.h> > +#include <linux/hwmon.h> > +#include <linux/hwmon-sysfs.h> > +#include <linux/err.h> > +#include <linux/mutex.h> > +#include <linux/sysfs.h> Alphabetic include file order, please. > + > +/* > + * Registers description. > + */ > +#define MAX3440X_STATUS 0x00 > +#define MAX3440X_CONTROL 0x01 > +#define MAX3440X_OCDELAY 0x02 > +#define MAX3440X_SDDELAY 0x03 > +#define MAX3440X_ADC1 0x04 /* readonly */ > +#define MAX3440X_ADC2 0x05 /* readonly */ > +#define MAX3440X_ADC3 0x06 /* readonly */ > +#define MAX3440X_ADC4 0x07 /* readonly */ > +#define MAX3440X_OCT1 0x08 > +#define MAX3440X_OCT2 0x09 > +#define MAX3440X_OCT3 0x0A > +#define MAX3440X_OCT4 0x0B > +#define MAX3440X_DID 0x0C /* readonly */ > +#define MAX3440X_DCYY 0x0D /* readonly */ > +#define MAX3440X_DCWW 0x0E /* readonly */ > + > +//maximal current in mA throw RSENSE, that can be measured. see datasheet table 18 No C++ comments, please. > +static unsigned short imax[4]; > +module_param_array(imax, short, NULL, 0); > +MODULE_PARM_DESC(imax, > + "maximal current in mA throw RSENSE, that can be measured. see datasheet table 18"); This is not an appropriate way to set limits or to provide scaling. The driver should assume a default for Rsense (such as 1 mOhm), and possibly provide the ability (via devicetree properties) to configure actual rsense values. Anything else should be done using sensors3.conf. > +struct max3440x_data { > + struct i2c_client *client; > + struct device *hwmon_dev; > + const char *name; 'name' and 'hwmon_dev' are unused. > + > + struct mutex update_lock; > + bool valid; > + valid is never read and pointless. Obviously some formatting problems. See checkpatch output. > + u16 adc[4]; > + u8 oct[4]; 'oct' is unused. I assume this is supposed to be for "Over Current Threshold", which should be implemented as curr[1-4]_max sysfs attributes. > +}; > + > +static const char * const input_names[] = { > + [MAX3440X_ADC1] = "curr1", > + [MAX3440X_ADC2] = "curr2", > + [MAX3440X_ADC3] = "curr3", > + [MAX3440X_ADC4] = "curr4", Those names have no value. It is obvious that curr1_input is curr1. > +}; > + > +static void max3440x_init_client(struct max3440x_data *data, > + struct i2c_client *client) > +{ > + u8 status; > + u16 val = 0; Pointless initialization > + /* > + * Start the conversions. > + */ There is nothing in the datasheet suggesting that it would be necessary to read the status register to start conversions. > + status = i2c_smbus_read_byte_data(client, MAX3440X_STATUS); Please do not ignore errors. Anyway, "status" is neither used nor saved, and reading it is thus just pointless. > + > +val = (u16)i2c_smbus_read_byte_data(client, MAX3440X_ADC1); > + data->adc[0] = DIV_ROUND_CLOSEST((imax[0] * val), 256); > + val = i2c_smbus_read_byte_data(client, MAX3440X_ADC2); > + data->adc[1] = DIV_ROUND_CLOSEST((imax[1] * val), 256); > + val = i2c_smbus_read_byte_data(client, MAX3440X_ADC3); > + data->adc[2] = DIV_ROUND_CLOSEST((imax[2] * val), 256); > + val = i2c_smbus_read_byte_data(client, MAX3440X_ADC4); > + data->adc[3] = DIV_ROUND_CLOSEST((imax[3] * val), 256); This function for the most part duplicates max3440x_update_device() and does not add any value. > +} > + > +static struct max3440x_data *max3440x_update_device(struct device *dev) > +{ > + struct max3440x_data *data = dev_get_drvdata(dev); > + struct i2c_client *client = data->client; > + u16 val; > + > + mutex_lock(&data->update_lock); > + > + dev_dbg(dev, "Updating max3440 data.\n"); > + val = (u16)i2c_smbus_read_byte_data(client, > + MAX3440X_ADC1); i2c_smbus_read_byte_data() returns a negative error code. Please do not ignore. > + data->adc[0] = DIV_ROUND_CLOSEST((imax[0] * val), 256); > + val = (u16)i2c_smbus_read_byte_data(client, > + MAX3440X_ADC2); > + data->adc[1] = DIV_ROUND_CLOSEST((imax[1] * val), 256); > + val = (u16)i2c_smbus_read_byte_data(client, > + MAX3440X_ADC3); > + data->adc[2] = DIV_ROUND_CLOSEST((imax[2] * val), 256); > + val = (u16)i2c_smbus_read_byte_data(client, > + MAX3440X_ADC4); > + data->adc[3] = DIV_ROUND_CLOSEST((imax[3] * val), 256); > + > + data->valid = 1; > + mutex_unlock(&data->update_lock); > + > + return data; > +} > +static ssize_t adc1_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct sensor_device_attribute *attr2 = to_sensor_dev_attr(attr); > + struct max3440x_data *data = max3440x_update_device(dev); Reading all sensors to report one is a waste of i2c bandwidth. Please drop max3440x_update_device() entirely and only read the data for the sensor which is reported. > + > + return sprintf(buf, "%d\n", data->adc[0]); > +} > +static ssize_t adc2_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct sensor_device_attribute *attr2 = to_sensor_dev_attr(attr); > + struct max3440x_data *data = max3440x_update_device(dev); > + > + return sprintf(buf, "%d\n", data->adc[1]); > +} > +static ssize_t adc3_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct sensor_device_attribute *attr2 = to_sensor_dev_attr(attr); > + struct max3440x_data *data = max3440x_update_device(dev); > + > + return sprintf(buf, "%d\n", data->adc[2]); > +} > +static ssize_t adc4_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct sensor_device_attribute *attr2 = to_sensor_dev_attr(attr); > + struct max3440x_data *data = max3440x_update_device(dev); > + > + return sprintf(buf, "%d\n", data->adc[3]); > +} > + > +static ssize_t label_show(struct device *dev, > + struct device_attribute *devattr, char *buf) > +{ > + return sprintf(buf, "%s\n", > + input_names[to_sensor_dev_attr(devattr)->index]); > +} > + > +static SENSOR_DEVICE_ATTR_RO(curr1_input, adc1, 0); > +static SENSOR_DEVICE_ATTR_RO(curr1_label, label, MAX3440X_ADC1); > +static SENSOR_DEVICE_ATTR_RO(curr2_input, adc2, 0); > +static SENSOR_DEVICE_ATTR_RO(curr2_label, label, MAX3440X_ADC2); > +static SENSOR_DEVICE_ATTR_RO(curr3_input, adc3, 0); > +static SENSOR_DEVICE_ATTR_RO(curr3_label, label, MAX3440X_ADC3); > +static SENSOR_DEVICE_ATTR_RO(curr4_input, adc4, 0); > +static SENSOR_DEVICE_ATTR_RO(curr4_label, label, MAX3440X_ADC4); Not that it matters because you should use the _with_info API, but the last parameter of SENSOR_DEVICE_ATTR_{RO,RW} is supposed to be the index, which is supposed to be used by the called function to determine which of the sensors should be accessed. This means that separate functions are not necessary. > + > +static struct attribute *max3440x_attrs[] = { > + &sensor_dev_attr_curr1_input.dev_attr.attr, > + &sensor_dev_attr_curr1_label.dev_attr.attr, > + &sensor_dev_attr_curr2_input.dev_attr.attr, > + &sensor_dev_attr_curr2_label.dev_attr.attr, > + &sensor_dev_attr_curr3_input.dev_attr.attr, > + &sensor_dev_attr_curr3_label.dev_attr.attr, > + &sensor_dev_attr_curr4_input.dev_attr.attr, > + &sensor_dev_attr_curr4_label.dev_attr.attr, > + NULL > +}; > + > +ATTRIBUTE_GROUPS(max3440x); > + > +static int max3440x_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct device *dev = &client->dev; > + struct max3440x_data *data; > + struct device *hwmon_dev; > + > + data = devm_kzalloc(dev, sizeof(struct max3440x_data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + data->client = client; > + mutex_init(&data->update_lock); > + > + /* Initialize the MAX3440x chip */ > + max3440x_init_client(data, client); > + > + hwmon_dev = devm_hwmon_device_register_with_groups(&client->dev, > + client->name, data, > + max3440x_groups); Please rewrite to use devm_hwmon_device_register_with_info(). > + return PTR_ERR_OR_ZERO(hwmon_dev); > +} > + > + > +static const struct i2c_device_id max3440x_id[] = { > + { "max34409", 0 }, > + { } > +}; > + > +MODULE_DEVICE_TABLE(i2c, max3440x_id); > + > +static const struct i2c_driver max3440x_driver = { > + .class = I2C_CLASS_HWMON, > + .driver = { > + .name = "max3440x", > + }, > + .probe = max3440x_probe, > + .id_table = max3440x_id, > +}; > + > +module_i2c_driver(max3440x_driver); > + > +MODULE_AUTHOR("Andrey Kononov"); > +MODULE_DESCRIPTION("I2C adc driver"); There is an emphasis on adc, not on hardware monitoring. The limit registers / attributes are not implemented, and neither is status reporting, making its value as hwmon driver quite limited. Are you sure you want/need a hardware monitoring driver ? If this is just used as an ADC and not as current monitor it may be better to implement it as an IIO driver. Guenter > +MODULE_LICENSE("GPL"); > -- > 2.34.1
