13.11.2020 12:37, Lee Jones пишет:
...

quoted

+config MFD_ACER_A500_EC
+	tristate "Embedded Controller driver for Acer Iconia Tab A500"

Drop "driver".  This is meant to be describing the device.

quoted

+	depends on I2C

depends on OF ?

...

quoted

+	depends on ARCH_TEGRA_2x_SOC || COMPILE_TEST
+	select MFD_CORE
+	select REGMAP
+	help

ARCH_TEGRA_2x_SOC selects OF.

For COMPILE_TEST it doesn't matter since OF framework provides stubs for
!OF.

...

quoted

+static int a500_ec_read(void *context, const void *reg_buf, size_t reg_size,
+			void *val_buf, size_t val_sizel)
+{
+	struct i2c_client *client = context;
+	unsigned int reg, retries = 5;
+	u16 *ret_val = val_buf;
+	s32 ret = 0;
+
+	if (reg_size != 1 || val_sizel != 2)

No magic numbers please.

What does this *mean*?

These are the size of address register and size of a read value, both in
bytes.

quoted

+		return -EOPNOTSUPP;

Why EOPNOTSUPP?

Other sizes aren't supported by embedded controller.

Although, perhaps this check isn't really needed at all since the sizes
are already expressed in the a500_ec_regmap_config.

I'll need to take a closer look at why this size-checking was added
because don't quite remember already. If it's not needed, then I'll
remove it in the next revision, otherwise will add a clarifying comment.

quoted

+	reg = *(u8 *)reg_buf;
+
+	while (retries-- > 0) {
+		ret = i2c_smbus_read_word_data(client, reg);
+		if (ret >= 0)
+			break;
+
+		msleep(A500_EC_I2C_ERR_TIMEOUT);
+	}
+
+	if (ret < 0) {
+		dev_err(&client->dev, "read 0x%x failed: %d\n", reg, ret);
+		return ret;
+	}
+
+	*ret_val = ret;
+
+	if (reg == REG_CURRENT_NOW)
+		fsleep(10000);

Ooo, new toy!

quoted

+	return 0;
+}

I'm surprised there isn't a generic function which does this kind of
read.  Seems like pretty common/boilerplate stuff.

I'm not quite sure that this is a really very common pattern which
deserves a generic helper.

...

quoted

+static int a500_ec_restart_notify(struct notifier_block *this,
+				  unsigned long reboot_mode, void *data)
+{
+	if (reboot_mode == REBOOT_WARM)
+		i2c_smbus_write_word_data(a500_ec_client_pm_off,
+					  REG_WARM_REBOOT, 0);
+	else
+		i2c_smbus_write_word_data(a500_ec_client_pm_off,
+					  REG_COLD_REBOOT, 1);

What's with the magic '0' and '1's at the end?

These are the values which controller's firmware wants to see, otherwise
it will reject command as invalid. I'll add a clarifying comment to the
code.

Thank you for the review. I'll address all the comments in the v7.