| Message ID | 20201109132643.457932-1-lars.povlsen@microchip.com |
|---|---|
| Headers | show |
| Series | Adding support for Microchip/Microsemi serial GPIO controller | expand |
On Mon, Nov 9, 2020 at 5:27 PM Alexandre Belloni <alexandre.belloni@bootlin.com> wrote: > On 09/11/2020 17:16:49+0200, Andy Shevchenko wrote: > > On Mon, Nov 9, 2020 at 4:32 PM Alexandre Belloni > > <alexandre.belloni@bootlin.com> wrote: > > > On 09/11/2020 16:17:40+0200, Andy Shevchenko wrote: ... > > > > > + dev_err(pctldev->dev, "Pin %d direction as %s is not possible\n", > > > > > + pin, input ? "input" : "output"); > > > > > > > > Do we need this noise? Isn't user space getting a proper error code as > > > > per doc and can handle this? > > > > > > Why would userspace get the error code? > > > > Huh?! Why it shouldn't. How will users know if they are doing something wrong? > > > > > Userspace should never have to > > > handle gpios directly or you are doing something wrong. > > > > This is true, but check how error codes are propagated to the user space. > > > > your point is to remove an error message because the error may be > propagated to userspace. My point is that userspace should never use > gpios and the kernel has to be the consumer. Tell this to plenty of users of old sysfs interface and to libgpiod ones. If what you are saying had been true, we would have never had the new ABI for GPIOs. > I don't see how your answer > is relevant here. I have an opposite opinion. > Did you already check all the call sites from the > kernel too? If you think we have to print a message on each possible error case (but not always the one) we will get lost in the messages disaster and dmesg overflow. It is consumer who should decide if the setting is critical or not to be printed to user.
Andy Shevchenko writes: > On Mon, Nov 9, 2020 at 3:27 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. > > Please, elaborate what you said previously, because now it has no > justification to be a pin control driver. > As previously stated, the individual pins have possible other functions than GPIO. When these functions are added, the driver will need pinctrl functinality. This was accepted by Linux Walleij. > ... > >> +#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))) > > I answered the old thread that it probably makes sense to make > something like field_get() / field_prep() available for everybody. I assume its not in there for a reason. Anyway, I have changed the code to use <linux/bitfield.h> directly, so this is now gone. > ... > >> + unsigned int bit = 3 * addr->bit; > >> + unsigned int bit = 3 * addr->bit; > > Magic number. Perhaps a defined constant? > I can add a constant. > ... > >> + return -EOPNOTSUPP; > > Are you sure? IIRC internally we are using ENOTSUPP. > > Couple of drivers seem to be wrongly using the other one. > Checkpatch complains about ENOTSUPP: # ENOTSUPP is not a standard error code and should be avoided in new patches. # Folks usually mean EOPNOTSUPP (also called ENOTSUP), when they type ENOTSUPP. > ... > >> + err = -EOPNOTSUPP; > > Ditto. > Ditto. > ... > >> + if (input != bank->is_input) { > >> + dev_err(pctldev->dev, "Pin %d direction as %s is not possible\n", >> + pin, input ? "input" : "output"); > > Do we need this noise? Isn't user space getting a proper error code as > per doc and can handle this? > This need not go to user space, as one use-case is using the pin as a i2c mux. In this case no signs of the error condition is recorded, it just doesn't work. So I concur it is not noise, it is sign of an erroneous situation which should be fixed, IMHO. The message makes it easy to locate the issue, if any. The message will not occur on a properly configured system. Lets have the maintainer make the call. >> + return -EINVAL; >> + } > > ... > >> + if ((priv->ports & BIT(addr.port)) == 0) { > > '!' is equivalent to ' == 0', but it's up to you. > Thank you, I prefer the current form. >> + dev_warn(priv->dev, "Request port %d for pin %d is not activated\n", >> + addr.port, offset); >> + } > > ... > >> + return bank->is_input ? >> + sgpio_input_get(priv, &addr) : sgpio_output_get(priv, &addr); > > I would leave it on one line. > I can make that one line. > ... > >> + pin = gpiospec->args[1] + (gpiospec->args[0] * priv->bitcount); > > Redundant parentheses. > Yes, they are. > ... > >> +static int microchip_sgpio_get_ports(struct sgpio_priv *priv) >> +{ >> + struct device *dev = priv->dev; >> + struct device_node *np = dev->of_node; >> + u32 range_params[64]; >> + int i, ret; >> + >> + /* 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"); >> + 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; >> +} > > As per previous version comment, i.e. perhaps find an existing API for > this kind of parser or introduce a generic one. > I fixed the use of OF api's - that was surely an oversight. I have searched for a suitable API without finding one. The closest thing was the parsing of "gpio-reserved-ranges" in gpiolib-of.c, but that was coded directly. So I think this might not be of general use. If it is, lets do that after the driver is merged. > ... > >> + bool is_input = (bankno == 0); > > Now I'm not sure why you need this variable here and in the structure. > Can't you use the above check directly? > If you want to have flexible settings (i.e. not all generations of hw > have it like this) I would rather recommend to do it via DT. > I have eliminated the local variable - its only in the bank structure. The pins are (by earlier reviews) separated in distinct input and output banks. (All variants have both). > ... > >> + for (i = 0; i < ngpios; i++) { >> + char name[sizeof("SGPIO_D_pXXbY\0")]; >> + struct sgpio_port_addr addr; >> + >> + sgpio_pin_to_addr(priv, i, &addr); >> + snprintf(name, sizeof(name), "SGPIO_%c_p%db%d", >> + is_input ? 'I' : 'O', >> + addr.port, addr.bit); >> + >> + pins[i].number = i; >> + pins[i].name = devm_kstrdup(dev, name, GFP_KERNEL); > > Above with this is a NIH of devm_kasprintf(). Good catch, I'll apply that. > >> + if (!pins[i].name) >> + return -ENOMEM; >> + } > > ... > >> + /* Get clock */ > > Useless comment. Sorry, forgot to remove that. Gone! > > ... > >> + div_clock = clk_get_rate(clk); >> + if (of_property_read_u32(dev->of_node, "bus-frequency", &priv->clock)) > > Again, choose one API, no need to spread among several. > > As I have told you already: if any comment is given against one > location in the code, check the entire contribution for similar places > and address accordingly. > >> + priv->clock = 12500000; >> + if (priv->clock <= 0 || priv->clock > (div_clock / 2)) { > > How can an unsigned value be possible less than 0? I'll change that to "==", thanks. > >> + dev_err(dev, "Invalid frequency %d\n", priv->clock); >> + return -EINVAL; > >> + } > > ... > >> + /* Get register map */ > > Useless. > OK, removed > ... > >> + /* Get rest of device properties */ > > This one though may be left. Thank you for your continued interest in improving the driver. I hope you'll find the next version palatable. Cheers, ---Lars -- Lars Povlsen, Microchip
On Tue, Nov 10, 2020 at 5:51 PM Lars Povlsen <lars.povlsen@microchip.com> wrote: > > On Mon, Nov 9, 2020 at 3:27 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. > > > > Please, elaborate what you said previously, because now it has no > > justification to be a pin control driver. > > As previously stated, the individual pins have possible other functions > than GPIO. When these functions are added, the driver will need pinctrl > functinality. This was accepted by Linux Walleij. Yes, I understand that. What I meant is to update the commit message to tell this to the reviewers / readers / anthropologists. ... > >> + return -EOPNOTSUPP; > > > > Are you sure? IIRC internally we are using ENOTSUPP. > > > > Couple of drivers seem to be wrongly using the other one. > > Checkpatch complains about ENOTSUPP: > > # ENOTSUPP is not a standard error code and should be avoided in new patches. > # Folks usually mean EOPNOTSUPP (also called ENOTSUP), when they type ENOTSUPP. checkpatch is wrong if this is internal code and to me sounds like it's not going out of the kernel. ... > >> + err = -EOPNOTSUPP; > > > > Ditto. > > Ditto. Ditto. ... > >> + dev_err(pctldev->dev, "Pin %d direction as %s is not possible\n", > >> + pin, input ? "input" : "output"); > > > > Do we need this noise? Isn't user space getting a proper error code as > > per doc and can handle this? > > > > This need not go to user space, as one use-case is using the pin as a > i2c mux. In this case no signs of the error condition is recorded, it > just doesn't work. So I concur it is not noise, it is sign of an > erroneous situation which should be fixed, IMHO. > > The message makes it easy to locate the issue, if any. The message will > not occur on a properly configured system. It's noise. As we discussed with Alexandre (and I guess came to the same page) that its consumer's business how to treat the error. > Lets have the maintainer make the call. ... > >> +static int microchip_sgpio_get_ports(struct sgpio_priv *priv) > >> +{ > >> +} > > > > As per previous version comment, i.e. perhaps find an existing API for > > this kind of parser or introduce a generic one. > > I fixed the use of OF api's - that was surely an oversight. > > I have searched for a suitable API without finding one. The closest > thing was the parsing of "gpio-reserved-ranges" in gpiolib-of.c, but > that was coded directly. So I think this might not be of general use. > > If it is, lets do that after the driver is merged. I guess it will be a lot of benefit to have such API earlier than later. -- With Best Regards, Andy Shevchenko
Andy Shevchenko writes: > On Tue, Nov 10, 2020 at 5:51 PM Lars Povlsen <lars.povlsen@microchip.com> wrote: >> > On Mon, Nov 9, 2020 at 3:27 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. >> > >> > Please, elaborate what you said previously, because now it has no >> > justification to be a pin control driver. >> >> As previously stated, the individual pins have possible other functions >> than GPIO. When these functions are added, the driver will need pinctrl >> functinality. This was accepted by Linux Walleij. > > Yes, I understand that. What I meant is to update the commit message > to tell this to the reviewers / readers / anthropologists. Ok, will do. > > ... > >> >> + return -EOPNOTSUPP; >> > >> > Are you sure? IIRC internally we are using ENOTSUPP. >> > >> > Couple of drivers seem to be wrongly using the other one. >> >> Checkpatch complains about ENOTSUPP: >> >> # ENOTSUPP is not a standard error code and should be avoided in new patches. >> # Folks usually mean EOPNOTSUPP (also called ENOTSUP), when they type ENOTSUPP. > > checkpatch is wrong if this is internal code and to me sounds like > it's not going out of the kernel. > > ... As it appears there are different opinions on this I'll let the pinctrl maintainer decide. > >> >> + err = -EOPNOTSUPP; >> > >> > Ditto. >> >> Ditto. > > Ditto. > > ... > >> >> + dev_err(pctldev->dev, "Pin %d direction as %s is not possible\n", >> >> + pin, input ? "input" : "output"); >> > >> > Do we need this noise? Isn't user space getting a proper error code as >> > per doc and can handle this? >> > >> >> This need not go to user space, as one use-case is using the pin as a >> i2c mux. In this case no signs of the error condition is recorded, it >> just doesn't work. So I concur it is not noise, it is sign of an >> erroneous situation which should be fixed, IMHO. >> >> The message makes it easy to locate the issue, if any. The message will >> not occur on a properly configured system. > > It's noise. As we discussed with Alexandre (and I guess came to the > same page) that its consumer's business how to treat the error. > >> Lets have the maintainer make the call. > > ... I digress. I'll remove it. > >> >> +static int microchip_sgpio_get_ports(struct sgpio_priv *priv) >> >> +{ > >> >> +} >> > >> > As per previous version comment, i.e. perhaps find an existing API for >> > this kind of parser or introduce a generic one. >> >> I fixed the use of OF api's - that was surely an oversight. >> >> I have searched for a suitable API without finding one. The closest >> thing was the parsing of "gpio-reserved-ranges" in gpiolib-of.c, but >> that was coded directly. So I think this might not be of general use. >> >> If it is, lets do that after the driver is merged. > > I guess it will be a lot of benefit to have such API earlier than later. Thank you for your comments. I'll send the new version shortly. ---Lars -- Lars Povlsen, Microchip
The series add support for the serial GPIO controller used by Microchip Sparx5, as well as (MSCC) ocelot/jaguar2 SoCs. v8 changes (from Andy): - Removed OF dependency/usage entirely. - Trimmed+sorted include files. - Made local variables reverse xmas sorted. - Removed __func__ usage. - Changed some occurences of "if (x) { ..." to early return. - Use dev_err_probe() where possible. - Replace of_device_get_match_data() with device_get_match_data() - Some minor formatting corrections - Do per-pin string allocation as opposed to bulk allocation+chop. v7 changes: - Fixed wrong sizeof in pin string name template. (Andy) - Collapsed sgpio_input_get() to one liner. (Andy) - Eliminated unneeded variable in microchip_sgpio_get_value() - Removed noisy dev_info(). (Andy) - Replaced platform_get_resource()+devm_ioremap_resource() with devm_platform_ioremap_resource(). (Andy) - Replaced device_property_read_u32() with of_property_read_u32(). (Andy) - Replaced __builtin_ffsll() with __builtin_ffs() for MIPS32 targets. v6 changes: - Use "bus-frequency" instead of "microchip,sgpio-frequency". Drop '$ref'. (Robh) - Added "ngpios" description, bumped minimum to 32. (Linus) - Added "#size-cells" description. (Linus) - Changed "bus-frequency" validation in driver to reflect the YAML description. v5 changes (driver comments from Linus): - Collect bank data in sgpio_bank struct - Add is_input boolean to sgpio_bank struct - Use single-bit bitmasks in sgpio_output_set() and sgpio_output_get() - Eliminate superfluous struct pinctrl_dev *pctl_dev in bank data - Fix wrong ngpio consistency check v4 changes (binding comments from Rob): - microchip,sgpio-port-ranges changed to uint32-matrix so tuples can be represented properly. - gpio controller node name changed to "gpio@[0-1]" - whitespace fixes - DT files updated as per schema changes v3 changes: - Renamed all usage of "mchp" abbrevation with "microchip". - Split the in/output directions into (two) separate banks. - Eliminated the bindings include file (from above) - Changed SPDX license to "GPL-2.0-or-later" - Change -ENOTSUPP to -EOPNOTSUPP - Minor type/symbol naming changes v2 changes: - Adds both in and output modes. - Use direct adressing of the individual banks (#gpio-cells = <4>), also osoleting need for addressing macros in bindings include file. - Property 'microchip,sgpio-ports' (uint32, bitmask) replaced by proper range set (array of [start,end]) 'microchip,sgpio-port-ranges'. - Fixes whitespace issues in Kconfig file Lars Povlsen (3): dt-bindings: pinctrl: Add bindings for pinctrl-microchip-sgpio driver pinctrl: pinctrl-microchip-sgpio: Add pinctrl driver for Microsemi Serial GPIO arm64: dts: sparx5: Add SGPIO devices .../pinctrl/microchip,sparx5-sgpio.yaml | 145 ++++ MAINTAINERS | 1 + arch/arm64/boot/dts/microchip/sparx5.dtsi | 91 +++ .../boot/dts/microchip/sparx5_pcb125.dts | 5 + .../dts/microchip/sparx5_pcb134_board.dtsi | 258 +++++++ .../dts/microchip/sparx5_pcb135_board.dtsi | 55 ++ drivers/pinctrl/Kconfig | 16 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-microchip-sgpio.c | 640 ++++++++++++++++++ 9 files changed, 1212 insertions(+) create mode 100644 Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml create mode 100644 drivers/pinctrl/pinctrl-microchip-sgpio.c -- 2.25.1