[RFC PATCH 1/2] iio: hmc5843 Add channel attribute for bias configuration

[RFC PATCH 1/2] iio: hmc5843 Add channel attribute for bias configuration

[Cristina Moraru]

Replace non standard meas_conf attribute with the standard IIO
calibbias attribute.

API for setting bias measurement configuration:

0 - Normal measurement configuration (default): In normal measurement
    configuration the device follows normal measurement flow. Pins BP
    and BN are left floating and high impedance.

1 - Positive bias configuration: In positive bias configuration, a
    positive current is forced across the resistive load on pins BP
    and BN.

2 - Negative bias configuration. In negative bias configuration, a
    negative current is forced across the resistive load on pins BP
    and BN.

3 - Only available on HMC5983. Magnetic sensor is disabled.
    Temperature sensor is enabled.

With this in place, we can think of moving this driver out of staging.

Signed-off-by: Cristina Moraru <cristina.moraru09@gmail.com>
---
 drivers/staging/iio/magnetometer/hmc5843_core.c | 62 +++++++------------------
 1 file changed, 16 insertions(+), 46 deletions(-)

diff --git a/drivers/staging/iio/magnetometer/hmc5843_core.c b/drivers/staging/iio/magnetometer/hmc5843_core.c
index 4aab022..c5f16da 100644
--- a/drivers/staging/iio/magnetometer/hmc5843_core.c
+++ b/drivers/staging/iio/magnetometer/hmc5843_core.c
@@ -205,50 +205,6 @@ static int hmc5843_set_meas_conf(struct hmc5843_data *data, u8 meas_conf)
 }
 
 static
-ssize_t hmc5843_show_measurement_configuration(struct device *dev,
-					       struct device_attribute *attr,
-					       char *buf)
-{
-	struct hmc5843_data *data = iio_priv(dev_to_iio_dev(dev));
-	unsigned int val;
-	int ret;
-
-	ret = regmap_read(data->regmap, HMC5843_CONFIG_REG_A, &val);
-	if (ret)
-		return ret;
-	val &= HMC5843_MEAS_CONF_MASK;
-
-	return sprintf(buf, "%d\n", val);
-}
-
-static
-ssize_t hmc5843_set_measurement_configuration(struct device *dev,
-					      struct device_attribute *attr,
-					      const char *buf,
-					      size_t count)
-{
-	struct hmc5843_data *data = iio_priv(dev_to_iio_dev(dev));
-	unsigned long meas_conf = 0;
-	int ret;
-
-	ret = kstrtoul(buf, 10, &meas_conf);
-	if (ret)
-		return ret;
-	if (meas_conf >= HMC5843_MEAS_CONF_MASK)
-		return -EINVAL;
-
-	ret = hmc5843_set_meas_conf(data, meas_conf);
-
-	return (ret < 0) ? ret : count;
-}
-
-static IIO_DEVICE_ATTR(meas_conf,
-			S_IWUSR | S_IRUGO,
-			hmc5843_show_measurement_configuration,
-			hmc5843_set_measurement_configuration,
-			0);
-
-static
 ssize_t hmc5843_show_samp_freq_avail(struct device *dev,
 				     struct device_attribute *attr, char *buf)
 {
@@ -371,6 +327,13 @@ static int hmc5843_read_raw(struct iio_dev *indio_dev,
 		*val = data->variant->regval_to_samp_freq[rval][0];
 		*val2 = data->variant->regval_to_samp_freq[rval][1];
 		return IIO_VAL_INT_PLUS_MICRO;
+	case IIO_CHAN_INFO_CALIBBIAS:
+		ret = regmap_read(data->regmap, HMC5843_CONFIG_REG_A, &rval);
+		if (ret)
+			return ret;
+		rval &= HMC5843_MEAS_CONF_MASK;
+		*val = rval;
+		return IIO_VAL_INT;
 	}
 	return -EINVAL;
 }
@@ -395,6 +358,11 @@ static int hmc5843_write_raw(struct iio_dev *indio_dev,
 			return -EINVAL;
 
 		return hmc5843_set_range_gain(data, range);
+	case IIO_CHAN_INFO_CALIBBIAS:
+		if (val >= HMC5843_MEAS_CONF_MASK)
+			return -EINVAL;
+
+		return hmc5843_set_meas_conf(data, val);
 	default:
 		return -EINVAL;
 	}
