lkml.org 
[lkml]   [2019]   [Feb]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v2 12/15] pinctrl: milbeaut: Add Milbeaut M10V pinctrl
Date
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@socionext.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@socionext.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@linaro.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@linaro.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
>

\
 
 \ /
  Last update: 2019-02-13 06:03    [W:0.080 / U:0.404 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site