| Message ID | 20230721161840.1393996-1-hugo@hugovil.com |
|---|---|
| Headers | show |
| Series | serial: sc16is7xx: fix GPIO regression and rs485 improvements | expand |
On Fri, Jul 21, 2023 at 10:19 AM Hugo Villeneuve <hugo@hugovil.com> wrote: > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > Commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines") > and commit 21144bab4f11 ("sc16is7xx: Handle modem status lines") > changed the function of the GPIOs pins to act as modem control > lines without any possibility of selecting GPIO function. Requiring a new DT property is not fixing a kernel regression. You should be returning the kernel to original behavior and then have a new DT property for new behavior. > As a consequence, applications that depends on GPIO lines configured > by default as GPIO pins no longer work as expected. > > Also, the change to select modem control lines function was done only > for channel A of dual UART variants (752/762). This was not documented > in the log message. > > Allow to specify GPIO or modem control line function in the device > tree, and for each of the ports (A or B). > > Do so by using the new device-tree property named > "modem-control-line-ports" (property added in separate patch). That's not the name in the patch. > When registering GPIO chip controller, mask-out GPIO pins declared as > modem control lines according to this new "modem-control-line-ports" > DT property. > > Boards that need to have GPIOS configured as modem control lines > should add that property to their device tree. Here is a list of > boards using the sc16is7xx driver in their device tree and that may > need to be modified: > arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts > mips/boot/dts/ingenic/cu1830-neo.dts > mips/boot/dts/ingenic/cu1000-neo.dts Then again, if no one cares about those boards needing a change then it can be okay. Rob
On Fri, 21 Jul 2023 13:24:19 -0600 Rob Herring <robh+dt@kernel.org> wrote: > On Fri, Jul 21, 2023 at 10:19 AM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > > > Commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines") > > and commit 21144bab4f11 ("sc16is7xx: Handle modem status lines") > > changed the function of the GPIOs pins to act as modem control > > lines without any possibility of selecting GPIO function. > > Requiring a new DT property is not fixing a kernel regression. You > should be returning the kernel to original behavior and then have a > new DT property for new behavior. Hi Rob, please read the entire patch history starting from V1 and you will understand why this course of action was not selected. > > As a consequence, applications that depends on GPIO lines configured > > by default as GPIO pins no longer work as expected. > > > > Also, the change to select modem control lines function was done only > > for channel A of dual UART variants (752/762). This was not documented > > in the log message. > > > > Allow to specify GPIO or modem control line function in the device > > tree, and for each of the ports (A or B). > > > > Do so by using the new device-tree property named > > "modem-control-line-ports" (property added in separate patch). > > That's not the name in the patch. We added a "nxp," prefix at some point. Do you want me to send a V9 only with this change? Hugo. > > When registering GPIO chip controller, mask-out GPIO pins declared as > > modem control lines according to this new "modem-control-line-ports" > > DT property. > > > > Boards that need to have GPIOS configured as modem control lines > > should add that property to their device tree. Here is a list of > > boards using the sc16is7xx driver in their device tree and that may > > need to be modified: > > arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts > > mips/boot/dts/ingenic/cu1830-neo.dts > > mips/boot/dts/ingenic/cu1000-neo.dts > > Then again, if no one cares about those boards needing a change then > it can be okay. > > > Rob >
On Sat, Jul 22, 2023 at 10:47:24AM -0400, Hugo Villeneuve wrote: > On Fri, 21 Jul 2023 13:24:19 -0600 > Rob Herring <robh+dt@kernel.org> wrote: > > > On Fri, Jul 21, 2023 at 10:19 AM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > > > > > Commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines") > > > and commit 21144bab4f11 ("sc16is7xx: Handle modem status lines") > > > changed the function of the GPIOs pins to act as modem control > > > lines without any possibility of selecting GPIO function. > > > > Requiring a new DT property is not fixing a kernel regression. You > > should be returning the kernel to original behavior and then have a > > new DT property for new behavior. > > Hi Rob, > please read the entire patch history starting from V1 > and you will understand why this course of action was > not selected. That's not going to happen, sorry, you need to explain it here, in this patch series, why a specific action is being taken over another one, as no one has time to go dig through past history, sorry. thanks, greg k-h
On Sat, 22 Jul 2023 17:15:26 +0200 Greg KH <gregkh@linuxfoundation.org> wrote: > On Sat, Jul 22, 2023 at 10:47:24AM -0400, Hugo Villeneuve wrote: > > On Fri, 21 Jul 2023 13:24:19 -0600 > > Rob Herring <robh+dt@kernel.org> wrote: > > > > > On Fri, Jul 21, 2023 at 10:19 AM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > > > > > > > Commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines") > > > > and commit 21144bab4f11 ("sc16is7xx: Handle modem status lines") > > > > changed the function of the GPIOs pins to act as modem control > > > > lines without any possibility of selecting GPIO function. > > > > > > Requiring a new DT property is not fixing a kernel regression. You > > > should be returning the kernel to original behavior and then have a > > > new DT property for new behavior. > > > > Hi Rob, > > please read the entire patch history starting from V1 > > and you will understand why this course of action was > > not selected. > > That's not going to happen, sorry, you need to explain it here, in this > patch series, why a specific action is being taken over another one, as > no one has time to go dig through past history, sorry. Hi Rob, I initially submitted a patch to revert the kernel to original behavior, but it created more problems because the patch was unfortunately split in two separate patches, and mixed with other non closely-related changes. It was also noted to me that reverting to the old behavior would break things for some users. It was suggested to me by a more experienced kernel developer to "suggest a fix, instead of hurrying a revert": https://lkml.org/lkml/2023/5/17/758 That is what we decided to do in the end, and it worked quite well. Hugo.
On Mon, 31 Jul 2023 09:31:53 -0600 Rob Herring <robh+dt@kernel.org> wrote: > On Mon, Jul 24, 2023 at 9:54 AM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > On Sat, 22 Jul 2023 17:15:26 +0200 > > Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > On Sat, Jul 22, 2023 at 10:47:24AM -0400, Hugo Villeneuve wrote: > > > > On Fri, 21 Jul 2023 13:24:19 -0600 > > > > Rob Herring <robh+dt@kernel.org> wrote: > > > > > > > > > On Fri, Jul 21, 2023 at 10:19 AM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > > > > > > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > > > > > > > > > > > Commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines") > > > > > > and commit 21144bab4f11 ("sc16is7xx: Handle modem status lines") > > > > > > changed the function of the GPIOs pins to act as modem control > > > > > > lines without any possibility of selecting GPIO function. > > > > > > > > > > Requiring a new DT property is not fixing a kernel regression. You > > > > > should be returning the kernel to original behavior and then have a > > > > > new DT property for new behavior. > > > > > > > > Hi Rob, > > > > please read the entire patch history starting from V1 > > > > and you will understand why this course of action was > > > > not selected. > > > > > > That's not going to happen, sorry, you need to explain it here, in this > > > patch series, why a specific action is being taken over another one, as > > > no one has time to go dig through past history, sorry. > > > > Hi Rob, > > I initially submitted a patch to revert the kernel to original > > behavior, but it created more problems because the patch was > > unfortunately split in two separate patches, and mixed with other non > > closely-related changes. It was also noted to me that reverting to the > > old behavior would break things for some users. > > > > It was suggested to me by a more experienced kernel developer to > > "suggest a fix, instead of hurrying a revert": > > > > https://lkml.org/lkml/2023/5/17/758 > > Do I have to go read this to decipher the justification and reasoning? > When Greg says "in this patch series", he means in the commit messages > of the patches. You send v9 already and it doesn't have that. The > patchset needs to stand on its own summarizing any relevant prior > discussions. > > I never suggested doing a revert. Hi Rob, I am sorry, but this is exactly what I "deciphered" from your original email. I am trying very hard to understand exactly what you mean, but it is not that obvious for me. If something is not clear in my commit message, I will try to improve it. But before, let's try to focus on making sure I understand more clearly what you want exactly. > Obviously, someone still wants the > new feature. I assume that you refer to the "new feature" as what was added in the commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")? Because I did not add a "new feature" myself, I simply restored (or want to restore) what was working before commit 679875d1d880 (restore GPIO pins as GPIO functions). I will wait for your clarification on this, and answer your other comments after. Hugo. > The issue is a new feature was added to the kernel, but > you are requiring a DT change to platforms NOT using the feature. > Make > the platforms wanting the new feature to need a DT change. That's > still not great, but it's much better to affect new platforms rather > than old, stable platforms. The period of time that regresses is much > smaller (a few kernel releases vs. years potentially). Of course, if > it's just those 3 platforms and their maintainers are fine with > needing this DT change, then that works too. But there's no evidence > here that they are okay with it. You didn't even do the update of the > dts files and just left them broken.
On Mon, Jul 31, 2023 at 10:46 AM Hugo Villeneuve <hugo@hugovil.com> wrote: > > On Mon, 31 Jul 2023 09:31:53 -0600 > Rob Herring <robh+dt@kernel.org> wrote: > > > On Mon, Jul 24, 2023 at 9:54 AM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > On Sat, 22 Jul 2023 17:15:26 +0200 > > > Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > > > On Sat, Jul 22, 2023 at 10:47:24AM -0400, Hugo Villeneuve wrote: > > > > > On Fri, 21 Jul 2023 13:24:19 -0600 > > > > > Rob Herring <robh+dt@kernel.org> wrote: > > > > > > > > > > > On Fri, Jul 21, 2023 at 10:19 AM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > > > > > > > > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > > > > > > > > > > > > > Commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines") > > > > > > > and commit 21144bab4f11 ("sc16is7xx: Handle modem status lines") > > > > > > > changed the function of the GPIOs pins to act as modem control > > > > > > > lines without any possibility of selecting GPIO function. > > > > > > > > > > > > Requiring a new DT property is not fixing a kernel regression. You > > > > > > should be returning the kernel to original behavior and then have a > > > > > > new DT property for new behavior. > > > > > > > > > > Hi Rob, > > > > > please read the entire patch history starting from V1 > > > > > and you will understand why this course of action was > > > > > not selected. > > > > > > > > That's not going to happen, sorry, you need to explain it here, in this > > > > patch series, why a specific action is being taken over another one, as > > > > no one has time to go dig through past history, sorry. > > > > > > Hi Rob, > > > I initially submitted a patch to revert the kernel to original > > > behavior, but it created more problems because the patch was > > > unfortunately split in two separate patches, and mixed with other non > > > closely-related changes. It was also noted to me that reverting to the > > > old behavior would break things for some users. > > > > > > It was suggested to me by a more experienced kernel developer to > > > "suggest a fix, instead of hurrying a revert": > > > > > > https://lkml.org/lkml/2023/5/17/758 > > > > Do I have to go read this to decipher the justification and reasoning? > > When Greg says "in this patch series", he means in the commit messages > > of the patches. You send v9 already and it doesn't have that. The > > patchset needs to stand on its own summarizing any relevant prior > > discussions. > > > > I never suggested doing a revert. > > Hi Rob, > I am sorry, but this is exactly what I "deciphered" from your > original email. > > I am trying very hard to understand exactly what you mean, but it is > not that obvious for me. If something is not clear in my commit message, > I will try to improve it. But before, let's try to focus on making sure > I understand more clearly what you want exactly. > > > Obviously, someone still wants the > > new feature. > > I assume that you refer to the "new feature" as what was added in > the commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control > lines")? Shrug. It's one of the 2 commits mentioned, I don't know which one exactly. Whichever one changed default behavior from use GPIOs to use modem ctrl lines. Reading it again, I *think* this patch is correct. Default behavior is restored to use GPIOs. The DT property is needed to enable modem ctrl lines. What's not okay is just saying, these platforms may or may not need an update: arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts mips/boot/dts/ingenic/cu1830-neo.dts mips/boot/dts/ingenic/cu1000-neo.dts You need to figure that out. Have you checked with maintainers of these boards? When were they added and by who? At the same time or by the same person would be a good indication the platform uses modem ctrl lines. Or were these platforms in use before adding modem ctrl support? Then they probably use GPIOs or nothing. If there are platforms which would regress if the modem ctrl feature was just reverted, which ones are those? Rob
On Mon, 31 Jul 2023 12:04:45 -0600 Rob Herring <robh+dt@kernel.org> wrote: > On Mon, Jul 31, 2023 at 10:46 AM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > On Mon, 31 Jul 2023 09:31:53 -0600 > > Rob Herring <robh+dt@kernel.org> wrote: > > > > > On Mon, Jul 24, 2023 at 9:54 AM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > > > On Sat, 22 Jul 2023 17:15:26 +0200 > > > > Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > > > > > On Sat, Jul 22, 2023 at 10:47:24AM -0400, Hugo Villeneuve wrote: > > > > > > On Fri, 21 Jul 2023 13:24:19 -0600 > > > > > > Rob Herring <robh+dt@kernel.org> wrote: > > > > > > > > > > > > > On Fri, Jul 21, 2023 at 10:19 AM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > > > > > > > > > > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > > > > > > > > > > > > > > > Commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines") > > > > > > > > and commit 21144bab4f11 ("sc16is7xx: Handle modem status lines") > > > > > > > > changed the function of the GPIOs pins to act as modem control > > > > > > > > lines without any possibility of selecting GPIO function. > > > > > > > > > > > > > > Requiring a new DT property is not fixing a kernel regression. You > > > > > > > should be returning the kernel to original behavior and then have a > > > > > > > new DT property for new behavior. > > > > > > > > > > > > Hi Rob, > > > > > > please read the entire patch history starting from V1 > > > > > > and you will understand why this course of action was > > > > > > not selected. > > > > > > > > > > That's not going to happen, sorry, you need to explain it here, in this > > > > > patch series, why a specific action is being taken over another one, as > > > > > no one has time to go dig through past history, sorry. > > > > > > > > Hi Rob, > > > > I initially submitted a patch to revert the kernel to original > > > > behavior, but it created more problems because the patch was > > > > unfortunately split in two separate patches, and mixed with other non > > > > closely-related changes. It was also noted to me that reverting to the > > > > old behavior would break things for some users. > > > > > > > > It was suggested to me by a more experienced kernel developer to > > > > "suggest a fix, instead of hurrying a revert": > > > > > > > > https://lkml.org/lkml/2023/5/17/758 > > > > > > Do I have to go read this to decipher the justification and reasoning? > > > When Greg says "in this patch series", he means in the commit messages > > > of the patches. You send v9 already and it doesn't have that. The > > > patchset needs to stand on its own summarizing any relevant prior > > > discussions. > > > > > > I never suggested doing a revert. > > > > Hi Rob, > > I am sorry, but this is exactly what I "deciphered" from your > > original email. > > > > I am trying very hard to understand exactly what you mean, but it is > > not that obvious for me. If something is not clear in my commit message, > > I will try to improve it. But before, let's try to focus on making sure > > I understand more clearly what you want exactly. > > > > > Obviously, someone still wants the > > > new feature. > > > > I assume that you refer to the "new feature" as what was added in > > the commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control > > lines")? > > Shrug. It's one of the 2 commits mentioned, I don't know which one > exactly. Whichever one changed default behavior from use GPIOs to use > modem ctrl lines. > > Reading it again, I *think* this patch is correct. Default behavior is > restored to use GPIOs. The DT property is needed to enable modem ctrl > lines. Hi, this is correct. > What's not okay is just saying, these platforms may or may not need an update: > > arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts > mips/boot/dts/ingenic/cu1830-neo.dts > mips/boot/dts/ingenic/cu1000-neo.dts Yes, my bad. I initially mentioned them and hoped to get some feedback, which I never got, and I kind of forgot about it. > You need to figure that out. Have you checked with maintainers of > these boards? When were they added and by who? At the same time or by > the same person would be a good indication the platform uses modem > ctrl lines. Or were these platforms in use before adding modem ctrl > support? Then they probably use GPIOs or nothing. > > If there are platforms which would regress if the modem ctrl feature > was just reverted, which ones are those? Ok, let me do some checks and get back to you on this. Thank you, Hugo.
On Mon, 31 Jul 2023 14:41:15 -0400 Hugo Villeneuve <hugo@hugovil.com> wrote: > On Mon, 31 Jul 2023 12:04:45 -0600 > Rob Herring <robh+dt@kernel.org> wrote: > > > On Mon, Jul 31, 2023 at 10:46 AM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > On Mon, 31 Jul 2023 09:31:53 -0600 > > > Rob Herring <robh+dt@kernel.org> wrote: > > > > > > > On Mon, Jul 24, 2023 at 9:54 AM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > > > > > On Sat, 22 Jul 2023 17:15:26 +0200 > > > > > Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > > > > > > > On Sat, Jul 22, 2023 at 10:47:24AM -0400, Hugo Villeneuve wrote: > > > > > > > On Fri, 21 Jul 2023 13:24:19 -0600 > > > > > > > Rob Herring <robh+dt@kernel.org> wrote: > > > > > > > > > > > > > > > On Fri, Jul 21, 2023 at 10:19 AM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > > > > > > > > > > > > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > > > > > > > > > > > > > > > > > Commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines") > > > > > > > > > and commit 21144bab4f11 ("sc16is7xx: Handle modem status lines") > > > > > > > > > changed the function of the GPIOs pins to act as modem control > > > > > > > > > lines without any possibility of selecting GPIO function. > > > > > > > > > > > > > > > > Requiring a new DT property is not fixing a kernel regression. You > > > > > > > > should be returning the kernel to original behavior and then have a > > > > > > > > new DT property for new behavior. > > > > > > > > > > > > > > Hi Rob, > > > > > > > please read the entire patch history starting from V1 > > > > > > > and you will understand why this course of action was > > > > > > > not selected. > > > > > > > > > > > > That's not going to happen, sorry, you need to explain it here, in this > > > > > > patch series, why a specific action is being taken over another one, as > > > > > > no one has time to go dig through past history, sorry. > > > > > > > > > > Hi Rob, > > > > > I initially submitted a patch to revert the kernel to original > > > > > behavior, but it created more problems because the patch was > > > > > unfortunately split in two separate patches, and mixed with other non > > > > > closely-related changes. It was also noted to me that reverting to the > > > > > old behavior would break things for some users. > > > > > > > > > > It was suggested to me by a more experienced kernel developer to > > > > > "suggest a fix, instead of hurrying a revert": > > > > > > > > > > https://lkml.org/lkml/2023/5/17/758 > > > > > > > > Do I have to go read this to decipher the justification and reasoning? > > > > When Greg says "in this patch series", he means in the commit messages > > > > of the patches. You send v9 already and it doesn't have that. The > > > > patchset needs to stand on its own summarizing any relevant prior > > > > discussions. > > > > > > > > I never suggested doing a revert. > > > > > > Hi Rob, > > > I am sorry, but this is exactly what I "deciphered" from your > > > original email. > > > > > > I am trying very hard to understand exactly what you mean, but it is > > > not that obvious for me. If something is not clear in my commit message, > > > I will try to improve it. But before, let's try to focus on making sure > > > I understand more clearly what you want exactly. > > > > > > > Obviously, someone still wants the > > > > new feature. > > > > > > I assume that you refer to the "new feature" as what was added in > > > the commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control > > > lines")? > > > > Shrug. It's one of the 2 commits mentioned, I don't know which one > > exactly. Whichever one changed default behavior from use GPIOs to use > > modem ctrl lines. > > > > Reading it again, I *think* this patch is correct. Default behavior is > > restored to use GPIOs. The DT property is needed to enable modem ctrl > > lines. > > Hi, > this is correct. > > > > What's not okay is just saying, these platforms may or may not need an update: > > > > arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts > > mips/boot/dts/ingenic/cu1830-neo.dts > > mips/boot/dts/ingenic/cu1000-neo.dts > > Yes, my bad. I initially mentioned them and hoped to get some > feedback, which I never got, and I kind of forgot about it. > > > You need to figure that out. Have you checked with maintainers of > > these boards? When were they added and by who? At the same time or by > > the same person would be a good indication the platform uses modem > > ctrl lines. Or were these platforms in use before adding modem ctrl > > support? Then they probably use GPIOs or nothing. > > > > If there are platforms which would regress if the modem ctrl feature > > was just reverted, which ones are those? > > Ok, let me do some checks and get back to you on this. Hi Rob, for this board: arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts it uses a SC16IS740, which doesn't have any GPIOs nor modem control lines, so no DT changes required. For these two Ingenic boards: mips/boot/dts/ingenic/cu1830-neo.dts mips/boot/dts/ingenic/cu1000-neo.dts They use a SC16IS752, which has shared modem control lines and GPIOs. Unfortunately, the maintainers have not (yet) responded to my inquiries. Also, I tried to search for schematics or block diagrams on the net but couldn't find anything. These platforms were in use before the patch to add the modem control lines was added. Then like you said they probably use these shared lines as GPIOs or nothing, so no DT changes would be required. Hugo.
On Thu, Aug 3, 2023 at 11:54 AM Hugo Villeneuve <hugo@hugovil.com> wrote: > > On Mon, 31 Jul 2023 14:41:15 -0400 > Hugo Villeneuve <hugo@hugovil.com> wrote: > > > On Mon, 31 Jul 2023 12:04:45 -0600 > > Rob Herring <robh+dt@kernel.org> wrote: > > > > > On Mon, Jul 31, 2023 at 10:46 AM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > > > On Mon, 31 Jul 2023 09:31:53 -0600 > > > > Rob Herring <robh+dt@kernel.org> wrote: > > > > > > > > > On Mon, Jul 24, 2023 at 9:54 AM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > > > > > > > On Sat, 22 Jul 2023 17:15:26 +0200 > > > > > > Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > > > > > > > > > On Sat, Jul 22, 2023 at 10:47:24AM -0400, Hugo Villeneuve wrote: > > > > > > > > On Fri, 21 Jul 2023 13:24:19 -0600 > > > > > > > > Rob Herring <robh+dt@kernel.org> wrote: > > > > > > > > > > > > > > > > > On Fri, Jul 21, 2023 at 10:19 AM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > > > > > > > > > > > > > > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > > > > > > > > > > > > > > > > > > > Commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines") > > > > > > > > > > and commit 21144bab4f11 ("sc16is7xx: Handle modem status lines") > > > > > > > > > > changed the function of the GPIOs pins to act as modem control > > > > > > > > > > lines without any possibility of selecting GPIO function. > > > > > > > > > > > > > > > > > > Requiring a new DT property is not fixing a kernel regression. You > > > > > > > > > should be returning the kernel to original behavior and then have a > > > > > > > > > new DT property for new behavior. > > > > > > > > > > > > > > > > Hi Rob, > > > > > > > > please read the entire patch history starting from V1 > > > > > > > > and you will understand why this course of action was > > > > > > > > not selected. > > > > > > > > > > > > > > That's not going to happen, sorry, you need to explain it here, in this > > > > > > > patch series, why a specific action is being taken over another one, as > > > > > > > no one has time to go dig through past history, sorry. > > > > > > > > > > > > Hi Rob, > > > > > > I initially submitted a patch to revert the kernel to original > > > > > > behavior, but it created more problems because the patch was > > > > > > unfortunately split in two separate patches, and mixed with other non > > > > > > closely-related changes. It was also noted to me that reverting to the > > > > > > old behavior would break things for some users. > > > > > > > > > > > > It was suggested to me by a more experienced kernel developer to > > > > > > "suggest a fix, instead of hurrying a revert": > > > > > > > > > > > > https://lkml.org/lkml/2023/5/17/758 > > > > > > > > > > Do I have to go read this to decipher the justification and reasoning? > > > > > When Greg says "in this patch series", he means in the commit messages > > > > > of the patches. You send v9 already and it doesn't have that. The > > > > > patchset needs to stand on its own summarizing any relevant prior > > > > > discussions. > > > > > > > > > > I never suggested doing a revert. > > > > > > > > Hi Rob, > > > > I am sorry, but this is exactly what I "deciphered" from your > > > > original email. > > > > > > > > I am trying very hard to understand exactly what you mean, but it is > > > > not that obvious for me. If something is not clear in my commit message, > > > > I will try to improve it. But before, let's try to focus on making sure > > > > I understand more clearly what you want exactly. > > > > > > > > > Obviously, someone still wants the > > > > > new feature. > > > > > > > > I assume that you refer to the "new feature" as what was added in > > > > the commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control > > > > lines")? > > > > > > Shrug. It's one of the 2 commits mentioned, I don't know which one > > > exactly. Whichever one changed default behavior from use GPIOs to use > > > modem ctrl lines. > > > > > > Reading it again, I *think* this patch is correct. Default behavior is > > > restored to use GPIOs. The DT property is needed to enable modem ctrl > > > lines. > > > > Hi, > > this is correct. > > > > > > > What's not okay is just saying, these platforms may or may not need an update: > > > > > > arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts > > > mips/boot/dts/ingenic/cu1830-neo.dts > > > mips/boot/dts/ingenic/cu1000-neo.dts > > > > Yes, my bad. I initially mentioned them and hoped to get some > > feedback, which I never got, and I kind of forgot about it. > > > > > You need to figure that out. Have you checked with maintainers of > > > these boards? When were they added and by who? At the same time or by > > > the same person would be a good indication the platform uses modem > > > ctrl lines. Or were these platforms in use before adding modem ctrl > > > support? Then they probably use GPIOs or nothing. > > > > > > If there are platforms which would regress if the modem ctrl feature > > > was just reverted, which ones are those? > > > > Ok, let me do some checks and get back to you on this. > > Hi Rob, > for this board: > arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts > > it uses a SC16IS740, which doesn't have any GPIOs nor modem > control lines, so no DT changes required. > > For these two Ingenic boards: > mips/boot/dts/ingenic/cu1830-neo.dts > mips/boot/dts/ingenic/cu1000-neo.dts > > They use a SC16IS752, which has shared modem control lines and GPIOs. > Unfortunately, the maintainers have not (yet) responded to my > inquiries. Also, I tried to search for schematics or block diagrams on > the net but couldn't find anything. > > These platforms were in use before the patch to add the modem control > lines was added. Then like you said they probably use these shared > lines as GPIOs or nothing, so no DT changes would be required. Okay, that's useful (please add to the commit msg). Still, what platform(s) need the modem control feature? Presumably that's whatever platform Lech and Tomasz work on. I guess given the Reviewed-by they are fine with needing a DT change. Rob
On Thu, 3 Aug 2023 14:10:30 -0600 Rob Herring <robh+dt@kernel.org> wrote: > On Thu, Aug 3, 2023 at 11:54 AM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > On Mon, 31 Jul 2023 14:41:15 -0400 > > Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > On Mon, 31 Jul 2023 12:04:45 -0600 > > > Rob Herring <robh+dt@kernel.org> wrote: > > > > > > > On Mon, Jul 31, 2023 at 10:46 AM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > > > > > On Mon, 31 Jul 2023 09:31:53 -0600 > > > > > Rob Herring <robh+dt@kernel.org> wrote: > > > > > > > > > > > On Mon, Jul 24, 2023 at 9:54 AM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > > > > > > > > > On Sat, 22 Jul 2023 17:15:26 +0200 > > > > > > > Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > > > > > > > > > > > On Sat, Jul 22, 2023 at 10:47:24AM -0400, Hugo Villeneuve wrote: > > > > > > > > > On Fri, 21 Jul 2023 13:24:19 -0600 > > > > > > > > > Rob Herring <robh+dt@kernel.org> wrote: > > > > > > > > > > > > > > > > > > > On Fri, Jul 21, 2023 at 10:19 AM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > > > > > > > > > > > > > > > > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > > > > > > > > > > > > > > > > > > > > > Commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines") > > > > > > > > > > > and commit 21144bab4f11 ("sc16is7xx: Handle modem status lines") > > > > > > > > > > > changed the function of the GPIOs pins to act as modem control > > > > > > > > > > > lines without any possibility of selecting GPIO function. > > > > > > > > > > > > > > > > > > > > Requiring a new DT property is not fixing a kernel regression. You > > > > > > > > > > should be returning the kernel to original behavior and then have a > > > > > > > > > > new DT property for new behavior. > > > > > > > > > > > > > > > > > > Hi Rob, > > > > > > > > > please read the entire patch history starting from V1 > > > > > > > > > and you will understand why this course of action was > > > > > > > > > not selected. > > > > > > > > > > > > > > > > That's not going to happen, sorry, you need to explain it here, in this > > > > > > > > patch series, why a specific action is being taken over another one, as > > > > > > > > no one has time to go dig through past history, sorry. > > > > > > > > > > > > > > Hi Rob, > > > > > > > I initially submitted a patch to revert the kernel to original > > > > > > > behavior, but it created more problems because the patch was > > > > > > > unfortunately split in two separate patches, and mixed with other non > > > > > > > closely-related changes. It was also noted to me that reverting to the > > > > > > > old behavior would break things for some users. > > > > > > > > > > > > > > It was suggested to me by a more experienced kernel developer to > > > > > > > "suggest a fix, instead of hurrying a revert": > > > > > > > > > > > > > > https://lkml.org/lkml/2023/5/17/758 > > > > > > > > > > > > Do I have to go read this to decipher the justification and reasoning? > > > > > > When Greg says "in this patch series", he means in the commit messages > > > > > > of the patches. You send v9 already and it doesn't have that. The > > > > > > patchset needs to stand on its own summarizing any relevant prior > > > > > > discussions. > > > > > > > > > > > > I never suggested doing a revert. > > > > > > > > > > Hi Rob, > > > > > I am sorry, but this is exactly what I "deciphered" from your > > > > > original email. > > > > > > > > > > I am trying very hard to understand exactly what you mean, but it is > > > > > not that obvious for me. If something is not clear in my commit message, > > > > > I will try to improve it. But before, let's try to focus on making sure > > > > > I understand more clearly what you want exactly. > > > > > > > > > > > Obviously, someone still wants the > > > > > > new feature. > > > > > > > > > > I assume that you refer to the "new feature" as what was added in > > > > > the commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control > > > > > lines")? > > > > > > > > Shrug. It's one of the 2 commits mentioned, I don't know which one > > > > exactly. Whichever one changed default behavior from use GPIOs to use > > > > modem ctrl lines. > > > > > > > > Reading it again, I *think* this patch is correct. Default behavior is > > > > restored to use GPIOs. The DT property is needed to enable modem ctrl > > > > lines. > > > > > > Hi, > > > this is correct. > > > > > > > > > > What's not okay is just saying, these platforms may or may not need an update: > > > > > > > > arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts > > > > mips/boot/dts/ingenic/cu1830-neo.dts > > > > mips/boot/dts/ingenic/cu1000-neo.dts > > > > > > Yes, my bad. I initially mentioned them and hoped to get some > > > feedback, which I never got, and I kind of forgot about it. > > > > > > > You need to figure that out. Have you checked with maintainers of > > > > these boards? When were they added and by who? At the same time or by > > > > the same person would be a good indication the platform uses modem > > > > ctrl lines. Or were these platforms in use before adding modem ctrl > > > > support? Then they probably use GPIOs or nothing. > > > > > > > > If there are platforms which would regress if the modem ctrl feature > > > > was just reverted, which ones are those? > > > > > > Ok, let me do some checks and get back to you on this. > > > > Hi Rob, > > for this board: > > arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts > > > > it uses a SC16IS740, which doesn't have any GPIOs nor modem > > control lines, so no DT changes required. > > > > For these two Ingenic boards: > > mips/boot/dts/ingenic/cu1830-neo.dts > > mips/boot/dts/ingenic/cu1000-neo.dts > > > > They use a SC16IS752, which has shared modem control lines and GPIOs. > > Unfortunately, the maintainers have not (yet) responded to my > > inquiries. Also, I tried to search for schematics or block diagrams on > > the net but couldn't find anything. > > > > These platforms were in use before the patch to add the modem control > > lines was added. Then like you said they probably use these shared > > lines as GPIOs or nothing, so no DT changes would be required. > > Okay, that's useful (please add to the commit msg). I added the information in the cover letter, but I can add it to the actual patch commit message if you prefer. > Still, what platform(s) need the modem control feature? Presumably > that's whatever platform Lech and Tomasz work on. I guess given the > Reviewed-by they are fine with needing a DT change. Ok. I have previously also emailed Lech about that, but he has not responded yet. Thank you, Hugo.
On Thu, 3 Aug 2023 17:38:54 -0400 Hugo Villeneuve <hugo@hugovil.com> wrote: > On Thu, 3 Aug 2023 14:10:30 -0600 > Rob Herring <robh+dt@kernel.org> wrote: > > > On Thu, Aug 3, 2023 at 11:54 AM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > On Mon, 31 Jul 2023 14:41:15 -0400 > > > Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > > On Mon, 31 Jul 2023 12:04:45 -0600 > > > > Rob Herring <robh+dt@kernel.org> wrote: > > > > > > > > > On Mon, Jul 31, 2023 at 10:46 AM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > > > > > > > On Mon, 31 Jul 2023 09:31:53 -0600 > > > > > > Rob Herring <robh+dt@kernel.org> wrote: > > > > > > > > > > > > > On Mon, Jul 24, 2023 at 9:54 AM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > > > > > > > > > > > On Sat, 22 Jul 2023 17:15:26 +0200 > > > > > > > > Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > > > > > > > > > > > > > On Sat, Jul 22, 2023 at 10:47:24AM -0400, Hugo Villeneuve wrote: > > > > > > > > > > On Fri, 21 Jul 2023 13:24:19 -0600 > > > > > > > > > > Rob Herring <robh+dt@kernel.org> wrote: > > > > > > > > > > > > > > > > > > > > > On Fri, Jul 21, 2023 at 10:19 AM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > > > > > > > > > > > > > > > > > > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > > > > > > > > > > > > > > > > > > > > > > > Commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines") > > > > > > > > > > > > and commit 21144bab4f11 ("sc16is7xx: Handle modem status lines") > > > > > > > > > > > > changed the function of the GPIOs pins to act as modem control > > > > > > > > > > > > lines without any possibility of selecting GPIO function. > > > > > > > > > > > > > > > > > > > > > > Requiring a new DT property is not fixing a kernel regression. You > > > > > > > > > > > should be returning the kernel to original behavior and then have a > > > > > > > > > > > new DT property for new behavior. > > > > > > > > > > > > > > > > > > > > Hi Rob, > > > > > > > > > > please read the entire patch history starting from V1 > > > > > > > > > > and you will understand why this course of action was > > > > > > > > > > not selected. > > > > > > > > > > > > > > > > > > That's not going to happen, sorry, you need to explain it here, in this > > > > > > > > > patch series, why a specific action is being taken over another one, as > > > > > > > > > no one has time to go dig through past history, sorry. > > > > > > > > > > > > > > > > Hi Rob, > > > > > > > > I initially submitted a patch to revert the kernel to original > > > > > > > > behavior, but it created more problems because the patch was > > > > > > > > unfortunately split in two separate patches, and mixed with other non > > > > > > > > closely-related changes. It was also noted to me that reverting to the > > > > > > > > old behavior would break things for some users. > > > > > > > > > > > > > > > > It was suggested to me by a more experienced kernel developer to > > > > > > > > "suggest a fix, instead of hurrying a revert": > > > > > > > > > > > > > > > > https://lkml.org/lkml/2023/5/17/758 > > > > > > > > > > > > > > Do I have to go read this to decipher the justification and reasoning? > > > > > > > When Greg says "in this patch series", he means in the commit messages > > > > > > > of the patches. You send v9 already and it doesn't have that. The > > > > > > > patchset needs to stand on its own summarizing any relevant prior > > > > > > > discussions. > > > > > > > > > > > > > > I never suggested doing a revert. > > > > > > > > > > > > Hi Rob, > > > > > > I am sorry, but this is exactly what I "deciphered" from your > > > > > > original email. > > > > > > > > > > > > I am trying very hard to understand exactly what you mean, but it is > > > > > > not that obvious for me. If something is not clear in my commit message, > > > > > > I will try to improve it. But before, let's try to focus on making sure > > > > > > I understand more clearly what you want exactly. > > > > > > > > > > > > > Obviously, someone still wants the > > > > > > > new feature. > > > > > > > > > > > > I assume that you refer to the "new feature" as what was added in > > > > > > the commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control > > > > > > lines")? > > > > > > > > > > Shrug. It's one of the 2 commits mentioned, I don't know which one > > > > > exactly. Whichever one changed default behavior from use GPIOs to use > > > > > modem ctrl lines. > > > > > > > > > > Reading it again, I *think* this patch is correct. Default behavior is > > > > > restored to use GPIOs. The DT property is needed to enable modem ctrl > > > > > lines. > > > > > > > > Hi, > > > > this is correct. > > > > > > > > > > > > > What's not okay is just saying, these platforms may or may not need an update: > > > > > > > > > > arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts > > > > > mips/boot/dts/ingenic/cu1830-neo.dts > > > > > mips/boot/dts/ingenic/cu1000-neo.dts > > > > > > > > Yes, my bad. I initially mentioned them and hoped to get some > > > > feedback, which I never got, and I kind of forgot about it. > > > > > > > > > You need to figure that out. Have you checked with maintainers of > > > > > these boards? When were they added and by who? At the same time or by > > > > > the same person would be a good indication the platform uses modem > > > > > ctrl lines. Or were these platforms in use before adding modem ctrl > > > > > support? Then they probably use GPIOs or nothing. > > > > > > > > > > If there are platforms which would regress if the modem ctrl feature > > > > > was just reverted, which ones are those? > > > > > > > > Ok, let me do some checks and get back to you on this. > > > > > > Hi Rob, > > > for this board: > > > arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts > > > > > > it uses a SC16IS740, which doesn't have any GPIOs nor modem > > > control lines, so no DT changes required. > > > > > > For these two Ingenic boards: > > > mips/boot/dts/ingenic/cu1830-neo.dts > > > mips/boot/dts/ingenic/cu1000-neo.dts > > > > > > They use a SC16IS752, which has shared modem control lines and GPIOs. > > > Unfortunately, the maintainers have not (yet) responded to my > > > inquiries. Also, I tried to search for schematics or block diagrams on > > > the net but couldn't find anything. > > > > > > These platforms were in use before the patch to add the modem control > > > lines was added. Then like you said they probably use these shared > > > lines as GPIOs or nothing, so no DT changes would be required. > > > > Okay, that's useful (please add to the commit msg). > > I added the information in the cover letter, but I can add it to the > actual patch commit message if you prefer. > > > Still, what platform(s) need the modem control feature? Presumably > > that's whatever platform Lech and Tomasz work on. I guess given the > > Reviewed-by they are fine with needing a DT change. > > Ok. I have previously also emailed Lech about that, but he has not > responded yet. Hi Rob, Lech just confirmed that he made the DT changes to a board that was used only internally, and that no other DT changes should be necessary for in-tree DTS. Hugo.
From: Hugo Villeneuve <hvilleneuve@dimonoff.com> Hello, this patch series mainly fixes a GPIO regression and improve RS485 flags and properties detection from DT. It now also includes various small fixes and improvements that were previously sent as separate patches, but that made testing everything difficult. Patch 1 fixes an issue with init of first port during probing. Patch 2 fixes an issue when debugging IOcontrol register, but it is also necessary for patch "fix regression with GPIO configuration" to work. Patch 3 fixes an incorrect label in sc16is7xx_probe() cleanup code. Patch 4 is a refactor of GPIO registration code in preparation for patch 5. Patches 5 and 6 fix a GPIO regression by (re)allowing to choose GPIO function for GPIO pins shared with modem status lines. Patch 7 fixes a bug with the output value when first setting the GPIO direction. Patch 8 allows to read common rs485 device-tree flags and properties. Patch 9 introduces a delay after a reset operation to respect datasheet timing recommandations. Patch 10 improves comments about chip variants. I have tested the changes on a custom board with two SC16IS752 DUART using a Variscite IMX8MN NANO SOM. Thank you. Link: [v1] https://lkml.org/lkml/2023/5/17/967 [v1] https://lkml.org/lkml/2023/5/17/777 [v1] https://lkml.org/lkml/2023/5/17/780 [v1] https://lkml.org/lkml/2023/5/17/785 [v1] https://lkml.org/lkml/2023/5/17/1311 [v2] https://lkml.org/lkml/2023/5/18/516 [v3] https://lkml.org/lkml/2023/5/25/7 [v4] https://lkml.org/lkml/2023/5/29/656 [v5] https://lkml.org/lkml/2023/6/1/1046 [v6] https://lkml.org/lkml/2023/6/1/1328 [v7] https://lkml.org/lkml/2023/6/2/861 Changes for V3: - Integrated all patches into single serie to facilitate debugging and tests. - Reduce number of exported GPIOs depending on new property nxp,modem-control-line-ports - Added additional example in DT bindings Changes for V4: - Increase reset post delay to relax scheduler. - Put comments patches at the end. - Remove Fixes tag for patch "mark IOCONTROL register as volatile". - Improve commit messages after reviews. - Fix coding style issues after reviews. - Change GPIO registration to always register the maximum number of GPIOs supported by the chip, but maks-out GPIOs declared as modem control lines. - Add patch to refactor GPIO registration. - Remove patch "serial: sc16is7xx: fix syntax error in comments". - Remove patch "add dump registers function" Changes for V5: - Change patch order to facilitate stable backport(s). - Change duplicate device addresses in DT binding examples. - Use GENMASK for bit masks. - Replace of_property_for_each_u32() with device_property_read_u32_array - Add "Cc: stable..." tags Changes for V6: - Fix compilation bug introduced by patch 3 Changes for V7: - Minor changes and coding style fixes after review for patch 5 "fix regression with GPIO configuration". Changes for V8: - Move mctrl_mask to "struct sc16is7xx_port" to avoid compiler warning when CONFIG_GPIOLIB is undefined. - Add "struct device" member to "struct sc16is7xx_port", in order to avoid passing a raw "struct device" to called functions from sc16is7xx_probe(). - Add new patch "serial: sc16is7xx: remove obsolete out_thread label" Hugo Villeneuve (10): serial: sc16is7xx: fix broken port 0 uart init serial: sc16is7xx: mark IOCONTROL register as volatile serial: sc16is7xx: remove obsolete out_thread label serial: sc16is7xx: refactor GPIO controller registration dt-bindings: sc16is7xx: Add property to change GPIO function serial: sc16is7xx: fix regression with GPIO configuration serial: sc16is7xx: fix bug when first setting GPIO direction serial: sc16is7xx: add call to get rs485 DT flags and properties serial: sc16is7xx: add post reset delay serial: sc16is7xx: improve comments about variants .../bindings/serial/nxp,sc16is7xx.txt | 46 +++++ drivers/tty/serial/sc16is7xx.c | 177 +++++++++++++----- 2 files changed, 181 insertions(+), 42 deletions(-) base-commit: f7e3a1bafdea735050dfde00523cf505dc7fd309