@@ -409,6 +377,8 @@ static int hmc5843_write_raw_get_fmt(struct iio_dev *indio_dev,
 		return IIO_VAL_INT_PLUS_MICRO;
 	case IIO_CHAN_INFO_SCALE:
 		return IIO_VAL_INT_PLUS_NANO;
+	case IIO_CHAN_INFO_CALIBBIAS:
+		return IIO_VAL_INT;
 	default:
 		return -EINVAL;
 	}
@@ -451,7 +421,8 @@ done:
 		.channel2 = IIO_MOD_##axis,				\
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
 		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |	\
-			BIT(IIO_CHAN_INFO_SAMP_FREQ),			\
+					BIT(IIO_CHAN_INFO_SAMP_FREQ) |  \
+					BIT(IIO_CHAN_INFO_CALIBBIAS),   \
 		.scan_index = idx,					\
 		.scan_type = {						\
 			.sign = 's',					\
@@ -477,7 +448,6 @@ static const struct iio_chan_spec hmc5883_channels[] = {
 };
 
 static struct attribute *hmc5843_attributes[] = {
-	&iio_dev_attr_meas_conf.dev_attr.attr,
 	&iio_dev_attr_scale_available.dev_attr.attr,
 	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
 	NULL
-- 
1.9.1

[RFC PATCH 2/2] iio: hmc5843: Add attribute for available bias values

[Cristina Moraru]

Add static attribute calibbias_available to show available
configurations for the bias current.

API for setting bias measurement configuration:

0 - Normal measurement configuration (default): In normal measurement
    configuration the device follows normal measurement flow. Pins BP
    and BN are left floating and high impedance.

1 - Positive bias configuration: In positive bias configuration, a
    positive current is forced across the resistive load on pins BP
    and BN.

2 - Negative bias configuration. In negative bias configuration, a
    negative current is forced across the resistive load on pins BP
    and BN.

3 - Only available on HMC5983. Magnetic sensor is disabled.
    Temperature sensor is enabled.

Signed-off-by: Cristina Moraru <cristina.moraru09@gmail.com>
---
 drivers/staging/iio/magnetometer/hmc5843_core.c | 83 +++++++++++++++++++------
 1 file changed, 65 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/iio/magnetometer/hmc5843_core.c b/drivers/staging/iio/magnetometer/hmc5843_core.c
index c5f16da..9fc0eaa 100644
--- a/drivers/staging/iio/magnetometer/hmc5843_core.c
+++ b/drivers/staging/iio/magnetometer/hmc5843_core.c
@@ -66,6 +66,34 @@
 #define HMC5843_MEAS_CONF_NEGATIVE_BIAS		0x02
 #define HMC5843_MEAS_CONF_MASK			0x03
 
+/*
+ * API for setting the measurement configuration to
+ * Normal, Positive bias and Negative bias
+ *
+ * From the datasheet:
+ * 0 - Normal measurement configuration (default): In normal measurement
+ *     configuration the device follows normal measurement flow. Pins BP
+ *     and BN are left floating and high impedance.
+ *
+ * 1 - Positive bias configuration: In positive bias configuration, a
+ *     positive current is forced across the resistive load on pins BP
+ *     and BN.
+ *
+ * 2 - Negative bias configuration. In negative bias configuration, a
+ *     negative current is forced across the resistive load on pins BP
+ *     and BN.
+ *
+ * 3 - Only available on HMC5983. Magnetic sensor is disabled.
+ *     Temperature sensor is enabled.
+ */
+static const int hmc5843_regval_to_calibbias[] = {
+	0, 1, 2
+};
+
+static const int hmc5983_regval_to_calibbias[] = {
+	0, 1, 2, 3
+};
+
 /* Scaling factors: 10000000/Gain */
 static const int hmc5843_regval_to_nanoscale[] = {
 	6173, 7692, 10309, 12821, 18868, 21739, 25641, 35714
@@ -109,6 +137,8 @@ static const int hmc5983_regval_to_samp_freq[][2] = {
 /* Describe chip variants */
 struct hmc5843_chip_info {
 	const struct iio_chan_spec *channels;
+	const int *regval_to_calibbias;
+	const int n_regval_to_calibbias;
 	const int (*regval_to_samp_freq)[2];
 	const int n_regval_to_samp_freq;
 	const int *regval_to_nanoscale;
@@ -174,24 +204,6 @@ static int hmc5843_read_measurement(struct hmc5843_data *data,
 	return IIO_VAL_INT;
 }
 
-/*
- * API for setting the measurement configuration to
- * Normal, Positive bias and Negative bias
- *
- * From the datasheet:
- * 0 - Normal measurement configuration (default): In normal measurement
- *     configuration the device follows normal measurement flow. Pins BP
- *     and BN are left floating and high impedance.
- *
- * 1 - Positive bias configuration: In positive bias configuration, a
- *     positive current is forced across the resistive load on pins BP
- *     and BN.
- *
- * 2 - Negative bias configuration. In negative bias configuration, a
- *     negative current is forced across the resistive load on pins BP
- *     and BN.
- *
- */
 static int hmc5843_set_meas_conf(struct hmc5843_data *data, u8 meas_conf)
 {
 	int ret;
@@ -286,6 +298,28 @@ static ssize_t hmc5843_show_scale_avail(struct device *dev,
 static IIO_DEVICE_ATTR(scale_available, S_IRUGO,
 	hmc5843_show_scale_avail, NULL, 0);
 
+static ssize_t hmc5843_show_calibbias_avail(struct device *dev,
+					    struct device_attribute *attr,
+					    char *buf)
+{
+	struct hmc5843_data *data = iio_priv(dev_to_iio_dev(dev));
+
+	size_t len = 0;
+	int i;
+
+	for (i = 0; i < data->variant->n_regval_to_calibbias; i++)
+		len += scnprintf(buf + len, PAGE_SIZE - len,
+			"%d ", data->variant->regval_to_calibbias[i]);
+
+	/* replace trailing space by newline */
+	buf[len - 1] = '\n';
+
+	return len;
+}
+
+static IIO_DEVICE_ATTR(calibbias_available, S_IRUGO,
+	hmc5843_show_calibbias_avail, NULL, 0);
+
 static int hmc5843_get_scale_index(struct hmc5843_data *data, int val, int val2)
 {
 	int i;
@@ -448,6 +482,7 @@ static const struct iio_chan_spec hmc5883_channels[] = {
 };
 
 static struct attribute *hmc5843_attributes[] = {
+	&iio_dev_attr_calibbias_available.dev_attr.attr,
 	&iio_dev_attr_scale_available.dev_attr.attr,
 	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
 	NULL
@@ -460,6 +495,9 @@ static const struct attribute_group hmc5843_group = {
 static const struct hmc5843_chip_info hmc5843_chip_info_tbl[] = {
 	[HMC5843_ID] = {
 		.channels = hmc5843_channels,
+		.regval_to_calibbias = hmc5843_regval_to_calibbias,
+		.n_regval_to_calibbias =
+				ARRAY_SIZE(hmc5843_regval_to_calibbias),
 		.regval_to_samp_freq = hmc5843_regval_to_samp_freq,
 		.n_regval_to_samp_freq =
 				ARRAY_SIZE(hmc5843_regval_to_samp_freq),
@@ -469,6 +507,9 @@ static const struct hmc5843_chip_info hmc5843_chip_info_tbl[] = {
 	},
 	[HMC5883_ID] = {
 		.channels = hmc5883_channels,
+		.regval_to_calibbias = hmc5843_regval_to_calibbias,
+		.n_regval_to_calibbias =
+				ARRAY_SIZE(hmc5843_regval_to_calibbias),
 		.regval_to_samp_freq = hmc5883_regval_to_samp_freq,
 		.n_regval_to_samp_freq =
 				ARRAY_SIZE(hmc5883_regval_to_samp_freq),
@@ -478,6 +519,9 @@ static const struct hmc5843_chip_info hmc5843_chip_info_tbl[] = {
 	},
 	[HMC5883L_ID] = {
 		.channels = hmc5883_channels,
+		.regval_to_calibbias = hmc5843_regval_to_calibbias,
+		.n_regval_to_calibbias =
+				ARRAY_SIZE(hmc5843_regval_to_calibbias),
 		.regval_to_samp_freq = hmc5883_regval_to_samp_freq,
 		.n_regval_to_samp_freq =
 				ARRAY_SIZE(hmc5883_regval_to_samp_freq),
@@ -487,6 +531,9 @@ static const struct hmc5843_chip_info hmc5843_chip_info_tbl[] = {
 	},
 	[HMC5983_ID] = {
 		.channels = hmc5883_channels,
+		.regval_to_calibbias = hmc5983_regval_to_calibbias,
+		.n_regval_to_calibbias =
+				ARRAY_SIZE(hmc5983_regval_to_calibbias),
 		.regval_to_samp_freq = hmc5983_regval_to_samp_freq,
 		.n_regval_to_samp_freq =
 				ARRAY_SIZE(hmc5983_regval_to_samp_freq),
-- 
1.9.1

Re: [RFC PATCH 1/2] iio: hmc5843 Add channel attribute for bias configuration

[Lars-Peter Clausen]

On 02/04/2016 03:50 PM, Cristina Moraru wrote:
> Replace non standard meas_conf attribute with the standard IIO
> calibbias attribute.
> 
> API for setting bias measurement configuration:
> 
> 0 - Normal measurement configuration (default): In normal measurement
>     configuration the device follows normal measurement flow. Pins BP
>     and BN are left floating and high impedance.
> 
> 1 - Positive bias configuration: In positive bias configuration, a
>     positive current is forced across the resistive load on pins BP
>     and BN.
> 
> 2 - Negative bias configuration. In negative bias configuration, a
>     negative current is forced across the resistive load on pins BP
>     and BN.
> 
> 3 - Only available on HMC5983. Magnetic sensor is disabled.
>     Temperature sensor is enabled.
> 
> With this in place, we can think of moving this driver out of staging.

Using a standard attribute, but overloading it with custom semantics doesn't
do any good either. calibbias is supposed to be a integer that gets added to
measurements internally in the device (unit is device specific though).

This attribute seems to do something else. In that case it might be better
to stay with a custom attribute (as long as it is documented) or come up
with a better way to map the device configuration onto standard attributes.

Re: [RFC PATCH 1/2] iio: hmc5843 Add channel attribute for bias configuration

[Daniel Baluta]

On Thu, Feb 4, 2016 at 7:45 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 02/04/2016 03:50 PM, Cristina Moraru wrote:
>> Replace non standard meas_conf attribute with the standard IIO
>> calibbias attribute.
>>
>> API for setting bias measurement configuration:
>>
>> 0 - Normal measurement configuration (default): In normal measurement
>>     configuration the device follows normal measurement flow. Pins BP
>>     and BN are left floating and high impedance.
>>
>> 1 - Positive bias configuration: In positive bias configuration, a
>>     positive current is forced across the resistive load on pins BP
>>     and BN.
>>
>> 2 - Negative bias configuration. In negative bias configuration, a
>>     negative current is forced across the resistive load on pins BP
>>     and BN.
>>
>> 3 - Only available on HMC5983. Magnetic sensor is disabled.
>>     Temperature sensor is enabled.
>>
>> With this in place, we can think of moving this driver out of staging.
>
> Using a standard attribute, but overloading it with custom semantics doesn't
> do any good either. calibbias is supposed to be a integer that gets added to
> measurements internally in the device (unit is device specific though).
>
> This attribute seems to do something else. In that case it might be better
> to stay with a custom attribute (as long as it is documented) or come up
> with a better way to map the device configuration onto standard attributes.

We missed the RFC tag. So, with this in mind we still have some minor
issues to fix and then we can move the code out of staging.

1) checkpatch.pl -- strict warnings
2) suspend/resume functions in hmc5843_core.c.

int hmc5843_common_suspend(struct device *dev)
{
        return hmc5843_set_mode(iio_priv(dev_get_drvdata(dev)),
                        HMC5843_MODE_CONVERSION_CONTINUOUS);
}

int hmc5843_common_resume(struct device *dev)
{
        return hmc5843_set_mode(iio_priv(dev_get_drvdata(dev)),
                                HMC5843_MODE_SLEEP);
}

I think the function names or implementations were swapped.
The device should go to sleep at suspend and back to continuous
mode on resume.

thanks,
Daniel.

Re: [RFC PATCH 1/2] iio: hmc5843 Add channel attribute for bias configuration

[Jonathan Cameron]

On 04/02/16 17:45, Lars-Peter Clausen wrote:
> On 02/04/2016 03:50 PM, Cristina Moraru wrote:
>> Replace non standard meas_conf attribute with the standard IIO
>> calibbias attribute.
>>
>> API for setting bias measurement configuration:
>>
>> 0 - Normal measurement configuration (default): In normal measurement
>>     configuration the device follows normal measurement flow. Pins BP
>>     and BN are left floating and high impedance.
>>
>> 1 - Positive bias configuration: In positive bias configuration, a
>>     positive current is forced across the resistive load on pins BP
>>     and BN.
>>
>> 2 - Negative bias configuration. In negative bias configuration, a
>>     negative current is forced across the resistive load on pins BP
>>     and BN.
>>
>> 3 - Only available on HMC5983. Magnetic sensor is disabled.
>>     Temperature sensor is enabled.
>>
>> With this in place, we can think of moving this driver out of staging.
> 
> Using a standard attribute, but overloading it with custom semantics doesn't
> do any good either. calibbias is supposed to be a integer that gets added to
> measurements internally in the device (unit is device specific though).
> 
> This attribute seems to do something else. In that case it might be better
> to stay with a custom attribute (as long as it is documented) or come up
> with a better way to map the device configuration onto standard attributes.
> 
Agree with Lars on this.  Only consistent way that this 'could' (it's not a good idea!)
be supported would be to make this an explicit output channel in a similar way
to we have debated doing with LED drivers in some of the proximity devices.

If it was meant to be a refined compensation of the measured output (perhaps
to correct for some other local field) then perhaps we could argue it as
an analog correction for which calibbias etc would be just about applicable.
Here it's really about self test I think...  One option would be to write
a sane self test entirely in the driver and run that on probe, similar to
we do for some other devices.  Then you could remove this attribute entirely.

Mind you it's not entirely obvious to me what steps the self test would actually
have so perhaps best to leave this to userspace (at least for now!)

Jonathan

Re: [RFC PATCH 1/2] iio: hmc5843 Add channel attribute for bias configuration

[Jonathan Cameron]

On 05/02/16 09:58, Daniel Baluta wrote:
> On Thu, Feb 4, 2016 at 7:45 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> On 02/04/2016 03:50 PM, Cristina Moraru wrote:
>>> Replace non standard meas_conf attribute with the standard IIO
>>> calibbias attribute.
>>>
>>> API for setting bias measurement configuration:
>>>
>>> 0 - Normal measurement configuration (default): In normal measurement
>>>     configuration the device follows normal measurement flow. Pins BP
>>>     and BN are left floating and high impedance.
>>>
>>> 1 - Positive bias configuration: In positive bias configuration, a
>>>     positive current is forced across the resistive load on pins BP
>>>     and BN.
>>>
>>> 2 - Negative bias configuration. In negative bias configuration, a
>>>     negative current is forced across the resistive load on pins BP
>>>     and BN.
>>>
>>> 3 - Only available on HMC5983. Magnetic sensor is disabled.
>>>     Temperature sensor is enabled.
>>>
>>> With this in place, we can think of moving this driver out of staging.
>>
>> Using a standard attribute, but overloading it with custom semantics doesn't
>> do any good either. calibbias is supposed to be a integer that gets added to
>> measurements internally in the device (unit is device specific though).
>>
>> This attribute seems to do something else. In that case it might be better
>> to stay with a custom attribute (as long as it is documented) or come up
>> with a better way to map the device configuration onto standard attributes.
> 
> We missed the RFC tag. So, with this in mind we still have some minor
> issues to fix and then we can move the code out of staging.
> 
> 1) checkpatch.pl -- strict warnings
> 2) suspend/resume functions in hmc5843_core.c.
> 
> int hmc5843_common_suspend(struct device *dev)
> {
>         return hmc5843_set_mode(iio_priv(dev_get_drvdata(dev)),
>                         HMC5843_MODE_CONVERSION_CONTINUOUS);
> }
> 
> int hmc5843_common_resume(struct device *dev)
> {
>         return hmc5843_set_mode(iio_priv(dev_get_drvdata(dev)),
>                                 HMC5843_MODE_SLEEP);
> }
> 
> I think the function names or implementations were swapped.
> The device should go to sleep at suspend and back to continuous
> mode on resume.
Yup, from a quick glance - it otherwise looks sensible to me.
Will as ever attract more reviews once the move out of staging
patch shows up.

Jonathan

> 
> thanks,
> Daniel.
> --
> 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
>