* Re: [PATCH RFC 0/3] Implement refcounting for OF GPIO chips
From: Anton Vorontsov @ 2010-02-09 19:06 UTC (permalink / raw)
To: Michal Simek
Cc: Michal Simek, David Brownell, devicetree-discuss, linux-kernel,
linuxppc-dev, microblaze-uclinux, David Miller
In-Reply-To: <4B712DA5.4000802@petalogix.com>
On Tue, Feb 09, 2010 at 10:40:53AM +0100, Michal Simek wrote:
[...]
> >The above situation is hard to trigger, but the issue is there
> >nonetheless, and so needs fixing.
>
>
> I tested xilinx gpio driver, heartbeat trigger and access through
> sysfs and I haven't found any problem. There is only small part of
> code for Microblaze and it is the same with PowerPC. If is Ben OK
> with it, I am ok too.
Thanks for testing!
--
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2
^ permalink raw reply
* Re: [PATCH v4 3/3] powerpc: doc/dts-bindings: update doc of FSL I2C bindings
From: Wolfgang Grandegger @ 2010-02-09 18:57 UTC (permalink / raw)
To: Grant Likely
Cc: Ben Dooks, Devicetree-discuss, Linuxppc-dev, Linux-i2c,
Wolfgang Grandegger
In-Reply-To: <fa686aa41002090950w2c3cd008k47784089dd3417d7@mail.gmail.com>
Hi Grant,
Grant Likely wrote:
> On Thu, Jan 28, 2010 at 6:25 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>> From: Wolfgang Grandegger <wg@denx.de>
>>
>> This patch adds the MPC5121 to the list of supported devices,
>> enhances the doc of the "clock-frequency" property and removes
>> the obsolete "cell-index" property from the example nodes.
>> Furthermore and example for the MPC5121 has been added.
>>
>> Signed-off-by: Wolfgang Grandegger <wg@denx.de>
>
> Thanks Wolfgang. Comments below.
>
>> ---
>> Documentation/powerpc/dts-bindings/fsl/i2c.txt | 30 +++++++++++++++++++----
>> 1 files changed, 24 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/powerpc/dts-bindings/fsl/i2c.txt b/Documentation/powerpc/dts-bindings/fsl/i2c.txt
>> index b6d2e21..2f62dae 100644
>> --- a/Documentation/powerpc/dts-bindings/fsl/i2c.txt
>> +++ b/Documentation/powerpc/dts-bindings/fsl/i2c.txt
>> @@ -9,8 +9,9 @@ Recommended properties :
>>
>> - compatible : compatibility list with 2 entries, the first should
>> be "fsl,CHIP-i2c" where CHIP is the name of a compatible processor,
>> - e.g. mpc8313, mpc8543, mpc8544, mpc5200 or mpc5200b. The second one
>> - should be "fsl-i2c".
>> + e.g. mpc8313, mpc8543, mpc8544, mpc5121, mpc5200 or mpc5200b. The
>> + second one should be "fsl-i2c". For the mpc5121, an additional node
>> + "fsl,mpc5121-i2c-ctrl" is required as shown in the example below.
>
> While you're editing this line; drop the requirement for the second
> value to be 'fsl-i2c'. We don't use it anymore, and only preserve it
> for backwards compatibility with old trees.
OK.
>> - interrupts : <a b> where a is the interrupt number and b is a
>> field that represents an encoding of the sense and level
>> information for the interrupt. This should be encoded based on
>> @@ -20,29 +21,46 @@ Recommended properties :
>> services interrupts for this device.
>> - fsl,preserve-clocking : boolean; if defined, the clock settings
>> from the bootloader are preserved (not touched).
>> - - clock-frequency : desired I2C bus clock frequency in Hz.
>> + - clock-frequency : desired I2C bus clock frequency in Hz. If this
>> + property and "fsl,preserve-clocking" is not defined, a safe fixed
>> + clock divider value is used (resulting in a small clock frequency).
>
> Nah, leave this as is. Don't make it sound like omitting both
> properties is a valid option. The driver may (and should!) handle the
> situation gracefully, but that fact does not need to be documented.
The safe value is not a good choice, indeed. Then it will also change
MPC_I2C_CLOCK_SAFE to MPC_I2C_CLOCK_LEGACY in i2c-mpc.c.
I will also fix the other issues you commented on.
Thanks,
Wolfgang.
^ permalink raw reply
* Re: [PATCH v4 3/3] powerpc: doc/dts-bindings: update doc of FSL I2C bindings
From: Grant Likely @ 2010-02-09 17:50 UTC (permalink / raw)
To: Wolfgang Grandegger
Cc: Ben Dooks, Devicetree-discuss, Linuxppc-dev, Linux-i2c,
Wolfgang Grandegger
In-Reply-To: <1264685141-26391-4-git-send-email-wg@grandegger.com>
On Thu, Jan 28, 2010 at 6:25 AM, Wolfgang Grandegger <wg@grandegger.com> wr=
ote:
> From: Wolfgang Grandegger <wg@denx.de>
>
> This patch adds the MPC5121 to the list of supported devices,
> enhances the doc of the "clock-frequency" property and removes
> the obsolete "cell-index" property from the example nodes.
> Furthermore and example for the MPC5121 has been added.
>
> Signed-off-by: Wolfgang Grandegger <wg@denx.de>
Thanks Wolfgang. Comments below.
> ---
> =A0Documentation/powerpc/dts-bindings/fsl/i2c.txt | =A0 30 ++++++++++++++=
+++++----
> =A01 files changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/powerpc/dts-bindings/fsl/i2c.txt b/Documentati=
on/powerpc/dts-bindings/fsl/i2c.txt
> index b6d2e21..2f62dae 100644
> --- a/Documentation/powerpc/dts-bindings/fsl/i2c.txt
> +++ b/Documentation/powerpc/dts-bindings/fsl/i2c.txt
> @@ -9,8 +9,9 @@ Recommended properties :
>
> =A0- compatible : compatibility list with 2 entries, the first should
> =A0 =A0be "fsl,CHIP-i2c" where CHIP is the name of a compatible processor=
,
> - =A0 e.g. mpc8313, mpc8543, mpc8544, mpc5200 or mpc5200b. The second one
> - =A0 should be "fsl-i2c".
> + =A0 e.g. mpc8313, mpc8543, mpc8544, mpc5121, mpc5200 or mpc5200b. The
> + =A0 second one should be "fsl-i2c". For the mpc5121, an additional node
> + =A0 "fsl,mpc5121-i2c-ctrl" is required as shown in the example below.
While you're editing this line; drop the requirement for the second
value to be 'fsl-i2c'. We don't use it anymore, and only preserve it
for backwards compatibility with old trees.
> =A0- interrupts : <a b> where a is the interrupt number and b is a
> =A0 =A0field that represents an encoding of the sense and level
> =A0 =A0information for the interrupt. =A0This should be encoded based on
> @@ -20,29 +21,46 @@ Recommended properties :
> =A0 =A0services interrupts for this device.
> =A0- fsl,preserve-clocking : boolean; if defined, the clock settings
> =A0 =A0from the bootloader are preserved (not touched).
> - - clock-frequency : desired I2C bus clock frequency in Hz.
> + - clock-frequency : desired I2C bus clock frequency in Hz. =A0If this
> + =A0 property and "fsl,preserve-clocking" is not defined, a safe fixed
> + =A0 clock divider value is used (resulting in a small clock frequency).
Nah, leave this as is. Don't make it sound like omitting both
properties is a valid option. The driver may (and should!) handle the
situation gracefully, but that fact does not need to be documented.
>
> =A0Examples :
>
> + =A0 =A0 =A0 /* MPC5121 based board */
> + =A0 =A0 =A0 i2c@1740 {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 #address-cells =3D <1>;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 #size-cells =3D <0>;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 compatible =3D "fsl,mpc5121-i2c", "fsl-i2c"=
;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 reg =3D <0x1740 0x20>;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 interrupts =3D <11 0x8>;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 interrupt-parent =3D <&ipic>;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 clock-frequency =3D <100000>;
> + =A0 =A0 =A0 };
> +
> + =A0 =A0 =A0 i2ccontrol@1760 {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 compatible =3D "fsl,mpc5121-i2c-ctrl";
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 reg =3D <0x1760 0x8>;
> + =A0 =A0 =A0 };
> +
> + =A0 =A0 =A0 /* MPC5200B based board */
> =A0 =A0 =A0 =A0i2c@3d00 {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0#address-cells =3D <1>;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0#size-cells =3D <0>;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0compatible =3D "fsl,mpc5200b-i2c","fsl,mpc=
5200-i2c","fsl-i2c";
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 cell-index =3D <0>;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0reg =3D <0x3d00 0x40>;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0interrupts =3D <2 15 0>;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0interrupt-parent =3D <&mpc5200_pic>;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0fsl,preserve-clocking;
> =A0 =A0 =A0 =A0};
>
> + =A0 =A0 =A0 /* MPC8544 base board */
> =A0 =A0 =A0 =A0i2c@3100 {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0#address-cells =3D <1>;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0#size-cells =3D <0>;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 cell-index =3D <1>;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0compatible =3D "fsl,mpc8544-i2c", "fsl-i2c=
";
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0reg =3D <0x3100 0x100>;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0interrupts =3D <43 2>;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0interrupt-parent =3D <&mpic>;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0clock-frequency =3D <400000>;
> =A0 =A0 =A0 =A0};
> -
> --
> 1.6.2.5
>
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
>
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [PATCH v4 2/3] i2c-mpc: add support for the MPC512x processors from Freescale
From: Grant Likely @ 2010-02-09 17:45 UTC (permalink / raw)
To: Wolfgang Grandegger
Cc: Ben Dooks, Devicetree-discuss, Linuxppc-dev, Linux-i2c,
Wolfgang Grandegger
In-Reply-To: <1264685141-26391-3-git-send-email-wg@grandegger.com>
On Thu, Jan 28, 2010 at 6:25 AM, Wolfgang Grandegger <wg@grandegger.com> wr=
ote:
> From: Wolfgang Grandegger <wg@denx.de>
>
> The "setclock" initialization functions have been renamed to "setup"
> because I2C interrupts must be enabled for the MPC512x. This requires
> to handle "fsl,preserve-clocking" in a slighly different way. Also,
> the old settings are now reported calling dev_dbg(). For the MPC512x
> the clock setup function of the MPC52xx can be re-used. Furthermore,
> the Kconfig help has been updated and corrected.
Please split the rename portion into a separate patch.
Otherwise this looks pretty okay.
g.
>
> Signed-off-by: Wolfgang Grandegger <wg@denx.de>
> ---
> =A0drivers/i2c/busses/Kconfig =A0 | =A0 =A07 +-
> =A0drivers/i2c/busses/i2c-mpc.c | =A0127 ++++++++++++++++++++++++++++++--=
----------
> =A02 files changed, 94 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 5f318ce..5477e41 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -418,13 +418,12 @@ config I2C_IXP2000
> =A0 =A0 =A0 =A0 =A0instead.
>
> =A0config I2C_MPC
> - =A0 =A0 =A0 tristate "MPC107/824x/85xx/52xx/86xx"
> + =A0 =A0 =A0 tristate "MPC107/824x/85xx/512x/52xx/83xx/86xx"
> =A0 =A0 =A0 =A0depends on PPC32
> =A0 =A0 =A0 =A0help
> =A0 =A0 =A0 =A0 =A0If you say yes to this option, support will be include=
d for the
> - =A0 =A0 =A0 =A0 built-in I2C interface on the MPC107/Tsi107/MPC8240/MPC=
8245 and
> - =A0 =A0 =A0 =A0 MPC85xx/MPC8641 family processors. The driver may also =
work on 52xx
> - =A0 =A0 =A0 =A0 family processors, though interrupts are known not to w=
ork.
> + =A0 =A0 =A0 =A0 built-in I2C interface on the MPC107, Tsi107, MPC512x, =
MPC52xx,
> + =A0 =A0 =A0 =A0 MPC8240, MPC8245, MPC83xx, MPC85xx and MPC8641 family p=
rocessors.
>
> =A0 =A0 =A0 =A0 =A0This driver can also be built as a module. =A0If so, t=
he module
> =A0 =A0 =A0 =A0 =A0will be called i2c-mpc.
> diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> index 275ebe6..1af0730 100644
> --- a/drivers/i2c/busses/i2c-mpc.c
> +++ b/drivers/i2c/busses/i2c-mpc.c
> @@ -31,6 +31,9 @@
>
> =A0#define DRV_NAME "mpc-i2c"
>
> +#define MPC_I2C_CLOCK_SAFE =A0 =A0 0
> +#define MPC_I2C_CLOCK_PRESERVE (~0U)
> +
> =A0#define MPC_I2C_FDR =A0 0x04
> =A0#define MPC_I2C_CR =A0 =A00x08
> =A0#define MPC_I2C_SR =A0 =A00x0c
> @@ -67,9 +70,8 @@ struct mpc_i2c_divider {
> =A0};
>
> =A0struct mpc_i2c_data {
> - =A0 =A0 =A0 void (*setclock)(struct device_node *node,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct mpc_i2c *i2c,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0u32 clock, u32 prescaler=
);
> + =A0 =A0 =A0 void (*setup)(struct device_node *node, struct mpc_i2c *i2c=
,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 u32 clock, u32 prescaler);
> =A0 =A0 =A0 =A0u32 prescaler;
> =A0};
>
> @@ -164,7 +166,7 @@ static int i2c_wait(struct mpc_i2c *i2c, unsigned tim=
eout, int writing)
> =A0 =A0 =A0 =A0return 0;
> =A0}
>
> -#ifdef CONFIG_PPC_MPC52xx
> +#if defined(CONFIG_PPC_MPC52xx) || defined(CONFIG_PPC_MPC512x)
> =A0static const struct __devinitdata mpc_i2c_divider mpc_i2c_dividers_52x=
x[] =3D {
> =A0 =A0 =A0 =A0{20, 0x20}, {22, 0x21}, {24, 0x22}, {26, 0x23},
> =A0 =A0 =A0 =A0{28, 0x24}, {30, 0x01}, {32, 0x25}, {34, 0x02},
> @@ -216,12 +218,18 @@ static int __devinit mpc_i2c_get_fdr_52xx(struct de=
vice_node *node, u32 clock,
> =A0 =A0 =A0 =A0return div ? (int)div->fdr : -EINVAL;
> =A0}
>
> -static void __devinit mpc_i2c_setclock_52xx(struct device_node *node,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0 =A0 struct mpc_i2c *i2c,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0 =A0 u32 clock, u32 prescaler)
> +static void __devinit mpc_i2c_setup_52xx(struct device_node *node,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0struct mpc_i2c *i2c,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0u32 clock, u32 prescaler)
> =A0{
> =A0 =A0 =A0 =A0int ret, fdr;
>
> + =A0 =A0 =A0 if (clock =3D=3D MPC_I2C_CLOCK_PRESERVE) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_dbg(i2c->dev, "using fdr %d\n",
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 readb(i2c->base + MPC_I2C_F=
DR));
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return;
> + =A0 =A0 =A0 }
> +
> =A0 =A0 =A0 =A0ret =3D mpc_i2c_get_fdr_52xx(node, clock, prescaler);
> =A0 =A0 =A0 =A0fdr =3D (ret >=3D 0) ? ret : 0x3f; /* backward compatibili=
ty */
>
> @@ -230,13 +238,49 @@ static void __devinit mpc_i2c_setclock_52xx(struct =
device_node *node,
> =A0 =A0 =A0 =A0if (ret >=3D 0)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dev_info(i2c->dev, "clock %d Hz (fdr=3D%d)=
\n", clock, fdr);
> =A0}
> -#else /* !CONFIG_PPC_MPC52xx */
> -static void __devinit mpc_i2c_setclock_52xx(struct device_node *node,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0 =A0 struct mpc_i2c *i2c,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0 =A0 u32 clock, u32 prescaler)
> +#else /* !(CONFIG_PPC_MPC52xx || CONFIG_PPC_MPC512x) */
> +static void __devinit mpc_i2c_setup_52xx(struct device_node *node,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0struct mpc_i2c *i2c,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0u32 clock, u32 prescaler)
> +{
> +}
> +#endif /* CONFIG_PPC_MPC52xx || CONFIG_PPC_MPC512x */
> +
> +#ifdef CONFIG_PPC_MPC512x
> +static void __devinit mpc_i2c_setup_512x(struct device_node *node,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0struct mpc_i2c *i2c,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0u32 clock, u32 prescaler)
> +{
> + =A0 =A0 =A0 struct device_node *node_ctrl;
> + =A0 =A0 =A0 void __iomem *ctrl;
> + =A0 =A0 =A0 const u32 *pval;
> + =A0 =A0 =A0 u32 idx;
> +
> + =A0 =A0 =A0 /* Enable I2C interrupts for mpc5121 */
> + =A0 =A0 =A0 node_ctrl =3D of_find_compatible_node(NULL, NULL,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0 =A0 "fsl,mpc5121-i2c-ctrl");
> + =A0 =A0 =A0 if (node_ctrl) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 ctrl =3D of_iomap(node_ctrl, 0);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (ctrl) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Interrupt enable bits fo=
r i2c-0/1/2: bit 24/26/28 */
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 pval =3D of_get_property(no=
de, "reg", NULL);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 idx =3D (*pval & 0xff) / 0x=
20;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 setbits32(ctrl, 1 << (24 + =
idx * 2));
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 iounmap(ctrl);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 of_node_put(node_ctrl);
> + =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 /* The clock setup for the 52xx works also fine for the 512=
x */
> + =A0 =A0 =A0 mpc_i2c_setup_52xx(node, i2c, clock, prescaler);
> +}
> +#else /* CONFIG_PPC_MPC512x */
> +static void __devinit mpc_i2c_setup_512x(struct device_node *node,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0struct mpc_i2c *i2c,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0u32 clock, u32 prescaler)
> =A0{
> =A0}
> -#endif /* CONFIG_PPC_MPC52xx*/
> +#endif /* CONFIG_PPC_MPC512x */
>
> =A0#ifdef CONFIG_FSL_SOC
> =A0static const struct __devinitdata mpc_i2c_divider mpc_i2c_dividers_8xx=
x[] =3D {
> @@ -322,12 +366,19 @@ static int __devinit mpc_i2c_get_fdr_8xxx(struct de=
vice_node *node, u32 clock,
> =A0 =A0 =A0 =A0return div ? (int)div->fdr : -EINVAL;
> =A0}
>
> -static void __devinit mpc_i2c_setclock_8xxx(struct device_node *node,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0 =A0 struct mpc_i2c *i2c,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0 =A0 u32 clock, u32 prescaler)
> +static void __devinit mpc_i2c_setup_8xxx(struct device_node *node,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0struct mpc_i2c *i2c,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0u32 clock, u32 prescaler)
> =A0{
> =A0 =A0 =A0 =A0int ret, fdr;
>
> + =A0 =A0 =A0 if (clock =3D=3D MPC_I2C_CLOCK_PRESERVE) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_dbg(i2c->dev, "using dfsrr %d, fdr %d\n=
",
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 readb(i2c->base + MPC_I2C_D=
FSRR),
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 readb(i2c->base + MPC_I2C_F=
DR));
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return;
> + =A0 =A0 =A0 }
> +
> =A0 =A0 =A0 =A0ret =3D mpc_i2c_get_fdr_8xxx(node, clock, prescaler);
> =A0 =A0 =A0 =A0fdr =3D (ret >=3D 0) ? ret : 0x1031; /* backward compatibi=
lity */
>
> @@ -340,9 +391,9 @@ static void __devinit mpc_i2c_setclock_8xxx(struct de=
vice_node *node,
> =A0}
>
> =A0#else /* !CONFIG_FSL_SOC */
> -static void __devinit mpc_i2c_setclock_8xxx(struct device_node *node,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0 =A0 struct mpc_i2c *i2c,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0 =A0 u32 clock, u32 prescaler)
> +static void __devinit mpc_i2c_setup_8xxx(struct device_node *node,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0struct mpc_i2c *i2c,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0u32 clock, u32 prescaler)
> =A0{
> =A0}
> =A0#endif /* CONFIG_FSL_SOC */
> @@ -496,7 +547,7 @@ static int __devinit fsl_i2c_probe(struct of_device *=
op,
> =A0{
> =A0 =A0 =A0 =A0struct mpc_i2c *i2c;
> =A0 =A0 =A0 =A0const u32 *prop;
> - =A0 =A0 =A0 u32 clock =3D 0;
> + =A0 =A0 =A0 u32 clock =3D MPC_I2C_CLOCK_SAFE;
> =A0 =A0 =A0 =A0int result =3D 0;
> =A0 =A0 =A0 =A0int plen;
>
> @@ -525,21 +576,21 @@ static int __devinit fsl_i2c_probe(struct of_device=
*op,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0}
>
> - =A0 =A0 =A0 if (!of_get_property(op->node, "fsl,preserve-clocking", NUL=
L)) {
> + =A0 =A0 =A0 if (of_get_property(op->node, "fsl,preserve-clocking", NULL=
)) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 clock =3D MPC_I2C_CLOCK_PRESERVE;
> + =A0 =A0 =A0 } else {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0prop =3D of_get_property(op->node, "clock-=
frequency", &plen);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (prop && plen =3D=3D sizeof(u32))
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0clock =3D *prop;
> + =A0 =A0 =A0 }
>
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (match->data) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 struct mpc_i2c_data *data =
=3D
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (struct mpc=
_i2c_data *)match->data;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 data->setclock(op->node, i2=
c, clock, data->prescaler);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 } else {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Backwards compatibility =
*/
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (of_get_property(op->nod=
e, "dfsrr", NULL))
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 mpc_i2c_set=
clock_8xxx(op->node, i2c,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 clock, 0);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> + =A0 =A0 =A0 if (match->data) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 struct mpc_i2c_data *data =3D match->data;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 data->setup(op->node, i2c, clock, data->pre=
scaler);
> + =A0 =A0 =A0 } else {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Backwards compatibility */
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (of_get_property(op->node, "dfsrr", NULL=
))
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 mpc_i2c_setup_8xxx(op->node=
, i2c, clock, 0);
> =A0 =A0 =A0 =A0}
>
> =A0 =A0 =A0 =A0dev_set_drvdata(&op->dev, i2c);
> @@ -584,21 +635,25 @@ static int __devexit fsl_i2c_remove(struct of_devic=
e *op)
> =A0 =A0 =A0 =A0return 0;
> =A0};
>
> +static struct mpc_i2c_data __devinitdata mpc_i2c_data_512x =3D {
> + =A0 =A0 =A0 .setup =3D mpc_i2c_setup_512x,
> +};
> +
> =A0static struct mpc_i2c_data __devinitdata mpc_i2c_data_52xx =3D {
> - =A0 =A0 =A0 .setclock =3D mpc_i2c_setclock_52xx,
> + =A0 =A0 =A0 .setup =3D mpc_i2c_setup_52xx,
> =A0};
>
> =A0static struct mpc_i2c_data __devinitdata mpc_i2c_data_8313 =3D {
> - =A0 =A0 =A0 .setclock =3D mpc_i2c_setclock_8xxx,
> + =A0 =A0 =A0 .setup =3D mpc_i2c_setup_8xxx,
> =A0};
>
> =A0static struct mpc_i2c_data __devinitdata mpc_i2c_data_8543 =3D {
> - =A0 =A0 =A0 .setclock =3D mpc_i2c_setclock_8xxx,
> + =A0 =A0 =A0 .setup =3D mpc_i2c_setup_8xxx,
> =A0 =A0 =A0 =A0.prescaler =3D 2,
> =A0};
>
> =A0static struct mpc_i2c_data __devinitdata mpc_i2c_data_8544 =3D {
> - =A0 =A0 =A0 .setclock =3D mpc_i2c_setclock_8xxx,
> + =A0 =A0 =A0 .setup =3D mpc_i2c_setup_8xxx,
> =A0 =A0 =A0 =A0.prescaler =3D 3,
> =A0};
>
> @@ -606,6 +661,7 @@ static const struct of_device_id mpc_i2c_of_match[] =
=3D {
> =A0 =A0 =A0 =A0{.compatible =3D "mpc5200-i2c", .data =3D &mpc_i2c_data_52=
xx, },
> =A0 =A0 =A0 =A0{.compatible =3D "fsl,mpc5200b-i2c", .data =3D &mpc_i2c_da=
ta_52xx, },
> =A0 =A0 =A0 =A0{.compatible =3D "fsl,mpc5200-i2c", .data =3D &mpc_i2c_dat=
a_52xx, },
> + =A0 =A0 =A0 {.compatible =3D "fsl,mpc5121-i2c", .data =3D &mpc_i2c_data=
_512x, },
> =A0 =A0 =A0 =A0{.compatible =3D "fsl,mpc8313-i2c", .data =3D &mpc_i2c_dat=
a_8313, },
> =A0 =A0 =A0 =A0{.compatible =3D "fsl,mpc8543-i2c", .data =3D &mpc_i2c_dat=
a_8543, },
> =A0 =A0 =A0 =A0{.compatible =3D "fsl,mpc8544-i2c", .data =3D &mpc_i2c_dat=
a_8544, },
> @@ -615,7 +671,6 @@ static const struct of_device_id mpc_i2c_of_match[] =
=3D {
> =A0};
> =A0MODULE_DEVICE_TABLE(of, mpc_i2c_of_match);
>
> -
> =A0/* Structure for a device driver */
> =A0static struct of_platform_driver mpc_i2c_driver =3D {
> =A0 =A0 =A0 =A0.match_table =A0 =A0=3D mpc_i2c_of_match,
> @@ -648,5 +703,5 @@ module_exit(fsl_i2c_exit);
>
> =A0MODULE_AUTHOR("Adrian Cox <adrian@humboldt.co.uk>");
> =A0MODULE_DESCRIPTION("I2C-Bus adapter for MPC107 bridge and "
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"MPC824x/85xx/52xx processors");
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"MPC824x/85xx/512x/52xx processors")=
;
> =A0MODULE_LICENSE("GPL");
> --
> 1.6.2.5
>
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
>
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [PATCH v4 1/3] i2c-mpc: use __devinit[data] for initialization functions and data
From: Grant Likely @ 2010-02-09 17:41 UTC (permalink / raw)
To: Wolfgang Grandegger
Cc: Ben Dooks, Devicetree-discuss, Linuxppc-dev, Linux-i2c,
Wolfgang Grandegger
In-Reply-To: <1264685141-26391-2-git-send-email-wg@grandegger.com>
On Thu, Jan 28, 2010 at 6:25 AM, Wolfgang Grandegger <wg@grandegger.com> wr=
ote:
> From: Wolfgang Grandegger <wg@denx.de>
>
> "__devinit[data]" has not yet been used for all initialization functions
> and data. To avoid truncating lines, the struct "mpc_i2c_match_data" has
> been renamed to "mpc_i2c_data", which is even the better name.
>
> Signed-off-by: Wolfgang Grandegger <wg@denx.de>
Several comments below.
> ---
> =A0drivers/i2c/busses/i2c-mpc.c | =A0103 +++++++++++++++++++-------------=
---------
> =A01 files changed, 48 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> index f627001..275ebe6 100644
> --- a/drivers/i2c/busses/i2c-mpc.c
> +++ b/drivers/i2c/busses/i2c-mpc.c
> @@ -66,7 +66,7 @@ struct mpc_i2c_divider {
> =A0 =A0 =A0 =A0u16 fdr; =A0 =A0 =A0 =A0/* including dfsrr */
> =A0};
>
> -struct mpc_i2c_match_data {
> +struct mpc_i2c_data {
> =A0 =A0 =A0 =A0void (*setclock)(struct device_node *node,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 struct mpc_i2c *i2c,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 u32 clock, u32 prescaler)=
;
> @@ -165,7 +165,7 @@ static int i2c_wait(struct mpc_i2c *i2c, unsigned tim=
eout, int writing)
> =A0}
>
> =A0#ifdef CONFIG_PPC_MPC52xx
> -static const struct mpc_i2c_divider mpc_i2c_dividers_52xx[] =3D {
> +static const struct __devinitdata mpc_i2c_divider mpc_i2c_dividers_52xx[=
] =3D {
__devinitdata goes at the end, immediately before the '=3D'. Ditto
throughout the file.
> =A0 =A0 =A0 =A0{20, 0x20}, {22, 0x21}, {24, 0x22}, {26, 0x23},
> =A0 =A0 =A0 =A0{28, 0x24}, {30, 0x01}, {32, 0x25}, {34, 0x02},
> =A0 =A0 =A0 =A0{36, 0x26}, {40, 0x27}, {44, 0x04}, {48, 0x28},
> @@ -582,44 +584,35 @@ static int __devexit fsl_i2c_remove(struct of_devic=
e *op)
> =A0 =A0 =A0 =A0return 0;
> =A0};
>
> +static struct mpc_i2c_data __devinitdata mpc_i2c_data_52xx =3D {
> + =A0 =A0 =A0 .setclock =3D mpc_i2c_setclock_52xx,
> +};
> +
> +static struct mpc_i2c_data __devinitdata mpc_i2c_data_8313 =3D {
> + =A0 =A0 =A0 .setclock =3D mpc_i2c_setclock_8xxx,
> +};
> +
> +static struct mpc_i2c_data __devinitdata mpc_i2c_data_8543 =3D {
> + =A0 =A0 =A0 .setclock =3D mpc_i2c_setclock_8xxx,
> + =A0 =A0 =A0 .prescaler =3D 2,
> +};
> +
> +static struct mpc_i2c_data __devinitdata mpc_i2c_data_8544 =3D {
> + =A0 =A0 =A0 .setclock =3D mpc_i2c_setclock_8xxx,
> + =A0 =A0 =A0 .prescaler =3D 3,
> +};
> +
> =A0static const struct of_device_id mpc_i2c_of_match[] =3D {
You can make this __devinitdata too.
> - =A0 =A0 =A0 {.compatible =3D "mpc5200-i2c",
> - =A0 =A0 =A0 =A0.data =3D &(struct mpc_i2c_match_data) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 .setclock =3D mpc_i2c_setcl=
ock_52xx,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 },
> - =A0 =A0 =A0 },
> - =A0 =A0 =A0 {.compatible =3D "fsl,mpc5200b-i2c",
> - =A0 =A0 =A0 =A0.data =3D &(struct mpc_i2c_match_data) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 .setclock =3D mpc_i2c_setcl=
ock_52xx,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 },
> - =A0 =A0 =A0 },
> - =A0 =A0 =A0 {.compatible =3D "fsl,mpc5200-i2c",
> - =A0 =A0 =A0 =A0.data =3D &(struct mpc_i2c_match_data) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 .setclock =3D mpc_i2c_setcl=
ock_52xx,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 },
> - =A0 =A0 =A0 },
> - =A0 =A0 =A0 {.compatible =3D "fsl,mpc8313-i2c",
> - =A0 =A0 =A0 =A0.data =3D &(struct mpc_i2c_match_data) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 .setclock =3D mpc_i2c_setcl=
ock_8xxx,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 },
> - =A0 =A0 =A0 },
> - =A0 =A0 =A0 {.compatible =3D "fsl,mpc8543-i2c",
> - =A0 =A0 =A0 =A0.data =3D &(struct mpc_i2c_match_data) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 .setclock =3D mpc_i2c_setcl=
ock_8xxx,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 .prescaler =3D 2,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 },
> - =A0 =A0 =A0 },
> - =A0 =A0 =A0 {.compatible =3D "fsl,mpc8544-i2c",
> - =A0 =A0 =A0 =A0.data =3D &(struct mpc_i2c_match_data) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 .setclock =3D mpc_i2c_setcl=
ock_8xxx,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 .prescaler =3D 3,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 },
> + =A0 =A0 =A0 {.compatible =3D "mpc5200-i2c", .data =3D &mpc_i2c_data_52x=
x, },
> + =A0 =A0 =A0 {.compatible =3D "fsl,mpc5200b-i2c", .data =3D &mpc_i2c_dat=
a_52xx, },
> + =A0 =A0 =A0 {.compatible =3D "fsl,mpc5200-i2c", .data =3D &mpc_i2c_data=
_52xx, },
> + =A0 =A0 =A0 {.compatible =3D "fsl,mpc8313-i2c", .data =3D &mpc_i2c_data=
_8313, },
> + =A0 =A0 =A0 {.compatible =3D "fsl,mpc8543-i2c", .data =3D &mpc_i2c_data=
_8543, },
> + =A0 =A0 =A0 {.compatible =3D "fsl,mpc8544-i2c", .data =3D &mpc_i2c_data=
_8544, },
> =A0 =A0 =A0 =A0/* Backward compatibility */
> - =A0 =A0 =A0 },
> =A0 =A0 =A0 =A0{.compatible =3D "fsl-i2c", },
> =A0 =A0 =A0 =A0{},
> =A0};
> -
> =A0MODULE_DEVICE_TABLE(of, mpc_i2c_of_match);
>
>
> @@ -634,7 +627,7 @@ static struct of_platform_driver mpc_i2c_driver =3D {
> =A0 =A0 =A0 =A0},
> =A0};
>
> -static int __init fsl_i2c_init(void)
> +static int __devinit fsl_i2c_init(void)
No, __init was correct. This is called as a module_init() not as a .probe =
hook.
> =A0{
> =A0 =A0 =A0 =A0int rv;
>
> @@ -645,7 +638,7 @@ static int __init fsl_i2c_init(void)
> =A0 =A0 =A0 =A0return rv;
> =A0}
>
> -static void __exit fsl_i2c_exit(void)
> +static void __devexit fsl_i2c_exit(void)
Same here for __exit.
> =A0{
> =A0 =A0 =A0 =A0of_unregister_platform_driver(&mpc_i2c_driver);
> =A0}
> --
> 1.6.2.5
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [PATCH RFC 0/3] Implement refcounting for OF GPIO chips
From: Grant Likely @ 2010-02-09 17:29 UTC (permalink / raw)
To: michal.simek
Cc: Michal Simek, David Brownell, devicetree-discuss, linux-kernel,
linuxppc-dev, microblaze-uclinux, avorontsov, David Miller
In-Reply-To: <4B712DA5.4000802@petalogix.com>
On Tue, Feb 9, 2010 at 2:40 AM, Michal Simek <michal.simek@petalogix.com> w=
rote:
> Anton Vorontsov wrote:
>>
>> Hi all,
>>
>> OF GPIO infrastructure is using dynamic GPIO bases, so it is possible
>> that of_get_gpio()'s returned GPIO number will be no longer valid, or
>> worse, it may point to an unexpected GPIO controller.
>>
>> This scenario is possible:
>>
>> driver A: =A0 =A0 =A0 =A0 =A0 =A0 =A0 driver B: =A0 =A0 =A0 =A0 =A0 =A0 =
=A0driver C:
>> --------- =A0 =A0 =A0 =A0 =A0 =A0 =A0 --------- =A0 =A0 =A0 =A0 =A0 =A0 =
=A0---------
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0gpiochip_add()
>> gpio =3D of_get_gpio()
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0gpiochip_remove()
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 gpiochip_add()
>> gpio_request(gpio);
>> gpio_set_value(gpio);
>>
>> That is, driver A assumes that it is working with GPIO from driver B,
>> but in practice it may disappear and driver C will take its GPIO base
>> number, so it will provide the same GPIO numbers.
>>
>> The above situation is hard to trigger, but the issue is there
>> nonetheless, and so needs fixing.
>
>
> I tested xilinx gpio driver, heartbeat trigger and access through sysfs a=
nd
> I haven't found any problem. There is only small part of code for Microbl=
aze
> and it is the same with PowerPC. If is Ben OK with it, I am ok too.
I'm not convinced the changes take the best approach, so I'm not okay
with it yet.
g.
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [PATCH 3/3] of/gpio: Introduce of_put_gpio(), add ref counting for OF GPIO chips
From: Grant Likely @ 2010-02-09 17:28 UTC (permalink / raw)
To: Anton Vorontsov
Cc: Michal Simek, David Brownell, devicetree-discuss, linux-kernel,
linuxppc-dev, microblaze-uclinux, David Miller
In-Reply-To: <20100205205045.GC4178@oksana.dev.rtsoft.ru>
On Fri, Feb 5, 2010 at 1:50 PM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> OF GPIO infrastructure is using dynamic GPIO bases, so it is possible
> that of_get_gpio()'s returned GPIO number will be no longer valid, or
> worse, it may point to an unexpected GPIO controller.
>
> This scenario is possible:
>
> driver A: =A0 =A0 =A0 =A0 =A0 =A0 =A0 driver B: =A0 =A0 =A0 =A0 =A0 =A0 =
=A0driver C:
> --------- =A0 =A0 =A0 =A0 =A0 =A0 =A0 --------- =A0 =A0 =A0 =A0 =A0 =A0 =
=A0---------
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0gpiochip_add()
> gpio =3D of_get_gpio()
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0gpiochip_remove()
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 gpiochip_add()
> gpio_request(gpio);
> gpio_set_value(gpio);
>
> That is, driver A assumes that it is working with GPIO from driver B,
> but in practice it may disappear and driver C will take its GPIO base
> number, so it will provide the same GPIO numbers.
>
> With this patch that situation is no longer possible. Though drivers
> will need to learn to put GPIOs back, so that GPIO controllers could
> be removed.
>
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
Rather than having a lock at the device tree data pointer level which
mixes usage with potentially many other drivers; wouldn't it make more
sense to use a mutex at the of_gc subsystem context?
g.
^ permalink raw reply
* Re: [PATCH 2/3] of: Introduce safe accessors for node->data
From: Grant Likely @ 2010-02-09 17:25 UTC (permalink / raw)
To: Anton Vorontsov
Cc: Michal Simek, David Brownell, devicetree-discuss, linux-kernel,
linuxppc-dev, microblaze-uclinux, David Miller
In-Reply-To: <20100205205043.GB4178@oksana.dev.rtsoft.ru>
On Fri, Feb 5, 2010 at 1:50 PM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> Platform code use node->data to store some private information
> associated with a node.
>
> Previously there was no need for any locks and accessors since we were
> initializing the data mostly at boot time and never modified it later.
>
> Though, nowadays OF GPIO infrastructure supports GPIO chips detaching,
> so to handle this correctly we have to introduce locking for the
> node->data field.
I'm not convinced this is needed. What's wrong with using the
whole-tree devtree_lock?
g.
^ permalink raw reply
* Re: [PATCH 1/3] of platforms: Move common static initialization to of_node_init()
From: Grant Likely @ 2010-02-09 17:22 UTC (permalink / raw)
To: Anton Vorontsov
Cc: Michal Simek, David Brownell, devicetree-discuss, linux-kernel,
linuxppc-dev, microblaze-uclinux, David Miller
In-Reply-To: <20100205205041.GA4178@oksana.dev.rtsoft.ru>
On Fri, Feb 5, 2010 at 1:50 PM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> So far of_node_init() just initializes a kref, later we'll have to
> initialize other fields (for example node->data_lock).
>
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
> =A0arch/microblaze/kernel/prom.c =A0 =A0 =A0 =A0 =A0 =A0 | =A0 =A04 ++--
> =A0arch/powerpc/kernel/prom.c =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0| =A0 =A04 +=
+--
> =A0arch/powerpc/platforms/iseries/vio.c =A0 =A0 =A0| =A0 =A05 +++--
> =A0arch/powerpc/platforms/pseries/dlpar.c =A0 =A0| =A0 =A07 ++++---
> =A0arch/powerpc/platforms/pseries/reconfig.c | =A0 =A07 ++++---
> =A0arch/powerpc/sysdev/msi_bitmap.c =A0 =A0 =A0 =A0 =A0| =A0 =A04 ++--
> =A0arch/sparc/kernel/prom_common.c =A0 =A0 =A0 =A0 =A0 | =A0 =A06 +++---
> =A0drivers/of/base.c =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 | =
=A0 10 ++++++++++
> =A0include/linux/of.h =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0| =
=A0 =A02 ++
> =A09 files changed, 32 insertions(+), 17 deletions(-)
unflatten_dt_*() has been merged and moved to drivers/of. Please
rebase this series on top of my next-devicetree branch in this git
tree:
git://git.secretlab.ca/git/linux-2.6
Otherwise, this looks fine to me.
g.
^ permalink raw reply
* Re: [PATCH 1/4] gpiolib: Introduce chip addition/removal notifier
From: Grant Likely @ 2010-02-09 17:16 UTC (permalink / raw)
To: Anton Vorontsov
Cc: David Brownell, Dmitry Eremin-Solenikov, linux-kernel,
linuxppc-dev, Bill Gatliff, Andrew Morton
In-Reply-To: <20100205203232.GA1475@oksana.dev.rtsoft.ru>
On Fri, Feb 5, 2010 at 1:32 PM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> Some platforms (e.g. OpenFirmware) want to know when a particular chip
> added or removed, so that the platforms could add their specifics for
> non-platform devices, like I2C or SPI GPIO chips.
>
> This patch implements the notifier for chip addition and removal events.
>
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
> =A0drivers/gpio/gpiolib.c =A0 =A0 | =A0 14 ++++++++++++++
> =A0include/asm-generic/gpio.h | =A0 =A08 ++++++++
> =A02 files changed, 22 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 350842a..375c03a 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -9,6 +9,7 @@
> =A0#include <linux/seq_file.h>
> =A0#include <linux/gpio.h>
> =A0#include <linux/idr.h>
> +#include <linux/notifier.h>
>
>
> =A0/* Optional implementation infrastructure for GPIO interfaces.
> @@ -1029,6 +1030,9 @@ static inline void gpiochip_unexport(struct gpio_ch=
ip *chip)
>
> =A0#endif /* CONFIG_GPIO_SYSFS */
>
> +BLOCKING_NOTIFIER_HEAD(gpio_notifier);
> +EXPORT_SYMBOL_GPL(gpio_notifier);
> +
> =A0/**
> =A0* gpiochip_add() - register a gpio_chip
> =A0* @chip: the chip to register, with chip->base initialized
> @@ -1103,6 +1107,9 @@ fail:
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pr_err("gpiochip_add: gpios %d..%d (%s) no=
t registered\n",
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0chip->base, chip->base + c=
hip->ngpio - 1,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0chip->label ? : "generic")=
;
> + =A0 =A0 =A0 else
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 blocking_notifier_call_chain(&gpio_notifier=
,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0 =A0 =A0GPIO_NOTIFY_CHIP_ADDED, chip);
Rather than doing an else block which will need to be reworked if/when
any additional code is added to the bottom of this routine, please
rework the if() block to bail on failure instead of implicitly falling
through to the return statement.
Otherwise, this patch looks okay to me, so you can go ahead and add my:
Acked-by: Grant Likely <grant.likely@secretlab.ca>
*however* (and don't kill me for saying this because I know I
suggested the notifier approach in the first place). Looking at the
whole patch series, the notifier call adds a lot of code for very
little gain. If you dropped just the notifier bits (but left the rest
of the series the same), then the the of gpio bits would be
considerably simpler, and the only impact on the core gpiolib would be
the addition of an of_gpiochip_register_simple() and
of_gpiochip_unregister() hooks that will be conditionally compiled.
And to address one of my previous concerns, I've got no problem with
the automatic registration of GPIO devices for OF usage, as long as
of-aware drivers have the option of overriding the simple defaults
when needed.
g.
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [PATCH 3/4] of/gpio: Implement GPIOLIB notifier hooks
From: Grant Likely @ 2010-02-09 17:13 UTC (permalink / raw)
To: Anton Vorontsov
Cc: David Brownell, Dmitry Eremin-Solenikov, linux-kernel,
linuxppc-dev, Bill Gatliff, Andrew Morton
In-Reply-To: <20100205203236.GC1475@oksana.dev.rtsoft.ru>
On Fri, Feb 5, 2010 at 1:32 PM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> This patch implements GPIOLIB notifier hooks, and thus makes device-enabl=
ed
> GPIO chips (i.e. the ones that have gpio_chip->dev specified) automatical=
ly
> attached to the OpenFirmware subsystem. Which means that now we can handl=
e
> I2C and SPI GPIO chips almost* transparently.
>
> * "Almost" because some chips still require platform data, and for these
> =A0chips OF-glue is still needed, though with this support the glue will
> =A0be much smaller.
>
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
> +static struct notifier_block of_gpio_nb =3D {
> + =A0 =A0 =A0 .notifier_call =3D of_gpio_notify,
> +};
> +
> +static int __init of_gpio_notifier_init(void)
> +{
> + =A0 =A0 =A0 return blocking_notifier_chain_register(&gpio_notifier, &of=
_gpio_nb);
> +}
> +arch_initcall(of_gpio_notifier_init);
Another concern; if any gpio chips get registered before this
arch_initcall (not sure if it is possible or not), then those chips
won't get registered with the of gpio infrastructure.
g.
^ permalink raw reply
* Re: [PATCH 3/4] of/gpio: Implement GPIOLIB notifier hooks
From: Grant Likely @ 2010-02-09 17:08 UTC (permalink / raw)
To: Anton Vorontsov
Cc: David Brownell, Dmitry Eremin-Solenikov, linux-kernel,
linuxppc-dev, Bill Gatliff, Andrew Morton
In-Reply-To: <20100205203236.GC1475@oksana.dev.rtsoft.ru>
On Fri, Feb 5, 2010 at 1:32 PM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> This patch implements GPIOLIB notifier hooks, and thus makes device-enabl=
ed
> GPIO chips (i.e. the ones that have gpio_chip->dev specified) automatical=
ly
> attached to the OpenFirmware subsystem. Which means that now we can handl=
e
> I2C and SPI GPIO chips almost* transparently.
>
> * "Almost" because some chips still require platform data, and for these
> =A0chips OF-glue is still needed, though with this support the glue will
> =A0be much smaller.
>
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
> =A0drivers/of/gpio.c | =A0100 +++++++++++++++++++++++++++++++++++++++++++=
++++++++++
> =A01 files changed, 100 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/of/gpio.c b/drivers/of/gpio.c
> index 12c4af0..9d8df77 100644
> --- a/drivers/of/gpio.c
> +++ b/drivers/of/gpio.c
> @@ -13,6 +13,7 @@
>
> =A0#include <linux/kernel.h>
> =A0#include <linux/errno.h>
> +#include <linux/notifier.h>
> =A0#include <linux/io.h>
> =A0#include <linux/of.h>
> =A0#include <linux/of_gpio.h>
> @@ -236,3 +237,102 @@ err0:
> =A0 =A0 =A0 =A0return ret;
> =A0}
> =A0EXPORT_SYMBOL(of_mm_gpiochip_add);
> +
> +/**
> + * of_gpiochip_register_simple - Register a chip with the OF GPIO subsys=
tem
> + * @chip =A0 =A0 =A0 pointer to a GPIO chip
> + * @np: =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0device node to register the GPIO =
chip with
> + *
> + * This function registers a GPIO chip with the OF infrastructure. It is
> + * assumed that the chip was previsously allocated and added to a generi=
c
> + * GPIOLIB framework (using gpiochip_add() function).
> + *
> + * The `simple' name means that the chip is using simple two-cells schem=
e for
> + * the gpio-specifier.
> + */
> +static int of_gpiochip_register_simple(struct gpio_chip *chip,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0struct device_node *np)
> +{
> + =A0 =A0 =A0 struct of_gpio_chip *of_gc;
> +
> + =A0 =A0 =A0 if (np->data) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 WARN_ON(1);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EBUSY;
> + =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 of_gc =3D kzalloc(sizeof(*of_gc), GFP_KERNEL);
> + =A0 =A0 =A0 if (!of_gc)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -ENOMEM;
> +
> + =A0 =A0 =A0 of_gc->gpio_cells =3D 2;
> + =A0 =A0 =A0 of_gc->xlate =3D of_gpio_simple_xlate;
> + =A0 =A0 =A0 of_gc->chip =3D chip;
One concern.
How does an OF-aware GPIO driver override these settings? What is to
be done when a GPIO chip requires a different xlate hook? Or a
different number of gpio_cells?
g.
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [PATCH 3/3] powerpc: mpc5121: enable support for more PSC UARTs
From: Grant Likely @ 2010-02-09 16:20 UTC (permalink / raw)
To: Anatolij Gustschin; +Cc: linuxppc-dev, wd, dzu
In-Reply-To: <1265096864-3506-4-git-send-email-agust@denx.de>
On Tue, Feb 2, 2010 at 12:47 AM, Anatolij Gustschin <agust@denx.de> wrote:
> MPC5121 has 12 PSC devices. Enable UART support for all of
> them by defining the number of max. PSCs depending on
> selection of PPC_MPC512x platform support.
>
> Signed-off-by: Anatolij Gustschin <agust@denx.de>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> ---
> =A0arch/powerpc/include/asm/mpc52xx_psc.h | =A0 =A04 ++++
> =A01 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/mpc52xx_psc.h b/arch/powerpc/includ=
e/asm/mpc52xx_psc.h
> index fb84120..42561f4 100644
> --- a/arch/powerpc/include/asm/mpc52xx_psc.h
> +++ b/arch/powerpc/include/asm/mpc52xx_psc.h
> @@ -25,7 +25,11 @@
> =A0#include <asm/types.h>
>
> =A0/* Max number of PSCs */
> +#ifdef CONFIG_PPC_MPC512x
> +#define MPC52xx_PSC_MAXNUM =A0 =A0 12
> +#else
> =A0#define MPC52xx_PSC_MAXNUM =A0 =A0 6
> +#endif
It would be nice to be rid of the stupid pre-allocated port structures
entirely, but that is a much more invasive and complex change.
Regardless;
Acked-by: Grant Likely <grant.likely@secretlab.ca>
Please repost this series and add linux-kernel@vger.kernel.org and
gregkh@suse.de to the cc: list. I'm happy to either let Greg pick
this series up, or pick up into my tree, but we need to coordinate
with Greg either way.
./scripts/get_maintainer.pl is your friend when posting patches.
g.
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [PATCH 2/3] powerpc: doc/dts-bindings: document mpc5121 psc uart dts-bindings
From: Grant Likely @ 2010-02-09 16:15 UTC (permalink / raw)
To: Anatolij Gustschin; +Cc: linuxppc-dev, wd, dzu
In-Reply-To: <1265096864-3506-3-git-send-email-agust@denx.de>
On Tue, Feb 2, 2010 at 12:47 AM, Anatolij Gustschin <agust@denx.de> wrote:
> Support for MPC5121 PSC UART in the mpc52xx_uart driver
> added new DTS properties for FSL MPC5121 PSC FIFO Controller.
> Provide documentation of the new properties and some examples.
>
> Signed-off-by: Anatolij Gustschin <agust@denx.de>
> Cc: Grant Likely <grant.likely@secretlab.ca>
Acked-by: Grant Likely <grant.likely@secretlab.ca>
> ---
> =A0.../powerpc/dts-bindings/fsl/mpc5121-psc.txt =A0 =A0 =A0 | =A0 70 ++++=
++++++++++++++++
> =A01 files changed, 70 insertions(+), 0 deletions(-)
> =A0create mode 100644 Documentation/powerpc/dts-bindings/fsl/mpc5121-psc.=
txt
>
> diff --git a/Documentation/powerpc/dts-bindings/fsl/mpc5121-psc.txt b/Doc=
umentation/powerpc/dts-bindings/fsl/mpc5121-psc.txt
> new file mode 100644
> index 0000000..8832e87
> --- /dev/null
> +++ b/Documentation/powerpc/dts-bindings/fsl/mpc5121-psc.txt
> @@ -0,0 +1,70 @@
> +MPC5121 PSC Device Tree Bindings
> +
> +PSC in UART mode
> +----------------
> +
> +For PSC in UART mode the needed PSC serial devices
> +are specified by fsl,mpc5121-psc-uart nodes in the
> +fsl,mpc5121-immr SoC node. Additionally the PSC FIFO
> +Controller node fsl,mpc5121-psc-fifo is requered there:
> +
> +fsl,mpc5121-psc-uart nodes
> +--------------------------
> +
> +Required properties :
> + - compatible : Should contain "fsl,mpc5121-psc-uart" and "fsl,mpc5121-p=
sc"
> + - cell-index : Index of the PSC in hardware
> + - reg : Offset and length of the register set for the PSC device
> + - interrupts : <a b> where a is the interrupt number of the
> + =A0 PSC FIFO Controller and b is a field that represents an
> + =A0 encoding of the sense and level information for the interrupt.
> + - interrupt-parent : the phandle for the interrupt controller that
> + =A0 services interrupts for this device.
> +
> +Recommended properties :
> + - fsl,rx-fifo-size : the size of the RX fifo slice (a multiple of 4)
> + - fsl,tx-fifo-size : the size of the TX fifo slice (a multiple of 4)
> +
> +
> +fsl,mpc5121-psc-fifo node
> +-------------------------
> +
> +Required properties :
> + - compatible : Should be "fsl,mpc5121-psc-fifo"
> + - reg : Offset and length of the register set for the PSC
> + =A0 =A0 =A0 =A0 FIFO Controller
> + - interrupts : <a b> where a is the interrupt number of the
> + =A0 PSC FIFO Controller and b is a field that represents an
> + =A0 encoding of the sense and level information for the interrupt.
> + - interrupt-parent : the phandle for the interrupt controller that
> + =A0 services interrupts for this device.
> +
> +
> +Example for a board using PSC0 and PSC1 devices in serial mode:
> +
> +serial@11000 {
> + =A0 =A0 =A0 compatible =3D "fsl,mpc5121-psc-uart", "fsl,mpc5121-psc";
> + =A0 =A0 =A0 cell-index =3D <0>;
> + =A0 =A0 =A0 reg =3D <0x11000 0x100>;
> + =A0 =A0 =A0 interrupts =3D <40 0x8>;
> + =A0 =A0 =A0 interrupt-parent =3D < &ipic >;
> + =A0 =A0 =A0 fsl,rx-fifo-size =3D <16>;
> + =A0 =A0 =A0 fsl,tx-fifo-size =3D <16>;
> +};
> +
> +serial@11100 {
> + =A0 =A0 =A0 compatible =3D "fsl,mpc5121-psc-uart", "fsl,mpc5121-psc";
> + =A0 =A0 =A0 cell-index =3D <1>;
> + =A0 =A0 =A0 reg =3D <0x11100 0x100>;
> + =A0 =A0 =A0 interrupts =3D <40 0x8>;
> + =A0 =A0 =A0 interrupt-parent =3D < &ipic >;
> + =A0 =A0 =A0 fsl,rx-fifo-size =3D <16>;
> + =A0 =A0 =A0 fsl,tx-fifo-size =3D <16>;
> +};
> +
> +pscfifo@11f00 {
> + =A0 =A0 =A0 compatible =3D "fsl,mpc5121-psc-fifo";
> + =A0 =A0 =A0 reg =3D <0x11f00 0x100>;
> + =A0 =A0 =A0 interrupts =3D <40 0x8>;
> + =A0 =A0 =A0 interrupt-parent =3D < &ipic >;
> +};
> --
> 1.6.3.3
>
>
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [PATCH 1/3] serial: mpc52xx_uart: re-enable mpc5121 PSC UART support
From: Grant Likely @ 2010-02-09 16:13 UTC (permalink / raw)
To: Anatolij Gustschin; +Cc: linuxppc-dev, wd, dzu
In-Reply-To: <1265096864-3506-2-git-send-email-agust@denx.de>
On Tue, Feb 2, 2010 at 12:47 AM, Anatolij Gustschin <agust@denx.de> wrote:
> Currently the support for MPC5121 PSC UART in the mpc52xx_uart
> driver is broken (only console pre-initialized by the bootloader
> works). Re-enable it now by providing MPC5121 specific ops
> for PSCx clock activation, FIFO controller init/uninit and
> MPC5121 PSC FIFO shared interrupt handling functions.
>
> Signed-off-by: Anatolij Gustschin <agust@denx.de>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> ---
> This patch has been tested using 5200/5121 multiplatform kernel
> on tqm5200 and mpc5121ads boards (mpc52xx_uart staticaly linked
> and as a driver module).
>
> @@ -1164,7 +1378,8 @@ mpc52xx_uart_of_probe(struct of_device *op, const s=
truct of_device_id *match)
> =A0 =A0 =A0 =A0/* Add the port to the uart sub-system */
> =A0 =A0 =A0 =A0ret =3D uart_add_one_port(&mpc52xx_uart_driver, port);
> =A0 =A0 =A0 =A0if (ret) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 irq_dispose_mapping(port->irq);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!(port->irqflags & IRQF_SHARED))
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 irq_dispose_mapping(port->i=
rq);
I'd just drop the irq_dispose_mapping() entirely. It doesn't really
cost anything to leave the mapping around when unloading the driver.
Otherwise, looks good to me. You can add my acked-by in the next spin.
g.
^ permalink raw reply
* register long sp asm("r1") incorrect
From: Pavel Machek @ 2010-02-09 15:24 UTC (permalink / raw)
To: benh, paulus, linuxppc-dev, kernel list
...according to gcc docs, sp should be global, or placement in
register is not guaranteed (except at asm boundaries, but there are
none).
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply
* RE: [PATCH] [V2] net: emaclite: adding MDIO and phy lib support
From: John Linn @ 2010-02-09 15:57 UTC (permalink / raw)
To: Grant Likely
Cc: linuxppc-dev, netdev, Sadanand Mutyala, jgarzik, john.williams
In-Reply-To: <fa686aa41002090745x19d569acx3b50e8078ebb16f7@mail.gmail.com>
> -----Original Message-----
> From: glikely@secretlab.ca [mailto:glikely@secretlab.ca] On Behalf Of Gra=
nt Likely
> Sent: Tuesday, February 09, 2010 8:45 AM
> To: John Linn
> Cc: netdev@vger.kernel.org; linuxppc-dev@ozlabs.org; jgarzik@pobox.com; j=
wboyer@linux.vnet.ibm.com;
> john.williams@petalogix.com; Sadanand Mutyala
> Subject: Re: [PATCH] [V2] net: emaclite: adding MDIO and phy lib support
> =
> On Mon, Feb 8, 2010 at 7:09 AM, John Linn <John.Linn@xilinx.com> wrote:
> >> -----Original Message-----
> >> From: John Linn [mailto:john.linn@xilinx.com]
> >> Sent: Friday, February 05, 2010 3:41 PM
> >> To: netdev@vger.kernel.org; linuxppc-dev@ozlabs.org;
> > jgarzik@pobox.com; grant.likely@secretlab.ca;
> >> jwboyer@linux.vnet.ibm.com
> >> Cc: john.williams@petalogix.com; John Linn; Sadanand Mutyala
> >> Subject: [PATCH] [V2] net: emaclite: adding MDIO and phy lib support
> >>
> >> These changes add MDIO and phy lib support to the driver as the
> >> IP core now supports the MDIO bus.
> >>
> >> The MDIO bus and phy are added as a child to the emaclite in the
> > device
> >> tree as illustrated below.
> >>
> >> mdio {
> >> =A0 =A0 =A0 #address-cells =3D <1>;
> >> =A0 =A0 =A0 #size-cells =3D <0>;
> >> =A0 =A0 =A0 compatible =3D "xlnx,emaclite-mdio";
> >> =A0 =A0 =A0 phy0: phy@7 {
> >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 reg =3D <7>;
> >> =A0 =A0 =A0 } ;
> >> }
> =
> You misunderstood me about the compatible property. You don't need a
> compatible prop in the mdio node because it is *alwasy* a child of the
> emaclite node, and the emaclite driver knows to go looking for it (ie.
> it is a part of the device).
OK, now I see, sorry for my denseness there.
> =
> What I want to see is a compatible value in the phy@7 node for the
> make & model of the phy.
Makes more sense.
> =
> Otherwise, looks good to me. Fix the mutex timeout problem and then
> you can add my acked-by line to the patch.
Will spin V3 patch with fixes and add your ack, appreciate the help and you=
r patience.
- John
> =
> Cheers,
> g.
This email and any attachments are intended for the sole use of the named r=
ecipient(s) and contain(s) confidential information that may be proprietary=
, privileged or copyrighted under applicable law. If you are not the intend=
ed recipient, do not read, copy, or forward this email message or any attac=
hments. Delete this email message and any attachments immediately.
^ permalink raw reply
* Re: [PATCH] [V2] net: emaclite: adding MDIO and phy lib support
From: Grant Likely @ 2010-02-09 15:45 UTC (permalink / raw)
To: John Linn; +Cc: linuxppc-dev, netdev, Sadanand Mutyala, jgarzik, john.williams
In-Reply-To: <260b8801-53bd-44fe-a6b8-7d1d0f56fef5@SG2EHSMHS011.ehs.local>
On Mon, Feb 8, 2010 at 7:09 AM, John Linn <John.Linn@xilinx.com> wrote:
>> -----Original Message-----
>> From: John Linn [mailto:john.linn@xilinx.com]
>> Sent: Friday, February 05, 2010 3:41 PM
>> To: netdev@vger.kernel.org; linuxppc-dev@ozlabs.org;
> jgarzik@pobox.com; grant.likely@secretlab.ca;
>> jwboyer@linux.vnet.ibm.com
>> Cc: john.williams@petalogix.com; John Linn; Sadanand Mutyala
>> Subject: [PATCH] [V2] net: emaclite: adding MDIO and phy lib support
>>
>> These changes add MDIO and phy lib support to the driver as the
>> IP core now supports the MDIO bus.
>>
>> The MDIO bus and phy are added as a child to the emaclite in the
> device
>> tree as illustrated below.
>>
>> mdio {
>> =A0 =A0 =A0 #address-cells =3D <1>;
>> =A0 =A0 =A0 #size-cells =3D <0>;
>> =A0 =A0 =A0 compatible =3D "xlnx,emaclite-mdio";
>> =A0 =A0 =A0 phy0: phy@7 {
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 reg =3D <7>;
>> =A0 =A0 =A0 } ;
>> }
You misunderstood me about the compatible property. You don't need a
compatible prop in the mdio node because it is *alwasy* a child of the
emaclite node, and the emaclite driver knows to go looking for it (ie.
it is a part of the device).
What I want to see is a compatible value in the phy@7 node for the
make & model of the phy.
Otherwise, looks good to me. Fix the mutex timeout problem and then
you can add my acked-by line to the patch.
Cheers,
g.
^ permalink raw reply
* Re: [net-next-2.6 PATCH 2/3] fs_enet: Add support for MPC512x to fs_enet driver
From: Anatolij Gustschin @ 2010-02-09 14:23 UTC (permalink / raw)
To: David Miller; +Cc: kosmo, dzu, netdev, linuxppc-dev, wd
In-Reply-To: <20100121.180311.228791894.davem@davemloft.net>
On Thu, 21 Jan 2010 18:03:11 -0800 (PST)
David Miller <davem@davemloft.net> wrote:
> From: Wolfgang Grandegger <wg@grandegger.com>
> Date: Thu, 21 Jan 2010 16:25:38 +0100
>
> > Do you see a more clever solution to this problem?
>
> See how we handle this in the ESP scsi driver. We have a set of
> defines for the register offsets, and a set of methods a chip driver
> implements for register accesses.
>
> If the offsets differ, the register access method can translate the
> generic register offsets into whatever layout their implementation
> actually uses.
First of all thanks for your suggestion. I have seen how you
handle register access in the ESP scsi driver. The reason I didn't
try to implement register access using similar approach is that
we have different sort of problem.
In my understanding, in the ESP scsi driver the set of defines for
the register offsets is common for all chip drivers. The chip driver
methods for register access translate the offsets because the
registers on some chips are at different intervals (4-byte, 1-byte,
16-byte for mac_esp.c). But the register order is the same for
different chips.
In our case non only the register order is not the same for 8xx
FEC and 5121 FEC, but there are also other differences, different
reserved areas between several registers, some registers are
available only on 8xx and some only on 5121.
Now at least tree people suggested to fork the driver. My question
is if you would accept a forked 5121 FEC specific driver realised
similar to drivers/net/fs_enet/mac-fec.c and
drivers/net/fs_enet/mii-fec.c drivers?
Thanks,
Anatolij
^ permalink raw reply
* Re: [ELDK] GPIO pin in interrupt handler/AMCC processor
From: Zoolu R @ 2010-02-09 9:51 UTC (permalink / raw)
To: Stefan Roese
In-Reply-To: <201002081913.57920.sr@denx.de>
[-- Attachment #1: Type: text/plain, Size: 2155 bytes --]
Hi Stefan,
thanks for you reply, as per Amcc 440 ep h/w spec . I can see 10 External
interrupts can be configured !! So can i use the GPIO ping mapped to one of
these external IRQ's ? Or should the h/w deisgn also support it or is it
pure s/w configurations. So i would just like to get an indication via
interrupt when status is changed (0/1). I would like to do this is linux.
Are there any reference drivers for this ? how to go about it ?
Thanks & Regards
Zoolu
On Mon, Feb 8, 2010 at 11:43 PM, Stefan Roese <sr@denx.de> wrote:
> Hi Zoolu,
>
> first, this question is not ELDK related. You should send it to Linux-PPC
> mailing list instead <linuxppc-dev@ozlabs.org>. But still here a few quick
> answers/comments.
>
> On Monday 08 February 2010 17:37:20 Zoolu R wrote:
> > We are using a target with AMCC 440ep processor. We need to read the a
> GPIO
> > pin value in a interrupt mode in linux kernel. Is there some modules
> where
> > i can refer so some generic implementation(configuring gpio pin as
> > inteerupt pin and handlling the same).
>
> The PPC4xx GPIO support is available in the current Linux kernel. See:
>
> arch/powerpc/sysdev/ppc4xx_gpio.c
>
> for more details. Please note that you also need to write a GPIO device
> node
> in the device tree source (dts) of your Linux port. See this file for
> details
> in the GPIO node:
>
> arch/powerpc/boot/dts/warp.dts
>
> > I would like to detect the dynamic
> > value change(0/1) using interrupt and perform isr_call back. Could some
> > one show me some pointers.
>
> So you don't want to use the GPIO pin functionality, but the external
> interrupt functionality (the pin functions are multiplexed). You need to
> write
> a device driver for your custom device. And you need to configure the pin
> as
> external interrupt and again add a device node including the interrupt
> description into your device tree source (dts).
>
> Cheers,
> Stefan
>
> --
> DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office@denx.de
>
[-- Attachment #2: Type: text/html, Size: 2739 bytes --]
^ permalink raw reply
* Re: [PATCH RFC 0/3] Implement refcounting for OF GPIO chips
From: Michal Simek @ 2010-02-09 9:40 UTC (permalink / raw)
To: avorontsov
Cc: Michal Simek, David Brownell, devicetree-discuss, linux-kernel,
linuxppc-dev, microblaze-uclinux, David Miller
In-Reply-To: <20100205204949.GA2575@oksana.dev.rtsoft.ru>
Anton Vorontsov wrote:
> Hi all,
>
> OF GPIO infrastructure is using dynamic GPIO bases, so it is possible
> that of_get_gpio()'s returned GPIO number will be no longer valid, or
> worse, it may point to an unexpected GPIO controller.
>
> This scenario is possible:
>
> driver A: driver B: driver C:
> --------- --------- ---------
> gpiochip_add()
> gpio = of_get_gpio()
> gpiochip_remove()
> gpiochip_add()
> gpio_request(gpio);
> gpio_set_value(gpio);
>
> That is, driver A assumes that it is working with GPIO from driver B,
> but in practice it may disappear and driver C will take its GPIO base
> number, so it will provide the same GPIO numbers.
>
> The above situation is hard to trigger, but the issue is there
> nonetheless, and so needs fixing.
I tested xilinx gpio driver, heartbeat trigger and access through sysfs
and I haven't found any problem. There is only small part of code for
Microblaze and it is the same with PowerPC. If is Ben OK with it, I am
ok too.
Thanks,
Michal
>
> Thanks,
>
> p.s. The patches are based on top of
> '[PATCH v2 0/4] OF GPIO integration for I2C/SPI GPIO chips'
> http://lkml.org/lkml/2010/2/5/243
>
--
Michal Simek, Ing. (M.Eng)
PetaLogix - Linux Solutions for a Reconfigurable World
w: www.petalogix.com p: +61-7-30090663,+42-0-721842854 f: +61-7-30090663
^ permalink raw reply
* Re: [PATCH 3/3] of/gpio: Introduce of_put_gpio(), add ref counting for OF GPIO chips
From: Michal Simek @ 2010-02-09 9:20 UTC (permalink / raw)
To: akpm, Anton Vorontsov
Cc: Michal Simek, David Brownell, devicetree-discuss, linux-kernel,
linuxppc-dev, microblaze-uclinux, David Miller
In-Reply-To: <4B7127AC.9020600@petalogix.com>
Michal Simek wrote:
> Anton Vorontsov wrote:
>> OF GPIO infrastructure is using dynamic GPIO bases, so it is possible
>> that of_get_gpio()'s returned GPIO number will be no longer valid, or
>> worse, it may point to an unexpected GPIO controller.
>
> I am not able to apply this last patch.
>
> $ git-am < \[PATCH\ 3_3\]\ of_gpio\:\ Introduce\ of_put_gpio\(\)\,\ add\
> ref\ counting\ for\ OF\ GPIO\ chips.eml
> Applying of/gpio: Introduce of_put_gpio(), add ref counting for OF GPIO
> chips
> error: patch failed: drivers/of/gpio.c:254
> error: drivers/of/gpio.c: patch does not apply
> Patch failed at 0001.
> When you have resolved this problem run "git-am --resolved".
> If you would prefer to skip this patch, instead run "git-am --skip".
>
> Below is my git-log which patches I applied.
> Please fix it.
I see where the problem is. I applied Andrews patch too that's why I am
getting the fault above.
Anyway will be good to fix it. Will test gpio and let you know.
Michal
>
> Thanks,
> Michal
>
> 99d5baaa53562252a896080bf426b4ae71a5e55f of: Introduce safe accessors
> for node->data
> df6d71af9c0691716ad8a368609a7e26a8dfdaa0 of platforms: Move common
> static initialization to of_node_init()
> 566969302a9dd70ce8a07c978bd08804ee73d0d6 powerpc/mcu_mpc8349emitx:
> Remove OF GPIO handling stuff
> 816932ea74919867c1f899758ea4cdd138145453 of/gpio: Implement GPIOLIB
> notifier hooks
> 3ea035a9e0f446b2b9cd9bacf73395a0c80ae3e9 of/gpio: Implement GPIOLIB
> notifier hooks
> b7b96a9ee215c800476d002b06a338c4499f3813 of/gpio: Add support for
> two-stage registration for the of_gpio_chips
> 15678cc192c03a2b1bbb36da18ff2a3fe2d78897 gpiolib: Introduce chip
> addition/removal notifier
> deb0c98c7f6035d47a247e548384517a955314a5 Merge branch 'for-2.6.33' of
> git://linux-nfs.org/~bfields/linux
>
>
>
>>
>> This scenario is possible:
>>
>> driver A: driver B: driver C:
>> --------- --------- ---------
>> gpiochip_add()
>> gpio = of_get_gpio()
>> gpiochip_remove()
>> gpiochip_add()
>> gpio_request(gpio);
>> gpio_set_value(gpio);
>>
>> That is, driver A assumes that it is working with GPIO from driver B,
>> but in practice it may disappear and driver C will take its GPIO base
>> number, so it will provide the same GPIO numbers.
>>
>> With this patch that situation is no longer possible. Though drivers
>> will need to learn to put GPIOs back, so that GPIO controllers could
>> be removed.
>>
>> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
>> ---
>> drivers/of/gpio.c | 82
>> ++++++++++++++++++++++++++++++++++++++++-------
>> include/linux/of_gpio.h | 5 +++
>> 2 files changed, 75 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/of/gpio.c b/drivers/of/gpio.c
>> index 9d8df77..e94c5c8 100644
>> --- a/drivers/of/gpio.c
>> +++ b/drivers/of/gpio.c
>> @@ -28,6 +28,8 @@
>> * Returns GPIO number to use with Linux generic GPIO API, or one of
>> the errno
>> * value on the error condition. If @flags is not NULL the function
>> also fills
>> * in flags for the GPIO.
>> + *
>> + * Remeber to put the GPIO back using of_put_gpio() call.
>> */
>> int of_get_gpio_flags(struct device_node *np, int index,
>> enum of_gpio_flags *flags)
>> @@ -46,6 +48,8 @@ int of_get_gpio_flags(struct device_node *np, int
>> index,
>> goto err0;
>> }
>>
>> + spin_lock(&gc->data_lock);
>> +
>> of_gc = gc->data;
>> if (!of_gc) {
>> pr_debug("%s: gpio controller %s isn't registered\n",
>> @@ -72,15 +76,62 @@ int of_get_gpio_flags(struct device_node *np, int
>> index,
>> goto err1;
>>
>> ret += of_gc->chip->base;
>> +
>> + if (!try_module_get(of_gc->chip->owner)) {
>> + ret = -EINVAL;
>> + goto err1;
>> + }
>> +
>> + of_gc->refcnt++;
>> err1:
>> + spin_unlock(&gc->data_lock);
>> +
>> of_node_put(gc);
>> err0:
>> pr_debug("%s exited with status %d\n", __func__, ret);
>> +
>> return ret;
>> }
>> EXPORT_SYMBOL(of_get_gpio_flags);
>>
>> /**
>> + * of_put_gpio - Put a GPIO back to the OF subsystem
>> + * @np: device node of the GPIO owner
>> + * @index: index of the GPIO
>> + */
>> +static inline void of_put_gpio(struct device_node *np, int index)
>> +{
>> + int ret;
>> + struct device_node *gc;
>> + struct of_gpio_chip *of_gc;
>> +
>> + ret = of_parse_phandles_with_args(np, "gpios", "#gpio-cells", index,
>> + &gc, NULL);
>> + if (ret) {
>> + pr_debug("%s: can't parse gpios property\n", __func__);
>> + return;
>> + }
>> +
>> + spin_lock(&gc->data_lock);
>> +
>> + of_gc = gc->data;
>> + if (!of_gc) {
>> + pr_debug("%s: gpio controller %s isn't registered\n",
>> + np->full_name, gc->full_name);
>> + goto err;
>> + }
>> +
>> + if (of_gc->refcnt)
>> + of_gc->refcnt--;
>> + else
>> + WARN_ON(1);
>> +
>> + module_put(of_gc->chip->owner);
>> +err:
>> + spin_unlock(&gc->data_lock);
>> +}
>> +
>> +/**
>> * of_gpio_count - Count GPIOs for a device
>> * @np: device node to count GPIOs for
>> *
>> @@ -254,11 +305,7 @@ static int of_gpiochip_register_simple(struct
>> gpio_chip *chip,
>> struct device_node *np)
>> {
>> struct of_gpio_chip *of_gc;
>> -
>> - if (np->data) {
>> - WARN_ON(1);
>> - return -EBUSY;
>> - }
>> + int ret;
>>
>> of_gc = kzalloc(sizeof(*of_gc), GFP_KERNEL);
>> if (!of_gc)
>> @@ -267,10 +314,12 @@ static int of_gpiochip_register_simple(struct
>> gpio_chip *chip,
>> of_gc->gpio_cells = 2;
>> of_gc->xlate = of_gpio_simple_xlate;
>> of_gc->chip = chip;
>> - np->data = of_gc;
>> - of_node_get(np);
>>
>> - return 0;
>> + ret = of_node_set_data(np, of_gc);
>> + if (ret)
>> + kfree(of_gc);
>> +
>> + return ret;
>> }
>> EXPORT_SYMBOL(of_gpiochip_register_simple);
>>
>> @@ -286,17 +335,26 @@ static int of_gpiochip_unregister(struct
>> gpio_chip *chip,
>> struct device_node *np)
>> {
>> struct of_gpio_chip *of_gc = np->data;
>> + int ret = 0;
>>
>> if (!of_gc || of_gc->chip != chip) {
>> WARN_ON(1);
>> return -EINVAL;
>> }
>>
>> - np->data = NULL;
>> - kfree(of_gc);
>> - of_node_put(np);
>> + spin_lock(&np->data_lock);
>>
>> - return 0;
>> + if (of_gc->refcnt)
>> + ret = -EBUSY;
>> + else
>> + of_node_release_data_unlocked(np);
>> +
>> + spin_unlock(&np->data_lock);
>> +
>> + if (!ret)
>> + kfree(of_gc);
>> +
>> + return ret;
>> }
>>
>> static int of_gpio_notify(struct notifier_block *nb, unsigned long msg,
>> diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
>> index c74cb37..aca7ab1 100644
>> --- a/include/linux/of_gpio.h
>> +++ b/include/linux/of_gpio.h
>> @@ -38,6 +38,7 @@ enum of_gpio_flags {
>> struct of_gpio_chip {
>> struct gpio_chip gc; /* legacy, don't use for a new code */
>> struct gpio_chip *chip;
>> + unsigned int refcnt;
>> int gpio_cells;
>> int (*xlate)(struct of_gpio_chip *of_gc, struct device_node *np,
>> const void *gpio_spec, enum of_gpio_flags *flags);
>> @@ -83,6 +84,8 @@ static inline int of_get_gpio_flags(struct
>> device_node *np, int index,
>> return -ENOSYS;
>> }
>>
>> +static inline void of_put_gpio(struct device_node *np, int index) {}
>> +
>> static inline unsigned int of_gpio_count(struct device_node *np)
>> {
>> return 0;
>> @@ -97,6 +100,8 @@ static inline unsigned int of_gpio_count(struct
>> device_node *np)
>> *
>> * Returns GPIO number to use with Linux generic GPIO API, or one of
>> the errno
>> * value on the error condition.
>> + *
>> + * Remeber to put the GPIO back using of_put_gpio() call.
>> */
>> static inline int of_get_gpio(struct device_node *np, int index)
>> {
>
>
--
Michal Simek, Ing. (M.Eng)
PetaLogix - Linux Solutions for a Reconfigurable World
w: www.petalogix.com p: +61-7-30090663,+42-0-721842854 f: +61-7-30090663
^ permalink raw reply
* Re: [PATCH 3/3] of/gpio: Introduce of_put_gpio(), add ref counting for OF GPIO chips
From: Michal Simek @ 2010-02-09 9:15 UTC (permalink / raw)
To: Anton Vorontsov
Cc: Michal Simek, David Brownell, devicetree-discuss, linux-kernel,
linuxppc-dev, microblaze-uclinux, David Miller
In-Reply-To: <20100205205045.GC4178@oksana.dev.rtsoft.ru>
Anton Vorontsov wrote:
> OF GPIO infrastructure is using dynamic GPIO bases, so it is possible
> that of_get_gpio()'s returned GPIO number will be no longer valid, or
> worse, it may point to an unexpected GPIO controller.
I am not able to apply this last patch.
$ git-am < \[PATCH\ 3_3\]\ of_gpio\:\ Introduce\ of_put_gpio\(\)\,\ add\
ref\ counting\ for\ OF\ GPIO\ chips.eml
Applying of/gpio: Introduce of_put_gpio(), add ref counting for OF GPIO
chips
error: patch failed: drivers/of/gpio.c:254
error: drivers/of/gpio.c: patch does not apply
Patch failed at 0001.
When you have resolved this problem run "git-am --resolved".
If you would prefer to skip this patch, instead run "git-am --skip".
Below is my git-log which patches I applied.
Please fix it.
Thanks,
Michal
99d5baaa53562252a896080bf426b4ae71a5e55f of: Introduce safe accessors
for node->data
df6d71af9c0691716ad8a368609a7e26a8dfdaa0 of platforms: Move common
static initialization to of_node_init()
566969302a9dd70ce8a07c978bd08804ee73d0d6 powerpc/mcu_mpc8349emitx:
Remove OF GPIO handling stuff
816932ea74919867c1f899758ea4cdd138145453 of/gpio: Implement GPIOLIB
notifier hooks
3ea035a9e0f446b2b9cd9bacf73395a0c80ae3e9 of/gpio: Implement GPIOLIB
notifier hooks
b7b96a9ee215c800476d002b06a338c4499f3813 of/gpio: Add support for
two-stage registration for the of_gpio_chips
15678cc192c03a2b1bbb36da18ff2a3fe2d78897 gpiolib: Introduce chip
addition/removal notifier
deb0c98c7f6035d47a247e548384517a955314a5 Merge branch 'for-2.6.33' of
git://linux-nfs.org/~bfields/linux
>
> This scenario is possible:
>
> driver A: driver B: driver C:
> --------- --------- ---------
> gpiochip_add()
> gpio = of_get_gpio()
> gpiochip_remove()
> gpiochip_add()
> gpio_request(gpio);
> gpio_set_value(gpio);
>
> That is, driver A assumes that it is working with GPIO from driver B,
> but in practice it may disappear and driver C will take its GPIO base
> number, so it will provide the same GPIO numbers.
>
> With this patch that situation is no longer possible. Though drivers
> will need to learn to put GPIOs back, so that GPIO controllers could
> be removed.
>
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
> drivers/of/gpio.c | 82 ++++++++++++++++++++++++++++++++++++++++-------
> include/linux/of_gpio.h | 5 +++
> 2 files changed, 75 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/of/gpio.c b/drivers/of/gpio.c
> index 9d8df77..e94c5c8 100644
> --- a/drivers/of/gpio.c
> +++ b/drivers/of/gpio.c
> @@ -28,6 +28,8 @@
> * Returns GPIO number to use with Linux generic GPIO API, or one of the errno
> * value on the error condition. If @flags is not NULL the function also fills
> * in flags for the GPIO.
> + *
> + * Remeber to put the GPIO back using of_put_gpio() call.
> */
> int of_get_gpio_flags(struct device_node *np, int index,
> enum of_gpio_flags *flags)
> @@ -46,6 +48,8 @@ int of_get_gpio_flags(struct device_node *np, int index,
> goto err0;
> }
>
> + spin_lock(&gc->data_lock);
> +
> of_gc = gc->data;
> if (!of_gc) {
> pr_debug("%s: gpio controller %s isn't registered\n",
> @@ -72,15 +76,62 @@ int of_get_gpio_flags(struct device_node *np, int index,
> goto err1;
>
> ret += of_gc->chip->base;
> +
> + if (!try_module_get(of_gc->chip->owner)) {
> + ret = -EINVAL;
> + goto err1;
> + }
> +
> + of_gc->refcnt++;
> err1:
> + spin_unlock(&gc->data_lock);
> +
> of_node_put(gc);
> err0:
> pr_debug("%s exited with status %d\n", __func__, ret);
> +
> return ret;
> }
> EXPORT_SYMBOL(of_get_gpio_flags);
>
> /**
> + * of_put_gpio - Put a GPIO back to the OF subsystem
> + * @np: device node of the GPIO owner
> + * @index: index of the GPIO
> + */
> +static inline void of_put_gpio(struct device_node *np, int index)
> +{
> + int ret;
> + struct device_node *gc;
> + struct of_gpio_chip *of_gc;
> +
> + ret = of_parse_phandles_with_args(np, "gpios", "#gpio-cells", index,
> + &gc, NULL);
> + if (ret) {
> + pr_debug("%s: can't parse gpios property\n", __func__);
> + return;
> + }
> +
> + spin_lock(&gc->data_lock);
> +
> + of_gc = gc->data;
> + if (!of_gc) {
> + pr_debug("%s: gpio controller %s isn't registered\n",
> + np->full_name, gc->full_name);
> + goto err;
> + }
> +
> + if (of_gc->refcnt)
> + of_gc->refcnt--;
> + else
> + WARN_ON(1);
> +
> + module_put(of_gc->chip->owner);
> +err:
> + spin_unlock(&gc->data_lock);
> +}
> +
> +/**
> * of_gpio_count - Count GPIOs for a device
> * @np: device node to count GPIOs for
> *
> @@ -254,11 +305,7 @@ static int of_gpiochip_register_simple(struct gpio_chip *chip,
> struct device_node *np)
> {
> struct of_gpio_chip *of_gc;
> -
> - if (np->data) {
> - WARN_ON(1);
> - return -EBUSY;
> - }
> + int ret;
>
> of_gc = kzalloc(sizeof(*of_gc), GFP_KERNEL);
> if (!of_gc)
> @@ -267,10 +314,12 @@ static int of_gpiochip_register_simple(struct gpio_chip *chip,
> of_gc->gpio_cells = 2;
> of_gc->xlate = of_gpio_simple_xlate;
> of_gc->chip = chip;
> - np->data = of_gc;
> - of_node_get(np);
>
> - return 0;
> + ret = of_node_set_data(np, of_gc);
> + if (ret)
> + kfree(of_gc);
> +
> + return ret;
> }
> EXPORT_SYMBOL(of_gpiochip_register_simple);
>
> @@ -286,17 +335,26 @@ static int of_gpiochip_unregister(struct gpio_chip *chip,
> struct device_node *np)
> {
> struct of_gpio_chip *of_gc = np->data;
> + int ret = 0;
>
> if (!of_gc || of_gc->chip != chip) {
> WARN_ON(1);
> return -EINVAL;
> }
>
> - np->data = NULL;
> - kfree(of_gc);
> - of_node_put(np);
> + spin_lock(&np->data_lock);
>
> - return 0;
> + if (of_gc->refcnt)
> + ret = -EBUSY;
> + else
> + of_node_release_data_unlocked(np);
> +
> + spin_unlock(&np->data_lock);
> +
> + if (!ret)
> + kfree(of_gc);
> +
> + return ret;
> }
>
> static int of_gpio_notify(struct notifier_block *nb, unsigned long msg,
> diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
> index c74cb37..aca7ab1 100644
> --- a/include/linux/of_gpio.h
> +++ b/include/linux/of_gpio.h
> @@ -38,6 +38,7 @@ enum of_gpio_flags {
> struct of_gpio_chip {
> struct gpio_chip gc; /* legacy, don't use for a new code */
> struct gpio_chip *chip;
> + unsigned int refcnt;
> int gpio_cells;
> int (*xlate)(struct of_gpio_chip *of_gc, struct device_node *np,
> const void *gpio_spec, enum of_gpio_flags *flags);
> @@ -83,6 +84,8 @@ static inline int of_get_gpio_flags(struct device_node *np, int index,
> return -ENOSYS;
> }
>
> +static inline void of_put_gpio(struct device_node *np, int index) {}
> +
> static inline unsigned int of_gpio_count(struct device_node *np)
> {
> return 0;
> @@ -97,6 +100,8 @@ static inline unsigned int of_gpio_count(struct device_node *np)
> *
> * Returns GPIO number to use with Linux generic GPIO API, or one of the errno
> * value on the error condition.
> + *
> + * Remeber to put the GPIO back using of_put_gpio() call.
> */
> static inline int of_get_gpio(struct device_node *np, int index)
> {
--
Michal Simek, Ing. (M.Eng)
PetaLogix - Linux Solutions for a Reconfigurable World
w: www.petalogix.com p: +61-7-30090663,+42-0-721842854 f: +61-7-30090663
^ permalink raw reply
* Re: [PATCH 0/3] OF Device tree merge patches
From: Michal Simek @ 2010-02-09 9:01 UTC (permalink / raw)
To: Grant Likely
Cc: sfr, microblaze-uclinux, devicetree-discuss, linux-kernel,
jeremy.kerr, linuxppc-dev
In-Reply-To: <20100203185753.26862.6818.stgit@angua>
Hi Grant,
Grant Likely wrote:
> Hi everyone. Here are a few patches that I've got sitting in my
> test-devicetree branch, but I haven't posted for review yet. Please
> take a look and let me know if they are okay. Once I've collected
> acks I'll move them over to my next-devicetree branch.
I am not able applied these patches on Linus head but I have tested your
test-devicetree branch and I haven't found any problem for Microblaze,
that's why you can add my ACK for Microblaze part
Acked-by: Michal Simek <monstr@monstr.eu>
BTW: I am testing Stephen's next tree every day that's why if you add
that patches to next-tree you can look at
http://www.monstr.eu/wiki/doku.php?id=log:log
to see if is any problem on Microblaze or not.
Thanks,
Michal
>
> Thanks,
> g.
>
> ---
>
> Grant Likely (3):
> of/flattree: merge early_init_dt_scan_memory() common code
> of: add 'of_' prefix to machine_is_compatible()
> microblaze: remove early_init_dt_scan_cpus() and phyp_dump_*()
>
>
> arch/microblaze/include/asm/prom.h | 4
> arch/microblaze/kernel/prom.c | 284 -------------------------
> arch/powerpc/include/asm/prom.h | 4
> arch/powerpc/kernel/pci_64.c | 2
> arch/powerpc/kernel/prom.c | 149 ++-----------
> arch/powerpc/platforms/85xx/xes_mpc85xx.c | 4
> arch/powerpc/platforms/cell/cbe_powerbutton.c | 2
> arch/powerpc/platforms/cell/ras.c | 2
> arch/powerpc/platforms/pasemi/cpufreq.c | 4
> arch/powerpc/platforms/powermac/cpufreq_32.c | 14 +
> arch/powerpc/platforms/powermac/cpufreq_64.c | 14 +
> arch/powerpc/platforms/powermac/feature.c | 2
> arch/powerpc/platforms/powermac/smp.c | 12 +
> arch/powerpc/platforms/powermac/time.c | 8 -
> arch/powerpc/platforms/powermac/udbg_scc.c | 6 -
> arch/powerpc/sysdev/grackle.c | 4
> arch/sparc/kernel/prom_common.c | 12 -
> drivers/char/hvc_beat.c | 2
> drivers/gpu/drm/radeon/radeon_combios.c | 44 ++--
> drivers/macintosh/adb.c | 4
> drivers/macintosh/therm_pm72.c | 8 -
> drivers/macintosh/therm_windtunnel.c | 2
> drivers/macintosh/via-pmu-backlight.c | 8 -
> drivers/macintosh/via-pmu.c | 8 -
> drivers/macintosh/windfarm_core.c | 6 -
> drivers/macintosh/windfarm_cpufreq_clamp.c | 6 -
> drivers/macintosh/windfarm_lm75_sensor.c | 6 -
> drivers/macintosh/windfarm_max6690_sensor.c | 6 -
> drivers/macintosh/windfarm_pm112.c | 2
> drivers/macintosh/windfarm_pm121.c | 2
> drivers/macintosh/windfarm_pm81.c | 4
> drivers/macintosh/windfarm_pm91.c | 2
> drivers/macintosh/windfarm_smu_sensors.c | 6 -
> drivers/net/mace.c | 2
> drivers/of/Kconfig | 4
> drivers/of/base.c | 98 ++++++++-
> drivers/of/fdt.c | 50 ++++
> drivers/serial/pmac_zilog.c | 6 -
> drivers/video/aty/aty128fb.c | 14 +
> drivers/video/aty/atyfb_base.c | 8 -
> drivers/video/aty/radeon_backlight.c | 6 -
> include/linux/of.h | 6 +
> include/linux/of_fdt.h | 5
> sound/ppc/awacs.c | 24 +-
> sound/ppc/burgundy.c | 4
> sound/ppc/pmac.c | 18 +-
> sound/soc/fsl/efika-audio-fabric.c | 2
> sound/soc/fsl/pcm030-audio-fabric.c | 2
> 48 files changed, 317 insertions(+), 575 deletions(-)
>
--
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian
^ permalink raw reply
* Re: [PATCH] Restrict initial stack space expansion to rlimit
From: Michael Neuling @ 2010-02-09 8:59 UTC (permalink / raw)
To: KOSAKI Motohiro
Cc: stable, aeb, Oleg Nesterov, miltonm, James Morris, linuxppc-dev,
Paul Mackerras, Anton Blanchard, Serge Hallyn, linux-fsdevel,
Americo Wang, Andrew Morton, Linus Torvalds, Ingo Molnar,
linux-kernel, Alexander Viro
In-Reply-To: <20100209154141.03F0.A69D9226@jp.fujitsu.com>
In message <20100209154141.03F0.A69D9226@jp.fujitsu.com> you wrote:
> > When reserving stack space for a new process, make sure we're not
> > attempting to expand the stack by more than rlimit allows.
> >
> > This fixes a bug caused by b6a2fea39318e43fee84fa7b0b90d68bed92d2ba "mm:
> > variable length argument support" and unmasked by
> > fc63cf237078c86214abcb2ee9926d8ad289da9b "exec: setup_arg_pages() fails
> > to return errors". This bug means when limiting the stack to less the
> > 20*PAGE_SIZE (eg. 80K on 4K pages or 'ulimit -s 79') all processes will
> > be killed before they start. This is particularly bad with 64K pages,
> > where a ulimit below 1280K will kill every process.
> >
> > Signed-off-by: Michael Neuling <mikey@neuling.org>
> > Cc: stable@kernel.org
> > ---
> > Attempts to answer comments from Kosaki Motohiro.
> >
> > Tested on PPC only, hence !CONFIG_STACK_GROWSUP. Someone should
> > probably ACK for an arch with CONFIG_STACK_GROWSUP.
> >
> > As noted, stable needs the same patch, but 2.6.32 doesn't have the
> > rlimit() helper.
> >
> > fs/exec.c | 21 ++++++++++++++++++---
> > 1 file changed, 18 insertions(+), 3 deletions(-)
> >
> > Index: linux-2.6-ozlabs/fs/exec.c
> > ===================================================================
> > --- linux-2.6-ozlabs.orig/fs/exec.c
> > +++ linux-2.6-ozlabs/fs/exec.c
> > @@ -555,6 +555,7 @@ static int shift_arg_pages(struct vm_are
> > }
> >
> > #define EXTRA_STACK_VM_PAGES 20 /* random */
> > +#define ALIGN_DOWN(addr,size) ((addr)&(~((size)-1)))
> >
> > /*
> > * Finalizes the stack vm_area_struct. The flags and permissions are updat
ed,
> > @@ -570,7 +571,7 @@ int setup_arg_pages(struct linux_binprm
> > struct vm_area_struct *vma = bprm->vma;
> > struct vm_area_struct *prev = NULL;
> > unsigned long vm_flags;
> > - unsigned long stack_base;
> > + unsigned long stack_base, stack_expand, stack_expand_lim, stack_size;
> >
> > #ifdef CONFIG_STACK_GROWSUP
> > /* Limit stack size to 1GB */
> > @@ -627,10 +628,24 @@ int setup_arg_pages(struct linux_binprm
> > goto out_unlock;
> > }
> >
> > + stack_expand = EXTRA_STACK_VM_PAGES * PAGE_SIZE;
> > + stack_size = vma->vm_end - vma->vm_start;
> > + if (rlimit(RLIMIT_STACK) < stack_size)
> > + stack_expand_lim = 0; /* don't shrick the stack */
> > + else
> > + /*
> > + * Align this down to a page boundary as expand_stack
> > + * will align it up.
> > + */
> > + stack_expand_lim = ALIGN_DOWN(rlimit(RLIMIT_STACK) - stack_size
,
> > + PAGE_SIZE);
> > + /* Initial stack must not cause stack overflow. */
> > + if (stack_expand > stack_expand_lim)
> > + stack_expand = stack_expand_lim;
> > #ifdef CONFIG_STACK_GROWSUP
> > - stack_base = vma->vm_end + EXTRA_STACK_VM_PAGES * PAGE_SIZE;
> > + stack_base = vma->vm_end + stack_expand;
> > #else
> > - stack_base = vma->vm_start - EXTRA_STACK_VM_PAGES * PAGE_SIZE;
> > + stack_base = vma->vm_start - stack_expand;
> > #endif
> > ret = expand_stack(vma, stack_base);
> > if (ret)
>
> Umm.. It looks correct. but the nested complex if statement seems a bit ugly.
> Instead, How about following?
I don't like the duplicated code in the #ifdef/else but I can live with it.
> note: it's untested.
Works for me on ppc64 with 4k and 64k pages. Thanks!
I'd still like someone with a CONFIG_STACK_GROWSUP arch to test/ACK it
as well.
Mikey
>
>
>
> ===============
> From: Michael Neuling <mikey@neuling.org>
> Subject: Restrict initial stack space expansion to rlimit
>
> When reserving stack space for a new process, make sure we're not
> attempting to expand the stack by more than rlimit allows.
>
> This fixes a bug caused by b6a2fea39318e43fee84fa7b0b90d68bed92d2ba "mm:
> variable length argument support" and unmasked by
> fc63cf237078c86214abcb2ee9926d8ad289da9b "exec: setup_arg_pages() fails
> to return errors". This bug means when limiting the stack to less the
> 20*PAGE_SIZE (eg. 80K on 4K pages or 'ulimit -s 79') all processes will
> be killed before they start. This is particularly bad with 64K pages,
> where a ulimit below 1280K will kill every process.
>
> [kosaki.motohiro@jp.fujitsu.com: cleanups]
> Signed-off-by: Michael Neuling <mikey@neuling.org>
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Cc: stable@kernel.org
> ---
> Attempts to answer comments from Kosaki Motohiro.
>
> Tested on PPC only, hence !CONFIG_STACK_GROWSUP. Someone should
> probably ACK for an arch with CONFIG_STACK_GROWSUP.
>
> As noted, stable needs the same patch, but 2.6.32 doesn't have the
> rlimit() helper.
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 6f7fb0c..325bad4 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -573,6 +573,9 @@ int setup_arg_pages(struct linux_binprm *bprm,
> struct vm_area_struct *prev = NULL;
> unsigned long vm_flags;
> unsigned long stack_base;
> + unsigned long stack_size;
> + unsigned long stack_expand;
> + unsigned long rlim_stack;
>
> #ifdef CONFIG_STACK_GROWSUP
> /* Limit stack size to 1GB */
> @@ -629,10 +632,27 @@ int setup_arg_pages(struct linux_binprm *bprm,
> goto out_unlock;
> }
>
> + stack_expand = EXTRA_STACK_VM_PAGES * PAGE_SIZE;
> + stack_size = vma->vm_end - vma->vm_start;
> + /*
> + * Align this down to a page boundary as expand_stack
> + * will align it up.
> + */
> + rlim_stack = rlimit(RLIMIT_STACK) & PAGE_MASK;
> + if (rlim_stack < stack_size)
> + rlim_stack = stack_size;
> #ifdef CONFIG_STACK_GROWSUP
> - stack_base = vma->vm_end + EXTRA_STACK_VM_PAGES * PAGE_SIZE;
> + if (stack_size + stack_expand > rlim_stack) {
> + stack_base = vma->vm_start + rlim_stack;
> + } else {
> + stack_base = vma->vm_end + stack_expand;
> + }
> #else
> - stack_base = vma->vm_start - EXTRA_STACK_VM_PAGES * PAGE_SIZE;
> + if (stack_size + stack_expand > rlim_stack) {
> + stack_base = vma->vm_end - rlim_stack;
> + } else {
> + stack_base = vma->vm_start - stack_expand;
> + }
> #endif
> ret = expand_stack(vma, stack_base);
> if (ret)
>
>
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox