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

On Wed, Mar 15, 2017 at 8:12 PM, Kevin Hilman [off-list ref] wrote:

Neil Armstrong [off-list ref] writes:

quoted

On 03/14/2017 04:42 PM, Linus Walleij wrote:

quoted

On Thu, Mar 9, 2017 at 8:47 PM, Martin Blumenstingl
[off-list ref] wrote:

quoted

On Mon, Mar 6, 2017 at 3:42 PM, Jerome Brunet [off-list ref] wrote:

quoted

On Sat, 2017-03-04 at 22:23 +0100, Martin Blumenstingl wrote:

quoted

quoted

quoted

+     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?

quoted

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?

quoted

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?

it would look like this (node name, label and group stay the same,
function does not contain the _x/_clk suffix anymore):
pwm_f_clk_pins: pwm_f_clk {
    mux {
        groups = "pwm_f_clk";
        function = "pwm_f";
    };
};

pwm_f_x_pins: pwm_f_x {
    mux {
        groups = "pwm_f_x";
        function = "pwm_f";
    };
};

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)

if the same pin is assigned to two devices then the pinctrl subsystem
will throw an error (we don't have to take care of this, it's how I
discovered as GPIOAO_1 was used by uart_rx_ao_a and uart_rx_ao_b).
however, I have not tested yet what happens if the same function is
assigned to multiple pins (let's say you pass both, pwm_f_clk_pins and
pwm_f_x_pins to the pwm_ef node - will this result in the PWM output
being routed to *both* pins or just one pin?). if the same function
cannot be used by two pins simultaneously then we should probably use
function "pwm_f" instead of "pwm_f_x" (just an example) so we can
detect these "conflicts".


Regards,
Martin


[0] https://github.com/torvalds/linux/commit/b27e36482c02a94194fec71fb29696f4c8e9241c