Received-SPF: softfail (google.com: domain of transitioning linux-wireless-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
References: <20220512195319.14635-1-wlooi@ucalgary.ca> <20220512195319.14635-3-wlooi@ucalgary.ca>
 <69feb893-d73a-46e5-fbe6-2f49e091c625@quicinc.com>
In-Reply-To: <69feb893-d73a-46e5-fbe6-2f49e091c625@quicinc.com>
From:   Wenli Looi <wlooi@ucalgary.ca>
Date:   Tue, 17 May 2022 23:08:31 -0700
Message-ID: <CAKe_nd3u+iiCGC7M_i+ovF+_bgDMkFUh0OpJ7HFWMhmgZQa7bw@mail.gmail.com>
Subject: Re: [PATCH v2 2/9] ath9k: basic support for QCN550x
To:     Jeff Johnson <quic_jjohnson@quicinc.com>
Cc:     =?UTF-8?B?VG9rZSBIw7hpbGFuZC1Kw7hyZ2Vuc2Vu?= <toke@toke.dk>,
        Kalle Valo <kvalo@kernel.org>, linux-wireless@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?dFVObnNHYXBPZ2E1VXc2SCtPTThTYktWRG9HZFAvcGdPdHU2RnEzSHR6emI1?=
 =?utf-8?B?ZWp0cEJqTlpQOFVRRXRWTWlHalR1cG1ORmlMcEloalQ3blFZZGJKQVlNSy9n?=
 =?utf-8?B?MkcyVkdIZk5rMHJVVzJ2M0xwTnE3ZUNzNWdJV2llYkhYbmN4TjY1KzFuSXZ5?=
 =?utf-8?B?TjBTSDVqcjZ6WU1zbjQzU3pwMEVpVVlGRE1wc216YUgva0lKWXlXRW9VRFNq?=
 =?utf-8?B?cEZWWHVPeWwyYXlNY0crL2JhcFFoWXRpbHlsaDJmYitpaThMNHpLNzNvSDVk?=
 =?utf-8?B?b050STNGaFJJMDE1VE5ybXBVeVZ5cXQvNDVETUhrSjFBUlV5RWRCTXByQmlF?=
 =?utf-8?B?UDN0MnZNUllpMHJNNzU1bkNTQkVGMVE5bkNxUGF0c1pDSkhGSzVHZ2trTjlO?=
 =?utf-8?B?dDJmakE2TTFObnpMV0JBejlIMjVNOXYyMHhzYnpqZ0htUHBSZDBqaTMyT0Vn?=
 =?utf-8?B?UjlWak0yN0hMZDhGR1FhSnhqWVFxSkgwcDBXdWhHTTEwSWI0NjlMR3gxWWNR?=
 =?utf-8?B?aE5jQys1WTZaOHhLRzNhUWptMXh6MXRBREYrNWpabmpwRFgyKzVqUWgwWG9T?=
 =?utf-8?B?UFkzTG1NYlBERGdBN005UXFhMHB5djA3YS9Hb0FUcDFWc0ZYUy9QellWUUtP?=
 =?utf-8?B?SU1KQmlBYVA4WjBDWUhITmRZY0VNMi9zQm9qajZBNlVpMmtxY1RnWmQ4L1Zr?=
 =?utf-8?B?d0RRUElFNXVKYVorNkpHV1pKbXBzaUlIY1dJZTR2Y0VlVjIxVnRYdTNHdXE4?=
 =?utf-8?B?dGNQT2UvRFVqZ284K05tMGJ0RTlzcnFXRU53VU1qQTU5Uk10OUlpN3ZpK1g2?=
 =?utf-8?B?bWo5c0hxU3NyUWVYWTFGMThLVWRuVkt5NDZwYytNMVVkMHA0TExyWnVLNEpo?=
 =?utf-8?B?bmpTRWs2SlJ2WSs1eXVvM3Z1Sk84SXQ5Tm43Z0NveDdReTVvMGVza295M2Iv?=
 =?utf-8?B?R3FBWDlQNUJhOHNvZDRheVdOdm9wS0tFdGJvbzk3ZWRvVC9SL0J4YklDVGdv?=
 =?utf-8?B?TVZPZWovbVJtaFRQSGRDRkhSb1REbGtwNmZ0QkFpVTF0K0RjYnBLaEhWUXhO?=
 =?utf-8?B?T3V5d0JYRk53VWVwY2w2V1oxNjRvUWZnc1lwQk1LZGx6d1NyOHFnZmFiWDR0?=
 =?utf-8?B?dVQrT0hGcTl6TVVJRWNubVdaUkxLaFdnb3gyTCtUM1pMb1kxemFMbUM3b3pn?=
 =?utf-8?B?aFVkSkkxZldMMlB0bWJ6eWZTQmJiMkV3dE1Dc20wTE1HRjg1S25YQ0dUTUpW?=
 =?utf-8?B?U3c3dHUwZ21sUVRTYVN3TDBQc2VZcTg3aGI4TThPTTIvL1RFKzIyZDZyUDhS?=
 =?utf-8?B?M2NYd095dndFU1V5QmgwSk1zZ1pSWUJkZ2hEL2wrc3JOeFA1b2I2dmZHbmw3?=
 =?utf-8?B?WXoxUEtoaTduSUhyeDF6aHRRbDVWL2RGYmdIaGM3WCtZZU94Qk9vb1g5RnpN?=
 =?utf-8?B?NmlMK05tM3pabXpYQkhEY1VlYWR2ZXRWTlBaTnR0NWQ1ZTczQVcreDJLN0sv?=
 =?utf-8?B?VmI5MFlCWlVyaGppRjdZd2ZlaXFCK0VkUjBUN0svMlZOK25JcThvb2hmbm5r?=
 =?utf-8?B?RXZ1SDU2TGE4TW0vbjJuaEJjK1BJWklkZnlpaHN6cE9yTHlrb1FIRElWTXpx?=
 =?utf-8?B?Q1lBQ0g2STg1OEFoS0NDK0NFUms3d2piV3VFeDNUUCtxOWRFYzJVSFhQUG1n?=
 =?utf-8?B?bVNSWEVjN1Q0TUtyZm95VWNoa09SQXF2Z3dlSGdQWFdNa0hwM1lVQzFXTXVN?=
 =?utf-8?B?ZStob0xxS0IxNHFsL2ZrTFZwTExZL255d0g5S3JuQ1puUmFHMG1mSE1SM09M?=
 =?utf-8?B?WmRHSmpRcGRYM3p6cDBnbURVRURMWVRzRUtHMlAwN2JyYnhYKzVNUm9FSDIv?=
 =?utf-8?B?ZjduLzhWMjdyNFY1S1lsVVR2SXZTcFh0N0NIOHV4eG9WYnB4aEhXYVppZk8y?=
 =?utf-8?B?T2lhYTJ1R2hNUkUyNlQrcHV0dGJvUUdTbHFjeE82WXE4RzNxck1DVGJEazM3?=
 =?utf-8?B?bU56K3ZwKzAyd0YwZkYwTSt3QnR4WnVkZHNhQkdoaENKalNLUjJSZGRUeXdx?=
 =?utf-8?B?WjFtUEpjTXZWM2RHZ3A1c1M3Qko2bFJkam1vU2dHdzBGYWh2RDlBZFM4OHRU?=
 =?utf-8?B?YXAvVGxIaUZpbnRIRW5ob3dSM1NZb0FObWlBenlsTHJFQXFBVEVvYXhzQmox?=
 =?utf-8?B?azIyOXBoNHA4bmZUTDV1K3lEQ2JoQ2RleWdXSHBrTmdCSGQyK3lLUERNSFpo?=
 =?utf-8?B?RC94V3FXVWkzZXkyQUhqY05adW5WRHBxcXVNVnBFbVpUM2VXcVk1UkU1UTh4?=
 =?utf-8?B?WVlUamM2RmJuYmwzc2RLN2ltckMrWTVRZ3orRVJtaWw0TGhvWUE2QT09?=
X-OriginatorOrg: ucalgary.ca
X-MS-Exchange-CrossTenant-Network-Message-Id: 42e5ac18-1f32-415c-ca25-08da3894dce7
X-MS-Exchange-CrossTenant-AuthSource: YQXPR01MB3302.CANPRD01.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 May 2022 06:08:44.1495
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: c609a0ec-a5e3-4631-9686-192280bd9151
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: vduTGwqQEJEIzSO29ZXg/UqCuedzZVW4nidsdypxL3ihLP1d3pmQ/hYPw5mJgYxRHT0LtVdVBJGbWkohei0Lcw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: YT2PR01MB5888
X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,
	HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RDNS_NONE,
	SPF_HELO_NONE,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-wireless.vger.kernel.org>
X-Mailing-List: linux-wireless@vger.kernel.org

On Thu, May 12, 2022 at 2:45 PM Jeff Johnson <quic_jjohnson@quicinc.com> wrote:
>
> On 5/12/2022 12:53 PM, Wenli Looi wrote:
> > QCN550x is very similar to QCA956x. Note that AR_CH0_XTAL is
> > intentionally unchanged. Certain arrays are no longer static because
> > they are no longer constant.
>
> I don't understand the last sentence. You removed the 'static' keyword
> but left the 'const' keyword, hence they are still constant.
>
> And I didn't actually see any instances where the arrays are being modified.
>
> Can you highlight which are being modified?

I changed several arrays from "static const" to "const" because their
values now depend on ah, so it won't compile if they are static. For
example, offset_array contains AR_PHY_RX_IQCAL_CORR_B0 which depends
on AR_CHAN_BASE (which now depends on ah but did not before).

> > -#define AR_CHAN_BASE 0x9800
> > +#define AR_CHAN_BASE (AR_SREV_5502(ah) ? 0x29800 : 0x9800)
>
> this violates the coding style:
> <https://www.kernel.org/doc/html/latest/process/coding-style.html#macros-enums-and-rtl>
>
> Things to avoid when using macros:
> macros that depend on having a local variable with a magic name
>
> So you should add ah as a parameter to the macro
>
> Repeat for all instances below where ah is being used
>

The macros like AR_CHAN_BASE and AR_SM_BASE are referenced by a lot of
other macros, some of which already have a "magic name" dependency on
ah like these ones:

https://github.com/torvalds/linux/blob/210e04ff768142b96452030c4c2627512b30ad95/drivers/net/wireless/ath/ath9k/ar9003_phy.h#L527

However I think it's probably good to avoid introducing new macros
with the magic name dependency. If desired, for every macro that I
have modified, I could try to add the ah parameter to all dependent
macros. This would probably be a rather large change so it might make
sense to be a separate commit.