Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1394607
| From | Jonathan Cameron <jic23@jic23.retrosnub.co.uk> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: [PATCH 04/13] iio: health/afe440x: Always use separate gain values |
| Date | 2016-05-04 20:40 +0200 |
| Message-ID | <rv9Sh-Pg-11@gated-at.bofh.it> (permalink) |
| References | <ru6jM-7pJ-3@gated-at.bofh.it> <ru6tr-7tP-1@gated-at.bofh.it> <rv6hJ-5S0-41@gated-at.bofh.it> <rv6KL-6s5-55@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
On 4 May 2016 16:13:29 BST, "Andrew F. Davis" <afd@ti.com> wrote:
>On 05/04/2016 05:02 AM, Jonathan Cameron wrote:
>> On 01/05/16 21:36, Andrew F. Davis wrote:
>>> Locking the two gain stages to the same setting adds no value for
>us,
>>> so initialize them as unlocked and remove the sysfs for unlocking
>them.
>>> This also allows us to greatly simplify showing and setting the gain
>>> registers.
>>>
>>> Signed-off-by: Andrew F. Davis <afd@ti.com>
>> Hmm. ABI change but as you said it's an improvement.
>>
>> Honestly I doubt anyone is using this device without also using
>userspace that
>> you are providing so lets apply this an cross our fingers that no one
>minds.
>>
>
>Actually we don't even provide a Linux userspace for these yet,
>starting
>to work on that is what prompted these changes. These parts have
>traditionally been paired with a microcontroller to perform the work in
>dedicated devices. This was put on my desk as they are hoping to start
>enabling these to be integrated into smart devices running Linux
>(Android). I'm not an expert in pulse metering, so my initial attempt
>at
>an ABI was a bit off compared to what I now believe to be the "correct"
>way to expose these parts.
*laughs* sometimes the only way to find out is developing real products.
>
>Thanks again for tolerating all this madness :),
It's cool. I worked with some medics who loved pulse oximeters a few years back.
Not sure what kit they were using though.
We were doing breathing measurement with structure light. Turns out you can get
pulse from that (sort of). Plus all sorts of weird plots that medics love which make
no sense at all from an engineering POV.
I like weird devices even if they can cause headaches :) Weird comes in many forms!
Jonathan
>Andrew
>
>> Applied.
>>> ---
>>> .../ABI/testing/sysfs-bus-iio-health-afe440x | 9 ----
>>> drivers/iio/health/afe4403.c | 60
>++++++----------------
>>> drivers/iio/health/afe4404.c | 60
>++++++----------------
>>> drivers/iio/health/afe440x.h | 15 ++----
>>> 4 files changed, 37 insertions(+), 107 deletions(-)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-health-afe440x
>b/Documentation/ABI/testing/sysfs-bus-iio-health-afe440x
>>> index 3740f25..b19053a 100644
>>> --- a/Documentation/ABI/testing/sysfs-bus-iio-health-afe440x
>>> +++ b/Documentation/ABI/testing/sysfs-bus-iio-health-afe440x
>>> @@ -8,15 +8,6 @@ Description:
>>> Transimpedance Amplifier. Y is 1 for Rf1 and Cf1, Y is 2 for
>>> Rf2 and Cf2 values.
>>>
>>> -What: /sys/bus/iio/devices/iio:deviceX/tia_separate_en
>>> -Date: December 2015
>>> -KernelVersion:
>>> -Contact: Andrew F. Davis <afd@ti.com>
>>> -Description:
>>> - Enable or disable separate settings for the TransImpedance
>>> - Amplifier above, when disabled both values are set by the
>>> - first channel.
>>> -
>>> What: /sys/bus/iio/devices/iio:deviceX/in_intensity_ledY_raw
>>> /sys/bus/iio/devices/iio:deviceX/in_intensity_ledY_ambient_raw
>>> Date: December 2015
>>> diff --git a/drivers/iio/health/afe4403.c
>b/drivers/iio/health/afe4403.c
>>> index 5484785..bcff528 100644
>>> --- a/drivers/iio/health/afe4403.c
>>> +++ b/drivers/iio/health/afe4403.c
>>> @@ -180,9 +180,9 @@ static ssize_t afe440x_show_register(struct
>device *dev,
>>> struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> struct afe4403_data *afe = iio_priv(indio_dev);
>>> struct afe440x_attr *afe440x_attr = to_afe440x_attr(attr);
>>> - unsigned int reg_val, type;
>>> + unsigned int reg_val;
>>> int vals[2];
>>> - int ret, val_len;
>>> + int ret;
>>>
>>> ret = regmap_read(afe->regmap, afe440x_attr->reg, ®_val);
>>> if (ret)
>>> @@ -191,27 +191,13 @@ static ssize_t afe440x_show_register(struct
>device *dev,
>>> reg_val &= afe440x_attr->mask;
>>> reg_val >>= afe440x_attr->shift;
>>>
>>> - switch (afe440x_attr->type) {
>>> - case SIMPLE:
>>> - type = IIO_VAL_INT;
>>> - val_len = 1;
>>> - vals[0] = reg_val;
>>> - break;
>>> - case RESISTANCE:
>>> - case CAPACITANCE:
>>> - type = IIO_VAL_INT_PLUS_MICRO;
>>> - val_len = 2;
>>> - if (reg_val < afe440x_attr->table_size) {
>>> - vals[0] = afe440x_attr->val_table[reg_val].integer;
>>> - vals[1] = afe440x_attr->val_table[reg_val].fract;
>>> - break;
>>> - }
>>> + if (reg_val >= afe440x_attr->table_size)
>>> return -EINVAL;
>>> - default:
>>> - return -EINVAL;
>>> - }
>>>
>>> - return iio_format_value(buf, type, val_len, vals);
>>> + vals[0] = afe440x_attr->val_table[reg_val].integer;
>>> + vals[1] = afe440x_attr->val_table[reg_val].fract;
>>> +
>>> + return iio_format_value(buf, IIO_VAL_INT_PLUS_MICRO, 2, vals);
>>> }
>>>
>>> static ssize_t afe440x_store_register(struct device *dev,
>>> @@ -227,22 +213,12 @@ static ssize_t afe440x_store_register(struct
>device *dev,
>>> if (ret)
>>> return ret;
>>>
>>> - switch (afe440x_attr->type) {
>>> - case SIMPLE:
>>> - val = integer;
>>> - break;
>>> - case RESISTANCE:
>>> - case CAPACITANCE:
>>> - for (val = 0; val < afe440x_attr->table_size; val++)
>>> - if (afe440x_attr->val_table[val].integer == integer &&
>>> - afe440x_attr->val_table[val].fract == fract)
>>> - break;
>>> - if (val == afe440x_attr->table_size)
>>> - return -EINVAL;
>>> - break;
>>> - default:
>>> + for (val = 0; val < afe440x_attr->table_size; val++)
>>> + if (afe440x_attr->val_table[val].integer == integer &&
>>> + afe440x_attr->val_table[val].fract == fract)
>>> + break;
>>> + if (val == afe440x_attr->table_size)
>>> return -EINVAL;
>>> - }
>>>
>>> ret = regmap_update_bits(afe->regmap, afe440x_attr->reg,
>>> afe440x_attr->mask,
>>> @@ -253,16 +229,13 @@ static ssize_t afe440x_store_register(struct
>device *dev,
>>> return count;
>>> }
>>>
>>> -static AFE440X_ATTR(tia_separate_en, AFE4403_TIAGAIN,
>AFE440X_TIAGAIN_ENSEPGAIN, SIMPLE, NULL, 0);
>>> -
>>> -static AFE440X_ATTR(tia_resistance1, AFE4403_TIAGAIN,
>AFE4403_TIAGAIN_RES, RESISTANCE, afe4403_res_table,
>ARRAY_SIZE(afe4403_res_table));
>>> -static AFE440X_ATTR(tia_capacitance1, AFE4403_TIAGAIN,
>AFE4403_TIAGAIN_CAP, CAPACITANCE, afe4403_cap_table,
>ARRAY_SIZE(afe4403_cap_table));
>>> +static AFE440X_ATTR(tia_resistance1, AFE4403_TIAGAIN,
>AFE4403_TIAGAIN_RES, afe4403_res_table);
>>> +static AFE440X_ATTR(tia_capacitance1, AFE4403_TIAGAIN,
>AFE4403_TIAGAIN_CAP, afe4403_cap_table);
>>>
>>> -static AFE440X_ATTR(tia_resistance2, AFE4403_TIA_AMB_GAIN,
>AFE4403_TIAGAIN_RES, RESISTANCE, afe4403_res_table,
>ARRAY_SIZE(afe4403_res_table));
>>> -static AFE440X_ATTR(tia_capacitance2, AFE4403_TIA_AMB_GAIN,
>AFE4403_TIAGAIN_RES, CAPACITANCE, afe4403_cap_table,
>ARRAY_SIZE(afe4403_cap_table));
>>> +static AFE440X_ATTR(tia_resistance2, AFE4403_TIA_AMB_GAIN,
>AFE4403_TIAGAIN_RES, afe4403_res_table);
>>> +static AFE440X_ATTR(tia_capacitance2, AFE4403_TIA_AMB_GAIN,
>AFE4403_TIAGAIN_RES, afe4403_cap_table);
>>>
>>> static struct attribute *afe440x_attributes[] = {
>>> - &afe440x_attr_tia_separate_en.dev_attr.attr,
>>> &afe440x_attr_tia_resistance1.dev_attr.attr,
>>> &afe440x_attr_tia_capacitance1.dev_attr.attr,
>>> &afe440x_attr_tia_resistance2.dev_attr.attr,
>>> @@ -473,6 +446,7 @@ static const struct iio_trigger_ops
>afe4403_trigger_ops = {
>>> static const struct reg_sequence afe4403_reg_sequences[] = {
>>> AFE4403_TIMING_PAIRS,
>>> { AFE440X_CONTROL1, AFE440X_CONTROL1_TIMEREN },
>>> + { AFE4403_TIAGAIN, AFE440X_TIAGAIN_ENSEPGAIN },
>>> };
>>>
>>> static const struct regmap_range afe4403_yes_ranges[] = {
>>> diff --git a/drivers/iio/health/afe4404.c
>b/drivers/iio/health/afe4404.c
>>> index 2d4c522..b9c1666 100644
>>> --- a/drivers/iio/health/afe4404.c
>>> +++ b/drivers/iio/health/afe4404.c
>>> @@ -193,9 +193,9 @@ static ssize_t afe440x_show_register(struct
>device *dev,
>>> struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> struct afe4404_data *afe = iio_priv(indio_dev);
>>> struct afe440x_attr *afe440x_attr = to_afe440x_attr(attr);
>>> - unsigned int reg_val, type;
>>> + unsigned int reg_val;
>>> int vals[2];
>>> - int ret, val_len;
>>> + int ret;
>>>
>>> ret = regmap_read(afe->regmap, afe440x_attr->reg, ®_val);
>>> if (ret)
>>> @@ -204,27 +204,13 @@ static ssize_t afe440x_show_register(struct
>device *dev,
>>> reg_val &= afe440x_attr->mask;
>>> reg_val >>= afe440x_attr->shift;
>>>
>>> - switch (afe440x_attr->type) {
>>> - case SIMPLE:
>>> - type = IIO_VAL_INT;
>>> - val_len = 1;
>>> - vals[0] = reg_val;
>>> - break;
>>> - case RESISTANCE:
>>> - case CAPACITANCE:
>>> - type = IIO_VAL_INT_PLUS_MICRO;
>>> - val_len = 2;
>>> - if (reg_val < afe440x_attr->table_size) {
>>> - vals[0] = afe440x_attr->val_table[reg_val].integer;
>>> - vals[1] = afe440x_attr->val_table[reg_val].fract;
>>> - break;
>>> - }
>>> + if (reg_val >= afe440x_attr->table_size)
>>> return -EINVAL;
>>> - default:
>>> - return -EINVAL;
>>> - }
>>>
>>> - return iio_format_value(buf, type, val_len, vals);
>>> + vals[0] = afe440x_attr->val_table[reg_val].integer;
>>> + vals[1] = afe440x_attr->val_table[reg_val].fract;
>>> +
>>> + return iio_format_value(buf, IIO_VAL_INT_PLUS_MICRO, 2, vals);
>>> }
>>>
>>> static ssize_t afe440x_store_register(struct device *dev,
>>> @@ -240,22 +226,12 @@ static ssize_t afe440x_store_register(struct
>device *dev,
>>> if (ret)
>>> return ret;
>>>
>>> - switch (afe440x_attr->type) {
>>> - case SIMPLE:
>>> - val = integer;
>>> - break;
>>> - case RESISTANCE:
>>> - case CAPACITANCE:
>>> - for (val = 0; val < afe440x_attr->table_size; val++)
>>> - if (afe440x_attr->val_table[val].integer == integer &&
>>> - afe440x_attr->val_table[val].fract == fract)
>>> - break;
>>> - if (val == afe440x_attr->table_size)
>>> - return -EINVAL;
>>> - break;
>>> - default:
>>> + for (val = 0; val < afe440x_attr->table_size; val++)
>>> + if (afe440x_attr->val_table[val].integer == integer &&
>>> + afe440x_attr->val_table[val].fract == fract)
>>> + break;
>>> + if (val == afe440x_attr->table_size)
>>> return -EINVAL;
>>> - }
>>>
>>> ret = regmap_update_bits(afe->regmap, afe440x_attr->reg,
>>> afe440x_attr->mask,
>>> @@ -266,16 +242,13 @@ static ssize_t afe440x_store_register(struct
>device *dev,
>>> return count;
>>> }
>>>
>>> -static AFE440X_ATTR(tia_separate_en, AFE4404_TIA_GAIN_SEP,
>AFE440X_TIAGAIN_ENSEPGAIN, SIMPLE, NULL, 0);
>>> -
>>> -static AFE440X_ATTR(tia_resistance1, AFE4404_TIA_GAIN,
>AFE4404_TIA_GAIN_RES, RESISTANCE, afe4404_res_table,
>ARRAY_SIZE(afe4404_res_table));
>>> -static AFE440X_ATTR(tia_capacitance1, AFE4404_TIA_GAIN,
>AFE4404_TIA_GAIN_CAP, CAPACITANCE, afe4404_cap_table,
>ARRAY_SIZE(afe4404_cap_table));
>>> +static AFE440X_ATTR(tia_resistance1, AFE4404_TIA_GAIN,
>AFE4404_TIA_GAIN_RES, afe4404_res_table);
>>> +static AFE440X_ATTR(tia_capacitance1, AFE4404_TIA_GAIN,
>AFE4404_TIA_GAIN_CAP, afe4404_cap_table);
>>>
>>> -static AFE440X_ATTR(tia_resistance2, AFE4404_TIA_GAIN_SEP,
>AFE4404_TIA_GAIN_RES, RESISTANCE, afe4404_res_table,
>ARRAY_SIZE(afe4404_res_table));
>>> -static AFE440X_ATTR(tia_capacitance2, AFE4404_TIA_GAIN_SEP,
>AFE4404_TIA_GAIN_CAP, CAPACITANCE, afe4404_cap_table,
>ARRAY_SIZE(afe4404_cap_table));
>>> +static AFE440X_ATTR(tia_resistance2, AFE4404_TIA_GAIN_SEP,
>AFE4404_TIA_GAIN_RES, afe4404_res_table);
>>> +static AFE440X_ATTR(tia_capacitance2, AFE4404_TIA_GAIN_SEP,
>AFE4404_TIA_GAIN_CAP, afe4404_cap_table);
>>>
>>> static struct attribute *afe440x_attributes[] = {
>>> - &afe440x_attr_tia_separate_en.dev_attr.attr,
>>> &afe440x_attr_tia_resistance1.dev_attr.attr,
>>> &afe440x_attr_tia_capacitance1.dev_attr.attr,
>>> &afe440x_attr_tia_resistance2.dev_attr.attr,
>>> @@ -443,6 +416,7 @@ static const struct iio_trigger_ops
>afe4404_trigger_ops = {
>>> static const struct reg_sequence afe4404_reg_sequences[] = {
>>> AFE4404_TIMING_PAIRS,
>>> { AFE440X_CONTROL1, AFE440X_CONTROL1_TIMEREN },
>>> + { AFE4404_TIA_GAIN_SEP, AFE440X_TIAGAIN_ENSEPGAIN },
>>> { AFE440X_CONTROL2, AFE440X_CONTROL3_OSC_ENABLE },
>>> };
>>>
>>> diff --git a/drivers/iio/health/afe440x.h
>b/drivers/iio/health/afe440x.h
>>> index c671ab7..544bbab 100644
>>> --- a/drivers/iio/health/afe440x.h
>>> +++ b/drivers/iio/health/afe440x.h
>>> @@ -71,8 +71,7 @@
>>> #define AFE440X_CONTROL1_TIMEREN BIT(8)
>>>
>>> /* TIAGAIN register fields */
>>> -#define AFE440X_TIAGAIN_ENSEPGAIN_MASK BIT(15)
>>> -#define AFE440X_TIAGAIN_ENSEPGAIN_SHIFT 15
>>> +#define AFE440X_TIAGAIN_ENSEPGAIN BIT(15)
>>>
>>> /* CONTROL2 register fields */
>>> #define AFE440X_CONTROL2_PDN_AFE BIT(0)
>>> @@ -133,12 +132,6 @@ struct afe440x_reg_info {
>>> .output = true, \
>>> }
>>>
>>> -enum afe440x_reg_type {
>>> - SIMPLE,
>>> - RESISTANCE,
>>> - CAPACITANCE,
>>> -};
>>> -
>>> struct afe440x_val_table {
>>> int integer;
>>> int fract;
>>> @@ -167,7 +160,6 @@ struct afe440x_attr {
>>> unsigned int reg;
>>> unsigned int shift;
>>> unsigned int mask;
>>> - enum afe440x_reg_type type;
>>> const struct afe440x_val_table *val_table;
>>> unsigned int table_size;
>>> };
>>> @@ -175,7 +167,7 @@ struct afe440x_attr {
>>> #define to_afe440x_attr(_dev_attr) \
>>> container_of(_dev_attr, struct afe440x_attr, dev_attr)
>>>
>>> -#define AFE440X_ATTR(_name, _reg, _field, _type, _table, _size) \
>>> +#define AFE440X_ATTR(_name, _reg, _field, _table) \
>>> struct afe440x_attr afe440x_attr_##_name = { \
>>> .dev_attr = __ATTR(_name, (S_IRUGO | S_IWUSR), \
>>> afe440x_show_register, \
>>> @@ -183,9 +175,8 @@ struct afe440x_attr {
>>> .reg = _reg, \
>>> .shift = _field ## _SHIFT, \
>>> .mask = _field ## _MASK, \
>>> - .type = _type, \
>>> .val_table = _table, \
>>> - .table_size = _size, \
>>> + .table_size = ARRAY_SIZE(_table), \
>>> }
>>>
>>> #endif /* _AFE440X_H */
>>>
>>
>--
>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
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread
[PATCH 00/13] Rework for AFE440x drivers to prepare for AFE4405 "Andrew F. Davis" <afd@ti.com> - 2016-05-01 22:40 +0200
[PATCH 10/13] iio: health/afe440x: Make gain settings a modifier for the stages "Andrew F. Davis" <afd@ti.com> - 2016-05-01 22:40 +0200
Re: [PATCH 10/13] iio: health/afe440x: Make gain settings a modifier for the stages Jonathan Cameron <jic23@kernel.org> - 2016-05-04 16:50 +0200
[PATCH 01/13] iio: health/afe440x: Fix kernel-doc format "Andrew F. Davis" <afd@ti.com> - 2016-05-01 22:40 +0200
Re: [PATCH 01/13] iio: health/afe440x: Fix kernel-doc format Jonathan Cameron <jic23@kernel.org> - 2016-05-04 16:50 +0200
[PATCH 02/13] iio: health/afe440x: Remove of_match_ptr and ifdefs "Andrew F. Davis" <afd@ti.com> - 2016-05-01 22:40 +0200
Re: [PATCH 02/13] iio: health/afe440x: Remove of_match_ptr and ifdefs Jonathan Cameron <jic23@kernel.org> - 2016-05-04 16:50 +0200
[PATCH 13/13] iio: health/afe4404: ENSEPGAIN is part of CONTROL2 register "Andrew F. Davis" <afd@ti.com> - 2016-05-01 22:40 +0200
Re: [PATCH 13/13] iio: health/afe4404: ENSEPGAIN is part of CONTROL2 register Jonathan Cameron <jic23@kernel.org> - 2016-05-04 16:50 +0200
[PATCH 07/13] iio: health/afe4404: Remove LED3 input channel "Andrew F. Davis" <afd@ti.com> - 2016-05-01 22:40 +0200
Re: [PATCH 07/13] iio: health/afe4404: Remove LED3 input channel Jonathan Cameron <jic23@kernel.org> - 2016-05-04 16:50 +0200
[PATCH 09/13] iio: health/afe440x: Use regmap fields "Andrew F. Davis" <afd@ti.com> - 2016-05-01 22:40 +0200
Re: [PATCH 09/13] iio: health/afe440x: Use regmap fields Jonathan Cameron <jic23@kernel.org> - 2016-05-04 16:50 +0200
Re: [PATCH 09/13] iio: health/afe440x: Use regmap fields "Andrew F. Davis" <afd@ti.com> - 2016-05-04 17:40 +0200
[PATCH 04/13] iio: health/afe440x: Always use separate gain values "Andrew F. Davis" <afd@ti.com> - 2016-05-01 22:50 +0200
Re: [PATCH 04/13] iio: health/afe440x: Always use separate gain values Jonathan Cameron <jic23@kernel.org> - 2016-05-04 16:50 +0200
Re: [PATCH 04/13] iio: health/afe440x: Always use separate gain values "Andrew F. Davis" <afd@ti.com> - 2016-05-04 17:20 +0200
Re: [PATCH 04/13] iio: health/afe440x: Always use separate gain values Jonathan Cameron <jic23@jic23.retrosnub.co.uk> - 2016-05-04 20:40 +0200
[PATCH 11/13] iio: health/afe440x: Match LED currents to stages "Andrew F. Davis" <afd@ti.com> - 2016-05-01 22:50 +0200
[PATCH 12/13] iio: health/afe440x: Remove unused definitions "Andrew F. Davis" <afd@ti.com> - 2016-05-01 22:50 +0200
Re: [PATCH 12/13] iio: health/afe440x: Remove unused definitions Jonathan Cameron <jic23@kernel.org> - 2016-05-04 16:50 +0200
[PATCH 08/13] iio: health/afe440x: Remove channel names "Andrew F. Davis" <afd@ti.com> - 2016-05-01 22:50 +0200
Re: [PATCH 08/13] iio: health/afe440x: Remove channel names Jonathan Cameron <jic23@kernel.org> - 2016-05-04 16:50 +0200
Re: [PATCH 08/13] iio: health/afe440x: Remove channel names "Andrew F. Davis" <afd@ti.com> - 2016-05-04 17:30 +0200
Re: [PATCH 00/13] Rework for AFE440x drivers to prepare for AFE4405 Jonathan Cameron <jic23@kernel.org> - 2016-05-04 16:50 +0200
csiph-web