Re: [PATCH] mmc: pwrseq: add support for Marvell SD8787 chip

[Javier Martinez Canillas]

Hello Matt,

On Tue, Nov 29, 2016 at 10:20 PM, Matt Ranostay
<matt@ranostay.consulting> wrote:
> On Tue, Nov 29, 2016 at 9:13 AM, Javier Martinez Canillas

[snip]

>
>
>>> +- pwndn-gpio: contains a power down GPIO specifier.
>>> +- reset-gpio: contains a reset GPIO specifier.
>>> +
>>
>> I wonder if we really need a custom power sequence provider for just
>> this SDIO WiFI chip though. AFAICT the only missing piece in
>> mmc-pwrseq-simple is the power down GPIO property, so maybe
>> mmc-pwrseq-simple could be extended instead to have an optional
>> powerdown-gpios property and instead in the Marvell SD8787 DT binding
>> can be mentioned which mmc-pwrseq-simple properties are required for
>> the device.
>>
>
> The reason we didn't do that is we need delay between the two
> assertions/desertions of GPIOs. It wouldn't seems good practice to
> hack the pwrseq-simple for this...
>

Yes, I noticed that. I wouldn't say that it would be a hack for the
pwrseq-simple since it already has a "post-power-on-delay-ms" DT
property, so AFAICT it would just be adding a "pre-power-on-delay-ms"
property for your use case.

It would also be more consistent since it would support a delay for
pre and post power callbacks. It would also make you avoid hardcoding
the 300 msec wait, in case other device has a similar need but with a
different wait time.

In summary, I think that devices having a power (or power down) and
enable GPIO, and needing to wait between the GPIO toggling are common.
So I would prefer to make pwrseq-simple usable for these instead of
adding device specific power sequence providers. But it's just my
opinion and not my call :-)

>>> +Example:
>>> +
>>> +   wifi_pwrseq: wifi_pwrseq {
>>> +   compatible = "mmc-pwrseq-sd8787";
>>> +   pwrdn-gpio = <_gpio 0 GPIO_ACTIVE_LOW>;
>>> +   reset-gpio = <_gpio 1 GPIO_ACTIVE_LOW>;
>>> +   }
>>> diff --git 
>>> a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt 
>>> b/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
>>
>> Does this patch depend on a previous posted series? I don't see this
>> file in today's linux-next...
>
> Got renamed to ->
> Documentation/devicetree/bindings/net/wireless/marvell-8xxx.txt it
> seems very recently.
>

I see, that's what I thought but I wasn't able to find the commit /
patch that did it.

