Re: [PATCH 1/1] input: misc: Add as1895 hall sensor driver

[Hans de Goede <hdegoede@redhat.com>]

Hi Wentong,

On 11-Apr-25 19:23, Wentong Wu wrote:
> The as1895 is designed for omnipolar detection hall-effect
> application, such as cover switch, contactless switch, solid
> state switch and lid close sensor etc battery operation.
> 
> When the magnetic flux density (B) is larger than operate
> point (BOP), the output will be turned on (low), the output
> is held until the magnetic flux density (B) is lower than
> release point (BRP), then turn off (high). And the connected
> GPIO will trigger interrupts to CPU according to the output
> change of as1895.
> 
> This driver reports the magnetic field change of as1895 via
> input subsystem to notify user space system suspend/resume
> status. It can alse report the state change to the external
> connectors via the Extcon (External Connector) subsystem.
> 
> Tested-by: Calvin Fang <fangtianwen@huaqin.com>
> Signed-off-by: Wentong Wu <wentongw@amazon.com>

Using an extcon device here feels weird/wrong. In an offlist
discussion about this you mentioned that:

> Could you please share more about the extcon? I just use extcon
> to notify other drivers the status change.

"extcon" stands for external connector, it is mainly designed for use
with micro-usb and/or 3.5 mm jack *connectors* to indicate if there is
something or nothing plugged in and if something is plugged in what
it is (e.g. charger/ USB-device / USB-host connected, or microphone/
headphones/headset/line-in/line-out/rs232-over-jack connected).

Since your LID switch is not a connector of any kind using extcon for
this is clearly wrong.

Also why do other drivers need to respond to the LID switch, typically
the LID switch state is reported to userspace and userspace will then
decide to suspend the whole system, or if an external display is
connected and active, to ignore the LID switch. So other drivers will
get their suspend callback called if they need to act on the LID switch.

If other drivers really need to respond to the LID switch for some reason
they can install an in kernel input-listener, see: net/rfkill/input.c for
an example, especially how that code calls input_register_handler().

As already mentioned by Jeff in his review just to turn a GPIO into
an input-device reporting LID_SW you do not need a new driver, you can
do this with the proper devicetree description of the switch in combination
with the existing gpio-keys driver.

So all in all it seems that you do not need this driver at all.

Regards,

Hans







