Message-ID: <20080820055533.GB16358@pazke.donpac.ru>
Date:	Wed, 20 Aug 2008 09:55:34 +0400
From:	Andrey Panin <pazke@...ke.donpac.ru>
To:	Claudio Nieder <private@...udio.ch>
Cc:	Andrew Morton <akpm@...ux-foundation.org>, rpurdie@...ys.net,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Backlight driver for Tabletkiosk Sahara TouchIT-213
	Tablet PC

On 233, 08 20, 2008 at 01:00:41AM +0200, Claudio Nieder wrote:
> Hi,
> 
> > Poking at random ports isn't very nice. Is it possible to autodetect
> > this tablet PC ? Has it specific PCI ids or usefull DMI table ?
> 
> I didn't find anthing with lspci but can detect through DMI - if you are
> interested you find the dmidecode output at the end of
> http://claudio.ch/Bin/viewcvs.cgi/Config/sysinfo_touchIt213?rev=294 - and do it
> in a similar way as in mbp_nvidia_bl.c. Have done it in the included patch.
> 
> Looking at the macbook driver I see it is much simpler and shorter than mine
> which I derived from corgi_bl.c. No platform driver, no probe, remove, suspend
> and resume.
> 
> Why is one much simpler than the other? Is functionality missing in the MacBook
> driver or is the corgi one just (historically) overblown? If I could safely
> reduce my driver to be as simple as the macbook one I would be more than happy
> to do it.
>
> > static ?
> 
> Fixed.
> 
> > Why do you need this wrapper ?
> 
> When I studied the corgi_bl driver I noticed that some stuff is done in the
> driver itself and then there is the "platform stuff". I didn't know how to
> handle it and was given the suggestion to put the platform stuff also in the
> driver which I did but tried to keep that textually separate. Thus the separate
> sahara_init. Yet I had to call it in kb3886_init otherwise nothing else would
> do. I surely didn't fully understand what I was doing when I wrote that. Have
> simplified it as suggested.
> 
> If all this can be simplified even more, like is done in macbook driver, that
> would be great.
> 
> > This function is unused. Looks like you copied it from corgi_bl.c where
> > it is called from platform code.
> 
> Fixed, i.e. removed.
> 
> Below the fixed patch, correcting all mistakes reported by Andrey.
> 
> 
> Signed-off-by: Claudio Nieder <private@...udio.ch>
> 
> ---
>  drivers/video/backlight/Kconfig     |    7 +
>  drivers/video/backlight/Makefile    |    1 +
>  drivers/video/backlight/kb3886_bl.c |  214 +++++++++++++++++++++++++++++++++++
>  3 files changed, 222 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index 452b770..bc65541 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -164,3 +164,10 @@ config BACKLIGHT_MBP_NVIDIA
>           If you have an Apple Macbook Pro with Nvidia graphics hardware say Y
>  	 to enable a driver for its backlight
> 
> +config BACKLIGHT_SAHARA
> +	tristate "Tabletkiosk Sahara Touch-iT Backlight Driver"
> +	depends on BACKLIGHT_CLASS_DEVICE && X86
> +	default y
> +	help
> +	  If you have a Tabletkiosk Sahara Touch-iT, say y to enable the
> +	  backlight driver.
> diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
> index b405aac..f4a1183 100644
> --- a/drivers/video/backlight/Makefile
> +++ b/drivers/video/backlight/Makefile
> @@ -16,4 +16,5 @@ obj-$(CONFIG_BACKLIGHT_PROGEAR) += progear_bl.o
>  obj-$(CONFIG_BACKLIGHT_CARILLO_RANCH) += cr_bllcd.o
>  obj-$(CONFIG_BACKLIGHT_PWM)	+= pwm_bl.o
>  obj-$(CONFIG_BACKLIGHT_MBP_NVIDIA) += mbp_nvidia_bl.o
> +obj-$(CONFIG_BACKLIGHT_SAHARA)	+= kb3886_bl.o
> 
> diff --git a/drivers/video/backlight/kb3886_bl.c
> b/drivers/video/backlight/kb3886_bl.c
> new file mode 100644
> index 0000000..1a1d8aa
> --- /dev/null
> +++ b/drivers/video/backlight/kb3886_bl.c
> @@ -0,0 +1,214 @@
> +/*
> + *  Backlight Driver for the KB3886 Backlight
> + *
> + *  Copyright (c) 2007-2008 Claudio Nieder
> + *
> + *  Based on corgi_bl.c by Richard Purdie and kb3886 driver by Robert Woerle
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2 as
> + *  published by the Free Software Foundation.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +#include <linux/mutex.h>
> +#include <linux/fb.h>
> +#include <linux/backlight.h>
> +#include <linux/delay.h>
> +#include <linux/dmi.h>
> +
> +/*
> + * The following is what corgi has in arch/arm/mach-pxa/corgi.c and
> + * corgi_lcd.c. On kernelnebwies it was suggested I put the platform_device
> + * code in my driver.
> + */
> +
> +#define KB3886_PARENT 0x64
> +#define KB3886_IO 0x60
> +#define KB3886_ADC_DAC_PWM 0xC4
> +#define KB3886_PWM0_WRITE 0x81
> +#define KB3886_PWM0_READ 0x41
> +
> +static DEFINE_MUTEX(bl_mutex);
> +
> +static void kb3886_bl_set_intensity(int intensity)
> +{
> +	mutex_lock(&bl_mutex);
> +	intensity = intensity&0xff;
> +	outb(KB3886_ADC_DAC_PWM, KB3886_PARENT);
> +	msleep(10);
> +	outb(KB3886_PWM0_WRITE, KB3886_IO);
> +	msleep(10);
> +	outb(intensity, KB3886_IO);
> +	mutex_unlock(&bl_mutex);
> +}
> +
> +struct kb3886bl_machinfo {
> +	int max_intensity;
> +	int default_intensity;
> +	int limit_mask;
> +	void (*set_bl_intensity)(int intensity);
> +};
> +
> +static struct kb3886bl_machinfo kb3886_bl_machinfo = {
> +	.max_intensity = 0xff,
> +	.default_intensity = 0xa0,
> +	.limit_mask = 0x7f,
> +	.set_bl_intensity = kb3886_bl_set_intensity,
> +};
> +
> +static struct platform_device kb3886bl_device = {
> +	.name		= "kb3886-bl",
> +	.dev		= {
> +		.platform_data	= &kb3886_bl_machinfo,
> +	},
> +	.id		= -1,
> +};
> +
> +static struct platform_device *devices[] __initdata = {
> +	&kb3886bl_device,
> +};
> +
> +/*
> + * Back to driver
> + */
> +
> +static int kb3886bl_intensity;
> +static struct backlight_device *kb3886_backlight_device;
> +static struct kb3886bl_machinfo *bl_machinfo;
> +
> +static unsigned long kb3886bl_flags;
> +#define KB3886BL_SUSPENDED     0x01
> +#define KB3886BL_BATTLOW       0x02
> +
> +static struct dmi_system_id __initdata kb3886bl_device_table[] = {
> +	{
> +		.ident = "Sahara Touch-iT",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "SDV"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "iTouch T201"),
> +		},
> +	},
> +	{ },
> +	{ },

