lkml.org 
[lkml]   [2023]   [Mar]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 2/2] leds: Add support for rohm,bd65b60 led driver
    From
    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@gmail.com>
    > ---

    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

    \
     
     \ /
      Last update: 2023-03-27 00:51    [W:2.303 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site