mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: Sascha Hauer <s.hauer@pengutronix.de>
To: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Cc: barebox@lists.infradead.org
Subject: Re: [PATCH 2/7] basic LED support
Date: Sat, 18 Dec 2010 18:18:05 +0100	[thread overview]
Message-ID: <20101218171805.GJ6017@pengutronix.de> (raw)
In-Reply-To: <20101218163813.GA15695@game.jcrosoft.org>

Hi J,

On Sat, Dec 18, 2010 at 05:38:13PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > +/**
> > + * led_set - set the value of a LED
> > + * @param led	the led
> > + * @param value	the value of the LED (0 is disabled)
> > + */
> > +int led_set(struct led *led, unsigned int value)
> > +{
> > +	if (value > led->max_value)
> > +		value = led->max_value;
> > +
> > +	led->set(led, value);
> > +
> > +	return 0;
> why always return 0?

Should make this void, see below.

> > +}
> > +
> > +/**
> > + * led_set_num - set the value of a LED
> > + * @param num	the number of the LED
> > + * @param value	the value of the LED (0 is disabled)
> > + */
> > +int led_set_num(int num, unsigned int value)
> > +{
> > +	struct led *led = led_by_number(num);
> > +
> > +	if (!led)
> > +		return -ENODEV;
> > +
> > +	return led_set(led, value);
> > +}
> > +
> > +/**
> > + * led_register - Register a LED
> > + * @param led	the led
> > + */
> > +int led_register(struct led *led)
> > +{
> no safe check?

What should we check for? NUll pointers? That's not worth it.

> > +	led->num = num_leds++;
> > +
> > +	list_add_tail(&led->list, &leds);
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * led_unregister - Unegister a LED
> > + * @param led	the led
> > + */
> > +void led_unregister(struct led *led)
> > +{
> > +	list_del(&led->list);
> > +}
> > diff --git a/include/led.h b/include/led.h
> > new file mode 100644
> > index 0000000..62d0d08
> > --- /dev/null
> > +++ b/include/led.h
> > @@ -0,0 +1,25 @@
> > +#ifndef __LED_H
> > +#define __LED_H
> > +
> > +struct led {
> > +	unsigned long triger;
> > +	void (*set)(struct led *, unsigned int value);
> return a int will good to known the result

I intentionally made this void because setting a LED is not really
supposed to fail. And even if does, I don't know if it's worth anything
to tell the user "setting a LED failed", when this may be even in a loop
and the user is bothered with a hundred of these messages.

> > +	int max_value;
> > +	int num;
> > +	struct list_head list;
> > +};
> > +
> > +struct led *led_by_number(int no);
> > +
> > +static inline int led_get_number(struct led *led)
> > +{
> > +	return led->num;
> > +}
> > +
> > +int led_set_num(int num, unsigned int value);
> > +int led_set(struct led *led, unsigned int value);
> > +int led_register(struct led *led);
> > +void led_unregister(struct led *led);
> > +void led_unregister(struct led *led);
> twice
> 
> Best Regards,
> J.
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

  reply	other threads:[~2010-12-18 17:18 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-12-18 15:15 LED framework Sascha Hauer
2010-12-18 15:15 ` [PATCH 1/7] Add generic poll infrastructure Sascha Hauer
2010-12-18 15:28   ` Sascha Hauer
2010-12-18 15:15 ` [PATCH 2/7] basic LED support Sascha Hauer
2010-12-18 16:38   ` Jean-Christophe PLAGNIOL-VILLARD
2010-12-18 17:18     ` Sascha Hauer [this message]
2010-12-18 16:48   ` Jean-Christophe PLAGNIOL-VILLARD
2010-12-18 19:06   ` Belisko Marek
2010-12-19 21:31   ` Marc Reilly
2010-12-20  8:27     ` Sascha Hauer
2010-12-18 15:15 ` [PATCH 3/7] LED: Add gpio " Sascha Hauer
2010-12-18 16:41   ` Jean-Christophe PLAGNIOL-VILLARD
2010-12-18 17:18     ` Sascha Hauer
2010-12-18 15:15 ` [PATCH 4/7] LED: Add LED trigger support Sascha Hauer
2010-12-18 16:51   ` Belisko Marek
2010-12-18 17:21     ` Sascha Hauer
2010-12-18 15:15 ` [PATCH 5/7] LED: Add led command Sascha Hauer
2010-12-18 16:45   ` Jean-Christophe PLAGNIOL-VILLARD
2010-12-18 17:24     ` Sascha Hauer
2010-12-18 15:15 ` [PATCH 6/7] LED: Add trigger command Sascha Hauer
2010-12-18 15:15 ` [PATCH 7/7] pcm038: led testing. Not to be committed Sascha Hauer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20101218171805.GJ6017@pengutronix.de \
    --to=s.hauer@pengutronix.de \
    --cc=barebox@lists.infradead.org \
    --cc=plagnioj@jcrosoft.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox