On 2018-10-03 15:32, Stanislaw Gruszka wrote:
> On Wed, Oct 03, 2018 at 01:49:23PM +0200, Felix Fietkau wrote:
>>
>>
>> > On 3. Oct 2018, at 13:12, Stanislaw Gruszka <sgruszka@xxxxxxxxxx> wrote:
>> >
>> > On Wed, Oct 03, 2018 at 01:01:40PM +0200, Lorenzo Bianconi wrote:
>> >>>
>> >>> On Tue, Oct 02, 2018 at 12:19:04AM +0200, Lorenzo Bianconi wrote:
>> >>>> Move mt76x2_dev in mt76x02_util.h and rename it in mt76x02_dev
>> >>>> in order to be shared between mt76x2 and mt76x0 driver
>> >>> <snip>
>> >>>> +struct mt76x02_dev {
>> >>>> + struct mt76_dev mt76; /* must be first */
>> >>>> +
>> >>>> + struct mac_address macaddr_list[8];
>> >>>> +
>> >>>> + struct mutex mutex;
>> >>>> +
>> >>>> + u8 txdone_seq;
>> >>>> + DECLARE_KFIFO_PTR(txstatus_fifo, struct mt76x02_tx_status);
>> >>>> +
>> >>>> + struct sk_buff *rx_head;
>> >>>> +
>> >>>> + struct tasklet_struct tx_tasklet;
>> >>>> + struct tasklet_struct pre_tbtt_tasklet;
>> >>>> + struct delayed_work cal_work;
>> >>>> + struct delayed_work mac_work;
>> >>>> +
>> >>>> + u32 aggr_stats[32];
>> >>>> +
>> >>>> + struct sk_buff *beacons[8];
>> >>>> + u8 beacon_mask;
>> >>>> + u8 beacon_data_mask;
>> >>>> +
>> >>>> + u8 tbtt_count;
>> >>>> + u16 beacon_int;
>> >>>> +
>> >>>> + struct mt76x02_calibration cal;
>> >>>> +
>> >>>> + s8 target_power;
>> >>>> + s8 target_power_delta[2];
>> >>>> + bool enable_tpc;
>> >>>> +
>> >>>> + u8 coverage_class;
>> >>>> + u8 slottime;
>> >>>> +
>> >>>> + struct mt76x02_dfs_pattern_detector dfs_pd;
>> >>>> +};
>> >>>> +
>> >>> <snip>
>> >>>> static bool
>> >>>> -mt76x2_has_cal_free_data(struct mt76x2_dev *dev, u8 *efuse)
>> >>>> +mt76x2_has_cal_free_data(struct mt76x02_dev *dev, u8 *efuse)
>> >>>
>> >>> I don't think this is right approach. I would rather prefer to have
>> >>> common data structures embeded in mt76x2_dev and mt76x0_dev
>> >>> structures to have chip sepcific fields/data separated.
>> >>>
>> >>
>> >> The reason of this patch is that mt76x0_dev fields are already in mt76x2_dev
>> >> so I guess there is no need to have different structures. Moreover in
>> >> this way we can
>> >> remove a lot of duplicated code between mt76x0 and mt76x2 drivers.
>> >
>> > But you can still create additional structures i.e.
>> >
>> > mt76x02_power {
>> > s8 target_power;
>> > s8 target_power_delta[2];
>> > bool enable_tpc;
>> > }
>> >
>> > mt76x02_conf {
>> > u8 coverage_class;
>> > u8 slottime;
>> > }
>> >
>> > put them into mt76xN_dev and still remove dupicated code ?
>> Quite often, mt76_dev would be needed as well for register access, which means extra parameters for a lot of functions.
>> I think Lorenzo’s approach makes the code a lot more concise, and makes it easier to share more code between mt76x0 and mt76x2.
>
> I think this could be solved very easly by container_of() macro if
> there will be one mt76x02_dev struct just after mt76_dev.
That's possible, yes. But given how much code can still be unified
between mt76x0 and mt76x2, I don't think there will be much need for a
x0 or x2 specific device struct. And in that case, the code will be more
readable if we avoid putting a lot of unnecessary &dev->mt76x02 or
container_of in the code.
- Felix
