Re: [PATCH 1/6] platform-drivers: msm: add single-wire serial bus interface (SSBI) driver

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Wed, Mar 06, 2013 at 04:29:42PM -0800, David Brown wrote:
> +menu "Qualcomm MSM SSBI bus support"
> +	depends on ARCH_MSM

Why?

> +config MSM_SSBI
> +	bool "Qualcomm Single-wire Serial Bus Interface (SSBI)"

Why can't this be a module?

The multi-platform (or whatever it's called), requires that things be
modules and not built in.

> +	help
> +	  If you say yes to this option, support will be included for the
> +	  built-in SSBI interface on Qualcomm MSM family processors.
> +
> +	  This is required for communicating with Qualcomm PMICs and
> +	  other devices that have the SSBI interface.
> +
> +endmenu
> diff --git a/drivers/ssbi/Makefile b/drivers/ssbi/Makefile
> new file mode 100644
> index 0000000..ea8c1e4
> --- /dev/null
> +++ b/drivers/ssbi/Makefile
> @@ -0,0 +1,4 @@
> +#
> +# Makefile for the MSM specific device drivers.

MSM?

No comment needed at all in this file :)

> --- /dev/null
> +++ b/drivers/ssbi/ssbi.c
> @@ -0,0 +1,397 @@
> +/* Copyright (c) 2009-2011, Code Aurora Forum. All rights reserved.
> + * Copyright (c) 2010, Google Inc.
> + *
> + * Original authors: Code Aurora Forum
> + *
> + * Author: Dima Zavin <dima@android.com>
> + *  - Largely rewritten from original to not be an i2c driver.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#define pr_fmt(fmt) "%s: " fmt, __func__
> +
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/msm_ssbi.h>
> +#include <linux/module.h>
> +
> +/* SSBI 2.0 controller registers */
> +#define SSBI2_CMD			0x0008
> +#define SSBI2_RD			0x0010
> +#define SSBI2_STATUS			0x0014
> +#define SSBI2_MODE2			0x001C
> +
> +/* SSBI_CMD fields */
> +#define SSBI_CMD_RDWRN			(1 << 24)
> +
> +/* SSBI_STATUS fields */
> +#define SSBI_STATUS_RD_READY		(1 << 2)
> +#define SSBI_STATUS_READY		(1 << 1)
> +#define SSBI_STATUS_MCHN_BUSY		(1 << 0)
> +
> +/* SSBI_MODE2 fields */
> +#define SSBI_MODE2_REG_ADDR_15_8_SHFT	0x04
> +#define SSBI_MODE2_REG_ADDR_15_8_MASK	(0x7f << SSBI_MODE2_REG_ADDR_15_8_SHFT)
> +
> +#define SET_SSBI_MODE2_REG_ADDR_15_8(MD, AD) \
> +	(((MD) & 0x0F) | ((((AD) >> 8) << SSBI_MODE2_REG_ADDR_15_8_SHFT) & \
> +	SSBI_MODE2_REG_ADDR_15_8_MASK))
> +
> +/* SSBI PMIC Arbiter command registers */
> +#define SSBI_PA_CMD			0x0000
> +#define SSBI_PA_RD_STATUS		0x0004
> +
> +/* SSBI_PA_CMD fields */
> +#define SSBI_PA_CMD_RDWRN		(1 << 24)
> +#define SSBI_PA_CMD_ADDR_MASK		0x7fff /* REG_ADDR_7_0, REG_ADDR_8_14*/
> +
> +/* SSBI_PA_RD_STATUS fields */
> +#define SSBI_PA_RD_STATUS_TRANS_DONE	(1 << 27)
> +#define SSBI_PA_RD_STATUS_TRANS_DENIED	(1 << 26)
> +
> +#define SSBI_TIMEOUT_US			100
> +
> +struct msm_ssbi {
> +	struct device		*dev;
> +	struct device		*slave;

Really?  Two pointers to devices?  Why?  Shouldn't this have a struct
device embedded in it instead of the dev member being a pointer?

> +	void __iomem		*base;
> +	spinlock_t		lock;
> +	enum msm_ssbi_controller_type controller_type;
> +	int (*read)(struct msm_ssbi *, u16 addr, u8 *buf, int len);
> +	int (*write)(struct msm_ssbi *, u16 addr, u8 *buf, int len);
> +};
> +
> +#define to_msm_ssbi(dev)	platform_get_drvdata(to_platform_device(dev))

That doesn't work for the above structure, right?

> +static inline u32 ssbi_readl(struct msm_ssbi *ssbi, u32 reg)
> +{
> +	return readl(ssbi->base + reg);
> +}
> +
> +static inline void ssbi_writel(struct msm_ssbi *ssbi, u32 val, u32 reg)
> +{
> +	writel(val, ssbi->base + reg);
> +}

Did you run sparse on this file?

