Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: pillair=codeaurora.org@mg.codeaurora.org
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 4943AC433ED
From:   "Rakesh Pillai" <pillair@codeaurora.org>
To:     "'Doug Anderson'" <dianders@chromium.org>,
        "'Abhishek Kumar'" <kuabhs@chromium.org>
Cc:     "'Kalle Valo'" <kvalo@codeaurora.org>,
        "'LKML'" <linux-kernel@vger.kernel.org>,
        "'ath10k'" <ath10k@lists.infradead.org>,
        "'Brian Norris'" <briannorris@chromium.org>,
        "'linux-wireless'" <linux-wireless@vger.kernel.org>,
        "'David S. Miller'" <davem@davemloft.net>,
        "'Jakub Kicinski'" <kuba@kernel.org>,
        "'netdev'" <netdev@vger.kernel.org>
References: <20201112200906.991086-1-kuabhs@chromium.org> <20201112200856.v2.1.Ia526132a366886e3b5cf72433d0d58bb7bb1be0f@changeid> <CAD=FV=XKCLgL6Bt+3KfqKByyP5fpwXOh6TNHXAoXkaQJRzjKjQ@mail.gmail.com>
In-Reply-To: <CAD=FV=XKCLgL6Bt+3KfqKByyP5fpwXOh6TNHXAoXkaQJRzjKjQ@mail.gmail.com>
Subject: RE: [PATCH v2 1/1] ath10k: add option for chip-id based BDF selection
Date:   Tue, 24 Nov 2020 14:48:52 +0530
Message-ID: <002401d6c242$d78f2140$86ad63c0$@codeaurora.org>
MIME-Version: 1.0
Content-Type: text/plain;
        charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Thread-Index: AQKxMSkDsdRsNwK99YDuloz1ISNQFAKbjoqbAYQwjuWoASIGoA==
Content-Language: en-us
Precedence: bulk


> -----Original Message-----
> From: Doug Anderson <dianders@chromium.org>
> Sent: Tuesday, November 24, 2020 6:27 AM
> To: Abhishek Kumar <kuabhs@chromium.org>
> Cc: Kalle Valo <kvalo@codeaurora.org>; Rakesh Pillai
> <pillair@codeaurora.org>; LKML <linux-kernel@vger.kernel.org>; ath10k
> <ath10k@lists.infradead.org>; Brian Norris <briannorris@chromium.org>;
> linux-wireless <linux-wireless@vger.kernel.org>; David S. Miller
> <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; netdev
> <netdev@vger.kernel.org>
> Subject: Re: [PATCH v2 1/1] ath10k: add option for chip-id based BDF
> selection
>=20
> Hi,
>=20
> On Thu, Nov 12, 2020 at 12:09 PM Abhishek Kumar <kuabhs@chromium.org>
> 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=3Dsnoc,qmi-board-id=3Dff,qmi-chip-id=3D320'
> >
> > 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@chromium.org>
> > ---
> >
> > (no changes since v1)
>=20
> 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.
>=20
>=20
> > @@ -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] !=3D '\0')
> > +               if (with_additional_params && ar->id.bdf_ext[0] !=3D =
'\0')
> >                         scnprintf(name, name_len,
> >                                   =
"bus=3D%s,qmi-board-id=3D%x,qmi-chip-id=3D%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=3D%s,qmi-board-id=3D%x,qmi-chip-id=3D%x",
> > +                                 ath10k_bus_str(ar->hif.bus),
> > +                                 ar->id.qmi_board_id, =
ar->id.qmi_chip_id);
>=20
> 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:
>=20
> https://crrev.com/c/255643
>=20
> -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=3D0,  ar->id.bdf_ext[0] =3D 0             ->   =
The default name will be used (bus=3Dsnoc,qmi_board_id=3D0xab)
Case 2: with_extra_params=3D1,  ar->id.bdf_ext[0] =3D 0             ->   =
bus=3Dsnoc,qmi_board_id=3D0xab,qmi_chip_id=3D0xcd
Case 3: with_extra_params=3D1,  ar->id.bdf_ext[0] =3D "xyz"      ->   =
bus=3Dsnoc,qmi_board_id=3D0xab,qmi_chip_id=3D0xcd,variant=3Dxyz

ar->id.bdf_ext[0] depends on the DT entry for variant field.

Thanks,
Rakesh Pillai.