From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bjorn Andersson Subject: Re: [PATCH v2 2/2] i2c: New bus driver for the QUP I2C controller Date: Thu, 16 Jan 2014 16:18:33 -0800 Message-ID: <20140117001833.GL30911@sonymobile.com> References: <1389659437-16406-1-git-send-email-bjorn.andersson@sonymobile.com> <1389659437-16406-3-git-send-email-bjorn.andersson@sonymobile.com> <20140115164604.GI14405@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <20140115164604.GI14405@codeaurora.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Stephen Boyd Cc: Mark Rutland , Wolfram Sang , "linux-i2c@vger.kernel.org" , Matt Porter , "linux-doc@vger.kernel.org" , Grant Likely , James Ralston , "devicetree@vger.kernel.org" , Rob Landley , Pawel Moll , Ian Campbell , "linux-arm-msm@vger.kernel.org" , Rob Herring , Martin Schwidefsky , Andy Shevchenko , "linux-arm-kernel@lists.infradead.org" , Bill Brown , Greg Kroah-Hartman "linux-kernel@vger.kernel.org" List-Id: devicetree@vger.kernel.org On Wed 15 Jan 08:46 PST 2014, Stephen Boyd wrote: > On 01/13, Bjorn Andersson wrote: > > +/* > > + * QUP driver for Qualcomm MSM platforms > > + * > > + */ > > This comment seems redundant, we know what file we're looking at. > Indeed. > > + > > +struct qup_i2c_dev { > > + struct device *dev; > > + void __iomem *base; > > + struct pinctrl *pctrl; > > This is unused. > Removed. > > + int irq; > > + struct clk *clk; > > + struct clk *pclk; > > + struct i2c_adapter adap; > > + > > + int clk_freq; > > This is only ever used in probe, so why do we need to store it > away? > Moved to probe() > > + int clk_ctl; > > + int one_bit_t; > > + int out_fifo_sz; > > + int in_fifo_sz; > > + int out_blk_sz; > > + int in_blk_sz; > > + unsigned long xfer_time; > > + unsigned long wait_idle; > > + > > + struct i2c_msg *msg; > > + /* Current possion in user message buffer */ > > s/possion/position/? > Thanks. > > + int pos; > > + /* Keep number of bytes left to be transmitted */ > > + int cnt; > > + /* I2C protocol errors */ > > + u32 bus_err; > > + /* QUP core errors */ > > + u32 qup_err; > > + /* > > + * maximum bytes that could be send (per iterration). could be > > s/iterration/iteration/? > Thanks > > + * equal of fifo size or block size (in block mode) > > + */ > > + int chunk_sz; > > + struct completion xfer; > > +}; > > + > > +static irqreturn_t qup_i2c_interrupt(int irq, void *dev) > > +{ > > + struct qup_i2c_dev *qup = dev; > > + u32 bus_err; > > + u32 qup_err; > > + u32 opflags; > > + > [...] > > + > > + if (opflags & QUP_OUT_SVC_FLAG) > > + writel(QUP_OUT_SVC_FLAG, qup->base + QUP_OPERATIONAL); > > + > > + if (!(qup->msg->flags == I2C_M_RD)) > > Should this be? > > if (!(qup->msg->flags & I2C_M_RD)) > > Otherwise it should be > > if (qup->msg->flags != I2C_M_RD) > In the codeaurora implementation this seem to be: if (qup->msg->flags == I2C_M_RD) { if ((opflags & QUP_MX_INPUT_DONE) || (opflags & QUP_IN_SVC_FLAG)) writel(QUP_IN_SVC_FLAG, qup->base + QUP_OPERATIONAL); else return IRQ_HANDLED; } So, do not complete the xfer unless there's something for us. I'll verify this and update the next version. Thanks. > > + return IRQ_HANDLED; > > + > > + if ((opflags & QUP_MX_INPUT_DONE) || (opflags & QUP_IN_SVC_FLAG)) > > + writel(QUP_IN_SVC_FLAG, qup->base + QUP_OPERATIONAL); > > + > > +done: > > + qup->qup_err = qup_err; > > + qup->bus_err = bus_err; > > + complete(&qup->xfer); > > + return IRQ_HANDLED; > > +} > > + > > +static int > > +qup_i2c_poll_state(struct qup_i2c_dev *qup, u32 req_state, bool only_valid) > > +{ > > + int retries = 0; > > + u32 state; > > + > > + do { > > + state = readl(qup->base + QUP_STATE); > > + > > + /* > > + * If only valid bit needs to be checked, requested state is > > + * 'don't care' > > + */ > > It looks like req_state == 0 means only_valid == true. Can we > drop the only_valid argument to this function? > In all cases but the reset in the beginning of qup_i2c_xfer, so it seems that it has to stay. > > + if (state & QUP_STATE_VALID) { > > + if (only_valid) > > + return 0; > > + if ((req_state & QUP_I2C_MAST_GEN) > > + && (state & QUP_I2C_MAST_GEN)) > > + return 0; > > + if ((state & QUP_STATE_MASK) == req_state) > > + return 0; > > + } > > + > > + if (retries++ == 1000) > > + udelay(100); > > + > > + } while (retries != 2000); > > Please makes #defines for 1000 and 2000. > This retry loop looks completely arbitrary to me now that you say it. I'll revise this. > > + > > + return -ETIMEDOUT; > > +} > > + > [...] > > +static void qup_i2c_issue_write(struct qup_i2c_dev *qup, struct i2c_msg *msg) > > +{ > > + u32 addr = msg->addr << 1; > > + u32 val, qup_tag; > > + int idx, entries; > > + > > + if (qup->pos == 0) { > > + val = QUP_OUT_START | addr; > > + } else { > > + /* > > + * Avoid setup time issue by adding 1 NOP when number of bytes > > + * are more than FIFO/BLOCK size. setup time issue can't appear > > + * otherwise since next byte to be written will always be ready > > + */ > > + val = (QUP_OUT_NOP | 1); > > + } > > + > > + entries = qup->cnt + 1; > > + > > + if (entries > qup->chunk_sz) > > + entries = qup->chunk_sz; > > + > > + qup_tag = QUP_OUT_DATA; > > + > > + /* Reserve one entry for STOP */ > > + for (idx = 1; idx < (entries - 1); idx++, qup->pos++) { > > Unnecessary () here around entries. > Sure. > > + > > + if (idx & 1) { > > + val |= (qup_tag | msg->buf[qup->pos]) << QUP_MSW_SHIFT; > > + writel(val, qup->base + QUP_OUT_FIFO_BASE); > > + } else { > > + val = qup_tag | msg->buf[qup->pos]; > > + } > [...] > > + > > +#ifdef CONFIG_PM > > This ifdef is probably wrong considering that you can disable > CONFIG_PM_RUNTIME without disabling CONFIG_PM and then these > runtime PM functions would be unused. > Will fix. > > +static int qup_i2c_pm_suspend_runtime(struct device *device) > > +{ > > + struct qup_i2c_dev *qup = dev_get_drvdata(device); > > + > > + dev_dbg(device, "pm_runtime: suspending...\n"); > > + qup_i2c_disable_clocks(qup); > > + return 0; > > +} > > + > > +static int qup_i2c_pm_resume_runtime(struct device *device) > > +{ > > + struct qup_i2c_dev *qup = dev_get_drvdata(device); > > + > > + dev_dbg(device, "pm_runtime: resuming...\n"); > > + qup_i2c_enable_clocks(qup); > > + return 0; > > +} > > + > > +static int qup_i2c_suspend(struct device *device) > > +{ > > + dev_dbg(device, "system suspend"); > > + qup_i2c_pm_suspend_runtime(device); > > + return 0; > > +} > > + > > +static int qup_i2c_resume(struct device *device) > > +{ > > + dev_dbg(device, "system resume"); > > + qup_i2c_pm_resume_runtime(device); > > + pm_runtime_mark_last_busy(device); > > + pm_request_autosuspend(device); > > + return 0; > > +} > > +#endif /* CONFIG_PM */ > > + > > +static const struct dev_pm_ops qup_i2c_qup_pm_ops = { > > + SET_SYSTEM_SLEEP_PM_OPS( > > + qup_i2c_suspend, > > + qup_i2c_resume) > > + SET_RUNTIME_PM_OPS( > > + qup_i2c_pm_suspend_runtime, > > + qup_i2c_pm_resume_runtime, > > + NULL) > > +}; > > + > > +static struct of_device_id qup_i2c_dt_match[] = { > > const? > Sure > > + {.compatible = "qcom,i2c-qup"}, > > + {} > > +}; > > MODULE_DEVICE_TABLE(of, qup_i2c_dt_match)? > OK Thanks Stephen! // Bjorn