Re: [PATCH 15/20] iio: buffer: Make timestamp s64 in iio_push_to_buffers_with_timestamp()

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 15 Dec 2024 23:12:13 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Sun, Dec 15, 2024 at 06:29:06PM +0000, Jonathan Cameron wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > This is a bit of a corner case for selecting between the in kernel types
> > and standard c integer types we tend to prefer for userspace interfaces.  
> 
> s/c/C/
> 
> > The interface is entirely within the kernel but in many cases the data
> > ultimately ends up in userspace (via some time in a kfifo).  On balance
> > the value passed is almost always an s64, so standardize on that.
> > Main reason to change this is that it has led to some inconsistency in
> > the storage type used.  The majority use aligned_s64 rather than
> > int64_t __aligned(8) and this will ensure there is one obvious choice.  
> 
> ...
> 
> >  static inline int iio_push_to_buffers_with_timestamp(struct iio_dev *indio_dev,
> > -	void *data, int64_t timestamp)
> > +	void *data, s64 timestamp)  
> 
> Hmm... Is it the indentation used for other static inline definitions there?
> Otherwise I would fix it to follow standard pattern (use same column as the
> first argument).

Makes sense.

> 
> ...
> 
> >  	if (indio_dev->scan_timestamp) {
> > -		size_t ts_offset = indio_dev->scan_bytes / sizeof(int64_t) - 1;
> > -		((int64_t *)data)[ts_offset] = timestamp;
> > +		size_t ts_offset = indio_dev->scan_bytes / sizeof(s64) - 1;  
> 
> sizeof(timestamp) ?

I think not on this one. It's about the type of the cast on the next line more
than timestamp type. If timestamp were an s32 and we were writing it into
a 64 bit location then sizeof(timestamp) would be the wrong size but whereas
the proposal here would still work.  If we were doing a memcpy rather than
a simple assignment the sizes would have to be the same and your suggestion
would make sense.

Could introduce a local variable to make this more obvious and provide a
parameter for the sizeof()

		s64 *datas64 = (s64 *)data;
		size_t ts_offset = indio-dev->scan_bytes / sizeof(*datas64) - 1;
		
		datas64[ts_offset] = timestamp;

Do you think that is worth doing or just adding complexity we don't need?
I don't see it as much more readable, so think on balance sticking to original
proposal in this patch makes more sense.

Note this needed a rebase anyway as scan_timestamp is now private.

> 
> > +		((s64 *)data)[ts_offset] = timestamp;
> >  	}  
>