Re: [PATCH v2 2/2] leds: add Panasonic AN30259A support

[Pavel Machek <pavel@ucw.cz>]

[-- Attachment #1: Type: text/plain, Size: 1658 bytes --]

Hi!

> The datasheet is freely available:
> https://www.alliedelec.com/m/d/a9d2b3ee87c2d1a535a41dd747b1c247.pdf

> +	  To compile this driver as a module, choose M here: the module
> +	  will be called leds-an30259a.

leds-an30259a.ko?

> @@ -0,0 +1,382 @@
> +// SPDX-License-Identifier: GPL-2.0

I'd prefer 2.0+, but... your choice.

> +/*
> + * Driver for Panasonic AN30259A 3-channel LED driver
> + *
> + * Copyright (c) 2018 Simon Shields <simon@lineageos.org>
> + *
> + * Datasheet:
> + * https://www.alliedelec.com/m/d/a9d2b3ee87c2d1a535a41dd747b1c247.pdf
> + */
> +#include <linux/i2c.h>
> +#include <linux/leds.h>

I'd do empty line after the comment.

> +static u8 an30259a_get_dutymax(u8 brightness)
> +{
> +	u8 duty_max, floor, ceil;
> +
> +	/* squash 8 bit number into 7-bit PWM range */
> +	duty_max = brightness >> 1;

We should really start using max_brightness, so that userspace knows
what 

> +	/* bottom 3 bits are always set for DUTYMAX
> +	 * figure out the closest value
> +	 */

/*
 * Multiline comments like this.
 */

> +	/* slope time controls on/off cycle length */
> +	ret = regmap_write(led->chip->regmap, REG_SLOPE(num),
> +			   LED_SLOPETIME1(off) | LED_SLOPETIME2(on));
> +	if (ret)
> +		return ret;
> +
> +	/* Finally, enable slope mode. */

And yes, having sentences in all the comments would be nice. Or maybe
nowhere. But consistent ;-).

Since all I'm commenting is whitespace...

Acked-by: Pavel Machek <pavel@ucw.cz>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]