iio: adc: add Nuvoton NCT7201 ADC driver
Subsystems (3)
Eason Yang
maintainerv6 cover iio: adc: add Nuvoton NCT7201 ADC driver
Change since version 6:
- Fix comments
- Add use_single_read in regmap_config
- Remove unused definitions
- Do not shadow the return code by -EIO and let regmap API caller to decide
- Use simple English in all error messages
- Use a local variable for the struct device pointers. This increases code readability with shortened lines.
- Use fsleep instead of mdelay
- Use 16 bits type __le16 instead of u8 data[2]
Change since version 5:
- Fix comments
- Add NUVOTON NCT7201 IIO DRIVER section in MAINTAINERS
- Add vdd-supply and vref-supply to the DT example
- Remove mutex since the regmap should already have an internal lock
- Remove redundant assigning values
- Check errors on regmap_write
Change since version 4:
- Fix comments
- Add interrupts and reset-gpios to the DT example
- Use the FIELD_PREP and FIELD_GET
- Add use_single_write in regmap_config
- Use regmap_access_table
Change since version 3:
- Fix comments
- Don't put nct720"x" in the name, just call it nct7201
- Remove differential inputs until conversions are finished
- Add NCT7201_ prefix in all macros and avoid the tables
- Correct event threshold values in raw units
- Add with and without interrupt callback function to have the event config part and one that doesn't
- Remove print an error message if regmap_wirte failed case
Change since version 2:
- Remvoe read-vin-data-size property, default use read word vin data
- Use regmap instead of i2c smbus API
- IIO should be IIO_CHAN_INFO_RAW and _SCALE not _PROCESSED
- Use dev_xxx_probe in probe function and dev_xxx in other functions
- Use devm_iio_device_register replace of iio_device_register
- Use guard(mutex) replace of mutex_lock
- Use get_unaligned_le16 conversion API
Changes since version 1:
- Add new property in iio:adc binding document
- Add new driver for Nuvoton NCT720x driver
Eason Yang (2):
dt-bindings: iio: adc: add NCT7201 ADCs
iio: adc: add support for Nuvoton NCT7201
.../bindings/iio/adc/nuvoton,nct7201.yaml | 70 +++ MAINTAINERS | 7 + drivers/iio/adc/Kconfig | 11 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/nct7201.c | 471 ++++++++++++++++++ 5 files changed, 560 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml create mode 100644 drivers/iio/adc/nct7201.c
Signature · 1 line
2.34.1
Eason Yang
maintainerPATCH v6 1/2 dt-bindings: iio: adc: add NCT7201 ADCs
Add a binding specification for the Nuvoton NCT7201/NCT7202 up to 12-bit ADCs with I2C interface.
--- .../bindings/iio/adc/nuvoton,nct7201.yaml | 70 +++++++++++++++++++ MAINTAINERS | 6 ++ 2 files changed, 76 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml
Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml new file +70 -0
@@ -0,0 +1,70 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/adc/nuvoton,nct7201.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Nuvoton nct7201 and similar ADCs + +maintainers: + - Eason Yang <[email protected]> + +description: | + The NCT7201/NCT7202 is a Nuvoton Hardware Monitor IC, contains up to 12 + voltage monitoring channels, with SMBus interface, and up to 4 sets SMBus + address selection by ADDR connection. It also provides ALERT# signal for + event notification and reset input RSTIN# to recover it from a fault + condition. + + NCT7201 contains 8 voltage monitor inputs (VIN1~VIN8). + NCT7202 contains 12 voltage monitor inputs (VIN1~VIN12). + +properties: + compatible: + enum: + - nuvoton,nct7201 + - nuvoton,nct7202 + + reg: + maxItems: 1 + + vdd-supply: + description: + A 3.3V to supply that powers the chip. + + vref-supply: + description: + The regulator supply for the ADC reference voltage. + + interrupts: + maxItems: 1 + + reset-gpios: + maxItems: 1 + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/interrupt-controller/irq.h> + + i2c { + #address-cells = <1>; + #size-cells = <0>; + + adc@1d { + compatible = "nuvoton,nct7202"; + reg = <0x1d>; + vdd-supply = <&vdd>; + vref-supply = <&vref>; + interrupt-parent = <&gpio3>; + interrupts = <30 IRQ_TYPE_LEVEL_LOW>; + reset-gpios = <&gpio3 28 GPIO_ACTIVE_LOW>; + }; + }; +...
MAINTAINERS +6 -0
@@ -17304,6 +17304,12 @@ F: drivers/nubus/ F: include/linux/nubus.h F: include/uapi/linux/nubus.h +NUVOTON NCT7201 IIO DRIVER +M: Eason Yang <[email protected]> +L: [email protected] +S: Maintained +F: Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml + NVIDIA (rivafb and nvidiafb) FRAMEBUFFER DRIVER M: Antonino Daplas <[email protected]> L: [email protected]
Signature · 1 line
2.34.1
Re: [PATCH v6 1/2] dt-bindings: iio: adc: add NCT7201 ADCs
Quoted 4 lines · Eason Yang
Add a binding specification for the Nuvoton NCT7201/NCT7202 up to 12-bit ADCs with I2C interface.
Signed-off-by Eason Yang <[email protected]>
Did you read the message you got from me last time?
<form letter>
This is a friendly reminder during the review process.
It looks like you received a tag and forgot to add it.
If you do not know the process, here is a short explanation:
Please add Acked-by/Reviewed-by/Tested-by tags when posting new versions of patchset, under or above your Signed-off-by tag, unless patch changed significantly (e.g. new properties added to the DT bindings). Tag is "received", when provided in a message replied to you on the mailing list. Tools like b4 can help here. However, there's no need to repost patches *only* to add the tags. The upstream maintainer will do that for tags received on the version they apply.
Please read:
https://elixir.bootlin.com/linux/v6.12-rc3/source/Documentation/process/submitting-patches.rst#L577
If a tag was not added on purpose, please state why and what changed. </form letter>
Best regards,
Krzysztof
Eason Yang
maintainerPATCH v6 2/2 iio: adc: add support for Nuvoton NCT7201
In reply to Eason YangAdd Nuvoton NCT7201/NCT7202 system voltage monitor 12-bit ADC driver
NCT7201/NCT7202 supports up to 12 analog voltage monitor inputs and up to 4 SMBus addresses by ADDR pin. Meanwhile, ALERT# hardware event pins for independent alarm signals, and all the threshold values could be set for system protection without any timing delay. It also supports reset input RSTIN# to recover system from a fault condition.
Currently, only single-edge mode conversion and threshold events are supported.
--- MAINTAINERS | 1 + drivers/iio/adc/Kconfig | 11 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/nct7201.c | 471 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 484 insertions(+) create mode 100644 drivers/iio/adc/nct7201.c
MAINTAINERS +1 -0
@@ -17309,6 +17309,7 @@ M: Eason Yang <[email protected]> L: [email protected] S: Maintained F: Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml +F: drivers/iio/adc/nct7201.c NVIDIA (rivafb and nvidiafb) FRAMEBUFFER DRIVER M: Antonino Daplas <[email protected]>
drivers/iio/adc/Kconfig +11 -0
@@ -1092,6 +1092,17 @@ config NAU7802 To compile this driver as a module, choose M here: the module will be called nau7802. +config NCT7201 + tristate "Nuvoton Instruments NCT7201 and NCT7202 Power Monitor" + depends on I2C + select REGMAP_I2C + help + If you say yes here you get support for the Nuvoton NCT7201 and + NCT7202 Voltage Monitor. + + This driver can also be built as a module. If so, the module + will be called nct7201. + config NPCM_ADC tristate "Nuvoton NPCM ADC driver" depends on ARCH_NPCM || COMPILE_TEST
drivers/iio/adc/Makefile +1 -0
@@ -97,6 +97,7 @@ obj-$(CONFIG_MESON_SARADC) += meson_saradc.o obj-$(CONFIG_MP2629_ADC) += mp2629_adc.o obj-$(CONFIG_MXS_LRADC_ADC) += mxs-lradc-adc.o obj-$(CONFIG_NAU7802) += nau7802.o +obj-$(CONFIG_NCT7201) += nct7201.o obj-$(CONFIG_NPCM_ADC) += npcm_adc.o obj-$(CONFIG_PAC1921) += pac1921.o obj-$(CONFIG_PAC1934) += pac1934.o
drivers/iio/adc/nct7201.c new file +471 -0
471 lines not on this page · raw message
Signature · 1 line
2.34.1
Re: [PATCH v6 2/2] iio: adc: add support for Nuvoton NCT7201
Quoted 10 lines · Eason Yang
Add Nuvoton NCT7201/NCT7202 system voltage monitor 12-bit ADC driver
NCT7201/NCT7202 supports up to 12 analog voltage monitor inputs and up to 4 SMBus addresses by ADDR pin. Meanwhile, ALERT# hardware event pins for independent alarm signals, and all the threshold values could be set for system protection without any timing delay. It also supports reset input RSTIN# to recover system from a fault condition.
Currently, only single-edge mode conversion and threshold events are supported.
...
Quoted 1 line
+#define
NCT7201_REG_VIN(i)(i)
This doesn't do anything useful. Why do you need this rather useless macro?
...
drivers/iio/adc/nct7201.c line 120
Quoted 2 lines
drivers/iio/adc/nct7201.c new file +2
-0+struct nct7201_chip_info { +struct device *dev;
This can be derived from the respective regmap. No need to have it here.
Quoted 5 lines
drivers/iio/adc/nct7201.c new file +5
-0+struct regmap *regmap; +struct regmap *regmap16; +int num_vin_channels; +u16 vin_mask; +};
...
drivers/iio/adc/nct7201.c line 130
Quoted 5 lines
drivers/iio/adc/nct7201.c new file +4
-0+struct nct7201_adc_model_data { +const char *model_name; +const struct iio_chan_spec *channels; +const int num_channels;
What is the meaning of const here?
Quoted 2 lines
(quoted diff) +2
-0+ int num_vin_channels; +};
...
drivers/iio/adc/nct7201.c line 152
Quoted 5 lines
drivers/iio/adc/nct7201.c new file +5
-0+#define NCT7201_VOLTAGE_CHANNEL (num) \ + { \ + .type = IIO_VOLTAGE, \ + .indexed = 1, \ + .channel = (num + 1), \
Parentheses are in a wrong place
Quoted 6 lines
drivers/iio/adc/nct7201.c new file +6
-0+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ + .address = num, \ + .event_spec = nct7201_events, \ + .num_event_specs = ARRAY_SIZE(nct7201_events), \ + }
...
Quoted 4 lines
drivers/iio/adc/nct7201.c new file +4
-0+static int nct7201_write_event_config(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir,
drivers/iio/adc/nct7201.c line 299
Quoted 10 lines
drivers/iio/adc/nct7201.c new file +9
-0+ bool state) +{ +struct nct7201_chip_info *chip =iio_priv (indio_dev); +unsigned int mask; +int err; + +if (chan->type !=IIO_VOLTAGE ) +return -EOPNOTSUPP ; + mask =BIT (chan->address);
Just join this with the definition above.
drivers/iio/adc/nct7201.c line 307
Quoted 7 lines
drivers/iio/adc/nct7201.c new file +7
-0+if (state) + chip->vin_mask |= mask; +else + chip->vin_mask &= ~mask; + +if (chip->num_vin_channels <=8 ) + err = regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1,
Remove _1.
drivers/iio/adc/nct7201.c line 315
Quoted 9 lines
drivers/iio/adc/nct7201.c new file +8
-0+ chip->vin_mask); + else + err =regmap_bulk_write (chip->regmap,NCT7201_REG_CHANNEL_ENABLE_1 , + &chip->vin_mask,sizeof (chip->vin_mask)); +if (err) +return err; + +return 0 ;
As simple as
return err;
Quoted 1 line
+}
...
drivers/iio/adc/nct7201.c line 346
Quoted 3 lines
drivers/iio/adc/nct7201.c new file +3
-0+static int nct7201_init_chip (struct nct7201_chip_info *chip) +{ +struct device *dev = chip->dev;
Derive this from chip->regmap.
drivers/iio/adc/nct7201.c line 358
Quoted 12 lines
drivers/iio/adc/nct7201.c new file +12
-0+__le16 data =cpu_to_le16 (NCT7201_REG_CHANNEL_ENABLE_MASK ); +unsigned int value; +int err; + + err =regmap_write (chip->regmap,NCT7201_REG_CONFIGURATION , +NCT7201_BIT_CONFIGURATION_RESET ); +if (err) +return dev_err_probe (dev, err,"Failed to reset chip\n" ); + +/* +* After about 25 msecs, the device should be ready and then the Power +* Up bit will be set to 1. If not, wait for it.
"Up bit" is odd, please be more specific.
drivers/iio/adc/nct7201.c line 360
Quoted 2 lines
drivers/iio/adc/nct7201.c new file +2
-0+*/ +fsleep (25000 );
+ Blank line.
Quoted 30 lines
drivers/iio/adc/nct7201.c new file +30
-0+ err =regmap_read (chip->regmap,NCT7201_REG_BUSY_STATUS , &value); +if (err) +return dev_err_probe (dev, err,"Failed to read busy status\n" ); +if (!(value &NCT7201_BIT_PWR_UP )) +return dev_err_probe (dev, -EIO ,"Failed to power up after reset\n" ); + +/* Enable Channel */ +if (chip->num_vin_channels <=8 ) + err =regmap_write (chip->regmap,NCT7201_REG_CHANNEL_ENABLE_1 , +NCT7201_REG_CHANNEL_ENABLE_MASK ); +else + err =regmap_bulk_write (chip->regmap,NCT7201_REG_CHANNEL_ENABLE_1 , + &data,sizeof (data)); +if (err) +return dev_err_probe (dev, err,"Failed to enable channel\n" ); + + err =regmap_bulk_read (chip->regmap,NCT7201_REG_CHANNEL_ENABLE_1 , + &chip->vin_mask,sizeof (chip->vin_mask)); +if (err) +return dev_err_probe (dev, err, +"Failed to read channel enable register\n" ); + +/* Start monitoring if needed */ + err =regmap_set_bits (chip->regmap,NCT7201_REG_CONFIGURATION , +NCT7201_BIT_CONFIGURATION_START ); +if (err) +return dev_err_probe (dev, err,"Failed to start monitoring\n" ); + +return 0 ; +}
...
drivers/iio/adc/nct7201.c line 424
Quoted 2 lines
drivers/iio/adc/nct7201.c new file +2
-0+ ret =nct7201_init_chip (chip); +if (ret <0 )
Why ' < 0' ?
Quoted 1 line
drivers/iio/adc/nct7201.c new file +1
-0+return ret;
Signature · 2 lines
With Best Regards, Andy Shevchenko
Yu-Hsian Yang
maintainerRe: [PATCH v6 2/2] iio: adc: add support for Nuvoton NCT7201
Dear Andy,
Thanks for the review and the comments.
Will fix all, have a few questions (below)
Andy Shevchenko <[email protected]> 於 2025年4月16日 週三 下午5:34寫道：
Quoted 10 lines · Eason Yang
Quoted 8 lines
Add Nuvoton NCT7201/NCT7202 system voltage monitor 12-bit ADC driver
NCT7201/NCT7202 supports up to 12 analog voltage monitor inputs and up to 4 SMBus addresses by ADDR pin. Meanwhile, ALERT# hardware event pins for independent alarm signals, and all the threshold values could be set for system protection without any timing delay. It also supports reset input RSTIN# to recover system from a fault condition.
Quoted 9 lines
Quoted 2 lines
Currently, only single-edge mode conversion and threshold events are supported.
...
Quoted 1 line
+#define
NCT7201_REG_VIN(i)(i)This doesn't do anything useful. Why do you need this rather useless macro?
Actually here we should define NCT7201_REG_VIN(i) as (0x00 + i), We simply it as (i).
Quoted 13 lines
...
drivers/iio/adc/nct7201.c line 120
Quoted 2 lines
drivers/iio/adc/nct7201.c new file +2
-0+struct nct7201_chip_info { +struct device *dev;This can be derived from the respective regmap. No need to have it here.
Quoted 5 lines
drivers/iio/adc/nct7201.c new file +5
-0+struct regmap *regmap; +struct regmap *regmap16; +int num_vin_channels; +u16 vin_mask; +};
Use regmap->dev is okay if use regmap API. But if we need to print message not from regmap API, how suggestions to do in this case?
The code is in nct7201_init_chip(struct nct7201_chip_info *chip)
...
Quoted 9 lines
Quoted 5 lines
drivers/iio/adc/nct7201.c new file +5
-0+ err =regmap_read (chip->regmap,NCT7201_REG_BUSY_STATUS , &value); +if (err) +return dev_err_probe (dev, err,"Failed to read busy status\n" ); +if (!(value &NCT7201_BIT_PWR_UP )) +return dev_err_probe (dev, -EIO ,"Failed to power up after reset\n" );...
Quoted 8 lines
drivers/iio/adc/nct7201.c line 130
Quoted 5 lines
drivers/iio/adc/nct7201.c new file +4
-0+struct nct7201_adc_model_data { +const char *model_name; +const struct iio_chan_spec *channels; +const int num_channels;What is the meaning of const here?
We don't need const int , just int.
Quoted 5 lines
Quoted 2 lines
(quoted diff) +2
-0+ int num_vin_channels; +};...
Quoted 8 lines
drivers/iio/adc/nct7201.c line 152
Quoted 5 lines
drivers/iio/adc/nct7201.c new file +5
-0+#define NCT7201_VOLTAGE_CHANNEL (num) \ + { \ + .type = IIO_VOLTAGE, \ + .indexed = 1, \ + .channel = (num + 1), \Parentheses are in a wrong place
We remove parentheses.
Quoted 22 lines
Quoted 6 lines
drivers/iio/adc/nct7201.c new file +6
-0+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ + .address = num, \ + .event_spec = nct7201_events, \ + .num_event_specs = ARRAY_SIZE(nct7201_events), \ + }...
Quoted 12 lines
drivers/iio/adc/nct7201.c new file +12
-0+static int nct7201_write_event_config (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan, +enum iio_event_type type, +enum iio_event_direction dir, +bool state) +{ +struct nct7201_chip_info *chip =iio_priv (indio_dev); +unsigned int mask; +int err; + +if (chan->type !=IIO_VOLTAGE ) +return -EOPNOTSUPP ;
Quoted 5 lines
Quoted 1 line
+ mask =
BIT(chan->address);Just join this with the definition above.
okay
Quoted 99 lines
drivers/iio/adc/nct7201.c line 307
Quoted 7 lines
drivers/iio/adc/nct7201.c new file +7
-0+if (state) + chip->vin_mask |= mask; +else + chip->vin_mask &= ~mask; + +if (chip->num_vin_channels <=8 ) + err = regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1,Remove _1.
drivers/iio/adc/nct7201.c line 315
Quoted 9 lines
drivers/iio/adc/nct7201.c new file +8
-0+ chip->vin_mask); + else + err =regmap_bulk_write (chip->regmap,NCT7201_REG_CHANNEL_ENABLE_1 , + &chip->vin_mask,sizeof (chip->vin_mask)); +if (err) +return err; + +return 0 ;As simple as
return err;Quoted 1 line
+}
...
drivers/iio/adc/nct7201.c line 346
Quoted 3 lines
drivers/iio/adc/nct7201.c new file +3
-0+static int nct7201_init_chip (struct nct7201_chip_info *chip) +{ +struct device *dev = chip->dev;Derive this from
chip->regmap.drivers/iio/adc/nct7201.c line 358
Quoted 12 lines
drivers/iio/adc/nct7201.c new file +12
-0+__le16 data =cpu_to_le16 (NCT7201_REG_CHANNEL_ENABLE_MASK ); +unsigned int value; +int err; + + err =regmap_write (chip->regmap,NCT7201_REG_CONFIGURATION , +NCT7201_BIT_CONFIGURATION_RESET ); +if (err) +return dev_err_probe (dev, err,"Failed to reset chip\n" ); + +/* +* After about 25 msecs, the device should be ready and then the Power +* Up bit will be set to 1. If not, wait for it. "Up bit" is odd, please be more specific.
drivers/iio/adc/nct7201.c line 360
Quoted 2 lines
drivers/iio/adc/nct7201.c new file +2
-0+*/ +fsleep (25000 );+ Blank line.
Quoted 30 lines
drivers/iio/adc/nct7201.c new file +30
-0+ err =regmap_read (chip->regmap,NCT7201_REG_BUSY_STATUS , &value); +if (err) +return dev_err_probe (dev, err,"Failed to read busy status\n" ); +if (!(value &NCT7201_BIT_PWR_UP )) +return dev_err_probe (dev, -EIO ,"Failed to power up after reset\n" ); + +/* Enable Channel */ +if (chip->num_vin_channels <=8 ) + err =regmap_write (chip->regmap,NCT7201_REG_CHANNEL_ENABLE_1 , +NCT7201_REG_CHANNEL_ENABLE_MASK ); +else + err =regmap_bulk_write (chip->regmap,NCT7201_REG_CHANNEL_ENABLE_1 , + &data,sizeof (data)); +if (err) +return dev_err_probe (dev, err,"Failed to enable channel\n" ); + + err =regmap_bulk_read (chip->regmap,NCT7201_REG_CHANNEL_ENABLE_1 , + &chip->vin_mask,sizeof (chip->vin_mask)); +if (err) +return dev_err_probe (dev, err, +"Failed to read channel enable register\n" ); + +/* Start monitoring if needed */ + err =regmap_set_bits (chip->regmap,NCT7201_REG_CONFIGURATION , +NCT7201_BIT_CONFIGURATION_START ); +if (err) +return dev_err_probe (dev, err,"Failed to start monitoring\n" ); + +return 0 ; +}...
drivers/iio/adc/nct7201.c line 424
Quoted 2 lines
drivers/iio/adc/nct7201.c new file +2
-0+ ret =nct7201_init_chip (chip); +if (ret <0 )Why ' < 0' ?
Quoted 1 line
+ return ret;
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v6 2/2] iio: adc: add support for Nuvoton NCT7201
Sun, Apr 20, 2025 at 09:03:05PM +0800, Yu-Hsian Yang kirjoitti:
Quoted 2 lines
Andy Shevchenko <[email protected]> 於 2025年4月16日 週三 下午5:34寫道：
Quoted 1 line · Eason Yang
...
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
+#define
NCT7201_REG_VIN(i)(i)This doesn't do anything useful. Why do you need this rather useless macro?
Actually here we should define
NCT7201_REG_VIN(i)as (0x00+ i), We simply it as (i).
Please, don't. Use the full form which makes a big difference to the perception of this macro.
...
Quoted 5 lines
Quoted 5 lines
drivers/iio/adc/nct7201.c line 120
Quoted 2 lines
drivers/iio/adc/nct7201.c new file +2
-0+struct nct7201_chip_info { +struct device *dev;This can be derived from the respective regmap. No need to have it here.
Quoted 7 lines
Quoted 5 lines
drivers/iio/adc/nct7201.c line 125
Quoted 5 lines
drivers/iio/adc/nct7201.c new file +5
-0+struct regmap *regmap; +struct regmap *regmap16; +int num_vin_channels; +u16 vin_mask; +};Use
regmap->devis okay if use regmap API.
No, you are not supposed to use regmap->dev (and you can't), you need to call a getter API and get the device pointer.
Quoted 1 line
But if we need to print message not from regmap API,
I don't get this. What do you mean? The example you showed prints a message for the device. The same device that was used to create a regmap.
Quoted 1 line
how suggestions to do in this case?
Use the one that you can retrieve from regmap.
...
So, you have commented on some with agreement, and left uncommented a lot. Does it mean you are agree on all points? The rule of thumb do not comment on the cases you are fully agree with. Current email just makes a confusion.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Jonathan Cameron
maintainerRe: [PATCH v6 2/2] iio: adc: add support for Nuvoton NCT7201
In reply to Eason YangQuoted 8 lines · On Wed, 16 Apr 2025 16:17:34 +0800 Eason Yang <[email protected]> wrote:
Add Nuvoton NCT7201/NCT7202 system voltage monitor 12-bit ADC driver
NCT7201/NCT7202 supports up to 12 analog voltage monitor inputs and up to 4 SMBus addresses by ADDR pin. Meanwhile, ALERT# hardware event pins for independent alarm signals, and all the threshold values could be set for system protection without any timing delay. It also supports reset input RSTIN# to recover system from a fault condition.
Quoted 4 lines
Currently, only single-edge mode conversion and threshold events are supported.
Signed-off-by Eason Yang <[email protected]>
Hi Eason,
A few trivial things from another glance through.
Thanks,
Jonathan
Quoted 9 lines
drivers/iio/adc/nct7201.c new file +9
-0+ +static const struct nct7201_adc_model_data nct7201_model_data = { + .model_name ="nct7201" , + .channels = nct7201_channels, + .num_channels =ARRAY_SIZE (nct7201_channels), + .num_vin_channels =8 , +}; + +static const struct nct7201_adc_model_data nct7202_model_data = {
drivers/iio/adc/nct7201.c line 347
Quoted 10 lines
drivers/iio/adc/nct7201.c new file +10
-0+ .model_name = "nct7202", + .channels = nct7202_channels, + .num_channels = ARRAY_SIZE(nct7202_channels), + .num_vin_channels = 12, +}; + +static int nct7201_init_chip (struct nct7201_chip_info *chip) +{ +struct device *dev = chip->dev; +__le16 data =cpu_to_le16 (NCT7201_REG_CHANNEL_ENABLE_MASK );
Assign this value down near where it is used. That will generally help readability.
drivers/iio/adc/nct7201.c line 358
Quoted 11 lines
drivers/iio/adc/nct7201.c new file +11
-0+unsigned int value; +int err; + + err =regmap_write (chip->regmap,NCT7201_REG_CONFIGURATION , +NCT7201_BIT_CONFIGURATION_RESET ); +if (err) +return dev_err_probe (dev, err,"Failed to reset chip\n" ); + +/* +* After about 25 msecs, the device should be ready and then the Power +* Up bit will be set to 1. If not, wait for it.
I'd hyphenate power-up perhaps.
drivers/iio/adc/nct7201.c line 370
Quoted 12 lines
drivers/iio/adc/nct7201.c new file +12
-0+*/ +fsleep (25000 ); + err =regmap_read (chip->regmap,NCT7201_REG_BUSY_STATUS , &value); +if (err) +return dev_err_probe (dev, err,"Failed to read busy status\n" ); +if (!(value &NCT7201_BIT_PWR_UP )) +return dev_err_probe (dev, -EIO ,"Failed to power up after reset\n" ); + +/* Enable Channel */ +if (chip->num_vin_channels <=8 ) + err =regmap_write (chip->regmap,NCT7201_REG_CHANNEL_ENABLE_1 , +NCT7201_REG_CHANNEL_ENABLE_MASK );
This is a little odd as I think you are writing an 8 bit register with a 12 bit mask. Sure it works, but confusing thing to see. If some future 6 channel device comes along this <= 8 will seem odd too. Maybe generate the mask from teh number of channels?
Quoted 20 lines
drivers/iio/adc/nct7201.c new file +20
-0+else + err =regmap_bulk_write (chip->regmap,NCT7201_REG_CHANNEL_ENABLE_1 , + &data,sizeof (data)); +if (err) +return dev_err_probe (dev, err,"Failed to enable channel\n" ); + + err =regmap_bulk_read (chip->regmap,NCT7201_REG_CHANNEL_ENABLE_1 , + &chip->vin_mask,sizeof (chip->vin_mask)); +if (err) +return dev_err_probe (dev, err, +"Failed to read channel enable register\n" ); + +/* Start monitoring if needed */ + err =regmap_set_bits (chip->regmap,NCT7201_REG_CONFIGURATION , +NCT7201_BIT_CONFIGURATION_START ); +if (err) +return dev_err_probe (dev, err,"Failed to start monitoring\n" ); + +return 0 ; +}
Re: [PATCH v6 2/2] iio: adc: add support for Nuvoton NCT7201
Quoted 2 lines · Jonathan Cameron
On Wed, 16 Apr 2025 16:17:34 +0800
Eason Yang <[email protected]> wrote:
...
Quoted 5 lines
drivers/iio/adc/nct7201.c line 347
Quoted 2 lines
drivers/iio/adc/nct7201.c new file +2
-0+struct device *dev = chip->dev; +__le16 data =cpu_to_le16 (NCT7201_REG_CHANNEL_ENABLE_MASK );Assign this value down near where it is used. That will generally help readability.
Actually it is a good, but not a main reason, the problem with this style is long-term maintenance when some code may appear in between and become so long that one can screw up this assignment at some point.
Signature · 2 lines
With Best Regards, Andy Shevchenko