[PATCH] drm/bridge: sil_sii8620: depend on INPUT instead of selecting it.

[PATCH] drm/bridge: sil_sii8620: depend on INPUT instead of selecting it.

[Ronald Tschalär]

commit d6abe6df706c66d803e6dd4fe98c1b6b7f125a56 (drm/bridge:
sil_sii8620: do not have a dependency of RC_CORE) added a dependency on
INPUT. However, this causes problems with other drivers, in particular
an input driver that depends on MFD_INTEL_LPSS_PCI (to be added in a
future commit):

  drivers/clk/Kconfig:9:error: recursive dependency detected!
  drivers/clk/Kconfig:9:        symbol COMMON_CLK is selected by MFD_INTEL_LPSS
  drivers/mfd/Kconfig:566:      symbol MFD_INTEL_LPSS is selected by MFD_INTEL_LPSS_PCI
  drivers/mfd/Kconfig:580:      symbol MFD_INTEL_LPSS_PCI is implied by KEYBOARD_APPLESPI
  drivers/input/keyboard/Kconfig:73:    symbol KEYBOARD_APPLESPI depends on INPUT
  drivers/input/Kconfig:8:      symbol INPUT is selected by DRM_SIL_SII8620
  drivers/gpu/drm/bridge/Kconfig:83:    symbol DRM_SIL_SII8620 depends on DRM_BRIDGE
  drivers/gpu/drm/bridge/Kconfig:1:     symbol DRM_BRIDGE is selected by DRM_PL111
  drivers/gpu/drm/pl111/Kconfig:1:      symbol DRM_PL111 depends on COMMON_CLK

According to the docs, select should only be used for non-visible
symbols. Furthermore almost all other references to INPUT throughout the
kernel config are depends, not selects. Hence this change.

CC: Inki Dae <inki.dae@samsung.com>
CC: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
---
 drivers/gpu/drm/bridge/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 2fee47b0d50b..eabedc83f25c 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -83,9 +83,9 @@ config DRM_PARADE_PS8622
 config DRM_SIL_SII8620
 	tristate "Silicon Image SII8620 HDMI/MHL bridge"
 	depends on OF
+	depends on INPUT
 	select DRM_KMS_HELPER
 	imply EXTCON
-	select INPUT
 	select RC_CORE
 	help
 	  Silicon Image SII8620 HDMI/MHL bridge chip driver.
-- 
2.20.1

Re: [PATCH] drm/bridge: sil_sii8620: depend on INPUT instead of selecting it.

[Laurent Pinchart]

Hi Ronald,

Thank you for the patch.

On Tue, Jan 22, 2019 at 06:13:11AM -0800, Ronald Tschalär wrote:
> commit d6abe6df706c66d803e6dd4fe98c1b6b7f125a56 (drm/bridge:

Commits are usually quoted using the short 12 digits version of their
SHA-1.

> sil_sii8620: do not have a dependency of RC_CORE) added a dependency on
> INPUT. However, this causes problems with other drivers, in particular
> an input driver that depends on MFD_INTEL_LPSS_PCI (to be added in a
> future commit):
> 
>   drivers/clk/Kconfig:9:error: recursive dependency detected!
>   drivers/clk/Kconfig:9:        symbol COMMON_CLK is selected by MFD_INTEL_LPSS
>   drivers/mfd/Kconfig:566:      symbol MFD_INTEL_LPSS is selected by MFD_INTEL_LPSS_PCI
>   drivers/mfd/Kconfig:580:      symbol MFD_INTEL_LPSS_PCI is implied by KEYBOARD_APPLESPI
>   drivers/input/keyboard/Kconfig:73:    symbol KEYBOARD_APPLESPI depends on INPUT
>   drivers/input/Kconfig:8:      symbol INPUT is selected by DRM_SIL_SII8620
>   drivers/gpu/drm/bridge/Kconfig:83:    symbol DRM_SIL_SII8620 depends on DRM_BRIDGE
>   drivers/gpu/drm/bridge/Kconfig:1:     symbol DRM_BRIDGE is selected by DRM_PL111
>   drivers/gpu/drm/pl111/Kconfig:1:      symbol DRM_PL111 depends on COMMON_CLK
> 
> According to the docs, select should only be used for non-visible
> symbols. Furthermore almost all other references to INPUT throughout the
> kernel config are depends, not selects. Hence this change.
> 
> CC: Inki Dae <inki.dae@samsung.com>
> CC: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
> ---
>  drivers/gpu/drm/bridge/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 2fee47b0d50b..eabedc83f25c 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -83,9 +83,9 @@ config DRM_PARADE_PS8622
>  config DRM_SIL_SII8620
>  	tristate "Silicon Image SII8620 HDMI/MHL bridge"
>  	depends on OF
> +	depends on INPUT
>  	select DRM_KMS_HELPER
>  	imply EXTCON
> -	select INPUT
>  	select RC_CORE

RC_CORE is always a user-visible symbol (which by the way depends on
INPUT). I think reverting commit d6abe6df706c would be a better
solution.

>  	help
>  	  Silicon Image SII8620 HDMI/MHL bridge chip driver.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm/bridge: sil_sii8620: depend on INPUT instead of selecting it.

[Life is hard, and then you die]

  Hi Laurent,

