On Sun, Feb 05, 2012 at 10:59:40AM -0800, Tony Lindgren wrote:
> * Russell King - ARM Linux <linux@xxxxxxxxxxxxxxxx> [120205 10:10]:
> > On Sun, Feb 05, 2012 at 10:33:16AM -0800, Tony Lindgren wrote:
> > > * Russell King - ARM Linux <linux@xxxxxxxxxxxxxxxx> [120205 09:37]:
> > > > So in the meantime, people should put up with the kernel reporting an
> > > > "Error" at error-message level at boot time because they didn't configure
> > > > something?
> > > >
> > > > No, it needs fixing, because it doesn't justify being an error. It's
> > > > wrong, plain and simple. Again, if you don't want to send it during
> > > > -rc, I'll send it to Linus as a patch for him to decide whether he wants
> > > > to take it as -rc material.
> > >
> > > Hmm, maybe I misunderstood you.
> > >
> > > Certainly fixing the "Error" makes sense for -rc, but are also thinking
> > > about adding error checking to all platform_device_register() calls?
> >
> > I do think they're valid for -rc, because should it fail, things won't
> > work as one desires.
>
> OK, this easily gets into the area where we might get flames from
> Linus like "fixing features during -rc that never worked properly
> before"..
There is a sliding scale of what's acceptable during the -rc.
Certainly this kind of thing would not be welcome during the final
-rc or even the previous -rc, but we're not there yet.
And it's not like it's a feature that never worked.
> I doubt that there's anything else behind it except trying to leave
> out some ifdefs.
I don't think you've properly understood what _should_ be going on
here. Let me illustrate one possibility of how these code sections
might look:
+static void __init xxx_wl12xx_init(void)
+{
+#ifdef CONFIG_WL12XX_PLATFORM_DATA
+ int ret;
+
+ /* WL12xx WLAN Init */
+ ret = wl12xx_set_platform_data(&xxx_wlan_data);
+ if (ret) {
+ pr_err("error setting wl12xx data: %d\n", ret);
+ return;
+ }
+ ret = platform_device_register(&xxx_wlan_regulator);
+ if (ret)
+ pr_err("error registering wl12xx device: %d\n", ret);
+#endif
+}
In that case, the xxx_wlan_regulator device should also be ifdef'd.
In other words, if we fail to setup the platform data, we avoid
registering the device.
But... wait a moment, that device isn't the actual card itself but is a
regulator for the card, and that makes it even less clear whether the
ifdef should include it. So, maybe this is the proper solution:
+static void __init xxx_wl12xx_init(void)
+{
+ int ret;
+
+#ifdef CONFIG_WL12XX_PLATFORM_DATA
+ /* WL12xx WLAN Init */
+ ret = wl12xx_set_platform_data(&xxx_wlan_data);
+ if (ret)
+ pr_err("error setting wl12xx data: %d\n", ret);
+#endif
+ ret = platform_device_register(&xxx_wlan_regulator);
+ if (ret)
+ pr_err("error registering wl12xx device: %d\n", ret);
+}
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
