Re: [RFC PATCH] pwm: add TLC59108/TLC59116 PWM driver

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 1816 bytes --]



On 18/08/15 17:19, Andrew Lunn wrote:

>> +Required properties:
>> +  - compatible: "ti,tlc59108-pwm" or "ti,tlc59116-pwm"
>> +  - #pwm-cells: should be 2. See pwm.txt in this directory for a description of
>> +    the cells format.
>> +  - reg: physical base address and size of the registers map.
> 
> Cut and paste error. Reg is the i2c address.

Oh, right. Thanks.

>> +static int tlc591xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>> +			       int duty_ns, int period_ns)
>> +{
>> +	struct tlc591xx_priv *priv = to_tlc591xx_priv(chip);
>> +	unsigned val;
>> +	int r;
>> +
>> +	if (period_ns != TLC591XX_CLK_PERIOD) {
>> +		dev_err(chip->dev, "only period of 10309 ns is supported\n");
>> +		return -EINVAL;
>> +	}
> 
> The hardware does allow you to change the period, but there is only
> one clock generator for all the channels. If you are going to make the

Hmm... How does that work?

The per-led output is fixed 97-kHz. Then there's the group dimming
signal, which can be superimposed on all the individual outputs, with
fixed 190-Hz. Neither frequency can be tuned. And when using group
dimming, the output isn't exactly PWM anymore, but two PWM signals combined.

Is there some other setting I'm missing?

These group dimming features don't quite fit into PWM model, I guess.
Then again, I don't think LED framework offers any ways to utilize them
either.

> effort to try to model the hardware as a PWM, you should try to
> actually implement the PWM API. Let the first channel which calls
> config set the period, and return an error for any other channel which
> picks a different period.

If there is a way to set the period, I think it may be better to define
it in the TLC's DT data than by "whoever gets there first".

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]