--- Em seg, 8/12/08, Jouni Malinen <j@xxxxx> escreveu:
> De: Jouni Malinen <j@xxxxx>
> Assunto: Re: [PATCH] mac80211_hwsim: configfs support
> Para: "Igor Trindade Oliveira" <igor_trindade@xxxxxxxxxxxx>
> Cc: johannes@xxxxxxxxxxxxxxxx, linux-wireless@xxxxxxxxxxxxxxx, linville@xxxxxxxxxxxxx
> Data: Segunda-feira, 8 de Dezembro de 2008, 11:05
> On Fri, Dec 05, 2008 at 01:37:48PM -0800, Igor Trindade
> Oliveira wrote:
>
> > Mac80211_hwsim: Support to configfs operations for
> create a new radio
>
> The change in general looks reasonable, but I have couple
> of comments
> about the details.
>
> Could you please also update
> Documentation/networking/mac80211_hwsim/
> README to replace the kernel module parameter with
> description on how to
> add virtual radios?
Ok i will go to do that.
>
> > +++
> wireless-testing/drivers/net/wireless/mac80211_hwsim.c 2008-12-02
> 06:21:56.000000000 -0400
> > @@ -140,10 +138,15 @@ struct mac80211_hwsim_data {
> > PS_DISABLED, PS_ENABLED, PS_AUTO_POLL,
> PS_MANUAL_POLL
> > } ps;
> > bool ps_poll_pending;
> > - struct dentry *debugfs;
> > - struct dentry *debugfs_ps;
> > + struct config_item item;
> > };
> >
> > +struct mac80211_hwsim_attr {
> > + struct configfs_attribute attr;
> > + ssize_t(*show) (struct mac80211_hwsim_data *hwsim,
> char *buf);
> > + ssize_t(*store) (struct mac80211_hwsim_data *hwsim,
> > + const char *buf, ssize_t count);
>
> There is extra space (<TAB><SPACE>ssize_t) in
> indentation here. As far
> as function pointer style is concerned, my preference would
> be to move
> the space to be before the variable name: ssize_t
> (*show)(struct..).
>
> > +static struct configfs_atribbute
> *mac80211_hwsim_attrs[] = {
>
> Has this been even compile tested? I would assume no, since
> that struct
> name has two typos in it..
>
> > +/*
> > + * now we just have one attribute with this function
> we can
> > + * add new attributes in mac80211_hwsim_attrs and do
> not change
> > + * others parts of the code
> > + */
>
> Is that comment specific to this function or is it just
> description of
> what the change in whole is doing? Is it describing the
> change, it
> should be in commit message, not in the source code. If it
> describe this
> particular function, I would suggest somewhat different
> wording to make
> that cleaner. It could also be moved above
> mac80211_hwsim_attrs
> initialization if it is describing how new attributes are
> added.
It's a comment to describing what we need to do for add new attributes.
>
> > @@ -875,3 +993,4 @@ static void __exit
> exit_mac80211_hwsim(v
> >
> > module_init(init_mac80211_hwsim);
> > module_exit(exit_mac80211_hwsim);
> > +
>
> Please don't add unnecessary (and unwanted) whitespace
> changes.
>
Ok. i will to go to fix that.
> --
> Jouni Malinen
> PGP id EFC895FA
> --
> To unsubscribe from this list: send the line
> "unsubscribe linux-wireless" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at
> http://vger.kernel.org/majordomo-info.html
cheers,
igor
Veja quais são os assuntos do momento no Yahoo! +Buscados
http://br.maisbuscados.yahoo.com
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
