[PATCH v7 3/5] drivers/i2c/busses/i2c-at91.c: add new driver

[PATCH v7 3/5] drivers/i2c/busses/i2c-at91.c: add new driver

[Arnd Bergmann]

On Tuesday 08 November 2011, Nikolaus Voss wrote:


> +
> +static unsigned at91_twi_read(struct at91_twi_dev *dev, unsigned reg)
> +{
> +	return __raw_readl(dev->base + reg);
> +}
> +
> +static void at91_twi_write(struct at91_twi_dev *dev, unsigned reg, unsigned val)
> +{
> +	__raw_writel(val, dev->base + reg);
> +}

Better use readl/writel or readl_relaxed/writel_relaxed.

> +/*
> + * Calculate and set symmetric clock as stated in datasheet:
> + * twi_clk = F_MAIN / (2 * cdiv * (1 << ckdiv) + 4)
> + */
> +static void __devinit at91_set_twi_clock(struct at91_twi_dev *dev, int twi_clk)
> +{
> +	const int div = DIV_ROUND_UP(clk_get_rate(dev->clk), 2 * twi_clk) - 2;
> +	int ckdiv = fls(div >> 8);
> +	const int cdiv = div >> ckdiv;
> +
> +	if (cpu_is_at91rm9200() && (ckdiv > 5)) {
> +		dev_warn(dev->dev, "AT91RM9200 erratum 22: using ckdiv = 5.\n");
> +		ckdiv = 5;
> +	}

Don't check a global property like this locally in the driver. Instead,
make it a property of the device that you check here and set it based on
the the device name set by the platform, or by a device tree property.

> diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
> new file mode 100644
> index 0000000..a898159
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-at91.h
> @@ -0,0 +1,80 @@
> +
> +#ifndef AT91_TWI_H
> +#define AT91_TWI_H
> +

No need for a header file if all the values in it are used in only one
source file. Just move everything to i2c_at91.c

> +#define	AT91_TWI_MMR		0x04		/* Master Mode Register */
> +#define	AT91_TWI_IADRSZ		(3    <<  8)	/* Internal Device Address
> +						 *  Size */
> +#define	AT91_TWI_IADRSZ_NO	(0 << 8)
> +#define	AT91_TWI_IADRSZ_1	(1 << 8)
> +#define	AT91_TWI_IADRSZ_2	(2 << 8)
> +#define	AT91_TWI_IADRSZ_3	(3 << 8)
> +#define	AT91_TWI_MREAD		(1    << 12)	/* Master Read Direction */
> +#define	AT91_TWI_DADR		(0x7f << 16)	/* Device Address */

These are harder to read than just using hexadecimal bitmasks:

#define	AT91_TWI_MMR		0x00000004
#define	AT91_TWI_IADRSZ		0x00000300
#define	AT91_TWI_IADRSZ_NO	0x00000000
#define	AT91_TWI_IADRSZ_1	0x00000100
...

	Arnd

[PATCH v7 3/5] drivers/i2c/busses/i2c-at91.c: add new driver

[Voss, Nikolaus]

Hi,

Arnd Bergmann wrote on 2011-11-23:
> On Tuesday 08 November 2011, Nikolaus Voss wrote:
> 
> 
> > +
> > +static unsigned at91_twi_read(struct at91_twi_dev *dev, unsigned reg)
> > +{
> > +	return __raw_readl(dev->base + reg);
> > +}
> > +
> > +static void at91_twi_write(struct at91_twi_dev *dev, unsigned reg, unsigned
> val)
> > +{
> > +	__raw_writel(val, dev->base + reg);
> > +}
> 
> Better use readl/writel or readl_relaxed/writel_relaxed.

ok, changed.

> 
> > +/*
> > + * Calculate and set symmetric clock as stated in datasheet:
> > + * twi_clk = F_MAIN / (2 * cdiv * (1 << ckdiv) + 4)
> > + */
> > +static void __devinit at91_set_twi_clock(struct at91_twi_dev *dev, int
> twi_clk)
> > +{
> > +	const int div = DIV_ROUND_UP(clk_get_rate(dev->clk), 2 * twi_clk) - 2;
> > +	int ckdiv = fls(div >> 8);
> > +	const int cdiv = div >> ckdiv;
> > +
> > +	if (cpu_is_at91rm9200() && (ckdiv > 5)) {
> > +		dev_warn(dev->dev, "AT91RM9200 erratum 22: using ckdiv = 5.\n");
> > +		ckdiv = 5;
> > +	}
> 
> Don't check a global property like this locally in the driver. Instead,
> make it a property of the device that you check here and set it based on
> the the device name set by the platform, or by a device tree property.

Yes, this is a known problem and was discussed in
https://lkml.org/lkml/2011/11/8/217 . The main problem is that there a no
revisions for specific at91 IP devices but the revision is implicitly
defined by the cpu type and version. Changing this would need to add some
arch infrastructure which I think is beyond the scope of this driver.

> > diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
> > new file mode 100644
> > index 0000000..a898159
> > --- /dev/null
> > +++ b/drivers/i2c/busses/i2c-at91.h
> > @@ -0,0 +1,80 @@
> > +
> > +#ifndef AT91_TWI_H
> > +#define AT91_TWI_H
> > +
> 
> No need for a header file if all the values in it are used in only one
> source file. Just move everything to i2c_at91.c
> 
> > +#define	AT91_TWI_MMR		0x04		/* Master Mode Register */
> > +#define	AT91_TWI_IADRSZ		(3    <<  8)	/* Internal Device Address
> > +						 *  Size */
> > +#define	AT91_TWI_IADRSZ_NO	(0 << 8)
> > +#define	AT91_TWI_IADRSZ_1	(1 << 8)
> > +#define	AT91_TWI_IADRSZ_2	(2 << 8)
> > +#define	AT91_TWI_IADRSZ_3	(3 << 8)
> > +#define	AT91_TWI_MREAD		(1    << 12)	/* Master Read Direction
> */
> > +#define	AT91_TWI_DADR		(0x7f << 16)	/* Device Address */
> 
> These are harder to read than just using hexadecimal bitmasks:
> 
> #define	AT91_TWI_MMR		0x00000004
> #define	AT91_TWI_IADRSZ		0x00000300
> #define	AT91_TWI_IADRSZ_NO	0x00000000
> #define	AT91_TWI_IADRSZ_1	0x00000100
> ...

I agree, but this header file was already used by the old driver and
converting would add possible errors to register definitions which are
not (yet) used. That's why I've left it as is and just made it a local
include.

Thanks,
Niko

[PATCH v7 3/5] drivers/i2c/busses/i2c-at91.c: add new driver

[Arnd Bergmann]

On Thursday 24 November 2011, Voss, Nikolaus wrote:
> > 
> > > +/*
> > > + * Calculate and set symmetric clock as stated in datasheet:
> > > + * twi_clk = F_MAIN / (2 * cdiv * (1 << ckdiv) + 4)
> > > + */
> > > +static void __devinit at91_set_twi_clock(struct at91_twi_dev *dev, int
> > twi_clk)
> > > +{
> > > +   const int div = DIV_ROUND_UP(clk_get_rate(dev->clk), 2 * twi_clk) - 2;
> > > +   int ckdiv = fls(div >> 8);
> > > +   const int cdiv = div >> ckdiv;
> > > +
> > > +   if (cpu_is_at91rm9200() && (ckdiv > 5)) {
> > > +           dev_warn(dev->dev, "AT91RM9200 erratum 22: using ckdiv = 5.\n");
> > > +           ckdiv = 5;
> > > +   }
> > 
> > Don't check a global property like this locally in the driver. Instead,
> > make it a property of the device that you check here and set it based on
> > the the device name set by the platform, or by a device tree property.
> 
> Yes, this is a known problem and was discussed in
> https://lkml.org/lkml/2011/11/8/217 . The main problem is that there a no
> revisions for specific at91 IP devices but the revision is implicitly
> defined by the cpu type and version. Changing this would need to add some
> arch infrastructure which I think is beyond the scope of this driver.

Aside from the flamewar in that thread, my impression is that in general
people (me certainly) prefer to have driver-local workarounds be expressed
in a driver specific way, not in a platform or architecture specific way
because that makes the driver less portable.

Anyway, I don't see this point as a show-stopper since the driver is already
platform specific, but it's generally a good idea to write code like this
defensively, if only to avoid getting comments about it.

> > > diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
> > > new file mode 100644
> > > index 0000000..a898159
> > > --- /dev/null
> > > +++ b/drivers/i2c/busses/i2c-at91.h
> > > @@ -0,0 +1,80 @@
> > > +
> > > +#ifndef AT91_TWI_H
> > > +#define AT91_TWI_H
> > > +
> > 
> > No need for a header file if all the values in it are used in only one
> > source file. Just move everything to i2c_at91.c
> > 
> > > +#define    AT91_TWI_MMR            0x04            /* Master Mode Register */
> > > +#define    AT91_TWI_IADRSZ         (3    <<  8)    /* Internal Device Address
> > > +                                            *  Size */
> > > +#define    AT91_TWI_IADRSZ_NO      (0 << 8)
> > > +#define    AT91_TWI_IADRSZ_1       (1 << 8)
> > > +#define    AT91_TWI_IADRSZ_2       (2 << 8)
> > > +#define    AT91_TWI_IADRSZ_3       (3 << 8)
> > > +#define    AT91_TWI_MREAD          (1    << 12)    /* Master Read Direction
> > */
> > > +#define    AT91_TWI_DADR           (0x7f << 16)    /* Device Address */
> > 
> > These are harder to read than just using hexadecimal bitmasks:
> > 
> > #define       AT91_TWI_MMR            0x00000004
> > #define       AT91_TWI_IADRSZ         0x00000300
> > #define       AT91_TWI_IADRSZ_NO      0x00000000
> > #define       AT91_TWI_IADRSZ_1       0x00000100
> > ...
> 
> I agree, but this header file was already used by the old driver and
> converting would add possible errors to register definitions which are
> not (yet) used. That's why I've left it as is and just made it a local
> include.

But you are presenting the driver as a new one, so you should be
prepared to get review comments like any other new code.

Please at least move the data into the main driver file to get rid of
the header file.

	Arnd

[PATCH v7 3/5] drivers/i2c/busses/i2c-at91.c: add new driver

[Voss, Nikolaus]

Hi,

Arnd Bergmann wrote on 2011-11-24:
> On Thursday 24 November 2011, Voss, Nikolaus wrote:
> > >
> > > > +/*
> > > > + * Calculate and set symmetric clock as stated in datasheet:
> > > > + * twi_clk = F_MAIN / (2 * cdiv * (1 << ckdiv) + 4)
> > > > + */
> > > > +static void __devinit at91_set_twi_clock(struct at91_twi_dev *dev, int
> > > twi_clk)
> > > > +{
> > > > +   const int div = DIV_ROUND_UP(clk_get_rate(dev->clk), 2 * twi_clk) -
> 2;
> > > > +   int ckdiv = fls(div >> 8);
> > > > +   const int cdiv = div >> ckdiv;
> > > > +
> > > > +   if (cpu_is_at91rm9200() && (ckdiv > 5)) {
> > > > +           dev_warn(dev->dev, "AT91RM9200 erratum 22: using ckdiv =
> 5.\n");
> > > > +           ckdiv = 5;
> > > > +   }
> > >
> > > Don't check a global property like this locally in the driver. Instead,
> > > make it a property of the device that you check here and set it based on
> > > the the device name set by the platform, or by a device tree property.
> >
> > Yes, this is a known problem and was discussed in
> > https://lkml.org/lkml/2011/11/8/217 . The main problem is that there a no
> > revisions for specific at91 IP devices but the revision is implicitly
> > defined by the cpu type and version. Changing this would need to add some
> > arch infrastructure which I think is beyond the scope of this driver.
> 
> Aside from the flamewar in that thread, my impression is that in general
> people (me certainly) prefer to have driver-local workarounds be expressed
> in a driver specific way, not in a platform or architecture specific way
> because that makes the driver less portable.

I guess I see your point now. So you want something like pdev.has_bugX to be
set by mach setup and later check this flag in the driver?

> > > > diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-
> at91.h
> > > > new file mode 100644
> > > > index 0000000..a898159
> > > > --- /dev/null
> > > > +++ b/drivers/i2c/busses/i2c-at91.h
> > > > @@ -0,0 +1,80 @@
> > > > +
> > > > +#ifndef AT91_TWI_H
> > > > +#define AT91_TWI_H
> > > > +
> > >
> > > No need for a header file if all the values in it are used in only one
> > > source file. Just move everything to i2c_at91.c
> > >
> > > > +#define    AT91_TWI_MMR            0x04            /* Master Mode
> Register */
> > > > +#define    AT91_TWI_IADRSZ         (3    <<  8)    /* Internal Device
> Address
> > > > +                                            *  Size */
> > > > +#define    AT91_TWI_IADRSZ_NO      (0 << 8)
> > > > +#define    AT91_TWI_IADRSZ_1       (1 << 8)
> > > > +#define    AT91_TWI_IADRSZ_2       (2 << 8)
> > > > +#define    AT91_TWI_IADRSZ_3       (3 << 8)
> > > > +#define    AT91_TWI_MREAD          (1    << 12)    /* Master Read
> Direction
> > > */
> > > > +#define    AT91_TWI_DADR           (0x7f << 16)    /* Device Address */
> > >
> > > These are harder to read than just using hexadecimal bitmasks:
> > >
> > > #define       AT91_TWI_MMR            0x00000004
> > > #define       AT91_TWI_IADRSZ         0x00000300
> > > #define       AT91_TWI_IADRSZ_NO      0x00000000
> > > #define       AT91_TWI_IADRSZ_1       0x00000100
> > > ...
> >
> > I agree, but this header file was already used by the old driver and
> > converting would add possible errors to register definitions which are
> > not (yet) used. That's why I've left it as is and just made it a local
> > include.
> 
> But you are presenting the driver as a new one, so you should be
> prepared to get review comments like any other new code.
> 
> Please at least move the data into the main driver file to get rid of
> the header file.

I didn't want to appear ignorant about this, I actually appreciate your
comment. I just wanted to point out that there might be a reason to keep
the old file which you weren't aware of (because I presented this as a new
driver). So, I will move the register definitions to the main driver.

Thanks,
Niko

[PATCH v7 3/5] drivers/i2c/busses/i2c-at91.c: add new driver

[Arnd Bergmann]

On Thursday 24 November 2011, Voss, Nikolaus wrote:
> > Aside from the flamewar in that thread, my impression is that in general
> > people (me certainly) prefer to have driver-local workarounds be expressed
> > in a driver specific way, not in a platform or architecture specific way
> > because that makes the driver less portable.
> 
> I guess I see your point now. So you want something like pdev.has_bugX to be
> set by mach setup and later check this flag in the driver?

Yes, that would be the idea. I would not introduce platform_data for one
driver just for this though, because we are generally moving away from
platform_data towards device tree probing.

You can do it with a match table that has two entries with different
names for the two kinds of device that you need to distinguish and
set the platform_device_id->driver_data field to '1' for the type
that needs the fixup.

> > > > #define       AT91_TWI_MMR            0x00000004
> > > > #define       AT91_TWI_IADRSZ         0x00000300
> > > > #define       AT91_TWI_IADRSZ_NO      0x00000000
> > > > #define       AT91_TWI_IADRSZ_1       0x00000100
> > > > ...
> > >
> > > I agree, but this header file was already used by the old driver and
> > > converting would add possible errors to register definitions which are
> > > not (yet) used. That's why I've left it as is and just made it a local
> > > include.
> > 
> > But you are presenting the driver as a new one, so you should be
> > prepared to get review comments like any other new code.
> > 
> > Please at least move the data into the main driver file to get rid of
> > the header file.
> 
> I didn't want to appear ignorant about this, I actually appreciate your
> comment. I just wanted to point out that there might be a reason to keep
> the old file which you weren't aware of (because I presented this as a new
> driver). So, I will move the register definitions to the main driver.

Ok, good. Moving it into the driver is really the important part anyway,
and I understand your reasoning for not wanting to modify the definitions,
it just didn't apply to the one of the two comments I made about the header.

	Arnd

[PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c

[Ben Dooks]

On Wed, Nov 23, 2011 at 04:35:55PM +0100, Nikolaus Voss wrote:
> The old driver has two main deficencies:
> i)  No repeated start (Sr) condiction is possible, this makes it unusable
>     e.g. for most SMBus transfers.
> ii) I/O was done with polling/busy waiting what caused over-/underruns
>     even at light system loads and clock speeds.
> 
> The new driver overcomes these deficencies and in addition allows for
> more than one TWI interface.
> 
> A remaining limitation is the fact, that only one repeated start is
> possible (two concatenated messages). This limitation is imposed by
> the hardware. However, this should not be a problem as all common
> i2c-client communication does not rely on more than one repeated start.
> 
> v7: Patch 4/5: i)  fix bug if internal address > 1 byte
>                ii) send stop when len == 1
>                    (both reported by Carsten Behling)
> v6: Patch 5/5: support for I2C_SMBUS_BLOCK_DATA transfers.
>     Better use of clk_(un)prepare().
>     More sensible transfer timeout.
> v5: Another round of review comments from Ryan Mallon, Felipe Balbi
>     and Russell King: convert twi clk to use .dev_id, cleanups
> v4: Integrated more review comments from Ryan Mallon and Felipe Balbi:
>     Moved register include file to local include, code cleanups
> v3: Integrated review comments from Ryan Mallon and Felipe Balbi
> v2: Fixed whitespace issue
> 
> Nikolaus Voss (5):
>   drivers/i2c/busses/i2c-at91.c: remove broken driver
>   Replace clk_lookup.con_id with clk_lookup.dev_id entries for twi clk
>   drivers/i2c/busses/i2c-at91.c: add new driver
>   G45 TWI: remove open drain setting for twi function gpios
>   i2c-at91.c: add SMBUS_READ_BLOCK_DATA functionality