> ---
>  drivers/input/misc/Kconfig  |   9 ++
>  drivers/input/misc/Makefile |   1 +
>  drivers/input/misc/as1895.c | 193 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 203 insertions(+)
>  create mode 100644 drivers/input/misc/as1895.c
> 
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index 6a852c76331b..6ba26052354b 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -956,4 +956,13 @@ config INPUT_STPMIC1_ONKEY
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called stpmic1_onkey.
>  
> +config INPUT_AS1895
> +	tristate "AS1895 hall sensor support"
> +	depends on GPIOLIB || COMPILE_TEST
> +	help
> +	  Say Y here if you have a as1895 hall sensor connected to a GPIO pin.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called as1895.
> +
>  endif
> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> index 4f7f736831ba..38b364a6c8c1 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -92,3 +92,4 @@ obj-$(CONFIG_INPUT_WM831X_ON)		+= wm831x-on.o
>  obj-$(CONFIG_INPUT_XEN_KBDDEV_FRONTEND)	+= xen-kbdfront.o
>  obj-$(CONFIG_INPUT_YEALINK)		+= yealink.o
>  obj-$(CONFIG_INPUT_IDEAPAD_SLIDEBAR)	+= ideapad_slidebar.o
> +obj-$(CONFIG_INPUT_AS1895)		+= as1895.o
> diff --git a/drivers/input/misc/as1895.c b/drivers/input/misc/as1895.c
> new file mode 100644
> index 000000000000..d87318f1bda4
> --- /dev/null
> +++ b/drivers/input/misc/as1895.c
> @@ -0,0 +1,193 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2025 Amazon.com, Inc. or its affiliates. All Rights Reserved.
> + */
> +
> +#include <linux/extcon-provider.h>
> +#include <linux/gpio.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/module.h>
> +#include <linux/of_gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm.h>
> +#include <linux/pm_wakeup.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +
> +/* Timeout value for processing the wakeup event */
> +#define AS1895_WAKEUP_TIMEOUT_MS 100
> +
> +#define AS1895_DRV_NAME "AMZN-AS1895"
> +
> +struct as1895_dev {
> +	struct input_dev *input;
> +	struct extcon_dev *edev;
> +	struct wakeup_source *ws;
> +
> +	/* the connected gpio pin number */
> +	int gpio_pin;
> +	int irq;
> +};
> +
> +static const unsigned int as1895_cable[] = {
> +	EXTCON_MECHANICAL,
> +	EXTCON_NONE,
> +};
> +
> +static irqreturn_t as1895_irq_thread_handler(int irq, void *data)
> +{
> +	struct as1895_dev *as1895 = (struct as1895_dev *)data;
> +	struct input_dev *input = as1895->input;
> +	int gpio_val;
> +
> +	/* allow user space to consume the event */
> +	__pm_wakeup_event(as1895->ws, AS1895_WAKEUP_TIMEOUT_MS);
> +
> +	gpio_val = gpio_get_value(as1895->gpio_pin);
> +
> +	extcon_set_state_sync(as1895->edev, EXTCON_MECHANICAL,
> +			      gpio_val ? false : true);
> +
> +	input_report_switch(input, SW_LID, !gpio_val);
> +
> +	input_sync(input);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int as1895_probe(struct platform_device *pdev)
> +{
> +	struct device_node *node = pdev->dev.of_node;
> +	struct device *dev = &pdev->dev;
> +	struct as1895_dev *as1895;
> +	unsigned long irqflags;
> +	int ret, gpio_pin;
> +	const char *name;
> +
> +	ret = of_property_read_string(node, "name", &name);
> +	if (ret)
> +		return ret;
> +
> +	gpio_pin = of_get_named_gpio(node, "int-gpio", 0);
> +	if (!gpio_is_valid(gpio_pin))
> +		return -EINVAL;
> +
> +	as1895 = devm_kzalloc(dev, sizeof(struct as1895_dev), GFP_KERNEL);
> +	if (!as1895)
> +		return -ENOMEM;
> +
> +	as1895->edev = devm_extcon_dev_allocate(dev, as1895_cable);
> +	if (IS_ERR(as1895->edev))
> +		return -ENOMEM;
> +
> +	/* register extcon device */
> +	ret = devm_extcon_dev_register(dev, as1895->edev);
> +	if (ret) {
> +		dev_err(dev, "can't register extcon device: %d\n", ret);
> +		return ret;
> +	}
> +
> +	as1895->input = devm_input_allocate_device(dev);
> +	if (!as1895->input)
> +		return -ENOMEM;
> +
> +	as1895->input->name = name;
> +	set_bit(EV_SW, as1895->input->evbit);
> +	set_bit(SW_LID, as1895->input->swbit);
> +
> +	/* register input device */
> +	ret = input_register_device(as1895->input);
> +	if (ret) {
> +		dev_err(dev, "can't register input device: %d\n", ret);
> +		return ret;
> +	}
> +
> +	as1895->ws = wakeup_source_register(NULL, name);
> +	if (!as1895->ws)
> +		return -ENOMEM;
> +
> +	as1895->gpio_pin = gpio_pin;
> +	as1895->irq = gpio_to_irq(gpio_pin);
> +
> +	irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT;
> +
> +	ret = request_threaded_irq(as1895->irq, NULL,
> +				   as1895_irq_thread_handler,
> +				   irqflags, name, as1895);
> +	if (ret)
> +		goto err_unregister_ws;
> +
> +	platform_set_drvdata(pdev, as1895);
> +
> +	device_init_wakeup(dev, true);
> +
> +	return 0;
> +
> +err_unregister_ws:
> +	wakeup_source_unregister(as1895->ws);
> +
> +	return ret;
> +}
> +
> +static void as1895_remove(struct platform_device *pdev)
> +{
> +	struct as1895_dev *as1895 = platform_get_drvdata(pdev);
> +
> +	device_init_wakeup(&pdev->dev, false);
> +
> +	free_irq(as1895->irq, as1895);
> +
> +	wakeup_source_unregister(as1895->ws);
> +}
> +
> +static int as1895_suspend(struct device *dev)
> +{
> +	struct as1895_dev *as1895 = dev_get_drvdata(dev);
> +
> +	if (device_may_wakeup(dev))
> +		enable_irq_wake(as1895->irq);
> +
> +	return 0;
> +}
> +
> +static int as1895_resume(struct device *dev)
> +{
> +	struct as1895_dev *as1895 = dev_get_drvdata(dev);
> +
> +	if (device_may_wakeup(dev))
> +		disable_irq_wake(as1895->irq);
> +
> +	return 0;
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(as1895_pm_ops, as1895_suspend, as1895_resume);
> +
> +#if IS_ENABLED(CONFIG_OF)
> +static const struct of_device_id as1895_of_match[] = {
> +	{ .compatible = "amazon,as1895-hall-sensor" },
> +	{ /* sentinel */ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, as1895_of_match);
> +#endif
> +
> +static struct platform_driver as1895_driver = {
> +	.probe = as1895_probe,
> +	.remove = as1895_remove,
> +	.driver = {
> +		.name = AS1895_DRV_NAME,
> +		.pm = pm_sleep_ptr(&as1895_pm_ops),
> +		.of_match_table = of_match_ptr(as1895_of_match),
> +	}
> +};
> +
> +module_platform_driver(as1895_driver);
> +
> +MODULE_AUTHOR("Wentong Wu <wentongw@amazon.com>");
> +MODULE_AUTHOR("Zengjin Zhao <zzengjin@amazon.com>");
> +MODULE_AUTHOR("Xinkai Liu <xinka@amazon.com>");
> +MODULE_AUTHOR("Weihua Wu <wuweihua@amazon.com>");
> +MODULE_DESCRIPTION("Amazon as1895 hall sensor driver");
> +MODULE_LICENSE("GPL");