Re: [PATCH 2/3] iio: adc: add driver support for AD5940

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 14 Nov 2019 22:05:54 +0800
Song Qiang <songqiang1304521@gmail.com> wrote:


> >> +static ssize_t ad5940_read_info(struct iio_dev *indio_dev,
> >> +				uintptr_t private,
> >> +				const struct iio_chan_spec *chan,
> >> +				char *buf)
> >> +{
> >> +	struct ad5940_state *st = iio_priv(indio_dev);
> >> +
> >> +	switch ((u32)private) {
> >> +	case AD5940_CHANNEL_NAME:  
> > 
> > What is the logic here in this magic define?
> >   
> 
> Do you mean this is not necessary? In this driver, 'ad5940_read_info' is
> only used in name reading, but I was thinking maybe there will be some
> other stuff to be reading from this in the future.

That might happen, but you may also extend this by providing more
callbacks.  Certainly don't put code in place to support things
that might be added.  Keep it simple.

> 
> >> +		return sprintf(buf, "%s\n",
> >> +			st->channel_config[chan->address].channel_name);
> >> +	default:
> >> +		return -EINVAL;
> >> +	}
> >> +}
> >> +

...

> >> +static int ad5940_of_parse_channel_config(struct iio_dev *indio_dev,
> >> +					  struct device_node *np)
> >> +{
> >> +	struct ad5940_state *st = iio_priv(indio_dev);
> >> +	struct iio_chan_spec *chan;
> >> +	struct device_node *child;
> >> +	u32 channel, ain[2];
> >> +	int ret;
> >> +
> >> +	st->num_channels = of_get_available_child_count(np);
> >> +	if (!st->num_channels) {
> >> +		dev_err(indio_dev->dev.parent, "no channel children\n");
> >> +		return -ENODEV;
> >> +	}
> >> +
> >> +	chan = devm_kcalloc(indio_dev->dev.parent, st->num_channels,
> >> +			    sizeof(*chan), GFP_KERNEL);
> >> +	if (!chan)
> >> +		return -ENOMEM;
> >> +
> >> +	st->channel_config = devm_kcalloc(indio_dev->dev.parent,
> >> +					  st->num_channels,
> >> +					  sizeof(*st->channel_config),
> >> +					  GFP_KERNEL);
> >> +	if (!st->channel_config)
> >> +		return -ENOMEM;
> >> +
> >> +	indio_dev->channels = chan;
> >> +	indio_dev->num_channels = st->num_channels;
> >> +
> >> +	for_each_available_child_of_node(np, child) {
> >> +		ret = of_property_read_u32(child, "reg", &channel);
> >> +		if (ret)
> >> +			goto err;
> >> +
> >> +		ret = of_property_read_u32_array(child, "diff-channels",
> >> +						 ain, 2);
> >> +		if (ret)
> >> +			goto err;
> >> +
> >> +		ret = of_property_read_string(child, "channel-name",
> >> +				&st->channel_config[channel].channel_name);
> >> +		if (ret)
> >> +			st->channel_config[channel].channel_name = "none-name";  
> > 
> > You have this as required I think in the dt properties.  If that is the case then
> > enforce it and refuse to load the driver if not supplied. Otherwise change
> > the dt docs to make it optional (which is probably better)
> >   
> 
> I prefer to have name required because a channel here is a combination
> of some input and some output. Without name, we will have to look into
> dt to see which input and which output is used in this channel.

Then don't make it optional.

> 
> >> +
> >> +		ret = ad5940_check_channel_indexes(indio_dev->dev.parent, ain);
> >> +		if (ret) {
> >> +			dev_err(indio_dev->dev.parent,
> >> +				"some input channel index does not exist: %d, %d, %d",
> >> +				channel, ain[0], ain[1]);
> >> +			goto err;
> >> +		}
> >> +
> >> +		st->channel_config[channel].ain = AD5940_CHANNEL_AINP(ain[0]) |
> >> +						  AD5940_CHANNEL_AINN(ain[1]);
> >> +
> >> +		*chan = ad5940_channel_template;
> >> +		chan->address = channel;
> >> +		chan->scan_index = channel;
> >> +		chan->channel = ain[0];
> >> +		chan->channel2 = ain[1];
> >> +
> >> +		chan++;
> >> +	}
> >> +
> >> +	return 0;
> >> +err:
> >> +	of_node_put(child);
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +static int ad5940_config_polarity(struct ad5940_state *st, u32 polarity)
> >> +{
> >> +	u32 val;
> >> +
> >> +	if (polarity == IRQF_TRIGGER_RISING)
> >> +		val = AD5940_INTCPOL_POS;
> >> +	else
> >> +		val = AD5940_INTCPOL_NEG;
> >> +
> >> +	return ad5940_write_reg_mask(st, AD5940_REG_INTCPOL,
> >> +				     AD5940_INTCPOL_MSK, val);
> >> +}
> >> +
> >> +static int ad5940_config_int_io(struct ad5940_state *st, u8 int_io)
> >> +{
> >> +	int ret = 0;
> >> +
> >> +	if (int_io == 3)  
> > 
> > Switch statement preferred for matches like this.
> >   
> >> +		ret = ad5940_write_reg_mask(st, AD5940_REG_GP0CON,
> >> +					    AD5940_GP0CON_3_MSK,
> >> +					    AD5940_GP0CON_3_INT);
> >> +	else if (int_io == 6)
> >> +		ret = ad5940_write_reg_mask(st, AD5940_REG_GP0CON,
> >> +					    AD5940_GP0CON_6_MSK,
> >> +					    AD5940_GP0CON_6_INT);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +
> >> +	return  ad5940_write_reg(st, AD5940_REG_GP0OEN, BIT(int_io));
> >> +}
> >> +
> >> +static const u32 ad5940_powerup_setting[][2] = {  
> > 
> > Hmm. This is not good practice when we have docs for the values.
> > If we can provide a breakdown into what is being set that would be
> > great.  I can't find docs immediately for some of these however...
> >   
> 
> We have docs for some of them, but not for all. I got some of the init
> register values from Analog's example code, which didn't explain much.
> I'll add comment to these values as much as I can.

Great.

Jonathan

> 
> yours,
> Song Qiang
> 
> >> +	{ 0x0908, 0x02c9 },
> >> +	{ 0x0c08, 0x206c },
> >> +	{ 0x21f0, 0x0010 },  
> > 
> > This one is is simply saying only 1 repeat conversion for example.
> > Add some defines and you can make that clear.
> >   
> >> +	{ 0x0410, 0x02c9 },
> >> +	{ 0x0a28, 0x0009 },
> >> +	{ 0x238c, 0x0104 },
> >> +	{ 0x0a04, 0x4859 },
> >> +	{ 0x0a04, 0xf27b },
> >> +	{ 0x0a00, 0x8009 },
> >> +	{ 0x22f0, 0x0000 },
> >> +	{ 0x2230, 0xde87a5af },
> >> +	{ 0x2250, 0x103f },
> >> +	{ 0x22b0, 0x203c },
> >> +	{ 0x2230, 0xde87a5a0 },
> >> +};
> >> +  
> 
> ...