From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752902AbbKWVmN (ORCPT ); Mon, 23 Nov 2015 16:42:13 -0500 Received: from mail.kmu-office.ch ([178.209.48.109]:60305 "EHLO mail.kmu-office.ch" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751127AbbKWVmL (ORCPT ); Mon, 23 Nov 2015 16:42:11 -0500 MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Date: Mon, 23 Nov 2015 13:40:43 -0800 From: Stefan Agner To: Thierry Reding , Li.Xiubo@freescale.com Cc: linux-pwm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH RESEND] pwm: ftm: fix clock enable/disable when using PM In-Reply-To: <20151123094550.GA28740@ulmo.nvidia.com> References: <1445142552-3530-1-git-send-email-stefan@agner.ch> <290ed017061ef41e1fe5c3fd5a523511@agner.ch> <20151123094550.GA28740@ulmo.nvidia.com> Message-ID: User-Agent: Roundcube Webmail/1.1.3 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Thanks Thierry for looking into that! Added Li as recipient to start discussion. Some comments below: On 2015-11-23 01:45, Thierry Reding wrote: > On Wed, Nov 18, 2015 at 06:04:12PM -0800, Stefan Agner wrote: >> Thierry, >> >> I realized that this patch did not make it into 4.4-rc1, while others, >> IMHO less important patches which have been posted later (e.g. sunxi >> whitespace fixes) have made it! :-( > > The reason why I merged them is because they are low-risk, whereas this > patch of yours changes existing behaviour, and hasn't received any > feedback from anyone. So the choice that I need to make is to either > trust the original author to have tested the driver and it was working, > or you to have verified that it is still working after the patch on all > setups that it used to work on. The first option obviously carries the > least risk, and that's the reason why the patch hasn't been merged. > >> Anything wrong with that? Or am I on your spam list? Note that this is >> already a RESEND :-) > > If you want to get this merged, you should try to get some feedback from > at least the original author. Xiubo Li and I extensively discussed this > back at the time when he submitted the driver and we came up with the > current code as the best approach to making sure that clocks are on and > off when they should be. So if it's not working for you, I'm fine taking > the patch if Xiubo or somebody else has had the chance to look at it and > review or test it. So a Reviewed-by or Tested-by tag will go a long way > to convince me that it's safe to apply. In mainline, the driver is only used in Freescale Vybrid device trees. I think most issues have been introduced with the patches adding suspend support: http://thread.gmane.org/gmane.linux.kernel/1806940 Not sure against what suspend/resume implementation Li created the patches, so far there is no suspend/resume implementation for Vybrid upstream. While working on Vybrid's low power mode LPSTOP3, I noticed the issues.... > Also you enumerate all the various bits that are broken, and it would > seem to me that they could each be fixed individually rather than go and > implement something completely different which might have undesirable > side-effects. Such an approach would also make it more likely for me to > merge the patch because it would hopefully be more obvious what is being > fixed and that it's a correct fix. There are different issues addressed by one solution: Using the clock frameworks enable/disable counts... I looked through the code again, it is really quite hard to split it up. I could create one patch which only switches the PWM enable/disable part to clock framework counts, and leave the suspend/resume part broken. Than, in a second patch fix the suspend/resume part. But that sounds like the wrong approach to me too... I took inspiration from other PWM drivers, I think the suspend/resume idea was from TI EHRPWM PWM driver. So this shouldn't be far off what others are doing. > > It's not that I mind the rework, but I'd at least like for someone to > verify that it's all still working on existing setups. Now, I understand > that people can go missing, so if nobody were to give you a Reviewed-by > or Tested-by for a couple of weeks I'd even consider applying without, > but as it is, you didn't even Cc Xiubo on the patch, so he's likely > missed it entirely. I have had Li in the initial email, don't know/remember why I removed him from the list in RESEND... Will keep him in the loop. Just realized that there is now a helper function for test_bit(PWMF_ENABLED,... Will send a v2 anyway then. -- Stefan