Hi Jonathan, Please find my comment on in line. Thank you. Abhisit S. On Wed, Aug 2, 2017 at 11:43 AM, Abhisit Sangjan wrote: > Hi Jonathan, > > Thank you for your review and good suggestion. This is my first driver, > first work for Linux. I will be follow your advice and send new once in > soon. Please help me to review. > > Thank you, > Abhisit S. > > On Tue, Aug 1, 2017 at 5:10 PM, Jonathan Cameron < > Jonathan.Cameron@huawei.com> wrote: > >> On Tue, 1 Aug 2017 16:12:22 +0700 >> wrote: >> >> > From: Abhisit Sangjan >> Hi, >> >> A very quick initial review covering stuff you'll want to clean up before >> you get some in depth reviews. >> > > Abhisit: OK, I will do. > > >> 1) Please cc linux-iio on the whole series - it's useful to see the mfd >> driver and review it as well. >> > > Abhisit: OK. > > >> 2) Description of the patch is needed. For a new driver, a quick run down >> of what it is and what is supported. > > > Abhisit: I will do. > > >> > More comments inline. >> >> Note this is not a full review by any means, just a starting point whilst >> I'm waiting for a board to boot... Will do a full review once these bits >> are tidied up. >> >> Fundamental code looks fine, it's docs and coding style that need work >> (plus locking ;) >> >> Jonathan >> > >> > --- >> > drivers/iio/adc/Kconfig | 10 + >> > drivers/iio/adc/Makefile | 1 + >> > drivers/iio/adc/lmp92001-adc.c | 479 ++++++++++++++++++++++++++++++ >> +++++++++++ >> > 3 files changed, 490 insertions(+) >> > create mode 100644 drivers/iio/adc/lmp92001-adc.c >> > >> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig >> > index 614fa41..b623b4d 100644 >> > --- a/drivers/iio/adc/Kconfig >> > +++ b/drivers/iio/adc/Kconfig >> > @@ -857,4 +857,14 @@ config XILINX_XADC >> > The driver can also be build as a module. If so, the module >> will be called >> > xilinx-xadc. >> > >> > +config LMP92001_ADC >> Alphabetical order please. >> > > Abhisit: I got you. > > >> > + tristate "TI LMP92001 ADC Driver" >> > + depends on MFD_LMP92001 >> > + help >> > + If you say yes here you get support for TI LMP92001 ADCs >> > + conversion. >> > + >> > + This driver can also be built as a module. If so, the module >> will >> > + be called lmp92001_adc. >> > + >> > endmenu >> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile >> > index b546736a..75b24b1 100644 >> > --- a/drivers/iio/adc/Makefile >> > +++ b/drivers/iio/adc/Makefile >> > @@ -78,3 +78,4 @@ obj-$(CONFIG_VF610_ADC) += vf610_adc.o >> > obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o >> > xilinx-xadc-y := xilinx-xadc-core.o xilinx-xadc-events.o >> > obj-$(CONFIG_XILINX_XADC) += xilinx-xadc.o >> > +obj-$(CONFIG_LMP92001_ADC) += lmp92001-adc.o >> Alphabetical order please. >> > > Abhisit: I got you. > > >> >> > diff --git a/drivers/iio/adc/lmp92001-adc.c >> b/drivers/iio/adc/lmp92001-adc.c >> > new file mode 100644 >> > index 0000000..909ff47 >> > --- /dev/null >> > +++ b/drivers/iio/adc/lmp92001-adc.c >> > @@ -0,0 +1,479 @@ >> > +/* >> > + * lmp92001-adc.c - Support for TI LMP92001 ADCs >> > + * >> > + * Copyright 2016-2017 Celestica Ltd. >> > + * >> > + * Author: Abhisit Sangjan >> > + * >> > + * Inspired by wm831x and ad5064 drivers. >> > + * >> > + * This program is free software; you can redistribute it and/or modify >> > + * it under the terms of the GNU General Public License as published by >> > + * the Free Software Foundation; either version 2 of the License, or >> > + * (at your option) any later version. >> > + * >> > + * 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. >> > + * >> > + */ >> > + >> > +#include >> > +#include >> > +#include >> > +#include >> > +#include >> > +#include >> > + >> > +#include >> > + >> > +static int lmp92001_read_raw(struct iio_dev *indio_dev, >> > + struct iio_chan_spec const *channel, >> int *val, >> > + int *val2, long mask) >> > +{ >> Nothing prevents multiple simultaneous runs of this function. >> Given the write then read cycles below, I'm guessing you'll be wanting >> a local lock in your lmp92001 structure. >> > > Abhisit: It is hardware lock register to prevent multiple access. > Abhisit: I thought, the function regmap_read(), regmap_update_bits() and regmap_write() are managed the lock access. Do I need to lock it by myself? Example function regmap_read(). /** * regmap_read() - Read a value from a single register * * @map: Register map to read from * @reg: Register to be read from * @val: Pointer to store read value * * A value of zero will be returned on success, a negative errno will * be returned in error cases. */ int regmap_read(struct regmap *map, unsigned int reg, unsigned int *val) { int ret; if (!IS_ALIGNED(reg, map->reg_stride)) return -EINVAL; map->lock(map->lock_arg); ret = _regmap_read(map, reg, val); map->unlock(map->lock_arg); return ret; } > > >> > + struct lmp92001 *lmp92001 = iio_device_get_drvdata(indio_dev); >> > + unsigned int code, cgen, sgen, try; >> > + int ret; >> > + >> > + ret = regmap_read(lmp92001->regmap, LMP92001_CGEN, &cgen); >> > + if (ret < 0) >> > + return ret; >> > + >> > + /* >> > + * Is not continuous conversion? >> > + * Lock the registers (if needed). >> > + * Triggering single-short conversion. >> > + * Waiting for conversion successfully. >> > + */ >> > + if (!(cgen & 1)) >> Kernel goes with K&R style on if statements and brackets. >> if (!(cgen & 1)) { >> >> 1 is a magic value - so please add a define telling us what it >> represents. >> > > Abhisit: I will double check again for all my source code for K&R and no > magic number. > > >> >> > + { >> > + if (!(cgen & 2)) >> > + { >> > + ret = regmap_update_bits(lmp92001->regmap, >> > + LMP92001_CGEN, >> 2, 2); >> Register field values and masks should use defined constants so the code >> lets >> us know what they are without consulting the datasheet. >> > > Abhisit: I will give them to be defined. > > >> >> > + if (ret < 0) >> > + return ret; >> > + } >> > + >> > + ret = regmap_write(lmp92001->regmap, LMP92001_CTRIG, >> 1); >> > + if (ret < 0) >> > + return ret; >> > + >> > + try = 10; >> Why - a try look doing repeats like this need an explanatory comment. >> > > Abhisit: Do you think, should be jiffies time? > > >> > + do { >> > + ret = regmap_read(lmp92001->regmap, >> > + LMP92001_SGEN, &sgen); >> > + if(ret < 0) >> > + return ret; >> > + } while ((sgen & 1<<7) && (--try > 0)); >> > + >> > + if (!try) >> > + return -ETIME; >> > + } >> > + >> > + ret = regmap_read(lmp92001->regmap, 0x1F + channel->channel, >> &code); >> > + if (ret < 0) >> > + return ret; >> > + >> > + switch (mask) >> > + { >> switch (mask) { >> >> Please run checkpatch over this - preferably with --strict >> and fix everything it comes up with. Saves everyone time in reviews! >> > > Abhisit: Thank you for your suggestion. It is my first driver :) > > >> > + case IIO_CHAN_INFO_RAW: >> > + switch (channel->type) { >> > + case IIO_VOLTAGE: >> > + case IIO_TEMP: >> > + *val = code; >> > + return IIO_VAL_INT; >> > + default: >> return directly here if still possible after considering locking. >> > + break; >> > + } >> > + break; >> > + default: >> > + break; >> > + } >> > + >> > + return -EINVAL; >> > +} >> > + >> > +/* >> > + * TODO: do your attributes even handler for >> > + * Current limit low/high for CH 1-3, 9-11! >> > + * In case INT1 and INT2 were connected to i.MX6. >> > + */ >> > +static const struct iio_info lmp92001_info = { >> > + .read_raw = lmp92001_read_raw, >> > + .driver_module = THIS_MODULE, >> > +}; >> > + >> > +static ssize_t lmp92001_avref_read(struct iio_dev *indio_dev, >> uintptr_t private, >> > + struct iio_chan_spec const *channel, char *buf) >> > +{ >> > + struct lmp92001 *lmp92001 = iio_device_get_drvdata(indio_dev); >> > + unsigned int cref; >> > + int ret; >> > + >> > + ret = regmap_read(lmp92001->regmap, LMP92001_CREF, &cref); >> > + if (ret < 0) >> > + return ret; >> > + >> > + return sprintf(buf, "%s\n", cref & 2 ? "external" : >> "internal"); >> This looks like new ABI. Please add ABI docs so that we can discuss this >> interface >> without having to pull it from the code. >> >> Docuentation/ABI/testing/sysfs-bus-iio-lmp920001 is probably the right >> place. >> > > Abhisit: I will do. > > >> >> Note IIO also has some nice utility functions to deal with enums like >> this. >> > > Abhisit: I have my enums version. No compile error, no work! Lets me add > it later on, after this once is working. > > >> > +} >> > + >> > +static ssize_t lmp92001_avref_write(struct iio_dev *indio_dev, >> uintptr_t private, >> > + struct iio_chan_spec const *channel, const >> char *buf, >> > + size_t len) >> > +{ >> > + struct lmp92001 *lmp92001 = iio_device_get_drvdata(indio_dev); >> > + unsigned int cref; >> > + int ret; >> > + >> > + if (strcmp("external\n", buf) == 0) >> > + cref = 2; >> > + else if (strcmp("internal\n", buf) == 0) >> > + cref = 0; >> > + else >> > + return -EINVAL; >> > + >> > + ret = regmap_update_bits(lmp92001->regmap, LMP92001_CREF, 2, >> cref); >> > + if (ret < 0) >> > + return ret; >> > + >> > + return len; >> > +} >> > + >> > +static ssize_t lmp92001_enable_read(struct iio_dev *indio_dev, >> uintptr_t private, >> > + struct iio_chan_spec const *channel, char *buf) >> > +{ >> > + struct lmp92001 *lmp92001 = iio_device_get_drvdata(indio_dev); >> > + unsigned int reg, cad; >> > + int ret; >> > + >> > + switch (channel->channel) >> > + { >> > + case 1 ... 8: >> > + reg = LMP92001_CAD1; >> > + break; >> > + case 9 ... 16: >> > + reg = LMP92001_CAD2; >> > + break; >> > + case 17: >> > + reg = LMP92001_CAD3; >> > + break; >> > + default: >> > + return -EINVAL; >> > + } >> > + >> > + ret = regmap_read(lmp92001->regmap, reg, &cad); >> > + if (ret < 0) >> > + return ret; >> > + >> > + if (channel->channel <= 8) >> > + cad >>= channel->channel - 1; >> > + else if(channel->channel > 8) >> > + cad >>= channel->channel - 9; >> > + else if(channel->channel > 16) >> > + cad >>= channel->channel - 17; >> > + else >> > + return -EINVAL; >> > + >> > + return sprintf(buf, "%s\n", cad & 1 ? "enable" : "disable"); >> Again this is new ABI. Need documenting in this series and detailed >> discussion. >> > > Abhisit: I will do. > > >> > +} >> > + >> > +static ssize_t lmp92001_enable_write(struct iio_dev *indio_dev, >> uintptr_t private, >> > + struct iio_chan_spec const *channel, const >> char *buf, >> > + size_t len) >> > +{ >> > + struct lmp92001 *lmp92001 = iio_device_get_drvdata(indio_dev); >> > + unsigned int reg, enable, shif, mask; >> > + int ret; >> > + >> > + switch (channel->channel) >> > + { >> > + case 1 ... 8: >> > + reg = LMP92001_CAD1; >> > + shif = (channel->channel - 1); >> > + break; >> > + case 9 ... 16: >> > + reg = LMP92001_CAD2; >> > + shif = (channel->channel - 9); >> > + break; >> > + case 17: >> > + reg = LMP92001_CAD3; >> > + shif = (channel->channel - 17); >> > + break; >> > + default: >> > + return -EINVAL; >> > + } >> > + >> > + if (strcmp("enable\n", buf) == 0) >> > + enable = 1; >> > + else if (strcmp("disable\n", buf) == 0) >> > + enable = 0; >> > + else >> > + return -EINVAL; >> > + >> > + enable <<= shif; >> > + mask = 1 << shif; >> > + >> > + ret = regmap_update_bits(lmp92001->regmap, reg, mask, enable); >> > + if (ret < 0) >> > + return ret; >> > + >> > + return len; >> > +} >> > + >> > +static ssize_t lmp92001_mode_read(struct iio_dev *indio_dev, uintptr_t >> private, >> > + struct iio_chan_spec const *channel, char *buf) >> > +{ >> > + struct lmp92001 *lmp92001 = iio_device_get_drvdata(indio_dev); >> > + unsigned int cgen; >> > + int ret; >> > + >> > + ret = regmap_read(lmp92001->regmap, LMP92001_CGEN, &cgen); >> > + if (ret < 0) >> > + return ret; >> > + >> > + return sprintf(buf, "%s\n", cgen & 1 ? "continuous" : >> "single-shot"); >> Again, documentation. Note that exposing explicit controls for single-shot >> or continuous capture hasn't yet made sense in any other drivers. >> Normally this is something that can be optimized in driver. >> >> There is always a trade off between flexibility and keeping the userspace >> ABI >> short and clean. >> > > Abhisit: Basically, It is hardware conversion mode operation. I will take > a look on how to optimize. > > >> > +} >> > + >> > +static ssize_t lmp92001_mode_write(struct iio_dev *indio_dev, >> uintptr_t private, >> > + struct iio_chan_spec const *channel, const >> char *buf, >> > + size_t len) >> > +{ >> > + struct lmp92001 *lmp92001 = iio_device_get_drvdata(indio_dev); >> > + unsigned int cgen; >> > + int ret; >> > + >> > + if (strcmp("continuous\n", buf) == 0) >> > + cgen = 1; >> > + else if (strcmp("single-shot\n", buf) == 0) >> > + cgen = 0; >> > + else >> > + return -EINVAL; >> > + >> > + /* >> > + * Unlock the registers. >> > + * Set conversion mode. >> > + * Lock the registers. >> > + */ >> > + ret = regmap_update_bits(lmp92001->regmap, LMP92001_CGEN, 2, >> 0); >> > + if (ret < 0) >> > + return ret; >> What stops a race with this lock? You need a driver lock to prevent >> multiple >> simultaneous sysfs actions from messing things up. >> > > Abhisit: The lock is hardware register function for prevent multiple > access to change the chip configuration. > > >> > + >> > + ret = regmap_update_bits(lmp92001->regmap, LMP92001_CGEN, 1, >> cgen); >> > + if (ret < 0) >> > + return ret; >> > + >> > + ret = regmap_update_bits(lmp92001->regmap, LMP92001_CGEN, 2, >> 2); >> > + if (ret < 0) >> > + return ret; >> > + >> > + return len; >> > +} >> > + >> > +static const struct iio_chan_spec_ext_info lmp92001_ext_info[] = { >> > + { >> > + .name = "vref", >> > + .read = lmp92001_avref_read, >> > + .write = lmp92001_avref_write, >> > + .shared = IIO_SHARED_BY_ALL, >> > + }, >> > + { >> > + .name = "en", >> > + .read = lmp92001_enable_read, >> > + .write = lmp92001_enable_write, >> > + .shared = IIO_SEPARATE, >> > + }, >> > + { >> > + .name = "mode", >> > + .read = lmp92001_mode_read, >> > + .write = lmp92001_mode_write, >> > + .shared = IIO_SHARED_BY_ALL, >> All of this must be defined in the documentation. >> >> Vref is almost always a board specific element. >> You don't fit an external reference if you are intending to >> use the internal one. >> >> If necessary we can use differential channels to represent the two >> possible voltage references, but that is rather ugly. >> > > Abhisit: I would like to design for changeable vref source. > > >> > + }, >> > + { }, >> > +}; >> > + >> > +static const struct iio_event_spec lmp92001_events[] = { >> > + { >> > + .type = IIO_EV_TYPE_THRESH, >> > + .dir = IIO_EV_DIR_RISING, >> > + .mask_separate = BIT(IIO_EV_INFO_ENABLE) | >> > + BIT(IIO_EV_INFO_VALUE), >> > + }, >> > + { >> > + .type = IIO_EV_TYPE_THRESH, >> > + .dir = IIO_EV_DIR_FALLING, >> > + .mask_separate = BIT(IIO_EV_INFO_ENABLE) | >> > + BIT(IIO_EV_INFO_VALUE), >> > + }, >> > + { }, >> > +}; >> > + >> > +#define LMP92001_CHAN_SPEC(_ch, _type, _event, _nevent) \ >> > +{ \ >> > + .channel = _ch, \ >> > + .scan_index = _ch, \ >> > + .scan_type = { \ >> > + .sign = 'u', \ >> > + .realbits = 12, \ >> > + .storagebits = 16, \ >> > + .repeat = 1, \ >> > + .endianness = IIO_BE, \ >> Don't specify stuff you aren't using. The scan stuff only >> typically becomes relevant when you providing the 'push' >> or buffered interfaces. This driver isn't yet. >> > > Abhisit: Could you help me to coding this, I have no idea, what is your > point. This is my first iio driver. > > >> > + }, \ >> > + .type = _type, \ >> > + .indexed = 1, \ >> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ >> > + .event_spec = _event, \ >> > + .num_event_specs = _nevent, \ >> > + .ext_info = lmp92001_ext_info, \ >> > +} >> > + >> > +/* >> > + * TODO: do your ext_info for current low/high limit. >> > + * Example driver/iio/dac/ad5064.c >> > + */ >> > +static const struct iio_chan_spec lmp92001_adc_channels[] = { >> > + LMP92001_CHAN_SPEC( 1, IIO_VOLTAGE, lmp92001_events, >> ARRAY_SIZE(lmp92001_events)), >> > + LMP92001_CHAN_SPEC( 2, IIO_VOLTAGE, lmp92001_events, >> ARRAY_SIZE(lmp92001_events)), >> > + LMP92001_CHAN_SPEC( 3, IIO_VOLTAGE, lmp92001_events, >> ARRAY_SIZE(lmp92001_events)), >> > + LMP92001_CHAN_SPEC( 4, IIO_VOLTAGE, NULL, 0), >> > + LMP92001_CHAN_SPEC( 5, IIO_VOLTAGE, NULL, 0), >> > + LMP92001_CHAN_SPEC( 6, IIO_VOLTAGE, NULL, 0), >> > + LMP92001_CHAN_SPEC( 7, IIO_VOLTAGE, NULL, 0), >> > + LMP92001_CHAN_SPEC( 8, IIO_VOLTAGE, NULL, 0), >> > + LMP92001_CHAN_SPEC( 9, IIO_VOLTAGE, lmp92001_events, >> ARRAY_SIZE(lmp92001_events)), >> > + LMP92001_CHAN_SPEC(10, IIO_VOLTAGE, lmp92001_events, >> ARRAY_SIZE(lmp92001_events)), >> > + LMP92001_CHAN_SPEC(11, IIO_VOLTAGE, lmp92001_events, >> ARRAY_SIZE(lmp92001_events)), >> > + LMP92001_CHAN_SPEC(12, IIO_VOLTAGE, NULL, 0), >> > + LMP92001_CHAN_SPEC(13, IIO_VOLTAGE, NULL, 0), >> > + LMP92001_CHAN_SPEC(14, IIO_VOLTAGE, NULL, 0), >> > + LMP92001_CHAN_SPEC(15, IIO_VOLTAGE, NULL, 0), >> > + LMP92001_CHAN_SPEC(16, IIO_VOLTAGE, NULL, 0), >> > + LMP92001_CHAN_SPEC(17, IIO_TEMP, NULL, 0), >> > +}; >> > + >> > +static int lmp92001_adc_probe(struct platform_device *pdev) >> > +{ >> > + struct lmp92001 *lmp92001 = dev_get_drvdata(pdev->dev.parent); >> > + struct iio_dev *indio_dev; >> > + struct device_node *np = pdev->dev.of_node; >> > + const char *conversion; >> > + unsigned int cgen = 0, cad1, cad2, cad3; >> > + u32 mask; >> > + int ret; >> > + >> > + indio_dev = devm_iio_device_alloc(&pdev->dev, >> sizeof(*lmp92001)); >> > + if (!indio_dev) >> > + return -ENOMEM; >> > + >> > + iio_device_set_drvdata(indio_dev, lmp92001); >> > + >> > + indio_dev->name = pdev->name; >> > + indio_dev->dev.parent = &pdev->dev; >> > + indio_dev->modes = INDIO_DIRECT_MODE; >> > + indio_dev->info = &lmp92001_info; >> > + indio_dev->channels = lmp92001_adc_channels; >> > + indio_dev->num_channels = ARRAY_SIZE(lmp92001_adc_channels); >> > + >> > + ret = regmap_update_bits(lmp92001->regmap, LMP92001_CGEN, >> 0x80, 0x80); >> > + if (ret < 0) >> 0x80 is a magic number. Use a #define to give it meaning to someone who >> can't be bothered to find the datasheet (i.e. me ;) >> > > Abhisit: I will defined it. > > >> > + { >> > + dev_err(&pdev->dev,"failed to self reset all >> registers\n"); >> > + return ret; >> > + } >> > + >> > + ret = of_property_read_u32(np, "ti,lmp92001-adc-mask", &mask); >> > + if (ret < 0) >> > + { >> > + cad1 = cad2 = cad3 = 0xFF; >> > + dev_info(&pdev->dev, "turn on all of channels by >> default\n"); >> Talk me through this - why can we not turn them on when we want a reading? >> Just how slow is this device to start up? >> Or are we looking at stopping certain channels from being used because >> the pin >> is in use for something else? >> > > Abhisit: I would like to design to support initialization on start-up. > > >> >> > + } >> > + else >> > + { >> > + cad1 = mask & 0xFF; >> > + cad2 = (mask >> 8) & 0xFF; >> > + cad3 = (mask >> 16) & 0xFF; >> > + } >> > + >> > + ret = regmap_update_bits(lmp92001->regmap, LMP92001_CAD1, >> 0xFF, cad1); >> > + if (ret < 0) >> > + { >> > + dev_err(&pdev->dev,"failed to enable channels 1-8\n"); >> > + return ret; >> > + } >> > + >> > + ret = regmap_update_bits(lmp92001->regmap, LMP92001_CAD2, >> 0xFF, cad2); >> > + if (ret < 0) >> > + { >> > + dev_err(&pdev->dev, "failed to enable channels >> 9-16\n"); >> > + return ret; >> > + } >> > + >> > + ret = regmap_update_bits(lmp92001->regmap, LMP92001_CAD3, 1, >> cad3); >> > + if (ret < 0) >> > + { >> > + dev_err(&pdev->dev, "failed to enable channel 17 >> (temperature)\n"); >> > + return ret; >> These presumably need disabling again on remove or error to save power? >> >> Basic rule of thumb is that if something is setup in the probe routine, we >> expect it to be reversed in the remove and any error path in probe. >> > > Abhisit: This chip can not stop. We have no to do remove or disable it. > > >> > + } >> > + >> > + ret = of_property_read_string_index(np, >> "ti,lmp92001-adc-mode", 0, >> > + &conversion); >> > + if (!ret) >> > + { >> > + if (strcmp("continuous", conversion) == 0) >> > + cgen |= 1; >> > + else if (strcmp("single-shot", conversion) == 0) >> > + { /* Okay */ } >> > + else >> > + dev_warn(&pdev->dev, >> > + "wrong adc mode! set to single-short >> conversion\n"); >> Is this a characteristic of the hardware? Don't think so and isn't a >> policy >> that makes much sense to push into the devicetree. Just pick one as >> the default and let the driver figure out when to change. >> > > Abhisit: I will set continuous mode as default. > > >> >> > + } >> > + else >> > + dev_info(&pdev->dev, >> > + "single-short conversion was chosen by default\n"); >> > + >> > + /* >> > + * Lock the registers and set conversion mode. >> > + */ >> > + ret = regmap_update_bits(lmp92001->regmap, >> > + LMP92001_CGEN, 3, cgen | 2); >> > + if (ret < 0) >> > + return ret; >> > + >> > + platform_set_drvdata(pdev, indio_dev); >> > + >> > + return iio_device_register(indio_dev); >> > +} >> > + >> > +static int lmp92001_adc_remove(struct platform_device *pdev) >> > +{ >> > + struct iio_dev *indio_dev = platform_get_drvdata(pdev); >> > + >> > + iio_device_unregister(indio_dev); >> No disabling of the device to be done? >> > > Abhisit: HW is no way to stop. > > >> > + >> > + return 0; >> > +} >> > + >> > +static struct platform_driver lmp92001_adc_driver = { >> > + .driver.name = "lmp92001-adc", >> > + .driver.owner = THIS_MODULE, >> > + .probe = lmp92001_adc_probe, >> > + .remove = lmp92001_adc_remove, >> > +}; >> > + >> > +static int __init lmp92001_adc_init(void) >> > +{ >> > + return platform_driver_register(&lmp92001_adc_driver); >> > +} >> > +subsys_initcall(lmp92001_adc_init); >> > + >> > +static void __exit lmp92001_adc_exit(void) >> > +{ >> > + platform_driver_unregister(&lmp92001_adc_driver); >> > +} >> > +module_exit(lmp92001_adc_exit); >> > + >> > +MODULE_AUTHOR("Abhisit Sangjan "); >> > +MODULE_DESCRIPTION("IIO ADC interface for TI LMP92001"); >> > +MODULE_LICENSE("GPL"); >> > +MODULE_ALIAS("platform:lmp92001-adc"); >> >> >