Is the original driver so broken that the two could not co-exist, or are
we making so many changes that there's no point in keeping the original
one?
 
>  arch/arm/mach-at91/at91cap9.c              |    1 +
>  arch/arm/mach-at91/at91rm9200.c            |    1 +
>  arch/arm/mach-at91/at91sam9260.c           |    1 +
>  arch/arm/mach-at91/at91sam9261.c           |    1 +
>  arch/arm/mach-at91/at91sam9263.c           |    1 +
>  arch/arm/mach-at91/at91sam9g45.c           |    2 +
>  arch/arm/mach-at91/at91sam9g45_devices.c   |    6 -
>  arch/arm/mach-at91/at91sam9rl.c            |    2 +
>  arch/arm/mach-at91/include/mach/at91_twi.h |   68 ----
>  drivers/i2c/busses/Kconfig                 |   11 +-
>  drivers/i2c/busses/i2c-at91.c              |  529 +++++++++++++++++-----------
>  drivers/i2c/busses/i2c-at91.h              |   80 +++++
>  12 files changed, 408 insertions(+), 295 deletions(-)
>  delete mode 100644 arch/arm/mach-at91/include/mach/at91_twi.h
>  create mode 100644 drivers/i2c/busses/i2c-at91.h
> 
> -- 
> 1.7.5.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Ben Dooks, ben at fluff.org, http://www.fluff.org/ben/

