Hi Andrei,
thanks for your patch!
On Wed, Nov 7, 2018 at 4:12 PM <Andrei.Stefanescu@xxxxxxxxxxxxx> wrote:
> PIOBU pins do not lose their voltage during Backup/Self-refresh.
> This patch adds a simple GPIO controller for them.
>
> This driver adds support for using the pins as GPIO
> offering the possibility to read/set the voltage.
>
> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@xxxxxxxxxxxxx>
(...)
> +config GPIO_SAMA5D2_PIOBU
> + tristate "SAMA5D2 PIOBU GPIO support"
> + depends on GPIO_SYSCON
Should it not be:
depends on MFD_SYSCON
select GPIO_SYSCON
> +#include <linux/gpio.h>
Drivers should only
#include <linux/gpio/driver.h>
Also add:
#include <linux/bits.h>
because you use BIT() macros.
> +#define PIOBU_NUM 8
> +#define PIOBU_REG_SIZE 4
Isnt register size 4 implied by syscon, I think it is hardwired
to 32 bits.
> +/*
> + * sama5d2_piobu_setup_pin - prepares a pin for sama5d2_piobu_set_direction call
> + *
> + * Do not consider pin for intrusion detection (normal and backup modes)
> + * Do not consider pin as intrusion wakeup interrupt source
Intrusion ... sounds like some burglar alarm :D
> +/*
> + * sama5d2_piobu_get_direction - gpiochip get_direction
> + */
Check all your kerneldoc comments please,
/**
* this_is_a_function() - This is a function
*/
See
Documentation/doc-guide/kernel-doc.rst
> +static int sama5d2_piobu_get_direction(struct gpio_chip *chip,
> + unsigned int pin)
> +{
> + int ret = sama5d2_piobu_read_value(chip, pin, PIOBU_DIRECTION);
> +
> + if (ret < 0)
> + return ret;
> +
> + return (ret == PIOBU_IN) ? GPIOF_DIR_IN : GPIOF_DIR_OUT;
Please do not use these flags in drivers: they are for consumers.
Just return 0/1.
> +static int sama5d2_piobu_get(struct gpio_chip *chip, unsigned int pin)
> +{
> + /* if pin is input, read value from PDS else read from SOD */
> + int ret = sama5d2_piobu_get_direction(chip, pin);
> +
> + if (ret == GPIOF_DIR_IN)
> + ret = sama5d2_piobu_read_value(chip, pin, PIOBU_PDS);
> + else if (ret == GPIOF_DIR_OUT)
> + ret = sama5d2_piobu_read_value(chip, pin, PIOBU_SOD);
Dito.
> +static void sama5d2_piobu_set(struct gpio_chip *chip, unsigned int pin,
> + int value)
> +{
> + value = (value == 0) ? PIOBU_LOW : PIOBU_HIGH;
That looks unorthodox. Just use an if() statement.
> +const struct gpio_chip sama5d2_piobu_template_chip = {
> + .owner = THIS_MODULE,
> + .get_direction = sama5d2_piobu_get_direction,
> + .direction_input = sama5d2_piobu_direction_input,
> + .direction_output = sama5d2_piobu_direction_output,
> + .get = sama5d2_piobu_get,
> + .set = sama5d2_piobu_set,
> + .base = -1,
> + .ngpio = PIOBU_NUM,
> + .can_sleep = 0,
(...)
> + piobu->chip = sama5d2_piobu_template_chip;
> + piobu->chip.label = pdev->name;
> + piobu->chip.parent = &pdev->dev;
> + piobu->chip.of_node = pdev->dev.of_node;
This is just overcomplicating things. Instead of assigning the template
just assign all function here in the probe() code, get rid of the
templaye. It's easier to follow.
> + piobu->regmap =
> + syscon_regmap_lookup_by_compatible("microchip,sama5d2-piobu");
So maybe just put this inside your syscon node and use:
syscon_node_to_regmap(pdev->dev.parent->of_node);
I might have more comments on the next version, but keep at it!
Yours,
Linus Walleij
