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

["Duje Mihanović" <duje.mihanovic@skole.hr>]

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@skole.hr> 
wrote:
> > Add driver for the Kinetic KTD2801 backlight driver.>
> > Signed-off-by: Duje Mihanović <duje.mihanovic@skole.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