[PATCH v1 1/3] types: Complement the aligned types with signed 64-bit one

[PATCH v1 1/3] types: Complement the aligned types with signed 64-bit one

[Andy Shevchenko]

Some user may want to use aligned signed 64-bit type.
Provide it for them.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/types.h      | 3 ++-
 include/uapi/linux/types.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/types.h b/include/linux/types.h
index 253168bb3fe1..78d87c751ff5 100644
--- a/include/linux/types.h
+++ b/include/linux/types.h
@@ -115,8 +115,9 @@ typedef u64			u_int64_t;
 typedef s64			int64_t;
 #endif
 
-/* this is a special 64bit data type that is 8-byte aligned */
+/* These are the special 64-bit data types that are 8-byte aligned */
 #define aligned_u64		__aligned_u64
+#define aligned_s64		__aligned_s64
 #define aligned_be64		__aligned_be64
 #define aligned_le64		__aligned_le64
 
diff --git a/include/uapi/linux/types.h b/include/uapi/linux/types.h
index 6375a0684052..48b933938877 100644
--- a/include/uapi/linux/types.h
+++ b/include/uapi/linux/types.h
@@ -53,6 +53,7 @@ typedef __u32 __bitwise __wsum;
  * No conversions are necessary between 32-bit user-space and a 64-bit kernel.
  */
 #define __aligned_u64 __u64 __attribute__((aligned(8)))
+#define __aligned_s64 __s64 __attribute__((aligned(8)))
 #define __aligned_be64 __be64 __attribute__((aligned(8)))
 #define __aligned_le64 __le64 __attribute__((aligned(8)))
 
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 2/3] iio: imu: st_lsm6dsx: Use aligned data type for timestamp

[Andy Shevchenko]

Use __aligned_s64 for the timestamp field.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
index c19237717e81..d3d4b3281aa6 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
@@ -442,10 +442,9 @@ struct st_lsm6dsx_hw {
 	const struct st_lsm6dsx_settings *settings;
 
 	struct iio_mount_matrix orientation;
-	/* Ensure natural alignment of buffer elements */
 	struct {
 		__le16 channels[3];
-		s64 ts __aligned(8);
+		__aligned_s64 ts;
 	} scan[ST_LSM6DSX_ID_MAX];
 };
 
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v1 2/3] iio: imu: st_lsm6dsx: Use aligned data type for timestamp

[Jonathan Cameron]

On Tue, 15 Aug 2023 18:40:26 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Use __aligned_s64 for the timestamp field.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> index c19237717e81..d3d4b3281aa6 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> @@ -442,10 +442,9 @@ struct st_lsm6dsx_hw {
>  	const struct st_lsm6dsx_settings *settings;
>  
>  	struct iio_mount_matrix orientation;
> -	/* Ensure natural alignment of buffer elements */
>  	struct {
>  		__le16 channels[3];
> -		s64 ts __aligned(8);
> +		__aligned_s64 ts;

aligned_s64 as it's internal to the kernel?

>  	} scan[ST_LSM6DSX_ID_MAX];
>  };
>  

Re: [PATCH v1 2/3] iio: imu: st_lsm6dsx: Use aligned data type for timestamp

[Andy Shevchenko]

On Mon, Aug 28, 2023 at 05:03:41PM +0100, Jonathan Cameron wrote:
> On Tue, 15 Aug 2023 18:40:26 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

...

> >  	struct {
> >  		__le16 channels[3];
> > -		s64 ts __aligned(8);
> > +		__aligned_s64 ts;
> 
> aligned_s64 as it's internal to the kernel?

Either works, but strictly speaking you are right, better no underscored
variant.

> >  	} scan[ST_LSM6DSX_ID_MAX];

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 3/3] iio: hid-sensor: Use aligned data type for timestamp

[Andy Shevchenko]

Use aligned_s64 for the timestamp field.

Note, the actual data is signed, hence with this we also amend that.
While at it, drop redundant __alignment directive.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/accel/hid-sensor-accel-3d.c              | 3 +--
 drivers/iio/gyro/hid-sensor-gyro-3d.c                | 2 +-
 drivers/iio/humidity/hid-sensor-humidity.c           | 2 +-
 drivers/iio/light/hid-sensor-als.c                   | 2 +-
 drivers/iio/orientation/hid-sensor-incl-3d.c         | 2 +-
 drivers/iio/orientation/hid-sensor-rotation.c        | 4 ++--
 drivers/iio/position/hid-sensor-custom-intel-hinge.c | 2 +-
 drivers/iio/pressure/hid-sensor-press.c              | 2 +-
 drivers/iio/temperature/hid-sensor-temperature.c     | 2 +-
 9 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/accel/hid-sensor-accel-3d.c b/drivers/iio/accel/hid-sensor-accel-3d.c
index 5eac7ea19993..f739589564c5 100644
--- a/drivers/iio/accel/hid-sensor-accel-3d.c
+++ b/drivers/iio/accel/hid-sensor-accel-3d.c
@@ -25,10 +25,9 @@ struct accel_3d_state {
 	struct hid_sensor_hub_callbacks callbacks;
 	struct hid_sensor_common common_attributes;
 	struct hid_sensor_hub_attribute_info accel[ACCEL_3D_CHANNEL_MAX];
-	/* Ensure timestamp is naturally aligned */
 	struct {
 		u32 accel_val[3];
-		s64 timestamp __aligned(8);
+		aligned_s64 timestamp;
 	} scan;
 	int scale_pre_decml;
 	int scale_post_decml;
diff --git a/drivers/iio/gyro/hid-sensor-gyro-3d.c b/drivers/iio/gyro/hid-sensor-gyro-3d.c
index 698c50da1f10..a7050a6328d6 100644
--- a/drivers/iio/gyro/hid-sensor-gyro-3d.c
+++ b/drivers/iio/gyro/hid-sensor-gyro-3d.c
@@ -27,7 +27,7 @@ struct gyro_3d_state {
 	struct hid_sensor_hub_attribute_info gyro[GYRO_3D_CHANNEL_MAX];
 	struct {
 		u32 gyro_val[GYRO_3D_CHANNEL_MAX];
-		u64 timestamp __aligned(8);
+		aligned_s64 timestamp;
 	} scan;
 	int scale_pre_decml;
 	int scale_post_decml;
diff --git a/drivers/iio/humidity/hid-sensor-humidity.c b/drivers/iio/humidity/hid-sensor-humidity.c
index fa0fe404a70a..0e484b78b735 100644
--- a/drivers/iio/humidity/hid-sensor-humidity.c
+++ b/drivers/iio/humidity/hid-sensor-humidity.c
@@ -18,7 +18,7 @@ struct hid_humidity_state {
 	struct hid_sensor_hub_attribute_info humidity_attr;
 	struct {
 		s32 humidity_data;
-		u64 timestamp __aligned(8);
+		aligned_s64 timestamp;
 	} scan;
 	int scale_pre_decml;
 	int scale_post_decml;
diff --git a/drivers/iio/light/hid-sensor-als.c b/drivers/iio/light/hid-sensor-als.c
index eb1aedad7edc..8a906d95edd4 100644
--- a/drivers/iio/light/hid-sensor-als.c
+++ b/drivers/iio/light/hid-sensor-als.c
@@ -27,7 +27,7 @@ struct als_state {
 	struct hid_sensor_hub_attribute_info als_illum;
 	struct {
 		u32 illum[CHANNEL_SCAN_INDEX_MAX];
-		u64 timestamp __aligned(8);
+		aligned_s64 timestamp;
 	} scan;
 	int scale_pre_decml;
 	int scale_post_decml;
diff --git a/drivers/iio/orientation/hid-sensor-incl-3d.c b/drivers/iio/orientation/hid-sensor-incl-3d.c
index ba5b581d5b25..3e5f2c58dfa9 100644
--- a/drivers/iio/orientation/hid-sensor-incl-3d.c
+++ b/drivers/iio/orientation/hid-sensor-incl-3d.c
@@ -29,7 +29,7 @@ struct incl_3d_state {
 	struct hid_sensor_hub_attribute_info incl[INCLI_3D_CHANNEL_MAX];
 	struct {
 		u32 incl_val[INCLI_3D_CHANNEL_MAX];
-		u64 timestamp __aligned(8);
+		aligned_s64 timestamp;
 	} scan;
 	int scale_pre_decml;
 	int scale_post_decml;
diff --git a/drivers/iio/orientation/hid-sensor-rotation.c b/drivers/iio/orientation/hid-sensor-rotation.c
index a033699910e8..864ecbcad26e 100644
--- a/drivers/iio/orientation/hid-sensor-rotation.c
+++ b/drivers/iio/orientation/hid-sensor-rotation.c
@@ -19,8 +19,8 @@ struct dev_rot_state {
 	struct hid_sensor_common common_attributes;
 	struct hid_sensor_hub_attribute_info quaternion;
 	struct {
-		s32 sampled_vals[4] __aligned(16);
-		u64 timestamp __aligned(8);
+		s32 sampled_vals[4];
+		aligned_s64 timestamp;
 	} scan;
 	int scale_pre_decml;
 	int scale_post_decml;
diff --git a/drivers/iio/position/hid-sensor-custom-intel-hinge.c b/drivers/iio/position/hid-sensor-custom-intel-hinge.c
index 07c30d217255..48005b568dd9 100644
--- a/drivers/iio/position/hid-sensor-custom-intel-hinge.c
+++ b/drivers/iio/position/hid-sensor-custom-intel-hinge.c
@@ -39,7 +39,7 @@ struct hinge_state {
 	const char *labels[CHANNEL_SCAN_INDEX_MAX];
 	struct {
 		u32 hinge_val[3];
-		u64 timestamp __aligned(8);
+		aligned_s64 timestamp;
 	} scan;
 
 	int scale_pre_decml;
diff --git a/drivers/iio/pressure/hid-sensor-press.c b/drivers/iio/pressure/hid-sensor-press.c
index a9215eb32d70..a964c7b65402 100644
--- a/drivers/iio/pressure/hid-sensor-press.c
+++ b/drivers/iio/pressure/hid-sensor-press.c
@@ -24,7 +24,7 @@ struct press_state {
 	struct hid_sensor_hub_attribute_info press_attr;
 	struct {
 		u32 press_data;
-		u64 timestamp __aligned(8);
+		aligned_s64 timestamp;
 	} scan;
 	int scale_pre_decml;
 	int scale_post_decml;
diff --git a/drivers/iio/temperature/hid-sensor-temperature.c b/drivers/iio/temperature/hid-sensor-temperature.c
index d40f235af1d4..32f4b13fd554 100644
--- a/drivers/iio/temperature/hid-sensor-temperature.c
+++ b/drivers/iio/temperature/hid-sensor-temperature.c
@@ -18,7 +18,7 @@ struct temperature_state {
 	struct hid_sensor_hub_attribute_info temperature_attr;
 	struct {
 		s32 temperature_data;
-		u64 timestamp __aligned(8);
+		aligned_s64 timestamp;
 	} scan;
 	int scale_pre_decml;
 	int scale_post_decml;
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v1 3/3] iio: hid-sensor: Use aligned data type for timestamp

[Jonathan Cameron]

On Tue, 15 Aug 2023 18:40:27 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Use aligned_s64 for the timestamp field.
> 
> Note, the actual data is signed, hence with this we also amend that.
> While at it, drop redundant __alignment directive.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/iio/accel/hid-sensor-accel-3d.c              | 3 +--
>  drivers/iio/gyro/hid-sensor-gyro-3d.c                | 2 +-
>  drivers/iio/humidity/hid-sensor-humidity.c           | 2 +-
>  drivers/iio/light/hid-sensor-als.c                   | 2 +-
>  drivers/iio/orientation/hid-sensor-incl-3d.c         | 2 +-
>  drivers/iio/orientation/hid-sensor-rotation.c        | 4 ++--
>  drivers/iio/position/hid-sensor-custom-intel-hinge.c | 2 +-
>  drivers/iio/pressure/hid-sensor-press.c              | 2 +-
>  drivers/iio/temperature/hid-sensor-temperature.c     | 2 +-
>  9 files changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iio/accel/hid-sensor-accel-3d.c b/drivers/iio/accel/hid-sensor-accel-3d.c
> index 5eac7ea19993..f739589564c5 100644
> --- a/drivers/iio/accel/hid-sensor-accel-3d.c
> +++ b/drivers/iio/accel/hid-sensor-accel-3d.c
> @@ -25,10 +25,9 @@ struct accel_3d_state {
>  	struct hid_sensor_hub_callbacks callbacks;
>  	struct hid_sensor_common common_attributes;
>  	struct hid_sensor_hub_attribute_info accel[ACCEL_3D_CHANNEL_MAX];
> -	/* Ensure timestamp is naturally aligned */

Comment is still true and no more or less obvious than it was with the old code
I think so I don't really see why it should be removed with this change.

>  	struct {
>  		u32 accel_val[3];
> -		s64 timestamp __aligned(8);
> +		aligned_s64 timestamp;
>  	} scan;
>  	int scale_pre_decml;
>  	int scale_post_decml;
...


> diff --git a/drivers/iio/orientation/hid-sensor-rotation.c b/drivers/iio/orientation/hid-sensor-rotation.c
> index a033699910e8..864ecbcad26e 100644
> --- a/drivers/iio/orientation/hid-sensor-rotation.c
> +++ b/drivers/iio/orientation/hid-sensor-rotation.c
> @@ -19,8 +19,8 @@ struct dev_rot_state {
>  	struct hid_sensor_common common_attributes;
>  	struct hid_sensor_hub_attribute_info quaternion;
>  	struct {
> -		s32 sampled_vals[4] __aligned(16);
> -		u64 timestamp __aligned(8);
> +		s32 sampled_vals[4];

Hmm. I can't immediately recall why that aligned(16) is therebut
it's not related to the rest of this change so I'm not sure we should touch it.
I don't think we ever required quaternions to be aligned as a whole so this does feel odd but
in the category of something we should be careful touching or at very least do it in
a different patch where it stands out.


> +		aligned_s64 timestamp;
>  	} scan;
>  	int scale_pre_decml;
>  	int scale_post_decml;
> diff --git a/drivers/iio/position/hid-sensor-custom-intel-hinge.c b/drivers/iio/position/hid-sensor-custom-intel-hinge.c
> index 07c30d217255..48005b568dd9 100644
> --- a/drivers/iio/position/hid-sensor-custom-intel-hinge.c
> +++ b/drivers/iio/position/hid-sensor-custom-intel-hinge.c
> @@ -39,7 +39,7 @@ struct hinge_state {
>  	const char *labels[CHANNEL_SCAN_INDEX_MAX];
>  	struct {
>  		u32 hinge_val[3];
> -		u64 timestamp __aligned(8);
> +		aligned_s64 timestamp;
>  	} scan;
>  
>  	int scale_pre_decml;
> diff --git a/drivers/iio/pressure/hid-sensor-press.c b/drivers/iio/pressure/hid-sensor-press.c
> index a9215eb32d70..a964c7b65402 100644
> --- a/drivers/iio/pressure/hid-sensor-press.c
> +++ b/drivers/iio/pressure/hid-sensor-press.c
> @@ -24,7 +24,7 @@ struct press_state {
>  	struct hid_sensor_hub_attribute_info press_attr;
>  	struct {
>  		u32 press_data;
> -		u64 timestamp __aligned(8);
> +		aligned_s64 timestamp;
>  	} scan;
>  	int scale_pre_decml;
>  	int scale_post_decml;
> diff --git a/drivers/iio/temperature/hid-sensor-temperature.c b/drivers/iio/temperature/hid-sensor-temperature.c
> index d40f235af1d4..32f4b13fd554 100644
> --- a/drivers/iio/temperature/hid-sensor-temperature.c
> +++ b/drivers/iio/temperature/hid-sensor-temperature.c
> @@ -18,7 +18,7 @@ struct temperature_state {
>  	struct hid_sensor_hub_attribute_info temperature_attr;
>  	struct {
>  		s32 temperature_data;
> -		u64 timestamp __aligned(8);
> +		aligned_s64 timestamp;
>  	} scan;
>  	int scale_pre_decml;
>  	int scale_post_decml;

Re: [PATCH v1 3/3] iio: hid-sensor: Use aligned data type for timestamp

[Andy Shevchenko]

On Mon, Aug 28, 2023 at 05:09:28PM +0100, Jonathan Cameron wrote:
> On Tue, 15 Aug 2023 18:40:27 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

...

> > -	/* Ensure timestamp is naturally aligned */
> 
> Comment is still true and no more or less obvious than it was with the old code
> I think so I don't really see why it should be removed with this change.

Doesn't the prefix "aligned" make this comment somehow redundant?

> >  	struct {
> >  		u32 accel_val[3];
> > -		s64 timestamp __aligned(8);
> > +		aligned_s64 timestamp;
> >  	} scan;

...

> > -		s32 sampled_vals[4] __aligned(16);
> > +		s32 sampled_vals[4];
> 
> Hmm. I can't immediately recall why that aligned(16) is therebut
> it's not related to the rest of this change so I'm not sure we should touch
> it. I don't think we ever required quaternions to be aligned as a whole so
> this does feel odd but in the category of something we should be careful
> touching or at very least do it in a different patch where it stands out.

I have checked the code and find nothing that justifies this, I can split it
to a separate patch, though.

Note, among ISH HID drivers it's the only one with a such...

> > +		aligned_s64 timestamp;
> >  	} scan;


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/3] types: Complement the aligned types with signed 64-bit one

[Jonathan Cameron]

On Tue, 15 Aug 2023 18:40:25 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Some user may want to use aligned signed 64-bit type.
> Provide it for them.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Excellent.  I'll pick this up once the minor things in the first two
users are resolved (as long as no one argues we shouldn't have this
for some reason!)

Jonathan

> ---
>  include/linux/types.h      | 3 ++-
>  include/uapi/linux/types.h | 1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/types.h b/include/linux/types.h
> index 253168bb3fe1..78d87c751ff5 100644
> --- a/include/linux/types.h
> +++ b/include/linux/types.h
> @@ -115,8 +115,9 @@ typedef u64			u_int64_t;
>  typedef s64			int64_t;
>  #endif
>  
> -/* this is a special 64bit data type that is 8-byte aligned */
> +/* These are the special 64-bit data types that are 8-byte aligned */
>  #define aligned_u64		__aligned_u64
> +#define aligned_s64		__aligned_s64
>  #define aligned_be64		__aligned_be64
>  #define aligned_le64		__aligned_le64
>  
> diff --git a/include/uapi/linux/types.h b/include/uapi/linux/types.h
> index 6375a0684052..48b933938877 100644
> --- a/include/uapi/linux/types.h
> +++ b/include/uapi/linux/types.h
> @@ -53,6 +53,7 @@ typedef __u32 __bitwise __wsum;
>   * No conversions are necessary between 32-bit user-space and a 64-bit kernel.
>   */
>  #define __aligned_u64 __u64 __attribute__((aligned(8)))
> +#define __aligned_s64 __s64 __attribute__((aligned(8)))
>  #define __aligned_be64 __be64 __attribute__((aligned(8)))
>  #define __aligned_le64 __le64 __attribute__((aligned(8)))
>  

Re: [PATCH v1 1/3] types: Complement the aligned types with signed 64-bit one

[Andy Shevchenko]

On Mon, Aug 28, 2023 at 05:10:18PM +0100, Jonathan Cameron wrote:
> On Tue, 15 Aug 2023 18:40:25 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > Some user may want to use aligned signed 64-bit type.
> > Provide it for them.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Excellent.  I'll pick this up once the minor things in the first two
> users are resolved (as long as no one argues we shouldn't have this
> for some reason!)

I'll cook a new series sooner than later. Thank you for the review!

-- 
With Best Regards,
Andy Shevchenko