LinuxPPC-Dev Archive on lore.kernel.org
 help / color / mirror / Atom feed
* Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
From: Stephen Rothwell @ 2007-11-06  2:28 UTC (permalink / raw)
  To: Jon Smirl; +Cc: Tjernlund, Jean Delvare, i2c, linuxppc-dev
In-Reply-To: <9e4733910711051734m7cf1fac0y5fb39feb37bad548@mail.gmail.com>

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

On Mon, 5 Nov 2007 20:34:51 -0500 "Jon Smirl" <jonsmirl@gmail.com> wrote:
>
> On 11/5/07, Scott Wood <scottwood@freescale.com> wrote:
> > > +static struct of_platform_driver mpc_i2c_driver = {
> > > +     .owner          = THIS_MODULE,
> > > +     .name           = DRV_NAME,
> > > +     .match_table    = mpc_i2c_of_match,
> > > +     .probe          = mpc_i2c_probe,
> > > +     .remove         = __devexit_p(mpc_i2c_remove),
> > > +     .driver         = {
> > > +             .name   = DRV_NAME,
> > >       },
> > >  };
> >
> > Do we still need .name if we have .driver.name?
> 
> This is a general question, if of_platform_driver doesn't need .name
> it should be removed from the structure.

I am in the process of doing that.  However there a quite a few drivers
that need to be fixed first.  In the meantime,
of_register_platform_driver will copy which ever of the name and owner
fields you fill in to the others, so we can convert over time.  For new
drivers (and changing), please use the name and owner fields in the
embedded device_driver struct. (this is the same as done by the platform
drivers currently ...)

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

^ permalink raw reply

* [PATCH] [POWERPC] pasemi: Don't reset mpic at boot
From: Olof Johansson @ 2007-11-06  2:21 UTC (permalink / raw)
  To: linuxppc-dev

[POWERPC] pasemi: Don't reset mpic at boot
    
Due to an erratum, we don't want to reset the mpic at boot time. It can
sometimes cause problems with lost interrupts later on while running.
    
Signed-off-by: Olof Johansson <olof@lixom.net>

diff --git a/arch/powerpc/platforms/pasemi/setup.c b/arch/powerpc/platforms/pasemi/setup.c
index 857d238..bd85853 100644
--- a/arch/powerpc/platforms/pasemi/setup.c
+++ b/arch/powerpc/platforms/pasemi/setup.c
@@ -214,7 +214,7 @@ static __init void pas_init_IRQ(void)
 	printk(KERN_DEBUG "OpenPIC addr: %lx\n", openpic_addr);
 
 	mpic = mpic_alloc(mpic_node, openpic_addr,
-			  MPIC_PRIMARY|MPIC_LARGE_VECTORS|MPIC_WANTS_RESET,
+			  MPIC_PRIMARY|MPIC_LARGE_VECTORS,
 			  0, 0, " PAS-OPIC  ");
 	BUG_ON(!mpic);
 

^ permalink raw reply related

* Re: [PATCH] powerpc: prpmc2800 - Don't trust documented memory size
From: Mark A. Greer @ 2007-11-06  1:59 UTC (permalink / raw)
  To: Mark A. Greer; +Cc: linuxppc-dev
In-Reply-To: <20071106012805.GA20909@mag.az.mvista.com>

On Mon, Nov 05, 2007 at 06:28:05PM -0700, Mark A. Greer wrote:
> It turns out that the firmware on some PrPMC2800 processor modules
> initializes the memory controller for 512 MB even when there is more
> memory.  As a simple work around, set the amount of memory in the
> device tree passed to the kernel to the lesser of what the memory
> controller is set up for and the actual amount of memory.
> 
> Signed-off-by: Mark A. Greer <mgreer@mvista.com>
> ---

Paul, please ignore this patch.  There is still something wrong.

Sorry for the noise.

Mark

^ permalink raw reply

* Re: [PATCH] [POWERPC] pasemi: Broaden specific references to 1682M
From: Olof Johansson @ 2007-11-06  2:01 UTC (permalink / raw)
  To: Segher Boessenkool; +Cc: linuxppc-dev, Doug Thompson
In-Reply-To: <cd9a9e67893406e486d2ffc7599e28b7@kernel.crashing.org>

