From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933454AbcGGIrG (ORCPT ); Thu, 7 Jul 2016 04:47:06 -0400 Received: from mailout2.w1.samsung.com ([210.118.77.12]:23464 "EHLO mailout2.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756845AbcGGIrB (ORCPT ); Thu, 7 Jul 2016 04:47:01 -0400 X-AuditID: cbfec7f5-f792a6d000001302-87-577e17015978 Message-id: <577E1700.1050408@samsung.com> Date: Thu, 07 Jul 2016 10:46:56 +0200 From: Jacek Anaszewski User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130804 Thunderbird/17.0.8 MIME-version: 1.0 To: "H. Nikolaus Schaller" Cc: Jacek Anaszewski , "David Rivshin (Allworx)" , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Richard Purdie , devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-leds@vger.kernel.org, kernel@pyra-handheld.com, marek@goldelico.com, letux-kernel@openphoenux.org Subject: Re: [PATCH] drivers: led: is31fl319x: 6/9-channel light effect led driver References: <75127f4bf2bb11343bdff5bfb1129a092e668c61.1461004995.git.hns@goldelico.com> <20160418212504.29d67592.drivshin.allworx@gmail.com> <80A3B3D8-2309-4A41-91F9-4A2EE6DDF975@goldelico.com> <5717EECB.8010401@gmail.com> In-reply-to: Content-type: text/plain; charset=windows-1252; format=flowed Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprDIsWRmVeSWpSXmKPExsVy+t/xa7qM4nXhBp/uS1vMP3KO1WLx5Fns Fv1vFrJa/Nj2lcni3KuVjBa3t25gsbj0tcZi659LbBaXd81hs9j6Zh2jxb+lW9gsll6/yGQx YfpaFovWvUfYLXbvesrqwO+xZt4aRo/Lfb1MHjtn3WX3WPP+FLPHyuVf2Dw2repk82iZtIvd 40tLM7PHnvk/WD0+b5IL4IrisklJzcksSy3St0vgynjT8pyx4LtMRf/2h2wNjHfFuhg5OCQE TCR+brPpYuQEMsUkLtxbz9bFyMUhJLCUUeLlhL1MEM4zRokPjS+ZQKp4BbQkdixYxQJiswio SnSt7mIGsdkEDCV+vngNViMqECHx5/Q+Voh6QYkfk++B1YsI6El0fv/DAjKUWeAns8SW1duZ Qa4QFgiROHjRGWLZMSaJ07t2gg3iFHCUeHD3H9gCZgFbiQXv17FA2PISm9e8ZZ7AKDALyY5Z SMpmISlbwMi8ilE0tTS5oDgpPddIrzgxt7g0L10vOT93EyMkzr7uYFx6zOoQowAHoxIP74Kc 2nAh1sSy4srcQ4wSHMxKIrxmonXhQrwpiZVVqUX58UWlOanFhxilOViUxHln7nofIiSQnliS mp2aWpBaBJNl4uCUamDc7t8anxTyXv/67bU/V4kVcnhdLwjM+P0s7M7h/hwHiz8KU6a+5nJ7 4C9Y9f5c6gTnHdrPze1XzevxSS1boXlRatfc/8rVac8zT6x8Oj35z4zZGt1ie7Z5Mb1bXnKT 5z5jv3bNcdcVx9fYR03jzdCYoh0799H247xPL7zKOXOSZ6JE3lHp/ZuKlViKMxINtZiLihMB TXroCa8CAAA= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Nikolaus, On 07/06/2016 12:02 PM, H. Nikolaus Schaller wrote: > Hi, > finally, I found the time to update the driver according to the many comments > received a while ago. > > Most of them have been worked in, including the regmap idea and > brightness_set_blocking(). > > The driver works on our system, so that I will mail [PATCH v2] as a followup. > > There is only one aspect of the new solution I am not sure if it is > really better than our old proposal (see below). > > >> Am 20.04.2016 um 23:04 schrieb Jacek Anaszewski : >> >> On 04/19/2016 07:21 PM, H. Nikolaus Schaller wrote: >>>> I believe the LEDS core now handles the workqueues generically for >>>> blocking operations, so it's no longer needed in the individual drivers. >>> >>> We had a lot of trouble with locking and blocking especially if we >>> want to indicate CPU or (root) disk activity. >> >> What kind of troubles you had? Could you share more details? >> Does it mean that current LED core design doesn't fit for your >> use cases? > > The system started to flicker the LEDs irregularily and sometimes > the whole kernel stalled. > >> >>> So it is implemented in a way that changes can be requested faster >>> than the I2C bus can write new values to the chip. >>> >>> Only after one sequence of I2C writes is done, another work function >>> can be scheduled. And each group of writes updates as many LEDs >>> in parallel if necessary. >> >> You can serialize the operations in brightness_set_blocking with >> a mutex. > > Yes, that works fine in our (incomplete) test setup. > > But I think it assumes that the i2c bus is never congested by other i2c traffic. > > I have not found code that obviously takes care of the situation if led > trigger events (e.g. mmc or cpu triggers) are coming in faster than the > i2c (even using regmap) can write out over i2c. > > If I understand the led core code correctly, it will just do another schedule_work > for every single change of led brightness. Please look at schedule_work documentation in include/linux/workqueue.h: /** * schedule_work - put work task in global workqueue * @work: job to be done * * Returns %false if @work was already on the kernel-global workqueue and * %true otherwise. * * This puts a job in the kernel-global workqueue if it was not already * queued and leaves it in the same position on the kernel-global * workqueue otherwise. */ static inline bool schedule_work(struct work_struct *work) { return queue_work(system_wq, work); } > > So I wonder if Is there some guarantee that this work queue will not fill > up memory and is really processed faster than being filled? I.e. can the > queue overflow? > > To reduce this risk, my original implementation strategy was different. The > update speed was limited by i2c writing. A new register update batch job > was only scheduled if the previous one is finished. If i2c was blocked/congested, > the writing worker thread would come to a halt. > > All incoming led brightness changes were simply accumulated until a new > batch job is started, because LEDs would anyways flicker invisibly fast. > > Tests with the new driver have shown that it seems not to run into this situation > on our system but it might depend on factors we have not yet tested (slow i2c, > other i2c traffic on the same bus, CPU speed, event types choosen). > > So I am a little in doubt about this risk. But I may have simply missed > the reason why the standard approach works and can never overflow. > > BR, > Nikolaus > > > -- Best regards, Jacek Anaszewski