Large Hadron Colada: A large Pina Colada that makes the universe disappear.

[PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c

[Voss, Nikolaus]

Hi,

Ben Dooks wrote on 2011-11-24:
> On Wed, Nov 23, 2011 at 04:35:55PM +0100, Nikolaus Voss wrote:
> > The old driver has two main deficencies:
> > i)  No repeated start (Sr) condiction is possible, this makes it unusable
> >     e.g. for most SMBus transfers.
> > ii) I/O was done with polling/busy waiting what caused over-/underruns
> >     even at light system loads and clock speeds.
> >
> > The new driver overcomes these deficencies and in addition allows for
> > more than one TWI interface.
> >
> > A remaining limitation is the fact, that only one repeated start is
> > possible (two concatenated messages). This limitation is imposed by
> > the hardware. However, this should not be a problem as all common
> > i2c-client communication does not rely on more than one repeated start.
> >
> > v7: Patch 4/5: i)  fix bug if internal address > 1 byte
> >                ii) send stop when len == 1
> >                    (both reported by Carsten Behling)
> > v6: Patch 5/5: support for I2C_SMBUS_BLOCK_DATA transfers.
> >     Better use of clk_(un)prepare().
> >     More sensible transfer timeout.
> > v5: Another round of review comments from Ryan Mallon, Felipe Balbi
> >     and Russell King: convert twi clk to use .dev_id, cleanups
> > v4: Integrated more review comments from Ryan Mallon and Felipe Balbi:
> >     Moved register include file to local include, code cleanups
> > v3: Integrated review comments from Ryan Mallon and Felipe Balbi
> > v2: Fixed whitespace issue
> >
> > Nikolaus Voss (5):
> >   drivers/i2c/busses/i2c-at91.c: remove broken driver
> >   Replace clk_lookup.con_id with clk_lookup.dev_id entries for twi clk
> >   drivers/i2c/busses/i2c-at91.c: add new driver
> >   G45 TWI: remove open drain setting for twi function gpios
> >   i2c-at91.c: add SMBUS_READ_BLOCK_DATA functionality
> 
> Is the original driver so broken that the two could not co-exist, or are
> we making so many changes that there's no point in keeping the original
> one?

