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

[Sonic Zhang]

Hi Stephen,

On Wed, Aug 28, 2013 at 5:39 AM, Stephen Warren <swarren@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> wrote:
>>>>> On 08/21/2013 12:30 AM, Sonic Zhang wrote:
>>>>>> From: Sonic Zhang <sonic.zhang@xxxxxxxxxx>
>>>>>>
>>>>>> 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?

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.

GPIO devices with peripheral pin id collision have to put into
different pin controller logic devices.

So, I think it is fine to either binding pinctrl device to gpio device
or leave it as a logic device.

Regards,

Sonic

>
>> And it is more complicated for peripheral driver to handle pin group
>> and function in multiple pin controllers. The peripheral driver has to
>> know explicit which pinctrl devices to select other than calling the
>> default devm_pinctrl_get_select_default().
>
> No; see above.
>
>> So, I think if multiple gpio devices should be set up together for one
>> peripheral, having one pin controller device to handle all of them is
>> a more reasonable and clear solution.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/