Message-ID: <de0177d8-65a4-0111-9060-606f25868bd1@socionext.com>
Date:   Wed, 13 Feb 2019 14:02:07 +0900
From:   "Sugaya, Taichi" <sugaya.taichi@...ionext.com>
To:     Linus Walleij <linus.walleij@...aro.org>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Takao Orito <orito.takao@...ionext.com>,
        Kazuhiro Kasai <kasai.kazuhiro@...ionext.com>,
        Shinji Kanematsu <kanematsu.shinji@...ionext.com>,
        Jassi Brar <jaswinder.singh@...aro.org>,
        Masami Hiramatsu <masami.hiramatsu@...aro.org>,
        Brian Masney <masneyb@...tation.org>
Subject: Re: [PATCH v2 12/15] pinctrl: milbeaut: Add Milbeaut M10V pinctrl

Hi,

Thank you for your comments.

On 2019/02/08 22:26, Linus Walleij wrote:
> Hi Sugaya,
> 
> thank you for the patch!
> 
> Since Masahiro has previously added the Uniphier pin control driver
> I would like him to provide a review of this patch, if possible. I also
> want to make sure that the hardware is different enough from the
> existing Uniphier pin control so that it really needs a new
> driver, else I suppose it should be refactored to be part of
> the pinctrl/uniphier drivers (we can of course rename it
> pinctrl/socionext if only naming is an issue).

As Masahiro mentioned Millbeaut is totally different from UniPhier. So I 
would like to put files individually.

> 
> On Fri, Feb 8, 2019 at 1:27 PM Sugaya Taichi
> <sugaya.taichi@...ionext.com> wrote:
> 
>> Add Milbeaut M10V pinctrl.
>> The M10V has the pins that can be used GPIOs or take multiple other
>> functions.
>>
>> Signed-off-by: Sugaya Taichi <sugaya.taichi@...ionext.com>
> 
> This file seems to be mostly authored by Jassi Brar 
That's right.
as he is
> listed as author in the top of the source code.
> 
> Please at least add Signed-off-by: Jassi Brar <jassi.brar@...aro.org>
> above your signed-off-by to reflect the delivery path.
> 
> Also usually author is set to indicate the person who wrote
> the majority of the code so consider:
> git commit --amend --author="Jassi Brar <jassi.brar@...aro.org>"
> if Jassi wrote the majority of the code.
> Since I will ask you to change a bunch of stuff in the driver
> I don't know who the majority author will be in the end.
> 
> When you send out the patches this will reflect in a
> From: ... line at the top of the patch.
> 
> I'm not overly sensitive about credits but this seems like
> a simple thing to fix.
> 
I will decide the author in discussion with him. If Jassi will be,
I remember to add a "From:..." line.

>> +config PINCTRL_MILBEAUT
>> +       bool
>> +       select PINMUX
>> +       select PINCONF
>> +       select GENERIC_PINCONF
>> +       select OF_GPIO
>> +       select REGMAP_MMIO
>> +       select GPIOLIB_IRQCHIP
> 
> Nice reuse of the frameworks! But this driver doesn't
> really use GPIOLIB_IRQCHIP, at least not as it looks now.
> 

I got it, drop this line.

>> +#include <linux/gpio.h>
> 
> Please use only <linux/gpio/driver.h> on new code.
> It should be just as fine.
> 
OK, use it instead.


>> +#include <linux/pinctrl/machine.h>
> 
> This include should not be needed.
> 

I got it, drop this line.

>> +struct pin_irq_map {
>> +       int pin; /* offset of pin in the managed range */
>> +       int irq; /* virq of the pin as fpint */
>> +       int type;
>> +       char irqname[8];
>> +};
> 
> If pins are mapped 1-to-1 to IRQs from another irqchip,
> we are dealing with a hierarchical interrupt controller.
> But I guess I learn more as I read the code.
>
Yes, some pins of GPIO are available to be used as external interrupt.


>> +static void _set_mux(struct m10v_pinctrl *pctl, unsigned int pin, bool gpio)
> 
> I don't like __underscore_prefix on functions since they
> are semantically ambiguous. Try to find the perfect name that
> reflects what the function is really doing. m10v_pmx_commit_mux_setting()
> if nothing else.
> 

OK, consider perfect name.

