16.11.2020 11:48, Lee Jones пишет:

quoted

quoted

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

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.

I always thought it was best to be explicit.

Alright, I see that the OF selection is all over the MFD Kconfig, hence
let's keep it consistent.

I also prefer the explicit variant more, but some other kernel
maintainers may disagree.

quoted

...

quoted

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.

"Operation not supported on transport endpoint" doesn't seem like a
good fit here.  If the check says, please consider changing it to
something like -EINVAL.

The regmap core code performs all the necessary checks before driver's
code is reached, perhaps I just overlooked something before. Hence it
will be removed in the next revision.

...

quoted

quoted

quoted

+	while (retries-- > 0) {
+		ret = i2c_smbus_read_word_data(client, reg);
+		if (ret >= 0)
+			break;
+
+		msleep(A500_EC_I2C_ERR_TIMEOUT);
+	}

...

quoted

quoted

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.

What?  Read from I2C a few times, then sleep sounds like a pretty
common pattern to me.

Maybe the read_poll_timeout() helper could be used for that, but I think
the open-coded variant is much easier to perceive, don't you agree?

quoted

...

quoted

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.

Thanks.  Hopefully with a bit more information as to why the firmware
expects to see them.  Hopefully they're not random.

These are the firmware-defined specific command opcodes, I'll add
defines for them to make it more clear, thanks.