Messages in this thread | | | From | Lars Povlsen <> | Subject | Re: [PATCH v7 2/3] pinctrl: pinctrl-microchip-sgpio: Add pinctrl driver for Microsemi Serial GPIO | Date | Mon, 9 Nov 2020 13:07:23 +0100 |
| |
Andy Shevchenko writes:
> On Thu, Oct 29, 2020 at 3:40 PM Lars Povlsen <lars.povlsen@microchip.com> wrote: >> >> This adds a pinctrl driver for the Microsemi/Microchip Serial GPIO >> (SGPIO) device used in various SoC's. > > First Q is can you use gpio-regmap? > Second one, why this driver is a pin control? I haven't found any > evidence it can be plain GPIO.
I think I responded in <87blgp9hhv.fsf@soft-dev15.microsemi.net>, did you not see that?
> > Also note, if comment is given about one part of the code, you need to > check all the rest which are similar and address accordingly. > > ... > >> +config PINCTRL_MICROCHIP_SGPIO >> + bool "Pinctrl driver for Microsemi/Microchip Serial GPIO" > >> + depends on OF > > I think this is not needed, see below.
I will remove the OF dependency, I think you are right it can be done. I just did not see that as a goal in itself.
> >> + depends on HAS_IOMEM >> + select GPIOLIB >> + select GENERIC_PINCONF >> + select GENERIC_PINCTRL_GROUPS >> + select GENERIC_PINMUX_FUNCTIONS > >> + select OF_GPIO > > ...neither this...
OK.
> >> + help >> + Support for the serial GPIO interface used on Microsemi and >> + Microchip SoC's. By using a serial interface, the SIO >> + controller significantly extends the number of available >> + GPIOs with a minimum number of additional pins on the >> + device. The primary purpose of the SIO controller is to >> + connect control signals from SFP modules and to act as an >> + LED controller. > > ... > > Missed header here, like bits.h.
I'll add that.
> >> +#include <linux/gpio/driver.h> >> +#include <linux/interrupt.h> >> +#include <linux/io.h> > >> +#include <linux/of_device.h> >> +#include <linux/of_irq.h> >> +#include <linux/of_platform.h> > > I think this driver is OF independent, if you convert the OF leftovers > to device_/fwnode_ API.
As stated, I'll be removing the OF parts.
> > Then you need to drop these headers (most of them actually are > redundant even now) and add property.h. Also you missed > mod_devicetable.h. > >> +#include <linux/clk.h> >> +#include <linux/pinctrl/pinctrl.h> >> +#include <linux/pinctrl/pinmux.h> >> +#include <linux/pinctrl/pinconf.h> >> +#include <linux/pinctrl/pinconf-generic.h> >> +#include <linux/platform_device.h> > > Perhaps ordered and linux/pinctrl/ be grouped after generic ones?
Sure, I can do that. There's some that *are* needed, but your're right that some are redundant.
> > ... > >> +#define __shf(x) (__builtin_ffs(x) - 1) >> +#define __BF_PREP(bf, x) (bf & ((x) << __shf(bf))) >> +#define __BF_GET(bf, x) (((x & bf) >> __shf(bf))) > > Isn't it home grown reimplementation of bitfield.h? >
This was answered in the aforementioned mail.
> ... > >> +static int sgpio_gpio_request_enable(struct pinctrl_dev *pctldev, >> + struct pinctrl_gpio_range *range, >> + unsigned int offset) >> +{ >> + struct sgpio_bank *bank = pinctrl_dev_get_drvdata(pctldev); >> + struct sgpio_priv *priv = bank->priv; >> + struct sgpio_port_addr addr; >> + >> + sgpio_pin_to_addr(priv, offset, &addr); >> + >> + if ((priv->ports & BIT(addr.port)) == 0) { >> + dev_warn(priv->dev, "%s: Request port %d for pin %d is not activated\n", >> + __func__, addr.port, offset); > > Don't use __func__ in messages, it's rarely needed and here I believe > is not the case. >
Ok, I will drop that.
>> + } >> + >> + return 0; >> +} > > ... > >> + /* Note that the SGIO pin is defined by *2* numbers, a port >> + * number between 0 and 31, and a bit index, 0 to 3. >> + */ > > /* > * Fix multi-line comment > * style. Like in this example. > */
Sure. A drag network style insist to be different, but thats another sory...
> > ... > >> +static int microchip_sgpio_get_ports(struct sgpio_priv *priv) >> +{ >> + struct device *dev = priv->dev; >> + struct device_node *np = dev->of_node; >> + int i, ret; >> + u32 range_params[64]; > > Better to use reversed xmas tree order. >
Ack.
>> + /* Calculate port mask */ >> + ret = of_property_read_variable_u32_array(np, >> + "microchip,sgpio-port-ranges", >> + range_params, >> + 2, >> + ARRAY_SIZE(range_params)); >> + if (ret < 0 || ret % 2) { >> + dev_err(dev, "%s port range\n", >> + ret == -EINVAL ? "Missing" : "Invalid"); > >
?? Did you have a comment?
> >> + return ret; >> + } >> + for (i = 0; i < ret; i += 2) { >> + int start, end; >> + >> + start = range_params[i]; >> + end = range_params[i + 1]; >> + if (start > end || end >= SGPIO_BITS_PER_WORD) { >> + dev_err(dev, "Ill-formed port-range [%d:%d]\n", >> + start, end); >> + } >> + priv->ports |= GENMASK(end, start); >> + } >> + >> + return 0; >> +} > > Doesn't GPIO / pin control framework have this helper already? > If no, have you considered to use proper bitmap API here? (For > example, bitmap_parselist() or so) >
Past reviews suggested using an array form. And as the binding is already reviewed, I would like to keep this as is.
> ... > >> + if (fwnode_property_read_u32(fwnode, "ngpios", &ngpios)) { >> + dev_info(dev, "failed to get number of gpios for bank%d\n", >> + bankno); >> + ngpios = 64; >> + } > > Don't mix OF APIs with fwnode APIs. >
OF is gone.
> ... > >> + pins = devm_kzalloc(dev, sizeof(*pins)*ngpios, GFP_KERNEL); >> + if (pins) { > > Use usual pattern and drop one level of indentation ('else' is redundant).
Yes, done.
> >> + int i; >> + char *p, *names; > >> + names = devm_kzalloc(dev, PIN_NAM_SZ*ngpios, GFP_KERNEL); >> + >> + if (!names) > > Redundant blank line. >
Gone.
>> + return -ENOMEM; > >> + for (p = names, i = 0; i < ngpios; i++, p += PIN_NAM_SZ) { >> + struct sgpio_port_addr addr; >> + >> + sgpio_pin_to_addr(priv, i, &addr); > >> + snprintf(p, PIN_NAM_SZ, "SGPIO_%c_p%db%d", >> + is_input ? 'I' : 'O', >> + addr.port, addr.bit); > > Wow, snprintf() with constant size argument in a loop. Are you sure > you are doing correct?
I changed this to per-string allocation.
> >> + pins[i].number = i; >> + pins[i].name = p; >> + } >> + } else >> + return -ENOMEM; > > ... > >> + pctldev = devm_pinctrl_register(dev, pctl_desc, bank); >> + if (IS_ERR(pctldev)) { >> + dev_err(dev, "Failed to register pinctrl\n"); >> + return PTR_ERR(pctldev); >> + } > > return dev_err_probe(...); >
Yes.
> ... > >> + /* Get clock */ > > Useless comment.
Ok, removed.
> >> + clk = devm_clk_get(dev, NULL); >> + if (IS_ERR(clk)) { > >> + dev_err(dev, "Failed to get clock\n"); >> + return PTR_ERR(clk); > > dev_err_probe() as above. >
Yes.
>> + } > > ... > >> + /* Get register map */ > > Useless comment. >
Removed.
> ... > >> + nbanks = device_get_child_node_count(dev); >> + if (nbanks != 2) { >> + dev_err(dev, "Must have 2 banks (have %d)\n", nbanks); >> + return -EINVAL; >> + } > > Don't mix device_property API with OF one. >
Removed OF.
>> + i = 0; >> + device_for_each_child_node(dev, fwnode) { > > Ditto. >
Don't sure I understand this comment, but device_for_each_child_node() is from <linux/property.h> - this should be OK I think.
>> + ret = microchip_sgpio_register_bank(dev, priv, fwnode, i++); >> + if (ret) >> + return ret; >> + }
Thank you for your comments. They are much appreciated.
I will refresh the patch later today.
Cheers,
---Lars
-- Lars Povlsen, Microchip
| |