| Message ID | 20180118160208.16461-1-ludovic.desroches@microchip.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | pinctrl: at91-pio4: add support for drive-strength property | expand |
On Thu, Jan 18, 2018 at 5:02 PM, Ludovic Desroches <ludovic.desroches@microchip.com> wrote: > Add support for the drive-strength property. Usually its value is > expressed in mA. Since the numeric value depends on VDDIOP voltage, > the controller uses low, medium and high to define the drive-strengh. Aha I see. That's complex. It certainly results in a certain mA drive strength in the end, but what you're saying is that this is not usually what we configure. > The PIO controller accepts two values for the low drive: 0 or 1. Most > of the time, we don't care about the drive strength, there is no need > to change it, so 0 is considered as the default value. Do you mean default value as in "whatever the hardware was set up as at boot time"? > The low-drive > value won't be advertised through pinconf-pins file excepted if it except? > has been set explicitly in the device tree ie if its value is > different from 0. > > Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com> OK I think I get it. > Optional properties: > - GENERIC_PINCONFIG: generic pinconfig options to use, bias-disable, > -bias-pull-down, bias-pull-up, drive-open-drain, input-schmitt-enable, > -input-debounce, output-low, output-high. > +bias-pull-down, bias-pull-up, drive-open-drain, drive-strength, > +input-schmitt-enable, input-debounce, output-low, output-high. (...) > + drive-strength = <ATMEL_PIO_DRVSTR_LO>; So you say you support this argument and it will be something like include/dt-bindings/pinctrl/at91.h:#define ATMEL_PIO_DRVSTR_LO 1 include/dt-bindings/pinctrl/at91.h:#define ATMEL_PIO_DRVSTR_ME 2 include/dt-bindings/pinctrl/at91.h:#define ATMEL_PIO_DRVSTR_HI 3 But the definition if generic drive strength is actually in mA. I think it is OK to deviate from stating it in mA, but you should write this in the DT bindings so people do not get confused. > @@ -814,6 +834,19 @@ static void atmel_conf_pin_config_dbg_show(struct pinctrl_dev *pctldev, > seq_printf(s, "%s ", "open-drain"); > if (conf & ATMEL_PIO_SCHMITT_MASK) > seq_printf(s, "%s ", "schmitt"); > + if (conf & ATMEL_PIO_DRVSTR_MASK) { > + switch ((conf & ATMEL_PIO_DRVSTR_MASK) >> ATMEL_PIO_DRVSTR_OFFSET) { > + case ATMEL_PIO_DRVSTR_LO: > + seq_printf(s, "%s ", "low-drive"); > + break; > + case ATMEL_PIO_DRVSTR_ME: > + seq_printf(s, "%s ", "medium-drive"); > + break; > + case ATMEL_PIO_DRVSTR_HI: > + seq_printf(s, "%s ", "high-drive"); > + break; But here you know what VDDIOP is, right? So in debugfs you could actually print the real drive strength in mA, or both "medium-drive, %u mA"? Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 22, 2018 at 09:37:38AM +0100, Linus Walleij wrote: > On Thu, Jan 18, 2018 at 5:02 PM, Ludovic Desroches > <ludovic.desroches@microchip.com> wrote: > > > Add support for the drive-strength property. Usually its value is > > expressed in mA. Since the numeric value depends on VDDIOP voltage, > > the controller uses low, medium and high to define the drive-strengh. > > Aha I see. That's complex. It certainly results in a certain mA drive > strength in the end, but what you're saying is that this is not usually > what we configure. > The PIO Controller Interface is: Drive Strength: 0 Low drive 1 Low drive 2 Medium drive 3 High drive In the Electrical Characteristics, you have: GPIO 1.8V Low: max 1 mA GPIO 1.8V Medium: max 10 mA GPIO 1.8V High: max 18 mA GPIO 3.3V Low: max 2 mA GPIO 3.3V Medium: max 20 mA GPIO 3.3V High: max 32 mA > > The PIO controller accepts two values for the low drive: 0 or 1. Most > > of the time, we don't care about the drive strength, there is no need > > to change it, so 0 is considered as the default value. > > Do you mean default value as in "whatever the hardware was set > up as at boot time"? Yes > > > The low-drive > > value won't be advertised through pinconf-pins file excepted if it > > except? Yes > > > has been set explicitly in the device tree ie if its value is > > different from 0. > > > > Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com> > > OK I think I get it. > > > Optional properties: > > - GENERIC_PINCONFIG: generic pinconfig options to use, bias-disable, > > -bias-pull-down, bias-pull-up, drive-open-drain, input-schmitt-enable, > > -input-debounce, output-low, output-high. > > +bias-pull-down, bias-pull-up, drive-open-drain, drive-strength, > > +input-schmitt-enable, input-debounce, output-low, output-high. > (...) > > + drive-strength = <ATMEL_PIO_DRVSTR_LO>; > > So you say you support this argument and it will be something like > > include/dt-bindings/pinctrl/at91.h:#define ATMEL_PIO_DRVSTR_LO 1 > include/dt-bindings/pinctrl/at91.h:#define ATMEL_PIO_DRVSTR_ME 2 > include/dt-bindings/pinctrl/at91.h:#define ATMEL_PIO_DRVSTR_HI 3 > > But the definition if generic drive strength is actually in mA. > > I think it is OK to deviate from stating it in mA, but you should > write this in the DT bindings so people do not get confused. > Ok > > @@ -814,6 +834,19 @@ static void atmel_conf_pin_config_dbg_show(struct pinctrl_dev *pctldev, > > seq_printf(s, "%s ", "open-drain"); > > if (conf & ATMEL_PIO_SCHMITT_MASK) > > seq_printf(s, "%s ", "schmitt"); > > + if (conf & ATMEL_PIO_DRVSTR_MASK) { > > + switch ((conf & ATMEL_PIO_DRVSTR_MASK) >> ATMEL_PIO_DRVSTR_OFFSET) { > > + case ATMEL_PIO_DRVSTR_LO: > > + seq_printf(s, "%s ", "low-drive"); > > + break; > > + case ATMEL_PIO_DRVSTR_ME: > > + seq_printf(s, "%s ", "medium-drive"); > > + break; > > + case ATMEL_PIO_DRVSTR_HI: > > + seq_printf(s, "%s ", "high-drive"); > > + break; > > But here you know what VDDIOP is, right? > No, I have no way to retrieve the power for the several VDDIOP power rails. > So in debugfs you could actually print the real drive strength > in mA, or both "medium-drive, %u mA"? > > Yours, > Linus Walleij Regards Ludovic -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 22, 2018 at 09:37:38AM +0100, Linus Walleij wrote: > On Thu, Jan 18, 2018 at 5:02 PM, Ludovic Desroches > <ludovic.desroches@microchip.com> wrote: > > > Add support for the drive-strength property. Usually its value is > > expressed in mA. Since the numeric value depends on VDDIOP voltage, > > the controller uses low, medium and high to define the drive-strengh. > > Aha I see. That's complex. It certainly results in a certain mA drive > strength in the end, but what you're saying is that this is not usually > what we configure. > > > The PIO controller accepts two values for the low drive: 0 or 1. Most > > of the time, we don't care about the drive strength, there is no need > > to change it, so 0 is considered as the default value. > > Do you mean default value as in "whatever the hardware was set > up as at boot time"? > > > The low-drive > > value won't be advertised through pinconf-pins file excepted if it > > except? > > > has been set explicitly in the device tree ie if its value is > > different from 0. > > > > Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com> > > OK I think I get it. > > > Optional properties: > > - GENERIC_PINCONFIG: generic pinconfig options to use, bias-disable, > > -bias-pull-down, bias-pull-up, drive-open-drain, input-schmitt-enable, > > -input-debounce, output-low, output-high. > > +bias-pull-down, bias-pull-up, drive-open-drain, drive-strength, > > +input-schmitt-enable, input-debounce, output-low, output-high. > (...) > > + drive-strength = <ATMEL_PIO_DRVSTR_LO>; > > So you say you support this argument and it will be something like > > include/dt-bindings/pinctrl/at91.h:#define ATMEL_PIO_DRVSTR_LO 1 > include/dt-bindings/pinctrl/at91.h:#define ATMEL_PIO_DRVSTR_ME 2 > include/dt-bindings/pinctrl/at91.h:#define ATMEL_PIO_DRVSTR_HI 3 > > But the definition if generic drive strength is actually in mA. Yes, and the reason we put unit suffixes on properties is to avoid differing units. > I think it is OK to deviate from stating it in mA, but you should > write this in the DT bindings so people do not get confused. I don't think it is okay. If "drive-strength" doesn't work, then use a vendor specific property ("atmel,drive-strength"). Of course, I might forget this in the next version and tell you to use a standard property in mA. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 29, 2018 at 01:01:30PM -0600, Rob Herring wrote: > On Mon, Jan 22, 2018 at 09:37:38AM +0100, Linus Walleij wrote: > > On Thu, Jan 18, 2018 at 5:02 PM, Ludovic Desroches > > <ludovic.desroches@microchip.com> wrote: > > > > > Add support for the drive-strength property. Usually its value is > > > expressed in mA. Since the numeric value depends on VDDIOP voltage, > > > the controller uses low, medium and high to define the drive-strengh. > > > > Aha I see. That's complex. It certainly results in a certain mA drive > > strength in the end, but what you're saying is that this is not usually > > what we configure. > > > > > The PIO controller accepts two values for the low drive: 0 or 1. Most > > > of the time, we don't care about the drive strength, there is no need > > > to change it, so 0 is considered as the default value. > > > > Do you mean default value as in "whatever the hardware was set > > up as at boot time"? > > > > > The low-drive > > > value won't be advertised through pinconf-pins file excepted if it > > > > except? > > > > > has been set explicitly in the device tree ie if its value is > > > different from 0. > > > > > > Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com> > > > > OK I think I get it. > > > > > Optional properties: > > > - GENERIC_PINCONFIG: generic pinconfig options to use, bias-disable, > > > -bias-pull-down, bias-pull-up, drive-open-drain, input-schmitt-enable, > > > -input-debounce, output-low, output-high. > > > +bias-pull-down, bias-pull-up, drive-open-drain, drive-strength, > > > +input-schmitt-enable, input-debounce, output-low, output-high. > > (...) > > > + drive-strength = <ATMEL_PIO_DRVSTR_LO>; > > > > So you say you support this argument and it will be something like > > > > include/dt-bindings/pinctrl/at91.h:#define ATMEL_PIO_DRVSTR_LO 1 > > include/dt-bindings/pinctrl/at91.h:#define ATMEL_PIO_DRVSTR_ME 2 > > include/dt-bindings/pinctrl/at91.h:#define ATMEL_PIO_DRVSTR_HI 3 > > > > But the definition if generic drive strength is actually in mA. > > Yes, and the reason we put unit suffixes on properties is to avoid > differing units. > > > I think it is OK to deviate from stating it in mA, but you should > > write this in the DT bindings so people do not get confused. > > I don't think it is okay. If "drive-strength" doesn't work, then use > a vendor specific property ("atmel,drive-strength"). Of course, I might > forget this in the next version and tell you to use a standard property > in mA. Ok, so I am going to send the next version! The purpose is to keep using the generic pinconf parsing function. If I need to use "atmel,drive-strength" property, I assume I could manage it with custom_params, isn't it? Regards Ludovic -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pio4-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pio4-pinctrl.txt index 61ac75706cc9..183064bc4bda 100644 --- a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pio4-pinctrl.txt +++ b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pio4-pinctrl.txt @@ -34,8 +34,8 @@ right representation of the pin. Optional properties: - GENERIC_PINCONFIG: generic pinconfig options to use, bias-disable, -bias-pull-down, bias-pull-up, drive-open-drain, input-schmitt-enable, -input-debounce, output-low, output-high. +bias-pull-down, bias-pull-up, drive-open-drain, drive-strength, +input-schmitt-enable, input-debounce, output-low, output-high. Example: @@ -66,6 +66,7 @@ Example: pinmux = <PIN_PB0>, <PIN_PB5>; bias-pull-up; + drive-strength = <ATMEL_PIO_DRVSTR_LO>; }; pinctrl_sdmmc1_default: sdmmc1_default { diff --git a/drivers/pinctrl/pinctrl-at91-pio4.c b/drivers/pinctrl/pinctrl-at91-pio4.c index 4b57a13758a4..857c0a41a905 100644 --- a/drivers/pinctrl/pinctrl-at91-pio4.c +++ b/drivers/pinctrl/pinctrl-at91-pio4.c @@ -14,6 +14,7 @@ * GNU General Public License for more details. */ +#include <dt-bindings/pinctrl/at91.h> #include <linux/clk.h> #include <linux/gpio/driver.h> /* FIXME: needed for gpio_to_irq(), get rid of this */ @@ -49,6 +50,8 @@ #define ATMEL_PIO_IFSCEN_MASK BIT(13) #define ATMEL_PIO_OPD_MASK BIT(14) #define ATMEL_PIO_SCHMITT_MASK BIT(15) +#define ATMEL_PIO_DRVSTR_MASK GENMASK(17, 16) +#define ATMEL_PIO_DRVSTR_OFFSET 16 #define ATMEL_PIO_CFGR_EVTSEL_MASK GENMASK(26, 24) #define ATMEL_PIO_CFGR_EVTSEL_FALLING (0 << 24) #define ATMEL_PIO_CFGR_EVTSEL_RISING (1 << 24) @@ -685,6 +688,11 @@ static int atmel_conf_pin_config_group_get(struct pinctrl_dev *pctldev, return -EINVAL; arg = 1; break; + case PIN_CONFIG_DRIVE_STRENGTH: + if (!(res & ATMEL_PIO_DRVSTR_MASK)) + return -EINVAL; + arg = (res & ATMEL_PIO_DRVSTR_MASK) >> ATMEL_PIO_DRVSTR_OFFSET; + break; case PIN_CONFIG_INPUT_SCHMITT_ENABLE: if (!(res & ATMEL_PIO_SCHMITT_MASK)) return -EINVAL; @@ -737,6 +745,18 @@ static int atmel_conf_pin_config_group_set(struct pinctrl_dev *pctldev, else conf |= ATMEL_PIO_OPD_MASK; break; + case PIN_CONFIG_DRIVE_STRENGTH: + switch (arg) { + case ATMEL_PIO_DRVSTR_LO: + case ATMEL_PIO_DRVSTR_ME: + case ATMEL_PIO_DRVSTR_HI: + conf &= (~ATMEL_PIO_DRVSTR_MASK); + conf |= arg << ATMEL_PIO_DRVSTR_OFFSET; + break; + default: + dev_warn(pctldev->dev, "drive strength not updated (incorrect value)\n"); + } + break; case PIN_CONFIG_INPUT_SCHMITT_ENABLE: if (arg == 0) conf |= ATMEL_PIO_SCHMITT_MASK; @@ -814,6 +834,19 @@ static void atmel_conf_pin_config_dbg_show(struct pinctrl_dev *pctldev, seq_printf(s, "%s ", "open-drain"); if (conf & ATMEL_PIO_SCHMITT_MASK) seq_printf(s, "%s ", "schmitt"); + if (conf & ATMEL_PIO_DRVSTR_MASK) { + switch ((conf & ATMEL_PIO_DRVSTR_MASK) >> ATMEL_PIO_DRVSTR_OFFSET) { + case ATMEL_PIO_DRVSTR_LO: + seq_printf(s, "%s ", "low-drive"); + break; + case ATMEL_PIO_DRVSTR_ME: + seq_printf(s, "%s ", "medium-drive"); + break; + case ATMEL_PIO_DRVSTR_HI: + seq_printf(s, "%s ", "high-drive"); + break; + } + } } static const struct pinconf_ops atmel_confops = { diff --git a/include/dt-bindings/pinctrl/at91.h b/include/dt-bindings/pinctrl/at91.h index 2732d6c0fb39..eb81867eac77 100644 --- a/include/dt-bindings/pinctrl/at91.h +++ b/include/dt-bindings/pinctrl/at91.h @@ -39,4 +39,8 @@ #define AT91_PERIPH_C 3 #define AT91_PERIPH_D 4 +#define ATMEL_PIO_DRVSTR_LO 1 +#define ATMEL_PIO_DRVSTR_ME 2 +#define ATMEL_PIO_DRVSTR_HI 3 + #endif /* __DT_BINDINGS_AT91_PINCTRL_H__ */
Add support for the drive-strength property. Usually its value is expressed in mA. Since the numeric value depends on VDDIOP voltage, the controller uses low, medium and high to define the drive-strengh. The PIO controller accepts two values for the low drive: 0 or 1. Most of the time, we don't care about the drive strength, there is no need to change it, so 0 is considered as the default value. The low-drive value won't be advertised through pinconf-pins file excepted if it has been set explicitly in the device tree ie if its value is different from 0. Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com> --- .../bindings/pinctrl/atmel,at91-pio4-pinctrl.txt | 5 ++-- drivers/pinctrl/pinctrl-at91-pio4.c | 33 ++++++++++++++++++++++ include/dt-bindings/pinctrl/at91.h | 4 +++ 3 files changed, 40 insertions(+), 2 deletions(-)