All of lore.kernel.org
 help / color / mirror / Atom feed
From: Andrew Lunn <andrew@lunn.ch>
To: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: linux-pwm@vger.kernel.org,
	Thierry Reding <thierry.reding@gmail.com>,
	linux-leds@vger.kernel.org,
	Jacek Anaszewski <j.anaszewski@samsung.com>,
	Bryan Wu <cooloney@gmail.com>
Subject: Re: [RFC PATCH] pwm: TLC591xx PWM driver
Date: Tue, 18 Aug 2015 17:09:16 +0200	[thread overview]
Message-ID: <20150818150916.GG4381@lunn.ch> (raw)
In-Reply-To: <1439905927-27861-1-git-send-email-tomi.valkeinen@ti.com>

> On the other hand, there's pwm_bl.c which give us backlight device
> with PWM,

Lets look at this. A backlight device seems to do most of its work in
the update_status callback. It is given a brightness in
bl->props.brightness, which takes a value between 0 and
props.max_brightness.

What pwm_bl.c does it then turn this brightness value into an
artificial PWM configuration. Your proposed PWM driver then turns this
back into a brightness, since you don't actually implement the period
part of the PWM interface.

>From an architecture point of view, doesn't an LED class device, which
takes a brightness value, seem much more naturally?

It seems like implementing a generic led_bl.c driver would make sense.
It would also allow some of the code in drivers/video/backlight/ to be
eliminated. There seems to be both an LED class driver for lp55xx and
a blacklight driver lp855x_bl.c. There are duplicate lp8788, 88pm860x,
adp5520, da903x, da9052, hp6xx, and lm3533 drivers which might all be
removed if a led_bl.c generic driver existed.

> and a GPIO over PWM sounds more sane to me than GPIO over LED.

Currently two LED class drivers are calling gpiochip_add:

~/linux/drivers/leds$ grep gpiochip_add *.c
leds-pca9532.c:	      	   err = gpiochip_add(&data->gpio);
leds-tca6507.c:		   err = gpiochip_add(&tca->gpio);

The pca9532 has full GPIO capabilities, in as well as out. But it
seems like tca6507 is output only. The TLC59108/TLC59116 is also
output only. So a generic GPO driver on top of LED would make sense
for these two, and save some code/bugs.

>From a stand back, lets take a look at the architecture point of view,
generic led_bl and gpio-led drivers seem to make sense.

       Andrew

WARNING: multiple messages have this Message-ID (diff)
From: Andrew Lunn <andrew@lunn.ch>
To: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: linux-pwm@vger.kernel.org,
	Thierry Reding <thierry.reding@gmail.com>,
	linux-leds@vger.kernel.org,
	Jacek Anaszewski <j.anaszewski@samsung.com>,
	Bryan Wu <cooloney@gmail.com>
Subject: Re: [RFC PATCH] pwm: TLC591xx PWM driver
Date: Tue, 18 Aug 2015 17:09:16 +0200	[thread overview]
Message-ID: <20150818150916.GG4381@lunn.ch> (raw)
In-Reply-To: <1439905927-27861-1-git-send-email-tomi.valkeinen@ti.com>

> On the other hand, there's pwm_bl.c which give us backlight device
> with PWM,

Lets look at this. A backlight device seems to do most of its work in
the update_status callback. It is given a brightness in
bl->props.brightness, which takes a value between 0 and
props.max_brightness.

What pwm_bl.c does it then turn this brightness value into an
artificial PWM configuration. Your proposed PWM driver then turns this
back into a brightness, since you don't actually implement the period
part of the PWM interface.

From an architecture point of view, doesn't an LED class device, which
takes a brightness value, seem much more naturally?

It seems like implementing a generic led_bl.c driver would make sense.
It would also allow some of the code in drivers/video/backlight/ to be
eliminated. There seems to be both an LED class driver for lp55xx and
a blacklight driver lp855x_bl.c. There are duplicate lp8788, 88pm860x,
adp5520, da903x, da9052, hp6xx, and lm3533 drivers which might all be
removed if a led_bl.c generic driver existed.

> and a GPIO over PWM sounds more sane to me than GPIO over LED.

Currently two LED class drivers are calling gpiochip_add:

~/linux/drivers/leds$ grep gpiochip_add *.c
leds-pca9532.c:	      	   err = gpiochip_add(&data->gpio);
leds-tca6507.c:		   err = gpiochip_add(&tca->gpio);

The pca9532 has full GPIO capabilities, in as well as out. But it
seems like tca6507 is output only. The TLC59108/TLC59116 is also
output only. So a generic GPO driver on top of LED would make sense
for these two, and save some code/bugs.

From a stand back, lets take a look at the architecture point of view,
generic led_bl and gpio-led drivers seem to make sense.

       Andrew

  parent reply	other threads:[~2015-08-18 15:09 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-08-18 13:52 [RFC PATCH] pwm: TLC591xx PWM driver Tomi Valkeinen
2015-08-18 13:52 ` [RFC PATCH] pwm: add TLC59108/TLC59116 " Tomi Valkeinen
2015-08-18 14:19   ` Andrew Lunn
2015-08-18 14:58     ` Tomi Valkeinen
2015-08-18 15:14       ` Andrew Lunn
2015-08-18 15:09 ` Andrew Lunn [this message]
2015-08-18 15:09   ` [RFC PATCH] pwm: TLC591xx " Andrew Lunn
2015-08-19 10:24   ` Thierry Reding
2015-08-19 10:52   ` Tomi Valkeinen
2015-08-19 13:31     ` Andrew Lunn

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=20150818150916.GG4381@lunn.ch \
    --to=andrew@lunn.ch \
    --cc=cooloney@gmail.com \
    --cc=j.anaszewski@samsung.com \
    --cc=linux-leds@vger.kernel.org \
    --cc=linux-pwm@vger.kernel.org \
    --cc=thierry.reding@gmail.com \
    --cc=tomi.valkeinen@ti.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.