From: Lee Jones <lee@kernel.org>
To: "André Apitzsch" <git@apitzsch.eu>
Cc: Pavel Machek <pavel@ucw.cz>, Rob Herring <robh@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Conor Dooley <conor+dt@kernel.org>,
Kees Cook <keescook@chromium.org>,
"Gustavo A. R. Silva" <gustavoars@kernel.org>,
Bjorn Andersson <andersson@kernel.org>,
Konrad Dybcio <konrad.dybcio@linaro.org>,
linux-leds@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org,
linux-arm-msm@vger.kernel.org,
~postmarketos/upstreaming@lists.sr.ht,
phone-devel@vger.kernel.org
Subject: Re: [PATCH v2 2/3] leds: sy7802: Add support for Silergy SY7802 flash LED controller
Date: Tue, 7 May 2024 09:40:52 +0100 [thread overview]
Message-ID: <20240507084052.GW1227636@google.com> (raw)
In-Reply-To: <3309a9f1f5848681d0acf3bfdf9b6525fc88e1bc.camel@apitzsch.eu>
On Sat, 04 May 2024, André Apitzsch wrote:
> Am Freitag, dem 03.05.2024 um 08:19 +0100 schrieb Lee Jones:
> > On Thu, 11 Apr 2024, Lee Jones wrote:
> >
> > > On Mon, 01 Apr 2024, André Apitzsch via B4 Relay wrote:
> > >
> > > > From: André Apitzsch <git@apitzsch.eu>
> > > >
> > > > Add support for SY7802 flash LED controller. It can support up to
> > > > 1.8A
> > > > flash current.
> > >
> > > This is a very small commit message for a 500+ line change!
> > >
> > > Please, tell us more.
> > >
> > > > Signed-off-by: André Apitzsch <git@apitzsch.eu>
> > > > ---
> > > > drivers/leds/flash/Kconfig | 11 +
> > > > drivers/leds/flash/Makefile | 1 +
> > > > drivers/leds/flash/leds-sy7802.c | 532
> > > > +++++++++++++++++++++++++++++++++++++++
> > > > 3 files changed, 544 insertions(+)
> > > >
> > > > diff --git a/drivers/leds/flash/Kconfig
> > > > b/drivers/leds/flash/Kconfig
> > > > index 809b6d98bb3e..f39f0bfe6eef 100644
> > > > --- a/drivers/leds/flash/Kconfig
> > > > +++ b/drivers/leds/flash/Kconfig
> > > > @@ -121,4 +121,15 @@ config LEDS_SGM3140
> > > > This option enables support for the SGM3140 500mA
> > > > Buck/Boost Charge
> > > > Pump LED Driver.
> > > >
> > > > +config LEDS_SY7802
> > > > + tristate "LED support for the Silergy SY7802"
> > > > + depends on I2C && OF
> > > > + depends on GPIOLIB
> > > > + select REGMAP_I2C
> > > > + help
> > > > + This option enables support for the SY7802 flash LED
> > > > controller.
> > > > + SY7802 includes torch and flash functions with
> > > > programmable current.
> > > > +
> > > > + This driver can be built as a module, it will be
> > > > called "leds-sy7802".
> > > > +
> > > > endif # LEDS_CLASS_FLASH
> > > > diff --git a/drivers/leds/flash/Makefile
> > > > b/drivers/leds/flash/Makefile
> > > > index 91d60a4b7952..48860eeced79 100644
> > > > --- a/drivers/leds/flash/Makefile
> > > > +++ b/drivers/leds/flash/Makefile
> > > > @@ -11,3 +11,4 @@ obj-$(CONFIG_LEDS_QCOM_FLASH) += leds-qcom-
> > > > flash.o
> > > > obj-$(CONFIG_LEDS_RT4505) += leds-rt4505.o
> > > > obj-$(CONFIG_LEDS_RT8515) += leds-rt8515.o
> > > > obj-$(CONFIG_LEDS_SGM3140) += leds-sgm3140.o
> > > > +obj-$(CONFIG_LEDS_SY7802) += leds-sy7802.o
> > > > diff --git a/drivers/leds/flash/leds-sy7802.c
> > > > b/drivers/leds/flash/leds-sy7802.c
> > > > new file mode 100644
> > > > index 000000000000..c03a571b0e08
> > > > --- /dev/null
> > > > +++ b/drivers/leds/flash/leds-sy7802.c
> > > > @@ -0,0 +1,532 @@
> >
> > [...]
> >
> > > > +static int sy7802_torch_brightness_set(struct led_classdev
> > > > *lcdev, enum led_brightness level)
> > >
> > > s/level/brightness/
> > >
> > > > +{
> > > > + struct sy7802_led *led = container_of(lcdev, struct
> > > > sy7802_led, flash.led_cdev);
> > > > + u32 led_enable_mask = led->led_no == SY7802_LED_JOINT ?
> > > > SY7802_LEDS_MASK_ALL :
> > > > + SY7802_LEDS_MASK(led->led_no);
> > >
> > > Do all of the fancy multi-line assignment outside of the
> > > declaration block.
> > >
> > > > + u32 enable_mask = SY7802_MODE_MASK | led_enable_mask;
> > > > + u32 val = level ? led_enable_mask : SY7802_MODE_OFF;
> > > > + struct sy7802 *chip = led->chip;
> > > > + u32 curr;
> > >
> > > This is a temporary placeholder for fled_torch_used, right?
> > >
> > > fled_torch_used_tmp? Sometimes abbreviated to tmp.
> > >
> > > > + u32 mask;
> > >
> > > That's a lot of masks. Which one is this?
> > >
> > > > + int ret;
> > > > +
> > > > + mutex_lock(&chip->mutex);
> > > > +
> > > > + /*
> > > > + * There is only one set of flash control logic, and
> > > > this flag is used to check if 'strobe'
> > >
> > > The ',' before 'and' is superfluous.
> > >
> > > > + * is currently being used.
> > > > + */
> > >
> > > Doesn't the variable name kind of imply this?
> > >
> > > > + if (chip->fled_strobe_used) {
> > > > + dev_warn(chip->dev, "Please disable strobe first
> > > > [%d]\n", chip->fled_strobe_used);
> > >
> > > "Cannot set torch brightness whilst strobe is enabled"
> > >
> > > > + ret = -EBUSY;
> > > > + goto unlock;
> > > > + }
> > > > +
> > > > + if (level)
> > > > + curr = chip->fled_torch_used | BIT(led->led_no);
> > > > + else
> > > > + curr = chip->fled_torch_used & ~BIT(led-
> > > > >led_no);
> > > > +
> > > > + if (curr)
> > > > + val |= SY7802_MODE_TORCH;
> > > > +
> > > > + /* Torch needs to be disabled first to apply new
> > > > brightness */
> > >
> > > "Disable touch to apply brightness"
> > >
> > > > + ret = regmap_update_bits(chip->regmap,
> > > > SY7802_REG_ENABLE, SY7802_MODE_MASK,
> > > > + SY7802_MODE_OFF);
> > > > + if (ret)
> > > > + goto unlock;
> > > > +
> > > > + mask = led->led_no == SY7802_LED_JOINT ?
> > > > SY7802_TORCH_CURRENT_MASK_ALL :
> > >
> > > Why not just use led->led_no in place of mask?
> >
> > mask and led->led_no are assigned the same value from this point on.
>
> Thanks for the clarification.
> How to you come to the conclusion that mask and led->led_no are
> assigned the same value from this point on?
Because I am blind and didn't see the double equals ("==").
#shouldhavegonetospecsavers
--
Lee Jones [李琼斯]
next prev parent reply other threads:[~2024-05-07 8:40 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-01 21:23 [PATCH v2 0/3] Add sy7802 flash led driver André Apitzsch via B4 Relay
2024-04-01 21:23 ` [PATCH v2 1/3] dt-bindings: leds: Add Silergy SY7802 flash LED André Apitzsch via B4 Relay
2024-04-02 16:22 ` Rob Herring
2024-04-01 21:23 ` [PATCH v2 2/3] leds: sy7802: Add support for Silergy SY7802 flash LED controller André Apitzsch via B4 Relay
2024-04-11 12:48 ` Lee Jones
2024-05-01 14:59 ` André Apitzsch
2024-05-02 9:10 ` Lee Jones
2024-05-02 19:19 ` André Apitzsch
2024-05-03 7:20 ` Lee Jones
2024-05-03 7:19 ` Lee Jones
2024-05-04 18:27 ` André Apitzsch
2024-05-07 8:40 ` Lee Jones [this message]
2024-05-02 20:13 ` Christophe JAILLET
2024-04-01 21:23 ` [PATCH v2 3/3] arm64: dts: qcom: msm8939-longcheer-l9100: Add rear flash André Apitzsch via B4 Relay
2024-04-01 21:45 ` [PATCH v2 0/3] Add sy7802 flash led driver Trilok Soni
2024-04-01 22:11 ` André Apitzsch
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240507084052.GW1227636@google.com \
--to=lee@kernel.org \
--cc=andersson@kernel.org \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=git@apitzsch.eu \
--cc=gustavoars@kernel.org \
--cc=keescook@chromium.org \
--cc=konrad.dybcio@linaro.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-hardening@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-leds@vger.kernel.org \
--cc=pavel@ucw.cz \
--cc=phone-devel@vger.kernel.org \
--cc=robh@kernel.org \
--cc=~postmarketos/upstreaming@lists.sr.ht \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).