| Message ID | 20200221114943.2056-1-alexandru.ardelean@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] iio: imu: adis: add doc-string for 'adis' struct | expand |
On Fri, 21 Feb 2020 13:49:41 +0200 Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > This change adds a doc-string for the 'adis' struct. It details the fields > and their roles. > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > --- > include/linux/iio/imu/adis.h | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h > index ac7cfd073804..0787a3aabd05 100644 > --- a/include/linux/iio/imu/adis.h > +++ b/include/linux/iio/imu/adis.h > @@ -73,6 +73,20 @@ struct adis_data { > bool has_paging; > }; > > +/** > + * struct adis - ADIS device instance data > + * @spi: Reference to SPI device which owns this ADIS IIO device > + * @trig: IIO trigger object data > + * @data: ADIS chip variant specific data > + * @burst: ADIS burst transfer information > + * @state_lock: Lock used by the device to protect state > + * @msg: SPI message object > + * @xfer: SPI transfer objects to be used for a @msg > + * @current_page: Some ADIS devices have registers, this selects current page > + * @buffer: Data buffer for information read from the device > + * @tx: Cacheline aligned TX buffer for SPI transfers > + * @rx: Cacheline aligned RX buffer for SPI transfers This last one isn't true.. > + */ > struct adis { > struct spi_device *spi; > struct iio_trigger *trig;
On Fri, 2020-02-21 at 13:11 +0000, Jonathan Cameron wrote: > On Fri, 21 Feb 2020 13:49:41 +0200 > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > > > This change adds a doc-string for the 'adis' struct. It details the fields > > and their roles. > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > --- > > include/linux/iio/imu/adis.h | 14 ++++++++++++++ > > 1 file changed, 14 insertions(+) > > > > diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h > > index ac7cfd073804..0787a3aabd05 100644 > > --- a/include/linux/iio/imu/adis.h > > +++ b/include/linux/iio/imu/adis.h > > @@ -73,6 +73,20 @@ struct adis_data { > > bool has_paging; > > }; > > > > +/** > > + * struct adis - ADIS device instance data > > + * @spi: Reference to SPI device which owns this ADIS IIO device > > + * @trig: IIO trigger object data > > + * @data: ADIS chip variant specific data > > + * @burst: ADIS burst transfer information > > + * @state_lock: Lock used by the device to protect state > > + * @msg: SPI message object > > + * @xfer: SPI transfer objects to be used for a @msg > > + * @current_page: Some ADIS devices have registers, this selects current > > page > > + * @buffer: Data buffer for information read from the device > > + * @tx: Cacheline aligned TX buffer for SPI transfers > > + * @rx: Cacheline aligned RX buffer for SPI transfers > > This last one isn't true.. Oh right. I noticed this at some point, then forgot about it. [ The "joys" of multi-tasking; sorry about it :) ] I guess I should also add a patch in this series making it cacheline-aligned. I don't see a reason why not-to [unless I am misunderstanging something] > > > + */ > > struct adis { > > struct spi_device *spi; > > struct iio_trigger *trig;
On Fri, 21 Feb 2020 15:59:35 +0000 "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote: > On Fri, 2020-02-21 at 13:11 +0000, Jonathan Cameron wrote: > > On Fri, 21 Feb 2020 13:49:41 +0200 > > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > > > > > This change adds a doc-string for the 'adis' struct. It details the fields > > > and their roles. > > > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > > --- > > > include/linux/iio/imu/adis.h | 14 ++++++++++++++ > > > 1 file changed, 14 insertions(+) > > > > > > diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h > > > index ac7cfd073804..0787a3aabd05 100644 > > > --- a/include/linux/iio/imu/adis.h > > > +++ b/include/linux/iio/imu/adis.h > > > @@ -73,6 +73,20 @@ struct adis_data { > > > bool has_paging; > > > }; > > > > > > +/** > > > + * struct adis - ADIS device instance data > > > + * @spi: Reference to SPI device which owns this ADIS IIO device > > > + * @trig: IIO trigger object data > > > + * @data: ADIS chip variant specific data > > > + * @burst: ADIS burst transfer information > > > + * @state_lock: Lock used by the device to protect state > > > + * @msg: SPI message object > > > + * @xfer: SPI transfer objects to be used for a @msg > > > + * @current_page: Some ADIS devices have registers, this selects current > > > page > > > + * @buffer: Data buffer for information read from the device > > > + * @tx: Cacheline aligned TX buffer for SPI transfers > > > + * @rx: Cacheline aligned RX buffer for SPI transfers > > > > This last one isn't true.. > > Oh right. > I noticed this at some point, then forgot about it. > [ The "joys" of multi-tasking; sorry about it :) ] > I guess I should also add a patch in this series making it cacheline-aligned. > I don't see a reason why not-to [unless I am misunderstanging something] No. The code is fine. The purpose of the cacheline stuff is to ensure no other data ends up in the same cacheline and might be changed in parallel with the spi dma taking place. You only need to force it for the first element when you have 2 buffers like this. So you need to update the comment in this patch to say something less specific. Perhaps @tx: DMA safe TX buffer for SPI transfers @rx: DMA safe RX buffer for SPI transfers. Thanks, Jonathan > > > > > > > + */ > > > struct adis { > > > struct spi_device *spi; > > > struct iio_trigger *trig;
On Fri, 2020-02-21 at 16:14 +0000, Jonathan Cameron wrote: > On Fri, 21 Feb 2020 15:59:35 +0000 > "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote: > > > On Fri, 2020-02-21 at 13:11 +0000, Jonathan Cameron wrote: > > > On Fri, 21 Feb 2020 13:49:41 +0200 > > > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > > > > > > > This change adds a doc-string for the 'adis' struct. It details the > > > > fields > > > > and their roles. > > > > > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > > > --- > > > > include/linux/iio/imu/adis.h | 14 ++++++++++++++ > > > > 1 file changed, 14 insertions(+) > > > > > > > > diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h > > > > index ac7cfd073804..0787a3aabd05 100644 > > > > --- a/include/linux/iio/imu/adis.h > > > > +++ b/include/linux/iio/imu/adis.h > > > > @@ -73,6 +73,20 @@ struct adis_data { > > > > bool has_paging; > > > > }; > > > > > > > > +/** > > > > + * struct adis - ADIS device instance data > > > > + * @spi: Reference to SPI device which owns this ADIS IIO device > > > > + * @trig: IIO trigger object data > > > > + * @data: ADIS chip variant specific data > > > > + * @burst: ADIS burst transfer information > > > > + * @state_lock: Lock used by the device to protect state > > > > + * @msg: SPI message object > > > > + * @xfer: SPI transfer objects to be used for a @msg > > > > + * @current_page: Some ADIS devices have registers, this selects > > > > current > > > > page > > > > + * @buffer: Data buffer for information read from the device > > > > + * @tx: Cacheline aligned TX buffer for SPI transfers > > > > + * @rx: Cacheline aligned RX buffer for SPI transfers > > > > > > This last one isn't true.. > > > > Oh right. > > I noticed this at some point, then forgot about it. > > [ The "joys" of multi-tasking; sorry about it :) ] > > I guess I should also add a patch in this series making it cacheline- > > aligned. > > I don't see a reason why not-to [unless I am misunderstanging something] > No. The code is fine. The purpose of the cacheline stuff is to ensure > no other data ends up in the same cacheline and might be changed in parallel > with the spi dma taking place. You only need to force it for the first > element when you have 2 buffers like this. > > So you need to update the comment in this patch to say something less > specific. > Perhaps > > @tx: DMA safe TX buffer for SPI transfers > @rx: DMA safe RX buffer for SPI transfers. > Sounds good to me. Thanks Alex > Thanks, > > Jonathan > > > > > > > > > > + */ > > > > struct adis { > > > > struct spi_device *spi; > > > > struct iio_trigger *trig;
diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h index ac7cfd073804..0787a3aabd05 100644 --- a/include/linux/iio/imu/adis.h +++ b/include/linux/iio/imu/adis.h @@ -73,6 +73,20 @@ struct adis_data { bool has_paging; }; +/** + * struct adis - ADIS device instance data + * @spi: Reference to SPI device which owns this ADIS IIO device + * @trig: IIO trigger object data + * @data: ADIS chip variant specific data + * @burst: ADIS burst transfer information + * @state_lock: Lock used by the device to protect state + * @msg: SPI message object + * @xfer: SPI transfer objects to be used for a @msg + * @current_page: Some ADIS devices have registers, this selects current page + * @buffer: Data buffer for information read from the device + * @tx: Cacheline aligned TX buffer for SPI transfers + * @rx: Cacheline aligned RX buffer for SPI transfers + */ struct adis { struct spi_device *spi; struct iio_trigger *trig;
This change adds a doc-string for the 'adis' struct. It details the fields and their roles. Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> --- include/linux/iio/imu/adis.h | 14 ++++++++++++++ 1 file changed, 14 insertions(+)