IIO: Refactor info_mask* handling and introduce info_mask_shared_[by_all/by_type]

Introduce an enum to specify whether the attribute is separate or shared.

Factor out the bitmap handling for loop into a separate function.

Tidy up error handling and add a NULL assignment to squish a false positive warning from GCC.

Signed-off-by Jonathan Cameron <[email protected]>

---
 drivers/iio/iio_core.h            |   5 +-
 drivers/iio/industrialio-buffer.c |   2 +-
 drivers/iio/industrialio-core.c   | 148 ++++++++++++++++++++------------------
 3 files changed, 84 insertions(+), 71 deletions(-)

drivers/iio/iio_core.h +4 -1

index 05c1b74..75470ef 100644
@@ -20,6 +20,9 @@ struct iio_dev;
 
 extern struct device_type iio_device_type;
 
+enum iio_shared_by { __IIO_SEPARATE,
+		     __IIO_SHARED_BY_TYPE };
+
 int __iio_add_chan_devattr(const char *postfix,
 			   struct iio_chan_spec const *chan,
 			   ssize_t (*func)(struct device *dev,

@@ -30,7 +33,7 @@ int __iio_add_chan_devattr(const char *postfix,
 						const char *buf,
 						size_t len),
 			   u64 mask,
-			   bool generic,
+			   enum iio_shared_by generic,
 			   struct device *dev,
 			   struct list_head *attr_list);
 

drivers/iio/industrialio-buffer.c +1 -1

index e73033f..69e4a45 100644
@@ -214,7 +214,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
 				     &iio_show_scan_index,
 				     NULL,
 				     0,
-				     0,
+				     __IIO_SEPARATE,
 				     &indio_dev->dev,
 				     &buffer->scan_el_dev_attr_list);
 	if (ret)

drivers/iio/industrialio-core.c +79 -69

index 97f0297..720cea1 100644
@@ -516,14 +516,15 @@ int __iio_device_attr_init(struct device_attribute *dev_attr,
 						struct device_attribute *attr,
 						const char *buf,
 						size_t len),
