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

[Maniyam, Dinesh]

> -----Original Message-----
> From: Heinrich Schuchardt <xypron.glpk at gmx.de>
> Sent: Thursday, 17 April 2025 3:40 pm
> To: Maniyam, Dinesh <dinesh.maniyam at altera.com>
> Cc: Marek <marex at denx.de>; Simon <simon.k.r.goldschmidt at gmail.com>;
> Simon Glass <sjg at chromium.org>; Tom Rini <trini at konsulko.com>; Dario
> Binacchi <dario.binacchi at amarulasolutions.com>; Ilias Apalodimas
> <ilias.apalodimas at linaro.org>; Jerome Forissier <jerome.forissier at linaro.org>;
> Mattijs Korpershoek <mkorpershoek at baylibre.com>; Ibai Erkiaga <ibai.erkiaga-
> elorza at amd.com>; Michal Simek <michal.simek at amd.com>; Dmitry Rokosov
> <ddrokosov at salutedevices.com>; Jonas Karlman <jonas at kwiboo.se>; Sebastian
> Reichel <sebastian.reichel at collabora.com>; Meng, Tingting
> <tingting.meng at altera.com>; Chee, Tien Fong <tien.fong.chee at altera.com>;
> Hea, Kok Kiang <kok.kiang.hea at altera.com>; Ng, Boon Khai
> <boon.khai.ng at altera.com>; Yuslaimi, Alif Zakuan
> <alif.zakuan.yuslaimi at altera.com>; Zamri, Muhammad Hazim Izzat
> <muhammad.hazim.izzat.zamri at altera.com>; Lim, Jit Loon
> <jit.loon.lim at altera.com>; Tang, Sieu Mun <sieu.mun.tang at altera.com>; u-
> boot at lists.denx.de
> Subject: Re: [resend v4 03/12] drivers: i3c: Add i3c uclass driver.
> 
> [CAUTION: This email is from outside your organization. Unless you trust the
> sender, do not click on links or open attachments as it may be a fraudulent email
> attempting to steal your information and/or compromise your computer.]
> 
> 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.
> 

Noted, will update the param and provide valid descriptions as per the documentation.

> > +      *
> > +      * @return @see i3c_transfer
> 
> %s/@return/Return:/
> 

Will be replaced.

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

Noted. Will add the header to check for the error in the documentation.

> > +     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?
> 

Should avoid the negative value. Will use the unsigned.

> The parameter names should match the description.

Will recheck the parameter names.

> > +};
> > +
> 
> Please, add the missing documentation of the macro.
> 

Noted. I will add it.

> > +#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.
> 

Will correct it.

> > + *
> > + * @return 0 for success
> 
> %s/@return/Return:/
> 

Will be replaced.

> > + */
> > +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.
> 

Yes you are right. Will use the unsigned.

> > +
> > +/**
> > + * @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
> 

Will use the unsigned.

> Best regards
> 
> Heinrich

Thanks for your time.
I will rework the comments and submit v5.

Dinesh