[v6,2/4] leds: Add driver for Qualcomm LPG

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

Hi Bjorn,

On Mittwoch, 21. Oktober 2020 22:12:22 CEST 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>

> ---

> 

> 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


Tested on msm8974 (pm8941) on the Fairphone 2, works great there!

Tested-by: Luca Weiss <luca@z3ntu.xyz>


Regards
Luca

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.


> +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.

> +#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...

> +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?


> +	ret = lpg_lut_store(lpg, pattern, len, &lo_idx, &hi_idx);
> +	if (ret < 0)
> +		goto out;

Just do direct return.

> +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...

Best regards,
							Pavel

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
-- 
http://www.livejournal.com/~pavelmachek

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

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Hello,

On Tue, May 04, 2021 at 11:13:27AM -0500, Bjorn Andersson wrote:
> On Tue 04 May 10:43 CDT 2021, Pavel Machek wrote:

> > > 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.


My personal opinion here is that this is too special to be worth the
generalisation.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

On Wed 05 May 00:15 CDT 2021, Uwe Kleine-K?nig wrote:

> Hello Bjorn,

> 


Thanks for your feedback, and the input on extending the PWM api related
to patterns. I'll revisit the calculations, and PWM_DEBUG accordingly.

Regards,
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/ |