> I do not understand the "gpio" argument for this function so please
> explain it:
> 
>> +       if (gpio)
>> +               val &= ~BIT(offset);
>> +       else
>> +               val |= BIT(offset);
> 
> So this bit is set if "gpio" is true, and that comes from here:
> 
>> +static int m10v_pmx_set_mux(struct pinctrl_dev *pctldev,
>> +                            unsigned int function,
>> +                            unsigned int group)
>> +{
>> +       struct m10v_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
>> +       u32 pin = pctl->gpins[group][0]; /* each group has exactly 1 pin */
>> +
>> +       _set_mux(pctl, pin, !function);
> 
> So if no special function is passed to the .set_mux() callback,
> we assume that the pin is used for GPIO. Write this in a comment
> if I understand it correctly, so it gets easy to read the code.

That's right!!
OK, add a comment about "gpio".

> 
>> +static int _set_direction(struct m10v_pinctrl *pctl,
>> +                       unsigned int pin, bool input)
> 
> Same issue with __underscore.
> m10v_set_direction_commit()?
> 
I got it too.
Try to consider nice name.

>> +static int
>> +m10v_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
>> +                       struct pinctrl_gpio_range *range,
>> +                       unsigned int pin, bool input)
>> +{
>> +       struct m10v_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
>> +
>> +       return _set_direction(pctl, pin, input);
>> +}
>> +
>> +static int
>> +m10v_pmx_gpio_request_enable(struct pinctrl_dev *pctldev,
>> +                           struct pinctrl_gpio_range *range,
>> +                           unsigned int pin)
>> +{
>> +       struct m10v_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
>> +
>> +       _set_mux(pctl, pin, true);
>> +       return 0;
>> +}
> 
> This is a good solution.
> 

Thank you.

>> +static const struct pinmux_ops m10v_pmx_ops = {
>> +       .get_functions_count    = m10v_pmx_get_funcs_cnt,
>> +       .get_function_name      = m10v_pmx_get_func_name,
>> +       .get_function_groups    = m10v_pmx_get_func_groups,
>> +       .set_mux                = m10v_pmx_set_mux,
>> +       .gpio_set_direction     = m10v_pmx_gpio_set_direction,
>> +       .gpio_request_enable    = m10v_pmx_gpio_request_enable,
>> +};
> 
> If GPIO and other functions cannot be used at the same time,
> then consider setting .strict = true, in struct pinmux_ops.
> (Else skip it, maybe add a comment that GPIO lines can be
> used orthogonal to functions.)
> 

They cannot be used the same time, so add a line ".strict = true,"

