On Fri, 23 Dec 2022 14:47:19 +0000 Jonathan Cameron <jic23@xxxxxxxxxx> wrote: > On Thu, 22 Dec 2022 15:36:09 -0500 > Hugo Villeneuve <hugo@xxxxxxxxxxx> wrote: > > > From: Hugo Villeneuve <hvilleneuve@xxxxxxxxxxxx> > > > > The Texas Instruments ADS7924 is a 4 channels, 12-bit analog to > > digital converter (ADC) with an I2C interface. > > > > Signed-off-by: Hugo Villeneuve <hvilleneuve@xxxxxxxxxxxx> > > Hi Hugo, > > Thanks for the driver. Some comments inline. > Given the various bits of support present, are you planning to follow up > with buffered support (chardev etc). > > Jonathan Hi Jonathan, thank you for your extensive feedback! As a general note, I wrote that driver using the ADS1015 as a starting point (circa january 2022). So in fact I removed buffered support, and also other unused features to have the simplest driver possible. That explain why you see a lot of defines/variables that could be removed for now. I was thinking of maybe adding those features at a later time, once a first version of this simple driver is accepted into the kernel. In the meantime, I will remove these unused defines/variables. > > --- > > MAINTAINERS | 7 + > > drivers/iio/adc/Kconfig | 13 + > > drivers/iio/adc/Makefile | 1 + > > drivers/iio/adc/ti-ads7924.c | 514 +++++++++++++++++++++++++++++++++++ > > 4 files changed, 535 insertions(+) > > create mode 100644 drivers/iio/adc/ti-ads7924.c > > > > ... > > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > > index 235319546974..18adcd6200b0 100644 > > --- a/drivers/iio/adc/Kconfig > > +++ b/drivers/iio/adc/Kconfig > > @@ -1207,6 +1207,19 @@ config TI_ADS1015 > > This driver can also be built as a module. If so, the module will be > > called ti-ads1015. > > > > +config TI_ADS7924 > > + tristate "Texas Instruments ADS7924 ADC" > > + depends on I2C > > + select REGMAP_I2C > > + select IIO_BUFFER > > + select IIO_TRIGGERED_BUFFER > > No buffer support yet. So drop these for now. Done. > > + help > > + If you say yes here you get support for Texas Instruments ADS7924 > > + 4 channels, 12-bit I2C ADC chip. > > + > > + This driver can also be built as a module. If so, the module will be > > + called ti-ads7924. > > + > > config TI_ADS7950 > > tristate "Texas Instruments ADS7950 ADC driver" > > depends on SPI && GPIOLIB > > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile > > index 6e08415c3f3a..1e5bdf47a091 100644 > > --- a/drivers/iio/adc/Makefile > > +++ b/drivers/iio/adc/Makefile > > @@ -107,6 +107,7 @@ obj-$(CONFIG_TI_ADC108S102) += ti-adc108s102.o > > obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o > > obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o > > obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o > > +obj-$(CONFIG_TI_ADS7924) += ti-ads7924.o > > obj-$(CONFIG_TI_ADS7950) += ti-ads7950.o > > obj-$(CONFIG_TI_ADS8344) += ti-ads8344.o > > obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o > > diff --git a/drivers/iio/adc/ti-ads7924.c b/drivers/iio/adc/ti-ads7924.c > > new file mode 100644 > > index 000000000000..e9ca7a045d18 > > --- /dev/null > > +++ b/drivers/iio/adc/ti-ads7924.c > > @@ -0,0 +1,514 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * IIO driver for Texas Instruments ADS7924 ADC, 12-bit, 4-Channels, I2C > > + * > > + * iio/adc/ti-ads7924.c > > + * Author: Hugo Villeneuve <hvilleneuve@xxxxxxxxxxxx> > > + * Copyright 2022 DimOnOff > > + * > > + * based on iio/adc/ti-ads1015.c > > + * Copyright (c) 2016, Intel Corporation. > > + * > > + * Datasheets: https://www.ti.com/lit/gpn/ads7924 > Add as a tag to the tag block of the patch description. > > Datasheet: https://www.ti.com/lit/gpn/ads7924 > on the line above your signoff. > There are some automated tools that will pick this up I think. Done. > > + */ > > + > > +#include <linux/module.h> > > +#include <linux/init.h> > > +#include <linux/irq.h> > > +#include <linux/i2c.h> > > +#include <linux/property.h> > > hmm. you include property.h then use the of ones... Now fixed after implementing your suggestions below, so keeping this include. > > +#include <linux/regmap.h> > > +#include <linux/mutex.h> > > +#include <linux/delay.h> > > Put this block in alphabetical order. Ok done. > > + > > +#include <linux/iio/iio.h> > > +#include <linux/iio/types.h> > > +#include <linux/iio/sysfs.h> > > Don't think that's used. Normally only needed for custom attributes. Ok removed. > > +#include <linux/iio/buffer.h> > > Not currently used. Ok removed. > > +#include <linux/gpio/consumer.h> > > +#include <linux/regulator/consumer.h> > > + > > +#define ADS7924_DRV_NAME "ads7924" > > + > > +#define ADS7924_CHANNELS 4 > > + > > +/* Registers. */ > > +#define ADS7924_MODECNTRL_REG 0x00 > > +#define ADS7924_INTCNTRL_REG 0x01 > > +#define ADS7924_DATA0_U_REG 0x02 > > +#define ADS7924_DATA0_L_REG 0x03 > > +#define ADS7924_DATA1_U_REG 0x04 > > +#define ADS7924_DATA1_L_REG 0x05 > > +#define ADS7924_DATA2_U_REG 0x06 > > +#define ADS7924_DATA2_L_REG 0x07 > > +#define ADS7924_DATA3_U_REG 0x08 > > +#define ADS7924_DATA3_L_REG 0x09 > > +#define ADS7924_ULR0_REG 0x0A > > +#define ADS7924_LLR0_REG 0x0B > > +#define ADS7924_ULR1_REG 0x0C > > +#define ADS7924_LLR1_REG 0x0D > > +#define ADS7924_ULR2_REG 0x0E > > +#define ADS7924_LLR2_REG 0x0F > > +#define ADS7924_ULR3_REG 0x10 > > +#define ADS7924_LLR3_REG 0x11 > > +#define ADS7924_INTCONFIG_REG 0x12 > > +#define ADS7924_SLPCONFIG_REG 0x13 > > +#define ADS7924_ACQCONFIG_REG 0x14 > > +#define ADS7924_PWRCONFIG_REG 0x15 > > +#define ADS7924_RESET_REG 0x16 > > + > > Comment syntax as below. Ok done. > > > +/* Register address INC bit: when set to '1', the register address is > > + * automatically incremented after every register read which allows convenient > > + * reading of multiple registers. Set INC to '0' when reading a single register. > > + */ > > +#define ADS7924_AUTO_INCREMENT_BIT BIT(7) > > + > > +#define ADS7924_MODECNTRL_MODE_SHIFT 2 > > +#define ADS7924_MODECNTRL_MODE_MASK GENMASK(7, ADS7924_MODECNTRL_MODE_SHIFT) > > + > > +#define ADS7924_MODECNTRL_SEL_SHIFT 0 > > As below, don't provide a _SHIFT macro for any of these. > The MASK should be the only one needed as it implicitly contains the shift anyway. Ok, done, now using FIELD_PREP as you suggested. Much simpler this way! > > +#define ADS7924_MODECNTRL_SEL_MASK GENMASK(1, ADS7924_MODECNTRL_SEL_SHIFT) > > + > > +#define ADS7924_CFG_INTPOL_BIT 1 > > +#define ADS7924_CFG_INTTRIG_BIT 0 > > + > > +#define ADS7924_CFG_INTPOL_MASK BIT(ADS7924_CFG_INTPOL_BIT) > > +#define ADS7924_CFG_INTTRIG_MASK BIT(ADS7924_CFG_INTTRIG_BIT) > > + > > +/* Interrupt pin polarity */ > > +#define ADS7924_CFG_INTPOL_LOW 0 > > +#define ADS7924_CFG_INTPOL_HIGH 1 > > + > > +/* Interrupt pin signaling */ > > +#define ADS7924_CFG_INTTRIG_LEVEL 0 > > +#define ADS7924_CFG_INTTRIG_EDGE 1 > > + > > +/* Mode control values */ > > +#define ADS7924_MODECNTRL_IDLE 0x00 > > +#define ADS7924_MODECNTRL_AWAKE 0x20 > > +#define ADS7924_MODECNTRL_MANUAL_SINGLE 0x30 > > +#define ADS7924_MODECNTRL_MANUAL_SCAN 0x32 > > +#define ADS7924_MODECNTRL_AUTO_SINGLE 0x31 > > +#define ADS7924_MODECNTRL_AUTO_SCAN 0x33 > > +#define ADS7924_MODECNTRL_AUTO_SINGLE_SLEEP 0x39 > > +#define ADS7924_MODECNTRL_AUTO_SCAN_SLEEP 0x3B > > +#define ADS7924_MODECNTRL_AUTO_BURST_SLEEP 0x3F > > + > > +#define ADS7924_ACQTIME_SHIFT 0 > > +#define ADS7924_ACQTIME_MASK GENMASK(4, ADS7924_ACQTIME_SHIFT) > > + > > +#define ADS7924_PWRUPTIME_SHIFT 0 > > No point in providing shift. The mask definition should always be sufficient > if using FIELD_GET / FIELD_PREP() Ok, done (now using FIELD_PREP as you suggested). > > +#define ADS7924_PWRUPTIME_MASK GENMASK(4, ADS7924_PWRUPTIME_SHIFT) > > + > > +/* The power-up time is allowed to elapse whenever the device has been shutdown > Comment syntax > /* > * The power-up.... Done. > > + * in idle mode. Power-up time can allow external circuits, such as an > > + * operational amplifier, between the MUXOUT and ADCIN pins to turn on. > > + * The nominal time programmed by the PUTIME[4:0] register bits is given by: > > + * t PU = PWRUPTIME[4:0] × 2 μs > > + * If a power-up time is not required, set the bits to '0' to effectively bypass. > > + */ > > +#define ADS7924_PWRUPTIME_US 0 /* Bypass (0us). */ > > + > > +/* Acquisition Time. Minimum is 6us. */ > > +#define ADS7924_ACQTIME_US 6 /* Disable (6us). */ > > Disable? Strange comment for something called acquisition time. Agreed, I have reworded the comment to make it clearer. > > + > > +/* The conversion time is always 4μs and cannot be programmed by the user. */ > > +#define ADS7924_CONVTIME_US 4 > > + > > +#define ADS7924_TOTAL_CONVTIME (ADS7924_PWRUPTIME_US + ADS7924_ACQTIME_US + \ > > + ADS7924_CONVTIME_US) > > Postfix this with _US as well. It's good habit to put units in defines, so > do it everywhere it makes sense. Ok done. > > + > > +#define ADS7924_SLEEP_DELAY_MS 2000 > > + > > +enum ads7924_channels { > > + ADS7924_CH0, > > + ADS7924_CH1, > > + ADS7924_CH2, > > + ADS7924_CH3, > > + ADS7924_TIMESTAMP, > > +}; > > + > > +#define ADS7924_V_CHAN(_chan, _addr) { \ > > + .type = IIO_VOLTAGE, \ > > + .indexed = 1, \ > > + .address = _addr, \ > > + .channel = _chan, \ > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > > + .scan_index = _addr, \ > > + .scan_type = { \ > > + .sign = 's', \ > > + .realbits = 12, \ > > You are using the shift element which is fine, but I think the majority of this > isn't yet used as no buffer support. As such, I'd drop anything you aren't > using for now. Add it back if you add buffered support in future. > > > + .storagebits = 16, \ > > + .shift = 4, \ > > + .endianness = IIO_CPU, \ > > It's big endian off the devices, so that's how you should present it > to userspace via a buffered interface (this scan_type entry has no > standard meaning except for buffered interfaces). Ok, dropped scan_index and scan_type fields. > > > + }, \ > > + .datasheet_name = "AIN"#_chan, \ > > +} > > + > > +/* To map a channel with its corresponding data register (MSB). */ > > +static const unsigned int ads7924_channel_data_msb_regs[] = { > > + ADS7924_DATA0_U_REG, > > + ADS7924_DATA1_U_REG, > > + ADS7924_DATA2_U_REG, > > + ADS7924_DATA3_U_REG > > It's possible this driver might one day support more elements in here (maybe > there is an 8 channel otherwise compatible part), so add a trailing comma. > > Generally the only time we don't ask for trailing commas is after NULL terminators > or other entries that will by definition always be the last element. Noted, but I finally removed this array, as I am now using the "address" field inside structure iio_chan_spec to store the register address. > > +}; > > + > > > > +static const struct iio_chan_spec ads7924_channels[] = { > > + ADS7924_V_CHAN(0, ADS7924_CH0), > > + ADS7924_V_CHAN(1, ADS7924_CH1), > > + ADS7924_V_CHAN(2, ADS7924_CH2), > > + ADS7924_V_CHAN(3, ADS7924_CH3), > > + IIO_CHAN_SOFT_TIMESTAMP(ADS7924_TIMESTAMP), > > Timestamp doesn't do anything useful until you support buffered modes. > So drop it for now. Ok dropped. > > +}; > > + > > +static > > +int ads7924_get_adc_result(struct ads7924_data *data, int chan, int *val) > > +{ > > + int ret; > > + __be16 be_val; > > + > > + if (chan < 0 || chan >= ADS7924_CHANNELS) > > + return -EINVAL; > > + > > + if (data->conv_invalid) { > > + int conv_time; > > + > > + conv_time = ADS7924_TOTAL_CONVTIME; > > + /* Allow 10% for internal clock inaccuracy. */ > > + conv_time += conv_time / 10; > > + usleep_range(conv_time, conv_time + 1); > > + data->conv_invalid = false; > > + } > > + > > + ret = regmap_raw_read(data->regmap, ADS7924_AUTO_INCREMENT_BIT | > > + ads7924_channel_data_msb_regs[chan], > > + &be_val, sizeof(be_val)); > > + if (ret) > > + return ret; > > + > > + *val = be16_to_cpu(be_val); > > + *val = *val >> ads7924_channels[chan].scan_type.shift; > > + > > + return ret; > > +} > > + > > +static int ads7924_read_raw(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, int *val, > > + int *val2, long mask) > > +{ > > + int ret, vref_uv; > > + struct ads7924_data *data = iio_priv(indio_dev); > > + > > + mutex_lock(&data->lock); > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: { > > + ret = iio_device_claim_direct_mode(indio_dev); > > + if (ret) > > + break; > > + > > + ret = ads7924_get_adc_result(data, chan->address, val); > > + if (ret < 0) > > Check the error code after iio_device_release_direct / mutex_unlock() > That way you can return directly. > > Note, if you add support later for buffered mode, taking a local lock > outside of claiming direct mode can deadlock. You are better off > moving the mutex_lock/unlock inside iio_device_claim_direct_mode() Ok, noted for the future if I add buffered modes. > As a side note, iio_device_claim_direct_mode() only does something useful > if you are supporting buffered mode... So for now it shouldn't really > be in this driver at all. The point of that call is protect against > transitions into buffered modes whilst a call is flight that might > make a mess of that transition or be affected by it. Ok, removed iio_device_claim_direct_mode() for now. > > > + dev_dbg(data->dev, "%s error1\n", __func__); > > + else > > + ret = IIO_VAL_INT; > > + > > + iio_device_release_direct_mode(indio_dev); > > + break; > > + } > > + case IIO_CHAN_INFO_SCALE: > > + vref_uv = regulator_get_voltage(data->vref_reg); > > + if (vref_uv < 0) { > > + /* dummy regulator "get_voltage" returns -EINVAL */ > > + ret = -EINVAL; > > + } else { > > + *val = vref_uv / 1000; /* Convert reg voltage to mV */ > > + *val2 = chan->scan_type.realbits; /* Data bits */ > > + ret = IIO_VAL_FRACTIONAL_LOG2; > > + } > > + break; > > + default: > > + ret = -EINVAL; > > + break; > > + } > > + > > + mutex_unlock(&data->lock); > > You only need the lock for the INFO_RAW switch branch as far as I can see. > Hence good to reduce it's scope to just that. It may make sense to factor > out the code there to a separate function to simplify the error paths > (a goto out of a switch is a bit nasty to read) Done for the lock/unlock. > > + > > + return ret; > > +} > > + > > +static const struct iio_info ads7924_info = { > > + .read_raw = ads7924_read_raw, > > +}; > > + > > +static int ads7924_of_parse_channel_config(struct iio_dev *indio_dev, > > + struct device_node *np) > > +{ > > + struct ads7924_data *priv = iio_priv(indio_dev); > > + struct device *dev = priv->dev; > > + struct device_node *child; > > + int num_channels; > > + unsigned int channel = 0; > > + int ret; > > + > > + num_channels = of_get_available_child_count(np); > > As mentioned below, this should be changed to use the generic firmware > property accessors in include/linux/property.h > Amongst other things that lets us use the driver with various software > added properties (e.g. if it's on a daughter board etc) or via ACPI > and the PRP0001 magic ID. Ok, converted. > > + if (!num_channels) { > > + dev_err(dev, "no channel children\n"); > > + return -ENODEV; > > + } > > + dev_dbg(dev, "found %d ADC channels\n", num_channels); > > + > > + for_each_available_child_of_node(np, child) { > > + ret = of_property_read_u32(child, "reg", &channel); > > + if (ret) > > + goto err; > > + > > + if (channel >= ADS7924_CHANNELS) { > > + dev_err(dev, "invalid channel index %d\n", channel); > > + ret = -EINVAL; > > + goto err; > > + } > > + } > > + > > + return 0; > > +err: > > + of_node_put(child); > > + > > + return ret; > > +} > > + > > +static int ads7924_set_conv_mode(struct ads7924_data *data, int mode) > > +{ > > + /* When switching between modes, be sure to first select the Awake mode > > IIO comment syntax is the most common one in kernel code (a few exceptions exist > for historical reasons). Anyhow. > > /* > * When switching... > * ... > */ Done. > > + * and then switch to the desired mode. This procedure ensures the > > + * internal control logic is properly synchronized. > > + */ > > + if (mode != ADS7924_MODECNTRL_IDLE) { > > + int ret; > > + > > + ret = regmap_update_bits(data->regmap, ADS7924_MODECNTRL_REG, > > + ADS7924_MODECNTRL_MODE_MASK, > > + ADS7924_MODECNTRL_AWAKE << > > + ADS7924_MODECNTRL_MODE_SHIFT); > Use FIELD_PREP() Done. > > + if (ret) > > + return ret; > > + } > > + > > + return regmap_update_bits(data->regmap, ADS7924_MODECNTRL_REG, > > + ADS7924_MODECNTRL_MODE_MASK, > > + mode << ADS7924_MODECNTRL_MODE_SHIFT); > > +} > > ... > > > + > > +static int ads7924_probe(struct i2c_client *client) > > +{ > > + struct iio_dev *indio_dev; > > + struct ads7924_data *data; > > + struct device *dev = &client->dev; > > + int ret; > > + > > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); > > + if (!indio_dev) > > + return -ENOMEM; > > + > > + i2c_set_clientdata(client, indio_dev); > > Once you've dropped remove after taking all the calls dev managed > you can drop this as I don't think it's used elswhere. Done. > > > + > > + data = iio_priv(indio_dev); > > + > > + data->dev = dev; > > + > > + /* Initialize the reset GPIO as output with an initial value of 0. */ > > + data->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); > > + if (IS_ERR(data->reset_gpio)) > > + return PTR_ERR(data->reset_gpio); > > + > > + mutex_init(&data->lock); > > + > > + indio_dev->name = ADS7924_DRV_NAME; > > I'd rather see the string here. There is no particular reason why it should > be the driver name. Done. > > + indio_dev->modes = INDIO_DIRECT_MODE; > > + > > + indio_dev->channels = ads7924_channels; > > + indio_dev->num_channels = ARRAY_SIZE(ads7924_channels); > > + indio_dev->info = &ads7924_info; > > + > > + ret = ads7924_of_parse_channel_config(indio_dev, dev->of_node); > > We shouldn't see device tree specific handling in a modern driver. > Use the generic accessors in include/linux/property.h Done. > > + if (ret < 0) > > + return ret; > > + > > + data->regmap = devm_regmap_init_i2c(client, &ads7924_regmap_config); > > + if (IS_ERR(data->regmap)) { > > + dev_err(dev, "Failed to allocate register map\n"); > > + return PTR_ERR(data->regmap); > > + } > > + > > + data->vref_reg = devm_regulator_get(dev, "vref"); > > + if (IS_ERR(data->vref_reg)) { > > + dev_err(dev, "devm_regulator_get() failed\n"); > > + return PTR_ERR(data->vref_reg); > > This is an example of a call that can defer so you should definitely > be using dev_err_probe() as we want the resulting debug info. > It's good practice to just use that everwhere in probe() functions. Done. > > + } > > + > > + ret = regulator_enable(data->vref_reg); > > + if (ret) { > > + dev_err(dev, "regulator_enable() failed\n"); > > + return ret; > > + } > > + > > + ret = devm_add_action_or_reset(dev, ads7924_reg_disable, data->vref_reg); > > + if (ret) { > > + dev_err(dev, "devm_add_action_or_reset() failed\n"); > > + return ret; > > + } > > + > > + ret = ads7924_reset(indio_dev); > > + if (ret < 0) { > > + dev_err(dev, "Failed to reset device\n"); > > + return ret; > > + } > > + > > + ret = ads7924_set_conv_mode(data, > > + ADS7924_MODECNTRL_AUTO_SCAN); > > + if (ret) > > + return ret; > > You set this, but whilst you have it set back to idle in remove() > you don't do that in the error paths for this function. > If you change to registering the switch to IDLE to a devm_add_action_or_reset() > callback then it will get automatically called in both paths. Done. > > + > > + /* Disable signal acquire time. */ > > + ret = regmap_update_bits(data->regmap, ADS7924_ACQCONFIG_REG, > > + ADS7924_ACQTIME_MASK, 0); > > + if (ret < 0) { > > + dev_err(dev, "Disable signal acquire time failed\n"); > > + return ret; > > + } > > + > > + /* Disable power-up time. */ > > + ret = regmap_update_bits(data->regmap, ADS7924_PWRCONFIG_REG, > > + ADS7924_PWRUPTIME_MASK, 0); > > + if (ret < 0) { > > + dev_err(dev, "Disable power-up time failed\n"); > > + return ret; > > + } > > + > > + data->conv_invalid = true; > > + > > + ret = iio_device_register(indio_dev); > > + if (ret < 0) { > > + dev_err(dev, "Failed to register IIO device\n"); > > For errors in probe function use the cleaner (and deferred probe aware) > return dev_err_probe(dev, ret, "Failed to register IIO device\n"); Done. > Same for all other error messages in probe(). > Done. > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static void ads7924_remove(struct i2c_client *client) > > +{ > > + struct iio_dev *indio_dev = i2c_get_clientdata(client); > > + struct ads7924_data *data = iio_priv(indio_dev); > > + int ret; > > + > > + iio_device_unregister(indio_dev); > Having made the changes suggested below, you can call devm_iio_device_register() > and avoid need for this call. Done. > > + > > + ret = ads7924_set_conv_mode(data, ADS7924_MODECNTRL_IDLE); > > I would suggest wrapping this call up in a callback registered > with devm_add_action_or_reset() That will allow you to > drop the remove function entirely. It also simplifies error handling. Done. > > > + if (ret) > > + dev_warn(&client->dev, "Failed to power down (%pe)\n", > > + ERR_PTR(ret)); > > +} > > + > > +static const struct of_device_id ads7924_of_match[] = { > > strange though it may seem, you also need the legacy > i2c_device_id table. There are a couple of reasons why: > > 1) It's the table used to generate the module aliases to make > autoprobing of the module used (there was an attempt to do that > via the of_device_id table, but for a reason I can't remember it > didn't work out. > 2) There are fun routes via which that old school table is still used. > > Hence for now we are in the slightly silly position of having to provide > both. Done. > > + { .compatible = "ti,ads7924", }, > > + {} > > +}; > > +MODULE_DEVICE_TABLE(of, ads7924_of_match); > > + > > +static struct i2c_driver ads7924_driver = { > > + .driver = { > > + .name = ADS7924_DRV_NAME, > With the other use inline, just put the string here as well. > Saves a reviewer having to check the define is reasonable. Done. > > > + .of_match_table = ads7924_of_match, > > + }, > > + .probe_new = ads7924_probe, > > + .remove = ads7924_remove, > > +}; > > + > > +module_i2c_driver(ads7924_driver); > > + > > +MODULE_AUTHOR("Hugo Villeneuve <hvilleneuve@xxxxxxxxxxxx>"); > > +MODULE_DESCRIPTION("Texas Instruments ADS7924 ADC I2C driver"); > > +MODULE_LICENSE("GPL"); I will submit a V2 version with all those changes soon. Thank you, Hugo V.
