[resend v4 03/12] drivers: i3c: Add i3c uclass driver.

[Heinrich Schuchardt]

On 4/17/25 04:18, dinesh.maniyam at altera.com wrote:
> From: Dinesh Maniyam <dinesh.maniyam at altera.com>
> 
> Enable i3c general uclass driver. This uclass driver will have
> genaral read and write api to call the specific i3c driver.
> 
> Signed-off-by: Dinesh Maniyam <dinesh.maniyam at altera.com>
> ---
>   drivers/i3c/i3c-uclass.c | 38 ++++++++++++++++++++++++
>   include/dw-i3c.h         |  1 +
>   include/i3c.h            | 63 ++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 102 insertions(+)
>   create mode 100644 drivers/i3c/i3c-uclass.c
>   create mode 100644 include/i3c.h
> 
> diff --git a/drivers/i3c/i3c-uclass.c b/drivers/i3c/i3c-uclass.c
> new file mode 100644
> index 00000000000..644f5788898
> --- /dev/null
> +++ b/drivers/i3c/i3c-uclass.c
> @@ -0,0 +1,38 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2025 Altera Corporation <www.altera.com>
> + */
> +
> +#include <dm.h>
> +#include <i3c.h>
> +#include <errno.h>
> +#include <log.h>
> +#include <dm/device-internal.h>
> +#include <linux/ctype.h>
> +
> +int dm_i3c_read(struct udevice *dev, u8 dev_number,
> +		u8 *buf, int num_bytes)
> +{
> +	struct dm_i3c_ops *ops = i3c_get_ops(dev);
> +
> +	if (!ops->read)
> +		return -ENOSYS;
> +
> +	return ops->read(dev, dev_number, buf, num_bytes);
> +}
> +
> +int dm_i3c_write(struct udevice *dev, u8 dev_number,
> +		 u8 *buf, int num_bytes)
> +{
> +	struct dm_i3c_ops *ops = i3c_get_ops(dev);
> +
> +	if (!ops->write)
> +		return -ENOSYS;
> +
> +	return ops->write(dev, dev_number, buf, num_bytes);
> +}
> +
> +UCLASS_DRIVER(i3c) = {
> +	.id		= UCLASS_I3C,
> +	.name		= "i3c",
> +};
> diff --git a/include/dw-i3c.h b/include/dw-i3c.h
> index e37fd4dc325..920f18bccb4 100644
> --- a/include/dw-i3c.h
> +++ b/include/dw-i3c.h
> @@ -7,6 +7,7 @@
>   #define _DW_I3C_H_
>   
>   #include <clk.h>
> +#include <i3c.h>
>   #include <reset.h>
>   #include <dm/device.h>
>   #include <linux/bitops.h>
> diff --git a/include/i3c.h b/include/i3c.h
> new file mode 100644
> index 00000000000..6ef4982dcf6
> --- /dev/null
> +++ b/include/i3c.h
> @@ -0,0 +1,63 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (C) 2025 Altera Corporation <www.altera.com>
> + */
> +
> +#include <linux/i3c/master.h>
> +
> +/**
> + * struct dm_i3c_ops - driver operations for i3c uclass
> + *
> + * Drivers should support these operations unless otherwise noted. These
> + * operations are intended to be used by uclass code, not directly from
> + * other code.
> + */
> +struct dm_i3c_ops {
> +	/**
> +	 * Transfer messages in I3C mode.
> +	 *
> +	 * @see i3c_transfer
> +	 *
> +	 * @param dev Pointer to controller device driver instance.
> +	 * @param target Pointer to target device descriptor.
> +	 * @param msg Pointer to I3C messages.
> +	 * @param num_msgs Number of messages to transfer.

We follow
https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation

@param looks wrong.

Please, provide formally valid descriptions.

> +	 *
> +	 * @return @see i3c_transfer

%s/@return/Return:/

> +	 */

Please, add the header to doc/api/ and run `make htmldocs`. This will 
highlight all formal documentation errors.

> +	int (*i3c_xfers)(struct i3c_dev_desc *dev,
> +			 struct i3c_priv_xfer *i3c_xfers,
> +			 int i3c_nxfers);

Why do you allow negative values of i3c_nxfers?

The parameter names should match the description.
> +};
> +

Please, add the missing documentation of the macro.

> +#define i3c_get_ops(dev)	((struct dm_i3c_ops *)(dev)->driver->ops)
> +
> +/**
> + * @brief Do i3c write
> + *
> + * Uclass general function to start write to i3c target
> + *
> + * @udevice pointer to i3c controller.
> + * @dev_number target device number.
> + * @buf target Buffer to write.
> + * @num_bytes length of bytes to write.

After each parameter a colon ':' is missing.

> + *
> + * @return 0 for success

%s/@return/Return:/

> + */
> +int dm_i3c_write(struct udevice *dev, u8 dev_number,
> +		 u8 *buf, int num_bytes);

What would be the meaning of a negative value of num_bytes?
An unsigned type probably would be more appropriate.

> +
> +/**
> + * @brief Do i3c read
> + *
> + * Uclass general function to start read from i3c target
> + *
> + * @udevice pointer to i3c controller.
> + * @dev_number target device number.
> + * @buf target Buffer to read.
> + * @num_bytes length of bytes to read.
> + *
> + * @return 0 for success
> + */
> +int dm_i3c_read(struct udevice *dev, u8 dev_number,
> +		u8 *buf, int num_bytes);

ditto

Best regards

Heinrich