RE: [PATCH v3 02/19] crypto: cmh - add core platform driver

On Tue, Aug 11, 2026 at 10:31:47AM +0200, Krzysztof Kozlowski <[email protected]> wrote:
> > Reviewed-by: Joel Wittenauer <[email protected]>
> > Reviewed-by: Thi Nguyen <[email protected]>
>
> These two reviews did not happen, drop the tags. The code has trivial
> mistakes from downstream code. Internal review would point these trivial
> issues, so I do not believe you received valid, meaningful internal
> review which would justify the tags.

We will drop them. Any future Reviewed-by will come from on-list review.

> > +MODULE_ALIAS("platform:cmh");
>
> Why?

Redundant with the OF match table; we will remove it.

> > +MODULE_IMPORT_NS("CRYPTO_INTERNAL");
>
> Why all this is in completely odd place, not next to the module init
> code?

We will move it next to the module code.

> > +static struct cmh_device *g_cmh_dev;
>
> Eh, no. Don't implement singletons.

We will remove the singleton and use dev_get_drvdata() /
platform_get_drvdata().

> > +	dev_info(cmh_dev(), "CMH eSW fully operational ...\n", sw_boot);
>
> Drop

We will remove the dev_info() logging across the driver,
keeping only genuine error paths.

> > +	if (g_cmh_dev)
> > +		return -EBUSY;
>
> NAK

We will drop the single-instance guard together with the singleton.

> > +	dev_info(&pdev->dev, "loading v%s\n", CMH_VERSION);
>
> NAK and internal review should tell you that

Will remove.

> > +	dev_info(cmh_dev(), "sic_base=0x%llx size=0x%zx mbx_count=%u\n",
>
> How many dev_info are you going to print?

Will remove.

> > +		dev_info(cmh_dev(), "enabled %d clock(s)\n", ret);
>
> Again?

Will remove.

> > +		dev_err(cmh_dev(), "failed to acquire reset GPIO (rc=%d)\n", ret);
>
> Syntax is ret = dev_err_probe and internal review should tell you that.

We will use return dev_err_probe(...) on the probe error paths.

> > +	(void)cmh_debugfs_init(cfg);
>
> Why the cast?

We will drop the cast and check the return.

> > +	g_cmh_dev = dev;
>
> Nope (and we do not call things "g" from globals)

Will be gone with the singleton removal.

> > +	dev_info(cmh_dev(), "initialized successfully\n");
>
> NAK

Will remove.

> > +	dev_info(&pdev->dev, "unloaded successfully\n");
>
> NAK

Will remove.

> > +		.name           = CMH_DRV_NAME,
>
> Use name directly.

We will use "cmh" directly.

> > +	 * g_cmh_dev -- that would defeat deferred probe.
> > +	 */
>
> Why do you need this? Why your driver is done differently than all
> others?

We will drop the custom init/exit and use module_platform_driver().

> > +module_init(cmh_init);
> > +module_exit(cmh_exit);
>
> And this is not module_platform_driver because...?

Will switch to module_platform_driver() in v4.

> > +	dev_info(cmh_dev(), "MQI init complete (%u mailboxes)\n", cfg->mbx_count);
>
> NAK

Will remove.

> > +	dev_info(cmh_dev(), "MQI cleanup complete\n");
>
> NAK

Will remove.

> > +		dev_info(cmh_dev(), "rh: no mailbox IRQs -- polling mode\n");
>
> So a warning or info?

Will remove -- polling is a normal configuration, not worth logging.

> > +		dev_warn(cmh_dev(), "rh: no IRQs -- using watchdog polling ...\n",
>
> And here it is a warning?

Will remove.

> > +	{
>
> What is with this indentation?

We will drop the naked block and restructure.

> > +	dev_info(cmh_dev(), "rh: initialized (%u IRQs, ...)\n",
>
> One more dev_info....

Will remove.

> > +	dev_dbg(cmh_dev(), "rh: suspended\n");
>
> No, you cannot have even debug messages which re-implement and
> duplicated standard tracing and standard debugging code.

We will remove these debug messages.

> > +static unsigned int backlog_max_depth = 1024;
> > +module_param(backlog_max_depth, uint, 0444);
>
> Drop all module params, not really right way to do things.

We will drop all module parameters and move the genuinely useful knobs
to debugfs.

> > +#define CMH_VERSION    "1.0.0"
>
> NAK, this is not allowed in Linux kernel code. Do you see drivers
> (except complete junk from AMD) doing this?

We will remove the driver-version define entirely.

Thanks for the review.

Regards,
Alex