Re: [PATCH v5 2/3] Bluetooth: btqca: Add wcn3990 firmware download support.

[Balakrishna Godavarthi <bgodavar@codeaurora.org>]

Hi Matthias,

as i was on vacation i couldn't able to reply to your comments.
find my comments inline.

On 2018-05-11 23:10, Matthias Kaehlcke wrote:
> Hi Bala,
> 
> On Fri, May 11, 2018 at 05:51:02PM +0530, Balakrishna Godavarthi wrote:
>> This patch enables the RAM and NV patch download for wcn3990.
>> 
>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> ---
>>  drivers/bluetooth/btqca.c | 46 
>> +++++++++++++++++++++++++++++++++++++++++++++-
>>  drivers/bluetooth/btqca.h | 13 +++++++++++++
>>  2 files changed, 58 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
>> index 8219816..40c6b4f 100644
>> --- a/drivers/bluetooth/btqca.c
>> +++ b/drivers/bluetooth/btqca.c
>> @@ -27,7 +27,7 @@
>> 
>>  #define VERSION "0.1"
>> 
>> -static int rome_patch_ver_req(struct hci_dev *hdev, u32 
>> *rome_version)
>> +int rome_patch_ver_req(struct hci_dev *hdev, u32 *rome_version)
> 
> If this and other functions aren't really Rome specific they should
> probably be renamed to qca_...
> 
[Bala]:- will update the functions name in separate patch.

>> +int qca_uart_setup_cherokee(struct hci_dev *hdev, uint8_t baudrate,
>> +			    u32 *soc_ver)
>> +{
>> +	struct rome_config config;
>> +	int err;
>> +
>> +	/* we are using the existing funciton of ROME,
>> +	 * instead of duplicating the function for wcn3990.
>> +	 */
> 
> The comment isn't quite accurate, the qca_uart_setup_cherokee() calls
> existing functions, but is essentially a copy of qca_uart_setup_rome().
> 
> The main difference is that the patch version is passed as a parameter
> instead of determining it in the uart setup function. There seems to
> be no need to pass the version number in, or if you prefer to do it
> this way, you could change the Rome code to do this.
> 
> The other delta is the filename extension of the rampatch file, which
> is .bin for Rome and .tlv for Cherokee. Is there a good reason to use
> a different extension? If not just stick to the existing naming
> scheme, otherwise you could pass the chip type as a parameter and
> chose the extensions based on that.
> 
[Bala]:- as we see the process to setup of BTchip ROME and wcn3990 
setup.
          For Rome:
          1. change the operating speed to 3.0mbps.
          2. request the version
          3. download RAM patch (.bin extestion)
          4. downlaod NV patch (.bin extersnion)
          5. Reset ROME

         steps 2 to 5 are done in the function qca_uart_setup_rome()

         For WCN3990:
         1. Set baudrate to 115200.
         2. request the version
         3. set baudrate to operating speed.
         4. download RAM patch (.tlv extestion)
         5. downlaod NV patch (.bin extersnion)
         6. Reset wcn3990

        steps 2 is done in function rome_patch_ver_req(), where as steps 
4 to 6 qca_uart_setup_cherokee()

        the big difference is .bin extension for RAM patch is deprecated 
and BTCCHIP vendor will not support .bin for ram patch any more for 
latest chips.
        so that could be reason we using a separate function 
qca_uart_setup_cherokee(). if i use the same function by passing the 
chip name to decide the file extension.
        but again unnecessary i have to request the version command to 
BTCHIP.


> Thanks
> 
> Matthias

-- 
Regards
Balakrishna.