2023-11-24 19:48:33

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH v2,1/2] wifi: mac80211: Add utilities for converting op_class

Btw, I ended looking at this again...

On Mon, 2023-11-13 at 10:11 +0800, Michael-CY Lee wrote:
> +/**
> + * ieee80211_operating_class_to_center_freq - convert operating class to
> + * center frequency
> + *
> + * @operating_class: the operating class to convert
> + * @chan: the ieee80211_channel to convert
> + * @center_freq1: cneter frequency 1 pointer to fill
> + * @center_freq2: cneter frequency 2 pointer to fill

typos here ("center")

But maybe it'd be better to fill (or update, we could pass the channel
pointer in it) a chandef struct? Then it could also be more easily
extended to understand more opclasses in the future, perhaps S1G or DMG?

> + *
> + * Returns %true if the conversion was successful, %false otherwise.
> + */
> +bool ieee80211_operating_class_to_center_freq(u8 operating_class,
> + struct ieee80211_channel *chan,
> + u32 *center_freq1,
> + u32 *center_freq2);
> +
> +/**
> + * ieee80211_operating_class_to_chan_width - convert operating class to
> + * nl80211 channel width
> + *
> + * @operating_class: the operating class to convert
> + */
> +enum nl80211_chan_width
> +ieee80211_operating_class_to_chan_width(u8 operating_class);

And you'd actually get both in one function call? The chan ->
center_freq anyway implies you know the width, no? Is this really needed
separately?

> /**
> * ieee8 0211_chandef_to_operating_class - convert chandef to operation class

This also converts the other way around, btw.

> + case 135: /* 6 GHz band; 80+80 MHz; channels 1,5,..,229 */
> + /* TODO How to know the center_freq2 of 80+80 MHz?*/
> + *center_freq1 = 0;

Well, you don't, from this. I'm actually a bit surprised 80+80 exists in
6 GHz, I thought it was treated more or less as a dead end.

johannes


Subject: Re: [PATCH v2,1/2] wifi: mac80211: Add utilities for converting op_class

Hi Johannes,

Thanks for the suggestion. It's truely more reasonable to do the
conversion in one single function.

we'll modify it along with the typo in next version.

Best,
Michael

On Fri, 2023-11-24 at 20:25 +0100, Johannes Berg wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> Btw, I ended looking at this again...
>
> On Mon, 2023-11-13 at 10:11 +0800, Michael-CY Lee wrote:
> > +/**
> > + * ieee80211_operating_class_to_center_freq - convert operating
> class to
> > + * center frequency
> > + *
> > + * @operating_class: the operating class to convert
> > + * @chan: the ieee80211_channel to convert
> > + * @center_freq1: cneter frequency 1 pointer to fill
> > + * @center_freq2: cneter frequency 2 pointer to fill
>
> typos here ("center")
>
> But maybe it'd be better to fill (or update, we could pass the
> channel
> pointer in it) a chandef struct? Then it could also be more easily
> extended to understand more opclasses in the future, perhaps S1G or
> DMG?
>
> > + *
> > + * Returns %true if the conversion was successful, %false
> otherwise.
> > + */
> > +bool ieee80211_operating_class_to_center_freq(u8 operating_class,
> > + struct ieee80211_channel *chan,
> > + u32 *center_freq1,
> > + u32 *center_freq2);
> > +
> > +/**
> > + * ieee80211_operating_class_to_chan_width - convert operating
> class to
> > + * nl80211 channel width
> > + *
> > + * @operating_class: the operating class to convert
> > + */
> > +enum nl80211_chan_width
> > +ieee80211_operating_class_to_chan_width(u8 operating_class);
>
> And you'd actually get both in one function call? The chan ->
> center_freq anyway implies you know the width, no? Is this really
> needed
> separately?
>
> > /**
> > * ieee8 0211_chandef_to_operating_class - convert chandef to
> operation class
>
> This also converts the other way around, btw.
>
> > +case 135: /* 6 GHz band; 80+80 MHz; channels 1,5,..,229 */
> > +/* TODO How to know the center_freq2 of 80+80 MHz?*/
> > +*center_freq1 = 0;
>
> Well, you don't, from this. I'm actually a bit surprised 80+80 exists
> in
> 6 GHz, I thought it was treated more or less as a dead end.


>
> johannes