On Tue, 28 Sept 2021 at 19:31, Mark Brown [off-list ref] wrote:

On Tue, Sep 28, 2021 at 03:36:08PM +0800, Chunyan Zhang wrote:

quoted

+++ b/drivers/regulator/sc2730-regulator.c

@@ -0,0 +1,502 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018-2021 Unisoc Inc.
+ */

Please make the entire comment a C++ one so things look more
intentional.

Ok.

quoted

+static int debugfs_enable_get(void *data, u64 *val)
+{
+     struct regulator_dev *rdev = data;
+

quoted

+static int debugfs_enable_set(void *data, u64 val)
+{
+     struct regulator_dev *rdev = data;
+

quoted

+static int debugfs_voltage_get(void *data, u64 *val)
+{

quoted

+static int debugfs_voltage_set(void *data, u64 val)
+{

If these were to be implemented they should be in the core as there's
nothing device specific about them (the read side is there), please
remove them from the driver.

Ok.

quoted

+static const struct of_device_id sc2730_regulator_match[] = {
+     { .compatible = "sprd,sc2730-regulator" },
+     {},
+};
+MODULE_DEVICE_TABLE(of, sc2730_regulator_match);

Since this is a part of a MFD I'd not expect it to have a compatible
string?

Since we switched to use devm_of_platform_populate() [1] to register
MFD subdevices, compatible is required, IIUC.

[1] https://elixir.bootlin.com/linux/latest/source/drivers/mfd/sprd-sc27xx-spi.c#L199

Thanks for the review,
Chunyan