[PATCH] staging: (iio) restore macro IIO_ATTR_2

[PATCH] staging: (iio) restore macro IIO_ATTR_2

[Vivien Didelot]

For some reason, IIO_ATTR_2 has been deleted by commit
99e5dc45b854b4b661044e807905152911ed3fdb but it is still used by the macro
IIO_DEVICE_ATTR_2. This patch restores it.
Please ignore it if it shouldn't exist anymore. If so, this would need to
remove IIO_DEVICE_ATTR_2 as well. Obviously it seems to be useful as the struct
iio_dev_attr has a val2 member.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/staging/iio/sysfs.h |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/iio/sysfs.h b/drivers/staging/iio/sysfs.h
index dd79b58..cfc0965 100644
--- a/drivers/staging/iio/sysfs.h
+++ b/drivers/staging/iio/sysfs.h
@@ -56,6 +56,11 @@ struct iio_const_attr {
 	{ .dev_attr = __ATTR(_name, _mode, _show, _store),	\
 	  .address = _addr }
 
+#define IIO_ATTR_2(_name, _mode, _show, _store, _addr, _val2)	\
+	{ .dev_attr = __ATTR(_name, _mode, _show, _store),	\
+	  .address = _addr,					\
+	  .val2 = _val2 }
+
 #define IIO_DEVICE_ATTR(_name, _mode, _show, _store, _addr)	\
 	struct iio_dev_attr iio_dev_attr_##_name		\
 	= IIO_ATTR(_name, _mode, _show, _store, _addr)
-- 
1.7.6

Re: [PATCH] staging: (iio) restore macro IIO_ATTR_2

[Jonathan Cameron]

On 07/22/11 23:26, Vivien Didelot wrote:
> For some reason, IIO_ATTR_2 has been deleted by commit
> 99e5dc45b854b4b661044e807905152911ed3fdb but it is still used by the macro
> IIO_DEVICE_ATTR_2. This patch restores it.
> Please ignore it if it shouldn't exist anymore. If so, this would need to
> remove IIO_DEVICE_ATTR_2 as well. Obviously it seems to be useful as the struct
> iio_dev_attr has a val2 member.
Good spot.  Do you have a user for the two parameter version?  If not
I'd prefer to just scrap the IIO_DEVICE_ATTR_2 until we actually need it.
That also means we can scrap val2 from struct iio_dev_attr which is a nice
little clean up.  How about the following?

Looking at it this structure could put address and the chan_spec
pointer in the same memory as they are never used together.
Different issue though so that can be in a later patch.

Thanks,

Jonathan


Subject: [PATCH] staging:iio: sysfs.h remove unused val2 and dead macro.

Vivien pointed out that 99e5dc45b854b4b661044e807905152911ed3fdb
removed the IIO_DEVICE_ATTR_2 macro but didn't clean up the other
macros that used it.

As it turns out, no one is using val2 in tree, so lets scrap that
until it is needed.

Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
Reported-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/staging/iio/sysfs.h |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/iio/sysfs.h b/drivers/staging/iio/sysfs.h
index cecc7ec..9e0b67c 100644
--- a/drivers/staging/iio/sysfs.h
+++ b/drivers/staging/iio/sysfs.h
@@ -18,13 +18,11 @@
  * struct iio_dev_attr - iio specific device attribute
  * @dev_attr:	underlying device attribute
  * @address:	associated register address
- * @val2:	secondary attribute value
  * @l:		list head for maintaining list of dynamically created attrs.
  */
 struct iio_dev_attr {
 	struct device_attribute dev_attr;
 	int address;
-	int val2;
 	struct list_head l;
 	struct iio_chan_spec const *c;
 };
@@ -64,10 +62,6 @@ struct iio_const_attr {
 	struct iio_dev_attr iio_dev_attr_##_vname			\
 	= IIO_ATTR(_name, _mode, _show, _store, _addr)
 
-#define IIO_DEVICE_ATTR_2(_name, _mode, _show, _store, _addr, _val2)	\
-	struct iio_dev_attr iio_dev_attr_##_name			\
-	= IIO_ATTR_2(_name, _mode, _show, _store, _addr, _val2)
-
 #define IIO_CONST_ATTR(_name, _string)					\
 	struct iio_const_attr iio_const_attr_##_name			\
 	= { .string = _string,						\
-- 
1.7.3.4

Re: [PATCH] staging: (iio) restore macro IIO_ATTR_2

[Vivien Didelot]

On Mon, 25 Jul 2011 12:30:41 +0100,
Jonathan Cameron <jic23@cam.ac.uk> wrote:

> On 07/22/11 23:26, Vivien Didelot wrote:
> > For some reason, IIO_ATTR_2 has been deleted by commit
> > 99e5dc45b854b4b661044e807905152911ed3fdb but it is still used by
> > the macro IIO_DEVICE_ATTR_2. This patch restores it.
> > Please ignore it if it shouldn't exist anymore. If so, this would
> > need to remove IIO_DEVICE_ATTR_2 as well. Obviously it seems to be
> > useful as the struct iio_dev_attr has a val2 member.
> Good spot.  Do you have a user for the two parameter version?  If not
> I'd prefer to just scrap the IIO_DEVICE_ATTR_2 until we actually need
> it. That also means we can scrap val2 from struct iio_dev_attr which
> is a nice little clean up.  How about the following?

I'm actually using the second member, as I'm trying to move the TS-5500
ADC driver from HWMON to IIO (you can see the HWMON patch here:
https://lkml.org/lkml/diff/2011/7/19/223/1). It is used in the
ts5500_adc_show_range() function (nr is the equivalent of val2).

But I've found the ad7291.c driver which is 8 channels as well. 
It might offer a solution that avoids using this second parameter. Is
it a good driver to refer to in this case?

Regards,
Vivien.

Re: [PATCH] staging: (iio) restore macro IIO_ATTR_2

[Jonathan Cameron]

On 07/25/11 16:51, Vivien Didelot wrote:
> On Mon, 25 Jul 2011 12:30:41 +0100,
> Jonathan Cameron <jic23@cam.ac.uk> wrote:
> 
>> On 07/22/11 23:26, Vivien Didelot wrote:
>>> For some reason, IIO_ATTR_2 has been deleted by commit
>>> 99e5dc45b854b4b661044e807905152911ed3fdb but it is still used by
>>> the macro IIO_DEVICE_ATTR_2. This patch restores it.
>>> Please ignore it if it shouldn't exist anymore. If so, this would
>>> need to remove IIO_DEVICE_ATTR_2 as well. Obviously it seems to be
>>> useful as the struct iio_dev_attr has a val2 member.
>> Good spot.  Do you have a user for the two parameter version?  If not
>> I'd prefer to just scrap the IIO_DEVICE_ATTR_2 until we actually need
>> it. That also means we can scrap val2 from struct iio_dev_attr which
>> is a nice little clean up.  How about the following?
> 
> I'm actually using the second member, as I'm trying to move the TS-5500
> ADC driver from HWMON to IIO (you can see the HWMON patch here:
> https://lkml.org/lkml/diff/2011/7/19/223/1). It is used in the
> ts5500_adc_show_range() function (nr is the equivalent of val2).
Encode whether we have a min or a max in the single value along with
the channel. Or just have a separate function as it'll only cost you
a couple of lines.  ts5500_adc_range is a misleadingly named trivial
wrapper so I'd squash that into a ts5500_show_range_min and ts5500_show_range_max.

I guess we'll need to have a rethink on supporting range
directly in the iio_chan_spec structures.  People are remarkably keen on
it.  (the reason this is controversial is that it interacts with scale
which is a necessary parameter for interpreting raw values anyway.)
We let it into the abi so should probably make it easy to use.. hmm.

> 
> But I've found the ad7291.c driver which is 8 channels as well. 
> It might offer a solution that avoids using this second parameter. Is
> it a good driver to refer to in this case?
> 
> Regards,
> Vivien.
> --
> 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
> 
No.  Look at any of max1363, ad799x, ad7887 and indeed pretty much any
of the others that have undergone chan_spec conversion.

Usual principal with drivers is find one that has a lot of active development
to copy.

Michael is the ad7291 on your cleanup list? It's on another planet abi wise...

Sorry Vivien if you wasted any time copying stuff from there.
We decided just the other day to start maintaining broken out TODO files for
the drivers. Sorry we didn't have those in place already or it might have saved
you time.

Jonathan

Re: [PATCH] staging: (iio) restore macro IIO_ATTR_2

[Vivien Didelot]

On Mon, 25 Jul 2011 17:14:43 +0100,
Jonathan Cameron <jic23@cam.ac.uk> wrote :
> > But I've found the ad7291.c driver which is 8 channels as well. 
> > It might offer a solution that avoids using this second parameter.
> > Is it a good driver to refer to in this case?
> > 
> > Regards,
> > Vivien.
> No.  Look at any of max1363, ad799x, ad7887 and indeed pretty much any
> of the others that have undergone chan_spec conversion.
> 
> Usual principal with drivers is find one that has a lot of active
> development to copy.
> 
> Michael is the ad7291 on your cleanup list? It's on another planet
> abi wise...
> 
> Sorry Vivien if you wasted any time copying stuff from there.
> We decided just the other day to start maintaining broken out TODO
> files for the drivers. Sorry we didn't have those in place already or
> it might have saved you time.
> 
> Jonathan
That's ok, I understand that this new subsystem is moving fast.
By the way, that's why I've proposed an HWMON version of the TS-5500
ADC driver in the new set of patches for the TS-5500 support
(https://lkml.org/lkml/2011/7/19/220, which has no comment yet). As it
is integrated as part of this platform and functional at the moment,
those patches could be applied initially, then a future patch could
move the ADC driver from HWMON to IIO.
This discussion could continue on the TS-5500 patches request if you
prefer. What do you think?

Regards,
Vivien.

RE: [PATCH] staging: (iio) restore macro IIO_ATTR_2

[Hennerich, Michael]

Jonathan Cameron wrote on 2011-07-25:
> Michael is the ad7291 on your cleanup list? It's on another planet abi
> wise...

It should be definitely on my list. This driver requires more or less a
complete rewrite.

Greetings,
Michael

--
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368; Gesch=
aeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin, Margaret=
 Seif