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

[Luis Oliveira]

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@xxxxxxxxxxxx>
>> ---
>> 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;
>> +	}
>> +}
>> +
>>
> 
>