* Re: [PATCH v1 2/2] iio: adc: add Nuvoton NCT720x ADC driver
2024-11-06 2:39 ` [PATCH v1 2/2] iio: adc: add Nuvoton NCT720x ADC driver Eason Yang
@ 2024-11-06 12:30 ` kernel test robot
2024-11-07 0:36 ` Yu-Hsian Yang
2024-11-06 13:41 ` Krzysztof Kozlowski
` (3 subsequent siblings)
4 siblings, 1 reply; 26+ messages in thread
From: kernel test robot @ 2024-11-06 12:30 UTC (permalink / raw)
To: Eason Yang, avifishman70, tmaimon77, tali.perry1, venture, yuenn,
benjaminfair, jic23, lars, robh, krzk+dt, conor+dt, nuno.sa,
dlechner, javier.carrasco.cruz, andy, marcelo.schmitt,
olivier.moysan, mitrutzceclan, matteomartelli3, alisadariana,
joao.goncalves, marius.cristea, mike.looijmans, chanh, KWLIU,
yhyang2
Cc: llvm, oe-kbuild-all, openbmc, linux-iio, devicetree, linux-kernel
Hi Eason,
kernel test robot noticed the following build warnings:
[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on linus/master v6.12-rc6 next-20241106]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Eason-Yang/dt-bindings-iio-adc-Add-binding-for-Nuvoton-NCT720x-ADCs/20241106-104046
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link: https://lore.kernel.org/r/20241106023916.440767-3-j2anfernee%40gmail.com
patch subject: [PATCH v1 2/2] iio: adc: add Nuvoton NCT720x ADC driver
config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20241106/202411062051.TLRkJSSL-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 592c0fe55f6d9a811028b5f3507be91458ab2713)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241106/202411062051.TLRkJSSL-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411062051.TLRkJSSL-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from drivers/iio/adc/nct720x.c:9:
In file included from include/linux/device.h:32:
In file included from include/linux/device/driver.h:21:
In file included from include/linux/module.h:19:
In file included from include/linux/elf.h:6:
In file included from arch/s390/include/asm/elf.h:181:
In file included from arch/s390/include/asm/mmu_context.h:11:
In file included from arch/s390/include/asm/pgalloc.h:18:
In file included from include/linux/mm.h:2213:
include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
504 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
505 | item];
| ~~~~
include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
511 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
512 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
518 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
524 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
525 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
>> drivers/iio/adc/nct720x.c:526:16: warning: cast to smaller integer type 'enum nct720x_chips' from 'const void *' [-Wvoid-pointer-to-enum-cast]
526 | chip->type = (enum nct720x_chips)device_get_match_data(&client->dev);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
5 warnings generated.
vim +526 drivers/iio/adc/nct720x.c
511
512 static int nct720x_probe(struct i2c_client *client)
513 {
514 const struct i2c_device_id *id = i2c_client_get_device_id(client);
515 struct nct720x_chip_info *chip;
516 struct iio_dev *indio_dev;
517 int ret;
518 u32 tmp;
519
520 indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
521 if (!indio_dev)
522 return -ENOMEM;
523 chip = iio_priv(indio_dev);
524
525 if (client->dev.of_node)
> 526 chip->type = (enum nct720x_chips)device_get_match_data(&client->dev);
527 else
528 chip->type = i2c_match_id(nct720x_id, client)->driver_data;
529
530 chip->vin_max = (chip->type == nct7201) ? NCT7201_VIN_MAX : NCT7202_VIN_MAX;
531
532 ret = of_property_read_u32(client->dev.of_node, "read-vin-data-size", &tmp);
533 if (ret < 0) {
534 pr_err("read-vin-data-size property not found\n");
535 return ret;
536 }
537
538 if (tmp == 8) {
539 chip->use_read_byte_vin = true;
540 } else if (tmp == 16) {
541 chip->use_read_byte_vin = false;
542 } else {
543 pr_err("invalid read-vin-data-size (%d)\n", tmp);
544 return -EINVAL;
545 }
546
547 mutex_init(&chip->access_lock);
548
549 /* this is only used for device removal purposes */
550 i2c_set_clientdata(client, indio_dev);
551
552 chip->client = client;
553
554 ret = nct720x_init_chip(chip);
555 if (ret < 0)
556 return ret;
557
558 indio_dev->name = id->name;
559 indio_dev->channels = nct720x_channels;
560 indio_dev->num_channels = ARRAY_SIZE(nct720x_channels);
561 indio_dev->info = &nct720x_info;
562 indio_dev->modes = INDIO_DIRECT_MODE;
563
564 iio_device_register(indio_dev);
565
566 return 0;
567 }
568
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v1 2/2] iio: adc: add Nuvoton NCT720x ADC driver
2024-11-06 12:30 ` kernel test robot
@ 2024-11-07 0:36 ` Yu-Hsian Yang
0 siblings, 0 replies; 26+ messages in thread
From: Yu-Hsian Yang @ 2024-11-07 0:36 UTC (permalink / raw)
To: kernel test robot
Cc: avifishman70, tmaimon77, tali.perry1, venture, yuenn,
benjaminfair, jic23, lars, robh, krzk+dt, conor+dt, nuno.sa,
dlechner, javier.carrasco.cruz, andy, marcelo.schmitt,
olivier.moysan, mitrutzceclan, matteomartelli3, alisadariana,
joao.goncalves, marius.cristea, mike.looijmans, chanh, KWLIU,
yhyang2, llvm, oe-kbuild-all, openbmc, linux-iio, devicetree,
linux-kernel
Dear kernel test robot,
Thank you for your response.
I would check these warnings.
kernel test robot <lkp@intel.com> 於 2024年11月6日 週三 下午8:32寫道：
>
> Hi Eason,
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on jic23-iio/togreg]
> [also build test WARNING on linus/master v6.12-rc6 next-20241106]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Eason-Yang/dt-bindings-iio-adc-Add-binding-for-Nuvoton-NCT720x-ADCs/20241106-104046
> base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
> patch link: https://lore.kernel.org/r/20241106023916.440767-3-j2anfernee%40gmail.com
> patch subject: [PATCH v1 2/2] iio: adc: add Nuvoton NCT720x ADC driver
> config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20241106/202411062051.TLRkJSSL-lkp@intel.com/config)
> compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 592c0fe55f6d9a811028b5f3507be91458ab2713)
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241106/202411062051.TLRkJSSL-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202411062051.TLRkJSSL-lkp@intel.com/
>
> All warnings (new ones prefixed by >>):
>
> In file included from drivers/iio/adc/nct720x.c:9:
> In file included from include/linux/device.h:32:
> In file included from include/linux/device/driver.h:21:
> In file included from include/linux/module.h:19:
> In file included from include/linux/elf.h:6:
> In file included from arch/s390/include/asm/elf.h:181:
> In file included from arch/s390/include/asm/mmu_context.h:11:
> In file included from arch/s390/include/asm/pgalloc.h:18:
> In file included from include/linux/mm.h:2213:
> include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
> 504 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
> | ~~~~~~~~~~~~~~~~~~~~~ ^
> 505 | item];
> | ~~~~
> include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
> 511 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
> | ~~~~~~~~~~~~~~~~~~~~~ ^
> 512 | NR_VM_NUMA_EVENT_ITEMS +
> | ~~~~~~~~~~~~~~~~~~~~~~
> include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
> 518 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
> | ~~~~~~~~~~~ ^ ~~~
> include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
> 524 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
> | ~~~~~~~~~~~~~~~~~~~~~ ^
> 525 | NR_VM_NUMA_EVENT_ITEMS +
> | ~~~~~~~~~~~~~~~~~~~~~~
> >> drivers/iio/adc/nct720x.c:526:16: warning: cast to smaller integer type 'enum nct720x_chips' from 'const void *' [-Wvoid-pointer-to-enum-cast]
> 526 | chip->type = (enum nct720x_chips)device_get_match_data(&client->dev);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 5 warnings generated.
>
>
> vim +526 drivers/iio/adc/nct720x.c
>
> 511
> 512 static int nct720x_probe(struct i2c_client *client)
> 513 {
> 514 const struct i2c_device_id *id = i2c_client_get_device_id(client);
> 515 struct nct720x_chip_info *chip;
> 516 struct iio_dev *indio_dev;
> 517 int ret;
> 518 u32 tmp;
> 519
> 520 indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
> 521 if (!indio_dev)
> 522 return -ENOMEM;
> 523 chip = iio_priv(indio_dev);
> 524
> 525 if (client->dev.of_node)
> > 526 chip->type = (enum nct720x_chips)device_get_match_data(&client->dev);
> 527 else
> 528 chip->type = i2c_match_id(nct720x_id, client)->driver_data;
> 529
> 530 chip->vin_max = (chip->type == nct7201) ? NCT7201_VIN_MAX : NCT7202_VIN_MAX;
> 531
> 532 ret = of_property_read_u32(client->dev.of_node, "read-vin-data-size", &tmp);
> 533 if (ret < 0) {
> 534 pr_err("read-vin-data-size property not found\n");
> 535 return ret;
> 536 }
> 537
> 538 if (tmp == 8) {
> 539 chip->use_read_byte_vin = true;
> 540 } else if (tmp == 16) {
> 541 chip->use_read_byte_vin = false;
> 542 } else {
> 543 pr_err("invalid read-vin-data-size (%d)\n", tmp);
> 544 return -EINVAL;
> 545 }
> 546
> 547 mutex_init(&chip->access_lock);
> 548
> 549 /* this is only used for device removal purposes */
> 550 i2c_set_clientdata(client, indio_dev);
> 551
> 552 chip->client = client;
> 553
> 554 ret = nct720x_init_chip(chip);
> 555 if (ret < 0)
> 556 return ret;
> 557
> 558 indio_dev->name = id->name;
> 559 indio_dev->channels = nct720x_channels;
> 560 indio_dev->num_channels = ARRAY_SIZE(nct720x_channels);
> 561 indio_dev->info = &nct720x_info;
> 562 indio_dev->modes = INDIO_DIRECT_MODE;
> 563
> 564 iio_device_register(indio_dev);
> 565
> 566 return 0;
> 567 }
> 568
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 2/2] iio: adc: add Nuvoton NCT720x ADC driver
2024-11-06 2:39 ` [PATCH v1 2/2] iio: adc: add Nuvoton NCT720x ADC driver Eason Yang
2024-11-06 12:30 ` kernel test robot
@ 2024-11-06 13:41 ` Krzysztof Kozlowski
2024-11-07 0:41 ` Yu-Hsian Yang
2024-11-07 6:13 ` anish kumar
` (2 subsequent siblings)
4 siblings, 1 reply; 26+ messages in thread
From: Krzysztof Kozlowski @ 2024-11-06 13:41 UTC (permalink / raw)
To: Eason Yang, avifishman70, tmaimon77, tali.perry1, venture, yuenn,
benjaminfair, jic23, lars, robh, krzk+dt, conor+dt, nuno.sa,
dlechner, javier.carrasco.cruz, andy, marcelo.schmitt,
olivier.moysan, mitrutzceclan, matteomartelli3, alisadariana,
joao.goncalves, marius.cristea, mike.looijmans, chanh, KWLIU,
yhyang2
Cc: openbmc, linux-iio, devicetree, linux-kernel
On 06/11/2024 03:39, Eason Yang 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 the all 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 support.
>
> Signed-off-by: Eason Yang <j2anfernee@gmail.com>
...
> +
> +static int nct720x_probe(struct i2c_client *client)
> +{
> + const struct i2c_device_id *id = i2c_client_get_device_id(client);
> + struct nct720x_chip_info *chip;
> + struct iio_dev *indio_dev;
> + int ret;
> + u32 tmp;
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
> + if (!indio_dev)
> + return -ENOMEM;
> + chip = iio_priv(indio_dev);
> +
> + if (client->dev.of_node)
> + chip->type = (enum nct720x_chips)device_get_match_data(&client->dev);
> + else
> + chip->type = i2c_match_id(nct720x_id, client)->driver_data;
I believe there is a I2C wrapper for above.
> +
> + chip->vin_max = (chip->type == nct7201) ? NCT7201_VIN_MAX : NCT7202_VIN_MAX;
> +
> + ret = of_property_read_u32(client->dev.of_node, "read-vin-data-size", &tmp);
> + if (ret < 0) {
> + pr_err("read-vin-data-size property not found\n");
Please use dev_xxx in your driver code.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v1 2/2] iio: adc: add Nuvoton NCT720x ADC driver
2024-11-06 13:41 ` Krzysztof Kozlowski
@ 2024-11-07 0:41 ` Yu-Hsian Yang
2024-11-09 13:45 ` Jonathan Cameron
0 siblings, 1 reply; 26+ messages in thread
From: Yu-Hsian Yang @ 2024-11-07 0:41 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: avifishman70, tmaimon77, tali.perry1, venture, yuenn,
benjaminfair, jic23, lars, robh, krzk+dt, conor+dt, nuno.sa,
dlechner, javier.carrasco.cruz, andy, marcelo.schmitt,
olivier.moysan, mitrutzceclan, matteomartelli3, alisadariana,
joao.goncalves, marius.cristea, mike.looijmans, chanh, KWLIU,
yhyang2, openbmc, linux-iio, devicetree, linux-kernel
Dear Krzysztof Kozlowski,
Thank you for your response.
Krzysztof Kozlowski <krzk@kernel.org> 於 2024年11月6日 週三 下午9:41寫道：
>
> On 06/11/2024 03:39, Eason Yang 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 the all 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 support.
> >
> > Signed-off-by: Eason Yang <j2anfernee@gmail.com>
>
> ...
>
> > +
> > +static int nct720x_probe(struct i2c_client *client)
> > +{
> > + const struct i2c_device_id *id = i2c_client_get_device_id(client);
> > + struct nct720x_chip_info *chip;
> > + struct iio_dev *indio_dev;
> > + int ret;
> > + u32 tmp;
> > +
> > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > + chip = iio_priv(indio_dev);
> > +
> > + if (client->dev.of_node)
> > + chip->type = (enum nct720x_chips)device_get_match_data(&client->dev);
> > + else
> > + chip->type = i2c_match_id(nct720x_id, client)->driver_data;
>
> I believe there is a I2C wrapper for above.
>
Got it.
> > +
> > + chip->vin_max = (chip->type == nct7201) ? NCT7201_VIN_MAX : NCT7202_VIN_MAX;
> > +
> > + ret = of_property_read_u32(client->dev.of_node, "read-vin-data-size", &tmp);
> > + if (ret < 0) {
> > + pr_err("read-vin-data-size property not found\n");
>
> Please use dev_xxx in your driver code.
Got it.
>
>
> Best regards,
> Krzysztof
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v1 2/2] iio: adc: add Nuvoton NCT720x ADC driver
2024-11-07 0:41 ` Yu-Hsian Yang
@ 2024-11-09 13:45 ` Jonathan Cameron
2024-12-04 1:49 ` Yu-Hsian Yang
0 siblings, 1 reply; 26+ messages in thread
From: Jonathan Cameron @ 2024-11-09 13:45 UTC (permalink / raw)
To: Yu-Hsian Yang
Cc: Krzysztof Kozlowski, avifishman70, tmaimon77, tali.perry1,
venture, yuenn, benjaminfair, lars, robh, krzk+dt, conor+dt,
nuno.sa, dlechner, javier.carrasco.cruz, andy, marcelo.schmitt,
olivier.moysan, mitrutzceclan, matteomartelli3, alisadariana,
joao.goncalves, marius.cristea, mike.looijmans, chanh, KWLIU,
yhyang2, openbmc, linux-iio, devicetree, linux-kernel
On Thu, 7 Nov 2024 08:41:21 +0800
Yu-Hsian Yang <j2anfernee@gmail.com> wrote:
> Dear Krzysztof Kozlowski,
>
> Thank you for your response.
>
> Krzysztof Kozlowski <krzk@kernel.org> 於 2024年11月6日 週三 下午9:41寫道：
> >
> > On 06/11/2024 03:39, Eason Yang 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 the all 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 support.
> > >
> > > Signed-off-by: Eason Yang <j2anfernee@gmail.com>
> >
> > ...
> >
> > > +
> > > +static int nct720x_probe(struct i2c_client *client)
> > > +{
> > > + const struct i2c_device_id *id = i2c_client_get_device_id(client);
> > > + struct nct720x_chip_info *chip;
> > > + struct iio_dev *indio_dev;
> > > + int ret;
> > > + u32 tmp;
> > > +
> > > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
> > > + if (!indio_dev)
> > > + return -ENOMEM;
> > > + chip = iio_priv(indio_dev);
> > > +
> > > + if (client->dev.of_node)
> > > + chip->type = (enum nct720x_chips)device_get_match_data(&client->dev);
> > > + else
> > > + chip->type = i2c_match_id(nct720x_id, client)->driver_data;
> >
> > I believe there is a I2C wrapper for above.
> >
>
> Got it.
Don't pass an enum value as data. Pass a pointer to a structure that describes the particular
variant. The 0 value which tends to end up in enums is an error for device_get_match_data.
>
> > > +
> > > + chip->vin_max = (chip->type == nct7201) ? NCT7201_VIN_MAX : NCT7202_VIN_MAX;
> > > +
> > > + ret = of_property_read_u32(client->dev.of_node, "read-vin-data-size", &tmp);
> > > + if (ret < 0) {
> > > + pr_err("read-vin-data-size property not found\n");
> >
> > Please use dev_xxx in your driver code.
>
> Got it.
>
> >
> >
> > Best regards,
> > Krzysztof
> >
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v1 2/2] iio: adc: add Nuvoton NCT720x ADC driver
2024-11-09 13:45 ` Jonathan Cameron
@ 2024-12-04 1:49 ` Yu-Hsian Yang
0 siblings, 0 replies; 26+ messages in thread
From: Yu-Hsian Yang @ 2024-12-04 1:49 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Krzysztof Kozlowski, avifishman70, tmaimon77, tali.perry1,
venture, yuenn, benjaminfair, lars, robh, krzk+dt, conor+dt,
nuno.sa, dlechner, javier.carrasco.cruz, andy, marcelo.schmitt,
olivier.moysan, mitrutzceclan, matteomartelli3, alisadariana,
joao.goncalves, marius.cristea, mike.looijmans, chanh, KWLIU,
yhyang2, openbmc, linux-iio, devicetree, linux-kernel
Thank you for your comment.
Jonathan Cameron <jic23@kernel.org> 於 2024年11月9日 週六 下午9:45寫道：
>
> On Thu, 7 Nov 2024 08:41:21 +0800
> Yu-Hsian Yang <j2anfernee@gmail.com> wrote:
>
> > Dear Krzysztof Kozlowski,
> >
> > Thank you for your response.
> >
> > Krzysztof Kozlowski <krzk@kernel.org> 於 2024年11月6日 週三 下午9:41寫道：
> > >
> > > On 06/11/2024 03:39, Eason Yang 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 the all 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 support.
> > > >
> > > > Signed-off-by: Eason Yang <j2anfernee@gmail.com>
> > >
> > > ...
> > >
> > > > +
> > > > +static int nct720x_probe(struct i2c_client *client)
> > > > +{
> > > > + const struct i2c_device_id *id = i2c_client_get_device_id(client);
> > > > + struct nct720x_chip_info *chip;
> > > > + struct iio_dev *indio_dev;
> > > > + int ret;
> > > > + u32 tmp;
> > > > +
> > > > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
> > > > + if (!indio_dev)
> > > > + return -ENOMEM;
> > > > + chip = iio_priv(indio_dev);
> > > > +
> > > > + if (client->dev.of_node)
> > > > + chip->type = (enum nct720x_chips)device_get_match_data(&client->dev);
> > > > + else
> > > > + chip->type = i2c_match_id(nct720x_id, client)->driver_data;
> > >
> > > I believe there is a I2C wrapper for above.
> > >
> >
> > Got it.
>
> Don't pass an enum value as data. Pass a pointer to a structure that describes the particular
> variant. The 0 value which tends to end up in enums is an error for device_get_match_data.
>
I would pass a pointer to the data structure not id to describe the
particular variant.
I would rewrite the code as below,
static const struct nct720x_adc_model_data nct7201_model_data = {
.model_name = "nct7201",
.channels = nct7201_channels,
.num_channels = ARRAY_SIZE(nct7201_channels),
.vin_max = 8,
};
static const struct nct720x_adc_model_data nct7202_model_data = {
.model_name = "nct7202",
.channels = nct7202_channels,
.num_channels = ARRAY_SIZE(nct7202_channels),
.vin_max = 12,
};
static const struct i2c_device_id nct720x_id[] = {
{ "nct7201", (kernel_ulong_t)&nct7201_model_data },
{ "nct7202", (kernel_ulong_t)&nct7202_model_data },
{ }
};
MODULE_DEVICE_TABLE(i2c, nct720x_id);
static const struct of_device_id nct720x_of_match[] = {
{
.compatible = "nuvoton,nct7201",
.data = &nct7201_model_data,
},
{
.compatible = "nuvoton,nct7202",
.data = &nct7202_model_data,
},
{ }
};
MODULE_DEVICE_TABLE(of, nct720x_of_match);
> >
> > > > +
> > > > + chip->vin_max = (chip->type == nct7201) ? NCT7201_VIN_MAX : NCT7202_VIN_MAX;
> > > > +
> > > > + ret = of_property_read_u32(client->dev.of_node, "read-vin-data-size", &tmp);
> > > > + if (ret < 0) {
> > > > + pr_err("read-vin-data-size property not found\n");
> > >
> > > Please use dev_xxx in your driver code.
> >
> > Got it.
> >
> > >
> > >
> > > Best regards,
> > > Krzysztof
> > >
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 2/2] iio: adc: add Nuvoton NCT720x ADC driver
2024-11-06 2:39 ` [PATCH v1 2/2] iio: adc: add Nuvoton NCT720x ADC driver Eason Yang
2024-11-06 12:30 ` kernel test robot
2024-11-06 13:41 ` Krzysztof Kozlowski
@ 2024-11-07 6:13 ` anish kumar
2024-11-07 7:17 ` Yu-Hsian Yang
2024-11-07 8:02 ` Andy Shevchenko
2024-11-09 14:37 ` Jonathan Cameron
4 siblings, 1 reply; 26+ messages in thread
From: anish kumar @ 2024-11-07 6:13 UTC (permalink / raw)
To: Eason Yang
Cc: avifishman70, tmaimon77, tali.perry1, venture, yuenn,
benjaminfair, jic23, lars, robh, krzk+dt, conor+dt, nuno.sa,
dlechner, javier.carrasco.cruz, andy, marcelo.schmitt,
olivier.moysan, mitrutzceclan, matteomartelli3, alisadariana,
joao.goncalves, marius.cristea, mike.looijmans, chanh, KWLIU,
yhyang2, openbmc, linux-iio, devicetree, linux-kernel
On Tue, Nov 5, 2024 at 6:39 PM Eason Yang <j2anfernee@gmail.com> 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 the all 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 support.
>
> Signed-off-by: Eason Yang <j2anfernee@gmail.com>
> ---
> MAINTAINERS | 1 +
> drivers/iio/adc/Kconfig | 9 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/nct720x.c | 617 ++++++++++++++++++++++++++++++++++++++
> 4 files changed, 628 insertions(+)
> create mode 100644 drivers/iio/adc/nct720x.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 68570c58e7aa..9940de0ddca2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2753,6 +2753,7 @@ F: arch/arm/mach-npcm/
> F: arch/arm64/boot/dts/nuvoton/
> F: drivers/*/*/*npcm*
> F: drivers/*/*npcm*
> +F: drivers/iio/adc/nct720x.c
> F: drivers/rtc/rtc-nct3018y.c
> F: include/dt-bindings/clock/nuvoton,npcm7xx-clock.h
> F: include/dt-bindings/clock/nuvoton,npcm845-clk.h
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 6c4e74420fd2..adbbf0ca6f57 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -1008,6 +1008,15 @@ config NAU7802
> To compile this driver as a module, choose M here: the
> module will be called nau7802.
>
> +config NCT720X
> + tristate "Nuvoton Instruments NCT7201 and NCT7202 Power Monitor"
> + depends on 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 nct720x.
> +
> config NPCM_ADC
> tristate "Nuvoton NPCM ADC driver"
> depends on ARCH_NPCM || COMPILE_TEST
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 7b91cd98c0e0..f53318e5aa04 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -91,6 +91,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_NCT720X) += nct720x.o
> obj-$(CONFIG_NPCM_ADC) += npcm_adc.o
> obj-$(CONFIG_PAC1921) += pac1921.o
> obj-$(CONFIG_PAC1934) += pac1934.o
> diff --git a/drivers/iio/adc/nct720x.c b/drivers/iio/adc/nct720x.c
> new file mode 100644
> index 000000000000..e589479fd06e
> --- /dev/null
> +++ b/drivers/iio/adc/nct720x.c
> @@ -0,0 +1,617 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Driver for Nuvoton nct7201 and nct7202 power monitor chips.
> + *
> + * Copyright (c) 2022 Nuvoton Inc.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +
> +#define REG_CHIP_ID 0xFD
> +#define NCT720X_ID 0xD7
> +#define REG_VENDOR_ID 0xFE
> +#define NUVOTON_ID 0x50
> +#define REG_DEVICE_ID 0xFF
> +#define NCT720X_DEVICE_ID 0x12
> +#define VIN_MAX 12 /* Counted from 1 */
> +#define NCT7201_VIN_MAX 8
> +#define NCT7202_VIN_MAX 12
> +#define NCT720X_IN_SCALING 4995
> +
> +#define REG_INTERRUPT_STATUS_1 0x0C
> +#define REG_INTERRUPT_STATUS_2 0x0D
> +#define REG_VOLT_LOW_BYTE 0x0F
> +#define REG_CONFIGURATION 0x10
> +#define CONFIGURATION_START BIT(0)
> +#define CONFIGURATION_ALERT_MSK BIT(1)
> +#define CONFIGURATION_CONV_RATE BIT(2)
> +#define CONFIGURATION_INIT BIT(7)
> +
> +#define REG_ADVANCED_CONFIGURATION 0x11
> +#define ADVANCED_CONF_MOD_ALERT BIT(0)
> +#define ADVANCED_CONF_MOD_STS BIT(1)
> +#define ADVANCED_CONF_FAULT_QUEUE BIT(2)
> +#define ADVANCED_CONF_EN_DEEP_SHUTDOWN BIT(4)
> +#define ADVANCED_CONF_EN_SMB_TIMEOUT BIT(5)
> +#define ADVANCED_CONF_MOD_RSTIN BIT(7)
> +
> +#define REG_CHANNEL_INPUT_MODE 0x12
> +#define REG_CHANNEL_ENABLE_1 0x13
> +#define REG_CHANNEL_ENABLE_2 0x14
> +#define REG_INTERRUPT_MASK_1 0x15
> +#define REG_INTERRUPT_MASK_2 0x16
> +#define REG_BUSY_STATUS 0x1E
> +#define REG_ONE_SHOT 0x1F
> +#define REG_SMUS_ADDRESS 0xFC
> +
> +static const u8 REG_VIN[VIN_MAX] = { 0x00, 0x01, 0x02, 0x03, 0x04, 0x05,
> + 0x06, 0x07, 0x08, 0x09, 0x0A, 0x0B};
> +static const u8 REG_VIN_HIGH_LIMIT[VIN_MAX] = { 0x20, 0x22, 0x24, 0x26, 0x28, 0x2A,
> + 0x2C, 0x2E, 0x30, 0x32, 0x34, 0x36};
> +static const u8 REG_VIN_LOW_LIMIT[VIN_MAX] = { 0x21, 0x23, 0x25, 0x27, 0x29, 0x2B,
> + 0x2D, 0x2F, 0x31, 0x33, 0x35, 0x37};
> +static const u8 REG_VIN_HIGH_LIMIT_LSB[VIN_MAX] = { 0x40, 0x42, 0x44, 0x46, 0x48, 0x4A,
> + 0x4C, 0x4E, 0x50, 0x52, 0x54, 0x56};
> +static const u8 REG_VIN_LOW_LIMIT_LSB[VIN_MAX] = { 0x41, 0x43, 0x45, 0x47, 0x49, 0x4B,
> + 0x4D, 0x4F, 0x51, 0x53, 0x55, 0x57};
> +static u8 nct720x_chan_to_index[] = {
> + 0, /* Not used */
> + 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11
> +};
> +
> +
> +/* List of supported devices */
> +enum nct720x_chips {
> + nct7201, nct7202
> +};
> +
> +struct nct720x_chip_info {
> + struct i2c_client *client;
> + enum nct720x_chips type;
> + struct mutex access_lock; /* for multi-byte read and write operations */
> + int vin_max; /* number of VIN channels */
> + u32 vin_mask;
> + bool use_read_byte_vin;
> +};
> +
> +static const struct iio_event_spec nct720x_events[] = {
> + {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_RISING,
> + .mask_separate = BIT(IIO_EV_INFO_VALUE) |
> + BIT(IIO_EV_INFO_ENABLE),
> + }, {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_FALLING,
> + .mask_separate = BIT(IIO_EV_INFO_VALUE) |
> + BIT(IIO_EV_INFO_ENABLE),
> + },
> +};
> +
> +#define NCT720X_VOLTAGE_CHANNEL(chan, addr) \
> + { \
> + .type = IIO_VOLTAGE, \
> + .indexed = 1, \
> + .channel = chan, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> + .address = addr, \
> + .event_spec = nct720x_events, \
> + .num_event_specs = ARRAY_SIZE(nct720x_events), \
> + }
> +
> +#define NCT720X_VOLTAGE_CHANNEL_DIFF(chan1, chan2, addr) \
> + { \
> + .type = IIO_VOLTAGE, \
> + .indexed = 1, \
> + .channel = (chan1), \
> + .channel2 = (chan2), \
> + .differential = 1, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> + .address = addr, \
> + .event_spec = nct720x_events, \
> + .num_event_specs = ARRAY_SIZE(nct720x_events), \
> + }
> +
> +static const struct iio_chan_spec nct720x_channels[] = {
> + NCT720X_VOLTAGE_CHANNEL(1, 1),
> + NCT720X_VOLTAGE_CHANNEL(2, 2),
> + NCT720X_VOLTAGE_CHANNEL(3, 3),
> + NCT720X_VOLTAGE_CHANNEL(4, 4),
> + NCT720X_VOLTAGE_CHANNEL(5, 5),
> + NCT720X_VOLTAGE_CHANNEL(6, 6),
> + NCT720X_VOLTAGE_CHANNEL(7, 7),
> + NCT720X_VOLTAGE_CHANNEL(8, 8),
> + NCT720X_VOLTAGE_CHANNEL(9, 9),
> + NCT720X_VOLTAGE_CHANNEL(10, 10),
> + NCT720X_VOLTAGE_CHANNEL(11, 11),
> + NCT720X_VOLTAGE_CHANNEL(12, 12),
> + NCT720X_VOLTAGE_CHANNEL_DIFF(1, 2, 1),
> + NCT720X_VOLTAGE_CHANNEL_DIFF(3, 4, 3),
> + NCT720X_VOLTAGE_CHANNEL_DIFF(5, 6, 5),
> + NCT720X_VOLTAGE_CHANNEL_DIFF(7, 8, 7),
> + NCT720X_VOLTAGE_CHANNEL_DIFF(9, 10, 9),
> + NCT720X_VOLTAGE_CHANNEL_DIFF(11, 12, 11),
> +};
> +
> +/* Read 1-byte register. Returns unsigned byte data or -ERRNO on error. */
> +static int nct720x_read_reg(struct nct720x_chip_info *chip, u8 reg)
> +{
> + struct i2c_client *client = chip->client;
> +
> + return i2c_smbus_read_byte_data(client, reg);
> +}
> +
> +/* Read 1-byte register. Returns unsigned word data or -ERRNO on error. */
> +static int nct720x_read_word_swapped_reg(struct nct720x_chip_info *chip, u8 reg)
> +{
> + struct i2c_client *client = chip->client;
> +
> + return i2c_smbus_read_word_swapped(client, reg);
> +}
> +
> +/*
> + * Read 2-byte register. Returns register in big-endian format or
> + * -ERRNO on error.
> + */
> +static int nct720x_read_reg16(struct nct720x_chip_info *chip, u8 reg)
> +{
> + struct i2c_client *client = chip->client;
> + int ret, low;
> +
> + mutex_lock(&chip->access_lock);
> + ret = i2c_smbus_read_byte_data(client, reg);
> + if (ret >= 0) {
> + low = ret;
> + ret = i2c_smbus_read_byte_data(client, reg + 1);
> + if (ret >= 0)
> + ret = low | (ret << 8);
> + }
> +
> + mutex_unlock(&chip->access_lock);
> + return ret;
> +}
> +
> +/* Write 1-byte register. Returns 0 or -ERRNO on error. */
> +static int nct720x_write_reg(struct nct720x_chip_info *chip, u8 reg, u8 val)
> +{
> + struct i2c_client *client = chip->client;
> + int err;
> +
> + err = i2c_smbus_write_byte_data(client, reg, val);
> + /* wait for write command to be finished */
> + mdelay(10);
> +
> + return err;
> +}
> +
> +static int nct720x_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + int index = nct720x_chan_to_index[chan->address];
> + int v1, v2, volt, err;
> + struct nct720x_chip_info *chip = iio_priv(indio_dev);
> +
> + if (chan->type != IIO_VOLTAGE)
> + return -EOPNOTSUPP;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_PROCESSED:
> + mutex_lock(&chip->access_lock);
> + if (chip->use_read_byte_vin) {
> + /*
> + * MNTVIN Low Byte together with MNTVIN High Byte
> + * forms the 13-bit count value. If MNTVIN High
> + * Byte readout is read successively, the
> + * NCT7201/NCT7202 will latch the MNTVIN Low Byte
> + * for next read.
> + */
> + v1 = nct720x_read_reg(chip, REG_VIN[index]);
> + if (v1 < 0) {
> + err = v1;
> + goto abort;
> + }
> +
> + v2 = nct720x_read_reg(chip, REG_VOLT_LOW_BYTE);
> + if (v2 < 0) {
> + err = v2;
> + goto abort;
> + }
> + volt = (v1 << 8) | v2; /* Convert back to 16-bit value */
> + } else {
> + /* NCT7201/NCT7202 also supports read word-size data */
> + volt = nct720x_read_word_swapped_reg(chip, REG_VIN[index]);
> + }
> +
> + /* Voltage(V) = 13bitCountValue * 0.0004995 */
> + volt = (volt >> 3) * NCT720X_IN_SCALING;
> + *val = volt / 10000;
> + mutex_unlock(&chip->access_lock);
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +abort:
> + mutex_unlock(&chip->access_lock);
> + return err;
> +}
> +
> +static int nct720x_read_event_value(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info,
> + int *val, int *val2)
> +{
> + struct nct720x_chip_info *chip = iio_priv(indio_dev);
> + int v1, v2, err;
> + int volt = 0;
> + int index = nct720x_chan_to_index[chan->address];
> +
> + if (chan->type != IIO_VOLTAGE)
> + return -EOPNOTSUPP;
> +
> + if (info == IIO_EV_INFO_VALUE) {
> + if (dir == IIO_EV_DIR_FALLING) {
> + if (chip->use_read_byte_vin) {
> + /*
> + * Low limit VIN Low Byte together with Low limit VIN High Byte
> + forms the 13-bit count value
> + */
> + mutex_lock(&chip->access_lock);
> + v1 = nct720x_read_reg(chip, REG_VIN_LOW_LIMIT[index]);
> + if (v1 < 0) {
> + err = v1;
> + goto abort;
> + }
> +
> + v2 = nct720x_read_reg(chip, REG_VIN_LOW_LIMIT_LSB[index]);
> + if (v2 < 0) {
> + err = v2;
> + goto abort;
> + }
> + mutex_unlock(&chip->access_lock);
> + volt = (v1 << 8) | v2; /* Convert back to 16-bit value */
> + } else {
> + /* NCT7201/NCT7202 also supports read word-size data */
> + volt = nct720x_read_word_swapped_reg(chip,
> + REG_VIN_LOW_LIMIT[index]);
> + }
> + } else {
> + if (chip->use_read_byte_vin) {
> + /*
> + * High limit VIN Low Byte together with high limit VIN High Byte
> + * forms the 13-bit count value
> + */
> + mutex_lock(&chip->access_lock);
> + v1 = nct720x_read_reg(chip, REG_VIN_HIGH_LIMIT[index]);
> + if (v1 < 0) {
> + err = v1;
> + goto abort;
> + }
> +
> + v2 = nct720x_read_reg(chip, REG_VIN_HIGH_LIMIT_LSB[index]);
> + if (v2 < 0) {
> + err = v2;
> + goto abort;
> + }
> + mutex_unlock(&chip->access_lock);
> + volt = (v1 << 8) | v2; /* Convert back to 16-bit value */
> + } else {
> + /* NCT7201/NCT7202 also supports read word-size data */
> + volt = nct720x_read_word_swapped_reg(chip,
> + REG_VIN_HIGH_LIMIT[index]);
> + }
> + }
> + }
> + /* Voltage(V) = 13bitCountValue * 0.0004995 */
> + volt = (volt >> 3) * NCT720X_IN_SCALING;
> + *val = volt / 10000;
> +
> + return IIO_VAL_INT;
> +abort:
> + mutex_unlock(&chip->access_lock);
> + return err;
> +}
> +
> +static int nct720x_write_event_value(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info,
> + int val, int val2)
> +{
> + struct nct720x_chip_info *chip = iio_priv(indio_dev);
> + int index, err = 0;
> + long v1, v2, volt;
> +
> + index = nct720x_chan_to_index[chan->address];
> + volt = (val * 10000) / NCT720X_IN_SCALING;
> + v1 = volt >> 5;
> + v2 = (volt & 0x1f) << 3;
> +
> + if (chan->type != IIO_VOLTAGE)
> + return -EOPNOTSUPP;
> +
> + if (info == IIO_EV_INFO_VALUE) {
> + if (dir == IIO_EV_DIR_FALLING) {
> + mutex_lock(&chip->access_lock);
> + err = nct720x_write_reg(chip, REG_VIN_LOW_LIMIT[index], v1);
> + if (err < 0) {
> + pr_err("Failed to write REG_VIN%d_LOW_LIMIT\n", index + 1);
> + goto abort;
> + }
> +
> + err = nct720x_write_reg(chip, REG_VIN_LOW_LIMIT_LSB[index], v2);
> + if (err < 0) {
> + pr_err("Failed to write REG_VIN%d_LOW_LIMIT_LSB\n", index + 1);
> + goto abort;
> + }
> + } else {
> + mutex_lock(&chip->access_lock);
> + err = nct720x_write_reg(chip, REG_VIN_HIGH_LIMIT[index], v1);
> + if (err < 0) {
> + pr_err("Failed to write REG_VIN%d_HIGH_LIMIT\n", index + 1);
> + goto abort;
> + }
> +
> + err = nct720x_write_reg(chip, REG_VIN_HIGH_LIMIT_LSB[index], v2);
> + if (err < 0) {
> + pr_err("Failed to write REG_VIN%d_HIGH_LIMIT_LSB\n", index + 1);
> + goto abort;
> + }
> + }
> + }
> +abort:
> + mutex_unlock(&chip->access_lock);
> + return err;
> +}
> +
> +static int nct720x_read_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir)
> +{
> + struct nct720x_chip_info *chip = iio_priv(indio_dev);
> + int index = nct720x_chan_to_index[chan->address];
> +
> + if (chan->type != IIO_VOLTAGE)
> + return -EOPNOTSUPP;
> +
> + return !!(chip->vin_mask & BIT(index));
> +}
> +
> +static int nct720x_write_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + int state)
> +{
> + int err = 0;
> + struct nct720x_chip_info *chip = iio_priv(indio_dev);
> + int index = nct720x_chan_to_index[chan->address];
> + unsigned int mask;
> +
> + mask = BIT(index);
nitpick:
You can set the mask near to the place where it is used i.e.
just after below if statement and one added advantage will
be in case below "return -EOPNOTSUPP" gets executed,
you don't even have to set mask.
> +
> + if (chan->type != IIO_VOLTAGE)
> + return -EOPNOTSUPP;
> +
> + if (!state && (chip->vin_mask & mask))
> + chip->vin_mask &= ~mask;
> + else if (state && !(chip->vin_mask & mask))
> + chip->vin_mask |= mask;
> +
> + mutex_lock(&chip->access_lock);
> +
> + err = nct720x_write_reg(chip, REG_CHANNEL_ENABLE_1, chip->vin_mask & 0xff);
> + if (err < 0) {
> + pr_err("Failed to write REG_CHANNEL_ENABLE_1\n");
> + goto abort;
> + }
> +
> + if (chip->type == nct7202) {
> + err = nct720x_write_reg(chip, REG_CHANNEL_ENABLE_2, chip->vin_mask >> 8);
> + if (err < 0) {
> + pr_err("Failed to write REG_CHANNEL_ENABLE_2\n");
> + goto abort;
> + }
> + }
> +abort:
> + mutex_unlock(&chip->access_lock);
> + return err;
> +}
> +
> +static int nct720x_detect(struct i2c_client *client,
> + struct i2c_board_info *info)
> +{
> + struct i2c_adapter *adapter = client->adapter;
> +
> + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> + I2C_FUNC_SMBUS_WORD_DATA))
> + return -ENODEV;
> +
> + /* Determine the chip type. */
> + if (i2c_smbus_read_byte_data(client, REG_VENDOR_ID) != NUVOTON_ID ||
> + i2c_smbus_read_byte_data(client, REG_CHIP_ID) != NCT720X_ID ||
> + i2c_smbus_read_byte_data(client, REG_DEVICE_ID) != NCT720X_DEVICE_ID)
> + return -ENODEV;
> +
> + strscpy(info->type, "nct720x", I2C_NAME_SIZE);
> +
> + return 0;
> +}
> +
> +static const struct iio_info nct720x_info = {
> + .read_raw = &nct720x_read_raw,
> + .read_event_config = &nct720x_read_event_config,
> + .write_event_config = &nct720x_write_event_config,
> + .read_event_value = &nct720x_read_event_value,
> + .write_event_value = &nct720x_write_event_value,
> +};
> +
> +static const struct i2c_device_id nct720x_id[];
> +
> +static int nct720x_init_chip(struct nct720x_chip_info *chip)
> +{
> + int value = 0;
> + int err = 0;
Nit pick:
You don't need to initialize err and value.
> +
> + /* Initial reset */
> + err = nct720x_write_reg(chip, REG_CONFIGURATION, CONFIGURATION_INIT);
> + if (err) {
> + pr_err("Failed to write REG_CONFIGURATION\n");
> + return err;
> + }
> +
> + /* Enable Channel */
> + err = nct720x_write_reg(chip, REG_CHANNEL_ENABLE_1, 0xff);
> + if (err) {
> + pr_err("Failed to write REG_CHANNEL_ENABLE_1\n");
> + return err;
> + }
> +
> + if (chip->type == nct7202) {
> + err = nct720x_write_reg(chip, REG_CHANNEL_ENABLE_2, 0xf);
> + if (err) {
> + pr_err("Failed to write REG_CHANNEL_ENABLE_2\n");
> + return err;
> + }
> + }
> +
> + value = nct720x_read_reg16(chip, REG_CHANNEL_ENABLE_1);
> + if (value < 0)
> + return value;
> + chip->vin_mask = value;
> +
> + /* Start monitoring if needed */
> + value = nct720x_read_reg(chip, REG_CONFIGURATION);
> + if (value < 0) {
> + pr_err("Failed to read REG_CONFIGURATION\n");
> + return value;
> + }
> +
> + value |= CONFIGURATION_START;
> + err = nct720x_write_reg(chip, REG_CONFIGURATION, value);
> + if (err < 0) {
> + pr_err("Failed to write REG_CONFIGURATION\n");
> + return err;
> + }
> +
> + return 0;
> +}
> +
> +static int nct720x_probe(struct i2c_client *client)
> +{
> + const struct i2c_device_id *id = i2c_client_get_device_id(client);
> + struct nct720x_chip_info *chip;
> + struct iio_dev *indio_dev;
> + int ret;
> + u32 tmp;
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
> + if (!indio_dev)
> + return -ENOMEM;
> + chip = iio_priv(indio_dev);
> +
> + if (client->dev.of_node)
> + chip->type = (enum nct720x_chips)device_get_match_data(&client->dev);
> + else
> + chip->type = i2c_match_id(nct720x_id, client)->driver_data;
> +
> + chip->vin_max = (chip->type == nct7201) ? NCT7201_VIN_MAX : NCT7202_VIN_MAX;
> +
> + ret = of_property_read_u32(client->dev.of_node, "read-vin-data-size", &tmp);
> + if (ret < 0) {
> + pr_err("read-vin-data-size property not found\n");
> + return ret;
nit: dev_err_probe
> + }
> +
> + if (tmp == 8) {
> + chip->use_read_byte_vin = true;
> + } else if (tmp == 16) {
> + chip->use_read_byte_vin = false;
> + } else {
> + pr_err("invalid read-vin-data-size (%d)\n", tmp);
> + return -EINVAL;
nit: dev_err_probe
> + }
> +
> + mutex_init(&chip->access_lock);
> +
> + /* this is only used for device removal purposes */
> + i2c_set_clientdata(client, indio_dev);
> +
> + chip->client = client;
> +
> + ret = nct720x_init_chip(chip);
> + if (ret < 0)
> + return ret;
> +
> + indio_dev->name = id->name;
> + indio_dev->channels = nct720x_channels;
> + indio_dev->num_channels = ARRAY_SIZE(nct720x_channels);
> + indio_dev->info = &nct720x_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + iio_device_register(indio_dev);
devm_iio_device_register?
> +
> + return 0;
> +}
> +
> +static void nct720x_remove(struct i2c_client *client)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +
> + iio_device_unregister(indio_dev);
wouldn't need if used devm_ version
> +}
> +
> +static const unsigned short nct720x_address_list[] = {
> + 0x1d, 0x1e, 0x35, 0x36, I2C_CLIENT_END
> +};
> +
> +static const struct i2c_device_id nct720x_id[] = {
> + { "nct7201", nct7201 },
> + { "nct7202", nct7202 },
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, nct720x_id);
> +
> +static const struct of_device_id nct720x_of_match[] = {
> + {
> + .compatible = "nuvoton,nct7201",
> + .data = (void *)nct7201
> + },
> + {
> + .compatible = "nuvoton,nct7202",
> + .data = (void *)nct7202
> + },
> + { },
> +};
> +
> +MODULE_DEVICE_TABLE(of, nct720x_of_match);
> +
> +static struct i2c_driver nct720x_driver = {
> + .driver = {
> + .name = "nct720x",
> + .of_match_table = nct720x_of_match,
> + },
> + .probe = nct720x_probe,
> + .remove = nct720x_remove,
> + .id_table = nct720x_id,
> + .detect = nct720x_detect,
> + .address_list = nct720x_address_list,
> +};
> +
> +module_i2c_driver(nct720x_driver);
> +
> +MODULE_AUTHOR("Eason Yang <YHYANG2@nuvoton.com>");
> +MODULE_DESCRIPTION("Nuvoton NCT720x voltage monitor driver");
> +MODULE_LICENSE("GPL");
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v1 2/2] iio: adc: add Nuvoton NCT720x ADC driver
2024-11-07 6:13 ` anish kumar
@ 2024-11-07 7:17 ` Yu-Hsian Yang
0 siblings, 0 replies; 26+ messages in thread
From: Yu-Hsian Yang @ 2024-11-07 7:17 UTC (permalink / raw)
To: anish kumar
Cc: avifishman70, tmaimon77, tali.perry1, venture, yuenn,
benjaminfair, jic23, lars, robh, krzk+dt, conor+dt, nuno.sa,
dlechner, javier.carrasco.cruz, andy, marcelo.schmitt,
olivier.moysan, mitrutzceclan, matteomartelli3, alisadariana,
joao.goncalves, marius.cristea, mike.looijmans, chanh, KWLIU,
yhyang2, openbmc, linux-iio, devicetree, linux-kernel
Dear anish kumar,
Thank you for your response.
anish kumar <yesanishhere@gmail.com> 於 2024年11月7日 週四 下午2:14寫道：
>
> On Tue, Nov 5, 2024 at 6:39 PM Eason Yang <j2anfernee@gmail.com> 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 the all 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 support.
> >
> > Signed-off-by: Eason Yang <j2anfernee@gmail.com>
> > ---
> > MAINTAINERS | 1 +
> > drivers/iio/adc/Kconfig | 9 +
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/nct720x.c | 617 ++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 628 insertions(+)
> > create mode 100644 drivers/iio/adc/nct720x.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 68570c58e7aa..9940de0ddca2 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -2753,6 +2753,7 @@ F: arch/arm/mach-npcm/
> > F: arch/arm64/boot/dts/nuvoton/
> > F: drivers/*/*/*npcm*
> > F: drivers/*/*npcm*
> > +F: drivers/iio/adc/nct720x.c
> > F: drivers/rtc/rtc-nct3018y.c
> > F: include/dt-bindings/clock/nuvoton,npcm7xx-clock.h
> > F: include/dt-bindings/clock/nuvoton,npcm845-clk.h
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 6c4e74420fd2..adbbf0ca6f57 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -1008,6 +1008,15 @@ config NAU7802
> > To compile this driver as a module, choose M here: the
> > module will be called nau7802.
> >
> > +config NCT720X
> > + tristate "Nuvoton Instruments NCT7201 and NCT7202 Power Monitor"
> > + depends on 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 nct720x.
> > +
> > config NPCM_ADC
> > tristate "Nuvoton NPCM ADC driver"
> > depends on ARCH_NPCM || COMPILE_TEST
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index 7b91cd98c0e0..f53318e5aa04 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -91,6 +91,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_NCT720X) += nct720x.o
> > obj-$(CONFIG_NPCM_ADC) += npcm_adc.o
> > obj-$(CONFIG_PAC1921) += pac1921.o
> > obj-$(CONFIG_PAC1934) += pac1934.o
> > diff --git a/drivers/iio/adc/nct720x.c b/drivers/iio/adc/nct720x.c
> > new file mode 100644
> > index 000000000000..e589479fd06e
> > --- /dev/null
> > +++ b/drivers/iio/adc/nct720x.c
> > @@ -0,0 +1,617 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Driver for Nuvoton nct7201 and nct7202 power monitor chips.
> > + *
> > + * Copyright (c) 2022 Nuvoton Inc.
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/i2c.h>
> > +#include <linux/iio/events.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +
> > +#define REG_CHIP_ID 0xFD
> > +#define NCT720X_ID 0xD7
> > +#define REG_VENDOR_ID 0xFE
> > +#define NUVOTON_ID 0x50
> > +#define REG_DEVICE_ID 0xFF
> > +#define NCT720X_DEVICE_ID 0x12
> > +#define VIN_MAX 12 /* Counted from 1 */
> > +#define NCT7201_VIN_MAX 8
> > +#define NCT7202_VIN_MAX 12
> > +#define NCT720X_IN_SCALING 4995
> > +
> > +#define REG_INTERRUPT_STATUS_1 0x0C
> > +#define REG_INTERRUPT_STATUS_2 0x0D
> > +#define REG_VOLT_LOW_BYTE 0x0F
> > +#define REG_CONFIGURATION 0x10
> > +#define CONFIGURATION_START BIT(0)
> > +#define CONFIGURATION_ALERT_MSK BIT(1)
> > +#define CONFIGURATION_CONV_RATE BIT(2)
> > +#define CONFIGURATION_INIT BIT(7)
> > +
> > +#define REG_ADVANCED_CONFIGURATION 0x11
> > +#define ADVANCED_CONF_MOD_ALERT BIT(0)
> > +#define ADVANCED_CONF_MOD_STS BIT(1)
> > +#define ADVANCED_CONF_FAULT_QUEUE BIT(2)
> > +#define ADVANCED_CONF_EN_DEEP_SHUTDOWN BIT(4)
> > +#define ADVANCED_CONF_EN_SMB_TIMEOUT BIT(5)
> > +#define ADVANCED_CONF_MOD_RSTIN BIT(7)
> > +
> > +#define REG_CHANNEL_INPUT_MODE 0x12
> > +#define REG_CHANNEL_ENABLE_1 0x13
> > +#define REG_CHANNEL_ENABLE_2 0x14
> > +#define REG_INTERRUPT_MASK_1 0x15
> > +#define REG_INTERRUPT_MASK_2 0x16
> > +#define REG_BUSY_STATUS 0x1E
> > +#define REG_ONE_SHOT 0x1F
> > +#define REG_SMUS_ADDRESS 0xFC
> > +
> > +static const u8 REG_VIN[VIN_MAX] = { 0x00, 0x01, 0x02, 0x03, 0x04, 0x05,
> > + 0x06, 0x07, 0x08, 0x09, 0x0A, 0x0B};
> > +static const u8 REG_VIN_HIGH_LIMIT[VIN_MAX] = { 0x20, 0x22, 0x24, 0x26, 0x28, 0x2A,
> > + 0x2C, 0x2E, 0x30, 0x32, 0x34, 0x36};
> > +static const u8 REG_VIN_LOW_LIMIT[VIN_MAX] = { 0x21, 0x23, 0x25, 0x27, 0x29, 0x2B,
> > + 0x2D, 0x2F, 0x31, 0x33, 0x35, 0x37};
> > +static const u8 REG_VIN_HIGH_LIMIT_LSB[VIN_MAX] = { 0x40, 0x42, 0x44, 0x46, 0x48, 0x4A,
> > + 0x4C, 0x4E, 0x50, 0x52, 0x54, 0x56};
> > +static const u8 REG_VIN_LOW_LIMIT_LSB[VIN_MAX] = { 0x41, 0x43, 0x45, 0x47, 0x49, 0x4B,
> > + 0x4D, 0x4F, 0x51, 0x53, 0x55, 0x57};
> > +static u8 nct720x_chan_to_index[] = {
> > + 0, /* Not used */
> > + 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11
> > +};
> > +
> > +
> > +/* List of supported devices */
> > +enum nct720x_chips {
> > + nct7201, nct7202
> > +};
> > +
> > +struct nct720x_chip_info {
> > + struct i2c_client *client;
> > + enum nct720x_chips type;
> > + struct mutex access_lock; /* for multi-byte read and write operations */
> > + int vin_max; /* number of VIN channels */
> > + u32 vin_mask;
> > + bool use_read_byte_vin;
> > +};
> > +
> > +static const struct iio_event_spec nct720x_events[] = {
> > + {
> > + .type = IIO_EV_TYPE_THRESH,
> > + .dir = IIO_EV_DIR_RISING,
> > + .mask_separate = BIT(IIO_EV_INFO_VALUE) |
> > + BIT(IIO_EV_INFO_ENABLE),
> > + }, {
> > + .type = IIO_EV_TYPE_THRESH,
> > + .dir = IIO_EV_DIR_FALLING,
> > + .mask_separate = BIT(IIO_EV_INFO_VALUE) |
> > + BIT(IIO_EV_INFO_ENABLE),
> > + },
> > +};
> > +
> > +#define NCT720X_VOLTAGE_CHANNEL(chan, addr) \
> > + { \
> > + .type = IIO_VOLTAGE, \
> > + .indexed = 1, \
> > + .channel = chan, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> > + .address = addr, \
> > + .event_spec = nct720x_events, \
> > + .num_event_specs = ARRAY_SIZE(nct720x_events), \
> > + }
> > +
> > +#define NCT720X_VOLTAGE_CHANNEL_DIFF(chan1, chan2, addr) \
> > + { \
> > + .type = IIO_VOLTAGE, \
> > + .indexed = 1, \
> > + .channel = (chan1), \
> > + .channel2 = (chan2), \
> > + .differential = 1, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> > + .address = addr, \
> > + .event_spec = nct720x_events, \
> > + .num_event_specs = ARRAY_SIZE(nct720x_events), \
> > + }
> > +
> > +static const struct iio_chan_spec nct720x_channels[] = {
> > + NCT720X_VOLTAGE_CHANNEL(1, 1),
> > + NCT720X_VOLTAGE_CHANNEL(2, 2),
> > + NCT720X_VOLTAGE_CHANNEL(3, 3),
> > + NCT720X_VOLTAGE_CHANNEL(4, 4),
> > + NCT720X_VOLTAGE_CHANNEL(5, 5),
> > + NCT720X_VOLTAGE_CHANNEL(6, 6),
> > + NCT720X_VOLTAGE_CHANNEL(7, 7),
> > + NCT720X_VOLTAGE_CHANNEL(8, 8),
> > + NCT720X_VOLTAGE_CHANNEL(9, 9),
> > + NCT720X_VOLTAGE_CHANNEL(10, 10),
> > + NCT720X_VOLTAGE_CHANNEL(11, 11),
> > + NCT720X_VOLTAGE_CHANNEL(12, 12),
> > + NCT720X_VOLTAGE_CHANNEL_DIFF(1, 2, 1),
> > + NCT720X_VOLTAGE_CHANNEL_DIFF(3, 4, 3),
> > + NCT720X_VOLTAGE_CHANNEL_DIFF(5, 6, 5),
> > + NCT720X_VOLTAGE_CHANNEL_DIFF(7, 8, 7),
> > + NCT720X_VOLTAGE_CHANNEL_DIFF(9, 10, 9),
> > + NCT720X_VOLTAGE_CHANNEL_DIFF(11, 12, 11),
> > +};
> > +
> > +/* Read 1-byte register. Returns unsigned byte data or -ERRNO on error. */
> > +static int nct720x_read_reg(struct nct720x_chip_info *chip, u8 reg)
> > +{
> > + struct i2c_client *client = chip->client;
> > +
> > + return i2c_smbus_read_byte_data(client, reg);
> > +}
> > +
> > +/* Read 1-byte register. Returns unsigned word data or -ERRNO on error. */
> > +static int nct720x_read_word_swapped_reg(struct nct720x_chip_info *chip, u8 reg)
> > +{
> > + struct i2c_client *client = chip->client;
> > +
> > + return i2c_smbus_read_word_swapped(client, reg);
> > +}
> > +
> > +/*
> > + * Read 2-byte register. Returns register in big-endian format or
> > + * -ERRNO on error.
> > + */
> > +static int nct720x_read_reg16(struct nct720x_chip_info *chip, u8 reg)
> > +{
> > + struct i2c_client *client = chip->client;
> > + int ret, low;
> > +
> > + mutex_lock(&chip->access_lock);
> > + ret = i2c_smbus_read_byte_data(client, reg);
> > + if (ret >= 0) {
> > + low = ret;
> > + ret = i2c_smbus_read_byte_data(client, reg + 1);
> > + if (ret >= 0)
> > + ret = low | (ret << 8);
> > + }
> > +
> > + mutex_unlock(&chip->access_lock);
> > + return ret;
> > +}
> > +
> > +/* Write 1-byte register. Returns 0 or -ERRNO on error. */
> > +static int nct720x_write_reg(struct nct720x_chip_info *chip, u8 reg, u8 val)
> > +{
> > + struct i2c_client *client = chip->client;
> > + int err;
> > +
> > + err = i2c_smbus_write_byte_data(client, reg, val);
> > + /* wait for write command to be finished */
> > + mdelay(10);
> > +
> > + return err;
> > +}
> > +
> > +static int nct720x_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val, int *val2, long mask)
> > +{
> > + int index = nct720x_chan_to_index[chan->address];
> > + int v1, v2, volt, err;
> > + struct nct720x_chip_info *chip = iio_priv(indio_dev);
> > +
> > + if (chan->type != IIO_VOLTAGE)
> > + return -EOPNOTSUPP;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_PROCESSED:
> > + mutex_lock(&chip->access_lock);
> > + if (chip->use_read_byte_vin) {
> > + /*
> > + * MNTVIN Low Byte together with MNTVIN High Byte
> > + * forms the 13-bit count value. If MNTVIN High
> > + * Byte readout is read successively, the
> > + * NCT7201/NCT7202 will latch the MNTVIN Low Byte
> > + * for next read.
> > + */
> > + v1 = nct720x_read_reg(chip, REG_VIN[index]);
> > + if (v1 < 0) {
> > + err = v1;
> > + goto abort;
> > + }
> > +
> > + v2 = nct720x_read_reg(chip, REG_VOLT_LOW_BYTE);
> > + if (v2 < 0) {
> > + err = v2;
> > + goto abort;
> > + }
> > + volt = (v1 << 8) | v2; /* Convert back to 16-bit value */
> > + } else {
> > + /* NCT7201/NCT7202 also supports read word-size data */
> > + volt = nct720x_read_word_swapped_reg(chip, REG_VIN[index]);
> > + }
> > +
> > + /* Voltage(V) = 13bitCountValue * 0.0004995 */
> > + volt = (volt >> 3) * NCT720X_IN_SCALING;
> > + *val = volt / 10000;
> > + mutex_unlock(&chip->access_lock);
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > +abort:
> > + mutex_unlock(&chip->access_lock);
> > + return err;
> > +}
> > +
> > +static int nct720x_read_event_value(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir,
> > + enum iio_event_info info,
> > + int *val, int *val2)
> > +{
> > + struct nct720x_chip_info *chip = iio_priv(indio_dev);
> > + int v1, v2, err;
> > + int volt = 0;
> > + int index = nct720x_chan_to_index[chan->address];
> > +
> > + if (chan->type != IIO_VOLTAGE)
> > + return -EOPNOTSUPP;
> > +
> > + if (info == IIO_EV_INFO_VALUE) {
> > + if (dir == IIO_EV_DIR_FALLING) {
> > + if (chip->use_read_byte_vin) {
> > + /*
> > + * Low limit VIN Low Byte together with Low limit VIN High Byte
> > + forms the 13-bit count value
> > + */
> > + mutex_lock(&chip->access_lock);
> > + v1 = nct720x_read_reg(chip, REG_VIN_LOW_LIMIT[index]);
> > + if (v1 < 0) {
> > + err = v1;
> > + goto abort;
> > + }
> > +
> > + v2 = nct720x_read_reg(chip, REG_VIN_LOW_LIMIT_LSB[index]);
> > + if (v2 < 0) {
> > + err = v2;
> > + goto abort;
> > + }
> > + mutex_unlock(&chip->access_lock);
> > + volt = (v1 << 8) | v2; /* Convert back to 16-bit value */
> > + } else {
> > + /* NCT7201/NCT7202 also supports read word-size data */
> > + volt = nct720x_read_word_swapped_reg(chip,
> > + REG_VIN_LOW_LIMIT[index]);
> > + }
> > + } else {
> > + if (chip->use_read_byte_vin) {
> > + /*
> > + * High limit VIN Low Byte together with high limit VIN High Byte
> > + * forms the 13-bit count value
> > + */
> > + mutex_lock(&chip->access_lock);
> > + v1 = nct720x_read_reg(chip, REG_VIN_HIGH_LIMIT[index]);
> > + if (v1 < 0) {
> > + err = v1;
> > + goto abort;
> > + }
> > +
> > + v2 = nct720x_read_reg(chip, REG_VIN_HIGH_LIMIT_LSB[index]);
> > + if (v2 < 0) {
> > + err = v2;
> > + goto abort;
> > + }
> > + mutex_unlock(&chip->access_lock);
> > + volt = (v1 << 8) | v2; /* Convert back to 16-bit value */
> > + } else {
> > + /* NCT7201/NCT7202 also supports read word-size data */
> > + volt = nct720x_read_word_swapped_reg(chip,
> > + REG_VIN_HIGH_LIMIT[index]);
> > + }
> > + }
> > + }
> > + /* Voltage(V) = 13bitCountValue * 0.0004995 */
> > + volt = (volt >> 3) * NCT720X_IN_SCALING;
> > + *val = volt / 10000;
> > +
> > + return IIO_VAL_INT;
> > +abort:
> > + mutex_unlock(&chip->access_lock);
> > + return err;
> > +}
> > +
> > +static int nct720x_write_event_value(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir,
> > + enum iio_event_info info,
> > + int val, int val2)
> > +{
> > + struct nct720x_chip_info *chip = iio_priv(indio_dev);
> > + int index, err = 0;
> > + long v1, v2, volt;
> > +
> > + index = nct720x_chan_to_index[chan->address];
> > + volt = (val * 10000) / NCT720X_IN_SCALING;
> > + v1 = volt >> 5;
> > + v2 = (volt & 0x1f) << 3;
> > +
> > + if (chan->type != IIO_VOLTAGE)
> > + return -EOPNOTSUPP;
> > +
> > + if (info == IIO_EV_INFO_VALUE) {
> > + if (dir == IIO_EV_DIR_FALLING) {
> > + mutex_lock(&chip->access_lock);
> > + err = nct720x_write_reg(chip, REG_VIN_LOW_LIMIT[index], v1);
> > + if (err < 0) {
> > + pr_err("Failed to write REG_VIN%d_LOW_LIMIT\n", index + 1);
> > + goto abort;
> > + }
> > +
> > + err = nct720x_write_reg(chip, REG_VIN_LOW_LIMIT_LSB[index], v2);
> > + if (err < 0) {
> > + pr_err("Failed to write REG_VIN%d_LOW_LIMIT_LSB\n", index + 1);
> > + goto abort;
> > + }
> > + } else {
> > + mutex_lock(&chip->access_lock);
> > + err = nct720x_write_reg(chip, REG_VIN_HIGH_LIMIT[index], v1);
> > + if (err < 0) {
> > + pr_err("Failed to write REG_VIN%d_HIGH_LIMIT\n", index + 1);
> > + goto abort;
> > + }
> > +
> > + err = nct720x_write_reg(chip, REG_VIN_HIGH_LIMIT_LSB[index], v2);
> > + if (err < 0) {
> > + pr_err("Failed to write REG_VIN%d_HIGH_LIMIT_LSB\n", index + 1);
> > + goto abort;
> > + }
> > + }
> > + }
> > +abort:
> > + mutex_unlock(&chip->access_lock);
> > + return err;
> > +}
> > +
> > +static int nct720x_read_event_config(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir)
> > +{
> > + struct nct720x_chip_info *chip = iio_priv(indio_dev);
> > + int index = nct720x_chan_to_index[chan->address];
> > +
> > + if (chan->type != IIO_VOLTAGE)
> > + return -EOPNOTSUPP;
> > +
> > + return !!(chip->vin_mask & BIT(index));
> > +}
> > +
> > +static int nct720x_write_event_config(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir,
> > + int state)
> > +{
> > + int err = 0;
> > + struct nct720x_chip_info *chip = iio_priv(indio_dev);
> > + int index = nct720x_chan_to_index[chan->address];
> > + unsigned int mask;
> > +
> > + mask = BIT(index);
>
> nitpick:
> You can set the mask near to the place where it is used i.e.
> just after below if statement and one added advantage will
> be in case below "return -EOPNOTSUPP" gets executed,
> you don't even have to set mask.
>
Got it.
> > +
> > + if (chan->type != IIO_VOLTAGE)
> > + return -EOPNOTSUPP;
> > +
> > + if (!state && (chip->vin_mask & mask))
> > + chip->vin_mask &= ~mask;
> > + else if (state && !(chip->vin_mask & mask))
> > + chip->vin_mask |= mask;
> > +
> > + mutex_lock(&chip->access_lock);
> > +
> > + err = nct720x_write_reg(chip, REG_CHANNEL_ENABLE_1, chip->vin_mask & 0xff);
> > + if (err < 0) {
> > + pr_err("Failed to write REG_CHANNEL_ENABLE_1\n");
> > + goto abort;
> > + }
> > +
> > + if (chip->type == nct7202) {
> > + err = nct720x_write_reg(chip, REG_CHANNEL_ENABLE_2, chip->vin_mask >> 8);
> > + if (err < 0) {
> > + pr_err("Failed to write REG_CHANNEL_ENABLE_2\n");
> > + goto abort;
> > + }
> > + }
> > +abort:
> > + mutex_unlock(&chip->access_lock);
> > + return err;
> > +}
> > +
> > +static int nct720x_detect(struct i2c_client *client,
> > + struct i2c_board_info *info)
> > +{
> > + struct i2c_adapter *adapter = client->adapter;
> > +
> > + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> > + I2C_FUNC_SMBUS_WORD_DATA))
> > + return -ENODEV;
> > +
> > + /* Determine the chip type. */
> > + if (i2c_smbus_read_byte_data(client, REG_VENDOR_ID) != NUVOTON_ID ||
> > + i2c_smbus_read_byte_data(client, REG_CHIP_ID) != NCT720X_ID ||
> > + i2c_smbus_read_byte_data(client, REG_DEVICE_ID) != NCT720X_DEVICE_ID)
> > + return -ENODEV;
> > +
> > + strscpy(info->type, "nct720x", I2C_NAME_SIZE);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct iio_info nct720x_info = {
> > + .read_raw = &nct720x_read_raw,
> > + .read_event_config = &nct720x_read_event_config,
> > + .write_event_config = &nct720x_write_event_config,
> > + .read_event_value = &nct720x_read_event_value,
> > + .write_event_value = &nct720x_write_event_value,
> > +};
> > +
> > +static const struct i2c_device_id nct720x_id[];
> > +
> > +static int nct720x_init_chip(struct nct720x_chip_info *chip)
> > +{
> > + int value = 0;
> > + int err = 0;
>
> Nit pick:
> You don't need to initialize err and value.
>
Got it.
> > +
> > + /* Initial reset */
> > + err = nct720x_write_reg(chip, REG_CONFIGURATION, CONFIGURATION_INIT);
> > + if (err) {
> > + pr_err("Failed to write REG_CONFIGURATION\n");
> > + return err;
> > + }
> > +
> > + /* Enable Channel */
> > + err = nct720x_write_reg(chip, REG_CHANNEL_ENABLE_1, 0xff);
> > + if (err) {
> > + pr_err("Failed to write REG_CHANNEL_ENABLE_1\n");
> > + return err;
> > + }
> > +
> > + if (chip->type == nct7202) {
> > + err = nct720x_write_reg(chip, REG_CHANNEL_ENABLE_2, 0xf);
> > + if (err) {
> > + pr_err("Failed to write REG_CHANNEL_ENABLE_2\n");
> > + return err;
> > + }
> > + }
> > +
> > + value = nct720x_read_reg16(chip, REG_CHANNEL_ENABLE_1);
> > + if (value < 0)
> > + return value;
> > + chip->vin_mask = value;
> > +
> > + /* Start monitoring if needed */
> > + value = nct720x_read_reg(chip, REG_CONFIGURATION);
> > + if (value < 0) {
> > + pr_err("Failed to read REG_CONFIGURATION\n");
> > + return value;
> > + }
> > +
> > + value |= CONFIGURATION_START;
> > + err = nct720x_write_reg(chip, REG_CONFIGURATION, value);
> > + if (err < 0) {
> > + pr_err("Failed to write REG_CONFIGURATION\n");
> > + return err;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int nct720x_probe(struct i2c_client *client)
> > +{
> > + const struct i2c_device_id *id = i2c_client_get_device_id(client);
> > + struct nct720x_chip_info *chip;
> > + struct iio_dev *indio_dev;
> > + int ret;
> > + u32 tmp;
> > +
> > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > + chip = iio_priv(indio_dev);
> > +
> > + if (client->dev.of_node)
> > + chip->type = (enum nct720x_chips)device_get_match_data(&client->dev);
> > + else
> > + chip->type = i2c_match_id(nct720x_id, client)->driver_data;
> > +
> > + chip->vin_max = (chip->type == nct7201) ? NCT7201_VIN_MAX : NCT7202_VIN_MAX;
> > +
> > + ret = of_property_read_u32(client->dev.of_node, "read-vin-data-size", &tmp);
> > + if (ret < 0) {
> > + pr_err("read-vin-data-size property not found\n");
> > + return ret;
>
> nit: dev_err_probe
>
I would use dev_err_probe to replace pr_err in probe function
> > + }
> > +
> > + if (tmp == 8) {
> > + chip->use_read_byte_vin = true;
> > + } else if (tmp == 16) {
> > + chip->use_read_byte_vin = false;
> > + } else {
> > + pr_err("invalid read-vin-data-size (%d)\n", tmp);
> > + return -EINVAL;
>
> nit: dev_err_probe
>
I would use dev_err_probe to replace pr_err in probe function.
> > + }
> > +
> > + mutex_init(&chip->access_lock);
> > +
> > + /* this is only used for device removal purposes */
> > + i2c_set_clientdata(client, indio_dev);
> > +
> > + chip->client = client;
> > +
> > + ret = nct720x_init_chip(chip);
> > + if (ret < 0)
> > + return ret;
> > +
> > + indio_dev->name = id->name;
> > + indio_dev->channels = nct720x_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(nct720x_channels);
> > + indio_dev->info = &nct720x_info;
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > + iio_device_register(indio_dev);
>
> devm_iio_device_register?
>
Yes, I would use devm_iio_device_registe to replace
iio_device_register and no need to use remove function.
> > +
> > + return 0;
> > +}
> > +
> > +static void nct720x_remove(struct i2c_client *client)
> > +{
> > + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > +
> > + iio_device_unregister(indio_dev);
>
> wouldn't need if used devm_ version
Got it.
> > +}
> > +
> > +static const unsigned short nct720x_address_list[] = {
> > + 0x1d, 0x1e, 0x35, 0x36, I2C_CLIENT_END
> > +};
> > +
> > +static const struct i2c_device_id nct720x_id[] = {
> > + { "nct7201", nct7201 },
> > + { "nct7202", nct7202 },
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(i2c, nct720x_id);
> > +
> > +static const struct of_device_id nct720x_of_match[] = {
> > + {
> > + .compatible = "nuvoton,nct7201",
> > + .data = (void *)nct7201
> > + },
> > + {
> > + .compatible = "nuvoton,nct7202",
> > + .data = (void *)nct7202
> > + },
> > + { },
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, nct720x_of_match);
> > +
> > +static struct i2c_driver nct720x_driver = {
> > + .driver = {
> > + .name = "nct720x",
> > + .of_match_table = nct720x_of_match,
> > + },
> > + .probe = nct720x_probe,
> > + .remove = nct720x_remove,
> > + .id_table = nct720x_id,
> > + .detect = nct720x_detect,
> > + .address_list = nct720x_address_list,
> > +};
> > +
> > +module_i2c_driver(nct720x_driver);
> > +
> > +MODULE_AUTHOR("Eason Yang <YHYANG2@nuvoton.com>");
> > +MODULE_DESCRIPTION("Nuvoton NCT720x voltage monitor driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.34.1
> >
> >
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 2/2] iio: adc: add Nuvoton NCT720x ADC driver
2024-11-06 2:39 ` [PATCH v1 2/2] iio: adc: add Nuvoton NCT720x ADC driver Eason Yang
` (2 preceding siblings ...)
2024-11-07 6:13 ` anish kumar
@ 2024-11-07 8:02 ` Andy Shevchenko
2024-11-09 14:37 ` Jonathan Cameron
4 siblings, 0 replies; 26+ messages in thread
From: Andy Shevchenko @ 2024-11-07 8:02 UTC (permalink / raw)
To: Eason Yang
Cc: avifishman70, tmaimon77, tali.perry1, venture, yuenn,
benjaminfair, jic23, lars, robh, krzk+dt, conor+dt, nuno.sa,
dlechner, javier.carrasco.cruz, marcelo.schmitt, olivier.moysan,
mitrutzceclan, matteomartelli3, alisadariana, joao.goncalves,
marius.cristea, mike.looijmans, chanh, KWLIU, yhyang2, openbmc,
linux-iio, devicetree, linux-kernel
On Wed, Nov 06, 2024 at 10:39:16AM +0800, Eason Yang 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 the all 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 support.
...
> + * Copyright (c) 2022 Nuvoton Inc.
2024?
...
+ array_size.h
+ bits.h
> +#include <linux/delay.h>
> +#include <linux/device.h>
+ err.h
> +#include <linux/i2c.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
Move these to be a separate group...
> +#include <linux/init.h>
+ mod_devicetable.h
> +#include <linux/module.h>
> +#include <linux/mutex.h>
+ types.h
...somewhere here after a blank line.
...
> +static const u8 REG_VIN[VIN_MAX] = { 0x00, 0x01, 0x02, 0x03, 0x04, 0x05,
> + 0x06, 0x07, 0x08, 0x09, 0x0A, 0x0B};
Better to have it done like this
static const u8 REG_VIN[VIN_MAX] = {
0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, /* 0 - 7 */
0x08, 0x09, 0x0A, 0x0B, /* 8 - 11 */
};
Note the following: the values are placed on separate lines with trailing
commas on each line and with grouping by powwer-of-2 per line.
> +static const u8 REG_VIN_HIGH_LIMIT[VIN_MAX] = { 0x20, 0x22, 0x24, 0x26, 0x28, 0x2A,
> + 0x2C, 0x2E, 0x30, 0x32, 0x34, 0x36};
> +static const u8 REG_VIN_LOW_LIMIT[VIN_MAX] = { 0x21, 0x23, 0x25, 0x27, 0x29, 0x2B,
> + 0x2D, 0x2F, 0x31, 0x33, 0x35, 0x37};
> +static const u8 REG_VIN_HIGH_LIMIT_LSB[VIN_MAX] = { 0x40, 0x42, 0x44, 0x46, 0x48, 0x4A,
> + 0x4C, 0x4E, 0x50, 0x52, 0x54, 0x56};
> +static const u8 REG_VIN_LOW_LIMIT_LSB[VIN_MAX] = { 0x41, 0x43, 0x45, 0x47, 0x49, 0x4B,
> + 0x4D, 0x4F, 0x51, 0x53, 0x55, 0x57};
> +static u8 nct720x_chan_to_index[] = {
> + 0, /* Not used */
> + 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11
> +};
Ditto for the above initialisers.
> +
> +
One blank line is enough.
> +/* List of supported devices */
> +enum nct720x_chips {
> + nct7201, nct7202
Leave trailing comma.
> +};
...
> +/* Read 1-byte register. Returns unsigned byte data or -ERRNO on error. */
> +static int nct720x_read_reg(struct nct720x_chip_info *chip, u8 reg)
> +{
> + struct i2c_client *client = chip->client;
> +
> + return i2c_smbus_read_byte_data(client, reg);
> +}
> +
> +/* Read 1-byte register. Returns unsigned word data or -ERRNO on error. */
> +static int nct720x_read_word_swapped_reg(struct nct720x_chip_info *chip, u8 reg)
> +{
> + struct i2c_client *client = chip->client;
> +
> + return i2c_smbus_read_word_swapped(client, reg);
> +}
> +
> +/*
> + * Read 2-byte register. Returns register in big-endian format or
> + * -ERRNO on error.
> + */
> +static int nct720x_read_reg16(struct nct720x_chip_info *chip, u8 reg)
> +{
> + struct i2c_client *client = chip->client;
> + int ret, low;
> + mutex_lock(&chip->access_lock);
If you are going with this, use cleanup.h.
> + ret = i2c_smbus_read_byte_data(client, reg);
> + if (ret >= 0) {
> + low = ret;
> + ret = i2c_smbus_read_byte_data(client, reg + 1);
> + if (ret >= 0)
> + ret = low | (ret << 8);
Hmm... Does it require to have two separate transactions? In case of regmap,
can't bulk transfer be performed for that?
> + }
> +
> + mutex_unlock(&chip->access_lock);
> + return ret;
> +}
I have no explanations why regmap I2C can't be used.
...
> +/* Write 1-byte register. Returns 0 or -ERRNO on error. */
> +static int nct720x_write_reg(struct nct720x_chip_info *chip, u8 reg, u8 val)
> +{
> + struct i2c_client *client = chip->client;
> + int err;
> +
> + err = i2c_smbus_write_byte_data(client, reg, val);
> + /* wait for write command to be finished */
Is there any justification for the chosen value (I mean datasheet
recommendations or so)?
> + mdelay(10);
> +
> + return err;
> +}
...
> +static int nct720x_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + int index = nct720x_chan_to_index[chan->address];
> + int v1, v2, volt, err;
> + struct nct720x_chip_info *chip = iio_priv(indio_dev);
> +
> + if (chan->type != IIO_VOLTAGE)
> + return -EOPNOTSUPP;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_PROCESSED:
> + mutex_lock(&chip->access_lock);
> + if (chip->use_read_byte_vin) {
> + /*
> + * MNTVIN Low Byte together with MNTVIN High Byte
> + * forms the 13-bit count value. If MNTVIN High
> + * Byte readout is read successively, the
> + * NCT7201/NCT7202 will latch the MNTVIN Low Byte
> + * for next read.
> + */
> + v1 = nct720x_read_reg(chip, REG_VIN[index]);
> + if (v1 < 0) {
> + err = v1;
> + goto abort;
> + }
> +
> + v2 = nct720x_read_reg(chip, REG_VOLT_LOW_BYTE);
> + if (v2 < 0) {
> + err = v2;
> + goto abort;
> + }
> + volt = (v1 << 8) | v2; /* Convert back to 16-bit value */
Hmm... I would expect volt to be __be16, but since you have two separate reads
it's probably fine.
> + } else {
> + /* NCT7201/NCT7202 also supports read word-size data */
> + volt = nct720x_read_word_swapped_reg(chip, REG_VIN[index]);
> + }
> +
> + /* Voltage(V) = 13bitCountValue * 0.0004995 */
> + volt = (volt >> 3) * NCT720X_IN_SCALING;
> + *val = volt / 10000;
> + mutex_unlock(&chip->access_lock);
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +abort:
> + mutex_unlock(&chip->access_lock);
> + return err;
Yeah, this will gone with cleanup.h in use.
Ditto for all other places like this.
> +}
...
> +{
> + struct nct720x_chip_info *chip = iio_priv(indio_dev);
> + int index, err = 0;
> + long v1, v2, volt;
Why long? Wouldn't int / unsigned int suffice for v1 / v2?
> +
> + index = nct720x_chan_to_index[chan->address];
> + volt = (val * 10000) / NCT720X_IN_SCALING;
Please, use named constant for 10000 as you used it at least twice in the code.
> + v1 = volt >> 5;
> + v2 = (volt & 0x1f) << 3;
In the similar way it seems like some endianess and bit width manipulations.
Can v be an array of u8:s which you just fill with put_unaligned_be16() or so?
> + if (chan->type != IIO_VOLTAGE)
> + return -EOPNOTSUPP;
> +
> + if (info == IIO_EV_INFO_VALUE) {
> + if (dir == IIO_EV_DIR_FALLING) {
> + mutex_lock(&chip->access_lock);
> + err = nct720x_write_reg(chip, REG_VIN_LOW_LIMIT[index], v1);
> + if (err < 0) {
> + pr_err("Failed to write REG_VIN%d_LOW_LIMIT\n", index + 1);
> + goto abort;
> + }
> +
> + err = nct720x_write_reg(chip, REG_VIN_LOW_LIMIT_LSB[index], v2);
> + if (err < 0) {
> + pr_err("Failed to write REG_VIN%d_LOW_LIMIT_LSB\n", index + 1);
> + goto abort;
> + }
> + } else {
> + mutex_lock(&chip->access_lock);
> + err = nct720x_write_reg(chip, REG_VIN_HIGH_LIMIT[index], v1);
> + if (err < 0) {
> + pr_err("Failed to write REG_VIN%d_HIGH_LIMIT\n", index + 1);
> + goto abort;
> + }
> +
> + err = nct720x_write_reg(chip, REG_VIN_HIGH_LIMIT_LSB[index], v2);
> + if (err < 0) {
> + pr_err("Failed to write REG_VIN%d_HIGH_LIMIT_LSB\n", index + 1);
> + goto abort;
> + }
> + }
> + }
> +abort:
> + mutex_unlock(&chip->access_lock);
> + return err;
> +}
...
> + strscpy(info->type, "nct720x", I2C_NAME_SIZE);
Use 2-argument strscpy().
...
> +static const struct i2c_device_id nct720x_id[];
Remove this. It's not needed.
...
> +static int nct720x_init_chip(struct nct720x_chip_info *chip)
> +{
> + int value = 0;
Why signed?
> + int err = 0;
Redundant assignments.
> + /* Initial reset */
> + err = nct720x_write_reg(chip, REG_CONFIGURATION, CONFIGURATION_INIT);
> + if (err) {
> + pr_err("Failed to write REG_CONFIGURATION\n");
> + return err;
> + }
> +
> + /* Enable Channel */
> + err = nct720x_write_reg(chip, REG_CHANNEL_ENABLE_1, 0xff);
Magic 0xff.
Should be GENMASK() ? Anything else?
> + if (err) {
> + pr_err("Failed to write REG_CHANNEL_ENABLE_1\n");
> + return err;
> + }
> +
> + if (chip->type == nct7202) {
> + err = nct720x_write_reg(chip, REG_CHANNEL_ENABLE_2, 0xf);
Ditto.
> + if (err) {
> + pr_err("Failed to write REG_CHANNEL_ENABLE_2\n");
> + return err;
> + }
> + }
> +
> + value = nct720x_read_reg16(chip, REG_CHANNEL_ENABLE_1);
> + if (value < 0)
> + return value;
> + chip->vin_mask = value;
> +
> + /* Start monitoring if needed */
> + value = nct720x_read_reg(chip, REG_CONFIGURATION);
> + if (value < 0) {
> + pr_err("Failed to read REG_CONFIGURATION\n");
> + return value;
> + }
> +
> + value |= CONFIGURATION_START;
> + err = nct720x_write_reg(chip, REG_CONFIGURATION, value);
> + if (err < 0) {
> + pr_err("Failed to write REG_CONFIGURATION\n");
> + return err;
> + }
> +
> + return 0;
> +}
...
> + ret = of_property_read_u32(client->dev.of_node, "read-vin-data-size", &tmp);
No OF-centric APIs in a new code, please.
Use device_property_read_u32() and include property.h for that.
> + if (ret < 0) {
> + pr_err("read-vin-data-size property not found\n");
> + return ret;
> + }
...
> + mutex_init(&chip->access_lock);
ret = devm_mutex_init(...);
...
> + /* this is only used for device removal purposes */
> + i2c_set_clientdata(client, indio_dev);
Will be gone after removing ->remove().
...
> + iio_device_register(indio_dev);
No error checks? In any case devm_ variant especially needs this.
> + return 0;
return devm_iio_device_register(...);
...
> +static const struct i2c_device_id nct720x_id[] = {
> + { "nct7201", nct7201 },
> + { "nct7202", nct7202 },
Please, use chip_info custom data structure instead of enum values.
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, nct720x_id);
> +
> +static const struct of_device_id nct720x_of_match[] = {
> + {
> + .compatible = "nuvoton,nct7201",
> + .data = (void *)nct7201
> + },
> + {
> + .compatible = "nuvoton,nct7202",
> + .data = (void *)nct7202
As per above and leave trailing commas.
> + },
> + { },
Drop trailing comma in the terminator entries.
> +};
> +
Redundant blank line.
> +MODULE_DEVICE_TABLE(of, nct720x_of_match);
> +
> +static struct i2c_driver nct720x_driver = {
> + .driver = {
> + .name = "nct720x",
> + .of_match_table = nct720x_of_match,
> + },
> + .probe = nct720x_probe,
> + .remove = nct720x_remove,
> + .id_table = nct720x_id,
> + .detect = nct720x_detect,
> + .address_list = nct720x_address_list,
> +};
> +
Redundant blank line.
> +module_i2c_driver(nct720x_driver);
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v1 2/2] iio: adc: add Nuvoton NCT720x ADC driver
2024-11-06 2:39 ` [PATCH v1 2/2] iio: adc: add Nuvoton NCT720x ADC driver Eason Yang
` (3 preceding siblings ...)
2024-11-07 8:02 ` Andy Shevchenko
@ 2024-11-09 14:37 ` Jonathan Cameron
2024-12-04 2:50 ` Yu-Hsian Yang
4 siblings, 1 reply; 26+ messages in thread
From: Jonathan Cameron @ 2024-11-09 14:37 UTC (permalink / raw)
To: Eason Yang
Cc: avifishman70, tmaimon77, tali.perry1, venture, yuenn,
benjaminfair, lars, robh, krzk+dt, conor+dt, nuno.sa, dlechner,
javier.carrasco.cruz, andy, marcelo.schmitt, olivier.moysan,
mitrutzceclan, matteomartelli3, alisadariana, joao.goncalves,
marius.cristea, mike.looijmans, chanh, KWLIU, yhyang2, openbmc,
linux-iio, devicetree, linux-kernel
On Wed, 6 Nov 2024 10:39:16 +0800
Eason Yang <j2anfernee@gmail.com> 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 the all 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 support.
>
> Signed-off-by: Eason Yang <j2anfernee@gmail.com>
You've gotten some good review already so I may well repeat stuff plus
will only take a fairly superficial look at this version.
> diff --git a/drivers/iio/adc/nct720x.c b/drivers/iio/adc/nct720x.c
> new file mode 100644
> index 000000000000..e589479fd06e
> --- /dev/null
> +++ b/drivers/iio/adc/nct720x.c
> +
> +/* List of supported devices */
> +enum nct720x_chips {
> + nct7201, nct7202
This should be replaced by chip specific information structures.
That ends up a lot more extensible than using an enum and fixing up
what happens in all code paths on a per enum value basis.
> +};
> +
> +struct nct720x_chip_info {
> + struct i2c_client *client;
> + enum nct720x_chips type;
> + struct mutex access_lock; /* for multi-byte read and write operations */
> + int vin_max; /* number of VIN channels */
> + u32 vin_mask;
> + bool use_read_byte_vin;
> +};
> +#define NCT720X_VOLTAGE_CHANNEL(chan, addr) \
> + { \
> + .type = IIO_VOLTAGE, \
> + .indexed = 1, \
> + .channel = chan, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> + .address = addr, \
> + .event_spec = nct720x_events, \
> + .num_event_specs = ARRAY_SIZE(nct720x_events), \
> + }
> +
> +#define NCT720X_VOLTAGE_CHANNEL_DIFF(chan1, chan2, addr) \
> + { \
> + .type = IIO_VOLTAGE, \
> + .indexed = 1, \
> + .channel = (chan1), \
> + .channel2 = (chan2), \
> + .differential = 1, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
See below - should be _RAW and _SCALE not _PROCESSED.
> + .address = addr, \
> + .event_spec = nct720x_events, \
> + .num_event_specs = ARRAY_SIZE(nct720x_events), \
> + }
> +
> +static const struct iio_chan_spec nct720x_channels[] = {
> + NCT720X_VOLTAGE_CHANNEL(1, 1),
> + NCT720X_VOLTAGE_CHANNEL(2, 2),
> + NCT720X_VOLTAGE_CHANNEL(3, 3),
> + NCT720X_VOLTAGE_CHANNEL(4, 4),
> + NCT720X_VOLTAGE_CHANNEL(5, 5),
> + NCT720X_VOLTAGE_CHANNEL(6, 6),
> + NCT720X_VOLTAGE_CHANNEL(7, 7),
> + NCT720X_VOLTAGE_CHANNEL(8, 8),
> + NCT720X_VOLTAGE_CHANNEL(9, 9),
> + NCT720X_VOLTAGE_CHANNEL(10, 10),
> + NCT720X_VOLTAGE_CHANNEL(11, 11),
> + NCT720X_VOLTAGE_CHANNEL(12, 12),
> + NCT720X_VOLTAGE_CHANNEL_DIFF(1, 2, 1),
> + NCT720X_VOLTAGE_CHANNEL_DIFF(3, 4, 3),
> + NCT720X_VOLTAGE_CHANNEL_DIFF(5, 6, 5),
> + NCT720X_VOLTAGE_CHANNEL_DIFF(7, 8, 7),
> + NCT720X_VOLTAGE_CHANNEL_DIFF(9, 10, 9),
> + NCT720X_VOLTAGE_CHANNEL_DIFF(11, 12, 11),
> +};
> +
> +/* Read 1-byte register. Returns unsigned byte data or -ERRNO on error. */
> +static int nct720x_read_reg(struct nct720x_chip_info *chip, u8 reg)
> +{
> + struct i2c_client *client = chip->client;
> +
> + return i2c_smbus_read_byte_data(client, reg);
> +}
> +
> +/* Read 1-byte register. Returns unsigned word data or -ERRNO on error. */
> +static int nct720x_read_word_swapped_reg(struct nct720x_chip_info *chip, u8 reg)
> +{
> + struct i2c_client *client = chip->client;
> +
> + return i2c_smbus_read_word_swapped(client, reg);
Don't provide these wrappers as they don't add anything useful.
Make the calls directly inline.
> +}
> +
> +/*
> + * Read 2-byte register. Returns register in big-endian format or
> + * -ERRNO on error.
> + */
> +static int nct720x_read_reg16(struct nct720x_chip_info *chip, u8 reg)
> +{
> + struct i2c_client *client = chip->client;
> + int ret, low;
> +
> + mutex_lock(&chip->access_lock);
guard()
> + ret = i2c_smbus_read_byte_data(client, reg);
> + if (ret >= 0) {
> + low = ret;
> + ret = i2c_smbus_read_byte_data(client, reg + 1);
> + if (ret >= 0)
> + ret = low | (ret << 8);
if (ret < 0)
return ret;
reg = get_unaligned_le16()
on an appropriate u8 data[2]; ideally filled by a bulk regmap read.
> + }
> +
> + mutex_unlock(&chip->access_lock);
> + return ret;
> +}
> +
> +/* Write 1-byte register. Returns 0 or -ERRNO on error. */
> +static int nct720x_write_reg(struct nct720x_chip_info *chip, u8 reg, u8 val)
> +{
> + struct i2c_client *client = chip->client;
> + int err;
> +
> + err = i2c_smbus_write_byte_data(client, reg, val);
> + /* wait for write command to be finished */
If this is needed, provide a datasheet reference. It is very ususual to
see a significant delay needed.
> + mdelay(10);
> +
> + return err;
> +}
> +
> +static int nct720x_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + int index = nct720x_chan_to_index[chan->address];
> + int v1, v2, volt, err;
> + struct nct720x_chip_info *chip = iio_priv(indio_dev);
> +
> + if (chan->type != IIO_VOLTAGE)
> + return -EOPNOTSUPP;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_PROCESSED:
> + mutex_lock(&chip->access_lock);
> + if (chip->use_read_byte_vin) {
Ah. So resolution doesn't change, you are controlling the i2c acceses?
That should come from the i2c controller capabilities, not DT for this
device.
> + /*
> + * MNTVIN Low Byte together with MNTVIN High Byte
> + * forms the 13-bit count value. If MNTVIN High
> + * Byte readout is read successively, the
> + * NCT7201/NCT7202 will latch the MNTVIN Low Byte
> + * for next read.
> + */
> + v1 = nct720x_read_reg(chip, REG_VIN[index]);
> + if (v1 < 0) {
> + err = v1;
> + goto abort;
> + }
> +
> + v2 = nct720x_read_reg(chip, REG_VOLT_LOW_BYTE);
> + if (v2 < 0) {
> + err = v2;
> + goto abort;
> + }
> + volt = (v1 << 8) | v2; /* Convert back to 16-bit value */
> + } else {
> + /* NCT7201/NCT7202 also supports read word-size data */
> + volt = nct720x_read_word_swapped_reg(chip, REG_VIN[index]);
> + }
> +
> + /* Voltage(V) = 13bitCountValue * 0.0004995 */
Present this as _RAW and provide _SCALE to userspace to be able to do this
maths. Very unusual for an ADC driver to provided processed channels. Normally
only occurs it there is something non linear going on.
> + volt = (volt >> 3) * NCT720X_IN_SCALING;
> + *val = volt / 10000;
> + mutex_unlock(&chip->access_lock);
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +abort:
> + mutex_unlock(&chip->access_lock);
> + return err;
> +}
> +
> +static int nct720x_read_event_value(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info,
> + int *val, int *val2)
> +{
> + struct nct720x_chip_info *chip = iio_priv(indio_dev);
> + int v1, v2, err;
> + int volt = 0;
> + int index = nct720x_chan_to_index[chan->address];
> +
> + if (chan->type != IIO_VOLTAGE)
> + return -EOPNOTSUPP;
> +
> + if (info == IIO_EV_INFO_VALUE) {
As below - flip logic.
> + if (dir == IIO_EV_DIR_FALLING) {
> + if (chip->use_read_byte_vin) {
> + /*
> + * Low limit VIN Low Byte together with Low limit VIN High Byte
> + forms the 13-bit count value
> + */
> + mutex_lock(&chip->access_lock);
> + v1 = nct720x_read_reg(chip, REG_VIN_LOW_LIMIT[index]);
> + if (v1 < 0) {
> + err = v1;
> + goto abort;
> + }
> +
> + v2 = nct720x_read_reg(chip, REG_VIN_LOW_LIMIT_LSB[index]);
> + if (v2 < 0) {
> + err = v2;
> + goto abort;
> + }
> + mutex_unlock(&chip->access_lock);
> + volt = (v1 << 8) | v2; /* Convert back to 16-bit value */
rather see this as a get_unaligned_le16 on an array of u8.
In some cases that ends up quite a bit cheaper and it also documents what is going on.
> + } else {
> + /* NCT7201/NCT7202 also supports read word-size data */
> + volt = nct720x_read_word_swapped_reg(chip,
> + REG_VIN_LOW_LIMIT[index]);
> + }
> + } else {
> + if (chip->use_read_byte_vin) {
> + /*
> + * High limit VIN Low Byte together with high limit VIN High Byte
> + * forms the 13-bit count value
> + */
> + mutex_lock(&chip->access_lock);
> + v1 = nct720x_read_reg(chip, REG_VIN_HIGH_LIMIT[index]);
> + if (v1 < 0) {
> + err = v1;
> + goto abort;
> + }
> +
> + v2 = nct720x_read_reg(chip, REG_VIN_HIGH_LIMIT_LSB[index]);
> + if (v2 < 0) {
> + err = v2;
> + goto abort;
> + }
> + mutex_unlock(&chip->access_lock);
> + volt = (v1 << 8) | v2; /* Convert back to 16-bit value */
> + } else {
> + /* NCT7201/NCT7202 also supports read word-size data */
> + volt = nct720x_read_word_swapped_reg(chip,
> + REG_VIN_HIGH_LIMIT[index]);
> + }
> + }
> + }
> + /* Voltage(V) = 13bitCountValue * 0.0004995 */
> + volt = (volt >> 3) * NCT720X_IN_SCALING;
> + *val = volt / 10000;
> +
> + return IIO_VAL_INT;
> +abort:
> + mutex_unlock(&chip->access_lock);
guard() in appropriate places.
Again, the lock and unlock should ideally be in same scope.
> + return err;
> +}
> +
> +static int nct720x_write_event_value(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info,
> + int val, int val2)
> +{
> + struct nct720x_chip_info *chip = iio_priv(indio_dev);
> + int index, err = 0;
> + long v1, v2, volt;
> +
> + index = nct720x_chan_to_index[chan->address];
> + volt = (val * 10000) / NCT720X_IN_SCALING;
> + v1 = volt >> 5;
> + v2 = (volt & 0x1f) << 3;
Some explanatory comments for this would be good.
> +
> + if (chan->type != IIO_VOLTAGE)
> + return -EOPNOTSUPP;
> +
> + if (info == IIO_EV_INFO_VALUE) {
Flip logic.
if (info != IIO_EV_INFO_VALUE)
return -EINVAL;
if (dir == IIO_EVE_DIR_FALLING) etc
> + if (dir == IIO_EV_DIR_FALLING) {
> + mutex_lock(&chip->access_lock);
This is badly nested. Mutex lock and unlock should be in same scope.
So I'd pull mutex_lock() out of this if stack and use guard(mutex) instead.
That way you can just return on error.
> + err = nct720x_write_reg(chip, REG_VIN_LOW_LIMIT[index], v1);
> + if (err < 0) {
> + pr_err("Failed to write REG_VIN%d_LOW_LIMIT\n", index + 1);
> + goto abort;
> + }
> +
> + err = nct720x_write_reg(chip, REG_VIN_LOW_LIMIT_LSB[index], v2);
> + if (err < 0) {
> + pr_err("Failed to write REG_VIN%d_LOW_LIMIT_LSB\n", index + 1);
> + goto abort;
> + }
> + } else {
> + mutex_lock(&chip->access_lock);
> + err = nct720x_write_reg(chip, REG_VIN_HIGH_LIMIT[index], v1);
> + if (err < 0) {
> + pr_err("Failed to write REG_VIN%d_HIGH_LIMIT\n", index + 1);
> + goto abort;
> + }
> +
> + err = nct720x_write_reg(chip, REG_VIN_HIGH_LIMIT_LSB[index], v2);
> + if (err < 0) {
> + pr_err("Failed to write REG_VIN%d_HIGH_LIMIT_LSB\n", index + 1);
> + goto abort;
> + }
> + }
> + }
> +abort:
> + mutex_unlock(&chip->access_lock);
> + return err;
> +}
> +
> +static int nct720x_write_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + int state)
> +{
> + int err = 0;
> + struct nct720x_chip_info *chip = iio_priv(indio_dev);
> + int index = nct720x_chan_to_index[chan->address];
> + unsigned int mask;
> +
> + mask = BIT(index);
> +
> + if (chan->type != IIO_VOLTAGE)
> + return -EOPNOTSUPP;
> +
> + if (!state && (chip->vin_mask & mask))
> + chip->vin_mask &= ~mask;
> + else if (state && !(chip->vin_mask & mask))
> + chip->vin_mask |= mask;
> +
> + mutex_lock(&chip->access_lock);
guard(mutex)(&chip->access_lock);
> +
> + err = nct720x_write_reg(chip, REG_CHANNEL_ENABLE_1, chip->vin_mask & 0xff);
> + if (err < 0) {
> + pr_err("Failed to write REG_CHANNEL_ENABLE_1\n");
> + goto abort;
dev_err()
return err;
> + }
> +
> + if (chip->type == nct7202) {
Gain, base this on a chip_info flag.
> + err = nct720x_write_reg(chip, REG_CHANNEL_ENABLE_2, chip->vin_mask >> 8);
> + if (err < 0) {
> + pr_err("Failed to write REG_CHANNEL_ENABLE_2\n");
> + goto abort;
Same as above.
> + }
> + }
> +abort:
> + mutex_unlock(&chip->access_lock);
> + return err;
> +}
> +
> +static int nct720x_detect(struct i2c_client *client,
> + struct i2c_board_info *info)
> +{
> + struct i2c_adapter *adapter = client->adapter;
> +
> + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> + I2C_FUNC_SMBUS_WORD_DATA))
> + return -ENODEV;
> +
> + /* Determine the chip type. */
> + if (i2c_smbus_read_byte_data(client, REG_VENDOR_ID) != NUVOTON_ID ||
> + i2c_smbus_read_byte_data(client, REG_CHIP_ID) != NCT720X_ID ||
> + i2c_smbus_read_byte_data(client, REG_DEVICE_ID) != NCT720X_DEVICE_ID)
> + return -ENODEV;
> +
> + strscpy(info->type, "nct720x", I2C_NAME_SIZE);
as below. It's unusual to find a detect in an IIO driver because the firmware
normally tells us what is there.
> +
> + return 0;
> +}
> +
> +static const struct iio_info nct720x_info = {
> + .read_raw = &nct720x_read_raw,
> + .read_event_config = &nct720x_read_event_config,
> + .write_event_config = &nct720x_write_event_config,
> + .read_event_value = &nct720x_read_event_value,
> + .write_event_value = &nct720x_write_event_value,
> +};
> +
> +static const struct i2c_device_id nct720x_id[];
> +
> +static int nct720x_init_chip(struct nct720x_chip_info *chip)
> +{
> + int value = 0;
> + int err = 0;
> +
> + /* Initial reset */
Maybe ignore datasheet naming and call that CONFIGURATION_INIT CONFIGURATION_RESET
at which point he comment is unneeded.
> + err = nct720x_write_reg(chip, REG_CONFIGURATION, CONFIGURATION_INIT);
> + if (err) {
> + pr_err("Failed to write REG_CONFIGURATION\n");
> + return err;
> + }
> +
> + /* Enable Channel */
> + err = nct720x_write_reg(chip, REG_CHANNEL_ENABLE_1, 0xff);
What does 0xFF represent? I guess all channels. If so maybe need
to build with GENMASK so it is obvious this is enabling 8 channels.
> + if (err) {
> + pr_err("Failed to write REG_CHANNEL_ENABLE_1\n");
> + return err;
> + }
> +
> + if (chip->type == nct7202) {
Make this 'data' in the chip_info structure. Probably a flat to say
there is a REG_CHANNEL_ENABLE_2. That chip->type wants to go away infavour
of chip->chip_info.X flags.
> + err = nct720x_write_reg(chip, REG_CHANNEL_ENABLE_2, 0xf);
> + if (err) {
> + pr_err("Failed to write REG_CHANNEL_ENABLE_2\n");
> + return err;
> + }
> + }
> +
> + value = nct720x_read_reg16(chip, REG_CHANNEL_ENABLE_1);
> + if (value < 0)
> + return value;
> + chip->vin_mask = value;
> +
> + /* Start monitoring if needed */
> + value = nct720x_read_reg(chip, REG_CONFIGURATION);
Using regmap would let you simple do a single bit write in one call
Definitely look at whether there is anything stopping you using that
helpful infrastructure.
> + if (value < 0) {
> + pr_err("Failed to read REG_CONFIGURATION\n");
> + return value;
> + }
> +
> + value |= CONFIGURATION_START;
> + err = nct720x_write_reg(chip, REG_CONFIGURATION, value);
> + if (err < 0) {
> + pr_err("Failed to write REG_CONFIGURATION\n");
dev_err() if not called only from probe, return dev_err_probe() if only called
from probe()
> + return err;
> + }
> +
> + return 0;
> +}
> +
> +static int nct720x_probe(struct i2c_client *client)
> +{
> + const struct i2c_device_id *id = i2c_client_get_device_id(client);
> + struct nct720x_chip_info *chip;
> + struct iio_dev *indio_dev;
> + int ret;
> + u32 tmp;
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
> + if (!indio_dev)
> + return -ENOMEM;
> + chip = iio_priv(indio_dev);
> +
> + if (client->dev.of_node)
> + chip->type = (enum nct720x_chips)device_get_match_data(&client->dev);
> + else
> + chip->type = i2c_match_id(nct720x_id, client)->driver_data;
i2c_get_match_data() but careful with zeros... It is much better to use that with
an actual pointer.
> +
> + chip->vin_max = (chip->type == nct7201) ? NCT7201_VIN_MAX : NCT7202_VIN_MAX;
> +
> + ret = of_property_read_u32(client->dev.of_node, "read-vin-data-size", &tmp);
As in dt binding. If we keep this (I'm doubtful that it makes sense) define a default
so that the property doesn't need to be provided. 16 would be the most obvious choice.
Then just don't check the error value when reading the property. That is.
tmp = 16;
of_property_read_u32();
> + if (ret < 0) {
> + pr_err("read-vin-data-size property not found\n");
> + return ret;
> + }
> +
> + if (tmp == 8) {
> + chip->use_read_byte_vin = true;
> + } else if (tmp == 16) {
> + chip->use_read_byte_vin = false;
> + } else {
> + pr_err("invalid read-vin-data-size (%d)\n", tmp);
> + return -EINVAL;
> + }
> +
> + mutex_init(&chip->access_lock);
For new code prefer
ret = devm_mutex_init()
if (ret)
return ret;
It only helps in weird debug cases but also costs us very little to support those.
> +
> + /* this is only used for device removal purposes */
> + i2c_set_clientdata(client, indio_dev);
Won't be needed after the change below.
> +
> + chip->client = client;
> +
> + ret = nct720x_init_chip(chip);
> + if (ret < 0)
> + return ret;
> +
> + indio_dev->name = id->name;
Put the name in the chip_info structure. id->name is a complex thing
when other firmwares come into play, so better to just hard code the strings
somewhere so we always know what we are getting.
> + indio_dev->channels = nct720x_channels;
> + indio_dev->num_channels = ARRAY_SIZE(nct720x_channels);
> + indio_dev->info = &nct720x_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + iio_device_register(indio_dev);
return devm_iio_device_register();
Then you can drop the remove callback.
> +
> + return 0;
> +}
> +
> +static void nct720x_remove(struct i2c_client *client)
Don't use wildcards even within the driver. Just name everything
after one supported part.
nct7202_remove() etc.
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +
> + iio_device_unregister(indio_dev);
> +}
> +
> +static const unsigned short nct720x_address_list[] = {
> + 0x1d, 0x1e, 0x35, 0x36, I2C_CLIENT_END
> +};
> +
> +static const struct i2c_device_id nct720x_id[] = {
> + { "nct7201", nct7201 },
> + { "nct7202", nct7202 },
> + {}
{ }
I'm trying to slowly standardise on this formatting in IIO
so keen not to introduce more cases. It's an arbitrary choice
but I went with the space.
> +};
> +MODULE_DEVICE_TABLE(i2c, nct720x_id);
> +
> +static const struct of_device_id nct720x_of_match[] = {
> + {
> + .compatible = "nuvoton,nct7201",
> + .data = (void *)nct7201
> + },
> + {
> + .compatible = "nuvoton,nct7202",
> + .data = (void *)nct7202
Use a pointer to a chip_info structure with all the chip specific
stuff encoded as data. That is both more extensible and removes some
ambiguities around whether zero is an error or not.
> + },
> + { },
No trailing comma
> +};
> +
> +MODULE_DEVICE_TABLE(of, nct720x_of_match);
> +
> +static struct i2c_driver nct720x_driver = {
> + .driver = {
> + .name = "nct720x",
> + .of_match_table = nct720x_of_match,
> + },
> + .probe = nct720x_probe,
> + .remove = nct720x_remove,
> + .id_table = nct720x_id,
> + .detect = nct720x_detect,
Do you need detect? That's kind of ancient infrastructure that I thought
no one used any more (same for the address list).
> + .address_list = nct720x_address_list,
> +};
> +
> +module_i2c_driver(nct720x_driver);
> +
> +MODULE_AUTHOR("Eason Yang <YHYANG2@nuvoton.com>");
> +MODULE_DESCRIPTION("Nuvoton NCT720x voltage monitor driver");
> +MODULE_LICENSE("GPL");
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v1 2/2] iio: adc: add Nuvoton NCT720x ADC driver
2024-11-09 14:37 ` Jonathan Cameron
@ 2024-12-04 2:50 ` Yu-Hsian Yang
0 siblings, 0 replies; 26+ messages in thread
From: Yu-Hsian Yang @ 2024-12-04 2:50 UTC (permalink / raw)
To: Jonathan Cameron
Cc: avifishman70, tmaimon77, tali.perry1, venture, yuenn,
benjaminfair, lars, robh, krzk+dt, conor+dt, nuno.sa, dlechner,
javier.carrasco.cruz, andy, marcelo.schmitt, olivier.moysan,
mitrutzceclan, matteomartelli3, alisadariana, joao.goncalves,
marius.cristea, mike.looijmans, chanh, KWLIU, yhyang2, openbmc,
linux-iio, devicetree, linux-kernel
Dear Jonathan Cameron,
Thank you for your comment.
Jonathan Cameron <jic23@kernel.org> 於 2024年11月9日 週六 下午10:37寫道：
>
> On Wed, 6 Nov 2024 10:39:16 +0800
> Eason Yang <j2anfernee@gmail.com> 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 the all 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 support.
> >
> > Signed-off-by: Eason Yang <j2anfernee@gmail.com>
> You've gotten some good review already so I may well repeat stuff plus
> will only take a fairly superficial look at this version.
>
>
> > diff --git a/drivers/iio/adc/nct720x.c b/drivers/iio/adc/nct720x.c
> > new file mode 100644
> > index 000000000000..e589479fd06e
> > --- /dev/null
> > +++ b/drivers/iio/adc/nct720x.c
>
> > +
> > +/* List of supported devices */
> > +enum nct720x_chips {
> > + nct7201, nct7202
> This should be replaced by chip specific information structures.
> That ends up a lot more extensible than using an enum and fixing up
> what happens in all code paths on a per enum value basis.
>
We would pass a pointer to the data structure not id to describe the
particular variant.
> > +};
> > +
> > +struct nct720x_chip_info {
> > + struct i2c_client *client;
> > + enum nct720x_chips type;
> > + struct mutex access_lock; /* for multi-byte read and write operations */
> > + int vin_max; /* number of VIN channels */
> > + u32 vin_mask;
> > + bool use_read_byte_vin;
> > +};
>
> > +#define NCT720X_VOLTAGE_CHANNEL(chan, addr) \
> > + { \
> > + .type = IIO_VOLTAGE, \
> > + .indexed = 1, \
> > + .channel = chan, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> > + .address = addr, \
> > + .event_spec = nct720x_events, \
> > + .num_event_specs = ARRAY_SIZE(nct720x_events), \
> > + }
> > +
> > +#define NCT720X_VOLTAGE_CHANNEL_DIFF(chan1, chan2, addr) \
> > + { \
> > + .type = IIO_VOLTAGE, \
> > + .indexed = 1, \
> > + .channel = (chan1), \
> > + .channel2 = (chan2), \
> > + .differential = 1, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> See below - should be _RAW and _SCALE not _PROCESSED.
We would use _RAW and _SCALE to instead of _PROCESSED.
> > + .address = addr, \
> > + .event_spec = nct720x_events, \
> > + .num_event_specs = ARRAY_SIZE(nct720x_events), \
> > + }
> > +
> > +static const struct iio_chan_spec nct720x_channels[] = {
> > + NCT720X_VOLTAGE_CHANNEL(1, 1),
> > + NCT720X_VOLTAGE_CHANNEL(2, 2),
> > + NCT720X_VOLTAGE_CHANNEL(3, 3),
> > + NCT720X_VOLTAGE_CHANNEL(4, 4),
> > + NCT720X_VOLTAGE_CHANNEL(5, 5),
> > + NCT720X_VOLTAGE_CHANNEL(6, 6),
> > + NCT720X_VOLTAGE_CHANNEL(7, 7),
> > + NCT720X_VOLTAGE_CHANNEL(8, 8),
> > + NCT720X_VOLTAGE_CHANNEL(9, 9),
> > + NCT720X_VOLTAGE_CHANNEL(10, 10),
> > + NCT720X_VOLTAGE_CHANNEL(11, 11),
> > + NCT720X_VOLTAGE_CHANNEL(12, 12),
> > + NCT720X_VOLTAGE_CHANNEL_DIFF(1, 2, 1),
> > + NCT720X_VOLTAGE_CHANNEL_DIFF(3, 4, 3),
> > + NCT720X_VOLTAGE_CHANNEL_DIFF(5, 6, 5),
> > + NCT720X_VOLTAGE_CHANNEL_DIFF(7, 8, 7),
> > + NCT720X_VOLTAGE_CHANNEL_DIFF(9, 10, 9),
> > + NCT720X_VOLTAGE_CHANNEL_DIFF(11, 12, 11),
> > +};
> > +
> > +/* Read 1-byte register. Returns unsigned byte data or -ERRNO on error. */
> > +static int nct720x_read_reg(struct nct720x_chip_info *chip, u8 reg)
> > +{
> > + struct i2c_client *client = chip->client;
> > +
> > + return i2c_smbus_read_byte_data(client, reg);
> > +}
> > +
> > +/* Read 1-byte register. Returns unsigned word data or -ERRNO on error. */
> > +static int nct720x_read_word_swapped_reg(struct nct720x_chip_info *chip, u8 reg)
> > +{
> > + struct i2c_client *client = chip->client;
> > +
> > + return i2c_smbus_read_word_swapped(client, reg);
>
> Don't provide these wrappers as they don't add anything useful.
> Make the calls directly inline.
>
We would remove this function and use regmap to instead of i2c API.
> > +}
> > +
> > +/*
> > + * Read 2-byte register. Returns register in big-endian format or
> > + * -ERRNO on error.
> > + */
> > +static int nct720x_read_reg16(struct nct720x_chip_info *chip, u8 reg)
> > +{
> > + struct i2c_client *client = chip->client;
> > + int ret, low;
> > +
> > + mutex_lock(&chip->access_lock);
> guard()
>
Got it.
> > + ret = i2c_smbus_read_byte_data(client, reg);
> > + if (ret >= 0) {
> > + low = ret;
> > + ret = i2c_smbus_read_byte_data(client, reg + 1);
> > + if (ret >= 0)
> > + ret = low | (ret << 8);
> if (ret < 0)
> return ret;
>
> reg = get_unaligned_le16()
> on an appropriate u8 data[2]; ideally filled by a bulk regmap read.
>
We would use regmap read.
>
> > + }
> > +
> > + mutex_unlock(&chip->access_lock);
> > + return ret;
> > +}
> > +
> > +/* Write 1-byte register. Returns 0 or -ERRNO on error. */
> > +static int nct720x_write_reg(struct nct720x_chip_info *chip, u8 reg, u8 val)
> > +{
> > + struct i2c_client *client = chip->client;
> > + int err;
> > +
> > + err = i2c_smbus_write_byte_data(client, reg, val);
> > + /* wait for write command to be finished */
> If this is needed, provide a datasheet reference. It is very ususual to
> see a significant delay needed.
Chip RD do the simulation, we need delay 25 msec in reset register
for registers ready to access.
And I2c write is not needed to delay.
These information would add into datasheet afterwards.
> > + mdelay(10);
> > +
> > + return err;
> > +}
> > +
> > +static int nct720x_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val, int *val2, long mask)
> > +{
> > + int index = nct720x_chan_to_index[chan->address];
> > + int v1, v2, volt, err;
> > + struct nct720x_chip_info *chip = iio_priv(indio_dev);
> > +
> > + if (chan->type != IIO_VOLTAGE)
> > + return -EOPNOTSUPP;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_PROCESSED:
> > + mutex_lock(&chip->access_lock);
> > + if (chip->use_read_byte_vin) {
>
> Ah. So resolution doesn't change, you are controlling the i2c acceses?
>
> That should come from the i2c controller capabilities, not DT for this
> device.
>
>
>
The use_read_byte_vin was discussed in dt-binding commit,
We would use read word first and remove read-vin-data-size property.
> > + /*
> > + * MNTVIN Low Byte together with MNTVIN High Byte
> > + * forms the 13-bit count value. If MNTVIN High
> > + * Byte readout is read successively, the
> > + * NCT7201/NCT7202 will latch the MNTVIN Low Byte
> > + * for next read.
> > + */
> > + v1 = nct720x_read_reg(chip, REG_VIN[index]);
> > + if (v1 < 0) {
> > + err = v1;
> > + goto abort;
> > + }
> > +
> > + v2 = nct720x_read_reg(chip, REG_VOLT_LOW_BYTE);
> > + if (v2 < 0) {
> > + err = v2;
> > + goto abort;
> > + }
> > + volt = (v1 << 8) | v2; /* Convert back to 16-bit value */
> > + } else {
> > + /* NCT7201/NCT7202 also supports read word-size data */
> > + volt = nct720x_read_word_swapped_reg(chip, REG_VIN[index]);
> > + }
> > +
> > + /* Voltage(V) = 13bitCountValue * 0.0004995 */
> Present this as _RAW and provide _SCALE to userspace to be able to do this
> maths. Very unusual for an ADC driver to provided processed channels. Normally
> only occurs it there is something non linear going on.
Yes, we would use _RAW and _SCALE to instead of _PROCESSED.
>
>
> > + volt = (volt >> 3) * NCT720X_IN_SCALING;
> > + *val = volt / 10000;
> > + mutex_unlock(&chip->access_lock);
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > +abort:
> > + mutex_unlock(&chip->access_lock);
> > + return err;
> > +}
> > +
> > +static int nct720x_read_event_value(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir,
> > + enum iio_event_info info,
> > + int *val, int *val2)
> > +{
> > + struct nct720x_chip_info *chip = iio_priv(indio_dev);
> > + int v1, v2, err;
> > + int volt = 0;
> > + int index = nct720x_chan_to_index[chan->address];
> > +
> > + if (chan->type != IIO_VOLTAGE)
> > + return -EOPNOTSUPP;
> > +
> > + if (info == IIO_EV_INFO_VALUE) {
> As below - flip logic.
>
> > + if (dir == IIO_EV_DIR_FALLING) {
> > + if (chip->use_read_byte_vin) {
> > + /*
> > + * Low limit VIN Low Byte together with Low limit VIN High Byte
> > + forms the 13-bit count value
> > + */
> > + mutex_lock(&chip->access_lock);
> > + v1 = nct720x_read_reg(chip, REG_VIN_LOW_LIMIT[index]);
> > + if (v1 < 0) {
> > + err = v1;
> > + goto abort;
> > + }
> > +
> > + v2 = nct720x_read_reg(chip, REG_VIN_LOW_LIMIT_LSB[index]);
> > + if (v2 < 0) {
> > + err = v2;
> > + goto abort;
> > + }
> > + mutex_unlock(&chip->access_lock);
> > + volt = (v1 << 8) | v2; /* Convert back to 16-bit value */
>
> rather see this as a get_unaligned_le16 on an array of u8.
> In some cases that ends up quite a bit cheaper and it also documents what is going on.
>
We would use get_unaligned_le16 to convert the array of u8.
> > + } else {
> > + /* NCT7201/NCT7202 also supports read word-size data */
> > + volt = nct720x_read_word_swapped_reg(chip,
> > + REG_VIN_LOW_LIMIT[index]);
> > + }
> > + } else {
> > + if (chip->use_read_byte_vin) {
> > + /*
> > + * High limit VIN Low Byte together with high limit VIN High Byte
> > + * forms the 13-bit count value
> > + */
> > + mutex_lock(&chip->access_lock);
> > + v1 = nct720x_read_reg(chip, REG_VIN_HIGH_LIMIT[index]);
> > + if (v1 < 0) {
> > + err = v1;
> > + goto abort;
> > + }
> > +
> > + v2 = nct720x_read_reg(chip, REG_VIN_HIGH_LIMIT_LSB[index]);
> > + if (v2 < 0) {
> > + err = v2;
> > + goto abort;
> > + }
> > + mutex_unlock(&chip->access_lock);
> > + volt = (v1 << 8) | v2; /* Convert back to 16-bit value */
> > + } else {
> > + /* NCT7201/NCT7202 also supports read word-size data */
> > + volt = nct720x_read_word_swapped_reg(chip,
> > + REG_VIN_HIGH_LIMIT[index]);
> > + }
> > + }
> > + }
> > + /* Voltage(V) = 13bitCountValue * 0.0004995 */
> > + volt = (volt >> 3) * NCT720X_IN_SCALING;
> > + *val = volt / 10000;
> > +
> > + return IIO_VAL_INT;
> > +abort:
> > + mutex_unlock(&chip->access_lock);
> guard() in appropriate places.
> Again, the lock and unlock should ideally be in same scope.
>
We would put guard(mutex) in appropriate places.
> > + return err;
> > +}
> > +
> > +static int nct720x_write_event_value(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir,
> > + enum iio_event_info info,
> > + int val, int val2)
> > +{
> > + struct nct720x_chip_info *chip = iio_priv(indio_dev);
> > + int index, err = 0;
> > + long v1, v2, volt;
> > +
> > + index = nct720x_chan_to_index[chan->address];
> > + volt = (val * 10000) / NCT720X_IN_SCALING;
> > + v1 = volt >> 5;
> > + v2 = (volt & 0x1f) << 3;
> Some explanatory comments for this would be good.
>
/* Voltage(V) = 13bitCountValue * 0.0004995 */
volt = (val * NCT720X_IN_SCALING_FACTOR) / NCT720X_IN_SCALING;
v1 = volt >> 5;
v2 = (volt & REG_VIN_LIMIT_LSB_MASK) << 3;
> > +
> > + if (chan->type != IIO_VOLTAGE)
> > + return -EOPNOTSUPP;
> > +
> > + if (info == IIO_EV_INFO_VALUE) {
> Flip logic.
> if (info != IIO_EV_INFO_VALUE)
> return -EINVAL;
>
> if (dir == IIO_EVE_DIR_FALLING) etc
>
> > + if (dir == IIO_EV_DIR_FALLING) {
> > + mutex_lock(&chip->access_lock);
> This is badly nested. Mutex lock and unlock should be in same scope.
> So I'd pull mutex_lock() out of this if stack and use guard(mutex) instead.
> That way you can just return on error.
>
We would put guard(mutex) in appropriate places.
> > + err = nct720x_write_reg(chip, REG_VIN_LOW_LIMIT[index], v1);
> > + if (err < 0) {
> > + pr_err("Failed to write REG_VIN%d_LOW_LIMIT\n", index + 1);
> > + goto abort;
> > + }
> > +
> > + err = nct720x_write_reg(chip, REG_VIN_LOW_LIMIT_LSB[index], v2);
> > + if (err < 0) {
> > + pr_err("Failed to write REG_VIN%d_LOW_LIMIT_LSB\n", index + 1);
> > + goto abort;
> > + }
> > + } else {
> > + mutex_lock(&chip->access_lock);
> > + err = nct720x_write_reg(chip, REG_VIN_HIGH_LIMIT[index], v1);
> > + if (err < 0) {
> > + pr_err("Failed to write REG_VIN%d_HIGH_LIMIT\n", index + 1);
> > + goto abort;
> > + }
> > +
> > + err = nct720x_write_reg(chip, REG_VIN_HIGH_LIMIT_LSB[index], v2);
> > + if (err < 0) {
> > + pr_err("Failed to write REG_VIN%d_HIGH_LIMIT_LSB\n", index + 1);
> > + goto abort;
> > + }
> > + }
> > + }
> > +abort:
> > + mutex_unlock(&chip->access_lock);
> > + return err;
> > +}
>
> > +
> > +static int nct720x_write_event_config(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir,
> > + int state)
> > +{
> > + int err = 0;
> > + struct nct720x_chip_info *chip = iio_priv(indio_dev);
> > + int index = nct720x_chan_to_index[chan->address];
> > + unsigned int mask;
> > +
> > + mask = BIT(index);
> > +
> > + if (chan->type != IIO_VOLTAGE)
> > + return -EOPNOTSUPP;
> > +
> > + if (!state && (chip->vin_mask & mask))
> > + chip->vin_mask &= ~mask;
> > + else if (state && !(chip->vin_mask & mask))
> > + chip->vin_mask |= mask;
> > +
> > + mutex_lock(&chip->access_lock);
> guard(mutex)(&chip->access_lock);
>
We would put guard(mutex) in appropriate places.
> > +
> > + err = nct720x_write_reg(chip, REG_CHANNEL_ENABLE_1, chip->vin_mask & 0xff);
> > + if (err < 0) {
> > + pr_err("Failed to write REG_CHANNEL_ENABLE_1\n");
> > + goto abort;
> dev_err()
> return err;
>
> > + }
> > +
> > + if (chip->type == nct7202) {
>
> Gain, base this on a chip_info flag.
>
We would pass a pointer to the data structure and not use chip->type anymore.
> > + err = nct720x_write_reg(chip, REG_CHANNEL_ENABLE_2, chip->vin_mask >> 8);
> > + if (err < 0) {
> > + pr_err("Failed to write REG_CHANNEL_ENABLE_2\n");
> > + goto abort;
> Same as above.
Use dev_err
> > + }
> > + }
> > +abort:
> > + mutex_unlock(&chip->access_lock);
> > + return err;
> > +}
> > +
> > +static int nct720x_detect(struct i2c_client *client,
> > + struct i2c_board_info *info)
> > +{
> > + struct i2c_adapter *adapter = client->adapter;
> > +
> > + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> > + I2C_FUNC_SMBUS_WORD_DATA))
> > + return -ENODEV;
> > +
> > + /* Determine the chip type. */
> > + if (i2c_smbus_read_byte_data(client, REG_VENDOR_ID) != NUVOTON_ID ||
> > + i2c_smbus_read_byte_data(client, REG_CHIP_ID) != NCT720X_ID ||
> > + i2c_smbus_read_byte_data(client, REG_DEVICE_ID) != NCT720X_DEVICE_ID)
> > + return -ENODEV;
> > +
> > + strscpy(info->type, "nct720x", I2C_NAME_SIZE);
> as below. It's unusual to find a detect in an IIO driver because the firmware
> normally tells us what is there.
We would pass a pointer to the data structure to obtain the name and
not use chip->type anymore.
> > +
> > + return 0;
> > +}
> > +
> > +static const struct iio_info nct720x_info = {
> > + .read_raw = &nct720x_read_raw,
> > + .read_event_config = &nct720x_read_event_config,
> > + .write_event_config = &nct720x_write_event_config,
> > + .read_event_value = &nct720x_read_event_value,
> > + .write_event_value = &nct720x_write_event_value,
> > +};
> > +
> > +static const struct i2c_device_id nct720x_id[];
> > +
> > +static int nct720x_init_chip(struct nct720x_chip_info *chip)
> > +{
> > + int value = 0;
> > + int err = 0;
> > +
> > + /* Initial reset */
> Maybe ignore datasheet naming and call that CONFIGURATION_INIT CONFIGURATION_RESET
> at which point he comment is unneeded.
>
Understand it.
> > + err = nct720x_write_reg(chip, REG_CONFIGURATION, CONFIGURATION_INIT);
> > + if (err) {
> > + pr_err("Failed to write REG_CONFIGURATION\n");
> > + return err;
> > + }
> > +
> > + /* Enable Channel */
> > + err = nct720x_write_reg(chip, REG_CHANNEL_ENABLE_1, 0xff);
> What does 0xFF represent? I guess all channels. If so maybe need
> to build with GENMASK so it is obvious this is enabling 8 channels.
>
Use GENMASK(7, 0) and declare it.
#define REG_CHANNEL_ENABLE_1_MASK GENMASK(7, 0)
> > + if (err) {
> > + pr_err("Failed to write REG_CHANNEL_ENABLE_1\n");
> > + return err;
> > + }
> > +
> > + if (chip->type == nct7202) {
>
> Make this 'data' in the chip_info structure. Probably a flat to say
> there is a REG_CHANNEL_ENABLE_2. That chip->type wants to go away infavour
> of chip->chip_info.X flags.
>
>
We would pass a pointer to the data structure to describe the
particular variant.
> > + err = nct720x_write_reg(chip, REG_CHANNEL_ENABLE_2, 0xf);
> > + if (err) {
> > + pr_err("Failed to write REG_CHANNEL_ENABLE_2\n");
> > + return err;
> > + }
> > + }
> > +
> > + value = nct720x_read_reg16(chip, REG_CHANNEL_ENABLE_1);
> > + if (value < 0)
> > + return value;
> > + chip->vin_mask = value;
> > +
> > + /* Start monitoring if needed */
> > + value = nct720x_read_reg(chip, REG_CONFIGURATION);
>
> Using regmap would let you simple do a single bit write in one call
> Definitely look at whether there is anything stopping you using that
> helpful infrastructure.
>
We take some effort to convert regmap API to instead of I2C API
> > + if (value < 0) {
> > + pr_err("Failed to read REG_CONFIGURATION\n");
> > + return value;
> > + }
> > +
> > + value |= CONFIGURATION_START;
> > + err = nct720x_write_reg(chip, REG_CONFIGURATION, value);
> > + if (err < 0) {
> > + pr_err("Failed to write REG_CONFIGURATION\n");
>
> dev_err() if not called only from probe, return dev_err_probe() if only called
> from probe()
>
>
Yes, use dev_err_probe() in probe function.
> > + return err;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int nct720x_probe(struct i2c_client *client)
> > +{
> > + const struct i2c_device_id *id = i2c_client_get_device_id(client);
> > + struct nct720x_chip_info *chip;
> > + struct iio_dev *indio_dev;
> > + int ret;
> > + u32 tmp;
> > +
> > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > + chip = iio_priv(indio_dev);
> > +
> > + if (client->dev.of_node)
> > + chip->type = (enum nct720x_chips)device_get_match_data(&client->dev);
> > + else
> > + chip->type = i2c_match_id(nct720x_id, client)->driver_data;
>
> i2c_get_match_data() but careful with zeros... It is much better to use that with
> an actual pointer.
>
>
We would pass a pointer to the data structure and i2c_get_match_data()
to get info
> > +
> > + chip->vin_max = (chip->type == nct7201) ? NCT7201_VIN_MAX : NCT7202_VIN_MAX;
> > +
> > + ret = of_property_read_u32(client->dev.of_node, "read-vin-data-size", &tmp);
> As in dt binding. If we keep this (I'm doubtful that it makes sense) define a default
> so that the property doesn't need to be provided. 16 would be the most obvious choice.
> Then just don't check the error value when reading the property. That is.
>
> tmp = 16;
> of_property_read_u32();
>
>
>
We already remove read-vin-data-size property and use word read for VIN data.
> > + if (ret < 0) {
> > + pr_err("read-vin-data-size property not found\n");
> > + return ret;
> > + }
> > +
> > + if (tmp == 8) {
> > + chip->use_read_byte_vin = true;
> > + } else if (tmp == 16) {
> > + chip->use_read_byte_vin = false;
> > + } else {
> > + pr_err("invalid read-vin-data-size (%d)\n", tmp);
> > + return -EINVAL;
> > + }
> > +
> > + mutex_init(&chip->access_lock);
> For new code prefer
> ret = devm_mutex_init()
> if (ret)
> return ret;
>
Understand it.
> It only helps in weird debug cases but also costs us very little to support those.
> > +
> > + /* this is only used for device removal purposes */
> > + i2c_set_clientdata(client, indio_dev);
> Won't be needed after the change below.
>
Understand it.
> > +
> > + chip->client = client;
> > +
> > + ret = nct720x_init_chip(chip);
> > + if (ret < 0)
> > + return ret;
> > +
> > + indio_dev->name = id->name;
> Put the name in the chip_info structure. id->name is a complex thing
> when other firmwares come into play, so better to just hard code the strings
> somewhere so we always know what we are getting.
>
We would pass a pointer to the data structure to obtain the name
> > + indio_dev->channels = nct720x_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(nct720x_channels);
> > + indio_dev->info = &nct720x_info;
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > + iio_device_register(indio_dev);
> return devm_iio_device_register();
>
> Then you can drop the remove callback.
>
Understand it.
> > +
> > + return 0;
> > +}
> > +
> > +static void nct720x_remove(struct i2c_client *client)
>
> Don't use wildcards even within the driver. Just name everything
> after one supported part.
> nct7202_remove() etc.
>
>
Remove the nct7202_remove()
> > +{
> > + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > +
> > + iio_device_unregister(indio_dev);
> > +}
> > +
> > +static const unsigned short nct720x_address_list[] = {
> > + 0x1d, 0x1e, 0x35, 0x36, I2C_CLIENT_END
> > +};
> > +
> > +static const struct i2c_device_id nct720x_id[] = {
> > + { "nct7201", nct7201 },
> > + { "nct7202", nct7202 },
> > + {}
> { }
>
> I'm trying to slowly standardise on this formatting in IIO
> so keen not to introduce more cases. It's an arbitrary choice
> but I went with the space.
>
Rewite the data structure as below,
static const struct nct720x_adc_model_data nct7201_model_data = {
.model_name = "nct7201",
.channels = nct7201_channels,
.num_channels = ARRAY_SIZE(nct7201_channels),
.vin_max = 8,
};
static const struct nct720x_adc_model_data nct7202_model_data = {
.model_name = "nct7202",
.channels = nct7202_channels,
.num_channels = ARRAY_SIZE(nct7202_channels),
.vin_max = 12,
};
static const struct i2c_device_id nct720x_id[] = {
{ "nct7201", (kernel_ulong_t)&nct7201_model_data },
{ "nct7202", (kernel_ulong_t)&nct7202_model_data },
{ }
};
> > +};
> > +MODULE_DEVICE_TABLE(i2c, nct720x_id);
> > +
> > +static const struct of_device_id nct720x_of_match[] = {
> > + {
> > + .compatible = "nuvoton,nct7201",
> > + .data = (void *)nct7201
> > + },
> > + {
> > + .compatible = "nuvoton,nct7202",
> > + .data = (void *)nct7202
> Use a pointer to a chip_info structure with all the chip specific
> stuff encoded as data. That is both more extensible and removes some
> ambiguities around whether zero is an error or not.
>
Rewrite as below,
static const struct of_device_id nct720x_of_match[] = {
{
.compatible = "nuvoton,nct7201",
.data = &nct7201_model_data,
},
{
.compatible = "nuvoton,nct7202",
.data = &nct7202_model_data,
},
{ }
};
> > + },
> > + { },
> No trailing comma
Understand it.
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, nct720x_of_match);
> > +
> > +static struct i2c_driver nct720x_driver = {
> > + .driver = {
> > + .name = "nct720x",
> > + .of_match_table = nct720x_of_match,
> > + },
> > + .probe = nct720x_probe,
> > + .remove = nct720x_remove,
> > + .id_table = nct720x_id,
> > + .detect = nct720x_detect,
>
> Do you need detect? That's kind of ancient infrastructure that I thought
> no one used any more (same for the address list).
>
We would remove detect and address list.
> > + .address_list = nct720x_address_list,
> > +};
> > +
> > +module_i2c_driver(nct720x_driver);
> > +
> > +MODULE_AUTHOR("Eason Yang <YHYANG2@nuvoton.com>");
> > +MODULE_DESCRIPTION("Nuvoton NCT720x voltage monitor driver");
> > +MODULE_LICENSE("GPL");
>
^ permalink raw reply [flat|nested] 26+ messages in thread