>>
>>> index c421aba0a5bc..08fd65d35725 100644
>>> --- a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
>>> +++ b/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
>>> @@ -32,6 +32,9 @@ Optional properties:
>>>  so that the wifi chip can wakeup host platform under 
>>> certain condition.
>>>  during system resume, the irq will be disabled to make sure
>>>  unnecessary interrupt is not received.
>>> +  - vmmc-supply: a phandle of a regulator, supplying VCC to the card
>>> +  - mmc-pwrseq:  phandle to the MMC power sequence node. See "mmc-pwrseq-*"
>>> +for documentation of MMC power sequence bindings.
>>>
>>>  Example:
>>>
>>> @@ -44,6 +47,7 @@ so that firmware can wakeup host using this device side 
>>> pin.
>>>   {
>>> status = "okay";
>>> vmmc-supply = <_en_reg>;
>>> +   mmc-pwrseq = <_pwrseq>;
>>> bus-width = <4>;
>>> cap-power-off-card;
>>> keep-power-in-suspend;
>>
>> I think this change should be split in a separate patch as well.
>>

You didn't answer about this, but I guess you agree it should be in a
separate patch.

Best regards,
Javier

Re: [PATCH] mmc: pwrseq: add support for Marvell SD8787 chip

[Javier Martinez Canillas]

Hello Matt,

On Thu, Nov 17, 2016 at 10:55 PM, Matt Ranostay
 wrote:
> Allow power sequencing for the Marvell SD8787 Wifi/BT chip.
> This can be abstracted to other chipsets if needed in the future.
>
> Cc: Tony Lindgren 
> Cc: Ulf Hansson 
> Cc: Mark Rutland 
> Cc: Srinivas Kandagatla 
> Signed-off-by: Matt Ranostay 
> ---
>  .../devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt  |  14 +++
>  .../bindings/net/wireless/marvell-sd8xxx.txt   |   4 +
>  drivers/mmc/core/Kconfig   |  10 ++
>  drivers/mmc/core/Makefile  |   1 +
>  drivers/mmc/core/pwrseq_sd8787.c   | 117 
> +
>  5 files changed, 146 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt
>  create mode 100644 drivers/mmc/core/pwrseq_sd8787.c
>
> diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt 
> b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt

According Documentation/devicetree/bindings/submitting-patches.txt,
the DT bindings patches should posted as a separate patch.

> new file mode 100644
> index ..1b658351629b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt
> @@ -0,0 +1,14 @@
> +* Marvell SD8787 power sequence provider
> +
> +Required properties:
> +- compatible: must be "mmc-pwrseq-sd8787".

Since this is not a generic binding, the compatible string should have
a vendor prefix.

> +- pwndn-gpio: contains a power down GPIO specifier.
> +- reset-gpio: contains a reset GPIO specifier.
> +

I wonder if we really need a custom power sequence provider for just
this SDIO WiFI chip though. AFAICT the only missing piece in
mmc-pwrseq-simple is the power down GPIO property, so maybe
mmc-pwrseq-simple could be extended instead to have an optional
powerdown-gpios property and instead in the Marvell SD8787 DT binding
can be mentioned which mmc-pwrseq-simple properties are required for
the device.

> +Example:
> +
> +   wifi_pwrseq: wifi_pwrseq {
> +   compatible = "mmc-pwrseq-sd8787";
> +   pwrdn-gpio = <_gpio 0 GPIO_ACTIVE_LOW>;
> +   reset-gpio = <_gpio 1 GPIO_ACTIVE_LOW>;
> +   }
> diff --git 
> a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt 
> b/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt

Does this patch depend on a previous posted series? I don't see this
file in today's linux-next...

> index c421aba0a5bc..08fd65d35725 100644
> --- a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
> +++ b/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
> @@ -32,6 +32,9 @@ Optional properties:
>  so that the wifi chip can wakeup host platform under certain 
> condition.
>  during system resume, the irq will be disabled to make sure
>  unnecessary interrupt is not received.
> +  - vmmc-supply: a phandle of a regulator, supplying VCC to the card
> +  - mmc-pwrseq:  phandle to the MMC power sequence node. See "mmc-pwrseq-*"
> +for documentation of MMC power sequence bindings.
>
>  Example:
>
> @@ -44,6 +47,7 @@ so that firmware can wakeup host using this device side pin.
>   {
> status = "okay";
> vmmc-supply = <_en_reg>;
> +   mmc-pwrseq = <_pwrseq>;
> bus-width = <4>;
> cap-power-off-card;
> keep-power-in-suspend;

I think this change should be split in a separate patch as well.

Best regards,
Javier

[PATCH 00/15] drivers: net: use IS_ENABLED() instead of checking for built-in or module

[Javier Martinez Canillas]

Hello David,

This trivial series is similar to [0] for net/ that you already merged, but
for drivers/net. The patches replaces the open coding to check for a Kconfig
symbol being built-in or module, with IS_ENABLED() macro that does the same.

Using the macro makes the code more readable by helping abstract away some
of the Kconfig built-in and module enable details.

[0]: https://lkml.org/lkml/2016/9/9/323

Best regards,
Javier


Javier Martinez Canillas (15):
  3c59x: use IS_ENABLED() instead of checking for built-in or module
  starfire: use IS_ENABLED() instead of checking for built-in or module
  ethernet: amd: use IS_ENABLED() instead of checking for built-in or
module
  bnx2: use IS_ENABLED() instead of checking for built-in or module
  sundance: use IS_ENABLED() instead of checking for built-in or module
  net/fsl_pq_mdio: use IS_ENABLED() instead of checking for built-in or
module
  i825xx: use IS_ENABLED() instead of checking for built-in or module
  ixgbe: use IS_ENABLED() instead of checking for built-in or module
  net: mvneta: use IS_ENABLED() instead of checking for built-in or
module
  natsemi: use IS_ENABLED() instead of checking for built-in or module
  sfc: use IS_ENABLED() instead of checking for built-in or module
  sis900: use IS_ENABLED() instead of checking for built-in or module
  stmmac: use IS_ENABLED() instead of checking for built-in or module
  hamradio: use IS_ENABLED() instead of checking for built-in or module
  iwlegacy: use IS_ENABLED() instead of checking for built-in or module

 drivers/net/ethernet/3com/3c59x.c| 2 +-
 drivers/net/ethernet/adaptec/starfire.c  | 2 +-
 drivers/net/ethernet/amd/7990.c  | 6 +++---
 drivers/net/ethernet/amd/amd8111e.c  | 2 +-
 drivers/net/ethernet/broadcom/bnx2.c | 2 +-
 drivers/net/ethernet/dlink/sundance.c| 2 +-
 drivers/net/ethernet/freescale/fsl_pq_mdio.c | 8 
 drivers/net/ethernet/i825xx/82596.c  | 4 ++--
 drivers/net/ethernet/intel/ixgbe/ixgbe.h | 4 ++--
 drivers/net/ethernet/marvell/mvneta_bm.h | 2 +-
 drivers/net/ethernet/natsemi/ns83820.c   | 2 +-
 drivers/net/ethernet/sfc/falcon_boards.c | 4 ++--
 drivers/net/ethernet/sis/sis900.c| 4 ++--
 drivers/net/ethernet/sis/sis900.h| 2 +-
 drivers/net/ethernet/stmicro/stmmac/common.h | 2 +-
 drivers/net/hamradio/bpqether.c  | 2 +-
 drivers/net/wireless/intel/iwlegacy/common.h | 4 ++--
 17 files changed, 27 insertions(+), 27 deletions(-)

-- 
2.7.4

[PATCH 15/15] iwlegacy: use IS_ENABLED() instead of checking for built-in or module

[Javier Martinez Canillas]

The IS_ENABLED() macro checks if a Kconfig symbol has been enabled either
built-in or as a module, use that macro instead of open coding the same.

Using the macro makes the code more readable by helping abstract away some
of the Kconfig built-in and module enable details.

Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>

---

 drivers/net/wireless/intel/iwlegacy/common.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlegacy/common.h 
b/drivers/net/wireless/intel/iwlegacy/common.h
index 726ede391cb9..3bba521d2cd9 100644
--- a/drivers/net/wireless/intel/iwlegacy/common.h
+++ b/drivers/net/wireless/intel/iwlegacy/common.h
@@ -1320,7 +1320,7 @@ struct il_priv {
u64 timestamp;
 
union {
-#if defined(CONFIG_IWL3945) || defined(CONFIG_IWL3945_MODULE)
+#if IS_ENABLED(CONFIG_IWL3945)
struct {
void *shared_virt;
dma_addr_t shared_phys;
@@ -1351,7 +1351,7 @@ struct il_priv {
 
} _3945;
 #endif
-#if defined(CONFIG_IWL4965) || defined(CONFIG_IWL4965_MODULE)
+#if IS_ENABLED(CONFIG_IWL4965)
struct {
struct il_rx_phy_res last_phy_res;
bool last_phy_res_valid;
-- 
2.7.4

Re: mwifiex: propagate error if IRQ request fails in mwifiex_sdio_of()

[Javier Martinez Canillas]

Hello Amitkumar,

On 09/08/2016 11:55 AM, Amitkumar Karwar wrote:
> Hi Javier,
> 
>> From: Javier Martinez Canillas [mailto:jav...@osg.samsung.com]
>> Sent: Tuesday, September 06, 2016 5:43 PM
>> To: Kalle Valo
>> Cc: linux-ker...@vger.kernel.org; Amitkumar Karwar;
>> net...@vger.kernel.org; linux-wireless@vger.kernel.org; Nishant
>> Sarmukadam; Arend van Spriel
>> Subject: Re: mwifiex: propagate error if IRQ request fails in
>> mwifiex_sdio_of()
>>
>> Hello Kalle,
>>
>> On 09/03/2016 12:35 PM, Kalle Valo wrote:
>>> Javier Martinez Canillas <jav...@osg.samsung.com> wrote:
>>>> If request_irq() fails in mwifiex_sdio_probe_of(), only an error
>>>> message is printed but the actual error is not propagated to the
>> caller function.
>>>>
>>>> Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
>>>
>>> What's the conclusion with this patch? Should I drop it or take it?
>>>
>>> (The discussion is available from the patchwork link in the
>>> signature.)
>>>
>>
>> My understanding is that Arend agrees with the patch and that the
>> question raised was caused by looking at an older kernel version. IOW,
>> the patch is OK and should be picked.
>>
>> I'm adding Arend to cc, so can comment in case I misunderstood him
>> though.
>>
> 
> This error doesn't affect actual wifi functionality. Only thing is wakeup on 
> interrupt when system is in suspended state won't work.
> I think, we can make below change. 
> 
> --
> @@ -122,9 +122,11 @@ static int mwifiex_sdio_probe_of(struct device *dev, 
> struct sdio_mmc_card *card)
>IRQF_TRIGGER_LOW,
>"wifi_wake", cfg);
>if (ret) {
> -dev_err(dev,
> +dev_dbg(dev,
>  "Failed to request irq_wifi %d (%d)\n",
>  cfg->irq_wifi, ret);
> +    card->plt_wake_cfg = NULL;
> +return 0;
>  }

I'm OK with that change. Feel free too add my Reviewed-by if you post it.

> disable_irq(cfg->irq_wifi);
> }
> -
> 
> Regards,
> Amitkumar
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: mwifiex: propagate error if IRQ request fails in mwifiex_sdio_of()

[Javier Martinez Canillas]

Hello Kalle,

On 09/03/2016 12:35 PM, Kalle Valo wrote:
> Javier Martinez Canillas <jav...@osg.samsung.com> wrote:
>> If request_irq() fails in mwifiex_sdio_probe_of(), only an error message
>> is printed but the actual error is not propagated to the caller function.
>>
>> Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
> 
> What's the conclusion with this patch? Should I drop it or take it?
> 
> (The discussion is available from the patchwork link in the signature.)
> 

My understanding is that Arend agrees with the patch and that the question
raised was caused by looking at an older kernel version. IOW, the patch is
OK and should be picked.

I'm adding Arend to cc, so can comment in case I misunderstood him though.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH] mwifiex: propagate error if IRQ request fails in mwifiex_sdio_of()

[Javier Martinez Canillas]

Hello Arend,

On 08/18/2016 03:49 PM, Arend van Spriel wrote:
> 
> 
> On 18-08-16 21:29, Javier Martinez Canillas wrote:
>> Hello Arend,
>>
>> Thanks a lot for your feedback.
>>
>> On 08/18/2016 03:14 PM, Arend van Spriel wrote:
>>> On 18-08-16 16:17, Javier Martinez Canillas wrote:
>>>> If request_irq() fails in mwifiex_sdio_probe_of(), only an error message
>>>> is printed but the actual error is not propagated to the caller function.
>>>
>>> Hmm. The caller function, ie. mwifiex_sdio_probe(), does not seem to care.
>>>
>>
>> Hmm, I'm not so sure about that. It's checking the wifiex_sdio_probe_of()
>> return value.
> 
> Ok. I looked at 4.7 sources on lxr [1].
>

Oh, right. That was fixed quite recently indeed.
 
>> If the IRQ request failing is not an error, then at the very least the call
>> to disable_irq() should be avoided if request_irq() fails, and the message
>> should be changed from dev_err() to dev_dgb() or dev_info().
> 
> agree.
> 
>>> The device may still function without this wake interrupt.
>>>
>>
>> That's correct, the binding says that the "interrupts" property in the child
>> node is optional since is just a wakeup IRQ. Now the question is if should
>> be an error if the IRQ is defined but fails to be requested.
> 
> Clearly it indicates an error in the DT specification so behavior is not
> as expected. Personally I would indeed consider it an error, but I was
> just indicating that it might have done like this intentionally.
>

Yes, might had been done intentionally indeed but I don't think that is
the case since the driver lacked error checking and propagation in many
places. But if someone thinks that's better to not honor the DT and at
least have the driver working without the wake up capability, then I'm
happy to respin the patch and change the print log level to info/debug.
 
> Regards,
> Arend
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH] mwifiex: propagate error if IRQ request fails in mwifiex_sdio_of()

[Javier Martinez Canillas]

Hello Arend,

Thanks a lot for your feedback.

On 08/18/2016 03:14 PM, Arend van Spriel wrote:
> On 18-08-16 16:17, Javier Martinez Canillas wrote:
>> If request_irq() fails in mwifiex_sdio_probe_of(), only an error message
>> is printed but the actual error is not propagated to the caller function.
> 
> Hmm. The caller function, ie. mwifiex_sdio_probe(), does not seem to care.
>

Hmm, I'm not so sure about that. It's checking the wifiex_sdio_probe_of()
return value.

If the IRQ request failing is not an error, then at the very least the call
to disable_irq() should be avoided if request_irq() fails, and the message
should be changed from dev_err() to dev_dgb() or dev_info().
 
> The device may still function without this wake interrupt.
>

That's correct, the binding says that the "interrupts" property in the child
node is optional since is just a wakeup IRQ. Now the question is if should
be an error if the IRQ is defined but fails to be requested.

> Regards,
> Arend
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

[PATCH] mwifiex: propagate error if IRQ request fails in mwifiex_sdio_of()

[Javier Martinez Canillas]

If request_irq() fails in mwifiex_sdio_probe_of(), only an error message
is printed but the actual error is not propagated to the caller function.

Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
---

 drivers/net/wireless/marvell/mwifiex/sdio.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c 
b/drivers/net/wireless/marvell/mwifiex/sdio.c
index d3e1561ca075..00727936ad6e 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -125,6 +125,7 @@ static int mwifiex_sdio_probe_of(struct device *dev, struct 
sdio_mmc_card *card)
dev_err(dev,
"Failed to request irq_wifi %d (%d)\n",
cfg->irq_wifi, ret);
+   return ret;
}
disable_irq(cfg->irq_wifi);
}
-- 
2.5.5

Re: [PATCH] mwifiex: fix unconditional error return in .add_virtual_intf callback

[Javier Martinez Canillas]

Hello Kalle,

On 07/05/2016 09:09 AM, Kalle Valo wrote:
> Javier Martinez Canillas <jav...@osg.samsung.com> writes:
> 
>> The commit 7311ea850079 ("mwifiex: fix AP start problem for newly added
>> interface") attempted to fix an issue when a new AP interface is added.
>>
>> But the patch didn't check the return value of the functions doing the
>> firmware calls and returned an error even if the functions didn't fail.
>>
>> This prevents the network device to be registered properly, so fix it.
>>
>> Fixes: commit 7311ea850079 ("mwifiex: fix AP start problem for newly added 
>> interface")
>> Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
> 
> The fixes line should be:
> 
> Fixes: 7311ea850079 ("mwifiex: fix AP start problem for newly added 
> interface")
> 
> I can fix that before I apply the patch.
> 

Sigh, it was a copy and paste error when I copied the SHA-1 from the
commit message. Sorry about that and thanks for taking care of this.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] mwifiex: fix unconditional error return in .add_virtual_intf callback

[Javier Martinez Canillas]

The commit 7311ea850079 ("mwifiex: fix AP start problem for newly added
interface") attempted to fix an issue when a new AP interface is added.

But the patch didn't check the return value of the functions doing the
firmware calls and returned an error even if the functions didn't fail.

This prevents the network device to be registered properly, so fix it.

Fixes: commit 7311ea850079 ("mwifiex: fix AP start problem for newly added 
interface")
Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
---

 drivers/net/wireless/marvell/mwifiex/cfg80211.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c 
b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index 99e8cf1ad610..5de9f63e2c01 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -2865,9 +2865,11 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct 
wiphy *wiphy,
 
ret = mwifiex_send_cmd(priv, HostCmd_CMD_SET_BSS_MODE,
   HostCmd_ACT_GEN_SET, 0, NULL, true);
+   if (ret)
return ERR_PTR(ret);
 
ret = mwifiex_sta_init_cmd(priv, false, false);
+   if (ret)
return ERR_PTR(ret);
 
mwifiex_setup_ht_caps(>bands[NL80211_BAND_2GHZ]->ht_cap, priv);
-- 
2.5.5

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] mwifiex: move .get_tx_power logic to station ioctl file

[Javier Martinez Canillas]

Hello Kalle,

On 06/22/2016 02:17 AM, Kalle Valo wrote:
> Javier Martinez Canillas <jav...@osg.samsung.com> writes:
> 
>>>> Patch 3/3 applies cleanly even after dropping patch 2/3.
>>>> Is that Ok for you or do you want me to re-resend a v3
>>>> with only patches 1/3 and 3/3?
>>>
>>> I can drop patch 2, no need to resend. Thanks.
>>>
>>
>> I saw that you sent your pull request for v4.8 but the
>> patches from this series were not included:
>>
>> https://lkml.org/lkml/2016/6/21/400
> 
> I had forgotten to change the state of patches 1 and 3 from 'Changes
> Requested' back to 'New' and that's why I missed them. I did that now so
> they are back in my queue:
> 
> https://patchwork.kernel.org/patch/9158855/
> https://patchwork.kernel.org/patch/9158851/
> 
> Thanks for checking.
> 

Ok, thanks for the information.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] mwifiex: move .get_tx_power logic to station ioctl file

[Javier Martinez Canillas]

Hello Kalle,

On 06/10/2016 03:54 PM, Kalle Valo wrote:
> Javier Martinez Canillas <jav...@osg.samsung.com> writes:
> 
>>> This patch (2/3) is only for code rearrangement and adds an
>>> unnecessary wrapper function. We can simply drop the patch.
>>
>> Agreed.
>>
>> Kalle,
>>
>> Patch 3/3 applies cleanly even after dropping patch 2/3.
>> Is that Ok for you or do you want me to re-resend a v3
>> with only patches 1/3 and 3/3?
> 
> I can drop patch 2, no need to resend. Thanks.
> 

I saw that you sent your pull request for v4.8 but the
patches from this series were not included:

https://lkml.org/lkml/2016/6/21/400

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] mwifiex: move .get_tx_power logic to station ioctl file

[Javier Martinez Canillas]

Hello Amitkumar,

On 06/10/2016 12:26 PM, Amitkumar Karwar wrote:
> Hi Kalle/Javier,
> 
>> From: Javier Martinez Canillas [mailto:jav...@osg.samsung.com]
>> Sent: Friday, June 10, 2016 8:07 PM
>> To: Kalle Valo
>> Cc: linux-ker...@vger.kernel.org; Julian Calaby; Shengzhen Li; Enric
>> Balletbo i Serra; Amitkumar Karwar; net...@vger.kernel.org; linux-
>> wirel...@vger.kernel.org; Nishant Sarmukadam
>> Subject: Re: [PATCH v2 2/3] mwifiex: move .get_tx_power logic to station
>> ioctl file
>>
>> Hello Kalle,
>>
>> On 06/10/2016 10:30 AM, Kalle Valo wrote:
>>> Javier Martinez Canillas <jav...@osg.samsung.com> writes:
>>>
>>>> From: Shengzhen Li <s...@marvell.com>
>>>>
>>>> Most cfg80211 operations are just a wrappers to functions defined in
>>>> the sta_ioctl.c file, so for consistency move the .get_tx_power logic
>> there.
>>>>
>>>> Signed-off-by: Shengzhen Li <s...@marvell.com>
>>>> Signed-off-by: Amitkumar Karwar <akar...@marvell.com>
>>>> [javier: update the subject line and commit message]
>>>> Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
>>>
>>> [...]
>>>
>>>> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
>>>> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
>>>> @@ -385,18 +385,10 @@ mwifiex_cfg80211_get_tx_power(struct wiphy
>> *wiphy,
>>>>  int *dbm)
>>>>  {
>>>>struct mwifiex_adapter *adapter =
>> mwifiex_cfg80211_get_adapter(wiphy);
>>>> -  struct mwifiex_private *priv = mwifiex_get_priv(adapter,
>>>> -  MWIFIEX_BSS_ROLE_ANY);
>>>> -  int ret = mwifiex_send_cmd(priv, HostCmd_CMD_RF_TX_PWR,
>>>> - HostCmd_ACT_GEN_GET, 0, NULL, true);
>>>> -
>>>> -  if (ret < 0)
>>>> -  return ret;
>>>> -
>>>> -  /* tx_power_level is set in HostCmd_CMD_RF_TX_PWR command handler
>> */
>>>> -  *dbm = priv->tx_power_level;
>>>> +  struct mwifiex_private *priv;
>>>>
>>>> -  return 0;
>>>> +  priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);
>>>> +  return mwifiex_get_tx_power(priv, dbm);
>>>>  }
>>>
>>> So in patch 1 you added the patch and in patch 2 you move it to a
>>> different location? That doesn't make any sense, can't you just fold
>>> the two patches into one so that the function is added only once.
>>>
>>
>> I posted this patch in v1 but then Amitkumar shared his patch with me
>> that was very similar to mine, only that the logic was in a different
>> location.
>>
>> So I included his delta as a separate patch to try keeping attribution
>> as best as possible.
>>
> 
> This patch (2/3) is only for code rearrangement and adds an unnecessary 
> wrapper function. We can simply drop the patch.
>

Agreed.

Kalle,

Patch 3/3 applies cleanly even after dropping patch 2/3.
Is that Ok for you or do you want me to re-resend a v3
with only patches 1/3 and 3/3?

> Regards,
> Amitkumar
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] mwifiex: move .get_tx_power logic to station ioctl file

[Javier Martinez Canillas]

Hello Kalle,

On 06/10/2016 10:30 AM, Kalle Valo wrote:
> Javier Martinez Canillas <jav...@osg.samsung.com> writes:
> 
>> From: Shengzhen Li <s...@marvell.com>
>>
>> Most cfg80211 operations are just a wrappers to functions defined in the
>> sta_ioctl.c file, so for consistency move the .get_tx_power logic there.
>>
>> Signed-off-by: Shengzhen Li <s...@marvell.com>
>> Signed-off-by: Amitkumar Karwar <akar...@marvell.com>
>> [javier: update the subject line and commit message]
>> Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
> 
> [...]
> 
>> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
>> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
>> @@ -385,18 +385,10 @@ mwifiex_cfg80211_get_tx_power(struct wiphy *wiphy,
>>int *dbm)
>>  {
>>  struct mwifiex_adapter *adapter = mwifiex_cfg80211_get_adapter(wiphy);
>> -struct mwifiex_private *priv = mwifiex_get_priv(adapter,
>> -MWIFIEX_BSS_ROLE_ANY);
>> -int ret = mwifiex_send_cmd(priv, HostCmd_CMD_RF_TX_PWR,
>> -   HostCmd_ACT_GEN_GET, 0, NULL, true);
>> -
>> -if (ret < 0)
>> -return ret;
>> -
>> -/* tx_power_level is set in HostCmd_CMD_RF_TX_PWR command handler */
>> -*dbm = priv->tx_power_level;
>> +struct mwifiex_private *priv;
>>  
>> -return 0;
>> +priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);
>> +return mwifiex_get_tx_power(priv, dbm);
>>  }
> 
> So in patch 1 you added the patch and in patch 2 you move it to a
> different location? That doesn't make any sense, can't you just fold the
> two patches into one so that the function is added only once.
>

