Re: [PATCH v5 1/3] bus: mhi: host: clients: Add loopback driver with sysfs interface

["Uwe Kleine-König" <u.kleine-koenig@baylibre.com>]

[-- Attachment #1: Type: text/plain, Size: 2199 bytes --]

Hello,

On Mon, Aug 17, 2026 at 03:44:34PM +0530, Sumit Kumar wrote:
> +#include <linux/mhi.h>
> +#include <linux/mod_devicetable.h>

Please rely on <linux/mhi.h> to provide mhi_device_id and drop the
inclusion for <linux/mod_devicetable.h>. The latter header is ugly and
should go away soon.

> [...]
> +static struct attribute *mhi_loopback_attrs[] = {
> +	&dev_attr_tre_size.attr,
> +	&dev_attr_max_tre_size.attr,
> +	&dev_attr_num_tre.attr,
> +	&dev_attr_start.attr,
> +	NULL,

Please no , after sentinel entries.

> +};
> +
> +static const struct attribute_group mhi_loopback_group = {
> +	.attrs = mhi_loopback_attrs,
> +};
> +
> +static int mhi_loopback_probe(struct mhi_device *mhi_dev,
> +			      const struct mhi_device_id *id)
> +{
> +	struct mhi_loopback *loopback;
> +	int ret;
> +
> +	loopback = devm_kzalloc(&mhi_dev->dev, sizeof(*loopback), GFP_KERNEL);
> +	if (!loopback)
> +		return -ENOMEM;
> +
> +	loopback->mdev = mhi_dev;
> +	loopback->tre_size = MHI_LOOPBACK_DEFAULT_TRE_SIZE;
> +	loopback->num_tre = MHI_LOOPBACK_DEFAULT_NUM_TRE;
> +
> +	mutex_init(&loopback->lb_mutex);
> +	init_completion(&loopback->comp);
> +
> +	dev_set_drvdata(&mhi_dev->dev, loopback);
> +
> +	ret = mhi_prepare_for_transfer(mhi_dev);
> +	if (ret) {
> +		dev_err(&mhi_dev->dev, "Failed to prepare for transfers: %d\n", ret);

dev_err_probe() please.

> +		return ret;
> +	}
> +
> +	ret = sysfs_create_group(&mhi_dev->dev.kobj, &mhi_loopback_group);
> +	if (ret) {
> +		dev_err(&mhi_dev->dev, "Failed to create sysfs attributes: %d\n", ret);
> +		mhi_unprepare_from_transfer(mhi_dev);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void mhi_loopback_remove(struct mhi_device *mhi_dev)
> +{
> +	/* Blocks until any in-progress store() has returned */
> +	sysfs_remove_group(&mhi_dev->dev.kobj, &mhi_loopback_group);
> +	mhi_unprepare_from_transfer(mhi_dev);
> +}
> +
> +static const struct mhi_device_id mhi_loopback_id_table[] = {
> +	{ .chan = "LOOPBACK"},
> +	{}

{ } (i.e. a space between the curly braces) is the more common way to
write that. Please also add a space before the closing } in the line
above.

> +};
> +MODULE_DEVICE_TABLE(mhi, mhi_loopback_id_table);

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]