Messages in this thread | | | Subject | Re: [PATCH] net: rfkill: add generic gpio rfkill driver | From | Johannes Berg <> | Date | Sat, 07 May 2011 12:01:43 +0200 |
| |
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
| |