On Tue, Jan 22, 2019 at 11:10:28PM +0200, Laurent Pinchart wrote:
> 
> Thank you for the patch.
> 
> On Tue, Jan 22, 2019 at 06:13:11AM -0800, Ronald Tschalär wrote:
> > commit d6abe6df706c66d803e6dd4fe98c1b6b7f125a56 (drm/bridge:
> 
> Commits are usually quoted using the short 12 digits version of their
> SHA-1.

Oh, right, sorry - will fix in next version of patch.

[snip]


  Cheers,

  Ronald

Re: [PATCH] drm/bridge: sil_sii8620: depend on INPUT instead of selecting it.

[Lukas Wunner]

On Tue, Jan 22, 2019 at 06:13:11AM -0800, Ronald Tschalär wrote:
> commit d6abe6df706c66d803e6dd4fe98c1b6b7f125a56 (drm/bridge:
> sil_sii8620: do not have a dependency of RC_CORE) added a dependency on
> INPUT. However, this causes problems with other drivers, in particular
> an input driver that depends on MFD_INTEL_LPSS_PCI (to be added in a
> future commit):
> 
>   drivers/clk/Kconfig:9:error: recursive dependency detected!
>   drivers/clk/Kconfig:9:        symbol COMMON_CLK is selected by MFD_INTEL_LPSS
>   drivers/mfd/Kconfig:566:      symbol MFD_INTEL_LPSS is selected by MFD_INTEL_LPSS_PCI
>   drivers/mfd/Kconfig:580:      symbol MFD_INTEL_LPSS_PCI is implied by KEYBOARD_APPLESPI
>   drivers/input/keyboard/Kconfig:73:    symbol KEYBOARD_APPLESPI depends on INPUT
>   drivers/input/Kconfig:8:      symbol INPUT is selected by DRM_SIL_SII8620
>   drivers/gpu/drm/bridge/Kconfig:83:    symbol DRM_SIL_SII8620 depends on DRM_BRIDGE
>   drivers/gpu/drm/bridge/Kconfig:1:     symbol DRM_BRIDGE is selected by DRM_PL111
>   drivers/gpu/drm/pl111/Kconfig:1:      symbol DRM_PL111 depends on COMMON_CLK
> 
> According to the docs, select should only be used for non-visible
> symbols. Furthermore almost all other references to INPUT throughout the
> kernel config are depends, not selects. Hence this change.
> 
> CC: Inki Dae <inki.dae@samsung.com>
> CC: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Ronald Tschalär <ronald@innovation.ch>

Reviewed-by: Lukas Wunner <lukas@wunner.de>

I think this needs to be merged through the input tree as a prerequisite
for the applespi.c driver (keyboard + touchpad driver for 2015+ MacBook,
MacBook Air and MacBook Pro which uses SPI instead of USB) to avoid
breaking the build.  Adding Dmitry.

Thanks,

Lukas


> ---
>  drivers/gpu/drm/bridge/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 2fee47b0d50b..eabedc83f25c 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -83,9 +83,9 @@ config DRM_PARADE_PS8622
>  config DRM_SIL_SII8620
>  	tristate "Silicon Image SII8620 HDMI/MHL bridge"
>  	depends on OF
> +	depends on INPUT
>  	select DRM_KMS_HELPER
>  	imply EXTCON
> -	select INPUT
>  	select RC_CORE
>  	help
>  	  Silicon Image SII8620 HDMI/MHL bridge chip driver.
> -- 
> 2.20.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/bridge: sil_sii8620: depend on INPUT instead of selecting it.

[Dmitry Torokhov]

On Wed, Jan 23, 2019 at 09:45:56AM +0100, Lukas Wunner wrote:
> On Tue, Jan 22, 2019 at 06:13:11AM -0800, Ronald Tschalär wrote:
> > commit d6abe6df706c66d803e6dd4fe98c1b6b7f125a56 (drm/bridge:
> > sil_sii8620: do not have a dependency of RC_CORE) added a dependency on
> > INPUT. However, this causes problems with other drivers, in particular
> > an input driver that depends on MFD_INTEL_LPSS_PCI (to be added in a
> > future commit):
> > 
> >   drivers/clk/Kconfig:9:error: recursive dependency detected!
> >   drivers/clk/Kconfig:9:        symbol COMMON_CLK is selected by MFD_INTEL_LPSS
> >   drivers/mfd/Kconfig:566:      symbol MFD_INTEL_LPSS is selected by MFD_INTEL_LPSS_PCI
> >   drivers/mfd/Kconfig:580:      symbol MFD_INTEL_LPSS_PCI is implied by KEYBOARD_APPLESPI
> >   drivers/input/keyboard/Kconfig:73:    symbol KEYBOARD_APPLESPI depends on INPUT
> >   drivers/input/Kconfig:8:      symbol INPUT is selected by DRM_SIL_SII8620
> >   drivers/gpu/drm/bridge/Kconfig:83:    symbol DRM_SIL_SII8620 depends on DRM_BRIDGE
> >   drivers/gpu/drm/bridge/Kconfig:1:     symbol DRM_BRIDGE is selected by DRM_PL111
> >   drivers/gpu/drm/pl111/Kconfig:1:      symbol DRM_PL111 depends on COMMON_CLK
> > 
> > According to the docs, select should only be used for non-visible
> > symbols. Furthermore almost all other references to INPUT throughout the
> > kernel config are depends, not selects. Hence this change.

I think this is not as cut and dry. We should be able to select needed
subsystems (such as INPUT, USB, etc) even if they are user visible.
User, when enabling a piece of hardware, does not need to know ultimate
details of all subsystems the driver might need ti function.

It looks like one of the drivers implies MFD_INTEL_LPSS_PCI, maybe
treating imply the same as select when detecting circular dependency is
wrong as we are allowed to deselect implied dependencies?

> > 
> > CC: Inki Dae <inki.dae@samsung.com>
> > CC: Andrzej Hajda <a.hajda@samsung.com>
> > Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
> 
> Reviewed-by: Lukas Wunner <lukas@wunner.de>
> 
> I think this needs to be merged through the input tree as a prerequisite
> for the applespi.c driver (keyboard + touchpad driver for 2015+ MacBook,
> MacBook Air and MacBook Pro which uses SPI instead of USB) to avoid
> breaking the build.  Adding Dmitry.

I have no idea what applespi.c is (it is definitely not in my tree), so
I think it should be merged through the same tree that the original
commit was introduced through.

> 
> Thanks,
> 
> Lukas
> 
> 
> > ---
> >  drivers/gpu/drm/bridge/Kconfig | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > index 2fee47b0d50b..eabedc83f25c 100644
> > --- a/drivers/gpu/drm/bridge/Kconfig
> > +++ b/drivers/gpu/drm/bridge/Kconfig
> > @@ -83,9 +83,9 @@ config DRM_PARADE_PS8622
> >  config DRM_SIL_SII8620
> >  	tristate "Silicon Image SII8620 HDMI/MHL bridge"
> >  	depends on OF
> > +	depends on INPUT
> >  	select DRM_KMS_HELPER
> >  	imply EXTCON
> > -	select INPUT
> >  	select RC_CORE

Keeping "select RC_CORE" is wrong though, as the driver appears to be
working find without RC. Maybe it should be stubbed out?

Thanks.

-- 
Dmitry

Re: [PATCH] drm/bridge: sil_sii8620: depend on INPUT instead of selecting it.

[Laurent Pinchart]

Hello Dmity,

On Wed, Jan 23, 2019 at 02:03:42PM -0800, Dmitry Torokhov wrote:
> On Wed, Jan 23, 2019 at 09:45:56AM +0100, Lukas Wunner wrote:
> > On Tue, Jan 22, 2019 at 06:13:11AM -0800, Ronald Tschalär wrote:
> >> commit d6abe6df706c66d803e6dd4fe98c1b6b7f125a56 (drm/bridge:
> >> sil_sii8620: do not have a dependency of RC_CORE) added a dependency on
> >> INPUT. However, this causes problems with other drivers, in particular
> >> an input driver that depends on MFD_INTEL_LPSS_PCI (to be added in a
> >> future commit):
> >> 
> >>   drivers/clk/Kconfig:9:error: recursive dependency detected!
> >>   drivers/clk/Kconfig:9:        symbol COMMON_CLK is selected by MFD_INTEL_LPSS
> >>   drivers/mfd/Kconfig:566:      symbol MFD_INTEL_LPSS is selected by MFD_INTEL_LPSS_PCI
> >>   drivers/mfd/Kconfig:580:      symbol MFD_INTEL_LPSS_PCI is implied by KEYBOARD_APPLESPI
> >>   drivers/input/keyboard/Kconfig:73:    symbol KEYBOARD_APPLESPI depends on INPUT
> >>   drivers/input/Kconfig:8:      symbol INPUT is selected by DRM_SIL_SII8620
> >>   drivers/gpu/drm/bridge/Kconfig:83:    symbol DRM_SIL_SII8620 depends on DRM_BRIDGE
> >>   drivers/gpu/drm/bridge/Kconfig:1:     symbol DRM_BRIDGE is selected by DRM_PL111
> >>   drivers/gpu/drm/pl111/Kconfig:1:      symbol DRM_PL111 depends on COMMON_CLK
> >> 
> >> According to the docs, select should only be used for non-visible
> >> symbols. Furthermore almost all other references to INPUT throughout the
> >> kernel config are depends, not selects. Hence this change.
> 
> I think this is not as cut and dry. We should be able to select needed
> subsystems (such as INPUT, USB, etc) even if they are user visible.

Semantically, maybe, but given the current state of Kconfig this results
in a recursive dependencies nightmare. It's a no-go.

> User, when enabling a piece of hardware, does not need to know ultimate
> details of all subsystems the driver might need ti function.
> 
> It looks like one of the drivers implies MFD_INTEL_LPSS_PCI, maybe
> treating imply the same as select when detecting circular dependency is
> wrong as we are allowed to deselect implied dependencies?
> 
> >> 
> >> CC: Inki Dae <inki.dae@samsung.com>
> >> CC: Andrzej Hajda <a.hajda@samsung.com>
> >> Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
> > 
> > Reviewed-by: Lukas Wunner <lukas@wunner.de>
> > 
> > I think this needs to be merged through the input tree as a prerequisite
> > for the applespi.c driver (keyboard + touchpad driver for 2015+ MacBook,
> > MacBook Air and MacBook Pro which uses SPI instead of USB) to avoid
> > breaking the build.  Adding Dmitry.
> 
> I have no idea what applespi.c is (it is definitely not in my tree), so
> I think it should be merged through the same tree that the original
> commit was introduced through.
> 
> >> ---
> >>  drivers/gpu/drm/bridge/Kconfig | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> >> index 2fee47b0d50b..eabedc83f25c 100644
> >> --- a/drivers/gpu/drm/bridge/Kconfig
> >> +++ b/drivers/gpu/drm/bridge/Kconfig
> >> @@ -83,9 +83,9 @@ config DRM_PARADE_PS8622
> >>  config DRM_SIL_SII8620
> >>  	tristate "Silicon Image SII8620 HDMI/MHL bridge"
> >>  	depends on OF
> >> +	depends on INPUT
> >>  	select DRM_KMS_HELPER
> >>  	imply EXTCON
> >> -	select INPUT
> >>  	select RC_CORE
> 
> Keeping "select RC_CORE" is wrong though, as the driver appears to be
> working find without RC. Maybe it should be stubbed out?

It should definitely not be select'ed as it's a user-visible symbol. My
preference would be to simply revert d6abe6df706c. If we want (and can)
work without RC core then it should be stubbed out.

Commit d6abe6df706c states

    And some boards not using remote controller device don't really
    need to know that RC_CORE config should be enabled to use sil_sii8620
    driver only for HDMI.

The same reasoning applies to INPUT, if we agree that depending on
RC_CORE is confusing for users, then depending on INPUT is confusing as
well. There's not reason to apply different standards to INPUT and
RC_CORE, depending on one and selecting the other doesn't make much
sense.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm/bridge: sil_sii8620: depend on INPUT instead of selecting it.

[Dmitry Torokhov]

Hi Laurent,

On Thu, Jan 24, 2019 at 12:17:35AM +0200, Laurent Pinchart wrote:
> Hello Dmity,
> 
> On Wed, Jan 23, 2019 at 02:03:42PM -0800, Dmitry Torokhov wrote:
> > On Wed, Jan 23, 2019 at 09:45:56AM +0100, Lukas Wunner wrote:
> > > On Tue, Jan 22, 2019 at 06:13:11AM -0800, Ronald Tschalär wrote:
> > >> commit d6abe6df706c66d803e6dd4fe98c1b6b7f125a56 (drm/bridge:
> > >> sil_sii8620: do not have a dependency of RC_CORE) added a dependency on
> > >> INPUT. However, this causes problems with other drivers, in particular
> > >> an input driver that depends on MFD_INTEL_LPSS_PCI (to be added in a
> > >> future commit):
> > >> 
> > >>   drivers/clk/Kconfig:9:error: recursive dependency detected!
> > >>   drivers/clk/Kconfig:9:        symbol COMMON_CLK is selected by MFD_INTEL_LPSS
> > >>   drivers/mfd/Kconfig:566:      symbol MFD_INTEL_LPSS is selected by MFD_INTEL_LPSS_PCI
> > >>   drivers/mfd/Kconfig:580:      symbol MFD_INTEL_LPSS_PCI is implied by KEYBOARD_APPLESPI
> > >>   drivers/input/keyboard/Kconfig:73:    symbol KEYBOARD_APPLESPI depends on INPUT
> > >>   drivers/input/Kconfig:8:      symbol INPUT is selected by DRM_SIL_SII8620
> > >>   drivers/gpu/drm/bridge/Kconfig:83:    symbol DRM_SIL_SII8620 depends on DRM_BRIDGE
> > >>   drivers/gpu/drm/bridge/Kconfig:1:     symbol DRM_BRIDGE is selected by DRM_PL111
> > >>   drivers/gpu/drm/pl111/Kconfig:1:      symbol DRM_PL111 depends on COMMON_CLK
> > >> 
> > >> According to the docs, select should only be used for non-visible
> > >> symbols. Furthermore almost all other references to INPUT throughout the
> > >> kernel config are depends, not selects. Hence this change.
> > 
> > I think this is not as cut and dry. We should be able to select needed
> > subsystems (such as INPUT, USB, etc) even if they are user visible.
> 
> Semantically, maybe, but given the current state of Kconfig this results
> in a recursive dependencies nightmare. It's a no-go.
> 
> > User, when enabling a piece of hardware, does not need to know ultimate
> > details of all subsystems the driver might need ti function.
> > 
> > It looks like one of the drivers implies MFD_INTEL_LPSS_PCI, maybe
> > treating imply the same as select when detecting circular dependency is
> > wrong as we are allowed to deselect implied dependencies?
> > 
> > >> 
> > >> CC: Inki Dae <inki.dae@samsung.com>
> > >> CC: Andrzej Hajda <a.hajda@samsung.com>
> > >> Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
> > > 
> > > Reviewed-by: Lukas Wunner <lukas@wunner.de>
> > > 
> > > I think this needs to be merged through the input tree as a prerequisite
> > > for the applespi.c driver (keyboard + touchpad driver for 2015+ MacBook,
> > > MacBook Air and MacBook Pro which uses SPI instead of USB) to avoid
> > > breaking the build.  Adding Dmitry.
> > 
> > I have no idea what applespi.c is (it is definitely not in my tree), so
> > I think it should be merged through the same tree that the original
> > commit was introduced through.
> > 
> > >> ---
> > >>  drivers/gpu/drm/bridge/Kconfig | 2 +-
> > >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >> 
> > >> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > >> index 2fee47b0d50b..eabedc83f25c 100644
> > >> --- a/drivers/gpu/drm/bridge/Kconfig
> > >> +++ b/drivers/gpu/drm/bridge/Kconfig
> > >> @@ -83,9 +83,9 @@ config DRM_PARADE_PS8622
> > >>  config DRM_SIL_SII8620
> > >>  	tristate "Silicon Image SII8620 HDMI/MHL bridge"
> > >>  	depends on OF
> > >> +	depends on INPUT
> > >>  	select DRM_KMS_HELPER
> > >>  	imply EXTCON
> > >> -	select INPUT
> > >>  	select RC_CORE
> > 
> > Keeping "select RC_CORE" is wrong though, as the driver appears to be
> > working find without RC. Maybe it should be stubbed out?
> 
> It should definitely not be select'ed as it's a user-visible symbol. My
> preference would be to simply revert d6abe6df706c. If we want (and can)
> work without RC core then it should be stubbed out.
> 
> Commit d6abe6df706c states
> 
>     And some boards not using remote controller device don't really
>     need to know that RC_CORE config should be enabled to use sil_sii8620
>     driver only for HDMI.
> 
> The same reasoning applies to INPUT, if we agree that depending on
> RC_CORE is confusing for users, then depending on INPUT is confusing as
> well. There's not reason to apply different standards to INPUT and
> RC_CORE, depending on one and selecting the other doesn't make much
> sense.

OK, so revert + patch to stub out RC calls? That works for me (and I
still say it should go through the same tree that introduced
d6abe6df706c).

Thanks.

-- 
Dmitry

Re: [PATCH] drm/bridge: sil_sii8620: depend on INPUT instead of selecting it.

[Laurent Pinchart]

Hello Dmity,

On Wed, Jan 23, 2019 at 02:21:05PM -0800, Dmitry Torokhov wrote:
> On Thu, Jan 24, 2019 at 12:17:35AM +0200, Laurent Pinchart wrote:
> > On Wed, Jan 23, 2019 at 02:03:42PM -0800, Dmitry Torokhov wrote:
> >> On Wed, Jan 23, 2019 at 09:45:56AM +0100, Lukas Wunner wrote:
> >>> On Tue, Jan 22, 2019 at 06:13:11AM -0800, Ronald Tschalär wrote:
> >>>> commit d6abe6df706c66d803e6dd4fe98c1b6b7f125a56 (drm/bridge:
> >>>> sil_sii8620: do not have a dependency of RC_CORE) added a dependency on
> >>>> INPUT. However, this causes problems with other drivers, in particular
> >>>> an input driver that depends on MFD_INTEL_LPSS_PCI (to be added in a
> >>>> future commit):
> >>>> 
> >>>>   drivers/clk/Kconfig:9:error: recursive dependency detected!
> >>>>   drivers/clk/Kconfig:9:        symbol COMMON_CLK is selected by MFD_INTEL_LPSS
> >>>>   drivers/mfd/Kconfig:566:      symbol MFD_INTEL_LPSS is selected by MFD_INTEL_LPSS_PCI
> >>>>   drivers/mfd/Kconfig:580:      symbol MFD_INTEL_LPSS_PCI is implied by KEYBOARD_APPLESPI
> >>>>   drivers/input/keyboard/Kconfig:73:    symbol KEYBOARD_APPLESPI depends on INPUT
> >>>>   drivers/input/Kconfig:8:      symbol INPUT is selected by DRM_SIL_SII8620
> >>>>   drivers/gpu/drm/bridge/Kconfig:83:    symbol DRM_SIL_SII8620 depends on DRM_BRIDGE
> >>>>   drivers/gpu/drm/bridge/Kconfig:1:     symbol DRM_BRIDGE is selected by DRM_PL111
> >>>>   drivers/gpu/drm/pl111/Kconfig:1:      symbol DRM_PL111 depends on COMMON_CLK
> >>>> 
> >>>> According to the docs, select should only be used for non-visible
> >>>> symbols. Furthermore almost all other references to INPUT throughout the
> >>>> kernel config are depends, not selects. Hence this change.
> >> 
> >> I think this is not as cut and dry. We should be able to select needed
> >> subsystems (such as INPUT, USB, etc) even if they are user visible.
> > 
> > Semantically, maybe, but given the current state of Kconfig this results
> > in a recursive dependencies nightmare. It's a no-go.
> > 
> >> User, when enabling a piece of hardware, does not need to know ultimate
> >> details of all subsystems the driver might need ti function.
> >> 
> >> It looks like one of the drivers implies MFD_INTEL_LPSS_PCI, maybe
> >> treating imply the same as select when detecting circular dependency is
> >> wrong as we are allowed to deselect implied dependencies?
> >> 
> >>>> 
> >>>> CC: Inki Dae <inki.dae@samsung.com>
> >>>> CC: Andrzej Hajda <a.hajda@samsung.com>
> >>>> Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
> >>> 
> >>> Reviewed-by: Lukas Wunner <lukas@wunner.de>
> >>> 
> >>> I think this needs to be merged through the input tree as a prerequisite
> >>> for the applespi.c driver (keyboard + touchpad driver for 2015+ MacBook,
> >>> MacBook Air and MacBook Pro which uses SPI instead of USB) to avoid
> >>> breaking the build.  Adding Dmitry.
> >> 
> >> I have no idea what applespi.c is (it is definitely not in my tree), so
> >> I think it should be merged through the same tree that the original
> >> commit was introduced through.
> >> 
> >>>> ---
> >>>>  drivers/gpu/drm/bridge/Kconfig | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>> 
> >>>> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> >>>> index 2fee47b0d50b..eabedc83f25c 100644
> >>>> --- a/drivers/gpu/drm/bridge/Kconfig
> >>>> +++ b/drivers/gpu/drm/bridge/Kconfig
> >>>> @@ -83,9 +83,9 @@ config DRM_PARADE_PS8622
> >>>>  config DRM_SIL_SII8620
> >>>>  	tristate "Silicon Image SII8620 HDMI/MHL bridge"
> >>>>  	depends on OF
> >>>> +	depends on INPUT
> >>>>  	select DRM_KMS_HELPER
> >>>>  	imply EXTCON
> >>>> -	select INPUT
> >>>>  	select RC_CORE
> >> 
> >> Keeping "select RC_CORE" is wrong though, as the driver appears to be
> >> working find without RC. Maybe it should be stubbed out?
> > 
> > It should definitely not be select'ed as it's a user-visible symbol. My
> > preference would be to simply revert d6abe6df706c. If we want (and can)
> > work without RC core then it should be stubbed out.
> > 
> > Commit d6abe6df706c states
> > 
> >     And some boards not using remote controller device don't really
> >     need to know that RC_CORE config should be enabled to use sil_sii8620
> >     driver only for HDMI.
> > 
> > The same reasoning applies to INPUT, if we agree that depending on
> > RC_CORE is confusing for users, then depending on INPUT is confusing as
> > well. There's not reason to apply different standards to INPUT and
> > RC_CORE, depending on one and selecting the other doesn't make much
> > sense.
> 
> OK, so revert + patch to stub out RC calls? That works for me (and I
> still say it should go through the same tree that introduced
> d6abe6df706c).

Yes, that sounds good to me.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm/bridge: sil_sii8620: depend on INPUT instead of selecting it.

[Life is hard, and then you die]

On Thu, Jan 24, 2019 at 12:22:00AM +0200, Laurent Pinchart wrote:
> 
> On Wed, Jan 23, 2019 at 02:21:05PM -0800, Dmitry Torokhov wrote:
> > On Thu, Jan 24, 2019 at 12:17:35AM +0200, Laurent Pinchart wrote:
> > > On Wed, Jan 23, 2019 at 02:03:42PM -0800, Dmitry Torokhov wrote:
> > >> On Wed, Jan 23, 2019 at 09:45:56AM +0100, Lukas Wunner wrote:
> > >>> On Tue, Jan 22, 2019 at 06:13:11AM -0800, Ronald Tschalär wrote:
> > >>>> commit d6abe6df706c66d803e6dd4fe98c1b6b7f125a56 (drm/bridge:
> > >>>> sil_sii8620: do not have a dependency of RC_CORE) added a dependency on
> > >>>> INPUT. However, this causes problems with other drivers, in particular
> > >>>> an input driver that depends on MFD_INTEL_LPSS_PCI (to be added in a
> > >>>> future commit):
> > >>>> 
> > >>>>   drivers/clk/Kconfig:9:error: recursive dependency detected!
> > >>>>   drivers/clk/Kconfig:9:        symbol COMMON_CLK is selected by MFD_INTEL_LPSS
> > >>>>   drivers/mfd/Kconfig:566:      symbol MFD_INTEL_LPSS is selected by MFD_INTEL_LPSS_PCI
> > >>>>   drivers/mfd/Kconfig:580:      symbol MFD_INTEL_LPSS_PCI is implied by KEYBOARD_APPLESPI
> > >>>>   drivers/input/keyboard/Kconfig:73:    symbol KEYBOARD_APPLESPI depends on INPUT
> > >>>>   drivers/input/Kconfig:8:      symbol INPUT is selected by DRM_SIL_SII8620
> > >>>>   drivers/gpu/drm/bridge/Kconfig:83:    symbol DRM_SIL_SII8620 depends on DRM_BRIDGE
> > >>>>   drivers/gpu/drm/bridge/Kconfig:1:     symbol DRM_BRIDGE is selected by DRM_PL111
> > >>>>   drivers/gpu/drm/pl111/Kconfig:1:      symbol DRM_PL111 depends on COMMON_CLK
> > >>>> 
> > >>>> According to the docs, select should only be used for non-visible
> > >>>> symbols. Furthermore almost all other references to INPUT throughout the
> > >>>> kernel config are depends, not selects. Hence this change.
> > >> 
> > >> I think this is not as cut and dry. We should be able to select needed
> > >> subsystems (such as INPUT, USB, etc) even if they are user visible.
> > > 
> > > Semantically, maybe, but given the current state of Kconfig this results
> > > in a recursive dependencies nightmare. It's a no-go.
> > > 
> > >> User, when enabling a piece of hardware, does not need to know ultimate
> > >> details of all subsystems the driver might need ti function.
> > >> 
> > >> It looks like one of the drivers implies MFD_INTEL_LPSS_PCI, maybe
> > >> treating imply the same as select when detecting circular dependency is
> > >> wrong as we are allowed to deselect implied dependencies?
> > >> 
> > >>>> 
> > >>>> CC: Inki Dae <inki.dae@samsung.com>
> > >>>> CC: Andrzej Hajda <a.hajda@samsung.com>
> > >>>> Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
> > >>> 
> > >>> Reviewed-by: Lukas Wunner <lukas@wunner.de>
> > >>> 
> > >>> I think this needs to be merged through the input tree as a prerequisite
> > >>> for the applespi.c driver (keyboard + touchpad driver for 2015+ MacBook,
> > >>> MacBook Air and MacBook Pro which uses SPI instead of USB) to avoid
> > >>> breaking the build.  Adding Dmitry.
> > >> 
> > >> I have no idea what applespi.c is (it is definitely not in my tree), so
> > >> I think it should be merged through the same tree that the original
> > >> commit was introduced through.

Apologies, I should've added a cover letter explain this: the applespi
driver is a new input driver I'm about to submit through the input
tree, hence why you wouldn't have it. But as Lukas says, the change
here is a prerequisite for the proposed Kconfig for that driver.

Since the two changes (the change here + the new driver) seem to be
best submitted through different trees, I'm trying to figure out how
best to handle this. I suppose I could temporarily change the driver
Kconfig to not trigger the conflict, and then once the change here has
been upstreamed (not sure at what point exactly that would be
considered the case, e.g. if in linux-next is sufficient, or has to
wait for Linus' merge, or something else) submit another change to
change the driver's Kconfig to the desired one.

> > >>>> ---
> > >>>>  drivers/gpu/drm/bridge/Kconfig | 2 +-
> > >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >>>> 
> > >>>> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > >>>> index 2fee47b0d50b..eabedc83f25c 100644
> > >>>> --- a/drivers/gpu/drm/bridge/Kconfig
> > >>>> +++ b/drivers/gpu/drm/bridge/Kconfig
> > >>>> @@ -83,9 +83,9 @@ config DRM_PARADE_PS8622
> > >>>>  config DRM_SIL_SII8620
> > >>>>  	tristate "Silicon Image SII8620 HDMI/MHL bridge"
> > >>>>  	depends on OF
> > >>>> +	depends on INPUT
> > >>>>  	select DRM_KMS_HELPER
> > >>>>  	imply EXTCON
> > >>>> -	select INPUT
> > >>>>  	select RC_CORE
> > >> 
> > >> Keeping "select RC_CORE" is wrong though, as the driver appears to be
> > >> working find without RC. Maybe it should be stubbed out?
> > > 
> > > It should definitely not be select'ed as it's a user-visible symbol. My
> > > preference would be to simply revert d6abe6df706c. If we want (and can)
> > > work without RC core then it should be stubbed out.
> > > 
> > > Commit d6abe6df706c states
> > > 
> > >     And some boards not using remote controller device don't really
> > >     need to know that RC_CORE config should be enabled to use sil_sii8620
> > >     driver only for HDMI.
> > > 
> > > The same reasoning applies to INPUT, if we agree that depending on
> > > RC_CORE is confusing for users, then depending on INPUT is confusing as
> > > well. There's not reason to apply different standards to INPUT and
> > > RC_CORE, depending on one and selecting the other doesn't make much
> > > sense.
> > 
> > OK, so revert + patch to stub out RC calls? That works for me (and I
> > still say it should go through the same tree that introduced
> > d6abe6df706c).
> 
> Yes, that sounds good to me.

Thanks for the reviews. Ok, I'll submit a new patch shortly for this.
However, other than compiling with RC_CORE and INPUT set/unset I can't
test the changes to the sil-sii8620 driver.


  Cheers,

  Ronald

[PATCH v2] drm/bridge: sil_sii8620: make remote control optional.

[Ronald Tschalär]

commit d6abe6df706c (drm/bridge: sil_sii8620: do not have a dependency
of RC_CORE) changed the driver to select both RC_CORE and INPUT.
However, this causes problems with other drivers, in particular an input
driver that depends on MFD_INTEL_LPSS_PCI (to be added in a separate
commit):

  drivers/clk/Kconfig:9:error: recursive dependency detected!
  drivers/clk/Kconfig:9:        symbol COMMON_CLK is selected by MFD_INTEL_LPSS
  drivers/mfd/Kconfig:566:      symbol MFD_INTEL_LPSS is selected by MFD_INTEL_LPSS_PCI
  drivers/mfd/Kconfig:580:      symbol MFD_INTEL_LPSS_PCI is implied by KEYBOARD_APPLESPI
  drivers/input/keyboard/Kconfig:73:    symbol KEYBOARD_APPLESPI depends on INPUT
  drivers/input/Kconfig:8:      symbol INPUT is selected by DRM_SIL_SII8620
  drivers/gpu/drm/bridge/Kconfig:83:    symbol DRM_SIL_SII8620 depends on DRM_BRIDGE
  drivers/gpu/drm/bridge/Kconfig:1:     symbol DRM_BRIDGE is selected by DRM_PL111
  drivers/gpu/drm/pl111/Kconfig:1:      symbol DRM_PL111 depends on COMMON_CLK

According to the docs and general consensus, select should only be used
for non user-visible symbols, but both RC_CORE and INPUT are
user-visible. Furthermore almost all other references to INPUT
throughout the kernel config are depends, not selects. For this reason
the first part of this change reverts commit d6abe6df706c.

In order to address the original reason for commit d6abe6df706c, namely
that not all boards use the remote controller functionality and hence
should not need have to deal with RC_CORE, the second part of this
change now makes the remote control support in the driver optional and
contingent on RC_CORE being defined. And with this the hard dependency
on INPUT also goes away as that is only needed if RC_CORE is defined
(which in turn already depends on INPUT).

CC: Inki Dae <inki.dae@samsung.com>
CC: Andrzej Hajda <a.hajda@samsung.com>
CC: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
---
Changes in v2:
 - completely remove dependencies on both RC_CORE and INPUT in Kconfig,
 - make remote control functionality in driver contingent on RC_CORE
   being defined

 drivers/gpu/drm/bridge/Kconfig       |  2 --
 drivers/gpu/drm/bridge/sil-sii8620.c | 17 +++++++++++++++++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 2fee47b0d50b..a11198a36005 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -85,8 +85,6 @@ config DRM_SIL_SII8620
 	depends on OF
 	select DRM_KMS_HELPER
 	imply EXTCON
-	select INPUT
-	select RC_CORE
 	help
 	  Silicon Image SII8620 HDMI/MHL bridge chip driver.
 
diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
index 0cc293a6ac24..dee47752791e 100644
--- a/drivers/gpu/drm/bridge/sil-sii8620.c
+++ b/drivers/gpu/drm/bridge/sil-sii8620.c
@@ -66,7 +66,9 @@ enum sii8620_mt_state {
 struct sii8620 {
 	struct drm_bridge bridge;
 	struct device *dev;
+#if IS_ENABLED(CONFIG_RC_CORE)
 	struct rc_dev *rc_dev;
+#endif
 	struct clk *clk_xtal;
 	struct gpio_desc *gpio_reset;
 	struct gpio_desc *gpio_int;
@@ -1756,6 +1758,7 @@ static void sii8620_send_features(struct sii8620 *ctx)
 	sii8620_write_buf(ctx, REG_MDT_XMIT_WRITE_PORT, buf, ARRAY_SIZE(buf));
 }
 
+#if IS_ENABLED(CONFIG_RC_CORE)
 static bool sii8620_rcp_consume(struct sii8620 *ctx, u8 scancode)
 {
 	bool pressed = !(scancode & MHL_RCP_KEY_RELEASED_MASK);
@@ -1774,6 +1777,12 @@ static bool sii8620_rcp_consume(struct sii8620 *ctx, u8 scancode)
 
 	return true;
 }
+#else
+static bool sii8620_rcp_consume(struct sii8620 *ctx, u8 scancode)
+{
+	return false;
+}
+#endif
 
 static void sii8620_msc_mr_set_int(struct sii8620 *ctx)
 {
@@ -2097,6 +2106,7 @@ static void sii8620_cable_in(struct sii8620 *ctx)
 	enable_irq(to_i2c_client(ctx->dev)->irq);
 }
 
+#if IS_ENABLED(CONFIG_RC_CORE)
 static void sii8620_init_rcp_input_dev(struct sii8620 *ctx)
 {
 	struct rc_dev *rc_dev;
@@ -2126,6 +2136,11 @@ static void sii8620_init_rcp_input_dev(struct sii8620 *ctx)
 	}
 	ctx->rc_dev = rc_dev;
 }
+#else
+static void sii8620_init_rcp_input_dev(struct sii8620 *ctx)
+{
+}
+#endif
 
 static void sii8620_cable_out(struct sii8620 *ctx)
 {
@@ -2214,9 +2229,11 @@ static int sii8620_attach(struct drm_bridge *bridge)
 
 static void sii8620_detach(struct drm_bridge *bridge)
 {
+#if IS_ENABLED(CONFIG_RC_CORE)
 	struct sii8620 *ctx = bridge_to_sii8620(bridge);
 
 	rc_unregister_device(ctx->rc_dev);
+#endif
 }
 
 static int sii8620_is_packing_required(struct sii8620 *ctx,
-- 
2.20.1

Re: [PATCH] drm/bridge: sil_sii8620: depend on INPUT instead of selecting it.

[Lukas Wunner]

On Wed, Jan 23, 2019 at 11:21:25PM -0800, Life is hard, and then you die wrote:
> Since the two changes (the change here + the new driver) seem to be
> best submitted through different trees, I'm trying to figure out how
> best to handle this. I suppose I could temporarily change the driver
> Kconfig to not trigger the conflict, and then once the change here has
> been upstreamed (not sure at what point exactly that would be
> considered the case, e.g. if in linux-next is sufficient, or has to
> wait for Linus' merge, or something else) submit another change to
> change the driver's Kconfig to the desired one.

If a series touches multiple subsystems and its patches are interdependent,
the pull requests sent to Linus would have to be merged in a specific order.

In practice that's too cumbersome, so either the series is split in multiple
parts and merged across multiple releases (which obviously can take a long
time) or, if the change to other subsystems is smallish (as is the case
here), the entire series is merged through a single subsystem tree and
those patches touching other subsystems need to have an Acked-by or
Reviewed-by tag from a maintainer of those other subsystems.

If a case can be made that the change to the other subsystem (e.g. DRM) is
actually a bug fix, then that change can go in immediately and will appear
in one of Linus' next -rc releases.  The rest of the series can then go
through the appropriate subsystem (e.g. input) and will land in Linus' tree
in the next merge window.

Either way, the correct patch order is preserved and it's guaranteed that
the build is not broken for someone ending up on an in-between commit while
bisecting.

HTH,

Lukas

[PATCH v2] drm/bridge: sil_sii8620: make remote control optional.

[Ronald Tschalär]

commit d6abe6df706c (drm/bridge: sil_sii8620: do not have a dependency
of RC_CORE) changed the driver to select both RC_CORE and INPUT.
However, this causes problems with other drivers, in particular an input
driver that depends on MFD_INTEL_LPSS_PCI (to be added in a separate
commit):

  drivers/clk/Kconfig:9:error: recursive dependency detected!
  drivers/clk/Kconfig:9:        symbol COMMON_CLK is selected by MFD_INTEL_LPSS
  drivers/mfd/Kconfig:566:      symbol MFD_INTEL_LPSS is selected by MFD_INTEL_LPSS_PCI
  drivers/mfd/Kconfig:580:      symbol MFD_INTEL_LPSS_PCI is implied by KEYBOARD_APPLESPI
  drivers/input/keyboard/Kconfig:73:    symbol KEYBOARD_APPLESPI depends on INPUT
  drivers/input/Kconfig:8:      symbol INPUT is selected by DRM_SIL_SII8620
  drivers/gpu/drm/bridge/Kconfig:83:    symbol DRM_SIL_SII8620 depends on DRM_BRIDGE
  drivers/gpu/drm/bridge/Kconfig:1:     symbol DRM_BRIDGE is selected by DRM_PL111
  drivers/gpu/drm/pl111/Kconfig:1:      symbol DRM_PL111 depends on COMMON_CLK

According to the docs and general consensus, select should only be used
for non user-visible symbols, but both RC_CORE and INPUT are
user-visible. Furthermore almost all other references to INPUT
throughout the kernel config are depends, not selects. For this reason
the first part of this change reverts commit d6abe6df706c.

In order to address the original reason for commit d6abe6df706c, namely
that not all boards use the remote controller functionality and hence
should not need have to deal with RC_CORE, the second part of this
change now makes the remote control support in the driver optional and
contingent on RC_CORE being defined. And with this the hard dependency
on INPUT also goes away as that is only needed if RC_CORE is defined
(which in turn already depends on INPUT).

CC: Inki Dae <inki.dae@samsung.com>
CC: Andrzej Hajda <a.hajda@samsung.com>
CC: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
---
Resending this, as I somehow managed to forget to cc dri-devel.
Apologies for the duplication.

Changes in v2:
 - completely remove dependencies on both RC_CORE and INPUT in Kconfig,
 - make remote control functionality in driver contingent on RC_CORE
   being defined

 drivers/gpu/drm/bridge/Kconfig       |  2 --
 drivers/gpu/drm/bridge/sil-sii8620.c | 17 +++++++++++++++++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 2fee47b0d50b..a11198a36005 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -85,8 +85,6 @@ config DRM_SIL_SII8620
 	depends on OF
 	select DRM_KMS_HELPER
 	imply EXTCON
-	select INPUT
-	select RC_CORE
 	help
 	  Silicon Image SII8620 HDMI/MHL bridge chip driver.
 
diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
index 0cc293a6ac24..dee47752791e 100644
--- a/drivers/gpu/drm/bridge/sil-sii8620.c
+++ b/drivers/gpu/drm/bridge/sil-sii8620.c
@@ -66,7 +66,9 @@ enum sii8620_mt_state {
 struct sii8620 {
 	struct drm_bridge bridge;
 	struct device *dev;
+#if IS_ENABLED(CONFIG_RC_CORE)
 	struct rc_dev *rc_dev;
+#endif
 	struct clk *clk_xtal;
 	struct gpio_desc *gpio_reset;
 	struct gpio_desc *gpio_int;
@@ -1756,6 +1758,7 @@ static void sii8620_send_features(struct sii8620 *ctx)
 	sii8620_write_buf(ctx, REG_MDT_XMIT_WRITE_PORT, buf, ARRAY_SIZE(buf));
 }
 
+#if IS_ENABLED(CONFIG_RC_CORE)
 static bool sii8620_rcp_consume(struct sii8620 *ctx, u8 scancode)
 {
 	bool pressed = !(scancode & MHL_RCP_KEY_RELEASED_MASK);
@@ -1774,6 +1777,12 @@ static bool sii8620_rcp_consume(struct sii8620 *ctx, u8 scancode)
 
 	return true;
 }
+#else
+static bool sii8620_rcp_consume(struct sii8620 *ctx, u8 scancode)
+{
+	return false;
+}
+#endif
 
 static void sii8620_msc_mr_set_int(struct sii8620 *ctx)
 {
@@ -2097,6 +2106,7 @@ static void sii8620_cable_in(struct sii8620 *ctx)
 	enable_irq(to_i2c_client(ctx->dev)->irq);
 }
 
+#if IS_ENABLED(CONFIG_RC_CORE)
 static void sii8620_init_rcp_input_dev(struct sii8620 *ctx)
 {
 	struct rc_dev *rc_dev;
@@ -2126,6 +2136,11 @@ static void sii8620_init_rcp_input_dev(struct sii8620 *ctx)
 	}
 	ctx->rc_dev = rc_dev;
 }
+#else
+static void sii8620_init_rcp_input_dev(struct sii8620 *ctx)
+{
+}
+#endif
 
 static void sii8620_cable_out(struct sii8620 *ctx)
 {
@@ -2214,9 +2229,11 @@ static int sii8620_attach(struct drm_bridge *bridge)
 
 static void sii8620_detach(struct drm_bridge *bridge)
 {
+#if IS_ENABLED(CONFIG_RC_CORE)
 	struct sii8620 *ctx = bridge_to_sii8620(bridge);
 
 	rc_unregister_device(ctx->rc_dev);
+#endif
 }
 
 static int sii8620_is_packing_required(struct sii8620 *ctx,
-- 
2.20.1

Re: [PATCH v2] drm/bridge: sil_sii8620: make remote control optional.

[Andrzej Hajda]

On 25.01.2019 02:33, Ronald Tschalär wrote:
> commit d6abe6df706c (drm/bridge: sil_sii8620: do not have a dependency
> of RC_CORE) changed the driver to select both RC_CORE and INPUT.
> However, this causes problems with other drivers, in particular an input
> driver that depends on MFD_INTEL_LPSS_PCI (to be added in a separate
> commit):
>
>   drivers/clk/Kconfig:9:error: recursive dependency detected!
>   drivers/clk/Kconfig:9:        symbol COMMON_CLK is selected by MFD_INTEL_LPSS
>   drivers/mfd/Kconfig:566:      symbol MFD_INTEL_LPSS is selected by MFD_INTEL_LPSS_PCI
>   drivers/mfd/Kconfig:580:      symbol MFD_INTEL_LPSS_PCI is implied by KEYBOARD_APPLESPI
>   drivers/input/keyboard/Kconfig:73:    symbol KEYBOARD_APPLESPI depends on INPUT
>   drivers/input/Kconfig:8:      symbol INPUT is selected by DRM_SIL_SII8620
>   drivers/gpu/drm/bridge/Kconfig:83:    symbol DRM_SIL_SII8620 depends on DRM_BRIDGE
>   drivers/gpu/drm/bridge/Kconfig:1:     symbol DRM_BRIDGE is selected by DRM_PL111
>   drivers/gpu/drm/pl111/Kconfig:1:      symbol DRM_PL111 depends on COMMON_CLK
>
> According to the docs and general consensus, select should only be used
> for non user-visible symbols, but both RC_CORE and INPUT are
> user-visible. Furthermore almost all other references to INPUT
> throughout the kernel config are depends, not selects. For this reason
> the first part of this change reverts commit d6abe6df706c.
>
> In order to address the original reason for commit d6abe6df706c, namely
> that not all boards use the remote controller functionality and hence
> should not need have to deal with RC_CORE, the second part of this
> change now makes the remote control support in the driver optional and
> contingent on RC_CORE being defined. And with this the hard dependency
> on INPUT also goes away as that is only needed if RC_CORE is defined
> (which in turn already depends on INPUT).


Thanks for fixing it, this seems to be a best way to deal with it, more
comments below.


>
> CC: Inki Dae <inki.dae@samsung.com>
> CC: Andrzej Hajda <a.hajda@samsung.com>
> CC: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
> ---
> Resending this, as I somehow managed to forget to cc dri-devel.
> Apologies for the duplication.
>
> Changes in v2:
>  - completely remove dependencies on both RC_CORE and INPUT in Kconfig,
>  - make remote control functionality in driver contingent on RC_CORE
>    being defined
>
>  drivers/gpu/drm/bridge/Kconfig       |  2 --
>  drivers/gpu/drm/bridge/sil-sii8620.c | 17 +++++++++++++++++
>  2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 2fee47b0d50b..a11198a36005 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -85,8 +85,6 @@ config DRM_SIL_SII8620
>  	depends on OF
>  	select DRM_KMS_HELPER
>  	imply EXTCON
> -	select INPUT
> -	select RC_CORE


Shouldn't you put here "imply RC_CORE"? To avoid RC_CORE as module and
sii8620 built-in.


>  	help
>  	  Silicon Image SII8620 HDMI/MHL bridge chip driver.
>  
> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
> index 0cc293a6ac24..dee47752791e 100644
> --- a/drivers/gpu/drm/bridge/sil-sii8620.c
> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
> @@ -66,7 +66,9 @@ enum sii8620_mt_state {
>  struct sii8620 {
>  	struct drm_bridge bridge;
>  	struct device *dev;
> +#if IS_ENABLED(CONFIG_RC_CORE)
>  	struct rc_dev *rc_dev;
> +#endif
>  	struct clk *clk_xtal;
>  	struct gpio_desc *gpio_reset;
>  	struct gpio_desc *gpio_int;
> @@ -1756,6 +1758,7 @@ static void sii8620_send_features(struct sii8620 *ctx)
>  	sii8620_write_buf(ctx, REG_MDT_XMIT_WRITE_PORT, buf, ARRAY_SIZE(buf));
>  }
>  
> +#if IS_ENABLED(CONFIG_RC_CORE)
>  static bool sii8620_rcp_consume(struct sii8620 *ctx, u8 scancode)
>  {
>  	bool pressed = !(scancode & MHL_RCP_KEY_RELEASED_MASK);
> @@ -1774,6 +1777,12 @@ static bool sii8620_rcp_consume(struct sii8620 *ctx, u8 scancode)
>  
>  	return true;
>  }
> +#else
> +static bool sii8620_rcp_consume(struct sii8620 *ctx, u8 scancode)
> +{
> +	return false;
> +}
> +#endif
>  
>  static void sii8620_msc_mr_set_int(struct sii8620 *ctx)
>  {
> @@ -2097,6 +2106,7 @@ static void sii8620_cable_in(struct sii8620 *ctx)
>  	enable_irq(to_i2c_client(ctx->dev)->irq);
>  }
>  
> +#if IS_ENABLED(CONFIG_RC_CORE)
>  static void sii8620_init_rcp_input_dev(struct sii8620 *ctx)
>  {
>  	struct rc_dev *rc_dev;
> @@ -2126,6 +2136,11 @@ static void sii8620_init_rcp_input_dev(struct sii8620 *ctx)
>  	}
>  	ctx->rc_dev = rc_dev;
>  }
> +#else
> +static void sii8620_init_rcp_input_dev(struct sii8620 *ctx)
> +{
> +}
> +#endif
>  
>  static void sii8620_cable_out(struct sii8620 *ctx)
>  {
> @@ -2214,9 +2229,11 @@ static int sii8620_attach(struct drm_bridge *bridge)
>  
>  static void sii8620_detach(struct drm_bridge *bridge)
>  {
> +#if IS_ENABLED(CONFIG_RC_CORE)
>  	struct sii8620 *ctx = bridge_to_sii8620(bridge);
>  
>  	rc_unregister_device(ctx->rc_dev);
> +#endif
>  }


In two cases you create stub functions, in the third you put ifdefs
inside function, some lack of consistency.

I wonder if it wouldn't be better to create stubs just for rc_core
functions, the best would be to put stubs into rc-core.h, but if the
case of 'optional rc-core' is too rare you can put stubs into the driver:

diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c
b/drivers/gpu/drm/bridge/sil-sii8620.c

index a6e8f4591e63..6ca838d30f93 100644
--- a/drivers/gpu/drm/bridge/sil-sii8620.c
+++ b/drivers/gpu/drm/bridge/sil-sii8620.c
@@ -35,6 +35,13 @@
 
 #include "sil-sii8620.h"
 
+#if !IS_ENABLED(CONFIG_RC_CORE)
+#define rc_unregister_device(dev) ({ (void)(dev); })
+#define rc_allocate_device(type) ({ NULL; })
+#define rc_keydown(...) ({ })
+#define rc_keyup(...) ({ })
+#endif
+
 #define SII8620_BURST_BUF_LEN 288
 #define VAL_RX_HDMI_CTRL2_DEFVAL VAL_RX_HDMI_CTRL2_IDLE_CNT(3)

Regards

Andrzej


>  
>  static int sii8620_is_packing_required(struct sii8620 *ctx,

Re: [PATCH v2] drm/bridge: sil_sii8620: make remote control optional.

[Life is hard, and then you die]

On Mon, Jan 28, 2019 at 11:53:38AM +0100, Andrzej Hajda wrote:
> On 25.01.2019 02:33, Ronald Tschalär wrote:
> > commit d6abe6df706c (drm/bridge: sil_sii8620: do not have a dependency
> > of RC_CORE) changed the driver to select both RC_CORE and INPUT.
> > However, this causes problems with other drivers, in particular an input
> > driver that depends on MFD_INTEL_LPSS_PCI (to be added in a separate
> > commit):
> >
> >   drivers/clk/Kconfig:9:error: recursive dependency detected!
> >   drivers/clk/Kconfig:9:        symbol COMMON_CLK is selected by MFD_INTEL_LPSS
> >   drivers/mfd/Kconfig:566:      symbol MFD_INTEL_LPSS is selected by MFD_INTEL_LPSS_PCI
> >   drivers/mfd/Kconfig:580:      symbol MFD_INTEL_LPSS_PCI is implied by KEYBOARD_APPLESPI
> >   drivers/input/keyboard/Kconfig:73:    symbol KEYBOARD_APPLESPI depends on INPUT
> >   drivers/input/Kconfig:8:      symbol INPUT is selected by DRM_SIL_SII8620
> >   drivers/gpu/drm/bridge/Kconfig:83:    symbol DRM_SIL_SII8620 depends on DRM_BRIDGE
> >   drivers/gpu/drm/bridge/Kconfig:1:     symbol DRM_BRIDGE is selected by DRM_PL111
> >   drivers/gpu/drm/pl111/Kconfig:1:      symbol DRM_PL111 depends on COMMON_CLK
> >
> > According to the docs and general consensus, select should only be used
> > for non user-visible symbols, but both RC_CORE and INPUT are
> > user-visible. Furthermore almost all other references to INPUT
> > throughout the kernel config are depends, not selects. For this reason
> > the first part of this change reverts commit d6abe6df706c.
> >
> > In order to address the original reason for commit d6abe6df706c, namely
> > that not all boards use the remote controller functionality and hence
> > should not need have to deal with RC_CORE, the second part of this
> > change now makes the remote control support in the driver optional and
> > contingent on RC_CORE being defined. And with this the hard dependency
> > on INPUT also goes away as that is only needed if RC_CORE is defined
> > (which in turn already depends on INPUT).
> 
> 
> Thanks for fixing it, this seems to be a best way to deal with it, more
> comments below.

Sorry I didn't respond to this earlier, but since I wasn't on the
to/cc list and I neglected to check the archives I completely missed
it.

> > CC: Inki Dae <inki.dae@samsung.com>
> > CC: Andrzej Hajda <a.hajda@samsung.com>
> > CC: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
> > ---
> > Resending this, as I somehow managed to forget to cc dri-devel.
> > Apologies for the duplication.
> >
> > Changes in v2:
> >  - completely remove dependencies on both RC_CORE and INPUT in Kconfig,
> >  - make remote control functionality in driver contingent on RC_CORE
> >    being defined
> >
> >  drivers/gpu/drm/bridge/Kconfig       |  2 --
> >  drivers/gpu/drm/bridge/sil-sii8620.c | 17 +++++++++++++++++
> >  2 files changed, 17 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > index 2fee47b0d50b..a11198a36005 100644
> > --- a/drivers/gpu/drm/bridge/Kconfig
> > +++ b/drivers/gpu/drm/bridge/Kconfig
> > @@ -85,8 +85,6 @@ config DRM_SIL_SII8620
> >  	depends on OF
> >  	select DRM_KMS_HELPER
> >  	imply EXTCON
> > -	select INPUT
> > -	select RC_CORE
> 
> Shouldn't you put here "imply RC_CORE"? To avoid RC_CORE as module and
> sii8620 built-in.

Good point - fixed.

> >  	help
> >  	  Silicon Image SII8620 HDMI/MHL bridge chip driver.
> >  
> > diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
> > index 0cc293a6ac24..dee47752791e 100644
> > --- a/drivers/gpu/drm/bridge/sil-sii8620.c
> > +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
> > @@ -66,7 +66,9 @@ enum sii8620_mt_state {
> >  struct sii8620 {
> >  	struct drm_bridge bridge;
> >  	struct device *dev;
> > +#if IS_ENABLED(CONFIG_RC_CORE)
> >  	struct rc_dev *rc_dev;
> > +#endif
> >  	struct clk *clk_xtal;
> >  	struct gpio_desc *gpio_reset;
> >  	struct gpio_desc *gpio_int;
> > @@ -1756,6 +1758,7 @@ static void sii8620_send_features(struct sii8620 *ctx)
> >  	sii8620_write_buf(ctx, REG_MDT_XMIT_WRITE_PORT, buf, ARRAY_SIZE(buf));
> >  }
> >  
> > +#if IS_ENABLED(CONFIG_RC_CORE)
> >  static bool sii8620_rcp_consume(struct sii8620 *ctx, u8 scancode)
> >  {
> >  	bool pressed = !(scancode & MHL_RCP_KEY_RELEASED_MASK);
> > @@ -1774,6 +1777,12 @@ static bool sii8620_rcp_consume(struct sii8620 *ctx, u8 scancode)
> >  
> >  	return true;
> >  }
> > +#else
> > +static bool sii8620_rcp_consume(struct sii8620 *ctx, u8 scancode)
> > +{
> > +	return false;
> > +}
> > +#endif
> >  
> >  static void sii8620_msc_mr_set_int(struct sii8620 *ctx)
> >  {
> > @@ -2097,6 +2106,7 @@ static void sii8620_cable_in(struct sii8620 *ctx)
> >  	enable_irq(to_i2c_client(ctx->dev)->irq);
> >  }
> >  
> > +#if IS_ENABLED(CONFIG_RC_CORE)
> >  static void sii8620_init_rcp_input_dev(struct sii8620 *ctx)
> >  {
> >  	struct rc_dev *rc_dev;
> > @@ -2126,6 +2136,11 @@ static void sii8620_init_rcp_input_dev(struct sii8620 *ctx)
> >  	}
> >  	ctx->rc_dev = rc_dev;
> >  }
> > +#else
> > +static void sii8620_init_rcp_input_dev(struct sii8620 *ctx)
> > +{
> > +}
> > +#endif
> >  
> >  static void sii8620_cable_out(struct sii8620 *ctx)
> >  {
> > @@ -2214,9 +2229,11 @@ static int sii8620_attach(struct drm_bridge *bridge)
> >  
> >  static void sii8620_detach(struct drm_bridge *bridge)
> >  {
> > +#if IS_ENABLED(CONFIG_RC_CORE)
> >  	struct sii8620 *ctx = bridge_to_sii8620(bridge);
> >  
> >  	rc_unregister_device(ctx->rc_dev);
> > +#endif
> >  }
> 
> In two cases you create stub functions, in the third you put ifdefs
> inside function, some lack of consistency.

Agreed - I'll make a stub for sii8620_detach() too.

> I wonder if it wouldn't be better to create stubs just for rc_core
> functions, the best would be to put stubs into rc-core.h, but if the
> case of 'optional rc-core' is too rare you can put stubs into the driver:
> 
> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c
> b/drivers/gpu/drm/bridge/sil-sii8620.c
> 
> index a6e8f4591e63..6ca838d30f93 100644
> --- a/drivers/gpu/drm/bridge/sil-sii8620.c
> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
> @@ -35,6 +35,13 @@
>  
>  #include "sil-sii8620.h"
>  
> +#if !IS_ENABLED(CONFIG_RC_CORE)
> +#define rc_unregister_device(dev) ({ (void)(dev); })
> +#define rc_allocate_device(type) ({ NULL; })
> +#define rc_keydown(...) ({ })
> +#define rc_keyup(...) ({ })
> +#endif
> +
>  #define SII8620_BURST_BUF_LEN 288
>  #define VAL_RX_HDMI_CTRL2_DEFVAL VAL_RX_HDMI_CTRL2_IDLE_CNT(3)

The reason I didn't do this was because it leads to unwanted log
messages: in particular in sii8620_init_rcp_input_dev() the failed
allocation results in an ugly dev_err() message; in the case of
sii8620_rcp_consume() it's a much more benign dev_dbg() though.

But if stubs for rc-core is the preferred approach, I'll certainly
do that instead.

As to how rare 'optional rc-core' is, the only other place I've found
is in drivers/media/usb/dvb-usb-v2/, and there the approach is to #if
out the whole rc related code and stub out the single entry point to
it (get_rc_config). So for now it looks like use for rc-core stubs
would be restricted to this module (if we go that route).


  Cheers,

  Ronald

Re: [PATCH v2] drm/bridge: sil_sii8620: make remote control optional.

[Life is hard, and then you die]

On Thu, Jan 24, 2019 at 05:33:55PM -0800, Ronald Tschalär wrote:
> commit d6abe6df706c (drm/bridge: sil_sii8620: do not have a dependency
> of RC_CORE) changed the driver to select both RC_CORE and INPUT.
> However, this causes problems with other drivers, in particular an input
> driver that depends on MFD_INTEL_LPSS_PCI (to be added in a separate
> commit):
> 
>   drivers/clk/Kconfig:9:error: recursive dependency detected!
>   drivers/clk/Kconfig:9:        symbol COMMON_CLK is selected by MFD_INTEL_LPSS
>   drivers/mfd/Kconfig:566:      symbol MFD_INTEL_LPSS is selected by MFD_INTEL_LPSS_PCI
>   drivers/mfd/Kconfig:580:      symbol MFD_INTEL_LPSS_PCI is implied by KEYBOARD_APPLESPI
>   drivers/input/keyboard/Kconfig:73:    symbol KEYBOARD_APPLESPI depends on INPUT
>   drivers/input/Kconfig:8:      symbol INPUT is selected by DRM_SIL_SII8620
>   drivers/gpu/drm/bridge/Kconfig:83:    symbol DRM_SIL_SII8620 depends on DRM_BRIDGE
>   drivers/gpu/drm/bridge/Kconfig:1:     symbol DRM_BRIDGE is selected by DRM_PL111
>   drivers/gpu/drm/pl111/Kconfig:1:      symbol DRM_PL111 depends on COMMON_CLK
> 
> According to the docs and general consensus, select should only be used
> for non user-visible symbols, but both RC_CORE and INPUT are
> user-visible. Furthermore almost all other references to INPUT
> throughout the kernel config are depends, not selects. For this reason
> the first part of this change reverts commit d6abe6df706c.
> 
> In order to address the original reason for commit d6abe6df706c, namely
> that not all boards use the remote controller functionality and hence
> should not need have to deal with RC_CORE, the second part of this
> change now makes the remote control support in the driver optional and
> contingent on RC_CORE being defined. And with this the hard dependency
> on INPUT also goes away as that is only needed if RC_CORE is defined
> (which in turn already depends on INPUT).
> 
> CC: Inki Dae <inki.dae@samsung.com>
> CC: Andrzej Hajda <a.hajda@samsung.com>
> CC: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
> ---
> Resending this, as I somehow managed to forget to cc dri-devel.
> Apologies for the duplication.
> 
> Changes in v2:
>  - completely remove dependencies on both RC_CORE and INPUT in Kconfig,
>  - make remote control functionality in driver contingent on RC_CORE
>    being defined
> 
>  drivers/gpu/drm/bridge/Kconfig       |  2 --
>  drivers/gpu/drm/bridge/sil-sii8620.c | 17 +++++++++++++++++
>  2 files changed, 17 insertions(+), 2 deletions(-)
[snip]

Is there anything I can do to help get this reviewed and moved forward?


  Cheers,

  Ronald

Re: [PATCH v2] drm/bridge: sil_sii8620: make remote control optional.

[Andrzej Hajda]

On 04.03.2019 03:13, Life is hard, and then you die wrote:
> On Thu, Jan 24, 2019 at 05:33:55PM -0800, Ronald Tschalär wrote:
>> commit d6abe6df706c (drm/bridge: sil_sii8620: do not have a dependency
>> of RC_CORE) changed the driver to select both RC_CORE and INPUT.
>> However, this causes problems with other drivers, in particular an input
>> driver that depends on MFD_INTEL_LPSS_PCI (to be added in a separate
>> commit):
>>
>>   drivers/clk/Kconfig:9:error: recursive dependency detected!
>>   drivers/clk/Kconfig:9:        symbol COMMON_CLK is selected by MFD_INTEL_LPSS
>>   drivers/mfd/Kconfig:566:      symbol MFD_INTEL_LPSS is selected by MFD_INTEL_LPSS_PCI
>>   drivers/mfd/Kconfig:580:      symbol MFD_INTEL_LPSS_PCI is implied by KEYBOARD_APPLESPI
>>   drivers/input/keyboard/Kconfig:73:    symbol KEYBOARD_APPLESPI depends on INPUT
>>   drivers/input/Kconfig:8:      symbol INPUT is selected by DRM_SIL_SII8620
>>   drivers/gpu/drm/bridge/Kconfig:83:    symbol DRM_SIL_SII8620 depends on DRM_BRIDGE
>>   drivers/gpu/drm/bridge/Kconfig:1:     symbol DRM_BRIDGE is selected by DRM_PL111
>>   drivers/gpu/drm/pl111/Kconfig:1:      symbol DRM_PL111 depends on COMMON_CLK
>>
>> According to the docs and general consensus, select should only be used
>> for non user-visible symbols, but both RC_CORE and INPUT are
>> user-visible. Furthermore almost all other references to INPUT
>> throughout the kernel config are depends, not selects. For this reason
>> the first part of this change reverts commit d6abe6df706c.
>>
>> In order to address the original reason for commit d6abe6df706c, namely
>> that not all boards use the remote controller functionality and hence
>> should not need have to deal with RC_CORE, the second part of this
>> change now makes the remote control support in the driver optional and
>> contingent on RC_CORE being defined. And with this the hard dependency
>> on INPUT also goes away as that is only needed if RC_CORE is defined
>> (which in turn already depends on INPUT).
>>
>> CC: Inki Dae <inki.dae@samsung.com>
>> CC: Andrzej Hajda <a.hajda@samsung.com>
>> CC: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
>> ---
>> Resending this, as I somehow managed to forget to cc dri-devel.
>> Apologies for the duplication.
>>
>> Changes in v2:
>>  - completely remove dependencies on both RC_CORE and INPUT in Kconfig,
>>  - make remote control functionality in driver contingent on RC_CORE
>>    being defined
>>
>>  drivers/gpu/drm/bridge/Kconfig       |  2 --
>>  drivers/gpu/drm/bridge/sil-sii8620.c | 17 +++++++++++++++++
>>  2 files changed, 17 insertions(+), 2 deletions(-)
> [snip]
>
> Is there anything I can do to help get this reviewed and moved forward?


Addressing my comments [1] ? :)

Ah I see, for some reasons (my mistake apparently) my response was not
sent to you, sorry.

[1]: https://lkml.org/lkml/2019/1/28/258


Regards

Andrzej


>
>
>   Cheers,
>
>   Ronald
>
>
>

[PATCH v3] drm/bridge: sil_sii8620: make remote control optional.

[Ronald Tschalär]

commit d6abe6df706c (drm/bridge: sil_sii8620: do not have a dependency
of RC_CORE) changed the driver to select both RC_CORE and INPUT.
However, this causes problems with other drivers, in particular an input
driver that depends on MFD_INTEL_LPSS_PCI (to be added in a separate
commit):

  drivers/clk/Kconfig:9:error: recursive dependency detected!
  drivers/clk/Kconfig:9:        symbol COMMON_CLK is selected by MFD_INTEL_LPSS
  drivers/mfd/Kconfig:566:      symbol MFD_INTEL_LPSS is selected by MFD_INTEL_LPSS_PCI
  drivers/mfd/Kconfig:580:      symbol MFD_INTEL_LPSS_PCI is implied by KEYBOARD_APPLESPI
  drivers/input/keyboard/Kconfig:73:    symbol KEYBOARD_APPLESPI depends on INPUT
  drivers/input/Kconfig:8:      symbol INPUT is selected by DRM_SIL_SII8620
  drivers/gpu/drm/bridge/Kconfig:83:    symbol DRM_SIL_SII8620 depends on DRM_BRIDGE
  drivers/gpu/drm/bridge/Kconfig:1:     symbol DRM_BRIDGE is selected by DRM_PL111
  drivers/gpu/drm/pl111/Kconfig:1:      symbol DRM_PL111 depends on COMMON_CLK

According to the docs and general consensus, select should only be used
for non user-visible symbols, but both RC_CORE and INPUT are
user-visible. Furthermore almost all other references to INPUT
throughout the kernel config are depends, not selects. For this reason
the first part of this change reverts commit d6abe6df706c.

In order to address the original reason for commit d6abe6df706c, namely
that not all boards use the remote controller functionality and hence
should not need have to deal with RC_CORE, the second part of this
change now makes the remote control support in the driver optional and
contingent on RC_CORE being defined. And with this the hard dependency
on INPUT also goes away as that is only needed if RC_CORE is defined
(which in turn already depends on INPUT).

CC: Inki Dae <inki.dae@samsung.com>
CC: Andrzej Hajda <a.hajda@samsung.com>
CC: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
---
Changes in v3, from review by Andrzej Hajda:
 - imply RC_CORE in Kconfig to avoid the possibility of sii8620 being
   built-in but rc-core being a module
 - completely stub out sii8620_detach() to make it consistent with the
   approach taken for the other rc-related functions

 drivers/gpu/drm/bridge/Kconfig       |  3 +--
 drivers/gpu/drm/bridge/sil-sii8620.c | 21 +++++++++++++++++++++
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 8840f396a7b6..298189067929 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -86,8 +86,7 @@ config DRM_SIL_SII8620
 	depends on OF
 	select DRM_KMS_HELPER
 	imply EXTCON
-	select INPUT
-	select RC_CORE
+	imply RC_CORE
 	help
 	  Silicon Image SII8620 HDMI/MHL bridge chip driver.
 
diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
index 0cc293a6ac24..9cac00579414 100644
--- a/drivers/gpu/drm/bridge/sil-sii8620.c
+++ b/drivers/gpu/drm/bridge/sil-sii8620.c
@@ -66,7 +66,9 @@ enum sii8620_mt_state {
 struct sii8620 {
 	struct drm_bridge bridge;
 	struct device *dev;
+#if IS_ENABLED(CONFIG_RC_CORE)
 	struct rc_dev *rc_dev;
+#endif
 	struct clk *clk_xtal;
 	struct gpio_desc *gpio_reset;
 	struct gpio_desc *gpio_int;
@@ -1756,6 +1758,7 @@ static void sii8620_send_features(struct sii8620 *ctx)
 	sii8620_write_buf(ctx, REG_MDT_XMIT_WRITE_PORT, buf, ARRAY_SIZE(buf));
 }
 
+#if IS_ENABLED(CONFIG_RC_CORE)
 static bool sii8620_rcp_consume(struct sii8620 *ctx, u8 scancode)
 {
 	bool pressed = !(scancode & MHL_RCP_KEY_RELEASED_MASK);
@@ -1774,6 +1777,12 @@ static bool sii8620_rcp_consume(struct sii8620 *ctx, u8 scancode)
 
 	return true;
 }
+#else
+static bool sii8620_rcp_consume(struct sii8620 *ctx, u8 scancode)
+{
+	return false;
+}
+#endif
 
 static void sii8620_msc_mr_set_int(struct sii8620 *ctx)
 {
@@ -2097,6 +2106,7 @@ static void sii8620_cable_in(struct sii8620 *ctx)
 	enable_irq(to_i2c_client(ctx->dev)->irq);
 }
 
+#if IS_ENABLED(CONFIG_RC_CORE)
 static void sii8620_init_rcp_input_dev(struct sii8620 *ctx)
 {
 	struct rc_dev *rc_dev;
@@ -2126,6 +2136,11 @@ static void sii8620_init_rcp_input_dev(struct sii8620 *ctx)
 	}
 	ctx->rc_dev = rc_dev;
 }
+#else
+static void sii8620_init_rcp_input_dev(struct sii8620 *ctx)
+{
+}
+#endif
 
 static void sii8620_cable_out(struct sii8620 *ctx)
 {
@@ -2212,12 +2227,18 @@ static int sii8620_attach(struct drm_bridge *bridge)
 	return sii8620_clear_error(ctx);
 }
 
+#if IS_ENABLED(CONFIG_RC_CORE)
 static void sii8620_detach(struct drm_bridge *bridge)
 {
 	struct sii8620 *ctx = bridge_to_sii8620(bridge);
 
 	rc_unregister_device(ctx->rc_dev);
 }
+#else
+static void sii8620_detach(struct drm_bridge *bridge)
+{
+}
+#endif
 
 static int sii8620_is_packing_required(struct sii8620 *ctx,
 				       const struct drm_display_mode *mode)
-- 
2.20.1