Re: [PATCHv3 1/4] phy: phy-core: Make GENERIC_PHY an invisible option

Felipe Balbi
2015-06-01T18:39:02Z

[-- Attachment #1: Type: text/plain, Size: 5984 bytes --]

On Mon, Jun 01, 2015 at 06:22:41PM +0530, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Friday 29 May 2015 08:34 PM, Felipe Balbi wrote:
> >Hi,
> >
> >On Fri, May 29, 2015 at 05:04:38PM +0530, Kishon Vijay Abraham I wrote:
> >>Hi Felipe,
> >>
> >>On Wednesday 27 May 2015 12:09 AM, Felipe Balbi wrote:
> >>>On Tue, May 26, 2015 at 11:37:17AM -0700, Arun Ramamurthy wrote:
> >>>>Hi
> >>>>
> >>>>On 15-05-26 07:19 AM, Felipe Balbi wrote:
> >>>>>HI,
> >>>>>
> >>>>>On Mon, May 25, 2015 at 02:19:58PM -0700, Arun Ramamurthy wrote:
> >>>>>>
> >>>>>>
> >>>>>>On 15-05-14 05:52 PM, Felipe Balbi wrote:
> >>>>>>>Hi,
> >>>>>>>
> >>>>>>>On Wed, Apr 22, 2015 at 04:04:10PM -0700, Arun Ramamurthy wrote:
> >>>>>>>>Most of the phy providers use "select" to enable GENERIC_PHY. Since select
> >>>>>>>>is only recommended when the config is not visible, GENERIC_PHY is changed
> >>>>>>>>an invisible option. To maintain consistency, all phy providers are changed
> >>>>>>>>to "select" GENERIC_PHY and all non-phy drivers use "depends on" when the
> >>>>>>>>phy framework is explicity required. USB_MUSB_OMAP2PLUS has a cyclic
> >>>>>>>>dependency, so it is left as "select".
> >>>>>>>>
> >>>>>>>>Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>
> >>>>>>>>---
> >>>>>>>> drivers/ata/Kconfig | 1 -
> >>>>>>>> drivers/media/platform/exynos4-is/Kconfig | 2 +-
> >>>>>>>> drivers/phy/Kconfig | 4 ++--
> >>>>>>>> drivers/usb/host/Kconfig | 4 ++--
> >>>>>>>> drivers/video/fbdev/exynos/Kconfig | 2 +-
> >>>>>>>> 5 files changed, 6 insertions(+), 7 deletions(-)
> >>>>>>>>
> >>>>>>>>diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> >>>>>>>>index 5f60155..6d2e881 100644
> >>>>>>>>--- a/drivers/ata/Kconfig
> >>>>>>>>+++ b/drivers/ata/Kconfig
> >>>>>>>>@@ -301,7 +301,6 @@ config SATA_MV
> >>>>>>>> tristate "Marvell SATA support"
> >>>>>>>> depends on PCI || ARCH_DOVE || ARCH_MV78XX0 || \
> >>>>>>>> ARCH_MVEBU || ARCH_ORION5X || COMPILE_TEST
> >>>>>>>>- select GENERIC_PHY
> >>>>>>>> help
> >>>>>>>> This option enables support for the Marvell Serial ATA family.
> >>>>>>>> Currently supports 88SX[56]0[48][01] PCI(-X) chips,
> >>>>>>>>diff --git a/drivers/media/platform/exynos4-is/Kconfig b/drivers/media/platform/exynos4-is/Kconfig
> >>>>>>>>index b7b2e47..b6f3eaa 100644
> >>>>>>>>--- a/drivers/media/platform/exynos4-is/Kconfig
> >>>>>>>>+++ b/drivers/media/platform/exynos4-is/Kconfig
> >>>>>>>>@@ -31,7 +31,7 @@ config VIDEO_S5P_FIMC
> >>>>>>>> config VIDEO_S5P_MIPI_CSIS
> >>>>>>>> tristate "S5P/EXYNOS MIPI-CSI2 receiver (MIPI-CSIS) driver"
> >>>>>>>> depends on REGULATOR
> >>>>>>>>- select GENERIC_PHY
> >>>>>>>>+ depends on GENERIC_PHY
> >>>>>>>> help
> >>>>>>>> This is a V4L2 driver for Samsung S5P and EXYNOS4 SoC MIPI-CSI2
> >>>>>>>> receiver (MIPI-CSIS) devices.
> >>>>>>>>diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> >>>>>>>>index 2962de2..edecdb1 100644
> >>>>>>>>--- a/drivers/phy/Kconfig
> >>>>>>>>+++ b/drivers/phy/Kconfig
> >>>>>>>>@@ -5,7 +5,7 @@
> >>>>>>>> menu "PHY Subsystem"
> >>>>>>>>
> >>>>>>>> config GENERIC_PHY
> >>>>>>>>- bool "PHY Core"
> >>>>>>>>+ bool
> >>>>>>>> help
> >>>>>>>> Generic PHY support.
> >>>>>>>>
> >>>>>>>>@@ -72,7 +72,7 @@ config PHY_MIPHY365X
> >>>>>>>> config PHY_RCAR_GEN2
> >>>>>>>> tristate "Renesas R-Car generation 2 USB PHY driver"
> >>>>>>>> depends on ARCH_SHMOBILE
> >>>>>>>>- depends on GENERIC_PHY
> >>>>>>>>+ select GENERIC_PHY
> >>>>>>>
> >>>>>>>so some you changed from depends to select...
> >>>>>>>
> >>>>>>>> help
> >>>>>>>> Support for USB PHY found on Renesas R-Car generation 2 SoCs.
> >>>>>>>>
> >>>>>>>>diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> >>>>>>>>index 5ad60e4..e2197e2 100644
> >>>>>>>>--- a/drivers/usb/host/Kconfig
> >>>>>>>>+++ b/drivers/usb/host/Kconfig
> >>>>>>>>@@ -182,7 +182,7 @@ config USB_EHCI_HCD_SPEAR
> >>>>>>>> config USB_EHCI_HCD_STI
> >>>>>>>> tristate "Support for ST STiHxxx on-chip EHCI USB controller"
> >>>>>>>> depends on ARCH_STI && OF
> >>>>>>>>- select GENERIC_PHY
> >>>>>>>>+ depends on GENERIC_PHY
> >>>>>>>
> >>>>>>>while others you changed from select to depends.
> >>>>>>>
> >>>>>>>NAK.
> >>>>>>>
> >>>>>>Felipe, I dont understand your concern, could you please explain it more
> >>>>>>detail? The logic behind the changes is that in cases where there was an
> >>>>>>explicit dependency, I changed it to "depends on" and in other cases I
> >>>>>>changed it to "selects". Thanks
> >>>>>
> >>>>>Since GENERIC_PHY is visible from Kconfig, it would be much nicer to
> >>>>>avoid select altogether.
> >>>>>
> >>>>Felipe, after discussion with the maintainers, I have made GENERIC_PHY an
> >>>>invisible option as part of this change. Thanks
> >>>
> >>>Then, if the option is invisible, how can you "depend" on it ? It can
> >>>never be selected by poking around in Kconfig. IMO, it's
> >>>counterintuitive that you need to enable a PHY driver before you can see
> >>>your EHCI/OHCI/whatever controller listed in Kconfig.
> >>
> >>If the controller requires PHY for it to be functional, it is okay to make
> >>the controller depend on PHY IMHO. We want to try and minimize the usage of
> >>'select' wherever possible or else 'select' is the most intuitive way. The
> >>other option is just to leave the 'depends on' and let the user select PHY.
> >
> >How can you 'depend' on something that the user can't select by
> >navigating through Kconfig ?
>
> hmm... Actually it's selected when the user selects the PHY driver.

that's my point, don't you think it's a little counter-intuitive ?

> Maybe we should directly depend on the PHY driver instead of Generic
> PHY?

maybe... But then what do you do when you have different boards using
different PHYs ?

--
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]