RE: [PATCH 2/3] gpio: gpio-rz: GPIO driver for Renesas RZ series

[Chris Brandt]

Hi Jacopo,

On Thursday, January 12, 2017, jacopo mondi wrote:
> Oh! That's weird I don't have that statement in my manual (public version
> R01UH0403EJ0060 Rev.0.60)

If you go to www.renesas.com and search for "RZ/A1H User Manual", you will
see that a new version 3.0 manual was recently posted. This new version contains
many new descriptions and precautions. It also now includes the SDHI chapter
which previously was always omitted because of legal reasons.

There is a separate manual for the RZ/A1L. The peripherals are all the same,
but the function pinouts are different because the packages are smaller. Hence,
all the existing upstream drivers for RZ/A1H work fine for RZ/A1M and RZ/A1L
without modification...except the pins comes out on different ports...and
the sh-pfc driver won't work well in this situation.


Chris

Re: [PATCH 2/3] gpio: gpio-rz: GPIO driver for Renesas RZ series

[jacopo mondi]

Hi Chris,

On 12/01/2017 15:39, Chris Brandt wrote:

Hi Jacopo,

On Thursday, January 12, 2017, jacopo mondi wrote:

To read the pin direction I would need to add one more register to the
"reg" property range provided in the DTS.
This is of course doable, but I would have two questions here:
- why did you chose to use PMSR register instead of reading/writing
directly to PM? Am I missing something?



I guess you can still use the PMSR register to get the current direction
of the I/O pin. According to the HW manual:

"When reading, the higher 16 bits are read as H. The lower 16 bits
 are read as the value of the PMn register."



Oh! That's weird I don't have that statement in my manual (public 
version R01UH0403EJ0060 Rev.0.60)


The only description for PMSR I have is:

"This register provides an alternative method for writing data to the 
PMn register.
The higher 16 bits of the PMSRn register specify whether data can be 
written to the PMn.PMnm bit specified by the

lower 16 bits of the PMSRn register."

So I assumed it was one direction only



While the PMSR register is a cool idea of having a special register that
can change a single port pin direction without doing a read-modify-write
by the CPU, it doesn't seem to be a standard option in Renesas SoCs (well,
except for the older NEC V850 parts where this PFC HW came from).
So a more traditional method of just using the Pmn register seems to be
more compatible across Renesas SoCs.


Chris

RE: [PATCH 2/3] gpio: gpio-rz: GPIO driver for Renesas RZ series

[Chris Brandt]

Hi Jacopo,

On Thursday, January 12, 2017, jacopo mondi wrote:
> To read the pin direction I would need to add one more register to the
> "reg" property range provided in the DTS.
> This is of course doable, but I would have two questions here:
> - why did you chose to use PMSR register instead of reading/writing
> directly to PM? Am I missing something?


I guess you can still use the PMSR register to get the current direction
of the I/O pin. According to the HW manual:

"When reading, the higher 16 bits are read as H. The lower 16 bits
 are read as the value of the PMn register."


While the PMSR register is a cool idea of having a special register that
can change a single port pin direction without doing a read-modify-write
by the CPU, it doesn't seem to be a standard option in Renesas SoCs (well,
except for the older NEC V850 parts where this PFC HW came from).
So a more traditional method of just using the Pmn register seems to be
more compatible across Renesas SoCs.


Chris

Re: [PATCH 2/3] gpio: gpio-rz: GPIO driver for Renesas RZ series

[jacopo mondi]

Hi Linus,
thanks for review

On 11/01/2017 15:55, Linus Walleij wrote:

On Mon, Jan 9, 2017 at 8:31 PM, Jacopo Mondi  wrote:





+static void rz_gpio_free(struct gpio_chip *chip, unsigned offset)
+{
+   pinctrl_free_gpio(chip->base + offset);
+
+   /* Set the GPIO as an input to ensure that the next GPIO request won't
+   * drive the GPIO pin as an output.
+   */
+   rz_gpio_direction_input(chip, offset);
+}


Very close to gpiochip_generic_free()

Maybe we should actually set lines as input in the
generic routine!



Is it ok then to simply substitute "pinctrl_free_gpio" with 
"gpiochip_generic_free" and keep rz_gpio_direction_input here?




+   gpio_chip = >gpio_chip;
+   gpio_chip->direction_input = rz_gpio_direction_input;
+   gpio_chip->get = rz_gpio_get;
+   gpio_chip->direction_output = rz_gpio_direction_output;


Please implement .get_direction() as well.



That's funny how a simple addition like this one would require several 
changes (possibly even in the dts ABI defined by this driver).


This is more a question for the original driver author I hope is 
listening here:


To read the pin direction I would need to add one more register to the 
"reg" property range provided in the DTS.

This is of course doable, but I would have two questions here:
- why did you chose to use PMSR register instead of reading/writing 
directly to PM? Am I missing something?
- wouldn't it be better to just receive the port base address from the 
"reg" property and offset from that instead of having the 3 register 
addresses specified in the dts?


