[v1] ROCK 4D audio enablement

[PATCH 2/4] Input: adc-keys - support types that aren't just keyboard keys

Posted by Nicolas Frattaroli 7 months, 1 week ago

Instead of doing something like what gpio-keys is doing, adc-keys
hardcodes that all keycodes must be of type EV_KEY.

This limits the usefulness of adc-keys, and overcomplicates the code
with manual bit-setting logic.

Instead, refactor the code to read the linux,input-type fwnode property,
and get rid of the custom bit setting logic, replacing it with
input_set_capability instead. input_report_key is replaced with
input_event, which allows us to explicitly pass the type.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/input/keyboard/adc-keys.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/input/keyboard/adc-keys.c b/drivers/input/keyboard/adc-keys.c
index f1753207429db02ce6510e5ec0da9b24d9edb61d..339dd4d4a0842108da2c6136b1e0098cd1f6a3cd 100644
--- a/drivers/input/keyboard/adc-keys.c
+++ b/drivers/input/keyboard/adc-keys.c
@@ -19,12 +19,14 @@
 struct adc_keys_button {
 	u32 voltage;
 	u32 keycode;
+	u32 type;
 };
 
 struct adc_keys_state {
 	struct iio_channel *channel;
 	u32 num_keys;
 	u32 last_key;
+	u32 last_type;
 	u32 keyup_voltage;
 	const struct adc_keys_button *map;
 };
@@ -35,6 +37,7 @@ static void adc_keys_poll(struct input_dev *input)
 	int i, value, ret;
 	u32 diff, closest = 0xffffffff;
 	int keycode = 0;
+	u32 type = EV_KEY;
 
 	ret = iio_read_channel_processed(st->channel, &value);
 	if (unlikely(ret < 0)) {
@@ -46,6 +49,7 @@ static void adc_keys_poll(struct input_dev *input)
 			if (diff < closest) {
 				closest = diff;
 				keycode = st->map[i].keycode;
+				type = st->map[i].type;
 			}
 		}
 	}
@@ -54,13 +58,14 @@ static void adc_keys_poll(struct input_dev *input)
 		keycode = 0;
 
 	if (st->last_key && st->last_key != keycode)
-		input_report_key(input, st->last_key, 0);
+		input_event(input, st->last_type, st->last_key, 0);
 
 	if (keycode)
-		input_report_key(input, keycode, 1);
+		input_event(input, type, keycode, 1);
 
 	input_sync(input);
 	st->last_key = keycode;
+	st->last_type = type;
 }
 
 static int adc_keys_load_keymap(struct device *dev, struct adc_keys_state *st)
@@ -93,6 +98,10 @@ static int adc_keys_load_keymap(struct device *dev, struct adc_keys_state *st)
 			return -EINVAL;
 		}
 
+		if (fwnode_property_read_u32(child, "linux,input-type",
+					     &map[i].type))
+			map[i].type = EV_KEY;
+
 		i++;
 	}
 
@@ -156,9 +165,8 @@ static int adc_keys_probe(struct platform_device *pdev)
 	input->id.product = 0x0001;
 	input->id.version = 0x0100;
 
-	__set_bit(EV_KEY, input->evbit);
 	for (i = 0; i < st->num_keys; i++)
-		__set_bit(st->map[i].keycode, input->keybit);
+		input_set_capability(input, st->map[i].type, st->map[i].keycode);
 
 	if (device_property_read_bool(dev, "autorepeat"))
 		__set_bit(EV_REP, input->evbit);

-- 
2.50.0

[PATCH 2/4] Input: adc-keys - support types that aren't just keyboard keys

Posted by Dragan Simic 3 months, 1 week ago

Hello Nicolas,

On Monday, June 30, 2025, 12:19:24 Nicolas Frattaroli wrote:
> Instead of doing something like what gpio-keys is doing, adc-keys
> hardcodes that all keycodes must be of type EV_KEY.
> 
> This limits the usefulness of adc-keys, and overcomplicates the code
> with manual bit-setting logic.
> 
> Instead, refactor the code to read the linux,input-type fwnode property,
> and get rid of the custom bit setting logic, replacing it with
> input_set_capability instead. input_report_key is replaced with
> input_event, which allows us to explicitly pass the type.

Thanks for this patch, it's indeed very useful!  Please see some
comments below.

> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>  drivers/input/keyboard/adc-keys.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/input/keyboard/adc-keys.c b/drivers/input/keyboard/adc-keys.c
> index f1753207429db02ce6510e5ec0da9b24d9edb61d..339dd4d4a0842108da2c6136b1e0098cd1f6a3cd 100644
> --- a/drivers/input/keyboard/adc-keys.c
> +++ b/drivers/input/keyboard/adc-keys.c
> @@ -19,12 +19,14 @@
>  struct adc_keys_button {
>  	u32 voltage;
>  	u32 keycode;
> +	u32 type;
>  };
>  
>  struct adc_keys_state {
>  	struct iio_channel *channel;
>  	u32 num_keys;
>  	u32 last_key;
> +	u32 last_type;
>  	u32 keyup_voltage;
>  	const struct adc_keys_button *map;
>  };
> @@ -35,6 +37,7 @@ static void adc_keys_poll(struct input_dev *input)
>  	int i, value, ret;
>  	u32 diff, closest = 0xffffffff;
>  	int keycode = 0;
> +	u32 type = EV_KEY;
>  
>  	ret = iio_read_channel_processed(st->channel, &value);
>  	if (unlikely(ret < 0)) {
> @@ -46,6 +49,7 @@ static void adc_keys_poll(struct input_dev *input)
>  			if (diff < closest) {
>  				closest = diff;
>  				keycode = st->map[i].keycode;
> +				type = st->map[i].type;
>  			}
>  		}
>  	}
> @@ -54,13 +58,14 @@ static void adc_keys_poll(struct input_dev *input)
>  		keycode = 0;
>  
>  	if (st->last_key && st->last_key != keycode)
> -		input_report_key(input, st->last_key, 0);
> +		input_event(input, st->last_type, st->last_key, 0);
>  
>  	if (keycode)
> -		input_report_key(input, keycode, 1);
> +		input_event(input, type, keycode, 1);

When EV_ABS is defined in the DT as the key type, which happens with
joysticks and whatnot, separate handling is needed, by requiring the
actual associated button values to be reported in the input_event()
invocations, more specifically on the keypresses only.

That's also visible in the gpio_keys_gpio_report_event() function in
drivers/input/keyboard/gpio_keys.c.

>  	input_sync(input);
>  	st->last_key = keycode;
> +	st->last_type = type;
>  }
>  
>  static int adc_keys_load_keymap(struct device *dev, struct adc_keys_state *st)
> @@ -93,6 +98,10 @@ static int adc_keys_load_keymap(struct device *dev, struct adc_keys_state *st)
>  			return -EINVAL;
>  		}
>  
> +		if (fwnode_property_read_u32(child, "linux,input-type",
> +					     &map[i].type))
> +			map[i].type = EV_KEY;

Going along with the remarks above, it will also be needed to read
and record the values of "linux,input-value" DT properties here, and
to extend the associated binding to define their presence.

>  		i++;
>  	}
>  
> @@ -156,9 +165,8 @@ static int adc_keys_probe(struct platform_device *pdev)
>  	input->id.product = 0x0001;
>  	input->id.version = 0x0100;
>  
> -	__set_bit(EV_KEY, input->evbit);
>  	for (i = 0; i < st->num_keys; i++)
> -		__set_bit(st->map[i].keycode, input->keybit);
> +		input_set_capability(input, st->map[i].type, st->map[i].keycode);
>  
>  	if (device_property_read_bool(dev, "autorepeat"))
>  		__set_bit(EV_REP, input->evbit);

Re: [PATCH 2/4] Input: adc-keys - support types that aren't just keyboard keys

Posted by Nicolas Frattaroli 1 month, 3 weeks ago

On Tuesday, 28 October 2025 22:32:09 Central European Standard Time Dragan Simic wrote:
> Hello Nicolas,
> 
> On Monday, June 30, 2025, 12:19:24 Nicolas Frattaroli wrote:
> > Instead of doing something like what gpio-keys is doing, adc-keys
> > hardcodes that all keycodes must be of type EV_KEY.
> > 
> > This limits the usefulness of adc-keys, and overcomplicates the code
> > with manual bit-setting logic.
> > 
> > Instead, refactor the code to read the linux,input-type fwnode property,
> > and get rid of the custom bit setting logic, replacing it with
> > input_set_capability instead. input_report_key is replaced with
> > input_event, which allows us to explicitly pass the type.
> 
> Thanks for this patch, it's indeed very useful!  Please see some
> comments below.
> 
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > ---
> >  drivers/input/keyboard/adc-keys.c | 16 ++++++++++++----
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/input/keyboard/adc-keys.c b/drivers/input/keyboard/adc-keys.c
> > index f1753207429db02ce6510e5ec0da9b24d9edb61d..339dd4d4a0842108da2c6136b1e0098cd1f6a3cd 100644
> > --- a/drivers/input/keyboard/adc-keys.c
> > +++ b/drivers/input/keyboard/adc-keys.c
> > @@ -19,12 +19,14 @@
> >  struct adc_keys_button {
> >  	u32 voltage;
> >  	u32 keycode;
> > +	u32 type;
> >  };
> >  
> >  struct adc_keys_state {
> >  	struct iio_channel *channel;
> >  	u32 num_keys;
> >  	u32 last_key;
> > +	u32 last_type;
> >  	u32 keyup_voltage;
> >  	const struct adc_keys_button *map;
> >  };
> > @@ -35,6 +37,7 @@ static void adc_keys_poll(struct input_dev *input)
> >  	int i, value, ret;
> >  	u32 diff, closest = 0xffffffff;
> >  	int keycode = 0;
> > +	u32 type = EV_KEY;
> >  
> >  	ret = iio_read_channel_processed(st->channel, &value);
> >  	if (unlikely(ret < 0)) {
> > @@ -46,6 +49,7 @@ static void adc_keys_poll(struct input_dev *input)
> >  			if (diff < closest) {
> >  				closest = diff;
> >  				keycode = st->map[i].keycode;
> > +				type = st->map[i].type;
> >  			}
> >  		}
> >  	}
> > @@ -54,13 +58,14 @@ static void adc_keys_poll(struct input_dev *input)
> >  		keycode = 0;
> >  
> >  	if (st->last_key && st->last_key != keycode)
> > -		input_report_key(input, st->last_key, 0);
> > +		input_event(input, st->last_type, st->last_key, 0);
> >  
> >  	if (keycode)
> > -		input_report_key(input, keycode, 1);
> > +		input_event(input, type, keycode, 1);
> 
> When EV_ABS is defined in the DT as the key type, which happens with
> joysticks and whatnot, separate handling is needed, by requiring the
> actual associated button values to be reported in the input_event()
> invocations, more specifically on the keypresses only.

