Re: [RFC 2/2] ath10k: add spectral scan feature

[Sven Eckelmann <sven@narfation.org>]

On Wednesday 16 July 2014 11:30:12 Michal Kazior wrote:
> > +static void ath_debug_send_fft_sample(struct ath10k *ar,
> > +                                     struct fft_sample_tlv
> > *fft_sample_tlv) +{
> 
> Why ath_debug_... instead of ath10k_debug_.. ?
> 
> 
> [...]

Will be modified.

> 
> > +static int ath_get_spectral_vdevid(struct ath10k *ar)
> 
> ath_get_..? Shouldn't this be ath10k_get?
> 
> > +{
> > +       struct ath10k_vif *arvif;
> > +
> > +       if (list_empty(&ar->arvifs))
> > +               return -ENODEV;
> > +
> > +       arvif = list_first_entry(&ar->arvifs, typeof(*arvif), list);
> > +       return arvif->vdev_id;
> > +}
> 
> No locks? Access to arvifs must be protected with conf_mutex.

Will be handled by the caller and lockdep_assert will be added to this 
function

> 
> What happens when an interface is removed without spectral scan being
> stopped?

This is a question for the QCA firmware guys.

> > +int ath10k_spectral_scan_config(struct ath10k *ar, enum spectral_mode
> > mode) +{
> > +       int count, ret;
> 
> We tend to use `res` in ath10k.

Thanks, will be changed. But actually, I found a lot of ret stuff in the 
ath10k code.

[...]
> + ath10k_warn("failed to configure spectral scan: %d\n", ret);
[...]
> "failed to parse phyerr tlv header at byte %d\n", i
[...]
> "failed to parse phyerr tlv payload at byte %d\n", i
[...]
> "failed to parse fft report at byte %d\n", i
[...]
> + ath10k_warn("failed to process fft report: %d\n", res)

Ok, warning messages will be modified.

> > +int ath10k_vdev_spectral_configure(struct ath10k *ar,
> > +                             const struct wmi_vdev_spectral_configure_arg
> > *arg)
> ath10k_wmi_vdev_spectral_configure
[...]
> > +int ath10k_vdev_spectral_enable(struct ath10k *ar, u32 vdev_id, u32
> > trigger, +                               u32 enable)
> 
> ath10k_wmi_vdev_spectral_enable

Ok, will be renamed.

> [...]
> > +struct wmi_vdev_spectral_configure_arg {
> > +       u32 vdev_id;
> > +       u32 scan_count;
> > +       u32 scan_period;
> > +       u32 scan_priority;
> > +       u32 scan_fft_size;
> > +       u32 scan_gc_ena;
> > +       u32 scan_restart_ena;
> > +       u32 scan_noise_floor_ref;
> > +       u32 scan_init_delay;
> > +       u32 scan_nb_tone_thr;
> > +       u32 scan_str_bin_thr;
> > +       u32 scan_wb_rpt_mode;
> > +       u32 scan_rssi_rpt_mode;
> > +       u32 scan_rssi_thr;
> > +       u32 scan_pwr_format;
> > +       u32 scan_rpt_mode;
> > +       u32 scan_bin_scale;
> > +       u32 scan_dBm_adj;
> > +       u32 scan_chn_mask;
> 
> Some stuff isn't self-explanatory. I'd love to see some of these
> fields documented to account for possible values (wb_rpt_mode?) and
> units (init_delay?).

Sorry, I got no documentation to anything. So I cannot provide one to you. 
Maybe you can get it easier than me.

> [...]
> 
> > +struct fft_sample_ath10k_ht20 {
> > +       struct fft_sample_tlv tlv;
> > +       u8 mhz;
> 
> A more suitable name would be "chan_width_mhz" I guess?

Ok,

> > +       __be16 freq1;
> > +       __be16 freq2;
> > +       __be16 noise;
> > +       __be16 max_magnitude;
> > +       __be16 total_gain_db;
> > +       __be16 base_pwr_db;
> > +       __be64 tsf;
> > +       s8 max_index;
> > +       u8 rssi;
> > +       u8 relpwr_db;
> > +       u8 avgpwr_db;
> > +
> > +       u8 data[SPECTRAL_ATH10K_HT20_NUM_BINS];
> 
> And as pointed out the size/count of bins is most likely not
> bandwidth-dependant. It's just a matter of resolution configured with
> fft_size + bin_scale. Maybe this should be a variable-sized array?

Ok, bin_scale didn't make a difference in my tests (I've just repeated them). 
But 2 ** (fft_size - 1) seems to be relevant. We have to change the format 
accordingly.

But currently we don't see any change in the bandwidth (20MHz/40MHz) for the 
different bin numbers. This has to be checked later in detail.

Kind regards,
	Sven