lkml.org 
[lkml]   [2020]   [Oct]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v5 2/4] leds: Add driver for Qualcomm LPG
    On Sun 18 Oct 15:12 CDT 2020, Andy Shevchenko wrote:

    > On Sat, Oct 17, 2020 at 8:41 AM Bjorn Andersson
    > <bjorn.andersson@linaro.org> 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.
    >
    > > +config LEDS_QCOM_LPG
    > > + tristate "LED support for Qualcomm LPG"
    > > + depends on LEDS_CLASS_MULTICOLOR
    > > + depends on OF
    > > + depends on SPMI
    >
    >
    > > +#include <linux/of.h>
    > > +#include <linux/of_device.h>
    >
    > ...
    >
    > > +struct lpg {
    > > + struct device *dev;
    > > + struct regmap *map;
    >
    > Can't you derive the former from the latter?
    >

    No, because map->dev is actually the dev->parent.

    > > +
    > > + struct pwm_chip pwm;
    > > +
    > > + const struct lpg_data *data;
    > > +
    > > + u32 lut_base;
    > > + u32 lut_size;
    > > + unsigned long *lut_bitmap;
    > > +
    > > + u32 triled_base;
    > > + u32 triled_src;
    > > +
    > > + struct lpg_channel *channels;
    > > + unsigned int num_channels;
    > > +};
    >
    > ...
    >
    > > +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;
    > > + u8 val[2];
    >
    > __be16 val;
    >
    > > + int i;
    > > +
    > > + /* Hardware does not behave when LO_IDX == HI_IDX */
    > > + if (len == 1)
    > > + return -EINVAL;
    > > +
    > > + idx = bitmap_find_next_zero_area(lpg->lut_bitmap, lpg->lut_size,
    > > + 0, len, 0);
    > > + if (idx >= lpg->lut_size)
    > > + return -ENOMEM;
    > > +
    > > + for (i = 0; i < len; i++) {
    > > + val[0] = pattern[i].brightness & 0xff;
    > > + val[1] = pattern[i].brightness >> 8;
    >
    > cpu_to_be16();
    >

    I like it, but isn't that a le16?

    > > +
    > > + regmap_bulk_write(lpg->map,
    > > + lpg->lut_base + LPG_LUT_REG(idx + i), val, 2);
    > > + }
    > > +
    > > + bitmap_set(lpg->lut_bitmap, idx, len);
    > > +
    > > + *lo_idx = idx;
    > > + *hi_idx = idx + len - 1;
    > > +
    > > + return 0;
    > > +}
    >
    > ...
    >
    > > +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 < ((unsigned int)(-1) / NSEC_PER_USEC)) {
    >
    > Please, replace all these -1 with castings to unsigned types with
    > corresponding limits, like
    > UINT_MAX here.
    >

    Sure thing.

    > > + period_n = (period_us * NSEC_PER_USEC) >> 6;
    > > + n = 6;
    > > + } else {
    > > + period_n = (period_us >> 9) * NSEC_PER_USEC;
    > > + n = 9;
    > > + }
    >
    > Why inconsistency in branches? Can you rather derive n and calculate
    > only once like
    >
    > period_n = (period_us >> n) * NSEC_PER_USEC;
    >
    > ?

    I inherited this piece from the downstream driver and I assume that the
    purpose was to avoid loss of precision. I will review this and if
    nothing else it seems like I would be able to cast period_us to more
    bits, do the multiply and then shift - in both cases.

    >
    > > + min_err = (unsigned int)(-1);
    > > + last_err = (unsigned int)(-1);
    > > + 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++) {
    > > + if (period_n > tmp_p)
    > > + cur_err = period_n - tmp_p;
    > > + else
    > > + cur_err = tmp_p - period_n;
    > > +
    > > + 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;
    > > + }
    > > + }
    > > + }
    > > +
    > > + /* 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 long max = (1 << chan->pwm_size) - 1;
    >
    > BIT() ?
    >
    > > + unsigned long val;
    > > +
    > > + /* Figure out pwm_value with overflow handling */
    >
    > > + if (duty_us < 1 << (sizeof(val) * 8 - chan->pwm_size))
    >
    > BITS_PER_TYPE, but actually BITS_PER_LONG here.
    >
    > BIT(BITS_PER_LONG - ...)
    >

    Again, this seems to just be a question of avoiding overflow of the 32
    bit duty_us. I'll double check the math here as well.

    > > + val = (duty_us << chan->pwm_size) / chan->period_us;
    > > + else
    > > + val = duty_us / (chan->period_us >> chan->pwm_size);
    > > +
    > > + if (val > max)
    > > + val = max;
    > > +
    > > + chan->pwm_value = val;
    > > +}
    >
    > ...
    >
    > > +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);
    > > +}
    >
    > Here and everywhere else when function declared as void there is no
    > possibility to know if we do something useful or already screwed up
    > the things.
    >

    Ultimately these are all coming from led_classdev->brightness_set, which
    is a void function. So there isn't much benefit of propagating this
    error upwards, today.

    > > +static void lpg_apply_pwm_value(struct lpg_channel *chan)
    > > +{
    > > + u8 val[] = { chan->pwm_value & 0xff, chan->pwm_value >> 8 };
    >
    >
    > __le16 and cpu_to_le16()
    >

    Sounds good.

    > > + struct lpg *lpg = chan->lpg;
    > > +
    > > + if (!chan->enabled)
    > > + return;
    > > +
    > > + regmap_bulk_write(lpg->map, chan->base + PWM_VALUE_REG, val, 2);
    > > +}
    >
    > > +#define LPG_PATTERN_CONFIG_LO_TO_HI BIT(4)
    > > +#define LPG_PATTERN_CONFIG_REPEAT BIT(3)
    > > +#define LPG_PATTERN_CONFIG_TOGGLE BIT(2)
    > > +#define LPG_PATTERN_CONFIG_PAUSE_HI BIT(1)
    > > +#define LPG_PATTERN_CONFIG_PAUSE_LO BIT(0)
    >
    > Did I miss bits.h inclusion at the beginning of the file?
    >

    Will add this.

    > ...
    >
    > > +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;
    > > + }
    >
    > Homegrown duty cycle?
    > I mean, why simply not to pass the duty cycle in percentage in the first place?
    >

    Can you explain what you're saying here.

    > > + duty_us = delay_on * USEC_PER_MSEC;
    > > + period_us = (delay_on + delay_off) * USEC_PER_MSEC;
    > > +
    > > + for (i = 0; i < led->num_channels; i++) {
    > > + chan = led->channels[i];
    > > +
    > > + lpg_calc_freq(chan, period_us);
    > > + lpg_calc_duty(chan, duty_us);
    > > +
    > > + chan->enabled = true;
    > > + chan->ramp_enabled = false;
    > > +
    > > + lpg_apply(chan);
    > > + }
    > > +
    > > + return 0;
    > > +}
    >
    > > +#define interpolate(x1, y1, x2, y2, x) \
    > > + ((y1) + ((y2) - (y1)) * ((x) - (x1)) / ((x2) - (x1)))
    >
    > Can you rather create a generic one under lib/ or start include/linux/math.h ?
    >

    Forgot about this, but I've seen one on LKML, will find it and work on
    getting that accepted.

    > https://elixir.bootlin.com/linux/latest/A/ident/interpolate
    >
    > ...
    >
    > > +out:
    >
    > Useless label.
    >
    > > + return ret;
    >
    > ...
    >
    > > + ret = of_property_read_u32(np, "color", &color);
    > > + if (ret < 0 && ret != -EINVAL)
    >
    > This check is fishy. Either you have optional property or not, in the
    > latter case return any error code.
    >

    There's three possible outcomes here:
    1) We found _one_ integer in the property, color is assigned and 0 is
    returned.
    2) We found no property named "color", -EINVAL is returned without color
    being modified.
    3) We found a property but it wasn't a single u32 value so a negative
    error (not EINVAL) is returned.

    > > + return ret;
    > > +
    > > + chan->color = color;
    >
    > So, it may be -EINVAL?!
    >

    So color will either be the value or the property color, or if omitted
    LED_COLOR_ID_GREEN.

    > > + ret = of_property_read_u32_array(np, "qcom,dtest", dtest, 2);
    > > + if (ret < 0 && ret != -EINVAL) {
    > > + dev_err(lpg->dev, "malformed qcom,dtest of %pOFn\n", np);
    > > + return ret;
    > > + } else if (!ret) {
    > > + chan->dtest_line = dtest[0];
    > > + chan->dtest_value = dtest[1];
    > > + }
    >
    > Ditto.
    >

    We're in !ret and as such dtest is initialized.

    > ...
    >
    > > + ret = of_property_read_u32(np, "color", &color);
    > > + if (ret < 0 && ret != -EINVAL)
    > > + return ret;
    >
    > Ditto.
    >

    As above, if no property color is specified, color remains 0 here which
    is not LED_COLOR_ID_MULTI and this is a single channel LED without its
    color specified.

    > ...
    >
    > > + size = sizeof(*led) + num_channels * sizeof(struct lpg_channel *);
    >
    > Use respective helpers from overflow.h.
    >

    Will do, thanks.

    > > + led = devm_kzalloc(lpg->dev, size, GFP_KERNEL);
    > > + if (!led)
    > > + return -ENOMEM;
    >
    > ...
    >
    > > +static const struct of_device_id lpg_of_table[] = {
    > > + { .compatible = "qcom,pm8916-pwm", .data = &pm8916_pwm_data },
    > > + { .compatible = "qcom,pm8941-lpg", .data = &pm8941_lpg_data },
    > > + { .compatible = "qcom,pm8994-lpg", .data = &pm8994_lpg_data },
    > > + { .compatible = "qcom,pmi8994-lpg", .data = &pmi8994_lpg_data },
    > > + { .compatible = "qcom,pmi8998-lpg", .data = &pmi8998_lpg_data },
    >
    > > + {},
    >
    > No comma needed for terminator lines.
    >

    Will drop.

    > > +};

    Thank you Andy for the review!

    Regards,
    Bjorn

    \
     
     \ /
      Last update: 2020-10-20 06:30    [W:3.551 / U:0.052 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site