On 08/03/2023 21:14, Bogdan Ionescu wrote: > This commit adds support for ROHM BD65B60 led driver. Do not use "This commit/patch". https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95 > The chip supports 2 outpus sharing the same current setting > and is controlled over I2C. > > Signed-off-by: Bogdan Ionescu <bogdan.ionescu.work@xxxxxxxxx> > --- Thank you for your patch. There is something to discuss/improve. > + > + /* Check required properties */ > + if (!fwnode_property_present(child, "rohm,enable-outputs")) { > + dev_err(dev, "No rohm,enable-outputs property found"); > + return -ENOENT; The property is not required by your binding, so you cannot bail here. > + } > + > + ret = fwnode_property_read_u32(child, "rohm,enable-outputs", &led->enable); > + if (ret || (led->enable & LEDSEL_MASK) != led->enable) { > + dev_err(dev, "Failed to read rohm,enable-outputs property"); No need to check for properties twice... > + return ret; > + } > + > + /* Check optional properties */ > + led->state = BD65B60_OFF; > + if (!fwnode_property_present(child, "default-state")) { > + ret = fwnode_property_read_string(child, "default-state", > + (const char **)&default_state); > + if (ret) { > + dev_err(dev, "Failed to read default-state property"); > + return ret; > + } > + > + if (strcmp(default_state, "keep") == 0) { > + led->state = BD65B60_KEEP; > + } else if (strcmp(default_state, "on") == 0) { > + led->state = BD65B60_ON; > + } else if (strcmp(default_state, "off") == 0) { > + led->state = BD65B60_OFF; > + } else { > + dev_err(dev, "Invalid default-state property"); > + return -EINVAL; > + } > + } > + > + led->ovp = BD65B60_DEFAULT_OVP_VAL; > + if (fwnode_property_present(child, "rohm,ovp")) { > + ret = fwnode_property_read_u32(child, "rohm,ovp", &led->ovp); > + > + if (ret || (led->ovp & OVP_MASK) != led->ovp) { > + dev_err(dev, "Failed to read rohm,ovp property"); > + return ret; > + } > + } > + > + *fwnode = child; > + > + return 0; > +} > + > +static int bd65b60_probe(struct i2c_client *client) > +{ > + struct bd65b60_led *led; > + struct led_init_data init_data = {}; > + struct fwnode_handle *fwnode = NULL; > + int ret; > + > + led = devm_kzalloc(&client->dev, sizeof(*led), GFP_KERNEL); > + if (!led) > + return -ENOMEM; > + > + led->client = client; > + i2c_set_clientdata(client, led); > + > + ret = bd65b60_parse_dt(led, &fwnode); > + if (ret) > + return ret; > + > + led->cdev.name = BD65B60_DEFAULT_NAME; > + led->cdev.brightness_set = bd65b60_brightness_set; > + led->cdev.brightness = BD65B60_DEFAULT_BRIGHTNESS; > + led->cdev.max_brightness = BD65B60_MAX_BRIGHTNESS; > + led->cdev.default_trigger = BD65B60_DEFAULT_TRIGGER; > + led->client = client; > + > + led->regmap = devm_regmap_init_i2c(client, &bd65b60_regmap_config); > + if (IS_ERR(led->regmap)) { > + ret = PTR_ERR(led->regmap); > + dev_err(&client->dev, "Failed to allocate register map: %d", > + ret); return dev_err_probe > + return ret; > + } > + > + mutex_init(&led->lock); > + > + ret = bd65b60_init(led); > + if (ret) { > + dev_err(&client->dev, "Failed to initialize led: %d", ret); return dev_err_probe > + mutex_destroy(&led->lock); or ret = dev_err_probe and goto to common cleanup part > + return ret; > + } > + > + init_data.fwnode = fwnode; > + init_data.devicename = led->client->name; > + init_data.default_label = ":"; > + ret = devm_led_classdev_register_ext(&client->dev, &led->cdev, > + &init_data); > + if (ret) { > + dev_err(&client->dev, "Failed to register led: %d", ret); return dev_err_probe > + mutex_destroy(&led->lock); > + return ret; > + } > + > + return 0; > +} > + > +static void bd65b60_remove(struct i2c_client *client) > +{ > + int ret; > + struct bd65b60_led *led = i2c_get_clientdata(client); > + > + ret = regmap_write(led->regmap, REG_PON, BD65B60_OFF); > + if (ret) > + dev_err(&client->dev, "Failed to turn off led: %d", ret); > + > + mutex_destroy(&led->lock); > +} > + > +static const struct i2c_device_id bd65b60_id[] = { > + { "bd65b60", 0 }, > + {}, > +}; > +MODULE_DEVICE_TABLE(i2c, bd65b60_id); > + > +static const struct of_device_id of_bd65b60_leds_match[] = { > + { .compatible = "rohm,bd65b60" }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, of_bd65b60_leds_match); > + > +static struct i2c_driver bd65b60_i2c_driver = { > + .driver = { > + .name = "bd65b60", > + .owner = THIS_MODULE, > + .of_match_table = of_match_ptr(of_bd65b60_leds_match), Drop of_match_ptr. It requires maybe_unused which you do not have, so this will cause warnings. Best regards, Krzysztof