-			   bool generic)
+			   enum iio_shared_by shared_by)
 {
-	int ret;
-	char *name_format, *full_postfix;
+	int ret = 0;
+	char *name_format = NULL;
+	char *full_postfix;
 	sysfs_attr_init(&dev_attr->attr);
 
 	/* Build up postfix of <extend_name>_<modifier>_postfix */
-	if (chan->modified && !generic) {
+	if (chan->modified && (shared_by == __IIO_SEPARATE)) {
 		if (chan->extend_name)
 			full_postfix = kasprintf(GFP_KERNEL, "%s_%s_%s",
 						 iio_modifier_names[chan

@@ -544,53 +545,62 @@ int __iio_device_attr_init(struct device_attribute *dev_attr,
 						 chan->extend_name,
 						 postfix);
 	}
-	if (full_postfix == NULL) {
-		ret = -ENOMEM;
-		goto error_ret;
-	}
+	if (full_postfix == NULL)
+		return -ENOMEM;
 
 	if (chan->differential) { /* Differential can not have modifier */
-		if (generic)
+		switch (shared_by) {
+		case __IIO_SHARED_BY_TYPE:
 			name_format
 				= kasprintf(GFP_KERNEL, "%s_%s-%s_%s",
 					    iio_direction[chan->output],
 					    iio_chan_type_name_spec[chan->type],
 					    iio_chan_type_name_spec[chan->type],
 					    full_postfix);
-		else if (chan->indexed)
+			break;
+		case __IIO_SEPARATE:
+			if (!chan->indexed) {
+				WARN_ON("Differential channels must be indexed\n");
+				ret = -EINVAL;
+				goto error_free_full_postfix;
+			}
 			name_format
-				= kasprintf(GFP_KERNEL, "%s_%s%d-%s%d_%s",
+				= kasprintf(GFP_KERNEL,
+					    "%s_%s%d-%s%d_%s",
 					    iio_direction[chan->output],
 					    iio_chan_type_name_spec[chan->type],
 					    chan->channel,
 					    iio_chan_type_name_spec[chan->type],
 					    chan->channel2,
 					    full_postfix);
-		else {
-			WARN_ON("Differential channels must be indexed\n");
-			ret = -EINVAL;
-			goto error_free_full_postfix;
+			break;
 		}
 	} else { /* Single ended */
-		if (generic)
-			name_format
-				= kasprintf(GFP_KERNEL, "%s_%s_%s",
-					    iio_direction[chan->output],
-					    iio_chan_type_name_spec[chan->type],
-					    full_postfix);
-		else if (chan->indexed)
-			name_format
-				= kasprintf(GFP_KERNEL, "%s_%s%d_%s",
-					    iio_direction[chan->output],
-					    iio_chan_type_name_spec[chan->type],
-					    chan->channel,
-					    full_postfix);
-		else
+		switch (shared_by) {
+		case __IIO_SHARED_BY_TYPE:
 			name_format
 				= kasprintf(GFP_KERNEL, "%s_%s_%s",
 					    iio_direction[chan->output],
 					    iio_chan_type_name_spec[chan->type],
 					    full_postfix);
+			break;
+
+		case __IIO_SEPARATE:
+			if (chan->indexed)
+				name_format
+					= kasprintf(GFP_KERNEL, "%s_%s%d_%s",
+						    iio_direction[chan->output],
+						    iio_chan_type_name_spec[chan->type],
+						    chan->channel,
+						    full_postfix);
+			else
+				name_format
+					= kasprintf(GFP_KERNEL, "%s_%s_%s",
+						    iio_direction[chan->output],
+						    iio_chan_type_name_spec[chan->type],
+						    full_postfix);
+			break;
+		}
 	}
 	if (name_format == NULL) {
 		ret = -ENOMEM;

@@ -614,16 +624,11 @@ int __iio_device_attr_init(struct device_attribute *dev_attr,
 		dev_attr->attr.mode |= S_IWUSR;
 		dev_attr->store = writefunc;
 	}
-	kfree(name_format);
-	kfree(full_postfix);
-
-	return 0;
-
 error_free_name_format:
 	kfree(name_format);
 error_free_full_postfix:
 	kfree(full_postfix);
-error_ret:
+
 	return ret;
 }
 

@@ -642,7 +647,7 @@ int __iio_add_chan_devattr(const char *postfix,
 						const char *buf,
 						size_t len),
 			   u64 mask,
-			   bool generic,
+			   enum iio_shared_by shared_by,
 			   struct device *dev,
 			   struct list_head *attr_list)
 {

@@ -656,7 +661,7 @@ int __iio_add_chan_devattr(const char *postfix,
 	}
 	ret = __iio_device_attr_init(&iio_attr->dev_attr,
 				     postfix, chan,
-				     readfunc, writefunc, generic);
+				     readfunc, writefunc, shared_by);
 	if (ret)
 		goto error_iio_dev_attr_free;
 	iio_attr->c = chan;

@@ -664,7 +669,7 @@ int __iio_add_chan_devattr(const char *postfix,
 	list_for_each_entry(t, attr_list, l)
 		if (strcmp(t->dev_attr.attr.name,
 			   iio_attr->dev_attr.attr.name) == 0) {
-			if (!generic)
+			if (shared_by == __IIO_SEPARATE)
 				dev_err(dev, "tried to double register : %s\n",
 					t->dev_attr.attr.name);
 			ret = -EBUSY;

@@ -682,46 +687,53 @@ error_ret:
 	return ret;
 }
 
-static int iio_device_add_channel_sysfs(struct iio_dev *indio_dev,
-					struct iio_chan_spec const *chan)
+static int iio_device_add_info_mask_type(struct iio_dev *indio_dev,
+					 struct iio_chan_spec const *chan,
+					 enum iio_shared_by shared_by,
+					 const long *infomask)
 {
-	int ret, attrcount = 0;
-	int i;
-	const struct iio_chan_spec_ext_info *ext_info;
+	int i, ret, attrcount = 0;
 
-	if (chan->channel < 0)
-		return 0;
-	for_each_set_bit(i, &chan->info_mask_separate, sizeof(long)*8) {
+	for_each_set_bit(i, infomask, sizeof(infomask)*8) {
 		ret = __iio_add_chan_devattr(iio_chan_info_postfix[i],
 					     chan,
 					     &iio_read_channel_info,
 					     &iio_write_channel_info,
 					     i,
-					     0,
+					     shared_by,
 					     &indio_dev->dev,
 					     &indio_dev->channel_attr_list);
-		if (ret < 0)
-			goto error_ret;
-		attrcount++;
-	}
-	for_each_set_bit(i, &chan->info_mask_shared_by_type, sizeof(long)*8) {
-		ret = __iio_add_chan_devattr(iio_chan_info_postfix[i],
-					     chan,
-					     &iio_read_channel_info,
-					     &iio_write_channel_info,
-					     i,
-					     1,
-					     &indio_dev->dev,
-					     &indio_dev->channel_attr_list);
-		if (ret == -EBUSY) {
-			ret = 0;
+		if ((ret == -EBUSY) && (shared_by != __IIO_SEPARATE))
 			continue;
-		} else if (ret < 0) {
-			goto error_ret;
-		}
+		else if (ret < 0)
+			return ret;
 		attrcount++;
 	}
 
+	return attrcount;
+}
+
+static int iio_device_add_channel_sysfs(struct iio_dev *indio_dev,
+					struct iio_chan_spec const *chan)
+{
+	int ret, attrcount = 0;
+	const struct iio_chan_spec_ext_info *ext_info;
+
+	if (chan->channel < 0)
+		return 0;
+	ret = iio_device_add_info_mask_type(indio_dev, chan,
+					    __IIO_SEPARATE,
+					    &chan->info_mask_separate);
+	if (ret < 0)
+		return ret;
+	attrcount += ret;
+
+	ret = iio_device_add_info_mask_type(indio_dev, chan,
+					    __IIO_SHARED_BY_TYPE,
+					    &chan->info_mask_shared_by_type);
+	if (ret < 0)
+		return ret;
+	attrcount += ret;
 	if (chan->ext_info) {
 		unsigned int i = 0;
 		for (ext_info = chan->ext_info; ext_info->name; ext_info++) {

@@ -740,15 +752,13 @@ static int iio_device_add_channel_sysfs(struct iio_dev *indio_dev,
 				continue;
 
 			if (ret)
-				goto error_ret;
+				return ret;
 
 			attrcount++;
 		}
 	}
 
-	ret = attrcount;
-error_ret:
-	return ret;
+	return attrcount;
 }
 
 static void iio_device_remove_and_free_read_attr(struct iio_dev *indio_dev,

Doing this makes it possible to access this control from within the kernel.

Signed-off-by Jonathan Cameron <[email protected]>

---
 drivers/iio/temperature/tmp006.c | 57 +++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 30 deletions(-)

drivers/iio/temperature/tmp006.c +27 -30

index 64ccde3..3df7a7e 100644
@@ -70,12 +70,16 @@ static int tmp006_read_measurement(struct tmp006_data *data, u8 reg)
 	return i2c_smbus_read_word_swapped(data->client, reg);
 }
 
+static const int const tmp006_freqs[5][2] = { {4, 0}, {2, 0}, {1, 0},
+					      {0, 500000}, {0, 250000} };
+
 static int tmp006_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *channel, int *val,
 			    int *val2, long mask)
 {
 	struct tmp006_data *data = iio_priv(indio_dev);
 	s32 ret;
+	int cr;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:

@@ -106,6 +110,12 @@ static int tmp006_read_raw(struct iio_dev *indio_dev,
 			break;
 		}
 		return IIO_VAL_INT_PLUS_MICRO;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		cr = (data->config & TMP006_CONFIG_CR_MASK)
+			>> TMP006_CONFIG_CR_SHIFT;
+		*val = tmp006_freqs[cr][0];
+		*val2 = tmp006_freqs[cr][1];
+		return IIO_VAL_INT_PLUS_MICRO;
 	default:
 		break;
 	}

@@ -113,48 +123,32 @@ static int tmp006_read_raw(struct iio_dev *indio_dev,
 	return -EINVAL;
 }
 
-static const char * const tmp006_freqs[] = { "4", "2", "1", "0.5", "0.25" };
-
-static ssize_t tmp006_show_freq(struct device *dev,
-				struct device_attribute *attr, char *buf)
-{
-	struct tmp006_data *data = iio_priv(dev_to_iio_dev(dev));
-	int cr = (data->config & TMP006_CONFIG_CR_MASK)
-		>> TMP006_CONFIG_CR_SHIFT;
-	return sprintf(buf, "%s\n", tmp006_freqs[cr]);
-}
-
-static ssize_t tmp006_store_freq(struct device *dev,
-				 struct device_attribute *attr,
-				 const char *buf, size_t len)
+static int tmp006_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val,
+			    int val2,
+			    long mask)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct tmp006_data *data = iio_priv(indio_dev);
 	int i;
-	bool found = false;
 
 	for (i = 0; i < ARRAY_SIZE(tmp006_freqs); i++)
-		if (sysfs_streq(buf, tmp006_freqs[i])) {
-			found = true;
-			break;
-		}
-	if (!found)
-		return -EINVAL;
+		if ((val == tmp006_freqs[i][0]) &&
+		    (val2 == tmp006_freqs[i][1])) {
+			data->config &= ~TMP006_CONFIG_CR_MASK;
+			data->config |= i << TMP006_CONFIG_CR_SHIFT;
 
-	data->config &= ~TMP006_CONFIG_CR_MASK;
-	data->config |= i << TMP006_CONFIG_CR_SHIFT;
+			return i2c_smbus_write_word_swapped(data->client,
+							    TMP006_CONFIG,
+							    data->config);
 
-	return i2c_smbus_write_word_swapped(data->client, TMP006_CONFIG,
-		data->config);
+		}
+	return -EINVAL;
 }
 
-static IIO_DEV_ATTR_SAMP_FREQ(S_IRUGO | S_IWUSR,
-			tmp006_show_freq, tmp006_store_freq);
-
 static IIO_CONST_ATTR(sampling_frequency_available, "4 2 1 0.5 0.25");
 
 static struct attribute *tmp006_attributes[] = {
-	&iio_dev_attr_sampling_frequency.dev_attr.attr,
 	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
 	NULL
 };

@@ -168,16 +162,19 @@ static const struct iio_chan_spec tmp006_channels[] = {
 		.type = IIO_VOLTAGE,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
 			BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
 	},
 	{
 		.type = IIO_TEMP,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
 			BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
 	}
 };
 
 static const struct iio_info tmp006_info = {
 	.read_raw = tmp006_read_raw,
+	.write_raw = tmp006_write_raw,
 	.attrs = &tmp006_attribute_group,
 	.driver_module = THIS_MODULE,
 };

Hello Jonathan,

Quoted 1 line

Doing this makes it possible to access this control from within the kernel.

I noticed this shortcoming as well after submitting;
so the plan is to remove IIO_DEV_ATTR_SAMP_FREQ() sometimes, right?

patch looks good!

Acked-by Peter Meerwald <[email protected]>

regards, p.

Quoted 123 lines

Signed-off-by Jonathan Cameron <[email protected]>

---
 drivers/iio/temperature/tmp006.c | 57 +++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 30 deletions(-)

drivers/iio/temperature/tmp006.c +27 -30

index 64ccde3..3df7a7e 100644
@@ -70,12 +70,16 @@ static int tmp006_read_measurement(struct tmp006_data *data, u8 reg)
 	return i2c_smbus_read_word_swapped(data->client, reg);
 }
 
+static const int const tmp006_freqs[5][2] = { {4, 0}, {2, 0}, {1, 0},
+					      {0, 500000}, {0, 250000} };
+
 static int tmp006_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *channel, int *val,
 			    int *val2, long mask)
 {
 	struct tmp006_data *data = iio_priv(indio_dev);
 	s32 ret;
+	int cr;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:

@@ -106,6 +110,12 @@ static int tmp006_read_raw(struct iio_dev *indio_dev,
 			break;
 		}
 		return IIO_VAL_INT_PLUS_MICRO;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		cr = (data->config & TMP006_CONFIG_CR_MASK)
+			>> TMP006_CONFIG_CR_SHIFT;
+		*val = tmp006_freqs[cr][0];
+		*val2 = tmp006_freqs[cr][1];
+		return IIO_VAL_INT_PLUS_MICRO;
 	default:
 		break;
 	}

@@ -113,48 +123,32 @@ static int tmp006_read_raw(struct iio_dev *indio_dev,
 	return -EINVAL;
 }
 
-static const char * const tmp006_freqs[] = { "4", "2", "1", "0.5", "0.25" };
-
-static ssize_t tmp006_show_freq(struct device *dev,
-				struct device_attribute *attr, char *buf)
-{
-	struct tmp006_data *data = iio_priv(dev_to_iio_dev(dev));
-	int cr = (data->config & TMP006_CONFIG_CR_MASK)
-		>> TMP006_CONFIG_CR_SHIFT;
-	return sprintf(buf, "%s\n", tmp006_freqs[cr]);
-}
-
-static ssize_t tmp006_store_freq(struct device *dev,
-				 struct device_attribute *attr,
-				 const char *buf, size_t len)
+static int tmp006_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val,
+			    int val2,
+			    long mask)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct tmp006_data *data = iio_priv(indio_dev);
 	int i;
-	bool found = false;
 
 	for (i = 0; i < ARRAY_SIZE(tmp006_freqs); i++)
-		if (sysfs_streq(buf, tmp006_freqs[i])) {
-			found = true;
-			break;
-		}
-	if (!found)
-		return -EINVAL;
+		if ((val == tmp006_freqs[i][0]) &&
+		    (val2 == tmp006_freqs[i][1])) {
+			data->config &= ~TMP006_CONFIG_CR_MASK;
+			data->config |= i << TMP006_CONFIG_CR_SHIFT;
 
-	data->config &= ~TMP006_CONFIG_CR_MASK;
-	data->config |= i << TMP006_CONFIG_CR_SHIFT;
+			return i2c_smbus_write_word_swapped(data->client,
+							    TMP006_CONFIG,
+							    data->config);
 
-	return i2c_smbus_write_word_swapped(data->client, TMP006_CONFIG,
-		data->config);
+		}
+	return -EINVAL;
 }
 
-static IIO_DEV_ATTR_SAMP_FREQ(S_IRUGO | S_IWUSR,
-			tmp006_show_freq, tmp006_store_freq);
-
 static IIO_CONST_ATTR(sampling_frequency_available, "4 2 1 0.5 0.25");
 
 static struct attribute *tmp006_attributes[] = {
-	&iio_dev_attr_sampling_frequency.dev_attr.attr,
 	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
 	NULL
 };

@@ -168,16 +162,19 @@ static const struct iio_chan_spec tmp006_channels[] = {
 		.type = IIO_VOLTAGE,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
 			BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
 	},
 	{
 		.type = IIO_TEMP,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
 			BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
 	}
 };
 
 static const struct iio_info tmp006_info = {
 	.read_raw = tmp006_read_raw,
+	.write_raw = tmp006_write_raw,
 	.attrs = &tmp006_attribute_group,
 	.driver_module = THIS_MODULE,
 };