Thank you Kyungmin Park for the review.<br><br>
<div class="gmail_quote">On 3 December 2012 22:28, Kyungmin Park <span dir="ltr"><<a href="mailto:kmpark@infradead.org" target="_blank">kmpark@infradead.org</a>></span> wrote:<br>
<blockquote style="BORDER-LEFT:#ccc 1px solid;MARGIN:0px 0px 0px 0.8ex;PADDING-LEFT:1ex" class="gmail_quote">Hi,<br>
<div>
<div class="h5"><br>On Mon, Dec 3, 2012 at 9:16 PM, Giridhar Maruthy<br><<a href="mailto:giridhar.maruthy@linaro.org">giridhar.maruthy@linaro.org</a>> wrote:<br>> This patch adds slave support to i2c. The dt entry i2c-mode<br>
> decides at probe time if the controller needs to work in<br>> slave mode and the controller is accordingly programmed.<br>><br>> Signed-off-by: Giridhar Maruthy <<a href="mailto:giridhar.maruthy@linaro.org">giridhar.maruthy@linaro.org</a>><br>
> ---<br>> drivers/i2c/busses/i2c-s3c2410.c | 100<br>> ++++++++++++++++++++++++++------------<br>> 1 file changed, 68 insertions(+), 32 deletions(-)<br>><br>> diff --git a/drivers/i2c/busses/i2c-s3c2410.c<br>
> b/drivers/i2c/busses/i2c-s3c2410.c<br>> index e93e7d6..d83a6d7 100644<br>> --- a/drivers/i2c/busses/i2c-s3c2410.c<br>> +++ b/drivers/i2c/busses/i2c-s3c2410.c<br>> @@ -53,6 +53,9 @@<br>> /* Max time to wait for bus to become idle after a xfer (in us) */<br>
> #define S3C2410_IDLE_TIMEOUT 5000<br>><br>> +/* To find the master/slave mode of current controller */<br>> +#define is_master(i2c) (!i2c->i2c_mode)<br>> +<br>> /* i2c controller state */<br>> enum s3c24xx_i2c_state {<br>
> STATE_IDLE,<br>> @@ -89,6 +92,8 @@ struct s3c24xx_i2c {<br>> #ifdef CONFIG_CPU_FREQ<br>> struct notifier_block freq_transition;<br>> #endif<br>> + /* i2c_mode: 0 is for master; and 1 is for slave */<br>
> + unsigned int i2c_mode;<br></div></div>If it's used for master or not, doesn't better to use 'is_master' or<br>'is_slave'? what's the meaning of 'i2c_mode'?<br>and<br>
#define is_master(i2c) (i2c->is_master)<br></blockquote>
<div> </div>
<div>I thought mode indicates master mode or slave mode, but I agree 'is_master' name is better. Will change that.</div>
<blockquote style="BORDER-LEFT:#ccc 1px solid;MARGIN:0px 0px 0px 0.8ex;PADDING-LEFT:1ex" class="gmail_quote"><br>Thank you,<br>Kyungmin Park<br>
<div>
<div class="h5">> };<br>><br>> static struct platform_device_id s3c24xx_driver_ids[] = {<br>> @@ -202,11 +207,21 @@ static void s3c24xx_i2c_message_start(struct<br>> s3c24xx_i2c *i2c,<br>> stat = 0;<br>
> stat |= S3C2410_IICSTAT_TXRXEN;<br>><br>> - if (msg->flags & I2C_M_RD) {<br>> - stat |= S3C2410_IICSTAT_MASTER_RX;<br>> - addr |= 1;<br>> - } else<br>
> - stat |= S3C2410_IICSTAT_MASTER_TX;<br>> + if (is_master(i2c)) {<br>> + /* Master mode */<br>> + if (msg->flags & I2C_M_RD) {<br>> + stat |= S3C2410_IICSTAT_MASTER_RX;<br>
> + addr |= 1;<br>> + } else<br>> + stat |= S3C2410_IICSTAT_MASTER_TX;<br>> + } else {<br>> + /* Slave mode */<br>> + if (msg->flags & I2C_M_RD) {<br>
> + stat |= S3C2410_IICSTAT_SLAVE_RX;<br>> + addr |= 1;<br>> + } else<br>> + stat |= S3C2410_IICSTAT_SLAVE_TX;<br>> + }<br>
><br>> if (msg->flags & I2C_M_REV_DIR_ADDR)<br>> addr ^= 1;<br>> @@ -228,8 +243,10 @@ static void s3c24xx_i2c_message_start(struct<br>> s3c24xx_i2c *i2c,<br>> dev_dbg(i2c->dev, "iiccon, %08lx\n", iiccon);<br>
> writel(iiccon, i2c->regs + S3C2410_IICCON);<br>><br>> - stat |= S3C2410_IICSTAT_START;<br>> - writel(stat, i2c->regs + S3C2410_IICSTAT);<br>> + if (is_master(i2c)) {<br>> + stat |= S3C2410_IICSTAT_START;<br>
> + writel(stat, i2c->regs + S3C2410_IICSTAT);<br>> + }<br>> }<br>><br>> static inline void s3c24xx_i2c_stop(struct s3c24xx_i2c *i2c, int ret)<br>> @@ -272,14 +289,19 @@ static inline void s3c24xx_i2c_stop(struct s3c24xx_i2c<br>
> *i2c, int ret)<br>> * devices, the host as Master and the HDMIPHY device as the slave.<br>> * Skipping the STOP condition has been tested on this bus and<br>> works.<br>> */<br>
> - if (i2c->quirks & QUIRK_HDMIPHY) {<br>> - /* Stop driving the I2C pins */<br>> - iicstat &= ~S3C2410_IICSTAT_TXRXEN;<br>> - } else {<br>> - /* stop the transfer */<br>
> - iicstat &= ~S3C2410_IICSTAT_START;<br>> + if (is_master(i2c)) {<br>> + if (i2c->quirks & QUIRK_HDMIPHY) {<br>> + /* Stop driving the I2C pins */<br>
> + iicstat &= ~S3C2410_IICSTAT_TXRXEN;<br>> + } else {<br>> + /* stop the transfer */<br>> + if (is_master(i2c)) {<br>> + /* Start/Stop required only for master */<br>
> + iicstat &= ~S3C2410_IICSTAT_START;<br>> + }<br>> + }<br>> + writel(iicstat, i2c->regs + S3C2410_IICSTAT);<br>> }<br>
> - writel(iicstat, i2c->regs + S3C2410_IICSTAT);<br>><br>> i2c->state = STATE_STOP;<br>><br>> @@ -348,7 +370,8 @@ static int i2c_s3c_irq_nextbyte(struct s3c24xx_i2c *i2c,<br>> unsigned long iicstat)<br>
> */<br>><br>> if (iicstat & S3C2410_IICSTAT_LASTBIT &&<br>> - !(i2c->msg->flags & I2C_M_IGNORE_NAK)) {<br>> + !(i2c->msg->flags & I2C_M_IGNORE_NAK) &&<br>
> + is_master(i2c)) {<br>> /* ack was not received... */<br>><br>> dev_dbg(i2c->dev, "ack was not received\n");<br>> @@ -380,7 +403,7 @@ static int i2c_s3c_irq_nextbyte(struct s3c24xx_i2c *i2c,<br>
> unsigned long iicstat)<br>> * end of the message, and if so, work out what to do<br>> */<br>><br>> - if (!(i2c->msg->flags & I2C_M_IGNORE_NAK)) {<br>
> + if (!(i2c->msg->flags & I2C_M_IGNORE_NAK) && is_master(i2c))<br>> {<br>> if (iicstat & S3C2410_IICSTAT_LASTBIT) {<br>> dev_dbg(i2c->dev, "WRITE: No Ack\n");<br>
><br>> @@ -432,7 +455,6 @@ static int i2c_s3c_irq_nextbyte(struct s3c24xx_i2c *i2c,<br>> unsigned long iicstat)<br>><br>> } else {<br>> /* send stop */<br>> -<br>
> s3c24xx_i2c_stop(i2c, 0);<br>> }<br>> break;<br>> @@ -447,7 +469,7 @@ static int i2c_s3c_irq_nextbyte(struct s3c24xx_i2c *i2c,<br>> unsigned long iicstat)<br>
> i2c->msg->buf[i2c->msg_ptr++] = byte;<br>><br>> prepare_read:<br>> - if (is_msglast(i2c)) {<br>> + if (is_msglast(i2c) && is_master(i2c)) {<br>
> /* last byte of buffer */<br>><br>> if (is_lastmsg(i2c))<br>> @@ -612,11 +634,13 @@ static int s3c24xx_i2c_doxfer(struct s3c24xx_i2c *i2c,<br>> if (i2c->suspended)<br>
> return -EIO;<br>><br>> - ret = s3c24xx_i2c_set_master(i2c);<br>> - if (ret != 0) {<br>> - dev_err(i2c->dev, "cannot get bus (error %d)\n", ret);<br>> - ret = -EAGAIN;<br>
> - goto out;<br>> + if (is_master(i2c)) {<br>> + ret = s3c24xx_i2c_set_master(i2c);<br>> + if (ret != 0) {<br>> + dev_err(i2c->dev, "cannot get bus (error %d)\n",<br>
> ret);<br>> + ret = -EAGAIN;<br>> + goto out;<br>> + }<br>> }<br>><br>> i2c->msg = msgs;<br>> @@ -628,23 +652,29 @@ static int s3c24xx_i2c_doxfer(struct s3c24xx_i2c *i2c,<br>
> s3c24xx_i2c_enable_irq(i2c);<br>> s3c24xx_i2c_message_start(i2c, msgs);<br>><br>> - timeout = wait_event_timeout(i2c->wait, i2c->msg_num == 0, HZ * 5);<br>> + if (is_master(i2c))<br>
> + timeout = wait_event_timeout(i2c->wait,\<br>> + i2c->msg_num == 0, HZ * 5);<br>> + else<br>> + wait_event_interruptible(i2c->wait, i2c->msg_num == 0);<br>
><br>> ret = i2c->msg_idx;<br>><br>> /* having these next two as dev_err() makes life very<br>> * noisy when doing an i2cdetect */<br>><br>> - if (timeout == 0)<br>> - dev_dbg(i2c->dev, "timeout\n");<br>
> - else if (ret != num)<br>> - dev_dbg(i2c->dev, "incomplete xfer (%d)\n", ret);<br>> + if (is_master(i2c)) {<br>> + if (timeout == 0)<br>> + dev_dbg(i2c->dev, "timeout\n");<br>
> + else if (ret != num)<br>> + dev_dbg(i2c->dev, "incomplete xfer (%d)\n", ret);<br>><br>> - /* For QUIRK_HDMIPHY, bus is already disabled */<br>> - if (i2c->quirks & QUIRK_HDMIPHY)<br>
> - goto out;<br>> + /* For QUIRK_HDMIPHY, bus is already disabled */<br>> + if (i2c->quirks & QUIRK_HDMIPHY)<br>> + goto out;<br>><br>
> - s3c24xx_i2c_wait_idle(i2c);<br>> + s3c24xx_i2c_wait_idle(i2c);<br>> + }<br>><br>> out:<br>> return ret;<br>> @@ -963,6 +993,7 @@ s3c24xx_i2c_parse_dt(struct device_node *np, struct<br>
> s3c24xx_i2c *i2c)<br>> of_property_read_u32(np, "samsung,i2c-slave-addr",<br>> &pdata->slave_addr);<br>> of_property_read_u32(np, "samsung,i2c-max-bus-freq",<br>> (u32 *)&pdata->frequency);<br>
> + of_property_read_u32(np, "samsung,i2c-mode", &i2c->i2c_mode);<br>> }<br>> #else<br>> static void<br>> @@ -1004,6 +1035,10 @@ static int s3c24xx_i2c_probe(struct platform_device<br>
> *pdev)<br>> goto err_noclk;<br>> }<br>><br>> + /* By default, i2c works in master mode */<br>> + /* This currently will be updated using DT */<br>> + i2c->i2c_mode = 0;<br>
> +<br>> i2c->quirks = s3c24xx_get_device_quirks(pdev);<br>> if (pdata)<br>> memcpy(i2c->pdata, pdata, sizeof(*pdata));<br>> @@ -1017,6 +1052,7 @@ static int s3c24xx_i2c_probe(struct platform_device<br>
> *pdev)<br>> i2c->adap.class = I2C_CLASS_HWMON | I2C_CLASS_SPD;<br>> i2c->tx_setup = 50;<br>><br>> +<br>> init_waitqueue_head(&i2c->wait);<br>><br>> /* find the clock and enable it */<br>
> --<br>> 1.7.9.5<br>><br>><br>><br></div></div>> _______________________________________________<br>> linux-arm-kernel mailing list<br>> <a href="mailto:linux-arm-kernel@lists.infradead.org">linux-arm-kernel@lists.infradead.org</a><br>
> <a href="http://lists.infradead.org/mailman/listinfo/linux-arm-kernel" target="_blank">http://lists.infradead.org/mailman/listinfo/linux-arm-kernel</a><br>><br></blockquote></div><br>