Hi, Thank you for this submission! Wow, this is a lot of code and I have only very minor comments. Good work and thanks to all previous reviewers! > @@ -0,0 +1,1415 @@ > +/* > + * I2C adapter for the IMG Serial Control Bus (SCB) IP block. > + * > + * Copyright (C) 2009, 2010, 2012, 2014 Imagination Technologies Ltd. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * There are three ways that this I2C controller can be driven: Interesting beast. Offers a lot of possibilities and complexity :) > + * Notice that the driver implements a timer-based timeout mechanism. > + * This is done to avoid slave events interrupts in automatic mode, > + * given the driver gets a slave event and transaction done interrupts > + * for each atomic mode command (start, data, ack, stop, etc) that gets > + * completed, none of which are of interest when using automatic mode > + * since those atomic mode commands are managed automatically by hardware > + * rather than by the I2C state machine in the interrupt handler. I read this three times and still have no clear picture. Please rephrase. Smaller sentences. Simple ones. ... all down to probe() > + i2c_set_adapdata(&i2c->adap, i2c); > + i2c->adap.dev.parent = &pdev->dev; > + i2c->adap.dev.of_node = node; > + i2c->adap.owner = THIS_MODULE; > + i2c->adap.class = I2C_CLASS_HWMON | I2C_CLASS_SPD; You probably won't need this one. You have DT probing. > + i2c->adap.algo = &img_i2c_algo; > + i2c->adap.retries = 5; > + i2c->adap.nr = pdev->id; > + snprintf(i2c->adap.name, sizeof(i2c->adap.name), > + "IMG i2c%d", i2c->adap.nr); Please use a static name, like "IMG <some type> I2C". This describes the IP block, not the bus in use. Very close to go... Thanks, Wolfram
Attachment:
signature.asc
Description: Digital signature