The old driver was marked as broken for the above reasons and I can hardly
imagine any setup in which it would be preferable to i2c-gpio. So it does
not make any sense to keep the old driver alive. Though inspired by the old
driver, the new one is almost a rewrite from scratch, so for better reviewing,
I removed the old instead of doing a diff.

Niko

[PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c

[Ryan Mallon]

On 24/11/11 17:33, Voss, Nikolaus wrote:

> Hi,
> 
> Ben Dooks wrote on 2011-11-24:
>> On Wed, Nov 23, 2011 at 04:35:55PM +0100, Nikolaus Voss wrote:
>>> The old driver has two main deficencies:
>>> i)  No repeated start (Sr) condiction is possible, this makes it unusable
>>>     e.g. for most SMBus transfers.
>>> ii) I/O was done with polling/busy waiting what caused over-/underruns
>>>     even at light system loads and clock speeds.
>>>
>>> The new driver overcomes these deficencies and in addition allows for
>>> more than one TWI interface.
>>>
>>> A remaining limitation is the fact, that only one repeated start is
>>> possible (two concatenated messages). This limitation is imposed by
>>> the hardware. However, this should not be a problem as all common
>>> i2c-client communication does not rely on more than one repeated start.
>>>
>>> v7: Patch 4/5: i)  fix bug if internal address > 1 byte
>>>                ii) send stop when len == 1
>>>                    (both reported by Carsten Behling)
>>> v6: Patch 5/5: support for I2C_SMBUS_BLOCK_DATA transfers.
>>>     Better use of clk_(un)prepare().
>>>     More sensible transfer timeout.
>>> v5: Another round of review comments from Ryan Mallon, Felipe Balbi
>>>     and Russell King: convert twi clk to use .dev_id, cleanups
>>> v4: Integrated more review comments from Ryan Mallon and Felipe Balbi:
>>>     Moved register include file to local include, code cleanups
>>> v3: Integrated review comments from Ryan Mallon and Felipe Balbi
>>> v2: Fixed whitespace issue
>>>
>>> Nikolaus Voss (5):
>>>   drivers/i2c/busses/i2c-at91.c: remove broken driver
>>>   Replace clk_lookup.con_id with clk_lookup.dev_id entries for twi clk
>>>   drivers/i2c/busses/i2c-at91.c: add new driver
>>>   G45 TWI: remove open drain setting for twi function gpios
>>>   i2c-at91.c: add SMBUS_READ_BLOCK_DATA functionality
>>
>> Is the original driver so broken that the two could not co-exist, or are
>> we making so many changes that there's no point in keeping the original
>> one?
> 
> The old driver was marked as broken for the above reasons and I can hardly
> imagine any setup in which it would be preferable to i2c-gpio. So it does
> not make any sense to keep the old driver alive. Though inspired by the old
> driver, the new one is almost a rewrite from scratch, so for better reviewing,
> I removed the old instead of doing a diff.


