From: Bruno Prémont on
On Wed, 07 April 2010 Randy Dunlap <randy.dunlap(a)oracle.com> wrote:
> On Wed, 7 Apr 2010 20:44:04 +0200 Bruno Prémont wrote:
>
> > On Tue, 06 April 2010 Randy Dunlap <randy.dunlap(a)oracle.com> wrote:
> > > >
> > > > One of my attempts did end up with a circular loop with regard to FB
> > > > (some of the FB drivers did select INPUT)?
> > >
> > > (not that I can find)
> > >
> > > CONFIG_VT does select INPUT
> > > and CONFIG_DRM_I915 does
> > > select INPUT if ACPI
> >
> > A newer attempt still produces the same result:
> >
> > drivers/input/Kconfig:9:error: found recursive dependency: INPUT ->
> > HID_SUPPORT -> HID_PICOLCD_FB -> FB -> FB_STI -> VT -> INPUT
> >
> > (it's only FB which causes the loop, LEDS, LCD and BACKLIGHT are fine)
>
> (yes, so I see)
>
> > This is with following patch on top of the improved deps patch I sent
> > a few minutes ago deeper in this thread.
> >
> > Is there a way around this?
> >
>
> Well, lesson #1 is that select is evil^W^W should only be used to enable
> library-like code, or as Documentation/kbuild/kconfig-language.txt says:
>
> Note:
> select should be used with care. select will force
> a symbol to a value without visiting the dependencies.
> By abusing select you are able to select a symbol FOO even
> if FOO depends on BAR that is not set.
> In general use select only for non-visible symbols
> (no prompts anywhere) and for symbols with no dependencies.
> That will limit the usefulness but on the other hand avoid
> the illegal configurations all over.
> kconfig should one day warn about such things.

I've read that paragraph and I would definitely prefer not to have to
use select for what this patch should achieve!

It should really be a helper for the user going through menuconfig like
"please check all what is needed to satisfy this item's dependencies"
A "try-select-deep" or whatever it could be called.

A well-behaved "GUI" implementation of this would show what would get newly
checked and give the user the opportunity to not proceed or fine-tune
y/m choices.

> (more below)
>
> >
> > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > index 782a34e..711c091 100644
> > --- a/drivers/hid/Kconfig
> > +++ b/drivers/hid/Kconfig
> > @@ -285,7 +285,7 @@ config HID_PICOLCD_FB
> > bool "Framebuffer support"
> > default !EMBEDDED
> > depends on HID_PICOLCD
> > - depends on HID_PICOLCD=FB || FB=y
> > + select FB
>
> If you'll go back to the unpatched (by this patch) version here,
> it seems to work OK.

So best is probably to just forget about all of this non-leaf select
usage for now. (and not have one half doing one way and the other half
doing it the other way)

Thanks,
Bruno

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jiri Kosina on
On Wed, 7 Apr 2010, Bruno Prémont wrote:

> HID_PICOLCD should depend on LCD_CLASS_DEVICE, otherwise the
> build fails when HID_PICOLCD=y and LCD_CLASS_DEVICE=m:
>
> hid-picolcd.c:(.text+0x84523f): undefined reference to `lcd_device_unregister'
> hid-picolcd.c:(.text+0x8478ab): undefined reference to `lcd_device_register'
> hid-picolcd.c:(.text+0x84c15f): undefined reference to `lcd_device_unregister'
>
> Same applies to FB, BACKLIGHT_CLASS_DEVICE and LEDS_CLASS.
>
> Add suboptions for those features to handle the deps on kbuild side
> and just check HID_PICOLCD_* in the code.
>
> Reported-by: Randy Dunlap <randy.dunlap(a)oracle.com>
> Signed-off-by: Bruno Prémont <bonbons(a)linux-vserver.org>
> ---
>
> On Wed, 07 April 2010 Randy Dunlap <randy.dunlap(a)oracle.com> wrote:
> > All of these user-visible kconfig options need help text also...
> >
>
> Here is a better patch, with added documentation and stripped
> select clauses under HID_PICOLCD as they are handled by HID_PICOLCD_FB.
>
>
>
>
> drivers/hid/Kconfig | 53 +++++++++++++++++++++++++++++++++++++-------
> drivers/hid/hid-picolcd.c | 40 +++++++++++++++++-----------------
> 2 files changed, 64 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index a2ecd83..782a34e 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -265,11 +265,6 @@ config HID_PETALYNX
> config HID_PICOLCD
> tristate "PicoLCD (graphic version)"
> depends on USB_HID
> - select FB_DEFERRED_IO if FB
> - select FB_SYS_FILLRECT if FB
> - select FB_SYS_COPYAREA if FB
> - select FB_SYS_IMAGEBLIT if FB
> - select FB_SYS_FOPS if FB
> ---help---
> This provides support for Minibox PicoLCD devices, currently
> only the graphical ones are supported.
> @@ -277,14 +272,54 @@ config HID_PICOLCD
> This includes support for the following device features:
> - Keypad
> - Switching between Firmware and Flash mode
> - - Framebuffer for monochrome 256x64 display
> - - Backlight control (needs CONFIG_BACKLIGHT_CLASS_DEVICE)
> - - Contrast control (needs CONFIG_LCD_CLASS_DEVICE)
> - - General purpose outputs (needs CONFIG_LEDS_CLASS)
> - EEProm / Flash access (via debugfs)
> + Features to selectively enable:
> + - Framebuffer for monochrome 256x64 display
> + - Backlight control
> + - Contrast control
> + - General purpose outputs
> Features that are not (yet) supported:
> - IR
>
> +config HID_PICOLCD_FB
> + bool "Framebuffer support"
> + default !EMBEDDED
> + depends on HID_PICOLCD
> + depends on HID_PICOLCD=FB || FB=y
> + select FB_DEFERRED_IO
> + select FB_SYS_FILLRECT
> + select FB_SYS_COPYAREA
> + select FB_SYS_IMAGEBLIT
> + select FB_SYS_FOPS

Could we perhaps also make the sub-choices for individual features
availabel only if !EMBEDDED as well?

It's probably too much to ask for a single device during oldconfig run,
for example ...

Thanks,

--
Jiri Kosina
SUSE Labs, Novell Inc.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Bruno Prémont on
HID_PICOLCD should depend on LCD_CLASS_DEVICE, otherwise the
build fails when HID_PICOLCD=y and LCD_CLASS_DEVICE=m:

hid-picolcd.c:(.text+0x84523f): undefined reference to `lcd_device_unregister'
hid-picolcd.c:(.text+0x8478ab): undefined reference to `lcd_device_register'
hid-picolcd.c:(.text+0x84c15f): undefined reference to `lcd_device_unregister'

Same applies to FB, BACKLIGHT_CLASS_DEVICE and LEDS_CLASS.

Add suboptions for those features to handle the deps on kbuild side
and just check HID_PICOLCD_* in the code.

Reported-by: Randy Dunlap <randy.dunlap(a)oracle.com>
Signed-off-by: Bruno Prémont <bonbons(a)linux-vserver.org>
---

On Thu, 08 April 2010 Jiri Kosina <jkosina(a)suse.cz> wrote:
> > +config HID_PICOLCD_FB
> > + bool "Framebuffer support"
> > + default !EMBEDDED
> > + depends on HID_PICOLCD
> > + depends on HID_PICOLCD=FB || FB=y
> > + select FB_DEFERRED_IO
> > + select FB_SYS_FILLRECT
> > + select FB_SYS_COPYAREA
> > + select FB_SYS_IMAGEBLIT
> > + select FB_SYS_FOPS
>
> Could we perhaps also make the sub-choices for individual features
> availabel only if !EMBEDDED as well?

Here is an updated patch to make the sub-choices visible only in
EMBEDDED case.

Thanks,
Bruno



drivers/hid/Kconfig | 53 +++++++++++++++++++++++++++++++++++++-------
drivers/hid/hid-picolcd.c | 40 +++++++++++++++++-----------------
2 files changed, 64 insertions(+), 29 deletions(-)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index a2ecd83..621c52c 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -265,11 +265,6 @@ config HID_PETALYNX
config HID_PICOLCD
tristate "PicoLCD (graphic version)"
depends on USB_HID
- select FB_DEFERRED_IO if FB
- select FB_SYS_FILLRECT if FB
- select FB_SYS_COPYAREA if FB
- select FB_SYS_IMAGEBLIT if FB
- select FB_SYS_FOPS if FB
---help---
This provides support for Minibox PicoLCD devices, currently
only the graphical ones are supported.
@@ -277,14 +272,54 @@ config HID_PICOLCD
This includes support for the following device features:
- Keypad
- Switching between Firmware and Flash mode
- - Framebuffer for monochrome 256x64 display
- - Backlight control (needs CONFIG_BACKLIGHT_CLASS_DEVICE)
- - Contrast control (needs CONFIG_LCD_CLASS_DEVICE)
- - General purpose outputs (needs CONFIG_LEDS_CLASS)
- EEProm / Flash access (via debugfs)
+ Features selectively enabled:
+ - Framebuffer for monochrome 256x64 display
+ - Backlight control
+ - Contrast control
+ - General purpose outputs
Features that are not (yet) supported:
- IR

+config HID_PICOLCD_FB
+ bool "Framebuffer support" if EMBEDDED
+ default !EMBEDDED
+ depends on HID_PICOLCD
+ depends on HID_PICOLCD=FB || FB=y
+ select FB_DEFERRED_IO
+ select FB_SYS_FILLRECT
+ select FB_SYS_COPYAREA
+ select FB_SYS_IMAGEBLIT
+ select FB_SYS_FOPS
+ ---help---
+ Provide access to PicoLCD's 256x64 monochrome display via a
+ frambuffer device.
+
+config HID_PICOLCD_BACKLIGHT
+ bool "Backlight control" if EMBEDDED
+ default !EMBEDDED
+ depends on HID_PICOLCD
+ depends on HID_PICOLCD=BACKLIGHT_CLASS_DEVICE || BACKLIGHT_CLASS_DEVICE=y
+ ---help---
+ Provide access to PicoLCD's backlight control via backlight
+ class.
+
+config HID_PICOLCD_LCD
+ bool "Contrast control" if EMBEDDED
+ default !EMBEDDED
+ depends on HID_PICOLCD
+ depends on HID_PICOLCD=LCD_CLASS_DEVICE || LCD_CLASS_DEVICE=y
+ ---help---
+ Provide access to PicoLCD's LCD contrast via lcd class.
+
+config HID_PICOLCD_LEDS
+ bool "GPO via leds class" if EMBEDDED
+ default !EMBEDDED
+ depends on HID_PICOLCD
+ depends on HID_PICOLCD=LEDS_CLASS || LEDS_CLASS=y
+ ---help---
+ Provide access to PicoLCD's GPO pins via leds class.
+
config HID_QUANTA
tristate "Quanta Optical Touch"
depends on USB_HID
diff --git a/drivers/hid/hid-picolcd.c b/drivers/hid/hid-picolcd.c
index 0eacc6b..0fbc7d3 100644
--- a/drivers/hid/hid-picolcd.c
+++ b/drivers/hid/hid-picolcd.c
@@ -77,7 +77,7 @@
#define REPORT_HOOK_VERSION 0xf7 /* LCD: IN[2], OUT[1] */
#define REPORT_EXIT_FLASHER 0xff /* Bootloader: OUT[2] */

-#if defined(CONFIG_FB) || defined(CONFIG_FB_MODULE)
+#ifdef CONFIG_HID_PICOLCD_FB
/* Framebuffer
*
* The PicoLCD use a Topway LCD module of 256x64 pixel
@@ -128,7 +128,7 @@ static const struct fb_var_screeninfo picolcdfb_var = {
.bits_per_pixel = 1,
.grayscale = 1,
};
-#endif /* CONFIG_FB */
+#endif /* CONFIG_HID_PICOLCD_FB */

/* Input device
*
@@ -183,7 +183,7 @@ struct picolcd_data {
struct input_dev *input_cir;
unsigned short keycode[PICOLCD_KEYS];

-#if defined(CONFIG_FB) || defined(CONFIG_FB_MODULE)
+#ifdef CONFIG_HID_PICOLCD_FB
/* Framebuffer stuff */
u8 fb_update_rate;
u8 fb_bpp;
@@ -191,21 +191,21 @@ struct picolcd_data {
u8 *fb_bitmap; /* framebuffer */
struct fb_info *fb_info;
struct fb_deferred_io fb_defio;
-#endif /* CONFIG_FB */
-#if defined(CONFIG_LCD_CLASS_DEVICE) || defined(CONFIG_LCD_CLASS_DEVICE_MODULE)
+#endif /* CONFIG_HID_PICOLCD_FB */
+#ifdef CONFIG_HID_PICOLCD_LCD
struct lcd_device *lcd;
u8 lcd_contrast;
-#endif
-#if defined(CONFIG_BACKLIGHT_CLASS_DEVICE) || defined(CONFIG_BACKLIGHT_CLASS_DEVICE_MODULE)
+#endif /* CONFIG_HID_PICOLCD_LCD */
+#ifdef CONFIG_HID_PICOLCD_BACKLIGHT
struct backlight_device *backlight;
u8 lcd_brightness;
u8 lcd_power;
-#endif /* CONFIG_BACKLIGHT_CLASS_DEVICE */
-#if defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE)
+#endif /* CONFIG_HID_PICOLCD_BACKLIGHT */
+#ifdef CONFIG_HID_PICOLCD_LEDS
/* LED stuff */
u8 led_state;
struct led_classdev *led[8];
-#endif /* CONFIG_LEDS_CLASS */
+#endif /* CONFIG_HID_PICOLCD_LEDS */

/* Housekeeping stuff */
spinlock_t lock;
@@ -287,7 +287,7 @@ static struct picolcd_pending *picolcd_send_and_wait(struct hid_device *hdev,
return work;
}

-#if defined(CONFIG_FB) || defined(CONFIG_FB_MODULE)
+#ifdef CONFIG_HID_PICOLCD_FB
/* Send a given tile to PicoLCD */
static int picolcd_fb_send_tile(struct hid_device *hdev, int chip, int tile)
{
@@ -766,9 +766,9 @@ static void picolcd_exit_framebuffer(struct picolcd_data *data)
{
}
#define picolcd_fbinfo(d) NULL
-#endif /* CONFIG_FB */
+#endif /* CONFIG_HID_PICOLCD_FB */

-#if defined(CONFIG_BACKLIGHT_CLASS_DEVICE) || defined(CONFIG_BACKLIGHT_CLASS_DEVICE_MODULE)
+#ifdef CONFIG_HID_PICOLCD_BACKLIGHT
/*
* backlight class device
*/
@@ -864,9 +864,9 @@ static inline int picolcd_resume_backlight(struct picolcd_data *data)
{
return 0;
}
-#endif /* CONFIG_BACKLIGHT_CLASS_DEVICE */
+#endif /* CONFIG_HID_PICOLCD_BACKLIGHT */

-#if defined(CONFIG_LCD_CLASS_DEVICE) || defined(CONFIG_LCD_CLASS_DEVICE_MODULE)
+#ifdef CONFIG_HID_PICOLCD_LCD
/*
* lcd class device
*/
@@ -957,9 +957,9 @@ static inline int picolcd_resume_lcd(struct picolcd_data *data)
{
return 0;
}
-#endif /* CONFIG_LCD_CLASS_DEVICE */
+#endif /* CONFIG_HID_PICOLCD_LCD */

-#if defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE)
+#ifdef CONFIG_HID_PICOLCD_LEDS
/**
* LED class device
*/
@@ -1104,7 +1104,7 @@ static inline int picolcd_leds_set(struct picolcd_data *data)
{
return 0;
}
-#endif /* CONFIG_LEDS_CLASS */
+#endif /* CONFIG_HID_PICOLCD_LEDS */

/*
* input class device
@@ -1243,10 +1243,10 @@ static int picolcd_reset(struct hid_device *hdev)

picolcd_resume_lcd(data);
picolcd_resume_backlight(data);
-#if defined(CONFIG_FB) || defined(CONFIG_FB_MODULE)
+#ifdef CONFIG_HID_PICOLCD_FB
if (data->fb_info)
schedule_delayed_work(&data->fb_info->deferred_work, 0);
-#endif /* CONFIG_FB */
+#endif /* CONFIG_HID_PICOLCD_FB */

picolcd_leds_set(data);
return 0;
--
1.6.4.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jiri Kosina on
On Sun, 11 Apr 2010, Bruno Prémont wrote:

> > > +config HID_PICOLCD_FB
> > > + bool "Framebuffer support"
> > > + default !EMBEDDED
> > > + depends on HID_PICOLCD
> > > + depends on HID_PICOLCD=FB || FB=y
> > > + select FB_DEFERRED_IO
> > > + select FB_SYS_FILLRECT
> > > + select FB_SYS_COPYAREA
> > > + select FB_SYS_IMAGEBLIT
> > > + select FB_SYS_FOPS
> >
> > Could we perhaps also make the sub-choices for individual features
> > availabel only if !EMBEDDED as well?
>
> Here is an updated patch to make the sub-choices visible only in
> EMBEDDED case.

Applied, thanks Bruno.

--
Jiri Kosina
SUSE Labs, Novell Inc.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/