These two empty initializers are not needed.

> +	{ }
> +};
> +
> +static int kb3886bl_send_intensity(struct backlight_device *bd)
> +{
> +	int intensity = bd->props.brightness;
> +
> +	if (bd->props.power != FB_BLANK_UNBLANK)
> +		intensity = 0;
> +	if (bd->props.fb_blank != FB_BLANK_UNBLANK)
> +		intensity = 0;
> +	if (kb3886bl_flags & KB3886BL_SUSPENDED)
> +		intensity = 0;
> +	if (kb3886bl_flags & KB3886BL_BATTLOW)
> +		intensity &= bl_machinfo->limit_mask;
> +
> +	bl_machinfo->set_bl_intensity(intensity);
> +
> +	kb3886bl_intensity = intensity;
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int kb3886bl_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	struct backlight_device *bd = platform_get_drvdata(pdev);
> +
> +	kb3886bl_flags |= KB3886BL_SUSPENDED;
> +	backlight_update_status(bd);
> +	return 0;
> +}
> +
> +static int kb3886bl_resume(struct platform_device *pdev)
> +{
> +	struct backlight_device *bd = platform_get_drvdata(pdev);
> +
> +	kb3886bl_flags &= ~KB3886BL_SUSPENDED;
> +	backlight_update_status(bd);
> +	return 0;
> +}
> +#else
> +#define kb3886bl_suspend	NULL
> +#define kb3886bl_resume		NULL
> +#endif
> +
> +static int kb3886bl_get_intensity(struct backlight_device *bd)
> +{
> +	return kb3886bl_intensity;
> +}
> +
> +static struct backlight_ops kb3886bl_ops = {
> +	.get_brightness = kb3886bl_get_intensity,
> +	.update_status  = kb3886bl_send_intensity,
> +};
> +
> +static int kb3886bl_probe(struct platform_device *pdev)
> +{
> +	struct kb3886bl_machinfo *machinfo = pdev->dev.platform_data;
> +
> +	bl_machinfo = machinfo;
> +	if (!machinfo->limit_mask)
> +		machinfo->limit_mask = -1;
> +
> +	kb3886_backlight_device = backlight_device_register("kb3886-bl",
> +		&pdev->dev, NULL, &kb3886bl_ops);
> +	if (IS_ERR (kb3886_backlight_device))
> +		return PTR_ERR(kb3886_backlight_device);
> +
> +	platform_set_drvdata(pdev, kb3886_backlight_device);
> +
> +	kb3886_backlight_device->props.max_brightness = machinfo->max_intensity;
> +	kb3886_backlight_device->props.power = FB_BLANK_UNBLANK;
> +	kb3886_backlight_device->props.brightness = machinfo->default_intensity;
> +	backlight_update_status(kb3886_backlight_device);
> +
> +	return 0;
> +}
> +
> +static int kb3886bl_remove(struct platform_device *pdev)
> +{
> +	struct backlight_device *bd = platform_get_drvdata(pdev);
> +
> +	backlight_device_unregister(bd);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver kb3886bl_driver = {
> +	.probe		= kb3886bl_probe,
> +	.remove		= kb3886bl_remove,
> +	.suspend	= kb3886bl_suspend,
> +	.resume		= kb3886bl_resume,
> +	.driver		= {
> +		.name	= "kb3886-bl",
> +	},
> +};
> +
> +static int __init kb3886_init(void)
> +{
> +	if (!dmi_check_system(kb3886bl_device_table))
> +		return -ENODEV;
> +
> +	platform_add_devices(devices, ARRAY_SIZE(devices));
> +	return platform_driver_register(&kb3886bl_driver);
> +}
> +
> +static void __exit kb3886_exit(void)
> +{
> +	platform_driver_unregister(&kb3886bl_driver);
> +}
> +
> +module_init(kb3886_init);
> +module_exit(kb3886_exit);
> +
> +MODULE_AUTHOR("Claudio Nieder <private@...udio.ch>");
> +MODULE_DESCRIPTION("Tabletkiosk Sahara Touch-iT Backlight Driver");
> +MODULE_LICENSE("GPL");

Now let's add module alias to ensure driver autoloading.
IMHO the line below should do the trick:

MODULE_ALIAS("dmi:*:svnSDV:pniTouchT201:*");

> -- 
> 1.4.4.4
> 
> claudio
> -- 
> Claudio Nieder, Talweg 6, CH-8610 Uster, Tel +4179 357 6743, www.claudio.ch

-- 
Andrey Panin		| Linux and UNIX system administrator
pazke@...pac.ru		| PGP key: wwwkeys.pgp.net

Download attachment "signature.asc" of type "application/pgp-signature" (190 bytes)

Powered by blists - more mailing lists