Re: [PATCH v2] mt76: mt7615: add support for MT7611N

[Alexander Schlichte <schlichte.alexander@xxxxxxxxx>]

Just some random drive-by nitpicks, so take it with a grain of salt.

On 24.05.20 05:41, DENG Qingfang wrote:
> MT7611N is basically the same as MT7615N, except it only supports 5GHz
> It is used by some TP-Link and Mercury wireless routers
>
> Signed-off-by: DENG Qingfang <dqfext@xxxxxxxxx>
> ---
> v1 -> v2:
> make is_mt7615(&dev->mt76) return true for mt7611
>
>   drivers/net/wireless/mediatek/mt76/mt7615/eeprom.c | 7 +++++++
>   drivers/net/wireless/mediatek/mt76/mt7615/mt7615.h | 7 ++++++-
>   drivers/net/wireless/mediatek/mt76/mt7615/pci.c    | 1 +
>   3 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/eeprom.c b/drivers/net/wireless/mediatek/mt76/mt7615/eeprom.c
> index 6a5ae047c63b..edac37e7847b 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt7615/eeprom.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt7615/eeprom.c
> @@ -111,6 +111,12 @@ mt7615_eeprom_parse_hw_band_cap(struct mt7615_dev *dev)
>   		return;
>   	}
>   
> +	if (is_mt7611(&dev->mt76)) {
> +		/* 5GHz only */
> +		dev->mt76.cap.has_5ghz = true;
> +		return;
> +	}
> +
>   	val = FIELD_GET(MT_EE_NIC_WIFI_CONF_BAND_SEL,
>   			eeprom[MT_EE_WIFI_CONF]);
>   	switch (val) {
> @@ -310,6 +316,7 @@ static void mt7615_cal_free_data(struct mt7615_dev *dev)
>   		mt7622_apply_cal_free_data(dev);
>   		break;
>   	case 0x7615:
> +	case 0x7611:
>   		mt7615_apply_cal_free_data(dev);
>   		break;
>   	}

Maybe sort this alphabetically?
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/mt7615.h b/drivers/net/wireless/mediatek/mt76/mt7615/mt7615.h
> index ebdfca64b079..4ce8b379a147 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt7615/mt7615.h
> +++ b/drivers/net/wireless/mediatek/mt76/mt7615/mt7615.h
> @@ -411,7 +411,7 @@ static inline bool is_mt7622(struct mt76_dev *dev)
>   
>   static inline bool is_mt7615(struct mt76_dev *dev)
>   {
> -	return mt76_chip(dev) == 0x7615;
> +	return mt76_chip(dev) == 0x7615 || mt76_chip(dev) == 0x7611;
>   }

Now the function name is a bit confusing because you can't use it to
differentiate between 7611 and 7615. Would it be an option to give it a more
fitting name? Maybe `is_mt761x` or `is_mt7611_or_7615`?

Apart from that you might want to call `is_mt7611` here. That would avoid
duplicating the check and order these is_* functions alphabetically.
>   
>   static inline bool is_mt7663(struct mt76_dev *dev)
> @@ -419,6 +419,11 @@ static inline bool is_mt7663(struct mt76_dev *dev)
>   	return mt76_chip(dev) == 0x7663;
>   }
>   
> +static inline bool is_mt7611(struct mt76_dev *dev)
> +{
> +	return mt76_chip(dev) == 0x7611;
> +}
> +
>   static inline void mt7615_irq_enable(struct mt7615_dev *dev, u32 mask)
>   {
>   	mt76_set_irq_mask(&dev->mt76, 0, 0, mask);
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/pci.c b/drivers/net/wireless/mediatek/mt76/mt7615/pci.c
> index 88ff14564521..b09d08d0dac9 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt7615/pci.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt7615/pci.c
> @@ -14,6 +14,7 @@
>   static const struct pci_device_id mt7615_pci_device_table[] = {
>   	{ PCI_DEVICE(0x14c3, 0x7615) },
>   	{ PCI_DEVICE(0x14c3, 0x7663) },
> +	{ PCI_DEVICE(0x14c3, 0x7611) },
>   	{ },
>   };

Maybe keep this sorted alphabetically?