| Message ID | 1435332204.23818.9.camel@codethink.co.uk (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
On Fri, Jun 26, 2015 at 5:23 PM, Ben Hutchings <ben.hutchings@codethink.co.uk> wrote: > The pfc in the R8A7790 (and probably others in the R-Car gen 2 family) > supports switching SDHI signals between 3.3V and 1.8V voltage, and the > SD driver should do that when selecting a higher-speed mode. > > Add a flag for pins that support low voltage mode and SoC operations to > get and set it. Implement the pinconf power-source parameter using these > operations. > > Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk> > --- > .../bindings/pinctrl/renesas,pfc-pinctrl.txt | 4 ++- > drivers/pinctrl/sh-pfc/pinctrl.c | 34 ++++++++++++++++++++-- > drivers/pinctrl/sh-pfc/sh_pfc.h | 6 ++++ > 3 files changed, 41 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt > index bfe72ec055e3..93651742f1c1 100644 > --- a/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt > +++ b/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt > @@ -69,7 +69,9 @@ Pin Configuration Node Properties: > > The pin configuration parameters use the generic pinconf bindings defined in > pinctrl-bindings.txt in this directory. The supported parameters are > -bias-disable, bias-pull-up and bias-pull-down. > +bias-disable, bias-pull-up, bias-pull-down and power-source. For pins that > +can operate with either 3.3V or 1.8V signals, the power-source value should > +be 0 for 3.3V or 1 for 1.8V. > @@ -121,6 +125,8 @@ struct sh_pfc_soc_operations { > unsigned int (*get_bias)(struct sh_pfc *pfc, unsigned int pin); > void (*set_bias)(struct sh_pfc *pfc, unsigned int pin, > unsigned int bias); > + bool (*get_low_voltage)(struct sh_pfc *pfc, unsigned int pin); > + void (*set_low_voltage)(struct sh_pfc *pfc, unsigned int pin, bool low); > }; Given the sh-pfc driver is used for a whole range of SoCs, I think a boolean value is a bit too limiting. E.g. In addition to 1.8V and 3.3 V, R-Car M2 has pins supporting three voltage levels (1.35V, 1.5V, and 18.V). R-Mobile APE6 supports 1.2V, 1.8V, and 3.3V, but max. only 2 out of 3 for a single pin. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ben, Thank you for the patch. On Friday 26 June 2015 16:23:24 Ben Hutchings wrote: > The pfc in the R8A7790 (and probably others in the R-Car gen 2 family) > supports switching SDHI signals between 3.3V and 1.8V voltage, and the > SD driver should do that when selecting a higher-speed mode. > > Add a flag for pins that support low voltage mode and SoC operations to > get and set it. Implement the pinconf power-source parameter using these > operations. As Geert has already pointed out, we should use an integer value instead of a boolean for voltages. How about expressing them in microvolts as done by the regulators API ? Apart from that the patch looks quite good to me. Please see below for a couple of other comments. > Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk> > --- > .../bindings/pinctrl/renesas,pfc-pinctrl.txt | 4 ++- > drivers/pinctrl/sh-pfc/pinctrl.c | 34 +++++++++++++++++-- > drivers/pinctrl/sh-pfc/sh_pfc.h | 6 ++++ > 3 files changed, 41 insertions(+), 3 deletions(-) > > diff --git > a/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt > b/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt index > bfe72ec055e3..93651742f1c1 100644 > --- a/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt > +++ b/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt > @@ -69,7 +69,9 @@ Pin Configuration Node Properties: > > The pin configuration parameters use the generic pinconf bindings defined > in pinctrl-bindings.txt in this directory. The supported parameters are > -bias-disable, bias-pull-up and bias-pull-down. > +bias-disable, bias-pull-up, bias-pull-down and power-source. For pins that > +can operate with either 3.3V or 1.8V signals, the power-source value should > +be 0 for 3.3V or 1 for 1.8V. > > > GPIO > diff --git a/drivers/pinctrl/sh-pfc/pinctrl.c > b/drivers/pinctrl/sh-pfc/pinctrl.c index 072e7c62cab7..fa4066d4bc61 100644 > --- a/drivers/pinctrl/sh-pfc/pinctrl.c > +++ b/drivers/pinctrl/sh-pfc/pinctrl.c > @@ -465,6 +465,9 @@ static bool sh_pfc_pinconf_validate(struct sh_pfc *pfc, > unsigned int _pin, case PIN_CONFIG_BIAS_PULL_DOWN: > return pin->configs & SH_PFC_PIN_CFG_PULL_DOWN; > > + case PIN_CONFIG_POWER_SOURCE: > + return pin->configs & SH_PFC_PIN_CFG_LOW_VOLTAGE; > + > default: > return false; > } > @@ -477,7 +480,6 @@ static int sh_pfc_pinconf_get(struct pinctrl_dev > *pctldev, unsigned _pin, struct sh_pfc *pfc = pmx->pfc; > enum pin_config_param param = pinconf_to_config_param(*config); > unsigned long flags; > - unsigned int bias; > > if (!sh_pfc_pinconf_validate(pfc, _pin, param)) > return -ENOTSUPP; > @@ -485,7 +487,9 @@ static int sh_pfc_pinconf_get(struct pinctrl_dev > *pctldev, unsigned _pin, switch (param) { > case PIN_CONFIG_BIAS_DISABLE: > case PIN_CONFIG_BIAS_PULL_UP: > - case PIN_CONFIG_BIAS_PULL_DOWN: > + case PIN_CONFIG_BIAS_PULL_DOWN: { > + unsigned int bias; > + > if (!pfc->info->ops || !pfc->info->ops->get_bias) > return -ENOTSUPP; > > @@ -498,6 +502,16 @@ static int sh_pfc_pinconf_get(struct pinctrl_dev > *pctldev, unsigned _pin, > > *config = 0; > break; > + } > + > + case PIN_CONFIG_POWER_SOURCE: > + if (!pfc->info->ops || !pfc->info->ops->get_low_voltage) > + return -ENOTSUPP; > + > + spin_lock_irqsave(&pfc->lock, flags); > + *config = pfc->info->ops->get_low_voltage(pfc, _pin); > + spin_unlock_irqrestore(&pfc->lock, flags); > + break; > > default: > return -ENOTSUPP; > @@ -534,6 +548,22 @@ static int sh_pfc_pinconf_set(struct pinctrl_dev > *pctldev, unsigned _pin, > > break; > > + case PIN_CONFIG_POWER_SOURCE: { > + unsigned int arg = > + pinconf_to_config_argument(configs[i]); > + > + if (!pfc->info->ops || !pfc->info->ops->set_low_voltage) > + return -ENOTSUPP; > + if (arg > 1) > + return -ENOTSUPP; > + > + spin_lock_irqsave(&pfc->lock, flags); > + pfc->info->ops->set_low_voltage(pfc, _pin, arg); > + spin_unlock_irqrestore(&pfc->lock, flags); > + > + break; > + } > + > default: > return -ENOTSUPP; > } > diff --git a/drivers/pinctrl/sh-pfc/sh_pfc.h > b/drivers/pinctrl/sh-pfc/sh_pfc.h index c7508d5f6886..b95d60bf2f1b 100644 > --- a/drivers/pinctrl/sh-pfc/sh_pfc.h > +++ b/drivers/pinctrl/sh-pfc/sh_pfc.h > @@ -13,6 +13,7 @@ > > #include <linux/bug.h> > #include <linux/stringify.h> > +#include <linux/pinctrl/pinconf-generic.h> Could you please keep the headers alphabetically sorted ? > > enum { > PINMUX_TYPE_NONE, > @@ -26,8 +27,11 @@ enum { > #define SH_PFC_PIN_CFG_OUTPUT (1 << 1) > #define SH_PFC_PIN_CFG_PULL_UP (1 << 2) > #define SH_PFC_PIN_CFG_PULL_DOWN (1 << 3) > +#define SH_PFC_PIN_CFG_LOW_VOLTAGE (1 << 4) As we should move away from using a bool to represent the voltage, I would name this SH_PFC_PIN_CFG_IO_VOLTAGE, or possibly SH_PFC_PIN_CFG_POWER_SOURCE. > #define SH_PFC_PIN_CFG_NO_GPIO (1 << 31) > > +struct sh_pfc; > + I don't think this is needed, struct sh_pfc is already forward-declared right above struct sh_pfc_soc_operations. > struct sh_pfc_pin { > u16 pin; > u16 enum_id; > @@ -121,6 +125,8 @@ struct sh_pfc_soc_operations { > unsigned int (*get_bias)(struct sh_pfc *pfc, unsigned int pin); > void (*set_bias)(struct sh_pfc *pfc, unsigned int pin, > unsigned int bias); > + bool (*get_low_voltage)(struct sh_pfc *pfc, unsigned int pin); > + void (*set_low_voltage)(struct sh_pfc *pfc, unsigned int pin, bool low); [gs]et_io_voltage() or [gs]et_power_source() ? > }; > > struct sh_pfc_soc_info {
On Mon, 2015-06-29 at 11:32 +0300, Laurent Pinchart wrote: > Hi Ben, > > Thank you for the patch. > > On Friday 26 June 2015 16:23:24 Ben Hutchings wrote: > > The pfc in the R8A7790 (and probably others in the R-Car gen 2 family) > > supports switching SDHI signals between 3.3V and 1.8V voltage, and the > > SD driver should do that when selecting a higher-speed mode. > > > > Add a flag for pins that support low voltage mode and SoC operations to > > get and set it. Implement the pinconf power-source parameter using these > > operations. > > As Geert has already pointed out, we should use an integer value instead of a > boolean for voltages. How about expressing them in microvolts as done by the > regulators API ? I did consider doing that but it seemed like over-engineering. However, given what Geert said about the other SoCs it seems worth doing. However, pinconf arguments are limited to 16 bits so I think I will have to make the units millivolts not microvolts. [...] > > diff --git a/drivers/pinctrl/sh-pfc/sh_pfc.h > > b/drivers/pinctrl/sh-pfc/sh_pfc.h index c7508d5f6886..b95d60bf2f1b 100644 > > --- a/drivers/pinctrl/sh-pfc/sh_pfc.h > > +++ b/drivers/pinctrl/sh-pfc/sh_pfc.h > > @@ -13,6 +13,7 @@ > > > > #include <linux/bug.h> > > #include <linux/stringify.h> > > +#include <linux/pinctrl/pinconf-generic.h> > > Could you please keep the headers alphabetically sorted ? > > > > > enum { > > PINMUX_TYPE_NONE, > > @@ -26,8 +27,11 @@ enum { > > #define SH_PFC_PIN_CFG_OUTPUT (1 << 1) > > #define SH_PFC_PIN_CFG_PULL_UP (1 << 2) > > #define SH_PFC_PIN_CFG_PULL_DOWN (1 << 3) > > +#define SH_PFC_PIN_CFG_LOW_VOLTAGE (1 << 4) > > As we should move away from using a bool to represent the voltage, I would > name this SH_PFC_PIN_CFG_IO_VOLTAGE, or possibly SH_PFC_PIN_CFG_POWER_SOURCE. Right. > > #define SH_PFC_PIN_CFG_NO_GPIO (1 << 31) > > > > +struct sh_pfc; > > + > > I don't think this is needed, struct sh_pfc is already forward-declared right > above struct sh_pfc_soc_operations. Right, this is left over from the previous version where I added an operation to struct sh_pfc_pin. > > struct sh_pfc_pin { > > u16 pin; > > u16 enum_id; > > @@ -121,6 +125,8 @@ struct sh_pfc_soc_operations { > > unsigned int (*get_bias)(struct sh_pfc *pfc, unsigned int pin); > > void (*set_bias)(struct sh_pfc *pfc, unsigned int pin, > > unsigned int bias); > > + bool (*get_low_voltage)(struct sh_pfc *pfc, unsigned int pin); > > + void (*set_low_voltage)(struct sh_pfc *pfc, unsigned int pin, bool low); > > [gs]et_io_voltage() or [gs]et_power_source() ? I prefer the former. Ben. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" 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/renesas,pfc-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt index bfe72ec055e3..93651742f1c1 100644 --- a/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt +++ b/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt @@ -69,7 +69,9 @@ Pin Configuration Node Properties: The pin configuration parameters use the generic pinconf bindings defined in pinctrl-bindings.txt in this directory. The supported parameters are -bias-disable, bias-pull-up and bias-pull-down. +bias-disable, bias-pull-up, bias-pull-down and power-source. For pins that +can operate with either 3.3V or 1.8V signals, the power-source value should +be 0 for 3.3V or 1 for 1.8V. GPIO diff --git a/drivers/pinctrl/sh-pfc/pinctrl.c b/drivers/pinctrl/sh-pfc/pinctrl.c index 072e7c62cab7..fa4066d4bc61 100644 --- a/drivers/pinctrl/sh-pfc/pinctrl.c +++ b/drivers/pinctrl/sh-pfc/pinctrl.c @@ -465,6 +465,9 @@ static bool sh_pfc_pinconf_validate(struct sh_pfc *pfc, unsigned int _pin, case PIN_CONFIG_BIAS_PULL_DOWN: return pin->configs & SH_PFC_PIN_CFG_PULL_DOWN; + case PIN_CONFIG_POWER_SOURCE: + return pin->configs & SH_PFC_PIN_CFG_LOW_VOLTAGE; + default: return false; } @@ -477,7 +480,6 @@ static int sh_pfc_pinconf_get(struct pinctrl_dev *pctldev, unsigned _pin, struct sh_pfc *pfc = pmx->pfc; enum pin_config_param param = pinconf_to_config_param(*config); unsigned long flags; - unsigned int bias; if (!sh_pfc_pinconf_validate(pfc, _pin, param)) return -ENOTSUPP; @@ -485,7 +487,9 @@ static int sh_pfc_pinconf_get(struct pinctrl_dev *pctldev, unsigned _pin, switch (param) { case PIN_CONFIG_BIAS_DISABLE: case PIN_CONFIG_BIAS_PULL_UP: - case PIN_CONFIG_BIAS_PULL_DOWN: + case PIN_CONFIG_BIAS_PULL_DOWN: { + unsigned int bias; + if (!pfc->info->ops || !pfc->info->ops->get_bias) return -ENOTSUPP; @@ -498,6 +502,16 @@ static int sh_pfc_pinconf_get(struct pinctrl_dev *pctldev, unsigned _pin, *config = 0; break; + } + + case PIN_CONFIG_POWER_SOURCE: + if (!pfc->info->ops || !pfc->info->ops->get_low_voltage) + return -ENOTSUPP; + + spin_lock_irqsave(&pfc->lock, flags); + *config = pfc->info->ops->get_low_voltage(pfc, _pin); + spin_unlock_irqrestore(&pfc->lock, flags); + break; default: return -ENOTSUPP; @@ -534,6 +548,22 @@ static int sh_pfc_pinconf_set(struct pinctrl_dev *pctldev, unsigned _pin, break; + case PIN_CONFIG_POWER_SOURCE: { + unsigned int arg = + pinconf_to_config_argument(configs[i]); + + if (!pfc->info->ops || !pfc->info->ops->set_low_voltage) + return -ENOTSUPP; + if (arg > 1) + return -ENOTSUPP; + + spin_lock_irqsave(&pfc->lock, flags); + pfc->info->ops->set_low_voltage(pfc, _pin, arg); + spin_unlock_irqrestore(&pfc->lock, flags); + + break; + } + default: return -ENOTSUPP; } diff --git a/drivers/pinctrl/sh-pfc/sh_pfc.h b/drivers/pinctrl/sh-pfc/sh_pfc.h index c7508d5f6886..b95d60bf2f1b 100644 --- a/drivers/pinctrl/sh-pfc/sh_pfc.h +++ b/drivers/pinctrl/sh-pfc/sh_pfc.h @@ -13,6 +13,7 @@ #include <linux/bug.h> #include <linux/stringify.h> +#include <linux/pinctrl/pinconf-generic.h> enum { PINMUX_TYPE_NONE, @@ -26,8 +27,11 @@ enum { #define SH_PFC_PIN_CFG_OUTPUT (1 << 1) #define SH_PFC_PIN_CFG_PULL_UP (1 << 2) #define SH_PFC_PIN_CFG_PULL_DOWN (1 << 3) +#define SH_PFC_PIN_CFG_LOW_VOLTAGE (1 << 4) #define SH_PFC_PIN_CFG_NO_GPIO (1 << 31) +struct sh_pfc; + struct sh_pfc_pin { u16 pin; u16 enum_id; @@ -121,6 +125,8 @@ struct sh_pfc_soc_operations { unsigned int (*get_bias)(struct sh_pfc *pfc, unsigned int pin); void (*set_bias)(struct sh_pfc *pfc, unsigned int pin, unsigned int bias); + bool (*get_low_voltage)(struct sh_pfc *pfc, unsigned int pin); + void (*set_low_voltage)(struct sh_pfc *pfc, unsigned int pin, bool low); }; struct sh_pfc_soc_info {
The pfc in the R8A7790 (and probably others in the R-Car gen 2 family) supports switching SDHI signals between 3.3V and 1.8V voltage, and the SD driver should do that when selecting a higher-speed mode. Add a flag for pins that support low voltage mode and SoC operations to get and set it. Implement the pinconf power-source parameter using these operations. Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk> --- .../bindings/pinctrl/renesas,pfc-pinctrl.txt | 4 ++- drivers/pinctrl/sh-pfc/pinctrl.c | 34 ++++++++++++++++++++-- drivers/pinctrl/sh-pfc/sh_pfc.h | 6 ++++ 3 files changed, 41 insertions(+), 3 deletions(-)