diff mbox series

[1/2] leds: aw21024: Add support for Awinic's AW21024

Message ID 20220513190409.3682501-1-kyle.swenson@est.tech
State New
Headers show
Series [1/2] leds: aw21024: Add support for Awinic's AW21024 | expand

Commit Message

Kyle Swenson May 13, 2022, 7:04 p.m. UTC
The Awinic AW21024 LED controller is a 24-channel RGB LED controller.
Each LED on the controller can be controlled individually or grouped
with other LEDs on the controller to form a multi-color LED.  Arbitrary
combinations of individual and grouped LED control should be possible.

Signed-off-by: Kyle Swenson <kyle.swenson@est.tech>
---
 drivers/leds/Kconfig        |  11 ++
 drivers/leds/Makefile       |   1 +
 drivers/leds/leds-aw21024.c | 314 ++++++++++++++++++++++++++++++++++++
 3 files changed, 326 insertions(+)
 create mode 100644 drivers/leds/leds-aw21024.c

Comments

Rob Herring (Arm) May 13, 2022, 8:48 p.m. UTC | #1
On Fri, 13 May 2022 13:04:09 -0600, Kyle Swenson wrote:
> Add device-tree bindings for Awinic's aw21024 24 channel RGB LED Driver.
> 
> Datasheet:
> https://www.awinic.com/Public/Uploads/uploadfile/files/20200511/20200511165751_5eb9138fcd9e3.PDF
> 
> Signed-off-by: Kyle Swenson <kyle.swenson@est.tech>
> ---
>  .../bindings/leds/leds-aw21024.yaml           | 157 ++++++++++++++++++
>  1 file changed, 157 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-aw21024.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/leds/leds-aw21024.yaml:54:1: [error] duplication of key "patternProperties" in mapping (key-duplicates)

dtschema/dtc warnings/errors:
make[1]: *** Deleting file 'Documentation/devicetree/bindings/leds/leds-aw21024.example.dts'
Documentation/devicetree/bindings/leds/leds-aw21024.yaml: found duplicate key "patternProperties" with value "{}" (original value: "{}")
make[1]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/leds/leds-aw21024.example.dts] Error 1
make[1]: *** Waiting for unfinished jobs....
Traceback (most recent call last):
  File "/usr/local/bin/dt-doc-validate", line 25, in check_doc
    testtree = dtschema.load(filename, line_number=line_number)
  File "/usr/local/lib/python3.10/dist-packages/dtschema/lib.py", line 914, in load
    return yaml.load(f.read())
  File "/usr/local/lib/python3.10/dist-packages/ruamel/yaml/main.py", line 434, in load
    return constructor.get_single_data()
  File "/usr/local/lib/python3.10/dist-packages/ruamel/yaml/constructor.py", line 121, in get_single_data
    return self.construct_document(node)
  File "/usr/local/lib/python3.10/dist-packages/ruamel/yaml/constructor.py", line 131, in construct_document
    for _dummy in generator:
  File "/usr/local/lib/python3.10/dist-packages/ruamel/yaml/constructor.py", line 674, in construct_yaml_map
    value = self.construct_mapping(node)
  File "/usr/local/lib/python3.10/dist-packages/ruamel/yaml/constructor.py", line 445, in construct_mapping
    return BaseConstructor.construct_mapping(self, node, deep=deep)
  File "/usr/local/lib/python3.10/dist-packages/ruamel/yaml/constructor.py", line 263, in construct_mapping
    if self.check_mapping_key(node, key_node, mapping, key, value):
  File "/usr/local/lib/python3.10/dist-packages/ruamel/yaml/constructor.py", line 294, in check_mapping_key
    raise DuplicateKeyError(*args)
ruamel.yaml.constructor.DuplicateKeyError: while constructing a mapping
  in "<unicode string>", line 4, column 1
found duplicate key "patternProperties" with value "{}" (original value: "{}")
  in "<unicode string>", line 54, column 1

To suppress this check see:
    http://yaml.readthedocs.io/en/latest/api.html#duplicate-keys


During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/local/bin/dt-doc-validate", line 74, in <module>
    ret = check_doc(f)
  File "/usr/local/bin/dt-doc-validate", line 30, in check_doc
    print(filename + ":", exc.path[-1], exc.message, file=sys.stderr)
AttributeError: 'DuplicateKeyError' object has no attribute 'path'
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/leds/leds-aw21024.yaml: ignoring, error parsing file
make: *** [Makefile:1401: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.
kernel test robot May 16, 2022, 10:21 a.m. UTC | #2
Hi Kyle,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on pavel-leds/for-next]
[also build test ERROR on robh/for-next v5.18-rc7 next-20220513]
[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]

