Re: [PATCH v11 2/7] mfd: Add driver for ASUS Transformer embedded controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] mfd: bespoke parent-child accessor and global shared header
- [Medium] mfd: hard-coded cell IDs instead of PLATFORM_DEVID_AUTO
--

commit b490e5f8ad65fb577857c831f5de2998a3c0d3de
Author: Michał Mirosław <mirq-linux@rere.qmqm.pl>

mfd: Add driver for ASUS Transformer embedded controller

Support Nuvoton NPCE795-based ECs as used in Asus Transformer TF201, TF300T,
TF300TG, TF300TL and TF700T pad and dock, as well as TF101 dock and TF600T,
P1801-T and TF701T pad. This is a glue driver handling detection and common
operations for EC's functions.

> diff --git a/drivers/mfd/asus-transformer-ec.c b/drivers/mfd/asus-transformer-ec.c
> new file mode 100644
> index 0000000000000..37dcd1b77465d
> --- /dev/null
> +++ b/drivers/mfd/asus-transformer-ec.c

[ ... ]

> +int asus_dockram_access_ctl(struct i2c_client *client, u64 *out, u64 mask,
> +			    u64 xor)
> +{
> +	struct dockram_ec_data *ddata = i2c_get_clientdata(client);
> +	u8 *buf = ddata->ctl_buf;
> +	u64 val;
> +	int ret = 0;

[Severity: Medium]
Does this introduce a bespoke parent-child accessor?

The MFD architecture guidelines strictly prohibit writing bespoke accessors or
helper functions in the parent driver to pass state to child devices, as this
creates tight coupling. Instead, child drivers should retrieve parent data
using standard APIs like dev_get_drvdata(pdev->dev.parent).

[ ... ]

> +static const struct mfd_cell asus_ec_tf101_dock_mfd_devices[] = {
> +	MFD_CELL_BASIC("asus-transformer-ec-battery", NULL, NULL, 0, 1),
> +	MFD_CELL_BASIC("asus-transformer-ec-charger", NULL, NULL, 0, 1),
> +	MFD_CELL_BASIC("asus-transformer-ec-led", NULL, NULL, 0, 1),
> +	MFD_CELL_NAME("asus-transformer-ec-kbc"),
> +	MFD_CELL_NAME("asus-transformer-ec-keys"),
> +};

[Severity: Medium]
Are these hard-coded cell IDs intended?

The MFD guidelines require using PLATFORM_DEVID_AUTO for automatic cell
indexing to avoid sysfs naming collisions and driver load failures. Here it
appears a hard-coded ID of 1 is assigned for the battery, charger, and led
subdevices.

[ ... ]

> +	/* Parent I2C controller uses DMA, ASUS EC and child devices do not */
> +	client->dev.coherent_dma_mask = 0;
> +	client->dev.dma_mask = &client->dev.coherent_dma_mask;
> +
> +	return devm_mfd_add_devices(dev, 0, cells, num_cells, NULL, 0, NULL);
> +}

[Severity: Medium]
Could we pass PLATFORM_DEVID_AUTO instead of 0 for the base id here?

Passing 0 bypasses auto-indexing and violates the MFD subsystem constraints
for cell identification.

> diff --git a/include/linux/mfd/asus-transformer-ec.h b/include/linux/mfd/asus-transformer-ec.h
> new file mode 100644
> index 0000000000000..d09d3b70171ef
> --- /dev/null
> +++ b/include/linux/mfd/asus-transformer-ec.h

[ ... ]

> +/**
> + * struct asusec_core - public part shared with all cells
> + *
> + * @model: firmware version running on the EC
> + * @name: prefix associated with the EC
> + * @dockram: pointer to Dockram's i2c_client
> + * @notify_list: notify list used by cells
> + * @keyboard_mask: mask of keyboard packets
> + */
> +struct asusec_core {
> +	const char *model;
> +	const char *name;
> +	struct i2c_client *dockram;
> +	struct blocking_notifier_head notify_list;
> +	u8 keyboard_mask;
> +};

[Severity: Medium]
Is this shared structure and header required in the global include directory?

This appears to violate the MFD API layering guidelines, which state that
driver-specific header files should not be included in the global
include/linux/mfd/ directory if they are only used by the parent and its
immediate children. They should be kept local to drivers/mfd/ to avoid
tight coupling.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013173504.451391-1-clamor95@gmail.com?part=2