On 11/06/2024 10:32, Yuxi Wang wrote: > This commit adds support for MPS MP3326 LED driver. Please do not use "This commit/patch/change", but imperative mood. See longer explanation here: https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95 > > Signed-off-by: Yuxi Wang <Yuxi.Wang@xxxxxxxxxxxxxxxxxxx> > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index d721b254e1e4..3ca7be35c834 100644 > --- a/drivers/leds/Kconfig > +++ b/drivers/leds/Kconfig > +/* > + * PWM in the range of [0 255] > + */ > +static int led_pwm_store(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) Nope. ... > + } > + r_val = r_val * 255 / 4095 + (r_val * 255 % 4095) / (4095 / 2); > + g_val = g_val * 255 / 4095 + (g_val * 255 % 4095) / (4095 / 2); > + b_val = b_val * 255 / 4095 + (b_val * 255 % 4095) / (4095 / 2); > + if (led->num_colors == 1) > + return sysfs_emit(buf, "0x%x\n", r_val); > + else > + return sysfs_emit(buf, "0x%x 0x%x 0x%x\n", r_val, g_val, b_val); > +} > + > +static int led_enable_store(struct device *dev, > + struct device_attribute *attr, const char *buf, > + size_t count) Eeeee? store to enable LED? Really? ... > +{ > + struct led_classdev *lcdev = dev_get_drvdata(dev); > + struct mp3326_led *led = container_of(lcdev, struct mp3326_led, cdev); > + struct mp3326 *chip = led->private_data; > + int ret; > + uint val, i; > + > +static DEVICE_ATTR_RW(led_pwm); > +static DEVICE_ATTR_RW(led_enable); > +static DEVICE_ATTR_RO(led_short_fault); > +static DEVICE_ATTR_RO(led_open_fault); No, for multiple reasons: 1. Where ABI documentation? 2. There is a standard sysfs interface. No need for most of that. Please explain why standard interface does not fit your needs - for each new interface. > + > +static struct attribute *led_sysfs_attrs[] = { > + &dev_attr_led_pwm.attr, > + &dev_attr_led_enable.attr, > + &dev_attr_led_short_fault.attr, > + &dev_attr_led_open_fault.attr, > + NULL, > +}; > + > +ATTRIBUTE_GROUPS(led_sysfs); > + > +static int mp3326_add_led(struct mp3326 *chip, struct device_node *np, > + int index) > +{ > + struct mp3326_led *led = &chip->leds[index]; > + struct mc_subled *info; > + struct device_node *child; > + struct led_classdev *cdev; > + struct led_init_data init_data = {}; > + int ret; > + int i = 0; > + int count; > + u32 color = 0; > + u32 reg = 0; > + > + ret = of_property_read_u32(np, "color", &color); > + if (ret) { > + dev_err(&chip->client->dev, "Miss color in the node\n"); > + return ret; > + } Blank line > + led->private_data = chip; > + if (color == LED_COLOR_ID_RGB) { > + count = of_get_child_count(np); > + if (count != 3) { > + dev_err(&chip->client->dev, > + "RGB must have three node.\n"); > + return -EINVAL; > + } > + > + info = devm_kcalloc(&chip->client->dev, 3, sizeof(*info), > + GFP_KERNEL); > + if (!info) > + return -ENOMEM; > + > + for_each_available_child_of_node(np, child) { > + ret = of_property_read_u32(child, "reg", ®); > + if (ret || reg > MAX_CHANNEL) { > + dev_err(&chip->client->dev, > + "reg must less or equal than %d\n", > + MAX_CHANNEL); > + return -EINVAL; > + } > + > + ret = of_property_read_u32(child, "color", &color); > + if (ret) { > + dev_err(&chip->client->dev, > + "color must have value\n"); > + return ret; > + } > + > + if (color > 3 || !color) { > + dev_err(&chip->client->dev, > + "color must be Red, Green and Blue. The color is %d\n", > + color); > + return ret; > + } > + info[i].color_index = color; > + info[i].channel = reg; > + info[i].brightness = 0; > + i++; > + } > + > + led->subled_info = info; > + led->num_colors = 3; > + cdev = &led->cdev; > + cdev->max_brightness = MAX_BRIGHTNESS; > + cdev->brightness_set_blocking = led_brightness_set; > + cdev->groups = led_sysfs_groups; > + init_data.fwnode = &np->fwnode; > + > + ret = devm_led_classdev_register_ext(&chip->client->dev, > + &led->cdev, &init_data); > + > + if (ret) { > + dev_err(&chip->client->dev, > + "Unable register multicolor:%s\n", cdev->name); > + return ret; > + } > + } else { > + ret = of_property_read_u32(np, "reg", ®); > + if (ret || reg > MAX_CHANNEL) { > + dev_err(&chip->client->dev, > + "reg must less or equal than %d\n", > + MAX_CHANNEL); > + return -EINVAL; > + } > + info = devm_kcalloc(&chip->client->dev, 1, sizeof(*info), > + GFP_KERNEL); > + led->num_colors = 1; > + info[i].color_index = LED_COLOR_ID_WHITE; > + info[i].channel = reg; > + info[i].brightness = 0; > + led->subled_info = info; > + cdev = &led->cdev; > + cdev->max_brightness = MAX_BRIGHTNESS; > + cdev->brightness_set_blocking = led_brightness_set; > + cdev->groups = led_sysfs_groups; > + init_data.fwnode = &np->fwnode; > + ret = devm_led_classdev_register_ext(&chip->client->dev, > + &led->cdev, &init_data); > + if (ret) { > + dev_err(&chip->client->dev, "Unable register led:%s\n", > + cdev->name); > + return ret; > + } > + } Blank line > + return ret; > +} > + > +static int mp3326_parse_dt(struct mp3326 *chip) > +{ > + struct device_node *np = dev_of_node(&chip->client->dev); > + struct device_node *child; > + int ret; > + int i = 0; > + > + for_each_available_child_of_node(np, child) { > + ret = mp3326_add_led(chip, child, i); > + if (ret) > + return ret; > + i++; > + } > + > + ret = regmap_write(chip->regmap, MP3326_PWM_CTRL_CHANNEL_9_16, 0); > + if (ret) > + return ret; Blank line > + ret = regmap_write(chip->regmap, MP3326_PWM_CTRL_CHANNEL_1_8, 0); > + if (ret) > + return ret; Blank line > + return 0; > +} > + > +static int mp3326_leds_probe(struct i2c_client *client) > +{ > + struct mp3326 *chip; > + const struct reg_field *reg_fields; > + int count, i, j; > + > + count = device_get_child_node_count(&client->dev); > + if (!count) { Drop { Please run scripts/checkpatch.pl and fix reported warnings. Then please run `scripts/checkpatch.pl --strict` and (probably) fix more warnings. Some warnings can be ignored, especially from --strict run, but the code here looks like it needs a fix. Feel free to get in touch if the warning is not clear. > + return dev_err_probe(&client->dev, -EINVAL, > + "Incorrect number of leds (%d)", count); > + } blank line > + chip = devm_kzalloc(&client->dev, struct_size(chip, leds, count), > + GFP_KERNEL); > + if (!chip) > + return -ENOMEM; > + > + chip->client = client; > + chip->num_of_leds = count; > + i2c_set_clientdata(client, chip); > + chip->regmap = devm_regmap_init_i2c(client, &MP3326_regmap_config); > + if (IS_ERR(chip->regmap)) > + return PTR_ERR(chip->regmap); > + > + for (i = 0; i < MAX_CHANNEL; i++) { > + reg_fields = channels_reg_fields[i]; > + for (j = 0; j < MAX_CTRL; j++) { > + chip->regmap_fields[i][j] = devm_regmap_field_alloc( > + &client->dev, chip->regmap, reg_fields[j]); > + if (IS_ERR(chip->regmap_fields[i][j])) > + return PTR_ERR(chip->regmap_fields[i][j]); > + } > + } Blank line > + if (mp3326_parse_dt(chip)) > + return 1; What is one? This looks like some sort of user-space or downstream approach. That's not how it works for upstream kernel. Do not introduce your downstream/user-space/other-system coding style and programming interface. You must use Linux approach. There is no way probe function returns a "1". See other files as example. > + else > + return 0; > +} > + > +static const struct i2c_device_id mp3326_id[] = { { "mp3326", 0 }, {} }; This must be formatted as kernel coding style. See other files as an example. > +MODULE_DEVICE_TABLE(i2c, mp3326_id); > + > +static const struct of_device_id mp3326_of_match[] = { { .compatible = > + "mps,mp3326" }, > + {} }; > +MODULE_DEVICE_TABLE(of, mp3326_of_match); > + > +static struct i2c_driver mp3326_driver = { > + .probe = mp3326_leds_probe, > + .driver = { > + .name = "mp3326_led", > + .of_match_table = mp3326_of_match, > + }, > + .id_table = mp3326_id, > +}; > + > +module_i2c_driver(mp3326_driver); > +MODULE_AUTHOR("Yuxi Wang <Yuxi.Wang@xxxxxxxxxxxxxxxxxxx>"); > +MODULE_DESCRIPTION("MPS MP3326 LED driver"); > +MODULE_LICENSE("GPL"); Best regards, Krzysztof
