On Wed, Mar 1, 2023 at 8:37 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> Hi,
>
> On 2/24/23 16:14, Dan Scally wrote:
> > On 22/02/2023 16:53, Hans de Goede wrote:
>
> <snip>
>
> >>> +
> >>> +static enum led_brightness tps68470_brightness_get(struct led_classdev *led_cdev)
> >>> +{
> >>> + struct tps68470_led_data *data = container_of(led_cdev,
> >>> + struct tps68470_led_data,
> >>> + ledb_cdev);
> >>
> >> This container_of only works for led_b not for led_a.
> >>
> >>> +
> >>> + if (!strncmp(led_cdev->name, "tps68470-ileda", 14))
> >>> + return data->brightness_a;
> >>> + else if (!strncmp(led_cdev->name, "tps68470-iledb", 14))
> >>> + return data->brightness_b;
> >>> +
> >>> + return -EINVAL;
> >>> +}
> >>
> >> Instead of this strcmp magic, please just use 2 separate
> >> brightness_get functions (thus also solving the container_of
> >> problem above). And please also do the same for brightness_set.
> >
> > I don't mind the single function so much but I don't particularly like the strcmp. I'm actually working on this at the moment too trying (but so far mostly failing) to get the WLED that drives the Surface Go's IR LED working properly (I can drive it...for a maximum of 13 seconds); I had modeled the problem as an array of structs for the LEDs and reference them with IDs:
> >
> > #define lcdev_to_led(lcdev) \
> > container_of(lcdev, struct tps68470_led, lcdev);
> >
> > #define led_to_tps68470(led, index) \
> > container_of(led, struct tps68470_device, leds[index])
> >
> > enum tps68470_led_ids {
> > TPS68470_ILED_A,
> > TPS68470_ILED_B,
> > TPS68470_WLED,
> > TPS68470_NUM_LEDS
> > };
> >
> > static const char *tps68470_led_names[] = {
> > [TPS68470_ILED_A] = "tps68470-iled_a",
> > [TPS68470_ILED_B] = "tps68470-iled_b",
> > [TPS68470_WLED] = "tps68470-wled",
> > };
> >
> > struct tps68470_led {
> > unsigned int led_id;
> > struct led_classdev lcdev;
> > };
> >
> > struct tps68470_device {
> > struct device *dev;
> > struct regmap *regmap;
> > struct tps68470_led leds[TPS68470_NUM_LEDS];
> > };
> >
> > int tps68470_led_brightness_set(...)
> > {
> > struct tps68470_led *led = lcdev_to_led(lcdev);
> > struct tps68470_device *tps68470 = led_to_tps68470(led, led->index);
>
> I assume led->index should be led->led_id here ?
>
> >
> > switch (led->led_id) {
> > case TPS68470_ILED_A:
> > return regmap_update_bits(...);
> > case TPS68470_ILED_B:
> > ...
> >
> > }
>
> But since the indices into the register are not simple a function of
> led->led_id, you still need this switch-case here and then a separate
> implementation for each LED.
>
> At which point IMHO just having a single set / get function per LED
> is much simpler then adding the complications with the struct wrapping
> struct led_class_dev to add an index to it.
>
> Anyways there is an easy solution here: Kate you get to choose between
> 1 set + get function per LED or Dan's solution, but please drop
> the strcmp() calls since neither Dan nor I like those.
Thank you for the comments.
Okay, let me think between both solutions and I'll remove strcmp() for sure.
>
> <snip>
>
> > Regardless of how it ends up being done; I think you need the LED_FUNCTION_INDICATOR part in lcdev->name to match the "devicename:color:function" that the LED subsystem seems to want.
>
> Agreed, Kate please switch to this, e.g.:
>
> tps68470_led->leda_cdev.name = "tps68470::" LED_FUNCTION_INDICATOR;
>
> LED names should always be in the format of "devicename:color:function"
> I missed you were not using that before. And since we don't know the
> color we just leave it empty (this is allowed).
OK. I have checked the document for the naming format. I'll correct this.
>
> Note LED_FUNCTION_INDICATOR is defined in include/dt-bindings/leds/common.h .
Thank you for noting this.
>
> Regards,
>
> Hans
>
>
>
>
--
BR,
Kate
