Re: [PATCH v4 3/3] i3c: add i3cdev module to expose i3c dev in /dev

[Frank Li <Frank.li@oss.nxp.com>]

On Fri, Sep 11, 2026 at 03:44:40AM +0000, Sam Agazaryan wrote:
> On Wed Sep 9, 2026 at 9:51 PM UTC, Frank Li wrote:
> > On Sun, Sep 06, 2026 at 08:27:47PM +0000, Sam Agazaryan wrote:
> >> From: Vitor Soares <vitor.soares@toradex.com>
> >>
> >> This patch adds userspace character device support for I3C SDR private
> >> transfers via /dev.
> >
> > Add userspace character device ...
>
> Updated in v5.
>
>
> >>
> >>   - Dynamically exposes /dev/bus/i3c/<device> character devices for I3C
> >>     devices when unbound from kernel drivers.
> >>   - Dynamically allocates character device minor numbers using the IDA
> >>     allocator.
> >>   - Implements private SDR read/write transfers via I3C_IOC_PRIV_XFER ioctl
> >>     with 64-bit aligned UAPI data structures.
> >
> > Consider PRIV for SDR, need unified HDR also,
> >
> > I3C_IOC_XFER
> >
>
> At this very moment the production use case we have (OCP Recovery) supports SDR mode. That being said
> supporting HDR mode also makes sense.
>
> >> + * Copyright (c) 2020 Synopsys, Inc. and/or its affiliates.
> >
> > new post, should be 2026
>
> Updated in v5.
>
> >> +
> >> +	i3cdev = kzalloc(sizeof(*i3cdev), GFP_KERNEL);
> >
> > kzalloc_obj()
> >
>
> Updated in v5.
>
> >> +	dev_dbg(&i3c->dev, "Reading %zu bytes.\n", count);
> >> +
> >> +	ret = i3c_device_do_xfers(i3c, &xfers, 1, I3C_SDR);
> >
> > why force to SDR here.
> >
>
> For reads and writes we don't really have an option of passing down a struct denoting cmd mode.
> Is there a different default behavior you had in mind for read/write besides SDR mode?

this is problem, Maybe we can use ioctrl to set default cmd, hdr/ sdr(default).

>
> >> +err_out:
> >> +	mutex_unlock(&i3cdev->xfer_lock);
> >
> > use cleanup guard()
> >
>
> Updated in v5.
>
> >> +	for (i = 0; i < nxfers; i++) {
> >> +		if (xfers[i].rnw) {
> >> +			if (copy_to_user(u64_to_user_ptr(xfers[i].data),
> >> +					 data_ptrs[i], xfers[i].len))
> >
> > Use actual_len,
> >
>
> Updated in v5.
>
> >> +				ret = -EFAULT;
> >> +		}
> >> +	}
> >> +
> >> +err_free_mem:
> >> +	for (j = 0; j < i; j++)
> >> +		kfree(data_ptrs[j]);
> >> +	kfree(k_xfers);
> >
> >
> > use clean up __free(kfree) when alloc.
> >
>
> Updated in v5.
>
> >> +	case BUS_NOTIFY_ADD_DEVICE:
> >> +	case BUS_NOTIFY_UNBOUND_DRIVER:
> >> +		return i3cdev_attach(dev, NULL);
> >
> > Does all i3c devices attach this driver?
> >
>
> Just i3c target devices without a bound driver, the controller should not attach.
> The controller itself is explicitly skipped:
>         if (dev->type == &i3c_masterdev_type || dev->driver)
>             return 0;

How about this driver built-in and other devices build as modules, modules
load maybe later than this driver?

Frank
>
> >> +struct i3c_ioc_priv_xfer {
> >> +	__u64 data;
> >> +	__u16 len;
> >> +	__u8 rnw;
> >
> > In HDR mode,  rnw is command.
> >
>
> Addressing this in the final comment in this message alongside the rest of
> the HDR suggestions.
>
> >> +	__u8 pad[5];
> >> +};
> >> +
> >> +#define I3C_PRIV_XFER_SIZE(N)	\
> >> +	((((sizeof(struct i3c_ioc_priv_xfer)) * (N)) < (1 << _IOC_SIZEBITS)) \
> >> +	? ((sizeof(struct i3c_ioc_priv_xfer)) * (N)) : 0)
> >> +
> >> +#define I3C_IOC_PRIV_XFER(N)	\
> >> +	_IOC(_IOC_READ|_IOC_WRITE, I3C_DEV_IOC_MAGIC, 30, I3C_PRIV_XFER_SIZE(N))
> >
> > I3C_IOC_SDR_XFER, support DDR, TSP, TSL future.
> >
> > Frank
>
>
> On HDR modes - I think it makes sense to ensure the UAPI supports HDR modes now that
> they're integrated into the i3c core. Would you prefer a scheme where we pass
> in a more unified struct to specify either: SDR mode r/w or an HDR mode r/w
> with ioctl?
>
> Drawing inspiration from i3c_xfer:
>
>     struct i3c_ioc_xfer {
>         __u64 data;
>         __u16 len;
>         __u16 actual_len;
>         union {
>             __u8 rnw;
>             __u8 cmd;
>         };
>         __u8 mode;
>         __u8 pad[2];
>     };
>
>
>
> Thanks,
> Sam