Re: [Freedreno] [DPU PATCH v2 2/2] drm/panel: add backlight control support for truly panel
Hi Bjorn Thanks for the comments. I will upload a new patch for this which will be dependent on https://patchwork.kernel.org/patch/10377537/ series. This series registers the backlight device we need. Thanks Abhinav On 2018-04-18 21:42, Bjorn Andersson wrote: On Wed 18 Apr 21:23 PDT 2018, abhin...@codeaurora.org wrote: Hi Bjorn Hi Abhinav, Thanks very much for the detailed response. You're welcome. Yes, we decided that userspace hardcoding this node name is not a strong enough reason to register as another backlight device. Had one follow up question though. The QC WLED driver, drivers/leds/leds-qpnp-wled.c is not registering itself as a backlight device. It only registers as a led device. In that case, we cannot invoke of_find_backlight_by_node to get a handle on it. One question we have is , is it required that every WLED driver register itself as a backlight device too? In this case since it is not doing so, but we need to trigger it for the backlight. Is there any way we can do this? It seems I answered this in private, so let me summarize my answer for the record. The downstream driver for the Qualcomm WLED registers a LED, but the WLED driver should register a backlight device. There is a driver (drivers/video/backlight/pm8941-wled.c) that does that for the WLED version found in PM8941, so the appropriate solution to this problem is to extend that to support the PMIC you're looking at. OR shall we just abandon the backlight control out of this driver entirely. The backlight handling in the panel driver serves the purpose of blanking and unblanking the associated backlight device, given the status of the panel. And this is still considered valuable. It does sounds like a reasonable idea to extend this to also cover brightness management, but as you might guess this becomes a larger and completely generic issue. Regards, Bjorn Thanks Abhinav On 2018-04-18 21:00, Bjorn Andersson wrote: > On Tue 17 Apr 17:04 PDT 2018, abhin...@codeaurora.org wrote: > > > Hi Bjorn > > > > Apologies if the prev reply wasnt clear. > > > > Hope this one is. > > > > Much better, now we can discuss the actual issues :) > > > reply inline. > > > > On 2018-04-17 14:29, Bjorn Andersson wrote: > > > On Tue 17 Apr 11:21 PDT 2018, abhin...@codeaurora.org wrote: > > > > On 2018-04-16 23:13, Bjorn Andersson wrote: > > > [..] > > > > > If the panel isn't actually a piece of backlight hardware then it should > > > > > not register a backlight device. Why do you need your own sysfs? > > > > > > > > > > Regards, > > > > > Bjorn > > > > [Abhinav] This particular panel isnt a piece of backlight hardware. > > > > But, we want to have our own sysfs to give flexibility to our > > > > userspace > > > > to write and read stuff for its proprietary uses. > > > > > > Please be more specific in your replies, no one will accept code that > > > "does stuff" and expecting a reviewer to spend time guessing or pulling > > > the information out of you is a sure way to get your patches ignored. > > > > > > Exactly what kind of stuff are you talking about here and exactly which > > > problem are you solving. > > > > > > > Thats how our downstream has been working so far and hence to maintain > > > > the compatibility would like to have it. > > > > > > Make your proprietary code work with the upstream kernel and you > > > shouldn't ever have to modify it. > > > > > > Regards, > > > Bjorn > > > > [Abhinav] We have a few userspace clients today for the backlight > > sysfs node > > which read/write directly to > > "/sys/class/backlight/panel0-backlight/brightness" > > When I said "stuff" I was only referring to the brightness value. > > This separate sysfs node allows us to validate those userspace > > features of ours which directly edit the backlight value on our > > reference platform. > > That's nice, but you're enforcing that either all panel drivers must > implement this backlight wrapper or that your customers must modify > their user space to match their backlight implementation. > > > Since our reference platform uses this panel,hence having our own > > sysfs alias helps. Otherwise, whenever we try to use this panel with > > upstream code we will have to end up changing all those places in our > > userspace/framework to change the sysfs path. > > The actual problem comes down to "how does user space know the name of > the backlight instance associated with the panel" and this is a valid > question to pursue. > > But given your current design you could just scan for the one and only > backlight device available in the system; as your use of the static name > "panel0-backlight" doesn't allow multiple backlights anyway. > > > If your goal is simply to ship your reference with something that you > can show work, then just replace the hard coded panel0-backlight with > the name of the wled backlight device. Customers can change panels as > they wish, but in the event that they plug in a different backli
Re: [Freedreno] [DPU PATCH v2 2/2] drm/panel: add backlight control support for truly panel
On Wed 18 Apr 21:23 PDT 2018, abhin...@codeaurora.org wrote: > Hi Bjorn > Hi Abhinav, > Thanks very much for the detailed response. > You're welcome. > Yes, we decided that userspace hardcoding this node name is not a > strong enough reason to register as another backlight device. > > Had one follow up question though. > > The QC WLED driver, drivers/leds/leds-qpnp-wled.c is not registering itself > as a backlight device. > > It only registers as a led device. > > In that case, we cannot invoke of_find_backlight_by_node to get a handle on > it. > > One question we have is , is it required that every WLED driver register > itself as a backlight device too? > > In this case since it is not doing so, but we need to trigger it for the > backlight. > > Is there any way we can do this? > It seems I answered this in private, so let me summarize my answer for the record. The downstream driver for the Qualcomm WLED registers a LED, but the WLED driver should register a backlight device. There is a driver (drivers/video/backlight/pm8941-wled.c) that does that for the WLED version found in PM8941, so the appropriate solution to this problem is to extend that to support the PMIC you're looking at. > OR shall we just abandon the backlight control out of this driver entirely. > The backlight handling in the panel driver serves the purpose of blanking and unblanking the associated backlight device, given the status of the panel. And this is still considered valuable. It does sounds like a reasonable idea to extend this to also cover brightness management, but as you might guess this becomes a larger and completely generic issue. Regards, Bjorn > Thanks > > Abhinav > > On 2018-04-18 21:00, Bjorn Andersson wrote: > > On Tue 17 Apr 17:04 PDT 2018, abhin...@codeaurora.org wrote: > > > > > Hi Bjorn > > > > > > Apologies if the prev reply wasnt clear. > > > > > > Hope this one is. > > > > > > > Much better, now we can discuss the actual issues :) > > > > > reply inline. > > > > > > On 2018-04-17 14:29, Bjorn Andersson wrote: > > > > On Tue 17 Apr 11:21 PDT 2018, abhin...@codeaurora.org wrote: > > > > > On 2018-04-16 23:13, Bjorn Andersson wrote: > > > > [..] > > > > > > If the panel isn't actually a piece of backlight hardware then it > > > > > > should > > > > > > not register a backlight device. Why do you need your own sysfs? > > > > > > > > > > > > Regards, > > > > > > Bjorn > > > > > [Abhinav] This particular panel isnt a piece of backlight hardware. > > > > > But, we want to have our own sysfs to give flexibility to our > > > > > userspace > > > > > to write and read stuff for its proprietary uses. > > > > > > > > Please be more specific in your replies, no one will accept code that > > > > "does stuff" and expecting a reviewer to spend time guessing or pulling > > > > the information out of you is a sure way to get your patches ignored. > > > > > > > > Exactly what kind of stuff are you talking about here and exactly which > > > > problem are you solving. > > > > > > > > > Thats how our downstream has been working so far and hence to maintain > > > > > the compatibility would like to have it. > > > > > > > > Make your proprietary code work with the upstream kernel and you > > > > shouldn't ever have to modify it. > > > > > > > > Regards, > > > > Bjorn > > > > > > [Abhinav] We have a few userspace clients today for the backlight > > > sysfs node > > > which read/write directly to > > > "/sys/class/backlight/panel0-backlight/brightness" > > > When I said "stuff" I was only referring to the brightness value. > > > This separate sysfs node allows us to validate those userspace > > > features of ours which directly edit the backlight value on our > > > reference platform. > > > > That's nice, but you're enforcing that either all panel drivers must > > implement this backlight wrapper or that your customers must modify > > their user space to match their backlight implementation. > > > > > Since our reference platform uses this panel,hence having our own > > > sysfs alias helps. Otherwise, whenever we try to use this panel with > > > upstream code we will have to end up changing all those places in our > > > userspace/framework to change the sysfs path. > > > > The actual problem comes down to "how does user space know the name of > > the backlight instance associated with the panel" and this is a valid > > question to pursue. > > > > But given your current design you could just scan for the one and only > > backlight device available in the system; as your use of the static name > > "panel0-backlight" doesn't allow multiple backlights anyway. > > > > > > If your goal is simply to ship your reference with something that you > > can show work, then just replace the hard coded panel0-backlight with > > the name of the wled backlight device. Customers can change panels as > > they wish, but in the event that they plug in a different backlight > > controller they would need to modify the c
Re: [Freedreno] [DPU PATCH v2 2/2] drm/panel: add backlight control support for truly panel
On Tue 17 Apr 17:04 PDT 2018, abhin...@codeaurora.org wrote: > Hi Bjorn > > Apologies if the prev reply wasnt clear. > > Hope this one is. > Much better, now we can discuss the actual issues :) > reply inline. > > On 2018-04-17 14:29, Bjorn Andersson wrote: > > On Tue 17 Apr 11:21 PDT 2018, abhin...@codeaurora.org wrote: > > > On 2018-04-16 23:13, Bjorn Andersson wrote: > > [..] > > > > If the panel isn't actually a piece of backlight hardware then it should > > > > not register a backlight device. Why do you need your own sysfs? > > > > > > > > Regards, > > > > Bjorn > > > [Abhinav] This particular panel isnt a piece of backlight hardware. > > > But, we want to have our own sysfs to give flexibility to our > > > userspace > > > to write and read stuff for its proprietary uses. > > > > Please be more specific in your replies, no one will accept code that > > "does stuff" and expecting a reviewer to spend time guessing or pulling > > the information out of you is a sure way to get your patches ignored. > > > > Exactly what kind of stuff are you talking about here and exactly which > > problem are you solving. > > > > > Thats how our downstream has been working so far and hence to maintain > > > the compatibility would like to have it. > > > > Make your proprietary code work with the upstream kernel and you > > shouldn't ever have to modify it. > > > > Regards, > > Bjorn > > [Abhinav] We have a few userspace clients today for the backlight sysfs node > which read/write directly to > "/sys/class/backlight/panel0-backlight/brightness" > When I said "stuff" I was only referring to the brightness value. > This separate sysfs node allows us to validate those userspace > features of ours which directly edit the backlight value on our > reference platform. That's nice, but you're enforcing that either all panel drivers must implement this backlight wrapper or that your customers must modify their user space to match their backlight implementation. > Since our reference platform uses this panel,hence having our own > sysfs alias helps. Otherwise, whenever we try to use this panel with > upstream code we will have to end up changing all those places in our > userspace/framework to change the sysfs path. The actual problem comes down to "how does user space know the name of the backlight instance associated with the panel" and this is a valid question to pursue. But given your current design you could just scan for the one and only backlight device available in the system; as your use of the static name "panel0-backlight" doesn't allow multiple backlights anyway. If your goal is simply to ship your reference with something that you can show work, then just replace the hard coded panel0-backlight with the name of the wled backlight device. Customers can change panels as they wish, but in the event that they plug in a different backlight controller they would need to modify the code. > Hence we wanted to preserve that sysfs node name. > The wled device is not created under /sys/class/backlight but under > /sys/class/leds/wled. > So we will have to change the name of this node across all our userspace. > Hard coding /sys/class/backlight/panel0-backlight in your user space instead of /sys/class/leds/wled is hardly a gain, in particular since the cost is 94 insertions - per panel driver. Regards, Bjorn ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Freedreno] [DPU PATCH v2 2/2] drm/panel: add backlight control support for truly panel
On Tue 17 Apr 17:42 PDT 2018, abhin...@codeaurora.org wrote: > Adding another point. > > On 2018-04-17 17:04, abhin...@codeaurora.org wrote: > > Hi Bjorn > > > > Apologies if the prev reply wasnt clear. > > > > Hope this one is. > > > > reply inline. > > > > On 2018-04-17 14:29, Bjorn Andersson wrote: > > > On Tue 17 Apr 11:21 PDT 2018, abhin...@codeaurora.org wrote: > > > > On 2018-04-16 23:13, Bjorn Andersson wrote: > > > [..] > > > > > If the panel isn't actually a piece of backlight hardware then it > > > > > should > > > > > not register a backlight device. Why do you need your own sysfs? > > > > > > > > > > Regards, > > > > > Bjorn > > > > [Abhinav] This particular panel isnt a piece of backlight hardware. > > > > But, we want to have our own sysfs to give flexibility to our > > > > userspace > > > > to write and read stuff for its proprietary uses. > > > > > > Please be more specific in your replies, no one will accept code that > > > "does stuff" and expecting a reviewer to spend time guessing or > > > pulling > > > the information out of you is a sure way to get your patches ignored. > > > > > > Exactly what kind of stuff are you talking about here and exactly > > > which > > > problem are you solving. > > > > > > > Thats how our downstream has been working so far and hence to > > > > maintain > > > > the compatibility would like to have it. > > > > > > Make your proprietary code work with the upstream kernel and you > > > shouldn't ever have to modify it. > > > > > > Regards, > > > Bjorn > > > > [Abhinav] We have a few userspace clients today for the backlight sysfs > > node > > which read/write directly to > > "/sys/class/backlight/panel0-backlight/brightness" > > When I said "stuff" I was only referring to the brightness value. > > This separate sysfs node allows us to validate those userspace features > > of ours > > which directly edit the backlight value on our reference platform. > > Since our reference platform uses this panel,hence having our own > > sysfs alias helps. > > Otherwise, whenever we try to use this panel with upstream code we > > will have to end up > > changing all those places in our userspace/framework to change the sysfs > > path. > > Hence we wanted to preserve that sysfs node name. > > The wled device is not created under /sys/class/backlight but under > > /sys/class/leds/wled. > > So we will have to change the name of this node across all our > > userspace. > > > > If this isnt a convincing reason enough to have its own sysfs node > > path, I will use > > your approach. > > > > Let me know your comments or if this is still not clear. > > > [Abhinav] We also might not be using the brightness value "as-it-is". > > We will potentially scale it up/down based on some requirements. > > If we do not have our own sysfs alias for this, how do we account for > providing this interface for our chipset specific backlight dependent > feature. > > Can you please comment on this? > What kind of requirements would this be and what do you mean with scale it up/down? As the current implementation is proposed any magic added on top would work for this panel driver and wouldn't be available for any other panel, which doesn't sounds optimal. Regards, Bjorn ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Freedreno] [DPU PATCH v2 2/2] drm/panel: add backlight control support for truly panel
Hi Bjorn Thanks very much for the detailed response. Yes, we decided that userspace hardcoding this node name is not a strong enough reason to register as another backlight device. Had one follow up question though. The QC WLED driver, drivers/leds/leds-qpnp-wled.c is not registering itself as a backlight device. It only registers as a led device. In that case, we cannot invoke of_find_backlight_by_node to get a handle on it. One question we have is , is it required that every WLED driver register itself as a backlight device too? In this case since it is not doing so, but we need to trigger it for the backlight. Is there any way we can do this? OR shall we just abandon the backlight control out of this driver entirely. Thanks Abhinav On 2018-04-18 21:00, Bjorn Andersson wrote: On Tue 17 Apr 17:04 PDT 2018, abhin...@codeaurora.org wrote: Hi Bjorn Apologies if the prev reply wasnt clear. Hope this one is. Much better, now we can discuss the actual issues :) reply inline. On 2018-04-17 14:29, Bjorn Andersson wrote: > On Tue 17 Apr 11:21 PDT 2018, abhin...@codeaurora.org wrote: > > On 2018-04-16 23:13, Bjorn Andersson wrote: > [..] > > > If the panel isn't actually a piece of backlight hardware then it should > > > not register a backlight device. Why do you need your own sysfs? > > > > > > Regards, > > > Bjorn > > [Abhinav] This particular panel isnt a piece of backlight hardware. > > But, we want to have our own sysfs to give flexibility to our > > userspace > > to write and read stuff for its proprietary uses. > > Please be more specific in your replies, no one will accept code that > "does stuff" and expecting a reviewer to spend time guessing or pulling > the information out of you is a sure way to get your patches ignored. > > Exactly what kind of stuff are you talking about here and exactly which > problem are you solving. > > > Thats how our downstream has been working so far and hence to maintain > > the compatibility would like to have it. > > Make your proprietary code work with the upstream kernel and you > shouldn't ever have to modify it. > > Regards, > Bjorn [Abhinav] We have a few userspace clients today for the backlight sysfs node which read/write directly to "/sys/class/backlight/panel0-backlight/brightness" When I said "stuff" I was only referring to the brightness value. This separate sysfs node allows us to validate those userspace features of ours which directly edit the backlight value on our reference platform. That's nice, but you're enforcing that either all panel drivers must implement this backlight wrapper or that your customers must modify their user space to match their backlight implementation. Since our reference platform uses this panel,hence having our own sysfs alias helps. Otherwise, whenever we try to use this panel with upstream code we will have to end up changing all those places in our userspace/framework to change the sysfs path. The actual problem comes down to "how does user space know the name of the backlight instance associated with the panel" and this is a valid question to pursue. But given your current design you could just scan for the one and only backlight device available in the system; as your use of the static name "panel0-backlight" doesn't allow multiple backlights anyway. If your goal is simply to ship your reference with something that you can show work, then just replace the hard coded panel0-backlight with the name of the wled backlight device. Customers can change panels as they wish, but in the event that they plug in a different backlight controller they would need to modify the code. Hence we wanted to preserve that sysfs node name. The wled device is not created under /sys/class/backlight but under /sys/class/leds/wled. So we will have to change the name of this node across all our userspace. Hard coding /sys/class/backlight/panel0-backlight in your user space instead of /sys/class/leds/wled is hardly a gain, in particular since the cost is 94 insertions - per panel driver. Regards, Bjorn ___ Freedreno mailing list freedr...@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Freedreno] [DPU PATCH v2 2/2] drm/panel: add backlight control support for truly panel
Thanks Daniel and Sean for your comments. Yes, the magic and algorithm is in userspace. After this discussion, it seems like the complexity of the userspace having to figure out which device node to use and to scale the backlight accordingly is not a strong enough reason to handle this in the driver as it seems like there are other opensource userspace clients doing the same thing. I will submit another patchset to use the method suggested by Bjorne. Thanks Abhinav On 2018-04-18 06:42, Sean Paul wrote: On Wed, Apr 18, 2018 at 11:52:18AM +0100, Daniel Thompson wrote: On Tue, Apr 17, 2018 at 05:42:04PM -0700, abhin...@codeaurora.org wrote: > Adding another point. > > On 2018-04-17 17:04, abhin...@codeaurora.org wrote: > > Hi Bjorn > > > > Apologies if the prev reply wasnt clear. > > > > Hope this one is. > > > > reply inline. > > > > On 2018-04-17 14:29, Bjorn Andersson wrote: > > > On Tue 17 Apr 11:21 PDT 2018, abhin...@codeaurora.org wrote: > > > > On 2018-04-16 23:13, Bjorn Andersson wrote: > > > [..] > > > > > If the panel isn't actually a piece of backlight hardware then it should > > > > > not register a backlight device. Why do you need your own sysfs? > > > > > > > > > > Regards, > > > > > Bjorn > > > > [Abhinav] This particular panel isnt a piece of backlight hardware. > > > > But, we want to have our own sysfs to give flexibility to our > > > > userspace > > > > to write and read stuff for its proprietary uses. > > > > > > Please be more specific in your replies, no one will accept code that > > > "does stuff" and expecting a reviewer to spend time guessing or > > > pulling > > > the information out of you is a sure way to get your patches ignored. > > > > > > Exactly what kind of stuff are you talking about here and exactly > > > which > > > problem are you solving. > > > > > > > Thats how our downstream has been working so far and hence to > > > > maintain > > > > the compatibility would like to have it. > > > > > > Make your proprietary code work with the upstream kernel and you > > > shouldn't ever have to modify it. > > > > > > Regards, > > > Bjorn > > > > [Abhinav] We have a few userspace clients today for the backlight sysfs > > node > > which read/write directly to > > "/sys/class/backlight/panel0-backlight/brightness" > > When I said "stuff" I was only referring to the brightness value. > > This separate sysfs node allows us to validate those userspace features > > of ours > > which directly edit the backlight value on our reference platform. > > Since our reference platform uses this panel,hence having our own > > sysfs alias helps. > > Otherwise, whenever we try to use this panel with upstream code we > > will have to end up > > changing all those places in our userspace/framework to change the sysfs > > path. > > Hence we wanted to preserve that sysfs node name. > > The wled device is not created under /sys/class/backlight but under > > /sys/class/leds/wled. > > So we will have to change the name of this node across all our > > userspace. > > > > If this isnt a convincing reason enough to have its own sysfs node > > path, I will use > > your approach. > > > > Let me know your comments or if this is still not clear. > > > [Abhinav] We also might not be using the brightness value "as-it-is". > > We will potentially scale it up/down based on some requirements. > > If we do not have our own sysfs alias for this, how do we account for > providing this interface for our chipset specific backlight dependent > feature. > > Can you please comment on this? Not easily. It's rather unclear what this chipset specific backlight dependent feature you have alluded to is so how can we suggest how to control or model it in the upstream kernel? The code is here: https://gitlab.freedesktop.org/seanpaul/dpu-staging/blob/mtp-squashed/drivers/gpu/drm/msm/dsi-staging/dsi_display.c#L76 AFAICT, there's nothing fancy in the kernel aside from scaling the brightness level down twice. I assume the magic is in userspace. My initial reaction was that the scaling factor should just be applied in userspace. Especially since the scaling factor reduces the resolution of the backlight, and that's not immediately obvious by looking at "brightness". Sean I can make a guess that is might be to do with brightness curves... but I'd really prefer not to have to guess. There are some problems with the current interface because it is not well defined with the brightness control is linear or logarithmic/perceptual (patches welcome) but for other common embedded backlights (pwm_bl particularly) we expect calibration of the brightness curve to be a job for the device tree (because it is a property of the hardware it can be described in the DT) and there are patches pending to improve this. Daniel. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel __
Re: [Freedreno] [DPU PATCH v2 2/2] drm/panel: add backlight control support for truly panel
On Wed, Apr 18, 2018 at 11:52:18AM +0100, Daniel Thompson wrote: > On Tue, Apr 17, 2018 at 05:42:04PM -0700, abhin...@codeaurora.org wrote: > > Adding another point. > > > > On 2018-04-17 17:04, abhin...@codeaurora.org wrote: > > > Hi Bjorn > > > > > > Apologies if the prev reply wasnt clear. > > > > > > Hope this one is. > > > > > > reply inline. > > > > > > On 2018-04-17 14:29, Bjorn Andersson wrote: > > > > On Tue 17 Apr 11:21 PDT 2018, abhin...@codeaurora.org wrote: > > > > > On 2018-04-16 23:13, Bjorn Andersson wrote: > > > > [..] > > > > > > If the panel isn't actually a piece of backlight hardware then it > > > > > > should > > > > > > not register a backlight device. Why do you need your own sysfs? > > > > > > > > > > > > Regards, > > > > > > Bjorn > > > > > [Abhinav] This particular panel isnt a piece of backlight hardware. > > > > > But, we want to have our own sysfs to give flexibility to our > > > > > userspace > > > > > to write and read stuff for its proprietary uses. > > > > > > > > Please be more specific in your replies, no one will accept code that > > > > "does stuff" and expecting a reviewer to spend time guessing or > > > > pulling > > > > the information out of you is a sure way to get your patches ignored. > > > > > > > > Exactly what kind of stuff are you talking about here and exactly > > > > which > > > > problem are you solving. > > > > > > > > > Thats how our downstream has been working so far and hence to > > > > > maintain > > > > > the compatibility would like to have it. > > > > > > > > Make your proprietary code work with the upstream kernel and you > > > > shouldn't ever have to modify it. > > > > > > > > Regards, > > > > Bjorn > > > > > > [Abhinav] We have a few userspace clients today for the backlight sysfs > > > node > > > which read/write directly to > > > "/sys/class/backlight/panel0-backlight/brightness" > > > When I said "stuff" I was only referring to the brightness value. > > > This separate sysfs node allows us to validate those userspace features > > > of ours > > > which directly edit the backlight value on our reference platform. > > > Since our reference platform uses this panel,hence having our own > > > sysfs alias helps. > > > Otherwise, whenever we try to use this panel with upstream code we > > > will have to end up > > > changing all those places in our userspace/framework to change the sysfs > > > path. > > > Hence we wanted to preserve that sysfs node name. > > > The wled device is not created under /sys/class/backlight but under > > > /sys/class/leds/wled. > > > So we will have to change the name of this node across all our > > > userspace. > > > > > > If this isnt a convincing reason enough to have its own sysfs node > > > path, I will use > > > your approach. > > > > > > Let me know your comments or if this is still not clear. > > > > > [Abhinav] We also might not be using the brightness value "as-it-is". > > > > We will potentially scale it up/down based on some requirements. > > > > If we do not have our own sysfs alias for this, how do we account for > > providing this interface for our chipset specific backlight dependent > > feature. > > > > Can you please comment on this? > > Not easily. It's rather unclear what this chipset specific backlight > dependent feature you have alluded to is so how can we suggest how to > control or model it in the upstream kernel? > The code is here: https://gitlab.freedesktop.org/seanpaul/dpu-staging/blob/mtp-squashed/drivers/gpu/drm/msm/dsi-staging/dsi_display.c#L76 AFAICT, there's nothing fancy in the kernel aside from scaling the brightness level down twice. I assume the magic is in userspace. My initial reaction was that the scaling factor should just be applied in userspace. Especially since the scaling factor reduces the resolution of the backlight, and that's not immediately obvious by looking at "brightness". Sean > I can make a guess that is might be to do with brightness curves... but > I'd really prefer not to have to guess. > > There are some problems with the current interface because it is not > well defined with the brightness control is linear or > logarithmic/perceptual (patches welcome) but for other common embedded > backlights (pwm_bl particularly) we expect calibration of the > brightness curve to be a job for the device tree (because it is a > property of the hardware it can be described in the DT) and there are > patches pending to improve this. > > > Daniel. > ___ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Sean Paul, Software Engineer, Google / Chromium OS ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Freedreno] [DPU PATCH v2 2/2] drm/panel: add backlight control support for truly panel
On Tue, Apr 17, 2018 at 05:04:56PM -0700, abhin...@codeaurora.org wrote: > Hi Bjorn > > Apologies if the prev reply wasnt clear. > > Hope this one is. > > reply inline. > > On 2018-04-17 14:29, Bjorn Andersson wrote: > > On Tue 17 Apr 11:21 PDT 2018, abhin...@codeaurora.org wrote: > > > On 2018-04-16 23:13, Bjorn Andersson wrote: > > [..] > > > > If the panel isn't actually a piece of backlight hardware then it should > > > > not register a backlight device. Why do you need your own sysfs? > > > > > > > > Regards, > > > > Bjorn > > > [Abhinav] This particular panel isnt a piece of backlight hardware. > > > But, we want to have our own sysfs to give flexibility to our > > > userspace > > > to write and read stuff for its proprietary uses. > > > > Please be more specific in your replies, no one will accept code that > > "does stuff" and expecting a reviewer to spend time guessing or pulling > > the information out of you is a sure way to get your patches ignored. > > > > Exactly what kind of stuff are you talking about here and exactly which > > problem are you solving. > > > > > Thats how our downstream has been working so far and hence to maintain > > > the compatibility would like to have it. > > > > Make your proprietary code work with the upstream kernel and you > > shouldn't ever have to modify it. > > > > Regards, > > Bjorn > > [Abhinav] We have a few userspace clients today for the backlight sysfs node > which read/write directly to > "/sys/class/backlight/panel0-backlight/brightness" > When I said "stuff" I was only referring to the brightness value. > This separate sysfs node allows us to validate those userspace features of > ours > which directly edit the backlight value on our reference platform. > Since our reference platform uses this panel,hence having our own sysfs > alias helps. > Otherwise, whenever we try to use this panel with upstream code we will have > to end up > changing all those places in our userspace/framework to change the sysfs > path. > Hence we wanted to preserve that sysfs node name. > The wled device is not created under /sys/class/backlight but under > /sys/class/leds/wled. > So we will have to change the name of this node across all our userspace. As I mentioned on the previous review, coding around closed source userspace isn't something that we want to do. It's hard enough to accommodate open source u/s as it is. Sean > > If this isnt a convincing reason enough to have its own sysfs node path, I > will use > your approach. > > Let me know your comments or if this is still not clear. > > > ___ > > Freedreno mailing list > > freedr...@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/freedreno > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Sean Paul, Software Engineer, Google / Chromium OS ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Freedreno] [DPU PATCH v2 2/2] drm/panel: add backlight control support for truly panel
On Tue, Apr 17, 2018 at 05:42:04PM -0700, abhin...@codeaurora.org wrote: > Adding another point. > > On 2018-04-17 17:04, abhin...@codeaurora.org wrote: > > Hi Bjorn > > > > Apologies if the prev reply wasnt clear. > > > > Hope this one is. > > > > reply inline. > > > > On 2018-04-17 14:29, Bjorn Andersson wrote: > > > On Tue 17 Apr 11:21 PDT 2018, abhin...@codeaurora.org wrote: > > > > On 2018-04-16 23:13, Bjorn Andersson wrote: > > > [..] > > > > > If the panel isn't actually a piece of backlight hardware then it > > > > > should > > > > > not register a backlight device. Why do you need your own sysfs? > > > > > > > > > > Regards, > > > > > Bjorn > > > > [Abhinav] This particular panel isnt a piece of backlight hardware. > > > > But, we want to have our own sysfs to give flexibility to our > > > > userspace > > > > to write and read stuff for its proprietary uses. > > > > > > Please be more specific in your replies, no one will accept code that > > > "does stuff" and expecting a reviewer to spend time guessing or > > > pulling > > > the information out of you is a sure way to get your patches ignored. > > > > > > Exactly what kind of stuff are you talking about here and exactly > > > which > > > problem are you solving. > > > > > > > Thats how our downstream has been working so far and hence to > > > > maintain > > > > the compatibility would like to have it. > > > > > > Make your proprietary code work with the upstream kernel and you > > > shouldn't ever have to modify it. > > > > > > Regards, > > > Bjorn > > > > [Abhinav] We have a few userspace clients today for the backlight sysfs > > node > > which read/write directly to > > "/sys/class/backlight/panel0-backlight/brightness" > > When I said "stuff" I was only referring to the brightness value. > > This separate sysfs node allows us to validate those userspace features > > of ours > > which directly edit the backlight value on our reference platform. > > Since our reference platform uses this panel,hence having our own > > sysfs alias helps. > > Otherwise, whenever we try to use this panel with upstream code we > > will have to end up > > changing all those places in our userspace/framework to change the sysfs > > path. > > Hence we wanted to preserve that sysfs node name. > > The wled device is not created under /sys/class/backlight but under > > /sys/class/leds/wled. > > So we will have to change the name of this node across all our > > userspace. > > > > If this isnt a convincing reason enough to have its own sysfs node > > path, I will use > > your approach. > > > > Let me know your comments or if this is still not clear. > > > [Abhinav] We also might not be using the brightness value "as-it-is". > > We will potentially scale it up/down based on some requirements. > > If we do not have our own sysfs alias for this, how do we account for > providing this interface for our chipset specific backlight dependent > feature. > > Can you please comment on this? Not easily. It's rather unclear what this chipset specific backlight dependent feature you have alluded to is so how can we suggest how to control or model it in the upstream kernel? I can make a guess that is might be to do with brightness curves... but I'd really prefer not to have to guess. There are some problems with the current interface because it is not well defined with the brightness control is linear or logarithmic/perceptual (patches welcome) but for other common embedded backlights (pwm_bl particularly) we expect calibration of the brightness curve to be a job for the device tree (because it is a property of the hardware it can be described in the DT) and there are patches pending to improve this. Daniel. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Freedreno] [DPU PATCH v2 2/2] drm/panel: add backlight control support for truly panel
On Tue 17 Apr 11:21 PDT 2018, abhin...@codeaurora.org wrote: > On 2018-04-16 23:13, Bjorn Andersson wrote: [..] > > If the panel isn't actually a piece of backlight hardware then it should > > not register a backlight device. Why do you need your own sysfs? > > > > Regards, > > Bjorn > [Abhinav] This particular panel isnt a piece of backlight hardware. > But, we want to have our own sysfs to give flexibility to our userspace > to write and read stuff for its proprietary uses. Please be more specific in your replies, no one will accept code that "does stuff" and expecting a reviewer to spend time guessing or pulling the information out of you is a sure way to get your patches ignored. Exactly what kind of stuff are you talking about here and exactly which problem are you solving. > Thats how our downstream has been working so far and hence to maintain > the compatibility would like to have it. Make your proprietary code work with the upstream kernel and you shouldn't ever have to modify it. Regards, Bjorn ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Freedreno] [DPU PATCH v2 2/2] drm/panel: add backlight control support for truly panel
Testing my responsde
On 04/17/2018 11:21 AM, abhin...@codeaurora.org wrote:
Hi Bjorn
Thanks for the comments.
Reply inline.
On 2018-04-16 23:13, Bjorn Andersson wrote:
On Mon 16 Apr 15:45 PDT 2018, abhin...@codeaurora.org wrote:
Hi Bjorn
Thanks for the review.
Reply inline.
On 2018-04-16 09:41, Bjorn Andersson wrote:
> On Sat 14 Apr 00:25 PDT 2018, Abhinav Kumar wrote:
>
> > Register truly panel as a backlight led device and
> > provide methods to control its backlight operation.
> >
> > Changes in v2:
> > - Removed redundant NULL checks
> > - Arranged headers alphabetically
> > - Formatting fixes
>
> The change log goes below the "---" line.
>
[Abhinav] As sean mentioned, its quite common to list it to view it
in the
log.
This practice has been followed by quite a few in DRM
Another reference here https://patchwork.freedesktop.org/patch/211361/
If that's the practice in DRM land, then that's what you should do.
> >
> > Signed-off-by: Abhinav Kumar
> > ---
> [..]
> > +static int truly_backlight_setup(struct truly_wqxga *ctx)
> > +{
> > +struct backlight_properties props;
> > +char bl_node_name[BL_NODE_NAME_SIZE];
> > +
> > +if (!ctx->backlight) {
> > +memset(&props, 0, sizeof(props));
> > +props.type = BACKLIGHT_RAW;
> > +props.power = FB_BLANK_UNBLANK;
> > +props.max_brightness = 4096;
> > +
> > +snprintf(bl_node_name, BL_NODE_NAME_SIZE,
"panel%u-backlight",
> > + PRIM_DISPLAY_NODE);
> > +
> > +ctx->backlight = backlight_device_register(bl_node_name,
> > +ctx->dev, ctx,
> > +&truly_backlight_device_ops, &props);
> > +
> > +if (IS_ERR_OR_NULL(ctx->backlight)) {
> > +pr_err("Failed to register backlight\n");
> > +ctx->backlight = NULL;
> > +return -ENODEV;
> > +}
> > +
> > +/* Register with the LED driver interface */
> > +led_trigger_register_simple("bkl-trigger", &ctx->wled);
> > +
> > +if (!ctx->wled) {
> > +pr_err("backlight led registration failed\n");
> > +return -ENODEV;
> > +}
>
> It seems like you're registering a backlight driver for the sake of
> invoking the LED backlight trigger to control the WLED.
>
> The WLED is a backlight driver, so all you should have to do is
add the
> following line to your probe:
>
> ctx->backlight = devm_of_find_backlight(dev);
>
> and then add "backlight = <&wled>" to your dt node.
>
> Regards,
> Bjorn
[Abhinav] Thats not the only purpose of backlight_device_register().
We want to hook up our panel with the parent backlight driver in
drivers/video/backlight/backlight.c and also route all the
update_backlight_status()
calls through the sysfs of the newly registered node.
The of_find_backlight() method doesnt seem to allow us to register
our own
sysfs method.
Are you saying that you want to be able to create an alias for the wled
entry already in /sys/class/backlight named panel0-backlight?
BTW, this isnt something which we are doing uniquely.
There are other panels which seem to be doing this :
drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c
What seems to be going on in the s6e3ha2 driver is that the hardware has
some sort of builtin backlight control, so the driver is creating a
backlight driver for the purpose of exposing those controls.
Can you please comment on whether we can have our own sysfs without
the device_register()?
If the panel isn't actually a piece of backlight hardware then it should
not register a backlight device. Why do you need your own sysfs?
Regards,
Bjorn
[Abhinav] This particular panel isnt a piece of backlight hardware.
But, we want to have our own sysfs to give flexibility to our userspace
to write and read stuff for its proprietary uses.
Thats how our downstream has been working so far and hence to maintain
the compatibility would like to have it.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
To unsubscribe from this list: send the line "unsubscribe
linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Freedreno] [DPU PATCH v2 2/2] drm/panel: add backlight control support for truly panel
Adding another point. On 2018-04-17 17:04, abhin...@codeaurora.org wrote: Hi Bjorn Apologies if the prev reply wasnt clear. Hope this one is. reply inline. On 2018-04-17 14:29, Bjorn Andersson wrote: On Tue 17 Apr 11:21 PDT 2018, abhin...@codeaurora.org wrote: On 2018-04-16 23:13, Bjorn Andersson wrote: [..] > If the panel isn't actually a piece of backlight hardware then it should > not register a backlight device. Why do you need your own sysfs? > > Regards, > Bjorn [Abhinav] This particular panel isnt a piece of backlight hardware. But, we want to have our own sysfs to give flexibility to our userspace to write and read stuff for its proprietary uses. Please be more specific in your replies, no one will accept code that "does stuff" and expecting a reviewer to spend time guessing or pulling the information out of you is a sure way to get your patches ignored. Exactly what kind of stuff are you talking about here and exactly which problem are you solving. Thats how our downstream has been working so far and hence to maintain the compatibility would like to have it. Make your proprietary code work with the upstream kernel and you shouldn't ever have to modify it. Regards, Bjorn [Abhinav] We have a few userspace clients today for the backlight sysfs node which read/write directly to "/sys/class/backlight/panel0-backlight/brightness" When I said "stuff" I was only referring to the brightness value. This separate sysfs node allows us to validate those userspace features of ours which directly edit the backlight value on our reference platform. Since our reference platform uses this panel,hence having our own sysfs alias helps. Otherwise, whenever we try to use this panel with upstream code we will have to end up changing all those places in our userspace/framework to change the sysfs path. Hence we wanted to preserve that sysfs node name. The wled device is not created under /sys/class/backlight but under /sys/class/leds/wled. So we will have to change the name of this node across all our userspace. If this isnt a convincing reason enough to have its own sysfs node path, I will use your approach. Let me know your comments or if this is still not clear. [Abhinav] We also might not be using the brightness value "as-it-is". We will potentially scale it up/down based on some requirements. If we do not have our own sysfs alias for this, how do we account for providing this interface for our chipset specific backlight dependent feature. Can you please comment on this? ___ Freedreno mailing list freedr...@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Freedreno] [DPU PATCH v2 2/2] drm/panel: add backlight control support for truly panel
Hi Bjorn Apologies if the prev reply wasnt clear. Hope this one is. reply inline. On 2018-04-17 14:29, Bjorn Andersson wrote: On Tue 17 Apr 11:21 PDT 2018, abhin...@codeaurora.org wrote: On 2018-04-16 23:13, Bjorn Andersson wrote: [..] > If the panel isn't actually a piece of backlight hardware then it should > not register a backlight device. Why do you need your own sysfs? > > Regards, > Bjorn [Abhinav] This particular panel isnt a piece of backlight hardware. But, we want to have our own sysfs to give flexibility to our userspace to write and read stuff for its proprietary uses. Please be more specific in your replies, no one will accept code that "does stuff" and expecting a reviewer to spend time guessing or pulling the information out of you is a sure way to get your patches ignored. Exactly what kind of stuff are you talking about here and exactly which problem are you solving. Thats how our downstream has been working so far and hence to maintain the compatibility would like to have it. Make your proprietary code work with the upstream kernel and you shouldn't ever have to modify it. Regards, Bjorn [Abhinav] We have a few userspace clients today for the backlight sysfs node which read/write directly to "/sys/class/backlight/panel0-backlight/brightness" When I said "stuff" I was only referring to the brightness value. This separate sysfs node allows us to validate those userspace features of ours which directly edit the backlight value on our reference platform. Since our reference platform uses this panel,hence having our own sysfs alias helps. Otherwise, whenever we try to use this panel with upstream code we will have to end up changing all those places in our userspace/framework to change the sysfs path. Hence we wanted to preserve that sysfs node name. The wled device is not created under /sys/class/backlight but under /sys/class/leds/wled. So we will have to change the name of this node across all our userspace. If this isnt a convincing reason enough to have its own sysfs node path, I will use your approach. Let me know your comments or if this is still not clear. ___ Freedreno mailing list freedr...@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Freedreno] [DPU PATCH v2 2/2] drm/panel: add backlight control support for truly panel
Hi Bjorn
Thanks for the comments.
Reply inline.
On 2018-04-16 23:13, Bjorn Andersson wrote:
On Mon 16 Apr 15:45 PDT 2018, abhin...@codeaurora.org wrote:
Hi Bjorn
Thanks for the review.
Reply inline.
On 2018-04-16 09:41, Bjorn Andersson wrote:
> On Sat 14 Apr 00:25 PDT 2018, Abhinav Kumar wrote:
>
> > Register truly panel as a backlight led device and
> > provide methods to control its backlight operation.
> >
> > Changes in v2:
> > - Removed redundant NULL checks
> > - Arranged headers alphabetically
> > - Formatting fixes
>
> The change log goes below the "---" line.
>
[Abhinav] As sean mentioned, its quite common to list it to view it in
the
log.
This practice has been followed by quite a few in DRM
Another reference here https://patchwork.freedesktop.org/patch/211361/
If that's the practice in DRM land, then that's what you should do.
> >
> > Signed-off-by: Abhinav Kumar
> > ---
> [..]
> > +static int truly_backlight_setup(struct truly_wqxga *ctx)
> > +{
> > + struct backlight_properties props;
> > + char bl_node_name[BL_NODE_NAME_SIZE];
> > +
> > + if (!ctx->backlight) {
> > + memset(&props, 0, sizeof(props));
> > + props.type = BACKLIGHT_RAW;
> > + props.power = FB_BLANK_UNBLANK;
> > + props.max_brightness = 4096;
> > +
> > + snprintf(bl_node_name, BL_NODE_NAME_SIZE, "panel%u-backlight",
> > + PRIM_DISPLAY_NODE);
> > +
> > + ctx->backlight = backlight_device_register(bl_node_name,
> > + ctx->dev, ctx,
> > + &truly_backlight_device_ops, &props);
> > +
> > + if (IS_ERR_OR_NULL(ctx->backlight)) {
> > + pr_err("Failed to register backlight\n");
> > + ctx->backlight = NULL;
> > + return -ENODEV;
> > + }
> > +
> > + /* Register with the LED driver interface */
> > + led_trigger_register_simple("bkl-trigger", &ctx->wled);
> > +
> > + if (!ctx->wled) {
> > + pr_err("backlight led registration failed\n");
> > + return -ENODEV;
> > + }
>
> It seems like you're registering a backlight driver for the sake of
> invoking the LED backlight trigger to control the WLED.
>
> The WLED is a backlight driver, so all you should have to do is add the
> following line to your probe:
>
>ctx->backlight = devm_of_find_backlight(dev);
>
> and then add "backlight = <&wled>" to your dt node.
>
> Regards,
> Bjorn
[Abhinav] Thats not the only purpose of backlight_device_register().
We want to hook up our panel with the parent backlight driver in
drivers/video/backlight/backlight.c and also route all the
update_backlight_status()
calls through the sysfs of the newly registered node.
The of_find_backlight() method doesnt seem to allow us to register our
own
sysfs method.
Are you saying that you want to be able to create an alias for the wled
entry already in /sys/class/backlight named panel0-backlight?
BTW, this isnt something which we are doing uniquely.
There are other panels which seem to be doing this :
drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c
What seems to be going on in the s6e3ha2 driver is that the hardware
has
some sort of builtin backlight control, so the driver is creating a
backlight driver for the purpose of exposing those controls.
Can you please comment on whether we can have our own sysfs without
the device_register()?
If the panel isn't actually a piece of backlight hardware then it
should
not register a backlight device. Why do you need your own sysfs?
Regards,
Bjorn
[Abhinav] This particular panel isnt a piece of backlight hardware.
But, we want to have our own sysfs to give flexibility to our userspace
to write and read stuff for its proprietary uses.
Thats how our downstream has been working so far and hence to maintain
the compatibility would like to have it.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Freedreno] [DPU PATCH v2 2/2] drm/panel: add backlight control support for truly panel
On Mon 16 Apr 15:45 PDT 2018, abhin...@codeaurora.org wrote:
> Hi Bjorn
>
> Thanks for the review.
>
> Reply inline.
>
> On 2018-04-16 09:41, Bjorn Andersson wrote:
> > On Sat 14 Apr 00:25 PDT 2018, Abhinav Kumar wrote:
> >
> > > Register truly panel as a backlight led device and
> > > provide methods to control its backlight operation.
> > >
> > > Changes in v2:
> > > - Removed redundant NULL checks
> > > - Arranged headers alphabetically
> > > - Formatting fixes
> >
> > The change log goes below the "---" line.
> >
> [Abhinav] As sean mentioned, its quite common to list it to view it in the
> log.
> This practice has been followed by quite a few in DRM
> Another reference here https://patchwork.freedesktop.org/patch/211361/
>
If that's the practice in DRM land, then that's what you should do.
> > >
> > > Signed-off-by: Abhinav Kumar
> > > ---
> > [..]
> > > +static int truly_backlight_setup(struct truly_wqxga *ctx)
> > > +{
> > > + struct backlight_properties props;
> > > + char bl_node_name[BL_NODE_NAME_SIZE];
> > > +
> > > + if (!ctx->backlight) {
> > > + memset(&props, 0, sizeof(props));
> > > + props.type = BACKLIGHT_RAW;
> > > + props.power = FB_BLANK_UNBLANK;
> > > + props.max_brightness = 4096;
> > > +
> > > + snprintf(bl_node_name, BL_NODE_NAME_SIZE, "panel%u-backlight",
> > > + PRIM_DISPLAY_NODE);
> > > +
> > > + ctx->backlight = backlight_device_register(bl_node_name,
> > > + ctx->dev, ctx,
> > > + &truly_backlight_device_ops, &props);
> > > +
> > > + if (IS_ERR_OR_NULL(ctx->backlight)) {
> > > + pr_err("Failed to register backlight\n");
> > > + ctx->backlight = NULL;
> > > + return -ENODEV;
> > > + }
> > > +
> > > + /* Register with the LED driver interface */
> > > + led_trigger_register_simple("bkl-trigger", &ctx->wled);
> > > +
> > > + if (!ctx->wled) {
> > > + pr_err("backlight led registration failed\n");
> > > + return -ENODEV;
> > > + }
> >
> > It seems like you're registering a backlight driver for the sake of
> > invoking the LED backlight trigger to control the WLED.
> >
> > The WLED is a backlight driver, so all you should have to do is add the
> > following line to your probe:
> >
> > ctx->backlight = devm_of_find_backlight(dev);
> >
> > and then add "backlight = <&wled>" to your dt node.
> >
> > Regards,
> > Bjorn
> [Abhinav] Thats not the only purpose of backlight_device_register().
> We want to hook up our panel with the parent backlight driver in
> drivers/video/backlight/backlight.c and also route all the
> update_backlight_status()
> calls through the sysfs of the newly registered node.
>
> The of_find_backlight() method doesnt seem to allow us to register our own
> sysfs method.
>
Are you saying that you want to be able to create an alias for the wled
entry already in /sys/class/backlight named panel0-backlight?
> BTW, this isnt something which we are doing uniquely.
> There are other panels which seem to be doing this :
>
> drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c
>
What seems to be going on in the s6e3ha2 driver is that the hardware has
some sort of builtin backlight control, so the driver is creating a
backlight driver for the purpose of exposing those controls.
> Can you please comment on whether we can have our own sysfs without
> the device_register()?
>
If the panel isn't actually a piece of backlight hardware then it should
not register a backlight device. Why do you need your own sysfs?
Regards,
Bjorn
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Freedreno] [DPU PATCH v2 2/2] drm/panel: add backlight control support for truly panel
Hi Bjorn
Thanks for the review.
Reply inline.
On 2018-04-16 09:41, Bjorn Andersson wrote:
On Sat 14 Apr 00:25 PDT 2018, Abhinav Kumar wrote:
Register truly panel as a backlight led device and
provide methods to control its backlight operation.
Changes in v2:
- Removed redundant NULL checks
- Arranged headers alphabetically
- Formatting fixes
The change log goes below the "---" line.
[Abhinav] As sean mentioned, its quite common to list it to view it in
the log.
This practice has been followed by quite a few in DRM
Another reference here https://patchwork.freedesktop.org/patch/211361/
Signed-off-by: Abhinav Kumar
---
[..]
+static int truly_backlight_setup(struct truly_wqxga *ctx)
+{
+ struct backlight_properties props;
+ char bl_node_name[BL_NODE_NAME_SIZE];
+
+ if (!ctx->backlight) {
+ memset(&props, 0, sizeof(props));
+ props.type = BACKLIGHT_RAW;
+ props.power = FB_BLANK_UNBLANK;
+ props.max_brightness = 4096;
+
+ snprintf(bl_node_name, BL_NODE_NAME_SIZE, "panel%u-backlight",
+PRIM_DISPLAY_NODE);
+
+ ctx->backlight = backlight_device_register(bl_node_name,
+ ctx->dev, ctx,
+ &truly_backlight_device_ops, &props);
+
+ if (IS_ERR_OR_NULL(ctx->backlight)) {
+ pr_err("Failed to register backlight\n");
+ ctx->backlight = NULL;
+ return -ENODEV;
+ }
+
+ /* Register with the LED driver interface */
+ led_trigger_register_simple("bkl-trigger", &ctx->wled);
+
+ if (!ctx->wled) {
+ pr_err("backlight led registration failed\n");
+ return -ENODEV;
+ }
It seems like you're registering a backlight driver for the sake of
invoking the LED backlight trigger to control the WLED.
The WLED is a backlight driver, so all you should have to do is add the
following line to your probe:
ctx->backlight = devm_of_find_backlight(dev);
and then add "backlight = <&wled>" to your dt node.
Regards,
Bjorn
[Abhinav] Thats not the only purpose of backlight_device_register().
We want to hook up our panel with the parent backlight driver in
drivers/video/backlight/backlight.c and also route all the
update_backlight_status()
calls through the sysfs of the newly registered node.
The of_find_backlight() method doesnt seem to allow us to register our
own
sysfs method.
BTW, this isnt something which we are doing uniquely.
There are other panels which seem to be doing this :
drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c
Can you please comment on whether we can have our own sysfs without
the device_register()?
___
Freedreno mailing list
freedr...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
