On Wed, Mar 15, 2017 at 8:12 PM, Kevin Hilman <khilman@xxxxxxxxxxxx> wrote: > 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? 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 -- 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