I posted this patch in v1 but then Amitkumar shared his patch with me that
was very similar to mine, only that the logic was in a different location.

So I included his delta as a separate patch to try keeping attribution as
best as possible.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 0/3] mwifiex: add .get_tx_power and .get_antenna cfg80211 operations

[Javier Martinez Canillas]

Hello,

Since commit de3bb771f471 ("cfg80211: add more warnings for inconsistent
ops") the wireless core warns if a driver implements a cfg80211 callback
but doesn't implements the inverse operation. The mwifiex driver has two
set operations defined without the get counterpat so warnings are shown:

WARNING: CPU: 3 PID: 127 at net/wireless/core.c:366 wiphy_new_nm+0x66c/0x6ac
Modules linked in: mwifiex_sdio mwifiex
CPU: 3 PID: 127 Comm: kworker/3:1 Tainted: GW   
4.7.0-rc1-next-20160531-6-g569df5b983f3
Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
Workqueue: events request_firmware_work_func
[] (unwind_backtrace) from [] (show_stack+0x10/0x14)
[] (show_stack) from [] (dump_stack+0x88/0x9c)
[] (dump_stack) from [] (__warn+0xe8/0x100)
[] (__warn) from [] (warn_slowpath_null+0x20/0x28)
[] (warn_slowpath_null) from [] 
(wiphy_new_nm+0x66c/0x6ac)
[] (wiphy_new_nm) from [] 
(mwifiex_register_cfg80211+0x28/0x3f0 [mwifiex])
[] (mwifiex_register_cfg80211 [mwifiex]) from [] 
(mwifiex_fw_dpc+0x2b0/0x474 [mwifiex])
[] (mwifiex_fw_dpc [mwifiex]) from [] 
(request_firmware_work_func+0x30/0x58)
[] (request_firmware_work_func) from [] 
(process_one_work+0x124/0x338)
[] (process_one_work) from [] (worker_thread+0x38/0x4d4)
[] (worker_thread) from [] (kthread+0xdc/0xf4)
[] (kthread) from [] (ret_from_fork+0x14/0x3c)

This patch series fixes the warnings by implementing callbacks for the
missing get operations.

Since the first version was posted [0], Amitkumar Karwar from Marvell
provided me their patches that adds the same. The .get_tx_power patch was
very similar to mine. So I kept my patch and added the delta as a follow
up patch but discarded my .get_antenna patch since the one provided by
Amitkumar queries the firmware so is the correct approach.

Patches were tested on an Exynos5800 Chromebook that has a mwifiex chip.

Changes since v1:
- Drop patch that reports antenna cached values and use Marvell's patch
  instead.
- Add patch that moves the .get_tx_power() logic to sta_ioctl.c file.

[0]: https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1160119.html

Best regards,
Javier


Javier Martinez Canillas (1):
  mwifiex: add a cfg80211 .get_tx_power operation callback

Shengzhen Li (2):
  mwifiex: move .get_tx_power logic to station ioctl file
  mwifiex: add get_antenna support for cfg80211

 drivers/net/wireless/marvell/mwifiex/cfg80211.c| 32 ++
 drivers/net/wireless/marvell/mwifiex/fw.h  |  3 ++
 drivers/net/wireless/marvell/mwifiex/init.c|  2 +
 drivers/net/wireless/marvell/mwifiex/main.h|  4 ++
 drivers/net/wireless/marvell/mwifiex/sta_cmd.c | 50 +++---
 drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c | 10 +++--
 drivers/net/wireless/marvell/mwifiex/sta_ioctl.c   | 18 
 7 files changed, 100 insertions(+), 19 deletions(-)

-- 
2.5.5

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 3/3] mwifiex: add get_antenna support for cfg80211

[Javier Martinez Canillas]

From: Shengzhen Li <s...@marvell.com>

Since commit de3bb771f471 ("cfg80211: add more warnings for inconsistent
ops") the wireless core warns if a driver implements a cfg80211 callback
but doesn't implements the inverse operation.

The mwifiex driver defines a .set_antenna handler but not a .get_antenna
so this not only makes the core to print a warning when creating a new
wiphy but also the antenna isn't reported to user-space apps such as iw.

This patch queries the antenna to the firmware so is properly reported to
user-space. With this patch, the wireless core does not warn anymore and:

$ iw phy phy0 info | grep Antennas
Available Antennas: TX 0x3 RX 0x3
Configured Antennas: TX 0x3 RX 0x3

Signed-off-by: Shengzhen Li <s...@marvell.com>
Signed-off-by: Amitkumar Karwar <akar...@marvell.com>
[javier: expand the commit message]
Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>

---

 drivers/net/wireless/marvell/mwifiex/cfg80211.c| 16 +++
 drivers/net/wireless/marvell/mwifiex/fw.h  |  3 ++
 drivers/net/wireless/marvell/mwifiex/init.c|  2 +
 drivers/net/wireless/marvell/mwifiex/main.h|  2 +
 drivers/net/wireless/marvell/mwifiex/sta_cmd.c | 50 +++---
 drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c | 10 +++--
 6 files changed, 64 insertions(+), 19 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c 
b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index ff3f63ed95e1..ff1eefe5087b 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -1819,6 +1819,21 @@ mwifiex_cfg80211_set_antenna(struct wiphy *wiphy, u32 
tx_ant, u32 rx_ant)
HostCmd_ACT_GEN_SET, 0, _cfg, true);
 }
 
+static int
+mwifiex_cfg80211_get_antenna(struct wiphy *wiphy, u32 *tx_ant, u32 *rx_ant)
+{
+   struct mwifiex_adapter *adapter = mwifiex_cfg80211_get_adapter(wiphy);
+   struct mwifiex_private *priv = mwifiex_get_priv(adapter,
+   MWIFIEX_BSS_ROLE_ANY);
+   mwifiex_send_cmd(priv, HostCmd_CMD_RF_ANTENNA,
+HostCmd_ACT_GEN_GET, 0, NULL, true);
+
+   *tx_ant = priv->tx_ant;
+   *rx_ant = priv->rx_ant;
+
+   return 0;
+}
+
 /* cfg80211 operation handler for stop ap.
  * Function stops BSS running at uAP interface.
  */
