Re: [PATCH 3/3] i3c: add i3cdev character device module for user-space access

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Fri, Sep 11, 2026 at 02:09:35PM -0700, Meagan Lloyd wrote:
> The i3cdev driver is a character device driver that allows user-space
> to control and interact with I3C devices.

> Currently, it has the ability to perform Single Data Rate (SDR)
> transfers - basic reads/writes.
> 
> With the addition of sysfs driver_override, there is now a
> straightforward and direct way to match the i3cdev driver to any i3c
> device without stepping on the toes of more specialized drivers that are
> loaded automatically.

Is it safe? Why on the earth do we need this? The commit message has not enough
information.

> This is accomplished by the i3cdev driver not having any entries in the
> i3c_device_id table. After boot, simply set the driver_override to
> "i3cdev" and bind the device manually via the sysfs bind knob. This can
> also be automated with udev rules as well.
> 
> The character device interface will be exposed at:
> /dev/bus/i3c/<bus id>-<Provisional ID>

...

> +	struct i3c_xfer xfer = {
> +		.rnw = I3C_WRITE

In such cases always leave a trailing comma. It will reduce possible churn in
the future.

> +	};

...

> +	return !ret ? len : ret;

My gosh, wouldn't Elvis just work naturally?

	return ret ?: len;

...

> +	for (int i = 0; i < metadata->nxfers; i++) {

Why is 'i' signed?

> +		ret = copy_struct_from_user(k_uxfer,
> +					    sizeof(*k_uxfers),
> +					    uxfer,
> +					    metadata->xfer_size);
> +		if (ret)
> +			goto out_free_k_uxfers;
> +
> +		/* Enforce that padding must be zero */
> +		if (memchr_inv(k_uxfer->pad, 0, sizeof(k_uxfer->pad))) {
> +			ret = -EINVAL;
> +			goto out_free_k_uxfers;
> +		}
> +
> +		uxfer += metadata->xfer_size; /* u8 pointer so use xfer_size */
> +		k_uxfer++;		      /* struct i3cdev_xfer pointer */
> +	}

...

> +		if (!ret)
> +			total_bytes += i3c_xfers[i].len;
> +		else
> +			return ret;

Yeah, you really need to reconsider patterns you use in the code. Here 'else'
is redundant. Homework to understand how (#easy).

...

> +/**
> + * print_i3c_err() - Prints the I3C error encountered during the prior
> + * call to the core's transfer function.
> + * @i3cdev: i3cdev_data object
> + * @metadata: Kernel's copy of i3cdev_xfers (ioctl I3CDEV_XFER input)
> + * @i3c_xfers: i3c_xfer array that was sent to the I3C core

> + * Returns: void

Huh?! Where is this coming from?

> + */

...

Please, rely less on AI and more on the common sense and proof-reading.

-- 
With Best Regards,
Andy Shevchenko