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