@@ -3962,6 +3977,7 @@ static struct cfg80211_ops mwifiex_cfg80211_ops = {
.change_beacon = mwifiex_cfg80211_change_beacon,
.set_cqm_rssi_config = mwifiex_cfg80211_set_cqm_rssi_config,
.set_antenna = mwifiex_cfg80211_set_antenna,
+   .get_antenna = mwifiex_cfg80211_get_antenna,
.del_station = mwifiex_cfg80211_del_station,
.sched_scan_start = mwifiex_cfg80211_sched_scan_start,
.sched_scan_stop = mwifiex_cfg80211_sched_scan_stop,
diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h 
b/drivers/net/wireless/marvell/mwifiex/fw.h
index 8e4145abdbfa..cef72343f5b6 100644
--- a/drivers/net/wireless/marvell/mwifiex/fw.h
+++ b/drivers/net/wireless/marvell/mwifiex/fw.h
@@ -462,6 +462,9 @@ enum P2P_MODES {
 #define HostCmd_ACT_SET_RX  0x0001
 #define HostCmd_ACT_SET_TX  0x0002
 #define HostCmd_ACT_SET_BOTH0x0003
+#define HostCmd_ACT_GET_RX  0x0004
+#define HostCmd_ACT_GET_TX  0x0008
+#define HostCmd_ACT_GET_BOTH0x000c
 
 #define RF_ANTENNA_AUTO 0x
 
diff --git a/drivers/net/wireless/marvell/mwifiex/init.c 
b/drivers/net/wireless/marvell/mwifiex/init.c
index 78c532f0d286..fbaf49056746 100644
--- a/drivers/net/wireless/marvell/mwifiex/init.c
+++ b/drivers/net/wireless/marvell/mwifiex/init.c
@@ -110,6 +110,8 @@ int mwifiex_init_priv(struct mwifiex_private *priv)
priv->tx_power_level = 0;
priv->max_tx_power_level = 0;
priv->min_tx_power_level = 0;
+   priv->tx_ant = 0;
+   priv->rx_ant = 0;
priv->tx_rate = 0;
priv->rxpd_htinfo = 0;
priv->rxpd_rate = 0;
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h 
b/drivers/net/wireless/marvell/mwifiex/main.h
index 79c28cfb7780..2ae7ff74e1c6 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -533,6 +533,8 @@ struct mwifiex_private {
u16 tx_power_level;
u8 max_tx_power_level;
u8 min_tx_power_level;
+   u32 tx_ant;
+   u32 rx_ant;
u8 tx_rate;
u8 tx_htinfo;
u8 rxpd_htinfo;
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c 
b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
index e436574b1698..8c658495bf66 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
@@ -313,23 +313,41 @@ static int mwifiex_cmd_rf_antenna(struct mwifiex_private 
*priv,
 
cmd->comm

[PATCH 2/2] mwifiex: add a cfg80211 .get_antenna operation callback

[Javier Martinez Canillas]

Since commit de3bb771f471 ("cfg80211: add more warnings for inconsistent
ops") the wireless core warns if a driver implements a cfg80211 callback
but doesn't implements the inverse operation.

The mwifiex driver defines a .set_antenna handler but not a .get_antenna
so this not only makes the core to print a warning when creating a new
wiphy but also the antenna isn't reported to user-space apps such as iw.

Unfortunately the driver doesn't have support to query the antena to the
firmware and there isn't public documentation about the interface so this
patch caches the values set in .set_antenna and reports those back in get.

With this patch, the wireless core does not warn anymore and the antenna
is properly reported once has been set:

$ iw phy phy0 set antenna 0x1

$ iw phy phy0 info | grep Antennas
Available Antennas: TX 0x3 RX 0x3
Configured Antennas: TX 0x1 RX 0x1

Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>

---
Hello,

Even though this approach prevents the warning and allows to reports the
antenna values, it would be better if instead of caching the set values,
these are asked to the firmware for each .get_antenna callback.

But the driver currently only implements a set antenna command and the
firmware interface documentation is not public so isn't clear if the
firmware doesn't support or is just that the driver didn't implement it.

Best regards,
Javier

 drivers/net/wireless/marvell/mwifiex/cfg80211.c | 26 +++--
 drivers/net/wireless/marvell/mwifiex/main.h |  1 +
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c 
b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index b17f3d09a2c7..e9efbc852d23 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -1771,6 +1771,7 @@ mwifiex_cfg80211_set_antenna(struct wiphy *wiphy, u32 
tx_ant, u32 rx_ant)
struct mwifiex_private *priv = mwifiex_get_priv(adapter,
MWIFIEX_BSS_ROLE_ANY);
struct mwifiex_ds_ant_cfg ant_cfg;
+   int ret;
 
if (!tx_ant || !rx_ant)
return -EOPNOTSUPP;
@@ -1823,8 +1824,28 @@ mwifiex_cfg80211_set_antenna(struct wiphy *wiphy, u32 
tx_ant, u32 rx_ant)
ant_cfg.tx_ant = tx_ant;
ant_cfg.rx_ant = rx_ant;
 
-   return mwifiex_send_cmd(priv, HostCmd_CMD_RF_ANTENNA,
-   HostCmd_ACT_GEN_SET, 0, _cfg, true);
+   ret = mwifiex_send_cmd(priv, HostCmd_CMD_RF_ANTENNA,
+  HostCmd_ACT_GEN_SET, 0, _cfg, true);
+
+   if (ret < 0)
+   return ret;
+
+   priv->ant_cfg.tx_ant = tx_ant;
+   priv->ant_cfg.rx_ant = rx_ant;
+
+   return 0;
+}
+
+static int
+mwifiex_cfg80211_get_antenna(struct wiphy *wiphy, u32 *tx_ant, u32 *rx_ant)
+{
+   struct mwifiex_adapter *adapter = mwifiex_cfg80211_get_adapter(wiphy);
+   struct mwifiex_private *priv = mwifiex_get_priv(adapter,
+   MWIFIEX_BSS_ROLE_ANY);
+   *tx_ant = priv->ant_cfg.tx_ant;
+   *rx_ant = priv->ant_cfg.rx_ant;
+
+   return 0;
 }
 
 /* cfg80211 operation handler for stop ap.
@@ -3970,6 +3991,7 @@ static struct cfg80211_ops mwifiex_cfg80211_ops = {
.change_beacon = mwifiex_cfg80211_change_beacon,
.set_cqm_rssi_config = mwifiex_cfg80211_set_cqm_rssi_config,
.set_antenna = mwifiex_cfg80211_set_antenna,
+   .get_antenna = mwifiex_cfg80211_get_antenna,
.del_station = mwifiex_cfg80211_del_station,
.sched_scan_start = mwifiex_cfg80211_sched_scan_start,
.sched_scan_stop = mwifiex_cfg80211_sched_scan_stop,
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h 
b/drivers/net/wireless/marvell/mwifiex/main.h
index 0207af00be42..b4fe10cbb34d 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -668,6 +668,7 @@ struct mwifiex_private {
struct delayed_work dfs_chan_sw_work;
struct cfg80211_beacon_data beacon_after;
struct mwifiex_11h_intf_state state_11h;
+   struct mwifiex_ds_ant_cfg ant_cfg;
struct mwifiex_ds_mem_rw mem_rw;
struct sk_buff_head bypass_txq;
struct mwifiex_user_scan_chan hidden_chan[MWIFIEX_USER_SCAN_CHAN_MAX];
-- 
2.5.5

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/2] mwifiex: add .get_tx_power and .get_antenna cfg80211 operations

[Javier Martinez Canillas]

Hello,

Since commit de3bb771f471 ("cfg80211: add more warnings for inconsistent
ops") the wireless core warns if a driver implements a cfg80211 callback
but doesn't implements the inverse operation. The mwifiex driver has two
set operations defined without the get counterpat so warnings are shown:

WARNING: CPU: 3 PID: 127 at net/wireless/core.c:366 wiphy_new_nm+0x66c/0x6ac
Modules linked in: mwifiex_sdio mwifiex
CPU: 3 PID: 127 Comm: kworker/3:1 Tainted: GW   
4.7.0-rc1-next-20160531-6-g569df5b983f3
Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
Workqueue: events request_firmware_work_func
[] (unwind_backtrace) from [] (show_stack+0x10/0x14)
[] (show_stack) from [] (dump_stack+0x88/0x9c)
[] (dump_stack) from [] (__warn+0xe8/0x100)
[] (__warn) from [] (warn_slowpath_null+0x20/0x28)
[] (warn_slowpath_null) from [] (wiphy_new_nm+0x66c/0x6ac)
[] (wiphy_new_nm) from [] 
(mwifiex_register_cfg80211+0x28/0x3f0 [mwifiex])
[] (mwifiex_register_cfg80211 [mwifiex]) from [] 
(mwifiex_fw_dpc+0x2b0/0x474 [mwifiex])
[] (mwifiex_fw_dpc [mwifiex]) from [] 
(request_firmware_work_func+0x30/0x58)
[] (request_firmware_work_func) from [] 
(process_one_work+0x124/0x338)
[] (process_one_work) from [] (worker_thread+0x38/0x4d4)
[] (worker_thread) from [] (kthread+0xdc/0xf4)
[] (kthread) from [] (ret_from_fork+0x14/0x3c)

This patch series fixes the warnings by implementing callbacks for the
get operations.

There isn't public documentation about the firmware interface so the
data is only queried to the firmware if the driver already supports
it as is the case of the Tx power. For the antenna the driver only
supports the get command so the get returns the cached values that
were previously set. This can be changed to query the firmware as a
followup by someone with access to the docs, if this is supported.

Best regards,
Javier


Javier Martinez Canillas (2):
  mwifiex: add a cfg80211 .get_tx_power operation callback
  mwifiex: add a cfg80211 .get_antenna operation callback

 drivers/net/wireless/marvell/mwifiex/cfg80211.c | 50 -
 drivers/net/wireless/marvell/mwifiex/main.h |  1 +
 2 files changed, 49 insertions(+), 2 deletions(-)

-- 
2.5.5

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] mwifiex: add a cfg80211 .get_tx_power operation callback

[Javier Martinez Canillas]

The mwifiex driver implements a cfg80211 .set_tx_power operation handler
but doesn't have the inverse .get_tx_power callback.

This not only has the effect that the Tx power can't be reported to user
space tools such as iwconfig and iwlist but also that the wireless core
prints a warning when a new wiphy is created due an cfg80211 operation
being implemented without its counterpart.

After this patch, the Tx power is properly reported to user-space tools:

$ iwlist mlan0 txpower
mlan0 unknown transmit-power information.

  Current Tx-Power=13 dBm   (19 mW)

and also the following warning isn't shown anymore on the driver probe:

WARNING: CPU: 3 PID: 127 at net/wireless/core.c:366 wiphy_new_nm+0x66c/0x6ac
Modules linked in: mwifiex_sdio mwifiex
CPU: 3 PID: 127 Comm: kworker/3:1 Tainted: GW   
4.7.0-rc1-next-20160531-6-g569df5b983f3
Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
Workqueue: events request_firmware_work_func
[] (unwind_backtrace) from [] (show_stack+0x10/0x14)
[] (show_stack) from [] (dump_stack+0x88/0x9c)
[] (dump_stack) from [] (__warn+0xe8/0x100)
[] (__warn) from [] (warn_slowpath_null+0x20/0x28)
[] (warn_slowpath_null) from [] (wiphy_new_nm+0x66c/0x6ac)
[] (wiphy_new_nm) from [] 
(mwifiex_register_cfg80211+0x28/0x3f0 [mwifiex])
[] (mwifiex_register_cfg80211 [mwifiex]) from [] 
(mwifiex_fw_dpc+0x2b0/0x474 [mwifiex])
[] (mwifiex_fw_dpc [mwifiex]) from [] 
(request_firmware_work_func+0x30/0x58)
[] (request_firmware_work_func) from [] 
(process_one_work+0x124/0x338)
[] (process_one_work) from [] (worker_thread+0x38/0x4d4)
[] (worker_thread) from [] (kthread+0xdc/0xf4)
[] (kthread) from [] (ret_from_fork+0x14/0x3c)

Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
---

 drivers/net/wireless/marvell/mwifiex/cfg80211.c | 24 
 1 file changed, 24 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c 
b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index ff948a92..b17f3d09a2c7 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -377,6 +377,29 @@ mwifiex_cfg80211_set_tx_power(struct wiphy *wiphy,
 }
 
 /*
+ * CFG802.11 operation handler to get Tx power.
+ */
+static int
+mwifiex_cfg80211_get_tx_power(struct wiphy *wiphy,
+ struct wireless_dev *wdev,
+ int *dbm)
+{
+   struct mwifiex_adapter *adapter = mwifiex_cfg80211_get_adapter(wiphy);
+   struct mwifiex_private *priv = mwifiex_get_priv(adapter,
+   MWIFIEX_BSS_ROLE_ANY);
+   int ret = mwifiex_send_cmd(priv, HostCmd_CMD_RF_TX_PWR,
+  HostCmd_ACT_GEN_GET, 0, NULL, true);
+
+   if (ret < 0)
+   return ret;
+
+   /* tx_power_level is set in HostCmd_CMD_RF_TX_PWR command handler */
+   *dbm = priv->tx_power_level;
+
+   return 0;
+}
+
+/*
  * CFG802.11 operation handler to set Power Save option.
  *
  * The timeout value, if provided, is currently ignored.
@@ -3940,6 +3963,7 @@ static struct cfg80211_ops mwifiex_cfg80211_ops = {
.set_default_key = mwifiex_cfg80211_set_default_key,
.set_power_mgmt = mwifiex_cfg80211_set_power_mgmt,
.set_tx_power = mwifiex_cfg80211_set_tx_power,
+   .get_tx_power = mwifiex_cfg80211_get_tx_power,
.set_bitrate_mask = mwifiex_cfg80211_set_bitrate_mask,
.start_ap = mwifiex_cfg80211_start_ap,
.stop_ap = mwifiex_cfg80211_stop_ap,
-- 
2.5.5

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/8] mwifiex: don't print an error if an optional DT property is missing

[Javier Martinez Canillas]

Hello Julian,

Thanks a lot for your feedback and reviews.

On 06/01/2016 12:20 AM, Julian Calaby wrote:
> Hi All,
> 
> On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas
> <jav...@osg.samsung.com> wrote:
>> The Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt DT
>> binding document say that the "interrupts" property in the child node is
>> optional. So the property being missed shouldn't be treated as an error.
> 
> Have you checked whether it is truly optional? I.e. nothing else
> breaks if this property isn't set?
>

That's what the DT binding says and the IRQ is only used as a wakeup source
during system suspend, it is not used during runtime. And that is why the
mwifiex_sdio_probe_of() function does not fail if the IRQ is missing.

Now, I just got to that conclusion by reading the binding docs, the message
in the commits that introduced this and the driver code. Xinming Hu should
comment on how critical this feature is for systems that needs to be wakeup.

In any case I think that the code should be consistent with what the binding
doc says and also the function does (i.e: dev_err only if returns an error).
 
>> Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
> 
> Other than that, this looks sensible to me.
> 
> Reviewed-by: Julian Calaby <julian.cal...@gmail.com>
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/8] mwifiex: propagate sdio_enable_func() errno code in mwifiex_sdio_probe()

[Javier Martinez Canillas]

If the sdio_enable_func() function fails on .probe, the -EIO errno code
is always returned but that could make more difficult to debug and find
the cause of why the function actually failed.

Since the driver/device core prints the value returned by .probe in its
error message propagate what was returned by sdio_enable_func() at fail.

Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
---

 drivers/net/wireless/marvell/mwifiex/sdio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c 
b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 285b1b68f7e9..ab64507c84e1 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -184,7 +184,7 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct 
sdio_device_id *id)
if (ret) {
pr_err("%s: failed to enable function\n", __func__);
kfree(card);
-   return -EIO;
+   return ret;
}
 
/* device tree node parsing and platform specific configuration*/
-- 
2.5.5

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/8] mwifiex: propagate mwifiex_add_card() errno code in mwifiex_sdio_probe()

[Javier Martinez Canillas]

There's only a check if mwifiex_add_card() returned a nonzero value, but
the actual error code is neither stored nor propagated to the caller. So
instead of always returning -1 (which is -EPERM and not a suitable errno
code in this case), propagate the value returned by mwifiex_add_card().

Patch also removes the assignment of sdio_disable_func() returned value
since it was overwritten anyways and what matters is to know the error
value returned by the first function that failed.

Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
---

 drivers/net/wireless/marvell/mwifiex/sdio.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c 
b/drivers/net/wireless/marvell/mwifiex/sdio.c
index ab64507c84e1..81003fbe5025 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -191,14 +191,14 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct 
sdio_device_id *id)
if (func->dev.of_node)
mwifiex_sdio_probe_of(>dev, card);
 
-   if (mwifiex_add_card(card, _remove_card_sem, _ops,
-MWIFIEX_SDIO)) {
+   ret = mwifiex_add_card(card, _remove_card_sem, _ops,
+  MWIFIEX_SDIO);
+   if (ret) {
pr_err("%s: add card failed\n", __func__);
kfree(card);
sdio_claim_host(func);
-   ret = sdio_disable_func(func);
+   sdio_disable_func(func);
sdio_release_host(func);
-   ret = -1;
}
 
return ret;
-- 
2.5.5

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/8] mwifiex: Fix some error handling issues in mwifiex_sdio_probe() function

[Javier Martinez Canillas]

Hello,

While booting a system with a mwifiex WiFi card, I noticed the following
missleading error message:

[  12.480042] mwifiex_sdio mmc2:0001:1: sdio platform data not available

This error only applies to platforms that define a child node for the SDIO
device, but it's currently shown even in platforms that don't have a child
node defined.

So this series fixes this issue and others I found in the .probe function
(mostly related to error handling and the error path) while looking at it.

Best regards,
Javier


Javier Martinez Canillas (8):
  mwifiex: only call mwifiex_sdio_probe_of() if dev has an OF node
  mwifiex: propagate sdio_enable_func() errno code in
mwifiex_sdio_probe()
  mwifiex: propagate mwifiex_add_card() errno code in
mwifiex_sdio_probe()
  mwifiex: consolidate mwifiex_sdio_probe() error paths
  mwifiex: use dev_err() instead of pr_err() in mwifiex_sdio_probe()
  mwifiex: check if mwifiex_sdio_probe_of() fails and return error
  mwifiex: don't print an error if an optional DT property is missing
  mwifiex: use better message and error code when OF node doesn't match

 drivers/net/wireless/marvell/mwifiex/sdio.c | 46 ++---
 1 file changed, 28 insertions(+), 18 deletions(-)

-- 
2.5.5

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/8] mwifiex: only call mwifiex_sdio_probe_of() if dev has an OF node

[Javier Martinez Canillas]

SDIO is an auto enumerable bus so the SDIO devices are matched using the
sdio_device_id table and not using compatible strings from a OF id table.

However, commit ce4f6f0c353b ("mwifiex: add platform specific wakeup
interrupt support") allowed to match nodes defined as child of the SDIO
host controller in the probe function using a compatible string to setup
platform specific parameters in the DT.

The problem is that the OF parse function is always called regardless if
the SDIO dev has an OF node associated or not, and prints an error if it
is not found. So, on a platform that doesn't have a node for a SDIO dev,
the following misleading error message will be printed:

[  12.480042] mwifiex_sdio mmc2:0001:1: sdio platform data not available

Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
---

 drivers/net/wireless/marvell/mwifiex/sdio.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c 
b/drivers/net/wireless/marvell/mwifiex/sdio.c
index bdc51ffd43ec..285b1b68f7e9 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -102,8 +102,7 @@ static int mwifiex_sdio_probe_of(struct device *dev, struct 
sdio_mmc_card *card)
struct mwifiex_plt_wake_cfg *cfg;
int ret;
 
-   if (!dev->of_node ||
-   !of_match_node(mwifiex_sdio_of_match_table, dev->of_node)) {
+   if (!of_match_node(mwifiex_sdio_of_match_table, dev->of_node)) {
dev_err(dev, "sdio platform data not available\n");
return -1;
}
@@ -189,7 +188,8 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct 
sdio_device_id *id)
}
 
/* device tree node parsing and platform specific configuration*/
-   mwifiex_sdio_probe_of(>dev, card);
+   if (func->dev.of_node)
+   mwifiex_sdio_probe_of(>dev, card);
 
if (mwifiex_add_card(card, _remove_card_sem, _ops,
 MWIFIEX_SDIO)) {
-- 
2.5.5

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/8] mwifiex: consolidate mwifiex_sdio_probe() error paths

[Javier Martinez Canillas]

Instead of duplicating part of the cleanups needed in case of an error
in .probe callback, have a single error path and use goto labels as is
common practice in the kernel.

This also has the nice side effect that the cleanup operations are made
in the inverse order of their counterparts, which was not the case for
the mwifiex_add_card() error path.

Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
---

 drivers/net/wireless/marvell/mwifiex/sdio.c | 17 +++--
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c 
b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 81003fbe5025..7aeee88b858f 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -183,8 +183,7 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct 
sdio_device_id *id)
 
