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

[Lee Jones]

On Mon, 16 Nov 2020, Dmitry Osipenko wrote:

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

I always thought it was best to be explicit.

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

Great.  But don't tell me that.  You need to tell the next person who
reads this code, and the next, and the next.  Defines work best for
this.  Comments are also okay.

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

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

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

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

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

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog