| Message ID | 20240611083236.1609-3-Yuxi.Wang@monolithicpower.com |
|---|---|
| State | New |
| Headers | show |
| Series | leds: Add a driver for MP3326 | expand |
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@monolithicpower.com> > > 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@monolithicpower.com>"); > +MODULE_DESCRIPTION("MPS MP3326 LED driver"); > +MODULE_LICENSE("GPL"); Best regards, Krzysztof
Hi Yuxi,
kernel test robot noticed the following build errors:
[auto build test ERROR on pavel-leds/for-next]
[also build test ERROR on linus/master v6.10-rc3 next-20240611]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Yuxi-Wang/dt-bindings-leds-add-mps-mp3326-LED/20240611-165810
base: git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git for-next
patch link: https://lore.kernel.org/r/20240611083236.1609-3-Yuxi.Wang%40monolithicpower.com
patch subject: [PATCH V2 2/2] leds: add mp3326 driver
config: s390-randconfig-r081-20240612 (https://download.01.org/0day-ci/archive/20240612/202406120751.SskLd0jn-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project 4403cdbaf01379de96f8d0d6ea4f51a085e37766)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240612/202406120751.SskLd0jn-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406120751.SskLd0jn-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from drivers/leds/leds-mp3326.c:10:
In file included from include/linux/module.h:19:
In file included from include/linux/elf.h:6:
In file included from arch/s390/include/asm/elf.h:173:
In file included from arch/s390/include/asm/mmu_context.h:11:
In file included from arch/s390/include/asm/pgalloc.h:18:
In file included from include/linux/mm.h:1856:
include/linux/vmstat.h:502:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
502 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
503 | item];
| ~~~~
include/linux/vmstat.h:509:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
509 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
510 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
include/linux/vmstat.h:516:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
516 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
include/linux/vmstat.h:521:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
521 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
522 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
include/linux/vmstat.h:530:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
530 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
531 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
In file included from drivers/leds/leds-mp3326.c:13:
In file included from include/linux/regmap.h:20:
In file included from include/linux/iopoll.h:14:
In file included from include/linux/io.h:13:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
547 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
560 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
| ^
include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
102 | #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
| ^
In file included from drivers/leds/leds-mp3326.c:13:
In file included from include/linux/regmap.h:20:
In file included from include/linux/iopoll.h:14:
In file included from include/linux/io.h:13:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
573 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
| ^
include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
115 | #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
| ^
In file included from drivers/leds/leds-mp3326.c:13:
In file included from include/linux/regmap.h:20:
In file included from include/linux/iopoll.h:14:
In file included from include/linux/io.h:13:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
584 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
594 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
604 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:692:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
692 | readsb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:700:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
700 | readsw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:708:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
708 | readsl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:717:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
717 | writesb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:726:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
726 | writesw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:735:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
735 | writesl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
drivers/leds/leds-mp3326.c:232:41: warning: variable 'i' is uninitialized when used here [-Wuninitialized]
232 | chip->regmap_fields[led->subled_info[i].channel]
| ^
drivers/leds/leds-mp3326.c:220:7: note: initialize the variable 'i' to silence this warning
220 | int i;
| ^
| = 0
>> drivers/leds/leds-mp3326.c:493:8: error: incompatible function pointer types initializing 'ssize_t (*)(struct device *, struct device_attribute *, char *)' (aka 'long (*)(struct device *, struct device_attribute *, char *)') with an expression of type 'int (struct device *, struct device_attribute *, char *)' [-Wincompatible-function-pointer-types]
493 | static DEVICE_ATTR_RW(led_pwm);
| ^~~~~~~~~~~~~~~~~~~~~~~
include/linux/device.h:132:45: note: expanded from macro 'DEVICE_ATTR_RW'
132 | struct device_attribute dev_attr_##_name = __ATTR_RW(_name)
| ^~~~~~~~~~~~~~~~
include/linux/sysfs.h:138:46: note: expanded from macro '__ATTR_RW'
138 | #define __ATTR_RW(_name) __ATTR(_name, 0644, _name##_show, _name##_store)
| ^~~~~~~~~~~~
<scratch space>:105:1: note: expanded from here
105 | led_pwm_show
| ^~~~~~~~~~~~
include/linux/sysfs.h:104:10: note: expanded from macro '__ATTR'
104 | .show = _show, \
| ^~~~~
>> drivers/leds/leds-mp3326.c:493:8: error: incompatible function pointer types initializing 'ssize_t (*)(struct device *, struct device_attribute *, const char *, size_t)' (aka 'long (*)(struct device *, struct device_attribute *, const char *, unsigned long)') with an expression of type 'int (struct device *, struct device_attribute *, const char *, size_t)' (aka 'int (struct device *, struct device_attribute *, const char *, unsigned long)') [-Wincompatible-function-pointer-types]
493 | static DEVICE_ATTR_RW(led_pwm);
| ^~~~~~~~~~~~~~~~~~~~~~~
include/linux/device.h:132:45: note: expanded from macro 'DEVICE_ATTR_RW'
132 | struct device_attribute dev_attr_##_name = __ATTR_RW(_name)
| ^~~~~~~~~~~~~~~~
include/linux/sysfs.h:138:60: note: expanded from macro '__ATTR_RW'
138 | #define __ATTR_RW(_name) __ATTR(_name, 0644, _name##_show, _name##_store)
| ^~~~~~~~~~~~~
<scratch space>:106:1: note: expanded from here
106 | led_pwm_store
| ^~~~~~~~~~~~~
include/linux/sysfs.h:105:11: note: expanded from macro '__ATTR'
105 | .store = _store, \
| ^~~~~~
drivers/leds/leds-mp3326.c:494:8: error: incompatible function pointer types initializing 'ssize_t (*)(struct device *, struct device_attribute *, char *)' (aka 'long (*)(struct device *, struct device_attribute *, char *)') with an expression of type 'int (struct device *, struct device_attribute *, char *)' [-Wincompatible-function-pointer-types]
494 | static DEVICE_ATTR_RW(led_enable);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/device.h:132:45: note: expanded from macro 'DEVICE_ATTR_RW'
132 | struct device_attribute dev_attr_##_name = __ATTR_RW(_name)
| ^~~~~~~~~~~~~~~~
include/linux/sysfs.h:138:46: note: expanded from macro '__ATTR_RW'
138 | #define __ATTR_RW(_name) __ATTR(_name, 0644, _name##_show, _name##_store)
| ^~~~~~~~~~~~
<scratch space>:109:1: note: expanded from here
109 | led_enable_show
| ^~~~~~~~~~~~~~~
include/linux/sysfs.h:104:10: note: expanded from macro '__ATTR'
104 | .show = _show, \
| ^~~~~
drivers/leds/leds-mp3326.c:494:8: error: incompatible function pointer types initializing 'ssize_t (*)(struct device *, struct device_attribute *, const char *, size_t)' (aka 'long (*)(struct device *, struct device_attribute *, const char *, unsigned long)') with an expression of type 'int (struct device *, struct device_attribute *, const char *, size_t)' (aka 'int (struct device *, struct device_attribute *, const char *, unsigned long)') [-Wincompatible-function-pointer-types]
494 | static DEVICE_ATTR_RW(led_enable);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/device.h:132:45: note: expanded from macro 'DEVICE_ATTR_RW'
132 | struct device_attribute dev_attr_##_name = __ATTR_RW(_name)
| ^~~~~~~~~~~~~~~~
include/linux/sysfs.h:138:60: note: expanded from macro '__ATTR_RW'
138 | #define __ATTR_RW(_name) __ATTR(_name, 0644, _name##_show, _name##_store)
| ^~~~~~~~~~~~~
<scratch space>:110:1: note: expanded from here
110 | led_enable_store
| ^~~~~~~~~~~~~~~~
include/linux/sysfs.h:105:11: note: expanded from macro '__ATTR'
105 | .store = _store, \
| ^~~~~~
drivers/leds/leds-mp3326.c:495:8: error: incompatible function pointer types initializing 'ssize_t (*)(struct device *, struct device_attribute *, char *)' (aka 'long (*)(struct device *, struct device_attribute *, char *)') with an expression of type 'int (struct device *, struct device_attribute *, char *)' [-Wincompatible-function-pointer-types]
495 | static DEVICE_ATTR_RO(led_short_fault);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/device.h:136:45: note: expanded from macro 'DEVICE_ATTR_RO'
136 | struct device_attribute dev_attr_##_name = __ATTR_RO(_name)
| ^~~~~~~~~~~~~~~~
include/linux/sysfs.h:117:10: note: expanded from macro '__ATTR_RO'
117 | .show = _name##_show, \
| ^~~~~~~~~~~~
<scratch space>:114:1: note: expanded from here
114 | led_short_fault_show
| ^~~~~~~~~~~~~~~~~~~~
drivers/leds/leds-mp3326.c:496:8: error: incompatible function pointer types initializing 'ssize_t (*)(struct device *, struct device_attribute *, char *)' (aka 'long (*)(struct device *, struct device_attribute *, char *)') with an expression of type 'int (struct device *, struct device_attribute *, char *)' [-Wincompatible-function-pointer-types]
496 | static DEVICE_ATTR_RO(led_open_fault);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/device.h:136:45: note: expanded from macro 'DEVICE_ATTR_RO'
136 | struct device_attribute dev_attr_##_name = __ATTR_RO(_name)
| ^~~~~~~~~~~~~~~~
include/linux/sysfs.h:117:10: note: expanded from macro '__ATTR_RO'
117 | .show = _name##_show, \
| ^~~~~~~~~~~~
<scratch space>:117:1: note: expanded from here
117 | led_open_fault_show
| ^~~~~~~~~~~~~~~~~~~
drivers/leds/leds-mp3326.c:686:11: error: incompatible function pointer types initializing 'int (*)(struct i2c_client *, const struct i2c_device_id *)' with an expression of type 'int (struct i2c_client *)' [-Wincompatible-function-pointer-types]
686 | .probe = mp3326_leds_probe,
| ^~~~~~~~~~~~~~~~~
18 warnings and 7 errors generated.
vim +493 drivers/leds/leds-mp3326.c
492
> 493 static DEVICE_ATTR_RW(led_pwm);
494 static DEVICE_ATTR_RW(led_enable);
495 static DEVICE_ATTR_RO(led_short_fault);
496 static DEVICE_ATTR_RO(led_open_fault);
497
> -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Tuesday, June 11, 2024 5:22 PM > To: Yuxi (Yuxi) Wang <Yuxi.Wang@monolithicpower.com>; pavel@ucw.cz; lee@kernel.org; robh+dt@kernel.org; > krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org; wyx137120466@gmail.com > Cc: linux-leds@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH V2 2/2] leds: add mp3326 driver > > 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://urldefense.com/v3/__https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting- > patches.rst*L95__;Iw!!FIHMVlGrYVGa5kwGHCY!VZLXCA6uD-SR9PbzzNhDFXYNwX0LIXNJnI81FkF3VFD0K3R8zpk_o61sSGCXDa- > mecEHFCdimzprLNeYlHJSLzaY4P_CT3YmPTXg$ > Sorry, it's my fault. I will fix it in the next version. > > > > Signed-off-by: Yuxi Wang <Yuxi.Wang@monolithicpower.com> > > > > 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. Hi Krzysztof, What do you mean this Nope? Is it format or function? > > ... > > > + } > > + 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? Yes. The users need this function and we provide it. > > ... > > > +{ > > + 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. Hi krzysztof, 1. Where ABI documentation? A: Sorry, the abi is insufficient. Can I add it as comment above the function? 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. A: Leds has two ways to light dim. One is analog dimming, another pwm dimming. They are different in practice. In RGB module, pwm dimming can control color and analog dimming can control intensity. Mp3326 supports the two ways which can operate contemporary. In practice, I have needs below. 1. Operate rgb color and intensity. 2. enable/disable some channel 3. short/open fault notice. However, The standard interface only has three functions below, they are not fit my needs. 1. multi_index 2. multi_intensity(only can dim using one way, so I use it as analog dimming) 3. led_mc_calc_color In order to fit my needs, I add the interface below. led_pwm led_enable led_short_fault led_open_fault > > > + > > +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 Sorry, it's my fault. I will fix it in the next version. > > > + 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 > Sorry, it's my fault. I will fix it in the next version. > > + 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 Sorry, it's my fault. I will fix it in the next version. > > > + ret = regmap_write(chip->regmap, MP3326_PWM_CTRL_CHANNEL_1_8, 0); > > + if (ret) > > + return ret; > > Blank line Sorry, it's my fault. I will fix it in the next version. > > > + 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. > Krzysztof, Thanks your suggestion. I run scripts/checkpatch.pl, but no warn and error occur. I will add `--strict` and check it carefully. > > + return dev_err_probe(&client->dev, -EINVAL, > > + "Incorrect number of leds (%d)", count); > > + } > > blank line > Sorry, it's my fault. I will fix it in the next version. > > + 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 > Sorry, it's my fault. I will fix it in the next version. > > + 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. > > Sorry, it's my fault. I will fix it in the next version. > > + 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. Ok, I use clang-format to format my code-style, maybe some incompatible. I will correct it. Thanks, > > > +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@monolithicpower.com>"); > > +MODULE_DESCRIPTION("MPS MP3326 LED driver"); > > +MODULE_LICENSE("GPL"); > > Best regards, > Krzysztof
On 12/06/2024 04:23, Yuxi (Yuxi) Wang wrote: >> >>> +/* >>> + * 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. > Hi Krzysztof, > > What do you mean this Nope? > Is it format or function? As sorry, you are reimplementing kernel interfaces, so that's a no. > >> >> ... >> >>> + } >>> + 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? > Yes. The users need this function and we provide it. NAK, I don't care about your users. You re-implemented existing ABI. Without any ABI docs that's just pure duplication. > > >> >> ... >> >>> +{ >>> + 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. > Hi krzysztof, > > 1. Where ABI documentation? > A: > Sorry, the abi is insufficient. Which one is insufficient? > > Can I add it as comment above the function? > > 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. > A: > Leds has two ways to light dim. One is analog dimming, another pwm dimming. > They are different in practice. > > In RGB module, pwm dimming can control color and analog dimming can control intensity. > > Mp3326 supports the two ways which can operate contemporary. > > In practice, I have needs below. > 1. Operate rgb color and intensity. > 2. enable/disable some channel > 3. short/open fault notice. > > However, The standard interface only has three functions below, they are not fit my needs. > 1. multi_index > 2. multi_intensity(only can dim using one way, so I use it as analog dimming) > 3. led_mc_calc_color > Please point to which ABI is not sufficient. > > In order to fit my needs, I add the interface below. > led_pwm > led_enable Because especially this one sounds like you are mocking us. > led_short_fault > led_open_fault > Best regards, Krzysztof
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index d721b254e1e4..3ca7be35c834 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -266,6 +266,15 @@ config LEDS_MIKROTIK_RB532 This option enables support for the so called "User LED" of Mikrotik's Routerboard 532. +config LEDS_MP3326 + tristate "LED Support for MPS MP3326" + depends on LEDS_CLASS + depends on LEDS_CLASS_MULTICOLOR + help + This option enables support for on-chip LED drivers found on + MPS MP3326. + + config LEDS_MT6323 tristate "LED Support for Mediatek MT6323 PMIC" depends on LEDS_CLASS diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile index ce07dc295ff0..c0a8b4b3e7c8 100644 --- a/drivers/leds/Makefile +++ b/drivers/leds/Makefile @@ -64,6 +64,7 @@ obj-$(CONFIG_LEDS_MENF21BMC) += leds-menf21bmc.o obj-$(CONFIG_LEDS_MIKROTIK_RB532) += leds-rb532.o obj-$(CONFIG_LEDS_MLXCPLD) += leds-mlxcpld.o obj-$(CONFIG_LEDS_MLXREG) += leds-mlxreg.o +obj-$(CONFIG_LEDS_MP3326) += leds-mp3326.o obj-$(CONFIG_LEDS_MT6323) += leds-mt6323.o obj-$(CONFIG_LEDS_NET48XX) += leds-net48xx.o obj-$(CONFIG_LEDS_NETXBIG) += leds-netxbig.o diff --git a/drivers/leds/leds-mp3326.c b/drivers/leds/leds-mp3326.c new file mode 100644 index 000000000000..8abbaf4f6740 --- /dev/null +++ b/drivers/leds/leds-mp3326.c @@ -0,0 +1,697 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * MP3326 Led driver + * + * Copyright 2024 Monolithic Power Systems, Inc + * + * Author: Yuxi Wang <Yuxi.Wang@monolithicpower.com> + */ +#include <linux/bits.h> +#include <linux/module.h> +#include <linux/mod_devicetable.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/i2c.h> +#include <linux/of.h> +#include <linux/slab.h> +#include <linux/interrupt.h> +#include <linux/mutex.h> +#include <linux/leds.h> +#include <linux/device.h> +#include <linux/led-class-multicolor.h> + +#define MP3326_PWM_DIM_FREQUENCY_CONFIG 0x00 +#define MP3326_PWM_CTRL 0x01 +#define MP3326_PWM_CTRL_CHANNEL_9_16 0x04 +#define MP3326_PWM_CTRL_CHANNEL_1_8 0x05 +#define MP3326_PWM_OPEN_FAULT_CHANNEL_9_16 0x06 +#define MP3326_PWM_OPEN_FAULT_CHANNEL_1_8 0x07 +#define MP3326_PWM_SHORT_FAULT_CHANNEL_9_16 0x08 +#define MP3326_PWM_SHORT_FAULT_CHANNEL_1_8 0x09 +#define MP3326_PWM_CURRENT_SET_CHANNEL1 0x0A +#define MP3326_PWM_DUTY_LSB_SET_CHANNEL1 0x0B +#define MP3326_PWM_DUTY_MSB_SET_CHANNEL1 0x0C +#define MP3326_PWM_CURRENT_SET_CHANNEL2 0x0D +#define MP3326_PWM_DUTY_LSB_SET_CHANNEL2 0x0E +#define MP3326_PWM_DUTY_MSB_SET_CHANNEL2 0x0F +#define MP3326_PWM_CURRENT_SET_CHANNEL3 0x10 +#define MP3326_PWM_DUTY_LSB_SET_CHANNEL3 0x11 +#define MP3326_PWM_DUTY_MSB_SET_CHANNEL3 0x12 +#define MP3326_PWM_CURRENT_SET_CHANNEL4 0x13 +#define MP3326_PWM_DUTY_LSB_SET_CHANNEL4 0x14 +#define MP3326_PWM_DUTY_MSB_SET_CHANNEL4 0x15 +#define MP3326_PWM_CURRENT_SET_CHANNEL5 0x16 +#define MP3326_PWM_DUTY_LSB_SET_CHANNEL5 0x17 +#define MP3326_PWM_DUTY_MSB_SET_CHANNEL5 0x18 +#define MP3326_PWM_CURRENT_SET_CHANNEL6 0x19 +#define MP3326_PWM_DUTY_LSB_SET_CHANNEL6 0x1A +#define MP3326_PWM_DUTY_MSB_SET_CHANNEL6 0x1B +#define MP3326_PWM_CURRENT_SET_CHANNEL7 0x1C +#define MP3326_PWM_DUTY_LSB_SET_CHANNEL7 0x1D +#define MP3326_PWM_DUTY_MSB_SET_CHANNEL7 0x1E +#define MP3326_PWM_CURRENT_SET_CHANNEL8 0x1F +#define MP3326_PWM_DUTY_LSB_SET_CHANNEL8 0x20 +#define MP3326_PWM_DUTY_MSB_SET_CHANNEL8 0x21 +#define MP3326_PWM_CURRENT_SET_CHANNEL9 0x22 +#define MP3326_PWM_DUTY_LSB_SET_CHANNEL9 0x23 +#define MP3326_PWM_DUTY_MSB_SET_CHANNEL9 0x24 +#define MP3326_PWM_CURRENT_SET_CHANNEL10 0x25 +#define MP3326_PWM_DUTY_LSB_SET_CHANNEL10 0x26 +#define MP3326_PWM_DUTY_MSB_SET_CHANNEL10 0x27 +#define MP3326_PWM_CURRENT_SET_CHANNEL11 0x28 +#define MP3326_PWM_DUTY_LSB_SET_CHANNEL11 0x29 +#define MP3326_PWM_DUTY_MSB_SET_CHANNEL11 0x2A +#define MP3326_PWM_CURRENT_SET_CHANNEL12 0x2B +#define MP3326_PWM_DUTY_LSB_SET_CHANNEL12 0x2C +#define MP3326_PWM_DUTY_MSB_SET_CHANNEL12 0x2D +#define MP3326_PWM_CURRENT_SET_CHANNEL13 0x2E +#define MP3326_PWM_DUTY_LSB_SET_CHANNEL13 0x2F +#define MP3326_PWM_DUTY_MSB_SET_CHANNEL13 0x30 +#define MP3326_PWM_CURRENT_SET_CHANNEL14 0x31 +#define MP3326_PWM_DUTY_LSB_SET_CHANNEL14 0x32 +#define MP3326_PWM_DUTY_MSB_SET_CHANNEL14 0x33 +#define MP3326_PWM_CURRENT_SET_CHANNEL15 0x34 +#define MP3326_PWM_DUTY_LSB_SET_CHANNEL15 0x35 +#define MP3326_PWM_DUTY_MSB_SET_CHANNEL15 0x36 +#define MP3326_PWM_CURRENT_SET_CHANNEL16 0x37 +#define MP3326_PWM_DUTY_LSB_SET_CHANNEL16 0x38 +#define MP3326_PWM_DUTY_MSB_SET_CHANNEL16 0x39 +#define MAX_BRIGHTNESS 63 + +enum led_ctrl { + ENABLE = 0, + BRIGHTNESS, + COLOR_L4, + COLOR_H8, + OPEN_FAULT, + SHORT_FAULT, + MAX_CTRL, +}; + +enum mp3326_channel { + CHANNEL1, + CHANNEL2, + CHANNEL3, + CHANNEL4, + CHANNEL5, + CHANNEL6, + CHANNEL7, + CHANNEL8, + CHANNEL9, + CHANNEL10, + CHANNEL11, + CHANNEL12, + CHANNEL13, + CHANNEL14, + CHANNEL15, + CHANNEL16, + MAX_CHANNEL, +}; + +#define MP3326_REG_CONNECT_INNER(prefix, range) prefix##range +#define MP3326_REG_CONNECT(prefix, range) \ + MP3326_REG_CONNECT_INNER(prefix, range) +#define MP3326_REG_FIELD(reg, minbit, maxbit) REG_FIELD(reg, minbit, maxbit) +#define RANGE(a, b) MP3326_REG_CONNECT_INNER(a, b) + +#define MP3326_CHANNEL_FIELD(bit, num, range) \ + { \ + MP3326_REG_FIELD(MP3326_REG_CONNECT(MP3326_PWM_CTRL_CHANNEL, \ + range), \ + bit, bit), \ + MP3326_REG_FIELD( \ + MP3326_REG_CONNECT( \ + MP3326_PWM_CURRENT_SET_CHANNEL, num), \ + 0, 5), \ + MP3326_REG_FIELD( \ + MP3326_REG_CONNECT( \ + MP3326_PWM_DUTY_LSB_SET_CHANNEL, num), \ + 0, 3), \ + MP3326_REG_FIELD( \ + MP3326_REG_CONNECT( \ + MP3326_PWM_DUTY_MSB_SET_CHANNEL, num), \ + 0, 7), \ + MP3326_REG_FIELD( \ + MP3326_REG_CONNECT( \ + MP3326_PWM_OPEN_FAULT_CHANNEL, range), \ + bit, bit), \ + MP3326_REG_FIELD( \ + MP3326_REG_CONNECT( \ + MP3326_PWM_SHORT_FAULT_CHANNEL, \ + range), \ + bit, bit), \ + } + +struct mp3326_led { + struct mp3326 *private_data; + struct led_classdev cdev; + struct mc_subled *subled_info; + int num_colors; +}; + +struct mp3326 { + struct i2c_client *client; + struct regmap *regmap; + struct regmap_field *regmap_fields[MAX_CHANNEL][MAX_CTRL]; + int num_of_leds; + struct mp3326_led leds[]; +}; + +static const struct regmap_config MP3326_regmap_config = { + .reg_bits = 8, + .val_bits = 8, +}; + +static const struct reg_field channels_reg_fields[MAX_CHANNEL][MAX_CTRL] = { + [CHANNEL1] = MP3326_CHANNEL_FIELD(0, 1, RANGE(_1, _8)), + [CHANNEL2] = MP3326_CHANNEL_FIELD(1, 2, RANGE(_1, _8)), + [CHANNEL3] = MP3326_CHANNEL_FIELD(2, 3, RANGE(_1, _8)), + [CHANNEL4] = MP3326_CHANNEL_FIELD(3, 4, RANGE(_1, _8)), + [CHANNEL5] = MP3326_CHANNEL_FIELD(4, 5, RANGE(_1, _8)), + [CHANNEL6] = MP3326_CHANNEL_FIELD(5, 6, RANGE(_1, _8)), + [CHANNEL7] = MP3326_CHANNEL_FIELD(6, 7, RANGE(_1, _8)), + [CHANNEL8] = MP3326_CHANNEL_FIELD(7, 8, RANGE(_1, _8)), + [CHANNEL9] = MP3326_CHANNEL_FIELD(0, 9, RANGE(_9, _16)), + [CHANNEL10] = MP3326_CHANNEL_FIELD(1, 10, RANGE(_9, _16)), + [CHANNEL11] = MP3326_CHANNEL_FIELD(2, 11, RANGE(_9, _16)), + [CHANNEL12] = MP3326_CHANNEL_FIELD(3, 12, RANGE(_9, _16)), + [CHANNEL13] = MP3326_CHANNEL_FIELD(4, 13, RANGE(_9, _16)), + [CHANNEL14] = MP3326_CHANNEL_FIELD(5, 14, RANGE(_9, _16)), + [CHANNEL15] = MP3326_CHANNEL_FIELD(6, 15, RANGE(_9, _16)), + [CHANNEL16] = MP3326_CHANNEL_FIELD(7, 16, RANGE(_9, _16)), +}; + +static int led_brightness_set(struct led_classdev *led_cdev, + enum led_brightness brightness) +{ + struct mp3326_led *led = + container_of(led_cdev, struct mp3326_led, cdev); + struct mp3326 *chip = led->private_data; + int ret; + int i; + + if (brightness > led_cdev->max_brightness) + brightness = led_cdev->max_brightness; + if (brightness < 0) + brightness = 0; + for (i = 0; i < led->num_colors; i++) { + ret = regmap_field_write( + chip->regmap_fields[led->subled_info[i].channel] + [BRIGHTNESS], + brightness); + if (ret) + return ret; + led->subled_info[i].brightness = brightness; + } + led_cdev->brightness = brightness; + return 0; +} +/* + * 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) +{ + 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; + ssize_t ret; + int r_val, g_val, b_val; + int i; + + ret = sscanf(buf, "%i %i %i", &r_val, &g_val, &b_val); + if (ret != 3 && ret != 1) + return ret; + + r_val = r_val * 4095 / 255 + (r_val * 4095 % 255) / (255 / 2); + g_val = g_val * 4095 / 255 + (g_val * 4095 % 255) / (255 / 2); + b_val = b_val * 4095 / 255 + (b_val * 4095 % 255) / (255 / 2); + + if (led->num_colors == 1) { + ret = regmap_field_write( + chip->regmap_fields[led->subled_info[i].channel] + [COLOR_L4], + r_val & 0x0f); + if (ret) + return ret; + ret = regmap_field_write( + chip->regmap_fields[led->subled_info[i].channel] + [COLOR_H8], + r_val >> 4); + if (ret) + return ret; + } else { + for (i = 0; i < led->num_colors; i++) { + switch (led->subled_info[i].color_index) { + case LED_COLOR_ID_RED: + ret = regmap_field_write( + chip->regmap_fields[led->subled_info[i] + .channel] + [COLOR_L4], + r_val & 0x0f); + if (ret) + return ret; + ret = regmap_field_write( + chip->regmap_fields[led->subled_info[i] + .channel] + [COLOR_H8], + r_val >> 4); + if (ret) + return ret; + break; + case LED_COLOR_ID_GREEN: + ret = regmap_field_write( + chip->regmap_fields[led->subled_info[i] + .channel] + [COLOR_L4], + g_val & 0x0f); + if (ret) + return ret; + ret = regmap_field_write( + chip->regmap_fields[led->subled_info[i] + .channel] + [COLOR_H8], + g_val >> 4); + if (ret) + return ret; + break; + case LED_COLOR_ID_BLUE: + ret = regmap_field_write( + chip->regmap_fields[led->subled_info[i] + .channel] + [COLOR_L4], + b_val & 0x0f); + if (ret) + return ret; + ret = regmap_field_write( + chip->regmap_fields[led->subled_info[i] + .channel] + [COLOR_H8], + b_val >> 4); + if (ret) + return ret; + break; + } + } + } + + return count; +} + +static int led_pwm_show(struct device *dev, struct device_attribute *attr, + char *buf) +{ + 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; + int r_val = 0, g_val = 0, b_val = 0, val; + int i; + + for (i = 0; i < led->num_colors; i++) { + switch (led->subled_info[i].color_index) { + case LED_COLOR_ID_RED: + ret = regmap_field_read( + chip->regmap_fields[led->subled_info[i].channel] + [COLOR_L4], + &val); + if (ret) + return ret; + r_val |= val; + ret = regmap_field_read( + chip->regmap_fields[led->subled_info[i].channel] + [COLOR_H8], + &val); + if (ret) + return ret; + r_val |= val << 4; + break; + case LED_COLOR_ID_GREEN: + ret = regmap_field_read( + chip->regmap_fields[led->subled_info[i].channel] + [COLOR_L4], + &val); + if (ret) + return ret; + g_val |= val; + ret = regmap_field_read( + chip->regmap_fields[led->subled_info[i].channel] + [COLOR_H8], + &val); + if (ret) + return ret; + g_val |= val << 4; + break; + case LED_COLOR_ID_BLUE: + ret = regmap_field_read( + chip->regmap_fields[led->subled_info[i].channel] + [COLOR_L4], + &val); + if (ret) + return ret; + b_val |= val; + ret = regmap_field_read( + chip->regmap_fields[led->subled_info[i].channel] + [COLOR_H8], + &val); + if (ret) + return ret; + b_val |= val << 4; + break; + default: + ret = regmap_field_read( + chip->regmap_fields[led->subled_info[i].channel] + [COLOR_L4], + &val); + if (ret) + return ret; + r_val |= val; + ret = regmap_field_read( + chip->regmap_fields[led->subled_info[i].channel] + [COLOR_H8], + &val); + if (ret) + return ret; + r_val |= val << 4; + break; + } + } + 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) +{ + 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; + + ret = kstrtouint(buf, 0, &val); + if (ret) + return ret; + for (i = 0; i < led->num_colors; i++) { + ret = regmap_field_write( + chip->regmap_fields[led->subled_info[i].channel] + [BRIGHTNESS], + led->subled_info[i].brightness); + if (ret) + return ret; + ret = regmap_field_write( + chip->regmap_fields[led->subled_info[i].channel][ENABLE], + !!val); + if (ret) + return ret; + } + + return count; +} + +static int led_enable_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + 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; + uint val, rval = 0; + int i, ret; + + for (i = 0; i < led->num_colors; i++) { + ret = regmap_field_read( + chip->regmap_fields[led->subled_info[i].channel][ENABLE], + &val); + + rval |= val << i; + if (ret) + return ret; + } + + if (rval) + return sysfs_emit(buf, "%s\n", "True"); + else + return sysfs_emit(buf, "%s\n", "False"); +} + +static int led_short_fault_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + 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; + uint val, rval = 0, i; + int ret; + + for (i = 0; i < led->num_colors; i++) { + ret = regmap_field_read( + chip->regmap_fields[led->subled_info[i].channel] + [SHORT_FAULT], + &val); + rval |= val << i; + if (ret) + return ret; + } + + if (rval) + return sysfs_emit(buf, "%s\n", "Occur"); + else + return sysfs_emit(buf, "%s\n", "None"); +} + +static int led_open_fault_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + 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; + uint val, rval = 0, i; + int ret; + + for (i = 0; i < led->num_colors; i++) { + ret = regmap_field_read( + chip->regmap_fields[led->subled_info[i].channel] + [OPEN_FAULT], + &val); + rval |= val << i; + if (ret) + return ret; + } + + if (rval) + return sysfs_emit(buf, "%s\n", "Occur"); + else + return sysfs_emit(buf, "%s\n", "None"); +} + +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); + +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; + } + 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; + } + } + 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; + ret = regmap_write(chip->regmap, MP3326_PWM_CTRL_CHANNEL_1_8, 0); + if (ret) + return ret; + 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) { + return dev_err_probe(&client->dev, -EINVAL, + "Incorrect number of leds (%d)", count); + } + 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]); + } + } + if (mp3326_parse_dt(chip)) + return 1; + else + return 0; +} + +static const struct i2c_device_id mp3326_id[] = { { "mp3326", 0 }, {} }; +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@monolithicpower.com>"); +MODULE_DESCRIPTION("MPS MP3326 LED driver"); +MODULE_LICENSE("GPL");
This commit adds support for MPS MP3326 LED driver. Signed-off-by: Yuxi Wang <Yuxi.Wang@monolithicpower.com>