> -----Original Message-----
> From: Heinrich Schuchardt <xypron.g...@gmx.de>
> Sent: Thursday, 17 April 2025 3:40 pm
> To: Maniyam, Dinesh <dinesh.mani...@altera.com>
> Cc: Marek <ma...@denx.de>; Simon <simon.k.r.goldschm...@gmail.com>;
> Simon Glass <s...@chromium.org>; Tom Rini <tr...@konsulko.com>; Dario
> Binacchi <dario.binac...@amarulasolutions.com>; Ilias Apalodimas
> <ilias.apalodi...@linaro.org>; Jerome Forissier <jerome.foriss...@linaro.org>;
> Mattijs Korpershoek <mkorpersh...@baylibre.com>; Ibai Erkiaga <ibai.erkiaga-
> elo...@amd.com>; Michal Simek <michal.si...@amd.com>; Dmitry Rokosov
> <ddroko...@salutedevices.com>; Jonas Karlman <jo...@kwiboo.se>; Sebastian
> Reichel <sebastian.reic...@collabora.com>; Meng, Tingting
> <tingting.m...@altera.com>; Chee, Tien Fong <tien.fong.c...@altera.com>;
> Hea, Kok Kiang <kok.kiang....@altera.com>; Ng, Boon Khai
> <boon.khai...@altera.com>; Yuslaimi, Alif Zakuan
> <alif.zakuan.yusla...@altera.com>; Zamri, Muhammad Hazim Izzat
> <muhammad.hazim.izzat.za...@altera.com>; Lim, Jit Loon
> <jit.loon....@altera.com>; Tang, Sieu Mun <sieu.mun.t...@altera.com>; u-
> b...@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.mani...@altera.com wrote:
> > From: Dinesh Maniyam <dinesh.mani...@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.mani...@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
