[PATCH RFC 0/6] iio: mark scan_timestamp __private

[PATCH RFC 0/6] iio: mark scan_timestamp __private

[Vasileios Amoiridis]

The scan_timestamp value of the struct iio_dev, even though is an
internal variable, it is being used in some drivers. To avoid any
unwanted overwrites of this value, create a getter and when all the
drivers are converted, mark the variable as __private.

The patch is an RFC because the added value might not be considered
high enough by someone to be implemented and/or it might need to be
done in a different way since it touches multiple drivers.

Vasileios Amoiridis (6):
  iio: create accessor for iio_dev->scan_timestamp
  iio: make use of iio_is_soft_ts_enabled()
  iio: adc: dln2-adc: make use of iio_is_soft_ts_enabled()
  iio: adc: max1363: make use of iio_is_soft_ts_enabled()
  iio: common: ssp_sensors: make use of iio_is_soft_ts_enabled()
  iio: core: mark scan_timestamp as __private

 drivers/iio/adc/dln2-adc.c               |  2 +-
 drivers/iio/adc/max1363.c                |  2 +-
 drivers/iio/common/ssp_sensors/ssp_iio.c |  2 +-
 drivers/iio/industrialio-buffer.c        |  2 +-
 include/linux/iio/buffer.h               |  2 +-
 include/linux/iio/iio.h                  | 11 ++++++++++-
 6 files changed, 15 insertions(+), 6 deletions(-)


base-commit: a61ff7eac77e86de828fe28c4e42b8ae9ec2b195
-- 
2.43.0

[PATCH RFC 1/6] iio: create accessor for iio_dev->scan_timestamp

[Vasileios Amoiridis]

"scan_timestamp" is supposed to be an internal member of the iio device
structure. However, there are some drivers that are using it directly.

For that reason, the following accessor is created:

	iio_is_soft_ts_enabled()

The goal of this accessor, is to ultimately mark "scan_timestamp" as
a __private member of the struct iio_dev.

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 include/linux/iio/iio.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index ae65890d4567..5661794d1127 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -902,6 +902,15 @@ int iio_active_scan_mask_index(struct iio_dev *indio_dev);
 	for_each_set_bit((chan), (indio_dev)->active_scan_mask, \
 			 iio_get_masklength(indio_dev))
 
