thanks Matthias for the new comments.
marcel :
do you agree with my comments which response to Matthias , if is OK, I
will make the final version for the next patch, thank you for the help.
regards.
tim
On 2021-10-20 23:30, Matthias Kaehlcke wrote:
> On Wed, Oct 20, 2021 at 12:00:52PM +0800, [email protected] wrote:
>> Thanks Matthias for the comments. please see my comments inline .
>>
>> BTW: marcel , do you agree with Matthias comments ? if fine , I will
>> align
>> Matthias comments and make the final version.
>>
>> regards.
>> tim
>> On 2021-10-20 03:29, Matthias Kaehlcke wrote:
>> > On Tue, Oct 12, 2021 at 03:55:56PM +0800, [email protected] wrote:
>> > > the RF performance of wcn6855 soc chip from different foundries will
>> > > be
>> > > difference, so we should use different nvm to configure them.
>> > >
>> > > Signed-off-by: Tim Jiang <[email protected]>
>> > > ---
>> > > drivers/bluetooth/btusb.c | 56
>> > > +++++++++++++++++++++++++++++++++++------------
>> > > 1 file changed, 42 insertions(+), 14 deletions(-)
>> > >
>> > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>> > > index 75c83768c257..f352ff351b61 100644
>> > > --- a/drivers/bluetooth/btusb.c
>> > > +++ b/drivers/bluetooth/btusb.c
>> > > @@ -3190,6 +3190,9 @@ static int btusb_set_bdaddr_wcn6855(struct
>> > > hci_dev
>> > > *hdev,
>> > > #define QCA_DFU_TIMEOUT 3000
>> > > #define QCA_FLAG_MULTI_NVM 0x80
>> > >
>> > > +#define WCN6855_2_0_RAM_VERSION_GF 0x400c1200
>> > > +#define WCN6855_2_1_RAM_VERSION_GF 0x400c1211
>> > > +
>> > > struct qca_version {
>> > > __le32 rom_version;
>> > > __le32 patch_version;
>> > > @@ -3221,6 +3224,7 @@ static const struct qca_device_info
>> > > qca_devices_table[] = {
>> > > { 0x00000302, 28, 4, 16 }, /* Rome 3.2 */
>> > > { 0x00130100, 40, 4, 16 }, /* WCN6855 1.0 */
>> > > { 0x00130200, 40, 4, 16 }, /* WCN6855 2.0 */
>> > > + { 0x00130201, 40, 4, 16 }, /* WCN6855 2.1 */
>> > > };
>> > >
>> > > static int btusb_qca_send_vendor_req(struct usb_device *udev, u8
>> > > request,
>> > > @@ -3375,6 +3379,43 @@ static int btusb_setup_qca_load_rampatch(struct
>> > > hci_dev *hdev,
>> > > return err;
>> > > }
>> > >
>> > > +static void btusb_generate_qca_nvm_name(char *fwname,
>> > > + size_t max_size,
>> > > + struct qca_version *ver)
>> >
>> > => const struct qca_version *ver
[Tim] fine ,will modify it in next version.
>> >
>> > > +{
>> > > + u32 rom_version = le32_to_cpu(ver->rom_version);
>> > > + u16 flag = le16_to_cpu(ver->flag);
>> > > +
>> > > + if (((flag >> 8) & 0xff) == QCA_FLAG_MULTI_NVM) {
>> > > + u16 board_id = le16_to_cpu(ver->board_id);
>> > > + u32 ram_version = le32_to_cpu(ver->ram_version);
>> > > + const char *variant;
>> > > +
>> > > + switch (ram_version) {
>> > > + case WCN6855_2_0_RAM_VERSION_GF:
>> > > + case WCN6855_2_1_RAM_VERSION_GF:
>> > > + variant = "_gf";
>> > > + break;
>> > > + default:
>> > > + variant = "";
>> >
>> > instead of the default branch you could assign a default to 'variant' at
>> > declaration time, but it's fine either way.
>>
>> [Tim] this code style is recommend by marcel.
>
> Both are ok, if Marcel prefers the default branch let's keep it that
> way.
>
>> >
>> > > + break;
>> > > + }
>> > > +
>> > > + /* if boardid equal 0, use default nvm without suffix */
>> >
>> > delete the comment, it just states the obvious
[Tim] fine, will modify it in next version.
>> >
>> > > + if (board_id == 0x0) {
>> >
>> > nit: is there really any value in using a hex number here instead of a
>> > plain decimal 0?
>>
>> [Tim] this line is inherit from last change , if you think I should
>> change
>> 0x0 to 0 , I am fine.
>
> Since this patch touches/moves this code it seems a good opportunity to
> clean
> things up a bit. It's also true that there are quite a few instances of
> this
> and comparisons with '0x00' in other parts of the kernel, so I guess
> it's
> also fine to leave it as is.
[Tim] I am OK to change from 0x0 to 0 in next version.