Re: [PATCH v3 2/3] leds: pwm: add support for default-state device property
From: Denis Osterland-Heim
Date: Thu Mar 19 2020 - 11:59:56 EST
Hi Jacek,
I have testet with a i.MX6 custom board.
It is possible to use qemu to test it.
Using the smdkc210 machine with modified device tree:
```patch
diff --git a/arch/arm/boot/dts/exynos4210-trats.dts b/arch/arm/boot/dts/exynos4210-trats.dts
index 7c39dd1c4d3a..9ca10f69b3a2 100644
--- a/arch/arm/boot/dts/exynos4210-trats.dts
+++ b/arch/arm/boot/dts/exynos4210-trats.dts
@@ -26,8 +26,8 @@
};
chosen {
- bootargs = "root=/dev/mmcblk0p5 rootwait earlyprintk panic=5";
- stdout-path = "serial2:115200n8";
+ bootargs = "console=ttySAC0 rdinit=/sbin/init root=/dev/mmcblk0p5 rootwait earlyprintk panic=5";
+ stdout-path = "serial0:115200n8";
};
regulators {
@@ -124,7 +124,7 @@
fixed-rate-clocks {
xxti {
compatible = "samsung,clock-xxti";
- clock-frequency = <0>;
+ clock-frequency = <24000000>;
};
xusbxti {
@@ -148,12 +148,27 @@
};
};
+ pwm-leds {
+ compatible = "pwm-leds";
+ led {
+ label = "led";
+ pwms = <&pwm 0 1000000000 0>;
+ max-brightness = <255>;
+ default-state = "keep";
+ };
+ };
+
+};
+
+&pwm {
+ status = "okay";
+ samsung,pwm-outputs = <0>;
};
&camera {
pinctrl-names = "default";
pinctrl-0 = <>;
- status = "okay";
+ status = "disabled";
};
&cpu0 {
@@ -166,7 +181,7 @@
samsung,burst-clock-frequency = <500000000>;
samsung,esc-clock-frequency = <20000000>;
samsung,pll-clock-frequency = <24000000>;
- status = "okay";
+ status = "disabled";
panel@0 {
reg = <0>;
@@ -196,15 +211,16 @@
};
};
};
+
};
&exynos_usbphy {
- status = "okay";
+ status = "disabled";
vbus-supply = <&safe1_sreg>;
};
&fimc_0 {
- status = "okay";
+ status = "disabled";
assigned-clocks = <&clock CLK_MOUT_FIMC0>,
<&clock CLK_SCLK_FIMC0>;
assigned-clock-parents = <&clock CLK_SCLK_MPLL>;
@@ -212,7 +228,7 @@
};
&fimc_1 {
- status = "okay";
+ status = "disabled";
assigned-clocks = <&clock CLK_MOUT_FIMC1>,
<&clock CLK_SCLK_FIMC1>;
assigned-clock-parents = <&clock CLK_SCLK_MPLL>;
@@ -220,7 +236,7 @@
};
&fimc_2 {
- status = "okay";
+ status = "disabled";
assigned-clocks = <&clock CLK_MOUT_FIMC2>,
<&clock CLK_SCLK_FIMC2>;
assigned-clock-parents = <&clock CLK_SCLK_MPLL>;
@@ -228,7 +244,7 @@
};
&fimc_3 {
- status = "okay";
+ status = "disabled";
assigned-clocks = <&clock CLK_MOUT_FIMC3>,
<&clock CLK_SCLK_FIMC3>;
assigned-clock-parents = <&clock CLK_SCLK_MPLL>;
@@ -236,18 +252,18 @@
};
&fimd {
- status = "okay";
+ status = "disabled";
};
&gpu {
- status = "okay";
+ status = "disabled";
};
&hsotg {
vusb_d-supply = <&vusb_reg>;
vusb_a-supply = <&vusbdac_reg>;
dr_mode = "peripheral";
- status = "okay";
+ status = "disabled";
};
&i2c_3 {
@@ -256,7 +272,7 @@
samsung,i2c-max-bus-freq = <400000>;
pinctrl-0 = <&i2c3_bus>;
pinctrl-names = "default";
- status = "okay";
+ status = "disabled";
mms114-touchscreen@48 {
compatible = "melfas,mms114";
@@ -276,7 +292,7 @@
samsung,i2c-max-bus-freq = <100000>;
pinctrl-0 = <&i2c5_bus>;
pinctrl-names = "default";
- status = "okay";
+ status = "disabled";
max8997_pmic@66 {
compatible = "maxim,max8997-pmic";
```
To actually see what happens on HW you need to patch qemu as well:
```patch
diff --git a/hw/timer/exynos4210_pwm.c b/hw/timer/exynos4210_pwm.c
index 9bc03277852d..a1c2bc05b935 100644
--- a/hw/timer/exynos4210_pwm.c
+++ b/hw/timer/exynos4210_pwm.c
@@ -30,7 +30,7 @@
#include "hw/arm/exynos4210.h"
-//#define DEBUG_PWM
+#define DEBUG_PWM
#ifdef DEBUG_PWM
#define DPRINTF(fmt, ...) \
@@ -199,8 +199,8 @@ static void exynos4210_pwm_tick(void *opaque)
}
if (cmp) {
- DPRINTF("auto reload timer %d count to %x\n", id,
- p->timer[id].reg_tcntb);
+ DPRINTF("auto reload timer %d count to 0x%08x and compare to 0x%08x\n", id,
+ p->timer[id].reg_tcntb, p->timer[id].reg_tcmpb);
ptimer_set_count(p->timer[id].ptimer, p->timer[id].reg_tcntb);
ptimer_run(p->timer[id].ptimer, 1);
} else {
```
With this changes you can test on and off default-state.
The on state will print:
PWM: [ exynos4210_pwm_tick: 202] auto reload timer 0 count to 0x00b7d73f and compare to 0xffffffff
The off state only checks that it is disabled.
To test the default-state keep, more changes are needed:
```patch
diff --git a/hw/timer/exynos4210_pwm.c b/hw/timer/exynos4210_pwm.c
index 9bc03277852d..a1c2bc05b935 100644
--- a/hw/timer/exynos4210_pwm.c
+++ b/hw/timer/exynos4210_pwm.c
@@ -370,6 +370,12 @@ static void exynos4210_pwm_reset(DeviceState *d)
exynos4210_pwm_update_freq(s, s->timer[i].id);
ptimer_stop(s->timer[i].ptimer);
}
+
+ exynos4210_pwm_write(s, TCON, 4, 4);
+ exynos4210_pwm_write(s, TCNTB0, 0x00b7d73f, 4);
+ exynos4210_pwm_write(s, TCMPB0, 0x005b8f57, 4);
+ exynos4210_pwm_write(s, TCON, 6, 4);
+ exynos4210_pwm_write(s, TCON, 0xd, 4);
}
static const MemoryRegionOps exynos4210_pwm_ops = {
```
```patch
diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c
index 87a886f7dc2f..cf578a235208 100644
--- a/drivers/pwm/pwm-samsung.c
+++ b/drivers/pwm/pwm-samsung.c
@@ -387,6 +387,23 @@ static int pwm_samsung_config(struct pwm_chip *chip, struct pwm_device *pwm,
return __pwm_samsung_config(chip, pwm, duty_ns, period_ns, false);
}
+static void pwm_samsung_get_state(struct pwm_chip *chip,
+ struct pwm_device *pwm, struct pwm_state *state)
+{
+ struct samsung_pwm_chip *our_chip = to_samsung_pwm_chip(chip);
+ u32 tin_rate = pwm_samsung_get_tin_rate(our_chip, pwm->hwpwm);
+ u32 tcnt = readl(our_chip->base + REG_TCNTB(pwm->hwpwm));
+ u32 tcmp = readl(our_chip->base + REG_TCMPB(pwm->hwpwm));
+ u32 tcon = readl(our_chip->base + REG_TCON);
+ u64 tmp;
+
+ state->enabled = !!(tcon & TCON_AUTORELOAD(pwm->hwpwm));
+ tmp = NSEC_PER_SEC * (u64)tcmp;
+ state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, tin_rate);
+ tmp = NSEC_PER_SEC * (u64)tcnt;
+ state->period = DIV_ROUND_CLOSEST_ULL(tmp, tin_rate);
+}
+
static void pwm_samsung_set_invert(struct samsung_pwm_chip *chip,
unsigned int channel, bool invert)
{
@@ -430,6 +447,7 @@ static const struct pwm_ops pwm_samsung_ops = {
.enable = pwm_samsung_enable,
.disable = pwm_samsung_disable,
.config = pwm_samsung_config,
+ .get_state = pwm_samsung_get_state,
.set_polarity = pwm_samsung_set_polarity,
.owner = THIS_MODULE,
};
```
Without the get_state() a divide by zero is triggered and device is not there.
I think it is valid to use default-state keep with a disabled PWM
and this may cause a division by zero, I will add a `if (pwmstate->enabled)`
to avoid this.
Regards Denis
Am Dienstag, den 17.03.2020, 21:43 +0100 schrieb Jacek Anaszewski:
> Hi Denis,
>
> On 3/16/20 9:24 PM, Denis Osterland-Heim wrote:
> > Hi Jacek,
> >
> > Am Montag, den 16.03.2020, 19:36 +0100 schrieb Jacek Anaszewski:
> > > Hi Denis,
> > >
> > > On 3/16/20 1:53 PM, Denis Osterland-Heim wrote:
> >
> > ...
> > > >
> > > > @@ -92,13 +96,27 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
> > > >
> > > > pwm_init_state(led_data->pwm, &led_data->pwmstate);
> > > >
> > > > + if (led->default_state == LEDS_PWM_DEFSTATE_ON)
> > > > + led_data->cdev.brightness = led->max_brightness;
> > > > + else if (led->default_state == LEDS_PWM_DEFSTATE_KEEP) {
> > > > + uint64_t brightness;
> > > > +
> > > > + pwm_get_state(led_data->pwm, &led_data->pwmstate);
> > >
> > > This seems to not be reading the device state, i.e. what you tried
> > > to address by direct call to pwm->chip->ops->get_state() before.
> > >
> > > Am I missing something?
> > >
> >
> > well, not you, but I missed cfc4c189bc70b1acc17e6f1abf1dc1c0ae890bd8.
> > Since this commit pwm_get_state() is sufficient.
>
> I assume you tested it?
>
> With that, for the whole set:
>
> Acked-by: Jacek Anaszewski <jacek.anaszewski@xxxxxxxxx>
>
Diehl Connectivity Solutions GmbH
GeschÃftsfÃhrung: Horst Leonberger
Sitz der Gesellschaft: NÃrnberg - Registergericht: Amtsgericht
NÃrnberg: HRB 32315
___________________________________________________________________________________________________
Der Inhalt der vorstehenden E-Mail ist nicht rechtlich bindend. Diese E-Mail enthaelt vertrauliche und/oder rechtlich geschuetzte Informationen.
Informieren Sie uns bitte, wenn Sie diese E-Mail faelschlicherweise erhalten haben. Bitte loeschen Sie in diesem Fall die Nachricht.
Jede unerlaubte Form der Reproduktion, Bekanntgabe, Aenderung, Verteilung und/oder Publikation dieser E-Mail ist strengstens untersagt.
- Informationen zum Datenschutz, insbesondere zu Ihren Rechten, erhalten Sie unter https://www.diehl.com/group/de/transparenz-und-informationspflichten/
The contents of the above mentioned e-mail is not legally binding. This e-mail contains confidential and/or legally protected information. Please inform us if you have received this e-mail by
mistake and delete it in such a case. Each unauthorized reproduction, disclosure, alteration, distribution and/or publication of this e-mail is strictly prohibited.
- For general information on data protection and your respective rights please visit https://www.diehl.com/group/en/transparency-and-information-obligations/