+/**
+ * iio_is_soft_ts_enabled - Check if the software timestamp is enabled
+ * @indio_dev: the IIO device
+ */
+static inline bool iio_is_soft_ts_enabled(const struct iio_dev *indio_dev)
+{
+	return indio_dev->scan_timestamp;
+}
+
 ssize_t iio_format_value(char *buf, unsigned int type, int size, int *vals);
 
 int iio_str_to_fixpoint(const char *str, int fract_mult, int *integer,
-- 
2.43.0

[PATCH RFC 2/6] iio: make use of iio_is_soft_ts_enabled()

[Vasileios Amoiridis]

Use the iio_is_soft_ts_enabled() accessor to access the value of the
scan_timestamp. This way, it can be marked as __private when there
are no direct accessors of it.

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 include/linux/iio/buffer.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h
index 418b1307d3f2..3d82b110a8b9 100644
--- a/include/linux/iio/buffer.h
+++ b/include/linux/iio/buffer.h
@@ -37,7 +37,7 @@ int iio_pop_from_buffer(struct iio_buffer *buffer, void *data);
 static inline int iio_push_to_buffers_with_timestamp(struct iio_dev *indio_dev,
 	void *data, int64_t timestamp)
 {
-	if (indio_dev->scan_timestamp) {
+	if (iio_is_soft_ts_enabled(indio_dev)) {
 		size_t ts_offset = indio_dev->scan_bytes / sizeof(int64_t) - 1;
 		((int64_t *)data)[ts_offset] = timestamp;
 	}
-- 
2.43.0

[PATCH RFC 3/6] iio: adc: dln2-adc: make use of iio_is_soft_ts_enabled()

[Vasileios Amoiridis]

Use the iio_is_soft_ts_enabled() accessor to access the value of the
scan_timestamp. This way, it can be marked as __private when there
are no direct accessors of it.

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/adc/dln2-adc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/dln2-adc.c b/drivers/iio/adc/dln2-adc.c
index 30328626d9be..f9cf132c41e6 100644
--- a/drivers/iio/adc/dln2-adc.c
+++ b/drivers/iio/adc/dln2-adc.c
@@ -128,7 +128,7 @@ static void dln2_adc_update_demux(struct dln2_adc *dln2)
 		in_loc += 2;
 	}
 
-	if (indio_dev->scan_timestamp) {
+	if (iio_is_soft_ts_enabled(indio_dev)) {
 		size_t ts_offset = indio_dev->scan_bytes / sizeof(int64_t) - 1;
 
 		dln2->ts_pad_offset = out_loc;
-- 
2.43.0

[PATCH RFC 4/6] iio: adc: max1363: make use of iio_is_soft_ts_enabled()

[Vasileios Amoiridis]

Use the iio_is_soft_ts_enabled() accessor to access the value of the
scan_timestamp. This way, it can be marked as __private when there
are no direct accessors of it.

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/adc/max1363.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
index 9a0baea08ab6..57d9aff729f4 100644
--- a/drivers/iio/adc/max1363.c
+++ b/drivers/iio/adc/max1363.c
@@ -1473,7 +1473,7 @@ static irqreturn_t max1363_trigger_handler(int irq, void *p)
 		d_size = numvals*2;
 	else
 		d_size = numvals;
-	if (indio_dev->scan_timestamp) {
+	if (iio_is_soft_ts_enabled(indio_dev)) {
 		d_size += sizeof(s64);
 		if (d_size % sizeof(s64))
 			d_size += sizeof(s64) - (d_size % sizeof(s64));
-- 
2.43.0

[PATCH RFC 5/6] iio: common: ssp_sensors: make use of iio_is_soft_ts_enabled()

[Vasileios Amoiridis]

Use the iio_is_soft_ts_enabled() accessor to access the value of the
scan_timestamp. This way, it can be marked as __private when there
are no direct accessors of it.

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/common/ssp_sensors/ssp_iio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/common/ssp_sensors/ssp_iio.c b/drivers/iio/common/ssp_sensors/ssp_iio.c
index 88b8b56bfa51..c38bf1dfb7bd 100644
--- a/drivers/iio/common/ssp_sensors/ssp_iio.c
+++ b/drivers/iio/common/ssp_sensors/ssp_iio.c
@@ -82,7 +82,7 @@ int ssp_common_process_data(struct iio_dev *indio_dev, void *buf,
 	 */
 	memcpy(spd->buffer, buf, len);
 
-	if (indio_dev->scan_timestamp) {
+	if (iio_is_soft_ts_enabled(indio_dev)) {
 		memcpy(&time, &((char *)buf)[len], SSP_TIME_SIZE);
 		calculated_time =
 			timestamp + (int64_t)le32_to_cpu(time) * 1000000;
-- 
2.43.0

[PATCH RFC 6/6] iio: core: mark scan_timestamp as __private

[Vasileios Amoiridis]

Since there are no more direct accesses to the indio_dev->scan_timestamp
value, it can be marked as __private and use the macro ACCESS_PRIVATE()
in order to access it. Like this, static checkers will be able to inform
in case someone tries to either write to the value, or read its value
directly.

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/industrialio-buffer.c | 2 +-
 include/linux/iio/iio.h           | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 8104696cd475..c332741f3cf4 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1137,7 +1137,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
 	int ret;
 
 	indio_dev->active_scan_mask = config->scan_mask;
-	indio_dev->scan_timestamp = config->scan_timestamp;
+	ACCESS_PRIVATE(indio_dev, scan_timestamp) = config->scan_timestamp;
 	indio_dev->scan_bytes = config->scan_bytes;
 	iio_dev_opaque->currentmode = config->mode;
 
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 5661794d1127..669b4ef1280d 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -611,7 +611,7 @@ struct iio_dev {
 	const unsigned long		*available_scan_masks;
 	unsigned int			__private masklength;
 	const unsigned long		*active_scan_mask;
-	bool				scan_timestamp;
+	bool				__private scan_timestamp;
 	struct iio_trigger		*trig;
 	struct iio_poll_func		*pollfunc;
 	struct iio_poll_func		*pollfunc_event;
@@ -908,7 +908,7 @@ int iio_active_scan_mask_index(struct iio_dev *indio_dev);
  */
 static inline bool iio_is_soft_ts_enabled(const struct iio_dev *indio_dev)
 {
-	return indio_dev->scan_timestamp;
+	return ACCESS_PRIVATE(indio_dev, scan_timestamp);
 }
 
 ssize_t iio_format_value(char *buf, unsigned int type, int size, int *vals);
-- 
2.43.0

Re: [PATCH RFC 3/6] iio: adc: dln2-adc: make use of iio_is_soft_ts_enabled()

[Jonathan Cameron]

On Sat, 30 Nov 2024 01:27:07 +0100
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> Use the iio_is_soft_ts_enabled() accessor to access the value of the
> scan_timestamp. This way, it can be marked as __private when there
> are no direct accessors of it.
> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>

The original code looks to me like a micro optimization that we should
consider dropping to reduce complexity.  It is only used to zero a hole
in a structure conditionally if the timestamp is enabled.

Better I think to just drop all the ts_pad_offset etc stuff in favour of
just zeroing the whole of the data structure in dln2_adc_trigger_h()
whether or not the timestamp is enabled.

My guess is that on a reasonably performance CPU the occasional cost
of a branch miss prediction will outweigh zeroing a fairly small structure
anyway.

+CC Jack who wrote this driver.

Jonathan


> ---
>  drivers/iio/adc/dln2-adc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/dln2-adc.c b/drivers/iio/adc/dln2-adc.c
> index 30328626d9be..f9cf132c41e6 100644
> --- a/drivers/iio/adc/dln2-adc.c
> +++ b/drivers/iio/adc/dln2-adc.c
> @@ -128,7 +128,7 @@ static void dln2_adc_update_demux(struct dln2_adc *dln2)
>  		in_loc += 2;
>  	}
>  
> -	if (indio_dev->scan_timestamp) {
> +	if (iio_is_soft_ts_enabled(indio_dev)) {
>  		size_t ts_offset = indio_dev->scan_bytes / sizeof(int64_t) - 1;
>  
>  		dln2->ts_pad_offset = out_loc;

Re: [PATCH RFC 4/6] iio: adc: max1363: make use of iio_is_soft_ts_enabled()

[Jonathan Cameron]

On Sat, 30 Nov 2024 01:27:08 +0100
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> Use the iio_is_soft_ts_enabled() accessor to access the value of the
> scan_timestamp. This way, it can be marked as __private when there
> are no direct accessors of it.
> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
Hmm.  A younger me one ;) 

A simple allocation that is always big enough is going to cost
us very little.  Should almost certainly be using kzalloc.

I'd change this driver to just stick an array of size
12 * 2  + 8 so 32bytes in the iio_priv()
(12 channels, possibly of 2 bytes each + aligned timestamp)
and always use that for the rx_buf.

Both a simplification and probably a performance improvement
as well by dropping the frequent allocations.


> ---
>  drivers/iio/adc/max1363.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
> index 9a0baea08ab6..57d9aff729f4 100644
> --- a/drivers/iio/adc/max1363.c
> +++ b/drivers/iio/adc/max1363.c
> @@ -1473,7 +1473,7 @@ static irqreturn_t max1363_trigger_handler(int irq, void *p)
>  		d_size = numvals*2;
>  	else
>  		d_size = numvals;
> -	if (indio_dev->scan_timestamp) {
> +	if (iio_is_soft_ts_enabled(indio_dev)) {
>  		d_size += sizeof(s64);
>  		if (d_size % sizeof(s64))
>  			d_size += sizeof(s64) - (d_size % sizeof(s64));

Re: [PATCH RFC 5/6] iio: common: ssp_sensors: make use of iio_is_soft_ts_enabled()

[Jonathan Cameron]

On Sat, 30 Nov 2024 01:27:09 +0100
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> Use the iio_is_soft_ts_enabled() accessor to access the value of the
> scan_timestamp. This way, it can be marked as __private when there
> are no direct accessors of it.
> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>

> ---
>  drivers/iio/common/ssp_sensors/ssp_iio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/common/ssp_sensors/ssp_iio.c b/drivers/iio/common/ssp_sensors/ssp_iio.c
> index 88b8b56bfa51..c38bf1dfb7bd 100644
> --- a/drivers/iio/common/ssp_sensors/ssp_iio.c
> +++ b/drivers/iio/common/ssp_sensors/ssp_iio.c
> @@ -82,7 +82,7 @@ int ssp_common_process_data(struct iio_dev *indio_dev, void *buf,
>  	 */
>  	memcpy(spd->buffer, buf, len);
>  
> -	if (indio_dev->scan_timestamp) {
> +	if (iio_is_soft_ts_enabled(indio_dev)) {

It might be simpler to just drop this conditional in favour of always computing the
value.



>  		memcpy(&time, &((char *)buf)[len], SSP_TIME_SIZE);
>  		calculated_time =
>  			timestamp + (int64_t)le32_to_cpu(time) * 1000000;

Good to replace this with something more modern like.

		calculated_time =
			timestamp + get_unaligned_le32(buf + len) * MEGA;

Jonathan

Re: [PATCH RFC 6/6] iio: core: mark scan_timestamp as __private

[Jonathan Cameron]

On Sat, 30 Nov 2024 01:27:10 +0100
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> Since there are no more direct accesses to the indio_dev->scan_timestamp
> value, it can be marked as __private and use the macro ACCESS_PRIVATE()
> in order to access it. Like this, static checkers will be able to inform
> in case someone tries to either write to the value, or read its value
> directly.
> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> ---
>  drivers/iio/industrialio-buffer.c | 2 +-
>  include/linux/iio/iio.h           | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 8104696cd475..c332741f3cf4 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -1137,7 +1137,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
>  	int ret;
>  
>  	indio_dev->active_scan_mask = config->scan_mask;
> -	indio_dev->scan_timestamp = config->scan_timestamp;
> +	ACCESS_PRIVATE(indio_dev, scan_timestamp) = config->scan_timestamp;
>  	indio_dev->scan_bytes = config->scan_bytes;
>  	iio_dev_opaque->currentmode = config->mode;
>  
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 5661794d1127..669b4ef1280d 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -611,7 +611,7 @@ struct iio_dev {
>  	const unsigned long		*available_scan_masks;
>  	unsigned int			__private masklength;
>  	const unsigned long		*active_scan_mask;
> -	bool				scan_timestamp;
> +	bool				__private scan_timestamp;
>  	struct iio_trigger		*trig;
>  	struct iio_poll_func		*pollfunc;
>  	struct iio_poll_func		*pollfunc_event;
> @@ -908,7 +908,7 @@ int iio_active_scan_mask_index(struct iio_dev *indio_dev);
>   */
>  static inline bool iio_is_soft_ts_enabled(const struct iio_dev *indio_dev)
>  {
> -	return indio_dev->scan_timestamp;
> +	return ACCESS_PRIVATE(indio_dev, scan_timestamp);
If we only end up with one use of this (based on feedback on other drivers)
I'd tempted to deliberately not provide this convenience function and instead
just use ACCESS_PRIVATE() directly in iio_push_to_buffers_with_timestamp()

Nice work. Particularly by highlighting some 'odd corners' in drivers that
probably make no real sense to keep ;)

Jonathan


>  }
>  
>  ssize_t iio_format_value(char *buf, unsigned int type, int size, int *vals);

Re: [PATCH RFC 3/6] iio: adc: dln2-adc: make use of iio_is_soft_ts_enabled()

[Vasileios Amoiridis]

On Sat, Nov 30, 2024 at 02:02:55PM +0000, Jonathan Cameron wrote:
> On Sat, 30 Nov 2024 01:27:07 +0100
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> 
> > Use the iio_is_soft_ts_enabled() accessor to access the value of the
> > scan_timestamp. This way, it can be marked as __private when there
> > are no direct accessors of it.
> > 
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> 
> The original code looks to me like a micro optimization that we should
> consider dropping to reduce complexity.  It is only used to zero a hole
> in a structure conditionally if the timestamp is enabled.
> 
> Better I think to just drop all the ts_pad_offset etc stuff in favour of
> just zeroing the whole of the data structure in dln2_adc_trigger_h()
> whether or not the timestamp is enabled.
> 
> My guess is that on a reasonably performance CPU the occasional cost
> of a branch miss prediction will outweigh zeroing a fairly small structure
> anyway.
> 
> +CC Jack who wrote this driver.
> 
> Jonathan
> 
> 

Hi Jonathan,

Thanks for the review once again! It looks like it should be fairly
straightforward to drop the zeroing of the ts_pad_{offset/length} so
indeed, if Jack doesn't have anything strong against it I could move
forward and send a v2.

Cheers,
Vasilis

> > ---
> >  drivers/iio/adc/dln2-adc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/adc/dln2-adc.c b/drivers/iio/adc/dln2-adc.c
> > index 30328626d9be..f9cf132c41e6 100644
> > --- a/drivers/iio/adc/dln2-adc.c
> > +++ b/drivers/iio/adc/dln2-adc.c
> > @@ -128,7 +128,7 @@ static void dln2_adc_update_demux(struct dln2_adc *dln2)
> >  		in_loc += 2;
> >  	}
> >  
> > -	if (indio_dev->scan_timestamp) {
> > +	if (iio_is_soft_ts_enabled(indio_dev)) {
> >  		size_t ts_offset = indio_dev->scan_bytes / sizeof(int64_t) - 1;
> >  
> >  		dln2->ts_pad_offset = out_loc;
> 

Re: [PATCH RFC 4/6] iio: adc: max1363: make use of iio_is_soft_ts_enabled()

[Vasileios Amoiridis]

On Sat, Nov 30, 2024 at 02:09:48PM +0000, Jonathan Cameron wrote:
> On Sat, 30 Nov 2024 01:27:08 +0100
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> 
> > Use the iio_is_soft_ts_enabled() accessor to access the value of the
> > scan_timestamp. This way, it can be marked as __private when there
> > are no direct accessors of it.
> > 
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> Hmm.  A younger me one ;) 
> 
> A simple allocation that is always big enough is going to cost
> us very little.  Should almost certainly be using kzalloc.
> 
> I'd change this driver to just stick an array of size
> 12 * 2  + 8 so 32bytes in the iio_priv()
> (12 channels, possibly of 2 bytes each + aligned timestamp)
> and always use that for the rx_buf.
> 
> Both a simplification and probably a performance improvement
> as well by dropping the frequent allocations.
> 
> 

Hi Jonathan,

I see your point, I can implement this as well.

Cheers,
Vasilis

> > ---
> >  drivers/iio/adc/max1363.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
> > index 9a0baea08ab6..57d9aff729f4 100644
> > --- a/drivers/iio/adc/max1363.c
> > +++ b/drivers/iio/adc/max1363.c
> > @@ -1473,7 +1473,7 @@ static irqreturn_t max1363_trigger_handler(int irq, void *p)
> >  		d_size = numvals*2;
> >  	else
> >  		d_size = numvals;
> > -	if (indio_dev->scan_timestamp) {
> > +	if (iio_is_soft_ts_enabled(indio_dev)) {
> >  		d_size += sizeof(s64);
> >  		if (d_size % sizeof(s64))
> >  			d_size += sizeof(s64) - (d_size % sizeof(s64));
> 

Re: [PATCH RFC 5/6] iio: common: ssp_sensors: make use of iio_is_soft_ts_enabled()

[Vasileios Amoiridis]

On Sat, Nov 30, 2024 at 02:17:14PM +0000, Jonathan Cameron wrote:
> On Sat, 30 Nov 2024 01:27:09 +0100
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> 
> > Use the iio_is_soft_ts_enabled() accessor to access the value of the
> > scan_timestamp. This way, it can be marked as __private when there
> > are no direct accessors of it.
> > 
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> 
> > ---
> >  drivers/iio/common/ssp_sensors/ssp_iio.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/common/ssp_sensors/ssp_iio.c b/drivers/iio/common/ssp_sensors/ssp_iio.c
> > index 88b8b56bfa51..c38bf1dfb7bd 100644
> > --- a/drivers/iio/common/ssp_sensors/ssp_iio.c
> > +++ b/drivers/iio/common/ssp_sensors/ssp_iio.c
> > @@ -82,7 +82,7 @@ int ssp_common_process_data(struct iio_dev *indio_dev, void *buf,
> >  	 */
> >  	memcpy(spd->buffer, buf, len);
> >  
> > -	if (indio_dev->scan_timestamp) {
> > +	if (iio_is_soft_ts_enabled(indio_dev)) {
> 
> It might be simpler to just drop this conditional in favour of always computing the
> value.
> 
> 
> 
> >  		memcpy(&time, &((char *)buf)[len], SSP_TIME_SIZE);
> >  		calculated_time =
> >  			timestamp + (int64_t)le32_to_cpu(time) * 1000000;
> 
> Good to replace this with something more modern like.
> 
> 		calculated_time =
> 			timestamp + get_unaligned_le32(buf + len) * MEGA;
> 
> Jonathan
>

Hi Jonathan,

I could definitely do this, thanks!

Cheers,
Vasilis

Re: [PATCH RFC 6/6] iio: core: mark scan_timestamp as __private

[Vasileios Amoiridis]

On Sat, Nov 30, 2024 at 02:19:54PM +0000, Jonathan Cameron wrote:
> On Sat, 30 Nov 2024 01:27:10 +0100
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> 
> > Since there are no more direct accesses to the indio_dev->scan_timestamp
> > value, it can be marked as __private and use the macro ACCESS_PRIVATE()
> > in order to access it. Like this, static checkers will be able to inform
> > in case someone tries to either write to the value, or read its value
> > directly.
> > 
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> > ---
> >  drivers/iio/industrialio-buffer.c | 2 +-
> >  include/linux/iio/iio.h           | 4 ++--
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> > index 8104696cd475..c332741f3cf4 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -1137,7 +1137,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
> >  	int ret;
> >  
> >  	indio_dev->active_scan_mask = config->scan_mask;
> > -	indio_dev->scan_timestamp = config->scan_timestamp;
> > +	ACCESS_PRIVATE(indio_dev, scan_timestamp) = config->scan_timestamp;
> >  	indio_dev->scan_bytes = config->scan_bytes;
> >  	iio_dev_opaque->currentmode = config->mode;
> >  
> > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > index 5661794d1127..669b4ef1280d 100644
> > --- a/include/linux/iio/iio.h
> > +++ b/include/linux/iio/iio.h
> > @@ -611,7 +611,7 @@ struct iio_dev {
> >  	const unsigned long		*available_scan_masks;
> >  	unsigned int			__private masklength;
> >  	const unsigned long		*active_scan_mask;
> > -	bool				scan_timestamp;
> > +	bool				__private scan_timestamp;
> >  	struct iio_trigger		*trig;
> >  	struct iio_poll_func		*pollfunc;
> >  	struct iio_poll_func		*pollfunc_event;
> > @@ -908,7 +908,7 @@ int iio_active_scan_mask_index(struct iio_dev *indio_dev);
> >   */
> >  static inline bool iio_is_soft_ts_enabled(const struct iio_dev *indio_dev)
> >  {
> > -	return indio_dev->scan_timestamp;
> > +	return ACCESS_PRIVATE(indio_dev, scan_timestamp);
> If we only end up with one use of this (based on feedback on other drivers)
> I'd tempted to deliberately not provide this convenience function and instead
> just use ACCESS_PRIVATE() directly in iio_push_to_buffers_with_timestamp()
> 
> Nice work. Particularly by highlighting some 'odd corners' in drivers that
> probably make no real sense to keep ;)
> 
> Jonathan
> 
> 
> >  }
> >  
> >  ssize_t iio_format_value(char *buf, unsigned int type, int size, int *vals);
>

Hi Jonathan,

Indeed, if it is only one case that this is being used, it wouldn't make
sense to provide an accessor. I wouldn't think of going directly to
touch the drivers without sending this RFC first, so it's good that you
like the solution of optimizing the drivers themselves. I might find
some time before the weekend to spin a v2 to discuss. Thanks for your
time, your comments are always of great help!

Cheers,
Vasilis