Re: [PATCH 1/2] pinctrl: meson: gxl: add the missing PWM pin definitions

[Kevin Hilman <khilman@xxxxxxxxxxxx>]

Neil Armstrong <narmstrong@xxxxxxxxxxxx> writes:

> On 03/14/2017 04:42 PM, Linus Walleij wrote:
>> On Thu, Mar 9, 2017 at 8:47 PM, Martin Blumenstingl
>> <martin.blumenstingl@xxxxxxxxxxxxxx> wrote:
>>> On Mon, Mar 6, 2017 at 3:42 PM, Jerome Brunet <jbrunet@xxxxxxxxxxxx> wrote:
>>>> On Sat, 2017-03-04 at 22:23 +0100, Martin Blumenstingl wrote:
>> 
>>>>> +     FUNCTION(pwm_f_clk),
>>>>> +     FUNCTION(pwm_f_x),
>>>>
>>>> I wonder if having function named "pwm_f_clk" really makes sense ?
>>>> Shouldn't it be just "pwm_f" ? This is real function, isn't it ?
>>>> The actual pin used will be provided in the dt. Here, I suppose we
>>>> could have this:
>>>>
>>>> +static const char * const pwm_f_groups[] = {
>>>> +       "pwm_f_x", "pwm_f_clk",
>>>> +};
>>>>
>>>> Has far as I can see, on meson arch, the function does not carry much
>>>> information anyway, except for prints.
>>>>
>>>> To be clear, I'm not questioning this change in particular. It looks
>>>> good, and follows what has been done in the past on meson. I know we
>>>> have been this a lot already, but I'm questioning whether we should
>>>> continue to do so ?
>>>>
>>>> I asking because I also have a lot case like this coming up on audio
>>>> for gxl and gxbb, where the same function can use different pins.
>>>
>>> could you please look into Jerome's question?
>>> personally I'm fine with either way, and changing my patch would be
>>> quite trivial. but I'd like to know what's "the way to go" before
>>> changing anything (and reverting that afterwards again).
>> 
>> I don't understand the question really.
>> 
>> I am not an expert on this system, if the people working with it
>> cannot tell a function from a group I don't know who can... certainly
>> not me.
>> 
>> What I can say is that pincontrol combines functions and groups to
>> states using a mapping. The functions should be something you poke
>> into a register, the groups are looser defined but may also be a
>> character of the hardware, but more usual a character of the
>> intended electronic usecase. Groups contain 1..n pins and can
>> be combined with some applicable functions.
>> 
>> Please re-read Documentation/pinctrl.txt very closely if anything is
>> unclear, I really put a lot of hours into getting that right. Especially
>> reexamine "Pinmux conventions".
>
> The point pushed by Jerome was purely cosmetic since the groups in the meson
> pinctrl driver are purely cosmetic, since only the GPIO group is handled,
> other groups are all handled the same.

handled the same... as what?

> This is because I pushed all the PWM pins in a separate group, but functionnaly
> the internal signal (i.e. PWM F) is the same for multiple pins and should be
> a single "PWM F" group instead of multiple ones.
>
> My advice is to leave the PWM groups as is,

Do you mean as we have in mainline today?  or as is proposed in $SUBJECT patch?

> and push new pins/functions/groups
> grouped with the internal signal name if split on multiple pins.

Can somone do a quick patch for PWM_F for example, also showing how this
will look in the DT if someone wants to switch between the PWM_F on GPIOX
or GPIOCLK?

We shouldalso verify that the driver is detecting/removing conflicts
properly when something else is already using that pin (e.g. SDIO_IRQ
shares pin GPIOX_7 with PWM_F)

Kevin


--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html