Re: [PATCH v1 1/7] zorro: warp mfd: add the CS-Lab Warp multifunction parent

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Low] mfd: warp_mfd_probe() places new MFD core driver in
  drivers/zorro/
- [Low] mfd: warp_mfd_driver hard-codes mfd in names and uses wrong
  subject prefix
- [Low] mfd: warp_mfd_cells registers only a single child cell and
  Kconfig mismatches code
- [Low] mfd: warp_mfd_probe() passes z->dev.id instead of
  PLATFORM_DEVID_AUTO
--

commit 9af1a8469b32d3686430ab985b8dd9f3f752e76e
Author: Paolo Pisati <p.pisati@gmail.com>

zorro: warp mfd: add the CS-Lab Warp multifunction parent

This commit adds a Zorro parent driver for the CS-Lab Warp 1260 board that
registers the on-board IDE window as an mfd cell for pata_cswarp.

> diff --git a/drivers/zorro/Kconfig b/drivers/zorro/Kconfig
> index 41475ad46e45f..c9ab9fb4b9ee7 100644
> --- a/drivers/zorro/Kconfig
> +++ b/drivers/zorro/Kconfig
> @@ -17,3 +17,33 @@ config ZORRO_NAMES
>  
>  	  When in doubt, say Y.
>  
> +# Zorro multifunction drivers
> +
> +config WARP_MFD
> +	tristate "CS-Lab Warp multifunction driver"
> +	depends on ZORRO
> +	select MFD_CORE
[ ... ]
> +	  This driver hands the IDE window to pata_cswarp - which needs
> +	  idemode=native set in Warp Configuration to be of any use - and
> +	  exposes the configuration and telemetry reachable over the dual-port
> +	  RAM in sysfs: voltages, temperatures, fan, firmware version and
> +	  Kickstart slot. Nothing else on the list has a driver yet.
[ ... ]
> diff --git a/drivers/zorro/Makefile b/drivers/zorro/Makefile
> index 91ba82e633e76..6fe7cc1822dc1 100644
> --- a/drivers/zorro/Makefile
> +++ b/drivers/zorro/Makefile
> @@ -7,6 +7,8 @@ obj-$(CONFIG_ZORRO)	+= zorro.o zorro-driver.o zorro-sysfs.o
>  obj-$(CONFIG_PROC_FS)	+= proc.o
>  obj-$(CONFIG_ZORRO_NAMES) +=  names.o
>  
> +obj-$(CONFIG_WARP_MFD) += warp-mfd.o

[Severity: Low]
Should this driver and its local header live under drivers/mfd/ instead of
drivers/zorro/?  Per the mfd subsystem guidelines, new core mfd drivers
that call mfd_add_devices() or devm_mfd_add_devices(), along with headers
shared only between the parent and its children, belong in drivers/mfd/.

> diff --git a/drivers/zorro/warp-mfd.c b/drivers/zorro/warp-mfd.c
> new file mode 100644
> index 0000000000000..94af7c9d0ea5e
> --- /dev/null
> +++ b/drivers/zorro/warp-mfd.c
[ ... ]
> +static const struct mfd_cell warp_mfd_cells[] = {
> +	{
> +		.name		= "warp-pata",
> +		.num_resources  = 1,
> +		.resources	= (const struct resource []) {
> +			DEFINE_RES_MEM_NAMED(WARP_OFFSET_ATA, WARP_ATA_SIZE,
> +					     "pata"),
> +		}
> +	}
> +};

[Severity: Low]
Is an mfd driver appropriate when warp_mfd_cells[] only registers a single
child device?  The mfd subsystem guidelines require mfd to be used only for
devices registering multiple children across different subsystems.

In addition, the Kconfig help text states that this driver exposes
dual-port RAM configuration and telemetry in sysfs (voltages, temperatures,
fan, firmware version, and Kickstart slot), none of which is present in
this commit, and drivers/zorro/warp-mfd.h defines dual-port RAM offsets and
sizes that have no users here.  Even later in the series, telemetry is
added as custom sysfs attributes on the parent Zorro device rather than as
a child device in the appropriate subsystem such as hwmon.

> +static int warp_mfd_probe(struct zorro_dev *z,
> +			  const struct zorro_device_id *ent)
> +{
> +	int error;
> +
> +	dev_info(&z->dev, "Warp at %pR\n", &z->resource);
> +
> +	error = mfd_add_devices(&z->dev, z->dev.id, warp_mfd_cells,
> +				ARRAY_SIZE(warp_mfd_cells), &z->resource,
> +				0, NULL);
> +	if (error) {
> +		dev_err(&z->dev, "mfd_add_devices() failed %d\n", error);
> +		return error;
> +	}
> +	return 0;
> +}
> +
> +static void warp_mfd_remove(struct zorro_dev *z)
> +{
> +	mfd_remove_devices(&z->dev);
> +}

[Severity: Low]
This isn't a bug, but mfd guidelines prefer automatic platform device IDs
over passing z->dev.id to mfd_add_devices() when subdevices do not require
explicit numbering.  Using devm_mfd_add_devices() would also drop
warp_mfd_remove() and keep child teardown ordered with devres resources
added later in the series.

Similarly, warp_mfd_cells[] could use the standard mfd cell resource helper
macro instead of open-coding .num_resources and a compound literal, and
warp_mfd_init() / warp_mfd_exit() could use module_driver().

[ ... ]

> +static struct zorro_driver warp_mfd_driver = {
> +	.name		= "warp-mfd",
> +	.id_table	= warp_mfd_ids,
> +	.probe		= warp_mfd_probe,
> +	.remove		= warp_mfd_remove
> +};

[Severity: Low]
This isn't a bug, but mfd subsystem conventions avoid hard-coding "mfd" in
driver, symbol, Kconfig, and file names (such as "warp-mfd",
warp_mfd_driver, and warp-mfd.c), and expect commit subjects in the form
"mfd: <driver>: <Capitalized description>".

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111163735.227286-1-p.pisati@gmail.com?part=1