Hi Stephen, On Wed, Aug 28, 2013 at 10:23 PM, Stephen Warren <swar...@wwwdotorg.org> wrote: > On 08/27/2013 09:56 PM, Sonic Zhang wrote: >> Hi Stephen, >> >> On Wed, Aug 28, 2013 at 5:39 AM, Stephen Warren <swar...@wwwdotorg.org> >> wrote: >>> On 08/27/2013 03:30 AM, Sonic Zhang wrote: >>>> Hi Stephen, >>>> >>>> On Fri, Aug 23, 2013 at 4:48 AM, Stephen Warren <swar...@wwwdotorg.org> >>>> wrote: >>>>> On 08/22/2013 01:07 AM, Sonic Zhang wrote: >>>>>> Hi Stephen, >>>>>> >>>>>> On Thu, Aug 22, 2013 at 2:45 AM, Stephen Warren <swar...@wwwdotorg.org> >>>>>> wrote: >>>>>>> On 08/21/2013 12:30 AM, Sonic Zhang wrote: >>>>>>>> From: Sonic Zhang <sonic.zh...@analog.com> >>>>>>>> >>>>>>>> The new ADI GPIO2 controller was introduced since the BF548 and BF60x >>>>>>>> processors. It differs a lot from the old one on BF5xx processors. So, >>>>>>>> create a pinctrl driver under the pinctrl framework. >>> >>>>>> The >>>>>> pinctrl_id field in platform data is to make the driver flexible for >>>>>> future SoCs. If the later case is true, I can just fix the pinctrl >>>>>> device name to "pinctrl-adi2.0". >>>>> >>>>> I was talking about pdata->port_pin_base being passed to >>>>> gpiochip_add_pin_range(), not the device name. >>>>> >>>>>> The GPIO device's HW regsiter base, pin base, pin number and the >>>>>> relationship with the PINT device are defined in the platform data. >>>>> >>>>> Hmmm. I suppose with a platform-data-based driver, there isn't a good >>>>> opportunity to encode which HW the code is running on, and then derive >>>>> those parameters from the SoC type and/or put that information into >>>>> device tree. Perhaps platform data is the only way, although isn't there >>>>> some kind of "device ID -> data" mapping table option, so that probe() >>>>> can be told which SoC is in use based on the device name, and use that >>>>> to look up SoC-specific data? >>>> >>>> The soc data driver is use to describe the pin group and function >>>> information of peripherals managed by a pin controller. It is more >>>> traditional and simpler to put the device specific parameters into the >>>> platform data. >>> >>> Sure, that's the way things have been done historically. However, if >>> there's a better way, one may as well use it. >>> >>>> >>>> >>>>> >>>>>>>> +static struct platform_driver adi_pinctrl_driver = { >>>>>>>> + .probe = adi_pinctrl_probe, >>>>>>>> + .remove = adi_pinctrl_remove, >>>>>>>> + .driver = { >>>>>>>> + .name = DRIVER_NAME, >>>>>>>> + }, >>>>>>>> +}; >>>>>>>> + >>>>>>>> +static struct platform_driver adi_gpio_pint_driver = { >>>>>>>> + .probe = adi_gpio_pint_probe, >>>>>>>> + .remove = adi_gpio_pint_remove, >>>>>>>> + .driver = { >>>>>>>> + .name = "adi-gpio-pint", >>>>>>>> + }, >>>>>>>> +}; >>>>>>>> + >>>>>>>> +static struct platform_driver adi_gpio_driver = { >>>>>>>> + .probe = adi_gpio_probe, >>>>>>>> + .remove = adi_gpio_remove, >>>>>>>> + .driver = { >>>>>>>> + .name = "adi-gpio", >>>>>>>> + }, >>>>>>>> +}; >>>>>>> >>>>>>> Hmmm. Is there one HW block that controls GPIOs and pinctrl, or are >>>>>>> there separate HW blocks? >>>>>>> >>>>>>> If there's one HW block, why not have just one driver? >>>>>>> >>>>>>> If there are separate HW blocks, then having separate GPIO and pinctrl >>>>>>> drivers seems like it would make sense. >>>>>> >>>>>> There are 6 to 9 GPIO HW blocks in one Blackfin SoC. Function >>>>>> pinmux_enable_setting() in current pinctrl framework assumes the >>>>>> function mux setting of one peripheral pin group is configured in one >>>>>> pinctrl device. But, the function mux setting of one blackfin >>>>>> peripheral may be done among different GPIO HW blocks. So, I have to >>>>>> separate the pinctrl driver from the GPIO block driver add the ranges >>>>>> of all GPIO blocks into one pinctrl device for Blackfin. >>>>> >>>>> I don't think you need separate device; the pin control mapping table >>>>> entries for a particular state simply needs to include entries for >>>>> multiple pin controllers. >>>> >>>> Calling pinctrl_select_state() multiple times with different pin >>>> controllers is not an atomic operation. If the second call fails, the >>>> pins requested successfully in the first call won't be freed >>>> automatically. >>> >>> Drivers should only call pinctrl_select_state() once. The state that >>> gets selected can affect multiple pin controllers at once. This should >>> be an atomic operation as far as the client driver is concerned. If any >>> of that isn't true, it's a bug in pinctrl. >> >> /** >> * pinctrl_select_state() - select/activate/program a pinctrl state to HW >> * @p: the pinctrl handle for the device that requests configuration >> * @state: the state handle to select/activate/program >> */ >> int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state) >> >> If drivers should still call pinctrl_select_state() once in case of >> multiple pin controllers, the first parameter of >> pinctrl_select_state() is wrong. Which pinctrl device among all >> affected pin controllers should the driver use? Or whatever pinctrl >> device? > > The function prototype is not wrong. "struct pinctrl *p" is not a > pinctrl device, but rather it's the result of calling pinctrl_get(). > This value encompasses all information required to program all pinctrl > HW devices that need to be programmed.
Thanks to explain. I didn't dig into struct pinctrl much. Regards, Sonic > >> To separate the pinctrl_settings of one peripheral to multiple pinctrl >> devices, multiple pinctrl group arrays and function arrays should be >> defined in the soc data file. This change increases the code size of >> the soc data file a lot without get any real benefits. The pin >> controller device can be defined as a logic device to cover all gpio >> devices, which are mapped into one peripheral pin id space without >> collision. All overhead in the soc data file can be removed in this >> way. > > It's possible to debate how to construct the pinctrl drivers themselves, > but this has no impact at all on how a client driver calls the pinctrl APIs. > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/