Re: [PATCH RFC leds-next] leds: initial support for Turris Omnia LEDs

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

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