lkml.org 
[lkml]   [2018]   [Nov]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH 3/3] gpio: add driver for sama5d2 PIOBU pins
Hi Andrei,

thanks for your patch!

On Wed, Nov 7, 2018 at 4:12 PM <Andrei.Stefanescu@microchip.com> 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@microchip.com>

(...)
> +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

\
 
 \ /
  Last update: 2018-11-15 10:57    [W:0.089 / U:27.096 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site