url:    https://github.com/intel-lab-lkp/linux/commits/Kyle-Swenson/leds-aw21024-Add-support-for-Awinic-s-AW21024/20220514-030705
base:   git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git for-next
config: mips-randconfig-r006-20220516 (https://download.01.org/0day-ci/archive/20220516/202205161802.8WYsbizM-lkp@intel.com/config)
compiler: mips-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/38eeda60299918b5599f4a58714dc91f9741677c
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Kyle-Swenson/leds-aw21024-Add-support-for-Awinic-s-AW21024/20220514-030705
        git checkout 38eeda60299918b5599f4a58714dc91f9741677c
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash drivers/leds/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

   drivers/leds/leds-aw21024.c: In function 'aw21024_led_brightness_set':
>> drivers/leds/leds-aw21024.c:64:31: error: implicit declaration of function 'i2c_smbus_write_byte_data' [-Werror=implicit-function-declaration]
      64 |                         ret = i2c_smbus_write_byte_data(parent->client,
         |                               ^~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/leds/leds-aw21024.c: In function 'aw21024_configure':
>> drivers/leds/leds-aw21024.c:213:15: error: implicit declaration of function 'i2c_smbus_read_byte_data' [-Werror=implicit-function-declaration]
     213 |         ret = i2c_smbus_read_byte_data(client, AW21024_REG_SW_RESET);
         |               ^~~~~~~~~~~~~~~~~~~~~~~~
   drivers/leds/leds-aw21024.c: At top level:
>> drivers/leds/leds-aw21024.c:310:1: warning: data definition has no type or storage class
     310 | module_i2c_driver(aw21024_driver);
         | ^~~~~~~~~~~~~~~~~
>> drivers/leds/leds-aw21024.c:310:1: error: type defaults to 'int' in declaration of 'module_i2c_driver' [-Werror=implicit-int]
>> drivers/leds/leds-aw21024.c:310:1: warning: parameter names (without types) in function declaration
   drivers/leds/leds-aw21024.c:301:26: warning: 'aw21024_driver' defined but not used [-Wunused-variable]
     301 | static struct i2c_driver aw21024_driver = {
         |                          ^~~~~~~~~~~~~~
   drivers/leds/leds-aw21024.c:295:34: warning: 'of_aw21024_leds_match' defined but not used [-Wunused-const-variable=]
     295 | static const struct of_device_id of_aw21024_leds_match[] = {
         |                                  ^~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +/i2c_smbus_write_byte_data +64 drivers/leds/leds-aw21024.c

    51	
    52	static int aw21024_led_brightness_set(struct led_classdev *led_cdev,
    53						enum led_brightness brightness)
    54	{
    55		struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(led_cdev);
    56		struct aw21024_led_data *led = container_of(mc_cdev, struct aw21024_led_data, mc_cdev);
    57		struct aw21024 *parent = led->parent;
    58		int i;
    59		int ret = 0;
    60	
    61		mutex_lock(&parent->lock);
    62		if (mc_cdev->num_colors && mc_cdev->subled_info) {
    63			for (i = 0; i < led->nregs; i++) {
  > 64				ret = i2c_smbus_write_byte_data(parent->client,
    65						AW21024_REG_DC_CURRENT(led->regs[i]),
    66						mc_cdev->subled_info[i].intensity);
    67				if (ret < 0)
    68					goto unlock_ret;
    69	
    70				ret = i2c_smbus_write_byte_data(parent->client,
    71						AW21024_REG_BRIGHTNESS(led->regs[i]),
    72						brightness);
    73				if (ret < 0)
    74					goto unlock_ret;
    75			}
    76		} else {
    77			ret = i2c_smbus_write_byte_data(parent->client,
    78						AW21024_REG_DC_CURRENT(led->regs[0]), 0xFF);
    79			if (ret < 0)
    80				goto unlock_ret;
    81	
    82			ret = i2c_smbus_write_byte_data(parent->client,
    83						AW21024_REG_BRIGHTNESS(led->regs[0]),
    84						brightness);
    85			if (ret < 0)
    86				goto unlock_ret;
    87		}
    88		ret = i2c_smbus_write_byte_data(parent->client, AW21024_REG_UPDATE, 0x0);
    89	unlock_ret:
    90		mutex_unlock(&parent->lock);
    91		return ret;
    92	}
    93	
    94	static int aw21024_probe_dt(struct aw21024 *data)
    95	{
    96		struct device *dev = &data->client->dev;
    97		struct fwnode_handle *child = NULL;
    98		struct fwnode_handle *led_node = NULL;
    99		struct led_init_data init_data = {};
   100		u32 color_id;
   101		int  ret, num_colors;
   102		unsigned int nleds = 0;
   103		struct aw21024_led_data *led;
   104		struct led_classdev *led_cdev;
   105		struct mc_subled *mc_led_info;
   106	
   107		nleds = device_get_child_node_count(dev);
   108	
   109		data->leds = devm_kcalloc(dev, nleds, sizeof(*(data->leds)), GFP_KERNEL);
   110		if (!data->leds)
   111			return -ENOMEM;
   112	
   113		device_for_each_child_node(dev, child) {
   114			led = devm_kzalloc(dev, sizeof(struct aw21024_led_data), GFP_KERNEL);
   115			if (!led) {
   116				ret = -ENOMEM;
   117				goto ret_put_child;
   118			}
   119			led->parent = data;
   120			led_cdev = &led->mc_cdev.led_cdev;
   121			init_data.fwnode = child;
   122	
   123			led_cdev->brightness_set_blocking = aw21024_led_brightness_set;
   124			data->leds[data->nleds] = led;
   125	
   126			ret = fwnode_property_count_u32(child, "reg");
   127			if (ret < 0) {
   128				dev_err(dev, "reg property is invalid in node %s\n",
   129						fwnode_get_name(child));
   130				goto ret_put_child;
   131			}
   132	
   133			led->regs = devm_kcalloc(dev, ret, sizeof(*(led->regs)), GFP_KERNEL);
   134			led->nregs = ret;
   135			if (!led->regs) {
   136				ret = -ENOMEM;
   137				goto ret_put_child;
   138			}
   139	
   140			ret = fwnode_property_read_u32_array(child, "reg", led->regs, led->nregs);
   141			if (ret) {
   142				dev_err(dev, "Failed to read reg array, error=%d\n", ret);
   143				goto ret_put_child;
   144			}
   145	
   146			if (led->nregs > 1) {
   147				mc_led_info = devm_kcalloc(dev, led->nregs,
   148							    sizeof(*mc_led_info), GFP_KERNEL);
   149				if (!mc_led_info) {
   150					ret = -ENOMEM;
   151					goto ret_put_child;
   152				}
   153	
   154				num_colors = 0;
   155				fwnode_for_each_child_node(child, led_node) {
   156					if (num_colors > led->nregs) {
   157						ret = -EINVAL;
   158						fwnode_handle_put(led_node);
   159						goto ret_put_child;
   160					}
   161	
   162					ret = fwnode_property_read_u32(led_node, "color",
   163								       &color_id);
   164					if (ret) {
   165						fwnode_handle_put(led_node);
   166						goto ret_put_child;
   167					}
   168					mc_led_info[num_colors].color_index = color_id;
   169					num_colors++;
   170				}
   171	
   172				led->mc_cdev.num_colors = num_colors;
   173				led->mc_cdev.subled_info = mc_led_info;
   174				ret = devm_led_classdev_multicolor_register_ext(dev,
   175									&led->mc_cdev,
   176									&init_data);
   177				if (ret < 0) {
   178					dev_warn(dev, "Failed to register multicolor LED %s, err=%d\n",
   179							    fwnode_get_name(child), ret);
   180					goto ret_put_child;
   181				}
   182			} else {
   183				ret = devm_led_classdev_register_ext(dev,
   184								    &led->mc_cdev.led_cdev, &init_data);
   185				if (ret < 0) {
   186					dev_warn(dev, "Failed to register LED %s, err=%d\n",
   187							fwnode_get_name(child), ret);
   188					goto ret_put_child;
   189				}
   190			}
   191			data->nleds++;
   192		}
   193	
   194		return 0;
   195	
   196	ret_put_child:
   197		fwnode_handle_put(child);
   198		return ret;
   199	}
   200	
   201	/* Expected to be called prior to registering with the LEDs class */
   202	static int aw21024_configure(struct aw21024 *priv)
   203	{
   204		int ret = 0;
   205		struct i2c_client *client = priv->client;
   206	
   207		ret = i2c_smbus_write_byte_data(client, AW21024_REG_GCR0, AW21024_GCR0_CHIPEN);
   208		if (ret < 0) {
   209			dev_err(&client->dev, "Failed to write chip enable\n");
   210			return -ENODEV;
   211		}
   212	
 > 213		ret = i2c_smbus_read_byte_data(client, AW21024_REG_SW_RESET);
   214		if (ret < 0) {
   215			dev_err(&client->dev, "Failed to read chip id\n");
   216			return -ENODEV;
   217		}
   218	
   219		if (ret != AW21024_CHIP_ID) {
   220			dev_err(&client->dev, "Chip ID 0x%02X doesn't match expected (0x%02X)\n",
   221									ret, AW21024_CHIP_ID);
   222			return -ENODEV;
   223		}
   224	
   225		ret = i2c_smbus_read_byte_data(client, AW21024_REG_VERSION);
   226		if (ret < 0) {
   227			dev_err(&client->dev, "Failed to read chip version\n");
   228			return -ENODEV;
   229		}
   230		if (ret != AW21024_CHIP_VERSION)
   231			dev_warn(&client->dev, "Chip version 0x%02X doesn't match expected 0x%02X\n",
   232									ret, AW21024_CHIP_VERSION);
   233	
   234		i2c_smbus_write_byte_data(client, AW21024_REG_SW_RESET, 0x00);
   235		mdelay(2);
   236		i2c_smbus_write_byte_data(client, AW21024_REG_GCR0, AW21024_GCR0_CHIPEN);
   237		i2c_smbus_write_byte_data(client, AW21024_REG_GCC, 0xFF);
   238	
   239		return 0;
   240	}
   241
kernel test robot May 16, 2022, 10:42 a.m. UTC | #3
Hi Kyle,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on pavel-leds/for-next]
[also build test WARNING on robh/for-next v5.18-rc7 next-20220513]
[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]

url:    https://github.com/intel-lab-lkp/linux/commits/Kyle-Swenson/leds-aw21024-Add-support-for-Awinic-s-AW21024/20220514-030705
base:   git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git for-next
config: x86_64-randconfig-r011-20220516 (https://download.01.org/0day-ci/archive/20220516/202205161859.bDAFU09i-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.2.0-20) 11.2.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/38eeda60299918b5599f4a58714dc91f9741677c
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Kyle-Swenson/leds-aw21024-Add-support-for-Awinic-s-AW21024/20220514-030705
        git checkout 38eeda60299918b5599f4a58714dc91f9741677c
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/leds/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/leds/leds-aw21024.c:295:34: warning: 'of_aw21024_leds_match' defined but not used [-Wunused-const-variable=]
     295 | static const struct of_device_id of_aw21024_leds_match[] = {
         |                                  ^~~~~~~~~~~~~~~~~~~~~


vim +/of_aw21024_leds_match +295 drivers/leds/leds-aw21024.c

   294	
 > 295	static const struct of_device_id of_aw21024_leds_match[] = {
   296		{ .compatible = "awinic,aw21024", },
   297		{},
   298	};
   299	MODULE_DEVICE_TABLE(of, of_aw21024_leds_match);
   300
kernel test robot May 16, 2022, 12:34 p.m. UTC | #4
Hi Kyle,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on pavel-leds/for-next]
[also build test WARNING on robh/for-next v5.18-rc7 next-20220513]
[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]

url:    https://github.com/intel-lab-lkp/linux/commits/Kyle-Swenson/leds-aw21024-Add-support-for-Awinic-s-AW21024/20220514-030705
base:   git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git for-next
config: hexagon-randconfig-r014-20220516 (https://download.01.org/0day-ci/archive/20220516/202205162025.GEAQ50Y7-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 853fa8ee225edf2d0de94b0dcbd31bea916e825e)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/38eeda60299918b5599f4a58714dc91f9741677c
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Kyle-Swenson/leds-aw21024-Add-support-for-Awinic-s-AW21024/20220514-030705
        git checkout 38eeda60299918b5599f4a58714dc91f9741677c
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/leds/leds-aw21024.c:295:34: warning: unused variable 'of_aw21024_leds_match' [-Wunused-const-variable]
   static const struct of_device_id of_aw21024_leds_match[] = {
                                    ^
   1 warning generated.


vim +/of_aw21024_leds_match +295 drivers/leds/leds-aw21024.c

   294	
 > 295	static const struct of_device_id of_aw21024_leds_match[] = {
   296		{ .compatible = "awinic,aw21024", },
   297		{},
   298	};
   299	MODULE_DEVICE_TABLE(of, of_aw21024_leds_match);
   300
Krzysztof Kozlowski May 17, 2022, 9:11 a.m. UTC | #5
On 13/05/2022 21:04, Kyle Swenson wrote:
> The Awinic AW21024 LED controller is a 24-channel RGB LED controller.
> Each LED on the controller can be controlled individually or grouped
> with other LEDs on the controller to form a multi-color LED.  Arbitrary
> combinations of individual and grouped LED control should be possible.
> 
> Signed-off-by: Kyle Swenson <kyle.swenson@est.tech>

Thank you for your patch. There is something to discuss/improve.

> +
> +static const struct i2c_device_id aw21024_id[] = {
> +	{ "aw21024", 0 }, /* 24 Channel */
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, aw21024_id);
> +
> +static const struct of_device_id of_aw21024_leds_match[] = {
> +	{ .compatible = "awinic,aw21024", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, of_aw21024_leds_match);
> +
> +static struct i2c_driver aw21024_driver = {
> +	.driver		= {
> +		.name	= "aw21024",
> +		.of_match_table = of_match_ptr(of_aw21024_leds_match),

of_match_ptr causes this being unused. kbuild robot probably pointed
this out... if not - of_match_ptr goes with maybe_unused. You need both
or none, depending on intended usage.

> +	},
> +	.probe_new		= aw21024_probe,
> +	.remove		= aw21024_remove,
> +	.id_table = aw21024_id,

Why other places are indented but this not?


> +};
> +module_i2c_driver(aw21024_driver);
> +
> +MODULE_AUTHOR("Kyle Swenson <kyle.swenson@est.tech>");
> +MODULE_DESCRIPTION("Awinic AW21024 LED driver");
> +MODULE_LICENSE("GPL");


Best regards,
Krzysztof
Kyle Swenson May 17, 2022, 6:36 p.m. UTC | #6
On Tue, May 17, 2022 at 11:11:37AM +0200, Krzysztof Kozlowski wrote:
> On 13/05/2022 21:04, Kyle Swenson wrote:
> > The Awinic AW21024 LED controller is a 24-channel RGB LED controller.
> > Each LED on the controller can be controlled individually or grouped
> > with other LEDs on the controller to form a multi-color LED.  Arbitrary
> > combinations of individual and grouped LED control should be possible.
> > 
> > Signed-off-by: Kyle Swenson <kyle.swenson@est.tech>
> 
> Thank you for your patch. There is something to discuss/improve.
> 
> > +
> > +static const struct i2c_device_id aw21024_id[] = {
> > +	{ "aw21024", 0 }, /* 24 Channel */
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, aw21024_id);
> > +
> > +static const struct of_device_id of_aw21024_leds_match[] = {
> > +	{ .compatible = "awinic,aw21024", },
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, of_aw21024_leds_match);
> > +
> > +static struct i2c_driver aw21024_driver = {
> > +	.driver		= {
> > +		.name	= "aw21024",
> > +		.of_match_table = of_match_ptr(of_aw21024_leds_match),
> 
> of_match_ptr causes this being unused. kbuild robot probably pointed
> this out... if not - of_match_ptr goes with maybe_unused. You need both
> or none, depending on intended usage.
> 
Ah, yes, the kbuild robot did point this out to me, and I had planned on
fixing by adding 'depends on OF' to the Kconfig.  Perhaps that isn't
correct or complete (or even relevant)?

I'll do some investigating and determine if I need to use of_match_ptr
or not and I'll fix it either by removing it or adding maybe_unused in
the next version.

> > +	},
> > +	.probe_new		= aw21024_probe,
> > +	.remove		= aw21024_remove,
> > +	.id_table = aw21024_id,
> 
> Why other places are indented but this not?
Sorry, it should be.  My editor was configured wrong and this now looks
bad.  There are a few other places in the driver that also now look bad
and I'll fix those before submitting v2.
> 
> 
> > +};
> > +module_i2c_driver(aw21024_driver);
> > +
> > +MODULE_AUTHOR("Kyle Swenson <kyle.swenson@est.tech>");
> > +MODULE_DESCRIPTION("Awinic AW21024 LED driver");
> > +MODULE_LICENSE("GPL");
> 
> 
> Best regards,
> Krzysztof

Thanks so much for taking a look at this, I really appreciate your time
and patience.

Thanks,
Kyle
Krzysztof Kozlowski May 18, 2022, 8:30 a.m. UTC | #7
On 17/05/2022 20:36, Kyle Swenson wrote:
>>> +static const struct of_device_id of_aw21024_leds_match[] = {
>>> +	{ .compatible = "awinic,aw21024", },
>>> +	{},
>>> +};
>>> +MODULE_DEVICE_TABLE(of, of_aw21024_leds_match);
>>> +
>>> +static struct i2c_driver aw21024_driver = {
>>> +	.driver		= {
>>> +		.name	= "aw21024",
>>> +		.of_match_table = of_match_ptr(of_aw21024_leds_match),
>>
>> of_match_ptr causes this being unused. kbuild robot probably pointed
>> this out... if not - of_match_ptr goes with maybe_unused. You need both
>> or none, depending on intended usage.
>>
> Ah, yes, the kbuild robot did point this out to me, and I had planned on
> fixing by adding 'depends on OF' to the Kconfig.  Perhaps that isn't
> correct or complete (or even relevant)?
> 
> I'll do some investigating and determine if I need to use of_match_ptr
> or not and I'll fix it either by removing it or adding maybe_unused in
> the next version.

Your has i2c_device_id so it could bind without OF, however obviously
aw21024_probe_dt() will do nothing and return 0.

Therefore it is up to you if you want to add dependency on OF. If you
add, please add it with "|| COMPILE_TEST".

Then in both cases you need to handle the case of building (not running)
a driver without OF: using maybe_unused+of_match_ptr() or nothing (thus
always referencing of_device_id). Which one to choose matters less.
Using it causes the code to be smaller for !OF case, which might matter
for some distros which build everything as module. Not using it allows
to match the driver on ACPI systems, although I am not sure if this is
relevant.

I don't have recommendation on that - just be sure there are no warnings.


Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index a49979f41eee..c813d7c16ff8 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -102,10 +102,21 @@  config LEDS_AW2013
 	  LED driver.
 
 	  To compile this driver as a module, choose M here: the module
 	  will be called leds-aw2013.
 
+config LEDS_AW21024
+	tristate "LED Support for Awinic AW21024"
+	depends on LEDS_CLASS
+	depends on LEDS_CLASS_MULTICOLOR || !LEDS_CLASS_MULTICOLOR
+	help
+	  If you say yes here you get support for Awinic's AW21024, a 24-channel
+	  RGB LED Driver.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called leds-aw21024.
+
 config LEDS_BCM6328
 	tristate "LED Support for Broadcom BCM6328"
 	depends on LEDS_CLASS
 	depends on HAS_IOMEM
 	depends on OF
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 4fd2f92cd198..09a0e3cb21cb 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -14,10 +14,11 @@  obj-$(CONFIG_LEDS_ADP5520)		+= leds-adp5520.o
 obj-$(CONFIG_LEDS_AN30259A)		+= leds-an30259a.o
 obj-$(CONFIG_LEDS_APU)			+= leds-apu.o
 obj-$(CONFIG_LEDS_ARIEL)		+= leds-ariel.o
 obj-$(CONFIG_LEDS_ASIC3)		+= leds-asic3.o
 obj-$(CONFIG_LEDS_AW2013)		+= leds-aw2013.o
+obj-$(CONFIG_LEDS_AW21024)		+= leds-aw21024.o
 obj-$(CONFIG_LEDS_BCM6328)		+= leds-bcm6328.o
 obj-$(CONFIG_LEDS_BCM6358)		+= leds-bcm6358.o
 obj-$(CONFIG_LEDS_BD2802)		+= leds-bd2802.o
 obj-$(CONFIG_LEDS_BLINKM)		+= leds-blinkm.o
 obj-$(CONFIG_LEDS_CLEVO_MAIL)		+= leds-clevo-mail.o
diff --git a/drivers/leds/leds-aw21024.c b/drivers/leds/leds-aw21024.c
new file mode 100644
index 000000000000..4eebc3de1a8a
--- /dev/null
+++ b/drivers/leds/leds-aw21024.c
@@ -0,0 +1,314 @@ 
+// SPDX-License-Identifier: GPL-2.0
+// Awinic AW21024 LED chip driver
+// Copyright (C) 2022 Nordix Foundation https://www.nordix.org
+
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/leds.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/delay.h>
+#include <linux/slab.h>
+#include <uapi/linux/uleds.h>
+
+#include <linux/led-class-multicolor.h>
+
+/* Called COL0, COL1,..., COL23 in datasheet */
+#define AW21024_REG_DC_CURRENT(_led)	(0x4a + (_led))
+
+/* Called BR0, BR1,..., BR23 in datasheet */
+#define AW21024_REG_BRIGHTNESS(_led)	(0x01 + (_led))
+
+#define AW21024_REG_UPDATE				0x49 /* Write 0x00 to update BR */
+
+#define AW21024_REG_GCR0				0x00 /* Global configuration register */
+#define AW21024_REG_GCC					0x6e /* Global current control */
+#define AW21024_REG_SW_RESET			0x7f
+#define AW21024_REG_VERSION				0x7e
+
+#define AW21024_GCR0_CHIPEN				BIT(0)
+#define AW21024_CHIP_ID					0x18
+#define AW21024_CHIP_VERSION			0xA8
+
+struct aw21024_led_data {
+	struct led_classdev_mc mc_cdev;
+	struct work_struct work;
+	unsigned int *regs;
+	unsigned int nregs;
+	struct aw21024 *parent;
+};
+
+struct aw21024 {
+	struct i2c_client *client;
+	struct device *dev;
+	struct gpio_desc *enable_gpio;
+	struct mutex lock;
+	struct aw21024_led_data **leds;
+	unsigned int nleds;
+};
+
+static int aw21024_led_brightness_set(struct led_classdev *led_cdev,
+					enum led_brightness brightness)
+{
+	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(led_cdev);
+	struct aw21024_led_data *led = container_of(mc_cdev, struct aw21024_led_data, mc_cdev);
+	struct aw21024 *parent = led->parent;
+	int i;
+	int ret = 0;
+
+	mutex_lock(&parent->lock);
+	if (mc_cdev->num_colors && mc_cdev->subled_info) {
+		for (i = 0; i < led->nregs; i++) {
+			ret = i2c_smbus_write_byte_data(parent->client,
+					AW21024_REG_DC_CURRENT(led->regs[i]),
+					mc_cdev->subled_info[i].intensity);
+			if (ret < 0)
+				goto unlock_ret;
+
+			ret = i2c_smbus_write_byte_data(parent->client,
+					AW21024_REG_BRIGHTNESS(led->regs[i]),
+					brightness);
+			if (ret < 0)
+				goto unlock_ret;
+		}
+	} else {
+		ret = i2c_smbus_write_byte_data(parent->client,
+					AW21024_REG_DC_CURRENT(led->regs[0]), 0xFF);
+		if (ret < 0)
+			goto unlock_ret;
+
+		ret = i2c_smbus_write_byte_data(parent->client,
+					AW21024_REG_BRIGHTNESS(led->regs[0]),
+					brightness);
+		if (ret < 0)
+			goto unlock_ret;
+	}
+	ret = i2c_smbus_write_byte_data(parent->client, AW21024_REG_UPDATE, 0x0);
+unlock_ret:
+	mutex_unlock(&parent->lock);
+	return ret;
+}
+
+static int aw21024_probe_dt(struct aw21024 *data)
+{
+	struct device *dev = &data->client->dev;
+	struct fwnode_handle *child = NULL;
+	struct fwnode_handle *led_node = NULL;
+	struct led_init_data init_data = {};
+	u32 color_id;
+	int  ret, num_colors;
+	unsigned int nleds = 0;
+	struct aw21024_led_data *led;
+	struct led_classdev *led_cdev;
+	struct mc_subled *mc_led_info;
+
+	nleds = device_get_child_node_count(dev);
+
+	data->leds = devm_kcalloc(dev, nleds, sizeof(*(data->leds)), GFP_KERNEL);
+	if (!data->leds)
+		return -ENOMEM;
+
+	device_for_each_child_node(dev, child) {
+		led = devm_kzalloc(dev, sizeof(struct aw21024_led_data), GFP_KERNEL);
+		if (!led) {
+			ret = -ENOMEM;
+			goto ret_put_child;
+		}
+		led->parent = data;
+		led_cdev = &led->mc_cdev.led_cdev;
+		init_data.fwnode = child;
+
+		led_cdev->brightness_set_blocking = aw21024_led_brightness_set;
+		data->leds[data->nleds] = led;
+
+		ret = fwnode_property_count_u32(child, "reg");
+		if (ret < 0) {
+			dev_err(dev, "reg property is invalid in node %s\n",
+					fwnode_get_name(child));
+			goto ret_put_child;
+		}
+
+		led->regs = devm_kcalloc(dev, ret, sizeof(*(led->regs)), GFP_KERNEL);
+		led->nregs = ret;
+		if (!led->regs) {
+			ret = -ENOMEM;
+			goto ret_put_child;
+		}
+
+		ret = fwnode_property_read_u32_array(child, "reg", led->regs, led->nregs);
+		if (ret) {
+			dev_err(dev, "Failed to read reg array, error=%d\n", ret);
+			goto ret_put_child;
+		}
+
+		if (led->nregs > 1) {
+			mc_led_info = devm_kcalloc(dev, led->nregs,
+						    sizeof(*mc_led_info), GFP_KERNEL);
+			if (!mc_led_info) {
+				ret = -ENOMEM;
+				goto ret_put_child;
+			}
+
+			num_colors = 0;
+			fwnode_for_each_child_node(child, led_node) {
+				if (num_colors > led->nregs) {
+					ret = -EINVAL;
+					fwnode_handle_put(led_node);
+					goto ret_put_child;
+				}
+
+				ret = fwnode_property_read_u32(led_node, "color",
+							       &color_id);
+				if (ret) {
+					fwnode_handle_put(led_node);
+					goto ret_put_child;
+				}
+				mc_led_info[num_colors].color_index = color_id;
+				num_colors++;
+			}
+
+			led->mc_cdev.num_colors = num_colors;
+			led->mc_cdev.subled_info = mc_led_info;
+			ret = devm_led_classdev_multicolor_register_ext(dev,
+								&led->mc_cdev,
+								&init_data);
+			if (ret < 0) {
+				dev_warn(dev, "Failed to register multicolor LED %s, err=%d\n",
+						    fwnode_get_name(child), ret);
+				goto ret_put_child;
+			}
+		} else {
+			ret = devm_led_classdev_register_ext(dev,
+							    &led->mc_cdev.led_cdev, &init_data);
+			if (ret < 0) {
+				dev_warn(dev, "Failed to register LED %s, err=%d\n",
+						fwnode_get_name(child), ret);
+				goto ret_put_child;
+			}
+		}
+		data->nleds++;
+	}
+
+	return 0;
+
+ret_put_child:
+	fwnode_handle_put(child);
+	return ret;
+}
+
+/* Expected to be called prior to registering with the LEDs class */
+static int aw21024_configure(struct aw21024 *priv)
+{
+	int ret = 0;
+	struct i2c_client *client = priv->client;
+
+	ret = i2c_smbus_write_byte_data(client, AW21024_REG_GCR0, AW21024_GCR0_CHIPEN);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to write chip enable\n");
+		return -ENODEV;
+	}
+
+	ret = i2c_smbus_read_byte_data(client, AW21024_REG_SW_RESET);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to read chip id\n");
+		return -ENODEV;
+	}
+
+	if (ret != AW21024_CHIP_ID) {
+		dev_err(&client->dev, "Chip ID 0x%02X doesn't match expected (0x%02X)\n",
+								ret, AW21024_CHIP_ID);
+		return -ENODEV;
+	}
+
+	ret = i2c_smbus_read_byte_data(client, AW21024_REG_VERSION);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to read chip version\n");
+		return -ENODEV;
+	}
+	if (ret != AW21024_CHIP_VERSION)
+		dev_warn(&client->dev, "Chip version 0x%02X doesn't match expected 0x%02X\n",
+								ret, AW21024_CHIP_VERSION);
+
+	i2c_smbus_write_byte_data(client, AW21024_REG_SW_RESET, 0x00);
+	mdelay(2);
+	i2c_smbus_write_byte_data(client, AW21024_REG_GCR0, AW21024_GCR0_CHIPEN);
+	i2c_smbus_write_byte_data(client, AW21024_REG_GCC, 0xFF);
+
+	return 0;
+}
+
+static int aw21024_probe(struct i2c_client *client)
+{
+	struct aw21024 *priv;
+	int ret;
+
+	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->client = client;
+	priv->dev = &client->dev;
+
+	mutex_init(&priv->lock);
+
+	priv->enable_gpio = devm_gpiod_get_optional(priv->dev, "enable", GPIOD_OUT_LOW);
+	if (IS_ERR(priv->enable_gpio))
+		return dev_err_probe(priv->dev, PTR_ERR(priv->enable_gpio),
+				       "Failed to get enable GPIO\n");
+
+	if (priv->enable_gpio) {
+		mdelay(1);
+		gpiod_direction_output(priv->enable_gpio, 1);
+		mdelay(1);
+	}
+
+	i2c_set_clientdata(client, priv);
+
+	ret = aw21024_configure(priv);
+	if (ret < 0)
+		return ret;
+
+	return aw21024_probe_dt(priv);
+}
+
+static int aw21024_remove(struct i2c_client *client)
+{
+	struct aw21024 *priv = i2c_get_clientdata(client);
+	int ret;
+
+	ret = gpiod_direction_output(priv->enable_gpio, 0);
+	if (ret)
+		dev_err(priv->dev, "Failed to disable chip, err=%d\n", ret);
+
+	mutex_destroy(&priv->lock);
+	return 0;
+}
+
+static const struct i2c_device_id aw21024_id[] = {
+	{ "aw21024", 0 }, /* 24 Channel */
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, aw21024_id);
+
+static const struct of_device_id of_aw21024_leds_match[] = {
+	{ .compatible = "awinic,aw21024", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, of_aw21024_leds_match);
+
+static struct i2c_driver aw21024_driver = {
+	.driver		= {
+		.name	= "aw21024",
+		.of_match_table = of_match_ptr(of_aw21024_leds_match),
+	},
+	.probe_new		= aw21024_probe,
+	.remove		= aw21024_remove,
+	.id_table = aw21024_id,
+};
+module_i2c_driver(aw21024_driver);
+
+MODULE_AUTHOR("Kyle Swenson <kyle.swenson@est.tech>");
+MODULE_DESCRIPTION("Awinic AW21024 LED driver");
+MODULE_LICENSE("GPL");