| 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
| ||
|
Message-ID: <8fc3ecc8-9377-f312-b17c-b755b1410ce1@linux.ibm.com>
Date: Wed, 11 May 2022 14:54:16 -0500
From: Eddie James <eajames@...ux.ibm.com>
To: Pavel Machek <pavel@....cz>
Cc: linux-leds@...r.kernel.org, linux-kernel@...r.kernel.org,
patrick@...cx.xyz, andy.shevchenko@...il.com,
openbmc@...ts.ozlabs.org, joel@....id.au
Subject: Re: [PATCH v3 4/4] leds: pca955x: Add HW blink support
On 5/4/22 12:24, Pavel Machek wrote:
> Hi!
>
>> 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.
> WHat happens to LED_HALF support in this case? Could we only do the accelerated blinking
> if HALF support is not needed?
Yes, we lose LED_HALF support in this case. Basically, only full
brightness is supported for HW blinking leds. The other channel
(non-blinking) can support any brightness, but of course only for 1 led
or all leds.
Thanks,
Eddie
>
> Best regards,
> Pavel
>
>> ---
>> drivers/leds/leds-pca955x.c | 222 +++++++++++++++++++++++++++---------
>> 1 file changed, 168 insertions(+), 54 deletions(-)
>>
>> diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
>> index 61f3cb84a945..7c156de215d7 100644
>> --- a/drivers/leds/leds-pca955x.c
>> +++ b/drivers/leds/leds-pca955x.c
>> @@ -62,6 +62,8 @@
>> #define PCA955X_GPIO_HIGH LED_OFF
>> #define PCA955X_GPIO_LOW LED_FULL
>>
>> +#define PCA955X_BLINK_DEFAULT_MS 1000
>> +
>> enum pca955x_type {
>> pca9550,
>> pca9551,
>> @@ -74,6 +76,7 @@ struct pca955x_chipdef {
>> int bits;
>> u8 slv_addr; /* 7-bit slave address mask */
>> int slv_addr_shift; /* Number of bits to ignore */
>> + int blink_div; /* PSC divider */
>> };
>>
>> static struct pca955x_chipdef pca955x_chipdefs[] = {
>> @@ -81,26 +84,31 @@ static struct pca955x_chipdef pca955x_chipdefs[] = {
>> .bits = 2,
>> .slv_addr = /* 110000x */ 0x60,
>> .slv_addr_shift = 1,
>> + .blink_div = 44,
>> },
>> [pca9551] = {
>> .bits = 8,
>> .slv_addr = /* 1100xxx */ 0x60,
>> .slv_addr_shift = 3,
>> + .blink_div = 38,
>> },
>> [pca9552] = {
>> .bits = 16,
>> .slv_addr = /* 1100xxx */ 0x60,
>> .slv_addr_shift = 3,
>> + .blink_div = 44,
>> },
>> [ibm_pca9552] = {
>> .bits = 16,
>> .slv_addr = /* 0110xxx */ 0x30,
>> .slv_addr_shift = 3,
>> + .blink_div = 44,
>> },
>> [pca9553] = {
>> .bits = 4,
>> .slv_addr = /* 110001x */ 0x62,
>> .slv_addr_shift = 1,
>> + .blink_div = 44,
>> },
>> };
>>
>> @@ -119,7 +127,9 @@ struct pca955x {
>> struct pca955x_led *leds;
>> struct pca955x_chipdef *chipdef;
>> struct i2c_client *client;
>> + unsigned long active_blink;
>> unsigned long active_pins;
>> + unsigned long blink_period;
>> #ifdef CONFIG_LEDS_PCA955X_GPIO
>> struct gpio_chip gpio;
>> #endif
>> @@ -170,7 +180,8 @@ static inline int pca955x_ledstate(u8 ls, int led_num)
>>
>> /*
>> * 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) / coeff
>> + * Where for pca9551 chips coeff = 38 and for all other chips coeff = 44
>> */
>> static int pca955x_write_psc(struct pca955x *pca955x, int n, u8 val)
>> {
>> @@ -251,6 +262,20 @@ static int pca955x_read_pwm(struct pca955x *pca955x, int n, u8 *val)
>> return 0;
>> }
>>
>> +static int pca955x_read_psc(struct pca955x *pca955x, int n, u8 *val)
>> +{
>> + u8 cmd = pca955x_num_input_regs(pca955x->chipdef->bits) + (2 * n);
>> + int ret;
>> +
>> + ret = i2c_smbus_read_byte_data(pca955x->client, cmd);
>> + if (ret < 0) {
>> + dev_err(&pca955x->client->dev, "%s: reg 0x%x, err %d\n", __func__, n, ret);
>> + return ret;
>> + }
>> + *val = (u8)ret;
>> + return 0;
>> +}
>> +
>> static enum led_brightness pca955x_led_get(struct led_classdev *led_cdev)
>> {
>> struct pca955x_led *pca955x_led = led_to_pca955x(led_cdev);
>> @@ -270,7 +295,10 @@ static enum led_brightness pca955x_led_get(struct led_classdev *led_cdev)
>> ret = LED_OFF;
>> break;
>> case PCA955X_LS_BLINK0:
>> - ret = LED_HALF;
>> + ret = pca955x_read_pwm(pca955x, 0, &pwm);
>> + if (ret)
>> + return ret;
>> + ret = 256 - pwm;
>> break;
>> case PCA955X_LS_BLINK1:
>> ret = pca955x_read_pwm(pca955x, 1, &pwm);
>> @@ -299,29 +327,36 @@ static int pca955x_led_set(struct led_classdev *led_cdev,
>> if (ret)
>> goto out;
>>
>> - switch (value) {
>> - case LED_FULL:
>> - ls = pca955x_ledsel(ls, bit, PCA955X_LS_LED_ON);
>> - break;
>> - case LED_OFF:
>> - ls = pca955x_ledsel(ls, bit, PCA955X_LS_LED_OFF);
>> - break;
>> - case LED_HALF:
>> - ls = pca955x_ledsel(ls, bit, PCA955X_LS_BLINK0);
>> - break;
>> - default:
>> - /*
>> - * Use PWM1 for all other values. This has the unwanted
>> - * side effect of making all LEDs on the chip share the
>> - * same brightness level if set to a value other than
>> - * OFF, HALF, or FULL. But, this is probably better than
>> - * just turning off for all other values.
>> - */
>> - ret = pca955x_write_pwm(pca955x, 1, 255 - value);
>> - if (ret)
>> + if (test_bit(pca955x_led->led_num, &pca955x->active_blink)) {
>> + if (value == LED_OFF) {
>> + clear_bit(pca955x_led->led_num, &pca955x->active_blink);
>> + ls = pca955x_ledsel(ls, bit, PCA955X_LS_LED_OFF);
>> + } else {
>> + ret = pca955x_write_pwm(pca955x, 0, 256 - value);
>> goto out;
>> - ls = pca955x_ledsel(ls, bit, PCA955X_LS_BLINK1);
>> - break;
>> + }
>> + } else {
>> + switch (value) {
>> + case LED_FULL:
>> + ls = pca955x_ledsel(ls, bit, PCA955X_LS_LED_ON);
>> + break;
>> + case LED_OFF:
>> + ls = pca955x_ledsel(ls, bit, PCA955X_LS_LED_OFF);
>> + break;
>> + default:
>> + /*
>> + * Use PWM1 for all other values. This has the unwanted
>> + * side effect of making all LEDs on the chip share the
>> + * same brightness level if set to a value other than
>> + * OFF or FULL. But, this is probably better than just
>> + * turning off for all other values.
>> + */
>> + ret = pca955x_write_pwm(pca955x, 1, 255 - value);
>> + if (ret)
>> + goto out;
>> + ls = pca955x_ledsel(ls, bit, PCA955X_LS_BLINK1);
>> + break;
>> + }
>> }
>>
>> ret = pca955x_write_ls(pca955x, reg, ls);
>> @@ -332,6 +367,94 @@ static int pca955x_led_set(struct led_classdev *led_cdev,
>> return ret;
>> }
>>
>> +static u8 pca955x_period_to_psc(struct pca955x *pca955x, unsigned long p)
>> +{
>> + p *= pca955x->chipdef->blink_div;
>> + p /= MSEC_PER_SEC;
>> + p -= 1;
>> +
>> + return p;
>> +}
>> +
>> +static unsigned long pca955x_psc_to_period(struct pca955x *pca955x, u8 psc)
>> +{
>> + unsigned long p = psc;
>> +
>> + p += 1;
>> + p *= MSEC_PER_SEC;
>> + p /= pca955x->chipdef->blink_div;
>> +
>> + return p;
>> +}
>> +
>> +static int pca955x_led_blink(struct led_classdev *led_cdev,
>> + unsigned long *delay_on, unsigned long *delay_off)
>> +{
>> + struct pca955x_led *pca955x_led = led_to_pca955x(led_cdev);
>> + struct pca955x *pca955x = pca955x_led->pca955x;
>> + unsigned long p = *delay_on + *delay_off;
>> + int ret;
>> +
>> + mutex_lock(&pca955x->lock);
>> +
>> + if (p) {
>> + if (*delay_on != *delay_off) {
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> + if (p < pca955x_psc_to_period(pca955x, 0) ||
>> + p > pca955x_psc_to_period(pca955x, 0xff)) {
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> + } else {
>> + p = pca955x->active_blink ? pca955x->blink_period :
>> + PCA955X_BLINK_DEFAULT_MS;
>> + }
>> +
>> + if (!pca955x->active_blink ||
>> + pca955x->active_blink == BIT(pca955x_led->led_num) ||
>> + pca955x->blink_period == p) {
>> + u8 psc = pca955x_period_to_psc(pca955x, p);
>> +
>> + if (!test_and_set_bit(pca955x_led->led_num,
>> + &pca955x->active_blink)) {
>> + u8 ls;
>> + int reg = pca955x_led->led_num / 4;
>> + int bit = pca955x_led->led_num % 4;
>> +
>> + ret = pca955x_read_ls(pca955x, reg, &ls);
>> + if (ret)
>> + goto out;
>> +
>> + ls = pca955x_ledsel(ls, bit, PCA955X_LS_BLINK0);
>> + ret = pca955x_write_ls(pca955x, reg, ls);
>> + if (ret)
>> + goto out;
>> + }
>> +
>> + if (pca955x->blink_period != p) {
>> + pca955x->blink_period = p;
>> + ret = pca955x_write_psc(pca955x, 0, psc);
>> + if (ret)
>> + goto out;
>> + }
>> +
>> + p = pca955x_psc_to_period(pca955x, psc);
>> + p /= 2;
>> + *delay_on = p;
>> + *delay_off = p;
>> + } else {
>> + ret = -EBUSY;
>> + }
>> +
>> +out:
>> + mutex_unlock(&pca955x->lock);
>> +
>> + return ret;
>> +}
>> +
>> #ifdef CONFIG_LEDS_PCA955X_GPIO
>> /*
>> * Read the INPUT register, which contains the state of LEDs.
>> @@ -487,8 +610,9 @@ static int pca955x_probe(struct i2c_client *client)
>> u8 ls1[4];
>> u8 ls2[4];
>> struct pca955x_platform_data *pdata;
>> + u8 psc0;
>> + bool keep_psc0 = false;
>> bool set_default_label = false;
>> - bool keep_pwm = false;
>> char default_label[8];
>> enum pca955x_type chip_type;
>> const void *md = device_get_match_data(&client->dev);
>> @@ -552,6 +676,7 @@ static int pca955x_probe(struct i2c_client *client)
>> mutex_init(&pca955x->lock);
>> pca955x->client = client;
>> pca955x->chipdef = chip;
>> + pca955x->blink_period = PCA955X_BLINK_DEFAULT_MS;
>>
>> init_data.devname_mandatory = false;
>> init_data.devicename = "pca955x";
>> @@ -581,15 +706,21 @@ static int pca955x_probe(struct i2c_client *client)
>> led = &pca955x_led->led_cdev;
>> led->brightness_set_blocking = pca955x_led_set;
>> led->brightness_get = pca955x_led_get;
>> + led->blink_set = pca955x_led_blink;
>>
>> if (pdata->leds[i].default_state ==
>> - LEDS_GPIO_DEFSTATE_OFF)
>> + LEDS_GPIO_DEFSTATE_OFF) {
>> ls2[reg] = pca955x_ledsel(ls2[reg], bit,
>> PCA955X_LS_LED_OFF);
>> - else if (pdata->leds[i].default_state ==
>> - LEDS_GPIO_DEFSTATE_ON)
>> + } else if (pdata->leds[i].default_state ==
>> + LEDS_GPIO_DEFSTATE_ON) {
>> ls2[reg] = pca955x_ledsel(ls2[reg], bit,
>> PCA955X_LS_LED_ON);
>> + } else if (pca955x_ledstate(ls2[reg], bit) ==
>> + PCA955X_LS_BLINK0) {
>> + keep_psc0 = true;
>> + set_bit(i, &pca955x->active_blink);
>> + }
>>
>> init_data.fwnode = pdata->leds[i].fwnode;
>>
>> @@ -617,20 +748,6 @@ static int pca955x_probe(struct i2c_client *client)
>> return err;
>>
>> set_bit(i, &pca955x->active_pins);
>> -
>> - /*
>> - * For default-state == "keep", let the core update the
>> - * brightness from the hardware, then check the
>> - * brightness to see if it's using PWM1. If so, PWM1
>> - * should not be written below.
>> - */
>> - if (pdata->leds[i].default_state ==
>> - LEDS_GPIO_DEFSTATE_KEEP) {
>> - if (led->brightness != LED_FULL &&
>> - led->brightness != LED_OFF &&
>> - led->brightness != LED_HALF)
>> - keep_pwm = true;
>> - }
>> }
>> }
>>
>> @@ -642,22 +759,19 @@ static int pca955x_probe(struct i2c_client *client)
>> }
>> }
>>
>> - /* PWM0 is used for half brightness or 50% duty cycle */
>> - err = pca955x_write_pwm(pca955x, 0, 255 - LED_HALF);
>> - if (err)
>> - return err;
>> -
>> - if (!keep_pwm) {
>> - /* PWM1 is used for variable brightness, default to OFF */
>> - err = pca955x_write_pwm(pca955x, 1, 0);
>> - if (err)
>> - return err;
>> + if (keep_psc0) {
>> + err = pca955x_read_psc(pca955x, 0, &psc0);
>> + } else {
>> + psc0 = pca955x_period_to_psc(pca955x, pca955x->blink_period);
>> + err = pca955x_write_psc(pca955x, 0, psc0);
>> }
>>
>> - /* Set to fast frequency so we do not see flashing */
>> - err = pca955x_write_psc(pca955x, 0, 0);
>> if (err)
>> return err;
>> +
>> + pca955x->blink_period = pca955x_psc_to_period(pca955x, psc0);
>> +
>> + /* Set PWM1 to fast frequency so we do not see flashing */
>> err = pca955x_write_psc(pca955x, 1, 0);
>> if (err)
>> return err;
>> --
>> 2.27.0
Powered by blists - more mailing lists