diff mbox series

[v3,1/3] iio: add struct declarations for iio types

Message ID 20221125083526.2422900-2-gerald.loacker@wolfvision.net
State New
Headers
Series add ti tmag5273 driver |

Commit Message

Gerald Loacker Nov. 25, 2022, 8:35 a.m. UTC 
  Add structs for iio type arrays such as IIO_AVAIL_LIST which can be used
instead of int arrays.

Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
---
 include/linux/iio/iio.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Comments

Andy Shevchenko Nov. 25, 2022, 10:45 a.m. UTC | #1 
On Fri, Nov 25, 2022 at 09:35:24AM +0100, Gerald Loacker wrote:
> Add structs for iio type arrays such as IIO_AVAIL_LIST which can be used
> instead of int arrays.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

And thank you for doing this!

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
(one comment below)

> Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
> ---
>  include/linux/iio/iio.h | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index f0ec8a5e5a7a..eaf6727445a6 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -383,6 +383,21 @@ s64 iio_get_time_ns(const struct iio_dev *indio_dev);
>  
>  struct iio_trigger; /* forward declaration */
>  
> +struct iio_val_int_plus_micro {
> +	int val_int;
> +	int val_micro;
> +};
> +
> +struct iio_val_int_plus_nano {
> +	int val_int;
> +	int val_nano;
> +};
> +
> +struct iio_val_int_plus_micro_db {
> +	int val_int;

	int val_int_db; ?

> +	int val_micro_db;
> +};

Actually why can't we simply do

typedef iio_val_int_plus_micro_db iio_val_int_plus_micro;

?

