Re: [PATCH v4 2/3] ARM: mvebu: Add quirk for i2c
On 08/01/2014 15:07, Gregory CLEMENT wrote: > Hi Wolfram, > > On 08/01/2014 14:39, Wolfram Sang wrote: >> >>> However, when I first read this I thought it should be a -a0 specific >>> compatible string, not a 'offload-broken' property - any idea what the >>> DT consensus is here? I've seen both approach in use .. >> >> I prefer the replacement of the compatible string. If it should really >> be a seperate property, then it should be a vendor specific property. It >> is not generic, at all. > > Something like "marvell,offload-broken" would be acceptable? A tad more, yes. Still, since this is a feature/quirk of the IP core revision, it should be deduced from the compatible property IMO. It cannot be configured anywhere, so it doesn't change on board level. >>> >>> So you would prefer using the "marvell,mv78230-a0-i2c" comaptible string and >>> updating it with the follwing piece of code? >> >> This is the approach I favour, yes. Can't say much about the >> implementation. Looks OK, but dunno if this is minimal... > > Allocating memory in each loop could seem convoluted. In my first approach > I just used a static struct but I got warning during boot about duplicated > node. It seems we can use the same property struct for two different nodes. Oh! I just meant the opposite: " It seems we can NOT use the same property struct for two different nodes." > >> > > -- Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/3] ARM: mvebu: Add quirk for i2c
On 08/01/2014 14:45, Arnd Bergmann wrote: > On Wednesday 08 January 2014 14:39:59 Wolfram Sang wrote: >>> However, when I first read this I thought it should be a -a0 specific >>> compatible string, not a 'offload-broken' property - any idea what the >>> DT consensus is here? I've seen both approach in use .. >> >> I prefer the replacement of the compatible string. If it should really >> be a seperate property, then it should be a vendor specific property. It >> is not generic, at all. > > Something like "marvell,offload-broken" would be acceptable? A tad more, yes. Still, since this is a feature/quirk of the IP core revision, it should be deduced from the compatible property IMO. It cannot be configured anywhere, so it doesn't change on board level. >>> >>> So you would prefer using the "marvell,mv78230-a0-i2c" comaptible string and >>> updating it with the follwing piece of code? >> >> This is the approach I favour, yes. Can't say much about the >> implementation. Looks OK, but dunno if this is minimal... >> > > I would prefer the separate property in this case, but only because it's > easier to add in the quirk than to change the compatible string, but it's > not a strong preference and I don't mind getting overruled if you all > favor the alternative. Great, all the different part seems to be validated, I can now send the last version. Thanks, Gregory -- Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/3] ARM: mvebu: Add quirk for i2c
Hi Wolfram, On 08/01/2014 14:39, Wolfram Sang wrote: > >> However, when I first read this I thought it should be a -a0 specific >> compatible string, not a 'offload-broken' property - any idea what the >> DT consensus is here? I've seen both approach in use .. > > I prefer the replacement of the compatible string. If it should really > be a seperate property, then it should be a vendor specific property. It > is not generic, at all. Something like "marvell,offload-broken" would be acceptable? >>> >>> A tad more, yes. Still, since this is a feature/quirk of the IP core >>> revision, it should be deduced from the compatible property IMO. It >>> cannot be configured anywhere, so it doesn't change on board level. >> >> So you would prefer using the "marvell,mv78230-a0-i2c" comaptible string and >> updating it with the follwing piece of code? > > This is the approach I favour, yes. Can't say much about the > implementation. Looks OK, but dunno if this is minimal... Allocating memory in each loop could seem convoluted. In my first approach I just used a static struct but I got warning during boot about duplicated node. It seems we can use the same property struct for two different nodes. > -- Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/3] ARM: mvebu: Add quirk for i2c
On Wednesday 08 January 2014 14:39:59 Wolfram Sang wrote: > > However, when I first read this I thought it should be a -a0 specific > > compatible string, not a 'offload-broken' property - any idea what the > > DT consensus is here? I've seen both approach in use .. > > >>> > > >>> I prefer the replacement of the compatible string. If it should really > > >>> be a seperate property, then it should be a vendor specific property. It > > >>> is not generic, at all. > > >> > > >> Something like "marvell,offload-broken" would be acceptable? > > > > > > A tad more, yes. Still, since this is a feature/quirk of the IP core > > > revision, it should be deduced from the compatible property IMO. It > > > cannot be configured anywhere, so it doesn't change on board level. > > > > So you would prefer using the "marvell,mv78230-a0-i2c" comaptible string and > > updating it with the follwing piece of code? > > This is the approach I favour, yes. Can't say much about the > implementation. Looks OK, but dunno if this is minimal... > I would prefer the separate property in this case, but only because it's easier to add in the quirk than to change the compatible string, but it's not a strong preference and I don't mind getting overruled if you all favor the alternative. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/3] ARM: mvebu: Add quirk for i2c
On Wednesday 08 January 2014 14:42:45 Gregory CLEMENT wrote:
> You means something like the following code ?
>
> static void __init armada_370_xp_dt_init(void)
> {
> + if (of_machine_is_compatible("plathome,openblocks-ax3-4"))
> + i2c_quirk();
> of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
> }
Yes, that looks like a good way to do it.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/3] ARM: mvebu: Add quirk for i2c
On 08/01/2014 13:52, Arnd Bergmann wrote:
> On Tuesday 07 January 2014, Gregory CLEMENT wrote:
>
>> static void __init armada_370_xp_dt_init(void)
>> {
>> + i2c_quirk();
>> of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
>> }
>
> I'd prefer to enable the quirk only on machines that we know may be affected,
> i.e.
> OpenBlocks AX3-4. That would make it easier in the future for everyone to
> figure
> out whether they need to include the quirk in their kernels or not, depending
> on whether they want to support these machines. Just a precaution in case we
> end up having lots of quirks in the long run.
You means something like the following code ?
static void __init armada_370_xp_dt_init(void)
{
+ if (of_machine_is_compatible("plathome,openblocks-ax3-4"))
+ i2c_quirk();
of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
}
>
> Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/3] ARM: mvebu: Add quirk for i2c
> However, when I first read this I thought it should be a -a0 specific > compatible string, not a 'offload-broken' property - any idea what the > DT consensus is here? I've seen both approach in use .. > >>> > >>> I prefer the replacement of the compatible string. If it should really > >>> be a seperate property, then it should be a vendor specific property. It > >>> is not generic, at all. > >> > >> Something like "marvell,offload-broken" would be acceptable? > > > > A tad more, yes. Still, since this is a feature/quirk of the IP core > > revision, it should be deduced from the compatible property IMO. It > > cannot be configured anywhere, so it doesn't change on board level. > > So you would prefer using the "marvell,mv78230-a0-i2c" comaptible string and > updating it with the follwing piece of code? This is the approach I favour, yes. Can't say much about the implementation. Looks OK, but dunno if this is minimal... signature.asc Description: Digital signature
Re: [PATCH v4 2/3] ARM: mvebu: Add quirk for i2c
On 08/01/2014 12:29, Wolfram Sang wrote:
> On Wed, Jan 08, 2014 at 11:15:05AM +0100, Gregory CLEMENT wrote:
>> Hi Wolfram,
>>
>> On 08/01/2014 00:06, Wolfram Sang wrote:
>>> On Tue, Jan 07, 2014 at 11:38:53AM -0700, Jason Gunthorpe wrote:
On Tue, Jan 07, 2014 at 05:35:03PM +0100, Gregory CLEMENT wrote:
> +static struct property i2c_offload_broken = {
> + .name = "offload-broken",
> +};
> +
> +static void __init i2c_quirk(void)
> +{
> + struct device_node *np;
> + u32 dev, rev;
> +
> + /*
> + * Only revisons more recent than A0 support the offload
> + * mechanism. We can exit only if we are sure that we can
> + * get the SoC revision and it is more recent than A0.
> + */
> + if (mvebu_get_soc_id(&rev, &dev) == 0 && dev > MV78XX0_A0_REV)
> + return;
> +
> + for_each_compatible_node(np, NULL, "marvell,mv78230-i2c")
> + of_add_property(np, &i2c_offload_broken);
I like this approach.
>>>
>>> Sorry, but I don't.
>>>
However, when I first read this I thought it should be a -a0 specific
compatible string, not a 'offload-broken' property - any idea what the
DT consensus is here? I've seen both approach in use ..
>>>
>>> I prefer the replacement of the compatible string. If it should really
>>> be a seperate property, then it should be a vendor specific property. It
>>> is not generic, at all.
>>
>> Something like "marvell,offload-broken" would be acceptable?
>
> A tad more, yes. Still, since this is a feature/quirk of the IP core
> revision, it should be deduced from the compatible property IMO. It
> cannot be configured anywhere, so it doesn't change on board level.
So you would prefer using the "marvell,mv78230-a0-i2c" comaptible string and
updating it with the follwing piece of code?
diff --git a/arch/arm/mach-mvebu/armada-370-xp.c
b/arch/arm/mach-mvebu/armada-370-xp.c
index e2acff98e750..8c4ecf987e82 100644
--- a/arch/arm/mach-mvebu/armada-370-xp.c
+++ b/arch/arm/mach-mvebu/armada-370-xp.c
@@ -21,6 +21,8 @@
#include
#include
#include
+#include
+#include
#include
#include
#include
@@ -45,8 +47,38 @@ static void __init armada_370_xp_timer_and_clk_init(void)
#endif
}
+static void __init i2c_quirk(void)
+{
+ struct device_node *np;
+ u32 dev, rev;
+ int i = 0;
+
+ /*
+* Only revisons more recent than A0 support the offload
+* mechanism. We can exit only if we are sure that we can
+* get the SoC revision and it is more recent than A0.
+*/
+ if (mvebu_get_soc_id(&rev, &dev) == 0 && dev > MV78XX0_A0_REV)
+ return;
+
+ for_each_compatible_node(np, NULL, "marvell,mv78230-i2c") {
+ struct property *new_compat;
+
+ new_compat = kzalloc(sizeof(*new_compat), GFP_KERNEL);
+
+ new_compat->name = kstrdup("compatible", GFP_KERNEL);
+ new_compat->length = sizeof("marvell,mv78230-a0-i2c");
+ new_compat->value = kstrdup("marvell,mv78230-a0-i2c",
+ GFP_KERNEL);
+
+ of_update_property(np, new_compat);
+ }
+ return;
+}
+
static void __init armada_370_xp_dt_init(void)
{
+ i2c_quirk();
of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
}
>
>
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/3] ARM: mvebu: Add quirk for i2c
On Tuesday 07 January 2014, Gregory CLEMENT wrote:
> static void __init armada_370_xp_dt_init(void)
> {
> + i2c_quirk();
> of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
> }
I'd prefer to enable the quirk only on machines that we know may be affected,
i.e.
OpenBlocks AX3-4. That would make it easier in the future for everyone to figure
out whether they need to include the quirk in their kernels or not, depending
on whether they want to support these machines. Just a precaution in case we
end up having lots of quirks in the long run.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/3] ARM: mvebu: Add quirk for i2c
On Wed, Jan 08, 2014 at 11:15:05AM +0100, Gregory CLEMENT wrote:
> Hi Wolfram,
>
> On 08/01/2014 00:06, Wolfram Sang wrote:
> > On Tue, Jan 07, 2014 at 11:38:53AM -0700, Jason Gunthorpe wrote:
> >> On Tue, Jan 07, 2014 at 05:35:03PM +0100, Gregory CLEMENT wrote:
> >>> +static struct property i2c_offload_broken = {
> >>> + .name = "offload-broken",
> >>> +};
> >>> +
> >>> +static void __init i2c_quirk(void)
> >>> +{
> >>> + struct device_node *np;
> >>> + u32 dev, rev;
> >>> +
> >>> + /*
> >>> + * Only revisons more recent than A0 support the offload
> >>> + * mechanism. We can exit only if we are sure that we can
> >>> + * get the SoC revision and it is more recent than A0.
> >>> + */
> >>> + if (mvebu_get_soc_id(&rev, &dev) == 0 && dev > MV78XX0_A0_REV)
> >>> + return;
> >>> +
> >>> + for_each_compatible_node(np, NULL, "marvell,mv78230-i2c")
> >>> + of_add_property(np, &i2c_offload_broken);
> >>
> >> I like this approach.
> >
> > Sorry, but I don't.
> >
> >> However, when I first read this I thought it should be a -a0 specific
> >> compatible string, not a 'offload-broken' property - any idea what the
> >> DT consensus is here? I've seen both approach in use ..
> >
> > I prefer the replacement of the compatible string. If it should really
> > be a seperate property, then it should be a vendor specific property. It
> > is not generic, at all.
>
> Something like "marvell,offload-broken" would be acceptable?
A tad more, yes. Still, since this is a feature/quirk of the IP core
revision, it should be deduced from the compatible property IMO. It
cannot be configured anywhere, so it doesn't change on board level.
signature.asc
Description: Digital signature
Re: [PATCH v4 2/3] ARM: mvebu: Add quirk for i2c
Hi Wolfram,
On 08/01/2014 00:06, Wolfram Sang wrote:
> On Tue, Jan 07, 2014 at 11:38:53AM -0700, Jason Gunthorpe wrote:
>> On Tue, Jan 07, 2014 at 05:35:03PM +0100, Gregory CLEMENT wrote:
>>> +static struct property i2c_offload_broken = {
>>> + .name = "offload-broken",
>>> +};
>>> +
>>> +static void __init i2c_quirk(void)
>>> +{
>>> + struct device_node *np;
>>> + u32 dev, rev;
>>> +
>>> + /*
>>> +* Only revisons more recent than A0 support the offload
>>> +* mechanism. We can exit only if we are sure that we can
>>> +* get the SoC revision and it is more recent than A0.
>>> +*/
>>> + if (mvebu_get_soc_id(&rev, &dev) == 0 && dev > MV78XX0_A0_REV)
>>> + return;
>>> +
>>> + for_each_compatible_node(np, NULL, "marvell,mv78230-i2c")
>>> + of_add_property(np, &i2c_offload_broken);
>>
>> I like this approach.
>
> Sorry, but I don't.
>
>> However, when I first read this I thought it should be a -a0 specific
>> compatible string, not a 'offload-broken' property - any idea what the
>> DT consensus is here? I've seen both approach in use ..
>
> I prefer the replacement of the compatible string. If it should really
> be a seperate property, then it should be a vendor specific property. It
> is not generic, at all.
Something like "marvell,offload-broken" would be acceptable?
Thanks,
Gregory
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/3] ARM: mvebu: Add quirk for i2c
On Tue, Jan 07, 2014 at 11:38:53AM -0700, Jason Gunthorpe wrote:
> On Tue, Jan 07, 2014 at 05:35:03PM +0100, Gregory CLEMENT wrote:
> > +static struct property i2c_offload_broken = {
> > + .name = "offload-broken",
> > +};
> > +
> > +static void __init i2c_quirk(void)
> > +{
> > + struct device_node *np;
> > + u32 dev, rev;
> > +
> > + /*
> > +* Only revisons more recent than A0 support the offload
> > +* mechanism. We can exit only if we are sure that we can
> > +* get the SoC revision and it is more recent than A0.
> > +*/
> > + if (mvebu_get_soc_id(&rev, &dev) == 0 && dev > MV78XX0_A0_REV)
> > + return;
> > +
> > + for_each_compatible_node(np, NULL, "marvell,mv78230-i2c")
> > + of_add_property(np, &i2c_offload_broken);
>
> I like this approach.
Sorry, but I don't.
> However, when I first read this I thought it should be a -a0 specific
> compatible string, not a 'offload-broken' property - any idea what the
> DT consensus is here? I've seen both approach in use ..
I prefer the replacement of the compatible string. If it should really
be a seperate property, then it should be a vendor specific property. It
is not generic, at all.
signature.asc
Description: Digital signature
Re: [PATCH v4 2/3] ARM: mvebu: Add quirk for i2c
On Tue, Jan 07, 2014 at 05:35:03PM +0100, Gregory CLEMENT wrote:
> +static struct property i2c_offload_broken = {
> + .name = "offload-broken",
> +};
> +
> +static void __init i2c_quirk(void)
> +{
> + struct device_node *np;
> + u32 dev, rev;
> +
> + /*
> + * Only revisons more recent than A0 support the offload
> + * mechanism. We can exit only if we are sure that we can
> + * get the SoC revision and it is more recent than A0.
> + */
> + if (mvebu_get_soc_id(&rev, &dev) == 0 && dev > MV78XX0_A0_REV)
> + return;
> +
> + for_each_compatible_node(np, NULL, "marvell,mv78230-i2c")
> + of_add_property(np, &i2c_offload_broken);
I like this approach.
However, when I first read this I thought it should be a -a0 specific
compatible string, not a 'offload-broken' property - any idea what the
DT consensus is here? I've seen both approach in use ..
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
