| Message ID | 20170920133927.17390-1-jbrunet@baylibre.com |
|---|---|
| Headers | show |
| Series | pinctrl: meson: clean pin offsets | expand |
On Wed, Sep 20, 2017 at 3:39 PM, Jerome Brunet <jbrunet@baylibre.com> wrote: > The initial goal of this series was move to TEST_N pin from the EE > controller to AO controller, where it belongs. This meant modify the > EE_OFF value. > > This offset is a quirk we brought from the vendor driver when it was > initially merged. There no reason to keep this around and we could simply > let pinctrl figure the pin base value. > > Removing this offset, while simple, ends up being quite a patch bomb. > This is why I split the change over 5 first patches, so the important > change, patch #1 remains visible. Of course, to avoid breaking bisect, > these first 5 patches should be squashed into one. (If you prefer that I > squash it myself, I may have to send you a PR as the patch would exceed > VGER 100000 characters limit) > > The last change is this series, while not directly related, also requires > to adjust the gpio-line-names property in DT. Having these changes going > together would make it easier to coordinate the DTS changes. > > This was changeset has been test on gxbb P200, gxl libretech-cc. It was > also boot tested on meson8 (Thx Martin!) > > Jerome Brunet (8): > pinctrl: meson: remove offset from pinctrl > pinctrl: meson: remove offset continued - gxbb > pinctrl: meson: remove offset continued - gxl > pinctrl: meson: remove offset continued - meson8 > pinctrl: meson: remove offset continued - meson8b > pinctrl: meson: get rid of pin_base > pinctrl: meson-gx: TEST_N belongs to the AO controller > pinctrl: meson-gxbb: add missing GPIOX_22 pin Looks good just waiting for review from Carlo && || Kevin. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2017-09-21 at 14:21 +0200, Linus Walleij wrote: > On Wed, Sep 20, 2017 at 3:39 PM, Jerome Brunet <jbrunet@baylibre.com> wrote: > > > The initial goal of this series was move to TEST_N pin from the EE > > controller to AO controller, where it belongs. This meant modify the > > EE_OFF value. > > > > This offset is a quirk we brought from the vendor driver when it was > > initially merged. There no reason to keep this around and we could simply > > let pinctrl figure the pin base value. > > > > Removing this offset, while simple, ends up being quite a patch bomb. > > This is why I split the change over 5 first patches, so the important > > change, patch #1 remains visible. Of course, to avoid breaking bisect, > > these first 5 patches should be squashed into one. (If you prefer that I > > squash it myself, I may have to send you a PR as the patch would exceed > > VGER 100000 characters limit) > > > > The last change is this series, while not directly related, also requires > > to adjust the gpio-line-names property in DT. Having these changes going > > together would make it easier to coordinate the DTS changes. > > > > This was changeset has been test on gxbb P200, gxl libretech-cc. It was > > also boot tested on meson8 (Thx Martin!) > > > > Jerome Brunet (8): > > pinctrl: meson: remove offset from pinctrl > > pinctrl: meson: remove offset continued - gxbb > > pinctrl: meson: remove offset continued - gxl > > pinctrl: meson: remove offset continued - meson8 > > pinctrl: meson: remove offset continued - meson8b > > pinctrl: meson: get rid of pin_base > > pinctrl: meson-gx: TEST_N belongs to the AO controller > > pinctrl: meson-gxbb: add missing GPIOX_22 pin > > Looks good just waiting for review from Carlo && || Kevin. Thanks Linus, After doing this rework, I noticed that this driver (not the only one though) assume gpio offset (param of gpio calls) and pin offset are the same thing ... instead of relying pinctrl (and gpio-ranges) to do the translation. To make things a bit more clean, I was thinking about forwarding all gpios framework calls to pinconf, so the gpio to pin offset would go through the proper mapping function. Is this the way to do it ? Using gpio_pinctrl_set_config() I should be able to achieve almost any "write" functions but I got stuck on gpio_get() ATM the moment there is no gpio_pinctrl_get_config() or something similar to read stuff in the gpio framework from pinconf. Would you be open to add something like this ? > > Yours, > Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 21, 2017 at 5:00 PM, Jerome Brunet <jbrunet@baylibre.com> wrote: > After doing this rework, I noticed that this driver (not the only one though) > assume gpio offset (param of gpio calls) and pin offset are the same thing ... > instead of relying pinctrl (and gpio-ranges) to do the translation. Hm yeah I guess drivers tend to do that if the two are identical. > To make things a bit more clean, I was thinking about forwarding all gpios > framework calls to pinconf, so the gpio to pin offset would go through the > proper mapping function. > > Is this the way to do it ? > > Using gpio_pinctrl_set_config() I should be able to achieve almost any "write" > functions but I got stuck on gpio_get() The intention is not to let pin config be the solve-all backend for combined GPIO drivers, we still want separation of concerns. The idea is that the GPIO part of the driver still drive a line high/low and that means it can also handle things like .set_multiple() to set several lines at once. There is also .get_multiple() in the works. I do not think these things should be relayed to pin config, pin config is not for driving GPIO lines, only for setting up the electrical properties of them. What we have is optional pin config back-end to set direction and set configs such as debounce or open drain by relaying the gpiochip .set_config() callback to pinctrl_gpio_set_config(). This function is in <linux/pinctrl/consumer.h> for a reason: the GPIO driver is a consumer of pinctrl services. These: extern int pinctrl_request_gpio(unsigned gpio); extern void pinctrl_free_gpio(unsigned gpio); extern int pinctrl_gpio_direction_input(unsigned gpio); extern int pinctrl_gpio_direction_output(unsigned gpio); extern int pinctrl_gpio_set_config(unsigned gpio, unsigned long config); Hm I should rename the first two to pinctrl_gpio_request() and pinctrl_gpio_free() don't you think... My OCD kicks in. Anyways: as you can see we even have special callbacks to set the lines as input and output, we do not use the pin config calls with parameters PIN_CONFIG_OUTPUT and there isn't even a corresponding PIN_CONFIG_INPUT that will really set the pin to input mode for GPIO. And that would have been the first refactoring here (getting rid of pinctrl_gpio_direction*). That is already a bit of a daunting task, and I don't even know if it makes sense :/ Relaying setting the output value or getting the input value to pinctrl doesn't make sense to me at all. > ATM the moment there is no gpio_pinctrl_get_config() or something similar to > read stuff in the gpio framework from pinconf. Would you be open to add > something like this ? I do not see the use case, but if you can describe it I can respond. .pin_config_get() in <linux/pinctrl/pinconf.h> is already seldom implemented correctly and drivers do not read out the hardware state at probe() time. And they don't read out the mux setting at all, ever, just set it. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Jerome Brunet <jbrunet@baylibre.com> writes: > The initial goal of this series was move to TEST_N pin from the EE > controller to AO controller, where it belongs. This meant modify the > EE_OFF value. > > This offset is a quirk we brought from the vendor driver when it was > initially merged. There no reason to keep this around and we could simply > let pinctrl figure the pin base value. > > Removing this offset, while simple, ends up being quite a patch bomb. > This is why I split the change over 5 first patches, so the important > change, patch #1 remains visible. Of course, to avoid breaking bisect, > these first 5 patches should be squashed into one. (If you prefer that I > squash it myself, I may have to send you a PR as the patch would exceed > VGER 100000 characters limit) > > The last change is this series, while not directly related, also requires > to adjust the gpio-line-names property in DT. Having these changes going > together would make it easier to coordinate the DTS changes. > > This was changeset has been test on gxbb P200, gxl libretech-cc. It was > also boot tested on meson8 (Thx Martin!) Really nice cleanup, thanks! Reviewed-by: Kevin Hilman <khilman@baylibre.com> Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 20, 2017 at 3:39 PM, Jerome Brunet <jbrunet@baylibre.com> wrote: > Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> Patch applied. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 20, 2017 at 3:39 PM, Jerome Brunet <jbrunet@baylibre.com> wrote: > Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> Patch applied. Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 20, 2017 at 3:39 PM, Jerome Brunet <jbrunet@baylibre.com> wrote: > On meson-gx platforms, TEST_N has been incorrectly declared in the EE > controller while it belongs to AO controller. > > Move the pin to the appropriate controller > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> Patch applied. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html