Date: Tue, 9 Jun 2015 19:22:31 +0200
From: Samuel Thibault <samuel.thibault@ens-lyon.org>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Pavel Machek <pavel@ucw.cz>,
        Pali =?iso-8859-1?Q?Roh=E1r?= <pali.rohar@gmail.com>,
        linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
        rpurdie@rpsys.net, Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: Re: [PATCH 1/3] Input: export LEDs as class devices in sysfs
Message-ID: <20150609172231.GB4336@type.youpi.perso.aquilenet.fr>
Mail-Followup-To: Samuel Thibault <samuel.thibault@ens-lyon.org>,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	Pavel Machek <pavel@ucw.cz>,
	Pali =?iso-8859-1?Q?Roh=E1r?= <pali.rohar@gmail.com>,
	linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
	rpurdie@rpsys.net, Greg Kroah-Hartman <gregkh@linuxfoundation.org>
References: <1433799790-31873-1-git-send-email-dmitry.torokhov@gmail.com>
 <1433799790-31873-2-git-send-email-dmitry.torokhov@gmail.com>
 <20150609131955.GA3045@type>
 <20150609164935.GD6338@dtor-ws>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20150609164935.GD6338@dtor-ws>
User-Agent: Mutt/1.5.21+34 (58baf7c9f32f) (2010-12-30)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2444
Lines: 53

Dmitry Torokhov, le Tue 09 Jun 2015 09:49:35 -0700, a ?crit :
> > > +	[LED_NUML]	= { "num-lock", VT_TRIGGER("kbd-numlock") },
> > > +	[LED_CAPSL]	= { "caps-lock", VT_TRIGGER("kbd-capslock") },
> > > +	[LED_SCROLLL]	= { "scroll-lock", VT_TRIGGER("kbd-scrollock") },
> > 
> > I'd tend to think we'd want to harmonize the user-visible LED names and
> > kbd trigger names, i.e. use "numlock", "capslock" and "scrollock" in
> > both case (or something else, but the same for LED and trigger). In my
> > patch I simply used the corresponding LED or kbd macro names, but there
> > is probably no strong reason to this, while there is probably a good
> > reason to choose coherent and nice user-visible names.
> 
> I can do either "num_lock - kbd-num-lock" or "numlock - kbd-numlock"
> with slight preference to the 1st. What is your preference?

I'd prefer numlock - kbd-numlock.

> > > +static enum led_brightness input_leds_brightness_get(struct led_classdev *cdev)
> > > +{
> > > +	struct input_led *led = container_of(cdev, struct input_led, cdev);
> > > +	struct input_dev *input = led->handle->dev;
> > > +
> > > +	return test_bit(led->code, input->ledbit) ? LED_FULL : LED_OFF;
> > 
> > This always returns LED_FULL, whatever the current state of the LED, is
> > that really what we want?  Userspace will be surprised to read 255 from
> > sysfs whatever it writes to it (with actual proper effect on the LED).
> 
> Right, I will change it to 0 and led->max_brightness (which I will set
> to 1).

Well, that won't fix the issue I'm having, see below.

> > Simply not defining input_leds_brightness_get and letting the LED core
> > manage the value does get a proper "brightness" sysfs file behavior, is
> > there a reason not to do that?
> 
> Yes, we want LED sysfs show correct result if state is altered via
> EV_LED/LED_* event. Basically the led bit state is the source of truth
> here.

Ok, I understand that. But it happens that your code does not work! It
is always 255 (or will be 1 with the modifications you mention above).
input->ledbit is whether the LED exists or not, not its state, right?
Did your perhaps mean input->led in input_leds_brightness_get instead of
input->ledbit?

Samuel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/