* [Patch v1] Bluetooth: Add Rfkill driver for Intel Bluetooth controller @ 2018-10-30 13:56 Chethan T N 2018-11-05 9:16 ` chethan tn 0 siblings, 1 reply; 8+ messages in thread From: Chethan T N @ 2018-10-30 13:56 UTC (permalink / raw) To: linux-bluetooth; +Cc: amit.k.bag, Raghuram Hegde, Chethan T N, Sukumar Ghorai From: Raghuram Hegde <raghuram.hegde@intel.com> 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@intel.com> Signed-off-by: Chethan T N <chethan.tumkur.narayan@intel.com> Signed-off-by: Sukumar Ghorai <sukumar.ghorai@intel.com> --- drivers/platform/x86/Kconfig | 9 +++ drivers/platform/x86/Makefile | 1 + drivers/platform/x86/intel_bt_rfkill.c | 123 +++++++++++++++++++++++++++++++++ 3 files changed, 133 insertions(+) create mode 100644 drivers/platform/x86/intel_bt_rfkill.c diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index 0c1aa6c314f5..bf050ba644c6 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -1229,6 +1229,15 @@ config I2C_MULTI_INSTANTIATE To compile this driver as a module, choose M here: the module will be called i2c-multi-instantiate. +config INTEL_BT_RFKILL + tristate "Intel bluetooth platform rfkill support" + depends on ACPI + depends on RFKILL || !RFKILL + ---help--- + This option adds support for rfkill switch on Intel bluetooth + controllers. + If you have a PC with Intel Bluetooth controller, choose Y. + endif # X86_PLATFORM_DEVICES config PMC_ATOM diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile index e6d1becf81ce..af9ac3cd6151 100644 --- a/drivers/platform/x86/Makefile +++ b/drivers/platform/x86/Makefile @@ -92,3 +92,4 @@ obj-$(CONFIG_MLX_PLATFORM) += mlx-platform.o obj-$(CONFIG_INTEL_TURBO_MAX_3) += intel_turbo_max_3.o obj-$(CONFIG_INTEL_CHTDC_TI_PWRBTN) += intel_chtdc_ti_pwrbtn.o obj-$(CONFIG_I2C_MULTI_INSTANTIATE) += i2c-multi-instantiate.o +obj-$(CONFIG_INTEL_BT_RFKILL) += intel_bt_rfkill.o diff --git a/drivers/platform/x86/intel_bt_rfkill.c b/drivers/platform/x86/intel_bt_rfkill.c new file mode 100644 index 000000000000..904440dadb64 --- /dev/null +++ b/drivers/platform/x86/intel_bt_rfkill.c @@ -0,0 +1,123 @@ +/* + * 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 intel_bt_rfkill_dev { + struct rfkill *rfkill; + struct gpio_desc *reset_gpio_handler; +}; + +static const struct acpi_gpio_params reset_gpios = { 0, 0, false }; +static const struct acpi_gpio_mapping acpi_intel_bt_rfkill_gpios[] = { + { "reset-gpios", &reset_gpios, 1 }, + { }, +}; + +static int intel_bt_disable(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 intel_bt_enable(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 intel_bt_rfkill_set_block(void *data, bool blocked) +{ + struct intel_bt_rfkill_dev *bt_dev = data; + int ret; + + if (blocked) + ret = intel_bt_disable(bt_dev->reset_gpio_handler); + else + ret = intel_bt_enable(bt_dev->reset_gpio_handler); + + return ret; +} +static const struct rfkill_ops rfk_ops = { + .set_block = intel_bt_rfkill_set_block, +}; + +/* ACPI object probe */ +static int intel_bt_rfkill_probe(struct platform_device *pdev) +{ + struct intel_bt_rfkill_dev *bt_dev; + int result; + + bt_dev = devm_kzalloc(&pdev->dev, sizeof(*bt_dev), GFP_KERNEL); + if (!bt_dev) + return -ENOMEM; + + dev_set_drvdata(&pdev->dev, bt_dev); + + if (acpi_dev_add_driver_gpios(ACPI_COMPANION(&pdev->dev), + acpi_intel_bt_rfkill_gpios)) + 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)) + return PTR_ERR(bt_dev->reset_gpio_handler); + + bt_dev->rfkill = rfkill_alloc("intel_bluetooth", + &pdev->dev, + RFKILL_TYPE_BLUETOOTH, + &rfk_ops, + bt_dev); + if (!bt_dev->rfkill) + return -ENOMEM; + + result = rfkill_register(bt_dev->rfkill); + if (result) + rfkill_destroy(bt_dev->rfkill); + + return result; +} + +static int intel_bt_rfkill_remove(struct platform_device *pdev) +{ + struct intel_bt_rfkill_dev *bt_dev = dev_get_drvdata(&pdev->dev); + + if (bt_dev->rfkill) { + rfkill_unregister(bt_dev->rfkill); + rfkill_destroy(bt_dev->rfkill); + } + + acpi_dev_remove_driver_gpios(ACPI_COMPANION(&pdev->dev)); + + return 0; +} + +static const struct acpi_device_id intel_bt_rfkill_acpi_match[] = { + { "INTL6205", 0 }, + { }, +}; +MODULE_DEVICE_TABLE(acpi, intel_bt_rfkill_acpi_match); + +static struct platform_driver intel_bt_rfkill_driver = { + .probe = intel_bt_rfkill_probe, + .remove = intel_bt_rfkill_remove, + .driver = { + .name = "intel_bt_rfkill", + .acpi_match_table = ACPI_PTR(intel_bt_rfkill_acpi_match), + }, +}; +module_platform_driver(intel_bt_rfkill_driver); -- 2.7.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Patch v1] Bluetooth: Add Rfkill driver for Intel Bluetooth controller 2018-10-30 13:56 [Patch v1] Bluetooth: Add Rfkill driver for Intel Bluetooth controller Chethan T N @ 2018-11-05 9:16 ` chethan tn 2018-11-07 1:44 ` Dmitry Torokhov 0 siblings, 1 reply; 8+ messages in thread From: chethan tn @ 2018-11-05 9:16 UTC (permalink / raw) To: chethan.tumkur.narayan Cc: linux-bluetooth, amit.k.bag, Raghuram Hegde, sukumar.ghorai Hi, We are planning to further implement the followings, kindly please provide your suggestions. 1. To handle more than 1 Intel BT controller connected to platform, will keep list of the objects in "static const struct acpi_device_id intel_bt_rfkill_acpi_match[] ". And keep a list of "struct intel_bt_rfkill_dev" for each of the acpi object. 2. With this implementation from user space RF kill for the device object is achieved, however need to map the rfkill object with the corresponding "hdev" so that on error from the controller kernel can do the reset through this RF Kill driver. Best Regards Chethan On Tue, Oct 30, 2018 at 7:27 PM Chethan T N <chethan.tumkur.narayan@intel.com> wrote: > > From: Raghuram Hegde <raghuram.hegde@intel.com> > > 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@intel.com> > Signed-off-by: Chethan T N <chethan.tumkur.narayan@intel.com> > Signed-off-by: Sukumar Ghorai <sukumar.ghorai@intel.com> > --- > drivers/platform/x86/Kconfig | 9 +++ > drivers/platform/x86/Makefile | 1 + > drivers/platform/x86/intel_bt_rfkill.c | 123 +++++++++++++++++++++++++++++++++ > 3 files changed, 133 insertions(+) > create mode 100644 drivers/platform/x86/intel_bt_rfkill.c > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > index 0c1aa6c314f5..bf050ba644c6 100644 > --- a/drivers/platform/x86/Kconfig > +++ b/drivers/platform/x86/Kconfig > @@ -1229,6 +1229,15 @@ config I2C_MULTI_INSTANTIATE > To compile this driver as a module, choose M here: the module > will be called i2c-multi-instantiate. > > +config INTEL_BT_RFKILL > + tristate "Intel bluetooth platform rfkill support" > + depends on ACPI > + depends on RFKILL || !RFKILL > + ---help--- > + This option adds support for rfkill switch on Intel bluetooth > + controllers. > + If you have a PC with Intel Bluetooth controller, choose Y. > + > endif # X86_PLATFORM_DEVICES > > config PMC_ATOM > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile > index e6d1becf81ce..af9ac3cd6151 100644 > --- a/drivers/platform/x86/Makefile > +++ b/drivers/platform/x86/Makefile > @@ -92,3 +92,4 @@ obj-$(CONFIG_MLX_PLATFORM) += mlx-platform.o > obj-$(CONFIG_INTEL_TURBO_MAX_3) += intel_turbo_max_3.o > obj-$(CONFIG_INTEL_CHTDC_TI_PWRBTN) += intel_chtdc_ti_pwrbtn.o > obj-$(CONFIG_I2C_MULTI_INSTANTIATE) += i2c-multi-instantiate.o > +obj-$(CONFIG_INTEL_BT_RFKILL) += intel_bt_rfkill.o > diff --git a/drivers/platform/x86/intel_bt_rfkill.c b/drivers/platform/x86/intel_bt_rfkill.c > new file mode 100644 > index 000000000000..904440dadb64 > --- /dev/null > +++ b/drivers/platform/x86/intel_bt_rfkill.c > @@ -0,0 +1,123 @@ > +/* > + * 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 intel_bt_rfkill_dev { > + struct rfkill *rfkill; > + struct gpio_desc *reset_gpio_handler; > +}; > + > +static const struct acpi_gpio_params reset_gpios = { 0, 0, false }; > +static const struct acpi_gpio_mapping acpi_intel_bt_rfkill_gpios[] = { > + { "reset-gpios", &reset_gpios, 1 }, > + { }, > +}; > + > +static int intel_bt_disable(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 intel_bt_enable(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 intel_bt_rfkill_set_block(void *data, bool blocked) > +{ > + struct intel_bt_rfkill_dev *bt_dev = data; > + int ret; > + > + if (blocked) > + ret = intel_bt_disable(bt_dev->reset_gpio_handler); > + else > + ret = intel_bt_enable(bt_dev->reset_gpio_handler); > + > + return ret; > +} > +static const struct rfkill_ops rfk_ops = { > + .set_block = intel_bt_rfkill_set_block, > +}; > + > +/* ACPI object probe */ > +static int intel_bt_rfkill_probe(struct platform_device *pdev) > +{ > + struct intel_bt_rfkill_dev *bt_dev; > + int result; > + > + bt_dev = devm_kzalloc(&pdev->dev, sizeof(*bt_dev), GFP_KERNEL); > + if (!bt_dev) > + return -ENOMEM; > + > + dev_set_drvdata(&pdev->dev, bt_dev); > + > + if (acpi_dev_add_driver_gpios(ACPI_COMPANION(&pdev->dev), > + acpi_intel_bt_rfkill_gpios)) > + 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)) > + return PTR_ERR(bt_dev->reset_gpio_handler); > + > + bt_dev->rfkill = rfkill_alloc("intel_bluetooth", > + &pdev->dev, > + RFKILL_TYPE_BLUETOOTH, > + &rfk_ops, > + bt_dev); > + if (!bt_dev->rfkill) > + return -ENOMEM; > + > + result = rfkill_register(bt_dev->rfkill); > + if (result) > + rfkill_destroy(bt_dev->rfkill); > + > + return result; > +} > + > +static int intel_bt_rfkill_remove(struct platform_device *pdev) > +{ > + struct intel_bt_rfkill_dev *bt_dev = dev_get_drvdata(&pdev->dev); > + > + if (bt_dev->rfkill) { > + rfkill_unregister(bt_dev->rfkill); > + rfkill_destroy(bt_dev->rfkill); > + } > + > + acpi_dev_remove_driver_gpios(ACPI_COMPANION(&pdev->dev)); > + > + return 0; > +} > + > +static const struct acpi_device_id intel_bt_rfkill_acpi_match[] = { > + { "INTL6205", 0 }, > + { }, > +}; > +MODULE_DEVICE_TABLE(acpi, intel_bt_rfkill_acpi_match); > + > +static struct platform_driver intel_bt_rfkill_driver = { > + .probe = intel_bt_rfkill_probe, > + .remove = intel_bt_rfkill_remove, > + .driver = { > + .name = "intel_bt_rfkill", > + .acpi_match_table = ACPI_PTR(intel_bt_rfkill_acpi_match), > + }, > +}; > +module_platform_driver(intel_bt_rfkill_driver); > -- > 2.7.4 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Patch v1] Bluetooth: Add Rfkill driver for Intel Bluetooth controller 2018-11-05 9:16 ` chethan tn @ 2018-11-07 1:44 ` Dmitry Torokhov 2018-11-07 7:28 ` Marcel Holtmann 0 siblings, 1 reply; 8+ messages in thread From: Dmitry Torokhov @ 2018-11-07 1:44 UTC (permalink / raw) To: chethan tn Cc: chethan.tumkur.narayan, linux-bluetooth, amit.k.bag, Raghuram Hegde, sukumar.ghorai, Marcel Holtmann, Rajat Jain Hi Chethan, On Mon, Nov 05, 2018 at 02:46:26PM +0530, chethan tn wrote: > Hi, > > We are planning to further implement the followings, kindly please > provide your suggestions. > 1. To handle more than 1 Intel BT controller connected to platform, > will keep list of the objects in "static const struct acpi_device_id > intel_bt_rfkill_acpi_match[] ". And keep a list of "struct > intel_bt_rfkill_dev" for each of the acpi object. > 2. With this implementation from user space RF kill for the device > object is achieved, however need to map the rfkill object with the > corresponding "hdev" so that on error from the controller kernel can > do the reset through this RF Kill driver. I am confused, why you model a generic chip reset functionality via RFKill subsystem. As far as I understand, the issue is that you want to be able to reset the chip when it gets confused and not actually disable the chip/stop it from emitting RF signals. I believe this functionality should be contained in the driver and you simply need to come with a way to tie the adapter instance with data in ACPI, probably based on physical USB connection. Thanks. > > Best Regards > Chethan > On Tue, Oct 30, 2018 at 7:27 PM Chethan T N > <chethan.tumkur.narayan@intel.com> wrote: > > > > From: Raghuram Hegde <raghuram.hegde@intel.com> > > > > 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@intel.com> > > Signed-off-by: Chethan T N <chethan.tumkur.narayan@intel.com> > > Signed-off-by: Sukumar Ghorai <sukumar.ghorai@intel.com> > > --- > > drivers/platform/x86/Kconfig | 9 +++ > > drivers/platform/x86/Makefile | 1 + > > drivers/platform/x86/intel_bt_rfkill.c | 123 +++++++++++++++++++++++++++++++++ > > 3 files changed, 133 insertions(+) > > create mode 100644 drivers/platform/x86/intel_bt_rfkill.c > > > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > > index 0c1aa6c314f5..bf050ba644c6 100644 > > --- a/drivers/platform/x86/Kconfig > > +++ b/drivers/platform/x86/Kconfig > > @@ -1229,6 +1229,15 @@ config I2C_MULTI_INSTANTIATE > > To compile this driver as a module, choose M here: the module > > will be called i2c-multi-instantiate. > > > > +config INTEL_BT_RFKILL > > + tristate "Intel bluetooth platform rfkill support" > > + depends on ACPI > > + depends on RFKILL || !RFKILL > > + ---help--- > > + This option adds support for rfkill switch on Intel bluetooth > > + controllers. > > + If you have a PC with Intel Bluetooth controller, choose Y. > > + > > endif # X86_PLATFORM_DEVICES > > > > config PMC_ATOM > > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile > > index e6d1becf81ce..af9ac3cd6151 100644 > > --- a/drivers/platform/x86/Makefile > > +++ b/drivers/platform/x86/Makefile > > @@ -92,3 +92,4 @@ obj-$(CONFIG_MLX_PLATFORM) += mlx-platform.o > > obj-$(CONFIG_INTEL_TURBO_MAX_3) += intel_turbo_max_3.o > > obj-$(CONFIG_INTEL_CHTDC_TI_PWRBTN) += intel_chtdc_ti_pwrbtn.o > > obj-$(CONFIG_I2C_MULTI_INSTANTIATE) += i2c-multi-instantiate.o > > +obj-$(CONFIG_INTEL_BT_RFKILL) += intel_bt_rfkill.o > > diff --git a/drivers/platform/x86/intel_bt_rfkill.c b/drivers/platform/x86/intel_bt_rfkill.c > > new file mode 100644 > > index 000000000000..904440dadb64 > > --- /dev/null > > +++ b/drivers/platform/x86/intel_bt_rfkill.c > > @@ -0,0 +1,123 @@ > > +/* > > + * 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 intel_bt_rfkill_dev { > > + struct rfkill *rfkill; > > + struct gpio_desc *reset_gpio_handler; > > +}; > > + > > +static const struct acpi_gpio_params reset_gpios = { 0, 0, false }; > > +static const struct acpi_gpio_mapping acpi_intel_bt_rfkill_gpios[] = { > > + { "reset-gpios", &reset_gpios, 1 }, > > + { }, > > +}; > > + > > +static int intel_bt_disable(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 intel_bt_enable(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 intel_bt_rfkill_set_block(void *data, bool blocked) > > +{ > > + struct intel_bt_rfkill_dev *bt_dev = data; > > + int ret; > > + > > + if (blocked) > > + ret = intel_bt_disable(bt_dev->reset_gpio_handler); > > + else > > + ret = intel_bt_enable(bt_dev->reset_gpio_handler); > > + > > + return ret; > > +} > > +static const struct rfkill_ops rfk_ops = { > > + .set_block = intel_bt_rfkill_set_block, > > +}; > > + > > +/* ACPI object probe */ > > +static int intel_bt_rfkill_probe(struct platform_device *pdev) > > +{ > > + struct intel_bt_rfkill_dev *bt_dev; > > + int result; > > + > > + bt_dev = devm_kzalloc(&pdev->dev, sizeof(*bt_dev), GFP_KERNEL); > > + if (!bt_dev) > > + return -ENOMEM; > > + > > + dev_set_drvdata(&pdev->dev, bt_dev); > > + > > + if (acpi_dev_add_driver_gpios(ACPI_COMPANION(&pdev->dev), > > + acpi_intel_bt_rfkill_gpios)) > > + 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)) > > + return PTR_ERR(bt_dev->reset_gpio_handler); > > + > > + bt_dev->rfkill = rfkill_alloc("intel_bluetooth", > > + &pdev->dev, > > + RFKILL_TYPE_BLUETOOTH, > > + &rfk_ops, > > + bt_dev); > > + if (!bt_dev->rfkill) > > + return -ENOMEM; > > + > > + result = rfkill_register(bt_dev->rfkill); > > + if (result) > > + rfkill_destroy(bt_dev->rfkill); > > + > > + return result; > > +} > > + > > +static int intel_bt_rfkill_remove(struct platform_device *pdev) > > +{ > > + struct intel_bt_rfkill_dev *bt_dev = dev_get_drvdata(&pdev->dev); > > + > > + if (bt_dev->rfkill) { > > + rfkill_unregister(bt_dev->rfkill); > > + rfkill_destroy(bt_dev->rfkill); > > + } > > + > > + acpi_dev_remove_driver_gpios(ACPI_COMPANION(&pdev->dev)); > > + > > + return 0; > > +} > > + > > +static const struct acpi_device_id intel_bt_rfkill_acpi_match[] = { > > + { "INTL6205", 0 }, > > + { }, > > +}; > > +MODULE_DEVICE_TABLE(acpi, intel_bt_rfkill_acpi_match); > > + > > +static struct platform_driver intel_bt_rfkill_driver = { > > + .probe = intel_bt_rfkill_probe, > > + .remove = intel_bt_rfkill_remove, > > + .driver = { > > + .name = "intel_bt_rfkill", > > + .acpi_match_table = ACPI_PTR(intel_bt_rfkill_acpi_match), > > + }, > > +}; > > +module_platform_driver(intel_bt_rfkill_driver); > > -- > > 2.7.4 -- Dmitry ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Patch v1] Bluetooth: Add Rfkill driver for Intel Bluetooth controller 2018-11-07 1:44 ` Dmitry Torokhov @ 2018-11-07 7:28 ` Marcel Holtmann 2018-11-07 18:40 ` Dmitry Torokhov 0 siblings, 1 reply; 8+ messages in thread From: Marcel Holtmann @ 2018-11-07 7:28 UTC (permalink / raw) To: Dmitry Torokhov Cc: chethan tn, Chethan T N, Bluez mailing list, amit.k.bag, Raghuram Hegde, sukumar.ghorai, Rajat Jain Hi Dmitry, >> We are planning to further implement the followings, kindly please >> provide your suggestions. >> 1. To handle more than 1 Intel BT controller connected to platform, >> will keep list of the objects in "static const struct acpi_device_id >> intel_bt_rfkill_acpi_match[] ". And keep a list of "struct >> intel_bt_rfkill_dev" for each of the acpi object. >> 2. With this implementation from user space RF kill for the device >> object is achieved, however need to map the rfkill object with the >> corresponding "hdev" so that on error from the controller kernel can >> do the reset through this RF Kill driver. > > I am confused, why you model a generic chip reset functionality via > RFKill subsystem. As far as I understand, the issue is that you want to > be able to reset the chip when it gets confused and not actually disable > the chip/stop it from emitting RF signals. > > I believe this functionality should be contained in the driver and you > simply need to come with a way to tie the adapter instance with data in > ACPI, probably based on physical USB connection. it is impossible to do that in the driver since what the GPIO is doing is to push the USB device off the bus. So you actually see an USB disconnect and a new re-enumeration when it comes back. Meaning the driver knows nothing during that time. This is a classic soft RFKILL switch like we have seen in the early Thinkpads. Yes, this could be done all nicely with proper integration into the driver, but not with this hardware setup. They took the big hammer approach and killed the power to the device. A proper defined Reset GPIO would have been better and then we could have done this nicely within the driver. However if the device disappears, there is nothing for the driver to do. Regards Marcel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Patch v1] Bluetooth: Add Rfkill driver for Intel Bluetooth controller 2018-11-07 7:28 ` Marcel Holtmann @ 2018-11-07 18:40 ` Dmitry Torokhov 2018-11-07 18:48 ` Marcel Holtmann 0 siblings, 1 reply; 8+ messages in thread From: Dmitry Torokhov @ 2018-11-07 18:40 UTC (permalink / raw) To: Marcel Holtmann Cc: chethantn, chethan.tumkur.narayan, linux-bluetooth, amit.k.bag, raghuram.hegde, sukumar.ghorai, rajatja On Tue, Nov 6, 2018 at 11:28 PM Marcel Holtmann <marcel@holtmann.org> wrote: > > Hi Dmitry, > > >> We are planning to further implement the followings, kindly please > >> provide your suggestions. > >> 1. To handle more than 1 Intel BT controller connected to platform, > >> will keep list of the objects in "static const struct acpi_device_id > >> intel_bt_rfkill_acpi_match[] ". And keep a list of "struct > >> intel_bt_rfkill_dev" for each of the acpi object. > >> 2. With this implementation from user space RF kill for the device > >> object is achieved, however need to map the rfkill object with the > >> corresponding "hdev" so that on error from the controller kernel can > >> do the reset through this RF Kill driver. > > > > I am confused, why you model a generic chip reset functionality via > > RFKill subsystem. As far as I understand, the issue is that you want to > > be able to reset the chip when it gets confused and not actually disable > > the chip/stop it from emitting RF signals. > > > > I believe this functionality should be contained in the driver and you > > simply need to come with a way to tie the adapter instance with data in > > ACPI, probably based on physical USB connection. > > it is impossible to do that in the driver since what the GPIO is doing is to push the USB device off the bus. So you actually see an USB disconnect and a new re-enumeration when it comes back. Meaning the driver knows nothing during that time. The driver would know that it is in the middle of resetting the device. The fact that the device disappears from the bus is not a big deal. You just need to make sure you finish the reset task running before finishing teardown of the device in disconnect method. > This is a classic soft RFKILL switch like we have seen in the early Thinkpads. It is not RFkill as, as far as I understand, it does not guarantee that it actually blocks the transmitter. It really is a reset line and its purpose is to unwedge the chip, not keep it in the off state. I do not think there is a reason to export this as RFkill switch to userspace and then build infrastructure to recognize that this is special kind of RFkill switch that can be used to work around issues in the controller. Have the driver assert and deassert it to kick itself off the bus, the USB hotplug will take care of the rest. Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Patch v1] Bluetooth: Add Rfkill driver for Intel Bluetooth controller 2018-11-07 18:40 ` Dmitry Torokhov @ 2018-11-07 18:48 ` Marcel Holtmann 2018-11-07 19:16 ` Dmitry Torokhov 0 siblings, 1 reply; 8+ messages in thread From: Marcel Holtmann @ 2018-11-07 18:48 UTC (permalink / raw) To: Dmitry Torokhov Cc: chethan tn, Chethan T N, Bluez mailing list, amit.k.bag, raghuram.hegde, sukumar.ghorai, rajatja Hi Dmitry, >>>> We are planning to further implement the followings, kindly please >>>> provide your suggestions. >>>> 1. To handle more than 1 Intel BT controller connected to platform, >>>> will keep list of the objects in "static const struct acpi_device_id >>>> intel_bt_rfkill_acpi_match[] ". And keep a list of "struct >>>> intel_bt_rfkill_dev" for each of the acpi object. >>>> 2. With this implementation from user space RF kill for the device >>>> object is achieved, however need to map the rfkill object with the >>>> corresponding "hdev" so that on error from the controller kernel can >>>> do the reset through this RF Kill driver. >>> >>> I am confused, why you model a generic chip reset functionality via >>> RFKill subsystem. As far as I understand, the issue is that you want to >>> be able to reset the chip when it gets confused and not actually disable >>> the chip/stop it from emitting RF signals. >>> >>> I believe this functionality should be contained in the driver and you >>> simply need to come with a way to tie the adapter instance with data in >>> ACPI, probably based on physical USB connection. >> >> it is impossible to do that in the driver since what the GPIO is doing is to push the USB device off the bus. So you actually see an USB disconnect and a new re-enumeration when it comes back. Meaning the driver knows nothing during that time. > > The driver would know that it is in the middle of resetting the > device. The fact that the device disappears from the bus is not a big > deal. You just need to make sure you finish the reset task running > before finishing teardown of the device in disconnect method. it is a big deal and it is unpredictable. We can not magically assign resources to a device that is about to go away. And more importantly you have no idea which device goes away. >> This is a classic soft RFKILL switch like we have seen in the early Thinkpads. > > It is not RFkill as, as far as I understand, it does not guarantee > that it actually blocks the transmitter. It really is a reset line and > its purpose is to unwedge the chip, not keep it in the off state. I do > not think there is a reason to export this as RFkill switch to > userspace and then build infrastructure to recognize that this is > special kind of RFkill switch that can be used to work around issues > in the controller. Have the driver assert and deassert it to kick > itself off the bus, the USB hotplug will take care of the rest. It is a RFKILL switch since the device looses power and with also turns RF off. As I said, this is the same as with the old Thinkpads where we have done exactly the same. Regards Marcel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Patch v1] Bluetooth: Add Rfkill driver for Intel Bluetooth controller 2018-11-07 18:48 ` Marcel Holtmann @ 2018-11-07 19:16 ` Dmitry Torokhov 2018-11-08 8:33 ` Marcel Holtmann 0 siblings, 1 reply; 8+ messages in thread From: Dmitry Torokhov @ 2018-11-07 19:16 UTC (permalink / raw) To: Marcel Holtmann Cc: chethantn, chethan.tumkur.narayan, linux-bluetooth, amit.k.bag, raghuram.hegde, sukumar.ghorai, rajatja On Wed, Nov 7, 2018 at 10:48 AM Marcel Holtmann <marcel@holtmann.org> wrote: > > Hi Dmitry, > > >>>> We are planning to further implement the followings, kindly please > >>>> provide your suggestions. > >>>> 1. To handle more than 1 Intel BT controller connected to platform, > >>>> will keep list of the objects in "static const struct acpi_device_id > >>>> intel_bt_rfkill_acpi_match[] ". And keep a list of "struct > >>>> intel_bt_rfkill_dev" for each of the acpi object. > >>>> 2. With this implementation from user space RF kill for the device > >>>> object is achieved, however need to map the rfkill object with the > >>>> corresponding "hdev" so that on error from the controller kernel can > >>>> do the reset through this RF Kill driver. > >>> > >>> I am confused, why you model a generic chip reset functionality via > >>> RFKill subsystem. As far as I understand, the issue is that you want to > >>> be able to reset the chip when it gets confused and not actually disable > >>> the chip/stop it from emitting RF signals. > >>> > >>> I believe this functionality should be contained in the driver and you > >>> simply need to come with a way to tie the adapter instance with data in > >>> ACPI, probably based on physical USB connection. > >> > >> it is impossible to do that in the driver since what the GPIO is doing is to push the USB device off the bus. So you actually see an USB disconnect and a new re-enumeration when it comes back. Meaning the driver knows nothing during that time. > > > > The driver would know that it is in the middle of resetting the > > device. The fact that the device disappears from the bus is not a big > > deal. You just need to make sure you finish the reset task running > > before finishing teardown of the device in disconnect method. > > it is a big deal and it is unpredictable. We can not magically assign resources to a device that is about to go away. Nor should you. You allocate the resources at bind time. > And more importantly you have no idea which device goes away. What do you mean? The reset line goes to exactly one controller. You just need to find a way to describe it in ACPI. Probably attach to the device that describes USB port the controller is attached to? This is not the first device hard-wired USB device that needs additional info supplied via ACPI or DT, and we already have a way to specify bindings in device tree for USB devices: Documentation/devicetree/bindings/usb/usb-device.txt and actually btusb itself: Documentation/devicetree/bindings/net/btusb.txt Can we adopt the above to ACPI? > > >> This is a classic soft RFKILL switch like we have seen in the early Thinkpads. > > > > It is not RFkill as, as far as I understand, it does not guarantee > > that it actually blocks the transmitter. It really is a reset line and > > its purpose is to unwedge the chip, not keep it in the off state. I do > > not think there is a reason to export this as RFkill switch to > > userspace and then build infrastructure to recognize that this is > > special kind of RFkill switch that can be used to work around issues > > in the controller. Have the driver assert and deassert it to kick > > itself off the bus, the USB hotplug will take care of the rest. > > It is a RFKILL switch since the device looses power and with also turns RF off. As I said, this is the same as with the old Thinkpads where we have done exactly the same. I believe you should not only look at the end result, but also at the purpose of the facility. What is being handled here I believe was not intended as a switch to turn device off. Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Patch v1] Bluetooth: Add Rfkill driver for Intel Bluetooth controller 2018-11-07 19:16 ` Dmitry Torokhov @ 2018-11-08 8:33 ` Marcel Holtmann 0 siblings, 0 replies; 8+ messages in thread From: Marcel Holtmann @ 2018-11-08 8:33 UTC (permalink / raw) To: Dmitry Torokhov Cc: chethan tn, Chethan T N, Bluez mailing list, Bag, Amit K, raghuram.hegde, sukumar.ghorai, rajatja Hi Dmitry, >>>>>> We are planning to further implement the followings, kindly please >>>>>> provide your suggestions. >>>>>> 1. To handle more than 1 Intel BT controller connected to platform, >>>>>> will keep list of the objects in "static const struct acpi_device_id >>>>>> intel_bt_rfkill_acpi_match[] ". And keep a list of "struct >>>>>> intel_bt_rfkill_dev" for each of the acpi object. >>>>>> 2. With this implementation from user space RF kill for the device >>>>>> object is achieved, however need to map the rfkill object with the >>>>>> corresponding "hdev" so that on error from the controller kernel can >>>>>> do the reset through this RF Kill driver. >>>>> >>>>> I am confused, why you model a generic chip reset functionality via >>>>> RFKill subsystem. As far as I understand, the issue is that you want to >>>>> be able to reset the chip when it gets confused and not actually disable >>>>> the chip/stop it from emitting RF signals. >>>>> >>>>> I believe this functionality should be contained in the driver and you >>>>> simply need to come with a way to tie the adapter instance with data in >>>>> ACPI, probably based on physical USB connection. >>>> >>>> it is impossible to do that in the driver since what the GPIO is doing is to push the USB device off the bus. So you actually see an USB disconnect and a new re-enumeration when it comes back. Meaning the driver knows nothing during that time. >>> >>> The driver would know that it is in the middle of resetting the >>> device. The fact that the device disappears from the bus is not a big >>> deal. You just need to make sure you finish the reset task running >>> before finishing teardown of the device in disconnect method. >> >> it is a big deal and it is unpredictable. We can not magically assign resources to a device that is about to go away. > > Nor should you. You allocate the resources at bind time. > >> And more importantly you have no idea which device goes away. > > What do you mean? The reset line goes to exactly one controller. You > just need to find a way to describe it in ACPI. Probably attach to the > device that describes USB port the controller is attached to? > > This is not the first device hard-wired USB device that needs > additional info supplied via ACPI or DT, and we already have a way to > specify bindings in device tree for USB devices: > Documentation/devicetree/bindings/usb/usb-device.txt and actually > btusb itself: Documentation/devicetree/bindings/net/btusb.txt > > Can we adopt the above to ACPI? I have no heard from anybody on how that can be describe nor that it is for this hardware. So for the driver this binding between the GPIO and the USB interface / device does not exist. >>>> This is a classic soft RFKILL switch like we have seen in the early Thinkpads. >>> >>> It is not RFkill as, as far as I understand, it does not guarantee >>> that it actually blocks the transmitter. It really is a reset line and >>> its purpose is to unwedge the chip, not keep it in the off state. I do >>> not think there is a reason to export this as RFkill switch to >>> userspace and then build infrastructure to recognize that this is >>> special kind of RFkill switch that can be used to work around issues >>> in the controller. Have the driver assert and deassert it to kick >>> itself off the bus, the USB hotplug will take care of the rest. >> >> It is a RFKILL switch since the device looses power and with also turns RF off. As I said, this is the same as with the old Thinkpads where we have done exactly the same. > > I believe you should not only look at the end result, but also at the > purpose of the facility. What is being handled here I believe was not > intended as a switch to turn device off. It is powering off and powering on the device. It is a classical Bluetooth RFKILL switch. As I said, that is how we have treated this big-hammer approach to RFKILL since the Thinkpads started doing that. Regards Marcel ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-11-08 8:33 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-30 13:56 [Patch v1] Bluetooth: Add Rfkill driver for Intel Bluetooth controller Chethan T N 2018-11-05 9:16 ` chethan tn 2018-11-07 1:44 ` Dmitry Torokhov 2018-11-07 7:28 ` Marcel Holtmann 2018-11-07 18:40 ` Dmitry Torokhov 2018-11-07 18:48 ` Marcel Holtmann 2018-11-07 19:16 ` Dmitry Torokhov 2018-11-08 8:33 ` Marcel Holtmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).