| Message ID | 20221125083526.2422900-2-gerald.loacker@wolfvision.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | add ti tmag5273 driver | expand |
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 >
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; > > ?
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; >> >> ? >
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; > >> > >> ?
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; >>>> >>>> ? >
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?
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
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 --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
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(+)