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 17:12:48 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     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 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).

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).

> +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?

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


-- 
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Intel Finland Oy