[PATCH 0/2] iio: adc: ti-ads1015: fix channel crosstalk

[PATCH 0/2] iio: adc: ti-ads1015: fix channel crosstalk

[Ladislav Michl]

Hi,

first patch of this serie fixes reading diffrent channel value than device
is currently processing, while second one is purely optional, making
drivers generate attributes from values it is actually working with.

Ladislav Michl (2):
  iio: adc: ti-ads1015: fix conversion time
  iio: adc: ti-ads1015: use dynamically generated attributes

 drivers/iio/adc/ti-ads1015.c | 107 ++++++++++++++++++++++++-------------------
 1 file changed, 61 insertions(+), 46 deletions(-)

[PATCH 1/2] iio: adc: ti-ads1015: fix conversion time

[Ladislav Michl]

When reading diffrent channel value than device is currently processing
wait time of conversion period is applied, which is not enough as device
might be already in the middle of conversion and therefore previously
converted value is returned - the one belonging to another channel.
Fix it by waiting for two sampling periods.

Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
---
 drivers/iio/adc/ti-ads1015.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
index 884b8e461b17..86fd2753869d 100644
--- a/drivers/iio/adc/ti-ads1015.c
+++ b/drivers/iio/adc/ti-ads1015.c
@@ -260,7 +260,7 @@ int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
 		return ret;
 
 	if (change) {
-		conv_time = DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr]);
+		conv_time = 2 * DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr]);
 		usleep_range(conv_time, conv_time + 1);
 	}
 
-- 
2.11.0

[PATCH 2/2] iio: adc: ti-ads1015: use dynamically generated attributes

[Ladislav Michl]

Generate attributes dynamically based on actual values used
by driver - leads to slightly smaller code (on ARM and X64)
Scale and sampling frequency definitions are at one place now.

Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
---
 drivers/iio/adc/ti-ads1015.c | 105 ++++++++++++++++++++++++-------------------
 1 file changed, 60 insertions(+), 45 deletions(-)

diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
index 86fd2753869d..2795553d5340 100644
--- a/drivers/iio/adc/ti-ads1015.c
+++ b/drivers/iio/adc/ti-ads1015.c
@@ -73,28 +73,30 @@ enum ads1015_channels {
 	ADS1015_TIMESTAMP,
 };
 
