RE: [PATCH 1/7] pinctrl: Renesas RZ/A1 pin and gpio controller

[Chris Brandt <Chris.Brandt@xxxxxxxxxxx>]

Hi Jacopo,

On Monday, February 20, 2017, Jacopo Mondi wrote:
> 
> Add combined gpio and pin controller driver for Renesas RZ/A1
> r7s72100 SoC.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
> ---
>  drivers/pinctrl/Kconfig        |   10 +
>  drivers/pinctrl/Makefile       |    1 +
>  drivers/pinctrl/pinctrl-rza1.c | 1026
> ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 1037 insertions(+)
>  create mode 100644 drivers/pinctrl/pinctrl-rza1.c
> 
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index 8f8c2af..61310ac 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -163,6 +163,16 @@ config PINCTRL_ROCKCHIP
>  	select GENERIC_IRQ_CHIP
>  	select MFD_SYSCON
> 
> +config PINCTRL_RZA1
> +	bool "Renesas r7s72100 RZ/A1 gpio and pinctrl driver"

You can drop the "r7s72100" and just say RZ/A1.



> +/**
> + * rza1_pin_set_direction() - set I/O direction on a pin in port mode
> + *
> + * When running in output port mode keep PBDC enabled to allow reading
> the
> + * pin value from PPR.
> + * When in alternate mode disable that (if not explicitly required) not
> to
> + * interfere with the alternate function mode.
> + *
> + * @port: port where pin sits on
> + * @offset: pin offset
> + * @input: input enable/disable flag
> + */
> +static inline void rza1_pin_set_direction(struct rza1_port *port,
> +					  unsigned int offset,
> +					  bool input)
> +{
> +	spin_lock(&port->lock);
> +	if (input)
> +		rza1_set_bit(port, PIBC_REG, offset, 1);
> +	else {
> +		rza1_set_bit(port, PM_REG, offset, 0);
> +		rza1_set_bit(port, PBDC_REG, offset, 1);
> +	}
> +	spin_unlock(&port->lock);

When input==true, you only set PIBC_REG. Otherwise you only set PM_REG and PBDC_REG. This would imply that this function will only ever get called once for a pin and never get called a second time with a different direction...meaning it would not really change a pin from output to input. I would assume that you would need to change this function to set those 3 registers either one way or the other.

I would suggest:
   PIBC_REG = 1 /* always gets set for GPIO mode */
 input:
   PM_REG = 1
   PBDC_REG = 0
output:
   PM_REG = 0
   PBDC_REG = 1



> +/**
> + * rza1_alternate_function_conf() - configure pin in alternate function
> mode
> + *
> + * @pinctrl: RZ/A1 pin controller device
> + * @pin_conf: single pin configuration descriptor
> + */
> +static int rza1_alternate_function_conf(struct rza1_pinctrl *rza1_pctl,
> +					struct rza1_pin_conf *pin_conf)
> +{
> +	unsigned int offset = pin_conf->offset;
> +	struct rza1_port *port = &rza1_pctl->ports[pin_conf->port];
> +	u8 mux_mode = (pin_conf->mux_conf - 1) & MUX_FUNC_MASK;
> +	u8 mux_conf = pin_conf->mux_conf >> MUX_FUNC_OFFS;
> +	bool swio_en = !!(mux_conf & MUX_CONF_SWIO);
> +	bool input_en = !!(mux_conf & MUX_CONF_INPUT_ENABLE);
> +
> +	rza1_pin_reset(port, offset);
> +
> +	/*
> +	 * When configuring pin with Software Controlled IO mode in
> alternate
> +	 * mode, do not enable bi-directional control to avoid driving Pn
> +	 * value to the pin input.
> +	 * When working in direct IO mode (aka alternate function drives the
> +	 * pin direction), enable bi-directional control for input pins in
> +	 * order to enable the pin's input buffer as a consequence.
> +	 */
> +	if ((!swio_en && input_en) || (swio_en && !input_en))
> +		rza1_set_bit(port, PBDC_REG, offset, 1);


Maybe just set PBDC_REG at the end of the function with everything else.
See below...



> +
> +	/*
> +	 * Enable alternate function mode and select it.
> +	 *
> +	 * ----------------------------------------------------
> +	 * Alternate mode selection table:
> +	 *
> +	 * PMC	PFC	PFCE	PFCAE	mux_mode
> +	 * 1	0	0	0	0
> +	 * 1	1	0	0	1
> +	 * 1	0	1	0	2
> +	 * 1	1	1	0	3
> +	 * 1	0	0	1	4
> +	 * 1	1	0	1	5
> +	 * 1	0	1	1	6
> +	 * 1	1	1	1	7
> +	 * ----------------------------------------------------
> +	 */
> +	rza1_set_bit(port, PFC_REG, offset, mux_mode & MUX_FUNC_PFC_MASK);
> +	rza1_set_bit(port, PFCE_REG, offset, mux_mode & MUX_FUNC_PFCE_MASK);
> +	rza1_set_bit(port, PFCEA_REG, offset, mux_mode &
> MUX_FUNC_PFCEA_MASK);
> +
> +	/*
> +	 * All alternate functions except a few (4) need PIPCn = 1.
> +	 * If PIPCn has to stay disabled (SW IO mode), configure PMn
> according
> +	 * to I/O direction specified by pin configuration -after- PMC has
> been
> +	 * set to one.
> +	 */
> +	if (!swio_en)
> +		rza1_set_bit(port, PIPC_REG, offset, 1);
> +
> +	rza1_set_bit(port, PMC_REG, offset, 1);
> +
> +	if (swio_en)
> +		rza1_set_bit(port, PM_REG, offset, input_en);


I would suggest something like this:

	if (bidir)
		rza1_set_bit(port, PBDC_REG, offset, 1);
	else {
		rza1_set_bit(port, PBDC_REG, offset, 0);
		rza1_set_bit(port, PM_REG, offset, swio_in);
	}

	rza1_set_bit(port, PMC_REG, offset, 1);


> +
> +	return 0;
> +}




Chris

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