>  /**
>   * struct iio_info - constant information about device
>   * @event_attrs:	event control attributes
> -- 
> 2.37.2
>
Andy Shevchenko Nov. 25, 2022, 11:01 a.m. UTC | #2 
On Fri, Nov 25, 2022 at 12:45:06PM +0200, Andy Shevchenko wrote:
> On Fri, Nov 25, 2022 at 09:35:24AM +0100, Gerald Loacker wrote:

...

> > +struct iio_val_int_plus_micro {
> > +	int val_int;
> > +	int val_micro;
> > +};

Thinking more about naming, why not drop val_ completely?

	int integer;
	int micro;

?

> > +struct iio_val_int_plus_nano {
> > +	int val_int;
> > +	int val_nano;
> > +};
> > +
> > +struct iio_val_int_plus_micro_db {
> > +	int val_int;
> 
> 	int val_int_db; ?
> 
> > +	int val_micro_db;
> > +};
> 
> Actually why can't we simply do
> 
> typedef iio_val_int_plus_micro_db iio_val_int_plus_micro;
> 
> ?
Gerald Loacker Nov. 28, 2022, 12:18 p.m. UTC | #3 
Am 25.11.2022 um 12:01 schrieb Andy Shevchenko:
> On Fri, Nov 25, 2022 at 12:45:06PM +0200, Andy Shevchenko wrote:
>> On Fri, Nov 25, 2022 at 09:35:24AM +0100, Gerald Loacker wrote:
> 
> ...
> 
>>> +struct iio_val_int_plus_micro {
>>> +	int val_int;
>>> +	int val_micro;
>>> +};
> 
> Thinking more about naming, why not drop val_ completely?
> 
> 	int integer;
> 	int micro;
> 
> ?
> 

Yes, this sounds good to me. I think of adding only

	typedef struct {
		int integer;
		int micro;
	} iio_val_int_plus_micro;

for now, and one can add similar structures when needed, like

	typedef struct {
		int integer;
		int nano;
	} iio_val_int_plus_nano;

or
	
	typedef iio_val_int_plus_micro iio_val_int_plus_micro_db;

If you think it's better to add them all, I can do that, of course.

>>> +struct iio_val_int_plus_nano {
>>> +	int val_int;
>>> +	int val_nano;
>>> +};
>>> +
>>> +struct iio_val_int_plus_micro_db {
>>> +	int val_int;
>>
>> 	int val_int_db; ?
>>
>>> +	int val_micro_db;
>>> +};
>>
>> Actually why can't we simply do
>>
>> typedef iio_val_int_plus_micro_db iio_val_int_plus_micro;
>>
>> ?
>
Andy Shevchenko Nov. 28, 2022, 1:27 p.m. UTC | #4 
On Mon, Nov 28, 2022 at 01:18:04PM +0100, Gerald Loacker wrote:
> Am 25.11.2022 um 12:01 schrieb Andy Shevchenko:
> > On Fri, Nov 25, 2022 at 12:45:06PM +0200, Andy Shevchenko wrote:
> >> On Fri, Nov 25, 2022 at 09:35:24AM +0100, Gerald Loacker wrote:

...

> >>> +struct iio_val_int_plus_micro {
> >>> +	int val_int;
> >>> +	int val_micro;
> >>> +};
> > 
> > Thinking more about naming, why not drop val_ completely?
> > 
> > 	int integer;
> > 	int micro;
> > 
> > ?
> 
> Yes, this sounds good to me. I think of adding only
> 
> 	typedef struct {
> 		int integer;
> 		int micro;
> 	} iio_val_int_plus_micro;
> 
> for now, and one can add similar structures when needed, like
> 
> 	typedef struct {
> 		int integer;
> 		int nano;
> 	} iio_val_int_plus_nano;

It's a rule to use _t for typedef:s in the kernel. That's why
I suggested to leave struct definition and only typedef the same structures
(existing) to new names (if needed).

> or

> 	typedef iio_val_int_plus_micro iio_val_int_plus_micro_db;

This is better as explained above.

> If you think it's better to add them all, I can do that, of course.
> 
> >>> +struct iio_val_int_plus_nano {
> >>> +	int val_int;
> >>> +	int val_nano;
> >>> +};
> >>> +
> >>> +struct iio_val_int_plus_micro_db {
> >>> +	int val_int;
> >>
> >> 	int val_int_db; ?
> >>
> >>> +	int val_micro_db;
> >>> +};
> >>
> >> Actually why can't we simply do
> >>
> >> typedef iio_val_int_plus_micro_db iio_val_int_plus_micro;
> >>
> >> ?
Michael Riesch Nov. 28, 2022, 1:48 p.m. UTC | #5 
Hi Gerald, Andy,

On 11/28/22 14:27, Andy Shevchenko wrote:
> On Mon, Nov 28, 2022 at 01:18:04PM +0100, Gerald Loacker wrote:
>> Am 25.11.2022 um 12:01 schrieb Andy Shevchenko:
>>> On Fri, Nov 25, 2022 at 12:45:06PM +0200, Andy Shevchenko wrote:
>>>> On Fri, Nov 25, 2022 at 09:35:24AM +0100, Gerald Loacker wrote:
> 
> ...
> 
>>>>> +struct iio_val_int_plus_micro {
>>>>> +	int val_int;
>>>>> +	int val_micro;
>>>>> +};
>>>
>>> Thinking more about naming, why not drop val_ completely?
>>>
>>> 	int integer;
>>> 	int micro;
>>>
>>> ?
>>
>> Yes, this sounds good to me. I think of adding only
>>
>> 	typedef struct {
>> 		int integer;
>> 		int micro;
>> 	} iio_val_int_plus_micro;

I think we actually want

struct iio_val_int_plus_micro {
	int integer;
	int micro;
};

here, right?

>> for now, and one can add similar structures when needed, like
>>
>> 	typedef struct {
>> 		int integer;
>> 		int nano;
>> 	} iio_val_int_plus_nano;

+1 for introducing things when they are actually used.

> It's a rule to use _t for typedef:s in the kernel. That's why
> I suggested to leave struct definition and only typedef the same structures
> (existing) to new names (if needed).

Andy, excuse our ignorance but we are not sure how this typedef approach
is supposed to look like...

>> or
> 
>> 	typedef iio_val_int_plus_micro iio_val_int_plus_micro_db;

... because

#include <stdio.h>

struct iio_val_int_plus_micro {
	int integer;
	int micro;
};

typedef iio_val_int_plus_micro iio_val_int_plus_micro_db;

int main()
{
  struct iio_val_int_plus_micro a = { .integer = 100, .micro = 10, };
  struct iio_val_int_plus_micro_db b = { .integer = 20, .micro = 10, };
  return 0;
}

won't compile.

> This is better as explained above.
> 
>> If you think it's better to add them all, I can do that, of course.

Anyway, seeing that only struct iio_val_int_plus_micro is used at the
moment, I believe the best path forward is to introduce only this struct
and move on.

Best regards,
Michael

>>>>> +struct iio_val_int_plus_nano {
>>>>> +	int val_int;
>>>>> +	int val_nano;
>>>>> +};
>>>>> +
>>>>> +struct iio_val_int_plus_micro_db {
>>>>> +	int val_int;
>>>>
>>>> 	int val_int_db; ?
>>>>
>>>>> +	int val_micro_db;
>>>>> +};
>>>>
>>>> Actually why can't we simply do
>>>>
>>>> typedef iio_val_int_plus_micro_db iio_val_int_plus_micro;
>>>>
>>>> ?
>
Andy Shevchenko Nov. 28, 2022, 2:05 p.m. UTC | #6 
On Mon, Nov 28, 2022 at 02:48:48PM +0100, Michael Riesch wrote:
> On 11/28/22 14:27, Andy Shevchenko wrote:
> > On Mon, Nov 28, 2022 at 01:18:04PM +0100, Gerald Loacker wrote:
> >> Am 25.11.2022 um 12:01 schrieb Andy Shevchenko:

...

> > It's a rule to use _t for typedef:s in the kernel. That's why
> > I suggested to leave struct definition and only typedef the same structures
> > (existing) to new names (if needed).
> 
> Andy, excuse our ignorance but we are not sure how this typedef approach
> is supposed to look like...
> 
> >> or
> > 
> >> 	typedef iio_val_int_plus_micro iio_val_int_plus_micro_db;
> 
> ... because
> 
> #include <stdio.h>
> 
> struct iio_val_int_plus_micro {
> 	int integer;
> 	int micro;
> };
> 
> typedef iio_val_int_plus_micro iio_val_int_plus_micro_db;
> 
> int main()
> {
>   struct iio_val_int_plus_micro a = { .integer = 100, .micro = 10, };
>   struct iio_val_int_plus_micro_db b = { .integer = 20, .micro = 10, };
>   return 0;
> }
> 
> won't compile.

I see. Thanks for pointing this out.

Then the question is why do we need the two same structures with different
names?
Michael Riesch Nov. 28, 2022, 2:26 p.m. UTC | #7 
Hi Andy,

On 11/28/22 15:05, Andy Shevchenko wrote:
> On Mon, Nov 28, 2022 at 02:48:48PM +0100, Michael Riesch wrote:
>> On 11/28/22 14:27, Andy Shevchenko wrote:
>>> On Mon, Nov 28, 2022 at 01:18:04PM +0100, Gerald Loacker wrote:
>>>> Am 25.11.2022 um 12:01 schrieb Andy Shevchenko:
> 
> ...
> 
>>> It's a rule to use _t for typedef:s in the kernel. That's why
>>> I suggested to leave struct definition and only typedef the same structures
>>> (existing) to new names (if needed).
>>
>> Andy, excuse our ignorance but we are not sure how this typedef approach
>> is supposed to look like...
>>
>>>> or
>>>
>>>> 	typedef iio_val_int_plus_micro iio_val_int_plus_micro_db;
>>
>> ... because
>>
>> #include <stdio.h>
>>
>> struct iio_val_int_plus_micro {
>> 	int integer;
>> 	int micro;
>> };
>>
>> typedef iio_val_int_plus_micro iio_val_int_plus_micro_db;
>>
>> int main()
>> {
>>   struct iio_val_int_plus_micro a = { .integer = 100, .micro = 10, };
>>   struct iio_val_int_plus_micro_db b = { .integer = 20, .micro = 10, };
>>   return 0;
>> }
>>
>> won't compile.
> 
> I see. Thanks for pointing this out.
> 
> Then the question is why do we need the two same structures with different
> names?

Most probably we don't need "struct iio_val_int_plus_micro_db" at all
since IIO_VAL_INT_PLUS_MICRO_DB and IIO_VAL_INT_PLUS_MICRO get the same
treatment in industrialio-core.c. At least it should not be introduced
in the scope of this series. In the end this is up to whoever writes the
first driver using the common data structures and IIO_VAL_INT_PLUS_MICRO_DB.

Best regards,
Michael
Jonathan Cameron Dec. 3, 2022, 5:11 p.m. UTC | #8 
On Mon, 28 Nov 2022 15:26:51 +0100
Michael Riesch <michael.riesch@wolfvision.net> wrote:

> Hi Andy,
> 
> On 11/28/22 15:05, Andy Shevchenko wrote:
> > On Mon, Nov 28, 2022 at 02:48:48PM +0100, Michael Riesch wrote:  
> >> On 11/28/22 14:27, Andy Shevchenko wrote:  
> >>> On Mon, Nov 28, 2022 at 01:18:04PM +0100, Gerald Loacker wrote:  
> >>>> Am 25.11.2022 um 12:01 schrieb Andy Shevchenko:  
> > 
> > ...
> >   
> >>> It's a rule to use _t for typedef:s in the kernel. That's why
> >>> I suggested to leave struct definition and only typedef the same structures
> >>> (existing) to new names (if needed).  
> >>
> >> Andy, excuse our ignorance but we are not sure how this typedef approach
> >> is supposed to look like...
> >>  
> >>>> or  
> >>>  
> >>>> 	typedef iio_val_int_plus_micro iio_val_int_plus_micro_db;  
> >>
> >> ... because
> >>
> >> #include <stdio.h>
> >>
> >> struct iio_val_int_plus_micro {
> >> 	int integer;
> >> 	int micro;
> >> };
> >>
> >> typedef iio_val_int_plus_micro iio_val_int_plus_micro_db;
> >>
> >> int main()
> >> {
> >>   struct iio_val_int_plus_micro a = { .integer = 100, .micro = 10, };
> >>   struct iio_val_int_plus_micro_db b = { .integer = 20, .micro = 10, };
> >>   return 0;
> >> }
> >>
> >> won't compile.  
> > 
> > I see. Thanks for pointing this out.
> > 
> > Then the question is why do we need the two same structures with different
> > names?  
> 
> Most probably we don't need "struct iio_val_int_plus_micro_db" at all
> since IIO_VAL_INT_PLUS_MICRO_DB and IIO_VAL_INT_PLUS_MICRO get the same
> treatment in industrialio-core.c. At least it should not be introduced
> in the scope of this series. In the end this is up to whoever writes the
> first driver using the common data structures and IIO_VAL_INT_PLUS_MICRO_DB.

They get same treatment today because we don't attempt to deal with
IIO_VAL_INT_PLUS_MICRO_DB in conjunction with any of the analog circuit type
front ends yet. Mind you, even though the handling in iio-rescale.c will be
different if anyone ever adds support for the DB form (I shudder at the maths
of combining this with other scale factors), I don't see the possibility meaning
we need a different structure.  

Jonathan


> 
> Best regards,
> Michael
>
diff mbox series

Patch

diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index f0ec8a5e5a7a..eaf6727445a6 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -383,6 +383,21 @@  s64 iio_get_time_ns(const struct iio_dev *indio_dev);
 
 struct iio_trigger; /* forward declaration */
 
+struct iio_val_int_plus_micro {
+	int val_int;
+	int val_micro;
+};
+
+struct iio_val_int_plus_nano {
+	int val_int;
+	int val_nano;
+};
+
+struct iio_val_int_plus_micro_db {
+	int val_int;
+	int val_micro_db;
+};
+
 /**
  * struct iio_info - constant information about device
  * @event_attrs:	event control attributes