Hi Bartosz,

Thank you for the patch.

I have few minor issues below.

On 1/18/19 2:42 PM, Bartosz Golaszewski wrote:

Please use uniform C++ comment style.

If in doubt, please refer to the following Linus statement
from [0]:

"And yes, feel free to replace block comments with // while at it."

+#include <linux/i2c.h>
+#include <linux/leds.h>
+#include <linux/mfd/max77650.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define MAX77650_NUM_LEDS		3
+
+#define MAX77650_LED_A_BASE		0x40
+#define MAX77650_LED_B_BASE		0x43
+
+#define MAX77650_LED_BR_MASK		GENMASK(4, 0)
+#define MAX77650_LED_EN_MASK		GENMASK(7, 6)
+
+/* Enable EN_LED_MSTR. */
+#define MAX77650_LED_TOP_DEFAULT	BIT(0)
+
+#define MAX77650_LED_ENABLE		GENMASK(7, 6)
+#define MAX77650_LED_DISABLE		0x00
+
+#define MAX77650_LED_A_DEFAULT		MAX77650_LED_DISABLE
+/* 100% on duty */
+#define MAX77650_LED_B_DEFAULT		GENMASK(3, 0)
+
+struct max77650_leds;
+
+struct max77650_led {
+	struct led_classdev cdev;
+	struct max77650_leds *parent;
+	unsigned int regA;
+	unsigned int regB;
+};
+
+struct max77650_leds {
+	struct regmap *map;
+	struct max77650_led leds[MAX77650_NUM_LEDS];
+};
+
+static struct max77650_led *max77650_to_led(struct led_classdev *cdev)
+{
+	return container_of(cdev, struct max77650_led, cdev);
+}
+
+static int max77650_leds_brightness_set(struct led_classdev *cdev,
+					enum led_brightness brightness)

I would change function names to max77650_led* (singular).
It would be consistent with most of the other LED class drivers.
LED driver for max77693 flash cell also uses this scheme
(drivers/leds/leds-max77693.c).

+{
+	struct max77650_led *led = max77650_to_led(cdev);
+	struct regmap *regmap = led->parent->map;
+	int val, mask;
+
+	mask = MAX77650_LED_BR_MASK | MAX77650_LED_EN_MASK;
+
+	if (brightness == LED_OFF) {
+		val = MAX77650_LED_DISABLE;
+	} else {
+		val = MAX77650_LED_ENABLE;
+		/*
+		 * We can set the brightness with 5-bit resolution.
+		 *
+		 * For brightness == 1, the bits we're writing will be 0, but
+		 * since we keep LED_FS0 set to 12.8mA full-scale range, the
+		 * LED will be lit slightly.
+		 */
+		val |= brightness / 8;
+	}
+
+	return regmap_update_bits(regmap, led->regA, mask, val);
+}
+
+static int max77650_leds_probe(struct platform_device *pdev)
+{
+	struct device_node *of_node, *child;
+	struct max77650_leds *leds;
+	struct max77650_led *led;
+	struct device *parent;
+	struct device *dev;
+	int rv, num_leds;
+	u32 reg;
+
+	dev = &pdev->dev;
+	parent = dev->parent;
+	of_node = dev->of_node;
+
+	if (!of_node)
+		return -ENODEV;
+
+	leds = devm_kzalloc(dev, sizeof(*leds), GFP_KERNEL);
+	if (!leds)
+		return -ENOMEM;
+
+	leds->map = dev_get_regmap(dev->parent, NULL);
+	if (!leds->map)
+		return -ENODEV;
+
+	num_leds = of_get_child_count(of_node);
+	if (!num_leds || num_leds > MAX77650_NUM_LEDS)
+		return -ENODEV;
+
+	for_each_child_of_node(of_node, child) {
+		rv = of_property_read_u32(child, "reg", &reg);
+		if (rv || reg >= MAX77650_NUM_LEDS)
+			return -EINVAL;
+
+		led = &leds->leds[reg];
+
+		led->parent = leds;
+		led->regA = MAX77650_LED_A_BASE + reg;
+		led->regB = MAX77650_LED_B_BASE + reg;
+		led->cdev.brightness_set_blocking
+					= max77650_leds_brightness_set;
+
+		led->cdev.name = of_get_property(child, "label", NULL);
+		if (!led->cdev.name)
+			led->cdev.name = child->name;

Please follow how other recent LED class drivers construct LED names,
e.g. drivers/leds/leds-cr0014114.c. We are in the course of creating
generic support for creating LED names, but until it is agreed upon
we've got to use other drivers as a reference.

+
+		of_property_read_string(child, "linux,default-trigger",
+					&led->cdev.default_trigger);
+
+		rv = devm_of_led_classdev_register(dev, child, &led->cdev);
+		if (rv)
+			return rv;
+
+		rv = regmap_write(leds->map, led->regA,
+				  MAX77650_LED_A_DEFAULT);
+		if (rv)
+			return rv;
+
+		rv = regmap_write(leds->map, led->regB,
+				  MAX77650_LED_B_DEFAULT);
+		if (rv)
+			return rv;
+	}
+
+	rv = regmap_write(leds->map,
+			  MAX77650_REG_CNFG_LED_TOP,
+			  MAX77650_LED_TOP_DEFAULT);
+	if (rv)
+		return rv;

No need to check rv here:

	return regmap_write(leds->map,
			  MAX77650_REG_CNFG_LED_TOP,
			  MAX77650_LED_TOP_DEFAULT)

+	return 0;
+}
+
+static struct platform_driver max77650_leds_driver = {
+	.driver = {
+		.name = "max77650-leds",

s/leds/led/

+	},
+	.probe = max77650_leds_probe,
+};
+module_platform_driver(max77650_leds_driver);
+
+MODULE_DESCRIPTION("MAXIM 77650/77651 LED driver");
+MODULE_AUTHOR("Bartosz Golaszewski [off-list ref]");
+MODULE_LICENSE("GPL");

s/GPL/GPL v2/


[0] https://lkml.org/lkml/2017/11/2/715

-- 
Best regards,
Jacek Anaszewski