[PATCH] ARM: Adds pin config API to set all configs in one function
Sherman Yin
syin at broadcom.com
Wed May 1 20:07:05 EDT 2013
>OK I thought a bit about this too and I see the problem.
>
>However I think it's cluttery to have both
>pin_config_set() and pin_config_set_all().
>
>If you want it to be done this way, I think you should modify
>pin_config_set() like so instead:
>
>- int (*pin_config_set) (struct pinctrl_dev *pctldev,
>- unsigned pin,
>- unsigned long config);
>+ int (*pin_config_set) (struct pinctrl_dev *pctldev,
>+ const struct
>pinctrl_setting_configs *configs, unsigned num_configs);
>
>And then change all the drivers.
>
>(And vice versa mutatis mutandis for the group function.)
So that's basically back to the initial suggestion of moving the
loop inside the driver's function instead of creating a separate
API? Stephen said he is concerned about the duplicated loop
but did say it's not a big deal.
>Notice especially the added num_configs parameter that was
>missing from pin_config_set_all() in your patch, please tell
>me how else you deduct the size of the configs array.
We are passing a pointer to struct pinctrl_setting_configs,
which should have the num_configs already:
105struct pinctrl_setting_configs {
106 unsigned group_or_pin;
107 unsigned long *configs;
108 unsigned num_configs;
109};
I want to be sure you are ok with exposing this struct to
consumers (ie. drivers). If not, we could pass the 3
components in the API instead.
>To the drivers to iterate over the array of configs,
>please introduce a static helper function:
>
>for_each_config() in something like <linux/pinctrl/pinconf.h>
>so it's easy for driver writers to get this right. (You'll be much
>helped by this function when refactoring the existing drivers as well
>I think.)
Yup ok.
>Doing the same thing with two different functions isn't
>helpful, refactoring like this is much better, it's just more
>work :-D
Yes it's more work, and a bit more risk of breaking stuff. Out of the
2 methods, I do like this better since there are less APIs. I will probably
have to get this patch in after my pinctrl driver since it is almost ready.
Thanks!
Sherman
More information about the linux-arm-kernel
mailing list