> -----Original Message----- > From: Doug Anderson <dianders@xxxxxxxxxxxx> > Sent: Tuesday, December 1, 2020 12:49 AM > To: Rakesh Pillai <pillair@xxxxxxxxxxxxxx> > Cc: Abhishek Kumar <kuabhs@xxxxxxxxxxxx>; Kalle Valo > <kvalo@xxxxxxxxxxxxxx>; LKML <linux-kernel@xxxxxxxxxxxxxxx>; ath10k > <ath10k@xxxxxxxxxxxxxxxxxxx>; Brian Norris <briannorris@xxxxxxxxxxxx>; > linux-wireless <linux-wireless@xxxxxxxxxxxxxxx>; David S. Miller > <davem@xxxxxxxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>; netdev > <netdev@xxxxxxxxxxxxxxx> > Subject: Re: [PATCH v2 1/1] ath10k: add option for chip-id based BDF > selection > > Hi, > > On Tue, Nov 24, 2020 at 7:44 PM Rakesh Pillai <pillair@xxxxxxxxxxxxxx> > wrote: > > > > > > I missed on reviewing this change. Also I agree with Doug that this is not > > > the change I was looking for. > > > > > > > > The argument "with_variant" can be renamed to "with_extra_params". > > > There is no need for any new argument to this function. > > > > Case 1: with_extra_params=0, ar->id.bdf_ext[0] = 0 -> The > default > > > name will be used (bus=snoc,qmi_board_id=0xab) > > > > Case 2: with_extra_params=1, ar->id.bdf_ext[0] = 0 -> > > > bus=snoc,qmi_board_id=0xab,qmi_chip_id=0xcd > > > > Case 3: with_extra_params=1, ar->id.bdf_ext[0] = "xyz" -> > > > bus=snoc,qmi_board_id=0xab,qmi_chip_id=0xcd,variant=xyz > > > > > > > > ar->id.bdf_ext[0] depends on the DT entry for variant field. > > > > > > I'm confused about your suggestion. Maybe you can help clarify. Are > > > you suggesting: > > > > > > a) Only two calls to ath10k_core_create_board_name() > > > > > > I'm pretty sure this will fail in some cases. Specifically consider > > > the case where the device tree has a "variant" defined but the BRD > > > file only has one entry for (board-id) and one for (board-id + > > > chip-id) but no entry for (board-id + chip-id + variant). If you are > > > only making two calls then I don't think you'll pick the right one. > > > > > > Said another way... > > > > > > If the device tree has a variant: > > > 1. We should prefer a BRD entry that has board-id + chip-id + variant > > > 2. If #1 isn't there, we should prefer a BRD entry that has board-id + chip- > id > > > 3. If #1 and #2 aren't there we fall back to a BRD entry that has board-id. > > > > > > ...without 3 calls to ath10k_core_create_board_name() we can't handle > > > all 3 cases. > > > > This can be handled by two calls to ath10k_core_create_board_name > > 1) ath10k_core_create_board_name(ar, boardname, sizeof(boardname), > true) : As per my suggestions, this can result in two possible board names > > a) If DT have the "variant" node, it outputs the #1 from your suggestion > (1. We should prefer a BRD entry that has board-id + chip-id + variant) > > b) If DT does not have the "variant" node, it outputs the #2 from your > suggestion (2. If #1 isn't there, we should prefer a BRD entry that has board- > id + chip-id) > > > > 2) ath10k_core_create_board_name(ar, boardname, sizeof(boardname), > false) : This is the second call to this function and outputs the #3 from your > suggestion (3. If #1 and #2 aren't there we fall back to a BRD entry that has > board-id) > > What I'm trying to say is this. Imagine that: > > a) the device tree has the "variant" property. > > b) the BRD file has two entries, one for "board-id" (1) and one for > "board-id + chip-id" (2). It doesn't have one for "board-id + chip-id > + variant" (3). > > With your suggestion we'll see the "variant" property in the device > tree. That means we'll search for (1) and (3). (3) isn't there, so > we'll pick (1). ...but we really should have picked (2), right? Do we expect board-2.bin to not be populated with the bdf with variant field (if its necessary ?) Seems fine for me, if we have 2 fallback names if that is needed. > > -Doug
