From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753526AbdASQMO (ORCPT ); Thu, 19 Jan 2017 11:12:14 -0500 Received: from mga02.intel.com ([134.134.136.20]:59590 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753137AbdASQK7 (ORCPT ); Thu, 19 Jan 2017 11:10:59 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,254,1477983600"; d="scan'208";a="924415562" Message-ID: <1484842208.2133.245.camel@linux.intel.com> Subject: Re: [PATCH 2/2] pwm: pca9685: fix prescaler initialization From: Andy Shevchenko To: Clemens Gruber , Thierry Reding Cc: linux-pwm@vger.kernel.org, linux-kernel@vger.kernel.org, Florian Vaussard , Mika Westerberg Date: Thu, 19 Jan 2017 18:10:08 +0200 In-Reply-To: <20170119144925.GA1660@archie.localdomain> References: <20161213155251.28684-1-clemens.gruber@pqgruber.com> <20161213155251.28684-2-clemens.gruber@pqgruber.com> <20170118105735.GM18989@ulmo.ba.sec> <1484737764.2133.193.camel@linux.intel.com> <20170118135325.GA2498@archie.localdomain> <1484748118.2133.206.camel@linux.intel.com> <20170118142533.GA17640@archie.localdomain> <1484829279.2133.236.camel@linux.intel.com> <20170119144925.GA1660@archie.localdomain> Organization: Intel Finland Oy Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.22.3-1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 2017-01-19 at 15:49 +0100, Clemens Gruber wrote: > On Thu, Jan 19, 2017 at 02:34:39PM +0200, Andy Shevchenko wrote: > > On Wed, 2017-01-18 at 15:25 +0100, Clemens Gruber wrote: > > > Yes, that's what this patch tries to solve by verifying that the > > > external setting (the prescale register) is set to its hardware > > > default > > > value of 0x1E (corresponding to a period of 1/200 Hz). > > > If it is not 0x1E, the driver will reconfigure the prescaler > > > according > > > to the desired period at the time of the next configuration. > > > > Yes, and my question is what is possible go wrong if you just > > enforce > > prescaler to be 1/200Hz? > > If we enforce a default of 1 / 200 Hz, we have to go through the SLEEP > mode and udelay for 0.5ms once for our default and then again for the > user, if he does not want a period of 1 / 200 Hz. > -> Number of prescaler changes: 1 or 2 > > I think it is better as it is now + my patch applied: We verify if the > prescaler is already set to 1 / 200 Hz. > Then, as soon as the user configures his PWM channels, we either do > not > have to change the prescaler at all (if he wants 1 / 200 Hz) or do it > once at the time of configuration. > -> Number of prescaler changes: 0 or 1 > > What's the advantage of enforcing the prescaler to 1 / 200 Hz in the > probe function when we do not know yet if 1 / 200 Hz is the period the > user is going to configure? Advantage of this proposal is to get to known state. Combining with your proposal I would see the best approach is to set pca->period_ns accordingly to current prescaler value if you want to. In your patch I see no benefit, since it's quite unlikely user will want to have 5ms period among all possibilities. The 500us delay at the _first_ configuration is not a big deal. So, summarize, I prefer (in order of preference from high to low): - enforce default prescaler value based on default period (it's just one line to write a register) - calculate default period based on actual prescaler value - your or similar solution If you still disagree with that, I rest my case. -- Andy Shevchenko Intel Finland Oy