linux-kernel - Re: [PATCH v7 25/34] i2c: tegra: Factor out register polling into separate function

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]

Date:   Thu, 17 Sep 2020 18:05:41 +0300
From:   Dmitry Osipenko <digetx@...il.com>
To:     Thierry Reding <thierry.reding@...il.com>
Cc:     Jonathan Hunter <jonathanh@...dia.com>,
        Laxman Dewangan <ldewangan@...dia.com>,
        Wolfram Sang <wsa@...-dreams.de>,
        Michał Mirosław <mirq-linux@...e.qmqm.pl>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        linux-i2c@...r.kernel.org, linux-tegra@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 25/34] i2c: tegra: Factor out register polling into
 separate function

17.09.2020 14:58, Thierry Reding пишет:
> On Wed, Sep 09, 2020 at 01:39:57AM +0300, Dmitry Osipenko wrote:
>> Factor out register polling into a separate function in order to remove
>> boilerplate code and make code cleaner.
>>
>> Reviewed-by: Michał Mirosław <mirq-linux@...e.qmqm.pl>
>> Signed-off-by: Dmitry Osipenko <digetx@...il.com>
>> ---
>>  drivers/i2c/busses/i2c-tegra.c | 57 +++++++++++++++-------------------
>>  1 file changed, 25 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
>> index 405b87e28a98..e071de9ce106 100644
>> --- a/drivers/i2c/busses/i2c-tegra.c
>> +++ b/drivers/i2c/busses/i2c-tegra.c
>> @@ -514,10 +514,24 @@ static void tegra_i2c_vi_init(struct tegra_i2c_dev *i2c_dev)
>>  	i2c_writel(i2c_dev, 0x0, I2C_TLOW_SEXT);
>>  }
>>  
>> +static int tegra_i2c_poll_register(struct tegra_i2c_dev *i2c_dev,
>> +				   u32 reg, u32 mask, u32 delay_us,
>> +				   u32 timeout_us)
>> +{
>> +	void __iomem *addr = i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg);
>> +	u32 val;
>> +
>> +	if (!i2c_dev->is_curr_atomic_xfer)
>> +		return readl_relaxed_poll_timeout(addr, val, !(val & mask),
>> +						  delay_us, timeout_us);
>> +
>> +	return readl_relaxed_poll_timeout_atomic(addr, val, !(val & mask),
>> +						 delay_us, timeout_us);
>> +}
>> +
>>  static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
>>  {
>> -	u32 mask, val, offset, reg_offset;
>> -	void __iomem *addr;
>> +	u32 mask, val, offset;
>>  	int err;
>>  
>>  	if (i2c_dev->hw->has_mst_fifo) {
>> @@ -534,16 +548,7 @@ static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
>>  	val |= mask;
>>  	i2c_writel(i2c_dev, val, offset);
>>  
>> -	reg_offset = tegra_i2c_reg_addr(i2c_dev, offset);
>> -	addr = i2c_dev->base + reg_offset;
>> -
>> -	if (i2c_dev->is_curr_atomic_xfer)
>> -		err = readl_relaxed_poll_timeout_atomic(addr, val, !(val & mask),
>> -							1000, 1000000);
>> -	else
>> -		err = readl_relaxed_poll_timeout(addr, val, !(val & mask),
>> -						 1000, 1000000);
>> -
>> +	err = tegra_i2c_poll_register(i2c_dev, offset, mask, 1000, 1000000);
>>  	if (err) {
>>  		dev_err(i2c_dev->dev, "failed to flush FIFO\n");
>>  		return err;
>> @@ -553,30 +558,18 @@ static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
>>  
>>  static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev)
>>  {
>> -	unsigned long reg_offset;
>> -	void __iomem *addr;
>> -	u32 val;
>>  	int err;
>>  
>> -	if (i2c_dev->hw->has_config_load_reg) {
>> -		reg_offset = tegra_i2c_reg_addr(i2c_dev, I2C_CONFIG_LOAD);
>> -		addr = i2c_dev->base + reg_offset;
>> -		i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
>> +	if (!i2c_dev->hw->has_config_load_reg)
>> +		return 0;
>>  
>> -		if (i2c_dev->is_curr_atomic_xfer)
>> -			err = readl_relaxed_poll_timeout_atomic(
>> -						addr, val, val == 0, 1000,
>> -						I2C_CONFIG_LOAD_TIMEOUT);
>> -		else
>> -			err = readl_relaxed_poll_timeout(
>> -						addr, val, val == 0, 1000,
>> -						I2C_CONFIG_LOAD_TIMEOUT);
>> +	i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
>>  
>> -		if (err) {
>> -			dev_warn(i2c_dev->dev,
>> -				 "timeout waiting for config load\n");
>> -			return err;
>> -		}
>> +	err = tegra_i2c_poll_register(i2c_dev, I2C_CONFIG_LOAD, 0xffffffff,
>> +				      1000, I2C_CONFIG_LOAD_TIMEOUT);
>> +	if (err) {
>> +		dev_warn(i2c_dev->dev, "timeout waiting for config load\n");
>> +		return err;
>>  	}
> 
> The deindentation in this hunk is messing up the diffstat in my opinion.
> It would probably be worth splitting that into a separate patch to make
> it more evident that this patch actually removes boilerplate.

This is intentional and it's mentioned in the v7 changelog.

Previously there was another patch that did what you're suggesting, but
Andy Shevchenko objected that it was causing a "ping-pong" code changes
where one patch did a change and then next patch changes the changed
code again.