if (ret) {
pr_err("%s: failed to enable function\n", __func__);
-   kfree(card);
-   return ret;
+   goto err_free;
}
 
/* device tree node parsing and platform specific configuration*/
@@ -195,12 +194,18 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct 
sdio_device_id *id)
   MWIFIEX_SDIO);
if (ret) {
pr_err("%s: add card failed\n", __func__);
-   kfree(card);
-   sdio_claim_host(func);
-   sdio_disable_func(func);
-   sdio_release_host(func);
+   goto err_disable;
}
 
+   return 0;
+
+err_disable:
+   sdio_claim_host(func);
+   sdio_disable_func(func);
+   sdio_release_host(func);
+err_free:
+   kfree(card);
+
return ret;
 }
 
-- 
2.5.5

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/8] mwifiex: use dev_err() instead of pr_err() in mwifiex_sdio_probe()

[Javier Martinez Canillas]

It's better to have the device name prefixed in the error message.

Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
---

 drivers/net/wireless/marvell/mwifiex/sdio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c 
b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 7aeee88b858f..1ffbb972318f 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -182,7 +182,7 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct 
sdio_device_id *id)
sdio_release_host(func);
 
if (ret) {
-   pr_err("%s: failed to enable function\n", __func__);
+   dev_err(>dev, "failed to enable function\n");
goto err_free;
}
 
@@ -193,7 +193,7 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct 
sdio_device_id *id)
ret = mwifiex_add_card(card, _remove_card_sem, _ops,
   MWIFIEX_SDIO);
if (ret) {
-   pr_err("%s: add card failed\n", __func__);
+   dev_err(>dev, "add card failed\n");
goto err_disable;
}
 
-- 
2.5.5

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 8/8] mwifiex: use better message and error code when OF node doesn't match

[Javier Martinez Canillas]

The Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt DT
binding document lists the possible compatible strings that a SDIO child
node can have, so the driver checks if the defined in the node matches.

But the error message when that's not the case is misleading, so change
for one that makes clear what the error really is. Also, returning a -1
as errno code is not correct since that's -EPERM. A -EINVAL seems to be
a more appropriate one.

Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>

---

 drivers/net/wireless/marvell/mwifiex/sdio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c 
b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 8b3292eaecb2..e6d56be04e08 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -103,8 +103,8 @@ static int mwifiex_sdio_probe_of(struct device *dev, struct 
sdio_mmc_card *card)
int ret;
 
if (!of_match_node(mwifiex_sdio_of_match_table, dev->of_node)) {
-   dev_err(dev, "sdio platform data not available\n");
-   return -1;
+   dev_err(dev, "required compatible string missing\n");
+   return -EINVAL;
}
 
card->plt_of_node = dev->of_node;
-- 
2.5.5

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 6/8] mwifiex: check if mwifiex_sdio_probe_of() fails and return error

[Javier Martinez Canillas]

The function can fail so the returned value should be checked
and the error propagated to the caller in case of a failure.

Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
---

 drivers/net/wireless/marvell/mwifiex/sdio.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c 
b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 1ffbb972318f..1c17e624547a 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -187,8 +187,13 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct 
sdio_device_id *id)
}
 
/* device tree node parsing and platform specific configuration*/
-   if (func->dev.of_node)
-   mwifiex_sdio_probe_of(>dev, card);
+   if (func->dev.of_node) {
+   ret = mwifiex_sdio_probe_of(>dev, card);
+   if (ret) {
+   dev_err(>dev, "SDIO dt node parse failed\n");
+   goto err_disable;
+   }
+   }
 
ret = mwifiex_add_card(card, _remove_card_sem, _ops,
   MWIFIEX_SDIO);
-- 
2.5.5

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 7/8] mwifiex: don't print an error if an optional DT property is missing

[Javier Martinez Canillas]

The Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt DT
binding document say that the "interrupts" property in the child node is
optional. So the property being missed shouldn't be treated as an error.

Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
---

 drivers/net/wireless/marvell/mwifiex/sdio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c 
