Hi Ilya, Thanks for the patch! On 2018-09-28 13:05, Ilya Novikov wrote: > From: Ilya Novikov <virredes@xxxxxxxxx> > Date: Fri, 28 Sep 2018 11:06:00 +0300 > Subject: [PATCH] i2c: support MAX735x family multiplexors > > It includes support enhanced mode for MAX7357 and MAX7358 > multiplexors. For details information about enhanced mode muxes > please take a look on > https://datasheets.maximintegrated.com/en/ds/MAX7356-MAX7358.pdf > Primary muxes settings in enhanced mode includes options: RST/INT, > Bus lock-up detection and Disconnect only the locked up bus if > the lock-up condition is present. > User can enable optionis "Flush-out sequence is sent automatically > on locked-up channels when a lock-up condition is detected" and > "Enables the preconnection wiggle test for SC_ and SD_" > using sysfs interfaces "flush_out" and "preconnection_tests". > For that need to write '1' in this file. > Checking of Lock-up register is also accessed for using. > For that need to read sysfs interface "lockup". > > Signed-off-by: Ilya Novikov <virredes@xxxxxxxxx> > --- > drivers/i2c/muxes/Kconfig | 10 + > drivers/i2c/muxes/Makefile | 1 + > drivers/i2c/muxes/i2c-mux-max735x.c | 472 ++++++++++++++++++++++++++++++++++ > include/linux/platform_data/max735x.h | 61 +++++ > 4 files changed, 544 insertions(+) > create mode 100644 drivers/i2c/muxes/i2c-mux-max735x.c > create mode 100644 include/linux/platform_data/max735x.h > > diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig > index 52a4a92..06bbba7 100644 > --- a/drivers/i2c/muxes/Kconfig > +++ b/drivers/i2c/muxes/Kconfig > @@ -73,6 +73,16 @@ config I2C_MUX_PCA954x > This driver can also be built as a module. If so, the module > will be called i2c-mux-pca954x. > > +config I2C_MUX_MAX735x Please name the driver, config entry, source file and the macro, function and variable prefix in the source after one of the supported chips, without wildcards. The driver can then support the whole family even if one chip is singled out. We do not want to risk having a chip that happens to match the wildcard but isn't supported by the driver. Also, please keep the entries sorted alphabetically in the Kconfig file. > + tristate "MAXIM MAX735x I2C Mux/switches" > + depends on GPIOLIB || COMPILE_TEST > + help > + If you say yes here you get support for the MAXIM MAX735x > + I2C mux/switch devices. > + > + This driver can also be built as a module. If so, the module > + will be called i2c-mux-max735x. > + > config I2C_MUX_PINCTRL > tristate "pinctrl-based I2C multiplexer" > depends on PINCTRL > diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile > index 6d9d865..0414d8f 100644 > --- a/drivers/i2c/muxes/Makefile > +++ b/drivers/i2c/muxes/Makefile > @@ -12,6 +12,7 @@ obj-$(CONFIG_I2C_MUX_LTC4306) += i2c-mux-ltc4306.o > obj-$(CONFIG_I2C_MUX_MLXCPLD) += i2c-mux-mlxcpld.o > obj-$(CONFIG_I2C_MUX_PCA9541) += i2c-mux-pca9541.o > obj-$(CONFIG_I2C_MUX_PCA954x) += i2c-mux-pca954x.o > +obj-$(CONFIG_I2C_MUX_MAX735x) += i2c-mux-max735x.o Sorted. > obj-$(CONFIG_I2C_MUX_PINCTRL) += i2c-mux-pinctrl.o > obj-$(CONFIG_I2C_MUX_REG) += i2c-mux-reg.o > > diff --git a/drivers/i2c/muxes/i2c-mux-max735x.c b/drivers/i2c/muxes/i2c-mux-max735x.c > new file mode 100644 > index 0000000..7d54ba4 > --- /dev/null > +++ b/drivers/i2c/muxes/i2c-mux-max735x.c > @@ -0,0 +1,472 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2018 Ilya Novikov <virredes@xxxxxxxxx> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of version 2 of the GNU General Public License as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, but > + * WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * General Public License for more details. One nice side effect of the SPDX identifier is that we can lose the wall of boilerplate license crap. Please do that. > + * > + * MAXIM Integrated I2C multiplexor Linux driver > + * > + * This module supports the MAX735x and MAX735x series of I2C multiplexer MAX735x repeated twice. > + * chips made by MAXIM Integrated. > + * > + * This includes the: > + * MAX7356, MAX7357, MAX7358. > + * Based on: > + * pca954x.c from Rodolfo Giometti <giometti@xxxxxxxx> > + * > + * Contact Information: > + * > + * Ilya Novikov <virredes@xxxxxxxxx> > + */ Add an empty line here, please. > +#include <linux/device.h> > +#include <linux/gpio/consumer.h> > +#include <linux/i2c.h> > +#include <linux/i2c-mux.h> > +#include <linux/platform_data/max735x.h> > +#include <linux/module.h> > +#include <linux/interrupt.h> > +#include <linux/string.h> > +#include <linux/of.h> > +#include <linux/pm.h> > +#include <linux/slab.h> > +#include <linux/ctype.h> Please sort the includes alphabetically. > + > +#define MAX735X_MAX_NCHANS 8 > + > +enum max735x_type { > + max_7356, > + max_7357, > + max_7358, > +}; > + > +struct max735x { > + enum max735x_type type; > + struct i2c_client *client; > + > + u8 deselect; > + u8 last_chan; /* last register value */ > +}; > + > +static const struct i2c_device_id max735x_id[] = { > + { "max7356", max_7356 }, > + { "max7357", max_7357 }, > + { "max7358", max_7358 }, > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, max735x_id); > + > +/* Write to mux register. Don't use i2c_transfer()/i2c_smbus_xfer() > + * for this as they will try to lock adapter a second time > + */ Start multi-line comments with /* on a line of its own and end with a period. Also, the comment applies for reading as well. Reading wasn't done in the pca954x driver, which is probably the reason for this? But that is not a valid defence, please be careful when you copy code, you want your new code to make sense! > + > +static int max735x_reg_read(struct i2c_adapter *adap, > + struct i2c_client *client, > + unsigned int len, u8 *buf) > +{ > + int ret = -ENODEV; > + > + if (adap->algo->master_xfer) { > + struct i2c_msg msg; > + > + msg.addr = client->addr; > + msg.flags = I2C_M_RD; > + msg.len = len; > + msg.buf = buf; > + ret = __i2c_transfer(adap, &msg, 1); > + } else { > + union i2c_smbus_data data; > + > + ret = adap->algo->smbus_xfer(adap, client->addr, > + client->flags, > + I2C_SMBUS_READ, > + *buf, I2C_SMBUS_BYTE, &data); This is bogus. The callers appear to sometimes read in enhanced mode, but the smbus branch is not using the len argument, so that just can't be correct. Reading in enhanced mode appears to only read, and thus not have the smbus notion of a "command byte" written first. So, if you want to support enhanced mode, you need to fall back to standard mode if the adapter only supports smbus. This again looks like copy-paste residue. > + } > + > + return ret; > + > +} > + > +static int max735x_reg_write(struct i2c_adapter *adap, > + struct i2c_client *client, > + unsigned int len, u8 *val) > +{ > + int ret = -ENODEV; > + > + if (adap->algo->master_xfer) { > + struct i2c_msg msg; > + > + msg.addr = client->addr; > + msg.flags = 0; > + msg.len = len; > + msg.buf = val; > + ret = __i2c_transfer(adap, &msg, 1); > + } else { > + union i2c_smbus_data data; > + > + ret = adap->algo->smbus_xfer(adap, client->addr, > + client->flags, > + I2C_SMBUS_WRITE, > + *val, I2C_SMBUS_BYTE, &data); Dito on the len issue from the above read function, but here it is possible to do the enhanced write with smbus. Not that I think that matters since you can't do the enhanced reads with smbus anyway... > + } > + > + return ret; > +} > + > +static irqreturn_t rst_int_isr_thread(int irq, void *dev_id) Missing max753x_ prefix for this function. > +{ > + struct i2c_client *client = dev_id; > + struct i2c_adapter *adap = to_i2c_adapter(client->dev.parent); > + u8 buffer[MAX735X_STUCK_FAULT_REG]; > + int ret = -ENODEV; > + int i; > + > + ret = max735x_reg_read(adap, client, MAX735X_STUCK_FAULT_REG, buffer); > + if (ret < 0) { > + dev_err(&client->dev, > + "Can not read Stuck High Fault register\n"); > + return IRQ_NONE; > + } > + > + for (i = 0; i < MAX735X_MAX_NCHANS; i++) { > + if (buffer[MAX735X_LOCKUP_REG - 1] & BIT(i)) > + dev_info(&client->dev, > + "Channel %d has been locked.\n", i); I'd phrase that as "...is locked up." > + > + if (buffer[MAX735X_STUCK_FAULT_REG - 1] & BIT(i)) > + dev_info(&client->dev, > + "Channel %d has been stuck. Address is %x. The First byte date is %x\n", date? "Channel %d is stuck high. Address and first byte was 0x%02x 0x%02x\n" Is this really the address though? Or is it the 8-bit address/rw-bit combo? What if two channels get stuck at the same time? Or is that not possible when only one channel is "open" at any one time? (you're using the chips as plain muxes, and never switches) Also, should some recovery be attempted here, or does the parent adapter already do that if it can? > + i, buffer[MAX735X_TRAFIC_REG_0 - 1], > + buffer[MAX735X_TRAFIC_REG_1 - 1]); > + } > + > + return IRQ_HANDLED; > +} > + > + > +static ssize_t lockup_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + int ret = -ENODEV; > + u8 buffer[MAX735X_LOCKUP_REG]; > + > + struct i2c_client *client = i2c_verify_client(dev); You are not checking the return value, so to_i2c_client seems more appropriate. > + struct i2c_adapter *adap = to_i2c_adapter(client->dev.parent); client->adapter? Also, put these two at the top of the declarations and lose the separating empty line. > + > + ret = max735x_reg_read(adap, client, MAX735X_LOCKUP_REG, > + buffer); No need to split this line. > + > + if (ret < 0) { > + dev_err(&client->dev, > + "Failed to read Lock-Up Indication register\n"); If you use dev instead of client->dev you can fit this on one line. > + return ret; > + } > + > + return sprintf(buf, "%x\n", buffer[MAX735X_LOCKUP_REG - 1]); > +} > + > +static DEVICE_ATTR_RO(lockup); > + > +static ssize_t store_config(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + int ret = -ENODEV; > + unsigned char val; > + u8 buffer[MAX735X_CONFIG_REG]; > + u8 config; > + > + struct i2c_client *client = i2c_verify_client(dev); You are not checking the return value, so to_i2c_client seems more appropriate. > + struct i2c_adapter *adap = to_i2c_adapter(client->dev.parent); client->adapter? Also, put these two at the top of the declarations and lose the separating empty line. > + > + ret = max735x_reg_read(adap, client, MAX735X_CONFIG_REG, > + buffer); One line. > + if (ret < 0) { > + dev_err(&client->dev, > + "Failed to read Configuration register\n"); dev, one line. > + return ret; > + } > + > + if (!strcmp(attr->attr.name, "flush_out")) > + config = BIT(MAX735X_FLASHOUT); > + else > + config = BIT(MAX735X_PRECONNECT_TEST); > + > + ret = sscanf(buf, "%c", &val); sscanf to get the first char of a buffer? Please... val = *buf; Also, I think you need to check count so that it is not zero? Is buf a zero terminated string? Hmm, how should a write of "01" be interpreted? 0 or 1? There is probably some rule, but I don't know what it is off the top of my head... > + > + switch (val) { > + Lose this empty line. > + case '1': > + buffer[MAX735X_CONFIG_REG - 1] |= config; > + dev_info(&client->dev, "%s enable\n", > + attr->attr.name); dev, one line. > + break; > + > + case '0': > + buffer[MAX735X_CONFIG_REG - 1] &= ~config; > + dev_info(&client->dev, "%s disable\n", > + attr->attr.name); dito. > + break; > + > + default: > + dev_err(&client->dev, "Incorrect value: %c\n", val); dev > + return -EINVAL; > + } > + ret = max735x_reg_write(adap, client, MAX735X_CONFIG_REG, > + buffer); One line. Also, you do a read-modify-write here without locking. That is racy as hell. What if a mux operation comes in in the middle modifying the switch register? What if you get two concurrent writes? > + if (ret < 0) { > + dev_err(&client->dev, > + "Failed to write Configuration register\n"); dev, one line. > + return ret; > + } > + > + return ret ?: count; ret is typically the return value of __i2c_transfer here, which is greater that zero on success. Which means that you return the wrong entity from this function. Which by pure luck happens to be 1 and therefore work fine since you happen to also write one byte most of the time, but please fix this to return count on success. > +} > + > +static DEVICE_ATTR(flush_out, 0200, NULL, store_config); > +static DEVICE_ATTR(preconnection_tests, 0200, NULL, store_config); > + > +static struct attribute *config_entries[] = { > + &dev_attr_lockup.attr, > + &dev_attr_flush_out.attr, > + &dev_attr_preconnection_tests.attr, > + NULL, > +}; > + > +static struct attribute_group config_group = { > + .attrs = config_entries, > +}; > + > +static int max735x_select_chan(struct i2c_mux_core *muxc, u32 chan) > +{ > + struct max735x *data = i2c_mux_priv(muxc); > + struct i2c_client *client = data->client; > + u8 regval; I would name this u8 buffer[MAX735X_SWITCH_REG]; and then adjust the below regval uses to use the "buffer". This makes it clearer in the code that the enhanced mode is the preferred mode of operation. (but see below (*) for why this would need a "+ 1" tail) > + int ret = 0; > + > + regval = BIT(chan); And this would then be buffer[MAX735X_SWITCH_REG - 1] = BIT(chan); (but see below (*) for why this would lose the "- 1" tail) > + > + /* Only select the channel > + * if its different from the last channel > + */ Start multi-line comments with /* on a line of its own and end with a period. > + if (data->last_chan != regval) { In the below deselect function you return early if there is nothing to do. You could do that here as well. > + ret = max735x_reg_write(muxc->parent, client, > + MAX735X_SWITCH_REG, ®val); > + data->last_chan = ret < 0 ? 0 : regval; > + } > + > + return ret; > +} > + > +static int max735x_deselect_mux(struct i2c_mux_core *muxc, u32 chan) > +{ > + struct max735x *data = i2c_mux_priv(muxc); > + struct i2c_client *client = data->client; > + > + if (!(data->deselect & BIT(chan))) > + return 0; > + > + /* Deselect active channel */ End the comment with a period. > + data->last_chan = 0; > + return max735x_reg_write(muxc->parent, client, > + MAX735X_SWITCH_REG, > + &(data->last_chan)); Similar to the comment for the select function, I would prefer it if you introduced a buffer variable here. > +} > + > +static int max735x_enhanced_mode(struct i2c_adapter *adap, > + struct i2c_client *client) > +{ > + int ret = -ENODEV; > + u8 buffer[MAX735X_CONFIG_REG]; > + struct max735x *data = i2c_get_clientdata(client); Reverse the order of these so that longer lines come first. > + > + switch (data->type) { > + Drop the empty line. > + case max_7356: > + dev_notice(&client->dev, > + "MAX7356 doesn't support enhanced mode so basic mode is activated\n"); I find this indentation ugly, I'd rather have a longer line instead. You are already breaking the 80-rule anyway... Also, is it really interesting to see this message in the log? It seems to be static information. > + return 0; > + > + case max_7358: > + if (i2c_check_functionality(adap, I2C_FUNC_SMBUS_QUICK)) { > + max735x_reg_write(adap, client, 0, NULL); Reading the documentation, this is not how you enter enhanced mode. The way I read it, entering enhanced mode requires a four-message transfer, while this is just a one message-transfer. What's going on? And why not fall back to basic mode on failure? > + } else { > + dev_warn(&client->dev, > + "%s doesn't support I2C_FUNC_SMBUS_QUICK so basic mode is activated\n", > + adap->name); > + return 0; > + } > + > + default: > + break; > + } > + > + ret = max735x_reg_read(adap, client, MAX735X_SWITCH_REG, > + &buffer[MAX735X_SWITCH_REG - 1]); I believe it to be more robust to have the last argument be just buffer, like it is for other reads. > + if (ret < 0) { > + dev_err(&client->dev, > + "Failed to read Switch control register\n"); If you introduce a dev variable, you can fit this and other messages below on single lines. > + return ret; > + } > + > + buffer[MAX735X_CONFIG_REG - 1] = BIT(MAX735X_RST_INT) | > + BIT(MAX735X_LOCKUP_BUS); > + > + ret = max735x_reg_write(adap, client, MAX735X_CONFIG_REG, > + buffer); One line. > + if (ret < 0) { > + dev_err(&client->dev, > + "Failed to write Configuration register\n"); > + return ret; > + } > + > + ret = sysfs_create_group(&client->dev.kobj, &config_group); > + > + if (ret < 0) { > + dev_err(&client->dev, > + "Failed to create sysfs group\n"); > + return ret; > + } > + > + ret = devm_request_threaded_irq(&client->dev, client->irq, > + NULL, rst_int_isr_thread, > + IRQF_ONESHOT, > + "rst_int_handler", client); > + if (ret < 0) { > + dev_err(&client->dev, > + "Failed to register interrupt handler\n"); > + goto fail; > + } > + > + dev_info(&client->dev, "Enhanced mode enabled\n"); > + return ret; > + > +fail: Name the label for what happens when you jump there. In this case, e.g. "remove_group", "fail_remove_group" or some such. > + sysfs_remove_group(&client->dev.kobj, &config_group); > + return ret; > +} > + > +/* > + * I2C init/probing/exit functions > + */ > +static int max735x_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct i2c_adapter *adap = to_i2c_adapter(client->dev.parent); client->adapter. > + struct max735x_platform_data *pdata = dev_get_platdata(&client->dev); > + struct device *dev = &client->dev; > + struct device_node *of_node = dev->of_node; > + bool idle_disconnect_dt; > + int num, force, class; > + struct mux_core *muxc; > + struct max735x *data; > + int ret = -ENODEV; > + > + if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE)) > + return ret; This is not what you are using, so it is wrong to require it. > + > + muxc = i2c_mux_alloc(adap, dev, MAX735X_MAX_NCHANS, sizeof(*data), 0, > + max735x_select_chan, max735x_deselect_mux); > + > + if (!muxc) > + return -ENOMEM; > + data = i2c_mux_priv(muxc); > + > + i2c_set_clientdata(client, muxc); > + > + /* Write the mux register at addr to verify > + * that the mux is in fact present. This also > + * initializes the mux to disconnected state. > + */ Multi-line comment should start with /* on a line of its own. > + if (i2c_smbus_write_byte(client, 0) < 0) { For consistency, I think you should use your ...reg_write function here. > + dev_warn(dev, "probe failed\n"); > + return ret; > + } > + > + data->type = id->driver_data; > + data->last_chan = 0; /* force the first selection */ > + > + idle_disconnect_dt = of_node && > + of_property_read_bool(of_node, "i2c-mux-idle-disconnect"); There is no device-tree binding documentation included. You need to add that (it should be added in a separate patch). Also, you need to drop the max735x_platform_data stuff. The pca954x driver you used as template is *old* and has some legacy crap. Take a peak at e.g. the ltc4306 driver instead. > + > + /* Now create an adapter for each channel */ > + for (num = 0; num < MAX735X_MAX_NCHANS; num++) { > + bool idle_disconnect_pd = false; > + > + force = 0; /* dynamic adap number */ > + class = 0; /* no class by default */ > + if (pdata) { > + if (num < pdata->num_modes) { > + /* force static number */ > + force = pdata->modes[num].adap_id; > + class = pdata->modes[num].class; > + } else > + /* discard unconfigured channels */ > + break; > + idle_disconnect_pd = pdata->modes[num].deselect_on_exit; > + } > + > + data->deselect |= (idle_disconnect_pd || > + idle_disconnect_dt) << num; > + ret = i2c_mux_add_adapter(muxc, force, num, class); > + if (ret) > + goto fail_cleanup; > + > + } > + > + ret = max735x_enhanced_mode(adap, client); You don't cleanup the mux adapters on failure, and you output a misleading message. Maybe you intended to fall back to basic mode on failure? But in that case you should return the chip to basic mode and also make sure to return zero from the probe. > + > + dev_info(dev, > + "registered %d multiplexed busses for I2C mux %s\n", > + num, client->name); > + > + return ret; > + > +fail_cleanup: Please rename to fail_del_adapters, or something similar. "cleanup" is too generic, should there be a need to add more labels. > + i2c_mux_del_adapters(muxc); > + return ret; > +} > + > +static int max735x_remove(struct i2c_client *client) > +{ > + struct i2c_mux_core *muxc = i2c_get_clientdata(client); > + > + i2c_mux_del_adapters(muxc); > + sysfs_remove_group(&client->dev.kobj, &config_group); > + return 0; > +} > + > +#ifdef CONFIG_PM_SLEEP > +static int max735x_resume(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct max735x *data = i2c_get_clientdata(client); > + > + data->last_chan = 0; > + return i2c_smbus_write_byte(client, 0); Another case where I think you should use ...reg_write. > +} > +#endif > + > +static SIMPLE_DEV_PM_OPS(max735x_pm, NULL, max735x_resume); > + > +static struct i2c_driver max735x_driver = { > + .driver = { > + .name = "max735x", > + .pm = &max735x_pm, > + }, > + .probe = max735x_probe, Please use .probe_new instead. > + .remove = max735x_remove, > + .id_table = max735x_id, > +}; > + > +module_i2c_driver(max735x_driver); > + > +MODULE_AUTHOR("Ilya Novikov"); > +MODULE_DESCRIPTION("MAX735x I2C mux/switch driver"); > +MODULE_LICENSE("GPL v2"); > diff --git a/include/linux/platform_data/max735x.h b/include/linux/platform_data/max735x.h > new file mode 100644 > index 0000000..3d8bc8a > --- /dev/null > +++ b/include/linux/platform_data/max735x.h > @@ -0,0 +1,61 @@ > +/* SPDX-License-Identifier: GPL-2.0 > + * > + * Copyright (C) 2018 Ilya Novikov <virredes@xxxxxxxxx> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of version 2 of the GNU General Public License as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, but > + * WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * General Public License for more details. > + * > + * MAXIM Integrated I2C multiplexor Linux driver > + * > + * This module supports the MAX735x and MAX735x series of I2C multiplexer > + * chips made by MAXIM Integrated. > + * > + * This includes the: > + * MAX7356, MAX7357, MAX7358. > + * Based on: > + * pca954x.c from Rodolfo Giometti <giometti@xxxxxxxx> > + * > + * Contact Information: > + * > + * Ilya Novikov <virredes@xxxxxxxxx> > + */ > + > +#ifndef _LINUX_I2C_MAX735X_H > +#define _LINUX_I2C_MAX735X_H > + > +#define MAX735X_SWITCH_REG 1 > +#define MAX735X_CONFIG_REG 2 > +#define MAX735X_FLUSHOUT_REG 3 > +#define MAX735X_LOCKUP_REG 4 > +#define MAX735X_TRAFIC_REG_0 5 TRAFFIC > +#define MAX735X_TRAFIC_REG_1 6 > +#define MAX735X_STUCK_FAULT_REG 7 I find these to be off by one? The switch reg is at index 0, not 1. The fact that you use these defines to determine how much of the register map to read does not move the register itself. So, either rename the defines to match what you actually use them for, or decrease the values by one. (*) I would decrease the values by one, and I would then rename the "len" arguments of the reg_read/_write functions to "reg", and then add one inside these function for the actual length of the transfers. That would get rid of a whole bunch of "- 1" noise from the code. Sure, you would need to add a few "+ 1" when you declare buffers, but that's still better in my opinion. > + > +#define MAX735X_RST_INT 0 > +#define MAX735X_FLASHOUT 1 > +#define MAX735X_RST_INT_RELEASE 2 > +#define MAX735X_LOCKUP_NO_CLEAR 3 > +#define MAX735X_LOCKUP_BUS 4 > +#define MAX735X_LOCKUP_NO_DETECT 5 > +#define MAX735X_BASIC_MODE 6 > +#define MAX735X_PRECONNECT_TEST 7 On a cursory scan I found no user of these macros that didn't wrap the macro in BIT(...), so you might as well do the BIT(...) thing here, once and for all. > + > +struct max735x_platform_mode { > + int adap_id; > + unsigned int deselect_on_exit:1; > + unsigned int class; > +}; > + > +/* Per mux/switch data, used with i2c_register_board_info */ > +struct max735x_platform_data { > + struct max735x_platform_mode *modes; > + int num_modes; > +}; > + This whole header file needs to disappear. We don't feed platform data to drivers this way anymore. The content should be moved into the .c file. Also, do you have an intended consumer for this header? That needs to be fixed as well in that case. Cheers, Peter > +#endif /* _LINUX_I2C_MAX735X_H */ > -- > 2.7.4 >
