lkml.org 
[lkml]   [2011]   [May]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] net: rfkill: add generic gpio rfkill driver
From
Date
On Fri, 2011-05-06 at 17:40 -0700, Rhyland Klein wrote:

[snip]

Looks nice! A few comments below:

> + * @blocked: block rf enabled (default is true)

(see below)

> +struct rfkill_gpio_platform_data {
> + char *name;

> + char *power_clk_name;

const?

> + bool blocked;

The comments say this is default "true", but I don't think that makes
sense since this struct would be defined somewhere else and be static
const so zero-initialised. Also, are you sure that even having this
makes sense? (see below)

> +config RFKILL_GPIO
> + bool "GPIO RFKILL driver"
> + depends on RFKILL && GPIOLIB
> + help
> + If you say yes here you get support of a generic gpio RFKILL
> + driver. Platform needs to define the resources required

That appears to be cut short?


> +#define GPIO_NAME_LEN 40

Is there really no definition yet? Maybe it would make sense to
dynamically allocate?

> +struct rfkill_gpio_data {
> + struct rfkill_gpio_platform_data *pdata;
> + struct rfkill *rfkill_dev;
> + char reset_name[GPIO_NAME_LEN];
> + char shutdown_name[GPIO_NAME_LEN];
> + struct clk *pwr_clk;

That last line is a little oddly formatted.

> + /* setup initial state */
> + rfkill_gpio_set_power(rfkill, pdata->blocked);
> + rfkill_set_states(rfkill->rfkill_dev, pdata->blocked, false);
> +
> + ret = rfkill_register(rfkill->rfkill_dev);

So if you do this, rfkill core will set the persistent thing. We
discovered a while ago (and I mentioned that in the original thread)
there will be quirky behaviour. I would recommend leaving this out and
just relying on the rfkill core to call set_block() a little after
rfkill_register() to sync the state of this device with the desired
system state.

johannes



\
 
 \ /
  Last update: 2011-05-07 12:05    [W:0.041 / U:0.804 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site