* RE: [PATCH 05/22] Input: ad7877 - use device core to create driver-specific device attributes
From: Hennerich, Michael @ 2023-07-31 5:23 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, Greg Kroah-Hartman
In-Reply-To: <20230729005133.1095051-5-dmitry.torokhov@gmail.com>
> -----Original Message-----
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Sent: Samstag, 29. Juli 2023 02:51
> To: linux-input@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; Hennerich, Michael
> <Michael.Hennerich@analog.com>
> Subject: [PATCH 05/22] Input: ad7877 - use device core to create driver-
> specific device attributes
>
> Instead of creating driver-specific device attributes with
> devm_device_add_group() have device core do this by setting up dev_groups
> pointer in the driver structure.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Acked-by: Michael Hennerich <michael.hennerich@analog.com>
> ---
> drivers/input/touchscreen/ad7877.c | 12 +++++-------
> 1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/input/touchscreen/ad7877.c
> b/drivers/input/touchscreen/ad7877.c
> index edb36d663f22..a0598e9c7aff 100644
> --- a/drivers/input/touchscreen/ad7877.c
> +++ b/drivers/input/touchscreen/ad7877.c
> @@ -612,10 +612,11 @@ static umode_t ad7877_attr_is_visible(struct
> kobject *kobj,
> return mode;
> }
>
> -static const struct attribute_group ad7877_attr_group = {
> +static const struct attribute_group ad7877_group = {
> .is_visible = ad7877_attr_is_visible,
> .attrs = ad7877_attributes,
> };
> +__ATTRIBUTE_GROUPS(ad7877);
>
> static void ad7877_setup_ts_def_msg(struct spi_device *spi, struct ad7877
> *ts) { @@ -777,10 +778,6 @@ static int ad7877_probe(struct spi_device
> *spi)
> return err;
> }
>
> - err = devm_device_add_group(&spi->dev, &ad7877_attr_group);
> - if (err)
> - return err;
> -
> err = input_register_device(input_dev);
> if (err)
> return err;
> @@ -810,8 +807,9 @@ static DEFINE_SIMPLE_DEV_PM_OPS(ad7877_pm,
> ad7877_suspend, ad7877_resume);
>
> static struct spi_driver ad7877_driver = {
> .driver = {
> - .name = "ad7877",
> - .pm = pm_sleep_ptr(&ad7877_pm),
> + .name = "ad7877",
> + .dev_groups = ad7877_groups,
> + .pm = pm_sleep_ptr(&ad7877_pm),
> },
> .probe = ad7877_probe,
> };
> --
> 2.41.0.487.g6d72f3e995-goog
^ permalink raw reply
* RE: [PATCH 06/22] Input: ad7879 - use device core to create driver-specific device attributes
From: Hennerich, Michael @ 2023-07-31 5:23 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, Greg Kroah-Hartman
In-Reply-To: <20230729005133.1095051-6-dmitry.torokhov@gmail.com>
> -----Original Message-----
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Sent: Samstag, 29. Juli 2023 02:51
> To: linux-input@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; Hennerich, Michael
> <Michael.Hennerich@analog.com>
> Subject: [PATCH 06/22] Input: ad7879 - use device core to create driver-
> specific device attributes
>
> Instead of creating driver-specific device attributes with
> devm_device_add_group() have device core do this by setting up dev_groups
> pointer in the driver structure.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Acked-by: Michael Hennerich <michael.hennerich@analog.com>
> ---
> drivers/input/touchscreen/ad7879-i2c.c | 7 ++++---
> drivers/input/touchscreen/ad7879-spi.c | 7 ++++---
> drivers/input/touchscreen/ad7879.c | 10 ++++++----
> drivers/input/touchscreen/ad7879.h | 3 +++
> 4 files changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/input/touchscreen/ad7879-i2c.c
> b/drivers/input/touchscreen/ad7879-i2c.c
> index feaa6f8b01ed..5c094ab74698 100644
> --- a/drivers/input/touchscreen/ad7879-i2c.c
> +++ b/drivers/input/touchscreen/ad7879-i2c.c
> @@ -58,9 +58,10 @@ MODULE_DEVICE_TABLE(of, ad7879_i2c_dt_ids);
>
> static struct i2c_driver ad7879_i2c_driver = {
> .driver = {
> - .name = "ad7879",
> - .pm = &ad7879_pm_ops,
> - .of_match_table = of_match_ptr(ad7879_i2c_dt_ids),
> + .name = "ad7879",
> + .dev_groups = ad7879_groups,
> + .pm = &ad7879_pm_ops,
> + .of_match_table = of_match_ptr(ad7879_i2c_dt_ids),
> },
> .probe = ad7879_i2c_probe,
> .id_table = ad7879_id,
> diff --git a/drivers/input/touchscreen/ad7879-spi.c
> b/drivers/input/touchscreen/ad7879-spi.c
> index 50e889846800..064968fe57cf 100644
> --- a/drivers/input/touchscreen/ad7879-spi.c
> +++ b/drivers/input/touchscreen/ad7879-spi.c
> @@ -56,9 +56,10 @@ MODULE_DEVICE_TABLE(of, ad7879_spi_dt_ids);
>
> static struct spi_driver ad7879_spi_driver = {
> .driver = {
> - .name = "ad7879",
> - .pm = &ad7879_pm_ops,
> - .of_match_table = of_match_ptr(ad7879_spi_dt_ids),
> + .name = "ad7879",
> + .dev_groups = ad7879_groups,
> + .pm = &ad7879_pm_ops,
> + .of_match_table = of_match_ptr(ad7879_spi_dt_ids),
> },
> .probe = ad7879_spi_probe,
> };
> diff --git a/drivers/input/touchscreen/ad7879.c
> b/drivers/input/touchscreen/ad7879.c
> index e850853328f1..e5d69bf2276e 100644
> --- a/drivers/input/touchscreen/ad7879.c
> +++ b/drivers/input/touchscreen/ad7879.c
> @@ -391,6 +391,12 @@ static const struct attribute_group
> ad7879_attr_group = {
> .attrs = ad7879_attributes,
> };
>
> +const struct attribute_group *ad7879_groups[] = {
> + &ad7879_attr_group,
> + NULL
> +};
> +EXPORT_SYMBOL_GPL(ad7879_groups);
> +
> #ifdef CONFIG_GPIOLIB
> static int ad7879_gpio_direction_input(struct gpio_chip *chip,
> unsigned gpio)
> @@ -612,10 +618,6 @@ int ad7879_probe(struct device *dev, struct regmap
> *regmap,
>
> __ad7879_disable(ts);
>
> - err = devm_device_add_group(dev, &ad7879_attr_group);
> - if (err)
> - return err;
> -
> err = ad7879_gpio_add(ts);
> if (err)
> return err;
> diff --git a/drivers/input/touchscreen/ad7879.h
> b/drivers/input/touchscreen/ad7879.h
> index ae8aa1428e56..d71a8e787290 100644
> --- a/drivers/input/touchscreen/ad7879.h
> +++ b/drivers/input/touchscreen/ad7879.h
> @@ -8,11 +8,14 @@
> #ifndef _AD7879_H_
> #define _AD7879_H_
>
> +#include <linux/pm.h>
> #include <linux/types.h>
>
> +struct attribute_group;
> struct device;
> struct regmap;
>
> +extern const struct attribute_group *ad7879_groups[];
> extern const struct dev_pm_ops ad7879_pm_ops;
>
> int ad7879_probe(struct device *dev, struct regmap *regmap,
> --
> 2.41.0.487.g6d72f3e995-goog
^ permalink raw reply
* Re: [PATCH] Input: pwm-beeper - Support volume setting via sysfs
From: Dmitry Torokhov @ 2023-07-31 5:36 UTC (permalink / raw)
To: Marek Vasut
Cc: Jeff LaBundy, linux-input, Uwe Kleine-König,
Frieder Schrempf, Manuel Traut, Thierry Reding, linux-pwm,
alsa-devel, Jaroslav Kysela, Takashi Iwai
In-Reply-To: <06379f26-ab24-85f9-783f-0c49d4291b23@denx.de>
On Sat, May 13, 2023 at 11:02:30PM +0200, Marek Vasut wrote:
> On 5/13/23 03:51, Marek Vasut wrote:
> > On 5/13/23 03:12, Jeff LaBundy wrote:
> > > Hi Marek,
> >
> > Hi,
> >
> > > On Fri, May 12, 2023 at 08:55:51PM +0200, Marek Vasut wrote:
> > > > The PWM beeper volume can be controlled by adjusting the PWM duty cycle,
> > > > expose volume setting via sysfs, so users can make the beeper quieter.
> > > > This patch adds sysfs attribute 'volume' in range 0..50000, i.e. from 0
> > > > to 50% in 1/1000th of percent steps, this resolution should be
> > > > sufficient.
> > > >
> > > > The reason for 50000 cap on volume or PWM duty cycle is because
> > > > duty cycle
> > > > above 50% again reduces the loudness, the PWM wave form is inverted wave
> > > > form of the one for duty cycle below 50% and the beeper gets quieter the
> > > > closer the setting is to 100% . Hence, 50% cap where the wave
> > > > form yields
> > > > the loudest result.
> > > >
> > > > Signed-off-by: Marek Vasut <marex@denx.de>
> > > > ---
> > > > An alternative option would be to extend the userspace input
> > > > ABI, e.g. by
> > > > using SND_TONE top 16bits to encode the duty cycle in 0..50000
> > > > range, and
> > > > bottom 16bit to encode the existing frequency in Hz . Since frequency in
> > > > Hz is likely to be below some 25 kHz for audible bell, this fits
> > > > in 16bits
> > > > just fine. Thoughts ?
> > > > ---
> > >
> > > Thanks for the patch; this seems like a useful feature.
> > >
> > > My first thought is that 50000 seems like an oddly specific limit to
> > > impose
> > > upon user space. Ideally, user space need not even care that the
> > > beeper is
> > > implemented via PWM and why 50000 is significant.
> > >
> > > Instead, what about accepting 0..255 as the LED subsystem does for
> > > brightness,
> > > then map these values to 0..50000 internally? In fact, the leds-pwm
> > > driver
> > > does something similar.
> >
> > The pwm_set_relative_duty_cycle() function can map whatever range to
> > whatever other range of the PWM already, so that's not an issues here.
> > It seems to me the 0..127 or 0..255 range is a bit too limiting . I
> > think even for the LEDs the reason for that limit is legacy design, but
> > here I might be wrong.
> >
> > > I'm also curious as to whether this function should be a rogue sysfs
> > > control
> > > limited to this driver, or a generic operation in input. For
> > > example, input
> > > already allows user space to specify the magnitude of an FF effect;
> > > perhaps
> > > something similar is warranted here?
> >
> > See the "An alternative ..." part above, I was wondering about this too,
> > whether this can be added into the input ABI, but I am somewhat
> > reluctant to fiddle with the ABI.
>
> Thinking about this further, we could try and add some
>
> EV_SND SND_TONE_WITH_VOLUME
>
> to avoid overloading EV_SND SND_TONE , and at the same time allow the user
> to set both frequency and volume for the tone without any race condition
> between the two.
>
> The EV_SND SND_TONE_WITH_VOLUME would still take one 32bit parameter, except
> this time the parameter 16 LSbits would be the frequency and 16 MSbits would
> be the volume.
>
> But again, here I would like input from the maintainers.
Beeper was supposed to be an extremely simple device with minimal
controls. I wonder if there is need for volume controls, etc, etc are we
not better moving it over to the sound subsystem. We already have:
sound/drivers/pcsp/pcsp.c
and
sound/pci/hda/hda_beep.c
there, can we have other "advanced" beepers there as well? Adding sound
maintainers to CC...
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH v4 1/3] input: pm8xxx-vib: refactor to easily support new SPMI vibrator
From: Fenglin Wu @ 2023-07-31 5:37 UTC (permalink / raw)
To: linux-arm-msm, linux-kernel, krzysztof.kozlowski+dt, robh+dt,
agross, andersson, dmitry.baryshkov, Konrad Dybcio,
Dmitry Torokhov, linux-input
Cc: quic_collinsd, quic_subbaram, quic_fenglinw, quic_kamalw, jestar
In-Reply-To: <20230731053712.2220898-1-quic_fenglinw@quicinc.com>
Currently, all vibrator control register addresses are hard coded,
including the base address and the offset, it's not flexible to support
new SPMI vibrator module which is usually included in different PMICs
with different base address. Refactor this by defining register offset
with HW type combination, and register base address which is defined
in 'reg' property is added for SPMI vibrators.
Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
---
drivers/input/misc/pm8xxx-vibrator.c | 122 ++++++++++++++++-----------
1 file changed, 73 insertions(+), 49 deletions(-)
diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
index 04cb87efd799..d6b468324c77 100644
--- a/drivers/input/misc/pm8xxx-vibrator.c
+++ b/drivers/input/misc/pm8xxx-vibrator.c
@@ -12,36 +12,44 @@
#include <linux/regmap.h>
#include <linux/slab.h>
+#define SSBL_VIB_DRV_REG 0x4A
+#define SSBI_VIB_DRV_EN_MANUAL_MASK GENMASK(7, 2)
+#define SSBI_VIB_DRV_LEVEL_MASK GENMASK(7, 3)
+#define SSBI_VIB_DRV_SHIFT 3
+
+#define SPMI_VIB_DRV_REG 0x41
+#define SPMI_VIB_DRV_LEVEL_MASK GENMASK(4, 0)
+#define SPMI_VIB_DRV_SHIFT 0
+
+#define SPMI_VIB_EN_REG 0x46
+#define SPMI_VIB_EN_BIT BIT(7)
+
#define VIB_MAX_LEVEL_mV (3100)
#define VIB_MIN_LEVEL_mV (1200)
#define VIB_MAX_LEVELS (VIB_MAX_LEVEL_mV - VIB_MIN_LEVEL_mV)
#define MAX_FF_SPEED 0xff
-struct pm8xxx_regs {
- unsigned int enable_addr;
- unsigned int enable_mask;
+enum vib_hw_type {
+ SSBI_VIB,
+ SPMI_VIB,
+};
- unsigned int drv_addr;
- unsigned int drv_mask;
- unsigned int drv_shift;
- unsigned int drv_en_manual_mask;
+struct pm8xxx_vib_data {
+ enum vib_hw_type hw_type;
+ unsigned int enable_addr;
+ unsigned int drv_addr;
};
-static const struct pm8xxx_regs pm8058_regs = {
- .drv_addr = 0x4A,
- .drv_mask = 0xf8,
- .drv_shift = 3,
- .drv_en_manual_mask = 0xfc,
+static const struct pm8xxx_vib_data ssbi_vib_data = {
+ .hw_type = SSBI_VIB,
+ .drv_addr = SSBL_VIB_DRV_REG,
};
-static struct pm8xxx_regs pm8916_regs = {
- .enable_addr = 0xc046,
- .enable_mask = BIT(7),
- .drv_addr = 0xc041,
- .drv_mask = 0x1F,
- .drv_shift = 0,
- .drv_en_manual_mask = 0,
+static const struct pm8xxx_vib_data spmi_vib_data = {
+ .hw_type = SPMI_VIB,
+ .enable_addr = SPMI_VIB_EN_REG,
+ .drv_addr = SPMI_VIB_DRV_REG,
};
/**
@@ -49,7 +57,8 @@ static struct pm8xxx_regs pm8916_regs = {
* @vib_input_dev: input device supporting force feedback
* @work: work structure to set the vibration parameters
* @regmap: regmap for register read/write
- * @regs: registers' info
+ * @data: vibrator HW info
+ * @reg_base: the register base of the module
* @speed: speed of vibration set from userland
* @active: state of vibrator
* @level: level of vibration to set in the chip
@@ -59,7 +68,8 @@ struct pm8xxx_vib {
struct input_dev *vib_input_dev;
struct work_struct work;
struct regmap *regmap;
- const struct pm8xxx_regs *regs;
+ const struct pm8xxx_vib_data *data;
+ unsigned int reg_base;
int speed;
int level;
bool active;
@@ -75,24 +85,31 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
{
int rc;
unsigned int val = vib->reg_vib_drv;
- const struct pm8xxx_regs *regs = vib->regs;
+ u32 mask = SPMI_VIB_DRV_LEVEL_MASK;
+ u32 shift = SPMI_VIB_DRV_SHIFT;
+
+ if (vib->data->hw_type == SSBI_VIB) {
+ mask = SSBI_VIB_DRV_LEVEL_MASK;
+ shift = SSBI_VIB_DRV_SHIFT;
+ }
if (on)
- val |= (vib->level << regs->drv_shift) & regs->drv_mask;
+ val |= (vib->level << shift) & mask;
else
- val &= ~regs->drv_mask;
+ val &= ~mask;
- rc = regmap_write(vib->regmap, regs->drv_addr, val);
+ rc = regmap_update_bits(vib->regmap, vib->reg_base + vib->data->drv_addr, mask, val);
if (rc < 0)
return rc;
vib->reg_vib_drv = val;
- if (regs->enable_mask)
- rc = regmap_update_bits(vib->regmap, regs->enable_addr,
- regs->enable_mask, on ? ~0 : 0);
+ if (vib->data->hw_type == SSBI_VIB)
+ return 0;
- return rc;
+ mask = SPMI_VIB_EN_BIT;
+ val = on ? SPMI_VIB_EN_BIT : 0;
+ return regmap_update_bits(vib->regmap, vib->reg_base + vib->data->enable_addr, mask, val);
}
/**
@@ -102,13 +119,6 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
static void pm8xxx_work_handler(struct work_struct *work)
{
struct pm8xxx_vib *vib = container_of(work, struct pm8xxx_vib, work);
- const struct pm8xxx_regs *regs = vib->regs;
- int rc;
- unsigned int val;
-
- rc = regmap_read(vib->regmap, regs->drv_addr, &val);
- if (rc < 0)
- return;
/*
* pmic vibrator supports voltage ranges from 1.2 to 3.1V, so
@@ -168,9 +178,9 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
{
struct pm8xxx_vib *vib;
struct input_dev *input_dev;
+ const struct pm8xxx_vib_data *data;
int error;
- unsigned int val;
- const struct pm8xxx_regs *regs;
+ unsigned int val, reg_base;
vib = devm_kzalloc(&pdev->dev, sizeof(*vib), GFP_KERNEL);
if (!vib)
@@ -187,19 +197,33 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
INIT_WORK(&vib->work, pm8xxx_work_handler);
vib->vib_input_dev = input_dev;
- regs = of_device_get_match_data(&pdev->dev);
+ data = of_device_get_match_data(&pdev->dev);
+ if (!data)
+ return -EINVAL;
- /* operate in manual mode */
- error = regmap_read(vib->regmap, regs->drv_addr, &val);
- if (error < 0)
- return error;
+ if (data->hw_type != SSBI_VIB) {
+ error = fwnode_property_read_u32(pdev->dev.fwnode, "reg", ®_base);
+ if (error < 0) {
+ dev_err(&pdev->dev, "Failed to read reg address, rc=%d\n", error);
+ return error;
+ }
+
+ vib->reg_base += reg_base;
+ }
- val &= regs->drv_en_manual_mask;
- error = regmap_write(vib->regmap, regs->drv_addr, val);
+ error = regmap_read(vib->regmap, vib->reg_base + data->drv_addr, &val);
if (error < 0)
return error;
- vib->regs = regs;
+ /* operate in manual mode */
+ if (data->hw_type == SSBI_VIB) {
+ val &= SSBI_VIB_DRV_EN_MANUAL_MASK;
+ error = regmap_write(vib->regmap, vib->reg_base + data->drv_addr, val);
+ if (error < 0)
+ return error;
+ }
+
+ vib->data = data;
vib->reg_vib_drv = val;
input_dev->name = "pm8xxx_vib_ffmemless";
@@ -239,9 +263,9 @@ static int pm8xxx_vib_suspend(struct device *dev)
static DEFINE_SIMPLE_DEV_PM_OPS(pm8xxx_vib_pm_ops, pm8xxx_vib_suspend, NULL);
static const struct of_device_id pm8xxx_vib_id_table[] = {
- { .compatible = "qcom,pm8058-vib", .data = &pm8058_regs },
- { .compatible = "qcom,pm8921-vib", .data = &pm8058_regs },
- { .compatible = "qcom,pm8916-vib", .data = &pm8916_regs },
+ { .compatible = "qcom,pm8058-vib", .data = &ssbi_vib_data },
+ { .compatible = "qcom,pm8921-vib", .data = &ssbi_vib_data },
+ { .compatible = "qcom,pm8916-vib", .data = &spmi_vib_data },
{ }
};
MODULE_DEVICE_TABLE(of, pm8xxx_vib_id_table);
--
2.25.1
^ permalink raw reply related
* [PATCH v4 2/3] dt-bindings: input: qcom,pm8xxx-vib: add new SPMI vibrator module
From: Fenglin Wu @ 2023-07-31 5:37 UTC (permalink / raw)
To: linux-arm-msm, linux-kernel, krzysztof.kozlowski+dt, robh+dt,
agross, andersson, dmitry.baryshkov, Konrad Dybcio,
Dmitry Torokhov, linux-input, devicetree
Cc: quic_collinsd, quic_subbaram, quic_fenglinw, quic_kamalw, jestar
In-Reply-To: <20230731053712.2220898-1-quic_fenglinw@quicinc.com>
Add compatible string 'qcom,spmi-vib-gen2' to support vibrator module
inside PMI632, PMI7250B, PM7325B, PM7550BA.
Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
---
.../bindings/input/qcom,pm8xxx-vib.yaml | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml
index c8832cd0d7da..4a2319fc1e3f 100644
--- a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml
+++ b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml
@@ -11,10 +11,18 @@ maintainers:
properties:
compatible:
- enum:
- - qcom,pm8058-vib
- - qcom,pm8916-vib
- - qcom,pm8921-vib
+ oneOf:
+ - enum:
+ - qcom,pm8058-vib
+ - qcom,pm8916-vib
+ - qcom,pm8921-vib
+ - items:
+ - enum:
+ - qcom,pmi632-vib
+ - qcom,pm7250b-vib
+ - qcom,pm7325b-vib
+ - qcom,pm7550b-vib
+ - const: qcom,spmi-vib-gen2
reg:
maxItems: 1
--
2.25.1
^ permalink raw reply related
* [PATCH v4 3/3] input: pm8xxx-vibrator: add new SPMI vibrator support
From: Fenglin Wu @ 2023-07-31 5:37 UTC (permalink / raw)
To: linux-arm-msm, linux-kernel, krzysztof.kozlowski+dt, robh+dt,
agross, andersson, dmitry.baryshkov, Konrad Dybcio,
Dmitry Torokhov, linux-input
Cc: quic_collinsd, quic_subbaram, quic_fenglinw, quic_kamalw, jestar
In-Reply-To: <20230731053712.2220898-1-quic_fenglinw@quicinc.com>
Add new SPMI vibrator module which is very similar to the SPMI vibrator
module inside PM8916 but just has a finer drive voltage step (1mV vs
100mV) hence its drive level control is expanded to across 2 registers.
Name the module as 'qcom,spmi-vib-gen2', and it can be found in
following Qualcomm PMICs: PMI632, PM7250B, PM7325B, PM7550BA.
Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
---
drivers/input/misc/pm8xxx-vibrator.c | 55 +++++++++++++++++++++++++---
1 file changed, 50 insertions(+), 5 deletions(-)
diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
index d6b468324c77..9cfd3dec5366 100644
--- a/drivers/input/misc/pm8xxx-vibrator.c
+++ b/drivers/input/misc/pm8xxx-vibrator.c
@@ -21,6 +21,13 @@
#define SPMI_VIB_DRV_LEVEL_MASK GENMASK(4, 0)
#define SPMI_VIB_DRV_SHIFT 0
+#define SPMI_VIB_GEN2_DRV_REG 0x40
+#define SPMI_VIB_GEN2_DRV_MASK GENMASK(7, 0)
+#define SPMI_VIB_GEN2_DRV_SHIFT 0
+#define SPMI_VIB_GEN2_DRV2_REG 0x41
+#define SPMI_VIB_GEN2_DRV2_MASK GENMASK(3, 0)
+#define SPMI_VIB_GEN2_DRV2_SHIFT 8
+
#define SPMI_VIB_EN_REG 0x46
#define SPMI_VIB_EN_BIT BIT(7)
@@ -33,12 +40,14 @@
enum vib_hw_type {
SSBI_VIB,
SPMI_VIB,
+ SPMI_VIB_GEN2
};
struct pm8xxx_vib_data {
enum vib_hw_type hw_type;
unsigned int enable_addr;
unsigned int drv_addr;
+ unsigned int drv2_addr;
};
static const struct pm8xxx_vib_data ssbi_vib_data = {
@@ -52,6 +61,13 @@ static const struct pm8xxx_vib_data spmi_vib_data = {
.drv_addr = SPMI_VIB_DRV_REG,
};
+static const struct pm8xxx_vib_data spmi_vib_gen2_data = {
+ .hw_type = SPMI_VIB_GEN2,
+ .enable_addr = SPMI_VIB_EN_REG,
+ .drv_addr = SPMI_VIB_GEN2_DRV_REG,
+ .drv2_addr = SPMI_VIB_GEN2_DRV2_REG,
+};
+
/**
* struct pm8xxx_vib - structure to hold vibrator data
* @vib_input_dev: input device supporting force feedback
@@ -85,12 +101,24 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
{
int rc;
unsigned int val = vib->reg_vib_drv;
- u32 mask = SPMI_VIB_DRV_LEVEL_MASK;
- u32 shift = SPMI_VIB_DRV_SHIFT;
+ u32 mask, shift;
- if (vib->data->hw_type == SSBI_VIB) {
+
+ switch (vib->data->hw_type) {
+ case SSBI_VIB:
mask = SSBI_VIB_DRV_LEVEL_MASK;
shift = SSBI_VIB_DRV_SHIFT;
+ break;
+ case SPMI_VIB:
+ mask = SPMI_VIB_DRV_LEVEL_MASK;
+ shift = SPMI_VIB_DRV_SHIFT;
+ break;
+ case SPMI_VIB_GEN2:
+ mask = SPMI_VIB_GEN2_DRV_MASK;
+ shift = SPMI_VIB_GEN2_DRV_SHIFT;
+ break;
+ default:
+ return -EINVAL;
}
if (on)
@@ -104,6 +132,19 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
vib->reg_vib_drv = val;
+ if (vib->data->hw_type == SPMI_VIB_GEN2) {
+ mask = SPMI_VIB_GEN2_DRV2_MASK;
+ shift = SPMI_VIB_GEN2_DRV2_SHIFT;
+ if (on)
+ val = (vib->level >> shift) & mask;
+ else
+ val = 0;
+ rc = regmap_update_bits(vib->regmap,
+ vib->reg_base + vib->data->drv2_addr, mask, val);
+ if (rc < 0)
+ return rc;
+ }
+
if (vib->data->hw_type == SSBI_VIB)
return 0;
@@ -128,10 +169,13 @@ static void pm8xxx_work_handler(struct work_struct *work)
vib->active = true;
vib->level = ((VIB_MAX_LEVELS * vib->speed) / MAX_FF_SPEED) +
VIB_MIN_LEVEL_mV;
- vib->level /= 100;
+ if (vib->data->hw_type != SPMI_VIB_GEN2)
+ vib->level /= 100;
} else {
vib->active = false;
- vib->level = VIB_MIN_LEVEL_mV / 100;
+ vib->level = VIB_MIN_LEVEL_mV;
+ if (vib->data->hw_type != SPMI_VIB_GEN2)
+ vib->level /= 100;
}
pm8xxx_vib_set(vib, vib->active);
@@ -266,6 +310,7 @@ static const struct of_device_id pm8xxx_vib_id_table[] = {
{ .compatible = "qcom,pm8058-vib", .data = &ssbi_vib_data },
{ .compatible = "qcom,pm8921-vib", .data = &ssbi_vib_data },
{ .compatible = "qcom,pm8916-vib", .data = &spmi_vib_data },
+ { .compatible = "qcom,spmi-vib-gen2", .data = &spmi_vib_gen2_data },
{ }
};
MODULE_DEVICE_TABLE(of, pm8xxx_vib_id_table);
--
2.25.1
^ permalink raw reply related
* Re: [PATCH] Input: elan_i2c - Implement inhibit/uninhibit functions.
From: 'Dmitry Torokhov' @ 2023-07-31 5:47 UTC (permalink / raw)
To: Jingle.Wu; +Cc: linux-kernel, linux-input, phoenix, josh.chen, dave.wang
In-Reply-To: <000001d9af07$3c003a20$b400ae60$@emc.com.tw>
Hi Jingle,
On Wed, Jul 05, 2023 at 02:09:18PM +0800, Jingle.Wu wrote:
> HI Dmitry:
>
> 1.
> > +static void elan_input_lid_event(struct input_handle *handle,
> > +unsigned
> int type,
> > + unsigned int code, int value) {
> > + struct elan_tp_data *data, *n;
> > +
> > + if (type == EV_SW && code == SW_LID) {
> > + list_for_each_entry_safe(data, n,
> &elan_devices_with_lid_handler,
> > +list) {
>
> Why do you need the "_safe()" variant here?
> -> Because I took the above link as reference.
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/3
> 578852/7/drivers/hid/hid-multitouch.c#394
It is wrong there too. The _safe() variant protects list traversal when
the list is being modified by the same thread, here we do not do that.
>
> 2.
> > + data->lid_switch = true;
> > + INIT_LIST_HEAD(&data->list);
> > + INIT_WORK(&data->lid_work, lid_work_handler);
> > + list_add_tail(&data->list, &elan_devices_with_lid_handler);
>
> It looks like you call elan_create_lid_handler() from elan_probe() which
> means it can be called several times (we should not assume there is only one
> controller), I do not see it being destroyed in remove() either, so it will
> break if you bind/unbind the driver.
>
> I also not sure why you need the list of you have a handler per device.
>
> -> If the elan_create_lid_handler() function not be put into probe(), the
> lid
> handler would be invalid in elan private data("struct elan_tp_data *data").
> Or do you have any suggestions for it? Thanks.
The handler's connect() is called for each matching device so you can
tie it up at that time.
>
> 3.
> > + error = elan_create_lid_handler(data);
> > + if (error)
> > + dev_err(dev, "failed to create lid handler: %d\n", error);
>
> Do we need this on _ALL_ devices with ELan controllers, or just certain
> ones? If we need this on all devices how did it work before?
>
> -> Yes, we need this on all device.
> In Chromeos kernel v5.4 before, the elan_inhibit()/uninhibit function was
> built and worked
> successfully in elan_i2c_driver.
> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/heads
> /chromeos-5.4/drivers/input/mouse/elan_i2c_core.c#353
We have the Elan touchpad driver without this functionality for many
years. I am aware that certain devices need this, but the fact that
Chrome OS kernel 5.4 (which is only used on a subset of Chromebooks) has
it does not necessarily mean that this functionality is needed on _ALL_
devices.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: pwm-beeper - Support volume setting via sysfs
From: Takashi Iwai @ 2023-07-31 6:21 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Marek Vasut, Jeff LaBundy, linux-input, Uwe Kleine-König,
Frieder Schrempf, Manuel Traut, Thierry Reding, linux-pwm,
alsa-devel, Jaroslav Kysela, Takashi Iwai
In-Reply-To: <ZMdIZiC453onyeHh@google.com>
On Mon, 31 Jul 2023 07:36:38 +0200,
Dmitry Torokhov wrote:
>
> On Sat, May 13, 2023 at 11:02:30PM +0200, Marek Vasut wrote:
> > On 5/13/23 03:51, Marek Vasut wrote:
> > > On 5/13/23 03:12, Jeff LaBundy wrote:
> > > > Hi Marek,
> > >
> > > Hi,
> > >
> > > > On Fri, May 12, 2023 at 08:55:51PM +0200, Marek Vasut wrote:
> > > > > The PWM beeper volume can be controlled by adjusting the PWM duty cycle,
> > > > > expose volume setting via sysfs, so users can make the beeper quieter.
> > > > > This patch adds sysfs attribute 'volume' in range 0..50000, i.e. from 0
> > > > > to 50% in 1/1000th of percent steps, this resolution should be
> > > > > sufficient.
> > > > >
> > > > > The reason for 50000 cap on volume or PWM duty cycle is because
> > > > > duty cycle
> > > > > above 50% again reduces the loudness, the PWM wave form is inverted wave
> > > > > form of the one for duty cycle below 50% and the beeper gets quieter the
> > > > > closer the setting is to 100% . Hence, 50% cap where the wave
> > > > > form yields
> > > > > the loudest result.
> > > > >
> > > > > Signed-off-by: Marek Vasut <marex@denx.de>
> > > > > ---
> > > > > An alternative option would be to extend the userspace input
> > > > > ABI, e.g. by
> > > > > using SND_TONE top 16bits to encode the duty cycle in 0..50000
> > > > > range, and
> > > > > bottom 16bit to encode the existing frequency in Hz . Since frequency in
> > > > > Hz is likely to be below some 25 kHz for audible bell, this fits
> > > > > in 16bits
> > > > > just fine. Thoughts ?
> > > > > ---
> > > >
> > > > Thanks for the patch; this seems like a useful feature.
> > > >
> > > > My first thought is that 50000 seems like an oddly specific limit to
> > > > impose
> > > > upon user space. Ideally, user space need not even care that the
> > > > beeper is
> > > > implemented via PWM and why 50000 is significant.
> > > >
> > > > Instead, what about accepting 0..255 as the LED subsystem does for
> > > > brightness,
> > > > then map these values to 0..50000 internally? In fact, the leds-pwm
> > > > driver
> > > > does something similar.
> > >
> > > The pwm_set_relative_duty_cycle() function can map whatever range to
> > > whatever other range of the PWM already, so that's not an issues here.
> > > It seems to me the 0..127 or 0..255 range is a bit too limiting . I
> > > think even for the LEDs the reason for that limit is legacy design, but
> > > here I might be wrong.
> > >
> > > > I'm also curious as to whether this function should be a rogue sysfs
> > > > control
> > > > limited to this driver, or a generic operation in input. For
> > > > example, input
> > > > already allows user space to specify the magnitude of an FF effect;
> > > > perhaps
> > > > something similar is warranted here?
> > >
> > > See the "An alternative ..." part above, I was wondering about this too,
> > > whether this can be added into the input ABI, but I am somewhat
> > > reluctant to fiddle with the ABI.
> >
> > Thinking about this further, we could try and add some
> >
> > EV_SND SND_TONE_WITH_VOLUME
> >
> > to avoid overloading EV_SND SND_TONE , and at the same time allow the user
> > to set both frequency and volume for the tone without any race condition
> > between the two.
> >
> > The EV_SND SND_TONE_WITH_VOLUME would still take one 32bit parameter, except
> > this time the parameter 16 LSbits would be the frequency and 16 MSbits would
> > be the volume.
> >
> > But again, here I would like input from the maintainers.
>
> Beeper was supposed to be an extremely simple device with minimal
> controls. I wonder if there is need for volume controls, etc, etc are we
> not better moving it over to the sound subsystem. We already have:
>
> sound/drivers/pcsp/pcsp.c
>
> and
>
> sound/pci/hda/hda_beep.c
>
> there, can we have other "advanced" beepers there as well? Adding sound
> maintainers to CC...
I don't mind it put to sound/*. But, note that pcsp.c you pointed in
the above is a PCM tone generator driver with a PC beep device, and it
provides the normal SND_BEEP input only for compatibility.
Indeed there have been already many sound drivers providing the beep
capability, and they bind with the input device using SND_BEEP. And,
for the beep volume, "Beep Playback Volume" mixer control is provided,
too.
thanks,
Takashi
^ permalink raw reply
* [PATCH 04/10] input: serport: remove casts from tty->disc_data
From: Jiri Slaby (SUSE) @ 2023-07-31 8:02 UTC (permalink / raw)
To: gregkh
Cc: linux-serial, linux-kernel, Jiri Slaby (SUSE), Dmitry Torokhov,
linux-input
In-Reply-To: <20230731080244.2698-1-jirislaby@kernel.org>
tty->disc_data is 'void *', so there is no need to cast from that.
Therefore remove the casts and assign the pointer directly.
Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: linux-input@vger.kernel.org
---
drivers/input/serio/serport.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/input/serio/serport.c b/drivers/input/serio/serport.c
index 7f7ef0e3a749..a5d8953f5904 100644
--- a/drivers/input/serio/serport.c
+++ b/drivers/input/serio/serport.c
@@ -103,7 +103,7 @@ static int serport_ldisc_open(struct tty_struct *tty)
static void serport_ldisc_close(struct tty_struct *tty)
{
- struct serport *serport = (struct serport *) tty->disc_data;
+ struct serport *serport = tty->disc_data;
kfree(serport);
}
@@ -117,7 +117,7 @@ static void serport_ldisc_close(struct tty_struct *tty)
static void serport_ldisc_receive(struct tty_struct *tty,
const unsigned char *cp, const char *fp, int count)
{
- struct serport *serport = (struct serport*) tty->disc_data;
+ struct serport *serport = tty->disc_data;
unsigned long flags;
unsigned int ch_flags = 0;
int i;
@@ -161,7 +161,7 @@ static ssize_t serport_ldisc_read(struct tty_struct * tty, struct file * file,
unsigned char *kbuf, size_t nr,
void **cookie, unsigned long offset)
{
- struct serport *serport = (struct serport*) tty->disc_data;
+ struct serport *serport = tty->disc_data;
struct serio *serio;
if (test_and_set_bit(SERPORT_BUSY, &serport->flags))
@@ -245,7 +245,7 @@ static int serport_ldisc_compat_ioctl(struct tty_struct *tty,
static void serport_ldisc_hangup(struct tty_struct *tty)
{
- struct serport *serport = (struct serport *) tty->disc_data;
+ struct serport *serport = tty->disc_data;
unsigned long flags;
spin_lock_irqsave(&serport->lock, flags);
@@ -257,7 +257,7 @@ static void serport_ldisc_hangup(struct tty_struct *tty)
static void serport_ldisc_write_wakeup(struct tty_struct * tty)
{
- struct serport *serport = (struct serport *) tty->disc_data;
+ struct serport *serport = tty->disc_data;
unsigned long flags;
spin_lock_irqsave(&serport->lock, flags);
--
2.41.0
^ permalink raw reply related
* Re: [PATCH v4 0/4] input: touchscreen: add initial support for Goodix Berlin touchscreen IC
From: Neil Armstrong @ 2023-07-31 9:15 UTC (permalink / raw)
To: Dmitry Torokhov, Bastien Nocera, Hans de Goede, Henrik Rydberg,
Jeff LaBundy
Cc: Conor Dooley, Krzysztof Kozlowski, Rob Herring, linux-input,
linux-arm-msm, devicetree, linux-kernel, Rob Herring
In-Reply-To: <20230606-topic-goodix-berlin-upstream-initial-v4-0-0947c489be17@linaro.org>
Hi,
On 29/06/2023 11:27, Neil Armstrong wrote:
> These touchscreen ICs support SPI, I2C and I3C interface, up to
> 10 finger touch, stylus and gestures events.
>
> This initial driver is derived from the Goodix goodix_ts_berlin
> available at [1] and [2] and only supports the GT9916 IC
> present on the Qualcomm SM8550 MTP & QRD touch panel.
>
> The current implementation only supports BerlinD, aka GT9916.
>
> Support for advanced features like:
> - Firmware & config update
> - Stylus events
> - Gestures events
> - Previous revisions support (BerlinA or BerlinB)
> is not included in current version.
>
> The current support will work with currently flashed firmware
> and config, and bail out if firmware or config aren't flashed yet.
>
> [1] https://github.com/goodix/goodix_ts_berlin
> [2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
Gentle ping,
Should I send a v5 rebased on linux-next ?
Thanks,
Neil
>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
> Changes in v4:
> - Core updates:
> - drop kconfig depends, deps will be handled by _SPI and _I2C
> - change power_on() error labels
> - print errors on all dev_err() prints
> - remove useless default variable initialization
> - switch irq touch checksum error to dev_err()
> - add Jeff's review tag
> - I2C changes
> - change REGMAP_I2C Kconfig from depends to select
> - add Jeff's review tag
> - SPI changes
> - add select REGMAP to Kconfig
> - added GOODIX_BERLIN_ prefix to defines
> - switched from ret to error
> - add Jeff's review tag
> - Link to v3: https://lore.kernel.org/r/20230606-topic-goodix-berlin-upstream-initial-v3-0-f0577cead709@linaro.org
>
> Changes in v3:
> - Another guge cleanups after Jeff's review:
> - appended goodix_berlin_ before all defines
> - removed some unused defines
> - removed retries on most of read functions, can be added back later
> - added __le to ic_info structures
> - reworked and simplified irq handling, dropped enum and ts_event structs
> - added struct for touch data
> - simplified and cleaned goodix_berlin_check_checksum & goodix_berlin_is_dummy_data
> - moved touch_data_addr to the end of the main code_data
> - reworked probe to get_irq last and right before setip input device
> - cleaned probe by removing the "cd->dev"
> - added short paragraph to justify new driver for berlin devices
> - defined all offsets & masks
> - Added bindings review tag
> - Link to v2: https://lore.kernel.org/r/20230606-topic-goodix-berlin-upstream-initial-v2-0-26bc8fe1e90e@linaro.org
>
> Changes in v2:
> - Huge cleanups after Jeff's review:
> - switch to error instead of ret
> - drop dummy vendor/product ids
> - drop unused defined/enums
> - drop unused ic_info and only keep needes values
> - cleanup namings and use goodix_berlin_ everywhere
> - fix regulator setup
> - fix default variables value when assigned afterwars
> - removed indirections
> - dropped debugfs
> - cleaned input_dev setup
> - dropped _remove()
> - sync'ed i2c and spi drivers
> - fixed yaml bindings
> - Link to v1: https://lore.kernel.org/r/20230606-topic-goodix-berlin-upstream-initial-v1-0-4a0741b8aefd@linaro.org
>
> ---
> Neil Armstrong (4):
> dt-bindings: input: document Goodix Berlin Touchscreen IC
> input: touchscreen: add core support for Goodix Berlin Touchscreen IC
> input: touchscreen: add I2C support for Goodix Berlin Touchscreen IC
> input: touchscreen: add SPI support for Goodix Berlin Touchscreen IC
>
> .../bindings/input/touchscreen/goodix,gt9916.yaml | 95 ++++
> drivers/input/touchscreen/Kconfig | 31 ++
> drivers/input/touchscreen/Makefile | 3 +
> drivers/input/touchscreen/goodix_berlin.h | 159 ++++++
> drivers/input/touchscreen/goodix_berlin_core.c | 581 +++++++++++++++++++++
> drivers/input/touchscreen/goodix_berlin_i2c.c | 69 +++
> drivers/input/touchscreen/goodix_berlin_spi.c | 173 ++++++
> 7 files changed, 1111 insertions(+)
> ---
> base-commit: 6db29e14f4fb7bce9eb5290288e71b05c2b0d118
> change-id: 20230606-topic-goodix-berlin-upstream-initial-ba97e8ec8f4c
>
> Best regards,
^ permalink raw reply
* RE: [HID Patchsets v1 2/2] HID: Removed USB Validation check
From: sandeep.cs @ 2023-07-31 11:13 UTC (permalink / raw)
To: 'Benjamin Tissoires'
Cc: 'Jiri Kosina', 'Benjamin Tissoires', junwan.cho,
jitender.s21, suhyun_.kim, ih0923.kim, gaudium.lee, linux-input,
linux-kernel
In-Reply-To: <np2hnhy7jcc66rsfwym42qwteqda6slszinrjqb6jg7ie4qt3e@2fllxsza544p>
Hi Benjamin,
Thanks for the quick review our changes
As suggested we will refactor as below and send you an update shortly.
1. USB check validation moving to appropriate function
(samsung_kbd_mouse_input_mapping())
2. fix the checkpatch complain
3+ Split the remaining changes one per device
Clarifying for the Point 1 , below is the pseudocode:
static int samsung_kbd_mouse_input_mapping(struct hid_device *hdev,
struct hid_input *hi, struct hid_field *field, struct hid_usage *usage,
unsigned long **bit, int *max)
{
+ if (!hid_is_usb(hdev))
+ return 0;
...
}
Thanks & Regards
Sandeep C S
-----Original Message-----
From: Benjamin Tissoires <bentiss@kernel.org>
Sent: 24 July 2023 15:40
To: sandeep.cs <sandeep.cs@samsung.com>
Cc: Jiri Kosina <jikos@kernel.org>; Benjamin Tissoires
<benjamin.tissoires@redhat.com>; junwan.cho@samsung.com;
jitender.s21@samsung.com; suhyun_.kim@samsung.com; ih0923.kim@samsung.com;
gaudium.lee@samsung.com; linux-input@vger.kernel.org;
linux-kernel@vger.kernel.org
Subject: Re: [HID Patchsets v1 2/2] HID: Removed USB Validation check
Hi Sandeep,
On Jul 24 2023, sandeep.cs wrote:
> Earlier Samsung driver only handles USB HID devices and returns an error
if it encounters a Bluetooth type of HID device.
> By removing this USB validation check, we allow the driver to handle other
types of HID devices including Bluetooth HID devices, which were previously
excluded.
Please no, not with that patch at least.
hid_is_usb() protects the kernel from making an oops if the actual transport
layer is not USB, let's say an emulated uhid device. So by removing that
check you are just allowing anybody with root access to access random memory
in the kernel.
The correct fix is to move the check where it's needed, in
samsung_kbd_mouse_input_mapping().
I'll let you decide what need should be done if it's not a USB device
there: consider the interface to be 0 or just abort the function.
Cheers,
Benjamin
>
> This change improves driver compatibility and extends its support for a
wide range of devices.
>
> Signed-off-by: Sandeep C S<sandeep.cs@samsung.com>
> Signed-off-by: Junwan Cho <junwan.cho@samsung.com>
> Signed-off-by: Jitender Sajwan <jitender.s21@samsung.com>
> ---
> drivers/hid/hid-samsung.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/hid/hid-samsung.c b/drivers/hid/hid-samsung.c
> index 33e963303d11..3cafbf4d9dc6 100644
> --- a/drivers/hid/hid-samsung.c
> +++ b/drivers/hid/hid-samsung.c
> @@ -517,9 +517,6 @@ static int samsung_probe(struct hid_device *hdev,
> int ret;
> unsigned int cmask = HID_CONNECT_DEFAULT;
>
> - if (!hid_is_usb(hdev))
> - return -EINVAL;
> -
> ret = hid_parse(hdev);
> if (ret) {
> hid_err(hdev, "parse failed\n");
> --
> 2.25.1
>
^ permalink raw reply
* Re: [PATCH] Input: pwm-beeper - Support volume setting via sysfs
From: Marek Vasut @ 2023-07-31 11:49 UTC (permalink / raw)
To: Takashi Iwai, Dmitry Torokhov
Cc: Jeff LaBundy, linux-input, Uwe Kleine-König,
Frieder Schrempf, Manuel Traut, Thierry Reding, linux-pwm,
alsa-devel, Jaroslav Kysela, Takashi Iwai
In-Reply-To: <873514d2ju.wl-tiwai@suse.de>
On 7/31/23 08:21, Takashi Iwai wrote:
> On Mon, 31 Jul 2023 07:36:38 +0200,
> Dmitry Torokhov wrote:
>>
>> On Sat, May 13, 2023 at 11:02:30PM +0200, Marek Vasut wrote:
>>> On 5/13/23 03:51, Marek Vasut wrote:
>>>> On 5/13/23 03:12, Jeff LaBundy wrote:
>>>>> Hi Marek,
>>>>
>>>> Hi,
>>>>
>>>>> On Fri, May 12, 2023 at 08:55:51PM +0200, Marek Vasut wrote:
>>>>>> The PWM beeper volume can be controlled by adjusting the PWM duty cycle,
>>>>>> expose volume setting via sysfs, so users can make the beeper quieter.
>>>>>> This patch adds sysfs attribute 'volume' in range 0..50000, i.e. from 0
>>>>>> to 50% in 1/1000th of percent steps, this resolution should be
>>>>>> sufficient.
>>>>>>
>>>>>> The reason for 50000 cap on volume or PWM duty cycle is because
>>>>>> duty cycle
>>>>>> above 50% again reduces the loudness, the PWM wave form is inverted wave
>>>>>> form of the one for duty cycle below 50% and the beeper gets quieter the
>>>>>> closer the setting is to 100% . Hence, 50% cap where the wave
>>>>>> form yields
>>>>>> the loudest result.
>>>>>>
>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>> ---
>>>>>> An alternative option would be to extend the userspace input
>>>>>> ABI, e.g. by
>>>>>> using SND_TONE top 16bits to encode the duty cycle in 0..50000
>>>>>> range, and
>>>>>> bottom 16bit to encode the existing frequency in Hz . Since frequency in
>>>>>> Hz is likely to be below some 25 kHz for audible bell, this fits
>>>>>> in 16bits
>>>>>> just fine. Thoughts ?
>>>>>> ---
>>>>>
>>>>> Thanks for the patch; this seems like a useful feature.
>>>>>
>>>>> My first thought is that 50000 seems like an oddly specific limit to
>>>>> impose
>>>>> upon user space. Ideally, user space need not even care that the
>>>>> beeper is
>>>>> implemented via PWM and why 50000 is significant.
>>>>>
>>>>> Instead, what about accepting 0..255 as the LED subsystem does for
>>>>> brightness,
>>>>> then map these values to 0..50000 internally? In fact, the leds-pwm
>>>>> driver
>>>>> does something similar.
>>>>
>>>> The pwm_set_relative_duty_cycle() function can map whatever range to
>>>> whatever other range of the PWM already, so that's not an issues here.
>>>> It seems to me the 0..127 or 0..255 range is a bit too limiting . I
>>>> think even for the LEDs the reason for that limit is legacy design, but
>>>> here I might be wrong.
>>>>
>>>>> I'm also curious as to whether this function should be a rogue sysfs
>>>>> control
>>>>> limited to this driver, or a generic operation in input. For
>>>>> example, input
>>>>> already allows user space to specify the magnitude of an FF effect;
>>>>> perhaps
>>>>> something similar is warranted here?
>>>>
>>>> See the "An alternative ..." part above, I was wondering about this too,
>>>> whether this can be added into the input ABI, but I am somewhat
>>>> reluctant to fiddle with the ABI.
>>>
>>> Thinking about this further, we could try and add some
>>>
>>> EV_SND SND_TONE_WITH_VOLUME
>>>
>>> to avoid overloading EV_SND SND_TONE , and at the same time allow the user
>>> to set both frequency and volume for the tone without any race condition
>>> between the two.
>>>
>>> The EV_SND SND_TONE_WITH_VOLUME would still take one 32bit parameter, except
>>> this time the parameter 16 LSbits would be the frequency and 16 MSbits would
>>> be the volume.
>>>
>>> But again, here I would like input from the maintainers.
>>
>> Beeper was supposed to be an extremely simple device with minimal
>> controls. I wonder if there is need for volume controls, etc, etc are we
>> not better moving it over to the sound subsystem. We already have:
>>
>> sound/drivers/pcsp/pcsp.c
>>
>> and
>>
>> sound/pci/hda/hda_beep.c
>>
>> there, can we have other "advanced" beepers there as well? Adding sound
>> maintainers to CC...
>
> I don't mind it put to sound/*. But, note that pcsp.c you pointed in
> the above is a PCM tone generator driver with a PC beep device, and it
> provides the normal SND_BEEP input only for compatibility.
>
> Indeed there have been already many sound drivers providing the beep
> capability, and they bind with the input device using SND_BEEP. And,
> for the beep volume, "Beep Playback Volume" mixer control is provided,
> too.
Uh, I don't need a full sound device to emit beeps, that's not even
possible with this hardware. I only need to control loudness of the
beeper that is controlled by PWM output. That's why I am trying to
extend the pwm-beeper driver, which seems the best fit for such a
device, it is only missing this one feature (loudness control).
^ permalink raw reply
* Re: [PATCH] Input: pwm-beeper - Support volume setting via sysfs
From: Takashi Iwai @ 2023-07-31 12:15 UTC (permalink / raw)
To: Marek Vasut
Cc: Dmitry Torokhov, Jeff LaBundy, linux-input, Uwe Kleine-König,
Frieder Schrempf, Manuel Traut, Thierry Reding, linux-pwm,
alsa-devel, Jaroslav Kysela, Takashi Iwai
In-Reply-To: <63adce9a-df65-b462-9055-0ece5216d680@denx.de>
On Mon, 31 Jul 2023 13:49:46 +0200,
Marek Vasut wrote:
>
> On 7/31/23 08:21, Takashi Iwai wrote:
> > On Mon, 31 Jul 2023 07:36:38 +0200,
> > Dmitry Torokhov wrote:
> >>
> >> On Sat, May 13, 2023 at 11:02:30PM +0200, Marek Vasut wrote:
> >>> On 5/13/23 03:51, Marek Vasut wrote:
> >>>> On 5/13/23 03:12, Jeff LaBundy wrote:
> >>>>> Hi Marek,
> >>>>
> >>>> Hi,
> >>>>
> >>>>> On Fri, May 12, 2023 at 08:55:51PM +0200, Marek Vasut wrote:
> >>>>>> The PWM beeper volume can be controlled by adjusting the PWM duty cycle,
> >>>>>> expose volume setting via sysfs, so users can make the beeper quieter.
> >>>>>> This patch adds sysfs attribute 'volume' in range 0..50000, i.e. from 0
> >>>>>> to 50% in 1/1000th of percent steps, this resolution should be
> >>>>>> sufficient.
> >>>>>>
> >>>>>> The reason for 50000 cap on volume or PWM duty cycle is because
> >>>>>> duty cycle
> >>>>>> above 50% again reduces the loudness, the PWM wave form is inverted wave
> >>>>>> form of the one for duty cycle below 50% and the beeper gets quieter the
> >>>>>> closer the setting is to 100% . Hence, 50% cap where the wave
> >>>>>> form yields
> >>>>>> the loudest result.
> >>>>>>
> >>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>>>> ---
> >>>>>> An alternative option would be to extend the userspace input
> >>>>>> ABI, e.g. by
> >>>>>> using SND_TONE top 16bits to encode the duty cycle in 0..50000
> >>>>>> range, and
> >>>>>> bottom 16bit to encode the existing frequency in Hz . Since frequency in
> >>>>>> Hz is likely to be below some 25 kHz for audible bell, this fits
> >>>>>> in 16bits
> >>>>>> just fine. Thoughts ?
> >>>>>> ---
> >>>>>
> >>>>> Thanks for the patch; this seems like a useful feature.
> >>>>>
> >>>>> My first thought is that 50000 seems like an oddly specific limit to
> >>>>> impose
> >>>>> upon user space. Ideally, user space need not even care that the
> >>>>> beeper is
> >>>>> implemented via PWM and why 50000 is significant.
> >>>>>
> >>>>> Instead, what about accepting 0..255 as the LED subsystem does for
> >>>>> brightness,
> >>>>> then map these values to 0..50000 internally? In fact, the leds-pwm
> >>>>> driver
> >>>>> does something similar.
> >>>>
> >>>> The pwm_set_relative_duty_cycle() function can map whatever range to
> >>>> whatever other range of the PWM already, so that's not an issues here.
> >>>> It seems to me the 0..127 or 0..255 range is a bit too limiting . I
> >>>> think even for the LEDs the reason for that limit is legacy design, but
> >>>> here I might be wrong.
> >>>>
> >>>>> I'm also curious as to whether this function should be a rogue sysfs
> >>>>> control
> >>>>> limited to this driver, or a generic operation in input. For
> >>>>> example, input
> >>>>> already allows user space to specify the magnitude of an FF effect;
> >>>>> perhaps
> >>>>> something similar is warranted here?
> >>>>
> >>>> See the "An alternative ..." part above, I was wondering about this too,
> >>>> whether this can be added into the input ABI, but I am somewhat
> >>>> reluctant to fiddle with the ABI.
> >>>
> >>> Thinking about this further, we could try and add some
> >>>
> >>> EV_SND SND_TONE_WITH_VOLUME
> >>>
> >>> to avoid overloading EV_SND SND_TONE , and at the same time allow the user
> >>> to set both frequency and volume for the tone without any race condition
> >>> between the two.
> >>>
> >>> The EV_SND SND_TONE_WITH_VOLUME would still take one 32bit parameter, except
> >>> this time the parameter 16 LSbits would be the frequency and 16 MSbits would
> >>> be the volume.
> >>>
> >>> But again, here I would like input from the maintainers.
> >>
> >> Beeper was supposed to be an extremely simple device with minimal
> >> controls. I wonder if there is need for volume controls, etc, etc are we
> >> not better moving it over to the sound subsystem. We already have:
> >>
> >> sound/drivers/pcsp/pcsp.c
> >>
> >> and
> >>
> >> sound/pci/hda/hda_beep.c
> >>
> >> there, can we have other "advanced" beepers there as well? Adding sound
> >> maintainers to CC...
> >
> > I don't mind it put to sound/*. But, note that pcsp.c you pointed in
> > the above is a PCM tone generator driver with a PC beep device, and it
> > provides the normal SND_BEEP input only for compatibility.
> >
> > Indeed there have been already many sound drivers providing the beep
> > capability, and they bind with the input device using SND_BEEP. And,
> > for the beep volume, "Beep Playback Volume" mixer control is provided,
> > too.
>
> Uh, I don't need a full sound device to emit beeps, that's not even
> possible with this hardware.
Heh, I also don't recommend that route, either :)
(Though, it must be possible to create a sound device with that beep
control in theory)
> I only need to control loudness of the
> beeper that is controlled by PWM output. That's why I am trying to
> extend the pwm-beeper driver, which seems the best fit for such a
> device, it is only missing this one feature (loudness control).
So the question is what's expected from user-space POV. If a more
generic control of beep volume is required, e.g. for desktop-like
usages, an implementation of sound driver wouldn't be too bad.
OTOH, for other specific use-cases, it doesn't matter much in which
interface it's implemented, and sysfs could be an easy choice.
And, IMO, extending the SND_BEEP with a volume value doesn't sound
like a good idea.
thanks,
Takashi
^ permalink raw reply
* [PATCH -next] input: serio: fix the return value handle for platform_get_irq()
From: Ruan Jinjie @ 2023-07-31 12:22 UTC (permalink / raw)
To: dmitry.torokhov, wsa+renesas, christophe.jaillet, linux-input; +Cc: ruanjinjie
There is no possible for platform_get_irq() to return 0,
and the return value of platform_get_irq() is more sensible
to show the error reason.
Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com>
---
drivers/input/serio/rpckbd.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/input/serio/rpckbd.c b/drivers/input/serio/rpckbd.c
index ce420eb1f51b..e8a9709f32eb 100644
--- a/drivers/input/serio/rpckbd.c
+++ b/drivers/input/serio/rpckbd.c
@@ -101,12 +101,12 @@ static int rpckbd_probe(struct platform_device *dev)
int tx_irq, rx_irq;
rx_irq = platform_get_irq(dev, 0);
- if (rx_irq <= 0)
- return rx_irq < 0 ? rx_irq : -ENXIO;
+ if (rx_irq < 0)
+ return rx_irq;
tx_irq = platform_get_irq(dev, 1);
- if (tx_irq <= 0)
- return tx_irq < 0 ? tx_irq : -ENXIO;
+ if (tx_irq < 0)
+ return tx_irq;
serio = kzalloc(sizeof(struct serio), GFP_KERNEL);
rpckbd = kzalloc(sizeof(*rpckbd), GFP_KERNEL);
--
2.34.1
^ permalink raw reply related
* Re: [PATCH] Input: pwm-beeper - Support volume setting via sysfs
From: Marek Vasut @ 2023-07-31 14:05 UTC (permalink / raw)
To: Takashi Iwai
Cc: Dmitry Torokhov, Jeff LaBundy, linux-input, Uwe Kleine-König,
Frieder Schrempf, Manuel Traut, Thierry Reding, linux-pwm,
alsa-devel, Jaroslav Kysela, Takashi Iwai
In-Reply-To: <87tttkjmyu.wl-tiwai@suse.de>
On 7/31/23 14:15, Takashi Iwai wrote:
> On Mon, 31 Jul 2023 13:49:46 +0200,
> Marek Vasut wrote:
>>
>> On 7/31/23 08:21, Takashi Iwai wrote:
>>> On Mon, 31 Jul 2023 07:36:38 +0200,
>>> Dmitry Torokhov wrote:
>>>>
>>>> On Sat, May 13, 2023 at 11:02:30PM +0200, Marek Vasut wrote:
>>>>> On 5/13/23 03:51, Marek Vasut wrote:
>>>>>> On 5/13/23 03:12, Jeff LaBundy wrote:
>>>>>>> Hi Marek,
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>>> On Fri, May 12, 2023 at 08:55:51PM +0200, Marek Vasut wrote:
>>>>>>>> The PWM beeper volume can be controlled by adjusting the PWM duty cycle,
>>>>>>>> expose volume setting via sysfs, so users can make the beeper quieter.
>>>>>>>> This patch adds sysfs attribute 'volume' in range 0..50000, i.e. from 0
>>>>>>>> to 50% in 1/1000th of percent steps, this resolution should be
>>>>>>>> sufficient.
>>>>>>>>
>>>>>>>> The reason for 50000 cap on volume or PWM duty cycle is because
>>>>>>>> duty cycle
>>>>>>>> above 50% again reduces the loudness, the PWM wave form is inverted wave
>>>>>>>> form of the one for duty cycle below 50% and the beeper gets quieter the
>>>>>>>> closer the setting is to 100% . Hence, 50% cap where the wave
>>>>>>>> form yields
>>>>>>>> the loudest result.
>>>>>>>>
>>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>>>> ---
>>>>>>>> An alternative option would be to extend the userspace input
>>>>>>>> ABI, e.g. by
>>>>>>>> using SND_TONE top 16bits to encode the duty cycle in 0..50000
>>>>>>>> range, and
>>>>>>>> bottom 16bit to encode the existing frequency in Hz . Since frequency in
>>>>>>>> Hz is likely to be below some 25 kHz for audible bell, this fits
>>>>>>>> in 16bits
>>>>>>>> just fine. Thoughts ?
>>>>>>>> ---
>>>>>>>
>>>>>>> Thanks for the patch; this seems like a useful feature.
>>>>>>>
>>>>>>> My first thought is that 50000 seems like an oddly specific limit to
>>>>>>> impose
>>>>>>> upon user space. Ideally, user space need not even care that the
>>>>>>> beeper is
>>>>>>> implemented via PWM and why 50000 is significant.
>>>>>>>
>>>>>>> Instead, what about accepting 0..255 as the LED subsystem does for
>>>>>>> brightness,
>>>>>>> then map these values to 0..50000 internally? In fact, the leds-pwm
>>>>>>> driver
>>>>>>> does something similar.
>>>>>>
>>>>>> The pwm_set_relative_duty_cycle() function can map whatever range to
>>>>>> whatever other range of the PWM already, so that's not an issues here.
>>>>>> It seems to me the 0..127 or 0..255 range is a bit too limiting . I
>>>>>> think even for the LEDs the reason for that limit is legacy design, but
>>>>>> here I might be wrong.
>>>>>>
>>>>>>> I'm also curious as to whether this function should be a rogue sysfs
>>>>>>> control
>>>>>>> limited to this driver, or a generic operation in input. For
>>>>>>> example, input
>>>>>>> already allows user space to specify the magnitude of an FF effect;
>>>>>>> perhaps
>>>>>>> something similar is warranted here?
>>>>>>
>>>>>> See the "An alternative ..." part above, I was wondering about this too,
>>>>>> whether this can be added into the input ABI, but I am somewhat
>>>>>> reluctant to fiddle with the ABI.
>>>>>
>>>>> Thinking about this further, we could try and add some
>>>>>
>>>>> EV_SND SND_TONE_WITH_VOLUME
>>>>>
>>>>> to avoid overloading EV_SND SND_TONE , and at the same time allow the user
>>>>> to set both frequency and volume for the tone without any race condition
>>>>> between the two.
>>>>>
>>>>> The EV_SND SND_TONE_WITH_VOLUME would still take one 32bit parameter, except
>>>>> this time the parameter 16 LSbits would be the frequency and 16 MSbits would
>>>>> be the volume.
>>>>>
>>>>> But again, here I would like input from the maintainers.
>>>>
>>>> Beeper was supposed to be an extremely simple device with minimal
>>>> controls. I wonder if there is need for volume controls, etc, etc are we
>>>> not better moving it over to the sound subsystem. We already have:
>>>>
>>>> sound/drivers/pcsp/pcsp.c
>>>>
>>>> and
>>>>
>>>> sound/pci/hda/hda_beep.c
>>>>
>>>> there, can we have other "advanced" beepers there as well? Adding sound
>>>> maintainers to CC...
>>>
>>> I don't mind it put to sound/*. But, note that pcsp.c you pointed in
>>> the above is a PCM tone generator driver with a PC beep device, and it
>>> provides the normal SND_BEEP input only for compatibility.
>>>
>>> Indeed there have been already many sound drivers providing the beep
>>> capability, and they bind with the input device using SND_BEEP. And,
>>> for the beep volume, "Beep Playback Volume" mixer control is provided,
>>> too.
>>
>> Uh, I don't need a full sound device to emit beeps, that's not even
>> possible with this hardware.
>
> Heh, I also don't recommend that route, either :)
> (Though, it must be possible to create a sound device with that beep
> control in theory)
I mean, I can imagine one could possibly use PCM DMA to cook samples to
feed some of the PWM devices so they could possibly be used to generate
low quality audio, as a weird limited DAC, but ... that's not really
generic, and not what I want.
>> I only need to control loudness of the
>> beeper that is controlled by PWM output. That's why I am trying to
>> extend the pwm-beeper driver, which seems the best fit for such a
>> device, it is only missing this one feature (loudness control).
>
> So the question is what's expected from user-space POV. If a more
> generic control of beep volume is required, e.g. for desktop-like
> usages, an implementation of sound driver wouldn't be too bad.
> OTOH, for other specific use-cases, it doesn't matter much in which
> interface it's implemented, and sysfs could be an easy choice.
The whole discussion above has been exactly about this. Basically the
thing is, we can either have:
- SND_TONE (via some /dev/input/eventX) + sysfs volume control
-> This is simple, but sounds racy between input and sysfs accesses
- SND_TONE + SND_TONE_SET_VOLUME
-> User needs to do two ioctls, hum
- some new SND_TONE_WITH_VOLUME
-> Probably the best option, user sets both tone frequency and volume
in one go, and it also only extends the IOCTL interface, so older
userspace won't have issues
> And, IMO, extending the SND_BEEP with a volume value doesn't sound
> like a good idea.
No, it doesn't, but see SND_TONE_WITH_VOLUME option above. I think that
might be the best so far.
^ permalink raw reply
* Re: [PATCH] Input: pwm-beeper - Support volume setting via sysfs
From: Takashi Iwai @ 2023-07-31 14:20 UTC (permalink / raw)
To: Marek Vasut
Cc: Dmitry Torokhov, Jeff LaBundy, linux-input, Uwe Kleine-König,
Frieder Schrempf, Manuel Traut, Thierry Reding, linux-pwm,
alsa-devel, Jaroslav Kysela, Takashi Iwai
In-Reply-To: <0cffe366-75af-d8a8-8920-6fb94c321a89@denx.de>
On Mon, 31 Jul 2023 16:05:18 +0200,
Marek Vasut wrote:
>
> On 7/31/23 14:15, Takashi Iwai wrote:
> > On Mon, 31 Jul 2023 13:49:46 +0200,
> > Marek Vasut wrote:
> >>
> >> On 7/31/23 08:21, Takashi Iwai wrote:
> >>> On Mon, 31 Jul 2023 07:36:38 +0200,
> >>> Dmitry Torokhov wrote:
> >>>>
> >>>> On Sat, May 13, 2023 at 11:02:30PM +0200, Marek Vasut wrote:
> >>>>> On 5/13/23 03:51, Marek Vasut wrote:
> >>>>>> On 5/13/23 03:12, Jeff LaBundy wrote:
> >>>>>>> Hi Marek,
> >>>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>>> On Fri, May 12, 2023 at 08:55:51PM +0200, Marek Vasut wrote:
> >>>>>>>> The PWM beeper volume can be controlled by adjusting the PWM duty cycle,
> >>>>>>>> expose volume setting via sysfs, so users can make the beeper quieter.
> >>>>>>>> This patch adds sysfs attribute 'volume' in range 0..50000, i.e. from 0
> >>>>>>>> to 50% in 1/1000th of percent steps, this resolution should be
> >>>>>>>> sufficient.
> >>>>>>>>
> >>>>>>>> The reason for 50000 cap on volume or PWM duty cycle is because
> >>>>>>>> duty cycle
> >>>>>>>> above 50% again reduces the loudness, the PWM wave form is inverted wave
> >>>>>>>> form of the one for duty cycle below 50% and the beeper gets quieter the
> >>>>>>>> closer the setting is to 100% . Hence, 50% cap where the wave
> >>>>>>>> form yields
> >>>>>>>> the loudest result.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>>>>>> ---
> >>>>>>>> An alternative option would be to extend the userspace input
> >>>>>>>> ABI, e.g. by
> >>>>>>>> using SND_TONE top 16bits to encode the duty cycle in 0..50000
> >>>>>>>> range, and
> >>>>>>>> bottom 16bit to encode the existing frequency in Hz . Since frequency in
> >>>>>>>> Hz is likely to be below some 25 kHz for audible bell, this fits
> >>>>>>>> in 16bits
> >>>>>>>> just fine. Thoughts ?
> >>>>>>>> ---
> >>>>>>>
> >>>>>>> Thanks for the patch; this seems like a useful feature.
> >>>>>>>
> >>>>>>> My first thought is that 50000 seems like an oddly specific limit to
> >>>>>>> impose
> >>>>>>> upon user space. Ideally, user space need not even care that the
> >>>>>>> beeper is
> >>>>>>> implemented via PWM and why 50000 is significant.
> >>>>>>>
> >>>>>>> Instead, what about accepting 0..255 as the LED subsystem does for
> >>>>>>> brightness,
> >>>>>>> then map these values to 0..50000 internally? In fact, the leds-pwm
> >>>>>>> driver
> >>>>>>> does something similar.
> >>>>>>
> >>>>>> The pwm_set_relative_duty_cycle() function can map whatever range to
> >>>>>> whatever other range of the PWM already, so that's not an issues here.
> >>>>>> It seems to me the 0..127 or 0..255 range is a bit too limiting . I
> >>>>>> think even for the LEDs the reason for that limit is legacy design, but
> >>>>>> here I might be wrong.
> >>>>>>
> >>>>>>> I'm also curious as to whether this function should be a rogue sysfs
> >>>>>>> control
> >>>>>>> limited to this driver, or a generic operation in input. For
> >>>>>>> example, input
> >>>>>>> already allows user space to specify the magnitude of an FF effect;
> >>>>>>> perhaps
> >>>>>>> something similar is warranted here?
> >>>>>>
> >>>>>> See the "An alternative ..." part above, I was wondering about this too,
> >>>>>> whether this can be added into the input ABI, but I am somewhat
> >>>>>> reluctant to fiddle with the ABI.
> >>>>>
> >>>>> Thinking about this further, we could try and add some
> >>>>>
> >>>>> EV_SND SND_TONE_WITH_VOLUME
> >>>>>
> >>>>> to avoid overloading EV_SND SND_TONE , and at the same time allow the user
> >>>>> to set both frequency and volume for the tone without any race condition
> >>>>> between the two.
> >>>>>
> >>>>> The EV_SND SND_TONE_WITH_VOLUME would still take one 32bit parameter, except
> >>>>> this time the parameter 16 LSbits would be the frequency and 16 MSbits would
> >>>>> be the volume.
> >>>>>
> >>>>> But again, here I would like input from the maintainers.
> >>>>
> >>>> Beeper was supposed to be an extremely simple device with minimal
> >>>> controls. I wonder if there is need for volume controls, etc, etc are we
> >>>> not better moving it over to the sound subsystem. We already have:
> >>>>
> >>>> sound/drivers/pcsp/pcsp.c
> >>>>
> >>>> and
> >>>>
> >>>> sound/pci/hda/hda_beep.c
> >>>>
> >>>> there, can we have other "advanced" beepers there as well? Adding sound
> >>>> maintainers to CC...
> >>>
> >>> I don't mind it put to sound/*. But, note that pcsp.c you pointed in
> >>> the above is a PCM tone generator driver with a PC beep device, and it
> >>> provides the normal SND_BEEP input only for compatibility.
> >>>
> >>> Indeed there have been already many sound drivers providing the beep
> >>> capability, and they bind with the input device using SND_BEEP. And,
> >>> for the beep volume, "Beep Playback Volume" mixer control is provided,
> >>> too.
> >>
> >> Uh, I don't need a full sound device to emit beeps, that's not even
> >> possible with this hardware.
> >
> > Heh, I also don't recommend that route, either :)
> > (Though, it must be possible to create a sound device with that beep
> > control in theory)
>
> I mean, I can imagine one could possibly use PCM DMA to cook samples
> to feed some of the PWM devices so they could possibly be used to
> generate low quality audio, as a weird limited DAC, but ... that's not
> really generic, and not what I want.
Oh I see how the misunderstanding came; I didn't mean the PCM
implementation like pcsp driver. The pcsp driver is a real hack and
it's there just for fun, not for any real practical use.
What I meant was rather that you can create a sound device containing
a mixer volume control that serves exactly like the sysfs or whatever
other interface, without any PCM stream or other interface.
> >> I only need to control loudness of the
> >> beeper that is controlled by PWM output. That's why I am trying to
> >> extend the pwm-beeper driver, which seems the best fit for such a
> >> device, it is only missing this one feature (loudness control).
> >
> > So the question is what's expected from user-space POV. If a more
> > generic control of beep volume is required, e.g. for desktop-like
> > usages, an implementation of sound driver wouldn't be too bad.
> > OTOH, for other specific use-cases, it doesn't matter much in which
> > interface it's implemented, and sysfs could be an easy choice.
>
> The whole discussion above has been exactly about this. Basically the
> thing is, we can either have:
> - SND_TONE (via some /dev/input/eventX) + sysfs volume control
> -> This is simple, but sounds racy between input and sysfs accesses
Hmm, how can it be racy if you do proper locking?
> - SND_TONE + SND_TONE_SET_VOLUME
> -> User needs to do two ioctls, hum
> - some new SND_TONE_WITH_VOLUME
> -> Probably the best option, user sets both tone frequency and volume
> in one go, and it also only extends the IOCTL interface, so older
> userspace won't have issues
Those are "extensions" I have mentioned, and I'm not a big fan for
that, honestly speaking.
The fact that the beep *output* stuff is provided by the *input*
device is already confusing (it was so just because of historical
reason), and yet you start implementing more full-featured mixer
control. I'd rather keep fingers away.
Again, if user-space requires the compatible behavior like the
existing desktop usages, it can be implemented in a similar way like
the existing ones; i.e. provide a mixer control with a proper sound
device. The sound device doesn't need to provide a PCM interface but
just with a mixer interface.
Or, if the purpose of your target device is a special usage, you don't
need to consider too much about the existing interface, and try to
keep the change as minimal as possible without too intrusive API
changes.
Takashi
^ permalink raw reply
* Re: [PATCH] Input: pwm-beeper - Support volume setting via sysfs
From: Marek Vasut @ 2023-07-31 14:36 UTC (permalink / raw)
To: Takashi Iwai
Cc: Dmitry Torokhov, Jeff LaBundy, linux-input, Uwe Kleine-König,
Frieder Schrempf, Manuel Traut, Thierry Reding, linux-pwm,
alsa-devel, Jaroslav Kysela, Takashi Iwai
In-Reply-To: <87h6pkjh7q.wl-tiwai@suse.de>
On 7/31/23 16:20, Takashi Iwai wrote:
[...]
>>>> Uh, I don't need a full sound device to emit beeps, that's not even
>>>> possible with this hardware.
>>>
>>> Heh, I also don't recommend that route, either :)
>>> (Though, it must be possible to create a sound device with that beep
>>> control in theory)
>>
>> I mean, I can imagine one could possibly use PCM DMA to cook samples
>> to feed some of the PWM devices so they could possibly be used to
>> generate low quality audio, as a weird limited DAC, but ... that's not
>> really generic, and not what I want.
>
> Oh I see how the misunderstanding came; I didn't mean the PCM
> implementation like pcsp driver. The pcsp driver is a real hack and
> it's there just for fun, not for any real practical use.
Ah :)
> What I meant was rather that you can create a sound device containing
> a mixer volume control that serves exactly like the sysfs or whatever
> other interface, without any PCM stream or other interface.
Ahhh, hum, I still feel like this might be a bit overkill here.
>>>> I only need to control loudness of the
>>>> beeper that is controlled by PWM output. That's why I am trying to
>>>> extend the pwm-beeper driver, which seems the best fit for such a
>>>> device, it is only missing this one feature (loudness control).
>>>
>>> So the question is what's expected from user-space POV. If a more
>>> generic control of beep volume is required, e.g. for desktop-like
>>> usages, an implementation of sound driver wouldn't be too bad.
>>> OTOH, for other specific use-cases, it doesn't matter much in which
>>> interface it's implemented, and sysfs could be an easy choice.
>>
>> The whole discussion above has been exactly about this. Basically the
>> thing is, we can either have:
>> - SND_TONE (via some /dev/input/eventX) + sysfs volume control
>> -> This is simple, but sounds racy between input and sysfs accesses
>
> Hmm, how can it be racy if you do proper locking?
I can imagine two applications can each grab one of the controls and
that makes the interface a bit not nice. That would require extra
synchronization in userspace and so on.
>> - SND_TONE + SND_TONE_SET_VOLUME
>> -> User needs to do two ioctls, hum
>> - some new SND_TONE_WITH_VOLUME
>> -> Probably the best option, user sets both tone frequency and volume
>> in one go, and it also only extends the IOCTL interface, so older
>> userspace won't have issues
>
> Those are "extensions" I have mentioned, and I'm not a big fan for
> that, honestly speaking.
>
> The fact that the beep *output* stuff is provided by the *input*
> device is already confusing
I agree, this confused me as well.
> (it was so just because of historical
> reason), and yet you start implementing more full-featured mixer
> control. I'd rather keep fingers away.
>
> Again, if user-space requires the compatible behavior like the
> existing desktop usages
It does not. These pwm-beeper devices keep showing up in various
embedded devices these days.
>, it can be implemented in a similar way like
> the existing ones; i.e. provide a mixer control with a proper sound
> device. The sound device doesn't need to provide a PCM interface but
> just with a mixer interface.
>
> Or, if the purpose of your target device is a special usage, you don't
> need to consider too much about the existing interface, and try to
> keep the change as minimal as possible without too intrusive API
> changes.
My use case is almost perfectly matched by the current input pwm-beeper
driver, the only missing bit is the ability to control the loudness at
runtime. I think adding the SND_TONE_WITH_VOLUME parameter would cover
it, with least intrusive API changes.
The SND_TONE already supports configuring tone frequency in Hz as its
parameter. Since anything above 64 kHz is certainly not hearable by
humans, I would say the SND_TONE_WITH_VOLUME could use 16 LSbits for
frequency (so up to 65535 Hz , 0 is OFF), and 16 MSbits for volume .
I'm hesitant to overcomplicate something which can currently be
controlled via single ioctl by pulling in sound subsystem into the picture.
^ permalink raw reply
* Re: [PATCH 03/22] Input: iqs269a - use device core to create driver-specific device attributes
From: Jeff LaBundy @ 2023-07-31 15:41 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input, linux-kernel, Greg Kroah-Hartman,
Mattijs Korpershoek
In-Reply-To: <20230729005133.1095051-3-dmitry.torokhov@gmail.com>
On Fri, Jul 28, 2023 at 05:51:12PM -0700, Dmitry Torokhov wrote:
> Instead of creating driver-specific device attributes with
> devm_device_add_group() have device core do this by setting up dev_groups
> pointer in the driver structure.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
> ---
> drivers/input/misc/iqs269a.c | 10 ++--------
> 1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/input/misc/iqs269a.c b/drivers/input/misc/iqs269a.c
> index c0a085639870..3c636c75e8a1 100644
> --- a/drivers/input/misc/iqs269a.c
> +++ b/drivers/input/misc/iqs269a.c
> @@ -1586,10 +1586,7 @@ static struct attribute *iqs269_attrs[] = {
> &dev_attr_ati_trigger.attr,
> NULL,
> };
> -
> -static const struct attribute_group iqs269_attr_group = {
> - .attrs = iqs269_attrs,
> -};
> +ATTRIBUTE_GROUPS(iqs269);
>
> static const struct regmap_config iqs269_regmap_config = {
> .reg_bits = 8,
> @@ -1671,10 +1668,6 @@ static int iqs269_probe(struct i2c_client *client)
> return error;
> }
>
> - error = devm_device_add_group(&client->dev, &iqs269_attr_group);
> - if (error)
> - dev_err(&client->dev, "Failed to add attributes: %d\n", error);
> -
> return error;
> }
>
> @@ -1743,6 +1736,7 @@ MODULE_DEVICE_TABLE(of, iqs269_of_match);
> static struct i2c_driver iqs269_i2c_driver = {
> .driver = {
> .name = "iqs269a",
> + .dev_groups = iqs269_groups,
> .of_match_table = iqs269_of_match,
> .pm = pm_sleep_ptr(&iqs269_pm),
> },
> --
> 2.41.0.487.g6d72f3e995-goog
>
^ permalink raw reply
* Re: [PATCH 15/22] Input: iqs5xx - use device core to create driver-specific device attributes
From: Jeff LaBundy @ 2023-07-31 15:43 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, Greg Kroah-Hartman
In-Reply-To: <20230729005133.1095051-15-dmitry.torokhov@gmail.com>
On Fri, Jul 28, 2023 at 05:51:24PM -0700, Dmitry Torokhov wrote:
> Instead of creating driver-specific device attributes with
> devm_device_add_group() have device core do this by setting up dev_groups
> pointer in the driver structure.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
> ---
> drivers/input/touchscreen/iqs5xx.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/input/touchscreen/iqs5xx.c b/drivers/input/touchscreen/iqs5xx.c
> index b4768b66eb10..a3f4fb85bee5 100644
> --- a/drivers/input/touchscreen/iqs5xx.c
> +++ b/drivers/input/touchscreen/iqs5xx.c
> @@ -974,10 +974,11 @@ static umode_t iqs5xx_attr_is_visible(struct kobject *kobj,
> return attr->mode;
> }
>
> -static const struct attribute_group iqs5xx_attr_group = {
> +static const struct attribute_group iqs5xx_group = {
> .is_visible = iqs5xx_attr_is_visible,
> .attrs = iqs5xx_attrs,
> };
> +__ATTRIBUTE_GROUPS(iqs5xx);
>
> static int iqs5xx_suspend(struct device *dev)
> {
> @@ -1053,12 +1054,6 @@ static int iqs5xx_probe(struct i2c_client *client)
> return error;
> }
>
> - error = devm_device_add_group(&client->dev, &iqs5xx_attr_group);
> - if (error) {
> - dev_err(&client->dev, "Failed to add attributes: %d\n", error);
> - return error;
> - }
> -
> if (iqs5xx->input) {
> error = input_register_device(iqs5xx->input);
> if (error)
> @@ -1089,6 +1084,7 @@ MODULE_DEVICE_TABLE(of, iqs5xx_of_match);
> static struct i2c_driver iqs5xx_i2c_driver = {
> .driver = {
> .name = "iqs5xx",
> + .dev_groups = iqs5xx_groups,
> .of_match_table = iqs5xx_of_match,
> .pm = pm_sleep_ptr(&iqs5xx_pm),
> },
> --
> 2.41.0.487.g6d72f3e995-goog
>
^ permalink raw reply
* Re: [PATCH 04/10] input: serport: remove casts from tty->disc_data
From: Dmitry Torokhov @ 2023-07-31 16:18 UTC (permalink / raw)
To: Jiri Slaby (SUSE); +Cc: gregkh, linux-serial, linux-kernel, linux-input
In-Reply-To: <20230731080244.2698-5-jirislaby@kernel.org>
On Mon, Jul 31, 2023 at 10:02:38AM +0200, Jiri Slaby (SUSE) wrote:
> tty->disc_data is 'void *', so there is no need to cast from that.
> Therefore remove the casts and assign the pointer directly.
>
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: linux-input@vger.kernel.org
Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: pwm-beeper - Support volume setting via sysfs
From: Dmitry Torokhov @ 2023-07-31 16:24 UTC (permalink / raw)
To: Marek Vasut
Cc: Takashi Iwai, Jeff LaBundy, linux-input, Uwe Kleine-König,
Frieder Schrempf, Manuel Traut, Thierry Reding, linux-pwm,
alsa-devel, Jaroslav Kysela, Takashi Iwai
In-Reply-To: <618add56-3675-4efe-5b20-665c10040e03@denx.de>
On Mon, Jul 31, 2023 at 04:36:01PM +0200, Marek Vasut wrote:
> On 7/31/23 16:20, Takashi Iwai wrote:
>
> [...]
>
> > > > > Uh, I don't need a full sound device to emit beeps, that's not even
> > > > > possible with this hardware.
> > > >
> > > > Heh, I also don't recommend that route, either :)
> > > > (Though, it must be possible to create a sound device with that beep
> > > > control in theory)
> > >
> > > I mean, I can imagine one could possibly use PCM DMA to cook samples
> > > to feed some of the PWM devices so they could possibly be used to
> > > generate low quality audio, as a weird limited DAC, but ... that's not
> > > really generic, and not what I want.
> >
> > Oh I see how the misunderstanding came; I didn't mean the PCM
> > implementation like pcsp driver. The pcsp driver is a real hack and
> > it's there just for fun, not for any real practical use.
>
> Ah :)
>
> > What I meant was rather that you can create a sound device containing
> > a mixer volume control that serves exactly like the sysfs or whatever
> > other interface, without any PCM stream or other interface.
>
> Ahhh, hum, I still feel like this might be a bit overkill here.
>
> > > > > I only need to control loudness of the
> > > > > beeper that is controlled by PWM output. That's why I am trying to
> > > > > extend the pwm-beeper driver, which seems the best fit for such a
> > > > > device, it is only missing this one feature (loudness control).
> > > >
> > > > So the question is what's expected from user-space POV. If a more
> > > > generic control of beep volume is required, e.g. for desktop-like
> > > > usages, an implementation of sound driver wouldn't be too bad.
> > > > OTOH, for other specific use-cases, it doesn't matter much in which
> > > > interface it's implemented, and sysfs could be an easy choice.
> > >
> > > The whole discussion above has been exactly about this. Basically the
> > > thing is, we can either have:
> > > - SND_TONE (via some /dev/input/eventX) + sysfs volume control
> > > -> This is simple, but sounds racy between input and sysfs accesses
> >
> > Hmm, how can it be racy if you do proper locking?
>
> I can imagine two applications can each grab one of the controls and that
> makes the interface a bit not nice. That would require extra synchronization
> in userspace and so on.
>
> > > - SND_TONE + SND_TONE_SET_VOLUME
> > > -> User needs to do two ioctls, hum
> > > - some new SND_TONE_WITH_VOLUME
> > > -> Probably the best option, user sets both tone frequency and volume
> > > in one go, and it also only extends the IOCTL interface, so older
> > > userspace won't have issues
> >
> > Those are "extensions" I have mentioned, and I'm not a big fan for
> > that, honestly speaking.
> >
> > The fact that the beep *output* stuff is provided by the *input*
> > device is already confusing
>
> I agree, this confused me as well.
This comes from the times when keyboards themselves were capable of
emitting bells (SUN, DEC, etc). In hindsight it was not the best way of
structuring things, same with the keyboard LEDs (that are now plugged
into the LED subsystem, but still allow be driven through input).
And in the same vein I wonder if we should bite the bullet and pay with
a bit of complexity but move sound-related things to sound subsystem.
>
> > (it was so just because of historical
> > reason), and yet you start implementing more full-featured mixer
> > control. I'd rather keep fingers away.
> >
> > Again, if user-space requires the compatible behavior like the
> > existing desktop usages
>
> It does not. These pwm-beeper devices keep showing up in various embedded
> devices these days.
>
> > , it can be implemented in a similar way like
> > the existing ones; i.e. provide a mixer control with a proper sound
> > device. The sound device doesn't need to provide a PCM interface but
> > just with a mixer interface.
> >
> > Or, if the purpose of your target device is a special usage, you don't
> > need to consider too much about the existing interface, and try to
> > keep the change as minimal as possible without too intrusive API
> > changes.
>
> My use case is almost perfectly matched by the current input pwm-beeper
> driver, the only missing bit is the ability to control the loudness at
> runtime. I think adding the SND_TONE_WITH_VOLUME parameter would cover it,
> with least intrusive API changes.
>
> The SND_TONE already supports configuring tone frequency in Hz as its
> parameter. Since anything above 64 kHz is certainly not hearable by humans,
> I would say the SND_TONE_WITH_VOLUME could use 16 LSbits for frequency (so
> up to 65535 Hz , 0 is OFF), and 16 MSbits for volume .
>
> I'm hesitant to overcomplicate something which can currently be controlled
> via single ioctl by pulling in sound subsystem into the picture.
Can you tell a bit more about your use case? What needs to control the
volume of beeps? Is this the only source of sounds on the system?
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v3 02/10] drm/panel: Check for already prepared/enabled in drm_panel
From: Chris Morgan @ 2023-07-31 16:33 UTC (permalink / raw)
To: Maxime Ripard
Cc: Doug Anderson, Jiri Kosina, Benjamin Tissoires, Bjorn Andersson,
Konrad Dybcio, Rob Herring, Frank Rowand, Krzysztof Kozlowski,
Conor Dooley, Neil Armstrong, Sam Ravnborg, Maarten Lankhorst,
Thomas Zimmermann, cros-qcom-dts-watchers, linux-input, hsinyi,
linux-arm-msm, dri-devel, linux-kernel, Dmitry Torokhov,
devicetree, Daniel Vetter, yangcong5
In-Reply-To: <i3cbgrc365lwscwux2itho6uv74ji3hsjuge4zoxfnlnhacyqc@r73mmifyxffj>
In my case a few different panel drivers disable the regulators in the
unprepare/disable routines. For at least the Rockchip DSI
implementations for some reason the panel gets unprepared more than
once, which triggers an unbalanced regulator disable. Obviously though
the correct course of action is to fix the reason why the panel is
disabled more than once, but that's at least the root cause of this
behavior on the few panels I've worked with.
Thank you.
On Thu, Jul 27, 2023 at 1:38 AM Maxime Ripard <mripard@kernel.org> wrote:
>
> Hi,
>
> On Wed, Jul 26, 2023 at 08:10:33AM -0700, Doug Anderson wrote:
> > On Wed, Jul 26, 2023 at 5:41 AM Maxime Ripard <mripard@kernel.org> wrote:
> > > On Tue, Jul 25, 2023 at 01:34:37PM -0700, Douglas Anderson wrote:
> > > > NOTE: arguably, the right thing to do here is actually to skip this
> > > > patch and simply remove all the extra checks from the individual
> > > > drivers. Perhaps the checks were needed at some point in time in the
> > > > past but maybe they no longer are? Certainly as we continue
> > > > transitioning over to "panel_bridge" then we expect there to be much
> > > > less variety in how these calls are made. When we're called as part of
> > > > the bridge chain, things should be pretty simple. In fact, there was
> > > > some discussion in the past about these checks [1], including a
> > > > discussion about whether the checks were needed and whether the calls
> > > > ought to be refcounted. At the time, I decided not to mess with it
> > > > because it felt too risky.
> > >
> > > Yeah, I'd agree here too. I've never found evidence that it was actually
> > > needed and it really looks like cargo cult to me.
> > >
> > > And if it was needed, then I'm not sure we need refcounting either. We
> > > don't have refcounting for atomic_enable / disable, we have a sound API
> > > design that makes sure we don't fall into that trap :)
> > >
> > > > Looking closer at it now, I'm fairly certain that nothing in the
> > > > existing codebase is expecting these calls to be refcounted. The only
> > > > real question is whether someone is already doing something to ensure
> > > > prepare()/unprepare() match and enabled()/disable() match. I would say
> > > > that, even if there is something else ensuring that things match,
> > > > there's enough complexity that adding an extra bool and an extra
> > > > double-check here is a good idea. Let's add a drm_warn() to let people
> > > > know that it's considered a minor error to take advantage of
> > > > drm_panel's double-checking but we'll still make things work fine.
> > >
> > > I'm ok with this, if we follow-up in a couple of releases and remove it
> > > and all the calls.
> > >
> > > Could you add a TODO item so that we can keep a track of it? A follow-up
> > > is fine if you don't send a new version of that series.
> >
> > By this, I think you mean to add a "TODO" comment inline in the code?
>
> No, sorry, I meant an entry in our TODO list: Documentation/gpu/todo.rst
>
> > Also: I was thinking that we'd keep the check in "drm_panel.c" with
> > the warning message indefinitely. You think it should be eventually
> > removed? If we are truly thinking of removing it eventually, this
> > feels like it should be a more serious warning message like a WARN(1,
> > ...) to make it really obvious to people that they're relying on
> > behavior that will eventually go away.
>
> Yeah, it really feels like this is cargo-cult to me. Your approach seems
> like a good short-term thing to do to warn everyone but eventually we'll
> want it to go away.
>
> So promoting it to a WARN could be a good thing, or let's say we do a
> drm_warn for now, WARN next release, and gone in two?
>
> Maxime
^ permalink raw reply
* Re: [PATCH v3 02/10] drm/panel: Check for already prepared/enabled in drm_panel
From: Maxime Ripard @ 2023-07-31 17:03 UTC (permalink / raw)
To: Chris Morgan
Cc: Doug Anderson, Jiri Kosina, Benjamin Tissoires, Bjorn Andersson,
Konrad Dybcio, Rob Herring, Frank Rowand, Krzysztof Kozlowski,
Conor Dooley, Neil Armstrong, Sam Ravnborg, Maarten Lankhorst,
Thomas Zimmermann, cros-qcom-dts-watchers, linux-input, hsinyi,
linux-arm-msm, dri-devel, linux-kernel, Dmitry Torokhov,
devicetree, Daniel Vetter, yangcong5
In-Reply-To: <CADcbR4JB0h8fByM2Z6diByvWaFprW9GDapBNt+YLWr9-vKoe7A@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1176 bytes --]
Hi,
On Mon, Jul 31, 2023 at 11:33:22AM -0500, Chris Morgan wrote:
> In my case a few different panel drivers disable the regulators in the
> unprepare/disable routines.
And that's totally fine.
> For at least the Rockchip DSI implementations for some reason the
> panel gets unprepared more than once, which triggers an unbalanced
> regulator disable.
"For some reason" being that DW-DSI apparently finds it ok to bypass any
kind of abstraction and randomly calling panel functions by itself:
https://elixir.bootlin.com/linux/v6.4.7/source/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L868
It looks like it's fixed it current drm-misc-next though.
> Obviously though the correct course of action is to fix the reason why
> the panel is disabled more than once, but that's at least the root
> cause of this behavior on the few panels I've worked with.
Like I said we already have a commit on the way to fix that, so it
shouldn't be an issue anymore.
I stand by what I was saying earlier though, I think it's mostly
cargo-cult or drivers being very wrong. If anything, the DW-DSI stuff
made me even more convinced that we shouldn't even entertain that idea
:)
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH] Input: pwm-beeper - Support volume setting via sysfs
From: Marek Vasut @ 2023-07-31 17:49 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Takashi Iwai, Jeff LaBundy, linux-input, Uwe Kleine-König,
Frieder Schrempf, Manuel Traut, Thierry Reding, linux-pwm,
alsa-devel, Jaroslav Kysela, Takashi Iwai
In-Reply-To: <ZMfgJ3o00nApkXGp@google.com>
On 7/31/23 18:24, Dmitry Torokhov wrote:
> On Mon, Jul 31, 2023 at 04:36:01PM +0200, Marek Vasut wrote:
>> On 7/31/23 16:20, Takashi Iwai wrote:
>>
>> [...]
>>
>>>>>> Uh, I don't need a full sound device to emit beeps, that's not even
>>>>>> possible with this hardware.
>>>>>
>>>>> Heh, I also don't recommend that route, either :)
>>>>> (Though, it must be possible to create a sound device with that beep
>>>>> control in theory)
>>>>
>>>> I mean, I can imagine one could possibly use PCM DMA to cook samples
>>>> to feed some of the PWM devices so they could possibly be used to
>>>> generate low quality audio, as a weird limited DAC, but ... that's not
>>>> really generic, and not what I want.
>>>
>>> Oh I see how the misunderstanding came; I didn't mean the PCM
>>> implementation like pcsp driver. The pcsp driver is a real hack and
>>> it's there just for fun, not for any real practical use.
>>
>> Ah :)
>>
>>> What I meant was rather that you can create a sound device containing
>>> a mixer volume control that serves exactly like the sysfs or whatever
>>> other interface, without any PCM stream or other interface.
>>
>> Ahhh, hum, I still feel like this might be a bit overkill here.
>>
>>>>>> I only need to control loudness of the
>>>>>> beeper that is controlled by PWM output. That's why I am trying to
>>>>>> extend the pwm-beeper driver, which seems the best fit for such a
>>>>>> device, it is only missing this one feature (loudness control).
>>>>>
>>>>> So the question is what's expected from user-space POV. If a more
>>>>> generic control of beep volume is required, e.g. for desktop-like
>>>>> usages, an implementation of sound driver wouldn't be too bad.
>>>>> OTOH, for other specific use-cases, it doesn't matter much in which
>>>>> interface it's implemented, and sysfs could be an easy choice.
>>>>
>>>> The whole discussion above has been exactly about this. Basically the
>>>> thing is, we can either have:
>>>> - SND_TONE (via some /dev/input/eventX) + sysfs volume control
>>>> -> This is simple, but sounds racy between input and sysfs accesses
>>>
>>> Hmm, how can it be racy if you do proper locking?
>>
>> I can imagine two applications can each grab one of the controls and that
>> makes the interface a bit not nice. That would require extra synchronization
>> in userspace and so on.
>>
>>>> - SND_TONE + SND_TONE_SET_VOLUME
>>>> -> User needs to do two ioctls, hum
>>>> - some new SND_TONE_WITH_VOLUME
>>>> -> Probably the best option, user sets both tone frequency and volume
>>>> in one go, and it also only extends the IOCTL interface, so older
>>>> userspace won't have issues
>>>
>>> Those are "extensions" I have mentioned, and I'm not a big fan for
>>> that, honestly speaking.
>>>
>>> The fact that the beep *output* stuff is provided by the *input*
>>> device is already confusing
>>
>> I agree, this confused me as well.
>
> This comes from the times when keyboards themselves were capable of
> emitting bells (SUN, DEC, etc). In hindsight it was not the best way of
> structuring things, same with the keyboard LEDs (that are now plugged
> into the LED subsystem, but still allow be driven through input).
>
> And in the same vein I wonder if we should bite the bullet and pay with
> a bit of complexity but move sound-related things to sound subsystem.
I am not sure that's the right approach here, since the device cannot do
PCM playback, just bleeps.
>>> (it was so just because of historical
>>> reason), and yet you start implementing more full-featured mixer
>>> control. I'd rather keep fingers away.
>>>
>>> Again, if user-space requires the compatible behavior like the
>>> existing desktop usages
>>
>> It does not. These pwm-beeper devices keep showing up in various embedded
>> devices these days.
>>
>>> , it can be implemented in a similar way like
>>> the existing ones; i.e. provide a mixer control with a proper sound
>>> device. The sound device doesn't need to provide a PCM interface but
>>> just with a mixer interface.
>>>
>>> Or, if the purpose of your target device is a special usage, you don't
>>> need to consider too much about the existing interface, and try to
>>> keep the change as minimal as possible without too intrusive API
>>> changes.
>>
>> My use case is almost perfectly matched by the current input pwm-beeper
>> driver, the only missing bit is the ability to control the loudness at
>> runtime. I think adding the SND_TONE_WITH_VOLUME parameter would cover it,
>> with least intrusive API changes.
>>
>> The SND_TONE already supports configuring tone frequency in Hz as its
>> parameter. Since anything above 64 kHz is certainly not hearable by humans,
>> I would say the SND_TONE_WITH_VOLUME could use 16 LSbits for frequency (so
>> up to 65535 Hz , 0 is OFF), and 16 MSbits for volume .
>>
>> I'm hesitant to overcomplicate something which can currently be controlled
>> via single ioctl by pulling in sound subsystem into the picture.
>
> Can you tell a bit more about your use case? What needs to control the
> volume of beeps? Is this the only source of sounds on the system?
Custom user space application. The entire userspace is custom built in
this case.
In this case, it is a single-use device (think e.g. the kind of
thermometer you stick in your ear when you're ill, to find out how warm
you are).
The beeper there is used to do just that, bleep (with different
frequencies to indicate different stuff), and that works. What I need in
addition to that is control the volume of the bleeps from the
application, so it isn't too noisy. And that needs to be
user-controllable at runtime, so not something that goes in DT.
Right now there is just the bleeper , yes.
^ permalink raw reply
* Re: [PATCH] Input: pwm-beeper - Support volume setting via sysfs
From: Jeff LaBundy @ 2023-08-01 2:56 UTC (permalink / raw)
To: Marek Vasut
Cc: Dmitry Torokhov, Takashi Iwai, linux-input, Uwe Kleine-König,
Frieder Schrempf, Manuel Traut, Thierry Reding, linux-pwm,
alsa-devel, Jaroslav Kysela, Takashi Iwai
In-Reply-To: <f4612dc5-a7d4-74ba-2ed8-ea70314625b6@denx.de>
Hi all,
On Mon, Jul 31, 2023 at 07:49:50PM +0200, Marek Vasut wrote:
> On 7/31/23 18:24, Dmitry Torokhov wrote:
> > On Mon, Jul 31, 2023 at 04:36:01PM +0200, Marek Vasut wrote:
> > > On 7/31/23 16:20, Takashi Iwai wrote:
> > >
> > > [...]
> > >
> > > > > > > Uh, I don't need a full sound device to emit beeps, that's not even
> > > > > > > possible with this hardware.
> > > > > >
> > > > > > Heh, I also don't recommend that route, either :)
> > > > > > (Though, it must be possible to create a sound device with that beep
> > > > > > control in theory)
> > > > >
> > > > > I mean, I can imagine one could possibly use PCM DMA to cook samples
> > > > > to feed some of the PWM devices so they could possibly be used to
> > > > > generate low quality audio, as a weird limited DAC, but ... that's not
> > > > > really generic, and not what I want.
> > > >
> > > > Oh I see how the misunderstanding came; I didn't mean the PCM
> > > > implementation like pcsp driver. The pcsp driver is a real hack and
> > > > it's there just for fun, not for any real practical use.
> > >
> > > Ah :)
> > >
> > > > What I meant was rather that you can create a sound device containing
> > > > a mixer volume control that serves exactly like the sysfs or whatever
> > > > other interface, without any PCM stream or other interface.
> > >
> > > Ahhh, hum, I still feel like this might be a bit overkill here.
> > >
> > > > > > > I only need to control loudness of the
> > > > > > > beeper that is controlled by PWM output. That's why I am trying to
> > > > > > > extend the pwm-beeper driver, which seems the best fit for such a
> > > > > > > device, it is only missing this one feature (loudness control).
> > > > > >
> > > > > > So the question is what's expected from user-space POV. If a more
> > > > > > generic control of beep volume is required, e.g. for desktop-like
> > > > > > usages, an implementation of sound driver wouldn't be too bad.
> > > > > > OTOH, for other specific use-cases, it doesn't matter much in which
> > > > > > interface it's implemented, and sysfs could be an easy choice.
> > > > >
> > > > > The whole discussion above has been exactly about this. Basically the
> > > > > thing is, we can either have:
> > > > > - SND_TONE (via some /dev/input/eventX) + sysfs volume control
> > > > > -> This is simple, but sounds racy between input and sysfs accesses
> > > >
> > > > Hmm, how can it be racy if you do proper locking?
> > >
> > > I can imagine two applications can each grab one of the controls and that
> > > makes the interface a bit not nice. That would require extra synchronization
> > > in userspace and so on.
> > >
> > > > > - SND_TONE + SND_TONE_SET_VOLUME
> > > > > -> User needs to do two ioctls, hum
> > > > > - some new SND_TONE_WITH_VOLUME
> > > > > -> Probably the best option, user sets both tone frequency and volume
> > > > > in one go, and it also only extends the IOCTL interface, so older
> > > > > userspace won't have issues
> > > >
> > > > Those are "extensions" I have mentioned, and I'm not a big fan for
> > > > that, honestly speaking.
> > > >
> > > > The fact that the beep *output* stuff is provided by the *input*
> > > > device is already confusing
> > >
> > > I agree, this confused me as well.
> >
> > This comes from the times when keyboards themselves were capable of
> > emitting bells (SUN, DEC, etc). In hindsight it was not the best way of
> > structuring things, same with the keyboard LEDs (that are now plugged
> > into the LED subsystem, but still allow be driven through input).
> >
> > And in the same vein I wonder if we should bite the bullet and pay with
> > a bit of complexity but move sound-related things to sound subsystem.
>
> I am not sure that's the right approach here, since the device cannot do PCM
> playback, just bleeps.
>
> > > > (it was so just because of historical
> > > > reason), and yet you start implementing more full-featured mixer
> > > > control. I'd rather keep fingers away.
> > > >
> > > > Again, if user-space requires the compatible behavior like the
> > > > existing desktop usages
> > >
> > > It does not. These pwm-beeper devices keep showing up in various embedded
> > > devices these days.
> > >
> > > > , it can be implemented in a similar way like
> > > > the existing ones; i.e. provide a mixer control with a proper sound
> > > > device. The sound device doesn't need to provide a PCM interface but
> > > > just with a mixer interface.
> > > >
> > > > Or, if the purpose of your target device is a special usage, you don't
> > > > need to consider too much about the existing interface, and try to
> > > > keep the change as minimal as possible without too intrusive API
> > > > changes.
> > >
> > > My use case is almost perfectly matched by the current input pwm-beeper
> > > driver, the only missing bit is the ability to control the loudness at
> > > runtime. I think adding the SND_TONE_WITH_VOLUME parameter would cover it,
> > > with least intrusive API changes.
> > >
> > > The SND_TONE already supports configuring tone frequency in Hz as its
> > > parameter. Since anything above 64 kHz is certainly not hearable by humans,
> > > I would say the SND_TONE_WITH_VOLUME could use 16 LSbits for frequency (so
> > > up to 65535 Hz , 0 is OFF), and 16 MSbits for volume .
> > >
> > > I'm hesitant to overcomplicate something which can currently be controlled
> > > via single ioctl by pulling in sound subsystem into the picture.
> >
> > Can you tell a bit more about your use case? What needs to control the
> > volume of beeps? Is this the only source of sounds on the system?
>
> Custom user space application. The entire userspace is custom built in this
> case.
>
> In this case, it is a single-use device (think e.g. the kind of thermometer
> you stick in your ear when you're ill, to find out how warm you are).
>
> The beeper there is used to do just that, bleep (with different frequencies
> to indicate different stuff), and that works. What I need in addition to
> that is control the volume of the bleeps from the application, so it isn't
> too noisy. And that needs to be user-controllable at runtime, so not
> something that goes in DT.
>
> Right now there is just the bleeper , yes.
It sounds like we essentially need an option within pcsp to drive PWM
instead of PCM, but input already has pwm-beeper; it seems harmless to
gently extend the latter for this use-case as opposed to reworking the
former.
I agree that we should not invest too heavily in a legacy ABI, however
something like SND_BELL_VOL seems like a low-cost addition that doesn't
work against extending pcsp in the future. In fact, input already has
precedent for this exact same thing by way of FF rumble effects, which
are often PWM-based themselves.
If SND_BELL_VOL or similar is not acceptable, then the original sysfs
approach seems like the next-best compromise. My only issue with it was
that I felt the range was not abstracted enough.
A fourth option would be to use leds-pwm with a oneshot trigger; this
would render the exact same function. This is obviously a hack, however
downstream vendor kernels implement this kind of blasphemy all the time.
Kind regards,
Jeff LaBundy
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox