16.11.2020 11:48, 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.
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.
>> ...
>>> 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.
...
>>>> + while (retries-- > 0) {
>>>> + ret = i2c_smbus_read_word_data(client, reg);
>>>> + if (ret >= 0)
>>>> + break;
>>>> +
>>>> + msleep(A500_EC_I2C_ERR_TIMEOUT);
>>>> + }
...
>>> 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?
>> ...
>>>> +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.
