> -----Original Message-----
> From: Doug Anderson <dianders@xxxxxxxxxxxx>
> Sent: Tuesday, November 24, 2020 6:27 AM
> To: Abhishek Kumar <kuabhs@xxxxxxxxxxxx>
> Cc: Kalle Valo <kvalo@xxxxxxxxxxxxxx>; Rakesh Pillai
> <pillair@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 Thu, Nov 12, 2020 at 12:09 PM Abhishek Kumar <kuabhs@xxxxxxxxxxxx>
> wrote:
> >
> > In some devices difference in chip-id should be enough to pick
> > the right BDF. Add another support for chip-id based BDF selection.
> > With this new option, ath10k supports 2 fallback options.
> >
> > The board name with chip-id as option looks as follows
> > board name 'bus=snoc,qmi-board-id=ff,qmi-chip-id=320'
> >
> > Tested-on: WCN3990 hw1.0 SNOC WLAN.HL.3.2.2-00696-QCAHLSWMTPL-1
> > Tested-on: QCA6174 HW3.2 WLAN.RM.4.4.1-00157-QCARMSWPZ-1
> > Signed-off-by: Abhishek Kumar <kuabhs@xxxxxxxxxxxx>
> > ---
> >
> > (no changes since v1)
>
> I think you need to work on the method you're using to generate your
> patches. There are most definitely changes since v1. You described
> them in your cover letter (which you don't really need for a singleton
> patch) instead of here.
>
>
> > @@ -1438,12 +1439,17 @@ static int
> ath10k_core_create_board_name(struct ath10k *ar, char *name,
> > }
> >
> > if (ar->id.qmi_ids_valid) {
> > - if (with_variant && ar->id.bdf_ext[0] != '\0')
> > + if (with_additional_params && ar->id.bdf_ext[0] != '\0')
> > scnprintf(name, name_len,
> > "bus=%s,qmi-board-id=%x,qmi-chip-id=%x%s",
> > ath10k_bus_str(ar->hif.bus),
> > ar->id.qmi_board_id, ar->id.qmi_chip_id,
> > variant);
> > + else if (with_additional_params)
> > + scnprintf(name, name_len,
> > + "bus=%s,qmi-board-id=%x,qmi-chip-id=%x",
> > + ath10k_bus_str(ar->hif.bus),
> > + ar->id.qmi_board_id, ar->id.qmi_chip_id);
>
> I believe this is exactly opposite of what Rakesh was requesting.
> Specifically, he was trying to eliminate the extra scnprintf() but I
> think he still agreed that it was a good idea to generate 3 different
> strings. I believe the proper diff to apply to v1 is:
>
> https://crrev.com/c/255643
>
> -Doug
Hi Abhishek/Doug,
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.
Thanks,
Rakesh Pillai.
