Johannes Berg <johannes@xxxxxxxxxxxxxxxx> writes:
> On Sat, 2008-12-06 at 19:32 +0200, Kalle Valo wrote:
>
>
>> +static const u8 default_cal_channels[] = {
>> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x6c, 0x09,
[...]
>> + 0x00 };
>> +
>> +static const u8 default_cal_rssi[] = {
>> + 0x0a, 0x01, 0x72, 0xfe, 0x1a, 0x00, 0x00, 0x00, 0x0a, 0x01, 0x72,
[...]
>> + 0x00, 0x00, 0x00, 0x00, 0x00 };
>
> Is this data actually usable?
Yes, it is. I tested it yesterday and it works ok.
> The static data in cx3110x didn't even parse correctly iirc.
I don't even remember cx3110x having any static calibration data,
wlan-cal provided it always. But then again I haven't looked at
cx3110x for a long time, I might remember wrong.
>> +static void stlc45xx_tx_edcf(struct stlc45xx *stlc);
>> +static void stlc45xx_tx_setup(struct stlc45xx *stlc);
>> +static void stlc45xx_tx_scan(struct stlc45xx *stlc);
>> +static void stlc45xx_tx_psm(struct stlc45xx *stlc, bool enable);
>> +static int stlc45xx_tx_nullfunc(struct stlc45xx *stlc, bool powersave);
>> +static int stlc45xx_tx_pspoll(struct stlc45xx *stlc, bool powersave);
>
> Can you reorder the file?
Sure.
> And I think you must be basing this on top of your PS patches, so
> could you not put Vivek's in the middle too?
Actually I didn't use my PS patches when I tested this patch. But
true, I should try Vivek's patch also with stlc45xx.
>> +static ssize_t stlc45xx_sysfs_store_cal_rssi(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>
> I still think it would be better to use the firmware framework with a
> custom helper for this. At least it should provide a uevent like crda
> does so userspace knows automatically when to upload the files after you
> reload the module for example, I think?
Yeah, you talked about this already and I liked it. I haven't
implemented it just because a lack of time. I'll add it to the todo
list.
>
>> +static ssize_t stlc45xx_sysfs_show_tx_buf(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>
> That should be in debugfs, I think?
I think I should even remove it.
>
>> +static int stlc45xx_request_firmware(struct stlc45xx *stlc)
>> +{
>> + const struct firmware *fw;
>> + int ret;
>> +
>> + /* FIXME: should driver use it's own struct device? */
>> + ret = request_firmware(&fw, "3826.arm", &stlc->spi->dev);
>
> own struct device? I don't see why it should? It doesn't have one
> anyway, does it?
There is stlc45xx_device:
ret = platform_device_register(&stlc45xx_device);
All this device class stuff is not yet fully clear to me, that's why I
added that comment. I guess I should study this more on a boring train
ride.
>> +static int stlc45xx_upload_firmware(struct stlc45xx *stlc)
>
>> + fw_addr = FIRMWARE_ADDRESS;
>> + fw_len = stlc->fw_len;
>
>> + stlc45xx_spi_write(stlc, SPI_ADRS_DMA_DATA, stlc->fw, _fw_len);
>> +
>> + /* FIXME: I think this doesn't work if firmware is large,
>> + * this loop goes to second round. fw->data is not
>> + * increased at all! */
>> + }
>
> Indeed, the last _spi_write above needs to have something like
> stlc->fw + fw_addr - FIRMWARE_ADDRESS
Thanks, I'll fix it at some point.
>> + /*
>> + * FIXME: this gives warning from __ieee80211_rx()
>> + *
>> + * status.rate_idx = data->rate;
>> + */
>
> That works on p54, afaik?
Yes, I haven't looked at this yet.
>> + edcf->queues[0].aifs = 2;
>> + edcf->queues[0].pad0 = 1;
>> + edcf->queues[0].cwmin = 3;
>> + edcf->queues[0].cwmax = 7;
>> + edcf->queues[0].txop = 47;
>> + edcf->queues[1].aifs = 2;
>> + edcf->queues[1].pad0 = 0;
>> + edcf->queues[1].cwmin = 7;
>> + edcf->queues[1].cwmax = 15;
>> + edcf->queues[1].txop = 94;
>> + edcf->queues[2].aifs = 3;
>> + edcf->queues[2].pad0 = 0;
>> + edcf->queues[2].cwmin = 15;
>> + edcf->queues[2].cwmax = 1023;
>> + edcf->queues[2].txop = 0;
>> + edcf->queues[3].aifs = 7;
>> + edcf->queues[3].pad0 = 0;
>> + edcf->queues[3].cwmin = 15;
>> + edcf->queues[3].cwmax = 1023;
>> + edcf->queues[3].txop = 0;
>
> Shouldn't that be taking values from mac80211?
Definitely, just has been pending on my todo list.
>> + setup->bratemask = 0xffffffff;
>
> That should also take values from the BSS config.
Will fix.
>> + scan->flags = LM_SCAN_EXIT;
>> + scan->bratemask = 0x15f;
>
> or that? not sure
Me neither :)
>> +static int stlc45xx_op_tx(struct ieee80211_hw *hw, struct sk_buff *skb)
>> +{
>
>> + entry = stlc45xx_txbuffer_alloc(stlc, skb->len);
>> + if (!entry) {
>> + /* the queue should be stopped before the firmware buffer
>> + * is full, so firmware buffer should always have enough
>> + * space */
>> + if (net_ratelimit())
>> + stlc45xx_warning("firmware buffer full");
>> + spin_unlock_bh(&stlc->tx_lock);
>> + return NETDEV_TX_BUSY;
>> + }
>
> Just drop the frame please, it's much easier and I want to remove the
> possibility of not doing so from mac80211 at some point.
Will do.
>
>> + for (i = 0; i < 8; i++) {
>> + rate = ieee80211_get_tx_rate(stlc->hw, info);
>> + data->aloft[i] = rate->hw_value;
>> + }
>
> You can do much better, like p54, which makes minstrel work a lot
> better :)
I'll fix it.
>> +static int stlc45xx_op_add_interface(struct ieee80211_hw *hw,
>> + struct ieee80211_if_init_conf *conf)
>> +{
>> + struct stlc45xx *stlc = hw->priv;
>> +
>> + stlc45xx_debug(DEBUG_FUNC, "%s", __func__);
>> +
>> + switch (conf->type) {
>> + case NL80211_IFTYPE_STATION:
>> + break;
>> + default:
>> + return -EOPNOTSUPP;
>> + }
>
> No need for this any more since we added the interface_types bitmap. But
> you can keep it as well, for extra error checking.
I'll keep it anyway.
>> +static void stlc45xx_op_remove_interface(struct ieee80211_hw *hw,
>> + struct ieee80211_if_init_conf *conf)
>> +{
>> + stlc45xx_debug(DEBUG_FUNC, "%s", __func__);
>> +}
>
> You really need to implement this better for proper monitor mode.
It's on my todo list.
> Maybe it would be simpler to make p54 work? :) It also implements
> basic rates, slot handling and other things correctly already.
Due to non-technical reasons I cannot work with p54 and that makes
things a bit difficult. So my solution is that I try to make stlc45xx
as good as possible, and if someone wants to add spi support to p54
based on stlc45xx that would be just great.
>> +/* can't be const, mac80211 writes to this */
>> +static struct ieee80211_rate stlc45xx_rates[] = {
>> + { .bitrate = 10, .hw_value = 0, .hw_value_short = 0, },
>
> No need to list hw_value_short since you don't use it, mac80211 never
> uses hw_value or hw_value-short. Since this is identical to the idx you
> could even just use the idx and leave it off, though this is probably
> easier to understand (just a little less efficient).
I'll drop hw_value_short but I want to keep hw_value. Like you said,
it's easier to understand that way.
>> +/* can't be const, mac80211 writes to this */
>> +static struct ieee80211_supported_band stlc45xx_band_2ghz = {
>> + .channels = stlc45xx_channels,
>> + .n_channels = ARRAY_SIZE(stlc45xx_channels),
>> + .bitrates = stlc45xx_rates,
>> + .n_bitrates = ARRAY_SIZE(stlc45xx_rates),
>> +};
>
> Actually, I don't think mac80211 writes to this. Not that const or not
> const matters in the kernel at all, except during compilation.
Ok, I'll remove the comment.
>> +static int stlc45xx_register_mac80211(struct stlc45xx *stlc)
>> +{
>> + /* FIXME: SET_IEEE80211_PERM_ADDR() requires default_mac_addr
>> + to be non-const for some strange reason */
>> + static u8 default_mac_addr[ETH_ALEN] = {
>> + 0x00, 0x02, 0xee, 0xc0, 0xff, 0xee
>> + };
>
> Probably just a missing const in the inline declaration :)
I'll send a patch.
>> + hw->flags = IEEE80211_HW_RX_INCLUDES_FCS |
>> + IEEE80211_HW_SIGNAL_DBM |
>> + IEEE80211_HW_NOISE_DBM;
>> + /* four bytes for padding */
>> + hw->extra_tx_headroom = sizeof(struct s_lm_data_out) + 4;
>> +
>> + /* unit us */
>> + hw->channel_change_time = 1000;
>> +
>> + hw->wiphy->bands[IEEE80211_BAND_2GHZ] = &stlc45xx_band_2ghz;
>> +
>> + SET_IEEE80211_DEV(hw, &spi->dev);
>
> I don't see you setting interface_types anywhere, that isn't a good
> thing. mac80211 will automatically create a sta mode interface but
> that's more a bug than a feature, and you won't be able to recreate it.
I'll fix it.
>> +#define MAC2STR(a) ((a)[0], (a)[1], (a)[2], (a)[3], (a)[4], (a)[5])
>> +#define MACSTR "%02x:%02x:%02x:%02x:%02x:%02x"
>
> Those should be %pM now, I think? Not sure that's in wireless-testing
> yet though, but that tree still has DECLARE_MAC_BUF etc.
%pM specifier is really cool and I'll add support for that as soon it
hits wireless-testing. But I didn't find it from wireless-testing, I
guess it will come after the next merge window.
Thank you for reviewing this again.
--
Kalle Valo
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
