Re: [PATCH v3] rfkill: Regulator consumer driver for rfkill
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
Otherwise, feel free to include that in v4.
johannes
Re: [PATCH v2] rfkill: Regulator consumer driver for rfkill
On Wed, 2011-04-13 at 10:44 +0200, Antonio Ospite wrote:
> On Tue, 12 Apr 2011 13:44:02 +0200
> Johannes Berg 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
Re: [PATCH v2] rfkill: Regulator consumer driver for rfkill
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
Re: [PATCH v2] rfkill: Regulator consumer driver for rfkill
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
Re: [PATCH v2] rfkill: Regulator consumer driver for rfkill
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
> Signed-off-by: Antonio Ospite
> ---
>
> 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
Re: [PATCH] rfkill: Regulator consumer driver for rfkill
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
Re: [PATCH] rfkill: Regulator consumer driver for rfkill
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
Re: [PATCH] rfkill: Regulator consumer driver for rfkill
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
Re: [PATCH] rfkill: Regulator consumer driver for rfkill
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
Re: [PATCH] rfkill: Regulator consumer driver for rfkill
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
Re: [PATCH] rfkill: Regulator consumer driver for rfkill
On Wed, 2011-04-06 at 16:29 +0200, Antonio Ospite wrote: > On Wed, 6 Apr 2011 23:11:33 +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*. > > 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
Re: [PATCH] rfkill: Regulator consumer driver for rfkill
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
Re: [PATCH] rfkill: Regulator consumer driver for rfkill
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
Re: [PATCH] rfkill: Regulator consumer driver for rfkill
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
