| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CA+4VgcKfDo2NSeBA6+z5AqCeEBds0-DwC0-e3H-bkJ7hEcHaWw@mail.gmail.com>
Date: Sun, 20 Apr 2025 21:03:05 +0800
From: Yu-Hsian Yang <j2anfernee@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: jic23@...nel.org, lars@...afoo.de, robh@...nel.org, krzk+dt@...nel.org,
conor+dt@...nel.org, dlechner@...libre.com, nuno.sa@...log.com,
javier.carrasco.cruz@...il.com, gstols@...libre.com, alisadariana@...il.com,
tgamblin@...libre.com, olivier.moysan@...s.st.com, antoniu.miclaus@...log.com,
eblanc@...libre.com, joao.goncalves@...adex.com, tobias.sperling@...ting.com,
marcelo.schmitt@...log.com, angelogioacchino.delregno@...labora.com,
thomas.bonnefille@...tlin.com, herve.codina@...tlin.com,
chanh@...amperecomputing.com, KWLIU@...oton.com, yhyang2@...oton.com,
linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 2/2] iio: adc: add support for Nuvoton NCT7201
Dear Andy,
Thanks for the review and the comments.
Will fix all, have a few questions (below)
Andy Shevchenko <andriy.shevchenko@...ux.intel.com> 於 2025年4月16日 週三 下午5:34寫道：
>
> On Wed, Apr 16, 2025 at 04:17:34PM +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 all the threshold values could be set
> > for system protection without any timing delay. It also supports reset
> > input RSTIN# to recover system from a fault condition.
> >
> > Currently, only single-edge mode conversion and threshold events are
> > supported.
>
> ...
>
> > +#define NCT7201_REG_VIN(i) (i)
>
> This doesn't do anything useful. Why do you need this rather useless macro?
>
Actually here we should define NCT7201_REG_VIN(i) as (0x00 + i),
We simply it as (i).
> ...
>
> > +struct nct7201_chip_info {
> > + struct device *dev;
>
> This can be derived from the respective regmap. No need to have it here.
>
> > + struct regmap *regmap;
> > + struct regmap *regmap16;
> > + int num_vin_channels;
> > + u16 vin_mask;
> > +};
>
Use regmap->dev is okay if use regmap API.
But if we need to print message not from regmap API,
how suggestions to do in this case?
The code is in nct7201_init_chip(struct nct7201_chip_info *chip)
...
> > + err = regmap_read(chip->regmap, NCT7201_REG_BUSY_STATUS, &value);
> > + if (err)
> > + return dev_err_probe(dev, err, "Failed to read busy status\n");
> > + if (!(value & NCT7201_BIT_PWR_UP))
> > + return dev_err_probe(dev, -EIO, "Failed to power up after reset\n");
> ...
>
> > +struct nct7201_adc_model_data {
> > + const char *model_name;
> > + const struct iio_chan_spec *channels;
>
> > + const int num_channels;
>
> What is the meaning of const here?
>
We don't need const int , just int.
> > + int num_vin_channels;
> > +};
>
> ...
>
> > +#define NCT7201_VOLTAGE_CHANNEL(num) \
> > + { \
> > + .type = IIO_VOLTAGE, \
> > + .indexed = 1, \
> > + .channel = (num + 1), \
>
> Parentheses are in a wrong place
>
We remove parentheses.
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> > + .address = num, \
> > + .event_spec = nct7201_events, \
> > + .num_event_specs = ARRAY_SIZE(nct7201_events), \
> > + }
>
> ...
>
> > +static int nct7201_write_event_config(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir,
> > + bool state)
> > +{
> > + struct nct7201_chip_info *chip = iio_priv(indio_dev);
> > + unsigned int mask;
> > + int err;
> > +
> > + if (chan->type != IIO_VOLTAGE)
> > + return -EOPNOTSUPP;
>
> > + mask = BIT(chan->address);
>
> Just join this with the definition above.
>
>
okay
> > + if (state)
> > + chip->vin_mask |= mask;
> > + else
> > + chip->vin_mask &= ~mask;
> > +
> > + if (chip->num_vin_channels <= 8)
> > + err = regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1,
>
> Remove _1.
>
> > + chip->vin_mask);
> > + else
> > + err = regmap_bulk_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1,
> > + &chip->vin_mask, sizeof(chip->vin_mask));
>
> > + if (err)
> > + return err;
> > +
> > + return 0;
>
> As simple as
>
> return err;
>
> > +}
>
> ...
>
> > +static int nct7201_init_chip(struct nct7201_chip_info *chip)
> > +{
> > + struct device *dev = chip->dev;
>
> Derive this from chip->regmap.
>
> > + __le16 data = cpu_to_le16(NCT7201_REG_CHANNEL_ENABLE_MASK);
> > + unsigned int value;
> > + int err;
> > +
> > + err = regmap_write(chip->regmap, NCT7201_REG_CONFIGURATION,
> > + NCT7201_BIT_CONFIGURATION_RESET);
> > + if (err)
> > + return dev_err_probe(dev, err, "Failed to reset chip\n");
> > +
> > + /*
> > + * After about 25 msecs, the device should be ready and then the Power
> > + * Up bit will be set to 1. If not, wait for it.
>
> "Up bit" is odd, please be more specific.
>
> > + */
> > + fsleep(25000);
>
> + Blank line.
>
> > + err = regmap_read(chip->regmap, NCT7201_REG_BUSY_STATUS, &value);
> > + if (err)
> > + return dev_err_probe(dev, err, "Failed to read busy status\n");
> > + if (!(value & NCT7201_BIT_PWR_UP))
> > + return dev_err_probe(dev, -EIO, "Failed to power up after reset\n");
> > +
> > + /* Enable Channel */
> > + if (chip->num_vin_channels <= 8)
> > + err = regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1,
> > + NCT7201_REG_CHANNEL_ENABLE_MASK);
> > + else
> > + err = regmap_bulk_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1,
> > + &data, sizeof(data));
> > + if (err)
> > + return dev_err_probe(dev, err, "Failed to enable channel\n");
> > +
> > + err = regmap_bulk_read(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1,
> > + &chip->vin_mask, sizeof(chip->vin_mask));
> > + if (err)
> > + return dev_err_probe(dev, err,
> > + "Failed to read channel enable register\n");
> > +
> > + /* Start monitoring if needed */
> > + err = regmap_set_bits(chip->regmap, NCT7201_REG_CONFIGURATION,
> > + NCT7201_BIT_CONFIGURATION_START);
> > + if (err)
> > + return dev_err_probe(dev, err, "Failed to start monitoring\n");
> > +
> > + return 0;
> > +}
>
> ...
>
> > + ret = nct7201_init_chip(chip);
> > + if (ret < 0)
>
> Why ' < 0' ?
>
> > + return ret;
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Powered by blists - more mailing lists