On 27/03/19 6:46 PM, Bartosz Golaszewski wrote: > śr., 27 mar 2019 o 12:37 Sekhar Nori <nsekhar@xxxxxx> napisał(a): >> >> Hi Bart, >> >> On 26/03/19 9:27 PM, Bartosz Golaszewski wrote: >>> From: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx> >>> >>> 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
