| Message ID | 20201021201224.3430546-1-bjorn.andersson@linaro.org |
|---|---|
| Headers | show |
| Series | Qualcomm Light Pulse Generator | expand |
Hi Bjorn, On 10/21/20 10:12 PM, Bjorn Andersson wrote: > The Light Pulse Generator (LPG) is a PWM-block found in a wide range of > PMICs from Qualcomm. It can operate on fixed parameters or based on a > lookup-table, altering the duty cycle over time - which provides the > means for e.g. hardware assisted transitions of LED brightness. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > Tested-by: Luca Weiss <luca@z3ntu.xyz> Thanks for these patches. I have tested them successfully on the Sony Xperia XA2 (Discovery, Nile platform) with the leds on the PM660l - feel free to add my Tested-by. Should I send the configuration your way for inclusion in this patch, or submit them separately (either applied after, or included as separate patches in the next version of this series)? > +/** > + * struct lpg_data - initialization data > + * @lut_base: base address of LUT block > + * @lut_size: number of entries in LUT > + * @triled_base: base address of TRILED > + * @pwm_9bit_mask: bitmask for switching from 6bit to 9bit pwm Our downstream kernel derives this from the "LPG subtype" for each distinct channel, read from register offset +0x5. A value of 0xb is subtype "PWM" with a shift of 2, a value of 0x11 is subtype "LPG_LITE" with a shift of 4. Can we do the same here instead of hardcoding it for all channels in the LPG at once? How should we determine if the mask is one or two bits wide, for the 3<<4 case? > + * @num_channels: number of channels in LPG > + * @channels: list of channel initialization data > + */ > + if (ping_pong) { > + if (len % 2) > + hi_pause = 0; > + else > + hi_pause = pattern[len + 1 / 2].delta_t; len + 1 should be wrapped in parentheses just like the reassignment to len= below, otherwise this is always an out of bounds read (at len + 0). > + lo_pause = pattern[len - 1].delta_t; > + > + len = (len + 1) / 2; > + } else { > + hi_pause = pattern[len - 1].delta_t; > + lo_pause = 0; > + } > + > + ret = lpg_lut_store(lpg, pattern, len, &lo_idx, &hi_idx); > + if (ret < 0) > + goto out; > + > + for (i = 0; i < led->num_channels; i++) { > + chan = led->channels[i]; > + > + chan->ramp_duration_ms = pattern[0].delta_t * len; Perhaps this could store the duration of a single step instead, since the only use in lpg_apply_lut_control divides it by pattern length again? > + chan->ramp_ping_pong = ping_pong; > + chan->ramp_oneshot = repeat != -1; > + > + chan->ramp_lo_pause_ms = lo_pause; > + chan->ramp_hi_pause_ms = hi_pause; > + > + chan->pattern_lo_idx = lo_idx; > + chan->pattern_hi_idx = hi_idx; > + } > + > +out: > + return ret; > +} > +static int lpg_init_lut(struct lpg *lpg) > +{ > + const struct lpg_data *data = lpg->data; > + size_t bitmap_size; > + > + if (!data->lut_base) > + return 0; > + > + lpg->lut_base = data->lut_base; > + lpg->lut_size = data->lut_size; > + > + bitmap_size = BITS_TO_LONGS(lpg->lut_size) * sizeof(unsigned long); > + lpg->lut_bitmap = devm_kzalloc(lpg->dev, bitmap_size, GFP_KERNEL); Would it be nicer to use BITS_TO_BYTES here, or otherwise devm_kcalloc(..., bitmap_size, sizeof(long), ...) without mutiplying with sizeof(unsigned long)? > + > + bitmap_clear(lpg->lut_bitmap, 0, lpg->lut_size); > + return lpg->lut_bitmap ? 0 : -ENOMEM; > +} > + > +static int lpg_probe(struct platform_device *pdev) > +{ > + struct device_node *np; > + struct lpg *lpg; > + int ret; > + int i; > + > + lpg = devm_kzalloc(&pdev->dev, sizeof(*lpg), GFP_KERNEL); > + if (!lpg) > + return -ENOMEM; > + > + lpg->data = of_device_get_match_data(&pdev->dev); > + if (!lpg->data) > + return -EINVAL; > + > + lpg->dev = &pdev->dev; > + > + lpg->map = dev_get_regmap(pdev->dev.parent, NULL); > + if (!lpg->map) { > + dev_err(&pdev->dev, "parent regmap unavailable\n"); > + return -ENXIO; > + } > + > + ret = lpg_init_channels(lpg); > + if (ret < 0) > + return ret; > + > + ret = lpg_init_triled(lpg); > + if (ret < 0) > + return ret; > + > + ret = lpg_init_lut(lpg); > + if (ret < 0) > + return ret; How about turning these returns into dev_err_probe? I'm not sure if that's the expected way to go nowadays, but having some form of logging when a driver fails to probe is always good to have. Thanks! Marijn > + > + for_each_available_child_of_node(pdev->dev.of_node, np) { > + ret = lpg_add_led(lpg, np); > + if (ret) > + return ret; > + } > + > + for (i = 0; i < lpg->num_channels; i++) > + lpg_apply_dtest(&lpg->channels[i]); > + > + ret = lpg_add_pwm(lpg); > + if (ret) > + return ret; > + > + platform_set_drvdata(pdev, lpg); > + > + return 0; > +}
On Sun 18 Apr 16:54 CDT 2021, Marijn Suijten wrote: > Hi Bjorn, > > On 10/21/20 10:12 PM, Bjorn Andersson wrote: > > The Light Pulse Generator (LPG) is a PWM-block found in a wide range of > > PMICs from Qualcomm. It can operate on fixed parameters or based on a > > lookup-table, altering the duty cycle over time - which provides the > > means for e.g. hardware assisted transitions of LED brightness. > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > Tested-by: Luca Weiss <luca@z3ntu.xyz> > > > Thanks for these patches. I have tested them successfully on the Sony > Xperia XA2 (Discovery, Nile platform) with the leds on the PM660l - feel > free to add my Tested-by. Should I send the configuration your way for > inclusion in this patch, or submit them separately (either applied after, or > included as separate patches in the next version of this series)? > Thanks for testing, let's try to land this first iteration first and then we can add PM660l and PM8150* definitions/support on top. > > +/** > > + * struct lpg_data - initialization data > > + * @lut_base: base address of LUT block > > + * @lut_size: number of entries in LUT > > + * @triled_base: base address of TRILED > > + * @pwm_9bit_mask: bitmask for switching from 6bit to 9bit pwm > > > Our downstream kernel derives this from the "LPG subtype" for each distinct > channel, read from register offset +0x5. A value of 0xb is subtype "PWM" > with a shift of 2, a value of 0x11 is subtype "LPG_LITE" with a shift of 4. > Can we do the same here instead of hardcoding it for all channels in the LPG > at once? How should we determine if the mask is one or two bits wide, for > the 3<<4 case? > I don't see any obvious solution to the latter, so perhaps we should just stick with defining this per compatible? Or am I reading your suggestion wrong? > > + * @num_channels: number of channels in LPG > > + * @channels: list of channel initialization data > > + */ > > > + if (ping_pong) { > > + if (len % 2) > > + hi_pause = 0; > > + else > > + hi_pause = pattern[len + 1 / 2].delta_t; > > > len + 1 should be wrapped in parentheses just like the reassignment to len= > below, otherwise this is always an out of bounds read (at len + 0). > Thanks for spotting this. > > + lo_pause = pattern[len - 1].delta_t; > > + > > + len = (len + 1) / 2; > > + } else { > > + hi_pause = pattern[len - 1].delta_t; > > + lo_pause = 0; > > + } > > + > > + ret = lpg_lut_store(lpg, pattern, len, &lo_idx, &hi_idx); > > + if (ret < 0) > > + goto out; > > + > > + for (i = 0; i < led->num_channels; i++) { > > + chan = led->channels[i]; > > + > > + chan->ramp_duration_ms = pattern[0].delta_t * len; > > > Perhaps this could store the duration of a single step instead, since the > only use in lpg_apply_lut_control divides it by pattern length again? > Sounds like a good idea. > > + chan->ramp_ping_pong = ping_pong; > > + chan->ramp_oneshot = repeat != -1; > > + > > + chan->ramp_lo_pause_ms = lo_pause; > > + chan->ramp_hi_pause_ms = hi_pause; > > + > > + chan->pattern_lo_idx = lo_idx; > > + chan->pattern_hi_idx = hi_idx; > > + } > > + > > +out: > > + return ret; > > +} > > > +static int lpg_init_lut(struct lpg *lpg) > > +{ > > + const struct lpg_data *data = lpg->data; > > + size_t bitmap_size; > > + > > + if (!data->lut_base) > > + return 0; > > + > > + lpg->lut_base = data->lut_base; > > + lpg->lut_size = data->lut_size; > > + > > + bitmap_size = BITS_TO_LONGS(lpg->lut_size) * sizeof(unsigned long); > > + lpg->lut_bitmap = devm_kzalloc(lpg->dev, bitmap_size, GFP_KERNEL); > > > Would it be nicer to use BITS_TO_BYTES here, or otherwise devm_kcalloc(..., > bitmap_size, sizeof(long), ...) without mutiplying with sizeof(unsigned > long)? > Yes, that's cleaner. > > + > > + bitmap_clear(lpg->lut_bitmap, 0, lpg->lut_size); > > + return lpg->lut_bitmap ? 0 : -ENOMEM; > > +} > > + > > +static int lpg_probe(struct platform_device *pdev) > > +{ > > + struct device_node *np; > > + struct lpg *lpg; > > + int ret; > > + int i; > > + > > + lpg = devm_kzalloc(&pdev->dev, sizeof(*lpg), GFP_KERNEL); > > + if (!lpg) > > + return -ENOMEM; > > + > > + lpg->data = of_device_get_match_data(&pdev->dev); > > + if (!lpg->data) > > + return -EINVAL; > > + > > + lpg->dev = &pdev->dev; > > + > > + lpg->map = dev_get_regmap(pdev->dev.parent, NULL); > > + if (!lpg->map) { > > + dev_err(&pdev->dev, "parent regmap unavailable\n"); > > + return -ENXIO; > > + } > > + > > + ret = lpg_init_channels(lpg); > > + if (ret < 0) > > + return ret; > > + > > + ret = lpg_init_triled(lpg); > > + if (ret < 0) > > + return ret; > > + > > + ret = lpg_init_lut(lpg); > > + if (ret < 0) > > + return ret; > > > How about turning these returns into dev_err_probe? I'm not sure if that's > the expected way to go nowadays, but having some form of logging when a > driver fails to probe is always good to have. > The intention is that each code path through these functions will either pass or spit out an error in the log. I looked through them again and think I cover all paths... Thanks, Bjorn
On Thu 29 Oct 13:13 CDT 2020, Pavel Machek wrote: > Hi! > > > The Light Pulse Generator (LPG) is a PWM-block found in a wide range of > > PMICs from Qualcomm. It can operate on fixed parameters or based on a > > lookup-table, altering the duty cycle over time - which provides the > > means for e.g. hardware assisted transitions of LED brightness. > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > --- > > > > Changes since v5: > > - Make sure to not used the state of the last channel in a group to determine > > if the current sink should be active for all channels in the group. > > - Replacement of unsigned -1 with UINT_MAX > > - Work around potential overflow by using larger data types, instead of separate code paths > > - Use cpu_to_l16() rather than hand rolling them > > - Minor style cleanups > > > > drivers/leds/Kconfig | 9 + > > drivers/leds/Makefile | 1 + > > drivers/leds/leds-qcom-lpg.c | 1190 ++++++++++++++++++++++++++++++++++ > > 3 files changed, 1200 insertions(+) > > create mode 100644 drivers/leds/leds-qcom-lpg.c > > Let's put this into drivers/leds/rgb/. You may need to create it. > Will do so. > > > +static int lpg_lut_store(struct lpg *lpg, struct led_pattern *pattern, > > + size_t len, unsigned int *lo_idx, unsigned int *hi_idx) > > +{ > > + unsigned int idx; > > + __le16 val; > > No need for __XX variants outside of headers meant for userspace. > __le16 is the in-kernel return type for cpu_to_le16(), but after further review I believe I don't need to do this. > > +#define LPG_ENABLE_GLITCH_REMOVAL BIT(5) > > + > > +static void lpg_enable_glitch(struct lpg_channel *chan) > > +{ > > + struct lpg *lpg = chan->lpg; > > + > > + regmap_update_bits(lpg->map, chan->base + PWM_TYPE_CONFIG_REG, > > + LPG_ENABLE_GLITCH_REMOVAL, 0); > > +} > > + > > +static void lpg_disable_glitch(struct lpg_channel *chan) > > +{ > > + struct lpg *lpg = chan->lpg; > > + > > + regmap_update_bits(lpg->map, chan->base + PWM_TYPE_CONFIG_REG, > > + LPG_ENABLE_GLITCH_REMOVAL, > > + LPG_ENABLE_GLITCH_REMOVAL); > > +} > > Helper functions for single register write is kind of overkill... > Yes, it is, but it keep lpg_apply() tidy. > > +static int lpg_blink_set(struct lpg_led *led, > > + unsigned long delay_on, unsigned long delay_off) > > +{ > > + struct lpg_channel *chan; > > + unsigned int period_us; > > + unsigned int duty_us; > > + int i; > > + > > + if (!delay_on && !delay_off) { > > + delay_on = 500; > > + delay_off = 500; > > + } > > Aren't you supposed to modify the values passed to you, so that > userspace knows what the default rate is? > I had missed this. > > > + ret = lpg_lut_store(lpg, pattern, len, &lo_idx, &hi_idx); > > + if (ret < 0) > > + goto out; > > Just do direct return. > Will do. > > +out: > > + return ret; > > +} > > > +static const struct pwm_ops lpg_pwm_ops = { > > + .request = lpg_pwm_request, > > + .apply = lpg_pwm_apply, > > + .owner = THIS_MODULE, > > +}; > > + > > +static int lpg_add_pwm(struct lpg *lpg) > > +{ > > + int ret; > > + > > + lpg->pwm.base = -1; > > + lpg->pwm.dev = lpg->dev; > > + lpg->pwm.npwm = lpg->num_channels; > > + lpg->pwm.ops = &lpg_pwm_ops; > > + > > + ret = pwmchip_add(&lpg->pwm); > > + if (ret) > > + dev_err(lpg->dev, "failed to add PWM chip: ret %d\n", ret); > > + > > + return ret; > > +} > > Do we need to do this? I'd rather have LED driver, than LED+PWM > driver... > Yes, I believe we need to do this. Because each piece of hardware has N channels, which can be wired to LEDs, grouped with other channels and wired to multicolor LEDs or be used as PWM signals. And this configuration is board specific. One such example is the laptop in front of me, which has 3 channels wired to an RGB LED and 1 channel wired as a backlight control signal (i.e. using pwm-backlight). Another example is a devboard where the 4 channels are wired to 4 LEDs. Regards, Bjorn
On 4/29/21 12:39 AM, Bjorn Andersson wrote: > On Sun 18 Apr 16:54 CDT 2021, Marijn Suijten wrote: > >> Hi Bjorn, >> >> On 10/21/20 10:12 PM, Bjorn Andersson wrote: >>> The Light Pulse Generator (LPG) is a PWM-block found in a wide range of >>> PMICs from Qualcomm. It can operate on fixed parameters or based on a >>> lookup-table, altering the duty cycle over time - which provides the >>> means for e.g. hardware assisted transitions of LED brightness. >>> >>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> >>> Tested-by: Luca Weiss <luca@z3ntu.xyz> >> >> >> Thanks for these patches. I have tested them successfully on the Sony >> Xperia XA2 (Discovery, Nile platform) with the leds on the PM660l - feel >> free to add my Tested-by. Should I send the configuration your way for >> inclusion in this patch, or submit them separately (either applied after, or >> included as separate patches in the next version of this series)? >> > > Thanks for testing, let's try to land this first iteration first and > then we can add PM660l and PM8150* definitions/support on top. I'll keep an eye out for when these patches land and send them on top. Feel free to add me to the CC list for future revisions. >>> +/** >>> + * struct lpg_data - initialization data >>> + * @lut_base: base address of LUT block >>> + * @lut_size: number of entries in LUT >>> + * @triled_base: base address of TRILED >>> + * @pwm_9bit_mask: bitmask for switching from 6bit to 9bit pwm >> >> >> Our downstream kernel derives this from the "LPG subtype" for each distinct >> channel, read from register offset +0x5. A value of 0xb is subtype "PWM" >> with a shift of 2, a value of 0x11 is subtype "LPG_LITE" with a shift of 4. >> Can we do the same here instead of hardcoding it for all channels in the LPG >> at once? How should we determine if the mask is one or two bits wide, for >> the 3<<4 case? >> > > I don't see any obvious solution to the latter, so perhaps we should > just stick with defining this per compatible? Or am I reading your > suggestion wrong? Assuming these devices have a different "LPG subtype" you should be able to read their value and add it to the list as third option. Alternatively, can you point out the driver this `3<<4` mask was based on? With all the information available it should be possible to derive this from hardware for every channel instead of hardcoding it. >>> + >>> + bitmap_clear(lpg->lut_bitmap, 0, lpg->lut_size); >>> + return lpg->lut_bitmap ? 0 : -ENOMEM; >>> +} >>> + >>> +static int lpg_probe(struct platform_device *pdev) >>> +{ >>> + struct device_node *np; >>> + struct lpg *lpg; >>> + int ret; >>> + int i; >>> + >>> + lpg = devm_kzalloc(&pdev->dev, sizeof(*lpg), GFP_KERNEL); >>> + if (!lpg) >>> + return -ENOMEM; >>> + >>> + lpg->data = of_device_get_match_data(&pdev->dev); >>> + if (!lpg->data) >>> + return -EINVAL; >>> + >>> + lpg->dev = &pdev->dev; >>> + >>> + lpg->map = dev_get_regmap(pdev->dev.parent, NULL); >>> + if (!lpg->map) { >>> + dev_err(&pdev->dev, "parent regmap unavailable\n"); >>> + return -ENXIO; >>> + } >>> + >>> + ret = lpg_init_channels(lpg); >>> + if (ret < 0) >>> + return ret; >>> + >>> + ret = lpg_init_triled(lpg); >>> + if (ret < 0) >>> + return ret; >>> + >>> + ret = lpg_init_lut(lpg); >>> + if (ret < 0) >>> + return ret; >> >> >> How about turning these returns into dev_err_probe? I'm not sure if that's >> the expected way to go nowadays, but having some form of logging when a >> driver fails to probe is always good to have. >> > > The intention is that each code path through these functions will either > pass or spit out an error in the log. I looked through them again and > think I cover all paths... That is true, all the errors not covered are extremely unlikely like -ENOMEM. I vaguely recall having to insert extra logging to get through initial probe, but that might have been something inside lpg_add_led as well. Fine to leave this as it is. - Marijn
On Thu 29 Apr 14:31 CDT 2021, Marijn Suijten wrote: > On 4/29/21 12:39 AM, Bjorn Andersson wrote: > > On Sun 18 Apr 16:54 CDT 2021, Marijn Suijten wrote: [..] > > > > + ret = lpg_init_lut(lpg); > > > > + if (ret < 0) > > > > + return ret; > > > > > > > > > How about turning these returns into dev_err_probe? I'm not sure if that's > > > the expected way to go nowadays, but having some form of logging when a > > > driver fails to probe is always good to have. > > > > > > > The intention is that each code path through these functions will either > > pass or spit out an error in the log. I looked through them again and > > think I cover all paths... > > > That is true, all the errors not covered are extremely unlikely like > -ENOMEM. I vaguely recall having to insert extra logging to get through > initial probe, but that might have been something inside lpg_add_led as > well. Fine to leave this as it is. > When kzalloc et al returns -ENOMEM it will be done with an error print, so that does not need an additional print. That said, another pass through lpg_add_led() made me spot that if you get a parse error on the "color" property we would silently return -EINVAL. I've corrected this. Thanks, Bjorn
Hi! > > > +static int lpg_add_pwm(struct lpg *lpg) > > > +{ > > > + int ret; > > > + > > > + lpg->pwm.base = -1; > > > + lpg->pwm.dev = lpg->dev; > > > + lpg->pwm.npwm = lpg->num_channels; > > > + lpg->pwm.ops = &lpg_pwm_ops; > > > + > > > + ret = pwmchip_add(&lpg->pwm); > > > + if (ret) > > > + dev_err(lpg->dev, "failed to add PWM chip: ret %d\n", ret); > > > + > > > + return ret; > > > +} > > > > Do we need to do this? I'd rather have LED driver, than LED+PWM > > driver... > > > > Yes, I believe we need to do this. > > Because each piece of hardware has N channels, which can be wired to > LEDs, grouped with other channels and wired to multicolor LEDs or be > used as PWM signals. And this configuration is board specific. > > One such example is the laptop in front of me, which has 3 channels > wired to an RGB LED and 1 channel wired as a backlight control signal > (i.e. using pwm-backlight). Another example is a devboard where the > 4 channels are wired to 4 LEDs. Ok, so this is actually important. In this case you should have PWM layer, exporting PWMs, and then rgb-LED driver that takes three of those PWMs and turns them into LED, no? And ... surprise ... that is likely to help other people, as LEDs connected to PWMs are quite common. Hmm.? If you can't do this for some reason, you should probably explain in the changelog, because this is going to be FAQ. Best regards, Pavel
On Thu 29 Apr 16:12 CDT 2021, Pavel Machek wrote: > Hi! > > > > > +static int lpg_add_pwm(struct lpg *lpg) > > > > +{ > > > > + int ret; > > > > + > > > > + lpg->pwm.base = -1; > > > > + lpg->pwm.dev = lpg->dev; > > > > + lpg->pwm.npwm = lpg->num_channels; > > > > + lpg->pwm.ops = &lpg_pwm_ops; > > > > + > > > > + ret = pwmchip_add(&lpg->pwm); > > > > + if (ret) > > > > + dev_err(lpg->dev, "failed to add PWM chip: ret %d\n", ret); > > > > + > > > > + return ret; > > > > +} > > > > > > Do we need to do this? I'd rather have LED driver, than LED+PWM > > > driver... > > > > > > > Yes, I believe we need to do this. > > > > Because each piece of hardware has N channels, which can be wired to > > LEDs, grouped with other channels and wired to multicolor LEDs or be > > used as PWM signals. And this configuration is board specific. > > > > One such example is the laptop in front of me, which has 3 channels > > wired to an RGB LED and 1 channel wired as a backlight control signal > > (i.e. using pwm-backlight). Another example is a devboard where the > > 4 channels are wired to 4 LEDs. > > Ok, so this is actually important. In this case you should have PWM > layer, exporting PWMs, and then rgb-LED driver that takes three of > those PWMs and turns them into LED, no? > > And ... surprise ... that is likely to help other people, as LEDs > connected to PWMs are quite common. > > Hmm.? > > If you can't do this for some reason, you should probably explain in > the changelog, because this is going to be FAQ. > This is exactly what the downstream implementation does and in the case of a solid color LED this works fine. But the hardware has a shared chunk of memory where you can write duty-cycle values, then for each PWM channel you can specify the start/stop index and pace for the PWM to read and update the configured duty-cycle. This is how the hardware implements pattern support. So downstream they have (last time I looked at the code) an addition in the PWM API where the LED driver can inform the PWM driver part about the indices to use. Naturally I don't think that's a good idea. Additionally, representing this as individual PWM channels means we're loosing the grouping that now comes from the description of multicolor LEDs, which serves the basis for synchronizing the pattern traversal between the involved channels. I just posted v7, but I'd be happy to incorporate such reasoning in the commit message (or do you really mean changelog in the email?) when we agree on a solution. Regards, Bjorn
Hi! > > > One such example is the laptop in front of me, which has 3 channels > > > wired to an RGB LED and 1 channel wired as a backlight control signal > > > (i.e. using pwm-backlight). Another example is a devboard where the > > > 4 channels are wired to 4 LEDs. > > > > Ok, so this is actually important. In this case you should have PWM > > layer, exporting PWMs, and then rgb-LED driver that takes three of > > those PWMs and turns them into LED, no? > > > > And ... surprise ... that is likely to help other people, as LEDs > > connected to PWMs are quite common. > > > > Hmm.? > > > > If you can't do this for some reason, you should probably explain in > > the changelog, because this is going to be FAQ. > > > > This is exactly what the downstream implementation does and in the case > of a solid color LED this works fine. > > But the hardware has a shared chunk of memory where you can write > duty-cycle values, then for each PWM channel you can specify the > start/stop index and pace for the PWM to read and update the configured > duty-cycle. This is how the hardware implements pattern support. Ok. > So downstream they have (last time I looked at the code) an addition in > the PWM API where the LED driver can inform the PWM driver part about > the indices to use. Naturally I don't think that's a good idea. Dunno. Is it bad idea? pattern support for other PWMs (vibration?) seems useful, too. Yes, it means more discussion and extending PWMs properly.. > Additionally, representing this as individual PWM channels means we're > loosing the grouping that now comes from the description of multicolor > LEDs, which serves the basis for synchronizing the pattern traversal > between the involved channels. Yes, keeping grouping would be nice, but perhaps pattern API for PWMs can do that too? You can have solid-color-only driver now, with patterns being added as discussion with PWM people progresses... Best regards, Pavel --
On Tue 04 May 10:43 CDT 2021, Pavel Machek wrote: > Hi! > > > > > One such example is the laptop in front of me, which has 3 channels > > > > wired to an RGB LED and 1 channel wired as a backlight control signal > > > > (i.e. using pwm-backlight). Another example is a devboard where the > > > > 4 channels are wired to 4 LEDs. > > > > > > Ok, so this is actually important. In this case you should have PWM > > > layer, exporting PWMs, and then rgb-LED driver that takes three of > > > those PWMs and turns them into LED, no? > > > > > > And ... surprise ... that is likely to help other people, as LEDs > > > connected to PWMs are quite common. > > > > > > Hmm.? > > > > > > If you can't do this for some reason, you should probably explain in > > > the changelog, because this is going to be FAQ. > > > > > > > This is exactly what the downstream implementation does and in the case > > of a solid color LED this works fine. > > > > But the hardware has a shared chunk of memory where you can write > > duty-cycle values, then for each PWM channel you can specify the > > start/stop index and pace for the PWM to read and update the configured > > duty-cycle. This is how the hardware implements pattern support. > > Ok. > > > So downstream they have (last time I looked at the code) an addition in > > the PWM API where the LED driver can inform the PWM driver part about > > the indices to use. Naturally I don't think that's a good idea. > > Dunno. Is it bad idea? > > pattern support for other PWMs (vibration?) seems useful, too. Yes, it > means more discussion and extending PWMs properly.. > @Thierry, @Lee, @Uwe, are you interested in extending the PWM api with some sort of support for specifying an array of "duty_cycle" and some property for how fast the hardware should cycle through those duty cycles? (And I need a bit/bool to indicate if the pattern should run once of be repeated) The (current) use case relates to being able to alter the duty cycle over time to create "effects" such as pulsing an LED. > > Additionally, representing this as individual PWM channels means we're > > loosing the grouping that now comes from the description of multicolor > > LEDs, which serves the basis for synchronizing the pattern traversal > > between the involved channels. > > Yes, keeping grouping would be nice, but perhaps pattern API for PWMs > can do that too? > I don't think it's viable to push this concept down to the PWM core, the LED framework needs to know which channels are grouped as LEDs (and which aren't) and the thing that drives the hardware needs to know which channels to sync with when starting the pattern. So afaict we'd have to describe these properties in two places - or ignore some properties of the problem. > You can have solid-color-only driver now, with patterns being added > as discussion with PWM people progresses... > I don't see how that would work, the overall design of the driver and the devicetree binding depends heavily on this fundamental decision. Regards, Bjorn
Hello Bjorn, On Wed, Oct 21, 2020 at 01:12:22PM -0700, Bjorn Andersson wrote: > +static const unsigned int lpg_clk_table[NUM_PWM_PREDIV][NUM_PWM_CLK] = { > + { > + 1 * (NSEC_PER_SEC / 1024), > + 1 * (NSEC_PER_SEC / 32768), > + 1 * (NSEC_PER_SEC / 19200000), > + }, > + { > + 3 * (NSEC_PER_SEC / 1024), > + 3 * (NSEC_PER_SEC / 32768), 1000000000 / 32768 is 30517.578125. Because of the parenthesis this is truncated to 30517. Multiplied by 3 this results in 91551. The exact result is 91552.734375 however. > + 3 * (NSEC_PER_SEC / 19200000), > + }, > + { > + 5 * (NSEC_PER_SEC / 1024), > + 5 * (NSEC_PER_SEC / 32768), > + 5 * (NSEC_PER_SEC / 19200000), > + }, > + { > + 6 * (NSEC_PER_SEC / 1024), > + 6 * (NSEC_PER_SEC / 32768), > + 6 * (NSEC_PER_SEC / 19200000), > + }, > +}; > + > +/* > + * PWM Frequency = Clock Frequency / (N * T) > + * or > + * PWM Period = Clock Period * (N * T) > + * where > + * N = 2^9 or 2^6 for 9-bit or 6-bit PWM size > + * T = Pre-divide * 2^m, where m = 0..7 (exponent) > + * > + * This is the formula to figure out m for the best pre-divide and clock: > + * (PWM Period / N) = (Pre-divide * Clock Period) * 2^m > + */ > +static void lpg_calc_freq(struct lpg_channel *chan, unsigned int period_us) > +{ > + int n, m, clk, div; > + int best_m, best_div, best_clk; > + unsigned int last_err, cur_err, min_err; > + unsigned int tmp_p, period_n; > + > + if (period_us == chan->period_us) > + return; > + > + /* PWM Period / N */ > + if (period_us < UINT_MAX / NSEC_PER_USEC) > + n = 6; > + else > + n = 9; > + > + period_n = ((u64)period_us * NSEC_PER_USEC) >> n; > + > + min_err = UINT_MAX; > + last_err = UINT_MAX; > + best_m = 0; > + best_clk = 0; > + best_div = 0; > + for (clk = 0; clk < NUM_PWM_CLK; clk++) { > + for (div = 0; div < NUM_PWM_PREDIV; div++) { > + /* period_n = (PWM Period / N) */ > + /* tmp_p = (Pre-divide * Clock Period) * 2^m */ > + tmp_p = lpg_clk_table[div][clk]; > + for (m = 0; m <= NUM_EXP; m++) { > + cur_err = abs(period_n - tmp_p); > + if (cur_err < min_err) { > + min_err = cur_err; > + best_m = m; > + best_clk = clk; > + best_div = div; > + } > + > + if (m && cur_err > last_err) > + /* Break for bigger cur_err */ > + break; > + > + last_err = cur_err; > + tmp_p <<= 1; This is inexact. Consider again the case where tmp_p is 3 * (NSEC_PER_SEC / 32768). The values you use and the exact values are: m | 0 | 1 | 2 | 3 | ... | 7 | tmp_p | 91551 | 183102 | 366204 | 732408 | | 11718528 | actual| 91552.734375 | 183105.46875 | 366210.9375 | 732421.875 | ... | 11718750 | So while you save some cycles by precalculating the values in lpg_clk_table, you trade that for lost precision. > + } > + } > + } Please don't pick a period that is longer than the requested period (for the PWM functionality that is). This can be simplified, you can at least calculate the optimal m directly. > + /* Use higher resolution */ > + if (best_m >= 3 && n == 6) { > + n += 3; > + best_m -= 3; > + } > + > + chan->clk = best_clk; > + chan->pre_div = best_div; > + chan->pre_div_exp = best_m; > + chan->pwm_size = n; > + > + chan->period_us = period_us; > +} > + > +static void lpg_calc_duty(struct lpg_channel *chan, unsigned int duty_us) > +{ > + unsigned int max = (1 << chan->pwm_size) - 1; > + unsigned int val = div_u64((u64)duty_us << chan->pwm_size, chan->period_us); Please use the actually implemented period here instead of the requested. This improves precision, see commit 8035e6c66a5e98f098edf7441667de74affb4e78 for a similar case. > + > + chan->pwm_value = min(val, max); > +} > + > [...] > +static const struct pwm_ops lpg_pwm_ops = { > + .request = lpg_pwm_request, > + .apply = lpg_pwm_apply, Can you please test your driver with PWM_DEBUG enabled? The first thing this will critizise is that there is no .get_state callback. > + .owner = THIS_MODULE, > +}; > + > +static int lpg_add_pwm(struct lpg *lpg) > +{ > + int ret; > + > + lpg->pwm.base = -1; Please drop this assignment. > + lpg->pwm.dev = lpg->dev; > + lpg->pwm.npwm = lpg->num_channels; > + lpg->pwm.ops = &lpg_pwm_ops; > + > + ret = pwmchip_add(&lpg->pwm); > + if (ret) > + dev_err(lpg->dev, "failed to add PWM chip: ret %d\n", ret); > + > + return ret; > +} Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Hello again, another feedback that only came to me after hitting "Send": It might be sensible to split the patch. In the first part add the LED stuff and in the second the PWM stuff. It would also be good if the PWM code could live in drivers/pwm. Best regards Uwe