| Message ID | 20211215151344.163036-3-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Light core IIO enhancements | expand |
On Wed, Dec 15, 2021 at 10:04 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Let's provide more details about these two variables because their > understanding may not be straightforward for someone not used to the IIO > subsystem internal logic. The different modes will soon be also be more > documented for the same reason. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > include/linux/iio/iio.h | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h > index 06433c2c2968..0312da2e83f8 100644 > --- a/include/linux/iio/iio.h > +++ b/include/linux/iio/iio.h > @@ -487,8 +487,14 @@ struct iio_buffer_setup_ops { > > /** > * struct iio_dev - industrial I/O device > - * @modes: [DRIVER] operating modes supported by device > - * @currentmode: [INTERN] current operating mode > + * @modes: [DRIVER] list of operating modes supported by the IIO I'd argue that it may make sense to highlight that this list of modes is represented as a bitmask. When reading docs, it may not be obvious at first (I guess). > + * device, this list should be initialized before > + * registering the IIO device and can be filed up by the > + * IIO core depending on the features advertised by the > + * driver during other steps of the registration > + * @currentmode: [INTERN] operating mode currently in use, may be > + * eventually checked by device drivers but should be > + * considered read-only as this is a core internal bit > * @dev: [DRIVER] device structure, should be assigned a parent > * and owner > * @buffer: [DRIVER] any buffer present > -- > 2.27.0 >
Hi Alexandru, ardeleanalex@gmail.com wrote on Thu, 16 Dec 2021 08:24:51 +0200: > On Wed, Dec 15, 2021 at 10:04 PM Miquel Raynal > <miquel.raynal@bootlin.com> wrote: > > > > Let's provide more details about these two variables because their > > understanding may not be straightforward for someone not used to the IIO > > subsystem internal logic. The different modes will soon be also be more > > documented for the same reason. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > include/linux/iio/iio.h | 10 ++++++++-- > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h > > index 06433c2c2968..0312da2e83f8 100644 > > --- a/include/linux/iio/iio.h > > +++ b/include/linux/iio/iio.h > > @@ -487,8 +487,14 @@ struct iio_buffer_setup_ops { > > > > /** > > * struct iio_dev - industrial I/O device > > - * @modes: [DRIVER] operating modes supported by device > > - * @currentmode: [INTERN] current operating mode > > + * @modes: [DRIVER] list of operating modes supported by the IIO > > I'd argue that it may make sense to highlight that this list of modes > is represented as a bitmask. > When reading docs, it may not be obvious at first (I guess). That is a good idea. I'll add this to the next iteration. > > + * device, this list should be initialized before > > + * registering the IIO device and can be filed up by the > > + * IIO core depending on the features advertised by the > > + * driver during other steps of the registration > > + * @currentmode: [INTERN] operating mode currently in use, may be > > + * eventually checked by device drivers but should be > > + * considered read-only as this is a core internal bit > > * @dev: [DRIVER] device structure, should be assigned a parent > > * and owner > > * @buffer: [DRIVER] any buffer present > > -- > > 2.27.0 > > Thanks, Miquèl
On Thu, 16 Dec 2021 09:15:52 +0100 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > Hi Alexandru, > > ardeleanalex@gmail.com wrote on Thu, 16 Dec 2021 08:24:51 +0200: > > > On Wed, Dec 15, 2021 at 10:04 PM Miquel Raynal > > <miquel.raynal@bootlin.com> wrote: > > > > > > Let's provide more details about these two variables because their > > > understanding may not be straightforward for someone not used to the IIO > > > subsystem internal logic. The different modes will soon be also be more > > > documented for the same reason. > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > --- > > > include/linux/iio/iio.h | 10 ++++++++-- > > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > > > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h > > > index 06433c2c2968..0312da2e83f8 100644 > > > --- a/include/linux/iio/iio.h > > > +++ b/include/linux/iio/iio.h > > > @@ -487,8 +487,14 @@ struct iio_buffer_setup_ops { > > > > > > /** > > > * struct iio_dev - industrial I/O device > > > - * @modes: [DRIVER] operating modes supported by device > > > - * @currentmode: [INTERN] current operating mode > > > + * @modes: [DRIVER] list of operating modes supported by the IIO > > > > I'd argue that it may make sense to highlight that this list of modes > > is represented as a bitmask. > > When reading docs, it may not be obvious at first (I guess). > > That is a good idea. I'll add this to the next iteration. Good. > > > > + * device, this list should be initialized before > > > + * registering the IIO device and can be filed up by the > > > + * IIO core depending on the features advertised by the > > > + * driver during other steps of the registration Perhaps make it a little clearer that some bits are set as a result of enabling particular features. Maybe even call out which functions do it. From what I recall, that's a very short list. > > > + * @currentmode: [INTERN] operating mode currently in use, may be > > > + * eventually checked by device drivers but should be > > > + * considered read-only as this is a core internal bit We should add an accessor function to read it. (maybe later in your series? :) > > > * @dev: [DRIVER] device structure, should be assigned a parent > > > * and owner > > > * @buffer: [DRIVER] any buffer present > > > -- > > > 2.27.0 > > > > > Thanks, > Miquèl
Hi Jonathan, jic23@kernel.org wrote on Sat, 15 Jan 2022 15:51:45 +0000: > On Thu, 16 Dec 2021 09:15:52 +0100 > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > Hi Alexandru, > > > > ardeleanalex@gmail.com wrote on Thu, 16 Dec 2021 08:24:51 +0200: > > > > > On Wed, Dec 15, 2021 at 10:04 PM Miquel Raynal > > > <miquel.raynal@bootlin.com> wrote: > > > > > > > > Let's provide more details about these two variables because their > > > > understanding may not be straightforward for someone not used to the IIO > > > > subsystem internal logic. The different modes will soon be also be more > > > > documented for the same reason. > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > --- > > > > include/linux/iio/iio.h | 10 ++++++++-- > > > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h > > > > index 06433c2c2968..0312da2e83f8 100644 > > > > --- a/include/linux/iio/iio.h > > > > +++ b/include/linux/iio/iio.h > > > > @@ -487,8 +487,14 @@ struct iio_buffer_setup_ops { > > > > > > > > /** > > > > * struct iio_dev - industrial I/O device > > > > - * @modes: [DRIVER] operating modes supported by device > > > > - * @currentmode: [INTERN] current operating mode > > > > + * @modes: [DRIVER] list of operating modes supported by the IIO > > > > > > I'd argue that it may make sense to highlight that this list of modes > > > is represented as a bitmask. > > > When reading docs, it may not be obvious at first (I guess). > > > > That is a good idea. I'll add this to the next iteration. > > Good. Done. > > > > > > > + * device, this list should be initialized before > > > > + * registering the IIO device and can be filed up by the > > > > + * IIO core depending on the features advertised by the > > > > + * driver during other steps of the registration > > Perhaps make it a little clearer that some bits are set as a result of > enabling particular features. Maybe even call out which functions do it. > From what I recall, that's a very short list. Done. > > > > > + * @currentmode: [INTERN] operating mode currently in use, may be > > > > + * eventually checked by device drivers but should be > > > > + * considered read-only as this is a core internal bit > > We should add an accessor function to read it. (maybe later in your series? :) Yup! > > > > > * @dev: [DRIVER] device structure, should be assigned a parent > > > > * and owner > > > > * @buffer: [DRIVER] any buffer present > > > > -- > > > > 2.27.0 > > > > > > > > Thanks, > > Miquèl > Thanks, Miquèl
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h index 06433c2c2968..0312da2e83f8 100644 --- a/include/linux/iio/iio.h +++ b/include/linux/iio/iio.h @@ -487,8 +487,14 @@ struct iio_buffer_setup_ops { /** * struct iio_dev - industrial I/O device - * @modes: [DRIVER] operating modes supported by device - * @currentmode: [INTERN] current operating mode + * @modes: [DRIVER] list of operating modes supported by the IIO + * device, this list should be initialized before + * registering the IIO device and can be filed up by the + * IIO core depending on the features advertised by the + * driver during other steps of the registration + * @currentmode: [INTERN] operating mode currently in use, may be + * eventually checked by device drivers but should be + * considered read-only as this is a core internal bit * @dev: [DRIVER] device structure, should be assigned a parent * and owner * @buffer: [DRIVER] any buffer present
Let's provide more details about these two variables because their understanding may not be straightforward for someone not used to the IIO subsystem internal logic. The different modes will soon be also be more documented for the same reason. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- include/linux/iio/iio.h | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)