Re: [PATCH v5 2/4] mfd: Add driver for Embedded Controller found on Acer Iconia Tab A500

From: Dmitry Osipenko <digetx@gmail.com>
To: Lee Jones <lee.jones@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>,
	Thierry Reding <thierry.reding@gmail.com>,
	Jonathan Hunter <jonathanh@nvidia.com>,
	Dan Murphy <dmurphy@ti.com>, Sebastian Reichel <sre@kernel.org>,
	devicetree@vger.kernel.org, linux-tegra@vger.kernel.org,
	linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 2/4] mfd: Add driver for Embedded Controller found on Acer Iconia Tab A500
Date: Mon, 16 Nov 2020 02:12:28 +0300	[thread overview]
Message-ID: <3ad644fd-cd03-a1e1-36d9-014304fdfcee@gmail.com> (raw)
In-Reply-To: <20201113093747.GJ2787115@dell>

13.11.2020 12:37, Lee Jones пишет:
...
>> +config MFD_ACER_A500_EC
>> +	tristate "Embedded Controller driver for Acer Iconia Tab A500"
> 
> Drop "driver".  This is meant to be describing the device.
> 
>> +	depends on I2C
> 
> depends on OF ?
...
>> +	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.

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

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

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

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