I can confirm this. I worked on a number of AT91 based platforms with
several different i2c client devices and we always had to use the
i2c-gpio driver because the at91-i2c driver would not reliably work with
our client devices.

None of the at91 defconfigs select I2C_AT91, so it should be fairly safe
to remove the old driver. Getting this driver into linux-next (if it
isn't already) would be good so we can see if it does cause problems for
anybody.

Thanks,
~Ryan

[PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c

[Hubert Feurstein]

Hi,

I've tested this driver on a 2.6.38 kernel with several i2c clients
(temp-sensor, audio-codec, touchscreen-controller, w1-bridge,
io-expanders) and works without problems. SoC: at91sam9g45

Because of the 2.6.38 kernel, I had to skip "[PATCH v7 2/5] Replace
clk_lookup.con_id with clk_lookup.dev_id entries for twi clk" and used
instead: at91_clock_associate("twi0_clk", &at91sam9g45_twi0_device.dev,
"twi_clk");

Best Regards
Hubert

On 2011-11-23 16:35, Nikolaus Voss wrote:
> The old driver has two main deficencies:
> i)  No repeated start (Sr) condiction is possible, this makes it unusable
>     e.g. for most SMBus transfers.
> ii) I/O was done with polling/busy waiting what caused over-/underruns
>     even at light system loads and clock speeds.
> 
> The new driver overcomes these deficencies and in addition allows for
> more than one TWI interface.
> 

