Re: [PATCH v6 1/2] i2c: Add Imagination Technologies I2C SCB driver

[Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 1895 bytes --]

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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]