Messages in this thread | | | Date | Fri, 18 Oct 2019 10:33:08 +0530 | From | kgunda@codeauro ... | Subject | Re: [PATCH V7 6/6] backlight: qcom-wled: Add auto string detection logic |
| |
On 2019-10-17 19:00, Lee Jones wrote: > On Thu, 17 Oct 2019, kgunda@codeaurora.org wrote: > >> On 2019-10-17 17:56, Lee Jones wrote: >> > On Wed, 16 Oct 2019, Kiran Gunda wrote: >> > >> > > The auto string detection algorithm checks if the current WLED >> > > sink configuration is valid. It tries enabling every sink and >> > > checks if the OVP fault is observed. Based on this information >> > > it detects and enables the valid sink configuration. >> > > Auto calibration will be triggered when the OVP fault interrupts >> > > are seen frequently thereby it tries to fix the sink configuration. >> > > >> > > The auto-detection also kicks in when the connected LED string >> > > of the display-backlight malfunctions (because of damage) and >> > > requires the damaged string to be turned off to prevent the >> > > complete panel and/or board from being damaged. >> > > >> > > Signed-off-by: Kiran Gunda <kgunda@codeaurora.org> >> > > --- >> > > drivers/video/backlight/qcom-wled.c | 410 >> > > +++++++++++++++++++++++++++++++++++- >> > > 1 file changed, 404 insertions(+), 6 deletions(-) >> > > >> > > diff --git a/drivers/video/backlight/qcom-wled.c >> > > b/drivers/video/backlight/qcom-wled.c >> > > index b5b125c..ff7c409 100644 >> > > --- a/drivers/video/backlight/qcom-wled.c >> > > +++ b/drivers/video/backlight/qcom-wled.c > > [...] > >> > > + if (int_sts & WLED3_CTRL_REG_OVP_FAULT_STATUS) >> > > + dev_dbg(wled->dev, "WLED OVP fault detected with SINK %d\n", >> > > + i + 1); >> > >> > I haven't reviewed the whole patch, but this caught my eye. >> > >> > I think this should be upgraded to dev_warn(). >> > >> Thought of keeping these messages silent, Because the string >> configuration >> will be corrected in this >> and informing it at end of the auto string detection. > > [...] > >> > > + } else { >> > > + dev_warn(wled->dev, "New WLED string configuration found %x\n", >> > > + sink_valid); >> > >> > Why would the user care about this? Is it not normal? >> > >> Actually, it comes here if the user provided string configuration in >> the >> device tree is in-correct. >> That's why just informing the user about the right string >> configuration, >> after the auto string detection. > > Then I think we need to be more forthcoming. Tell the user the > configuration is incorrect and what you've done to rectify it. > > "XXX is not a valid configuration - using YYY instead" > Sure. Will update it in the next series. > [...] > >> > > +static int wled_configure_ovp_irq(struct wled *wled, >> > > + struct platform_device *pdev) >> > > +{ >> > > + int rc; >> > > + u32 val; >> > > + >> > > + wled->ovp_irq = platform_get_irq_byname(pdev, "ovp"); >> > > + if (wled->ovp_irq < 0) { >> > > + dev_dbg(&pdev->dev, "ovp irq is not used\n"); >> > >> > I assume this is optional. What happens if no IRQ is provided? >> > >> Here OVP IRQ is used to detect the wrong string detection. If it is >> not >> provided the auto string detection logic won't work. > > "OVP IRQ not found - disabling automatic string detection" > Sure. Will update it in the next series. >> > If, for instance, polling mode is enabled, maybe something like this >> > would be better? >> > >> > dev_warn(&pdev->dev, "No IRQ found, falling back to polling >> > mode\n");
| |