Analogue control devices like joysticks are handled in adc-joystick.c.
adc-keys is specifically for resistor-ladder key devices. A driver or
binding for some absolute/relative positioning device that is not a
joystick should get its own binding first. Overloading the meaning
of the adc-keys binding and then adding functionality to the driver
is not the way to go.

I'll instead add code to only allow EV_KEY and EV_SW. Switches
function like keys for all intents and purposes, so they fit here.

> 
> That's also visible in the gpio_keys_gpio_report_event() function in
> drivers/input/keyboard/gpio_keys.c.
> 
> >  	input_sync(input);
> >  	st->last_key = keycode;
> > +	st->last_type = type;
> >  }
> >  
> >  static int adc_keys_load_keymap(struct device *dev, struct adc_keys_state *st)
> > @@ -93,6 +98,10 @@ static int adc_keys_load_keymap(struct device *dev, struct adc_keys_state *st)
> >  			return -EINVAL;
> >  		}
> >  
> > +		if (fwnode_property_read_u32(child, "linux,input-type",
> > +					     &map[i].type))
> > +			map[i].type = EV_KEY;
> 
> Going along with the remarks above, it will also be needed to read
> and record the values of "linux,input-value" DT properties here, and
> to extend the associated binding to define their presence.
> 
> >  		i++;
> >  	}
> >  
> > @@ -156,9 +165,8 @@ static int adc_keys_probe(struct platform_device *pdev)
> >  	input->id.product = 0x0001;
> >  	input->id.version = 0x0100;
> >  
> > -	__set_bit(EV_KEY, input->evbit);
> >  	for (i = 0; i < st->num_keys; i++)
> > -		__set_bit(st->map[i].keycode, input->keybit);
> > +		input_set_capability(input, st->map[i].type, st->map[i].keycode);
> >  
> >  	if (device_property_read_bool(dev, "autorepeat"))
> >  		__set_bit(EV_REP, input->evbit);
>

Re: [PATCH 2/4] Input: adc-keys - support types that aren't just keyboard keys

Posted by Heiko Stübner 7 months, 1 week ago

Am Montag, 30. Juni 2025, 12:19:25 Mitteleuropäische Sommerzeit schrieb Nicolas Frattaroli:
> Instead of doing something like what gpio-keys is doing, adc-keys
> hardcodes that all keycodes must be of type EV_KEY.
> 
> This limits the usefulness of adc-keys, and overcomplicates the code
> with manual bit-setting logic.
> 
> Instead, refactor the code to read the linux,input-type fwnode property,
> and get rid of the custom bit setting logic, replacing it with
> input_set_capability instead. input_report_key is replaced with
> input_event, which allows us to explicitly pass the type.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>  drivers/input/keyboard/adc-keys.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/input/keyboard/adc-keys.c b/drivers/input/keyboard/adc-keys.c
> index f1753207429db02ce6510e5ec0da9b24d9edb61d..339dd4d4a0842108da2c6136b1e0098cd1f6a3cd 100644
> --- a/drivers/input/keyboard/adc-keys.c
> +++ b/drivers/input/keyboard/adc-keys.c
> @@ -19,12 +19,14 @@
>  struct adc_keys_button {
>  	u32 voltage;
>  	u32 keycode;

nit: consistency ... the above is still "keycode"
Naming things "code" like in gpio-keys might make sense maybe?
Though I guess, it could also just be needless churn

> +	u32 type;
>  };
>  
>  struct adc_keys_state {
>  	struct iio_channel *channel;
>  	u32 num_keys;
>  	u32 last_key;

^^ same

I'v checked that the function transistions 
- __set_bit -> input_set_capability
- input_report_key -> input_event

do the right thing,

Reviewed-by: Heiko Stuebner <heiko@sntech.de>

[PATCH 1/4] dt-bindings: input: adc-keys: allow linux,input-type property
[PATCH 2/4] Input: adc-keys - support types that aren't just keyboard keys
[PATCH 3/4] arm64: dts: rockchip: add analog audio to ROCK 4D
[PATCH 4/4] arm64: dts: rockchip: add HDMI audio on ROCK 4D