| Message ID | 20190326155728.5432-1-brgl@bgdev.pl (mailing list archive) |
|---|---|
| Headers | show |
| Series | ARM: davinci: ohci-da8xx: model the vbus GPIO as a fixed regulator | expand |
Hi Bart, On 26/03/19 9:27 PM, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Adding the vbus GPIO support to the ohci-da8xx driver isn't really the > optimal solution. Rather: it should be modeled as a fixed regulator > in which case the driver already has support. Can you clarify "driver already has support"? You are introducing support to use the VBUS gpio as regulator as part of 3/3. I do see other instances of VBUS regulator being used in USB tree. But we just converted the driver to use VBUS and over-current GPIOs in v5.1. So this is a bit of "churn". Can you document why the current solution is not optimal? Is it to make future device-tree conversion for these boards easier? Or? > > This series adds necessary fixups to the board files and removes the > vbus GPIO from the ohci driver. Thanks, Sekhar
śr., 27 mar 2019 o 12:37 Sekhar Nori <nsekhar@ti.com> napisał(a): > > Hi Bart, > > On 26/03/19 9:27 PM, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > Adding the vbus GPIO support to the ohci-da8xx driver isn't really the > > optimal solution. Rather: it should be modeled as a fixed regulator > > in which case the driver already has support. > > Can you clarify "driver already has support"? You are introducing > support to use the VBUS gpio as regulator as part of 3/3. > The support is there as in: if the driver can obtain the regulator, it will enable it. The overcurrent protection does not work however and this is what patch 3 adds. Maybe I should rework the ordering in that I'd first add the irq thread disabling the regulator if it exists, then the regulator fixups to board files and then remove the vbus GPIO. > I do see other instances of VBUS regulator being used in USB tree. But > we just converted the driver to use VBUS and over-current GPIOs in v5.1. > So this is a bit of "churn". > Yes and it's my fault - I simply converted the legacy code without giving it enough consideration. I should have used a fixed regulator right away, but now it's upstream and we need a follow-up series. > Can you document why the current solution is not optimal? Is it to make > future device-tree conversion for these boards easier? Or? > It's sub-optimal from the HW modeling in SW PoV - it is in fact a regulator enabled/disabled by a GPIO. Also: it's code duplication as currently we check if the vbus GPIO exists and then use it or check if the regulator exists and use this as the second choice. The third patch actually shrinks the driver. Bart > > > > This series adds necessary fixups to the board files and removes the > > vbus GPIO from the ohci driver. > > Thanks, > Sekhar
On 27/03/19 6:46 PM, Bartosz Golaszewski wrote: > śr., 27 mar 2019 o 12:37 Sekhar Nori <nsekhar@ti.com> napisał(a): >> >> Hi Bart, >> >> On 26/03/19 9:27 PM, Bartosz Golaszewski wrote: >>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com> >>> >>> Adding the vbus GPIO support to the ohci-da8xx driver isn't really the >>> optimal solution. Rather: it should be modeled as a fixed regulator >>> in which case the driver already has support. >> >> Can you clarify "driver already has support"? You are introducing >> support to use the VBUS gpio as regulator as part of 3/3. >> > > The support is there as in: if the driver can obtain the regulator, it > will enable it. The overcurrent protection does not work however and > this is what patch 3 adds. Maybe I should rework the ordering in that > I'd first add the irq thread disabling the regulator if it exists, > then the regulator fixups to board files and then remove the vbus > GPIO. > >> I do see other instances of VBUS regulator being used in USB tree. But >> we just converted the driver to use VBUS and over-current GPIOs in v5.1. >> So this is a bit of "churn". >> > > Yes and it's my fault - I simply converted the legacy code without > giving it enough consideration. I should have used a fixed regulator > right away, but now it's upstream and we need a follow-up series. > >> Can you document why the current solution is not optimal? Is it to make >> future device-tree conversion for these boards easier? Or? >> > > It's sub-optimal from the HW modeling in SW PoV - it is in fact a > regulator enabled/disabled by a GPIO. Also: it's code duplication as > currently we check if the vbus GPIO exists and then use it or check if > the regulator exists and use this as the second choice. The third > patch actually shrinks the driver. I see now that the driver supports controlling the VBUS gpio as regulator already. Something I should have caught in review last time around. I agree this patch is an improvement. Lets see what Alan feels. Also, reg_enabled member of da8xx_ohci_hcd structure seems to be pretty useless considering regulator API already has use counting. Can you take a look and remove that too as an added bonus. Thanks, Sekhar
On Thu, 28 Mar 2019, Sekhar Nori wrote: > >> Can you document why the current solution is not optimal? Is it to make > >> future device-tree conversion for these boards easier? Or? > >> > > > > It's sub-optimal from the HW modeling in SW PoV - it is in fact a > > regulator enabled/disabled by a GPIO. Also: it's code duplication as > > currently we check if the vbus GPIO exists and then use it or check if > > the regulator exists and use this as the second choice. The third > > patch actually shrinks the driver. > > I see now that the driver supports controlling the VBUS gpio as > regulator already. Something I should have caught in review last time > around. > > I agree this patch is an improvement. Lets see what Alan feels. I'm not an expert on this stuff, but the patch looks reasonable. However, I do wish that in the devm_request_threaded_irq() call, the indentation of the continuation lines was left unchanged. Alan Stern > Also, reg_enabled member of da8xx_ohci_hcd structure seems to be pretty > useless considering regulator API already has use counting. Can you take > a look and remove that too as an added bonus. > > Thanks, > Sekhar
czw., 28 mar 2019 o 15:11 Alan Stern <stern@rowland.harvard.edu> napisał(a): > > On Thu, 28 Mar 2019, Sekhar Nori wrote: > > > >> Can you document why the current solution is not optimal? Is it to make > > >> future device-tree conversion for these boards easier? Or? > > >> > > > > > > It's sub-optimal from the HW modeling in SW PoV - it is in fact a > > > regulator enabled/disabled by a GPIO. Also: it's code duplication as > > > currently we check if the vbus GPIO exists and then use it or check if > > > the regulator exists and use this as the second choice. The third > > > patch actually shrinks the driver. > > > > I see now that the driver supports controlling the VBUS gpio as > > regulator already. Something I should have caught in review last time > > around. > > > > I agree this patch is an improvement. Lets see what Alan feels. > > I'm not an expert on this stuff, but the patch looks reasonable. > However, I do wish that in the devm_request_threaded_irq() call, the > indentation of the continuation lines was left unchanged. > I don't think it's possible - the function name is longer and the first line exceeds the 80 characters limit. I can put all the parameters below the function name if you prefer that? Bart > Alan Stern > > > Also, reg_enabled member of da8xx_ohci_hcd structure seems to be pretty > > useless considering regulator API already has use counting. Can you take > > a look and remove that too as an added bonus. > > > > Thanks, > > Sekhar > >
On Thu, 28 Mar 2019, Bartosz Golaszewski wrote: > czw., 28 mar 2019 o 15:11 Alan Stern <stern@rowland.harvard.edu> napisał(a): > > > > On Thu, 28 Mar 2019, Sekhar Nori wrote: > > > > > >> Can you document why the current solution is not optimal? Is it to make > > > >> future device-tree conversion for these boards easier? Or? > > > >> > > > > > > > > It's sub-optimal from the HW modeling in SW PoV - it is in fact a > > > > regulator enabled/disabled by a GPIO. Also: it's code duplication as > > > > currently we check if the vbus GPIO exists and then use it or check if > > > > the regulator exists and use this as the second choice. The third > > > > patch actually shrinks the driver. > > > > > > I see now that the driver supports controlling the VBUS gpio as > > > regulator already. Something I should have caught in review last time > > > around. > > > > > > I agree this patch is an improvement. Lets see what Alan feels. > > > > I'm not an expert on this stuff, but the patch looks reasonable. > > However, I do wish that in the devm_request_threaded_irq() call, the > > indentation of the continuation lines was left unchanged. > > > > I don't think it's possible - the function name is longer and the > first line exceeds the 80 characters limit. I can put all the > parameters below the function name if you prefer that? Which line the arguments go on doesn't matter. But increasing the amount of indentation, like the patch did, makes the whole thing less readable, IMO. It makes everything end up crammed against the right margin. Alan Stern
From: Bartosz Golaszewski <bgolaszewski@baylibre.com> Adding the vbus GPIO support to the ohci-da8xx driver isn't really the optimal solution. Rather: it should be modeled as a fixed regulator in which case the driver already has support. This series adds necessary fixups to the board files and removes the vbus GPIO from the ohci driver. Bartosz Golaszewski (3): ARM: davinci: omapl138-hawk: add a fixed regulator for ohci-da8xx ARM: davinci: da830-evm: add a fixed regulator for ohci-da8xx usb: ohci-da8xx: drop the vbus GPIO arch/arm/mach-davinci/board-da830-evm.c | 48 +++++++++++++++++++-- arch/arm/mach-davinci/board-omapl138-hawk.c | 48 +++++++++++++++++++-- drivers/usb/host/ohci-da8xx.c | 39 ++++++++--------- 3 files changed, 108 insertions(+), 27 deletions(-)