Tested-by: Hubert Feurstein <h.feurstein@gmail.com>

AW: [PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c

[Carsten Behling]

Hi,

I've tested this driver with the pca953x GPIO expander driver with a PCA9554.

In the case of 8 GPIO pins (my case) "i2c_smbus_read_byte_data(...)"
is called to read the registers of the GPIO expander. This results in a at91_twi_xfer with two messages. The first message is to put the register address into the IADR and the second is to read the one byte content of the register.

At the end we have a one byte read with a repeated start condition. 

I observed that reading out the GPIO status is one read delayed.
The first read to a register from the pca953x driver does not result in a RXRDY interrupt and at91_twi_read_next_byte(...) is not called.
Only the completion routine is triggered with a TXCOMP interrupt.

Additionally, I took a look at the status flags just after the control flags are set (in at91_do_twi_transfer(...), AT91_TWI_START|AT91_TWI_STOP for the one byte read). Surprisingly, the AT91_TWI_RXRDY flag is zero before the first register read and one for the following reads. According to the manual this flag should be zero after a read on AT91_TWI_RHR.

Reading the AT91_TWI_RHR before the control flags are set to be sure that the AT91_TWI_RXRDY is cleared leads to a never occurring RXRDY interrupt.

This looks very strange to me. Can anyone help?

Mit freundlichen Gr??en / Best regards
Carsten Behling

Development Engineer
Garz & Fricke GmbH
Tempowerkring 2, 21079 Hamburg - Germany
Amtsgericht Hamburg HRB 60514
Gesch?ftsf?hrer: Manfred Garz, Matthias Fricke
Phone: +49 (0) 40 791 899 - 56
Fax:    +49 40 / 791 899 - 39
www.garz-fricke.com 

 


-----Urspr?ngliche Nachricht-----
Von: Hubert Feurstein [mailto:h.feurstein at gmail.com] 
Gesendet: Freitag, 25. November 2011 16:42
An: Nikolaus Voss
Cc: linux-i2c at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org; ben-linux at fluff.org; Carsten Behling
Betreff: Re: [PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c

Hi,

I've tested this driver on a 2.6.38 kernel with several i2c clients
(temp-sensor, audio-codec, touchscreen-controller, w1-bridge,
io-expanders) and works without problems. SoC: at91sam9g45

Because of the 2.6.38 kernel, I had to skip "[PATCH v7 2/5] Replace
clk_lookup.con_id with clk_lookup.dev_id entries for twi clk" and used
instead: at91_clock_associate("twi0_clk", &at91sam9g45_twi0_device.dev,
"twi_clk");

Best Regards
Hubert

On 2011-11-23 16:35, Nikolaus Voss wrote:
> The old driver has two main deficencies:
> i)  No repeated start (Sr) condiction is possible, this makes it unusable
>     e.g. for most SMBus transfers.
> ii) I/O was done with polling/busy waiting what caused over-/underruns
>     even at light system loads and clock speeds.
> 
> The new driver overcomes these deficencies and in addition allows for
> more than one TWI interface.
> 