-static const unsigned int ads1015_data_rate[] = {
+static const unsigned short ads1015_data_rate[] = {
 	128, 250, 490, 920, 1600, 2400, 3300, 3300
 };
 
-static const unsigned int ads1115_data_rate[] = {
+static const unsigned short ads1115_data_rate[] = {
 	8, 16, 32, 64, 128, 250, 475, 860
 };
 
 static const struct {
-	int scale;
-	int uscale;
+	short scale;
+	short mscale;
 } ads1015_scale[] = {
-	{3, 0},
-	{2, 0},
-	{1, 0},
-	{0, 500000},
-	{0, 250000},
-	{0, 125000},
-	{0, 125000},
-	{0, 125000},
+	{ 3, 0 },
+	{ 2, 0 },
+	{ 1, 0 },
+	{ 0, 500 },
+	{ 0, 250 },
+	{ 0, 125 },
+	{ 0, 125 },
+	{ 0, 125 },
 };
 
+#define ADS1015_SCALES_SIZE	(ARRAY_SIZE(ads1015_scale) - 2)
+
 #define ADS1015_V_CHAN(_chan, _addr) {				\
 	.type = IIO_VOLTAGE,					\
 	.indexed = 1,						\
@@ -182,7 +184,8 @@ struct ads1015_data {
 	struct mutex lock;
 	struct ads1015_channel_data channel_data[ADS1015_CHANNELS];
 
-	unsigned int *data_rate;
+	unsigned short *data_rate;
+	unsigned char data_rate_size;
 };
 
 static bool ads1015_is_writeable_reg(struct device *dev, unsigned int reg)
@@ -303,9 +306,9 @@ static int ads1015_set_scale(struct ads1015_data *data, int chan,
 {
 	int i, ret, rindex = -1;
 
-	for (i = 0; i < ARRAY_SIZE(ads1015_scale); i++)
+	for (i = 0; i < ADS1015_SCALES_SIZE; i++)
 		if (ads1015_scale[i].scale == scale &&
-		    ads1015_scale[i].uscale == uscale) {
+		    ads1015_scale[i].mscale * 1000 == uscale) {
 			rindex = i;
 			break;
 		}
@@ -327,7 +330,7 @@ static int ads1015_set_data_rate(struct ads1015_data *data, int chan, int rate)
 {
 	int i, ret, rindex = -1;
 
-	for (i = 0; i < ARRAY_SIZE(ads1015_data_rate); i++)
+	for (i = 0; i < data->data_rate_size; i++)
 		if (data->data_rate[i] == rate) {
 			rindex = i;
 			break;
@@ -386,7 +389,7 @@ static int ads1015_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_SCALE:
 		idx = data->channel_data[chan->address].pga;
 		*val = ads1015_scale[idx].scale;
-		*val2 = ads1015_scale[idx].uscale;
+		*val2 = ads1015_scale[idx].mscale * 1000;
 		ret = IIO_VAL_INT_PLUS_MICRO;
 		break;
 	case IIO_CHAN_INFO_SAMP_FREQ:
@@ -446,16 +449,44 @@ static const struct iio_buffer_setup_ops ads1015_buffer_setup_ops = {
 	.validate_scan_mask = &iio_validate_scan_mask_onehot,
 };
 
-static IIO_CONST_ATTR(scale_available, "3 2 1 0.5 0.25 0.125");
+static ssize_t ads1015_show_scales(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	int i;
+	ssize_t l = 0;
+
+	for (i = 0; i < ADS1015_SCALES_SIZE; i++)
+		l += sprintf(buf + l, "%u.%03u%c",
+			ads1015_scale[i].scale, ads1015_scale[i].mscale,
+			i == ADS1015_SCALES_SIZE - 1 ? '\n' : ' ');
+
+	return l;
+}
+
+static ssize_t ads1015_show_sample_rates(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	int i;
+	ssize_t l = 0;
+	struct ads1015_data *data = iio_priv(dev_to_iio_dev(dev));
+
+	for (i = 0; i < data->data_rate_size; i++)
+		l += sprintf(buf + l, "%u%c", data->data_rate[i],
+			i == data->data_rate_size - 1 ? '\n' : ' ');
+
+	return l;
+}
+
+static IIO_DEVICE_ATTR(scale_available, S_IRUGO,
+		       ads1015_show_scales, NULL, 0);
+static IIO_DEVICE_ATTR(sampling_frequency_available, S_IRUGO,
+		       ads1015_show_sample_rates, NULL, 0);
 
-static IIO_CONST_ATTR_NAMED(ads1015_sampling_frequency_available,
-	sampling_frequency_available, "128 250 490 920 1600 2400 3300");
-static IIO_CONST_ATTR_NAMED(ads1115_sampling_frequency_available,
-	sampling_frequency_available, "8 16 32 64 128 250 475 860");
 
 static struct attribute *ads1015_attributes[] = {
-	&iio_const_attr_scale_available.dev_attr.attr,
-	&iio_const_attr_ads1015_sampling_frequency_available.dev_attr.attr,
+	&iio_dev_attr_scale_available.dev_attr.attr,
+	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
 	NULL,
 };
 
@@ -463,16 +494,6 @@ static const struct attribute_group ads1015_attribute_group = {
 	.attrs = ads1015_attributes,
 };
 
-static struct attribute *ads1115_attributes[] = {
-	&iio_const_attr_scale_available.dev_attr.attr,
-	&iio_const_attr_ads1115_sampling_frequency_available.dev_attr.attr,
-	NULL,
-};
-
-static const struct attribute_group ads1115_attribute_group = {
-	.attrs = ads1115_attributes,
-};
-
 static const struct iio_info ads1015_info = {
 	.driver_module	= THIS_MODULE,
 	.read_raw	= ads1015_read_raw,
@@ -480,13 +501,6 @@ static const struct iio_info ads1015_info = {
 	.attrs          = &ads1015_attribute_group,
 };
 
-static const struct iio_info ads1115_info = {
-	.driver_module	= THIS_MODULE,
-	.read_raw	= ads1015_read_raw,
-	.write_raw	= ads1015_write_raw,
-	.attrs          = &ads1115_attribute_group,
-};
-
 #ifdef CONFIG_OF
 static int ads1015_get_channels_config_of(struct i2c_client *client)
 {
@@ -594,6 +608,7 @@ static int ads1015_probe(struct i2c_client *client,
 	indio_dev->dev.of_node = client->dev.of_node;
 	indio_dev->name = ADS1015_DRV_NAME;
 	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &ads1015_info;
 
 	if (client->dev.of_node)
 		chip = (enum chip_ids)of_device_get_match_data(&client->dev);
@@ -603,14 +618,14 @@ static int ads1015_probe(struct i2c_client *client,
 	case ADS1015:
 		indio_dev->channels = ads1015_channels;
 		indio_dev->num_channels = ARRAY_SIZE(ads1015_channels);
-		indio_dev->info = &ads1015_info;
-		data->data_rate = (unsigned int *) &ads1015_data_rate;
+		data->data_rate = (unsigned short *) &ads1015_data_rate;
+		data->data_rate_size = ARRAY_SIZE(ads1015_data_rate) - 1;
 		break;
 	case ADS1115:
 		indio_dev->channels = ads1115_channels;
 		indio_dev->num_channels = ARRAY_SIZE(ads1115_channels);
-		indio_dev->info = &ads1115_info;
-		data->data_rate = (unsigned int *) &ads1115_data_rate;
+		data->data_rate = (unsigned short *) &ads1115_data_rate;
+		data->data_rate_size = ARRAY_SIZE(ads1115_data_rate);
 		break;
 	}
 
-- 
2.11.0

Re: [PATCH 1/2] iio: adc: ti-ads1015: fix conversion time

[Daniel Baluta]

On Tue, Aug 1, 2017 at 11:26 AM, Ladislav Michl <ladis@linux-mips.org> wrote:
> When reading diffrent channel value than device is currently processing
> wait time of conversion period is applied, which is not enough as device
> might be already in the middle of conversion and therefore previously
> converted value is returned - the one belonging to another channel.
> Fix it by waiting for two sampling periods.
>
> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>

Did you actually experienced this in practice? I guess you are right,
if you change the channel during a conversion, this change will be applied
to the next conversion *but* reading will still return old value because the
current conversion hasn't finished yet.

So,

Acked-by: Daniel Baluta <daniel.baluta@gmail.com>


> ---
>  drivers/iio/adc/ti-ads1015.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> index 884b8e461b17..86fd2753869d 100644
> --- a/drivers/iio/adc/ti-ads1015.c
> +++ b/drivers/iio/adc/ti-ads1015.c
> @@ -260,7 +260,7 @@ int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
>                 return ret;
>
>         if (change) {
> -               conv_time = DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr]);
> +               conv_time = 2 * DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr]);
>                 usleep_range(conv_time, conv_time + 1);
>         }
>
> --
> 2.11.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] iio: adc: ti-ads1015: fix conversion time

[Ladislav Michl]

On Tue, Aug 01, 2017 at 12:09:34PM +0300, Daniel Baluta wrote:
> On Tue, Aug 1, 2017 at 11:26 AM, Ladislav Michl <ladis@linux-mips.org> wrote:
> > When reading diffrent channel value than device is currently processing
> > wait time of conversion period is applied, which is not enough as device
> > might be already in the middle of conversion and therefore previously
> > converted value is returned - the one belonging to another channel.
> > Fix it by waiting for two sampling periods.
> >
> > Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> 
> Did you actually experienced this in practice? I guess you are right,
> if you change the channel during a conversion, this change will be applied
> to the next conversion *but* reading will still return old value because the
> current conversion hasn't finished yet.

Yes, that's what actually happens without patch and could be easily
demonstrated from command line:

$ cat in_voltage0_raw
847
$ cat in_voltage1_raw
846
$ cat in_voltage1_raw
195
$ cat in_voltage0_raw
196
$ cat in_voltage0_raw
846

Btw, driver is using continuous conversion mode, but I was unable to
make iio_generic_buffer work with it. And for reading values one by
one single shot mode is enough. Any hints?

Thank you,
	ladis

> So,
> 
> Acked-by: Daniel Baluta <daniel.baluta@gmail.com>

Re: [PATCH 1/2] iio: adc: ti-ads1015: fix conversion time

[Daniel Baluta]

On Tue, Aug 1, 2017 at 12:59 PM, Ladislav Michl <ladis@linux-mips.org> wrote:
> On Tue, Aug 01, 2017 at 12:09:34PM +0300, Daniel Baluta wrote:
>> On Tue, Aug 1, 2017 at 11:26 AM, Ladislav Michl <ladis@linux-mips.org> wrote:
>> > When reading diffrent channel value than device is currently processing
>> > wait time of conversion period is applied, which is not enough as device
>> > might be already in the middle of conversion and therefore previously
>> > converted value is returned - the one belonging to another channel.
>> > Fix it by waiting for two sampling periods.
>> >
>> > Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
>>
>> Did you actually experienced this in practice? I guess you are right,
>> if you change the channel during a conversion, this change will be applied
>> to the next conversion *but* reading will still return old value because the
>> current conversion hasn't finished yet.
>
> Yes, that's what actually happens without patch and could be easily
> demonstrated from command line:
>
> $ cat in_voltage0_raw
> 847
> $ cat in_voltage1_raw
> 846
> $ cat in_voltage1_raw
> 195
> $ cat in_voltage0_raw
> 196
> $ cat in_voltage0_raw
> 846
>
> Btw, driver is using continuous conversion mode, but I was unable to
> make iio_generic_buffer work with it. And for reading values one by
> one single shot mode is enough. Any hints?

Haven't looked at this sensor for long time. Can you tell us what have you
tried and what doesn't work?

Daniel.

Re: [PATCH 2/2] iio: adc: ti-ads1015: use dynamically generated attributes

[Jonathan Cameron]

On Tue, 1 Aug 2017 10:26:46 +0200
Ladislav Michl <ladis@linux-mips.org> wrote:

> Generate attributes dynamically based on actual values used
> by driver - leads to slightly smaller code (on ARM and X64)
> Scale and sampling frequency definitions are at one place now.
> 
> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
Sorry, for the small gain I am unconvinced.  If this was a new
driver and it had been submitted this way I wouldn't be asking
for it to be changed, but I'm also not interested in the churn and
additional code for what must be a pretty small chance.

Also if you really want to do tinification patches, please post
the actual sizes of the various regions.  In particular you need
to generally have a fairly large saving to justify replacing
stuff that is constant with code.

Jonathan
> ---
>  drivers/iio/adc/ti-ads1015.c | 105 ++++++++++++++++++++++++-------------------
>  1 file changed, 60 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> index 86fd2753869d..2795553d5340 100644
> --- a/drivers/iio/adc/ti-ads1015.c
> +++ b/drivers/iio/adc/ti-ads1015.c
> @@ -73,28 +73,30 @@ enum ads1015_channels {
>  	ADS1015_TIMESTAMP,
>  };
>  
> -static const unsigned int ads1015_data_rate[] = {
> +static const unsigned short ads1015_data_rate[] = {
>  	128, 250, 490, 920, 1600, 2400, 3300, 3300
>  };
>  
> -static const unsigned int ads1115_data_rate[] = {
> +static const unsigned short ads1115_data_rate[] = {
>  	8, 16, 32, 64, 128, 250, 475, 860
>  };
>  
>  static const struct {
> -	int scale;
> -	int uscale;
> +	short scale;
> +	short mscale;
>  } ads1015_scale[] = {
> -	{3, 0},
> -	{2, 0},
> -	{1, 0},
> -	{0, 500000},
> -	{0, 250000},
> -	{0, 125000},
> -	{0, 125000},
> -	{0, 125000},
> +	{ 3, 0 },
> +	{ 2, 0 },
> +	{ 1, 0 },
> +	{ 0, 500 },
> +	{ 0, 250 },
> +	{ 0, 125 },
> +	{ 0, 125 },
> +	{ 0, 125 },
>  };
>  
> +#define ADS1015_SCALES_SIZE	(ARRAY_SIZE(ads1015_scale) - 2)
> +
>  #define ADS1015_V_CHAN(_chan, _addr) {				\
>  	.type = IIO_VOLTAGE,					\
>  	.indexed = 1,						\
> @@ -182,7 +184,8 @@ struct ads1015_data {
>  	struct mutex lock;
>  	struct ads1015_channel_data channel_data[ADS1015_CHANNELS];
>  
> -	unsigned int *data_rate;
> +	unsigned short *data_rate;
> +	unsigned char data_rate_size;
>  };
>  
>  static bool ads1015_is_writeable_reg(struct device *dev, unsigned int reg)
> @@ -303,9 +306,9 @@ static int ads1015_set_scale(struct ads1015_data *data, int chan,
>  {
>  	int i, ret, rindex = -1;
>  
> -	for (i = 0; i < ARRAY_SIZE(ads1015_scale); i++)
> +	for (i = 0; i < ADS1015_SCALES_SIZE; i++)
>  		if (ads1015_scale[i].scale == scale &&
> -		    ads1015_scale[i].uscale == uscale) {
> +		    ads1015_scale[i].mscale * 1000 == uscale) {
>  			rindex = i;
>  			break;
>  		}
> @@ -327,7 +330,7 @@ static int ads1015_set_data_rate(struct ads1015_data *data, int chan, int rate)
>  {
>  	int i, ret, rindex = -1;
>  
> -	for (i = 0; i < ARRAY_SIZE(ads1015_data_rate); i++)
> +	for (i = 0; i < data->data_rate_size; i++)
>  		if (data->data_rate[i] == rate) {
>  			rindex = i;
>  			break;
> @@ -386,7 +389,7 @@ static int ads1015_read_raw(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_SCALE:
>  		idx = data->channel_data[chan->address].pga;
>  		*val = ads1015_scale[idx].scale;
> -		*val2 = ads1015_scale[idx].uscale;
> +		*val2 = ads1015_scale[idx].mscale * 1000;
>  		ret = IIO_VAL_INT_PLUS_MICRO;
>  		break;
>  	case IIO_CHAN_INFO_SAMP_FREQ:
> @@ -446,16 +449,44 @@ static const struct iio_buffer_setup_ops ads1015_buffer_setup_ops = {
>  	.validate_scan_mask = &iio_validate_scan_mask_onehot,
>  };
>  
> -static IIO_CONST_ATTR(scale_available, "3 2 1 0.5 0.25 0.125");
> +static ssize_t ads1015_show_scales(struct device *dev,
> +				   struct device_attribute *attr, char *buf)
> +{
> +	int i;
> +	ssize_t l = 0;
> +
> +	for (i = 0; i < ADS1015_SCALES_SIZE; i++)
> +		l += sprintf(buf + l, "%u.%03u%c",
> +			ads1015_scale[i].scale, ads1015_scale[i].mscale,
> +			i == ADS1015_SCALES_SIZE - 1 ? '\n' : ' ');
> +
> +	return l;
> +}
> +
> +static ssize_t ads1015_show_sample_rates(struct device *dev,
> +					 struct device_attribute *attr,
> +					 char *buf)
> +{
> +	int i;
> +	ssize_t l = 0;
> +	struct ads1015_data *data = iio_priv(dev_to_iio_dev(dev));
> +
> +	for (i = 0; i < data->data_rate_size; i++)
> +		l += sprintf(buf + l, "%u%c", data->data_rate[i],
> +			i == data->data_rate_size - 1 ? '\n' : ' ');
> +
> +	return l;
> +}
> +
> +static IIO_DEVICE_ATTR(scale_available, S_IRUGO,
> +		       ads1015_show_scales, NULL, 0);
> +static IIO_DEVICE_ATTR(sampling_frequency_available, S_IRUGO,
> +		       ads1015_show_sample_rates, NULL, 0);
>  
> -static IIO_CONST_ATTR_NAMED(ads1015_sampling_frequency_available,
> -	sampling_frequency_available, "128 250 490 920 1600 2400 3300");
> -static IIO_CONST_ATTR_NAMED(ads1115_sampling_frequency_available,
> -	sampling_frequency_available, "8 16 32 64 128 250 475 860");
>  
>  static struct attribute *ads1015_attributes[] = {
> -	&iio_const_attr_scale_available.dev_attr.attr,
> -	&iio_const_attr_ads1015_sampling_frequency_available.dev_attr.attr,
> +	&iio_dev_attr_scale_available.dev_attr.attr,
> +	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
>  	NULL,
>  };
>  
> @@ -463,16 +494,6 @@ static const struct attribute_group ads1015_attribute_group = {
>  	.attrs = ads1015_attributes,
>  };
>  
> -static struct attribute *ads1115_attributes[] = {
> -	&iio_const_attr_scale_available.dev_attr.attr,
> -	&iio_const_attr_ads1115_sampling_frequency_available.dev_attr.attr,
> -	NULL,
> -};
> -
> -static const struct attribute_group ads1115_attribute_group = {
> -	.attrs = ads1115_attributes,
> -};
> -
>  static const struct iio_info ads1015_info = {
>  	.driver_module	= THIS_MODULE,
>  	.read_raw	= ads1015_read_raw,
> @@ -480,13 +501,6 @@ static const struct iio_info ads1015_info = {
>  	.attrs          = &ads1015_attribute_group,
>  };
>  
> -static const struct iio_info ads1115_info = {
> -	.driver_module	= THIS_MODULE,
> -	.read_raw	= ads1015_read_raw,
> -	.write_raw	= ads1015_write_raw,
> -	.attrs          = &ads1115_attribute_group,
> -};
> -
>  #ifdef CONFIG_OF
>  static int ads1015_get_channels_config_of(struct i2c_client *client)
>  {
> @@ -594,6 +608,7 @@ static int ads1015_probe(struct i2c_client *client,
>  	indio_dev->dev.of_node = client->dev.of_node;
>  	indio_dev->name = ADS1015_DRV_NAME;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &ads1015_info;
>  
>  	if (client->dev.of_node)
>  		chip = (enum chip_ids)of_device_get_match_data(&client->dev);
> @@ -603,14 +618,14 @@ static int ads1015_probe(struct i2c_client *client,
>  	case ADS1015:
>  		indio_dev->channels = ads1015_channels;
>  		indio_dev->num_channels = ARRAY_SIZE(ads1015_channels);
> -		indio_dev->info = &ads1015_info;
> -		data->data_rate = (unsigned int *) &ads1015_data_rate;
> +		data->data_rate = (unsigned short *) &ads1015_data_rate;
> +		data->data_rate_size = ARRAY_SIZE(ads1015_data_rate) - 1;
>  		break;
>  	case ADS1115:
>  		indio_dev->channels = ads1115_channels;
>  		indio_dev->num_channels = ARRAY_SIZE(ads1115_channels);
> -		indio_dev->info = &ads1115_info;
> -		data->data_rate = (unsigned int *) &ads1115_data_rate;
> +		data->data_rate = (unsigned short *) &ads1115_data_rate;
> +		data->data_rate_size = ARRAY_SIZE(ads1115_data_rate);
>  		break;
>  	}
>  

Re: [PATCH 1/2] iio: adc: ti-ads1015: fix conversion time

[Jonathan Cameron]

On Tue, 1 Aug 2017 10:26:05 +0200
Ladislav Michl <ladis@linux-mips.org> wrote:

> When reading diffrent channel value than device is currently processing
> wait time of conversion period is applied, which is not enough as device
> might be already in the middle of conversion and therefore previously
> converted value is returned - the one belonging to another channel.
> Fix it by waiting for two sampling periods.
> 
> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
Strangely we already have a fix in place for this it seems.

See the fixes-togreg branch of iio.git

Clearly a popular device at the moment!

Thanks,

Jonathan
> ---
>  drivers/iio/adc/ti-ads1015.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> index 884b8e461b17..86fd2753869d 100644
> --- a/drivers/iio/adc/ti-ads1015.c
> +++ b/drivers/iio/adc/ti-ads1015.c
> @@ -260,7 +260,7 @@ int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
>  		return ret;
>  
>  	if (change) {
> -		conv_time = DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr]);
> +		conv_time = 2 * DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr]);
>  		usleep_range(conv_time, conv_time + 1);
>  	}
>  

Re: [PATCH 1/2] iio: adc: ti-ads1015: fix conversion time

[Ladislav Michl]

On Wed, Aug 09, 2017 at 03:15:23PM +0100, Jonathan Cameron wrote:
> On Tue, 1 Aug 2017 10:26:05 +0200
> Ladislav Michl <ladis@linux-mips.org> wrote:
> 
> > When reading diffrent channel value than device is currently processing
> > wait time of conversion period is applied, which is not enough as device
> > might be already in the middle of conversion and therefore previously
> > converted value is returned - the one belonging to another channel.
> > Fix it by waiting for two sampling periods.
> > 
> > Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> Strangely we already have a fix in place for this it seems.
> 
> See the fixes-togreg branch of iio.git

Thanks for the pointer, I obviously was not aware about patch serie made by
Mita-san. Really nice work, far more superior to this simple fix :-)

Best regards,
	ladis

> Clearly a popular device at the moment!
> 
> Thanks,
> 
> Jonathan
> > ---
> >  drivers/iio/adc/ti-ads1015.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> > index 884b8e461b17..86fd2753869d 100644
> > --- a/drivers/iio/adc/ti-ads1015.c
> > +++ b/drivers/iio/adc/ti-ads1015.c
> > @@ -260,7 +260,7 @@ int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
> >  		return ret;
> >  
> >  	if (change) {
> > -		conv_time = DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr]);
> > +		conv_time = 2 * DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr]);
> >  		usleep_range(conv_time, conv_time + 1);
> >  	}
> >  

Re: [PATCH 2/2] iio: adc: ti-ads1015: use dynamically generated attributes

[Ladislav Michl]

Hello Jonathan,

On Wed, Aug 09, 2017 at 03:11:00PM +0100, Jonathan Cameron wrote:
> On Tue, 1 Aug 2017 10:26:46 +0200
> Ladislav Michl <ladis@linux-mips.org> wrote:
> 
> > Generate attributes dynamically based on actual values used
> > by driver - leads to slightly smaller code (on ARM and X64)
> > Scale and sampling frequency definitions are at one place now.
> > 
> > Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> Sorry, for the small gain I am unconvinced.  If this was a new
> driver and it had been submitted this way I wouldn't be asking
> for it to be changed, but I'm also not interested in the churn and
> additional code for what must be a pretty small chance.

As stated in cover letter I was not really considering it to be
a huge improvement and in fact did it as excercise to better
uderstand iio, so I'm not afraid it doesn't go in.

> Also if you really want to do tinification patches, please post
> the actual sizes of the various regions.  In particular you need
> to generally have a fairly large saving to justify replacing
> stuff that is constant with code.

I'll see if something better can be done based on fixes-togreg branch.

Thank you,
	ladis

Re: [PATCH 2/2] iio: adc: ti-ads1015: use dynamically generated attributes

[Jonathan Cameron]

On Wed, 9 Aug 2017 18:44:26 +0200
Ladislav Michl <ladis@linux-mips.org> wrote:

> Hello Jonathan,
> 
> On Wed, Aug 09, 2017 at 03:11:00PM +0100, Jonathan Cameron wrote:
> > On Tue, 1 Aug 2017 10:26:46 +0200
> > Ladislav Michl <ladis@linux-mips.org> wrote:
> >   
> > > Generate attributes dynamically based on actual values used
> > > by driver - leads to slightly smaller code (on ARM and X64)
> > > Scale and sampling frequency definitions are at one place now.
> > > 
> > > Signed-off-by: Ladislav Michl <ladis@linux-mips.org>  
> > Sorry, for the small gain I am unconvinced.  If this was a new
> > driver and it had been submitted this way I wouldn't be asking
> > for it to be changed, but I'm also not interested in the churn and
> > additional code for what must be a pretty small chance.  
> 
> As stated in cover letter I was not really considering it to be
> a huge improvement and in fact did it as excercise to better
> uderstand iio, so I'm not afraid it doesn't go in.
Ooops. The fact I don't always read cover letters becomes apparent ;)

Reviewers are lazy, put comments about a particular patch in that
patch then there is a slightly larger chance that people will notice
it.
> 
> > Also if you really want to do tinification patches, please post
> > the actual sizes of the various regions.  In particular you need
> > to generally have a fairly large saving to justify replacing
> > stuff that is constant with code.  
> 
> I'll see if something better can be done based on fixes-togreg branch.
Cool.
> 
> Thank you,
> 	ladis