> +static int ssbi_wait_mask(struct msm_ssbi *ssbi, u32 set_mask, u32 clr_mask)
> +{
> +	u32 timeout = SSBI_TIMEOUT_US;
> +	u32 val;
> +
> +	while (timeout--) {
> +		val = ssbi_readl(ssbi, SSBI2_STATUS);
> +		if (((val & set_mask) == set_mask) && ((val & clr_mask) == 0))
> +			return 0;
> +		udelay(1);

Busy loop?  Really?

> +	}
> +
> +	dev_err(ssbi->dev, "%s: timeout (status %x set_mask %x clr_mask %x)\n",
> +		__func__, ssbi_readl(ssbi, SSBI2_STATUS), set_mask, clr_mask);

Why polute the log with this?  What can a user do with it?


> +	return -ETIMEDOUT;
> +}
> +
> +static int
> +msm_ssbi_read_bytes(struct msm_ssbi *ssbi, u16 addr, u8 *buf, int len)
> +{
> +	u32 cmd = SSBI_CMD_RDWRN | ((addr & 0xff) << 16);
> +	int ret = 0;
> +
> +	if (ssbi->controller_type == MSM_SBI_CTRL_SSBI2) {
> +		u32 mode2 = ssbi_readl(ssbi, SSBI2_MODE2);
> +		mode2 = SET_SSBI_MODE2_REG_ADDR_15_8(mode2, addr);
> +		ssbi_writel(ssbi, mode2, SSBI2_MODE2);
> +	}
> +
> +	while (len) {
> +		ret = ssbi_wait_mask(ssbi, SSBI_STATUS_READY, 0);
> +		if (ret)
> +			goto err;
> +
> +		ssbi_writel(ssbi, cmd, SSBI2_CMD);
> +		ret = ssbi_wait_mask(ssbi, SSBI_STATUS_RD_READY, 0);
> +		if (ret)
> +			goto err;
> +		*buf++ = ssbi_readl(ssbi, SSBI2_RD) & 0xff;
> +		len--;
> +	}
> +
> +err:
> +	return ret;
> +}
> +
> +static int
> +msm_ssbi_write_bytes(struct msm_ssbi *ssbi, u16 addr, u8 *buf, int len)
> +{
> +	int ret = 0;
> +
> +	if (ssbi->controller_type == MSM_SBI_CTRL_SSBI2) {
> +		u32 mode2 = ssbi_readl(ssbi, SSBI2_MODE2);
> +		mode2 = SET_SSBI_MODE2_REG_ADDR_15_8(mode2, addr);
> +		ssbi_writel(ssbi, mode2, SSBI2_MODE2);
> +	}

Perhaps have a controller type write function that can handle this type
of stuff instead of putting it in the "generic" write path?

> +	while (len) {
> +		ret = ssbi_wait_mask(ssbi, SSBI_STATUS_READY, 0);
> +		if (ret)
> +			goto err;
> +
> +		ssbi_writel(ssbi, ((addr & 0xff) << 16) | *buf, SSBI2_CMD);
> +		ret = ssbi_wait_mask(ssbi, 0, SSBI_STATUS_MCHN_BUSY);
> +		if (ret)
> +			goto err;
> +		buf++;
> +		len--;
> +	}
> +
> +err:
> +	return ret;
> +}
> +
> +static inline int
> +msm_ssbi_pa_transfer(struct msm_ssbi *ssbi, u32 cmd, u8 *data)
> +{
> +	u32 timeout = SSBI_TIMEOUT_US;
> +	u32 rd_status = 0;
> +
> +	ssbi_writel(ssbi, cmd, SSBI_PA_CMD);
> +
> +	while (timeout--) {
> +		rd_status = ssbi_readl(ssbi, SSBI_PA_RD_STATUS);
> +
> +		if (rd_status & SSBI_PA_RD_STATUS_TRANS_DENIED) {
> +			dev_err(ssbi->dev, "%s: transaction denied (0x%x)\n",
> +					__func__, rd_status);
> +			return -EPERM;
> +		}
> +
> +		if (rd_status & SSBI_PA_RD_STATUS_TRANS_DONE) {
> +			if (data)
> +				*data = rd_status & 0xff;
> +			return 0;
> +		}
> +		udelay(1);

Busy loop again?

> +	}
> +
> +	dev_err(ssbi->dev, "%s: timeout, status 0x%x\n", __func__, rd_status);

Don't polute the logs unless you want the user to do something with the
information.


> +	return -ETIMEDOUT;
> +}
> +
> +static int
> +msm_ssbi_pa_read_bytes(struct msm_ssbi *ssbi, u16 addr, u8 *buf, int len)
> +{
> +	u32 cmd;
> +	int ret = 0;
> +
> +	cmd = SSBI_PA_CMD_RDWRN | (addr & SSBI_PA_CMD_ADDR_MASK) << 8;
> +
> +	while (len) {
> +		ret = msm_ssbi_pa_transfer(ssbi, cmd, buf);
> +		if (ret)
> +			goto err;
> +		buf++;
> +		len--;
> +	}
> +
> +err:
> +	return ret;
> +}
> +
> +static int
> +msm_ssbi_pa_write_bytes(struct msm_ssbi *ssbi, u16 addr, u8 *buf, int len)
> +{
> +	u32 cmd;
> +	int ret = 0;
> +
> +	while (len) {
> +		cmd = (addr & SSBI_PA_CMD_ADDR_MASK) << 8 | *buf;
> +		ret = msm_ssbi_pa_transfer(ssbi, cmd, NULL);
> +		if (ret)
> +			goto err;
> +		buf++;
> +		len--;
> +	}
> +
> +err:
> +	return ret;
> +}
> +
> +int msm_ssbi_read(struct device *dev, u16 addr, u8 *buf, int len)
> +{
> +	struct msm_ssbi *ssbi = to_msm_ssbi(dev);
> +	unsigned long flags;
> +	int ret;
> +
> +	if (ssbi->dev != dev)
> +		return -ENXIO;
> +
> +	spin_lock_irqsave(&ssbi->lock, flags);
> +	ret = ssbi->read(ssbi, addr, buf, len);
> +	spin_unlock_irqrestore(&ssbi->lock, flags);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(msm_ssbi_read);

msm_*?  Why not just ssbi_*?

EXPORT_SYMBOL_GPL()?

> +static struct platform_driver msm_ssbi_driver = {
> +	.probe		= msm_ssbi_probe,
> +	.remove		= __exit_p(msm_ssbi_remove),

You just oopsed if someone unbound your device from the driver from
userspace.  Not good.

thanks,

greg k-h