From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756684AbdDRJPa (ORCPT ); Tue, 18 Apr 2017 05:15:30 -0400 Received: from mga03.intel.com ([134.134.136.65]:13716 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751676AbdDRJO2 (ORCPT ); Tue, 18 Apr 2017 05:14:28 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.37,218,1488873600"; d="scan'208";a="958168070" Message-ID: <1492506852.24567.54.camel@linux.intel.com> Subject: Re: [PATCH v4 1/1] pwm: pca9685: fix gpio-only operation. From: Andy Shevchenko To: Sven Van Asbroeck , thierry.reding@gmail.com, "Rafael J. Wysocki" Cc: linux-pwm@vger.kernel.org, linux-kernel@vger.kernel.org, clemens.gruber@pqgruber.com, mika.westerberg@linux.intel.com, Sven Van Asbroeck Date: Tue, 18 Apr 2017 12:14:12 +0300 In-Reply-To: <1492088291-5215-2-git-send-email-svenv@arcx.com> References: <1492088291-5215-1-git-send-email-svenv@arcx.com> <1492088291-5215-2-git-send-email-svenv@arcx.com> Organization: Intel Finland Oy Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.22.6-1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org +Cc: Rafael (one question to you below) On Thu, 2017-04-13 at 08:58 -0400, Sven Van Asbroeck wrote: > gpio-only driver operation never clears the SLEEP bit, which can > cause the gpios to become unusable. > > Example: > 1. user requests first pwm  ->      driver clears SLEEP bit > 2. user frees last pwm      ->      driver sets SLEEP bit > 3. user requests gpio > 4. user switches gpio on    ->      output does not turn on >                                     because SLEEP bit is set > > Prevent this behaviour by letting the runtime_pm framework > control the SLEEP bit. This will put the chip to SLEEP if > no pwms/gpios are exported/in use. > I know the patch is applied already, though please consider below to be addressed as usual (w/o Fixes tag). > +static void pca9685_set_sleep_mode(struct pca9685 *pca, int sleep) > +{ > + regmap_update_bits(pca->regmap, PCA9685_MODE1, > +    MODE1_SLEEP, sleep ? MODE1_SLEEP : 0); > + if (!sleep) { > + /* Wait 500us for the oscillator to be back up */ > + udelay(500); > + } I would go with /* Wait for @sleep microseconds for the oscillator to be back up */ if (sleep) udelay(sleep); Otherwise int sleep is oddly here. Or bool sleep /* Wait 500us ... */ if (sleep) udelay(500); > +} > +#ifdef CONFIG_PM > +static int pca9685_pwm_runtime_suspend(struct device *dev) __maybe_unused and remove ugly #ifdef:ery. > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct pca9685 *pca = i2c_get_clientdata(client); > + > + pca9685_set_sleep_mode(pca, 1); > + return 0; >  } >   > +static int pca9685_pwm_runtime_resume(struct device *dev) Ditto. > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct pca9685 *pca = i2c_get_clientdata(client); > + > + pca9685_set_sleep_mode(pca, 0); > + return 0; > +} > +#endif > +static const struct dev_pm_ops pca9685_pwm_pm = { > + SET_RUNTIME_PM_OPS(pca9685_pwm_runtime_suspend, > +    pca9685_pwm_runtime_resume, NULL) > +}; > + Perhaps we may introduce RUNTIME_DEV_PM_OPS() macro and re-use it here. Rafael? -- Andy Shevchenko Intel Finland Oy