Hi Marek,
Thank you for the patch.
On 3/19/20 7:16 PM, Marek Behún wrote:
> This adds basic support for LEDs on the front side of CZ.NIC's Turris
> Omnia router.
>
> There are 12 RGB LEDs. The controller supports HW triggering mode for
> the LEDs, but this driver does not support it yet, and sets all the LEDs
> into SW mode upon probe.
>
> The user can either group all three channels of one RGB LED into one LED
> classdev, or expose each channel as an individual LED classdev. This is
> done by utilizing the 'led-sources' and 'color' DT properties.
>
> In the following example the first RGB LED is exposed as one LED
> classdev with color WHITE, and the second RGB LED is exposed as three
> classdevs, one per each channel:
> led@0 {
> reg = <0>;
> led-sources = <0 1 2>;
> color = <LED_COLOR_ID_WHITE>;
> };
>
> led@1,0 {
> reg = <1>;
> led-sources = <3>;
> color = <LED_COLOR_ID_RED>;
> };
>
> led@1,1 {
> reg = <1>;
> led-sources = <4>;
> color = <LED_COLOR_ID_GREEN>;
> };
>
> led@1,2 {
> reg = <1>;
> led-sources = <5>;
> color = <LED_COLOR_ID_BLUE>;
> };
>
> I am not comfortable with the 'reg' property being same for multiple
> nodes. Perhaps the 'reg' property shouldn't be used, since the
> information needed by the driver can be deduced from the 'led-sources'.
I agree. You can name the sub-nodes like led0,led1,led2 etc.
reg is convenient if each sub-node refers to single iout, but
in this case it is unnecessary complication. You can infer the
reg in dt parser basing on led-sources.
And we need these bindings in a separate patch adding a new file
in Documentation/devicetree/bindings/leds.
You should also mention what are the allowed led-sources
configurations, i.e. I presume that only groups of (0,1,2),
(2,3,4) etc. are allowed, or a single iout per child node.
[...]
> +static int omnia_leds_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct device *dev = &client->dev;
> + struct device_node *np = dev->of_node, *child;
> + struct omnia_leds *leds;
> + int ret, count;
> +
> + count = of_get_available_child_count(np);
> + if (!count) {
> + dev_err(dev, "LEDs are not defined in device tree!\n");
> + return -ENODEV;
> + } else if (count > 3 * OMNIA_BOARD_LEDS) {
> + dev_err(dev, "Too many LEDs defined in device tree!\n");
> + return -EINVAL;
> + }
> +
> + leds = devm_kzalloc(dev, sizeof(*leds) + count * sizeof(leds->leds[0]),
> + GFP_KERNEL);
> + if (!leds)
> + return -ENOMEM;
> +
> + leds->client = client;
> + i2c_set_clientdata(client, leds);
> +
> + mutex_init(&leds->lock);
> +
> + for_each_available_child_of_node(np, child) {
> + ret = omnia_led_register(leds, &child->fwnode);
> + if (ret < 0)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int omnia_leds_remove(struct i2c_client *client)
> +{
> + u8 buf[OMNIA_CMD_LED_COLOR_LEN];
> +
> + /* put all LEDs into default (HW triggered) mode */
> + i2c_smbus_write_byte_data(client, CMD_LED_MODE,
> + CMD_LED_MODE_LED(OMNIA_BOARD_LEDS));
> +
> + /* set all LEDs color to [255, 255, 255] */
> + buf[OMNIA_CMD] = CMD_LED_COLOR;
> + buf[OMNIA_CMD_LED_COLOR_LED] = OMNIA_BOARD_LEDS;
> + buf[OMNIA_CMD_LED_COLOR_R] = 255;
> + buf[OMNIA_CMD_LED_COLOR_G] = 255;
> + buf[OMNIA_CMD_LED_COLOR_B] = 255;
What is the rationale behind setting all LEDs to max_brighntess
on driver removal?
--
Best regards,
Jacek Anaszewski
