RE: [PATCH 1/2] Bluetooth: Add Rfkill driver for Intel Bluetooth controller

["Tumkur Narayan, Chethan" <chethan.tumkur.narayan@xxxxxxxxx>]

Hi Marcel,

Thanks for your inputs, I will create v1 patch with the modification and sent it again.
Best  Regards
Chethan


-----Original Message-----
From: Marcel Holtmann [mailto:marcel@xxxxxxxxxxxx] 
Sent: Tuesday, October 30, 2018 4:38 PM
To: Tumkur Narayan, Chethan <chethan.tumkur.narayan@xxxxxxxxx>
Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Bag, Amit K <amit.k.bag@xxxxxxxxx>; Hegde, Raghuram <raghuram.hegde@xxxxxxxxx>; Ghorai, Sukumar <sukumar.ghorai@xxxxxxxxx>
Subject: Re: [PATCH 1/2] Bluetooth: Add Rfkill driver for Intel Bluetooth controller

Hi Chethan,

> Register ACPI object INTL6205 as platform Bluetooth RfKill driver to 
> attach/detach Intel Bluetooth controller from platform USB bus.
> 
> Signed-off-by: Raghuram Hegde <raghuram.hegde@xxxxxxxxx>
> Signed-off-by: Chethan T N <chethan.tumkur.narayan@xxxxxxxxx>
> Signed-off-by: Sukumar Ghorai <sukumar.ghorai@xxxxxxxxx>
> ---
> drivers/platform/x86/intel_bt_rfkill.c | 133 
> +++++++++++++++++++++++++++++++++
> 1 file changed, 133 insertions(+)
> create mode 100644 drivers/platform/x86/intel_bt_rfkill.c

I would include the Kconfig and Makefile changes here since it is a small driver.

> diff --git a/drivers/platform/x86/intel_bt_rfkill.c 
> b/drivers/platform/x86/intel_bt_rfkill.c
> new file mode 100644
> index 000000000000..89a3d7f4b32c
> --- /dev/null
> +++ b/drivers/platform/x86/intel_bt_rfkill.c
> @@ -0,0 +1,133 @@
> +/*
> + *  Intel Bluetooth Rfkill Driver
> + */
> +
> +#include <linux/module.h>
> +#include <linux/acpi.h>
> +#include <linux/platform_device.h>
> +#include <linux/rfkill.h>
> +#include <linux/gpio/consumer.h>
> +
> +struct btintel_rfkill_dev {
> +	struct rfkill *rfk;

Most drivers used rfkill here instead of rfk. I only found one that used this abbreviation.

> +	struct gpio_desc *reset_gpio_handler; };
> +
> +static const struct acpi_gpio_params reset_gpios = { 0, 0, false }; 
> +static const struct acpi_gpio_mapping acpi_btintel_gpios[] = {
> +	{ "reset-gpios", &reset_gpios, 1 },
> +	{ },
> +};
> +
> +static const struct acpi_device_id btintel_acpi_match[] = {
> +	{ "INTL6205", 0 },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(acpi, btintel_acpi_match);

Personally I put these above the platform_driver struct right before it is used.

> +
> +static int btintel_disable_bt(struct gpio_desc *reset_gpio) {
> +	if (!reset_gpio)
> +		return -EINVAL;
> +
> +	/* This will detach the Intel BT controller */
> +	gpiod_set_value(reset_gpio, 0);
> +	return 0;
> +}
> +
> +static int btintel_enable_bt(struct gpio_desc *reset_gpio) {
> +	if (!reset_gpio)
> +		return -EINVAL;
> +
> +	/* This will re-attach the Intel BT controller */
> +	gpiod_set_value(reset_gpio, 1);
> +	return 0;
> +}
> +
> +/* RFKill handlers */
> +static int bt_rfkill_set_block(void *data, bool blocked) {
> +	struct btintel_rfkill_dev *bt_dev = data;
> +	int ret;

This driver needs a bit more consistency. Either btintel_rfkill or intel_bt_rfkill, but lets not get this confused with actual btintel_ code that we already have.

> +
> +	if (blocked)
> +		ret = btintel_disable_bt(bt_dev->reset_gpio_handler);
> +	else
> +		ret = btintel_enable_bt(bt_dev->reset_gpio_handler);
> +
> +	return ret;
> +}
> +static const struct rfkill_ops rfk_ops = {
> +	.set_block = bt_rfkill_set_block,
> +};
> +
> +/* ACPI object probe */
> +static int btintel_probe(struct platform_device *pdev) {
> +	struct btintel_rfkill_dev *bt_dev;
> +	int result;
> +
> +	bt_dev = kzalloc(sizeof(*bt_dev), GFP_KERNEL);
> +	if (!bt_dev)
> +		return -ENOMEM;

Would’t it better to just use the devm_ version here and let it handle the lifetime tracking of the associated memory.

> +
> +	dev_set_drvdata(&pdev->dev, bt_dev);
> +
> +	if (acpi_dev_add_driver_gpios(ACPI_COMPANION(&pdev->dev),
> +				     acpi_btintel_gpios)) {
> +		kfree(bt_dev);
> +		return -EINVAL;
> +	}
> +
> +	bt_dev->reset_gpio_handler = devm_gpiod_get_optional(&pdev->dev,
> +					"reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(bt_dev->reset_gpio_handler)) {
> +		kfree(bt_dev);
> +		return PTR_ERR(bt_dev->reset_gpio_handler);
> +	}
> +
> +	bt_dev->rfk = rfkill_alloc("intel_bluetooth",
> +				   &pdev->dev,
> +				   RFKILL_TYPE_BLUETOOTH,
> +				   &rfk_ops,
> +				   bt_dev);
> +	if (!bt_dev->rfk) {
> +		kfree(bt_dev);
> +		return -ENOMEM;
> +	}
> +
> +	result = rfkill_register(bt_dev->rfk);
> +	if (result) {
> +		rfkill_destroy(bt_dev->rfk);
> +		kfree(bt_dev);
> +	}
> +
> +	return result;
> +}
> +
> +static int btintel_remove(struct platform_device *pdev) {
> +	struct btintel_rfkill_dev *bt_dev = dev_get_drvdata(&pdev->dev);
> +
> +	if (bt_dev->rfk) {
> +		rfkill_unregister(bt_dev->rfk);
> +		rfkill_destroy(bt_dev->rfk);
> +	}
> +
> +	kfree(bt_dev);
> +
> +	acpi_dev_remove_driver_gpios(ACPI_COMPANION(&pdev->dev));
> +
> +	return 0;
> +}
> +
> +static struct platform_driver btintel_driver = {
> +	.probe = btintel_probe,
> +	.remove = btintel_remove,
> +	.driver = {
> +		.name = "btintel”,

I don’t like this name since it is too generic. Call it btintel_rfkill or intel_bt_rfkill.

> +		.acpi_match_table = ACPI_PTR(btintel_acpi_match),
> +	},
> +};
> +module_platform_driver(btintel_driver);

Regards

Marcel