Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <ba4918dd-309c-5e6e-dc0a-eb5f1c4b1dfa@linaro.org>
Date:   Fri, 25 Nov 2022 09:19:38 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.0
Subject: Re: [PATCH v1 2/2] leds: add aw20xx driver
Content-Language: en-US
To:     Martin Kurbanov <mmkurbanov@sberdevices.ru>,
        Pavel Machek <pavel@ucw.cz>, Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Jonathan Corbet <corbet@lwn.net>,
        Andy Shevchenko <andy.shevchenko@gmail.com>
Cc:     linux-leds@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
        kernel@sberdevices.ru
References: <20221124204807.1593241-1-mmkurbanov@sberdevices.ru>
 <20221124204807.1593241-3-mmkurbanov@sberdevices.ru>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20221124204807.1593241-3-mmkurbanov@sberdevices.ru>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 24/11/2022 21:48, Martin Kurbanov wrote:
> This commit adds support for AWINIC AW20036/AW20054/AW20072 LED driver.
> This driver supports following AW200XX features:
>   - 3 pattern controllers for auto breathing or group dimming control
>   - Individual 64-level DIM currents
>   - Interrupt output, low active
> 
> Signed-off-by: Martin Kurbanov <mmkurbanov@sberdevices.ru>
> ---
>  Documentation/leds/leds-aw200xx.rst |  274 +++++++
>  drivers/leds/Kconfig                |   10 +
>  drivers/leds/Makefile               |    1 +
>  drivers/leds/leds-aw200xx.c         | 1113 +++++++++++++++++++++++++++
>  4 files changed, 1398 insertions(+)
>  create mode 100644 Documentation/leds/leds-aw200xx.rst
>  create mode 100644 drivers/leds/leds-aw200xx.c
> 
> diff --git a/Documentation/leds/leds-aw200xx.rst b/Documentation/leds/leds-aw200xx.rst
> new file mode 100644
> index 000000000000..a751b91dfda6
> --- /dev/null
> +++ b/Documentation/leds/leds-aw200xx.rst
> @@ -0,0 +1,274 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +=========================================
> +Kernel driver for AW20036/AW20054/AW20072
> +=========================================
> +
> +Description
> +-----------
> +
> +The AW20036/AW20054/AW20072 is a 3x12/6x9/6x12 matrix LED driver programmed via
> +an I2C interface. The brightness of each LED is independently controlled by
> +FADE and DIM parameter.
> +
> +Three integrated pattern controllers provide auto breathing or group dimming
> +control. Each pattern controller can work in auto breathing or manual control
> +mode. All breathing parameters including rising/falling slope, on/off time,
> +repeat times, min/max brightness and so on are configurable.
> +
> +Device attribute
> +-----------------------------------
> +
> +**/sys/class/leds/<led>/dim** - 64-level DIM current. If write negative value
> +or "auto", the dim will be calculated according to the brightness.
> +
> +The configuration files for each pattern are located::
> +
> +    /sys/bus/i2c/devices/xxxx/pattern0/
> +    /sys/bus/i2c/devices/xxxx/pattern1/
> +    /sys/bus/i2c/devices/xxxx/pattern2/
> +
> +Directory layout example for pattern
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +::
> +
> +    $ ls -l /sys/bus/i2c/devices/xxxx/pattern0/
> +    -rw-r--r--    1 root     root          4096 Jan  1 00:00 clear_leds
> +    -rw-r--r--    1 root     root          4096 Jan  1 00:00 fall_time
> +    -rw-r--r--    1 root     root          4096 Jan  1 00:00 loop_begin
> +    -rw-r--r--    1 root     root          4096 Jan  1 00:00 loop_end_on
> +    -rw-r--r--    1 root     root          4096 Jan  1 00:00 max_breathing_level
> +    -rw-r--r--    1 root     root          4096 Jan  1 00:00 min_breathing_level
> +    -rw-r--r--    1 root     root          4096 Jan  1 00:00 mode
> +    -rw-r--r--    1 root     root          4096 Jan  1 00:00 off_time
> +    -rw-r--r--    1 root     root          4096 Jan  1 00:00 on_time
> +    -rw-r--r--    1 root     root          4096 Jan  1 00:00 ramp
> +    -rw-r--r--    1 root     root          4096 Jan  1 00:00 repeat
> +    -rw-r--r--    1 root     root          4096 Jan  1 00:00 rise_time
> +    -r--r--r--    1 root     root          4096 Jan  1 00:00 running
> +    -rw-r--r--    1 root     root          4096 Jan  1 00:00 select_leds
> +    -rw-r--r--    1 root     root          4096 Jan  1 00:00 start
> +    -rw-r--r--    1 root     root          4096 Jan  1 00:00 toggle

