| Message ID | 1314739841-13522-1-git-send-email-broonie@opensource.wolfsonmicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
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.
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.
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.
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,
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(-)