Re: [PATCH v2 01/10] i2c-octeon: Cleanup i2c-octeon driver

[Wolfram Sang <wsa@the-dreams.de>]

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

Hi Jan,

The description is not enough. A list what kind of changes you applied
would be nice.

I'd like to have these checkpatch issues fixed:

ERROR: trailing statements should be on next line
#177: FILE: drivers/i2c/busses/i2c-octeon.c:133:
+	while ((tmp & SW_TWSI_V) != 0);

ERROR: trailing statements should be on next line
#202: FILE: drivers/i2c/busses/i2c-octeon.c:152:
+	while ((tmp & SW_TWSI_V) != 0);

CHECK: Prefer using the BIT_ULL macro
#52: FILE: drivers/i2c/busses/i2c-octeon.c:39:
+#define SW_TWSI_V		(1ULL << 63)

Note: I don't care so much about the 80 char limit as long as the line
length is not too excessive.


> -#define DRV_NAME "i2c-octeon"
> +#define DRV_NAME		"i2c-octeon"

I'm in favor for indenting register and bit defines, but other than that
I think one space is enough. I won't force my opinion on you, though.
Just wanted to let you know.

> +#define INT_ENA_ST		0x1
> +#define INT_ENA_TS		0x2
> +#define INT_ENA_CORE		0x4

I assume those are bits? Then, they shouldn't be in the registers
section.

> +/* TWSI_INT values */
> +#define ST_INT			0x01
> +#define TS_INT			0x02
> +#define CORE_INT		0x04
> +#define ST_EN			0x10
> +#define TS_EN			0x20
> +#define CORE_EN			0x40
> +#define SDA_OVR			0x100
> +#define SCL_OVR			0x200
> +#define SDA			0x400
> +#define SCL			0x800

I think those should have a prefix. SDA and SCL are dangerously generic.

>  struct octeon_i2c {
> -	wait_queue_head_t queue;
> -	struct i2c_adapter adap;
> -	int irq;
> -	u32 twsi_freq;
> -	int sys_freq;
> -	resource_size_t twsi_phys;
> -	void __iomem *twsi_base;
> -	resource_size_t regsize;
> -	struct device *dev;
> +	wait_queue_head_t	queue;
> +	struct i2c_adapter	adap;
> +	int			irq;
> +	u32			twsi_freq;
> +	int			sys_freq;
> +	void __iomem		*twsi_base;
> +	struct device		*dev;

NAK. structs change often, and then one needs to fix the whole
indentation. One space is enough here.

>  };

> -static u8 octeon_i2c_read_sw(struct octeon_i2c *i2c, u64 eop_reg)
> +static u64 octeon_i2c_read_sw64(struct octeon_i2c *i2c, u64 eop_reg)
...
> -	return tmp & 0xFF;
> +	return tmp;
> +}
...
> +
> +/**
> + * octeon_i2c_read_sw - read lower bits of an I2C core register
> + * @i2c: The struct octeon_i2c
> + * @eop_reg: Register selector
> + *
> + * Returns the data.
> + *
> + * The I2C core registers are accessed indirectly via the SW_TWSI CSR.
> + */
> +static u8 octeon_i2c_read_sw(struct octeon_i2c *i2c, u64 eop_reg)
> +{
> +	return octeon_i2c_read_sw64(i2c, eop_reg);
>  }

This is not a cleanup!

> +/* disable the CORE interrupt */
>  static void octeon_i2c_int_disable(struct octeon_i2c *i2c)
>  {
> -	octeon_i2c_write_int(i2c, 0);
> +	/* clear TS/ST/IFLG events */
> +	octeon_i2c_write_int(i2c, TS_INT | ST_INT);
>  }

Isn't this a functional change?

>  /**
> - * octeon_i2c_unblock - unblock the bus.
> - * @i2c: The struct octeon_i2c.
> + * bitbang_unblock - unblock the bus
> + * @i2c: The struct octeon_i2c
>   *
> - * If there was a reset while a device was driving 0 to bus,
> - * bus is blocked. We toggle it free manually by some clock
> - * cycles and send a stop.
> + * If there was a reset while a device was driving 0 to bus, bus is blocked.
> + * We toggle it free manually by some clock cycles and send a stop.
>   */
> -static void octeon_i2c_unblock(struct octeon_i2c *i2c)
> +static void bitbang_unblock(struct octeon_i2c *i2c)

I dunno understand the change of the function name. However, this should
be refactored to use the i2c_bus_recovery infrastructure anyhow.

> -	result = wait_event_timeout(i2c->queue,
> -					octeon_i2c_test_iflg(i2c),
> -					i2c->adap.timeout);
> -
> +	result = wait_event_timeout(i2c->queue, octeon_i2c_test_iflg(i2c),
> +				    i2c->adap.timeout);

You could rename this variable to 'time_left' to make the code even
easier to read.

>  static int octeon_i2c_write(struct octeon_i2c *i2c, int target,
> -			    const u8 *data, int length)
> +			    u8 *data, int length)

Why this change?

>  {
> -	int i, result;
> +	int result, i;

And this?

> -static int octeon_i2c_read(struct octeon_i2c *i2c, int target,
> -			   u8 *data, int length)
> +static int octeon_i2c_read(struct octeon_i2c *i2c, int target, u8 *data,
> +			   u16 *rlength)
>  {
> +	int length = *rlength;

And this?

> @@ -353,15 +411,14 @@ static int octeon_i2c_read(struct octeon_i2c *i2c, int target,
>  	if (result)
>  		return result;
>  
> -	octeon_i2c_write_sw(i2c, SW_TWSI_EOP_TWSI_DATA, (target<<1) | 1);
> -	octeon_i2c_write_sw(i2c, SW_TWSI_EOP_TWSI_CTL, TWSI_CTL_ENAB);
> -
> +	octeon_i2c_write_sw(i2c, SW_TWSI_EOP_TWSI_DATA, TWSI_CTL_ENAB);

Is this really the same?

>  static u32 octeon_i2c_functionality(struct i2c_adapter *adap)
> @@ -435,13 +490,10 @@ static struct i2c_adapter octeon_i2c_ops = {
>  	.owner = THIS_MODULE,
>  	.name = "OCTEON adapter",
>  	.algo = &octeon_i2c_algo,
> -	.timeout = HZ / 50,

This is a functional change, or?

> -	i2c->twsi_phys = res_mem->start;
> -	i2c->regsize = resource_size(res_mem);

Those are removed which is okay in general, but should be in a seperate
patch.

This patch was hard to review because so many changes were overlapping.
It really  should have been broken out. Like one patch only removing the
trailing "." in the kernel-doc. One fixing the indentation issues. One
removing the now superfluous fields in struct octeon_i2c, etc...

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]