Re: [PATCH v4 1/2] iio: dac: ad5770r: Add AD5770R support

[Ardelean, Alexandru]

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.
> 
> > +};