From: Wolfram Sang <hidden> Date: 2015-02-25 16:02:31
From: Wolfram Sang <wsa+renesas@sang-engineering.com>
Here is the second version of the patch series to describe i2c adapter quirks
in a generic way. For the motivation, please read description of patch 1. This
is still RFC because I would like to do some more tests on my own, but I need
to write a tool for that. However, I'd really like to have the driver authors
to have a look already. Actual testing is very much appreciated. Thanks to the
Mediatek guys for rebasing their new driver to this framework. That helps, too!
The branch is also here:
git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/quirks
Thanks,
Wolfram
Major changes since V1:
* more fine-grained options to describe modes with combined messages.
This should also cover the Mediatek HW now as well as all other
permutations I can think of.
* the core code and at91 driver had to be refactored to reflect the
above change
* added the bcm-iproc driver which came to mainline recently
Wolfram Sang (12):
i2c: add quirk structure to describe adapter flaws
i2c: add quirk checks to core
i2c: at91: make use of the new infrastructure for quirks
i2c: opal: make use of the new infrastructure for quirks
i2c: qup: make use of the new infrastructure for quirks
i2c: cpm: make use of the new infrastructure for quirks
i2c: axxia: make use of the new infrastructure for quirks
i2c: dln2: make use of the new infrastructure for quirks
i2c: powermac: make use of the new infrastructure for quirks
i2c: viperboard: make use of the new infrastructure for quirks
i2c: pmcmsp: make use of the new infrastructure for quirks
i2c: bcm-iproc: make use of the new infrastructure for quirks
drivers/i2c/busses/i2c-at91.c | 32 +++++++------------
drivers/i2c/busses/i2c-axxia.c | 11 ++++---
drivers/i2c/busses/i2c-bcm-iproc.c | 15 +++++----
drivers/i2c/busses/i2c-cpm.c | 20 ++++++------
drivers/i2c/busses/i2c-dln2.c | 12 +++----
drivers/i2c/busses/i2c-opal.c | 22 ++++++-------
drivers/i2c/busses/i2c-pmcmsp.c | 42 ++++++++++---------------
drivers/i2c/busses/i2c-powermac.c | 10 +++---
drivers/i2c/busses/i2c-qup.c | 21 ++++++-------
drivers/i2c/busses/i2c-viperboard.c | 10 +++---
drivers/i2c/i2c-core.c | 62 +++++++++++++++++++++++++++++++++++++
include/linux/i2c.h | 43 +++++++++++++++++++++++++
12 files changed, 191 insertions(+), 109 deletions(-)
--
2.1.4
@@ -487,30 +487,10 @@ static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num)if(ret<0)gotoout;-/*-*Thehardwarecanhandleatmosttwomessagesconcatenatedbya-*repeatedstartviait'sinternaladdressfeature.-*/-if(num>2){-dev_err(dev->dev,-"cannot handle more than two concatenated messages.\n");-ret=0;-gotoout;-}elseif(num==2){+if(num==2){intinternal_address=0;inti;-if(msg->flags&I2C_M_RD){-dev_err(dev->dev,"first transfer must be write.\n");-ret=-EINVAL;-gotoout;-}-if(msg->len>3){-dev_err(dev->dev,"first message size must be <= 3.\n");-ret=-EINVAL;-gotoout;-}-/* 1st msg is put into the internal address, start with 2nd */m_start=&msg[1];for(i=0;i<msg->len;++i){
From: Wolfram Sang <hidden> Date: 2015-02-25 16:02:32
From: Wolfram Sang <wsa+renesas@sang-engineering.com>
The number of I2C adapters which are not fully I2C compatible is rising,
sadly. Drivers usually do handle the flaws, still the user receives only
some errno for a transfer which normally can be expected to work. This
patch introduces a formal description of flaws. One advantage is that
the core can check before the actual transfer if the messages could be
transferred at all. This is done in the next patch. Another advantage is
that we can pass this information to the user so the restrictions are
exactly known and further actions can be based on that. This will be
done later after some stabilization period for this description.
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
include/linux/i2c.h | 43 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 43 insertions(+)
@@ -449,6 +449,48 @@ int i2c_recover_bus(struct i2c_adapter *adap);inti2c_generic_gpio_recovery(structi2c_adapter*adap);inti2c_generic_scl_recovery(structi2c_adapter*adap);+/**+*structi2c_adapter_quirks-describeflawsofani2cadapter+*@flags:seeI2C_AQ_*forpossibleflagsandreadbelow+*@max_num_msgs:maximumnumberofmessagespertransfer+*@max_write_len:maximumlengthofawritemessage+*@max_read_len:maximumlengthofareadmessage+*@max_comb_1st_msg_len:maximumlengthofthefirstmsginacombinedmessage+*@max_comb_2nd_msg_len:maximumlengthofthesecondmsginacombinedmessage+*+*Noteaboutcombinedmessages:SomeI2Ccontrollerscanonlysendonemessage+*pertransfer,plussomethingcalledcombinedmessageorwrite-then-read.+*Thisis(usually)asmallwritemessagefollowedbyareadmessageand+*barelyenoughtoaccessregisterbaseddeviceslikeEEPROMs.Thereisaflag+*tosupportthismode.Itimpliesmax_num_msg=2anddoesthelengthchecks+*withmax_comb_*_lenbecausecombinedmessagemodeusuallyhasitsown+*limitations.BecauseofHWimplementations,somecontrollerscanactuallydo+*write-then-anythingorothervariants.Tosupportthat,write-then-readhas+*beenbrokenoutintosmallerbitslikewrite-firstandread-secondwhichcan+*becombinedasneeded.+*/++structi2c_adapter_quirks{+u64flags;+intmax_num_msgs;+u16max_write_len;+u16max_read_len;+u16max_comb_1st_msg_len;+u16max_comb_2nd_msg_len;+};++/* enforce max_num_msgs = 2 and use max_comb_*_len for length checks */+#define I2C_AQ_COMB BIT(0)+/* first combined message must be write */+#define I2C_AQ_COMB_WRITE_FIRST BIT(1)+/* second combined message must be read */+#define I2C_AQ_COMB_READ_SECOND BIT(2)+/* both combined messages must have the same target address */+#define I2C_AQ_COMB_SAME_ADDR BIT(3)+/* convenience macro for typical write-then read case */+#define I2C_AQ_COMB_WRITE_THEN_READ (I2C_AQ_COMB | I2C_AQ_COMB_WRITE_FIRST | \+I2C_AQ_COMB_READ_SECOND|I2C_AQ_COMB_SAME_ADDR)+/**i2c_adapteristhestructureusedtoidentifyaphysicali2cbusalong*withtheaccessalgorithmsnecessarytoaccessit.
From: Wolfram Sang <hidden> Date: 2015-02-25 16:02:34
From: Wolfram Sang <wsa+renesas@sang-engineering.com>
Let the core do the checks if HW quirks prevent a transfer. Saves code
from drivers and adds consistency.
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
drivers/i2c/i2c-core.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 62 insertions(+)
@@ -1929,6 +1929,65 @@ module_exit(i2c_exit);*----------------------------------------------------*/+/* Check if val is exceeding the quirk IFF quirk is non 0 */+#define i2c_quirk_exceeded(val, quirk) ((quirk) && ((val) > (quirk)))++staticinti2c_quirk_error(structi2c_adapter*adap,structi2c_msg*msg,char*err_msg)+{+dev_err_ratelimited(&adap->dev,"quirk: %s (addr 0x%04x, size %u, %s)\n",+err_msg,msg->addr,msg->len,+msg->flags&I2C_M_RD?"read":"write");+return-EOPNOTSUPP;+}++staticinti2c_check_for_quirks(structi2c_adapter*adap,structi2c_msg*msgs,intnum)+{+conststructi2c_adapter_quirks*q=adap->quirks;+intmax_num=q->max_num_msgs,i;+booldo_len_check=true;++if(q->flags&I2C_AQ_COMB){+max_num=2;++/* special checks for combined messages */+if(num==2){+if(q->flags&I2C_AQ_COMB_WRITE_FIRST&&msgs[0].flags&I2C_M_RD)+returni2c_quirk_error(adap,&msgs[0],"1st comb msg not write");++if(q->flags&I2C_AQ_COMB_READ_SECOND&&!(msgs[1].flags&I2C_M_RD))+returni2c_quirk_error(adap,&msgs[1],"2nd comb msg not read");++if(q->flags&I2C_AQ_COMB_SAME_ADDR&&msgs[0].addr!=msgs[1].addr)+returni2c_quirk_error(adap,&msgs[0],"addresses do not match");++if(i2c_quirk_exceeded(msgs[0].len,q->max_comb_1st_msg_len))+returni2c_quirk_error(adap,&msgs[0],"msg too long");++if(i2c_quirk_exceeded(msgs[1].len,q->max_comb_2nd_msg_len))+returni2c_quirk_error(adap,&msgs[1],"msg too long");++do_len_check=false;+}+}++if(i2c_quirk_exceeded(num,max_num))+returni2c_quirk_error(adap,&msgs[0],"too many messages");++for(i=0;i<num;i++){+u16len=msgs[i].len;++if(msgs[i].flags&I2C_M_RD){+if(do_len_check&&i2c_quirk_exceeded(len,q->max_read_len))+returni2c_quirk_error(adap,&msgs[i],"msg too long");+}else{+if(do_len_check&&i2c_quirk_exceeded(len,q->max_write_len))+returni2c_quirk_error(adap,&msgs[i],"msg too long");+}+}++return0;+}+/***__i2c_transfer-unlockedflavorofi2c_transfer*@adap:HandletoI2Cbus
@@ -1946,6 +2005,9 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)unsignedlongorig_jiffies;intret,try;+if(adap->quirks&&i2c_check_for_quirks(adap,msgs,num))+return-EOPNOTSUPP;+/* i2c_trace_msg gets enabled when tracepoint i2c_transfer gets*enabled.Thisisanefficientwayofkeepingthefor-loopfrom*beingexecutedwhennotneeded.
@@ -412,17 +412,6 @@ static int qup_i2c_read_one(struct qup_i2c_dev *qup, struct i2c_msg *msg)unsignedlongleft;intret;-/*-*TheQUPblockwillissueaNACKandSTOPonthebuswhenreaching-*theendoftheread,thelengthofthereadisspecifiedasonebyte-*whichlimitsthepossiblereadto256(QUP_READ_LIMIT)bytes.-*/-if(msg->len>QUP_READ_LIMIT){-dev_err(qup->dev,"HW not capable of reads over %d bytes\n",-QUP_READ_LIMIT);-return-EINVAL;-}-qup->msg=msg;qup->pos=0;
@@ -308,22 +308,12 @@ static int cpm_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)structi2c_reg__iomem*i2c_reg=cpm->i2c_reg;structi2c_ram__iomem*i2c_ram=cpm->i2c_ram;structi2c_msg*pmsg;-intret,i;+intret;inttptr;intrptr;cbd_t__iomem*tbdf;cbd_t__iomem*rbdf;-if(num>CPM_MAXBD)-return-EINVAL;--/* Check if we have any oversized READ requests */-for(i=0;i<num;i++){-pmsg=&msgs[i];-if(pmsg->len>=CPM_MAX_READ)-return-EINVAL;-}-/* Reset to use first buffer */out_be16(&i2c_ram->rbptr,in_be16(&i2c_ram->rbase));out_be16(&i2c_ram->tbptr,in_be16(&i2c_ram->tbase));
@@ -424,10 +414,18 @@ static const struct i2c_algorithm cpm_i2c_algo = {.functionality=cpm_i2c_func,};+/* CPM_MAX_READ is also limiting writes according to the code! */+staticstructi2c_adapter_quirkscpm_i2c_quirks={+.max_num_msgs=CPM_MAXBD,+.max_read_len=CPM_MAX_READ,+.max_write_len=CPM_MAX_READ,+};+staticconststructi2c_adaptercpm_ops={.owner=THIS_MODULE,.name="i2c-cpm",.algo=&cpm_i2c_algo,+.quirks=&cpm_i2c_quirks,};staticintcpm_i2c_setup(structcpm_i2c*cpm)
@@ -288,10 +288,6 @@ static int vprbrd_i2c_xfer(struct i2c_adapter *i2c, struct i2c_msg *msgs,i,pmsg->flags&I2C_M_RD?"read":"write",pmsg->flags,pmsg->len,pmsg->addr);-/* msgs longer than 2048 bytes are not supported by adapter */-if(pmsg->len>2048)-return-EINVAL;-mutex_lock(&vb->lock);/* directly send the message */if(pmsg->flags&I2C_M_RD){
@@ -373,6 +374,7 @@ static int vprbrd_i2c_probe(struct platform_device *pdev)vb_i2c->i2c.owner=THIS_MODULE;vb_i2c->i2c.class=I2C_CLASS_HWMON;vb_i2c->i2c.algo=&vprbrd_algorithm;+vb_i2c->i2c.quirks=&vprbrd_quirks;vb_i2c->i2c.algo_data=vb;/* save the param in usb capabable memory */vb_i2c->bus_freq_param=i2c_bus_param;
@@ -160,14 +160,6 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,u32val;unsignedlongtime_left=msecs_to_jiffies(I2C_TIMEOUT_MESC);-/* need to reserve one byte in the FIFO for the slave address */-if(msg->len>M_TX_RX_FIFO_SIZE-1){-dev_err(iproc_i2c->device,-"only support data length up to %u bytes\n",-M_TX_RX_FIFO_SIZE-1);-return-EOPNOTSUPP;-}-/* check if bus is busy */if(!!(readl(iproc_i2c->base+M_CMD_OFFSET)&BIT(M_CMD_START_BUSY_SHIFT))){
@@ -287,6 +279,12 @@ static const struct i2c_algorithm bcm_iproc_algo = {.functionality=bcm_iproc_i2c_functionality,};+staticstructi2c_adapter_quirksbcm_iproc_i2c_quirks={+/* need to reserve one byte in the FIFO for the slave address */+.max_read_len=M_TX_RX_FIFO_SIZE-1,+.max_write_len=M_TX_RX_FIFO_SIZE-1,+};+staticintbcm_iproc_i2c_cfg_speed(structbcm_iproc_i2c_dev*iproc_i2c){unsignedintbus_speed;
@@ -456,14 +456,6 @@ static enum pmcmsptwi_xfer_result pmcmsptwi_xfer_cmd(return-EINVAL;}-if(cmd->read_len>MSP_MAX_BYTES_PER_RW||-cmd->write_len>MSP_MAX_BYTES_PER_RW){-dev_err(&pmcmsptwi_adapter.dev,-"%s: Cannot transfer more than %d bytes\n",-__func__,MSP_MAX_BYTES_PER_RW);-return-EINVAL;-}-mutex_lock(&data->lock);dev_dbg(&pmcmsptwi_adapter.dev,"Setting address to 0x%04x\n",cmd->addr);
@@ -520,25 +512,14 @@ static int pmcmsptwi_master_xfer(struct i2c_adapter *adap,structpmcmsptwi_cfgoldcfg,newcfg;intret;-if(num>2){-dev_dbg(&adap->dev,"%d messages unsupported\n",num);-return-EINVAL;-}elseif(num==2){-/* Check for a dual write-then-read command */+if(num==2){structi2c_msg*nextmsg=msg+1;-if(!(msg->flags&I2C_M_RD)&&-(nextmsg->flags&I2C_M_RD)&&-msg->addr==nextmsg->addr){-cmd.type=MSP_TWI_CMD_WRITE_READ;-cmd.write_len=msg->len;-cmd.write_data=msg->buf;-cmd.read_len=nextmsg->len;-cmd.read_data=nextmsg->buf;-}else{-dev_dbg(&adap->dev,-"Non write-read dual messages unsupported\n");-return-EINVAL;-}++cmd.type=MSP_TWI_CMD_WRITE_READ;+cmd.write_len=msg->len;+cmd.write_data=msg->buf;+cmd.read_len=nextmsg->len;+cmd.read_data=nextmsg->buf;}elseif(msg->flags&I2C_M_RD){cmd.type=MSP_TWI_CMD_READ;cmd.read_len=msg->len;
@@ -104,17 +104,6 @@ static int i2c_opal_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,req.buffer_ra=cpu_to_be64(__pa(msgs[0].buf));break;case2:-/* For two messages, we basically support only simple-*smbustransactionsofawriteplusaread.Wemight-*wanttoallowalsotwowritesbutwe'dhavetobounce-*thedataintoasinglebuffer.-*/-if((msgs[0].flags&I2C_M_RD)||!(msgs[1].flags&I2C_M_RD))-return-EOPNOTSUPP;-if(msgs[0].len>4)-return-EOPNOTSUPP;-if(msgs[0].addr!=msgs[1].addr)-return-EOPNOTSUPP;req.type=OPAL_I2C_SM_READ;req.addr=cpu_to_be16(msgs[0].addr);req.subaddr_sz=msgs[0].len;
@@ -210,6 +199,16 @@ static const struct i2c_algorithm i2c_opal_algo = {.functionality=i2c_opal_func,};+/* For two messages, we basically support only simple+*smbustransactionsofawriteplusaread.Wemight+*wanttoallowalsotwowritesbutwe'dhavetobounce+*thedataintoasinglebuffer.+*/+staticstructi2c_adapter_quirksi2c_opal_quirks={+.flags=I2C_AQ_COMB_WRITE_THEN_READ,+.max_comb_1st_msg_len=4,+};+staticinti2c_opal_probe(structplatform_device*pdev){structi2c_adapter*adapter;
@@ -232,6 +231,7 @@ static int i2c_opal_probe(struct platform_device *pdev)adapter->algo=&i2c_opal_algo;adapter->algo_data=(void*)(unsignedlong)opal_id;+adapter->quirks=&i2c_opal_quirks;adapter->dev.parent=&pdev->dev;adapter->dev.of_node=of_node_get(pdev->dev.of_node);pname=of_get_property(pdev->dev.of_node,"ibm,port-name",NULL);
@@ -160,14 +160,6 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,u32val;unsignedlongtime_left=msecs_to_jiffies(I2C_TIMEOUT_MESC);-/* need to reserve one byte in the FIFO for the slave address */-if(msg->len>M_TX_RX_FIFO_SIZE-1){-dev_err(iproc_i2c->device,-"only support data length up to %u bytes\n",-M_TX_RX_FIFO_SIZE-1);-return-EOPNOTSUPP;-}-/* check if bus is busy */if(!!(readl(iproc_i2c->base+M_CMD_OFFSET)&BIT(M_CMD_START_BUSY_SHIFT))){
@@ -287,6 +279,12 @@ static const struct i2c_algorithm bcm_iproc_algo = {.functionality=bcm_iproc_i2c_functionality,};+staticstructi2c_adapter_quirksbcm_iproc_i2c_quirks={+/* need to reserve one byte in the FIFO for the slave address */+.max_read_len=M_TX_RX_FIFO_SIZE-1,+.max_write_len=M_TX_RX_FIFO_SIZE-1,+};+staticintbcm_iproc_i2c_cfg_speed(structbcm_iproc_i2c_dev*iproc_i2c){unsignedintbus_speed;
Change on the iproc i2c driver looks good to me. Sanity tested the
change from Wolfram's i2c/quirks branch on Cygnus 958300K combo board.
Sanity tested with an attempt to transfer large amount of I2C data to
ensure the transfer is denied by the i2c-core:
/ # cat /dev/i2c-0
[ 657.310261] i2c i2c-0: quirk: msg too long (addr 0x0000, size 4096, read)
Reviewed-by: Ray Jui <rjui@broadcom.com>
Tested-by: Ray Jui <rjui@broadcom.com>
Thanks,
Ray
From: Ivan T. Ivanov <hidden> Date: 2015-03-05 13:27:11
On Wed, 2015-02-25 at 17:01 +0100, Wolfram Sang wrote:
From: Wolfram Sang <wsa+renesas@sang-engineering.com>
Here is the second version of the patch series to describe i2c adapter quirks
in a generic way. For the motivation, please read description of patch 1. This
is still RFC because I would like to do some more tests on my own, but I need
to write a tool for that. However, I'd really like to have the driver authors
to have a look already. Actual testing is very much appreciated. Thanks to the
Mediatek guys for rebasing their new driver to this framework. That helps, too!
The branch is also here:
git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/quirks
Thanks,
Wolfram
Major changes since V1:
* more fine-grained options to describe modes with combined messages.
This should also cover the Mediatek HW now as well as all other
permutations I can think of.
* the core code and at91 driver had to be refactored to reflect the
above change
* added the bcm-iproc driver which came to mainline recently
Wolfram Sang (12):
i2c: add quirk structure to describe adapter flaws
i2c: add quirk checks to core
i2c: at91: make use of the new infrastructure for quirks
i2c: opal: make use of the new infrastructure for quirks
i2c: qup: make use of the new infrastructure for quirks
For QUP driver.
Reviewed-by: Ivan T. Ivanov <redacted>
Tested-by: Ivan T. Ivanov <redacted>
Thanks,
Ivan
From: Wolfram Sang <hidden> Date: 2015-03-08 08:28:39
On Wed, Feb 25, 2015 at 05:01:54PM +0100, Wolfram Sang wrote:
From: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Hi Ludovic,
if you have a few minutes, could you please test this series? I'd like to
include it in 4.1. and because at91 is using the quirk infrastructure in
a more complex way, it is a really good test candidate.
Thanks,
Wolfram
@@ -487,30 +487,10 @@ static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num)if(ret<0)gotoout;-/*-*Thehardwarecanhandleatmosttwomessagesconcatenatedbya-*repeatedstartviait'sinternaladdressfeature.-*/-if(num>2){-dev_err(dev->dev,-"cannot handle more than two concatenated messages.\n");-ret=0;-gotoout;-}elseif(num==2){+if(num==2){intinternal_address=0;inti;-if(msg->flags&I2C_M_RD){-dev_err(dev->dev,"first transfer must be write.\n");-ret=-EINVAL;-gotoout;-}-if(msg->len>3){-dev_err(dev->dev,"first message size must be <= 3.\n");-ret=-EINVAL;-gotoout;-}-/* 1st msg is put into the internal address, start with 2nd */m_start=&msg[1];for(i=0;i<msg->len;++i){
Hi Wolfram,
On Sun, Mar 08, 2015 at 09:28:45AM +0100, Wolfram Sang wrote:
On Wed, Feb 25, 2015 at 05:01:54PM +0100, Wolfram Sang wrote:
quoted
From: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Hi Ludovic,
if you have a few minutes, could you please test this series? I'd like to
include it in 4.1. and because at91 is using the quirk infrastructure in
a more complex way, it is a really good test candidate.
It was in the pipe. I have reviewed it, this second version seems to be
good. I am just waiting a bit more to give you my ack since I have some
issues to read an i2c eeprom (it works with a temperature sensor).
I am investigating if it doesn't come from a previous regression.
Hi Wolfram,
You can add my
Acked-by and Tested-By: Ludovic Desroches [off-list ref]
Tested on sama5d3, some problems with at24 eeprom on sama5d4 but it
doesn't come from the i2c quirks patch series.
Regards
Ludovic
On Sun, Mar 08, 2015 at 09:28:45AM +0100, Wolfram Sang wrote:
On Wed, Feb 25, 2015 at 05:01:54PM +0100, Wolfram Sang wrote:
quoted
From: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Hi Ludovic,
if you have a few minutes, could you please test this series? I'd like to
include it in 4.1. and because at91 is using the quirk infrastructure in
a more complex way, it is a really good test candidate.
Thanks,
Wolfram
@@ -487,30 +487,10 @@ static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num)if(ret<0)gotoout;-/*-*Thehardwarecanhandleatmosttwomessagesconcatenatedbya-*repeatedstartviait'sinternaladdressfeature.-*/-if(num>2){-dev_err(dev->dev,-"cannot handle more than two concatenated messages.\n");-ret=0;-gotoout;-}elseif(num==2){+if(num==2){intinternal_address=0;inti;-if(msg->flags&I2C_M_RD){-dev_err(dev->dev,"first transfer must be write.\n");-ret=-EINVAL;-gotoout;-}-if(msg->len>3){-dev_err(dev->dev,"first message size must be <= 3.\n");-ret=-EINVAL;-gotoout;-}-/* 1st msg is put into the internal address, start with 2nd */m_start=&msg[1];for(i=0;i<msg->len;++i){
I tested the i2c opal driver after updating the patch as below.
Basically I think we can also support write-then-{read/write}
for the number of messages = 2.
Ben, any issues if we support both write plus read/write in the
opal driver ?
Regards,
Neelesh
drivers/i2c/busses/i2c-opal.c | 20 ++++++++------------
1 file changed, 8 insertions(+), 12 deletions(-)
@@ -104,18 +104,8 @@ static int i2c_opal_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,req.buffer_ra=cpu_to_be64(__pa(msgs[0].buf));break;case2:-/* For two messages, we basically support only simple-*smbustransactionsofawriteplusaread.Wemight-*wanttoallowalsotwowritesbutwe'dhavetobounce-*thedataintoasinglebuffer.-*/-if((msgs[0].flags&I2C_M_RD)||!(msgs[1].flags&I2C_M_RD))-return-EOPNOTSUPP;-if(msgs[0].len>4)-return-EOPNOTSUPP;-if(msgs[0].addr!=msgs[1].addr)-return-EOPNOTSUPP;-req.type=OPAL_I2C_SM_READ;+req.type=(msgs[1].flags&I2C_M_RD)?+OPAL_I2C_SM_READ:OPAL_I2C_SM_WRITE;req.addr=cpu_to_be16(msgs[0].addr);req.subaddr_sz=msgs[0].len;for(i=0;i<msgs[0].len;i++)
@@ -104,17 +104,6 @@ static int i2c_opal_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,req.buffer_ra=cpu_to_be64(__pa(msgs[0].buf));break;case2:-/* For two messages, we basically support only simple-*smbustransactionsofawriteplusaread.Wemight-*wanttoallowalsotwowritesbutwe'dhavetobounce-*thedataintoasinglebuffer.-*/-if((msgs[0].flags&I2C_M_RD)||!(msgs[1].flags&I2C_M_RD))-return-EOPNOTSUPP;-if(msgs[0].len>4)-return-EOPNOTSUPP;-if(msgs[0].addr!=msgs[1].addr)-return-EOPNOTSUPP;req.type=OPAL_I2C_SM_READ;req.addr=cpu_to_be16(msgs[0].addr);req.subaddr_sz=msgs[0].len;
@@ -210,6 +199,16 @@ static const struct i2c_algorithm i2c_opal_algo = {.functionality=i2c_opal_func,};+/* For two messages, we basically support only simple+*smbustransactionsofawriteplusaread.Wemight+*wanttoallowalsotwowritesbutwe'dhavetobounce+*thedataintoasinglebuffer.+*/+staticstructi2c_adapter_quirksi2c_opal_quirks={+.flags=I2C_AQ_COMB_WRITE_THEN_READ,+.max_comb_1st_msg_len=4,+};+staticinti2c_opal_probe(structplatform_device*pdev){structi2c_adapter*adapter;
@@ -232,6 +231,7 @@ static int i2c_opal_probe(struct platform_device *pdev)adapter->algo=&i2c_opal_algo;adapter->algo_data=(void*)(unsignedlong)opal_id;+adapter->quirks=&i2c_opal_quirks;adapter->dev.parent=&pdev->dev;adapter->dev.of_node=of_node_get(pdev->dev.of_node);pname=of_get_property(pdev->dev.of_node,"ibm,port-name",NULL);
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2015-03-10 23:13:17
On Tue, 2015-03-10 at 22:43 +0530, Neelesh Gupta wrote:
I tested the i2c opal driver after updating the patch as below.
Basically I think we can also support write-then-{read/write}
for the number of messages = 2.
Ben, any issues if we support both write plus read/write in the
opal driver ?
Nope, in fact it's a good idea, I found myself having to expoes such
an interface to some userspace tool of ours.
However...
@@ -104,18 +104,8 @@ static int i2c_opal_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,req.buffer_ra=cpu_to_be64(__pa(msgs[0].buf));break;case2:-/* For two messages, we basically support only simple-*smbustransactionsofawriteplusaread.Wemight-*wanttoallowalsotwowritesbutwe'dhavetobounce-*thedataintoasinglebuffer.-*/-if((msgs[0].flags&I2C_M_RD)||!(msgs[1].flags&I2C_M_RD))-return-EOPNOTSUPP;
Don't we still want to enforce that the first message is a write ?
Somebody may not be looking at the quirks...
- if (msgs[0].len > 4)
- return -EOPNOTSUPP;
And that the len is supported...
- if (msgs[0].addr != msgs[1].addr)
- return -EOPNOTSUPP;
Same...
Ie, the quirk indicates to the callers what we support, but we should
still check that we aren't called with something that doesn't match.
quoted hunk
- req.type = OPAL_I2C_SM_READ;
+ req.type = (msgs[1].flags & I2C_M_RD) ?
+ OPAL_I2C_SM_READ : OPAL_I2C_SM_WRITE;
req.addr = cpu_to_be16(msgs[0].addr);
req.subaddr_sz = msgs[0].len;
for (i = 0; i < msgs[0].len; i++)
@@ -104,17 +104,6 @@ static int i2c_opal_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,req.buffer_ra=cpu_to_be64(__pa(msgs[0].buf));break;case2:-/* For two messages, we basically support only simple-*smbustransactionsofawriteplusaread.Wemight-*wanttoallowalsotwowritesbutwe'dhavetobounce-*thedataintoasinglebuffer.-*/-if((msgs[0].flags&I2C_M_RD)||!(msgs[1].flags&I2C_M_RD))-return-EOPNOTSUPP;-if(msgs[0].len>4)-return-EOPNOTSUPP;-if(msgs[0].addr!=msgs[1].addr)-return-EOPNOTSUPP;req.type=OPAL_I2C_SM_READ;req.addr=cpu_to_be16(msgs[0].addr);req.subaddr_sz=msgs[0].len;
@@ -210,6 +199,16 @@ static const struct i2c_algorithm i2c_opal_algo = {.functionality=i2c_opal_func,};+/* For two messages, we basically support only simple+*smbustransactionsofawriteplusaread.Wemight+*wanttoallowalsotwowritesbutwe'dhavetobounce+*thedataintoasinglebuffer.+*/+staticstructi2c_adapter_quirksi2c_opal_quirks={+.flags=I2C_AQ_COMB_WRITE_THEN_READ,+.max_comb_1st_msg_len=4,+};+staticinti2c_opal_probe(structplatform_device*pdev){structi2c_adapter*adapter;
@@ -232,6 +231,7 @@ static int i2c_opal_probe(struct platform_device *pdev)adapter->algo=&i2c_opal_algo;adapter->algo_data=(void*)(unsignedlong)opal_id;+adapter->quirks=&i2c_opal_quirks;adapter->dev.parent=&pdev->dev;adapter->dev.of_node=of_node_get(pdev->dev.of_node);pname=of_get_property(pdev->dev.of_node,"ibm,port-name",NULL);
On 03/11/2015 04:42 AM, Benjamin Herrenschmidt wrote:
On Tue, 2015-03-10 at 22:43 +0530, Neelesh Gupta wrote:
quoted
I tested the i2c opal driver after updating the patch as below.
Basically I think we can also support write-then-{read/write}
for the number of messages = 2.
Ben, any issues if we support both write plus read/write in the
opal driver ?
Nope, in fact it's a good idea, I found myself having to expoes such
an interface to some userspace tool of ours.
However...
@@ -104,18 +104,8 @@ static int i2c_opal_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,req.buffer_ra=cpu_to_be64(__pa(msgs[0].buf));break;case2:-/* For two messages, we basically support only simple-*smbustransactionsofawriteplusaread.Wemight-*wanttoallowalsotwowritesbutwe'dhavetobounce-*thedataintoasinglebuffer.-*/-if((msgs[0].flags&I2C_M_RD)||!(msgs[1].flags&I2C_M_RD))-return-EOPNOTSUPP;
Don't we still want to enforce that the first message is a write ?
Somebody may not be looking at the quirks...
quoted
- if (msgs[0].len > 4)
- return -EOPNOTSUPP;
And that the len is supported...
quoted
- if (msgs[0].addr != msgs[1].addr)
- return -EOPNOTSUPP;
Same...
Ie, the quirk indicates to the callers what we support, but we should
still check that we aren't called with something that doesn't match.
Quirk *also* return error to the user if any of the conditions mismatch with
what we have indicated through the quriks structure...
I think we can't land up here by-passing the check for quirks so above
checks
are duplicated here..
Neelesh.
quoted
- req.type = OPAL_I2C_SM_READ;
+ req.type = (msgs[1].flags & I2C_M_RD) ?
+ OPAL_I2C_SM_READ : OPAL_I2C_SM_WRITE;
req.addr = cpu_to_be16(msgs[0].addr);
req.subaddr_sz = msgs[0].len;
for (i = 0; i < msgs[0].len; i++)
@@ -104,17 +104,6 @@ static int i2c_opal_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,req.buffer_ra=cpu_to_be64(__pa(msgs[0].buf));break;case2:-/* For two messages, we basically support only simple-*smbustransactionsofawriteplusaread.Wemight-*wanttoallowalsotwowritesbutwe'dhavetobounce-*thedataintoasinglebuffer.-*/-if((msgs[0].flags&I2C_M_RD)||!(msgs[1].flags&I2C_M_RD))-return-EOPNOTSUPP;-if(msgs[0].len>4)-return-EOPNOTSUPP;-if(msgs[0].addr!=msgs[1].addr)-return-EOPNOTSUPP;req.type=OPAL_I2C_SM_READ;req.addr=cpu_to_be16(msgs[0].addr);req.subaddr_sz=msgs[0].len;
@@ -210,6 +199,16 @@ static const struct i2c_algorithm i2c_opal_algo = {.functionality=i2c_opal_func,};+/* For two messages, we basically support only simple+*smbustransactionsofawriteplusaread.Wemight+*wanttoallowalsotwowritesbutwe'dhavetobounce+*thedataintoasinglebuffer.+*/+staticstructi2c_adapter_quirksi2c_opal_quirks={+.flags=I2C_AQ_COMB_WRITE_THEN_READ,+.max_comb_1st_msg_len=4,+};+staticinti2c_opal_probe(structplatform_device*pdev){structi2c_adapter*adapter;
@@ -232,6 +231,7 @@ static int i2c_opal_probe(struct platform_device *pdev)adapter->algo=&i2c_opal_algo;adapter->algo_data=(void*)(unsignedlong)opal_id;+adapter->quirks=&i2c_opal_quirks;adapter->dev.parent=&pdev->dev;adapter->dev.of_node=of_node_get(pdev->dev.of_node);pname=of_get_property(pdev->dev.of_node,"ibm,port-name",NULL);
From: Wolfram Sang <hidden> Date: 2015-03-12 14:50:50
You can add my
Acked-by and Tested-By: Ludovic Desroches [off-list ref]
Tested on sama5d3, some problems with at24 eeprom on sama5d4 but it
doesn't come from the i2c quirks patch series.
Thanks for testing! Are the eeprom problems something which needs fixing
upstream?
From: Wolfram Sang <hidden> Date: 2015-03-12 14:55:05
I think we can't land up here by-passing the check for quirks so above
checks are duplicated here..
True.
So, as Ben seems OK with write-then-anything, can you send me your
changes as an incremental patch to mine with your Signed-off, please?
Thanks,
Wolfram
From: Wolfram Sang <hidden> Date: 2015-03-12 14:55:57
Change on the iproc i2c driver looks good to me. Sanity tested the
change from Wolfram's i2c/quirks branch on Cygnus 958300K combo board.
Sanity tested with an attempt to transfer large amount of I2C data to
ensure the transfer is denied by the i2c-core:
/ # cat /dev/i2c-0
[ 657.310261] i2c i2c-0: quirk: msg too long (addr 0x0000, size 4096, read)
Reviewed-by: Ray Jui <rjui@broadcom.com>
Tested-by: Ray Jui <rjui@broadcom.com>
Thanks for testing, and especially describing your test! Much
appreciated.
Hi Wolfram,
Based on your patch:
"[RFC V2 04/12] i2c: opal: make use of the new infrastructure for quirks"
From: Neelesh Gupta <redacted>
Subject: [PATCH] i2c: opal: Update quirk flags to do write-then-anything
Support write-then-anything in the case of 2 i2c messages
for i2c transfer.
Signed-off-by: Neelesh Gupta<redacted>
---
drivers/i2c/busses/i2c-opal.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
@@ -199,13 +200,11 @@ static const struct i2c_algorithm i2c_opal_algo = {.functionality=i2c_opal_func,};-/* For two messages, we basically support only simple-*smbustransactionsofawriteplusaread.Wemight-*wanttoallowalsotwowritesbutwe'dhavetobounce-*thedataintoasinglebuffer.+/* For two messages, we basically support simple smbus transactions of a+*write-then-anything.*/staticstructi2c_adapter_quirksi2c_opal_quirks={-.flags=I2C_AQ_COMB_WRITE_THEN_READ,+.flags=I2C_AQ_COMB|I2C_AQ_COMB_WRITE_FIRST|I2C_AQ_COMB_SAME_ADDR,.max_comb_1st_msg_len=4,};
On 02/25/2015 09:31 PM, Wolfram Sang wrote:
quoted hunk
From: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
drivers/i2c/busses/i2c-opal.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
@@ -104,17 +104,6 @@ static int i2c_opal_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,req.buffer_ra=cpu_to_be64(__pa(msgs[0].buf));break;case2:-/* For two messages, we basically support only simple-*smbustransactionsofawriteplusaread.Wemight-*wanttoallowalsotwowritesbutwe'dhavetobounce-*thedataintoasinglebuffer.-*/-if((msgs[0].flags&I2C_M_RD)||!(msgs[1].flags&I2C_M_RD))-return-EOPNOTSUPP;-if(msgs[0].len>4)-return-EOPNOTSUPP;-if(msgs[0].addr!=msgs[1].addr)-return-EOPNOTSUPP;req.type=OPAL_I2C_SM_READ;req.addr=cpu_to_be16(msgs[0].addr);req.subaddr_sz=msgs[0].len;
@@ -210,6 +199,16 @@ static const struct i2c_algorithm i2c_opal_algo = {.functionality=i2c_opal_func,};+/* For two messages, we basically support only simple+*smbustransactionsofawriteplusaread.Wemight+*wanttoallowalsotwowritesbutwe'dhavetobounce+*thedataintoasinglebuffer.+*/+staticstructi2c_adapter_quirksi2c_opal_quirks={+.flags=I2C_AQ_COMB_WRITE_THEN_READ,+.max_comb_1st_msg_len=4,+};+staticinti2c_opal_probe(structplatform_device*pdev){structi2c_adapter*adapter;
@@ -232,6 +231,7 @@ static int i2c_opal_probe(struct platform_device *pdev)adapter->algo=&i2c_opal_algo;adapter->algo_data=(void*)(unsignedlong)opal_id;+adapter->quirks=&i2c_opal_quirks;adapter->dev.parent=&pdev->dev;adapter->dev.of_node=of_node_get(pdev->dev.of_node);pname=of_get_property(pdev->dev.of_node,"ibm,port-name",NULL);
From: Wolfram Sang <hidden> Date: 2015-03-14 11:14:49
On Wed, Feb 25, 2015 at 05:01:51PM +0100, Wolfram Sang wrote:
From: Wolfram Sang <wsa+renesas@sang-engineering.com>
Here is the second version of the patch series to describe i2c adapter quirks
in a generic way. For the motivation, please read description of patch 1. This
is still RFC because I would like to do some more tests on my own, but I need
to write a tool for that. However, I'd really like to have the driver authors
to have a look already. Actual testing is very much appreciated. Thanks to the
Mediatek guys for rebasing their new driver to this framework. That helps, too!
Thanks for all the testing. Merged this branch now into for-next!