Re: [PATCH v4] Bluetooth: Add hci_h4p driver

[Sebastian Reichel]

Hi Pavel,

You have missed some things. See inline comments below.

On Fri, Jan 10, 2014 at 01:18:05PM +0100, Pavel Machek wrote:
> diff --git a/drivers/bluetooth/nokia_core.c b/drivers/bluetooth/nokia_core.c
> index 5c7acad..d6b0701 100644
> --- a/drivers/bluetooth/nokia_core.c
> v+++ b/drivers/bluetooth/nokia_core.c
> @@ -39,6 +39,7 @@
>  #include <linux/gpio.h>
>  #include <linux/timer.h>
>  #include <linux/kthread.h>
> +#include <linux/io.h>
>  
>  #include <net/bluetooth/bluetooth.h>
>  #include <net/bluetooth/hci_core.h>
> @@ -1079,7 +1080,7 @@ static int hci_h4p_probe(struct platform_device *pdev)
>  	int err;
>  
>  	dev_info(&pdev->dev, "Registering HCI H4P device\n");
> -	info = kzalloc(sizeof(struct hci_h4p_info), GFP_KERNEL);
> +	info = devm_kzalloc(&pdev->dev, sizeof(struct hci_h4p_info), GFP_KERNEL);
>  	if (!info)
>  		return -ENOMEM;
>  
> @@ -1092,7 +1093,6 @@ static int hci_h4p_probe(struct platform_device *pdev)
>  
>  	if (pdev->dev.platform_data == NULL) {
>  		dev_err(&pdev->dev, "Could not get Bluetooth config data\n");
> -		kfree(info);
>  		return -ENODATA;
>  	}
>  
> @@ -1113,67 +1113,59 @@ static int hci_h4p_probe(struct platform_device *pdev)
>  	complete_all(&info->test_completion);
>  
>  	if (!info->reset_gpio_shared) {
> -		err = gpio_request(info->reset_gpio, "bt_reset");
> +		err = devm_gpio_request_one(&pdev->dev, info->reset_gpio,
> +					    GPIOF_OUT_INIT_LOW, "bt_reset");
>  		if (err < 0) {
>  			dev_err(&pdev->dev, "Cannot get GPIO line %d\n",
>  				info->reset_gpio);
> -			goto cleanup_setup;
> +			return err;
>  		}
>  	}
>  
> -	err = gpio_request(info->bt_wakeup_gpio, "bt_wakeup");
> +	err = devm_gpio_request_one(&pdev->dev, info->bt_wakeup_gpio,
> +				    GPIOF_OUT_INIT_LOW, "bt_wakeup");
> +
>  	if (err < 0) {
>  		dev_err(info->dev, "Cannot get GPIO line 0x%d",
>  			info->bt_wakeup_gpio);
> -		if (!info->reset_gpio_shared)
> -			gpio_free(info->reset_gpio);
> -		goto cleanup_setup;
> +		return err;
>  	}
>  
> -	err = gpio_request(info->host_wakeup_gpio, "host_wakeup");
> +	err = devm_gpio_request_one(&pdev->dev, info->host_wakeup_gpio,
> +				    GPIOF_DIR_IN, "host_wakeup");
>  	if (err < 0) {
>  		dev_err(info->dev, "Cannot get GPIO line %d",
>  		       info->host_wakeup_gpio);
> -		if (!info->reset_gpio_shared)
> -			gpio_free(info->reset_gpio);
> -		gpio_free(info->bt_wakeup_gpio);
> -		goto cleanup_setup;
> +		return err;
>  	}
>  
> -	gpio_direction_output(info->reset_gpio, 0);
> -	gpio_direction_output(info->bt_wakeup_gpio, 0);
> -	gpio_direction_input(info->host_wakeup_gpio);
> -
>  	info->irq = bt_plat_data->uart_irq;
> -	info->uart_base = ioremap(bt_plat_data->uart_base, SZ_2K);
> -	info->uart_iclk = clk_get(NULL, bt_plat_data->uart_iclk);
> -	info->uart_fclk = clk_get(NULL, bt_plat_data->uart_fclk);
> +	info->uart_base = devm_ioremap(&pdev->dev, bt_plat_data->uart_base, SZ_2K);
> +	info->uart_iclk = devm_clk_get(&pdev->dev, bt_plat_data->uart_iclk);
> +	info->uart_fclk = devm_clk_get(&pdev->dev, bt_plat_data->uart_fclk);
>  
>  	err = request_irq(info->irq, hci_h4p_interrupt, IRQF_DISABLED, "hci_h4p",
>  			  info);