On Tue, Nov 06, 2007 at 02:42:08AM +0100, Segher Boessenkool wrote:
> Hi Olof,
>
> Nice cleanup!  One minor thing...
>
>>  static struct of_device_id pasemi_bus_ids[] = {
>> +	/* Unfortunately needed for legacy firmwares */
>>  	{ .type = "localbus", },
>>  	{ .type = "sdc", },
>> +	{ .compatible = "pasemi,localbus", },
>> +	{ .compatible = "pasemi,sdc", },
>
> The new comment is for the device_type entries only, right?  You
> might want to make that more clear.

Good point. I'll clarify it with a new comment above the compatible
entries. I won't repost the patch since the change is minor, but include
it for the upstream push.


Thanks,

-Olof

^ permalink raw reply

* Re: [PATCH] [POWERPC] pasemi: Broaden specific references to 1682M
From: Segher Boessenkool @ 2007-11-06  1:42 UTC (permalink / raw)
  To: Olof Johansson; +Cc: linuxppc-dev, Doug Thompson
In-Reply-To: <20071105032802.GA16613@lixom.net>

Hi Olof,

Nice cleanup!  One minor thing...

>  static struct of_device_id pasemi_bus_ids[] = {
> +	/* Unfortunately needed for legacy firmwares */
>  	{ .type = "localbus", },
>  	{ .type = "sdc", },
> +	{ .compatible = "pasemi,localbus", },
> +	{ .compatible = "pasemi,sdc", },

The new comment is for the device_type entries only, right?  You
might want to make that more clear.


Segher

^ permalink raw reply

* Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
From: Jon Smirl @ 2007-11-06  1:34 UTC (permalink / raw)
  To: Scott Wood; +Cc: Tjernlund, linuxppc-dev, Jean Delvare, i2c
In-Reply-To: <472F7247.9070106@freescale.com>

On 11/5/07, Scott Wood <scottwood@freescale.com> wrote:
> > +static struct of_platform_driver mpc_i2c_driver = {
> > +     .owner          = THIS_MODULE,
> > +     .name           = DRV_NAME,
> > +     .match_table    = mpc_i2c_of_match,
> > +     .probe          = mpc_i2c_probe,
> > +     .remove         = __devexit_p(mpc_i2c_remove),
> > +     .driver         = {
> > +             .name   = DRV_NAME,
> >       },
> >  };
>
> Do we still need .name if we have .driver.name?

This is a general question, if of_platform_driver doesn't need .name
it should be removed from the structure.

-- 
Jon Smirl
jonsmirl@gmail.com

^ permalink raw reply

* [PATCH] powerpc: prpmc2800 - Don't trust documented memory size
From: Mark A. Greer @ 2007-11-06  1:28 UTC (permalink / raw)
  To: linuxppc-dev

It turns out that the firmware on some PrPMC2800 processor modules
initializes the memory controller for 512 MB even when there is more
memory.  As a simple work around, set the amount of memory in the
device tree passed to the kernel to the lesser of what the memory
controller is set up for and the actual amount of memory.

Signed-off-by: Mark A. Greer <mgreer@mvista.com>
---
 arch/powerpc/boot/prpmc2800.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/boot/prpmc2800.c b/arch/powerpc/boot/prpmc2800.c
index 9614e1d..559c45e 100644
--- a/arch/powerpc/boot/prpmc2800.c
+++ b/arch/powerpc/boot/prpmc2800.c
@@ -405,7 +405,10 @@ static void prpmc2800_fixups(void)
 
 	bip = prpmc2800_get_bip(); /* Get board info based on VPD */
 
-	mem_size = (bip) ? bip->mem_size : mv64x60_get_mem_size(bridge_base);
+	mem_size = mv64x60_get_mem_size(bridge_base);
+	if (bip)
+		mem_size = min(mem_size, bip->mem_size);
+
 	prpmc2800_bridge_setup(mem_size); /* Do necessary bridge setup */
 
 	/* If the VPD doesn't match what we know about, just use the

^ permalink raw reply related

* Re: [PATCH] 4xx; Replace #includes of asm/of_platform.h with linux/of_platform.h.
From: Stephen Rothwell @ 2007-11-06  1:10 UTC (permalink / raw)
  To: Josh Boyer; +Cc: linuxppc-dev@ozlabs.org
In-Reply-To: <20071105175341.6ee15106@vader.jdub.homelinux.org>

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

On Mon, 5 Nov 2007 17:53:41 -0600 Josh Boyer <jwboyer@gmail.com> wrote:
>
> Yeah, this is fine with me too.  But why haven't we put a #warning in
> asm/of_platform.h and asm/prom.h?

Because thee are way to many places to be fixed yet.  And asm/prom.h needs to be explicitly included if you want to use the flattened device tree accessors.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

^ permalink raw reply

* Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
From: Jon Smirl @ 2007-11-06  0:41 UTC (permalink / raw)
  To: Scott Wood; +Cc: Tjernlund, linuxppc-dev, Jean Delvare, i2c
In-Reply-To: <472F8267.8070106@freescale.com>

On 11/5/07, Scott Wood <scottwood@freescale.com> wrote:
> >>> One side effect is that legacy style i2c drivers won't work anymore.
> >> If you mean legacy-style client drivers, why not?
> >
> > i2c_new_device() doesn't work with legacy-style client drivers.
>
> No, but they should still work the old way.

I'm not in favor trying to support both legacy and new style i2c
drivers.  It took me all of five minutes to convert an existing legacy
driver to the new style. Pretty much all you need to do is delete code
(about 100 lines). So I'd recommend converting the drivers we are
interest in instead of trying to support both types.

-- 
Jon Smirl
jonsmirl@gmail.com

^ permalink raw reply

* Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
From: Jon Smirl @ 2007-11-06  0:33 UTC (permalink / raw)
  To: Grant Likely; +Cc: Tjernlund, Jean Delvare, i2c, linuxppc-dev
In-Reply-To: <fa686aa40711051446q1abe886dh6225fa1ec675ef9b@mail.gmail.com>

On 11/5/07, Grant Likely <grant.likely@secretlab.ca> wrote:
> On 11/5/07, Scott Wood <scottwood@freescale.com> wrote:
> > Jon Smirl wrote:
> > > On 11/5/07, Scott Wood <scottwood@freescale.com> wrote:
> > >> Jon Smirl wrote:
> > >>> This is my first pass at reworking the Freescale i2c driver. It
> > >>> switches the driver from being a platform driver to an open firmware
> > >>> one. I've checked it out on my hardware and it is working.
> > >> We may want to hold off on this until arch/ppc goes away (or at least
> > >> all users of this driver in arch/ppc).
> > >
> > > How about renaming the old driver file and leaving it hooked to ppc?
> > > Then it would get deleted when ppc goes away. That would let work
> > > progress on the powerpc version.
> >
> > Or we could have one driver that has two probe methods.  I don't like
> > forking the driver.
>
> I agree.  This driver can and should have multiple bus bindings.

What non-of platform uses this driver?

I believe this is the last struct platform driver left for the
mpc5200. Fixing this one allows the platform bus to be removed. With a
device tree there isn't any reason to keep a platform bus, everything
should be on the of_platform bus.

-- 
Jon Smirl
jonsmirl@gmail.com

^ permalink raw reply

* Re: [PATCH] 4xx; Replace #includes of asm/of_platform.h with linux/of_platform.h.
From: Josh Boyer @ 2007-11-05 23:53 UTC (permalink / raw)
  To: Stephen Rothwell; +Cc: linuxppc-dev@ozlabs.org
In-Reply-To: <20071106094014.cd429711.sfr@canb.auug.org.au>

On Tue, 6 Nov 2007 09:40:14 +1100
Stephen Rothwell <sfr@canb.auug.org.au> wrote:

> On Mon, 05 Nov 2007 15:43:43 -0600 Jon Loeliger <jdl@freescale.com> wrote:
> >
> > From: Jon Loeliger <jdl@freescale.com>
> > 
> > Signed-off-by: Jon Loeliger <jdl@freescale.com>
> 
> Acked-by: Stephen Rothwell <sfr@canb.auug.org.au>

Yeah, this is fine with me too.  But why haven't we put a #warning in
asm/of_platform.h and asm/prom.h?

josh

^ permalink raw reply

* Re: libfdt: Fix sw_tree1 testcase
From: David Gibson @ 2007-11-05 23:42 UTC (permalink / raw)
  To: Jon Loeliger; +Cc: linuxppc-dev
In-Reply-To: <E1Ip3bp-000260-Of@jdl.com>

On Mon, Nov 05, 2007 at 09:11:25AM -0600, Jon Loeliger wrote:
> So, like, the other day David Gibson mumbled:
> > There is a bug in the sw_tree1 testcase / utility which puts two
> > "compatible" properties in one node in the output tree.  This patch
> > fixes the bug, and also adds a new test checking that the sw_tree1
> > output is equal to test_tree1.dtb as its supposed to be, which should
> > catch future errors of this type.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> This change appears to cause a test to fail.
> I've not looked into it beyond "make check failed":

Crud, I screwed up and gave you an intermediate version of the patch
which tried to do the same thing for rw_tree1.  For that to work, I'll
need to write a dtbs_equal_notordered test.

Corrected version below.

libfdt: Fix sw_tree1 testcase

There is a bug in the sw_tree1 testcase / utility which puts two
"compatible" properties in one node in the output tree.  This patch
fixes the bug, and also adds a new test checking that the sw_tree1
output is equal to test_tree1.dtb as its supposed to be, which should
catch future errors of this type.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Index: dtc/tests/run_tests.sh
===================================================================
--- dtc.orig/tests/run_tests.sh	2007-11-06 10:38:21.000000000 +1100
+++ dtc/tests/run_tests.sh	2007-11-06 10:39:37.000000000 +1100
@@ -71,6 +71,7 @@
     run_test sw_tree1
     tree1_tests sw_tree1.test.dtb
     tree1_tests unfinished_tree1.test.dtb
+    run_test dtbs_equal_ordered test_tree1.dtb sw_tree1.test.dtb
 
     # fdt_move tests
     for tree in test_tree1.dtb sw_tree1.test.dtb unfinished_tree1.test.dtb; do
Index: dtc/tests/sw_tree1.c
===================================================================
--- dtc.orig/tests/sw_tree1.c	2007-11-05 16:59:13.000000000 +1100
+++ dtc/tests/sw_tree1.c	2007-11-06 10:39:37.000000000 +1100
@@ -64,7 +64,6 @@
 	CHECK(fdt_begin_node(fdt, "subsubnode"));
 	CHECK(fdt_property(fdt, "compatible", "subsubnode1\0subsubnode",
 			   23));
-	CHECK(fdt_property_string(fdt, "compatible", "subsubnode1\0"));
 	CHECK(fdt_property_typed(fdt, "prop-int", TEST_VALUE_1));
 	CHECK(fdt_end_node(fdt));
 	CHECK(fdt_end_node(fdt));


-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

^ permalink raw reply

* Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
From: Grant Likely @ 2007-11-05 23:03 UTC (permalink / raw)
  To: Scott Wood; +Cc: Tjernlund, Jean Delvare, i2c, linuxppc-dev
In-Reply-To: <472F915F.9010400@freescale.com>

On 11/5/07, Scott Wood <scottwood@freescale.com> wrote:
> Matt Sealey wrote:
> > Scott Wood wrote:
> >> Jon Smirl wrote:
> >
> >>>>> cell-index = <1>;
> >>>> What is cell-index for?
> >>> I was using it to control the bus number, is that the wrong
> >>> attribute?
> >>
> >> It shouldn't be specified at all -- the hardware has no concept of
> >> a device number.
> >
> > Well, all i2c devices have a chip id you can probe for,
>
> I meant a controller device number (a.k.a. bus number), which (outside
> of documentation) is purely a Linux invention, and which is what
> cell-index was being used for above.
>
> > as for buses I think cell-index is a holdover from the way the PSC
> > code is organised on the MPC5200 for example - if you have multiple
> > buses which use the same registers, for example. It's redundant on
> > the PSC's for programming because they all use different register
> > offsets but if you move to other devices like the GPTs, then it is
> > then useful for debugging (it is far more interesting to say GPT1
> > than GPT @ offset to match the)

Actually, it is not intended for this.  cell-index is not intended to
be able to say GPT1 instead of GPT@xxx (while that may be possible, it
is not the intent).  Nor is it a holdover from the PSC code design.
It is designed to describe the internal structure of the SoC.  The
5200 has 6 PSCs, and there are some chip registers which are shared
between all the PSCs (for clocking, IO mode, etc).  It is not
sufficient to simply plop down a device tree node for each PSC because
it doesn't give enough information about which bits to use in the
shared regs.  (For example, the port_config register)

> > and in general for tweaking OTHER
> > parts of the chip (for instance the CDM - very relevant!) which use
> > single registers to control entire swathes of units.
>
> Right, that's what cell-index is for.  This is different. :-)

Yes, this is correct.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

^ permalink raw reply

* Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
From: Grant Likely @ 2007-11-05 22:46 UTC (permalink / raw)
  To: Scott Wood; +Cc: Tjernlund, linuxppc-dev, Jean Delvare, i2c
In-Reply-To: <472F8267.8070106@freescale.com>

On 11/5/07, Scott Wood <scottwood@freescale.com> wrote:
> Jon Smirl wrote:
> > On 11/5/07, Scott Wood <scottwood@freescale.com> wrote:
> >> Jon Smirl wrote:
> >>> This is my first pass at reworking the Freescale i2c driver. It
> >>> switches the driver from being a platform driver to an open firmware
> >>> one. I've checked it out on my hardware and it is working.
> >> We may want to hold off on this until arch/ppc goes away (or at least
> >> all users of this driver in arch/ppc).
> >
> > How about renaming the old driver file and leaving it hooked to ppc?
> > Then it would get deleted when ppc goes away. That would let work
> > progress on the powerpc version.
>
> Or we could have one driver that has two probe methods.  I don't like
> forking the driver.

I agree.  This driver can and should have multiple bus bindings.

> >>>       cell-index = <1>;
> >> What is cell-index for?
> >
> > I was using it to control the bus number, is that the wrong attribute?
>
> It shouldn't be specified at all -- the hardware has no concept of a
> device number.

cell-index is important.  It describes the hardware, or more
specifically the layout of the SoC.  The SoC has 2 i2c busses which
are numbered 0 and 1.  This property should stay for the 5200.
However, that is the only purpose of it.  cell-index does *not*
describe the system level bus number.

> > I was allowing control of the bus number with "cell-index" and
> > i2c_add_numbered_adapter().
> > Should I get rid of this and switch to i2c_add_adapter()?
>
> Yes.

Yes, the purpose of cell-index is not to give an i2c bus number enumeration.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

^ permalink raw reply

* Re: [PATCH] 4xx; Replace #includes of asm/of_platform.h with linux/of_platform.h.
From: Stephen Rothwell @ 2007-11-05 22:40 UTC (permalink / raw)
  To: Jon Loeliger; +Cc: linuxppc-dev@ozlabs.org
In-Reply-To: <1194299023.7158.59.camel@ld0161-tx32>

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

On Mon, 05 Nov 2007 15:43:43 -0600 Jon Loeliger <jdl@freescale.com> wrote:
>
> From: Jon Loeliger <jdl@freescale.com>
> 
> Signed-off-by: Jon Loeliger <jdl@freescale.com>

Acked-by: Stephen Rothwell <sfr@canb.auug.org.au>

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

^ permalink raw reply

* Re: [PATCH] Replace some #includes of asm/of_platform.h with linux/of_platform.h.
From: Stephen Rothwell @ 2007-11-05 22:36 UTC (permalink / raw)
  To: Jon Loeliger; +Cc: linuxppc-dev@ozlabs.org
In-Reply-To: <1194296705.7158.45.camel@ld0161-tx32>

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

Hi Jon,

Thanks for starting this.

On Mon, 05 Nov 2007 15:05:06 -0600 Jon Loeliger <jdl@freescale.com> wrote:
>
> From: Jon Loeliger <jdl@freescale.com>
> 
> Signed-off-by: Jon Loeliger <jdl@freescale.com>

Acked-by: Stephen Rothwell <sfr@canb.auug.org.au>

>  #include <asm/of_device.h>

asm/of_device.h -> linux/of_device.h as well?  :-)

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

^ permalink raw reply

* Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
From: Scott Wood @ 2007-11-05 21:55 UTC (permalink / raw)
  To: Matt Sealey; +Cc: Tjernlund, linuxppc-dev, Jean Delvare, i2c
In-Reply-To: <472F9086.2060606@genesi-usa.com>

Matt Sealey wrote:
> Scott Wood wrote:
>> Jon Smirl wrote:
> 
>>>>> cell-index = <1>;
>>>> What is cell-index for?
>>> I was using it to control the bus number, is that the wrong
>>> attribute?
>> 
>> It shouldn't be specified at all -- the hardware has no concept of
>> a device number.
> 
> Well, all i2c devices have a chip id you can probe for,

I meant a controller device number (a.k.a. bus number), which (outside 
of documentation) is purely a Linux invention, and which is what 
cell-index was being used for above.

> as for buses I think cell-index is a holdover from the way the PSC
> code is organised on the MPC5200 for example - if you have multiple
> buses which use the same registers, for example. It's redundant on
> the PSC's for programming because they all use different register
> offsets but if you move to other devices like the GPTs, then it is
> then useful for debugging (it is far more interesting to say GPT1
> than GPT @ offset to match the) and in general for tweaking OTHER
> parts of the chip (for instance the CDM - very relevant!) which use
> single registers to control entire swathes of units.

Right, that's what cell-index is for.  This is different. :-)

-Scott

^ permalink raw reply

* Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
From: Matt Sealey @ 2007-11-05 21:52 UTC (permalink / raw)
  To: Scott Wood; +Cc: Tjernlund, linuxppc-dev, Jean Delvare, i2c
In-Reply-To: <472F8267.8070106@freescale.com>

Scott Wood wrote:
> Jon Smirl wrote:

>>>>       cell-index = <1>;
>>> What is cell-index for?
>> I was using it to control the bus number, is that the wrong attribute?
> 
> It shouldn't be specified at all -- the hardware has no concept of a 
> device number.

Well, all i2c devices have a chip id you can probe for, as for buses I
think cell-index is a holdover from the way the PSC code is organised
on the MPC5200 for example - if you have multiple buses which use the
same registers, for example. It's redundant on the PSC's for programming
because they all use different register offsets but if you move to
other devices like the GPTs, then it is then useful for debugging (it
is far more interesting to say GPT1 than GPT @ offset to match the)
and in general for tweaking OTHER parts of the chip (for instance
the CDM - very relevant!) which use single registers to control entire
swathes of units.

This way if you are in any doubt you can tell which one you should
be futzing with in other parts of the chip without complicated logic
based on MBAR offsets from the manual (magic numbers hinder the
maintainability and portability of the code). It's not relevant for
i2c but like I said, still valid, useful information..

-- 
Matt Sealey <matt@genesi-usa.com>
Genesi, Manager, Developer Relations

^ permalink raw reply

* [PATCH] 4xx; Replace #includes of asm/of_platform.h with linux/of_platform.h.
From: Jon Loeliger @ 2007-11-05 21:43 UTC (permalink / raw)
  To: linuxppc-dev@ozlabs.org

From: Jon Loeliger <jdl@freescale.com>

Signed-off-by: Jon Loeliger <jdl@freescale.com>
---
Chip away at some janitor work.

 arch/powerpc/platforms/40x/walnut.c  |    3 ++-
 arch/powerpc/platforms/44x/bamboo.c  |    3 ++-
 arch/powerpc/platforms/44x/ebony.c   |    3 ++-
 arch/powerpc/platforms/44x/sequoia.c |    3 ++-
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/platforms/40x/walnut.c b/arch/powerpc/platforms/40x/walnut.c
index eb0c136..ff6db24 100644
--- a/arch/powerpc/platforms/40x/walnut.c
+++ b/arch/powerpc/platforms/40x/walnut.c
@@ -17,12 +17,13 @@
  */
 
 #include <linux/init.h>
+#include <linux/of_platform.h>
+
 #include <asm/machdep.h>
 #include <asm/prom.h>
 #include <asm/udbg.h>
 #include <asm/time.h>
 #include <asm/uic.h>
-#include <asm/of_platform.h>
 
 static struct of_device_id walnut_of_bus[] = {
 	{ .compatible = "ibm,plb3", },
diff --git a/arch/powerpc/platforms/44x/bamboo.c b/arch/powerpc/platforms/44x/bamboo.c
index 470e1a3..be23f11 100644
--- a/arch/powerpc/platforms/44x/bamboo.c
+++ b/arch/powerpc/platforms/44x/bamboo.c
@@ -14,12 +14,13 @@
  * option) any later version.
  */
 #include <linux/init.h>
+#include <linux/of_platform.h>
+
 #include <asm/machdep.h>
 #include <asm/prom.h>
 #include <asm/udbg.h>
 #include <asm/time.h>
 #include <asm/uic.h>
-#include <asm/of_platform.h>
 #include "44x.h"
 
 static struct of_device_id bamboo_of_bus[] = {
diff --git a/arch/powerpc/platforms/44x/ebony.c b/arch/powerpc/platforms/44x/ebony.c
index 40e18fc..6cd3476 100644
--- a/arch/powerpc/platforms/44x/ebony.c
+++ b/arch/powerpc/platforms/44x/ebony.c
@@ -17,12 +17,13 @@
  */
 
 #include <linux/init.h>
+#include <linux/of_platform.h>
+
 #include <asm/machdep.h>
 #include <asm/prom.h>
 #include <asm/udbg.h>
 #include <asm/time.h>
 #include <asm/uic.h>
-#include <asm/of_platform.h>
 
 #include "44x.h"
 
diff --git a/arch/powerpc/platforms/44x/sequoia.c b/arch/powerpc/platforms/44x/sequoia.c
index 30700b3..21a9dd1 100644
--- a/arch/powerpc/platforms/44x/sequoia.c
+++ b/arch/powerpc/platforms/44x/sequoia.c
@@ -14,12 +14,13 @@
  * option) any later version.
  */
 #include <linux/init.h>
+#include <linux/of_platform.h>
+
 #include <asm/machdep.h>
 #include <asm/prom.h>
 #include <asm/udbg.h>
 #include <asm/time.h>
 #include <asm/uic.h>
-#include <asm/of_platform.h>
 #include "44x.h"
 
 static struct of_device_id sequoia_of_bus[] = {
-- 
1.5.3

^ permalink raw reply related

* Re: mmap question on ppc440
From: Josh Boyer @ 2007-11-05 21:16 UTC (permalink / raw)
  To: Steven A. Falco; +Cc: linuxppc-dev
In-Reply-To: <472F8503.2080705@harris.com>

On Mon, 05 Nov 2007 16:02:59 -0500
"Steven A. Falco" <sfalco@harris.com> wrote:

> I am attempting to access the CPLD on the AMCC Sequoia board from 
> user-land.  I open /dev/mem, and mmap it, then try to access the 
> resulting pointer.  That works fine when accessing physical addresses 
> that correspond to RAM, but as soon as I try to access the CPLD at 
> physical address 0xc0000000, I get an infinite machine check.

That's because the CPLD is actually at physical address 0x1C0000000.
Yay for 36-bit physical addresses.

josh

^ permalink raw reply

* [PATCH] Replace some #includes of asm/of_platform.h with linux/of_platform.h.
From: Jon Loeliger @ 2007-11-05 21:05 UTC (permalink / raw)
  To: linuxppc-dev@ozlabs.org

From: Jon Loeliger <jdl@freescale.com>

Signed-off-by: Jon Loeliger <jdl@freescale.com>
---
Chip away at some janitor work.

 arch/powerpc/platforms/82xx/pq2fads.c     |    2 +-
 arch/powerpc/platforms/83xx/mpc832x_mds.c |    2 +-
 arch/powerpc/platforms/83xx/mpc832x_rdb.c |    2 +-
 arch/powerpc/platforms/83xx/mpc836x_mds.c |    2 +-
 arch/powerpc/platforms/85xx/mpc85xx_mds.c |    2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/platforms/82xx/pq2fads.c b/arch/powerpc/platforms/82xx/pq2fads.c
index 4f457a9..1be5005 100644
--- a/arch/powerpc/platforms/82xx/pq2fads.c
+++ b/arch/powerpc/platforms/82xx/pq2fads.c
@@ -15,12 +15,12 @@
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/fsl_devices.h>
+#include <linux/of_platform.h>
 
 #include <asm/io.h>
 #include <asm/cpm2.h>
 #include <asm/udbg.h>
 #include <asm/machdep.h>
-#include <asm/of_platform.h>
 #include <asm/time.h>
 
 #include <sysdev/fsl_soc.h>
diff --git a/arch/powerpc/platforms/83xx/mpc832x_mds.c b/arch/powerpc/platforms/83xx/mpc832x_mds.c
index 972fa85..3ea59af 100644
--- a/arch/powerpc/platforms/83xx/mpc832x_mds.c
+++ b/arch/powerpc/platforms/83xx/mpc832x_mds.c
@@ -23,9 +23,9 @@
 #include <linux/seq_file.h>
 #include <linux/root_dev.h>
 #include <linux/initrd.h>
+#include <linux/of_platform.h>
 
 #include <asm/of_device.h>
-#include <asm/of_platform.h>
 #include <asm/system.h>
 #include <asm/atomic.h>
 #include <asm/time.h>
diff --git a/arch/powerpc/platforms/83xx/mpc832x_rdb.c b/arch/powerpc/platforms/83xx/mpc832x_rdb.c
index fbca336..24e6ce6 100644
--- a/arch/powerpc/platforms/83xx/mpc832x_rdb.c
+++ b/arch/powerpc/platforms/83xx/mpc832x_rdb.c
@@ -16,8 +16,8 @@
 
 #include <linux/pci.h>
 #include <linux/spi/spi.h>
+#include <linux/of_platform.h>
 
-#include <asm/of_platform.h>
 #include <asm/time.h>
 #include <asm/ipic.h>
 #include <asm/udbg.h>
diff --git a/arch/powerpc/platforms/83xx/mpc836x_mds.c b/arch/powerpc/platforms/83xx/mpc836x_mds.c
index 0f3855c..ec1b03b 100644
--- a/arch/powerpc/platforms/83xx/mpc836x_mds.c
+++ b/arch/powerpc/platforms/83xx/mpc836x_mds.c
@@ -29,9 +29,9 @@
 #include <linux/seq_file.h>
 #include <linux/root_dev.h>
 #include <linux/initrd.h>
+#include <linux/of_platform.h>
 
 #include <asm/of_device.h>
-#include <asm/of_platform.h>
 #include <asm/system.h>
 #include <asm/atomic.h>
 #include <asm/time.h>
diff --git a/arch/powerpc/platforms/85xx/mpc85xx_mds.c b/arch/powerpc/platforms/85xx/mpc85xx_mds.c
index 61b3eed..1d81d22 100644
--- a/arch/powerpc/platforms/85xx/mpc85xx_mds.c
+++ b/arch/powerpc/platforms/85xx/mpc85xx_mds.c
@@ -30,9 +30,9 @@
 #include <linux/initrd.h>
 #include <linux/module.h>
 #include <linux/fsl_devices.h>
+#include <linux/of_platform.h>
 
 #include <asm/of_device.h>
-#include <asm/of_platform.h>
 #include <asm/system.h>
 #include <asm/atomic.h>
 #include <asm/time.h>
-- 
1.5.3

^ permalink raw reply related

* mmap question on ppc440
From: Steven A. Falco @ 2007-11-05 21:02 UTC (permalink / raw)
  To: linuxppc-dev

I am attempting to access the CPLD on the AMCC Sequoia board from 
user-land.  I open /dev/mem, and mmap it, then try to access the 
resulting pointer.  That works fine when accessing physical addresses 
that correspond to RAM, but as soon as I try to access the CPLD at 
physical address 0xc0000000, I get an infinite machine check.

I've done this successfully on the ARM architecture, and I've found 
examples where people do this on PPC, so I would expect this to work.

Here are a few successful reads:

bash-3.00# ./mm -r -a 0
paddr 00000000, map_base 0x30018000
00000000: c0348200

bash-3.00# ./mm -r -a 100
paddr 00000000, map_base 0x30018100
00000100: 7c0004ac

And here is the machine check:

bash-3.00# ./mm -r -a c0000000
paddr c0000000, Machine check in kernel mode.
Machine check in kernel mode.
Machine check in kernel mode.
Machine check in kernel mode.

My code looks like this (I'll post the whole program if anybody wants to 
see it:

        if((fd = open("/dev/mem", O_RDWR | O_SYNC)) == -1) {
                fprintf(stderr, "cannot open\n");
                exit(1);
        }
        offset = addr & MAP_MASK;
        paddr = addr & ~MAP_MASK;
        map_base = (unsigned long *)mmap(NULL, MAP_SIZE,
                        PROT_READ | PROT_WRITE, MAP_SHARED, fd, paddr);

Is it possible to access devices like this from user-land?  If so, what 
am I doing wrong?

    Thanks,
    Steve

^ permalink raw reply

* Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
From: Scott Wood @ 2007-11-05 20:51 UTC (permalink / raw)
  To: Jon Smirl; +Cc: Tjernlund, linuxppc-dev, Jean Delvare, i2c
In-Reply-To: <9e4733910711051230w2d90a710idec3dcfc2e0f5c16@mail.gmail.com>

Jon Smirl wrote:
> On 11/5/07, Scott Wood <scottwood@freescale.com> wrote:
>> Jon Smirl wrote:
>>> This is my first pass at reworking the Freescale i2c driver. It
>>> switches the driver from being a platform driver to an open firmware
>>> one. I've checked it out on my hardware and it is working.
>> We may want to hold off on this until arch/ppc goes away (or at least
>> all users of this driver in arch/ppc).
> 
> How about renaming the old driver file and leaving it hooked to ppc?
> Then it would get deleted when ppc goes away. That would let work
> progress on the powerpc version.

Or we could have one driver that has two probe methods.  I don't like 
forking the driver.

> I'm working on this because I'm adding codecs under powerpc that use
> i2c and I want consistent device tree use.

We already support i2c clients in the device tree, via the code in 
fsl_soc.c.

>>>       cell-index = <1>;
>> What is cell-index for?
> 
> I was using it to control the bus number, is that the wrong attribute?

It shouldn't be specified at all -- the hardware has no concept of a 
device number.

>>>       rtc@32 {
>>>               device_type = "rtc";
>> This isn't really necessary.
> 
> Code doesn't access it. I copied it from a pre-existing device tree.

I'm just trying to keep the damage from spreading. :-)

>>> One side effect is that legacy style i2c drivers won't work anymore.
>> If you mean legacy-style client drivers, why not?
> 
> i2c_new_device() doesn't work with legacy-style client drivers.

No, but they should still work the old way.

>>> Or push these strings into the chip drivers and modify
>>> i2c-core to also match on the device tree style names.
>> That would be ideal; it just hasn't been done yet.
> 
> This is not hard to do but the i2c people will have to agree. I need
> to change the i2c_driver structure to include the additional names.

I got a fair bit of resistance from them on the topic of multiple match 
names for i2c clients.

>> Most of this code should be made generic and placed in drivers/of.
> 
> How so, it is specific to adding i2c drivers.

I meant generic with respect to the type of i2c controller, not generic 
to all device types. :-)

It could be drivers/of/i2c.c, or drivers/i2c/of.c, etc.

>> We might as well just use i2c_new_device() instead of messing around
>> with bus numbers.  Note that this is technically no longer platform
>> code, so it's harder to justify claiming the static numberspace.
> 
> I was allowing control of the bus number with "cell-index" and
> i2c_add_numbered_adapter().
> Should I get rid of this and switch to i2c_add_adapter()?

Yes.

>>> +     i2c->base = ioremap((phys_addr_t)mem.start, MPC_I2C_REGION);
>>>       if (!i2c->base) {
>>>               printk(KERN_ERR "i2c-mpc - failed to map controller\n");
>> Use of_iomap().
> 
> I didn't write this, how should it be done? MPC_I2C_REGION can be
> eliminated by using mem.start - mem.end.

i2c->base = of_iomap(op->node, 0);

of_address_to_resource() and ioremap() are combined into one.

>> Let's take this opportunity to stop matching on device_type here
>> (including updating booting-without-of.txt).
> 
> Remove the .type line and leave .compatible?

Yes.

>>> +static struct of_platform_driver mpc_i2c_driver = {
>>> +     .owner          = THIS_MODULE,
>>> +     .name           = DRV_NAME,
>>> +     .match_table    = mpc_i2c_of_match,
>>> +     .probe          = mpc_i2c_probe,
>>> +     .remove         = __devexit_p(mpc_i2c_remove),
>>> +     .driver         = {
>>> +             .name   = DRV_NAME,
>>>       },
>>>  };
>> Do we still need .name if we have .driver.name?
> 
> Yes, i2c-core is matching on mpc_i2c_driver.name, i2c-core would need
> to be changed to use mpc_i2c_driver.driver.name.

How can i2c-core be matching on a field in an OF-specific struct?

-Scott

^ permalink raw reply

* Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
From: Jon Smirl @ 2007-11-05 20:41 UTC (permalink / raw)
  To: i2c, linuxppc-dev; +Cc: Tjernlund, Jean Delvare
In-Reply-To: <9e4733910711050714l2aa3a5eeqf5327c3e0d8ca490@mail.gmail.com>

On 11/5/07, Jon Smirl <jonsmirl@gmail.com> wrote:
> This is my first pass at reworking the Freescale i2c driver. It
> switches the driver from being a platform driver to an open firmware
> one. I've checked it out on my hardware and it is working.

Is there any way to have the i2c chip modules match on the device tree
and be automatically loaded? This is complicated by the fact that
these modules are used on other platforms.

If there is a way to do this for the i2c bus I can apply the same code
to the audio bus as well.

-- 
Jon Smirl
jonsmirl@gmail.com

^ permalink raw reply

* Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
From: Jon Smirl @ 2007-11-05 20:30 UTC (permalink / raw)
  To: Scott Wood; +Cc: Tjernlund, linuxppc-dev, Jean Delvare, i2c
In-Reply-To: <472F7247.9070106@freescale.com>

On 11/5/07, Scott Wood <scottwood@freescale.com> wrote:
> Jon Smirl wrote:
> > This is my first pass at reworking the Freescale i2c driver. It
> > switches the driver from being a platform driver to an open firmware
> > one. I've checked it out on my hardware and it is working.
>
> We may want to hold off on this until arch/ppc goes away (or at least
> all users of this driver in arch/ppc).

How about renaming the old driver file and leaving it hooked to ppc?
Then it would get deleted when ppc goes away. That would let work
progress on the powerpc version.

I'm working on this because I'm adding codecs under powerpc that use
i2c and I want consistent device tree use.

> > You can specific i2c devices on a bus by adding child nodes for them
> > in the device tree. It does not know how to autoload the i2c chip
> > driver modules.
> >
> > i2c@3d40 {
> >       device_type = "i2c";
> >       compatible = "mpc5200b-i2c\0mpc5200-i2c\0fsl-i2c";
>
> dtc supports the "mpc5200b-i2c", "mpc5200-i2c", "fsl-i2c" syntax.
>
> >       cell-index = <1>;
>
> What is cell-index for?

I was using it to control the bus number, is that the wrong attribute?

> >       fsl5200-clocking;
>
> As Matt pointed out, this is redundant.
>
> >       rtc@32 {
> >               device_type = "rtc";
>
> This isn't really necessary.

Code doesn't access it. I copied it from a pre-existing device tree.

> > One side effect is that legacy style i2c drivers won't work anymore.
>
> If you mean legacy-style client drivers, why not?

i2c_new_device() doesn't work with legacy-style client drivers.

>
> > The driver contains a table for mapping device tree style names to
> > linux kernel ones.
>
> Can we please put this in common code instead?
>
> > +static struct i2c_driver_device i2c_devices[] = {
> > +     {"ricoh,rs5c372a", "rtc-rs5c372", "rs5c372a",},
> > +     {"ricoh,rs5c372b", "rtc-rs5c372", "rs5c372b",},
> > +     {"ricoh,rv5c386",  "rtc-rs5c372", "rv5c386",},
> > +     {"ricoh,rv5c387a", "rtc-rs5c372", "rv5c387a",},
> > +     {"epson,pcf8564", "rtc-pcf8563", "pcf8564",},
> > +};
>
> At the very least, include the entries that are already being used with
> this driver in fsl_soc.c.

This is the same table from fsl_soc.c it has been moved. The data
really belongs in the i2c drivers if I can figure out how to get it
there.


> > I'd like to get rid of this table.  There are two obvious solutions,
> > use names in the device tree that match up with the linux device
> > driver names.
>
> This was proposed and rejected a while ago.  For one thing, some drivers
> handle multiple chips, and we shouldn't base device tree bindings on
> what groupings Linux happens to make in what driver supports what.
>
> > Or push these strings into the chip drivers and modify
> > i2c-core to also match on the device tree style names.
>
> That would be ideal; it just hasn't been done yet.

This is not hard to do but the i2c people will have to agree. I need
to change the i2c_driver structure to include the additional names.

> A middle ground for now would be to keep one table in drivers/of or
> something.
>
> > Without one of
> > these changes the table is going to need a mapping for every i2c
> > device on the market.
>
> Nah, only one for every i2c device Linux supports. :-)
>
> > diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
> > index 1cf29c9..6f80216 100644
> > --- a/arch/powerpc/sysdev/fsl_soc.c
> > +++ b/arch/powerpc/sysdev/fsl_soc.c
> > @@ -306,122 +306,6 @@ err:
> >
> >  arch_initcall(gfar_of_init);
> >
> > -#ifdef CONFIG_I2C_BOARDINFO
> > -#include <linux/i2c.h>
> > -struct i2c_driver_device {
> > -     char    *of_device;
> > -     char    *i2c_driver;
> > -     char    *i2c_type;
> > -};
> > -
> > -static struct i2c_driver_device i2c_devices[] __initdata = {
> > -     {"ricoh,rs5c372a", "rtc-rs5c372", "rs5c372a",},
> > -     {"ricoh,rs5c372b", "rtc-rs5c372", "rs5c372b",},
> > -     {"ricoh,rv5c386",  "rtc-rs5c372", "rv5c386",},
> > -     {"ricoh,rv5c387a", "rtc-rs5c372", "rv5c387a",},
> > -};
>
> This is obviously not based on head-of-tree -- there are several more
> entries in this table.

It is based on 2.6.23. Head of tree hasn't been working very well for
me. I'll rebase it when I can get things working again.

> > diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> > index d8de4ac..5ceb936 100644
> > --- a/drivers/i2c/busses/i2c-mpc.c
> > +++ b/drivers/i2c/busses/i2c-mpc.c
> > @@ -17,7 +17,7 @@
> >  #include <linux/module.h>
> >  #include <linux/sched.h>
> >  #include <linux/init.h>
> > -#include <linux/platform_device.h>
> > +#include <asm/of_platform.h>
>
> Should be linux/of_platform.h
changed
>
> > @@ -180,7 +182,7 @@ static void mpc_i2c_stop(struct mpc_i2c *i2c)
> >  static int mpc_write(struct mpc_i2c *i2c, int target,
> >                    const u8 * data, int length, int restart)
> >  {
> > -     int i;
> > +     int i, result;
> >       unsigned timeout = i2c->adap.timeout;
> >       u32 flags = restart ? CCR_RSTA : 0;
> >
> > @@ -192,15 +194,15 @@ static int mpc_write(struct mpc_i2c *i2c, int target,
> >       /* Write target byte */
> >       writeb((target << 1), i2c->base + MPC_I2C_DR);
> >
> > -     if (i2c_wait(i2c, timeout, 1) < 0)
> > -             return -1;
> > +     if ((result = i2c_wait(i2c, timeout, 1)) < 0)
> > +             return result;
> >
> >       for (i = 0; i < length; i++) {
> >               /* Write data byte */
> >               writeb(data[i], i2c->base + MPC_I2C_DR);
> >
> > -             if (i2c_wait(i2c, timeout, 1) < 0)
> > -                     return -1;
> > +             if ((result = i2c_wait(i2c, timeout, 1)) < 0)
> > +                     return result;
> >       }
> >
> >       return 0;
>
> This is a separate change from the OF-ization -- at least note it in the
> changelog.
done
>
> > +     for (i = 0; i < ARRAY_SIZE(i2c_devices); i++) {
> > +             if (!of_device_is_compatible(node, i2c_devices[i].of_device))
> > +                     continue;
> > +             strncpy(info->driver_name, i2c_devices[i].i2c_driver, KOBJ_NAME_LEN);
> > +             strncpy(info->type, i2c_devices[i].i2c_type, I2C_NAME_SIZE);
> > +             printk("i2c driver is %s\n", info->driver_name);
>
> Should be KERN_DEBUG, if it stays at all.
deleted
>
> > +static void of_register_i2c_devices(struct i2c_adapter *adap, struct
> > device_node *adap_node)
> > +{
> > +     struct device_node *node = NULL;
> > +
> > +     while ((node = of_get_next_child(adap_node, node))) {
> > +             struct i2c_board_info info;
> > +             const u32 *addr;
> > +             int len;
> > +
> > +             addr = of_get_property(node, "reg", &len);
> > +             if (!addr || len < sizeof(int) || *addr > (1 << 10) - 1) {
> > +                     printk(KERN_WARNING "i2c-mpc.c: invalid i2c device entry\n");
> > +                     continue;
> > +             }
> > +
> > +             info.irq = irq_of_parse_and_map(node, 0);
> > +             if (info.irq == NO_IRQ)
> > +                     info.irq = -1;
> > +
> > +             if (of_find_i2c_driver(node, &info) < 0)
> > +                     continue;
> > +
> > +             info.platform_data = NULL;
> > +             info.addr = *addr;
> > +
> > +             i2c_new_device(adap, &info);
> > +     }
> > +}
>
> Most of this code should be made generic and placed in drivers/of.

How so, it is specific to adding i2c drivers.

>
> > +     index = of_get_property(op->node, "cell-index", NULL);
> > +    if (!index || *index > 5) {
> > +            printk(KERN_ERR "mpc_i2c_probe: Device node %s has invalid "
> > +                            "cell-index property\n", op->node->full_name);
> > +            return -EINVAL;
> > +    }
> > +
>
> We might as well just use i2c_new_device() instead of messing around
> with bus numbers.  Note that this is technically no longer platform
> code, so it's harder to justify claiming the static numberspace.

I was allowing control of the bus number with "cell-index" and
i2c_add_numbered_adapter().
Should I get rid of this and switch to i2c_add_adapter()?

>
> > +     i2c->base = ioremap((phys_addr_t)mem.start, MPC_I2C_REGION);
> >       if (!i2c->base) {
> >               printk(KERN_ERR "i2c-mpc - failed to map controller\n");
>
> Use of_iomap().

I didn't write this, how should it be done? MPC_I2C_REGION can be
eliminated by using mem.start - mem.end.

>
> >       if (i2c->irq != 0)
>
> if (i2c->irq != NO_IRQ)
>
> > +static struct of_device_id mpc_i2c_of_match[] = {
> > +     {
> > +             .type           = "i2c",
> > +             .compatible     = "fsl-i2c",
> > +     },
> > +};
> > +MODULE_DEVICE_TABLE(of, mpc_i2c_of_match);
>
> Let's take this opportunity to stop matching on device_type here
> (including updating booting-without-of.txt).

Remove the .type line and leave .compatible?

>
> > +static struct of_platform_driver mpc_i2c_driver = {
> > +     .owner          = THIS_MODULE,
> > +     .name           = DRV_NAME,
> > +     .match_table    = mpc_i2c_of_match,
> > +     .probe          = mpc_i2c_probe,
> > +     .remove         = __devexit_p(mpc_i2c_remove),
> > +     .driver         = {
> > +             .name   = DRV_NAME,
> >       },
> >  };
>
> Do we still need .name if we have .driver.name?

Yes, i2c-core is matching on mpc_i2c_driver.name, i2c-core would need
to be changed to use mpc_i2c_driver.driver.name.

>
> > diff --git a/drivers/rtc/rtc-pcf8563.c b/drivers/rtc/rtc-pcf8563.c
> > index 0242d80..b778d35 100644
> > --- a/drivers/rtc/rtc-pcf8563.c
> > +++ b/drivers/rtc/rtc-pcf8563.c
>
> This should be a separate patch.

It will be in final form.

>
> -Scott
>

-- 
Jon Smirl
jonsmirl@gmail.com

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox