linux-kernel - Re: [PATCH v5 2/7] i2c: designware: refactoring of the i2c-designware

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:   Wed, 28 Dec 2016 15:30:51 +0000
From:   Luis Oliveira <Luis.Oliveira@...opsys.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Luis Oliveira <Luis.Oliveira@...opsys.com>,
        <wsa@...-dreams.de>, <robh+dt@...nel.org>, <mark.rutland@....com>,
        <jarkko.nikula@...ux.intel.com>, <mika.westerberg@...ux.intel.com>,
        <linux-i2c@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
CC:     <Ramiro.Oliveira@...opsys.com>, <Joao.Pinto@...opsys.com>,
        <CARLOS.PALMINHA@...opsys.com>
Subject: Re: [PATCH v5 2/7] i2c: designware: refactoring of the i2c-designware

On 28-Dec-16 15:12, Andy Shevchenko wrote:
> On Wed, 2016-12-28 at 14:43 +0000, Luis Oliveira wrote:
>> - Factor out all _master() part of code from i2c-designware-core
>>   and i2c-designware-platdrv to separate functions.
>> - Standardize all code related with MASTER mode.
>> - I have to take off DW_IC_INTR_TX_EMPTY from DW_IC_INTR_DEFAULT_MASK
>>   because it is master specific.
>>
>> The purpose of this is to prepare the controller to have is I2C MASTER
>> flow in a separate driver. To do this first all the
>> functions/definitions related to the MASTER flow were identified.
> 
> Thanks for an update.
> Some style related comments below (For the code related is up to you, my
> tag still stands).
> 
>>
>> Signed-off-by: Luis Oliveira <lolivei@...opsys.com>
>> ---
>> Changes V4->V5: (ACK by Andy)
> 
> When you get an Ack, or other tag (Reviewed-by, Tested-by, etc), and you
> send new version, include this tag to your commit message (it applies to
> all affected patches in your series).
> 

Thank you. I didn't knew.

> It would be also good to have some high level changelog in the cover
> letter, from this series I don't see, for example, which base you did
> use (i2c-next? linux-next? v4.9? v4.10-rc1?).
> 
>> +       dev_dbg(dev->dev,
>> +               "%s: enabled=%#x stat=%#x\n", __func__, enabled,
> stat);
> 
> I hope you can fit format string on the first line. __func__ is
> redundant when you are using debug printing (Dynamic Debug would include
> it if asked for).

I will check that.

> 
>> +static void i2c_dw_configure_master(struct platform_device *pdev)
>> +{
>> +	struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
> 
> By the way, does it make sense to pass struct dw_i2c_dev * as a
> parameter of the function?
> 

Yes, by looking at it now I think I can pass just the struct dw_i2c_dev
to this function. And probably the same with the i2c_dw_configure_slave.

>> +
>> +	dev->master_cfg = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE
>> |
>> +			  DW_IC_CON_RESTART_EN;
>> +
>> +	dev_dbg(&pdev->dev, "I am registed as a I2C Master!\n");
>> +
>> +	switch (dev->clk_freq) {
>> +	case 100000:
>> +		dev->master_cfg |= DW_IC_CON_SPEED_STD;
>> +		break;
>> +	case 3400000:
>> +		dev->master_cfg |= DW_IC_CON_SPEED_HIGH;
>> +		break;
>> +	default:
>> +		dev->master_cfg |= DW_IC_CON_SPEED_FAST;
>> +	}
>> +}
>> +
>>
> 
>