Add a regulator consumer driver for rfkill to enable controlling radio
transmitters connected to voltage regulator using the regulator
framework.
A new "vrfkill" virtual supply is provided to use in platform code.
Signed-off-by: Guiming Zhuo <[email protected]>
Signed-off-by: Antonio Ospite <[email protected]>
---
Hi,
a first implementation of the driver which handled only bluetooth was
from Guiming Zhuo, I then made it more generic and tried to put it in
shape for mainline inclusion; that's why the SOB line of Guiming Zhuo
comes first.
Any comment is appreciated.
We are using this driver for blocking/unblocking power to the bluetooth
module on some EZX phones (Motorola A1200/A910/E6/E2) in the openezx.org
project.
Thanks,
Antonio Ospite
http://ao2.it
include/linux/rfkill-regulator.h | 48 +++++++++++
net/rfkill/Kconfig | 11 +++
net/rfkill/Makefile | 1 +
net/rfkill/rfkill-regulator.c | 166 ++++++++++++++++++++++++++++++++++++++
4 files changed, 226 insertions(+), 0 deletions(-)
create mode 100644 include/linux/rfkill-regulator.h
create mode 100644 net/rfkill/rfkill-regulator.c
diff --git a/include/linux/rfkill-regulator.h b/include/linux/rfkill-regulator.h
new file mode 100644
index 0000000..4aaef9c
--- /dev/null
+++ b/include/linux/rfkill-regulator.h
@@ -0,0 +1,48 @@
+/*
+ * rfkill-regulator.c - Regulator consumer driver for rfkill
+ *
+ * Copyright (C) 2009 Guiming Zhuo <[email protected]>
+ * Copyright (C) 2011 Antonio Ospite <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#ifndef __LINUX_RFKILL_REGULATOR_H
+#define __LINUX_RFKILL_REGULATOR_H
+
+/*
+ * Use "vrfkill" as supply id when declaring the regulator consumer:
+ *
+ * static struct regulator_consumer_supply pcap_regulator_V6_consumers [] = {
+ * { .dev_name = "rfkill-regulator.0", supply = "vrfkill" },
+ * };
+ *
+ * If you have several regulator driven rfkill, you can append a numerical id to
+ * .dev_name as done above, and use the same id when declaring the platform
+ * device:
+ *
+ * static struct rfkill_regulator_platform_data ezx_rfkill_bt_data = {
+ * .name = "ezx-bluetooth",
+ * .type = RFKILL_TYPE_BLUETOOTH,
+ * };
+ *
+ * static struct platform_device a910_rfkill = {
+ * .name = "rfkill-regulator",
+ * .id = 0,
+ * .dev = {
+ * .platform_data = &ezx_rfkill_bt_data,
+ * },
+ * };
+ */
+
+#include <linux/rfkill.h>
+
+struct rfkill_regulator_platform_data {
+ char *name; /* the name for the rfkill switch */
+ enum rfkill_type type; /* the type as specified in rfkill.h */
+};
+
+#endif /* __LINUX_RFKILL_REGULATOR_H */
diff --git a/net/rfkill/Kconfig b/net/rfkill/Kconfig
index 7fce6df..48464ca 100644
--- a/net/rfkill/Kconfig
+++ b/net/rfkill/Kconfig
@@ -22,3 +22,14 @@ config RFKILL_INPUT
depends on RFKILL
depends on INPUT = y || RFKILL = INPUT
default y if !EXPERT
+
+config RFKILL_REGULATOR
+ tristate "Generic rfkill regulator driver"
+ depends on RFKILL || !RFKILL
+ depends on REGULATOR
+ help
+ This options enable controlling radio transmitters connected to
+ voltage regulator using the regulator framework.
+
+ To compile this driver as a module, choose M here: the module will
+ be called rfkill-regulator.
diff --git a/net/rfkill/Makefile b/net/rfkill/Makefile
index 6621053..d9a5a58 100644
--- a/net/rfkill/Makefile
+++ b/net/rfkill/Makefile
@@ -5,3 +5,4 @@
rfkill-y += core.o
rfkill-$(CONFIG_RFKILL_INPUT) += input.o
obj-$(CONFIG_RFKILL) += rfkill.o
+obj-$(CONFIG_RFKILL_REGULATOR) += rfkill-regulator.o
diff --git a/net/rfkill/rfkill-regulator.c b/net/rfkill/rfkill-regulator.c
new file mode 100644
index 0000000..448a1bf
--- /dev/null
+++ b/net/rfkill/rfkill-regulator.c
@@ -0,0 +1,166 @@
+/*
+ * rfkill-regulator.c - Regulator consumer driver for rfkill
+ *
+ * Copyright (C) 2009 Guiming Zhuo <[email protected]>
+ * Copyright (C) 2011 Antonio Ospite <[email protected]>
+ *
+ * Implementation inspired by leds-regulator driver.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/rfkill.h>
+#include <linux/rfkill-regulator.h>
+
+struct rfkill_regulator_data {
+ struct rfkill *rf_kill;
+ bool reg_enabled;
+
+ struct regulator *vcc;
+};
+
+static int rfkill_regulator_set_block(void *data, bool blocked)
+{
+ struct rfkill_regulator_data *rfkill_data = data;
+
+ pr_debug("%s: blocked: %d\n", __func__, blocked);
+
+ if (blocked) {
+ if (rfkill_data->reg_enabled) {
+ regulator_disable(rfkill_data->vcc);
+ rfkill_data->reg_enabled = 0;
+ }
+ } else {
+ if (!rfkill_data->reg_enabled) {
+ regulator_enable(rfkill_data->vcc);
+ rfkill_data->reg_enabled = 1;
+ }
+ }
+
+ pr_debug("%s: regulator_is_enabled after set_block: %d\n", __func__,
+ regulator_is_enabled(rfkill_data->vcc));
+
+ return 0;
+}
+
+struct rfkill_ops rfkill_regulator_ops = {
+ .set_block = rfkill_regulator_set_block,
+};
+
+static int __devinit rfkill_regulator_probe(struct platform_device *pdev)
+{
+ struct rfkill_regulator_platform_data *pdata = pdev->dev.platform_data;
+ struct rfkill_regulator_data *rfkill_data;
+ struct regulator *vcc;
+ struct rfkill *rf_kill;
+ int ret = 0;
+
+ if (pdata == NULL) {
+ dev_err(&pdev->dev, "no platform data\n");
+ return -ENODEV;
+ }
+
+ if (pdata->name == NULL || pdata->type == 0) {
+ dev_err(&pdev->dev, "invalid name or type in platform data\n");
+ return -EINVAL;
+ }
+
+ vcc = regulator_get_exclusive(&pdev->dev, "vrfkill");
+ if (IS_ERR(vcc)) {
+ dev_err(&pdev->dev, "Cannot get vcc for %s\n", pdata->name);
+ ret = PTR_ERR(vcc);
+ goto out;
+ }
+
+ rfkill_data = kzalloc(sizeof(*rfkill_data), GFP_KERNEL);
+ if (rfkill_data == NULL) {
+ ret = -ENOMEM;
+ goto err_data_alloc;
+ }
+
+ rf_kill = rfkill_alloc(pdata->name, &pdev->dev,
+ pdata->type,
+ &rfkill_regulator_ops, rfkill_data);
+ if (rf_kill == NULL) {
+ dev_err(&pdev->dev, "Cannot alloc rfkill device\n");
+ ret = -ENOMEM;
+ goto err_rfkill_alloc;
+ }
+
+ if (regulator_is_enabled(vcc)) {
+ dev_dbg(&pdev->dev, "Regulator already enabled\n");
+ rfkill_data->reg_enabled = 1;
+ }
+ rfkill_init_sw_state(rf_kill, !rfkill_data->reg_enabled);
+
+ ret = rfkill_register(rf_kill);
+ if (ret) {
+ dev_err(&pdev->dev, "Cannot register rfkill device\n");
+ goto err_rfkill_register;
+ }
+
+ rfkill_data->rf_kill = rf_kill;
+ rfkill_data->vcc = vcc;
+
+ platform_set_drvdata(pdev, rfkill_data);
+ dev_info(&pdev->dev, "initialized\n");
+
+ return 0;
+
+err_rfkill_register:
+ rfkill_destroy(rf_kill);
+err_rfkill_alloc:
+ kfree(rfkill_data);
+err_data_alloc:
+ regulator_put(vcc);
+out:
+ return ret;
+}
+
+static int __devexit rfkill_regulator_remove(struct platform_device *pdev)
+{
+ struct rfkill_regulator_data *rfkill_data = platform_get_drvdata(pdev);
+ struct rfkill *rf_kill = rfkill_data->rf_kill;
+
+ rfkill_unregister(rf_kill);
+ rfkill_destroy(rf_kill);
+ regulator_put(rfkill_data->vcc);
+ kfree(rfkill_data);
+
+ return 0;
+}
+
+static struct platform_driver rfkill_regulator_driver = {
+ .probe = rfkill_regulator_probe,
+ .remove = __devexit_p(rfkill_regulator_remove),
+ .driver = {
+ .name = "rfkill-regulator",
+ .owner = THIS_MODULE,
+ },
+};
+
+static int __init rfkill_regulator_init(void)
+{
+ return platform_driver_register(&rfkill_regulator_driver);
+}
+module_init(rfkill_regulator_init);
+
+static void __exit rfkill_regulator_exit(void)
+{
+ platform_driver_unregister(&rfkill_regulator_driver);
+}
+module_exit(rfkill_regulator_exit);
+
+MODULE_AUTHOR("Guiming Zhuo <[email protected]>");
+MODULE_AUTHOR("Antonio Ospite <[email protected]>");
+MODULE_DESCRIPTION("Regulator consumer driver for rfkill");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:rfkill-regulator");
--
1.7.4.1
On Wed, 2011-04-06 at 16:06 +0200, Antonio Ospite wrote:
> > > + if (regulator_is_enabled(vcc)) {
> > > + dev_dbg(&pdev->dev, "Regulator already enabled\n");
> > > + rfkill_data->reg_enabled = 1;
> > > + }
> > > + rfkill_init_sw_state(rf_kill, !rfkill_data->reg_enabled);
> > > +
> > > + ret = rfkill_register(rf_kill);
> >
> > We recently had a thread about how rfkill_init_sw_state() isn't quite
> > working the right way. Also, it is indented to be used for devices that
> > keep their state over resume. I think you should remove it here and rely
> > on rfkill to sync you after registration.
> >
> > Cf. the long thread here:
> > http://thread.gmane.org/gmane.linux.acpi.devel/49577
> >
>
> Ok, but I still need to replace that call with a rfkill_set_sw_state()
> to expose the initial status of the regulator to the rfkill system,
> right?
Well, you could, but if you don't do that then the rfkill subsystem will
simply call set_block() shortly after registration to put it into the
state that it thinks it should be in, which is usually more useful.
johannes
Add a regulator consumer driver for rfkill to enable controlling radio
transmitters connected to voltage regulators using the regulator
framework.
A new "vrfkill" virtual supply is provided to use in platform code.
Signed-off-by: Guiming Zhuo <[email protected]>
Signed-off-by: Antonio Ospite <[email protected]>
---
Changes since v2:
- Fix struct field initializer syntax in example code from
include/linux/rfkill-regulator.h
This will make copying and pasting the code more straightforward.
- print the rfkill switch name in the info message when driver is
loaded successfully
Changes since v1:
- changed "voltage regulator" to "voltage regulators" in the commit
message
- drop rfkill_init_sw_state() as requested by Johannes Berg
- moved assignment fields of rfkill_data _before_ rfkill_register(),
as the latter will call .set_block (via schedule_work()) which would
find NULL pointers for the .vcc and .rf_kill fields otherwise.
This issue was masked when I was using rfkill_init_sw_state() which
was setting the persistent flag: rfkill_register() does not call
schedule_work() immediately when the persistent flag is set.
So please take another look at this part of rfkill_regulator_probe().
include/linux/rfkill-regulator.h | 48 +++++++++++
net/rfkill/Kconfig | 11 +++
net/rfkill/Makefile | 1 +
net/rfkill/rfkill-regulator.c | 164 ++++++++++++++++++++++++++++++++++++++
4 files changed, 224 insertions(+), 0 deletions(-)
create mode 100644 include/linux/rfkill-regulator.h
create mode 100644 net/rfkill/rfkill-regulator.c
diff --git a/include/linux/rfkill-regulator.h b/include/linux/rfkill-regulator.h
new file mode 100644
index 0000000..aca36bc
--- /dev/null
+++ b/include/linux/rfkill-regulator.h
@@ -0,0 +1,48 @@
+/*
+ * rfkill-regulator.c - Regulator consumer driver for rfkill
+ *
+ * Copyright (C) 2009 Guiming Zhuo <[email protected]>
+ * Copyright (C) 2011 Antonio Ospite <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#ifndef __LINUX_RFKILL_REGULATOR_H
+#define __LINUX_RFKILL_REGULATOR_H
+
+/*
+ * Use "vrfkill" as supply id when declaring the regulator consumer:
+ *
+ * static struct regulator_consumer_supply pcap_regulator_V6_consumers [] = {
+ * { .dev_name = "rfkill-regulator.0", .supply = "vrfkill" },
+ * };
+ *
+ * If you have several regulator driven rfkill, you can append a numerical id to
+ * .dev_name as done above, and use the same id when declaring the platform
+ * device:
+ *
+ * static struct rfkill_regulator_platform_data ezx_rfkill_bt_data = {
+ * .name = "ezx-bluetooth",
+ * .type = RFKILL_TYPE_BLUETOOTH,
+ * };
+ *
+ * static struct platform_device a910_rfkill = {
+ * .name = "rfkill-regulator",
+ * .id = 0,
+ * .dev = {
+ * .platform_data = &ezx_rfkill_bt_data,
+ * },
+ * };
+ */
+
+#include <linux/rfkill.h>
+
+struct rfkill_regulator_platform_data {
+ char *name; /* the name for the rfkill switch */
+ enum rfkill_type type; /* the type as specified in rfkill.h */
+};
+
+#endif /* __LINUX_RFKILL_REGULATOR_H */
diff --git a/net/rfkill/Kconfig b/net/rfkill/Kconfig
index 7fce6df..48464ca 100644
--- a/net/rfkill/Kconfig
+++ b/net/rfkill/Kconfig
@@ -22,3 +22,14 @@ config RFKILL_INPUT
depends on RFKILL
depends on INPUT = y || RFKILL = INPUT
default y if !EXPERT
+
+config RFKILL_REGULATOR
+ tristate "Generic rfkill regulator driver"
+ depends on RFKILL || !RFKILL
+ depends on REGULATOR
+ help
+ This options enable controlling radio transmitters connected to
+ voltage regulator using the regulator framework.
+
+ To compile this driver as a module, choose M here: the module will
+ be called rfkill-regulator.
diff --git a/net/rfkill/Makefile b/net/rfkill/Makefile
index 6621053..d9a5a58 100644
--- a/net/rfkill/Makefile
+++ b/net/rfkill/Makefile
@@ -5,3 +5,4 @@
rfkill-y += core.o
rfkill-$(CONFIG_RFKILL_INPUT) += input.o
obj-$(CONFIG_RFKILL) += rfkill.o
+obj-$(CONFIG_RFKILL_REGULATOR) += rfkill-regulator.o
diff --git a/net/rfkill/rfkill-regulator.c b/net/rfkill/rfkill-regulator.c
new file mode 100644
index 0000000..18dc512
--- /dev/null
+++ b/net/rfkill/rfkill-regulator.c
@@ -0,0 +1,164 @@
+/*
+ * rfkill-regulator.c - Regulator consumer driver for rfkill
+ *
+ * Copyright (C) 2009 Guiming Zhuo <[email protected]>
+ * Copyright (C) 2011 Antonio Ospite <[email protected]>
+ *
+ * Implementation inspired by leds-regulator driver.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/rfkill.h>
+#include <linux/rfkill-regulator.h>
+
+struct rfkill_regulator_data {
+ struct rfkill *rf_kill;
+ bool reg_enabled;
+
+ struct regulator *vcc;
+};
+
+static int rfkill_regulator_set_block(void *data, bool blocked)
+{
+ struct rfkill_regulator_data *rfkill_data = data;
+
+ pr_debug("%s: blocked: %d\n", __func__, blocked);
+
+ if (blocked) {
+ if (rfkill_data->reg_enabled) {
+ regulator_disable(rfkill_data->vcc);
+ rfkill_data->reg_enabled = 0;
+ }
+ } else {
+ if (!rfkill_data->reg_enabled) {
+ regulator_enable(rfkill_data->vcc);
+ rfkill_data->reg_enabled = 1;
+ }
+ }
+
+ pr_debug("%s: regulator_is_enabled after set_block: %d\n", __func__,
+ regulator_is_enabled(rfkill_data->vcc));
+
+ return 0;
+}
+
+struct rfkill_ops rfkill_regulator_ops = {
+ .set_block = rfkill_regulator_set_block,
+};
+
+static int __devinit rfkill_regulator_probe(struct platform_device *pdev)
+{
+ struct rfkill_regulator_platform_data *pdata = pdev->dev.platform_data;
+ struct rfkill_regulator_data *rfkill_data;
+ struct regulator *vcc;
+ struct rfkill *rf_kill;
+ int ret = 0;
+
+ if (pdata == NULL) {
+ dev_err(&pdev->dev, "no platform data\n");
+ return -ENODEV;
+ }
+
+ if (pdata->name == NULL || pdata->type == 0) {
+ dev_err(&pdev->dev, "invalid name or type in platform data\n");
+ return -EINVAL;
+ }
+
+ vcc = regulator_get_exclusive(&pdev->dev, "vrfkill");
+ if (IS_ERR(vcc)) {
+ dev_err(&pdev->dev, "Cannot get vcc for %s\n", pdata->name);
+ ret = PTR_ERR(vcc);
+ goto out;
+ }
+
+ rfkill_data = kzalloc(sizeof(*rfkill_data), GFP_KERNEL);
+ if (rfkill_data == NULL) {
+ ret = -ENOMEM;
+ goto err_data_alloc;
+ }
+
+ rf_kill = rfkill_alloc(pdata->name, &pdev->dev,
+ pdata->type,
+ &rfkill_regulator_ops, rfkill_data);
+ if (rf_kill == NULL) {
+ dev_err(&pdev->dev, "Cannot alloc rfkill device\n");
+ ret = -ENOMEM;
+ goto err_rfkill_alloc;
+ }
+
+ if (regulator_is_enabled(vcc)) {
+ dev_dbg(&pdev->dev, "Regulator already enabled\n");
+ rfkill_data->reg_enabled = 1;
+ }
+ rfkill_data->vcc = vcc;
+ rfkill_data->rf_kill = rf_kill;
+
+ ret = rfkill_register(rf_kill);
+ if (ret) {
+ dev_err(&pdev->dev, "Cannot register rfkill device\n");
+ goto err_rfkill_register;
+ }
+
+ platform_set_drvdata(pdev, rfkill_data);
+ dev_info(&pdev->dev, "%s initialized\n", pdata->name);
+
+ return 0;
+
+err_rfkill_register:
+ rfkill_destroy(rf_kill);
+err_rfkill_alloc:
+ kfree(rfkill_data);
+err_data_alloc:
+ regulator_put(vcc);
+out:
+ return ret;
+}
+
+static int __devexit rfkill_regulator_remove(struct platform_device *pdev)
+{
+ struct rfkill_regulator_data *rfkill_data = platform_get_drvdata(pdev);
+ struct rfkill *rf_kill = rfkill_data->rf_kill;
+
+ rfkill_unregister(rf_kill);
+ rfkill_destroy(rf_kill);
+ regulator_put(rfkill_data->vcc);
+ kfree(rfkill_data);
+
+ return 0;
+}
+
+static struct platform_driver rfkill_regulator_driver = {
+ .probe = rfkill_regulator_probe,
+ .remove = __devexit_p(rfkill_regulator_remove),
+ .driver = {
+ .name = "rfkill-regulator",
+ .owner = THIS_MODULE,
+ },
+};
+
+static int __init rfkill_regulator_init(void)
+{
+ return platform_driver_register(&rfkill_regulator_driver);
+}
+module_init(rfkill_regulator_init);
+
+static void __exit rfkill_regulator_exit(void)
+{
+ platform_driver_unregister(&rfkill_regulator_driver);
+}
+module_exit(rfkill_regulator_exit);
+
+MODULE_AUTHOR("Guiming Zhuo <[email protected]>");
+MODULE_AUTHOR("Antonio Ospite <[email protected]>");
+MODULE_DESCRIPTION("Regulator consumer driver for rfkill");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:rfkill-regulator");
--
1.7.4.1
On Wed, 2011-04-06 at 16:21 +0200, Johannes Berg wrote:
> On Wed, 2011-04-06 at 23:11 +0900, Mark Brown wrote:
> > On Wed, Apr 06, 2011 at 11:21:19AM +0200, Antonio Ospite wrote:
> >
> > > + tristate "Generic rfkill regulator driver"
> > > + depends on RFKILL || !RFKILL
> >
> > That looks *odd*.
>
> That's normal for rfkill -- if RFKILL==n then this can be anything since
> the rfkill API goes all no-op inlines, but if RFKILL==m then this can't
> be ==y. "depends on !RFKILL" covers the former, "depends on RFKILL" the
> latter.
But doesn't
depends on RFKILL || !RFKILL
always evaluate to true when running "make *config"? (Even if RFKILL is
an unknown symbol when that expression is parsed!)
I'd say that dependencies such as this one might as well be dropped from
their Kconfig file.
Paul Bolle
On Wed, 2011-04-13 at 10:44 +0200, Antonio Ospite wrote:
> On Tue, 12 Apr 2011 13:44:02 +0200
> Johannes Berg <[email protected]> wrote:
>
> > On Tue, 2011-04-12 at 13:41 +0200, Johannes Berg wrote:
> >
> > > > + * static struct regulator_consumer_supply pcap_regulator_V6_consumers [] = {
> > > > + * { .dev_name = "rfkill-regulator.0", supply = "vrfkill" },
> > > > + * };
> > >
> > > It's a comment, but it should be .supply = (missing the .)
> > >
>
> well spotted, I'll fix this.
This is the comment I was referring to -- I got confused and thought
this was a struct rfkill_regulator_platform_data.
> That's how the regulator framework works, I know Mark already replied to
> you but I try to elaborate more for the records and to organize my
> thoughts about that:
[snip explanation]
Thanks, ok, I'm just not familiar with the regulator framework and got
confused about the various structs involved.
So the only thing I see wrong is the missing . above, not sure if you
care enough to resend :-)
johannes
On Wed, 06 Apr 2011 16:09:28 +0200
Johannes Berg <[email protected]> wrote:
> On Wed, 2011-04-06 at 16:06 +0200, Antonio Ospite wrote:
>
> > > > + if (regulator_is_enabled(vcc)) {
> > > > + dev_dbg(&pdev->dev, "Regulator already enabled\n");
> > > > + rfkill_data->reg_enabled = 1;
> > > > + }
> > > > + rfkill_init_sw_state(rf_kill, !rfkill_data->reg_enabled);
> > > > +
> > > > + ret = rfkill_register(rf_kill);
> > >
> > > We recently had a thread about how rfkill_init_sw_state() isn't quite
> > > working the right way. Also, it is indented to be used for devices that
> > > keep their state over resume. I think you should remove it here and rely
> > > on rfkill to sync you after registration.
> > >
> > > Cf. the long thread here:
> > > http://thread.gmane.org/gmane.linux.acpi.devel/49577
> > >
> >
> > Ok, but I still need to replace that call with a rfkill_set_sw_state()
> > to expose the initial status of the regulator to the rfkill system,
> > right?
>
> Well, you could, but if you don't do that then the rfkill subsystem will
> simply call set_block() shortly after registration to put it into the
> state that it thinks it should be in, which is usually more useful.
>
I see, let's just drop rfkill_init_sw_state() then.
Regards,
Antonio
--
Antonio Ospite
http://ao2.it
PGP public key ID: 0x4553B001
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
On Wed, 2011-04-06 at 22:17 +0200, Johannes Berg wrote:
> On Wed, 2011-04-06 at 22:15 +0200, Johannes Berg wrote:
> > On Wed, 2011-04-06 at 22:10 +0200, Paul Bolle wrote:
> > > On Wed, 2011-04-06 at 14:38 -0400, John W. Linville wrote:
> > > > The syntax may seem strange,
> > >
> > > It does!
> > >
> > > > but basically it just says "don't let me by y if RFKILL is m"
> > >
> > > ... but, besides that, I can be any value. So in effect it's shorthand
> > > for
> > > depends on RFKILL=y || RFKILL=m && m || RFKILL=n
> > >
> > > (which actually looks equally strange). Is that correct?
> >
> > I don't think it is, I believe that an expression like "RFKILL=y" has a
> > bool type, and a tristate type value that depends on a bool type can
> > still take the value m.
>
> Err, which is of course perfectly fine since if RFKILL is built in this
> can be any value, and in the RFKILL=m case you force it to m by making
> it depend on m directly. So yes, you're right.
Whoops ... sorry about the talking to self ...
I still think the original is easier to understand. After all, just
depends on RFKILL
is trivial to understand even with tristates. And knowing that RFKILL
will provide no-op inlines when it is unconfigured, you add
depends on !RFKILL
for that case.
johannes
On Don, 2011-04-07 at 12:09 +0200, Johannes Berg wrote:
> On Thu, 2011-04-07 at 12:01 +0200, Bernd Petrovitsch wrote:
> > On Mit, 2011-04-06 at 20:46 +0200, Johannes Berg wrote:
> > > On Wed, 2011-04-06 at 20:12 +0200, Paul Bolle wrote:
> > [...]
> > > > But doesn't
> > > > depends on RFKILL || !RFKILL
> > > >
> > > > always evaluate to true when running "make *config"? (Even if RFKILL is
> > > > an unknown symbol when that expression is parsed!)
> > >
> > > No, it will not, you're forgetting that these things are tristate.
> >
> > Boolean operators for tristate logic isn't intuitive at all IMHO.
>
> *shrug*. You're free to propose patches to the kconfig system to make it
> more intuitive. :-)
FullACK;-)
But no intuitive tristate logic operators come to my mind (otherwise I
would have mentioned them above).
And there are more logic implications in "depends on RFKILL".
Hmm, I have to think more about it ....
Bernd
--
Bernd Petrovitsch Email : [email protected]
LUGA : http://www.luga.at
On Mit, 2011-04-06 at 20:46 +0200, Johannes Berg wrote:
> On Wed, 2011-04-06 at 20:12 +0200, Paul Bolle wrote:
[...]
> > But doesn't
> > depends on RFKILL || !RFKILL
> >
> > always evaluate to true when running "make *config"? (Even if RFKILL is
> > an unknown symbol when that expression is parsed!)
>
> No, it will not, you're forgetting that these things are tristate.
Boolean operators for tristate logic isn't intuitive at all IMHO.
Bernd
--
Bernd Petrovitsch Email : [email protected]
LUGA : http://www.luga.at
On Wed, 2011-04-06 at 23:11 +0900, Mark Brown wrote:
> On Wed, Apr 06, 2011 at 11:21:19AM +0200, Antonio Ospite wrote:
>
> > + tristate "Generic rfkill regulator driver"
> > + depends on RFKILL || !RFKILL
>
> That looks *odd*.
That's normal for rfkill -- if RFKILL==n then this can be anything since
the rfkill API goes all no-op inlines, but if RFKILL==m then this can't
be ==y. "depends on !RFKILL" covers the former, "depends on RFKILL" the
latter.
johannes
Hi Antonio,
於 三，2011-04-06 於 16:24 +0200，Antonio Ospite 提到：
> On Wed, 06 Apr 2011 16:09:28 +0200
> Johannes Berg <[email protected]> wrote:
>
> > On Wed, 2011-04-06 at 16:06 +0200, Antonio Ospite wrote:
> >
> > > > > + if (regulator_is_enabled(vcc)) {
> > > > > + dev_dbg(&pdev->dev, "Regulator already enabled\n");
> > > > > + rfkill_data->reg_enabled = 1;
> > > > > + }
> > > > > + rfkill_init_sw_state(rf_kill, !rfkill_data->reg_enabled);
> > > > > +
> > > > > + ret = rfkill_register(rf_kill);
> > > >
> > > > We recently had a thread about how rfkill_init_sw_state() isn't quite
> > > > working the right way. Also, it is indented to be used for devices that
> > > > keep their state over resume. I think you should remove it here and rely
> > > > on rfkill to sync you after registration.
> > > >
> > > > Cf. the long thread here:
> > > > http://thread.gmane.org/gmane.linux.acpi.devel/49577
> > > >
> > >
> > > Ok, but I still need to replace that call with a rfkill_set_sw_state()
> > > to expose the initial status of the regulator to the rfkill system,
> > > right?
> >
> > Well, you could, but if you don't do that then the rfkill subsystem will
> > simply call set_block() shortly after registration to put it into the
> > state that it thinks it should be in, which is usually more useful.
> >
>
> I see, let's just drop rfkill_init_sw_state() then.
>
> Regards,
> Antonio
>
Like Johannes's comment, the rfkill_init_sw_state is a bit tricky
especially when RFKILL_INPUT enabled. The rfkill_init_sw_state will
replicate the state to device global state, then rfkill will replicate
it to other killswitch.
If you want to use rfkill_init_sw_state to set rfkill initial state when
driver probed, then I suggest you need test it when:
- RFKILL_INPUT enabled
and
- when device initial state is disabled(BLOCKED)
If you want to maintain the rfkill initial state by your self, you can
reference this patch:
http://git.kernel.org/?p=linux/kernel/git/mjg59/platform-drivers-x86.git;a=commitdiff;h=8215af019040ce9182728afee9642d8fdeb17f59
The patch set intial state by rfkill_set_sw_state after rfkill register,
and don't touch the BIOS (firmware?) state in set_block until driver
probe finished.
Thank's a lot!
Joey Lee
On Tue, Apr 12, 2011 at 01:44:02PM +0200, Johannes Berg wrote:
> On Tue, 2011-04-12 at 13:41 +0200, Johannes Berg wrote:
> > > + if (pdata->name == NULL || pdata->type == 0) {
> > > + dev_err(&pdev->dev, "invalid name or type in platform data\n");
> > > + return -EINVAL;
> > > + }
> > > + vcc = regulator_get_exclusive(&pdev->dev, "vrfkill");
> > Wasn't that supposed to use pdata->supply? Actually, there's no member
> > "supply" in the struct?
No, if you're passing supply names through platform data something has
gone wrong - that's a big no no.
> Oh wait, I think I just misunderstood how this works. But if the name is
> "vrfkill" how does that really work with multiple instances?
That's what the struct device is there for. The names are mapped into
physical regulators relative to the device.
On Wed, 2011-04-06 at 22:10 +0200, Paul Bolle wrote:
> On Wed, 2011-04-06 at 14:38 -0400, John W. Linville wrote:
> > The syntax may seem strange,
>
> It does!
>
> > but basically it just says "don't let me by y if RFKILL is m"
>
> ... but, besides that, I can be any value. So in effect it's shorthand
> for
> depends on RFKILL=y || RFKILL=m && m || RFKILL=n
>
> (which actually looks equally strange). Is that correct?
I don't think it is, I believe that an expression like "RFKILL=y" has a
bool type, and a tristate type value that depends on a bool type can
still take the value m.
johannes
On Wed, 2011-04-06 at 22:15 +0200, Johannes Berg wrote:
> On Wed, 2011-04-06 at 22:10 +0200, Paul Bolle wrote:
> > On Wed, 2011-04-06 at 14:38 -0400, John W. Linville wrote:
> > > The syntax may seem strange,
> >
> > It does!
> >
> > > but basically it just says "don't let me by y if RFKILL is m"
> >
> > ... but, besides that, I can be any value. So in effect it's shorthand
> > for
> > depends on RFKILL=y || RFKILL=m && m || RFKILL=n
> >
> > (which actually looks equally strange). Is that correct?
>
> I don't think it is, I believe that an expression like "RFKILL=y" has a
> bool type, and a tristate type value that depends on a bool type can
> still take the value m.
Err, which is of course perfectly fine since if RFKILL is built in this
can be any value, and in the RFKILL=m case you force it to m by making
it depend on m directly. So yes, you're right.
johannes
On Wed, 2011-04-13 at 21:40 +0200, Antonio Ospite wrote:
> +static int __devinit rfkill_regulator_probe(struct platform_device *pdev)
...
> + platform_set_drvdata(pdev, rfkill_data);
> +static int __devexit rfkill_regulator_remove(struct platform_device *pdev)
> +{
> + struct rfkill_regulator_data *rfkill_data = platform_get_drvdata(pdev);
> + struct rfkill *rf_kill = rfkill_data->rf_kill;
> +
> + rfkill_unregister(rf_kill);
> + rfkill_destroy(rf_kill);
> + regulator_put(rfkill_data->vcc);
> + kfree(rfkill_data);
> +
> + return 0;
> +}
Should remove do platform_set_drvdata(pdev, NULL)?
In any case, that's the only thing I see now and I did read the code
carefully, so if that's not necessary:
Reviewed-by: Johannes Berg <[email protected]>
Otherwise, feel free to include that in v4.
johannes
On Tue, 2011-04-12 at 13:41 +0200, Johannes Berg wrote:
> > + * static struct regulator_consumer_supply pcap_regulator_V6_consumers [] = {
> > + * { .dev_name = "rfkill-regulator.0", supply = "vrfkill" },
> > + * };
>
> It's a comment, but it should be .supply = (missing the .)
>
> > + if (pdata->name == NULL || pdata->type == 0) {
> > + dev_err(&pdev->dev, "invalid name or type in platform data\n");
> > + return -EINVAL;
> > + }
> > +
> > + vcc = regulator_get_exclusive(&pdev->dev, "vrfkill");
>
> Wasn't that supposed to use pdata->supply? Actually, there's no member
> "supply" in the struct?
Oh wait, I think I just misunderstood how this works. But if the name is
"vrfkill" how does that really work with multiple instances?
johannes
On Thu, 2011-04-07 at 12:01 +0200, Bernd Petrovitsch wrote:
> On Mit, 2011-04-06 at 20:46 +0200, Johannes Berg wrote:
> > On Wed, 2011-04-06 at 20:12 +0200, Paul Bolle wrote:
> [...]
> > > But doesn't
> > > depends on RFKILL || !RFKILL
> > >
> > > always evaluate to true when running "make *config"? (Even if RFKILL is
> > > an unknown symbol when that expression is parsed!)
> >
> > No, it will not, you're forgetting that these things are tristate.
>
> Boolean operators for tristate logic isn't intuitive at all IMHO.
*shrug*. You're free to propose patches to the kconfig system to make it
more intuitive. :-)
johannes
On Wed, Apr 06, 2011 at 11:21:19AM +0200, Antonio Ospite wrote:
> + tristate "Generic rfkill regulator driver"
> + depends on RFKILL || !RFKILL
That looks *odd*. Otherwise this looks fine from a regulator API point
of view. You use an exclusive get() so you could get away without
remembering the enable state as nothing else could hold the device open
but there's no harm in doing so and it's defensive against silly
constraints that force the regulator on.
On Wed, 13 Apr 2011 21:53:03 +0200
Johannes Berg <[email protected]> wrote:
> On Wed, 2011-04-13 at 21:40 +0200, Antonio Ospite wrote:
>
> > +static int __devinit rfkill_regulator_probe(struct platform_device *pdev)
>
> ...
>
> > + platform_set_drvdata(pdev, rfkill_data);
>
> > +static int __devexit rfkill_regulator_remove(struct platform_device *pdev)
> > +{
> > + struct rfkill_regulator_data *rfkill_data = platform_get_drvdata(pdev);
> > + struct rfkill *rf_kill = rfkill_data->rf_kill;
> > +
> > + rfkill_unregister(rf_kill);
> > + rfkill_destroy(rf_kill);
> > + regulator_put(rfkill_data->vcc);
> > + kfree(rfkill_data);
> > +
> > + return 0;
> > +}
>
> Should remove do platform_set_drvdata(pdev, NULL)?
>
AFAICS this is not strictly necessary because we never check for NULL
here and we are setting drvdata again in _probe() each time the module
is loaded anyways. If it is considered a good practice for symmetry
reasons then I'll add it, no problem. Does anyone has comments on that?
> In any case, that's the only thing I see now and I did read the code
> carefully, so if that's not necessary:
>
> Reviewed-by: Johannes Berg <[email protected]>
>
> Otherwise, feel free to include that in v4.
>
> johannes
>
Thanks for taking the time to look at the code Johannes.
Regards,
Antonio
--
Antonio Ospite
http://ao2.it
PGP public key ID: 0x4553B001
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
On Tue, 12 Apr 2011 13:44:02 +0200
Johannes Berg <[email protected]> wrote:
> On Tue, 2011-04-12 at 13:41 +0200, Johannes Berg wrote:
>
> > > + * static struct regulator_consumer_supply pcap_regulator_V6_consumers [] = {
> > > + * { .dev_name = "rfkill-regulator.0", supply = "vrfkill" },
> > > + * };
> >
> > It's a comment, but it should be .supply = (missing the .)
> >
well spotted, I'll fix this.
> > > + if (pdata->name == NULL || pdata->type == 0) {
> > > + dev_err(&pdev->dev, "invalid name or type in platform data\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + vcc = regulator_get_exclusive(&pdev->dev, "vrfkill");
> >
> > Wasn't that supposed to use pdata->supply? Actually, there's no member
> > "supply" in the struct?
>
> Oh wait, I think I just misunderstood how this works. But if the name is
> "vrfkill" how does that really work with multiple instances?
>
That's how the regulator framework works, I know Mark already replied to
you but I try to elaborate more for the records and to organize my
thoughts about that:
- In the consumers for the regulator we choose the virtual supply,
"vrfkill" in this case, and which driver is going to use it.
- Wrt. to multiple instances, they are distinguished using device ids.
When we set consumers for a physical regulator we can tell: device
"rfkill-regulator.1" will call this regulator "vrfkill" and we
declare the relative rfkill-regulator platform device with .id=1,
this way the regulator framework knows what physical regulator to
return when asked for vrfkill _from_ a rfkill-regulator platform
device instance with .id==1
I hope I am not introducing any inaccuracies :)
A v3 of the patch is on its way.
Thanks,
Antonio
--
Antonio Ospite
http://ao2.it
PGP public key ID: 0x4553B001
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
On Wed, 2011-04-06 at 16:29 +0200, Antonio Ospite wrote:
> On Wed, 6 Apr 2011 23:11:33 +0900
> Mark Brown <[email protected]> wrote:
>
> > On Wed, Apr 06, 2011 at 11:21:19AM +0200, Antonio Ospite wrote:
> >
> > > + tristate "Generic rfkill regulator driver"
> > > + depends on RFKILL || !RFKILL
> >
> > That looks *odd*.
>
> Taken from Documentation/rfkill.txt section 3. Kernel API.
> I guess I can drop it if we want to be stricter and just require RFKILL
> to be enabled. Johannes?
I guess it depends on what you're looking to do. Since all you implement
is set_block() you might very well not need to be able to have this if
nothing is ever going to invoke set_block(), in which case you can do
"depends on RFKILL".
The reason for this usually is that a driver, like a wireless driver,
should work even if there's no rfkill API available, but it shouldn't
need to put #ifdefs into the code itself.
johannes
On Wed, 2011-04-06 at 11:21 +0200, Antonio Ospite wrote:
> + if (regulator_is_enabled(vcc)) {
> + dev_dbg(&pdev->dev, "Regulator already enabled\n");
> + rfkill_data->reg_enabled = 1;
> + }
> + rfkill_init_sw_state(rf_kill, !rfkill_data->reg_enabled);
> +
> + ret = rfkill_register(rf_kill);
We recently had a thread about how rfkill_init_sw_state() isn't quite
working the right way. Also, it is indented to be used for devices that
keep their state over resume. I think you should remove it here and rely
on rfkill to sync you after registration.
Cf. the long thread here:
http://thread.gmane.org/gmane.linux.acpi.devel/49577
Other than that, no comments from me from an rfkill POV.
johannes
On Wed, 2011-04-06 at 20:12 +0200, Paul Bolle wrote:
> On Wed, 2011-04-06 at 16:21 +0200, Johannes Berg wrote:
> > On Wed, 2011-04-06 at 23:11 +0900, Mark Brown wrote:
> > > On Wed, Apr 06, 2011 at 11:21:19AM +0200, Antonio Ospite wrote:
> > >
> > > > + tristate "Generic rfkill regulator driver"
> > > > + depends on RFKILL || !RFKILL
> > >
> > > That looks *odd*.
> >
> > That's normal for rfkill -- if RFKILL==n then this can be anything since
> > the rfkill API goes all no-op inlines, but if RFKILL==m then this can't
> > be ==y. "depends on !RFKILL" covers the former, "depends on RFKILL" the
> > latter.
>
> But doesn't
> depends on RFKILL || !RFKILL
>
> always evaluate to true when running "make *config"? (Even if RFKILL is
> an unknown symbol when that expression is parsed!)
No, it will not, you're forgetting that these things are tristate.
johannes
On Tue, 12 Apr 2011 13:41:28 +0200
Johannes Berg <[email protected]> wrote:
> > + dev_info(&pdev->dev, "initialized\n");
>
> Is that message really useful?
>
I can print the pdata->name here as well as an excuse to keep it,
reality is that I just like visual feedback in dmesg when a driver has
been loaded.
> Other than that, looks good to me.
>
> johannes
>
In another message you said that the comment in rfkill-regulator.h
still seemed a little wrong to you, would you elaborate more about what
you were referring to?
Thanks,
Antonio
--
Antonio Ospite
http://ao2.it
PGP public key ID: 0x4553B001
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
On Wed, 2011-04-06 at 14:38 -0400, John W. Linville wrote:
> The syntax may seem strange,
It does!
> but basically it just says "don't let me by y if RFKILL is m"
... but, besides that, I can be any value. So in effect it's shorthand
for
depends on RFKILL=y || RFKILL=m && m || RFKILL=n
(which actually looks equally strange). Is that correct?
Anyhow, perhaps this should be added to the "Kconfig hints" in
Documentation/kbuild/kconfig-language.txt.
Paul Bolle
Add a regulator consumer driver for rfkill to enable controlling radio
transmitters connected to voltage regulators using the regulator
framework.
A new "vrfkill" virtual supply is provided to use in platform code.
Signed-off-by: Guiming Zhuo <[email protected]>
Signed-off-by: Antonio Ospite <[email protected]>
---
Changes since v1:
- changed "voltage regulator" to "voltage regulators" in the commit
message
- drop rfkill_init_sw_state() as requested by Johannes Berg
- moved assignment fields of rfkill_data _before_ rfkill_register(),
as the latter will call .set_block (via schedule_work()) which would
find NULL pointers for the .vcc and .rf_kill fields otherwise.
This issue was masked when I was using rfkill_init_sw_state() which
was setting the persistent flag: rfkill_register() does not call
schedule_work() immediately when the persistent flag is set.
So please take another look at this part of rfkill_regulator_probe().
Mark, I left in the RFKILL || !RFKILL part.
If there are no more comments, who is going to merge the driver, Johannes?
Thanks,
Antonio Ospite
http://ao2.it
include/linux/rfkill-regulator.h | 48 +++++++++++
net/rfkill/Kconfig | 11 +++
net/rfkill/Makefile | 1 +
net/rfkill/rfkill-regulator.c | 164 ++++++++++++++++++++++++++++++++++++++
4 files changed, 224 insertions(+), 0 deletions(-)
create mode 100644 include/linux/rfkill-regulator.h
create mode 100644 net/rfkill/rfkill-regulator.c
diff --git a/include/linux/rfkill-regulator.h b/include/linux/rfkill-regulator.h
new file mode 100644
index 0000000..4aaef9c
--- /dev/null
+++ b/include/linux/rfkill-regulator.h
@@ -0,0 +1,48 @@
+/*
+ * rfkill-regulator.c - Regulator consumer driver for rfkill
+ *
+ * Copyright (C) 2009 Guiming Zhuo <[email protected]>
+ * Copyright (C) 2011 Antonio Ospite <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#ifndef __LINUX_RFKILL_REGULATOR_H
+#define __LINUX_RFKILL_REGULATOR_H
+
+/*
+ * Use "vrfkill" as supply id when declaring the regulator consumer:
+ *
+ * static struct regulator_consumer_supply pcap_regulator_V6_consumers [] = {
+ * { .dev_name = "rfkill-regulator.0", supply = "vrfkill" },
+ * };
+ *
+ * If you have several regulator driven rfkill, you can append a numerical id to
+ * .dev_name as done above, and use the same id when declaring the platform
+ * device:
+ *
+ * static struct rfkill_regulator_platform_data ezx_rfkill_bt_data = {
+ * .name = "ezx-bluetooth",
+ * .type = RFKILL_TYPE_BLUETOOTH,
+ * };
+ *
+ * static struct platform_device a910_rfkill = {
+ * .name = "rfkill-regulator",
+ * .id = 0,
+ * .dev = {
+ * .platform_data = &ezx_rfkill_bt_data,
+ * },
+ * };
+ */
+
+#include <linux/rfkill.h>
+
+struct rfkill_regulator_platform_data {
+ char *name; /* the name for the rfkill switch */
+ enum rfkill_type type; /* the type as specified in rfkill.h */
+};
+
+#endif /* __LINUX_RFKILL_REGULATOR_H */
diff --git a/net/rfkill/Kconfig b/net/rfkill/Kconfig
index 7fce6df..48464ca 100644
--- a/net/rfkill/Kconfig
+++ b/net/rfkill/Kconfig
@@ -22,3 +22,14 @@ config RFKILL_INPUT
depends on RFKILL
depends on INPUT = y || RFKILL = INPUT
default y if !EXPERT
+
+config RFKILL_REGULATOR
+ tristate "Generic rfkill regulator driver"
+ depends on RFKILL || !RFKILL
+ depends on REGULATOR
+ help
+ This options enable controlling radio transmitters connected to
+ voltage regulator using the regulator framework.
+
+ To compile this driver as a module, choose M here: the module will
+ be called rfkill-regulator.
diff --git a/net/rfkill/Makefile b/net/rfkill/Makefile
index 6621053..d9a5a58 100644
--- a/net/rfkill/Makefile
+++ b/net/rfkill/Makefile
@@ -5,3 +5,4 @@
rfkill-y += core.o
rfkill-$(CONFIG_RFKILL_INPUT) += input.o
obj-$(CONFIG_RFKILL) += rfkill.o
+obj-$(CONFIG_RFKILL_REGULATOR) += rfkill-regulator.o
diff --git a/net/rfkill/rfkill-regulator.c b/net/rfkill/rfkill-regulator.c
new file mode 100644
index 0000000..e99567f
--- /dev/null
+++ b/net/rfkill/rfkill-regulator.c
@@ -0,0 +1,164 @@
+/*
+ * rfkill-regulator.c - Regulator consumer driver for rfkill
+ *
+ * Copyright (C) 2009 Guiming Zhuo <[email protected]>
+ * Copyright (C) 2011 Antonio Ospite <[email protected]>
+ *
+ * Implementation inspired by leds-regulator driver.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/rfkill.h>
+#include <linux/rfkill-regulator.h>
+
+struct rfkill_regulator_data {
+ struct rfkill *rf_kill;
+ bool reg_enabled;
+
+ struct regulator *vcc;
+};
+
+static int rfkill_regulator_set_block(void *data, bool blocked)
+{
+ struct rfkill_regulator_data *rfkill_data = data;
+
+ pr_debug("%s: blocked: %d\n", __func__, blocked);
+
+ if (blocked) {
+ if (rfkill_data->reg_enabled) {
+ regulator_disable(rfkill_data->vcc);
+ rfkill_data->reg_enabled = 0;
+ }
+ } else {
+ if (!rfkill_data->reg_enabled) {
+ regulator_enable(rfkill_data->vcc);
+ rfkill_data->reg_enabled = 1;
+ }
+ }
+
+ pr_debug("%s: regulator_is_enabled after set_block: %d\n", __func__,
+ regulator_is_enabled(rfkill_data->vcc));
+
+ return 0;
+}
+
+struct rfkill_ops rfkill_regulator_ops = {
+ .set_block = rfkill_regulator_set_block,
+};
+
+static int __devinit rfkill_regulator_probe(struct platform_device *pdev)
+{
+ struct rfkill_regulator_platform_data *pdata = pdev->dev.platform_data;
+ struct rfkill_regulator_data *rfkill_data;
+ struct regulator *vcc;
+ struct rfkill *rf_kill;
+ int ret = 0;
+
+ if (pdata == NULL) {
+ dev_err(&pdev->dev, "no platform data\n");
+ return -ENODEV;
+ }
+
+ if (pdata->name == NULL || pdata->type == 0) {
+ dev_err(&pdev->dev, "invalid name or type in platform data\n");
+ return -EINVAL;
+ }
+
+ vcc = regulator_get_exclusive(&pdev->dev, "vrfkill");
+ if (IS_ERR(vcc)) {
+ dev_err(&pdev->dev, "Cannot get vcc for %s\n", pdata->name);
+ ret = PTR_ERR(vcc);
+ goto out;
+ }
+
+ rfkill_data = kzalloc(sizeof(*rfkill_data), GFP_KERNEL);
+ if (rfkill_data == NULL) {
+ ret = -ENOMEM;
+ goto err_data_alloc;
+ }
+
+ rf_kill = rfkill_alloc(pdata->name, &pdev->dev,
+ pdata->type,
+ &rfkill_regulator_ops, rfkill_data);
+ if (rf_kill == NULL) {
+ dev_err(&pdev->dev, "Cannot alloc rfkill device\n");
+ ret = -ENOMEM;
+ goto err_rfkill_alloc;
+ }
+
+ if (regulator_is_enabled(vcc)) {
+ dev_dbg(&pdev->dev, "Regulator already enabled\n");
+ rfkill_data->reg_enabled = 1;
+ }
+ rfkill_data->vcc = vcc;
+ rfkill_data->rf_kill = rf_kill;
+
+ ret = rfkill_register(rf_kill);
+ if (ret) {
+ dev_err(&pdev->dev, "Cannot register rfkill device\n");
+ goto err_rfkill_register;
+ }
+
+ platform_set_drvdata(pdev, rfkill_data);
+ dev_info(&pdev->dev, "initialized\n");
+
+ return 0;
+
+err_rfkill_register:
+ rfkill_destroy(rf_kill);
+err_rfkill_alloc:
+ kfree(rfkill_data);
+err_data_alloc:
+ regulator_put(vcc);
+out:
+ return ret;
+}
+
+static int __devexit rfkill_regulator_remove(struct platform_device *pdev)
+{
+ struct rfkill_regulator_data *rfkill_data = platform_get_drvdata(pdev);
+ struct rfkill *rf_kill = rfkill_data->rf_kill;
+
+ rfkill_unregister(rf_kill);
+ rfkill_destroy(rf_kill);
+ regulator_put(rfkill_data->vcc);
+ kfree(rfkill_data);
+
+ return 0;
+}
+
+static struct platform_driver rfkill_regulator_driver = {
+ .probe = rfkill_regulator_probe,
+ .remove = __devexit_p(rfkill_regulator_remove),
+ .driver = {
+ .name = "rfkill-regulator",
+ .owner = THIS_MODULE,
+ },
+};
+
+static int __init rfkill_regulator_init(void)
+{
+ return platform_driver_register(&rfkill_regulator_driver);
+}
+module_init(rfkill_regulator_init);
+
+static void __exit rfkill_regulator_exit(void)
+{
+ platform_driver_unregister(&rfkill_regulator_driver);
+}
+module_exit(rfkill_regulator_exit);
+
+MODULE_AUTHOR("Guiming Zhuo <[email protected]>");
+MODULE_AUTHOR("Antonio Ospite <[email protected]>");
+MODULE_DESCRIPTION("Regulator consumer driver for rfkill");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:rfkill-regulator");
--
1.7.4.1
On Wed, Apr 06, 2011 at 08:12:22PM +0200, Paul Bolle wrote:
> On Wed, 2011-04-06 at 16:21 +0200, Johannes Berg wrote:
> > On Wed, 2011-04-06 at 23:11 +0900, Mark Brown wrote:
> > > On Wed, Apr 06, 2011 at 11:21:19AM +0200, Antonio Ospite wrote:
> > >
> > > > + tristate "Generic rfkill regulator driver"
> > > > + depends on RFKILL || !RFKILL
> > >
> > > That looks *odd*.
> >
> > That's normal for rfkill -- if RFKILL==n then this can be anything since
> > the rfkill API goes all no-op inlines, but if RFKILL==m then this can't
> > be ==y. "depends on !RFKILL" covers the former, "depends on RFKILL" the
> > latter.
>
> But doesn't
> depends on RFKILL || !RFKILL
>
> always evaluate to true when running "make *config"? (Even if RFKILL is
> an unknown symbol when that expression is parsed!)
>
> I'd say that dependencies such as this one might as well be dropped from
> their Kconfig file.
The syntax may seem strange, but basically it just says "don't let
me by y if RFKILL is m".
--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.
On Wed, 06 Apr 2011 11:29:38 +0200
Johannes Berg <[email protected]> wrote:
> On Wed, 2011-04-06 at 11:21 +0200, Antonio Ospite wrote:
>
> > + if (regulator_is_enabled(vcc)) {
> > + dev_dbg(&pdev->dev, "Regulator already enabled\n");
> > + rfkill_data->reg_enabled = 1;
> > + }
> > + rfkill_init_sw_state(rf_kill, !rfkill_data->reg_enabled);
> > +
> > + ret = rfkill_register(rf_kill);
>
> We recently had a thread about how rfkill_init_sw_state() isn't quite
> working the right way. Also, it is indented to be used for devices that
> keep their state over resume. I think you should remove it here and rely
> on rfkill to sync you after registration.
>
> Cf. the long thread here:
> http://thread.gmane.org/gmane.linux.acpi.devel/49577
>
Ok, but I still need to replace that call with a rfkill_set_sw_state()
to expose the initial status of the regulator to the rfkill system,
right?
> Other than that, no comments from me from an rfkill POV.
>
> johannes
>
Thanks I am waiting a couple of days before sending a v2.
Regards,
Antonio
--
Antonio Ospite
http://ao2.it
PGP public key ID: 0x4553B001
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
On Tue, 2011-04-12 at 08:15 -0700, Mark Brown wrote:
> On Tue, Apr 12, 2011 at 01:44:02PM +0200, Johannes Berg wrote:
> > On Tue, 2011-04-12 at 13:41 +0200, Johannes Berg wrote:
>
> > > > + if (pdata->name == NULL || pdata->type == 0) {
> > > > + dev_err(&pdev->dev, "invalid name or type in platform data\n");
> > > > + return -EINVAL;
> > > > + }
>
> > > > + vcc = regulator_get_exclusive(&pdev->dev, "vrfkill");
>
> > > Wasn't that supposed to use pdata->supply? Actually, there's no member
> > > "supply" in the struct?
>
> No, if you're passing supply names through platform data something has
> gone wrong - that's a big no no.
Ok. The comment seems a little wrong still though, maybe leftover bits
from an older version?
> > Oh wait, I think I just misunderstood how this works. But if the name is
> > "vrfkill" how does that really work with multiple instances?
>
> That's what the struct device is there for. The names are mapped into
> physical regulators relative to the device.
Oh ok, makes sense, thanks for the clarification.
johannes
On Thu, Apr 14, 2011 at 12:39:12PM +0200, Antonio Ospite wrote:
> Johannes Berg <[email protected]> wrote:
> > Should remove do platform_set_drvdata(pdev, NULL)?
> AFAICS this is not strictly necessary because we never check for NULL
> here and we are setting drvdata again in _probe() each time the module
> is loaded anyways. If it is considered a good practice for symmetry
> reasons then I'll add it, no problem. Does anyone has comments on that?
We went round this loop with I2C - anything peering at driver data that
it didn't originally set is buggy anyway so no need to reset to zero.
If there was a need the platform bus or driver core should do it since
all devices would need it.
On Wed, 6 Apr 2011 23:11:33 +0900
Mark Brown <[email protected]> wrote:
> On Wed, Apr 06, 2011 at 11:21:19AM +0200, Antonio Ospite wrote:
>
> > + tristate "Generic rfkill regulator driver"
> > + depends on RFKILL || !RFKILL
>
> That looks *odd*.
Taken from Documentation/rfkill.txt section 3. Kernel API.
I guess I can drop it if we want to be stricter and just require RFKILL
to be enabled. Johannes?
> Otherwise this looks fine from a regulator API point
> of view. You use an exclusive get() so you could get away without
> remembering the enable state as nothing else could hold the device open
> but there's no harm in doing so and it's defensive against silly
> constraints that force the regulator on.
>
Thanks Mark.
Regards,
Antonio
--
Antonio Ospite
http://ao2.it
PGP public key ID: 0x4553B001
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
On Thu, 14 Apr 2011 06:06:26 -0700
Mark Brown <[email protected]> wrote:
> On Thu, Apr 14, 2011 at 12:39:12PM +0200, Antonio Ospite wrote:
> > Johannes Berg <[email protected]> wrote:
>
> > > Should remove do platform_set_drvdata(pdev, NULL)?
>
> > AFAICS this is not strictly necessary because we never check for NULL
> > here and we are setting drvdata again in _probe() each time the module
> > is loaded anyways. If it is considered a good practice for symmetry
> > reasons then I'll add it, no problem. Does anyone have comments on that?
>
> We went round this loop with I2C - anything peering at driver data that
> it didn't originally set is buggy anyway so no need to reset to zero.
> If there was a need the platform bus or driver core should do it since
> all devices would need it.
>
Ok, so v3 is the one which could be merged.
John if you are the one who is going to commit it, would you please add
the line:
Reviewed-by: Johannes Berg <[email protected]>
Thanks everybody for the comments.
Regards,
Antonio
--
Antonio Ospite
http://ao2.it
PGP public key ID: 0x4553B001
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
On Fri, 2011-04-08 at 12:59 +0200, Antonio Ospite wrote:
> Add a regulator consumer driver for rfkill to enable controlling radio
> transmitters connected to voltage regulators using the regulator
> framework.
>
> A new "vrfkill" virtual supply is provided to use in platform code.
>
> Signed-off-by: Guiming Zhuo <[email protected]>
> Signed-off-by: Antonio Ospite <[email protected]>
> ---
>
> Changes since v1:
>
> - changed "voltage regulator" to "voltage regulators" in the commit
> message
>
> - drop rfkill_init_sw_state() as requested by Johannes Berg
>
> - moved assignment fields of rfkill_data _before_ rfkill_register(),
> as the latter will call .set_block (via schedule_work()) which would
> find NULL pointers for the .vcc and .rf_kill fields otherwise.
>
> This issue was masked when I was using rfkill_init_sw_state() which
> was setting the persistent flag: rfkill_register() does not call
> schedule_work() immediately when the persistent flag is set.
>
> So please take another look at this part of rfkill_regulator_probe().
>
> Mark, I left in the RFKILL || !RFKILL part.
>
> If there are no more comments, who is going to merge the driver, Johannes?
I don't have a tree, John can merge it, but I found a few more bugs:
> + * static struct regulator_consumer_supply pcap_regulator_V6_consumers [] = {
> + * { .dev_name = "rfkill-regulator.0", supply = "vrfkill" },
> + * };
It's a comment, but it should be .supply = (missing the .)
> + if (pdata->name == NULL || pdata->type == 0) {
> + dev_err(&pdev->dev, "invalid name or type in platform data\n");
> + return -EINVAL;
> + }
> +
> + vcc = regulator_get_exclusive(&pdev->dev, "vrfkill");
Wasn't that supposed to use pdata->supply? Actually, there's no member
"supply" in the struct?
> + dev_info(&pdev->dev, "initialized\n");
Is that message really useful?
Other than that, looks good to me.
johannes
On Tue, Apr 12, 2011 at 05:23:01PM +0200, Johannes Berg wrote:
> On Tue, 2011-04-12 at 08:15 -0700, Mark Brown wrote:
> > No, if you're passing supply names through platform data something has
> > gone wrong - that's a big no no.
> Ok. The comment seems a little wrong still though, maybe leftover bits
> from an older version?
Dunno but it's definitely buggy.