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 <[email protected]> > + * - 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
