| Message ID | 1426164123-8853-2-git-send-email-wsa@the-dreams.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hello Wolfram, On Thu, Mar 12, 2015 at 01:42:01PM +0100, Wolfram Sang wrote: > From: Wolfram Sang <wsa+renesas@sang-engineering.com> > > After more discussion, brave users, and additional datasheet evaluation, > some API updates for the new I2C slave framework became imminent. The > slave events now get some easier to understand naming. Also, the event > handling has been simplified to only send one event per interrupt. what is an interrupt here? An event where the bus driver needs feedback from the backend? Other than that the patch looks fine. Thanks for working on my feedback! Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Best regards Uwe
On Thu, Mar 19, 2015 at 09:17:51PM +0100, Uwe Kleine-König wrote: > Hello Wolfram, > > On Thu, Mar 12, 2015 at 01:42:01PM +0100, Wolfram Sang wrote: > > From: Wolfram Sang <wsa+renesas@sang-engineering.com> > > > > After more discussion, brave users, and additional datasheet evaluation, > > some API updates for the new I2C slave framework became imminent. The > > slave events now get some easier to understand naming. Also, the event > > handling has been simplified to only send one event per interrupt. > what is an interrupt here? An event where the bus driver needs feedback > from the backend? More the other way around: when the bus driver needs to notify the backend. I wasn't 100% sure about the word 'interrupt', but then decided a HW slave support without interrupts would be so rare and adventurous that it is okay to use the term :) > Other than that the patch looks fine. Thanks for working on my feedback! > Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Thanks, Wolfram
Moin Wolfram, On Fri, Mar 20, 2015 at 08:15:04AM +0100, Wolfram Sang wrote: > On Thu, Mar 19, 2015 at 09:17:51PM +0100, Uwe Kleine-König wrote: > > Hello Wolfram, > > > > On Thu, Mar 12, 2015 at 01:42:01PM +0100, Wolfram Sang wrote: > > > From: Wolfram Sang <wsa+renesas@sang-engineering.com> > > > > > > After more discussion, brave users, and additional datasheet evaluation, > > > some API updates for the new I2C slave framework became imminent. The > > > slave events now get some easier to understand naming. Also, the event > > > handling has been simplified to only send one event per interrupt. > > what is an interrupt here? An event where the bus driver needs feedback > > from the backend? > > More the other way around: when the bus driver needs to notify the > backend. I wasn't 100% sure about the word 'interrupt', but then decided > a HW slave support without interrupts would be so rare and adventurous > that it is okay to use the term :) Yeah, I agree on HW slave support without interrupts is hardly possible. But I imagine that controllers differ in which situations they can issue an interrupt so talking about them for generic code feels strange to me because $flexiblecontrolerwithvariousirqs doesn't need to send more events than $bareminimumcontroler. Best regards Uwe
> > > > After more discussion, brave users, and additional datasheet evaluation, > > > > some API updates for the new I2C slave framework became imminent. The > > > > slave events now get some easier to understand naming. Also, the event > > > > handling has been simplified to only send one event per interrupt. > > > what is an interrupt here? An event where the bus driver needs feedback > > > from the backend? > > > > More the other way around: when the bus driver needs to notify the > > backend. I wasn't 100% sure about the word 'interrupt', but then decided > > a HW slave support without interrupts would be so rare and adventurous > > that it is okay to use the term :) > Yeah, I agree on HW slave support without interrupts is hardly possible. > But I imagine that controllers differ in which situations they can issue > an interrupt so talking about them for generic code feels strange to me > because $flexiblecontrolerwithvariousirqs doesn't need to send more > events than $bareminimumcontroler. Do you have a better word at hand? "...to send one event per event"? :)
On Fri, Mar 20, 2015 at 08:31:35AM +0100, Wolfram Sang wrote: > > > > > > After more discussion, brave users, and additional datasheet evaluation, > > > > > some API updates for the new I2C slave framework became imminent. The > > > > > slave events now get some easier to understand naming. Also, the event > > > > > handling has been simplified to only send one event per interrupt. > > > > what is an interrupt here? An event where the bus driver needs feedback > > > > from the backend? > > > > > > More the other way around: when the bus driver needs to notify the > > > backend. I wasn't 100% sure about the word 'interrupt', but then decided > > > a HW slave support without interrupts would be so rare and adventurous > > > that it is okay to use the term :) > > Yeah, I agree on HW slave support without interrupts is hardly possible. > > But I imagine that controllers differ in which situations they can issue > > an interrupt so talking about them for generic code feels strange to me > > because $flexiblecontrolerwithvariousirqs doesn't need to send more > > events than $bareminimumcontroler. > > Do you have a better word at hand? "...to send one event per event"? :) Maybe: Also, the event handling has been simplified to only need a single call to the slave callback when an action by the backend is required. Best regards Uwe
On Fri, Mar 20, 2015 at 08:44:47AM +0100, Uwe Kleine-König wrote: > On Fri, Mar 20, 2015 at 08:31:35AM +0100, Wolfram Sang wrote: > > > > > > > > After more discussion, brave users, and additional datasheet evaluation, > > > > > > some API updates for the new I2C slave framework became imminent. The > > > > > > slave events now get some easier to understand naming. Also, the event > > > > > > handling has been simplified to only send one event per interrupt. > > > > > what is an interrupt here? An event where the bus driver needs feedback > > > > > from the backend? > > > > > > > > More the other way around: when the bus driver needs to notify the > > > > backend. I wasn't 100% sure about the word 'interrupt', but then decided > > > > a HW slave support without interrupts would be so rare and adventurous > > > > that it is okay to use the term :) > > > Yeah, I agree on HW slave support without interrupts is hardly possible. > > > But I imagine that controllers differ in which situations they can issue > > > an interrupt so talking about them for generic code feels strange to me > > > because $flexiblecontrolerwithvariousirqs doesn't need to send more > > > events than $bareminimumcontroler. > > > > Do you have a better word at hand? "...to send one event per event"? :) > Maybe: > > Also, the event handling has been simplified to only need a > single call to the slave callback when an action by the backend > is required. Bought. Thank you.
diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c index 71a6e07eb7ab7c..5a84bea5b84514 100644 --- a/drivers/i2c/busses/i2c-rcar.c +++ b/drivers/i2c/busses/i2c-rcar.c @@ -382,11 +382,11 @@ static bool rcar_i2c_slave_irq(struct rcar_i2c_priv *priv) if (ssr_filtered & SAR) { /* read or write request */ if (ssr_raw & STM) { - i2c_slave_event(priv->slave, I2C_SLAVE_REQ_READ_START, &value); + i2c_slave_event(priv->slave, I2C_SLAVE_READ_REQUESTED, &value); rcar_i2c_write(priv, ICRXTX, value); rcar_i2c_write(priv, ICSIER, SDE | SSR | SAR); } else { - i2c_slave_event(priv->slave, I2C_SLAVE_REQ_WRITE_START, &value); + i2c_slave_event(priv->slave, I2C_SLAVE_WRITE_REQUESTED, &value); rcar_i2c_read(priv, ICRXTX); /* dummy read */ rcar_i2c_write(priv, ICSIER, SDR | SSR | SAR); } @@ -406,17 +406,15 @@ static bool rcar_i2c_slave_irq(struct rcar_i2c_priv *priv) int ret; value = rcar_i2c_read(priv, ICRXTX); - ret = i2c_slave_event(priv->slave, I2C_SLAVE_REQ_WRITE_END, &value); + ret = i2c_slave_event(priv->slave, I2C_SLAVE_WRITE_RECEIVED, &value); /* Send NACK in case of error */ rcar_i2c_write(priv, ICSCR, SIE | SDBS | (ret < 0 ? FNA : 0)); - i2c_slave_event(priv->slave, I2C_SLAVE_REQ_WRITE_START, &value); rcar_i2c_write(priv, ICSSR, ~SDR & 0xff); } /* master wants to read from us */ if (ssr_filtered & SDE) { - i2c_slave_event(priv->slave, I2C_SLAVE_REQ_READ_END, &value); - i2c_slave_event(priv->slave, I2C_SLAVE_REQ_READ_START, &value); + i2c_slave_event(priv->slave, I2C_SLAVE_READ_PROCESSED, &value); rcar_i2c_write(priv, ICRXTX, value); rcar_i2c_write(priv, ICSSR, ~SDE & 0xff); } diff --git a/drivers/i2c/i2c-slave-eeprom.c b/drivers/i2c/i2c-slave-eeprom.c index cf9b09db092f4e..3fb45d894d8072 100644 --- a/drivers/i2c/i2c-slave-eeprom.c +++ b/drivers/i2c/i2c-slave-eeprom.c @@ -36,7 +36,7 @@ static int i2c_slave_eeprom_slave_cb(struct i2c_client *client, struct eeprom_data *eeprom = i2c_get_clientdata(client); switch (event) { - case I2C_SLAVE_REQ_WRITE_END: + case I2C_SLAVE_WRITE_RECEIVED: if (eeprom->first_write) { eeprom->buffer_idx = *val; eeprom->first_write = false; @@ -47,17 +47,17 @@ static int i2c_slave_eeprom_slave_cb(struct i2c_client *client, } break; - case I2C_SLAVE_REQ_READ_START: + case I2C_SLAVE_READ_PROCESSED: + eeprom->buffer_idx++; + /* fallthrough */ + case I2C_SLAVE_READ_REQUESTED: spin_lock(&eeprom->buffer_lock); *val = eeprom->buffer[eeprom->buffer_idx]; spin_unlock(&eeprom->buffer_lock); break; - case I2C_SLAVE_REQ_READ_END: - eeprom->buffer_idx++; - break; - case I2C_SLAVE_STOP: + case I2C_SLAVE_WRITE_REQUESTED: eeprom->first_write = true; break; diff --git a/include/linux/i2c.h b/include/linux/i2c.h index f17da50402a4da..f76031608cb723 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -253,10 +253,10 @@ static inline void i2c_set_clientdata(struct i2c_client *dev, void *data) #if IS_ENABLED(CONFIG_I2C_SLAVE) enum i2c_slave_event { - I2C_SLAVE_REQ_READ_START, - I2C_SLAVE_REQ_READ_END, - I2C_SLAVE_REQ_WRITE_START, - I2C_SLAVE_REQ_WRITE_END, + I2C_SLAVE_READ_REQUESTED, + I2C_SLAVE_WRITE_REQUESTED, + I2C_SLAVE_READ_PROCESSED, + I2C_SLAVE_WRITE_RECEIVED, I2C_SLAVE_STOP, };