| Message ID | 20201109132643.457932-3-lars.povlsen@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Adding support for Microchip/Microsemi serial GPIO controller | expand |
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. ... > +#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. ... > + unsigned int bit = 3 * addr->bit; > + unsigned int bit = 3 * addr->bit; Magic number. Perhaps a defined constant? ... > + return -EOPNOTSUPP; Are you sure? IIRC internally we are using ENOTSUPP. Couple of drivers seem to be wrongly using the other one. ... > + err = -EOPNOTSUPP; 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? > + return -EINVAL; > + } ... > + if ((priv->ports & BIT(addr.port)) == 0) { '!' is equivalent to ' == 0', but it's up to you. > + 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. ... > + pin = gpiospec->args[1] + (gpiospec->args[0] * priv->bitcount); Redundant parentheses. ... > +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. ... > + 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. ... > + 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(). > + if (!pins[i].name) > + return -ENOMEM; > + } ... > + /* Get clock */ Useless comment. ... > + 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? > + dev_err(dev, "Invalid frequency %d\n", priv->clock); > + return -EINVAL; > + } ... > + /* Get register map */ Useless. ... > + /* Get rest of device properties */ This one though may be left.
On 09/11/2020 16:17:40+0200, Andy Shevchenko wrote: > > + 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? > Why would userspace get the error code? Userspace should never have to handle gpios directly or you are doing something wrong.
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: > > > + 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? > > 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.
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: > > > > + 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? > > > > 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. I don't see how your answer is relevant here. Did you already check all the call sites from the kernel too?
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.
On 09/11/2020 18:15:30+0200, Andy Shevchenko wrote: > > > > 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. Exactly, that is what I'm telling to them. > 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. > This is the valid reason and as you can see, it has nothing to do with userspace.
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
Andy Shevchenko writes: > 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. I think the message is a valid one. I will change it to dev_err_ratelimited() - that should prevent the dmesg flooding. ---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.
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
On Wed, Nov 11, 2020 at 10:52 AM Lars Povlsen <lars.povlsen@microchip.com> wrote: > 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: ... > >> >> + 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. There are no other opinions. Read description of struct pinconf_ops and fix the code. checkpatch is simply wrong here. > >> >> + err = -EOPNOTSUPP; > >> > > >> > Ditto. > >> > >> Ditto. > > > > Ditto.
Andy Shevchenko writes: > On Wed, Nov 11, 2020 at 10:52 AM Lars Povlsen > <lars.povlsen@microchip.com> wrote: >> 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: > > ... > >> >> >> + 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. > > There are no other opinions. > Read description of struct pinconf_ops and fix the code. > checkpatch is simply wrong here. Lets no start a war :-) - I'll change it... > >> >> >> + err = -EOPNOTSUPP; >> >> > >> >> > Ditto. >> >> >> >> Ditto. >> > >> > Ditto. Cheers, ---Lars
diff --git a/MAINTAINERS b/MAINTAINERS index e73636b75f29..75a00dfa824a 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2117,6 +2117,7 @@ L: linux-arm-kernel@lists.infradead.org (moderated for non-subscribers) S: Supported T: git git://github.com/microchip-ung/linux-upstream.git F: arch/arm64/boot/dts/microchip/ +F: drivers/pinctrl/pinctrl-microchip-sgpio.c N: sparx5 ARM/MIOA701 MACHINE SUPPORT diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig index 815095326e2d..9b8ed7516355 100644 --- a/drivers/pinctrl/Kconfig +++ b/drivers/pinctrl/Kconfig @@ -374,6 +374,22 @@ config PINCTRL_OCELOT select OF_GPIO select REGMAP_MMIO +config PINCTRL_MICROCHIP_SGPIO + bool "Pinctrl driver for Microsemi/Microchip Serial GPIO" + depends on HAS_IOMEM + select GPIOLIB + select GENERIC_PINCONF + select GENERIC_PINCTRL_GROUPS + select GENERIC_PINMUX_FUNCTIONS + 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. + source "drivers/pinctrl/actions/Kconfig" source "drivers/pinctrl/aspeed/Kconfig" source "drivers/pinctrl/bcm/Kconfig" diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile index f53933b2ff02..c9fcfafc45c7 100644 --- a/drivers/pinctrl/Makefile +++ b/drivers/pinctrl/Makefile @@ -46,6 +46,7 @@ obj-$(CONFIG_PINCTRL_ZYNQ) += pinctrl-zynq.o obj-$(CONFIG_PINCTRL_INGENIC) += pinctrl-ingenic.o obj-$(CONFIG_PINCTRL_RK805) += pinctrl-rk805.o obj-$(CONFIG_PINCTRL_OCELOT) += pinctrl-ocelot.o +obj-$(CONFIG_PINCTRL_MICROCHIP_SGPIO) += pinctrl-microchip-sgpio.o obj-$(CONFIG_PINCTRL_EQUILIBRIUM) += pinctrl-equilibrium.o obj-y += actions/ diff --git a/drivers/pinctrl/pinctrl-microchip-sgpio.c b/drivers/pinctrl/pinctrl-microchip-sgpio.c new file mode 100644 index 000000000000..0fc7543d5bd4 --- /dev/null +++ b/drivers/pinctrl/pinctrl-microchip-sgpio.c @@ -0,0 +1,640 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Microsemi/Microchip SoCs serial gpio driver + * + * Author: <lars.povlsen@microchip.com> + * + * Copyright (c) 2020 Microchip Technology Inc. and its subsidiaries. + */ + +#include <linux/bits.h> +#include <linux/clk.h> +#include <linux/gpio/driver.h> +#include <linux/io.h> +#include <linux/mod_devicetable.h> +#include <linux/module.h> +#include <linux/pinctrl/pinmux.h> +#include <linux/platform_device.h> +#include <linux/property.h> + +#include "core.h" +#include "pinconf.h" + +#define SGPIO_BITS_PER_WORD 32 +#define SGPIO_MAX_BITS 4 + +enum { + REG_INPUT_DATA, + REG_PORT_CONFIG, + REG_PORT_ENABLE, + REG_SIO_CONFIG, + REG_SIO_CLOCK, + MAXREG +}; + +struct sgpio_properties { + u8 regoff[MAXREG]; + u32 auto_repeat; + u32 port_width; + u32 clk_freq; + u32 bit_source; +}; + +#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))) + +#define SGPIO_M_CFG_SIO_AUTO_REPEAT(p) ((p)->properties->auto_repeat) +#define SGPIO_F_CFG_SIO_PORT_WIDTH(p, x) __BF_PREP((p)->properties->port_width, x) +#define SGPIO_M_CFG_SIO_PORT_WIDTH(p) ((p)->properties->port_width) +#define SGPIO_F_CLOCK_SIO_CLK_FREQ(p, x) __BF_PREP((p)->properties->clk_freq, x) +#define SGPIO_M_CLOCK_SIO_CLK_FREQ(p) ((p)->properties->clk_freq) +#define SGPIO_F_PORT_CFG_BIT_SOURCE(p, x) __BF_PREP((p)->properties->bit_source, x) +#define SGPIO_X_PORT_CFG_BIT_SOURCE(p, x) __BF_GET((p)->properties->bit_source, x) + +const struct sgpio_properties properties_luton = { + .regoff = { 0x00, 0x09, 0x29, 0x2a, 0x2b }, + .auto_repeat = BIT(5), + .port_width = GENMASK(3, 2), + .clk_freq = GENMASK(11, 0), + .bit_source = GENMASK(11, 0), +}; + +const struct sgpio_properties properties_ocelot = { + .regoff = { 0x00, 0x06, 0x26, 0x04, 0x05 }, + .auto_repeat = BIT(10), + .port_width = GENMASK(8, 7), + .clk_freq = GENMASK(19, 8), + .bit_source = GENMASK(23, 12), +}; + +const struct sgpio_properties properties_sparx5 = { + .regoff = { 0x00, 0x06, 0x26, 0x04, 0x05 }, + .auto_repeat = BIT(6), + .port_width = GENMASK(4, 3), + .clk_freq = GENMASK(19, 8), + .bit_source = GENMASK(23, 12), +}; + +static const char * const functions[] = { "gpio" }; + +struct sgpio_bank { + struct sgpio_priv *priv; + bool is_input; + struct gpio_chip gpio; + struct pinctrl_desc pctl_desc; +}; + +struct sgpio_priv { + struct device *dev; + struct sgpio_bank in; + struct sgpio_bank out; + u32 bitcount; + u32 ports; + u32 clock; + u32 __iomem *regs; + const struct sgpio_properties *properties; +}; + +struct sgpio_port_addr { + u8 port; + u8 bit; +}; + +static inline void sgpio_pin_to_addr(struct sgpio_priv *priv, int pin, + struct sgpio_port_addr *addr) +{ + addr->port = pin / priv->bitcount; + addr->bit = pin % priv->bitcount; +} + +static inline u32 sgpio_readl(struct sgpio_priv *priv, u32 rno, u32 off) +{ + u32 __iomem *reg = &priv->regs[priv->properties->regoff[rno] + off]; + + return readl(reg); +} + +static inline void sgpio_writel(struct sgpio_priv *priv, + u32 val, u32 rno, u32 off) +{ + u32 __iomem *reg = &priv->regs[priv->properties->regoff[rno] + off]; + + writel(val, reg); +} + +static inline void sgpio_clrsetbits(struct sgpio_priv *priv, + u32 rno, u32 off, u32 clear, u32 set) +{ + u32 __iomem *reg = &priv->regs[priv->properties->regoff[rno] + off]; + u32 val = readl(reg); + + val &= ~clear; + val |= set; + + writel(val, reg); +} + +static void sgpio_output_set(struct sgpio_priv *priv, + struct sgpio_port_addr *addr, + int value) +{ + unsigned int bit = 3 * addr->bit; + + sgpio_clrsetbits(priv, REG_PORT_CONFIG, addr->port, + SGPIO_F_PORT_CFG_BIT_SOURCE(priv, BIT(bit)), + SGPIO_F_PORT_CFG_BIT_SOURCE(priv, value << bit)); +} + +static int sgpio_output_get(struct sgpio_priv *priv, + struct sgpio_port_addr *addr) +{ + u32 portval = sgpio_readl(priv, REG_PORT_CONFIG, addr->port); + unsigned int bit = 3 * addr->bit; + + return !!(SGPIO_X_PORT_CFG_BIT_SOURCE(priv, portval) & BIT(bit)); +} + +static int sgpio_input_get(struct sgpio_priv *priv, + struct sgpio_port_addr *addr) +{ + return !!(sgpio_readl(priv, REG_INPUT_DATA, addr->bit) & BIT(addr->port)); +} + +static int sgpio_pinconf_get(struct pinctrl_dev *pctldev, + unsigned int pin, unsigned long *config) +{ + struct sgpio_bank *bank = pinctrl_dev_get_drvdata(pctldev); + u32 param = pinconf_to_config_param(*config); + struct sgpio_priv *priv = bank->priv; + struct sgpio_port_addr addr; + int val; + + sgpio_pin_to_addr(priv, pin, &addr); + + switch (param) { + case PIN_CONFIG_INPUT_ENABLE: + val = bank->is_input; + break; + + case PIN_CONFIG_OUTPUT_ENABLE: + val = !bank->is_input; + break; + + case PIN_CONFIG_OUTPUT: + if (bank->is_input) + return -EINVAL; + val = sgpio_output_get(priv, &addr); + break; + + default: + return -EOPNOTSUPP; + } + + *config = pinconf_to_config_packed(param, val); + + return 0; +} + +static int sgpio_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin, + unsigned long *configs, unsigned int num_configs) +{ + struct sgpio_bank *bank = pinctrl_dev_get_drvdata(pctldev); + struct sgpio_priv *priv = bank->priv; + struct sgpio_port_addr addr; + int cfg, err = 0; + u32 param, arg; + + sgpio_pin_to_addr(priv, pin, &addr); + + for (cfg = 0; cfg < num_configs; cfg++) { + param = pinconf_to_config_param(configs[cfg]); + arg = pinconf_to_config_argument(configs[cfg]); + + switch (param) { + case PIN_CONFIG_OUTPUT: + if (bank->is_input) + return -EINVAL; + sgpio_output_set(priv, &addr, arg); + break; + + default: + err = -EOPNOTSUPP; + } + } + + return err; +} + +static const struct pinconf_ops sgpio_confops = { + .is_generic = true, + .pin_config_get = sgpio_pinconf_get, + .pin_config_set = sgpio_pinconf_set, + .pin_config_config_dbg_show = pinconf_generic_dump_config, +}; + +static int sgpio_get_functions_count(struct pinctrl_dev *pctldev) +{ + return 1; +} + +static const char *sgpio_get_function_name(struct pinctrl_dev *pctldev, + unsigned int function) +{ + return functions[0]; +} + +static int sgpio_get_function_groups(struct pinctrl_dev *pctldev, + unsigned int function, + const char *const **groups, + unsigned *const num_groups) +{ + *groups = functions; + *num_groups = ARRAY_SIZE(functions); + + return 0; +} + +static int sgpio_pinmux_set_mux(struct pinctrl_dev *pctldev, + unsigned int selector, unsigned int group) +{ + return 0; +} + +static int sgpio_gpio_set_direction(struct pinctrl_dev *pctldev, + struct pinctrl_gpio_range *range, + unsigned int pin, bool input) +{ + struct sgpio_bank *bank = pinctrl_dev_get_drvdata(pctldev); + + if (input != bank->is_input) { + dev_err(pctldev->dev, "Pin %d direction as %s is not possible\n", + pin, input ? "input" : "output"); + return -EINVAL; + } + + return 0; +} + +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, "Request port %d for pin %d is not activated\n", + addr.port, offset); + } + + return 0; +} + +static const struct pinmux_ops sgpio_pmx_ops = { + .get_functions_count = sgpio_get_functions_count, + .get_function_name = sgpio_get_function_name, + .get_function_groups = sgpio_get_function_groups, + .set_mux = sgpio_pinmux_set_mux, + .gpio_set_direction = sgpio_gpio_set_direction, + .gpio_request_enable = sgpio_gpio_request_enable, +}; + +static int sgpio_pctl_get_groups_count(struct pinctrl_dev *pctldev) +{ + struct sgpio_bank *bank = pinctrl_dev_get_drvdata(pctldev); + + return bank->pctl_desc.npins; +} + +static const char *sgpio_pctl_get_group_name(struct pinctrl_dev *pctldev, + unsigned int group) +{ + struct sgpio_bank *bank = pinctrl_dev_get_drvdata(pctldev); + + return bank->pctl_desc.pins[group].name; +} + +static int sgpio_pctl_get_group_pins(struct pinctrl_dev *pctldev, + unsigned int group, + const unsigned int **pins, + unsigned int *num_pins) +{ + struct sgpio_bank *bank = pinctrl_dev_get_drvdata(pctldev); + + *pins = &bank->pctl_desc.pins[group].number; + *num_pins = 1; + + return 0; +} + +static const struct pinctrl_ops sgpio_pctl_ops = { + .get_groups_count = sgpio_pctl_get_groups_count, + .get_group_name = sgpio_pctl_get_group_name, + .get_group_pins = sgpio_pctl_get_group_pins, + .dt_node_to_map = pinconf_generic_dt_node_to_map_pin, + .dt_free_map = pinconf_generic_dt_free_map, +}; + +static int microchip_sgpio_direction_input(struct gpio_chip *gc, unsigned int gpio) +{ + struct sgpio_bank *bank = gpiochip_get_data(gc); + + /* Fixed-position function */ + return bank->is_input ? 0 : -EINVAL; +} + +static int microchip_sgpio_direction_output(struct gpio_chip *gc, + unsigned int gpio, int value) +{ + struct sgpio_bank *bank = gpiochip_get_data(gc); + struct sgpio_priv *priv = bank->priv; + struct sgpio_port_addr addr; + + /* Fixed-position function */ + if (bank->is_input) + return -EINVAL; + + sgpio_pin_to_addr(priv, gpio, &addr); + + sgpio_output_set(priv, &addr, value); + + return 0; +} + +static int microchip_sgpio_get_direction(struct gpio_chip *gc, unsigned int gpio) +{ + struct sgpio_bank *bank = gpiochip_get_data(gc); + + return bank->is_input ? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT; +} + +static void microchip_sgpio_set_value(struct gpio_chip *gc, + unsigned int gpio, int value) +{ + microchip_sgpio_direction_output(gc, gpio, value); +} + +static int microchip_sgpio_get_value(struct gpio_chip *gc, unsigned int gpio) +{ + struct sgpio_bank *bank = gpiochip_get_data(gc); + struct sgpio_priv *priv = bank->priv; + struct sgpio_port_addr addr; + + sgpio_pin_to_addr(priv, gpio, &addr); + + return bank->is_input ? + sgpio_input_get(priv, &addr) : sgpio_output_get(priv, &addr); +} + +static int microchip_sgpio_of_xlate(struct gpio_chip *gc, + const struct of_phandle_args *gpiospec, + u32 *flags) +{ + struct sgpio_bank *bank = gpiochip_get_data(gc); + struct sgpio_priv *priv = bank->priv; + int pin; + + /* + * Note that the SGIO pin is defined by *2* numbers, a port + * number between 0 and 31, and a bit index, 0 to 3. + */ + if (gpiospec->args[0] > SGPIO_BITS_PER_WORD || + gpiospec->args[1] > priv->bitcount) + return -EINVAL; + + pin = gpiospec->args[1] + (gpiospec->args[0] * priv->bitcount); + + if (pin > gc->ngpio) + return -EINVAL; + + if (flags) + *flags = gpiospec->args[2]; + + return pin; +} + +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; +} + +static int microchip_sgpio_register_bank(struct device *dev, + struct sgpio_priv *priv, + struct fwnode_handle *fwnode, + int bankno) +{ + bool is_input = (bankno == 0); + struct pinctrl_pin_desc *pins; + struct pinctrl_desc *pctl_desc; + struct pinctrl_dev *pctldev; + struct sgpio_bank *bank; + struct gpio_chip *gc; + u32 ngpios; + int i, ret; + + /* Get overall bank struct */ + bank = is_input ? &priv->in : &priv->out; + bank->is_input = is_input; + bank->priv = priv; + + if (fwnode_property_read_u32(fwnode, "ngpios", &ngpios)) { + dev_info(dev, "failed to get number of gpios for bank%d\n", + bankno); + ngpios = 64; + } + + priv->bitcount = ngpios / SGPIO_BITS_PER_WORD; + if (priv->bitcount > SGPIO_MAX_BITS) { + dev_err(dev, "Bit width exceeds maximum (%d)\n", + SGPIO_MAX_BITS); + return -EINVAL; + } + + pctl_desc = &bank->pctl_desc; + pctl_desc->name = devm_kasprintf(dev, GFP_KERNEL, "%s-%sput", + dev_name(dev), + is_input ? "in" : "out"); + pctl_desc->pctlops = &sgpio_pctl_ops; + pctl_desc->pmxops = &sgpio_pmx_ops; + pctl_desc->confops = &sgpio_confops; + pctl_desc->owner = THIS_MODULE; + + pins = devm_kzalloc(dev, sizeof(*pins)*ngpios, GFP_KERNEL); + if (!pins) + return -ENOMEM; + + pctl_desc->npins = ngpios; + pctl_desc->pins = pins; + + 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); + if (!pins[i].name) + return -ENOMEM; + } + + pctldev = devm_pinctrl_register(dev, pctl_desc, bank); + if (IS_ERR(pctldev)) + return dev_err_probe(dev, PTR_ERR(pctldev), "Failed to register pinctrl\n"); + + gc = &bank->gpio; + gc->label = pctl_desc->name; + gc->parent = dev; + gc->of_node = to_of_node(fwnode); + gc->owner = THIS_MODULE; + gc->get_direction = microchip_sgpio_get_direction; + gc->direction_input = microchip_sgpio_direction_input; + gc->direction_output = microchip_sgpio_direction_output; + gc->get = microchip_sgpio_get_value; + gc->set = microchip_sgpio_set_value; + gc->request = gpiochip_generic_request; + gc->free = gpiochip_generic_free; + gc->of_xlate = microchip_sgpio_of_xlate; + gc->of_gpio_n_cells = 3; + gc->base = -1; + gc->ngpio = ngpios; + + ret = devm_gpiochip_add_data(dev, gc, bank); + if (ret) + dev_err(dev, "Failed to register: ret %d\n", ret); + + return ret; +} + +static int microchip_sgpio_probe(struct platform_device *pdev) +{ + int div_clock = 0, ret, port, i, nbanks; + struct device *dev = &pdev->dev; + struct fwnode_handle *fwnode; + struct sgpio_priv *priv; + struct clk *clk; + u32 val; + + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + priv->dev = dev; + + /* Get clock */ + clk = devm_clk_get(dev, NULL); + if (IS_ERR(clk)) + return dev_err_probe(dev, PTR_ERR(clk), "Failed to get clock\n"); + + div_clock = clk_get_rate(clk); + if (of_property_read_u32(dev->of_node, "bus-frequency", &priv->clock)) + priv->clock = 12500000; + if (priv->clock <= 0 || priv->clock > (div_clock / 2)) { + dev_err(dev, "Invalid frequency %d\n", priv->clock); + return -EINVAL; + } + + /* Get register map */ + priv->regs = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(priv->regs)) + return PTR_ERR(priv->regs); + priv->properties = device_get_match_data(dev); + + /* Get rest of device properties */ + ret = microchip_sgpio_get_ports(priv); + if (ret) + return ret; + + nbanks = device_get_child_node_count(dev); + if (nbanks != 2) { + dev_err(dev, "Must have 2 banks (have %d)\n", nbanks); + return -EINVAL; + } + + i = 0; + device_for_each_child_node(dev, fwnode) { + ret = microchip_sgpio_register_bank(dev, priv, fwnode, i++); + if (ret) + return ret; + } + + if (priv->in.gpio.ngpio != priv->out.gpio.ngpio) { + dev_err(dev, "Banks must have same GPIO count\n"); + return -EINVAL; + } + + sgpio_clrsetbits(priv, REG_SIO_CONFIG, 0, + SGPIO_M_CFG_SIO_PORT_WIDTH(priv), + SGPIO_F_CFG_SIO_PORT_WIDTH(priv, priv->bitcount - 1) | + SGPIO_M_CFG_SIO_AUTO_REPEAT(priv)); + val = max(2U, div_clock / priv->clock); + sgpio_clrsetbits(priv, REG_SIO_CLOCK, 0, + SGPIO_M_CLOCK_SIO_CLK_FREQ(priv), + SGPIO_F_CLOCK_SIO_CLK_FREQ(priv, val)); + + for (port = 0; port < SGPIO_BITS_PER_WORD; port++) + sgpio_writel(priv, 0, REG_PORT_CONFIG, port); + sgpio_writel(priv, priv->ports, REG_PORT_ENABLE, 0); + + return 0; +} + +static const struct of_device_id microchip_sgpio_gpio_of_match[] = { + { + .compatible = "microchip,sparx5-sgpio", + .data = &properties_sparx5, + }, { + .compatible = "mscc,luton-sgpio", + .data = &properties_luton, + }, { + .compatible = "mscc,ocelot-sgpio", + .data = &properties_ocelot, + }, { + /* sentinel */ + } +}; + +static struct platform_driver microchip_sgpio_pinctrl_driver = { + .driver = { + .name = "pinctrl-microchip-sgpio", + .of_match_table = microchip_sgpio_gpio_of_match, + .suppress_bind_attrs = true, + }, + .probe = microchip_sgpio_probe, +}; +builtin_platform_driver(microchip_sgpio_pinctrl_driver);
This adds a pinctrl driver for the Microsemi/Microchip Serial GPIO (SGPIO) device used in various SoC's. Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com> --- MAINTAINERS | 1 + drivers/pinctrl/Kconfig | 16 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-microchip-sgpio.c | 640 ++++++++++++++++++++++ 4 files changed, 658 insertions(+) create mode 100644 drivers/pinctrl/pinctrl-microchip-sgpio.c