On Tue, Feb 20, 2018 at 11:08 AM, Ard Biesheuvel
[off-list ref] wrote:

This is a cleaned up version of the I2C controller driver for
the Fujitsu F_I2C IP, which was never supported upstream, and
has now been incorporated into the Socionext SynQuacer SoC.

Typical issues below.

+/* SPDX-License-Identifier: GPL-2.0 */

Shouldn't be // ?

+#include <linux/init.h>

+#include <linux/module.h>

Supposed to be one of them, not both.

+#define WAIT_PCLK(n, rate)     ndelay((((1000000000 +  (rate) - 1) / \
+                                        (rate) + n - 1) / n) + 10)

This split makes it harder to catch the calculus.
Also, you can use DIV_ROUND_UP(), though it longer, but adds a bit of
clarity to the calculus.

+#define SYNQUACER_I2C_TIMEOUT(x) (msecs_to_jiffies(x))

Isn't shorter to use original function in place?

+static inline unsigned long calc_timeout_ms(struct synquacer_i2c *i2c,
+                                           struct i2c_msg *msgs,
+                                           int num)
+{
+       unsigned long bit_count = 0;
+       int i;
+
+       for (i = 0; i < num; i++, msgs++)
+               bit_count += msgs->len;
+
+       return DIV_ROUND_UP(((bit_count * 9) + (10 * num)) * 3, 200) + 10;

Redundant parens surrounding multiplications.

bit_count * 9 + num * 10 ?

+}

+static void synquacer_i2c_stop(struct synquacer_i2c *i2c, int ret)
+{
+       dev_dbg(i2c->dev, "STOP\n");

Hmm... Can't use FTRACE ?

+
+       /*
+        * clear IRQ (INT=0, BER=0)
+        * set Stop Condition (MSS=0)
+        * Interrupt Disable
+        */
+       writeb(0, i2c->base + SYNQUACER_I2C_REG_BCR);
+
+       i2c->state = STATE_IDLE;
+
+       i2c->msg_ptr = 0;
+       i2c->msg = NULL;
+       i2c->msg_idx++;
+       i2c->msg_num = 0;
+       if (ret)
+               i2c->msg_idx = ret;
+
+       complete(&i2c->completion);
+}

+       default:
+               BUG();

Oh, oh. What is the strong argument to have this kind of crash here?

+static int synquacer_i2c_master_start(struct synquacer_i2c *i2c,
+                                     struct i2c_msg *pmsg)
+{
+       unsigned char bsr, bcr;

+       dev_dbg(i2c->dev, "%s slave:0x%02x\n", __func__, pmsg->addr);

__func__ is redundant (See dynamic debug manual how to enable run-time).

+       /* Force to make one clock pulse */

+       count = 0;
+       for (;;) {

+               if (++count > 9) {
+                       dev_err(i2c->dev, "%s: count: %i, bc2r: 0x%x\n",
+                               __func__, count, bc2r);
+                       return -EIO;
+               }
+       }

Personally I think for iterations do {} while approach looks cleaner
and more understandable:

unsigned int count = 10;

do {
...
} while (--count);

+static int synquacer_i2c_doxfer(struct synquacer_i2c *i2c,
+                               struct i2c_msg *msgs, int num)
+{
+       unsigned char bsr;
+       unsigned long timeout, bb_timeout;

+       int ret = 0;

Redundant assignment.

+       ret = synquacer_i2c_master_start(i2c, i2c->msg);
+       if (ret < 0) {
+               dev_dbg(i2c->dev, "Address failed: (0x%08x)\n", ret);

ret as a hex?!?!
So confusing.

A side note, %#08x will print 0x.

+out:
+       return ret;

Useless label since there is nothing except returning an error code.

+static int synquacer_i2c_probe(struct platform_device *pdev)
+{
+       struct synquacer_i2c *i2c;
+       struct resource *r;
+       int speed_khz;
+       int ret;

+       if (dev_of_node(&pdev->dev)) {
+               i2c->clk = devm_clk_get(&pdev->dev, "pclk");
+               if (IS_ERR(i2c->clk)) {
+                       dev_err(&pdev->dev, "cannot get clock\n");
+                       return PTR_ERR(i2c->clk);
+               }
+               dev_dbg(&pdev->dev, "clock source %p\n", i2c->clk);
+
+               i2c->clkrate = clk_get_rate(i2c->clk);
+               dev_dbg(&pdev->dev, "clock rate %d\n", i2c->clkrate);
+               clk_prepare_enable(i2c->clk);
+       } else {

+               ret = device_property_read_u32(&pdev->dev,
+                                              "socionext,pclk-rate",
+                                              &i2c->clkrate);

I suppose for ACPI we just register a fixed rate clock and use it in
the driver in the same way as in OF case.
I guess at some point we even can provide a generic clock provider for
ACPI based on rate property.

+               if (ret)
+                       return ret;
+       }

+       i2c->state = STATE_IDLE;
+       i2c->dev = &pdev->dev;

+       i2c->msg = NULL;

Isn't it done by z letter in allocation?

+#ifdef CONFIG_PM_SLEEP

__maybe_unused instead of ugly #ifdef:s.

+static int synquacer_i2c_suspend(struct device *dev)

+static int synquacer_i2c_resume(struct device *dev)

+#else
+#define SYNQUACER_I2C_PM       NULL
+#endif

+#ifdef CONFIG_OF

OK, this is fine since we have an ACPI ID for it.

+static const struct of_device_id synquacer_i2c_dt_ids[] = {
+       { .compatible = "socionext,synquacer-i2c" },
+       { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, synquacer_i2c_dt_ids);
+#endif
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id synquacer_i2c_acpi_ids[] = {
+       { "SCX0003" },
+       { /* sentinel */ }
+};
+#endif

+static struct platform_driver synquacer_i2c_driver = {
+       .probe  = synquacer_i2c_probe,
+       .remove = synquacer_i2c_remove,
+       .driver = {

+               .owner = THIS_MODULE,

Macro below will fill this for you.

+               .name = "synquacer_i2c",
+               .of_match_table = of_match_ptr(synquacer_i2c_dt_ids),
+               .acpi_match_table = ACPI_PTR(synquacer_i2c_acpi_ids),
+               .pm = SYNQUACER_I2C_PM,
+       },
+};
+module_platform_driver(synquacer_i2c_driver);

-- 
With Best Regards,
Andy Shevchenko