Re: [PATCH v9] Add new mac80211 driver mwlwifi.

[Steve deRosier <derosier@gmail.com>]

On Tue, Feb 7, 2017 at 10:15 PM, David Lin <dlin@marvell.com> wrote:
>> Rafa=C5=82 Mi=C5=82ecki [mailto:zajec5@gmail.com] wrote:
>> On 7 February 2017 at 20:12, Steve deRosier <derosier@gmail.com> wrote:
>> >> + /* look for all matching property names */
>> >> + for_each_property_of_node(priv->dt_node, prop) { if
>> >> + (strcmp(prop->name, "marvell,2ghz") =3D=3D 0)
>> >> + priv->disable_2g =3D true;
>> >> + if (strcmp(prop->name, "marvell,5ghz") =3D=3D 0)
>> >> + priv->disable_5g =3D true;
>> >> + if (strcmp(prop->name, "marvell,chainmask") =3D=3D 0) { prop_value =
=3D
>> >> + be32_to_cpu(*((__be32 *)prop->value)); if (prop_value =3D=3D 2)
>> >> + priv->antenna_tx =3D ANTENNA_TX_2;
>> >> +
>> >> + prop_value =3D be32_to_cpu(*((__be32 *) (prop->value + 4))); if
>> >> + (prop_value =3D=3D 2)
>> >> + priv->antenna_rx =3D ANTENNA_RX_2;
>> >> + }
>> >> + }
>> >> +
>> >> + priv->pwr_node =3D of_find_node_by_name(priv->dt_node,
>> >> +      "marvell,powertable");
>> >> +#endif
>> >> +}
>> >
>> > AFAICT, there's no documentation for these DT bindings. The mwlwifi
>> > node and the marvell,powertable both need full documentation in
>> > Documentation/devicetree/bindings/... .
>> >
>> > Frankly I have a feeling I'm going to need these DT nodes and I'd like
>> > to not have to guess-and-check based on the code.
>>
>> Please use ieee80211-freq-limit:
>> https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/?=
id=3Db3
>> 30b25eaabda00d74e47566d9200907da381896
>>
>> Most likely with wiphy_read_of_freq_limits helper:
>> https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/?=
id=3De6
>> 91ac2f75b69bee743f0370d79454ba4429b175
>
> I already replied meaning of these parameters:
> <marvell,2ghz> is used to disable 2g band.
> <marvell,5ghz> is used to disable 5g band.
> <marvell, chainmask> is used to specify antenna number (if default number=
 is suitable for you, there is no need to use this parameter).
> <marvell,powertable> should not be used for chip with device power table.=
 In fact, this parameter should not be used any more.
>

David, I think you're not understanding the comment, or at least
that's what it looks like to me. Yes, you did reply as to the meaning.
And, your reply, while informative, didn't tell us you understood and
were willing to fix the problem. I doubt you meant it this way, but it
feels defensive and like a brush-off.

First off, you will still have to document any DT bindings you're
adding. Just because you answer the question in the review doesn't
mean you're not responsible for doing so.

And second off, I think that Rafal (and sorry about my spelling, looks
like there's some sort of accent on the l that I don't know how to
make my keyboard do) is saying: there's already some generic bindings
that can be used to disable the 2g or 5g bands. Granted they're even
newer than your patch, but I do think if said bindings exist and are
appropriate, you should use them.

- Steve