>> +static int m10v_gpio_get(struct gpio_chip *gc, unsigned int group)
>> +{
>> +       struct m10v_pinctrl *pctl =
>> +               container_of(gc, struct m10v_pinctrl, gc);
> 
> Please set the gpio chip data to struct m10v_pinctrl * and use:
> 
> struct m10v_pinctrl *pctl = gpiochip_get_data(gc);
> 
> in all these functions.
>

I got it.

>> +static void (*gpio_set)(struct gpio_chip *, unsigned int, int);
> 
> Why is this forward-declaration here in the middle of the code?
> It seems unnecessary, at least it warrants a comment.
> 

This is a remnant of code refining and no longer needed.
I will drop this line and use m10v_gpio_set() directly.

>> +static int m10v_gpio_to_irq(struct gpio_chip *gc, unsigned int offset)
>> +{
>> +       struct m10v_pinctrl *pctl =
>> +               container_of(gc, struct m10v_pinctrl, gc);
>> +
>> +       return irq_linear_revmap(pctl->irqdom, offset);
>> +}
> 
> Something is fishy here because when you use GPIOLIB_IRQCHIP
> .to_irq() should be handled by the gpiolib core.
> 
> However if you rewrite this driver to use hierarchical irqdomain,
> you still have to provide a .to_irq() function.
> 

I see. Try to use hierarchical irqdomain.

>> +static struct lock_class_key gpio_lock_class;
>> +static struct lock_class_key gpio_request_class;
> 
> These forward declarations should not be here, they should
> be coming from <linux/gpio/driver.h>
> 

OK.
Therefore I think GPIOLIB_IRQCHIP is needed.

>> +static int pin_to_extint(struct m10v_pinctrl *pctl, int pin)
>> +{
>> +       int extint;
>> +
>> +       for (extint = 0; extint < pctl->extints; extint++)
>> +               if (pctl->fpint[extint].pin == pin)
>> +                       break;
>> +
>> +       if (extint == pctl->extints)
>> +               return -1;
>> +
>> +       return extint;
>> +}
> 
> This looks like it should be a hierarchical irqchip.
> 
> I am working on generic mappings of parent<->child IRQ
> offsets for gpiolib but don't know when it will be there.
> 

OK, study and try to use irqchip.

>> +static void update_trigger(struct m10v_pinctrl *pctl, int extint)
> 
> Normally this is .set_type() on the irqchip.
> 

OK.

>> +{
>> +       int type = pctl->fpint[extint].type;
> 
> But since you are not using hierarchical irqchip, this stuff
> appears here instead, which becomes a kind of workaround.
> So use hierarchical irqchip instead.
> 

I see.
I try to drop this line.

>> +static irqreturn_t m10v_gpio_irq_handler(int irq, void *data)
>> +{
>> +       struct m10v_pinctrl *pctl = data;
>> +       int i, pin;
>> +       u32 val;
>> +
>> +       for (i = 0; i < pctl->extints; i++)
>> +               if (pctl->fpint[i].irq == irq)
>> +                       break;
>> +       if (i == pctl->extints) {
>> +               pr_err("%s:%d IRQ(%d)!\n", __func__, __LINE__, irq);
>> +               return IRQ_NONE;
>> +       }
>> +
>> +       if (!pctl->exiu)
>> +               return IRQ_NONE;
>> +
>> +       val = readl_relaxed(pctl->exiu + EIREQSTA);
>> +       if (!(val & BIT(i))) {
>> +               pr_err("%s:%d i=%d EIREQSTA=0x%x IRQ(%d)!\n",
>> +                               __func__, __LINE__, i, val, irq);
>> +               return IRQ_NONE;
>> +       }
>> +
>> +       pin = pctl->fpint[i].pin;
>> +       generic_handle_irq(irq_linear_revmap(pctl->irqdom, pin));
>> +
>> +       return IRQ_HANDLED;
>> +}
> 
> This becomes a complex workaround for not having hierarchical
> irqchip.
> 

OKay!

>> +static int m10v_pinctrl_probe(struct platform_device *pdev)
>> +{
>> +       struct device_node *np = pdev->dev.of_node;
>> +       struct pinctrl_dev *pctl_dev;
>> +       struct pin_irq_map fpint[32];
>> +       struct m10v_pinctrl *pctl;
>> +       struct pinctrl_desc *pd;
>> +       struct gpio_chip *gc;
>> +       struct resource *res;
>> +       int idx, i, ret, extints, tpins;
>> +
>> +       extints = of_irq_count(np);
> 
> So this means that you have all the IRQs from the parent interrupt
> controller defined in the device tree.
> 

Yes.

> This has been done in some drivers but they are bad example.
> 
> Instead use hierarchical irqchip, and do the mappings from parent
> to child offset directly in the driver.
> 
> Hierarchial irqdomains are not very old, so I am sorry if the kernel
> contains a number of bad examples.
> 
> Examples:
> drivers/irqchip/irq-meson-gpio.c
> drivers/pinctrl/stm32/pinctrl-stm32.c
> https://marc.info/?l=linux-arm-kernel&m=154923023907623&w=2
> https://marc.info/?l=linux-arm-kernel&m=154923038707686&w=2
> 
> You can also look at Brian Masney's series where he's converting
> some of Qualcomms drivers to use hierarchical interrupts.
> 

Thank you for suggestion.
I will refer to it and try to converting.

>> +       pctl = devm_kzalloc(&pdev->dev, sizeof(*pctl) +
>> +                               sizeof(struct pin_irq_map) * extints,
>> +                               GFP_KERNEL);
> 
> This should use struct_size() these days, but this will not be needed
> when you switch to hierarchical irqdomain.
> 

I got it.

>> +       for (idx = 0, i = 0; i < pctl->extints; i++) {
>> +               int j = 0, irq = platform_get_irq(pdev, i);
> 
> And this code goes away as well. Instead the mapping from
> child to parent irq offset happens in statically assigned data.
> If that varies between implementations of this pin controller,
> you should use unique compatible strings to discern them.
> 

I see.

>> +       gc->base = -1;
>> +       gc->ngpio = tpins;
>> +       gc->label = dev_name(&pdev->dev);
>> +       gc->owner = THIS_MODULE;
>> +       gc->of_node = np;
>> +       gc->direction_input = m10v_gpio_direction_input;
>> +       gc->direction_output = m10v_gpio_direction_output;
>> +       gc->get = m10v_gpio_get;
>> +       gc->set = gpio_set;
>> +       gc->to_irq = m10v_gpio_to_irq;
>> +       gc->request = gpiochip_generic_request;
>> +       gc->free = gpiochip_generic_free;
>> +       ret = gpiochip_add(gc);
> 
> Please use devm_gpiochip_add_data() and pass the pin controller struct
> as data.
>

OK.

Thanks.
Sugaya Taichi

> Yours,
> Linus Walleij
> 

Powered by blists - more mailing lists