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 12:53:54 +0300	[thread overview]
Message-ID: <0e795281-ca18-fca6-1585-a487bcfabb86@gmail.com> (raw)
In-Reply-To: <20201116084837.GM3718728@dell>

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.