| 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
| ||
|
Date: Sun, 22 Mar 2020 09:17:06 +0000
From: "Ardelean, Alexandru" <alexandru.Ardelean@...log.com>
To: "andriy.shevchenko@...el.com" <andriy.shevchenko@...el.com>,
"Tachici, Alexandru" <Alexandru.Tachici@...log.com>
CC: "jic23@...nel.org" <jic23@...nel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
"Caprioru, Mircea" <Mircea.Caprioru@...log.com>
Subject: Re: [PATCH v4 1/2] iio: dac: ad5770r: Add AD5770R support
On Sat, 2020-03-21 at 23:09 +0200, Andy Shevchenko wrote:
> [External]
>
> On Tue, Feb 18, 2020 at 02:10:30PM +0200, Alexandru Tachici wrote:
> > The AD5770R is a 6-channel, 14-bit resolution, low noise, programmable
> > current output digital-to-analog converter (DAC) for photonics control
> > applications.
> >
> > It contains five 14-bit resolution current sourcing DAC channels and one
> > 14-bit resolution current sourcing/sinking DAC channel.
>
This patch made it into linux-next.
Doing a v2 now may be a bit noisy.
Maybe we can tidy-this-up later.
To be honest, it won't be the highest on our list.
[ I know how that sounds ]
We can probably leave it for other people to clean it up.
[ People that want to start contributing to the kernel ]
> ...
>
> > +#define AD5770R_CFG_CH0_SINK_EN(x) (((x) & 0x1) << 7)
> > +#define AD5770R_CFG_SHUTDOWN_B(x, ch) (((x) & 0x1) << (ch))
>
> BIT(0) ?
>
> > +#define AD5770R_REF_RESISTOR_SEL(x) (((x) & 0x1) << 2)
>
> Ditto.
>
> > +#define AD5770R_CH_SET(x, ch) (((x) & 0x1) << (ch))
>
> Ditto.
>
> ...
>
> > +enum ad5770r_ch0_modes {
> > + AD5770R_CH0_0_300 = 0,
> > + AD5770R_CH0_NEG_60_0,
> > + AD5770R_CH0_NEG_60_300
> > +};
> > +
> > +enum ad5770r_ch1_modes {
> > + AD5770R_CH1_0_140_LOW_HEAD = 1,
> > + AD5770R_CH1_0_140_LOW_NOISE,
> > + AD5770R_CH1_0_250
> > +};
> > +
> > +enum ad5770r_ch2_5_modes {
> > + AD5770R_CH_LOW_RANGE = 0,
> > + AD5770R_CH_HIGH_RANGE
> > +};
> > +
> > +enum ad5770r_ref_v {
> > + AD5770R_EXT_2_5_V = 0,
> > + AD5770R_INT_1_25_V_OUT_ON,
> > + AD5770R_EXT_1_25_V,
> > + AD5770R_INT_1_25_V_OUT_OFF
> > +};
> > +
> > +enum ad5770r_output_filter_resistor {
> > + AD5770R_FILTER_60_OHM = 0x0,
> > + AD5770R_FILTER_5_6_KOHM = 0x5,
> > + AD5770R_FILTER_11_2_KOHM,
> > + AD5770R_FILTER_22_2_KOHM,
> > + AD5770R_FILTER_44_4_KOHM,
> > + AD5770R_FILTER_104_KOHM,
>
> It would be nice to go consistent about last entry in enums. Are they
> terminators? For me it doesn't look like it (new revision of hardware
> always can use extended lists of modes, etc).
>
> So, I think leaving comma for all above is a right thing to do.
>
> > +};
>
> ...
>
> > +static const unsigned int ad5770r_filter_reg_vals[] = {
> > + AD5770R_FILTER_104_KOHM,
> > + AD5770R_FILTER_44_4_KOHM,
> > + AD5770R_FILTER_22_2_KOHM,
> > + AD5770R_FILTER_11_2_KOHM,
> > + AD5770R_FILTER_5_6_KOHM,
> > + AD5770R_FILTER_60_OHM
> > +};
>
> Ditto.
>
> ...
>
> > + return regmap_write(st->regmap,
> > + AD5770R_OUTPUT_RANGE(channel), regval);
>
> It fits one line (even has room for one character more)
> Perhaps you may consider to amend settings of your text editor?
> Please fix all similar places in all of your patches.
>
> ...
>
> > +static int ad5770r_reset(struct ad5770r_state *st)
> > +{
> > + /* Perform software reset if no GPIO provided */
> > + if (!st->gpio_reset)
> > + return ad5770r_soft_reset(st);
>
> Perhaps split this to _reset_gpio() and do
>
> if (gpio)
> return _reset_gpio();
> else // It's redundant, but some people use for a style
> return _soft_reset();
>
> > +
> > + gpiod_set_value_cansleep(st->gpio_reset, 0);
> > + usleep_range(10, 20);
> > + gpiod_set_value_cansleep(st->gpio_reset, 1);
> > +
> > + /* data must not be written during reset timeframe */
> > + usleep_range(100, 200);
> > +
> > + return 0;
> > +}
>
> ...
>
> > + ret = regmap_bulk_read(st->regmap,
> > + chan->address,
> > + st->transf_buf, 2);
>
> sizeof() ?
>
> > + if (ret)
> > + return 0;
>
> ...
>
> > + st->transf_buf[0] = ((u16)val >> 6);
>
> Why explicit casting?
>
> > + st->transf_buf[1] = (val & GENMASK(5, 0)) << 2;
> > + return regmap_bulk_write(st->regmap, chan->address,
> > + st->transf_buf, 2);
>
> sizeof() ?
>
> ...
>
> > +static int ad5770r_read_freq_avail(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + const int **vals, int *type, int *length,
> > + long mask)
> > +{
> > + switch (mask) {
> > + case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> > + *type = IIO_VAL_INT;
> > + *vals = ad5770r_filter_freqs;
> > + *length = ARRAY_SIZE(ad5770r_filter_freqs);
> > + return IIO_AVAIL_LIST;
> > + }
> > +
> > + return -EINVAL;
>
> default case?
>
> > +}
>
> ...
>
> > + ret = kstrtobool(buf, &readin);
> > + if (ret)
> > + return ret;
> > +
> > + readin = !readin;
>
> I think using ! directly in places where it's needed will be helpful ti
> understand the code.
>
> > +
> > + regval = AD5770R_CFG_SHUTDOWN_B(readin, chan->channel);
> > + if (chan->channel == 0 &&
> > + st->output_mode[0].out_range_mode > AD5770R_CH0_0_300) {
> > + regval |= AD5770R_CFG_CH0_SINK_EN(readin);
> > + mask = BIT(chan->channel) + BIT(7);
> > + } else {
> > + mask = BIT(chan->channel);
> > + }
> > + ret = regmap_update_bits(st->regmap, AD5770R_CHANNEL_CONFIG, mask,
> > + regval);
> > + if (ret)
> > + return ret;
> > +
> > + regval = AD5770R_CH_SET(readin, chan->channel);
> > + ret = regmap_update_bits(st->regmap, AD5770R_CH_ENABLE,
> > + BIT(chan->channel), regval);
> > + if (ret)
> > + return ret;
> > +
> > + st->ch_pwr_down[chan->channel] = !readin;
>
> ...
>
> > + int ret, tmp[2], min, max;
> > + unsigned int num;
> > + struct fwnode_handle *child;
> > +
> > + num = device_get_child_node_count(&st->spi->dev);
> > + if (num != AD5770R_MAX_CHANNELS)
> > + return -EINVAL;
> > +
> > + device_for_each_child_node(&st->spi->dev, child) {
> > + ret = fwnode_property_read_u32(child, "num", &num);
> > + if (ret)
> > + return ret;
> > + if (num > AD5770R_MAX_CHANNELS)
> > + return -EINVAL;
> > +
> > + ret = fwnode_property_read_u32_array(child,
> > + "adi,range-microamp",
> > + tmp, 2);
>
> sizeof()?
>
> And perhaps
> u32 tmp[2];
>
> > + if (ret)
> > + return ret;
> > +
> > + min = tmp[0] / 1000;
> > + max = tmp[1] / 1000;
> > + ret = ad5770r_store_output_range(st, min, max, num);
> > + if (ret)
> > + return ret;
> > + }
> > + return ret;
>
> ret might be uninitialized?
>
> In any case, shouldn't be
> return 0;
> ?
>
> > +}
>
> ...
>
> > + st->external_res = fwnode_property_read_bool(st->spi->dev.fwnode,
> > + "adi,external-resistor");
>
> Use of fwnode (and esp. w/o dev_fwnode() helper) looks inconsistent here.
> Why not device_property_...()?
>
> > + return ret;
>
> return 0;
>
> ...
>
> > +static const struct of_device_id ad5770r_of_id[] = {
> > + { .compatible = "adi,ad5770r", },
> > + {},
>
> No need comma in terminator line.
>
> > +};
> > +MODULE_DEVICE_TABLE(of, ad5770r_of_id);
> > +
> > +static const struct spi_device_id ad5770r_id[] = {
> > + { "ad5770r", 0 },
> > + {},
>
> Ditto.
>
> > +};
Powered by blists - more mailing lists