[PATCH] ARM: S3C64XX: Fix clkdev device names for I2C clocks

[PATCH] ARM: S3C64XX: Fix clkdev device names for I2C clocks

[Mark Brown]

When the S3C64xx CPUs were converted to clkdev mappings were added for the
I2C controllers on them but there's a couple of issues with these mappings:

- We're randomly using a device name for controller 1 but not controller 0
  which is odd.
- The controller type for controller 1 is s3c2440, not s3c2410 which is
  what the device is actually registered as so dev_name() based lookups
  fail.

The end result is that only controller 0 has a good mapping, though for
some reason this did used to work.

Something in recent -next appears to have triggered this but I can't for
the life of me figure out how it ever worked.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 arch/arm/mach-s3c64xx/clock.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-s3c64xx/clock.c b/arch/arm/mach-s3c64xx/clock.c
index 872e683..3f2bb26 100644
--- a/arch/arm/mach-s3c64xx/clock.c
+++ b/arch/arm/mach-s3c64xx/clock.c
@@ -138,12 +138,13 @@ static struct clk init_clocks_off[] = {
 		.ctrlbit	= S3C_CLKCON_PCLK_TSADC,
 	}, {
 		.name		= "i2c",
+		.devname	= "s3c2440-i2c.0",
 		.parent		= &clk_p,
 		.enable		= s3c64xx_pclk_ctrl,
 		.ctrlbit	= S3C_CLKCON_PCLK_IIC,
 	}, {
 		.name		= "i2c",
-		.devname	= "s3c2440-i2c.1",
+		.devname	= "s3c2410-i2c.1",
 		.parent		= &clk_p,
 		.enable		= s3c64xx_pclk_ctrl,
 		.ctrlbit	= S3C6410_CLKCON_PCLK_I2C1,
-- 
1.7.5.4

[PATCH] ARM: S3C64XX: Fix clkdev device names for I2C clocks

[Kyungmin Park]

On Wed, Aug 31, 2011 at 6:30 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> When the S3C64xx CPUs were converted to clkdev mappings were added for the
> I2C controllers on them but there's a couple of issues with these mappings:
>
> - We're randomly using a device name for controller 1 but not controller 0
> ?which is odd.
> - The controller type for controller 1 is s3c2440, not s3c2410 which is
> ?what the device is actually registered as so dev_name() based lookups
> ?fail.
>
> The end result is that only controller 0 has a good mapping, though for
> some reason this did used to work.
>
> Something in recent -next appears to have triggered this but I can't for
> the life of me figure out how it ever worked.
>
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> ---
> ?arch/arm/mach-s3c64xx/clock.c | ? ?3 ++-
> ?1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/mach-s3c64xx/clock.c b/arch/arm/mach-s3c64xx/clock.c
> index 872e683..3f2bb26 100644
> --- a/arch/arm/mach-s3c64xx/clock.c
> +++ b/arch/arm/mach-s3c64xx/clock.c
> @@ -138,12 +138,13 @@ static struct clk init_clocks_off[] = {
> ? ? ? ? ? ? ? ?.ctrlbit ? ? ? ?= S3C_CLKCON_PCLK_TSADC,
> ? ? ? ?}, {
> ? ? ? ? ? ? ? ?.name ? ? ? ? ? = "i2c",
> + ? ? ? ? ? ? ? .devname ? ? ? ?= "s3c2440-i2c.0",
> ? ? ? ? ? ? ? ?.parent ? ? ? ? = &clk_p,
> ? ? ? ? ? ? ? ?.enable ? ? ? ? = s3c64xx_pclk_ctrl,
> ? ? ? ? ? ? ? ?.ctrlbit ? ? ? ?= S3C_CLKCON_PCLK_IIC,
> ? ? ? ?}, {
> ? ? ? ? ? ? ? ?.name ? ? ? ? ? = "i2c",
> - ? ? ? ? ? ? ? .devname ? ? ? ?= "s3c2440-i2c.1",
> + ? ? ? ? ? ? ? .devname ? ? ? ?= "s3c2410-i2c.1",
It's not matched with your description. and also not matched with
codes at "arch/arm/mach-s3c64xx/s3c6410.c"

       /* the i2c devices are directly compatible with s3c2440 */
        s3c_i2c0_setname("s3c2440-i2c");
        s3c_i2c1_setname("s3c2440-i2c");

Thank you,
Kyungmin Park

> ? ? ? ? ? ? ? ?.parent ? ? ? ? = &clk_p,
> ? ? ? ? ? ? ? ?.enable ? ? ? ? = s3c64xx_pclk_ctrl,
> ? ? ? ? ? ? ? ?.ctrlbit ? ? ? ?= S3C6410_CLKCON_PCLK_I2C1,
> --
> 1.7.5.4
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

[PATCH] ARM: S3C64XX: Fix clkdev device names for I2C clocks

[Mark Brown]

On Wed, Aug 31, 2011 at 08:48:03AM +0900, Kyungmin Park wrote:

> It's not matched with your description. and also not matched with
> codes at "arch/arm/mach-s3c64xx/s3c6410.c"

>        /* the i2c devices are directly compatible with s3c2440 */
>         s3c_i2c0_setname("s3c2440-i2c");
>         s3c_i2c1_setname("s3c2440-i2c");

Oh, FFS.  That's what's broken - the names that are actually appearing
in the system are the names that the devices have in the relevant
dev-i2c?.c files and obviously there's no reason one would expect to
find code like the above since it's just a little abstruse to rewrite
the devices like that.

Looking at the code I bet what broke it is the CPU revision
identification changes - it's now responsible for making sure the fixups
are run.  Either it's not matching correctly or it's running too late to
make the fixup before the device is registered.

Though the first part of the issue (the fact that I2C0 isn't using a
device name and I2C1 is) remains an issue.

[PATCH] ARM: S3C64XX: Fix clkdev device names for I2C clocks

[Thomas Abraham]

On 31 August 2011 05:51, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, Aug 31, 2011 at 08:48:03AM +0900, Kyungmin Park wrote:
>
>> It's not matched with your description. and also not matched with
>> codes at "arch/arm/mach-s3c64xx/s3c6410.c"
>
>> ? ? ? ?/* the i2c devices are directly compatible with s3c2440 */
>> ? ? ? ? s3c_i2c0_setname("s3c2440-i2c");
>> ? ? ? ? s3c_i2c1_setname("s3c2440-i2c");
>
> Oh, FFS. ?That's what's broken - the names that are actually appearing
> in the system are the names that the devices have in the relevant
> dev-i2c?.c files and obviously there's no reason one would expect to
> find code like the above since it's just a little abstruse to rewrite
> the devices like that.
>
> Looking at the code I bet what broke it is the CPU revision
> identification changes - it's now responsible for making sure the fixups
> are run. ?Either it's not matching correctly or it's running too late to
> make the fixup before the device is registered.
>
> Though the first part of the issue (the fact that I2C0 isn't using a
> device name and I2C1 is) remains an issue.

There are two instances of clock with name i2c registered for s3c64xx.
One has a devname and the other does not. So this continued to work.
The i2c0 clock instance did not get a devname because its instance id
was -1 (prior to clkdev support).

If a devname is added to i2c0 clock instance for s3c64xx, it should be
set to "s3c2440-i2c". If the devname is set to "s3c2440-i2c.0", then
CONFIG_S3C_DEV_I2C1 will have to defined for s3c64xx, otherwise, as
per arch/arm/plat-samsung/dev-i2c0.c, the platform device id for i2c0
instance will be -1 and the clock lookup for i2c0 will fail.

Regards,
Thomas.

[PATCH] ARM: S3C64XX: Fix clkdev device names for I2C clocks

[Mark Brown]

On Wed, Aug 31, 2011 at 01:44:20PM +0530, Thomas Abraham wrote:

> There are two instances of clock with name i2c registered for s3c64xx.
> One has a devname and the other does not. So this continued to work.
> The i2c0 clock instance did not get a devname because its instance id
> was -1 (prior to clkdev support).

Not having devname will *work* - the problem is that it'll work too
often and return the wrong clock whenever anything goes wrong.

> If a devname is added to i2c0 clock instance for s3c64xx, it should be
> set to "s3c2440-i2c". If the devname is set to "s3c2440-i2c.0", then
> CONFIG_S3C_DEV_I2C1 will have to defined for s3c64xx, otherwise, as
> per arch/arm/plat-samsung/dev-i2c0.c, the platform device id for i2c0
> instance will be -1 and the clock lookup for i2c0 will fail.

All of which is just asking for fragility; the ifdefs for renumbering
the device are really unhelpful here - the system always has two I2C
controllers and the numbering is going to change depending on which
boards are built in so you can break things by enabling support for a
second board.

Frankly I'm not convinced that all this dancing about with explicitly
selecting which devices to enable is worth it; the space savings are
extremely small and the complexity regularly causes issues that just
shouldn't exist.