Hi Eugeniy,
sorry for the delay, the driver is different from what I'm
used to so I needed focuses attention and it took some time.
On Tue, Sep 11, 2018 at 5:09 PM Eugeniy Paltsev
<Eugeniy.Paltsev at synopsys.com> wrote:
> Add single-register MMIO GPIO driver for complex cases where
> only several fields in register belong to GPIO lines and each GPIO
> line owns a field with different length and on/off value.
>
> Such CREG GPIOs are used in Synopsys AXS10x and HSDK boards.
>
> Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev at synopsys.com>
> ---
> Changes v2->v3:
> * Move parameters into a lookup table instead of device tree.
> * Use the ngpios attribute for instead of snps,ngpios.
This is looking good! Just a few small things I want you to fix
(we can certainly queue this for the next kernel):
> +#include <linux/of.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/of_gpio.h>
Replace this with:
#include <linux/gpio/driver.h>
Drivers should only need to include that file. (Also move away from
mm_gpio_chip as per below.)
> +#include "gpiolib.h"
Why?
> +struct creg_gpio {
> + struct of_mm_gpio_chip mmchip;
I would prefer to move away from using this. Just create a regular driver
please. Just struct gpio_chip and add a member
void __iomem *base; instead of referring to mmchip.regs.
In fact I want to rewrite all mm_gpio_chips but I just haven't had time.
> +static void creg_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
Please rename the variable "gpio" to "offset".
This is clearer because "gpio" becomes quite ambigous.
> +static int creg_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
> +{
> + return 0; /* output */
> +}
I think with the recent code changes in gpiolib you do not need to define
this function at all.
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/commit/?h=devel&id=ae9847f48a4b4bff0335da20be63ac84d94eb54c
> +static int creg_gpio_xlate(struct gpio_chip *gc,
> + const struct of_phandle_args *gpiospec, u32 *flags)
> +{
> + if (gpiospec->args_count != 1) {
> + dev_err(&gc->gpiodev->dev, "invalid args_count: %d\n",
> + gpiospec->args_count);
> + return -EINVAL;
> + }
> +
> + if (gpiospec->args[0] >= gc->ngpio) {
> + dev_err(&gc->gpiodev->dev, "gpio number is too big: %d\n",
> + gpiospec->args[0]);
> + return -EINVAL;
> + }
> +
> + return gpiospec->args[0];
> +}
Another reason to not use mm_gpio_chip: just rely on standard twocell
translation and you can delete this.
> + /* Check that we suit in 32 bit register */
s/suit/fit
Yours,
Linus Walleij
