From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3D511C43461 for ; Thu, 29 Apr 2021 00:12:07 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 1FFF061449 for ; Thu, 29 Apr 2021 00:12:07 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231858AbhD2AMv (ORCPT ); Wed, 28 Apr 2021 20:12:51 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49726 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232095AbhD2AMv (ORCPT ); Wed, 28 Apr 2021 20:12:51 -0400 Received: from mail-oi1-x232.google.com (mail-oi1-x232.google.com [IPv6:2607:f8b0:4864:20::232]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 3BC2BC06138D for ; Wed, 28 Apr 2021 17:12:04 -0700 (PDT) Received: by mail-oi1-x232.google.com with SMTP id i11so2856384oig.8 for ; Wed, 28 Apr 2021 17:12:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=SBQkZZ+AM0rdRiolN8pMUJvJaHdqRcP6uvGtBB7hbVU=; b=ZoTQshL6LSUn3Lj4/ZAJK9Rw3XfU8aTAMNv0jvQ6LotAVOdLClfvB+3CeSpCEioye0 Xtopma3PqjaDfDfsQayOuP4TbqJWt9iSo+X07bTjJZrtqsrpD62HHGpNjKK8gv1NaNtS j+jH6knXMDHPhdU/nGtL0OIOSn/aAekpdbSYjfJzWoxRqf7OTjh4v4eEpfubQ2W/pv6d Z+RgHfkEWL9+um/+yz3n1MeBS04aaAlWZ1J852+CB8wQARNwCvxGxntYdB7vjtAt8hzm umKD+6zJY4V0bBk45x5b0yvwel00U0JtsWLN76hqI9/nZIU2JQov5EqcHbyL4pPBuLAT GL4w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=SBQkZZ+AM0rdRiolN8pMUJvJaHdqRcP6uvGtBB7hbVU=; b=UJHyJI2GvxC7g6TOqSJpxYy5NmzP5Uod+HEkfjLBxxwK9TeldNDUBbL88Xaj9vv+LK WzrNhQEfqm/26Dkwq/RM0X/21YqCqiKAQc+UAC05qz4cBzBLZreggB5bULp0VcY1CT5i Lpng9ioBHRBvNgJ8ITm0Fyhw/0aJZL3kNOfY81/xMBAYZsIKboB4Cplrno6htCbProja O3SNj2QbJegGmksCGev5XfZiJXKAKod21lTA141mtqg/mF36niwHXjlKeIBV3r4h2TJj 2XG8QCgzkv8AAprqI8Gqu8o8l8uVK1JuIAi8IDDUYRoxS8z6GQIclA9DMoUe9YQV0gVQ cLxA== X-Gm-Message-State: AOAM532vNJ1LDUAU9eFEojAYYaKcGMnEie0ioNLkMyhfsquBweZelhfT imyhkU9LhGpDgYK7KN44gpKr6g== X-Google-Smtp-Source: ABdhPJxVHBqD0ADdSUFDRMR3ZR31HODm2e3SR+lV5ybAgqah5lCuPwSWrEvzbrK39xY68/DgooIHzA== X-Received: by 2002:aca:5845:: with SMTP id m66mr5007370oib.0.1619655123470; Wed, 28 Apr 2021 17:12:03 -0700 (PDT) Received: from builder.lan (104-57-184-186.lightspeed.austtx.sbcglobal.net. [104.57.184.186]) by smtp.gmail.com with ESMTPSA id r63sm305743oia.43.2021.04.28.17.12.02 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 28 Apr 2021 17:12:03 -0700 (PDT) Date: Wed, 28 Apr 2021 19:12:00 -0500 From: Bjorn Andersson To: Pavel Machek Cc: Dan Murphy , Rob Herring , Andy Gross , Thierry Reding , Uwe Kleine-K?nig , Lee Jones , Martin Botka , linux-leds@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org, linux-pwm@vger.kernel.org Subject: Re: [PATCH v6 2/4] leds: Add driver for Qualcomm LPG Message-ID: References: <20201021201224.3430546-1-bjorn.andersson@linaro.org> <20201021201224.3430546-3-bjorn.andersson@linaro.org> <20201029181357.GE26053@duo.ucw.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201029181357.GE26053@duo.ucw.cz> Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org On Thu 29 Oct 13:13 CDT 2020, Pavel Machek wrote: > Hi! > > > The Light Pulse Generator (LPG) is a PWM-block found in a wide range of > > PMICs from Qualcomm. It can operate on fixed parameters or based on a > > lookup-table, altering the duty cycle over time - which provides the > > means for e.g. hardware assisted transitions of LED brightness. > > > > Signed-off-by: Bjorn Andersson > > --- > > > > Changes since v5: > > - Make sure to not used the state of the last channel in a group to determine > > if the current sink should be active for all channels in the group. > > - Replacement of unsigned -1 with UINT_MAX > > - Work around potential overflow by using larger data types, instead of separate code paths > > - Use cpu_to_l16() rather than hand rolling them > > - Minor style cleanups > > > > drivers/leds/Kconfig | 9 + > > drivers/leds/Makefile | 1 + > > drivers/leds/leds-qcom-lpg.c | 1190 ++++++++++++++++++++++++++++++++++ > > 3 files changed, 1200 insertions(+) > > create mode 100644 drivers/leds/leds-qcom-lpg.c > > Let's put this into drivers/leds/rgb/. You may need to create it. > Will do so. > > > +static int lpg_lut_store(struct lpg *lpg, struct led_pattern *pattern, > > + size_t len, unsigned int *lo_idx, unsigned int *hi_idx) > > +{ > > + unsigned int idx; > > + __le16 val; > > No need for __XX variants outside of headers meant for userspace. > __le16 is the in-kernel return type for cpu_to_le16(), but after further review I believe I don't need to do this. > > +#define LPG_ENABLE_GLITCH_REMOVAL BIT(5) > > + > > +static void lpg_enable_glitch(struct lpg_channel *chan) > > +{ > > + struct lpg *lpg = chan->lpg; > > + > > + regmap_update_bits(lpg->map, chan->base + PWM_TYPE_CONFIG_REG, > > + LPG_ENABLE_GLITCH_REMOVAL, 0); > > +} > > + > > +static void lpg_disable_glitch(struct lpg_channel *chan) > > +{ > > + struct lpg *lpg = chan->lpg; > > + > > + regmap_update_bits(lpg->map, chan->base + PWM_TYPE_CONFIG_REG, > > + LPG_ENABLE_GLITCH_REMOVAL, > > + LPG_ENABLE_GLITCH_REMOVAL); > > +} > > Helper functions for single register write is kind of overkill... > Yes, it is, but it keep lpg_apply() tidy. > > +static int lpg_blink_set(struct lpg_led *led, > > + unsigned long delay_on, unsigned long delay_off) > > +{ > > + struct lpg_channel *chan; > > + unsigned int period_us; > > + unsigned int duty_us; > > + int i; > > + > > + if (!delay_on && !delay_off) { > > + delay_on = 500; > > + delay_off = 500; > > + } > > Aren't you supposed to modify the values passed to you, so that > userspace knows what the default rate is? > I had missed this. > > > + ret = lpg_lut_store(lpg, pattern, len, &lo_idx, &hi_idx); > > + if (ret < 0) > > + goto out; > > Just do direct return. > Will do. > > +out: > > + return ret; > > +} > > > +static const struct pwm_ops lpg_pwm_ops = { > > + .request = lpg_pwm_request, > > + .apply = lpg_pwm_apply, > > + .owner = THIS_MODULE, > > +}; > > + > > +static int lpg_add_pwm(struct lpg *lpg) > > +{ > > + int ret; > > + > > + lpg->pwm.base = -1; > > + lpg->pwm.dev = lpg->dev; > > + lpg->pwm.npwm = lpg->num_channels; > > + lpg->pwm.ops = &lpg_pwm_ops; > > + > > + ret = pwmchip_add(&lpg->pwm); > > + if (ret) > > + dev_err(lpg->dev, "failed to add PWM chip: ret %d\n", ret); > > + > > + return ret; > > +} > > Do we need to do this? I'd rather have LED driver, than LED+PWM > driver... > Yes, I believe we need to do this. Because each piece of hardware has N channels, which can be wired to LEDs, grouped with other channels and wired to multicolor LEDs or be used as PWM signals. And this configuration is board specific. One such example is the laptop in front of me, which has 3 channels wired to an RGB LED and 1 channel wired as a backlight control signal (i.e. using pwm-backlight). Another example is a devboard where the 4 channels are wired to 4 LEDs. Regards, Bjorn