linux-kernel - Re: [PATCH v2 2/2] leds: pca955x: Add HW blink support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <fe69f04f-c356-b532-e82b-0b9b99e25c86@linux.ibm.com>
Date:   Mon, 11 Apr 2022 11:06:35 -0500
From:   Eddie James <eajames@...ux.ibm.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     OpenBMC Maillist <openbmc@...ts.ozlabs.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Joel Stanley <joel@....id.au>, Pavel Machek <pavel@....cz>,
        Linux LED Subsystem <linux-leds@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] leds: pca955x: Add HW blink support


On 4/8/22 06:19, Andy Shevchenko wrote:
> On Thu, Apr 7, 2022 at 10:43 PM Eddie James <eajames@...ux.ibm.com> wrote:
>> Support blinking using the PCA955x chip. Use PWM0 for blinking
>> instead of LED_HALF brightness. Since there is only one frequency
>> and brightness register for any blinking LED, track the blink state
>> of each LED and only support one HW blinking frequency. If another
>> frequency is requested, fallback to software blinking.
> ...
>
>> +#define PCA955X_BLINK_DEFAULT  1000
> What's the unit of this number?


milliseconds, I'll change the name to reflect that.


>
> ...
>
>>    * Write to frequency prescaler register, used to program the
>> - * period of the PWM output.  period = (PSCx + 1) / 38
>> + * period of the PWM output.  period = (PSCx + 1) / <38 or 44, chip dependent>
> Using <> in  formulas a bit confusing, what about
>
>   * period of the PWM output.  period = (PSCx + 1) / coeff
>   * where for ... chips coeff = 38, for ... chips coeff = 44.
>
> ?


Ack.


>
> ...
>
>> +               dev_err(&pca955x->client->dev, "%s: reg 0x%x, err %d\n",
>> +                       __func__, n, ret);
> Can be indented better. But I would rather see regmap, where this kind
> of debugging is for free and already present in the regmap core/.


Agree, but perhaps for a future enhancement?


>
> ...
>
>> +static u8 pca955x_period_to_psc(struct pca955x *pca955x, unsigned long p)
>> +{
>> +       p *= (unsigned long)pca955x->chipdef->blink_div;
> Why casting?


Ack, and all the rest. Will get a v2 up.


Thanks Andy.

Eddie