Re: [PATCH v3 2/3] leds: pwm: add support for default-state device property

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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/




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux