From mboxrd@z Thu Jan 1 00:00:00 1970 From: Brian Norris Date: Tue, 17 Oct 2017 16:51:48 +0000 Subject: Re: [RESEND PATCH v2 2/5] backlight: pwm_bl: Add device link for pwm_bl and pwm Message-Id: <20171017165146.GA3408@google.com> List-Id: References: <20171016100640.26575-1-jeffy.chen@rock-chips.com> <20171016100640.26575-3-jeffy.chen@rock-chips.com> <20171016235710.GA12188@google.com> <59E5BBC3.2020504@rock-chips.com> In-Reply-To: <59E5BBC3.2020504@rock-chips.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: jeffy Cc: linux-pwm@vger.kernel.org, linux-fbdev@vger.kernel.org, Bartlomiej Zolnierkiewicz , linux-kernel@vger.kernel.org, dmitry.torokhov@gmail.com, rjw@rjwysocki.net, dianders@chromium.org, dri-devel@lists.freedesktop.org, tfiga@chromium.org, broonie@kernel.org, Jingoo Han , Thierry Reding , Daniel Thompson , Lee Jones Hi, On Tue, Oct 17, 2017 at 04:13:55PM +0800, Jeffy Chen wrote: > On 10/17/2017 07:57 AM, Brian Norris wrote: > >This is going to be a*lot* of churn throughout the tree, if we expect > >all resource consumers to do this. I think we'd want some kind of > >agreement from the PM maintainers and (larger) subsystem owners before > >going down this route... > > > >And in the PWM case, pwm_get() already has the device pointer. Why can't > >we just instrument it instead? > > according to pwm_bl driver, we may need to take care of pwm_request() too: That's a legacy API. I wouldn't spend any time on improving it. In fact, the only other 2 users are: (a) drivers/input/misc/max8997_haptic.c: abandoned; nobody provides pdata for that driver, so pwm_request() can never be called... (b) arch/arm/mach-s3c24xx/mach-rx1950.c: can be easily converted to the lookup table approach (pwm_add_table() + pwm_get()) if needed > pb->pwm = devm_pwm_get(&pdev->dev, NULL); > if (IS_ERR(pb->pwm) && PTR_ERR(pb->pwm) != -EPROBE_DEFER && > !node) { > dev_err(&pdev->dev, "unable to request PWM, trying > legacy API\n"); > pb->legacy = true; > pb->pwm = pwm_request(data->pwm_id, "pwm-backlight"); > } > > and maybe also *of_pwm_get... > > maybe we can add a dummy pwm chip for those orphan pwms? What? That seems like a very silly idea. And judging by Thierry's response to your v4, he doesn't understand it either. All I was suggesting was that you should try to add the device links in the fewest places possible. Because if you require all consumers to add extra boilerplate to resolve some strange corner cases, those corner cases will likely go unsolved in many cases. Brian From mboxrd@z Thu Jan 1 00:00:00 1970 From: Brian Norris Subject: Re: [RESEND PATCH v2 2/5] backlight: pwm_bl: Add device link for pwm_bl and pwm Date: Tue, 17 Oct 2017 09:51:48 -0700 Message-ID: <20171017165146.GA3408@google.com> References: <20171016100640.26575-1-jeffy.chen@rock-chips.com> <20171016100640.26575-3-jeffy.chen@rock-chips.com> <20171016235710.GA12188@google.com> <59E5BBC3.2020504@rock-chips.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Content-Disposition: inline In-Reply-To: <59E5BBC3.2020504@rock-chips.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: jeffy Cc: linux-pwm@vger.kernel.org, linux-fbdev@vger.kernel.org, Bartlomiej Zolnierkiewicz , linux-kernel@vger.kernel.org, dmitry.torokhov@gmail.com, rjw@rjwysocki.net, dianders@chromium.org, dri-devel@lists.freedesktop.org, tfiga@chromium.org, broonie@kernel.org, Jingoo Han , Thierry Reding , Daniel Thompson , Lee Jones List-Id: linux-pwm@vger.kernel.org SGksCgpPbiBUdWUsIE9jdCAxNywgMjAxNyBhdCAwNDoxMzo1NVBNICswODAwLCBKZWZmeSBDaGVu IHdyb3RlOgo+IE9uIDEwLzE3LzIwMTcgMDc6NTcgQU0sIEJyaWFuIE5vcnJpcyB3cm90ZToKPiA+ VGhpcyBpcyBnb2luZyB0byBiZSBhKmxvdCogIG9mIGNodXJuIHRocm91Z2hvdXQgdGhlIHRyZWUs IGlmIHdlIGV4cGVjdAo+ID5hbGwgcmVzb3VyY2UgY29uc3VtZXJzIHRvIGRvIHRoaXMuIEkgdGhp bmsgd2UnZCB3YW50IHNvbWUga2luZCBvZgo+ID5hZ3JlZW1lbnQgZnJvbSB0aGUgUE0gbWFpbnRh aW5lcnMgYW5kIChsYXJnZXIpIHN1YnN5c3RlbSBvd25lcnMgYmVmb3JlCj4gPmdvaW5nIGRvd24g dGhpcyByb3V0ZS4uLgo+ID4KPiA+QW5kIGluIHRoZSBQV00gY2FzZSwgcHdtX2dldCgpIGFscmVh ZHkgaGFzIHRoZSBkZXZpY2UgcG9pbnRlci4gV2h5IGNhbid0Cj4gPndlIGp1c3QgaW5zdHJ1bWVu dCBpdCBpbnN0ZWFkPwo+IAo+IGFjY29yZGluZyB0byBwd21fYmwgZHJpdmVyLCB3ZSBtYXkgbmVl ZCB0byB0YWtlIGNhcmUgb2YgcHdtX3JlcXVlc3QoKSB0b286CgpUaGF0J3MgYSBsZWdhY3kgQVBJ LiBJIHdvdWxkbid0IHNwZW5kIGFueSB0aW1lIG9uIGltcHJvdmluZyBpdC4gSW4gZmFjdCwKdGhl IG9ubHkgb3RoZXIgMiB1c2VycyBhcmU6CihhKSBkcml2ZXJzL2lucHV0L21pc2MvbWF4ODk5N19o YXB0aWMuYzogYWJhbmRvbmVkOyBub2JvZHkgcHJvdmlkZXMKICAgIHBkYXRhIGZvciB0aGF0IGRy aXZlciwgc28gcHdtX3JlcXVlc3QoKSBjYW4gbmV2ZXIgYmUgY2FsbGVkLi4uCihiKSBhcmNoL2Fy bS9tYWNoLXMzYzI0eHgvbWFjaC1yeDE5NTAuYzogY2FuIGJlIGVhc2lseSBjb252ZXJ0ZWQgdG8K ICAgIHRoZSBsb29rdXAgdGFibGUgYXBwcm9hY2ggKHB3bV9hZGRfdGFibGUoKSArIHB3bV9nZXQo KSkgaWYgbmVlZGVkCgo+ICAgICAgICAgcGItPnB3bSA9IGRldm1fcHdtX2dldCgmcGRldi0+ZGV2 LCBOVUxMKTsKPiAgICAgICAgIGlmIChJU19FUlIocGItPnB3bSkgJiYgUFRSX0VSUihwYi0+cHdt KSAhPSAtRVBST0JFX0RFRkVSICYmCj4gIW5vZGUpIHsKPiAgICAgICAgICAgICAgICAgZGV2X2Vy cigmcGRldi0+ZGV2LCAidW5hYmxlIHRvIHJlcXVlc3QgUFdNLCB0cnlpbmcKPiBsZWdhY3kgQVBJ XG4iKTsKPiAgICAgICAgICAgICAgICAgcGItPmxlZ2FjeSA9IHRydWU7Cj4gICAgICAgICAgICAg ICAgIHBiLT5wd20gPSBwd21fcmVxdWVzdChkYXRhLT5wd21faWQsICJwd20tYmFja2xpZ2h0Iik7 Cj4gICAgICAgICB9Cj4gCj4gYW5kIG1heWJlIGFsc28gKm9mX3B3bV9nZXQuLi4KPiAKPiBtYXli ZSB3ZSBjYW4gYWRkIGEgZHVtbXkgcHdtIGNoaXAgZm9yIHRob3NlIG9ycGhhbiBwd21zPwoKV2hh dD8gVGhhdCBzZWVtcyBsaWtlIGEgdmVyeSBzaWxseSBpZGVhLiBBbmQganVkZ2luZyBieSBUaGll cnJ5J3MKcmVzcG9uc2UgdG8geW91ciB2NCwgaGUgZG9lc24ndCB1bmRlcnN0YW5kIGl0IGVpdGhl ci4KCkFsbCBJIHdhcyBzdWdnZXN0aW5nIHdhcyB0aGF0IHlvdSBzaG91bGQgdHJ5IHRvIGFkZCB0 aGUgZGV2aWNlIGxpbmtzIGluCnRoZSBmZXdlc3QgcGxhY2VzIHBvc3NpYmxlLiBCZWNhdXNlIGlm IHlvdSByZXF1aXJlIGFsbCBjb25zdW1lcnMgdG8gYWRkCmV4dHJhIGJvaWxlcnBsYXRlIHRvIHJl c29sdmUgc29tZSBzdHJhbmdlIGNvcm5lciBjYXNlcywgdGhvc2UgY29ybmVyCmNhc2VzIHdpbGwg bGlrZWx5IGdvIHVuc29sdmVkIGluIG1hbnkgY2FzZXMuCgpCcmlhbgpfX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0CmRy aS1kZXZlbEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5v cmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S936614AbdJQQvy (ORCPT ); Tue, 17 Oct 2017 12:51:54 -0400 Received: from mail-io0-f169.google.com ([209.85.223.169]:50351 "EHLO mail-io0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933030AbdJQQvw (ORCPT ); Tue, 17 Oct 2017 12:51:52 -0400 X-Google-Smtp-Source: ABhQp+S0t3mN3HzMjcKUTpFxeCa18mCBfHg3DzZxTJixAfgpHnP4+xv6B5h+KW76cXoIv1QjGRxK8g== Date: Tue, 17 Oct 2017 09:51:48 -0700 From: Brian Norris To: jeffy Cc: linux-kernel@vger.kernel.org, dmitry.torokhov@gmail.com, heiko@sntech.de, rjw@rjwysocki.net, dianders@chromium.org, tfiga@chromium.org, broonie@kernel.org, seanpaul@chromium.org, linux-pwm@vger.kernel.org, linux-fbdev@vger.kernel.org, Daniel Thompson , Thierry Reding , Bartlomiej Zolnierkiewicz , dri-devel@lists.freedesktop.org, Jingoo Han , Lee Jones Subject: Re: [RESEND PATCH v2 2/5] backlight: pwm_bl: Add device link for pwm_bl and pwm Message-ID: <20171017165146.GA3408@google.com> References: <20171016100640.26575-1-jeffy.chen@rock-chips.com> <20171016100640.26575-3-jeffy.chen@rock-chips.com> <20171016235710.GA12188@google.com> <59E5BBC3.2020504@rock-chips.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <59E5BBC3.2020504@rock-chips.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On Tue, Oct 17, 2017 at 04:13:55PM +0800, Jeffy Chen wrote: > On 10/17/2017 07:57 AM, Brian Norris wrote: > >This is going to be a*lot* of churn throughout the tree, if we expect > >all resource consumers to do this. I think we'd want some kind of > >agreement from the PM maintainers and (larger) subsystem owners before > >going down this route... > > > >And in the PWM case, pwm_get() already has the device pointer. Why can't > >we just instrument it instead? > > according to pwm_bl driver, we may need to take care of pwm_request() too: That's a legacy API. I wouldn't spend any time on improving it. In fact, the only other 2 users are: (a) drivers/input/misc/max8997_haptic.c: abandoned; nobody provides pdata for that driver, so pwm_request() can never be called... (b) arch/arm/mach-s3c24xx/mach-rx1950.c: can be easily converted to the lookup table approach (pwm_add_table() + pwm_get()) if needed > pb->pwm = devm_pwm_get(&pdev->dev, NULL); > if (IS_ERR(pb->pwm) && PTR_ERR(pb->pwm) != -EPROBE_DEFER && > !node) { > dev_err(&pdev->dev, "unable to request PWM, trying > legacy API\n"); > pb->legacy = true; > pb->pwm = pwm_request(data->pwm_id, "pwm-backlight"); > } > > and maybe also *of_pwm_get... > > maybe we can add a dummy pwm chip for those orphan pwms? What? That seems like a very silly idea. And judging by Thierry's response to your v4, he doesn't understand it either. All I was suggesting was that you should try to add the device links in the fewest places possible. Because if you require all consumers to add extra boilerplate to resolve some strange corner cases, those corner cases will likely go unsolved in many cases. Brian