Tested-by: Hubert Feurstein <h.feurstein@gmail.com>

[PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c

[Voss, Nikolaus]

Hi,

Carsten Behling wrote on 2011-12-28:
> I've tested this driver with the pca953x GPIO expander driver with a PCA9554.
> 
> In the case of 8 GPIO pins (my case) "i2c_smbus_read_byte_data(...)"
[...]
> I observed that reading out the GPIO status is one read delayed.
> The first read to a register from the pca953x driver does not result in a
> RXRDY interrupt and at91_twi_read_next_byte(...) is not called.
> Only the completion routine is triggered with a TXCOMP interrupt.

Which SOC are you using? This is probably a hardware bug since on my
at91sam9g45 i2c_smbus_read_byte_data() works reliably without any
problems. Please check the errata and see if there is a useable
workaround. If not, the driver should be disabled for your SOC.

> Additionally, I took a look at the status flags just after the control flags
> are set (in at91_do_twi_transfer(...), AT91_TWI_START|AT91_TWI_STOP for the
> one byte read). Surprisingly, the AT91_TWI_RXRDY flag is zero before the first
> register read and one for the following reads. According to the manual this
> flag should be zero after a read on AT91_TWI_RHR.
> 
> Reading the AT91_TWI_RHR before the control flags are set to be sure that the
> AT91_TWI_RXRDY is cleared leads to a never occurring RXRDY interrupt.

Again, this behavior does not conform to the datasheet, I suspect documented
or undocumented errata... At91rm9200 has at least one erratum for which I
imported a workaround from the old driver (which does not use interrupts).

Niko