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

[Lee Jones <lee@kernel.org>]

On Wed, 11 Nov 2026, sashiko-bot@kernel.org wrote:

> 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

All of these are hard blockers.

Why are you adding a whole new subsystem for your device?

This clearly lives in drivers/mfd.

> --
> 
> 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

-- 
Lee Jones