This one should also use devm_request_irq(). Especially since you
removed the cleanup in the probe function already ;)

>  	if (err < 0) {
>  		dev_err(info->dev, "hci_h4p: unable to get IRQ %d\n", info->irq);
> -		goto cleanup;
> +		return err;
>  	}
>  
> -	err = request_irq(gpio_to_irq(info->host_wakeup_gpio),
> +	err = devm_request_irq(&pdev->dev, gpio_to_irq(info->host_wakeup_gpio),
>  			  hci_h4p_wakeup_interrupt,  IRQF_TRIGGER_FALLING |
>  			  IRQF_TRIGGER_RISING | IRQF_DISABLED,
>  			  "hci_h4p_wkup", info);
>  	if (err < 0) {
>  		dev_err(info->dev, "hci_h4p: unable to get wakeup IRQ %d\n",
>  			  gpio_to_irq(info->host_wakeup_gpio));
> -		free_irq(info->irq, info);
> -		goto cleanup;
> +		return err;
>  	}
>  
>  	err = irq_set_irq_wake(gpio_to_irq(info->host_wakeup_gpio), 1);
>  	if (err < 0) {
>  		dev_err(info->dev, "hci_h4p: unable to set wakeup for IRQ %d\n",
>  				gpio_to_irq(info->host_wakeup_gpio));
> -		free_irq(info->irq, info);
> -		free_irq(gpio_to_irq(info->host_wakeup_gpio), info);
> -		goto cleanup;
> +		return err;
>  	}
>  
>  	init_timer_deferrable(&info->lazy_release);
> @@ -1182,7 +1174,7 @@ static int hci_h4p_probe(struct platform_device *pdev)
>  	hci_h4p_set_clk(info, &info->tx_clocks_en, 1);
>  	err = hci_h4p_reset_uart(info);
>  	if (err < 0)
> -		goto cleanup_irq;
> +		return err;
>  	gpio_set_value(info->reset_gpio, 0);
>  	hci_h4p_set_clk(info, &info->tx_clocks_en, 0);
>  
> @@ -1190,23 +1182,10 @@ static int hci_h4p_probe(struct platform_device *pdev)
>  
>  	if (hci_h4p_register_hdev(info) < 0) {
>  		dev_err(info->dev, "failed to register hci_h4p hci device\n");
> -		goto cleanup_irq;
> +		return -EINVAL;
>  	}
>  
>  	return 0;
> -
> -cleanup_irq:
> -	free_irq(info->irq, (void *)info);
> -	free_irq(gpio_to_irq(info->host_wakeup_gpio), info);
> -cleanup:
> -	gpio_set_value(info->reset_gpio, 0);
> -	if (!info->reset_gpio_shared)
> -		gpio_free(info->reset_gpio);
> -	gpio_free(info->bt_wakeup_gpio);
> -	gpio_free(info->host_wakeup_gpio);
> -cleanup_setup:
> -	kfree(info);
> -	return err;
>  }
>  
>  static int hci_h4p_remove(struct platform_device *pdev)
> @@ -1217,15 +1196,11 @@ static int hci_h4p_remove(struct platform_device *pdev)
>  
>  	hci_h4p_sysfs_remove_files(info->dev);
>  	hci_h4p_hci_close(info->hdev);
> -	free_irq(gpio_to_irq(info->host_wakeup_gpio), info);
>  	hci_unregister_dev(info->hdev);
>  	hci_free_dev(info->hdev);
> -	if (!info->reset_gpio_shared)
> -		gpio_free(info->reset_gpio);
>  	gpio_free(info->bt_wakeup_gpio);
>  	gpio_free(info->host_wakeup_gpio);

those two gpios are also managed. No need to free them manually.

>  	free_irq(info->irq, (void *) info);

Once this irq is also requested using managed resources you can also
drop this line.

> -	kfree(info);
>  
>  	return 0;
>  }
> [...]

-- Sebastian

Attachment: signature.asc
Description: Digital signature