Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.

[Stephen Warren <swarren@wwwdotorg.org>]

On 08/27/2013 09:56 PM, Sonic Zhang wrote:
> Hi Stephen,
> 
> On Wed, Aug 28, 2013 at 5:39 AM, Stephen Warren <swarren@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 <swarren@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 <swarren@wwwdotorg.org> wrote:
>>>>>> On 08/21/2013 12:30 AM, Sonic Zhang wrote:
>>>>>>> From: Sonic Zhang <sonic.zhang@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.

> 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.