From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ej1-f52.google.com (mail-ej1-f52.google.com [209.85.218.52]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 57AED1D5AD8 for ; Thu, 27 Feb 2025 17:05:19 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.218.52 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1740675921; cv=none; b=RcOEe22LLyY1jYUONRTRHYUq88tCTu2WR35T4nsW/ma13E0qOLtluRaOi6WXKkMaAMqAR/6TYqlLsseiza48V8rZUQ/V6WK+l2otZmyNkOrvPa5NTd7+3f3eosB3W8BcjQwbk3v7xHOqCKT/Ye7bB2QSYfKbad4Of7kBzyX8yuY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1740675921; c=relaxed/simple; bh=7XChtsa0NQK2jNQqByJFv42QLQZfG/59qbf5QMzlocg=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=rB+qeBcXSbKTRfiIX+bXxw7y8mXdXGejtHhB/F0iNN1fYBRqE8Fx6BTBaOCgUhgqWrpRjp213sATQ+1F5D/AmBDrW/Mre89tRCHA0TgLMPqbPe6Eb/MfINJ9RhHyZzZ13KHGKrStzJ1kUOo/Qb8co9WOyr3KDVAO/3k0zbdHGio= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=Ww5FgaNi; arc=none smtp.client-ip=209.85.218.52 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="Ww5FgaNi" Received: by mail-ej1-f52.google.com with SMTP id a640c23a62f3a-aaec111762bso200544966b.2 for ; Thu, 27 Feb 2025 09:05:19 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1740675917; x=1741280717; darn=vger.kernel.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=K6Y8JQULavsNlLpEQIX3xqjGtKxhdLW9C1hzs6N6L9o=; b=Ww5FgaNiNbH0Z21miuSO3vXT1jgydSjnpMGuH+EJC4ev26qbHHcfOQ2waCoRyE8x9g AU6pQ8lNE7Sj//4JCzvkHec91jMaGPoemEfPZWuPZquRo0SDyKAnHH6EA/oZAoTp85HR AI33fY7zKxiDKiLCPd4uqQxpi/+kLXMFrLCSb9g+OnZchauEowdwQSTZTLAU2p+ywsDH zxzvjImMyqFlcachADC2ftJpqkulwVUNLbJ9fN44EysKwGI7IiaCjUbZjzFLth0Z4g7a QiaSzvuOQqaBjGVFf57Jf6rSxNxe8FEuNjikpQa5TQVvjpIUTMVG8Skxaj71No3aGgqa wUTw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1740675917; x=1741280717; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=K6Y8JQULavsNlLpEQIX3xqjGtKxhdLW9C1hzs6N6L9o=; b=hgdTUQPaYu4V+bMxDFvZRE+H6a6+Xio2xbgtmmYN8/efWKJ7+I6C+oBKQCDbQEMFq0 YPrqwAUAfR0Mmbao6IUXlgpizCAOc6QvVhLSqinSor5CA0r8i06kfHpyrK4Q7HhTRWz4 P64vBnMR8G9m9LV31squfe98lkfm1intJbzypeX9c55a+609GbwSBpw+93O44Yrd4has ksQI8dWIQ3zCFxAEGv+wUrMT1P20fO3NQFUGfOj3zGofnSlWaRJkKkBNT5WyQ3CBHqwX H+1dwIu8MNwMl6RLhhYG3Fu6QAw9NFuz5yQIv9TTV+nDeJQ2mUW/lRk8h1Wz2BiVwGnX 5Brw== X-Forwarded-Encrypted: i=1; AJvYcCWdg9ms3Vt6/xq2hsGgXD8xKr2V3/lPLwfwsvVfxc3on7S1Adc36aVysusWiFh+8AXKdsxliEJCLpxTssM=@vger.kernel.org X-Gm-Message-State: AOJu0YyPXI1oA1aQa444h+zOV0YkcdNHIJIYYUleWbEyL9YRi0PJfzeg uwk/nBluE67oEISlIb2HZBkLkKLB8C1SAN2Qp8ePEVpOmfCBe3ZGMyg3dvyvfIU= X-Gm-Gg: ASbGnctqRhu/e4FByAIRG4vF6IWUBBD0FIiSEr+mflUz3eU2zyrJOLL1/D6+E9Cyav4 2QxiXJtD0CdJm7Mp3lPA/v1gc1vAVWn4s4bIOTgPmVEuqmZYbCptTkS883s14vqI7PnPRh35+lB igpkyUBGOJFYZoyg0CQtnhZdsvGP8ED9A7nWI88cbVbtZb4TERyXwNpdDpFW66J2GUetLmzAndm N5FNjww7Ta33woxBv4rwWe4rv5o6SRxGaTRN4jNi6ZHvDv0gQx7KlBbJTJ0t7uYUlUD0vyyE38G YVAFnz4Dhh/vdZ+WiBtu+rc= X-Google-Smtp-Source: AGHT+IEqzm7n+9CgLAtGOBByJjji4KY3D/kBRSRdTZK29CzMh0eKvJAC98wBTDUbj6GZHbvPXAzgOQ== X-Received: by 2002:a17:906:d542:b0:ab7:beeb:d1f1 with SMTP id a640c23a62f3a-abf26837fcemr20886166b.51.1740675916853; Thu, 27 Feb 2025 09:05:16 -0800 (PST) Received: from linaro.org ([62.231.96.41]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-abf0c0b9990sm150354966b.35.2025.02.27.09.05.15 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 27 Feb 2025 09:05:16 -0800 (PST) Date: Thu, 27 Feb 2025 19:05:14 +0200 From: Abel Vesa To: Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= Cc: Sebastian Reichel , Lee Jones , Pavel Machek , Anjelique Melendez , Kamal Wadhwa , Jishnu Prakash , Bjorn Andersson , Konrad Dybcio , Johan Hovold , Pavel Machek , linux-leds@vger.kernel.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org Subject: Re: [PATCH] leds: rgb: leds-qcom-lpg: Fix pwm resolution for Hi-Res PWMs Message-ID: References: <20250220-leds-qcom-lpg-fix-max-pwm-on-hi-res-v1-1-a161ec670ea5@linaro.org> <5euqboshlfwweie7tlaffajzg3siiy6bm3j4evr572ko54gtbv@7lan3vizskt3> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: On 25-02-27 18:32:41, Abel Vesa wrote: > On 25-02-27 17:44:35, Abel Vesa wrote: > > On 25-02-27 16:25:06, Uwe Kleine-König wrote: > > > Hello Abel, > > > > > > On Thu, Feb 27, 2025 at 04:26:14PM +0200, Abel Vesa wrote: > > > > On 25-02-27 10:58:47, Uwe Kleine-König wrote: > > > > > Can you please enable CONFIG_PWM_DEBUG, enable pwm tracing ( > > > > > > > > > > echo 1 > /sys/kernel/debug/tracing/events/pwm/enable > > > > > > > > > > ) then reproduce the problem and provide the output of > > > > > > > > > > cat /sys/kernel/debug/tracing/trace > > > > > > > > > > . > > > > > > > > $ cat trace > > > > # tracer: nop > > > > # > > > > # entries-in-buffer/entries-written: 13/13 #P:12 > > > > # > > > > # _-----=> irqs-off/BH-disabled > > > > # / _----=> need-resched > > > > # | / _---=> hardirq/softirq > > > > # || / _--=> preempt-depth > > > > # ||| / _-=> migrate-disable > > > > # |||| / delay > > > > # TASK-PID CPU# ||||| TIMESTAMP FUNCTION > > > > # | | | ||||| | | > > > > modprobe-203 [000] ..... 0.938668: pwm_get: pwmchip0.0: period=1066407 duty_cycle=533334 polarity=0 enabled=1 err=0 > > > > modprobe-203 [000] ..... 0.938775: pwm_apply: pwmchip0.0: period=5000000 duty_cycle=0 polarity=0 enabled=1 err=0 > > > > modprobe-203 [000] ..... 0.938821: pwm_get: pwmchip0.0: period=4266537 duty_cycle=0 polarity=0 enabled=1 err=0 > > > > modprobe-203 [000] ..... 0.938936: pwm_apply: pwmchip0.0: period=4266537 duty_cycle=0 polarity=0 enabled=1 err=0 > > > > modprobe-203 [000] ..... 0.938982: pwm_get: pwmchip0.0: period=4266537 duty_cycle=0 polarity=0 enabled=1 err=0 > > > > modprobe-203 [000] ..... 0.939274: pwm_apply: pwmchip0.0: period=5000000 duty_cycle=921458 polarity=0 enabled=1 err=0 > > > > modprobe-203 [000] ..... 0.939320: pwm_get: pwmchip0.0: period=4266537 duty_cycle=921355 polarity=0 enabled=1 err=0 > > > > modprobe-203 [000] ..... 0.939434: pwm_apply: pwmchip0.0: period=4266537 duty_cycle=921355 polarity=0 enabled=1 err=0 > > > > modprobe-203 [000] ..... 0.939480: pwm_get: pwmchip0.0: period=4266537 duty_cycle=921355 polarity=0 enabled=1 err=0 > > > > systemd-backlig-724 [006] ..... 9.079538: pwm_apply: pwmchip0.0: period=5000000 duty_cycle=5000000 polarity=0 enabled=1 err=0 > > > > systemd-backlig-724 [006] ..... 9.079585: pwm_get: pwmchip0.0: period=4266537 duty_cycle=4266537 polarity=0 enabled=1 err=0 > > > > systemd-backlig-724 [006] ..... 9.079698: pwm_apply: pwmchip0.0: period=4266537 duty_cycle=4266537 polarity=0 enabled=1 err=0 > > > > systemd-backlig-724 [006] ..... 9.079750: pwm_get: pwmchip0.0: period=4266537 duty_cycle=4266537 polarity=0 enabled=1 err=0 > > > > $ > > > > > > > > > > > > > > I didn't take a deeper dive in this driver combination, but here is a > > > > > description about what *should* happen: > > > > > > > > > > You're talking about period in MHz, the PWM abstraction uses > > > > > nanoseconds. So your summary translated to the PWM wording is (to the > > > > > best of my understanding): > > > > > > > > > > 1. PWM backlight driver requests PWM with .period = 200 ns and > > > > > .duty_cycle = 200 ns. > > > > > > > > > > 2. leds-qcom-lpg cannot pick 200 ns exactly and then chooses .period = > > > > > 1000000000 / 4.26666 MHz = 234.375 ns > > > > > > > > > > 3. leds-qcom-lpg then determines setting for requested .duty_cycle > > > > > based on .period = 200 ns which then ends up with something bogus. > > > > > > The trace looks better than what I expected. 2. is fine here because it > > > seems when Sebastian wrote "driver requests PWM with 5 MHz period" that > > > meant period = 5000000 ns. That was then rounded down to 4266537 ns. And > > > the request for period = 5000000 ns + duty_cycle = 5000000 ns was > > > serviced by configuring period = 4266537 ns + duty_cycle = 4266537 ns. > > > So that's a 100 % relative duty configuration as intended. > > > > > > So just from the traces I don't spot a problem. Do these logs not match > > > what actually happens on the signal? > > > > What I do not get is why do we expect 2 pwm_get() and 2 pwm_apply() > > calls each time ? > > OK, so the second pwm_apply() is due to CONFIG_PWM_DEBUG. > > But still, the first pwm_apply() requests duty cycle of 5MHz: > > systemd-backlig-724 [006] ..... 9.079538: pwm_apply: pwmchip0.0: period=5000000 duty_cycle=5000000 polarity=0 enabled=1 err=0 > > So since the period is 4.26MHz, due to the knobs selected by the > provider, this duty cycle will result in a PWM value that is above the > selected resolution, as I already mentioned. > On top of that, the duty cycle in debugfs is also reported as 5000000ns when in fact it is 4266666ns, as the trace shows. > > > > Need to dig a bit further. > > > > But meanwhile, if the first pwm_apply() call goes all the way to the > > provider, then the duty cycle value, when translated to the actual PWM > > value that gets written to reg, will overflow. So this is what is wrong. > > And this is what actually happens. > > > > > > > > Best regards > > > Uwe > > > > > > >