diff mbox series

[1/2] dt-binding: bcm43xx-fmac: add optional brcm,ccode-map

Message ID 20210408113022.18180-2-shawn.guo@linaro.org
State Deferred
Delegated to: Kalle Valo
Headers show
Series brcmfmac: support parse country code map from DT | expand

Commit Message

Shawn Guo April 8, 2021, 11:30 a.m. UTC
Add optional brcm,ccode-map property to support translation from ISO3166
country code to brcmfmac firmware country code and revision.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 .../devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Rob Herring April 9, 2021, 6:46 p.m. UTC | #1
On Thu, Apr 08, 2021 at 07:30:21PM +0800, Shawn Guo wrote:
> Add optional brcm,ccode-map property to support translation from ISO3166
> country code to brcmfmac firmware country code and revision.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  .../devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt | 7 +++++++
>  1 file changed, 7 insertions(+)

Can you convert this to schema first.

> 
> diff --git a/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt b/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
> index cffb2d6876e3..a65ac4384c04 100644
> --- a/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
> +++ b/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
> @@ -15,6 +15,12 @@ Optional properties:
>  	When not specified the device will use in-band SDIO interrupts.
>   - interrupt-names : name of the out-of-band interrupt, which must be set
>  	to "host-wake".
> + - brcm,ccode-map : multiple strings for translating ISO3166 country code to
> +	brcmfmac firmware country code and revision.  Each string must be in
> +	format "AA-BB-num" where:
> +	  AA is the ISO3166 country code which must be 2 characters.
> +	  BB is the firmware country code which must be 2 characters.
> +	  num is the revision number which must fit into signed integer.

Signed? So "AA-BB--num"?

You should be able to do something like:

items:
  pattern: '^[A-Z][A-Z]-[A-Z][A-Z]-[0-9]+$'

>  
>  Example:
>  
> @@ -34,5 +40,6 @@ mmc3: mmc@1c12000 {
>  		interrupt-parent = <&pio>;
>  		interrupts = <10 8>; /* PH10 / EINT10 */
>  		interrupt-names = "host-wake";
> +		brcm,ccode-map = "JP-JP-78", "US-Q2-86";
>  	};
>  };
> -- 
> 2.17.1
>
Kalle Valo April 11, 2021, 7:57 a.m. UTC | #2
Shawn Guo <shawn.guo@linaro.org> writes:

