From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751587AbcL1PMz (ORCPT ); Wed, 28 Dec 2016 10:12:55 -0500 Received: from mga02.intel.com ([134.134.136.20]:38292 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751458AbcL1PMy (ORCPT ); Wed, 28 Dec 2016 10:12:54 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,422,1477983600"; d="scan'208";a="207533777" Message-ID: <1482937968.9552.159.camel@linux.intel.com> Subject: Re: [PATCH v5 2/7] i2c: designware: refactoring of the i2c-designware From: Andy Shevchenko To: Luis Oliveira , wsa@the-dreams.de, robh+dt@kernel.org, mark.rutland@arm.com, jarkko.nikula@linux.intel.com, mika.westerberg@linux.intel.com, linux-i2c@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Cc: Ramiro.Oliveira@synopsys.com, Joao.Pinto@synopsys.com, CARLOS.PALMINHA@synopsys.com Date: Wed, 28 Dec 2016 17:12:48 +0200 In-Reply-To: <0dbbce3bb2ffd47d5a78ea49f992b48d7ea2f35d.1482934380.git.lolivei@synopsys.com> References: <0dbbce3bb2ffd47d5a78ea49f992b48d7ea2f35d.1482934380.git.lolivei@synopsys.com> Organization: Intel Finland Oy Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.22.3-1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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 Intel Finland Oy