sysfs documentation goes to Documentation/ABI/


(...)

> +static int aw200xx_probe(struct i2c_client *client)
> +{
> +	const struct aw200xx_chipdef *cdef;
> +	struct aw200xx *chip;
> +	int count;
> +	int ret;
> +
> +	cdef = device_get_match_data(&client->dev);
> +
> +	count = device_get_child_node_count(&client->dev);
> +	if (!count || count > cdef->channels)
> +		return dev_err_probe(&client->dev, -EINVAL,
> +				     "Incorrect number of leds (%d)", count);
> +
> +	chip = devm_kzalloc(&client->dev,
> +			    struct_size(chip, leds, count),

sizeof(*chip)

> +			    GFP_KERNEL);
> +	if (!chip)
> +		return -ENOMEM;
> +
> +	chip->cdef = cdef;
> +	chip->num_leds = count;
> +	chip->client = client;
> +	i2c_set_clientdata(client, chip);
> +
> +	chip->regmap = devm_regmap_init_i2c(client, &aw200xx_regmap_config);
> +	if (IS_ERR(chip->regmap))
> +		return PTR_ERR(chip->regmap);
> +
> +	ret = aw200xx_chip_check(chip);
> +	if (ret)
> +		return ret;
> +
> +	mutex_init(&chip->mutex);
> +
> +	/* Need a lock now since after call aw200xx_probe_dt, created sysfs nodes */
> +	mutex_lock(&chip->mutex);
> +
> +	ret = aw200xx_probe_dt(&client->dev, chip);
> +	if (ret < 0)
> +		goto exit;
> +
> +	ret = aw200xx_chip_reset(chip);
> +	if (ret)
> +		goto exit;
> +
> +	ret = aw200xx_chip_init(chip);
> +	if (ret)
> +		goto exit;
> +
> +	ret = aw200xx_setup_interrupts(chip);
> +
> +exit:
> +	mutex_unlock(&chip->mutex);
> +	return ret;
> +}
> +
> +static void aw200xx_remove(struct i2c_client *client)
> +{
> +	struct aw200xx *chip = i2c_get_clientdata(client);
> +
> +	aw200xx_chip_reset(chip);
> +	mutex_destroy(&chip->mutex);
> +}
> +
> +static const struct aw200xx_chipdef aw20036_cdef = {
> +	.channels = 36,
> +	.display_size_max = 2,
> +	.display_size_columns = 12,
> +};
> +
> +static const struct aw200xx_chipdef aw20054_cdef = {
> +	.channels = 54,
> +	.display_size_max = 5,
> +	.display_size_columns = 9,
> +};
> +
> +static const struct aw200xx_chipdef aw20072_cdef = {
> +	.channels = 72,
> +	.display_size_max = 5,
> +	.display_size_columns = 12,
> +};
> +
> +static const struct i2c_device_id aw200xx_id[] = {
> +	{ "aw20036" },
> +	{ "aw20054" },
> +	{ "aw20072" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, aw200xx_id);
> +
> +static const struct of_device_id aw200xx_match_table[] = {
> +	{ .compatible = "awinic,aw20036", .data = &aw20036_cdef, },
> +	{ .compatible = "awinic,aw20054", .data = &aw20054_cdef, },
> +	{ .compatible = "awinic,aw20072", .data = &aw20072_cdef, },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, aw200xx_match_table);
> +
> +static struct i2c_driver aw200xx_driver = {
> +	.driver = {
> +		.name = "aw200xx",
> +		.of_match_table = of_match_ptr(aw200xx_match_table),

You will have warning now. of_match_ptr goes with __maybe_unused. Drop it.

> +		.dev_groups = aw200xx_pattern_groups,
> +	},
> +	.probe_new = aw200xx_probe,
> +	.remove = aw200xx_remove,
> +	.id_table = aw200xx_id,
> +};
> +
> +module_i2c_driver(aw200xx_driver);
> +
> +MODULE_AUTHOR("Martin Kurbanov <mmkurbanov@sberdevices.ru>");
> +MODULE_DESCRIPTION("AW200XX LED driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:leds-aw200xx");

Best regards,
Krzysztof