* Re: [PATCH v2] Staging: iio: meter: ade7753: Replace IIO_DEV_ATTR_SAMP_FREQ attribute with IIO_CHAN_INFO_SAMP_FREQ handlers.
2016-10-01 22:29 ` [PATCH v2] Staging: iio: meter: ade7753: Replace IIO_DEV_ATTR_SAMP_FREQ attribute with IIO_CHAN_INFO_SAMP_FREQ handlers sandhya bankar
@ 2016-10-03 20:25 ` Jonathan Cameron
0 siblings, 0 replies; 2+ messages in thread
From: Jonathan Cameron @ 2016-10-03 20:25 UTC (permalink / raw)
To: bankarsandhya512
Cc: Lars-Peter Clausen, Michael.Hennerich, knaack.h,
Peter Meerwald-Stadler, Greg KH, outreachy-kernel, linux-iio
On 01/10/16 23:29, sandhya bankar wrote:
> On Sat, Oct 1, 2016 at 6:45 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 29/09/16 17:01, Sandhya Bankar wrote:
>>> Moved functionality from IIO_DEV_ATTR_SAMP_FREQ attribute into
>>> IIO_CHAN_INFO_SAMP_FREQ handlers. Added ade7753_read_raw() and
>>> ade7753_write_raw() to allow reading/writing the element as well.
>>>
>>> Signed-off-by: Sandhya Bankar <bankarsandhya512@gmail.com>
>> I'd forgotten this driver existed. There are a lot of similar
>> elements in here that can be cleaned up if you want to take
>> this further.
>>
>> Step 1. Convert all the attrs that are actually standard IIO abi.
>> Step 2. Start looking at the custom ABI in here and consider what
>> to do about it. (might be a job for someone familiar with the
>> hardware!)
>>
>> Anyhow, this doesn't actually work.
>>
>> Take a look at what calls the read_raw and write_raw functions
>> and how the relevant sysfs attributes are instantiated.
>>
>> Lars, I think any major ABI work on this is going to need
>> some input from Analog (or someone who actually has one of
>> these!)...
>>
>> The energy meter drivers are somewhat of an open space when
>> it comes to ABI definitions (there aren't really any yet!)
>
> Hello,
> It doesn't work because I have not defined the attribute
> IIO_CHAN_INFO* for sysfs. So, the attribute won't get exposed as a
> sysfs file. Right ?
> We typically use it as a channel attribute.
> So, in this case, Should I create a channel, or is there another
> possible way to do it ?
Got it in one. This driver doesn't have any channels
(was common in early drivers for people to 'bodge' in the same
interface and assume it was fine. Partly because it was no
where near as flexible back then, but still this one was ugly
even by those standards!)
>
> I am just curious to know that why the channel for this driver is not defined ?
> Please suggest on same.
It should be. The challenge is that this driver has a whole load
of channels of types that we don't actually have any defined
ABI for yet...
Have a read of the datasheet if you want to appreciate how fiddly
these devices are. I'm not even entirely sure what that
sampling frequency actually controls. Probably the two
front end ADC channels, but I'm not sure...
A number of such drivers ended up getting written in side Analog devices
back before they started all playing nicely. We took them into staging
on the basis we'd one day figure out what to do with them, but we haven't
quite managed it yet...
So upshot is that unless you are feeling brave, there many be lower
hanging fruits to pick elsewhere! If you are feeling brave, we will
fairly soon need someone to have hardware to test any changes...
For that we probably need input from Lars or Michael.
Thanks,
Jonathan
>
>
> Thanks,
> Sandhya
>
>
>
>
>
>
>> Jonathan
>>> ---
>>> Changes in v2:
>>> * Adding "Staging" keyword in subject line.
>>>
>>> drivers/staging/iio/meter/ade7753.c | 155 +++++++++++++++++++++---------------
>>> 1 file changed, 90 insertions(+), 65 deletions(-)
>>>
>>> diff --git a/drivers/staging/iio/meter/ade7753.c b/drivers/staging/iio/meter/ade7753.c
>>> index 4b5f05f..a2b4a18 100644
>>> --- a/drivers/staging/iio/meter/ade7753.c
>>> +++ b/drivers/staging/iio/meter/ade7753.c
>>> @@ -98,6 +98,94 @@ static int ade7753_spi_read_reg_16(struct device *dev,
>>> return 0;
>>> }
>>>
>>> +static int ade7753_read_raw_samp_freq(struct device *dev, int *val)
>>> +{
>>> + int ret;
>>> + u16 t;
>>> +
>>> + ret = ade7753_spi_read_reg_16(dev, ADE7753_MODE, &t);
>>> + if (ret)
>>> + return ret;
>>> + t = (t >> 11) & 0x3;
>>> + *val = 27900 / (1 + t);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int ade7753_write_raw_samp_freq(struct device *dev, int val)
>>> +{
>>> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> + struct ade7753_state *st = iio_priv(indio_dev);
>>> + int ret;
>>> + u16 reg, t;
>>> +
>>> + if (!val)
>>> + return -EINVAL;
>>> +
>>> + t = 27900 / val;
>>> + if (t > 0)
>>> + t--;
>>> +
>>> + if (t > 1)
>>> + st->us->max_speed_hz = ADE7753_SPI_SLOW;
>>> + else
>>> + st->us->max_speed_hz = ADE7753_SPI_FAST;
>>> +
>>> + ret = ade7753_spi_read_reg_16(dev, ADE7753_MODE, ®);
>>> + if (ret)
>>> + goto out;
>>> +
>>> + reg &= ~(3 << 11);
>>> + reg |= t << 11;
>>> +
>>> + ret = ade7753_spi_write_reg_16(dev, ADE7753_MODE, reg);
>>> +
>>> +out:
>>> + return ret;
>>> +}
>>> +
>>> +static int ade7753_write_raw(struct iio_dev *indio_dev,
>>> + struct iio_chan_spec const *chan,
>>> + int val, int val2, long mask)
>>> +{
>>> + int ret;
>>> +
>>> + switch (mask) {
>>> + case IIO_CHAN_INFO_SAMP_FREQ:
>>> + if (val2)
>>> + return -EINVAL;
>>> + mutex_lock(&indio_dev->mlock);
>>> + ret = ade7753_write_raw_samp_freq(&indio_dev->dev, val);
>>> + mutex_unlock(&indio_dev->mlock);
>>> + return ret;
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int ade7753_read_raw(struct iio_dev *indio_dev,
>>> + struct iio_chan_spec const *chan,
>>> + int *val,
>>> + int *val2,
>>> + long mask)
>>> +{
>>> + int ret;
>>> +
>>> + switch (mask) {
>>> + case IIO_CHAN_INFO_SAMP_FREQ:
>>> + mutex_lock(&indio_dev->mlock);
>>> + ret = ade7753_read_raw_samp_freq(&indio_dev->dev, val);
>>> + mutex_unlock(&indio_dev->mlock);
>>> + return ret;
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> +
>>> + return ret;
>>> +}
>>> +
>>> static int ade7753_spi_read_reg_24(struct device *dev,
>>> u8 reg_address,
>>> u32 *val)
>>> @@ -383,82 +471,17 @@ err_ret:
>>> return ret;
>>> }
>>>
>>> -static ssize_t ade7753_read_frequency(struct device *dev,
>>> - struct device_attribute *attr,
>>> - char *buf)
>>> -{
>>> - int ret;
>>> - u16 t;
>>> - int sps;
>>> -
>>> - ret = ade7753_spi_read_reg_16(dev, ADE7753_MODE, &t);
>>> - if (ret)
>>> - return ret;
>>> -
>>> - t = (t >> 11) & 0x3;
>>> - sps = 27900 / (1 + t);
>>> -
>>> - return sprintf(buf, "%d\n", sps);
>>> -}
>>> -
>>> -static ssize_t ade7753_write_frequency(struct device *dev,
>>> - struct device_attribute *attr,
>>> - const char *buf,
>>> - size_t len)
>>> -{
>>> - struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> - struct ade7753_state *st = iio_priv(indio_dev);
>>> - u16 val;
>>> - int ret;
>>> - u16 reg, t;
>>> -
>>> - ret = kstrtou16(buf, 10, &val);
>>> - if (ret)
>>> - return ret;
>>> - if (!val)
>>> - return -EINVAL;
>>> -
>>> - mutex_lock(&indio_dev->mlock);
>>> -
>>> - t = 27900 / val;
>>> - if (t > 0)
>>> - t--;
>>> -
>>> - if (t > 1)
>>> - st->us->max_speed_hz = ADE7753_SPI_SLOW;
>>> - else
>>> - st->us->max_speed_hz = ADE7753_SPI_FAST;
>>> -
>>> - ret = ade7753_spi_read_reg_16(dev, ADE7753_MODE, ®);
>>> - if (ret)
>>> - goto out;
>>> -
>>> - reg &= ~(3 << 11);
>>> - reg |= t << 11;
>>> -
>>> - ret = ade7753_spi_write_reg_16(dev, ADE7753_MODE, reg);
>>> -
>>> -out:
>>> - mutex_unlock(&indio_dev->mlock);
>>> -
>>> - return ret ? ret : len;
>>> -}
>>>
>>> static IIO_DEV_ATTR_TEMP_RAW(ade7753_read_8bit);
>>> static IIO_CONST_ATTR(in_temp_offset, "-25 C");
>>> static IIO_CONST_ATTR(in_temp_scale, "0.67 C");
>>>
>>> -static IIO_DEV_ATTR_SAMP_FREQ(S_IWUSR | S_IRUGO,
>>> - ade7753_read_frequency,
>>> - ade7753_write_frequency);
>>> -
>>> static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("27900 14000 7000 3500");
>>>
>>> static struct attribute *ade7753_attributes[] = {
>>> &iio_dev_attr_in_temp_raw.dev_attr.attr,
>>> &iio_const_attr_in_temp_offset.dev_attr.attr,
>>> &iio_const_attr_in_temp_scale.dev_attr.attr,
>>> - &iio_dev_attr_sampling_frequency.dev_attr.attr,
>>> &iio_const_attr_sampling_frequency_available.dev_attr.attr,
>>> &iio_dev_attr_phcal.dev_attr.attr,
>>> &iio_dev_attr_cfden.dev_attr.attr,
>>> @@ -496,6 +519,8 @@ static const struct attribute_group ade7753_attribute_group = {
>>>
>>> static const struct iio_info ade7753_info = {
>>> .attrs = &ade7753_attribute_group,
>>> + .read_raw = &ade7753_read_raw,
>>> + .write_raw = &ade7753_write_raw,
>>> .driver_module = THIS_MODULE,
>>> };
>>>
>>>
>>
> --
> 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
>
^ permalink raw reply [flat|nested] 2+ messages in thread