Send users mailing list submissions to
[email protected]
To subscribe or unsubscribe via the World Wide Web, visit
http://rt2x00.serialmonkey.com/mailman/listinfo/users_rt2x00.serialmonkey.com
or, via email, send a message with subject or body 'help' to
[email protected]
You can reach the person managing the list at
[email protected]
When replying, please edit your Subject line so it is more specific
than "Re: Contents of users digest..."
Today's Topics:
1. Re: [PATCH 1/2] rt2x00: rt2800pci: verify ioremap return
value (Gabor Juhos)
2. Re: [PATCH 2/2] rt2x00: rt2800pci: allow to load EEPROM data
via firmware API (Gabor Juhos)
3. Re: [PATCH 1/2] rt2x00: rt2800pci: verify ioremap return
value (Gabor Juhos)
4. Re: [PATCH 1/2] rt2x00: rt2800pci: verify ioremap return
value (Gabor Juhos)
----------------------------------------------------------------------
Message: 1
Date: Wed, 19 Dec 2012 10:34:37 +0100
From: Gabor Juhos <[email protected]>
To: Stanislaw Gruszka <[email protected]>
Cc: Daniel Golle <[email protected]>, [email protected],
[email protected]
Subject: Re: [rt2x00-users] [PATCH 1/2] rt2x00: rt2800pci: verify
ioremap return value
Message-ID: <[email protected]>
Content-Type: text/plain; charset=ISO-8859-1
2012.12.18. 22:57 keltez?ssel, Stanislaw Gruszka ?rta:
> On Tue, Dec 18, 2012 at 05:22:22PM +0100, Gabor Juhos wrote:
>> An ioremap call is allowed to fail, however
>> the return value of that is not checked in
>> the 'rt2800pci_read_eeprom_soc' function.
>>
>> The patch adds the missing check, and makes
>> the function return an int value. The patch
>> also converts the 'rt2800_read_eeprom' and
>> 'rt2800_ops.read_eeprom' functions to return
>> an int value, so the error value can be
>> propagated up to the 'rt2800_validate_eeprom'
>> function.
>>
>> Signed-off-by: Gabor Juhos <[email protected]>
> Reviewed-by: Stanislaw Gruszka <[email protected]>
Thanks!
-Gabor
------------------------------
Message: 2
Date: Wed, 19 Dec 2012 10:35:22 +0100
From: Gabor Juhos <[email protected]>
To: Stanislaw Gruszka <[email protected]>
Cc: Daniel Golle <[email protected]>, [email protected],
[email protected]
Subject: Re: [rt2x00-users] [PATCH 2/2] rt2x00: rt2800pci: allow to
load EEPROM data via firmware API
Message-ID: <[email protected]>
Content-Type: text/plain; charset=ISO-8859-1
2012.12.18. 23:22 keltez?ssel, Stanislaw Gruszka ?rta:
> On Tue, Dec 18, 2012 at 05:22:23PM +0100, Gabor Juhos wrote:
>> Currently the driver fetches the EEPROM data
>> from a fixed memory location for SoC devices
>> for SoC devices with a built-in wireless MAC.
>>
>> The usability of this approach is quite
>> limited, because it is only suitable if the
>> location of the EEPROM data is mapped into
>> the memory. This condition is true on embedded
>> boards equipped which are using a parallel NOR
>> flash, but it is not true for boards equipped
>> with SPI or NAND flashes. The fixed location
>> also does not work in all cases, because the
>> offset of the EEPROM data varies between
>> different boards.
>>
>> Additionally, various embedded boards are using
>> a PCI/PCIe chip soldered directly onto the PCB.
>> Such boards usually does not have a separate
>> EEPROM chip for the PCI/PCIe devices, the data
>> of the EEPROM is stored in the main flash
>> instead.
>>
>> The patch makes it possible to load the EEPROM
>> data via firmware API. This new method works
>> regardless of the type of the flash, and it is
>> usable with built-in and with PCI/PCIe devices.
>>
>> Signed-off-by: Gabor Juhos <[email protected]>
>
> I understand this patch will not broke NOR boards, which use
> ioremap approach currently?
The change will break those obviously, so those boards must be converted to use
the new method. I have added sanity check into the 'rt2800soc_probe' function
which ensures that the users of such boards will be informed about that. FWIW,
that approach is used by out-of-tree boards only.
>> + init_completion(&ec.complete);
>> + retval = request_firmware_nowait(THIS_MODULE, 1, name,
>> + rt2x00dev->dev, GFP_KERNEL, &ec,
>> + rt2800pci_eeprom_request_cb);
>> + if (retval < 0) {
>> + ERROR(rt2x00dev, "EEPROM request failed\n");
>> + return retval;
>> + }
>> +
>> + wait_for_completion(&ec.complete);
> Since we use completion here, why we can not just use normal synchronous
> version of request_firmware? I heard of request_firmware drawbacks, so
> this approach can be correct. Just want to know if we do not complicate
> things not necessarily here.
If the driver is built into the kernel, then the synchronous version would fail
because user-space is not up during probe time.
The initial version of the patch used the synchronous version, but Gertjan had
concerns about that:
http://rt2x00.serialmonkey.com/pipermail/users_rt2x00.serialmonkey.com/2012-December/005526.html
>> + goto release_eeprom;
>> + }
>> +
>> + memcpy(rt2x00dev->eeprom, ec.blob->data, EEPROM_SIZE);
>> + retval = 0;
>> +
>> +release_eeprom:
> We do not free memory - I guess we should do relase_firmware(ec.blob)?
Yes. I'm sure that I have added that call once, but it seems lost in the rebase
process. Will send and updated version.
Thank you for the comments!
-Gabor
------------------------------
Message: 3
Date: Wed, 19 Dec 2012 10:35:57 +0100
From: Gabor Juhos <[email protected]>
To: Julian Calaby <[email protected]>
Cc: [email protected], Daniel Golle <[email protected]>,
[email protected]
Subject: Re: [rt2x00-users] [PATCH 1/2] rt2x00: rt2800pci: verify
ioremap return value
Message-ID: <[email protected]>
Content-Type: text/plain; charset=ISO-8859-1
2012.12.19. 0:58 keltez?ssel, Julian Calaby ?rta:
> Hi Gabor,
>
> One minor question:
>
> On Wed, Dec 19, 2012 at 3:22 AM, Gabor Juhos <[email protected]> wrote:
>> An ioremap call is allowed to fail, however
>> the return value of that is not checked in
>> the 'rt2800pci_read_eeprom_soc' function.
>>
>> The patch adds the missing check, and makes
>> the function return an int value. The patch
>> also converts the 'rt2800_read_eeprom' and
>> 'rt2800_ops.read_eeprom' functions to return
>> an int value, so the error value can be
>> propagated up to the 'rt2800_validate_eeprom'
>> function.
>>
>> Signed-off-by: Gabor Juhos <[email protected]>
>> ---
>> drivers/net/wireless/rt2x00/rt2800lib.c | 5 ++++-
>> drivers/net/wireless/rt2x00/rt2800lib.h | 6 +++---
>> drivers/net/wireless/rt2x00/rt2800pci.c | 17 ++++++++++++-----
>> drivers/net/wireless/rt2x00/rt2800usb.c | 4 +++-
>> 4 files changed, 22 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/wireless/rt2x00/rt2800pci.c
>> b/drivers/net/wireless/rt2x00/rt2800pci.c
>> index 9224d87..5fc16dd 100644
>> --- a/drivers/net/wireless/rt2x00/rt2800pci.c
>> +++ b/drivers/net/wireless/rt2x00/rt2800pci.c
>> @@ -970,14 +974,17 @@ static irqreturn_t rt2800pci_interrupt(int irq, void
>> *dev_instance)
>> /*
>> * Device probe functions.
>> */
>> -static void rt2800pci_read_eeprom(struct rt2x00_dev *rt2x00dev)
>> +static int rt2800pci_read_eeprom(struct rt2x00_dev *rt2x00dev)
>> {
>> if (rt2x00_is_soc(rt2x00dev))
>> - rt2800pci_read_eeprom_soc(rt2x00dev);
>> - else if (rt2800pci_efuse_detect(rt2x00dev))
>> + return rt2800pci_read_eeprom_soc(rt2x00dev);
>> +
>> + if (rt2800pci_efuse_detect(rt2x00dev))
>> rt2800pci_read_eeprom_efuse(rt2x00dev);
>> else
>> rt2800pci_read_eeprom_pci(rt2x00dev);
>
> Would it make any sense to have rt2800pci_read_eeprom_efuse() and
> rt2800pci_read_eeprom_pci() return a value, simply for consistency
> with rt2800pci_read_eeprom_soc()?
I wanted to keep the change as minimal as possible, but this would make sense.
>
>> +
>> + return 0;
>> }
>>
>> static const struct ieee80211_ops rt2800pci_mac80211_ops = {
>> diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c
>> b/drivers/net/wireless/rt2x00/rt2800usb.c
>> index 5c149b5..48de5c9 100644
>> --- a/drivers/net/wireless/rt2x00/rt2800usb.c
>> +++ b/drivers/net/wireless/rt2x00/rt2800usb.c
>> @@ -735,13 +735,15 @@ static void rt2800usb_fill_rxdone(struct queue_entry
>> *entry,
>> /*
>> * Device probe functions.
>> */
>> -static void rt2800usb_read_eeprom(struct rt2x00_dev *rt2x00dev)
>> +static int rt2800usb_read_eeprom(struct rt2x00_dev *rt2x00dev)
>> {
>> if (rt2800_efuse_detect(rt2x00dev))
>> rt2800_read_eeprom_efuse(rt2x00dev);
>> else
>> rt2x00usb_eeprom_read(rt2x00dev, rt2x00dev->eeprom,
>> EEPROM_SIZE);
>
> Same here.
Yes, especially that the 'rt2x00usb_eeprom_read' function returns an int value
already.
Thank you for the review!
-Gabor
------------------------------
Message: 4
Date: Wed, 19 Dec 2012 12:59:26 +0100
From: Gabor Juhos <[email protected]>
To: Jones Desougi <[email protected]>
Cc: [email protected], Daniel Golle <[email protected]>,
[email protected]
Subject: Re: [rt2x00-users] [PATCH 1/2] rt2x00: rt2800pci: verify
ioremap return value
Message-ID: <[email protected]>
Content-Type: text/plain; charset=ISO-8859-1
2012.12.19. 12:05 keltez?ssel, Jones Desougi ?rta:
> Hi Gabor,
>
> One thing:
>
> On 12/18/2012 05:22 PM, Gabor Juhos wrote:
>> An ioremap call is allowed to fail, however
>> the return value of that is not checked in
>> the 'rt2800pci_read_eeprom_soc' function.
>>
>> The patch adds the missing check, and makes
>> the function return an int value. The patch
>> also converts the 'rt2800_read_eeprom' and
>> 'rt2800_ops.read_eeprom' functions to return
>> an int value, so the error value can be
>> propagated up to the 'rt2800_validate_eeprom'
>> function.
>>
>> Signed-off-by: Gabor Juhos <[email protected]>
>> +static int rt2800pci_read_eeprom_soc(struct rt2x00_dev *rt2x00dev)
>> {
>> void __iomem *base_addr = ioremap(0x1F040000, EEPROM_SIZE);
>>
>> + if (!base_addr)
>> + return -ENOMEM;
>> +
>> memcpy_fromio(rt2x00dev->eeprom, base_addr, EEPROM_SIZE);
>>
>> iounmap(base_addr);
>> }
>
> Missing return at the end, since it should now return an int.
You are right.
> Even if this is gone with the second patch.
I will post an updated version of the patch-set anyway and will fix this.
Thank you for the review!
-Gabor
------------------------------
Subject: Digest Footer
_______________________________________________
users mailing list
[email protected]
http://rt2x00.serialmonkey.com/mailman/listinfo/users_rt2x00.serialmonkey.com
------------------------------
End of users Digest, Vol 46, Issue 22
*************************************
