Adding a void * to pinctrl_pin_desc

Sherman Yin syin at broadcom.com
Mon Apr 29 19:46:51 EDT 2013


>> Hmm, while it would be easy to create a macro to fill the static foo_pin_desc array
>> in the driver, this array would not be suitable for use with pinctrl_register().
> 
> Now I start to change opinion. You're right about this.
> 
>> I would have to, in the probe function, loop through the foo_pin_desc array and
>> create another pinctrl_pin_desc array to use for pinctrl_desc.pins.
> 
> Yeah that sucks.
> 
>> I think I'll just use a separate struct in the driver to keep track of other pin-specific data.
> 
> No I think I have changed opinion. Can you send a patch adding a void * to
> struct pinctrl_pin_desc  as patch [1/n] when you send your driver, and
> I think I'll be happy with it.

Great!  This will definitely work for my chip, and I can do it as the first of n patches 
for my driver later.  While we are considering a change to pinctrl_pin_desc, I just 
want to point out a more forward-thinking solution:

The void* method works well for my chip where each pin can be one of only 
3 types, such as:

=====
static const int std_pin = PIN_TYPE_STD;
static const int i2c_pin = PIN_TYPE_I2C;

#define FOO_PIN(a,b,c) {.number=a, .name=b, .drv_data=&c)

static const struct pinctrl_pin_desc foo_pins[] = {
  FOO_PIN(PIN_NUM_1, "pin name 1", std_pin),
  FOO_PIN(PIN_NUM_2, "pin name 2", i2c_pin),
  FOO_PIN(PIN_NUM_3, "pin name 3", i2c_pin),
  ...
};
=====

but this may not work well for drivers where the private data is unique for every 
pin.  drv_data is a void*, so the data has to be defined first (potentially, this is another 
long array) before the pinctrl_pin_desc MACRO can refer to the address like the 
code above.  

I think ultimately the most flexible solution would be to let each driver define their own 
pin_desc like pin groups and pin functions.  This would involve some changes 
to the core:
- instead of having a pinctrl_pin_desc array in pinctrl_desc, expand it to 2 arrays - 
unsigned pin_number array and const char pin_name array
- APIs such as pinctrl_register() and pinctrl_free_pindescs() will have to similarly 
expand pinctrl_pin_desc into its 2 components.

Right now, I don't think it is worth the effort to change all the drivers and core code for
this flexibility, since no driver currently uses it.  Just wanted to point this out for 
consideration.

Thanks,
Sherman





More information about the linux-arm-kernel mailing list