LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Paul Sokolovsky <pmiscml@gmail.com>
To: Rodolfo Giometti <giometti@enneenne.com>
Cc: linux-arm-kernel@lists.arm.linux.org.uk,
	linux-kernel@vger.kernel.org,
	<linux-fbdev-devel@lists.sourceforge.net>,
	Richard Purdie <rpurdie@rpsys.net>
Subject: Re: [PATCH 1/1] PXAFB: Support for backlight control
Date: Wed, 21 Feb 2007 18:26:08 +0200	[thread overview]
Message-ID: <1741775233.20070221182608@gmail.com> (raw)
In-Reply-To: <20070221161210.GL4641@enneenne.com>

Hello Rodolfo,

Wednesday, February 21, 2007, 6:12:10 PM, you wrote:

> On Wed, Feb 21, 2007 at 06:00:37PM +0200, Paul Sokolovsky wrote:
>> 
>>   On the other hand, there's already
>> drivers/video/backlight/backlight.c which provides generic BL support,
>> implemented using notifier callback for FB core. Moreover, there's

> My patch _uses_ that support.

  Sorry if I missed it in quick review, as I told, there's a bit much
code ;-I

>> corgi_bl.c driver which, contrary to its name is a generic driver for
>> embedded/PDA device backlight. It essentially subclassses pretty
>> abstract backlight.c, and provides good implementation for most BL
>> implementation. What you really need to do to use it, is to supply
>> single machine-dependent method, set_bl_intensity(). That method is
>> usually <10 lines.

> I see, but in this manner you need to make a complete file for each
> board, while with my patch you can just put few lines into machine's
> definition file (a struct and 2 functions).

  Why? It's the same, except that it already exists, generic one (not
limited to pxafb), and requires 1 function (too bad that C doesn't
support lambda's):

==============
static void h4000_set_bl_intensity(int intensity)
{
        /* LCD brightness is driven by PWM0.
         * We'll set the pre-scaler to 8, and the period to 1024, this
         * means the backlight refresh rate will be 3686400/(8*1024) =
         * 450 Hz which is quite enough.
         */
        PWM_CTRL0 = 7;            /* pre-scaler */
        PWM_PWDUTY0 = intensity; /* duty cycle */
        PWM_PERVAL0 = H4000_MAX_INTENSITY;      /* period */

        if (intensity > 0) {
                pxa_set_cken(CKEN0_PWM0, 1);
                asic3_set_gpio_out_b(&h4000_asic3.dev,
                        GPIOB_BACKLIGHT_POWER_ON, GPIOB_BACKLIGHT_POWER_ON);
        } else {
                pxa_set_cken(CKEN0_PWM0, 0);
                asic3_set_gpio_out_b(&h4000_asic3.dev,
                        GPIOB_BACKLIGHT_POWER_ON, 0);
        }
}

static struct corgibl_machinfo h4000_bl_machinfo = {
        .default_intensity = H4000_MAX_INTENSITY / 4,
        .limit_mask = 0x0fff,
        .max_intensity = H4000_MAX_INTENSITY,
        .set_bl_intensity = h4000_set_bl_intensity,
};

struct platform_device h4000_bl = {
        .name = "corgi-bl",
        .dev = {
                .platform_data = &h4000_bl_machinfo,
        },
};
==============

>>   With this in mind, adhoc backlight handlers in pxafb and few other
>> drivers are artifacts of older times. And it's sad they are tried to
>> be resurrected instead of being removed.

> IMHO, the actual backlight support is not so much, infact I'd like to
> generalize it to support also backlighted keyboards (or input
> devices). :)

  I sent a bit of criticism for that too ;-). YMMV, but kernel
solutions are just bound to be pretty simple and generic and lack
any "niceties", which you'd likely want to do anyway eventually. For
example, what if you'll want to implement "fade out" effect for
keyboard backlight? Doing it in adhoc manner in kernel? Whereas with
the LCD classdev, you can write generic "fade out" trigger and
attach/detach it from userspace.

> Ciao,

> Rodolfo




-- 
Best regards,
 Paul                            mailto:pmiscml@gmail.com


  reply	other threads:[~2007-02-21 16:26 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-02-21 14:53 Rodolfo Giometti
2007-02-21 16:00 ` Paul Sokolovsky
2007-02-21 16:12   ` Rodolfo Giometti
2007-02-21 16:26     ` Paul Sokolovsky [this message]
2007-02-22  8:32       ` Rodolfo Giometti
2007-02-22 10:33         ` Paul Sokolovsky
2007-02-22 16:37           ` Rodolfo Giometti
2007-02-22 17:11             ` Richard Purdie
2007-02-28 16:54             ` [Linux-fbdev-devel] " James Simmons
2007-02-22  0:59 ` Richard Purdie
2007-02-22  8:28   ` Rodolfo Giometti
2007-02-22  9:27     ` Richard Purdie
2007-02-22  9:32       ` Rodolfo Giometti

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1741775233.20070221182608@gmail.com \
    --to=pmiscml@gmail.com \
    --cc=giometti@enneenne.com \
    --cc=linux-arm-kernel@lists.arm.linux.org.uk \
    --cc=linux-fbdev-devel@lists.sourceforge.net \
    --cc=linux-kernel@vger.kernel.org \
    --cc=rpurdie@rpsys.net \
    --subject='Re: [PATCH 1/1] PXAFB: Support for backlight control' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).