On 03/30/11 15:05, Jan Weitzel wrote:
> Jonathan Cameron <jic23@xxxxxxxxx> schrieb am 30.03.2011 15:16:55:
>
>> Von: Jonathan Cameron <jic23@xxxxxxxxx>
>> An: mailinglists@xxxxxxxxx
>> Kopie: linux-iio@xxxxxxxxxxxxxxx, Jan Weitzel <j.weitzel@xxxxxxxxx>
>> Datum: 30.03.2011 15:15
>> Betreff: Re: [PATCH] staging:iio:adc Add range to max1363
>>
>> On 03/30/11 13:20, mailinglists@xxxxxxxxx wrote:
>>> From: Jan Weitzel <j.weitzel@xxxxxxxxx>
>>>
>>> by default MAX1363_SETUP_AIN3_IS_AIN3_REF_IS_VDD is set, add VDD is
>>> the voltage reference, not the internal one. So in_scale is wrong.
>>> If a regulator is available it is ask about the voltage. If it is
>>> not available the old (wrong) internal reference is used.
>>>
>> Hi Jan,
>>
>> Nice to know this driver has some users.
> :)
>>
>> Firstly there are two completely different elements to this patch, so
> they
>> should be broken out into 2 patches.
>> For the fix, perhaps the easier option is just to change the defaultto
> use the
>> internal reference? Do you have a use that requires it to be up to
> VDD?
> Ok I can fix it. But in our application we messarue above the internal
> reference and need vdd reference support. So best solution is platformcode
> for selecting the source and the its value, right?
>
>> Now you have pointed out this bug, I am rather dubious about simply
> outputting
>> vref if the regulator isn't specified. We could do this via
>> platform data I guess.
>> I wonder how much trouble it would cause to just remove all options
>> for not having
>> the reg there? After all people can just use a fixed voltage
>> regulator to specify
> thats the way I do it now ;) I didn't want to add max1363 platformcode by
> now.
Sadly it looks like we will have to.
>> what it is.
>>
>> Hmm. These range attributes are a pain. Do you have a use case
>> actually needing
> No but in_scale depents also on the "range"
Sure, but no need to provide it to userspace. What happens inside the
driver doesn't bother me. It's just a case of avoiding ambiguous
userspace interfaces if we can.
>
> Kind regards,
> Jan
>
>> that value? I'm also tempted to suggest changing the abi to at
>> least identify them with
>> a type of channel (hence in_range). Right now they aren't well specified
>> (what's the syntax for something covering 1-2V only for example?)
>>
>> Thanks,
>>
>> Jonathan
>>
>>
>>
>>> Signed-off-by: Jan Weitzel <j.weitzel@xxxxxxxxx>
>>> ---
>>> drivers/staging/iio/adc/max1363_core.c | 29
> +++++++++++++++++++++++++----
>>> 1 files changed, 25 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/staging/iio/adc/max1363_core.c b/drivers/
>> staging/iio/adc/max1363_core.c
>>> index 905f856..cf49124 100644
>>> --- a/drivers/staging/iio/adc/max1363_core.c
>>> +++ b/drivers/staging/iio/adc/max1363_core.c
>>> @@ -279,22 +279,31 @@ static IIO_DEV_ATTR_IN_DIFF_RAW(7, 6,
>> max1363_read_single_channel, d7m6);
>>> static IIO_DEV_ATTR_IN_DIFF_RAW(9, 8, max1363_read_single_channel,
> d9m8);
>>> static IIO_DEV_ATTR_IN_DIFF_RAW(11, 10,
>> max1363_read_single_channel, d11m10);
>>>
>>> +static u16 max1363_get_range(struct max1363_state *st)
>>> +{
>>> + u16 range;
>>> + if (IS_ERR(st->reg))
>>> + range = st->chip_info->int_vref_mv;
>>> + else
>>> + range = regulator_get_voltage(st->reg) / 1000;
>>> +
>>> + return range;
>>> +}
>>>
>>> static ssize_t max1363_show_scale(struct device *dev,
>>> struct device_attribute *attr,
>>> char *buf)
>>> {
>>> - /* Driver currently only support internal vref */
>>> + /* Driver currently only support vcc vref */
>>> struct iio_dev *dev_info = dev_get_drvdata(dev);
>>> struct max1363_state *st = iio_dev_get_devdata(dev_info);
>>> /* Corresponds to Vref / 2^(bits) */
>>>
>>> - if ((1 << (st->chip_info->bits + 1))
>>> - > st->chip_info->int_vref_mv)
>>> + if ((1 << (st->chip_info->bits + 1)) > max1363_get_range(st))
>>> return sprintf(buf, "0.5\n");
>>> else
>>> return sprintf(buf, "%d\n",
>>> - st->chip_info->int_vref_mv >> st->chip_info->bits);
>>> + max1363_get_range(st) >> st->chip_info->bits);
>>> }
>>>
>>> static IIO_DEVICE_ATTR(in_scale, S_IRUGO, max1363_show_scale, NULL,
> 0);
>>> @@ -310,6 +319,17 @@ static ssize_t max1363_show_name(struct device
> *dev,
>>>
>>> static IIO_DEVICE_ATTR(name, S_IRUGO, max1363_show_name, NULL, 0);
>>>
>>> +static ssize_t max1363_show_range(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf)
>>> +{
>>> + struct iio_dev *dev_info = dev_get_drvdata(dev);
>>> + struct max1363_state *st = iio_dev_get_devdata(dev_info);
>>> + return sprintf(buf, "%d\n", max1363_get_range(st));
>>> +}
>>> +
>>> +static IIO_DEVICE_ATTR(range, S_IRUGO, max1363_show_range, NULL, 0);
>>> +
>>> /* Applies to max1363 */
>>> static const enum max1363_modes max1363_mode_list[] = {
>>> _s0, _s1, _s2, _s3,
>>> @@ -329,6 +349,7 @@ static struct attribute *max1363_device_attrs[] =
> {
>>> &iio_dev_attr_in3min2_raw.dev_attr.attr,
>>> &iio_dev_attr_name.dev_attr.attr,
>>> &iio_dev_attr_in_scale.dev_attr.attr,
>>> + &iio_dev_attr_range.dev_attr.attr,
>>> NULL
>>> };
>>>
>>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
