linux-kernel - Re: [PATCH v3 3/3] i2c: zx2967: add i2c controller driver for ZTE's zx2967 family

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] [day] [month] [year] [list]

Date:   Sat, 4 Feb 2017 15:15:47 +0800
From:   Shawn Guo <shawnguo@...nel.org>
To:     Baoyou Xie <baoyou.xie@...aro.org>
Cc:     andy.shevchenko@...il.com, jun.nie@...aro.org, wsa@...-dreams.de,
        robh+dt@...nel.org, mark.rutland@....com,
        linux-arm-kernel@...ts.infradead.org, linux-i2c@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        xie.baoyou@....com.cn, chen.chaokai@....com.cn,
        wang.qiang01@....com.cn
Subject: Re: [PATCH v3 3/3] i2c: zx2967: add i2c controller driver for ZTE's
 zx2967 family

Only a couple of comments below.  Otherwise, the patch looks fine to me.

On Thu, Jan 26, 2017 at 09:00:31PM +0800, Baoyou Xie wrote:

<snip>

> +static int zx2967_i2c_flush_fifos(struct zx2967_i2c_info *zx_i2c)
> +{
> +	u32 val;
> +	u32 offset;
> +
> +	if (zx_i2c->msg_rd) {
> +		offset = REG_RDCONF;
> +		val = I2C_RFIFO_RESET;
> +	} else {
> +		offset = REG_WRCONF;
> +		val = I2C_WFIFO_RESET;
> +	}
> +
> +	val |= zx2967_i2c_readl(zx_i2c, offset);
> +	zx2967_i2c_writel(zx_i2c, val, offset);
> +
> +	return 0;

Since the function will never return other values than 0, should we
simply define the return type as void?

> +}

<snip>

> +static int zx2967_i2c_probe(struct platform_device *pdev)
> +{
> +	struct zx2967_i2c_info *zx_i2c;
> +	void __iomem *reg_base;
> +	struct resource *res;
> +	struct clk *clk;
> +	int ret;
> +
> +	zx_i2c = devm_kzalloc(&pdev->dev, sizeof(*zx_i2c), GFP_KERNEL);
> +	if (!zx_i2c)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	reg_base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(reg_base))
> +		return PTR_ERR(reg_base);
> +
> +	clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(clk)) {
> +		dev_err(&pdev->dev, "missing controller clock");
> +		return PTR_ERR(clk);
> +	}
> +
> +	ret = clk_prepare_enable(clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to enable i2c_clk\n");
> +		return ret;
> +	}
> +
> +	ret = platform_get_irq(pdev, 0);
> +	if (ret < 0)
> +		return ret;
> +	zx_i2c->irq = ret;
> +
> +	ret = device_property_read_u32(&pdev->dev, "clock-frequency",
> +				       &zx_i2c->clk_freq);
> +	if (ret) {
> +		dev_err(&pdev->dev, "missing clock-frequency");
> +		return ret;
> +	}
> +
> +	zx_i2c->reg_base = reg_base;
> +	zx_i2c->clk = clk;
> +	zx_i2c->dev = &pdev->dev;
> +
> +	zx_i2c->pin_ctrl = devm_pinctrl_get_select_default(&pdev->dev);
> +	if (IS_ERR(zx_i2c->pin_ctrl)) {
> +		if (PTR_ERR(zx_i2c->pin_ctrl) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
> +		dev_info(&pdev->dev, "no pinctrl handle\n");
> +	}

You do not need this boilerplate code for request "default" pin state,
since device core will automatically issue the call.  See section "Pin
control requests from drivers" in Documentation/pinctrl.txt for more
info.

Shawn