linux-kernel - Re: [PATCH v2 2/2] backlight: Add Kinetic KTD2801 driver

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 19 Jan 2024 17:36:37 +0100
From: Duje Mihanović <duje.mihanovic@...le.hr>
To: Linus Walleij <linus.walleij@...aro.org>
Cc: Lee Jones <lee@...nel.org>, Daniel Thompson <daniel.thompson@...aro.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>,
 Karel Balej <balejk@...fyz.cz>, ~postmarketos/upstreaming@...ts.sr.ht,
 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 v2 2/2] backlight: Add Kinetic KTD2801 driver

On Friday, January 19, 2024 10:02:33 AM CET Linus Walleij wrote:
> Hi Duje,
> 
> thanks for your patch!
> 
> On Thu, Jan 18, 2024 at 6:33 PM Duje Mihanović <duje.mihanovic@...le.hr> 
wrote:
> > Add driver for the Kinetic KTD2801 backlight driver.>
> > Signed-off-by: Duje Mihanović <duje.mihanovic@...le.hr>
> 
> Add some commit message?

Besides the usual short explanation of the hardware I'd also add a link to the 
datasheet in the commit message if that's appropriate.

> > +#include <linux/backlight.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/of.h>
> 
> I don't think you need <linux/of.h>, the compatible table works without
> that (is in the device driver core).

Can confirm it compiles without.

> > +/* These values have been extracted from Samsung's driver. */
> > +#define KTD2801_EXPRESSWIRE_DETECT_DELAY_US    150
> > +#define KTD2801_EXPRESSWIRE_DETECT_US          270
> > +#define KTD2801_LOW_BIT_HIGH_TIME_US           5
> > +#define KTD2801_LOW_BIT_LOW_TIME_US            (4 *
> > KTD2801_HIGH_BIT_LOW_TIME_US) +#define KTD2801_HIGH_BIT_LOW_TIME_US        
> >   5
> > +#define KTD2801_HIGH_BIT_HIGH_TIME_US          (4 *
> > KTD2801_HIGH_BIT_LOW_TIME_US) +#define KTD2801_DATA_START_US               
> >   5
> > +#define KTD2801_END_OF_DATA_LOW_US             10
> > +#define KTD2801_END_OF_DATA_HIGH_US            350
> > +#define KTD2801_PWR_DOWN_DELAY_US              2600
> > +
> > +#define KTD2801_DEFAULT_BRIGHTNESS     100
> > +#define KTD2801_MAX_BRIGHTNESS         255
> > +
> > +struct ktd2801_backlight {
> > +       struct backlight_device *bd;
> > +       struct gpio_desc *gpiod;
> > +       bool was_on;
> > +};
> > +
> > +static int ktd2801_update_status(struct backlight_device *bd)
> > +{
> > +       struct ktd2801_backlight *ktd2801 = bl_get_data(bd);
> > +       u8 brightness = (u8) backlight_get_brightness(bd);
> > +
> > +       if (backlight_is_blank(bd)) {
> > +               gpiod_set_value(ktd2801->gpiod, 0);
> > +               udelay(KTD2801_PWR_DOWN_DELAY_US);
> 
> That's 2600 us, a pretty long delay in a hard loop or delay timer!
> 
> Can you use usleep_range() instead, at least for this one?

Sounds like a good idea. Should I also make that GPIO pulldown _cansleep while 
at it?

> > +       for (int i = 0; i < 8; i++) {
> > +               u8 next_bit = (brightness & 0x80) >> 7;
> 
> I would just:
> 
> #include <linux/bits.h>
> 
> bool next_bit = !!(brightness & BIT(7));

Will do.

Regards,
--
Duje