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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0e795281-ca18-fca6-1585-a487bcfabb86@gmail.com>
Date:   Mon, 16 Nov 2020 12:53:54 +0300
From:   Dmitry Osipenko <digetx@...il.com>
To:     Lee Jones <lee.jones@...aro.org>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Thierry Reding <thierry.reding@...il.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Dan Murphy <dmurphy@...com>,
        Sebastian Reichel <sre@...nel.org>, devicetree@...r.kernel.org,
        linux-tegra@...r.kernel.org, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 2/4] mfd: Add driver for Embedded Controller found on
 Acer Iconia Tab A500

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.