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

From: "Rafał Miłecki" <zajec5@gmail.com>
To: David Lin <dlin@marvell.com>
Cc: Steve deRosier <derosier@gmail.com>,
	Kalle Valo <kvalo@codeaurora.org>,
	"linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
	Johannes Berg <johannes@sipsolutions.net>,
	Chor Teck Law <ctlaw@marvell.com>, James Lin <jamesl@marvell.com>,
	Pete Hsieh <peteh@marvell.com>
Subject: Re: [PATCH v9] Add new mac80211 driver mwlwifi.
Date: Wed, 21 Nov 2018 09:24:55 +0100	[thread overview]
Message-ID: <CACna6rwXq2PJmNxX8j9V2yMTtCeFHf5-GrjcA-U8A1bPW9pqPg@mail.gmail.com> (raw)
In-Reply-To: <4b33da1fb23a476ca5bc21eb74f1ffd2@SC-EXCH02.marvell.com>

On Wed, 8 Feb 2017 at 08:55, David Lin <dlin@marvell.com> wrote:
> > From: Rafał Miłecki [mailto:zajec5@gmail.com] worte:
> > On 8 February 2017 at 08:28, David Lin <dlin@marvell.com> wrote:
> > >> From: Rafał Miłecki [mailto:zajec5@gmail.com] worte:
> > >> Sent: Wednesday, February 08, 2017 3:24 PM On 8 February 2017 at
> > >> 07:30, David Lin <dlin@marvell.com> wrote:
> > >> > steve.derosier@gmail.com [mailto:steve.derosier@gmail.com] wrote:
> > >> >> On Tue, Feb 7, 2017 at 10:15 PM, David Lin <dlin@marvell.com> wrote:
> > >> >> >> Rafał Miłecki [mailto:zajec5@gmail.com] wrote:
> > >> >> >> Please use ieee80211-freq-limit:
> > >> >> >> https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git
> > >> >> >> /co
> > >> >> >> mmi
> > >> >> >> t/?id=b3
> > >> >> >> 30b25eaabda00d74e47566d9200907da381896
> > >> >> >>
> > >> >> >> Most likely with wiphy_read_of_freq_limits helper:
> > >> >> >> https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git
> > >> >> >> /co
> > >> >> >> mmi
> > >> >> >> t/?id=e6
> > >> >> >> 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
> > >> >
> > >> > These parameters are marvell proprietary parameters, I don't think
> > >> > it should
> > >> be added to DT bindings.
> > >>
> > >> Steve is correct.
> > >>
> > >> You have to document new properties, just because they are Marvell
> > >> specific doesn't change anything. You just document them in a proper
> > place.
> > >>
> > >
> > > All right. I will do that.
> > >
> > >>
> > >> > BTW, <marvell,2ghz> and <marvell,5ghz> are only used for mwlwifi to
> > >> > report
> > >> supported bands, it is not related to limitation of frequency.
> > >>
> > >> How reporting a single band doesn't limit reported frequencies? You
> > >> can achieve exactly the same using generic property, so there is no
> > >> place for Marvell specific ones.
> > >>
> > >> In fact there were drivers of 3 vendors requiring band/freq-related
> > >> description in DT: Broadcom, Marvell & Mediatek. This property was
> > >> discussed & designed to support all limitation cases we found
> > >> possible to make it usable with
> > >> (hopefully) all drivers.
> > >>
> > >
> > > I only need simple way to disable 2g or 5g band. I will follow your suggestion
> > to document these marvell proprietary parameters.
> >
> > Seriously? Refusing to use generic binding because you think marvell,5ghz; is
> > simpler than ieee80211-freq-limit = <2402000 2482000>; (not to mention your
> > property seems reversed!)?
> >
> > I don't know how else to explain this to you. We don't want duplicated
> > properties where one can work. Just use existing one. Don't add new one even
> > if documented.
> >
>
> All right. I will check and let patch v10 to use it. For previous parameters, they will only be used by previous OpenWrt projects.

Hi David, how are things going? Could we get v10?

-- 
Rafał