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:

quoted

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.

[...]

quoted

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 ?

quoted

 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.

quoted

 #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.

quoted

 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.