[I seem to have lost my local copy of the mail I'm responding to, so I copied bits of it from an archive and broke threading in the process, sorry about that] On 2018-09-11 00:22, Ajay Gupta wrote: >> Hmm, that goto stop is however not perfect. Ideally, >> you shouldn't issue stop if i == 0 and gpu_i2c_read failed > How can there be a read without rab write first? > AFAIU, gpu_i2c_read() can get called only with i=1 onward. Well, you said that there could be other I2C devices on this I2C bus. I thought that meant that there could be other I2C client drivers using this I2C adapter. If so, then those drivers can issue I2C transfers where the first message is a read. >> > +static int gpu_populate_client(struct gpu_i2c_dev *i2cd, int irq) { >> > + static struct i2c_board_info gpu_ccgx_ucsi = { >> > + I2C_BOARD_INFO("ccgx-ucsi", 0x8), >> > + }; >> >> Shouldn't this struct be dynamically allocated and attached to struct >> gpu_i2c_dev so that you (maybe) could have more than one instance? > Currently the structure is local variable. Will that not work in multi > instance cases? Arrggh. NO! A static variable in function scope behaves very much the same as a static variable in file scope. The only difference is that the visibility is different. All calls to the function gets the same gpu_ccgx_ucsi instance which means that instances will clobber each other. You need to add a struct i2c_board_info (or a pointer to one) to struct gpu_i2c_dev so that it can be dynamically allocated. Cheers, Peter