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.