b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 1c17e624547a..8b3292eaecb2 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -114,7 +114,7 @@ static int mwifiex_sdio_probe_of(struct device *dev, struct 
sdio_mmc_card *card)
if (cfg && card->plt_of_node) {
cfg->irq_wifi = irq_of_parse_and_map(card->plt_of_node, 0);
if (!cfg->irq_wifi) {
-   dev_err(dev,
+   dev_dbg(dev,
"fail to parse irq_wifi from device tree\n");
} else {
ret = devm_request_irq(dev, cfg->irq_wifi,
-- 
2.5.5

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] bcma: mips: Allow build if COMPILE_TEST is enabled

[Javier Martinez Canillas]

Hello Kalle,

On 01/28/2016 10:05 AM, Kalle Valo wrote:

Kalle Valo <kv...@codeaurora.org> writes:


Javier Martinez Canillas <jav...@osg.samsung.com> writes:


On 10/14/2015 12:52 PM, Kalle Valo wrote:

Javier Martinez Canillas <jav...@osg.samsung.com> writes:


Thanks for reporting this issue. I only did a partial build with
make M=drivers/bcma but didn't think about implicit dependencies
on other drivers.

I've posted this patch that should avoid this issue:
https://patchwork.kernel.org/patch/7391551/


So this patch ("bcma: mips: Allow build if COMPILE_TEST is enabled")
depend on that patch ("mtd: Make MTD_BCM47XXSFLASH to depend on MIPS"),
right? So I cannot apply the bcma patch until the mtd patch is in my
tree.


That's correct, sorry for not stating it explicitly.


Ok, I put the patch now to "Awaiting Upstream" state in patchwork and
will apply it once I have the mtd patch.


I don't see "mtd: Make MTD_BCM47XXSFLASH to depend on MIPS" in 4.4-rc1.


Yeah, my patch started a very long discussion which has become too MIPS
specific for me to follow. The latest email from January 24 is [0].


I'll drop this now, please resend once the dependency patch is applied.



Yeah, just drop the patch and I can resend if/when the discussion of the
first patch settles and someone fix it properly.

[0]: https://lkml.org/lkml/2016/1/24/320

Best regards,
--
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] wlcore/wl12xx: spi: add device tree support

[Javier Martinez Canillas]

Hello Uri,

On Thu, Dec 24, 2015 at 12:35 PM, Uri Mashiach
 wrote:
> Add DT support for the wl1271 SPI WiFi.
>
> Add documentation file for the wl1271 SPI WiFi.
>
> Signed-off-by: Uri Mashiach 
> Acked-by: Igor Grinberg 
> ---
> v1 -> v2: update interrupt documentation.
>   replace interrupts and interrupt-parent with interrupts-extended.
>   IRQ parameters retrieved from spi_device instead of DT.
>   remove redundant #ifdef CONFIG_OF
>
>  .../bindings/net/wireless/ti,wlcore,spi.txt| 34 +
>  drivers/net/wireless/ti/wlcore/spi.c   | 55 
> --
>  2 files changed, 85 insertions(+), 4 deletions(-)
>  create mode 100644 
> Documentation/devicetree/bindings/net/wireless/ti,wlcore,spi.txt
>
> diff --git a/Documentation/devicetree/bindings/net/wireless/ti,wlcore,spi.txt 
> b/Documentation/devicetree/bindings/net/wireless/ti,wlcore,spi.txt
> new file mode 100644
> index 000..502c27e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/wireless/ti,wlcore,spi.txt
> @@ -0,0 +1,34 @@
> +* Texas Instruments wl1271 wireless lan controller
> +
> +The wl1271 chip can be connected via SPI or via SDIO. This
> +document describes the binding for the SPI connected chip.
> +
> +Required properties:
> +- compatible :  Should be "ti,wl1271"
> +- reg : Chip select address of device
> +- spi-max-frequency :   Maximum SPI clocking speed of device in Hz
> +- ref-clock-frequency : Reference clock frequency
> +- interrupts-extended : Should contain parameters for 1 interrupt line.
> +Interrupt parameters: parent, line number, type.
> +- vwlan-supply :Point the node of the regulator that powers/enable 
> the wl1271 chip
> +
> +Optional properties:
> +- clock-xtal :  boolean, clock is generated from XTAL
> +
> +- Please consult Documentation/devicetree/bindings/spi/spi-bus.txt
> +  for optional SPI connection related properties,
> +
> +Examples:
> +
> + {
> +   wl1271@1 {
> +   compatible = "ti,wl1271";
> +
> +   reg = <1>;
> +   spi-max-frequency = <4800>;
> +   clock-xtal;
> +   ref-clock-frequency = <3840>;
> +   interrupts-extended = < 8 IRQ_TYPE_LEVEL_HIGH>;
> +   vwlan-supply = <_fixed>;
> +   };
> +};
> diff --git a/drivers/net/wireless/ti/wlcore/spi.c 
> b/drivers/net/wireless/ti/wlcore/spi.c
> index d3a4bcb..e9e8d54 100644
> --- a/drivers/net/wireless/ti/wlcore/spi.c
> +++ b/drivers/net/wireless/ti/wlcore/spi.c
> @@ -30,6 +30,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>
>  #include "wlcore.h"
> @@ -357,6 +358,46 @@ static struct wl1271_if_operations spi_ops = {
> .set_block_size = NULL,
>  };
>
> +#ifdef CONFIG_OF
> +static const struct of_device_id wlcore_spi_of_match_table[] = {
> +   { .compatible = "ti,wl1271" },
> +   { }
> +};

Could you please add a MODULE_DEVICE_TABLE(of, wlcore_spi_of_match_table) here?

> +
> +/**
> + * wlcore_probe_of - DT node parsing.
> + * @spi: SPI slave device parameters.
> + * @res: resource parameters.
> + * @glue: wl12xx SPI bus to slave device glue parameters.
> + * @pdev_data: wlcore device parameters
> + */
> +static int wlcore_probe_of(struct spi_device *spi, struct wl12xx_spi_glue 
> *glue,
> +  struct wlcore_platdev_data *pdev_data)
> +{
> +   struct device_node *dt_node = spi->dev.of_node;
> +   int ret;
> +
> +   if (of_find_property(dt_node, "clock-xtal", NULL))
> +   pdev_data->ref_clock_xtal = true;
> +
> +   ret = of_property_read_u32(dt_node, "ref-clock-frequency",
> +  _data->ref_clock_freq);
> +   if (IS_ERR_VALUE(ret)) {
> +   dev_err(glue->dev,
> +   "can't get reference clock frequency (%d)\n", ret);
> +   return ret;
> +   }
> +
> +   return 0;
> +}
> +#else /* CONFIG_OF */
> +static int wlcore_probe_of(struct spi_device *spi, struct wl12xx_spi_glue 
> *glue,
> +  struct wlcore_platdev_data *pdev_data)
> +{
> +   return -ENODATA;
> +}
> +#endif /* CONFIG_OF */
> +

I don't understand what's the point of making the driver to be built
with !CONFIG_OF if the probe function is going to fail anyways. If
this driver really need then is better to remove the ifdefery and make
the Kconfig symbol to depend on OF.

Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/3] wlcore/wl12xx: spi: add wifi support to cm-t335

[Javier Martinez Canillas]

Hello Uri,

On Thu, Dec 24, 2015 at 12:35 PM, Uri Mashiach
 wrote:
> Device tree modifications:
> - Pinmux for SPI0 and WiFi GPIOs.
> - SPI0 node with wlcore as a child node.
>
> Cc: Tony Lindgren 
> Signed-off-by: Uri Mashiach 
> Acked-by: Igor Grinberg 
> ---
> v1 -> v2: replace interrupts and interrupt-parent with interrupts-extended.
>
>  arch/arm/boot/dts/am335x-cm-t335.dts | 57 
> +++-
>  1 file changed, 56 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/am335x-cm-t335.dts 
> b/arch/arm/boot/dts/am335x-cm-t335.dts
> index 42e9b66..31f8371 100644
> --- a/arch/arm/boot/dts/am335x-cm-t335.dts
> +++ b/arch/arm/boot/dts/am335x-cm-t335.dts
> @@ -11,6 +11,7 @@
>  /dts-v1/;
>
>  #include "am33xx.dtsi"
> +#include 
>
>  / {
> model = "CompuLab CM-T335";
> @@ -40,6 +41,15 @@
> regulator-max-microvolt = <330>;
> };
>
> +   /* Regulator for WiFi */
> +   vwlan_fixed: fixedregulator@2 {
> +   compatible = "regulator-fixed";
> +   regulator-name = "vwlan_fixed";
> +   gpio = < 20 GPIO_ACTIVE_HIGH>; /* gpio0_20 */
> +   enable-active-high;
> +   regulator-boot-off;
> +   };
> +
> backlight {
> compatible = "pwm-backlight";
> pwms = < 0 5 0>;
> @@ -50,7 +60,10 @@
>
>  _pinmux {
> pinctrl-names = "default";
> -   pinctrl-0 = <_pins>;
> +   pinctrl-0 = <
> + _pins
> + _pins
> +   >;

The pinctrl lines should be in the device node that needs the pin
muxing (unless there isn't a device node) so I think is better if you
mofe the pinctrl-0 = <_pins> to the wlcore dev node.

Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] bcma: mips: Allow build if COMPILE_TEST is enabled

[Javier Martinez Canillas]

Hello,

On 10/13/2015 05:25 PM, kbuild test robot wrote:
> Hi Javier,
> 
> [auto build test ERROR on v4.3-rc5 -- if it's inappropriate base, please 
> suggest rules for selecting the more suitable base]
> 
> url:
> https://github.com/0day-ci/linux/commits/Javier-Martinez-Canillas/bcma-mips-Allow-build-if-COMPILE_TEST-is-enabled/20151013-214630
> config: i386-allmodconfig (attached as .config)
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=i386 
> 
> All errors (new ones prefixed by >>):
> 
>drivers/mtd/devices/bcm47xxsflash.c: In function 'bcm47xxsflash_read':
>>> drivers/mtd/devices/bcm47xxsflash.c:112:37: error: implicit declaration of 
>>> function 'KSEG0ADDR' [-Werror=implicit-function-declaration]
>  memcpy_fromio(buf, (void __iomem *)KSEG0ADDR(b47s->window + from),
>

Thanks for reporting this issue. I only did a partial build with
make M=drivers/bcma but didn't think about implicit dependencies
on other drivers.

I've posted this patch that should avoid this issue:
https://patchwork.kernel.org/patch/7391551/

BTW, 0-day bot used to reports from email address fengguang...@intel.com
but now reports from l...@intel.com. I still added a Reported-by tag from
Fengguang Wu <fengguang...@intel.com> but I wonder if I should change it
to kbuild test robot <l...@intel.com> for future patches?

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] bcma: mips: Allow build if COMPILE_TEST is enabled

[Javier Martinez Canillas]

Hello Kalle,

On 10/14/2015 12:52 PM, Kalle Valo wrote:
> Javier Martinez Canillas <jav...@osg.samsung.com> writes:
> 
>> Hello,
>>
>> On 10/13/2015 05:25 PM, kbuild test robot wrote:
>>> Hi Javier,
>>>
>>> [auto build test ERROR on v4.3-rc5 -- if it's inappropriate base, please 
>>> suggest rules for selecting the more suitable base]
>>>
>>> url:
>>> https://github.com/0day-ci/linux/commits/Javier-Martinez-Canillas/bcma-mips-Allow-build-if-COMPILE_TEST-is-enabled/20151013-214630
>>> config: i386-allmodconfig (attached as .config)
>>> reproduce:
>>> # save the attached .config to linux build tree
>>> make ARCH=i386 
>>>
>>> All errors (new ones prefixed by >>):
>>>
>>>drivers/mtd/devices/bcm47xxsflash.c: In function 'bcm47xxsflash_read':
>>>>> drivers/mtd/devices/bcm47xxsflash.c:112:37: error: implicit declaration 
>>>>> of function 'KSEG0ADDR' [-Werror=implicit-function-declaration]
>>>  memcpy_fromio(buf, (void __iomem *)KSEG0ADDR(b47s->window + from),
>>>
>>
>> Thanks for reporting this issue. I only did a partial build with
>> make M=drivers/bcma but didn't think about implicit dependencies
>> on other drivers.
>>
>> I've posted this patch that should avoid this issue:
>> https://patchwork.kernel.org/patch/7391551/
> 
> So this patch ("bcma: mips: Allow build if COMPILE_TEST is enabled")
> depend on that patch ("mtd: Make MTD_BCM47XXSFLASH to depend on MIPS"),
> right? So I cannot apply the bcma patch until the mtd patch is in my
> tree.
>

That's correct, sorry for not stating it explicitly.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] bcma: mips: Allow build if COMPILE_TEST is enabled

[Javier Martinez Canillas]

Hello Kale,

On 10/14/2015 01:04 PM, Kalle Valo wrote:
> Javier Martinez Canillas <jav...@osg.samsung.com> writes:
> 
>> On 10/14/2015 12:52 PM, Kalle Valo wrote:
>>> Javier Martinez Canillas <jav...@osg.samsung.com> writes:
>>>
>>>> Thanks for reporting this issue. I only did a partial build with
>>>> make M=drivers/bcma but didn't think about implicit dependencies
>>>> on other drivers.
>>>>
>>>> I've posted this patch that should avoid this issue:
>>>> https://patchwork.kernel.org/patch/7391551/
>>>
>>> So this patch ("bcma: mips: Allow build if COMPILE_TEST is enabled")
>>> depend on that patch ("mtd: Make MTD_BCM47XXSFLASH to depend on MIPS"),
>>> right? So I cannot apply the bcma patch until the mtd patch is in my
>>> tree.
>>
>> That's correct, sorry for not stating it explicitly.
> 
> Ok, I put the patch now to "Awaiting Upstream" state in patchwork and
> will apply it once I have the mtd patch.
>

Great, thanks a lot for your help!
 
Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] bcma: mips: Allow build if COMPILE_TEST is enabled

[Javier Martinez Canillas]

This driver only has a runtime but no build time dependencies so can be
built for testing purposes if the Kconfig COMPILE_TEST option is enabled.

This is useful to have more build coverage and make sure that the driver
is not affected by changes that could cause a build regression.

Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>

---

 drivers/bcma/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bcma/Kconfig b/drivers/bcma/Kconfig
index 023d448ed3fa..22114995dfbf 100644
--- a/drivers/bcma/Kconfig
+++ b/drivers/bcma/Kconfig
@@ -63,7 +63,7 @@ config BCMA_DRIVER_PCI_HOSTMODE
 
 config BCMA_DRIVER_MIPS
bool "BCMA Broadcom MIPS core driver"
-   depends on BCMA && MIPS
+   depends on BCMA && (MIPS || COMPILE_TEST)
help
  Driver for the Broadcom MIPS core attached to Broadcom specific
  Advanced Microcontroller Bus.
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] staging: wicl1000: fix dereference after free in wilc_wlan_cleanup()

[Javier Martinez Canillas]

The wilc_wlan_cleanup() function iterates over the list of transmission
buffers freeing all of them and then iterates over the receive buffers
list to free all of them as well.

But on the receive loop a pointer to struct txq_entry_t is dereferenced
instead of the pointer to a struct rxq_entry_t. This not only causes a
dereference to a pointer already freed but also leaks the memory in the
struct rxq_entry_t buffer.

Also, the buffer is allocated when MEMORY_STATIC is not defined no when
MEMORY_DYNAMIC is defined. So use #ifndef MEMORY_STATIC instead as it's
done in the rest of the driver to avoid leaking the buffer memory.

Fixes: c5c77ba18ea6 ("staging: wilc1000: Add SDIO/SPI 802.11 driver")
Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>

---

Changes in v2:
- Change also the #ifdef MEMORY_DYNAMIC to #ifndef MEMORY_STATIC since buffer
  is allocated when MEMORY_STATIC isn't defined. Suggested by Sudip Mukherjee.

 drivers/staging/wilc1000/wilc_wlan.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_wlan.c 
b/drivers/staging/wilc1000/wilc_wlan.c
index 4c25179c2fec..fc9028d59dcd 100644
--- a/drivers/staging/wilc1000/wilc_wlan.c
+++ b/drivers/staging/wilc1000/wilc_wlan.c
@@ -1745,8 +1745,8 @@ static void wilc_wlan_cleanup(void)
rqe = wilc_wlan_rxq_remove();
if (rqe == NULL)
break;
-#ifdef MEMORY_DYNAMIC
-   kfree(tqe->buffer);
+#ifndef MEMORY_STATIC
+   kfree(rqe->buffer);
 #endif
kfree(rqe);
} while (1);
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] staging: wicl1000: fix dereference after free in wilc_wlan_cleanup()

[Javier Martinez Canillas]

The wilc_wlan_cleanup() function iterates over the list of transmission
buffers freeing all of them and then iterates over the receive buffers
list to free all of them as well.

But on the receive loop a pointer to struct txq_entry_t is dereferenced
instead of the pointer to a struct rxq_entry_t. This not only causes a
dereference to a pointer already freed but also leaks the memory in the
struct rxq_entry_t buffer.

Fixes: c5c77ba18ea6 ("staging: wilc1000: Add SDIO/SPI 802.11 driver")
Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>

---

 drivers/staging/wilc1000/wilc_wlan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/wilc1000/wilc_wlan.c 
b/drivers/staging/wilc1000/wilc_wlan.c
index 4c25179c2fec..c40f143b00b7 100644
--- a/drivers/staging/wilc1000/wilc_wlan.c
+++ b/drivers/staging/wilc1000/wilc_wlan.c
@@ -1746,7 +1746,7 @@ static void wilc_wlan_cleanup(void)
if (rqe == NULL)
break;
 #ifdef MEMORY_DYNAMIC
-   kfree(tqe->buffer);
+   kfree(rqe->buffer);
 #endif
kfree(rqe);
} while (1);
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] staging: wicl1000: remove duplicated operand in OR operation

[Javier Martinez Canillas]

The IEEE80211_STYPE_PROBE_REQ flag appears twice in the expression
and coccicheck complains with:

wilc_wfi_cfgoperations.h:80:3-38: duplicated argument to & or |

Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>

---

 drivers/staging/wilc1000/wilc_wfi_cfgoperations.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.h 
b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.h
index 4d37c4e859e9..25490c2af1e9 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.h
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.h
@@ -80,7 +80,6 @@ static const struct ieee80211_txrx_stypes
BIT(IEEE80211_STYPE_PROBE_REQ >> 4) |
BIT(IEEE80211_STYPE_ASSOC_REQ >> 4) |
BIT(IEEE80211_STYPE_REASSOC_REQ >> 4) |
-   BIT(IEEE80211_STYPE_PROBE_REQ >> 4) |
BIT(IEEE80211_STYPE_DISASSOC >> 4) |
BIT(IEEE80211_STYPE_AUTH >> 4) |
BIT(IEEE80211_STYPE_DEAUTH >> 4)
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] staging: wicl1000: fix dereference after free in wilc_wlan_cleanup()

[Javier Martinez Canillas]

Hello Sudip,

Thanks a lot for your feedback.

On 09/22/2015 02:16 PM, Sudip Mukherjee wrote:
> On Tue, Sep 22, 2015 at 12:24:50PM +0200, Javier Martinez Canillas wrote:
>> The wilc_wlan_cleanup() function iterates over the list of transmission
>> buffers freeing all of them and then iterates over the receive buffers
>> list to free all of them as well.
>>
>> But on the receive loop a pointer to struct txq_entry_t is dereferenced
>> instead of the pointer to a struct rxq_entry_t. This not only causes a
>> dereference to a pointer already freed but also leaks the memory in the
>> struct rxq_entry_t buffer.
>>
>> Fixes: c5c77ba18ea6 ("staging: wilc1000: Add SDIO/SPI 802.11 driver")
>> Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
>>
>> ---
>>
>>  drivers/staging/wilc1000/wilc_wlan.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/wilc1000/wilc_wlan.c 
>> b/drivers/staging/wilc1000/wilc_wlan.c
>> index 4c25179c2fec..c40f143b00b7 100644
>> --- a/drivers/staging/wilc1000/wilc_wlan.c
>> +++ b/drivers/staging/wilc1000/wilc_wlan.c
>> @@ -1746,7 +1746,7 @@ static void wilc_wlan_cleanup(void)
>>  if (rqe == NULL)
>>  break;
>>  #ifdef MEMORY_DYNAMIC
>> -kfree(tqe->buffer);
>> +kfree(rqe->buffer);
>>  #endif
> MEMORY_DYNAMIC is only used here and no where else. And buffer was
> allocated in the else part of #ifdef MEMORY_STATIC.
> So you should really be using #ifndef MEMORY_STATIC here instead of
> #ifdef MEMORY_DYNAMIC otherwise memory leak will still remain.
>

You are right that #ifndef MEMORY_STATIC should be used instead to fix
the memory leak. I'll post a v2 changing that as well.

> regards
> sudip
>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RESEND PATCH] NFC: trf7970a: Add OF match table

[Javier Martinez Canillas]

The Documentation/devicetree/bindings/net/nfc/trf7970a.txt DT binding doc
lists "ti,trf7970a" as a compatible string but the corresponding driver
does not have an OF match table. Add the table to the driver so the SPI
core can do an OF style match.

Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>

---

 drivers/nfc/trf7970a.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/nfc/trf7970a.c b/drivers/nfc/trf7970a.c
index 70b0707fd9a9..123aa981c9d8 100644
--- a/drivers/nfc/trf7970a.c
+++ b/drivers/nfc/trf7970a.c
@@ -2211,6 +2211,12 @@ static const struct dev_pm_ops trf7970a_pm_ops = {
trf7970a_pm_runtime_resume, NULL)
 };
 
+static const struct of_device_id trf7970a_of_match[] = {
+   { .compatible = "ti,trf7970a", },
+   { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, trf7970a_of_match);
+
 static const struct spi_device_id trf7970a_id_table[] = {
{ "trf7970a", 0 },
{ }
@@ -2223,6 +2229,7 @@ static struct spi_driver trf7970a_spi_driver = {
.id_table   = trf7970a_id_table,
.driver = {
.name   = "trf7970a",
+   .of_match_table = of_match_ptr(trf7970a_of_match),
.owner  = THIS_MODULE,
.pm = _pm_ops,
},
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 00/18] Export SPI and OF module aliases in missing drivers

[Javier Martinez Canillas]

Hello,

Short version:

This patch series is the SPI equivalent of the I2C one posted before [0].

This series add the missing MODULE_DEVICE_TABLE() for OF and SPI tables
to export that information so modules have the correct aliases built-in
and autoloading works correctly.

Longer version:

The SPI core always reports the MODALIAS uevent as spi:modalias
regardless of the mechanism that was used to register the device (i.e:
OF or board code) and the table that is used later to match the driver
with the device (i.e: SPI id table or OF match table).

But this means that OF-only drivers needs to have both OF and SPI id
tables that have to be kept in sync and also the device node's compatible
manufacturer prefix is stripped when reporting the MODALIAS. Which can
lead to issues if two vendors use the same SPI device name for example.

Also, there are many SPI drivers whose module auto-loading is not working
because of this fact that the SPI core always reports the MODALIAS as
spi:modalias and many developers didn't expect this since is not how
other subsystems behave.

I've identified SPI drivers with 3 types of different issues:

a) Those that have an spi_table but are not exported. The match works
   if the driver is built-in but since the ID table is not exported,
   module auto-load won't work.

b) Those that have a of_table but are not exported. This is currently
   not an issue since even when the of_table is used to match the dev
   with the driver, an OF modalias is not reported by the SPI core.
   But if the SPI core is changed to report the MODALIAS of the form
   of:N*T*C as it's made by other subsystems, then module auto-load
   will break for these drivers.

c) Those that don't have an of_table but should since are OF drivers
   with DT bindings doc for them. Since the SPI core does not report
   a OF modalias and since spi_match_device() fallbacks to match the
   device part of the compatible string with the SPI device ID table,
   many OF drivers don't have an of_table to match. After all having
   a SPI device ID table is mandatory so it works without a of_table.

So, in order to not make mandatory to have a SPI device ID table, all
these three kind of issues have to be addressed. This series does that.

I split the changes so the patches in this series are independent and
can be picked individually by subsystem maintainers.

Patches #1 and #2 solves a), patches #3 to #8 solves b) and patches

Patch #18 changes the logic of spi_uevent() to report an OF modalias if
the device was registered using OF. But this patch is included in the
series only as an RFC for illustration purposes since changing that
without first applying all the other patches in this series, will break
module autoloading for the drivers of devices registered using OF but
that lacks an of_match_table. I'll repost patch #18 once all the patches
in this series have landed.

[0]: https://lkml.org/lkml/2015/7/30/519

Best regards,
Javier


Javier Martinez Canillas (18):
  iio: Export SPI module alias information in missing drivers
  staging: iio: hmc5843: Export missing SPI module alias information
  mtd: dataflash: Export OF module alias information
  OMAPDSS: panel-sony-acx565akm: Export OF module alias information
  mmc: mmc_spi: Export OF module alias information
  staging: mt29f_spinand: Export OF module alias information
  net: ks8851: Export OF module alias information
  [media] s5c73m3: Export OF module alias information
  mfd: cros_ec: spi: Add OF match table
  iio: dac: ad7303: Add OF match table
  iio: adc: max1027: Set struct spi_driver .of_match_table
  mfd: stmpe: Add OF match table
  iio: adc: mcp320x: Set struct spi_driver .of_match_table
  iio: as3935: Add OF match table
  iio: adc128s052: Add OF match table
  iio: frequency: adf4350: Add OF match table
  NFC: trf7970a: Add OF match table
  spi: (RFC, don't apply) report OF style modalias when probing using DT

 drivers/iio/adc/max1027.c   |  1 +
 drivers/iio/adc/mcp320x.c   |  1 +
 drivers/iio/adc/ti-adc128s052.c |  8 
 drivers/iio/amplifiers/ad8366.c |  1 +
 drivers/iio/dac/ad7303.c|  7 +++
 drivers/iio/frequency/adf4350.c |  9 +
 drivers/iio/proximity/as3935.c  |  7 +++
 drivers/media/i2c/s5c73m3/s5c73m3-spi.c |  1 +
 drivers/mfd/cros_ec_spi.c   |  7 +++
 drivers/mfd/stmpe-spi.c | 13 +
 drivers/mmc/host/mmc_spi.c  |  1 +
 drivers/mtd/devices/mtd_dataflash.c |  1 +
 drivers/net/ethernet/micrel/ks8851.c|  1 +
 drivers/nfc/trf7970a.c  |  7 +++
 drivers/spi/spi.c   |  8

Re: [PATCH 00/18] Export SPI and OF module aliases in missing drivers

[Javier Martinez Canillas]

Hello Brian,

On 08/20/2015 11:11 PM, Brian Norris wrote:
 On Thu, Aug 20, 2015 at 09:07:13AM +0200, Javier Martinez Canillas wrote:
 Patches #1 and #2 solves a), patches #3 to #8 solves b) and patches
 
 ^^^ I'm dying to know how this sentence ends :)


Sigh, I did some last minute restructuring of the cover letter and
seems I missed a sentence. I meant to said:

and patches #9 to #17 solves c).
 
 Patch #18 changes the logic of spi_uevent() to report an OF modalias if
 the device was registered using OF. But this patch is included in the
 series only as an RFC for illustration purposes since changing that
 without first applying all the other patches in this series, will break
 module autoloading for the drivers of devices registered using OF but
 that lacks an of_match_table. I'll repost patch #18 once all the patches
 in this series have landed.
 
 On a more productive note, the patches I've looked at look good to me.
 The missing aliases are a problem enough that should be fixed (i.e.,
 part (b)). I'll leave the SPI framework changes to others to comment on.


Great, thanks a lot for your feedback.
 
 Brian
 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 5/6] ARM: dts: add wl12xx/wl18xx bindings

