Hello Dan,
thank you for your review!
07.06.19 17:59, Dan Murphy пише:
> Oleh
>
> On 6/7/19 6:48 AM, Oleh Kravchenko wrote:
>> +#include <linux/delay.h>
>> +#include <linux/leds.h>
>> +#include <linux/limits.h>
> I do not see any #defines used from this file
For U8_MAX, please see below.
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/workqueue.h>
>
> It does not look like you are using any work queues.
My bad, thank you!
>> + * LED ID, known values is 0x50 (Pipe), 0x53 (Screen Frame) and
>> + * 0x56 (Vending Area);
>> + * BRIGHTNESS Can be 0x30 (OFF), 0x31 (ON).
>> + * 0x32 (Effect) can be used for 0x50 (leaking) and
>> + * for 0x53 (blinking).
>> + *
>
> These can be #defined which makes them desctiptive
>
> #define EL15203000_LED_OFF 0x30
>
> #define EL15203000_LED_ON 0x31
>
> and so on
Hm, but just adding 0x30 not will be more clear and faster?
I think switch .. case or if .. else, will be more hard to read :)
>> + if (reg > U8_MAX) {
>> + dev_err(priv->dev, "LED value %d is invalid", reg);
>> + fwnode_handle_put(child);
>> +
>> + return -ENODEV;
> -EINVAL
Done
>> + }
>> + led->reg = reg;
>> +
>> + ret = fwnode_property_read_string(child, "label", &str);
>> + if (ret)
>> + snprintf(led->name, sizeof(led->name),
>> + "el15203000::");
>> + else
>> + snprintf(led->name, sizeof(led->name),
>> + "el15203000:%s", str);
>> +
>> + fwnode_property_read_string(child, "linux,default-trigger",
>> + &led->ldev.default_trigger);
>> +
>> + led->priv = priv;
>> + led->ldev.name = led->name;
>> + led->ldev.max_brightness = LED_ON;
> Do not need this as it should be stored when doing the fwnode read.
This is default value 1, by dtb we can override it to 2.
>> + led->ldev.brightness_set_blocking = el15203000_set_sync;
>> +
>> + ret = fwnode_property_read_u32(child, "max-brightness",
>> + &led->ldev.max_brightness);
>> + if (led->ldev.max_brightness > EL_MAX_BRIGHTNESS) {
>> + dev_err(priv->dev, "invalid max brightness %d",
>> + led->ldev.max_brightness);
>> + fwnode_handle_put(child);
>> +
>> + return -ENODEV;
>
> -EINVAL
Done
>> + ret = devm_of_led_classdev_register(priv->dev, np,
>> + &led->ldev);
>
> please use devm_led_classdev_register then there is no need to set np or store it.
>
> Dan
>
--
Best regards,
Oleh Kravchenko
Attachment:
signature.asc
Description: OpenPGP digital signature
