[PATCH v2 2/4] serdev: add private data to serdev_device

Markus Probst posted 4 patches 1 month ago
There is a newer version of this series
[PATCH v2 2/4] serdev: add private data to serdev_device
Posted by Markus Probst 1 month ago
Add private data to `struct serdev_device`, as it is required by the
rust abstraction added in the following commit
(rust: add basic serial device bus abstractions).

Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
 include/linux/serdev.h | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/include/linux/serdev.h b/include/linux/serdev.h
index 5654c58eb73c..b591af23faf0 100644
--- a/include/linux/serdev.h
+++ b/include/linux/serdev.h
@@ -33,12 +33,13 @@ struct serdev_device_ops {
 
 /**
  * struct serdev_device - Basic representation of an serdev device
- * @dev:	Driver model representation of the device.
- * @nr:		Device number on serdev bus.
- * @ctrl:	serdev controller managing this device.
- * @ops:	Device operations.
- * @write_comp	Completion used by serdev_device_write() internally
- * @write_lock	Lock to serialize access when writing data
+ * @dev:	 Driver model representation of the device.
+ * @nr:		 Device number on serdev bus.
+ * @ctrl:	 serdev controller managing this device.
+ * @ops:	 Device operations.
+ * @write_comp	 Completion used by serdev_device_write() internally
+ * @write_lock	 Lock to serialize access when writing data
+ * @private_data Private data for the device driver.
  */
 struct serdev_device {
 	struct device dev;
@@ -47,6 +48,7 @@ struct serdev_device {
 	const struct serdev_device_ops *ops;
 	struct completion write_comp;
 	struct mutex write_lock;
+	void *private_data;
 };
 
 static inline struct serdev_device *to_serdev_device(struct device *d)

-- 
2.52.0
Re: [PATCH v2 2/4] serdev: add private data to serdev_device
Posted by Danilo Krummrich 1 month ago
On Fri Mar 6, 2026 at 8:35 PM CET, Markus Probst wrote:
> Add private data to `struct serdev_device`, as it is required by the
> rust abstraction added in the following commit
> (rust: add basic serial device bus abstractions).
>
> Signed-off-by: Markus Probst <markus.probst@posteo.de>
> ---
>  include/linux/serdev.h | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/serdev.h b/include/linux/serdev.h
> index 5654c58eb73c..b591af23faf0 100644
> --- a/include/linux/serdev.h
> +++ b/include/linux/serdev.h
> @@ -33,12 +33,13 @@ struct serdev_device_ops {
>  
>  /**
>   * struct serdev_device - Basic representation of an serdev device
> - * @dev:	Driver model representation of the device.
> - * @nr:		Device number on serdev bus.
> - * @ctrl:	serdev controller managing this device.
> - * @ops:	Device operations.
> - * @write_comp	Completion used by serdev_device_write() internally
> - * @write_lock	Lock to serialize access when writing data
> + * @dev:	 Driver model representation of the device.
> + * @nr:		 Device number on serdev bus.
> + * @ctrl:	 serdev controller managing this device.
> + * @ops:	 Device operations.
> + * @write_comp	 Completion used by serdev_device_write() internally
> + * @write_lock	 Lock to serialize access when writing data
> + * @private_data Private data for the device driver.

I think this is a bit misleading, as the driver's device private data is stored
in the embedded struct device. This seems to be more about having a place to
store private data of the Rust abstraction.

So, you may want to name this something along the lines of rust_private_data.
Additionally, you should make it very clear that this field must not be used by
drivers directly (i.e. C drivers could easily be tempted to abuse this).

>   */
>  struct serdev_device {
>  	struct device dev;
> @@ -47,6 +48,7 @@ struct serdev_device {
>  	const struct serdev_device_ops *ops;
>  	struct completion write_comp;
>  	struct mutex write_lock;
> +	void *private_data;
>  };
>  
>  static inline struct serdev_device *to_serdev_device(struct device *d)
>
> -- 
> 2.52.0
Re: [PATCH v2 2/4] serdev: add private data to serdev_device
Posted by Randy Dunlap 1 month ago
Hi--

On 3/6/26 11:35 AM, Markus Probst wrote:
> Add private data to `struct serdev_device`, as it is required by the
> rust abstraction added in the following commit
> (rust: add basic serial device bus abstractions).
> 
> Signed-off-by: Markus Probst <markus.probst@posteo.de>
> ---
>  include/linux/serdev.h | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/serdev.h b/include/linux/serdev.h
> index 5654c58eb73c..b591af23faf0 100644
> --- a/include/linux/serdev.h
> +++ b/include/linux/serdev.h
> @@ -33,12 +33,13 @@ struct serdev_device_ops {
>  
>  /**
>   * struct serdev_device - Basic representation of an serdev device
> - * @dev:	Driver model representation of the device.
> - * @nr:		Device number on serdev bus.
> - * @ctrl:	serdev controller managing this device.
> - * @ops:	Device operations.
> - * @write_comp	Completion used by serdev_device_write() internally
> - * @write_lock	Lock to serialize access when writing data
> + * @dev:	 Driver model representation of the device.
> + * @nr:		 Device number on serdev bus.
> + * @ctrl:	 serdev controller managing this device.
> + * @ops:	 Device operations.
> + * @write_comp	 Completion used by serdev_device_write() internally
> + * @write_lock	 Lock to serialize access when writing data
> + * @private_data Private data for the device driver.
>   */

I don't quite get why each changed line has an extra tab added to it. ?
Also, struct member names in kernel-doc should with a colon  (':'), e.g.,

 * @private_data: Private data for the device driver.

Please correct that and the 2 lines above it also.
And maybe test it to check for warnings.

>  struct serdev_device {
>  	struct device dev;
> @@ -47,6 +48,7 @@ struct serdev_device {
>  	const struct serdev_device_ops *ops;
>  	struct completion write_comp;
>  	struct mutex write_lock;
> +	void *private_data;
>  };
>  
>  static inline struct serdev_device *to_serdev_device(struct device *d)
> 

thanks.
-- 
~Randy
Re: [PATCH v2 2/4] serdev: add private data to serdev_device
Posted by Markus Probst 1 month ago
On Fri, 2026-03-06 at 11:49 -0800, Randy Dunlap wrote:
> Hi--
> 
> On 3/6/26 11:35 AM, Markus Probst wrote:
> > Add private data to `struct serdev_device`, as it is required by the
> > rust abstraction added in the following commit
> > (rust: add basic serial device bus abstractions).
> > 
> > Signed-off-by: Markus Probst <markus.probst@posteo.de>
> > ---
> >  include/linux/serdev.h | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/include/linux/serdev.h b/include/linux/serdev.h
> > index 5654c58eb73c..b591af23faf0 100644
> > --- a/include/linux/serdev.h
> > +++ b/include/linux/serdev.h
> > @@ -33,12 +33,13 @@ struct serdev_device_ops {
> >  
> >  /**
> >   * struct serdev_device - Basic representation of an serdev device
> > - * @dev:	Driver model representation of the device.
> > - * @nr:		Device number on serdev bus.
> > - * @ctrl:	serdev controller managing this device.
> > - * @ops:	Device operations.
> > - * @write_comp	Completion used by serdev_device_write() internally
> > - * @write_lock	Lock to serialize access when writing data
> > + * @dev:	 Driver model representation of the device.
> > + * @nr:		 Device number on serdev bus.
> > + * @ctrl:	 serdev controller managing this device.
> > + * @ops:	 Device operations.
> > + * @write_comp	 Completion used by serdev_device_write() internally
> > + * @write_lock	 Lock to serialize access when writing data
> > + * @private_data Private data for the device driver.
> >   */
> 
> I don't quite get why each changed line has an extra tab added to it. ?
So all the fields have the same indentation.
Otherwise the description of private_data, because of its length, would
be 1 character more to the right.

> Also, struct member names in kernel-doc should with a colon  (':'), e.g.,
> 
>  * @private_data: Private data for the device driver.
> 
> Please correct that and the 2 lines above it also.
> And maybe test it to check for warnings.
I can do that.

> 
> >  struct serdev_device {
> >  	struct device dev;
> > @@ -47,6 +48,7 @@ struct serdev_device {
> >  	const struct serdev_device_ops *ops;
> >  	struct completion write_comp;
> >  	struct mutex write_lock;
> > +	void *private_data;
> >  };
> >  
> >  static inline struct serdev_device *to_serdev_device(struct device *d)
> > 
> 
> thanks.

Thanks
- Markus Probst