> Add optional brcm,ccode-map property to support translation from ISO3166
> country code to brcmfmac firmware country code and revision.
>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  .../devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt b/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
> index cffb2d6876e3..a65ac4384c04 100644
> --- a/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
> +++ b/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
> @@ -15,6 +15,12 @@ Optional properties:
>  	When not specified the device will use in-band SDIO interrupts.
>   - interrupt-names : name of the out-of-band interrupt, which must be set
>  	to "host-wake".
> + - brcm,ccode-map : multiple strings for translating ISO3166 country code to
> +	brcmfmac firmware country code and revision.  Each string must be in
> +	format "AA-BB-num" where:
> +	  AA is the ISO3166 country code which must be 2 characters.
> +	  BB is the firmware country code which must be 2 characters.
> +	  num is the revision number which must fit into signed integer.
>  
>  Example:
>  
> @@ -34,5 +40,6 @@ mmc3: mmc@1c12000 {
>  		interrupt-parent = <&pio>;
>  		interrupts = <10 8>; /* PH10 / EINT10 */
>  		interrupt-names = "host-wake";
> +		brcm,ccode-map = "JP-JP-78", "US-Q2-86";

The commit log does not answer "Why?". Why this needs to be in device
tree and, for example, not hard coded in the driver?
Shawn Guo April 12, 2021, 1:19 a.m. UTC | #3
On Fri, Apr 09, 2021 at 01:46:06PM -0500, Rob Herring wrote:
> On Thu, Apr 08, 2021 at 07:30:21PM +0800, Shawn Guo wrote:
> > Add optional brcm,ccode-map property to support translation from ISO3166
> > country code to brcmfmac firmware country code and revision.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > ---
> >  .../devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt | 7 +++++++
> >  1 file changed, 7 insertions(+)
> 
> Can you convert this to schema first.

Yes.  Will do, after driver maintainers agree with the direction.
> 
> > 
> > diff --git a/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt b/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
> > index cffb2d6876e3..a65ac4384c04 100644
> > --- a/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
> > +++ b/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
> > @@ -15,6 +15,12 @@ Optional properties:
> >  	When not specified the device will use in-band SDIO interrupts.
> >   - interrupt-names : name of the out-of-band interrupt, which must be set
> >  	to "host-wake".
> > + - brcm,ccode-map : multiple strings for translating ISO3166 country code to
> > +	brcmfmac firmware country code and revision.  Each string must be in
> > +	format "AA-BB-num" where:
> > +	  AA is the ISO3166 country code which must be 2 characters.
> > +	  BB is the firmware country code which must be 2 characters.
> > +	  num is the revision number which must fit into signed integer.
> 
> Signed? So "AA-BB--num"?

Hmm, for some reason, kernel driver uses signed integer to hold the
revision.  It's just a reflecting of that.

> 
> You should be able to do something like:
> 
> items:
>   pattern: '^[A-Z][A-Z]-[A-Z][A-Z]-[0-9]+$'

Ah, yes, that's much better and distinct.  Thanks for the suggestion.

Shawn

> 
> >  
> >  Example:
> >  
> > @@ -34,5 +40,6 @@ mmc3: mmc@1c12000 {
> >  		interrupt-parent = <&pio>;
> >  		interrupts = <10 8>; /* PH10 / EINT10 */
> >  		interrupt-names = "host-wake";
> > +		brcm,ccode-map = "JP-JP-78", "US-Q2-86";
> >  	};
> >  };
> > -- 
> > 2.17.1
> >
Shawn Guo April 12, 2021, 1:25 a.m. UTC | #4
On Sun, Apr 11, 2021 at 10:57:54AM +0300, Kalle Valo wrote:
> Shawn Guo <shawn.guo@linaro.org> writes:
> 
> > Add optional brcm,ccode-map property to support translation from ISO3166
> > country code to brcmfmac firmware country code and revision.
> >
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > ---
> >  .../devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt b/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
> > index cffb2d6876e3..a65ac4384c04 100644
> > --- a/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
> > +++ b/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
> > @@ -15,6 +15,12 @@ Optional properties:
> >  	When not specified the device will use in-band SDIO interrupts.
> >   - interrupt-names : name of the out-of-band interrupt, which must be set
> >  	to "host-wake".
> > + - brcm,ccode-map : multiple strings for translating ISO3166 country code to
> > +	brcmfmac firmware country code and revision.  Each string must be in
> > +	format "AA-BB-num" where:
> > +	  AA is the ISO3166 country code which must be 2 characters.
> > +	  BB is the firmware country code which must be 2 characters.
> > +	  num is the revision number which must fit into signed integer.
> >  
> >  Example:
> >  
> > @@ -34,5 +40,6 @@ mmc3: mmc@1c12000 {
> >  		interrupt-parent = <&pio>;
> >  		interrupts = <10 8>; /* PH10 / EINT10 */
> >  		interrupt-names = "host-wake";
> > +		brcm,ccode-map = "JP-JP-78", "US-Q2-86";
> 
> The commit log does not answer "Why?". Why this needs to be in device
> tree and, for example, not hard coded in the driver?

Thanks for the comment, Kalle.  Actually, this is something I need some
input from driver maintainers.  I can see this country code mapping
table is chipset specific, and can be hard coded in driver per chip id
and revision.  But on the other hand, it makes some sense to have this
table in device tree, as the country code that need to be supported
could be a device specific configuration.

Shawn
Arend Van Spriel April 12, 2021, 7:48 a.m. UTC | #5
On 08-04-2021 13:30, Shawn Guo wrote:
> Add optional brcm,ccode-map property to support translation from ISO3166
> country code to brcmfmac firmware country code and revision.

Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>   .../devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt | 7 +++++++
>   1 file changed, 7 insertions(+)
Kalle Valo April 12, 2021, 11:54 a.m. UTC | #6
Shawn Guo <shawn.guo@linaro.org> writes:

> On Sun, Apr 11, 2021 at 10:57:54AM +0300, Kalle Valo wrote:
>> Shawn Guo <shawn.guo@linaro.org> writes:
>> 
>> > Add optional brcm,ccode-map property to support translation from ISO3166
>> > country code to brcmfmac firmware country code and revision.
>> >
>> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
>> > ---
>> >  .../devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt | 7 +++++++
>> >  1 file changed, 7 insertions(+)
>> >
>> > diff --git a/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt b/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
>> > index cffb2d6876e3..a65ac4384c04 100644
>> > --- a/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
>> > +++ b/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
>> > @@ -15,6 +15,12 @@ Optional properties:
>> >  	When not specified the device will use in-band SDIO interrupts.
>> >   - interrupt-names : name of the out-of-band interrupt, which must be set
>> >  	to "host-wake".
>> > + - brcm,ccode-map : multiple strings for translating ISO3166 country code to
>> > +	brcmfmac firmware country code and revision.  Each string must be in
>> > +	format "AA-BB-num" where:
>> > +	  AA is the ISO3166 country code which must be 2 characters.
>> > +	  BB is the firmware country code which must be 2 characters.
>> > +	  num is the revision number which must fit into signed integer.
>> >  
>> >  Example:
>> >  
>> > @@ -34,5 +40,6 @@ mmc3: mmc@1c12000 {
>> >  		interrupt-parent = <&pio>;
>> >  		interrupts = <10 8>; /* PH10 / EINT10 */
>> >  		interrupt-names = "host-wake";
>> > +		brcm,ccode-map = "JP-JP-78", "US-Q2-86";
>> 
>> The commit log does not answer "Why?". Why this needs to be in device
>> tree and, for example, not hard coded in the driver?
>
> Thanks for the comment, Kalle.  Actually, this is something I need some
> input from driver maintainers.  I can see this country code mapping
> table is chipset specific, and can be hard coded in driver per chip id
> and revision.  But on the other hand, it makes some sense to have this
> table in device tree, as the country code that need to be supported
> could be a device specific configuration.

Could be? Does such a use case exist at the moment or are just guessing
future needs?

From what I have learned so far I think this kind of data should be in
the driver, but of course I might be missing something.
Shawn Guo April 13, 2021, 7:28 a.m. UTC | #7
On Mon, Apr 12, 2021 at 02:54:46PM +0300, Kalle Valo wrote:
> Shawn Guo <shawn.guo@linaro.org> writes:
> 
> > On Sun, Apr 11, 2021 at 10:57:54AM +0300, Kalle Valo wrote:
> >> Shawn Guo <shawn.guo@linaro.org> writes:
> >> 
> >> > Add optional brcm,ccode-map property to support translation from ISO3166
> >> > country code to brcmfmac firmware country code and revision.
> >> >
> >> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> >> > ---
> >> >  .../devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt | 7 +++++++
> >> >  1 file changed, 7 insertions(+)
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt b/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
> >> > index cffb2d6876e3..a65ac4384c04 100644
> >> > --- a/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
> >> > +++ b/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
> >> > @@ -15,6 +15,12 @@ Optional properties:
> >> >  	When not specified the device will use in-band SDIO interrupts.
> >> >   - interrupt-names : name of the out-of-band interrupt, which must be set
> >> >  	to "host-wake".
> >> > + - brcm,ccode-map : multiple strings for translating ISO3166 country code to
> >> > +	brcmfmac firmware country code and revision.  Each string must be in
> >> > +	format "AA-BB-num" where:
> >> > +	  AA is the ISO3166 country code which must be 2 characters.
> >> > +	  BB is the firmware country code which must be 2 characters.
> >> > +	  num is the revision number which must fit into signed integer.
> >> >  
> >> >  Example:
> >> >  
> >> > @@ -34,5 +40,6 @@ mmc3: mmc@1c12000 {
> >> >  		interrupt-parent = <&pio>;
> >> >  		interrupts = <10 8>; /* PH10 / EINT10 */
> >> >  		interrupt-names = "host-wake";
> >> > +		brcm,ccode-map = "JP-JP-78", "US-Q2-86";
> >> 
> >> The commit log does not answer "Why?". Why this needs to be in device
> >> tree and, for example, not hard coded in the driver?
> >
> > Thanks for the comment, Kalle.  Actually, this is something I need some
> > input from driver maintainers.  I can see this country code mapping
> > table is chipset specific, and can be hard coded in driver per chip id
> > and revision.  But on the other hand, it makes some sense to have this
> > table in device tree, as the country code that need to be supported
> > could be a device specific configuration.
> 
> Could be? Does such a use case exist at the moment or are just guessing
> future needs?

I hope that the patch [1] from RafaƂ (copied) is one use case.  And
also, the device I'm working on only needs to support some of the
countries in the mapping table. 

> 
> From what I have learned so far I think this kind of data should be in
> the driver, but of course I might be missing something.

I agree with you that such data are chipset specific and should ideally
be in the driver.  However, the brcmfmac driver implementation has been
taking the mapping table from platform_data [2][3], which is a logical
equivalent of DT data in case of booting with device tree.

Shawn

[1] https://gitlab.dai-labor.de/nadim/powquty-coap/-/blob/563b2bd658822375dcfa8e87707304b94de9901c/kernel/mac80211/patches/863-brcmfmac-add-in-driver-tables-with-country-codes.patch
[2] https://elixir.bootlin.com/linux/v5.12-rc7/source/include/linux/platform_data/brcmfmac.h#L154
[3] https://elixir.bootlin.com/linux/v5.12-rc7/source/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c#L433
Arend Van Spriel April 13, 2021, 12:43 p.m. UTC | #8
On 09-04-2021 20:46, Rob Herring wrote:
> On Thu, Apr 08, 2021 at 07:30:21PM +0800, Shawn Guo wrote:
>> Add optional brcm,ccode-map property to support translation from ISO3166
>> country code to brcmfmac firmware country code and revision.
>>
>> Signed-off-by: Shawn Guo<shawn.guo@linaro.org>
>> ---
>>   .../devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt | 7 +++++++
>>   1 file changed, 7 insertions(+)
> Can you convert this to schema first.
Hi Rob,

You mean to change it to YAML, right? You already applied a patch for 
that a few weeks ago:

https://lore.kernel.org/linux-devicetree/20210324174737.GA3319687@robh.at.kernel.org/

Regards,
Arend
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt b/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
index cffb2d6876e3..a65ac4384c04 100644
--- a/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
+++ b/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
@@ -15,6 +15,12 @@  Optional properties:
 	When not specified the device will use in-band SDIO interrupts.
  - interrupt-names : name of the out-of-band interrupt, which must be set
 	to "host-wake".
+ - brcm,ccode-map : multiple strings for translating ISO3166 country code to
+	brcmfmac firmware country code and revision.  Each string must be in
+	format "AA-BB-num" where:
+	  AA is the ISO3166 country code which must be 2 characters.
+	  BB is the firmware country code which must be 2 characters.
+	  num is the revision number which must fit into signed integer.
 
 Example:
 
@@ -34,5 +40,6 @@  mmc3: mmc@1c12000 {
 		interrupt-parent = <&pio>;
 		interrupts = <10 8>; /* PH10 / EINT10 */
 		interrupt-names = "host-wake";
+		brcm,ccode-map = "JP-JP-78", "US-Q2-86";
 	};
 };