On Wed, 15 Jul 2020 08:27:51 -0500
Dan Murphy <dmurphy@xxxxxx> wrote:
> Can this be built as a module?
Yes. But only if MC framework is compiled in. If MC framework is
compiled as module as well, this results in
FATAL: modpost: GPL-incompatible module led-class-multicolor.ko uses
GPL-only symbol 'led_set_brightness'
> > +struct omnia_led {
> > + struct led_classdev_mc mc_cdev;
> > + struct mc_subled subled_info[3];
> nit. #define the 3 and use it here and when indicating the num_colors
Meh, ok
> > +#define to_omnia_led(l) container_of(l, struct
> > omnia_led, mc_cdev)
> Spacing look to be off here.
thx
> > +
> > +struct omnia_leds {
> > + struct i2c_client *client;
> > + struct mutex lock;
> > + int nleds;
> This does not seem to be used anywhere except for during registering
Hmmm, I'll try to rewrite this
> > + struct omnia_led leds[0];
> Remove the 0 as this should be a flexible array
Thx
> > + led_mc_calc_color_components(&led->mc_cdev, brightness);
>
> Do you need this in the lock as well?
You are right
> > + led->subled_info[0].color_index = LED_COLOR_ID_RED;
> > + led->subled_info[0].channel = 0;
> > + led->subled_info[1].color_index = LED_COLOR_ID_GREEN;
> > + led->subled_info[1].channel = 1;
> > + led->subled_info[2].color_index = LED_COLOR_ID_BLUE;
> > + led->subled_info[2].channel = 2;
>
> OK this is why you may want to have sub-nodes. Where reg is the
> channel and color is the color. Then you can do a for_each_child.
Rob says that it doesn't need to be in DT if the controller only
supports RGB LEDs. This controller is only in Turris Omnia which was
never made with another kind of LEDs. So I think it is pointless and
would only grow the DT and complicate the driver.
> > + cdev->max_brightness = 255;
> This is not needed. It is defaulted to LED_FULL in led_class
This was discussed last year and resulted in LED_FULL being
declared obsolete in the header file.
> > +/*
> > + * On the front panel of the Turris Omnia router there is also a
> > button which can be used to control
> > + * the intensity of all the LEDs at once, so that if they are too
> > bright, user can dim them.
> > + * The microcontroller cycles between 8 levels of this global
> > brightness (from 100% to 0%), but this
> > + * setting can have any integer value between 0 and 100.
> > + * It is usable to be able to change this value from software, so
> > that it does not start at 100%
> This does not make sense.
It does. The user changes the brightness of all 12 LEDs with the button
to his liking and wants to have the same setting after powering
the router on again.
> > + * after every power on and annoy the user.
> > + * We expose this setting via a sysfs attribute file called
> > "brightness". This file lives in the
> > + * device directory of the LED controller, not an individual LED,
> > so it should not confuse users.
> > + */
>
> Sorry if this has been discussed already
>
> This seems a bit wonky. You are overriding the brightness set by the
> LED class.
I am not. Pressing the button does not change the brightness read from
the /sys/class/leds/<LED>/brightness file. This is different brightness,
it is above the classic brightnes in the PWM hierarchy in the
microcontroller. I discussed this with Pavel and he said we can call
this file brightness as well (since it is brightness of the whole
panel), and the file does not reside in /sys/class/leds/<LED> directory.
> Does this button have an interrupt to the processor or does it go to
> the micro controller?
>
> Where is the current power on value stored? If this is stored in the
> micro maybe reading that value at power up and setting the brightness
> would be better.
>
> > +static ssize_t brightness_show(struct device *dev, struct
> > device_attribute *a, char *buf) +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > + struct omnia_leds *leds = i2c_get_clientdata(client);
> > + int ret;
> > +
> > + mutex_lock(&leds->lock);
> > + ret = i2c_smbus_read_byte_data(client,
> > CMD_LED_GET_BRIGHTNESS);
> > + mutex_unlock(&leds->lock);
> > +
> > + if (ret < 0)
> > + return ret;
> > +
> > + return sprintf(buf, "%d\n", ret);
> > +}
> > +
> > +static ssize_t brightness_store(struct device *dev, struct
> > device_attribute *a, const char *buf,led = &leds->leds[leds->nleds];
> > + size_t count)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > + struct omnia_leds *leds = i2c_get_clientdata(client);
> > + unsigned int brightness;
> > + int ret;
> > +
> > + if (sscanf(buf, "%u", &brightness) != 1)
> > + return -EINVAL;
> > +
> > + if (brightness > 100)
> > + return -EINVAL;
> > +
> > + mutex_lock(&leds->lock);
> > + ret = i2c_smbus_write_byte_data(client,
> > CMD_LED_SET_BRIGHTNESS, (u8) brightness);
> > + mutex_unlock(&leds->lock);
> > +
> > + if (ret < 0)
> > + return ret;
> > +
> > + return count;
> > +}
> > +static DEVICE_ATTR_RW(brightness);
> > +
> > +static struct attribute *omnia_led_controller_attrs[] = {
> > + &dev_attr_brightness.attr,
> > + NULL,
> > +};
> > +ATTRIBUTE_GROUPS(omnia_led_controller);
> > +
> > +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 > 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);
> Use the macro struct_size(led, leds, count)
thx
