-----Original Message-----
From: Doug Anderson <dianders@chromium.org>
Sent: Tuesday, December 1, 2020 12:49 AM
To: Rakesh Pillai <redacted>
Cc: Abhishek Kumar <redacted>; Kalle Valo
[off-list ref]; LKML [off-list ref]; ath10k
[off-list ref]; Brian Norris [off-list ref];
linux-wireless [off-list ref]; David S. Miller
[off-list ref]; Jakub Kicinski [off-list ref]; netdev
[off-list ref]
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 [off-list ref]
wrote:

quoted

quoted

quoted

I missed on reviewing this change. Also I agree with Doug that this is not

the change I was looking for.

quoted

The argument "with_variant" can be renamed to "with_extra_params".

There is no need for any new argument to this function.

quoted

Case 1: with_extra_params=0,  ar->id.bdf_ext[0] = 0             ->   The

default

quoted

quoted

name will be used (bus=snoc,qmi_board_id=0xab)

quoted

Case 2: with_extra_params=1,  ar->id.bdf_ext[0] = 0             ->

bus=snoc,qmi_board_id=0xab,qmi_chip_id=0xcd

quoted

Case 3: with_extra_params=1,  ar->id.bdf_ext[0] = "xyz"      ->

bus=snoc,qmi_board_id=0xab,qmi_chip_id=0xcd,variant=xyz

quoted

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

quoted

quoted

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

quoted

    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)

quoted

    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)

quoted

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