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 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
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno
_______________________________________________
dri-devel mailing list
dri-de...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
