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]
Date: Mon, 22 Jan 2024 17:24:51 +0100
From: Duje Mihanović <duje.mihanovic@...le.hr>
To: Daniel Thompson <daniel.thompson@...aro.org>
Cc: Lee Jones <lee@...nel.org>, Jingoo Han <jingoohan1@...il.com>,
 Pavel Machek <pavel@....cz>, Rob Herring <robh+dt@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>, Helge Deller <deller@....de>,
 Linus Walleij <linus.walleij@...aro.org>, Karel Balej <balejk@...fyz.cz>,
 ~postmarketos/upstreaming@...ts.sr.ht, phone-devel@...r.kernel.org,
 dri-devel@...ts.freedesktop.org, linux-leds@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-fbdev@...r.kernel.org
Subject: Re: [PATCH v3 1/3] leds: ktd2692: move ExpressWire code to library

On Monday, January 22, 2024 11:19:26 AM CET Daniel Thompson wrote:
> On Sat, Jan 20, 2024 at 10:26:43PM +0100, Duje Mihanović wrote:
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index 6292fddcc55c..d29b6823e7d1 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -181,6 +181,9 @@ config LEDS_EL15203000
> > 
> >  	  To compile this driver as a module, choose M here: the module
> >  	  will be called leds-el15203000.
> > 
> > +config LEDS_EXPRESSWIRE
> > +	bool
> > +
> 
> Shouldn't there be a "select GPIOLIB" here? It seems odd to make the
> clients responsible for the dependencies.
> 
> BTW there seems to be very little consistency across the kernel between
> "depends on GPIOLIB" and "select GPIOLIB".. but select is marginally
> more popular (283 vs. 219 in the kernel I checked).

I believe a "select" would be more appropriate here unless these backlights 
should be hidden if GPIOLIB is disabled. The catch with "select" is that there 
seems to be no way to throw in the "|| COMPILE_TEST" other GPIO-based 
backlights have and I'm not sure what to do about that.

> > diff --git a/drivers/leds/flash/leds-ktd2692.c
> > b/drivers/leds/flash/leds-ktd2692.c index 598eee5daa52..8c17de3d621f 
100644
> > --- a/drivers/leds/flash/leds-ktd2692.c
> > +++ b/drivers/leds/flash/leds-ktd2692.c
> > 
> >  <snip>
> >  static void ktd2692_expresswire_write(struct ktd2692_context *led, u8
> >  value)
> >  {
> >  
> >  	int i;
> > 
> > -	ktd2692_expresswire_start(led);
> > +	expresswire_start(&led->props);
> > 
> >  	for (i = 7; i >= 0; i--)
> > 
> > -		ktd2692_expresswire_set_bit(led, value & BIT(i));
> > -	ktd2692_expresswire_end(led);
> > +		expresswire_set_bit(&led->props, value & BIT(i));
> > +	expresswire_end(&led->props);
> > 
> >  }
> 
> Is there any reason not to have an expresswire_write_u8() method in the
> library code? It is a concept that appears in both drivers.

Not really, I'll add it in v4.

Regards,
--
Duje




Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