| Message ID | 1394795353-1931-1-git-send-email-shc_work@mail.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On 14/03/14 13:09, Alexander Shiyan wrote: > Provide dummy functions for LCD register()/unregister() if > CONFIG_LCD_CLASS_DEVICE is not set. > This allows us to use the LCD class as an optional. > > Signed-off-by: Alexander Shiyan <shc_work@mail.ru> > --- > include/linux/lcd.h | 37 ++++++++++++++++++++++++++++++------- > 1 file changed, 30 insertions(+), 7 deletions(-) I'm still not convinced about this. Isn't it simpler to just select/depend on LCD_CLASS_DEVICE? The lcd.c is a bit less than 9 kB, so it's not like you're adding a huge amount of code into the kernel even if the driver doesn't happen to use it for a particular display. Tomi
Hello. Wed, 19 Mar 2014 15:31:49 +0200 ?? Tomi Valkeinen <tomi.valkeinen@ti.com>: > Hi, > > On 14/03/14 13:09, Alexander Shiyan wrote: > > Provide dummy functions for LCD register()/unregister() if > > CONFIG_LCD_CLASS_DEVICE is not set. > > This allows us to use the LCD class as an optional. > > > > Signed-off-by: Alexander Shiyan <shc_work@mail.ru> > > --- > > include/linux/lcd.h | 37 ++++++++++++++++++++++++++++++------- > > 1 file changed, 30 insertions(+), 7 deletions(-) > > I'm still not convinced about this. Isn't it simpler to just > select/depend on LCD_CLASS_DEVICE? The lcd.c is a bit less than 9 kB, so > it's not like you're adding a huge amount of code into the kernel even > if the driver doesn't happen to use it for a particular display. Ie we prefer to increase the size of the kernel, even for functions which are optional? OK, I'll post a patch for the imxfb driver which selects LCD_CLASS unconditionally. Nevertheless, the current patch is still valid for review. Thanks. ---
On 20/03/14 08:41, Alexander Shiyan wrote: > Hello. > > Wed, 19 Mar 2014 15:31:49 +0200 ?? Tomi Valkeinen <tomi.valkeinen@ti.com>: >> Hi, >> >> On 14/03/14 13:09, Alexander Shiyan wrote: >>> Provide dummy functions for LCD register()/unregister() if >>> CONFIG_LCD_CLASS_DEVICE is not set. >>> This allows us to use the LCD class as an optional. >>> >>> Signed-off-by: Alexander Shiyan <shc_work@mail.ru> >>> --- >>> include/linux/lcd.h | 37 ++++++++++++++++++++++++++++++------- >>> 1 file changed, 30 insertions(+), 7 deletions(-) >> >> I'm still not convinced about this. Isn't it simpler to just >> select/depend on LCD_CLASS_DEVICE? The lcd.c is a bit less than 9 kB, so >> it's not like you're adding a huge amount of code into the kernel even >> if the driver doesn't happen to use it for a particular display. > > Ie we prefer to increase the size of the kernel, even for functions which are optional? How often can LCD_CLASS_DEVICE be turned off when using imxfb? How often will it be turned off in practice? I fear this is a micro optimization for cases that never happen in real life. I'm fine with micro optimizing the kernel when it comes for free. In this case I see drawback: usually (correct me if I'm wrong) if a fb driver uses LCD_CLASS_DEVICE funcs, it requires it. Currently it's easy to catch missing LCD_CLASS_DEVICE as the kernel won't compile. This patch hides it until you try to run the driver. And compile time checking is much, much better than runtime checking. We have components that use the method you use in this patch, like gpio.h. However, the difference with gpio and this one is that the selection of gpio comes from the platform code, while the LCD_CLASS_DEVICE is selected by the fb driver in question. So the fb driver can easily select LCD_CLASS_DEVICE, while it cannot select GPIO. My point here is that for gpios we more or less need such functionality, whereas for LCD_CLASS_DEVICE we don't. I'm not strongly against this patch. But, as I said, I see a small drawback with it, so I'd like to either hear "yes, this is good" from other people, or see information showing that this optimization will actually help in lots of cases in real life. Tomi
diff --git a/include/linux/lcd.h b/include/linux/lcd.h index 504f624..39c41c7 100644 --- a/include/linux/lcd.h +++ b/include/linux/lcd.h @@ -110,14 +110,37 @@ static inline void lcd_set_power(struct lcd_device *ld, int power) mutex_unlock(&ld->update_lock); } -extern struct lcd_device *lcd_device_register(const char *name, - struct device *parent, void *devdata, struct lcd_ops *ops); -extern struct lcd_device *devm_lcd_device_register(struct device *dev, - const char *name, struct device *parent, +#if defined(CONFIG_LCD_CLASS_DEVICE) || defined(CONFIG_LCD_CLASS_DEVICE_MODULE) +struct lcd_device *lcd_device_register(const char *name, struct device *parent, void *devdata, struct lcd_ops *ops); -extern void lcd_device_unregister(struct lcd_device *ld); -extern void devm_lcd_device_unregister(struct device *dev, - struct lcd_device *ld); +struct lcd_device *devm_lcd_device_register(struct device *dev, + const char *name, struct device *parent, void *devdata, + struct lcd_ops *ops); +void lcd_device_unregister(struct lcd_device *ld); +void devm_lcd_device_unregister(struct device *dev, struct lcd_device *ld); +#else +static inline struct lcd_device *lcd_device_register(const char *name, + struct device *parent, void *devdata, struct lcd_ops *ops) +{ + return NULL; +} + +static inline struct lcd_device *devm_lcd_device_register(struct device *dev, + const char *name, struct device *parent, void *devdata, + struct lcd_ops *ops) +{ + return NULL; +} + +static inline void lcd_device_unregister(struct lcd_device *ld) +{ +} + +static inline void devm_lcd_device_unregister(struct device *dev, + struct lcd_device *ld) +{ +} +#endif #define to_lcd_device(obj) container_of(obj, struct lcd_device, dev)
Provide dummy functions for LCD register()/unregister() if CONFIG_LCD_CLASS_DEVICE is not set. This allows us to use the LCD class as an optional. Signed-off-by: Alexander Shiyan <shc_work@mail.ru> --- include/linux/lcd.h | 37 ++++++++++++++++++++++++++++++------- 1 file changed, 30 insertions(+), 7 deletions(-)