See, the dts now looks like this:

reg = <0xfcfe3100 0x4>, /* PSR */
  <0xfcfe3200 0x2>, /* PPR */
  <0xfcfe3800 0x4>; /* PMSR */

Shouldn't we just receive the gpiochip base address and the offset as we 
like?


Thanks
   j

Re: [PATCH 2/3] gpio: gpio-rz: GPIO driver for Renesas RZ series

[Linus Walleij]

On Mon, Jan 9, 2017 at 8:31 PM, Jacopo Mondi  wrote:

> From: Magnus Damm 
>
> This commit combines Magnus' original driver and minor fixes to
> forward-port it to a more recent kernel version (v4.10)
>
> Signed-off-by: Jacopo Mondi 
>
> ---
> gpio: Renesas RZ GPIO driver V3
>
> This patch adds a GPIO driver for the RZ series of SoCs from
> Renesas. The V3 of the driver requires DT to be used.
>
> The hardware allows control of GPIOs in blocks of up to 16 pins.
> Interrupts are not included in this hardware block, if interrupts
> are needed then the PFC needs to be configured to a IRQ pin function
> which is handled by the GIC hardware.
>
> Tested with yet-to-be-posted DT devices on r7s72100 and Genmai using
> LEDs, DIP switches and I2C bitbang.
>
> Signed-off-by: Magnus Damm 
>
> gpio: gpio-rz: Port to v4.10-rc1
>
> Fix invalid return value in gpio remove function and change gpiochip's
> "dev" field name to "parent" to compile driver against v4.10
>
> Signed-off-by: Jacopo Mondi 

This commit message looks seriously mangled, please make it look like a regular
one with all signed offs at the end.

> +config GPIO_RZ
> +   tristate "Renesas RZ GPIO"
> +   depends on ARM

ARM really? Not some Renesas or so?
Include || COMPILE_TEST?

> +#include 
> +#include 
> +#include 

No. Use only
#include 

> +static inline struct rz_gpio_priv *gpio_to_priv(struct gpio_chip *chip)
> +{
> +   return container_of(chip, struct rz_gpio_priv, gpio_chip);
> +}

Please get rid of this and use gpiochip_get_data()
after adding the chip with devm_gpiochip_add_data()
supplying the data you need.

Look at other GPIO drivers for examples.

> +static int rz_gpio_get(struct gpio_chip *chip, unsigned offset)
> +{
> +   /* Get bit from PPR register to determine pin state */
> +   return rz_gpio_read_ppr(gpio_to_priv(chip), offset);

return !!rz_gpio_read_ppr(gpio_to_priv(chip), offset);

To clamp to bool.

> +static void rz_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> +{
> +   /* Set bit in P register via PSR to control output */
> +   rz_gpio_write(gpio_to_priv(chip), REG_PSR, offset, !!value);
> +}

Ironically you can trust the value to be 0/1 here. Check the
gpiolib.

> +static int rz_gpio_request(struct gpio_chip *chip, unsigned offset)
> +{
> +   return pinctrl_request_gpio(chip->base + offset);
> +}

What about just using gpiochip_generic_request instead?

> +static void rz_gpio_free(struct gpio_chip *chip, unsigned offset)
> +{
> +   pinctrl_free_gpio(chip->base + offset);
> +
> +   /* Set the GPIO as an input to ensure that the next GPIO request won't
> +   * drive the GPIO pin as an output.
> +   */
> +   rz_gpio_direction_input(chip, offset);
> +}

Very close to gpiochip_generic_free()

Maybe we should actually set lines as input in the
generic routine!

> +   gpio_chip = >gpio_chip;
> +   gpio_chip->direction_input = rz_gpio_direction_input;
> +   gpio_chip->get = rz_gpio_get;
> +   gpio_chip->direction_output = rz_gpio_direction_output;

Please implement .get_direction() as well.

> +   gpio_chip->set = rz_gpio_set;
> +   gpio_chip->request = rz_gpio_request;
> +   gpio_chip->free = rz_gpio_free;
> +   gpio_chip->label = dev_name(>dev);
> +   gpio_chip->parent = >dev;
> +   gpio_chip->owner = THIS_MODULE;
> +   gpio_chip->base = -1;
> +   gpio_chip->ngpio = ret == 0 ? args.args[2] : RZ_GPIOS_PER_PORT;
> +
> +   ret = gpiochip_add(gpio_chip);

Use devm_gpiochip_add_data() providing struct rz_gpio_priv * as
data.

> +static int rz_gpio_remove(struct platform_device *pdev)
> +{
> +   struct rz_gpio_priv *p = platform_get_drvdata(pdev);
> +
> +   gpiochip_remove(>gpio_chip);
> +   return 0;
> +}

And with the devm_gpiochip_add_data() you don't even need
this remove().

Yours,
Linus Walleij