Re: [PATCH v6 2/5] Bluetooth: btqca: Rename ROME related functions to Generic functions

[Matthias Kaehlcke <mka@chromium.org>]

On Thu, May 24, 2018 at 09:30:48PM +0530, Balakrishna Godavarthi wrote:
> Some of the QCA BTSoC ROME functions, are used for different versions
> or different make of BTSoC's. Instead of duplicating the same functions
> for new chip, updating names of the functions that are used for both
> chip's to keep this generic and would help in future when we would have
> new BT SoC.

The commit message talks about 'functions', however only the name of
one function (rome_patch_ver_req()) has changed. From the changes in
log messages I assume you also intended to rename
rome_reset() and rome_download_firmware(). 

> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> ---
> 
> Changes in v6:
> 	* initial patch
> 	* updated names of functions that are used for both the chips to 
> 	  keep this generic and would help in future when we would have 
> 	  new BT SoC.
> 
> ---
>  drivers/bluetooth/btqca.c | 14 +++++++-------
>  drivers/bluetooth/btqca.h |  6 ++++++
>  2 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> index 8219816c54a0..5c3551239b12 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 qca_patch_ver_req(struct hci_dev *hdev, u32 *rome_version)

The different version names are confusing. The name of the function
calls it 'patch version', the parameter 'rome_version' and the callers
vary between 'soc_version' and 'rome_version'.

It would be unfair to ask you to 'fix the world' in this patch series,
but please stick at least to a consistent naming scheme in the code
you change. Obviously feel free to squeeze in another cleanup patch :)

>  static int rome_reset(struct hci_dev *hdev)
>  {
>  	struct sk_buff *skb;
>  	int err;
>  
> -	BT_DBG("%s: ROME HCI_RESET", hdev->name);
> +	bt_dev_dbg(hdev, "QCA BTSoC HCI_RESET");
>  
>  	skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIMEOUT);
>  	if (IS_ERR(skb)) {
> @@ -267,7 +267,7 @@ static int rome_download_firmware(struct hci_dev *hdev,
>  	const u8 *segment;
>  	int ret, remain, i = 0;
>  
> -	bt_dev_info(hdev, "ROME Downloading %s", config->fwname);
> +	bt_dev_info(hdev, "QCA BTSoC Downloading %s", config->fwname);
>  
>  	ret = request_firmware(&fw, config->fwname, &hdev->dev);
>  	if (ret) {
> @@ -339,7 +339,7 @@ int qca_uart_setup_rome(struct hci_dev *hdev, uint8_t baudrate)
>  	config.user_baud_rate = baudrate;
>  
>  	/* Get ROME version information */

'patch version'/'SoC version'/... , whatever the chosen consistent
name is.

> -	err = rome_patch_ver_req(hdev, &rome_ver);
> +	err = qca_patch_ver_req(hdev, &rome_ver);

rome_ver => ${consistent_name}_ver

Thanks

Matthias