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

On Tue, Aug 11, 2026 at 10:31:47AM +0200, Krzysztof Kozlowski [off-list ref] wrote:

quoted

Reviewed-by: Joel Wittenauer <redacted>
Reviewed-by: Thi Nguyen <redacted>

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.

quoted

+MODULE_ALIAS("platform:cmh");

Why?

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

quoted

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

quoted

+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().

quoted

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

quoted

+	if (g_cmh_dev)
+		return -EBUSY;

NAK

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

quoted

+	dev_info(&pdev->dev, "loading v%s\n", CMH_VERSION);

NAK and internal review should tell you that

Will remove.

quoted

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

quoted

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

Again?

Will remove.

quoted

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

quoted

+	(void)cmh_debugfs_init(cfg);

Why the cast?

We will drop the cast and check the return.

quoted

+	g_cmh_dev = dev;

Nope (and we do not call things "g" from globals)

Will be gone with the singleton removal.

quoted

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

NAK

Will remove.

quoted

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

NAK

Will remove.

quoted

+		.name           = CMH_DRV_NAME,

Use name directly.

We will use "cmh" directly.

quoted

+	 * 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().

quoted

+module_init(cmh_init);
+module_exit(cmh_exit);

And this is not module_platform_driver because...?

Will switch to module_platform_driver() in v4.

quoted

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

NAK

Will remove.

quoted

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

NAK

Will remove.

quoted

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

quoted

+		dev_warn(cmh_dev(), "rh: no IRQs -- using watchdog polling ...\n",

And here it is a warning?

Will remove.

quoted

+	{

What is with this indentation?

We will drop the naked block and restructure.

quoted

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

One more dev_info....

Will remove.

quoted

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

quoted

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

quoted

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