| 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
| ||
|
Date: Sun, 25 Jun 2017 18:06:44 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Baolin Wang <baolin.wang@...eadtrum.com>
Cc: Wolfram Sang <wsa@...-dreams.de>,
Mark Rutland <mark.rutland@....com>,
Rob Herring <robh+dt@...nel.org>,
linux-i2c <linux-i2c@...r.kernel.org>,
devicetree <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Mark Brown <broonie@...nel.org>,
Baolin Wang <baolin.wang@...aro.org>
Subject: Re: [PATCH v2 2/2] i2c: Add Spreadtrum I2C controller driver
On Wed, Jun 21, 2017 at 10:23 AM, Baolin Wang
<baolin.wang@...eadtrum.com> wrote:
> This patch adds the I2C controller driver for Spreadtrum platform.
Needs more work.
See my comments below.
> +#include <linux/init.h>
> +#include <linux/module.h>
Since your answer to the comment about arch_initcall you perhaps need
to get rid of tristate (use bool) and drop module.h here and all
leftovers like MODULE_*() calls.
> +#include <linux/slab.h>
Should we include this still explicitly (after kernel.h)?
> +
> +#define I2C_CTL 0x0
> +#define I2C_ADDR_CFG 0x4
> +#define I2C_COUNT 0x8
> +#define I2C_RX 0xC
> +#define I2C_TX 0x10
> +#define I2C_STATUS 0x14
> +#define I2C_HSMODE_CFG 0x18
> +#define I2C_VERSION 0x1C
> +#define ADDR_DVD0 0x20
> +#define ADDR_DVD1 0x24
> +#define ADDR_STA0_DVD 0x28
> +#define ADDR_RST 0x2C
1. Please, use fixed sized values
0x00 and so on.
2. Please, low case.
> +#define SPRD_I2C_TIMEOUT (msecs_to_jiffies(1000))
Redundant parens.
> +static void sprd_i2c_dump_reg(struct sprd_i2c *i2c_dev)
If you switch your driver to regmap API, you may drop this function completely.
> + writel(tmp & (~STP_EN), i2c_dev->base + I2C_CTL);
Redundant parens.
Also pay attention to other similar places.
> +static void sprd_i2c_write_bytes(struct sprd_i2c *i2c_dev, u8 *buf, u32 len)
So, isn't better to provide a writesb(), readsb() to ia64 instead of
doing below?
> +{
> + u32 i = 0;
> +
> + for (i = 0; i < len; i++) {
> + writel(buf[i], i2c_dev->base + I2C_TX);
> + dev_dbg(&i2c_dev->adap.dev, "write bytes[%d] = %x\n", i, buf[i]);
Moreover, don't waste lines in the kernel log buffer by doing this.
Choose either
%*ph (up to 64 bytes)
or
print_hex_dump()
(Same for _read_bytes() below)
> + }
> +}
> +static void sprd_i2c_set_full_thld(struct sprd_i2c *i2c_dev, u32 full_thld)
> +{
> + unsigned int tmp = readl(i2c_dev->base + I2C_CTL);
> +
> + tmp &= ~(0xf << FIFO_AF_LVL);
Magic.
Define it using GENMASK()
> + tmp |= (full_thld << FIFO_AF_LVL);
Redundant parens.
> + tmp &= ~(0xf << FIFO_AE_LVL);
> + tmp |= (empty_thld << FIFO_AE_LVL);
Same.
> +static void sprd_i2c_opt_mode(struct sprd_i2c *i2c_dev, int rw)
> +{
> + unsigned int cmd = readl(i2c_dev->base + I2C_CTL) & (~I2C_MODE);
Redundant parens.
> + writel((cmd | rw << 3), i2c_dev->base + I2C_CTL);
Ditto.
It's a C, and not a LISP :-)
> +}
> +static void sprd_i2c_data_transfer(struct sprd_i2c *i2c_dev)
> +{
> + if ((msg->flags & I2C_M_RD)) {
Redundant parens.
> + /* Reset rx/tx fifos */
Noise.
> + /* Set device address */
Ditto.
> + /* Set I2C read or write bytes count */
Ditto.
> + if ((msg->flags & I2C_M_RD)) {
> + /* Set read operation */
> + sprd_i2c_opt_mode(i2c_dev, 1);
> + /* Last byte transmission should excute stop operation */
> + sprd_i2c_send_stop(i2c_dev, 1);
Same comments as above.
> + } else {
> + /* Set write operation */
Noise.
> + /* Last byte transmission should excute stop operation */
> + if (is_last_msg)
> + sprd_i2c_send_stop(i2c_dev, 1);
> + else
> + sprd_i2c_send_stop(i2c_dev, 0);
sprd_i2c_send_stop(i2c_dev, !!is_last_msg);
Though, consider to make is_last_msg boolean.
> +static int sprd_i2c_master_xfer(struct i2c_adapter *i2c_adap,
> + struct i2c_msg *msgs, int num)
> +{
> + struct sprd_i2c *i2c_dev = i2c_adap->algo_data;
> + int im, ret;
> +
> + ret = pm_runtime_get_sync(i2c_dev->dev);
> + if (ret < 0)
> + return ret;
> +
> + for (im = 0; im != num; im++)
im < num - 1, and...
ret = sprd_i2c_handle_msg(i2c_adap, &msgs[im], 0);
... ret = sprd_i2c_handle_msg(i2c_adap, &msgs[im++], 1);
...and we clearly see that you have messed up with returned code on
each itteration here
> + pm_runtime_mark_last_busy(i2c_dev->dev);
> + pm_runtime_put_autosuspend(i2c_dev->dev);
> + return (ret >= 0) ? im : ret;
Usual pattern is
ret < 0 ? ret : im;
> +static void sprd_i2c_set_clk(struct sprd_i2c *i2c_dev, unsigned int freq)
> +{
> + u32 apb_clk = i2c_dev->src_clk;
> + u32 i2c_dvd = apb_clk / (4 * freq) - 1;
> + u32 high = ((i2c_dvd << 1) * 2) / 5;
> + u32 low = ((i2c_dvd << 1) * 3) / 5;
> + u32 div0 = (high & 0xffff) << 16 | (low & 0xffff);
> + u32 div1 = (high & 0xffff0000) | ((low & 0xffff0000) >> 16);
Magic masks all over.
Also it needs a comment what formula is used and how.
> +
> + writel(div0, i2c_dev->base + ADDR_DVD0);
> + writel(div1, i2c_dev->base + ADDR_DVD1);
> +
> + if (freq == 400000)
> + writel((6 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
> + else if (freq == 100000)
> + writel((4 * apb_clk) / 1000000, i2c_dev->base + ADDR_STA0_DVD);
What about 3400000?
What about the rest of the speeds, shouldn't you return an error from here?
> +}
> +
> +static void sprd_i2c_enable(struct sprd_i2c *i2c_dev)
> +{
> + unsigned int tmp = readl(i2c_dev->base + I2C_CTL);
> +
> + tmp &= ~(I2C_EN | I2C_TRIM_OPT | (0xF << FIFO_AF_LVL) |
> + (0xF << FIFO_AE_LVL));
Magic masks (I saw them already above).
> + /* Set rx fifo full data threshold */
Drop noise comments. They don't bring any value since you have nice
function names.
> + sprd_i2c_set_full_thld(i2c_dev, I2C_FIFO_FULL_THLD);
> + /* Set tx fifo empty data threshold */
> + sprd_i2c_set_empty_thld(i2c_dev, I2C_FIFO_EMPTY_THLD);
> +
> + sprd_i2c_set_clk(i2c_dev, i2c_dev->bus_freq);
> + /* Reset rx/tx fifo */
> + sprd_i2c_reset_fifo(i2c_dev);
> + sprd_i2c_clear_irq(i2c_dev);
> +static int sprd_i2c_clk_init(struct sprd_i2c *i2c_dev)
> +{
> + struct clk *clk_i2c, *clk_parent;
> + struct device_node *np = i2c_dev->adap.dev.of_node;
> +
> + clk_i2c = of_clk_get_by_name(np, "i2c");
What the issue to use resource agnostic API here, i.e. devm_clk_get() ?
> + clk_parent = of_clk_get_by_name(np, "source");
Ditto.
> + i2c_dev->clk = of_clk_get_by_name(np, "enable");
Ditto.
> + if (!of_property_read_u32(dev->of_node, "clock-frequency", &prop))
> + i2c_dev->bus_freq = prop;
> +
> + sprd_i2c_clk_init(i2c_dev);
> + platform_set_drvdata(pdev, i2c_dev);
> +
> + ret = clk_prepare_enable(i2c_dev->clk);
> + if (ret)
> + return ret;
> +
> + sprd_i2c_enable(i2c_dev);
> +
> +error:
I would put it as
err_rpm_put:
> + pm_runtime_put_noidle(i2c_dev->dev);
> + pm_runtime_disable(i2c_dev->dev);
> + clk_disable_unprepare(i2c_dev->clk);
> + return ret;
> +}
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists