From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753836AbdBPJ1Y (ORCPT ); Thu, 16 Feb 2017 04:27:24 -0500 Received: from mailapp01.imgtec.com ([195.59.15.196]:27906 "EHLO mailapp01.imgtec.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751982AbdBPJ1S (ORCPT ); Thu, 16 Feb 2017 04:27:18 -0500 Subject: Re: [PATCH 1/2] pwm: loongson1: Add PWM driver for Loongson1 SoC To: Yang Ling References: <20170213152801.GA32019@ubuntu> <20170215130902.GA32795@ubuntu> CC: , , , , From: Marcin Nowakowski Message-ID: <0d0c43f5-1016-4cb5-01f8-9ca82860b8ad@imgtec.com> Date: Thu, 16 Feb 2017 10:27:15 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: <20170215130902.GA32795@ubuntu> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.80.2.5] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Yang, On 15.02.2017 14:09, Yang Ling wrote: >>> + tmp = (unsigned long long)clk_get_rate(pc->clk) * period_ns; >>> + do_div(tmp, 1000000000); NSEC_PER_SEC ? >>> + period = tmp; >>> + >>> + tmp = (unsigned long long)period * duty_ns; >>> + do_div(tmp, period_ns); >>> + duty = period - tmp; >>> + >>> + if (duty >= period) >>> + duty = period - 1; >>> + >>> + if (duty >> 24 || period >> 24) >>> + return -EINVAL; >>> + >>> + chan->period_ns = period_ns; >>> + chan->duty_ns = duty_ns; >>> + >>> + writel(duty, pc->base + PWM_HRC(pwm->hwpwm)); >>> + writel(period, pc->base + PWM_LRC(pwm->hwpwm)); >>> + writel(0x00, pc->base + PWM_CNT(pwm->hwpwm)); >>> + >> >> PWM_HRC and PWM_LRC names suggest that you're using high/low state >> counters here rather than duty/period - but with no documentation >> I'm just guessing here. > > Indeed, the high/low state counters is used here. > Change the name to duty_cnt/period_cnt. > > What I was referring to here is that if you have a high/low value counters that you enter then these are not the same as duty/period, in simple terms: high_cnt = duty_cnt low_cnt = period_cnt - duty_cnt so please double check that this is what you want to be doing? As the names used suggest that this code may be wrong. Or maybe what you're doing is correct but the register access macros have misleading names? Marcin From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcin Nowakowski Subject: Re: [PATCH 1/2] pwm: loongson1: Add PWM driver for Loongson1 SoC Date: Thu, 16 Feb 2017 10:27:15 +0100 Message-ID: <0d0c43f5-1016-4cb5-01f8-9ca82860b8ad@imgtec.com> References: <20170213152801.GA32019@ubuntu> <20170215130902.GA32795@ubuntu> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20170215130902.GA32795@ubuntu> Sender: linux-kernel-owner@vger.kernel.org To: Yang Ling Cc: thierry.reding@gmail.com, keguang.zhang@gmail.com, linux-kernel@vger.kernel.org, linux-pwm@vger.kernel.org, linux-mips@linux-mips.org List-Id: linux-pwm@vger.kernel.org Hi Yang, On 15.02.2017 14:09, Yang Ling wrote: >>> + tmp = (unsigned long long)clk_get_rate(pc->clk) * period_ns; >>> + do_div(tmp, 1000000000); NSEC_PER_SEC ? >>> + period = tmp; >>> + >>> + tmp = (unsigned long long)period * duty_ns; >>> + do_div(tmp, period_ns); >>> + duty = period - tmp; >>> + >>> + if (duty >= period) >>> + duty = period - 1; >>> + >>> + if (duty >> 24 || period >> 24) >>> + return -EINVAL; >>> + >>> + chan->period_ns = period_ns; >>> + chan->duty_ns = duty_ns; >>> + >>> + writel(duty, pc->base + PWM_HRC(pwm->hwpwm)); >>> + writel(period, pc->base + PWM_LRC(pwm->hwpwm)); >>> + writel(0x00, pc->base + PWM_CNT(pwm->hwpwm)); >>> + >> >> PWM_HRC and PWM_LRC names suggest that you're using high/low state >> counters here rather than duty/period - but with no documentation >> I'm just guessing here. > > Indeed, the high/low state counters is used here. > Change the name to duty_cnt/period_cnt. > > What I was referring to here is that if you have a high/low value counters that you enter then these are not the same as duty/period, in simple terms: high_cnt = duty_cnt low_cnt = period_cnt - duty_cnt so please double check that this is what you want to be doing? As the names used suggest that this code may be wrong. Or maybe what you're doing is correct but the register access macros have misleading names? Marcin