* Russell King - ARM Linux <[email protected]> [120205 11:40]:
> On Sun, Feb 05, 2012 at 10:59:40AM -0800, Tony Lindgren wrote:
> > * Russell King - ARM Linux <[email protected]> [120205 10:10]:
> > > On Sun, Feb 05, 2012 at 10:33:16AM -0800, Tony Lindgren wrote:
> > > > * Russell King - ARM Linux <[email protected]> [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.
Well at least sounds like you have points to argue merging it :)
> > 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);
> +}
Hmm I see. Ohad, care to take a look this one?
Regards,
Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
