lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <86f8110e8edc24d0df035b77a1aa68422e48bde1.camel@apitzsch.eu>
Date: Sat, 22 Jun 2024 13:55:25 +0200
From: André Apitzsch <git@...tzsch.eu>
To: Lee Jones <lee@...nel.org>
Cc: Pavel Machek <pavel@....cz>, Rob Herring <robh@...nel.org>, Krzysztof
 Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley
 <conor+dt@...nel.org>, "Gustavo A. R. Silva" <gustavoars@...nel.org>, Bjorn
 Andersson <andersson@...nel.org>, Konrad Dybcio <konrad.dybcio@...aro.org>,
 Christophe JAILLET <christophe.jaillet@...adoo.fr>,  Trilok Soni
 <quic_tsoni@...cinc.com>, Kees Cook <kees@...nel.org>,
 linux-leds@...r.kernel.org,  devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org,  linux-hardening@...r.kernel.org,
 linux-arm-msm@...r.kernel.org,  ~postmarketos/upstreaming@...ts.sr.ht,
 phone-devel@...r.kernel.org
Subject: Re: [PATCH v4 2/3] leds: sy7802: Add support for Silergy SY7802
 flash LED controller

Hello Lee,

Am Freitag, dem 21.06.2024 um 11:26 +0100 schrieb Lee Jones:
> On Sun, 16 Jun 2024, André Apitzsch via B4 Relay wrote:
> 
> > From: André Apitzsch <git@...tzsch.eu>
> > 
> > The SY7802 is a current-regulated charge pump which can regulate
> > two
> > current levels for Flash and Torch modes.
> > 
> > It is a high-current synchronous boost converter with 2-channel
> > high
> > side current sources. Each channel is able to deliver 900mA
> > current.
> > 
> > Signed-off-by: André Apitzsch <git@...tzsch.eu>
> > ---
> >  drivers/leds/flash/Kconfig       |  11 +
> >  drivers/leds/flash/Makefile      |   1 +
> >  drivers/leds/flash/leds-sy7802.c | 542
> > +++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 554 insertions(+)
> 
> Generally very nice.
> 
> Just a couple of teensy nits to fix then add my and resubmit please.
> 
> Acked-by: Lee Jones <lee@...nel.org>
> 
> > [...]
> > diff --git a/drivers/leds/flash/leds-sy7802.c
> > b/drivers/leds/flash/leds-sy7802.c
> > new file mode 100644
> > index 000000000000..c4bea55a62d0
> > --- /dev/null
> > +++ b/drivers/leds/flash/leds-sy7802.c
> > @@ -0,0 +1,542 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Silergy SY7802 flash LED driver with I2C interface
> 
> "an I2C interface"
> 
> Or
> 
> "I2C interfaces"
> 
> > + * Copyright 2024 André Apitzsch <git@...tzsch.eu>
> > + */
> > +
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/i2c.h>
> > +#include <linux/kernel.h>
> > +#include <linux/led-class-flash.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
> > +
> > +#define SY7802_MAX_LEDS 2
> > +#define SY7802_LED_JOINT 2
> > +
> > +#define SY7802_REG_ENABLE		0x10
> > +#define SY7802_REG_TORCH_BRIGHTNESS	0xa0
> > +#define SY7802_REG_FLASH_BRIGHTNESS	0xb0
> > +#define SY7802_REG_FLASH_DURATION	0xc0
> > +#define SY7802_REG_FLAGS		0xd0
> > +#define SY7802_REG_CONFIG_1		0xe0
> > +#define SY7802_REG_CONFIG_2		0xf0
> > +#define SY7802_REG_VIN_MONITOR		0x80
> > +#define SY7802_REG_LAST_FLASH		0x81
> > +#define SY7802_REG_VLED_MONITOR		0x30
> > +#define SY7802_REG_ADC_DELAY		0x31
> > +#define SY7802_REG_DEV_ID		0xff
> > +
> > +#define SY7802_MODE_OFF		0
> > +#define SY7802_MODE_TORCH	2
> > +#define SY7802_MODE_FLASH	3
> > +#define SY7802_MODE_MASK	GENMASK(1, 0)
> > +
> > +#define SY7802_LEDS_SHIFT	3
> > +#define SY7802_LEDS_MASK(_id)	(BIT(_id) << SY7802_LEDS_SHIFT)
> > +#define SY7802_LEDS_MASK_ALL	(SY7802_LEDS_MASK(0) |
> > SY7802_LEDS_MASK(1))
> > +
> > +#define SY7802_TORCH_CURRENT_SHIFT	3
> > +#define SY7802_TORCH_CURRENT_MASK(_id) \
> > +	(GENMASK(2, 0) << (SY7802_TORCH_CURRENT_SHIFT * (_id)))
> > +#define SY7802_TORCH_CURRENT_MASK_ALL \
> > +	(SY7802_TORCH_CURRENT_MASK(0) |
> > SY7802_TORCH_CURRENT_MASK(1))
> > +
> > +#define SY7802_FLASH_CURRENT_SHIFT	4
> > +#define SY7802_FLASH_CURRENT_MASK(_id) \
> > +	(GENMASK(3, 0) << (SY7802_FLASH_CURRENT_SHIFT * (_id)))
> > +#define SY7802_FLASH_CURRENT_MASK_ALL \
> > +	(SY7802_FLASH_CURRENT_MASK(0) |
> > SY7802_FLASH_CURRENT_MASK(1))
> > +
> > +#define SY7802_TIMEOUT_DEFAULT_US	512000U
> > +#define SY7802_TIMEOUT_MIN_US		32000U
> > +#define SY7802_TIMEOUT_MAX_US		1024000U
> > +#define SY7802_TIMEOUT_STEPSIZE_US	32000U
> > +
> > +#define SY7802_TORCH_BRIGHTNESS_MAX 8
> > +
> > +#define SY7802_FLASH_BRIGHTNESS_DEFAULT	14
> > +#define SY7802_FLASH_BRIGHTNESS_MIN	0
> > +#define SY7802_FLASH_BRIGHTNESS_MAX	15
> > +#define SY7802_FLASH_BRIGHTNESS_STEP	1
> > +
> > +#define SY7802_FLAG_TIMEOUT			BIT(0)
> > +#define SY7802_FLAG_THERMAL_SHUTDOWN		BIT(1)
> > +#define SY7802_FLAG_LED_FAULT			BIT(2)
> > +#define SY7802_FLAG_TX1_INTERRUPT		BIT(3)
> > +#define SY7802_FLAG_TX2_INTERRUPT		BIT(4)
> > +#define SY7802_FLAG_LED_THERMAL_FAULT		BIT(5)
> > +#define SY7802_FLAG_FLASH_INPUT_VOLTAGE_LOW	BIT(6)
> > +#define SY7802_FLAG_INPUT_VOLTAGE_LOW		BIT(7)
> > +
> > +#define SY7802_CHIP_ID	0x51
> > +
> > +static const struct reg_default sy7802_regmap_defs[] = {
> > +	{ SY7802_REG_ENABLE, SY7802_LEDS_MASK_ALL },
> > +	{ SY7802_REG_TORCH_BRIGHTNESS, 0x92 },
> > +	{ SY7802_REG_FLASH_BRIGHTNESS,
> > SY7802_FLASH_BRIGHTNESS_DEFAULT |
> > +		SY7802_FLASH_BRIGHTNESS_DEFAULT <<
> > SY7802_FLASH_CURRENT_SHIFT },
> > +	{ SY7802_REG_FLASH_DURATION, 0x6f },
> > +	{ SY7802_REG_FLAGS, 0x0 },
> > +	{ SY7802_REG_CONFIG_1, 0x68 },
> > +	{ SY7802_REG_CONFIG_2, 0xf0 },
> 
> Not your fault, but this interface is frustrating since we have no
> idea
> what these register values mean.  IMHO, they should be defined and
> ORed
> together in some human readable way.
> 
> I say that it's not your fault because I see that this is the most
> common usage.
> 

I don't know how to interpret some bits of the default values. I don't
have the documentation and changing the bits and observing the behavior
of the device also didn't help.

Should I remove the entries from sy7802_regmap_defs, which have values
that we don't fully understand?

Regards,
André

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