[Javier Martinez Canillas]

Hello Eliad,

On Thu, Mar 12, 2015 at 1:09 PM, Eliad Peller el...@wizery.com wrote:
 Replace all the pdata-quirks for setting wl12xx/wl18xx
 platform data with proper DT definitions.

 The patch was compile-tested only.

 Signed-off-by: Eliad Peller el...@wizery.com
 ---

I wanted to test your series but I realized that the IGEPv2 I've
access is the old revision that has another SDIO wlan module instead
of a Wilink8.

Maybe Enric have the new revision? I also added to Pau Pajuel as cc
who still works in the company manufacturing these boards so he should
be able to test it.

But in any case, your patch looks good to me so for the
arch/arm/boot/dts/omap3-igep00* changes:

Acked-by: Javier Martinez Canillas jav...@dowhile0.org

Best regards,
Javier
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Javier Martinez Canillas]

Hello Arnd,

On Wed, Mar 11, 2015 at 10:53 AM, Arnd Bergmann a...@arndb.de wrote:
 On Wednesday 11 March 2015 02:00:59 Javier Martinez Canillas wrote:

 What do you mean by parsing here? IIUC there isn't a clock driver for
 these clocks and are setup directly in the
 drivers/net/wireless/ti/wl12xx/main.c driver.

 So you can't make the WLAN chip dev node a consumer of these clocks by
 adding a phandle to a clock provider and clock specifiers since there
 isn't a provider to be referenced in the DT. Or did I misunderstand?

 As I understand it, the clock signal is provided by an external oscillator,

According to [0], it seems the chip can be connected to both external
oscillators or internal clocks provided by the chip itself.

 which we can easily model in DT, and then you call clk_get_rate on that.


Right, my point wast that this can be done only if the external
oscillator have a proper clock driver / provider which I don't think
is the case here. Most of this stuff predates the common clock
framework.

Or at least Luciano Coelho had a patch on his series to make the
wilink driver a clock provider itself by registering the refclock and
tcxoclock clocks [0].

Luciano also had patches for:

* Adding the clock provider dev node in the DTS [1]
* Have a table to map the clock rate with the FW configuration values [2]
* Getting the clock from DT and the rate as you said to configure the
firmware accordingly [3]

I think that patch [0] should not be needed since for external clocks,
the IP providing the clocks should have its own clock driver and for
internal clocks, a property should be used instead as you said.

 If there is no external clock provider for this chip and the clocks
 are provided by the device itself, then all we need is a clock-frequency
 property in the device node.


Agreed, IIUC Luciano wanted to expose the internal clocks by
registering in the common clock framework but if those clocks are not
really accessible from outside the wlan chip, then I also think that a
device node property should be used instead.

 Arnd
 --

Best regards,
Javier

[0]: https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-July/037139.html
[1]: http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/187794.html
[2]: http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/181594.html
[3]: http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/181591.html
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Javier Martinez Canillas]

Hello Eliad,

On Wed, Mar 11, 2015 at 12:59 PM, Eliad Peller el...@wizery.com wrote:
 On Wed, Mar 11, 2015 at 3:19 AM, Javier Martinez Canillas
 jav...@dowhile0.org wrote:

 But there wasn't a DT binding to describe that so most boards use the
 same hack which is to define a fake fixed-regulator with an enable
 GPIO just to toggle that pin.

 But now the MMC subsystem has support for power sequence providers and
 the pwrseq-simple driver
 (Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt) has
 support for reset GPIOs so you may want to change that as well.

 interesting. i wasn't aware of it.
 but that's for a different time i guess :)


Indeed, I've on my TODO anyways so I can post a patch on top of your series :-)

 thanks,
 Eliad.

Best regards,
Javier
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Javier Martinez Canillas]

Hello Tony,

On Tue, Mar 10, 2015 at 6:35 PM, Tony Lindgren t...@atomide.com wrote:

 we do have to make sure these wl18xx bindings are future-compatible
 with the wl12xx ones, but i think the current bindings are pretty much
 standard (and are actually a subset of the bindings needed by wl12xx),
 so it should be fine.

 Well how about add just the parsing of the clock and assume it's always
 WL12XX_REFCLOCK_38 for now as that's the only thing we're currently
 using. Then we can add a property or compatible value if using something
 else as needed.


What do you mean by parsing here? IIUC there isn't a clock driver for
these clocks and are setup directly in the
drivers/net/wireless/ti/wl12xx/main.c driver.

So you can't make the WLAN chip dev node a consumer of these clocks by
adding a phandle to a clock provider and clock specifiers since there
isn't a provider to be referenced in the DT. Or did I misunderstand?

 Regards,

 Tony

Best regards,
Javier
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Javier Martinez Canillas]

Hello Eliad,

On Wed, Mar 11, 2015 at 1:28 AM, Javier Martinez Canillas
jav...@dowhile0.org wrote:
 On Mon, Mar 9, 2015 at 4:36 PM, Eliad Peller el...@wizery.com wrote:
 --- a/arch/arm/boot/dts/omap3-igep0020-rev-f.dts
 +++ b/arch/arm/boot/dts/omap3-igep0020-rev-f.dts
 @@ -42,4 +42,13 @@
 vmmc-supply = lbep5clwmc_wlen;

Something I forgot to mention is that this vmmc-supply is a DT hack
since the WLAN SDIO chip needs a WL_EN signal to be asserted as a part
of the chip power sequencing.

But there wasn't a DT binding to describe that so most boards use the
same hack which is to define a fake fixed-regulator with an enable
GPIO just to toggle that pin.

But now the MMC subsystem has support for power sequence providers and
the pwrseq-simple driver
(Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt) has
support for reset GPIOs so you may want to change that as well.

In fact, the pwrseq-simple also has support for an external clock (and
can be extended to support more than one) so if the clocks are not
internal to the wl12xx, maybe these should be defined in the
pwrseq-simple dev node assuming that there is a clock driver for them.

Best regards,
Javier
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 3/3] ARM: dts: igep00x0: add wl18xx bindings

[Javier Martinez Canillas]

Hello Eliad,

On Mon, Mar 9, 2015 at 4:36 PM, Eliad Peller el...@wizery.com wrote:
 Add wl18xx (wilink8) bindings to omap3-igep0030-rev-g and
 omap3-igep0020-rev-f dts files, instead of defining the
 platform data through the pdata-quirks.

 The patch was compile-tested only.


You should look at MAINTAINERS or use ./scripts/get_maintainer.pl to
know who should be cc'ed. Specially for this kind of patches where you
are not able to test on the actual platform. Added Enric to cc as well
since he also maintains the IGEP boards an has access to the hw too.

 Signed-off-by: Eliad Peller el...@wizery.com
 ---
  arch/arm/boot/dts/omap3-igep0020-rev-f.dts | 9 +
  arch/arm/boot/dts/omap3-igep0030-rev-g.dts | 9 +
  arch/arm/mach-omap2/pdata-quirks.c | 2 --
  3 files changed, 18 insertions(+), 2 deletions(-)

 diff --git a/arch/arm/boot/dts/omap3-igep0020-rev-f.dts 
 b/arch/arm/boot/dts/omap3-igep0020-rev-f.dts
 index cc8bd0c..8e5b44e 100644
 --- a/arch/arm/boot/dts/omap3-igep0020-rev-f.dts
 +++ b/arch/arm/boot/dts/omap3-igep0020-rev-f.dts
 @@ -42,4 +42,13 @@
 vmmc-supply = lbep5clwmc_wlen;
 bus-width = 4;
 non-removable;
 +
 +   #address-cells = 1;
 +   #size-cells = 0;
 +   wlcore: wlcore@2 {
 +   compatible = ti,wl1835;
 +   reg = 2;
 +   interrupt-parent = gpio6;
 +   interrupts = 17 IRQ_TYPE_NONE;

As Arnd said, it seems this should be IRQ_TYPE_LEVEL_HIGH to match
what the platform code is currently doing.

Best regards,
Javier
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/3] wl18xx: add basic device-tree support

[Javier Martinez Canillas]

Hello Eliad,

On Mon, Mar 9, 2015 at 4:36 PM, Eliad Peller el...@wizery.com wrote:
 When running with device-tree, we no longer have a board file
 that can set up the platform data for wlcore.
 Allow this data to be passed from DT.

 For now, parse only the irq used. Other (optional) properties
 can be added later on.

 Signed-off-by: Ido Yariv i...@wizery.com
 Signed-off-by: Eliad Peller el...@wizery.com
 ---

I see this is a v5 but I don't know what was changed from prior
revisions. It would be good if the patches had a versions history.

  drivers/net/wireless/ti/wlcore/sdio.c | 80 
 ---
  1 file changed, 74 insertions(+), 6 deletions(-)

 diff --git a/drivers/net/wireless/ti/wlcore/sdio.c 
 b/drivers/net/wireless/ti/wlcore/sdio.c
 index d3dd7bf..ee556ac 100644
 --- a/drivers/net/wireless/ti/wlcore/sdio.c
 +++ b/drivers/net/wireless/ti/wlcore/sdio.c
 @@ -34,6 +34,8 @@
  #include linux/wl12xx.h
  #include linux/pm_runtime.h
  #include linux/printk.h
 +#include linux/of.h
 +#include linux/of_irq.h

  #include wlcore.h
  #include wl12xx_80211.h
 @@ -214,6 +216,69 @@ static struct wl1271_if_operations sdio_ops = {
 .set_block_size = wl1271_sdio_set_block_size,
  };

 +#ifdef CONFIG_OF
 +static const struct of_device_id wlcore_sdio_of_match_table[] = {
 +   { .compatible = ti,wl1801 },
 +   { .compatible = ti,wl1805 },
 +   { .compatible = ti,wl1807 },
 +   { .compatible = ti,wl1831 },
 +   { .compatible = ti,wl1835 },
 +   { .compatible = ti,wl1837 },
 +   { }
 +};
 +
 +static struct wl12xx_platform_data *wlcore_probe_of(struct device *dev)
 +{
 +   struct device_node *np = dev-of_node;
 +   struct wl12xx_platform_data *pdata;
 +
 +   if (!np || !of_match_node(wlcore_sdio_of_match_table, np))
 +   return NULL;
 +
 +   pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
 +   if (!pdata)
 +   return NULL;
 +
 +   pdata-irq = irq_of_parse_and_map(np, 0);
 +   if (!pdata-irq) {
 +   dev_err(dev, No irq in platform data\n);
 +   kfree(pdata);
 +   return NULL;
 +   }
 +
 +   return pdata;
 +}
 +#else
 +static struct wl12xx_platform_data *wlcore_probe_of(struct device *dev)
 +{
 +   return NULL;
 +}
 +#endif
 +
 +static struct wl12xx_platform_data *
 +wlcore_get_platform_data(struct device *dev)
 +{
 +   struct wl12xx_platform_data *pdata;
 +
 +   /* first, look for DT data */

I thought it was the opposite, that platform data should over-rule DT.
That way you can still use the data filled in
arch/arm/mach-omap2/pdata-quirks.c even after the driver supports your
new DT binding.

 +   pdata = wlcore_probe_of(dev);
 +   if (pdata)
 +   return pdata;
 +
 +   /* if not found - fallback to static platform data */
 +   pdata = wl12xx_get_platform_data();
 +   if (!IS_ERR(pdata))
 +   return kmemdup(pdata, sizeof(*pdata), GFP_KERNEL);
 +
 +   dev_err(dev, No platform data set\n);
 +   return NULL;
 +}
 +
 +static void wlcore_del_platform_data(struct wl12xx_platform_data *pdata)
 +{
 +   kfree(pdata);
 +}
 +

This function seems to be an unnecessary, why not just call kfree() directly?

Or better, maybe the resource-managed devm_*() functions can be used
so the data doesn't have to be explicitly freed?

Best regards,
Javier
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html