* 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
* Re: [PATCH] Input: pwm-beeper - Support volume setting via sysfs
From: Takashi Iwai @ 2023-08-01 6:11 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Marek Vasut, Dmitry Torokhov, linux-input, Uwe Kleine-König,
Frieder Schrempf, Manuel Traut, Thierry Reding, linux-pwm,
alsa-devel, Jaroslav Kysela, Takashi Iwai
In-Reply-To: <ZMh0Sa9s25JHhWw5@nixie71>
On Tue, 01 Aug 2023 04:56:09 +0200,
Jeff LaBundy wrote:
>
> 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.
Nah, please forget pcsp driver. As mentioned earlier, it's a driver
that is present just for fun.
I believe what we need is a simple sound card instance providing a
mixer control for the beep volume, something like a patch like below
(totally untested!)
Takashi
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -595,6 +595,13 @@ config INPUT_PWM_BEEPER
To compile this driver as a module, choose M here: the module will be
called pwm-beeper.
+config INPUT_PWM_BEEPER_MIXER
+ bool "Mixer volume support for PWM beeper"
+ depends on INPUT_PWM_BEEPER
+ depends on SND=y || INPUT_PWM_BEEPER=SND
+ help
+ Say Y here to enable sound mixer for PWM beeper volume.
+
config INPUT_PWM_VIBRA
tristate "PWM vibrator support"
depends on PWM
--- a/drivers/input/misc/pwm-beeper.c
+++ b/drivers/input/misc/pwm-beeper.c
@@ -14,6 +14,8 @@
#include <linux/pwm.h>
#include <linux/slab.h>
#include <linux/workqueue.h>
+#include <sound/core.h>
+#include <sound/control.h>
struct pwm_beeper {
struct input_dev *input;
@@ -21,6 +23,7 @@ struct pwm_beeper {
struct regulator *amplifier;
struct work_struct work;
unsigned long period;
+ unsigned long duty_cycle;
unsigned int bell_frequency;
bool suspended;
bool amplifier_on;
@@ -37,7 +40,7 @@ static int pwm_beeper_on(struct pwm_beeper *beeper, unsigned long period)
state.enabled = true;
state.period = period;
- pwm_set_relative_duty_cycle(&state, 50, 100);
+ pwm_set_relative_duty_cycle(&state, beeper->duty_cycle, 100000);
error = pwm_apply_state(beeper->pwm, &state);
if (error)
@@ -112,6 +115,66 @@ static void pwm_beeper_stop(struct pwm_beeper *beeper)
pwm_beeper_off(beeper);
}
+#ifdef CONFIG_INPUT_PWM_BEEPER_MIXER
+static int pwm_beeper_mixer_info(struct snd_kcontrol *kcontrol,
+ struct snd_ctl_elem_info *uinfo)
+{
+ uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
+ uinfo->count = 1;
+ uinfo->value.integer.min = 0;
+ uinfo->value.integer.max = 50000;
+ return 0;
+}
+
+static int pwm_beeper_mixer_get(struct snd_kcontrol *kcontrol,
+ struct snd_ctl_elem_value *ucontrol)
+{
+ struct pwm_beeper *beeper = snd_kcontrol_chip(kcontrol);
+
+ ucontrol->value.integer.value[0] = beeper->duty_cycle;
+ return 0;
+}
+
+static int pwm_beeper_mixer_put(struct snd_kcontrol *kcontrol,
+ struct snd_ctl_elem_value *ucontrol)
+{
+ struct pwm_beeper *beeper = snd_kcontrol_chip(kcontrol);
+ unsigned long val = ucontrol->value.integer.value[0];
+
+ val = min(val, 50000UL);
+ if (beeper->duty_cycle == val)
+ return 0;
+ beeper->duty_cycle = val;
+ if (!beeper->suspended)
+ schedule_work(&beeper->work);
+ return 1;
+}
+
+static const struct snd_kcontrol_new pwm_beeper_mixer_ctl = {
+ .iface = SNDRV_CTL_ELEM_IFACE_MIXER,
+ .name = "Beep Playback Switch",
+ .info = pwm_beeper_mixer_info,
+ .get = pwm_beeper_mixer_get,
+ .put = pwm_beeper_mixer_put,
+};
+
+static int pwm_beeper_mixer_attach(struct device *dev, struct pwm_beeper *beeper)
+{
+ struct snd_card *card;
+ int err;
+
+ err = snd_devm_card_new(dev, 0, NULL, THIS_MODULE, 0, &card);
+ if (err)
+ return err;
+
+ err = snd_ctl_add(card, snd_ctl_new1(&pwm_beeper_mixer_ctl, beeper));
+ if (err)
+ return err;
+
+ return snd_card_register(card);
+}
+#endif /* CONFIG_INPUT_PWM_BEEPER_MIXER */
+
static void pwm_beeper_close(struct input_dev *input)
{
struct pwm_beeper *beeper = input_get_drvdata(input);
@@ -189,6 +252,7 @@ static int pwm_beeper_probe(struct platform_device *pdev)
beeper->input->event = pwm_beeper_event;
beeper->input->close = pwm_beeper_close;
+ beeper->duty_cycle = 50000;
input_set_drvdata(beeper->input, beeper);
@@ -200,6 +264,11 @@ static int pwm_beeper_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, beeper);
+#ifdef CONFIG_INPUT_PWM_BEEPER_MIXER
+ error = pwm_beeper_mixer_attach(dev, beeper);
+ if (error)
+ return error;
+#endif
return 0;
}
^ permalink raw reply
* Re: [PATCH] Input: pwm-beeper - Support volume setting via sysfs
From: Dmitry Torokhov @ 2023-08-01 7:28 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Marek Vasut, 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: <ZMh0Sa9s25JHhWw5@nixie71>
On Mon, Jul 31, 2023 at 09:56:09PM -0500, Jeff LaBundy wrote:
> 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.
If we want to extend the API we will need to define exactly how it will
all work. I.e. what happens if userspace mixes the old SND_TONE and
SND_BELL with the new SND_BELL_VOL or whatever. Does it play with
previously set volume? The default one? How to set the default one? How
to figure out what the current volume is if we decide to make volume
"sticky"?
As far as userspace I expect it is more common to have one program (or
component of a program) to set volume and then something else requests
sound, so having one-shot API is of dubious value to me.
I hope we can go with Takashi's proposal downthread, but if not I wonder
if the sysfs approach is not the simplest one. Do we expect more beepers
that can control volume besides pwm-beeper?
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH v4 2/2] input: zinitix: Add touchkey support
From: Nikita Travkin @ 2023-08-01 9:09 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Michael Srba, Linus Walleij
Cc: linux-input, devicetree, linux-kernel, Nikita Travkin
In-Reply-To: <20230801-zinitix-tkey-v4-0-b85526c5a474@trvn.ru>
Zinitix touch controllers can use some of the sense lines for virtual
keys (like those found on many phones). Add support for those keys.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Nikita Travkin <nikita@trvn.ru>
---
drivers/input/touchscreen/zinitix.c | 61 +++++++++++++++++++++++++++++++++++--
1 file changed, 58 insertions(+), 3 deletions(-)
diff --git a/drivers/input/touchscreen/zinitix.c b/drivers/input/touchscreen/zinitix.c
index 1b4807ba4624..75390d67689e 100644
--- a/drivers/input/touchscreen/zinitix.c
+++ b/drivers/input/touchscreen/zinitix.c
@@ -119,6 +119,7 @@
#define DEFAULT_TOUCH_POINT_MODE 2
#define MAX_SUPPORTED_FINGER_NUM 5
+#define MAX_SUPPORTED_BUTTON_NUM 8
#define CHIP_ON_DELAY 15 // ms
#define FIRMWARE_ON_DELAY 40 // ms
@@ -146,6 +147,8 @@ struct bt541_ts_data {
struct touchscreen_properties prop;
struct regulator_bulk_data supplies[2];
u32 zinitix_mode;
+ u32 keycodes[MAX_SUPPORTED_BUTTON_NUM];
+ int num_keycodes;
};
static int zinitix_read_data(struct i2c_client *client,
@@ -195,6 +198,7 @@ static int zinitix_init_touch(struct bt541_ts_data *bt541)
struct i2c_client *client = bt541->client;
int i;
int error;
+ u16 int_flags = 0;
error = zinitix_write_cmd(client, ZINITIX_SWRESET_CMD);
if (error) {
@@ -225,6 +229,11 @@ static int zinitix_init_touch(struct bt541_ts_data *bt541)
if (error)
return error;
+ error = zinitix_write_u16(client, ZINITIX_BUTTON_SUPPORTED_NUM,
+ bt541->num_keycodes);
+ if (error)
+ return error;
+
error = zinitix_write_u16(client, ZINITIX_INITIAL_TOUCH_MODE,
bt541->zinitix_mode);
if (error)
@@ -235,9 +244,12 @@ static int zinitix_init_touch(struct bt541_ts_data *bt541)
if (error)
return error;
- error = zinitix_write_u16(client, ZINITIX_INT_ENABLE_FLAG,
- BIT_PT_CNT_CHANGE | BIT_DOWN | BIT_MOVE |
- BIT_UP);
+ int_flags = BIT_PT_CNT_CHANGE | BIT_DOWN | BIT_MOVE | BIT_UP;
+
+ if (bt541->num_keycodes)
+ int_flags |= BIT_ICON_EVENT;
+
+ error = zinitix_write_u16(client, ZINITIX_INT_ENABLE_FLAG, int_flags);
if (error)
return error;
@@ -350,6 +362,15 @@ static void zinitix_report_finger(struct bt541_ts_data *bt541, int slot,
}
}
+static void zinitix_report_keys(struct bt541_ts_data *bt541, u16 icon_events)
+{
+ int i;
+
+ for (i = 0; i < bt541->num_keycodes; i++)
+ input_report_key(bt541->input_dev,
+ bt541->keycodes[i], !!(icon_events & BIT(i)));
+}
+
static irqreturn_t zinitix_ts_irq_handler(int irq, void *bt541_handler)
{
struct bt541_ts_data *bt541 = bt541_handler;
@@ -358,6 +379,7 @@ static irqreturn_t zinitix_ts_irq_handler(int irq, void *bt541_handler)
unsigned long finger_mask;
int error;
int i;
+ __le16 icon_events = 0;
memset(&touch_event, 0, sizeof(struct touch_event));
@@ -368,6 +390,17 @@ static irqreturn_t zinitix_ts_irq_handler(int irq, void *bt541_handler)
goto out;
}
+ if (le16_to_cpu(touch_event.status) & BIT_ICON_EVENT) {
+ error = zinitix_read_data(bt541->client, ZINITIX_ICON_STATUS_REG,
+ &icon_events, sizeof(icon_events));
+ if (error) {
+ dev_err(&client->dev, "Failed to read icon events\n");
+ goto out;
+ }
+
+ zinitix_report_keys(bt541, le16_to_cpu(icon_events));
+ }
+
finger_mask = touch_event.finger_mask;
for_each_set_bit(i, &finger_mask, MAX_SUPPORTED_FINGER_NUM) {
const struct point_coord *p = &touch_event.point_coord[i];
@@ -453,6 +486,7 @@ static int zinitix_init_input_dev(struct bt541_ts_data *bt541)
{
struct input_dev *input_dev;
int error;
+ int i;
input_dev = devm_input_allocate_device(&bt541->client->dev);
if (!input_dev) {
@@ -470,6 +504,14 @@ static int zinitix_init_input_dev(struct bt541_ts_data *bt541)
input_dev->open = zinitix_input_open;
input_dev->close = zinitix_input_close;
+ if (bt541->num_keycodes) {
+ input_dev->keycode = bt541->keycodes;
+ input_dev->keycodemax = bt541->num_keycodes;
+ input_dev->keycodesize = sizeof(bt541->keycodes[0]);
+ for (i = 0; i < bt541->num_keycodes; i++)
+ input_set_capability(input_dev, EV_KEY, bt541->keycodes[i]);
+ }
+
input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_X);
input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_Y);
input_set_abs_params(input_dev, ABS_MT_WIDTH_MAJOR, 0, 255, 0, 0);
@@ -534,6 +576,19 @@ static int zinitix_ts_probe(struct i2c_client *client)
return error;
}
+ bt541->num_keycodes = of_property_read_variable_u32_array(
+ client->dev.of_node, "linux,keycodes",
+ bt541->keycodes, 0,
+ ARRAY_SIZE(bt541->keycodes));
+ if (bt541->num_keycodes == -EINVAL) {
+ bt541->num_keycodes = 0;
+ } else if (bt541->num_keycodes < 0) {
+ dev_err(&client->dev,
+ "Unable to parse \"linux,keycodes\" property: %d\n",
+ bt541->num_keycodes);
+ return bt541->num_keycodes;
+ }
+
error = zinitix_init_input_dev(bt541);
if (error) {
dev_err(&client->dev,
--
2.41.0
^ permalink raw reply related
* [PATCH v4 0/2] Add touch-keys support to the Zinitix touch driver
From: Nikita Travkin @ 2023-08-01 9:09 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Michael Srba, Linus Walleij
Cc: linux-input, devicetree, linux-kernel, Nikita Travkin,
Rob Herring
This series adds support for the touch-keys that can be present on some
touchscreen configurations.
Signed-off-by: Nikita Travkin <nikita@trvn.ru>
---
Changes in v4:
- The series was partially applied, these patches dropped.
- Link to v3: https://lore.kernel.org/r/20220106072840.36851-1-nikita@trvn.ru
---
Nikita Travkin (2):
dt-bindings: input: zinitix: Document touch-keys support
input: zinitix: Add touchkey support
.../bindings/input/touchscreen/zinitix,bt400.yaml | 10 ++++
drivers/input/touchscreen/zinitix.c | 61 ++++++++++++++++++++--
2 files changed, 68 insertions(+), 3 deletions(-)
---
base-commit: a734662572708cf062e974f659ae50c24fc1ad17
change-id: 20230801-zinitix-tkey-5a3023bc304c
Best regards,
--
Nikita Travkin <nikita@trvn.ru>
^ permalink raw reply
* [PATCH v4 1/2] dt-bindings: input: zinitix: Document touch-keys support
From: Nikita Travkin @ 2023-08-01 9:09 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Michael Srba, Linus Walleij
Cc: linux-input, devicetree, linux-kernel, Nikita Travkin,
Rob Herring
In-Reply-To: <20230801-zinitix-tkey-v4-0-b85526c5a474@trvn.ru>
In some configurations the touch controller can support the touch-keys.
Document the linux,keycodes property that enables those keys and
specifies the keycodes that should be used to report the key events.
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Nikita Travkin <nikita@trvn.ru>
---
.../devicetree/bindings/input/touchscreen/zinitix,bt400.yaml | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/zinitix,bt400.yaml b/Documentation/devicetree/bindings/input/touchscreen/zinitix,bt400.yaml
index b1507463a03e..3f663ce3e44e 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/zinitix,bt400.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/zinitix,bt400.yaml
@@ -16,6 +16,7 @@ maintainers:
allOf:
- $ref: touchscreen.yaml#
+ - $ref: ../input.yaml#
properties:
$nodename:
@@ -79,6 +80,15 @@ properties:
$ref: /schemas/types.yaml#/definitions/uint32
enum: [1, 2]
+ linux,keycodes:
+ description:
+ This property specifies an array of keycodes assigned to the
+ touch-keys that can be present in some touchscreen configurations.
+ If the touch-keys are enabled, controller firmware will assign some
+ touch sense lines to those keys.
+ minItems: 1
+ maxItems: 8
+
touchscreen-size-x: true
touchscreen-size-y: true
touchscreen-fuzz-x: true
--
2.41.0
^ permalink raw reply related
* W sprawie samochodu
From: Radosław Grabowski @ 2023-07-31 8:35 UTC (permalink / raw)
To: linux-input
Dzień dobry,
chcielibyśmy zapewnić Państwu kompleksowe rozwiązania, jeśli chodzi o system monitoringu GPS.
Precyzyjne monitorowanie pojazdów na mapach cyfrowych, śledzenie ich parametrów eksploatacyjnych w czasie rzeczywistym oraz kontrola paliwa to kluczowe funkcjonalności naszego systemu.
Organizowanie pracy pracowników jest dzięki temu prostsze i bardziej efektywne, a oszczędności i optymalizacja w zakresie ponoszonych kosztów, mają dla każdego przedsiębiorcy ogromne znaczenie.
Dopasujemy naszą ofertę do Państwa oczekiwań i potrzeb organizacji. Czy moglibyśmy porozmawiać o naszej propozycji?
Pozdrawiam
Radosław Grabowski
^ permalink raw reply
* [PATCH -next] Input: serio - fix possible memory leak while device_add() fails
From: Zhu Wang @ 2023-08-01 11:51 UTC (permalink / raw)
To: dmitry.torokhov, rafael, christophe.jaillet, gregkh, linux-input; +Cc: wangzhu9
If device_add() returns error, the name allocated by dev_set_name() need
be freed. As comment of device_add() says, it should use put_device() to
release the reference in the error path. So fix this by calling
put_device, then the name can be freed in kobject_cleanp().
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Zhu Wang <wangzhu9@huawei.com>
---
drivers/input/serio/serio.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/input/serio/serio.c b/drivers/input/serio/serio.c
index 767fc9efb4a8..d3bb6ec91326 100644
--- a/drivers/input/serio/serio.c
+++ b/drivers/input/serio/serio.c
@@ -537,10 +537,12 @@ static void serio_add_port(struct serio *serio)
serio->start(serio);
error = device_add(&serio->dev);
- if (error)
+ if (error) {
+ put_device(&serio->dev);
dev_err(&serio->dev,
"device_add() failed for %s (%s), error: %d\n",
serio->phys, serio->name, error);
+ }
}
/*
--
2.17.1
^ permalink raw reply related
* Re: [PATCH] Input: pwm-beeper - Support volume setting via sysfs
From: Marek Vasut @ 2023-08-01 11:51 UTC (permalink / raw)
To: Dmitry Torokhov, Jeff LaBundy
Cc: 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: <ZMi0HT/yaTo9uTyi@google.com>
On 8/1/23 09:28, Dmitry Torokhov wrote:
> On Mon, Jul 31, 2023 at 09:56:09PM -0500, Jeff LaBundy wrote:
>> 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.
>
> If we want to extend the API we will need to define exactly how it will
> all work. I.e. what happens if userspace mixes the old SND_TONE and
> SND_BELL with the new SND_BELL_VOL or whatever. Does it play with
> previously set volume? The default one?
Default one, to preserve current behavior, yes.
> How to set the default one?
We do not, we can call pwm_get_duty_cycle() to get the current duty
cycle of the PWM to figure out the default.
> How
> to figure out what the current volume is if we decide to make volume
> "sticky"?
The patch stores the current volume configured via sysfs into
beeper->duty_cycle .
> As far as userspace I expect it is more common to have one program (or
> component of a program) to set volume and then something else requests
> sound, so having one-shot API is of dubious value to me.
Currently the use case I have for this is a single user facing
application which configures both.
> I hope we can go with Takashi's proposal downthread, but if not I wonder
> if the sysfs approach is not the simplest one. Do we expect more beepers
> that can control volume besides pwm-beeper?
It seems to me pulling in dependency on the entire sound subsystem only
to set beeper volume is overkill. I currently don't even have sound
subsystem compiled in.
^ permalink raw reply
* Re: [PATCH] Input: pwm-beeper - Support volume setting via sysfs
From: Marek Vasut @ 2023-08-01 11:38 UTC (permalink / raw)
To: Takashi Iwai, Jeff LaBundy
Cc: Dmitry Torokhov, linux-input, Uwe Kleine-König,
Frieder Schrempf, Manuel Traut, Thierry Reding, linux-pwm,
alsa-devel, Jaroslav Kysela, Takashi Iwai
In-Reply-To: <87a5vbi96v.wl-tiwai@suse.de>
On 8/1/23 08:11, Takashi Iwai wrote:
> On Tue, 01 Aug 2023 04:56:09 +0200,
> Jeff LaBundy wrote:
>>
>> 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.
>
> Nah, please forget pcsp driver. As mentioned earlier, it's a driver
> that is present just for fun.
>
> I believe what we need is a simple sound card instance providing a
> mixer control for the beep volume, something like a patch like below
> (totally untested!)
Do we really want to add dependency on the entire sound subsystem (which
is currently not needed on the device I care about) only to configure
one single tunable of the PWM beeper ? It seems to add too much bloat to me.
^ permalink raw reply
* [PATCH v5 0/4] input: touchscreen: add initial support for Goodix Berlin touchscreen IC
From: Neil Armstrong @ 2023-08-01 12:15 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, Jeff LaBundy
Cc: linux-input, linux-arm-msm, devicetree, linux-kernel,
Neil Armstrong, Rob Herring
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
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
Changes in v5:
- rebased on next-20230801
- Link to v4: https://lore.kernel.org/r/20230606-topic-goodix-berlin-upstream-initial-v4-0-0947c489be17@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: a734662572708cf062e974f659ae50c24fc1ad17
change-id: 20230606-topic-goodix-berlin-upstream-initial-ba97e8ec8f4c
Best regards,
--
Neil Armstrong <neil.armstrong@linaro.org>
^ permalink raw reply
* [PATCH v5 1/4] dt-bindings: input: document Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2023-08-01 12:15 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, Jeff LaBundy
Cc: linux-input, linux-arm-msm, devicetree, linux-kernel,
Neil Armstrong, Rob Herring
In-Reply-To: <20230801-topic-goodix-berlin-upstream-initial-v5-0-079252935593@linaro.org>
Document the Goodix GT9916 wich is part of the "Berlin" serie
of Touchscreen controllers IC from Goodix.
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
.../bindings/input/touchscreen/goodix,gt9916.yaml | 95 ++++++++++++++++++++++
1 file changed, 95 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix,gt9916.yaml b/Documentation/devicetree/bindings/input/touchscreen/goodix,gt9916.yaml
new file mode 100644
index 000000000000..d90f045ac06c
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/goodix,gt9916.yaml
@@ -0,0 +1,95 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/touchscreen/goodix,gt9916.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Goodix Berlin series touchscreen controller
+
+description: The Goodix Berlin series of touchscreen controllers
+ be connected to either I2C or SPI buses.
+
+maintainers:
+ - Neil Armstrong <neil.armstrong@linaro.org>
+
+allOf:
+ - $ref: touchscreen.yaml#
+ - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+ compatible:
+ enum:
+ - goodix,gt9916
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ reset-gpios:
+ maxItems: 1
+
+ avdd-supply:
+ description: Analog power supply regulator on AVDD pin
+
+ vddio-supply:
+ description: power supply regulator on VDDIO pin
+
+ spi-max-frequency: true
+ touchscreen-inverted-x: true
+ touchscreen-inverted-y: true
+ touchscreen-size-x: true
+ touchscreen-size-y: true
+ touchscreen-swapped-x-y: true
+
+additionalProperties: false
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - avdd-supply
+ - touchscreen-size-x
+ - touchscreen-size-y
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/gpio/gpio.h>
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ touchscreen@5d {
+ compatible = "goodix,gt9916";
+ reg = <0x5d>;
+ interrupt-parent = <&gpio>;
+ interrupts = <25 IRQ_TYPE_LEVEL_LOW>;
+ reset-gpios = <&gpio1 1 GPIO_ACTIVE_LOW>;
+ avdd-supply = <&ts_avdd>;
+ touchscreen-size-x = <1024>;
+ touchscreen-size-y = <768>;
+ };
+ };
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/gpio/gpio.h>
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ num-cs = <1>;
+ cs-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
+ touchscreen@0 {
+ compatible = "goodix,gt9916";
+ reg = <0>;
+ interrupt-parent = <&gpio>;
+ interrupts = <25 IRQ_TYPE_LEVEL_LOW>;
+ reset-gpios = <&gpio1 1 GPIO_ACTIVE_LOW>;
+ avdd-supply = <&ts_avdd>;
+ spi-max-frequency = <1000000>;
+ touchscreen-size-x = <1024>;
+ touchscreen-size-y = <768>;
+ };
+ };
+
+...
--
2.34.1
^ permalink raw reply related
* [PATCH v5 3/4] input: touchscreen: add I2C support for Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2023-08-01 12:15 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, Jeff LaBundy
Cc: linux-input, linux-arm-msm, devicetree, linux-kernel,
Neil Armstrong
In-Reply-To: <20230801-topic-goodix-berlin-upstream-initial-v5-0-079252935593@linaro.org>
Add initial support for the new Goodix "Berlin" touchscreen ICs
over the I2C interface.
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.
[1] https://github.com/goodix/goodix_ts_berlin
[2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
drivers/input/touchscreen/Kconfig | 14 ++++++
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/goodix_berlin_i2c.c | 69 +++++++++++++++++++++++++++
3 files changed, 84 insertions(+)
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 950da599ae1a..cc7b88118158 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -419,6 +419,20 @@ config TOUCHSCREEN_GOODIX
config TOUCHSCREEN_GOODIX_BERLIN_CORE
tristate
+config TOUCHSCREEN_GOODIX_BERLIN_I2C
+ tristate "Goodix Berlin I2C touchscreen"
+ depends on I2C
+ select REGMAP_I2C
+ select TOUCHSCREEN_GOODIX_BERLIN_CORE
+ help
+ Say Y here if you have a Goodix Berlin IC connected to
+ your system via I2C.
+
+ If unsure, say N.
+
+ To compile this driver as a module, choose M here: the
+ module will be called goodix_berlin_i2c.
+
config TOUCHSCREEN_HIDEEP
tristate "HiDeep Touch IC"
depends on I2C
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 2e2f3e70cd2c..7ef677cf7a10 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -48,6 +48,7 @@ obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_CORE) += goodix_berlin_core.o
+obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_I2C) += goodix_berlin_i2c.o
obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
diff --git a/drivers/input/touchscreen/goodix_berlin_i2c.c b/drivers/input/touchscreen/goodix_berlin_i2c.c
new file mode 100644
index 000000000000..6407b2258eb1
--- /dev/null
+++ b/drivers/input/touchscreen/goodix_berlin_i2c.c
@@ -0,0 +1,69 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Goodix Berlin Touchscreen Driver
+ *
+ * Copyright (C) 2020 - 2021 Goodix, Inc.
+ * Copyright (C) 2023 Linaro Ltd.
+ *
+ * Based on goodix_ts_berlin driver.
+ */
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#include "goodix_berlin.h"
+
+#define I2C_MAX_TRANSFER_SIZE 256
+
+static const struct regmap_config goodix_berlin_i2c_regmap_conf = {
+ .reg_bits = 32,
+ .val_bits = 8,
+ .max_raw_read = I2C_MAX_TRANSFER_SIZE,
+ .max_raw_write = I2C_MAX_TRANSFER_SIZE,
+};
+
+/* vendor & product left unassigned here, should probably be updated from fw info */
+static const struct input_id goodix_berlin_i2c_input_id = {
+ .bustype = BUS_I2C,
+};
+
+static int goodix_berlin_i2c_probe(struct i2c_client *client)
+{
+ struct regmap *regmap;
+
+ regmap = devm_regmap_init_i2c(client, &goodix_berlin_i2c_regmap_conf);
+ if (IS_ERR(regmap))
+ return PTR_ERR(regmap);
+
+ return goodix_berlin_probe(&client->dev, client->irq,
+ &goodix_berlin_i2c_input_id, regmap);
+}
+
+static const struct i2c_device_id goodix_berlin_i2c_id[] = {
+ { "gt9916", 0 },
+ { }
+};
+
+MODULE_DEVICE_TABLE(i2c, goodix_berlin_i2c_id);
+
+static const struct of_device_id goodix_berlin_i2c_of_match[] = {
+ { .compatible = "goodix,gt9916", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, goodix_berlin_i2c_of_match);
+
+static struct i2c_driver goodix_berlin_i2c_driver = {
+ .driver = {
+ .name = "goodix-berlin-i2c",
+ .of_match_table = goodix_berlin_i2c_of_match,
+ .pm = pm_sleep_ptr(&goodix_berlin_pm_ops),
+ },
+ .probe = goodix_berlin_i2c_probe,
+ .id_table = goodix_berlin_i2c_id,
+};
+module_i2c_driver(goodix_berlin_i2c_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Goodix Berlin I2C Touchscreen driver");
+MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
--
2.34.1
^ permalink raw reply related
* [PATCH v5 2/4] input: touchscreen: add core support for Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2023-08-01 12:15 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, Jeff LaBundy
Cc: linux-input, linux-arm-msm, devicetree, linux-kernel,
Neil Armstrong
In-Reply-To: <20230801-topic-goodix-berlin-upstream-initial-v5-0-079252935593@linaro.org>
Add initial support for the new Goodix "Berlin" touchscreen ICs.
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
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
drivers/input/touchscreen/Kconfig | 3 +
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/goodix_berlin.h | 159 +++++++
drivers/input/touchscreen/goodix_berlin_core.c | 581 +++++++++++++++++++++++++
4 files changed, 744 insertions(+)
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index e3e2324547b9..950da599ae1a 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -416,6 +416,9 @@ config TOUCHSCREEN_GOODIX
To compile this driver as a module, choose M here: the
module will be called goodix.
+config TOUCHSCREEN_GOODIX_BERLIN_CORE
+ tristate
+
config TOUCHSCREEN_HIDEEP
tristate "HiDeep Touch IC"
depends on I2C
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 62bd24f3ac8e..2e2f3e70cd2c 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -47,6 +47,7 @@ obj-$(CONFIG_TOUCHSCREEN_EGALAX_SERIAL) += egalax_ts_serial.o
obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
+obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_CORE) += goodix_berlin_core.o
obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
diff --git a/drivers/input/touchscreen/goodix_berlin.h b/drivers/input/touchscreen/goodix_berlin.h
new file mode 100644
index 000000000000..235f44947a28
--- /dev/null
+++ b/drivers/input/touchscreen/goodix_berlin.h
@@ -0,0 +1,159 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Goodix Touchscreen Driver
+ * Copyright (C) 2020 - 2021 Goodix, Inc.
+ * Copyright (C) 2023 Linaro Ltd.
+ *
+ * Based on goodix_berlin_berlin driver.
+ */
+
+#ifndef __GOODIX_BERLIN_H_
+#define __GOODIX_BERLIN_H_
+
+#include <linux/gpio/consumer.h>
+#include <linux/input.h>
+#include <linux/input/touchscreen.h>
+#include <linux/regulator/consumer.h>
+#include <linux/sizes.h>
+
+#define GOODIX_BERLIN_MAX_TOUCH 10
+
+#define GOODIX_BERLIN_NORMAL_RESET_DELAY_MS 100
+
+#define GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN 8
+#define GOODIX_BERLIN_STATUS_OFFSET 0
+#define GOODIX_BERLIN_REQUEST_TYPE_OFFSET 2
+
+#define GOODIX_BERLIN_BYTES_PER_POINT 8
+#define GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE 2
+#define GOODIX_BERLIN_COOR_DATA_CHECKSUM_MASK GENMASK(15, 0)
+
+/* Read n finger events */
+#define GOODIX_BERLIN_IRQ_READ_LEN(n) (GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN + \
+ (GOODIX_BERLIN_BYTES_PER_POINT * (n)) + \
+ GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE)
+
+#define GOODIX_BERLIN_TOUCH_EVENT BIT(7)
+#define GOODIX_BERLIN_REQUEST_EVENT BIT(6)
+#define GOODIX_BERLIN_TOUCH_COUNT_MASK GENMASK(3, 0)
+
+#define GOODIX_BERLIN_REQUEST_CODE_RESET 3
+
+#define GOODIX_BERLIN_POINT_TYPE_MASK GENMASK(3, 0)
+#define GOODIX_BERLIN_POINT_TYPE_STYLUS_HOVER 1
+#define GOODIX_BERLIN_POINT_TYPE_STYLUS 3
+
+#define GOODIX_BERLIN_TOUCH_ID_MASK GENMASK(7, 4)
+
+#define GOODIX_BERLIN_DEV_CONFIRM_VAL 0xAA
+#define GOODIX_BERLIN_BOOTOPTION_ADDR 0x10000
+#define GOODIX_BERLIN_FW_VERSION_INFO_ADDR 0x10014
+
+#define GOODIX_BERLIN_IC_INFO_MAX_LEN SZ_1K
+#define GOODIX_BERLIN_IC_INFO_ADDR 0x10070
+
+struct goodix_berlin_fw_version {
+ u8 rom_pid[6];
+ u8 rom_vid[3];
+ u8 rom_vid_reserved;
+ u8 patch_pid[8];
+ u8 patch_vid[4];
+ u8 patch_vid_reserved;
+ u8 sensor_id;
+ u8 reserved[2];
+ __le16 checksum;
+} __packed;
+
+struct goodix_berlin_ic_info_version {
+ u8 info_customer_id;
+ u8 info_version_id;
+ u8 ic_die_id;
+ u8 ic_version_id;
+ __le32 config_id;
+ u8 config_version;
+ u8 frame_data_customer_id;
+ u8 frame_data_version_id;
+ u8 touch_data_customer_id;
+ u8 touch_data_version_id;
+ u8 reserved[3];
+} __packed;
+
+struct goodix_berlin_ic_info_feature {
+ __le16 freqhop_feature;
+ __le16 calibration_feature;
+ __le16 gesture_feature;
+ __le16 side_touch_feature;
+ __le16 stylus_feature;
+} __packed;
+
+struct goodix_berlin_ic_info_misc {
+ __le32 cmd_addr;
+ __le16 cmd_max_len;
+ __le32 cmd_reply_addr;
+ __le16 cmd_reply_len;
+ __le32 fw_state_addr;
+ __le16 fw_state_len;
+ __le32 fw_buffer_addr;
+ __le16 fw_buffer_max_len;
+ __le32 frame_data_addr;
+ __le16 frame_data_head_len;
+ __le16 fw_attr_len;
+ __le16 fw_log_len;
+ u8 pack_max_num;
+ u8 pack_compress_version;
+ __le16 stylus_struct_len;
+ __le16 mutual_struct_len;
+ __le16 self_struct_len;
+ __le16 noise_struct_len;
+ __le32 touch_data_addr;
+ __le16 touch_data_head_len;
+ __le16 point_struct_len;
+ __le16 reserved1;
+ __le16 reserved2;
+ __le32 mutual_rawdata_addr;
+ __le32 mutual_diffdata_addr;
+ __le32 mutual_refdata_addr;
+ __le32 self_rawdata_addr;
+ __le32 self_diffdata_addr;
+ __le32 self_refdata_addr;
+ __le32 iq_rawdata_addr;
+ __le32 iq_refdata_addr;
+ __le32 im_rawdata_addr;
+ __le16 im_readata_len;
+ __le32 noise_rawdata_addr;
+ __le16 noise_rawdata_len;
+ __le32 stylus_rawdata_addr;
+ __le16 stylus_rawdata_len;
+ __le32 noise_data_addr;
+ __le32 esd_addr;
+} __packed;
+
+struct goodix_berlin_touch_data {
+ u8 id;
+ u8 unused;
+ __le16 x;
+ __le16 y;
+ __le16 w;
+} __packed;
+
+struct goodix_berlin_core {
+ struct device *dev;
+ struct regmap *regmap;
+ struct regulator *avdd;
+ struct regulator *iovdd;
+ struct gpio_desc *reset_gpio;
+ struct touchscreen_properties props;
+ struct goodix_berlin_fw_version fw_version;
+ struct input_dev *input_dev;
+ int irq;
+
+ /* Runtime parameters extracted from IC_INFO buffer */
+ u32 touch_data_addr;
+};
+
+int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
+ struct regmap *regmap);
+
+extern const struct dev_pm_ops goodix_berlin_pm_ops;
+
+#endif
diff --git a/drivers/input/touchscreen/goodix_berlin_core.c b/drivers/input/touchscreen/goodix_berlin_core.c
new file mode 100644
index 000000000000..08c508677669
--- /dev/null
+++ b/drivers/input/touchscreen/goodix_berlin_core.c
@@ -0,0 +1,581 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Goodix Touchscreen Driver
+ * Copyright (C) 2020 - 2021 Goodix, Inc.
+ * Copyright (C) 2023 Linaro Ltd.
+ *
+ * Based on goodix_ts_berlin driver.
+ */
+#include <asm/unaligned.h>
+#include <linux/input/mt.h>
+#include <linux/input/touchscreen.h>
+#include <linux/regmap.h>
+
+#include "goodix_berlin.h"
+
+/*
+ * Goodix "Berlin" Touchscreen ID driver
+ *
+ * This driver is distinct from goodix.c since hardware interface
+ * is different enough to require a new driver.
+ * None of the register address or data structure are close enough
+ * to the previous generations.
+ *
+ * Currently only handles Multitouch events with already
+ * programmed firmware and "config" for "Revision D" Berlin IC.
+ *
+ * Support is missing for:
+ * - ESD Management
+ * - Firmware update/flashing
+ * - "Config" update/flashing
+ * - Stylus Events
+ * - Gesture Events
+ * - Support for older revisions (A & B)
+ */
+
+static bool goodix_berlin_checksum_valid(const u8 *data, int size)
+{
+ u32 cal_checksum = 0;
+ u16 r_checksum;
+ u32 i;
+
+ if (size < GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE)
+ return false;
+
+ for (i = 0; i < size - GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE; i++)
+ cal_checksum += data[i];
+
+ r_checksum = get_unaligned_le16(&data[i]);
+
+ return FIELD_GET(GOODIX_BERLIN_COOR_DATA_CHECKSUM_MASK, cal_checksum) == r_checksum;
+}
+
+static bool goodix_berlin_is_dummy_data(struct goodix_berlin_core *cd,
+ const u8 *data, int size)
+{
+ int i;
+
+ /*
+ * If the device is missing or doesn't respond the buffer
+ * could be filled with bus default line state, 0x00 or 0xff,
+ * so declare success the first time we encounter neither.
+ */
+ for (i = 0; i < size; i++)
+ if (data[i] > 0 && data[i] < 0xff)
+ return false;
+
+ return true;
+}
+
+static int goodix_berlin_dev_confirm(struct goodix_berlin_core *cd)
+{
+ u8 tx_buf[8], rx_buf[8];
+ int retry = 3;
+ int error;
+
+ memset(tx_buf, GOODIX_BERLIN_DEV_CONFIRM_VAL, sizeof(tx_buf));
+ while (retry--) {
+ error = regmap_raw_write(cd->regmap, GOODIX_BERLIN_BOOTOPTION_ADDR, tx_buf,
+ sizeof(tx_buf));
+ if (error)
+ return error;
+
+ error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_BOOTOPTION_ADDR, rx_buf,
+ sizeof(rx_buf));
+ if (error)
+ return error;
+
+ if (!memcmp(tx_buf, rx_buf, sizeof(tx_buf)))
+ return 0;
+
+ usleep_range(5000, 5100);
+ }
+
+ dev_err(cd->dev, "device confirm failed, rx_buf: %*ph\n", 8, rx_buf);
+
+ return -EINVAL;
+}
+
+static int goodix_berlin_power_on(struct goodix_berlin_core *cd, bool on)
+{
+ int error = 0;
+
+ if (on) {
+ error = regulator_enable(cd->iovdd);
+ if (error) {
+ dev_err(cd->dev, "Failed to enable iovdd: %d\n", error);
+ return error;
+ }
+
+ /* Vendor waits 3ms for IOVDD to settle */
+ usleep_range(3000, 3100);
+
+ error = regulator_enable(cd->avdd);
+ if (error) {
+ dev_err(cd->dev, "Failed to enable avdd: %d\n", error);
+ goto err_iovdd_disable;
+ }
+
+ /* Vendor waits 15ms for IOVDD to settle */
+ usleep_range(15000, 15100);
+
+ gpiod_set_value(cd->reset_gpio, 0);
+
+ /* Vendor waits 4ms for Firmware to initialize */
+ usleep_range(4000, 4100);
+
+ error = goodix_berlin_dev_confirm(cd);
+ if (error)
+ goto err_dev_reset;
+
+ /* Vendor waits 100ms for Firmware to fully boot */
+ msleep(GOODIX_BERLIN_NORMAL_RESET_DELAY_MS);
+
+ return 0;
+ }
+
+err_dev_reset:
+ gpiod_set_value(cd->reset_gpio, 1);
+
+ regulator_disable(cd->avdd);
+
+err_iovdd_disable:
+ regulator_disable(cd->iovdd);
+
+ return error;
+}
+
+static int goodix_berlin_read_version(struct goodix_berlin_core *cd)
+{
+ u8 buf[sizeof(struct goodix_berlin_fw_version)];
+ int error;
+
+ error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_FW_VERSION_INFO_ADDR, buf, sizeof(buf));
+ if (error) {
+ dev_err(cd->dev, "error reading fw version, %d\n", error);
+ return error;
+ }
+
+ if (!goodix_berlin_checksum_valid(buf, sizeof(buf))) {
+ dev_err(cd->dev, "invalid fw version: checksum error\n");
+ return -EINVAL;
+ }
+
+ memcpy(&cd->fw_version, buf, sizeof(cd->fw_version));
+
+ return 0;
+}
+
+/* Only extract necessary data for runtime */
+static int goodix_berlin_convert_ic_info(struct goodix_berlin_core *cd,
+ const u8 *data, u16 length)
+{
+ struct goodix_berlin_ic_info_misc misc;
+ unsigned int offset = 0;
+ u8 param_num;
+
+ offset += sizeof(__le16); /* length */
+ offset += sizeof(struct goodix_berlin_ic_info_version);
+ offset += sizeof(struct goodix_berlin_ic_info_feature);
+
+ /* IC_INFO Parameters, variable width structure */
+ offset += 4 * sizeof(u8); /* drv_num, sen_num, button_num, force_num */
+
+ if (offset >= length)
+ goto invalid_offset;
+
+ param_num = data[offset++]; /* active_scan_rate_num */
+
+ offset += param_num * sizeof(__le16);
+
+ if (offset >= length)
+ goto invalid_offset;
+
+ param_num = data[offset++]; /* mutual_freq_num*/
+
+ offset += param_num * sizeof(__le16);
+
+ if (offset >= length)
+ goto invalid_offset;
+
+ param_num = data[offset++]; /* self_tx_freq_num */
+
+ offset += param_num * sizeof(__le16);
+
+ if (offset >= length)
+ goto invalid_offset;
+
+ param_num = data[offset++]; /* self_rx_freq_num */
+
+ offset += param_num * sizeof(__le16);
+
+ if (offset >= length)
+ goto invalid_offset;
+
+ param_num = data[offset++]; /* stylus_freq_num */
+
+ offset += param_num * sizeof(__le16);
+
+ if (offset + sizeof(misc) > length)
+ goto invalid_offset;
+
+ /* goodix_berlin_ic_info_misc */
+ memcpy(&misc, &data[offset], sizeof(misc));
+
+ cd->touch_data_addr = le32_to_cpu(misc.touch_data_addr);
+
+ return 0;
+
+invalid_offset:
+ dev_err(cd->dev, "ic_info length is invalid (offset %d length %d)\n",
+ offset, length);
+ return -EINVAL;
+}
+
+static int goodix_berlin_get_ic_info(struct goodix_berlin_core *cd)
+{
+ u8 afe_data[GOODIX_BERLIN_IC_INFO_MAX_LEN];
+ __le16 length_raw;
+ u16 length;
+ int error;
+
+ error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_IC_INFO_ADDR,
+ &length_raw, sizeof(length_raw));
+ if (error) {
+ dev_info(cd->dev, "failed get ic info length, %d\n", error);
+ return error;
+ }
+
+ length = le16_to_cpu(length_raw);
+ if (length >= GOODIX_BERLIN_IC_INFO_MAX_LEN) {
+ dev_info(cd->dev, "invalid ic info length %d\n", length);
+ return -EINVAL;
+ }
+
+ error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_IC_INFO_ADDR,
+ afe_data, length);
+ if (error) {
+ dev_info(cd->dev, "failed get ic info data, %d\n", error);
+ return error;
+ }
+
+ /* check whether the data is valid (ex. bus default values) */
+ if (goodix_berlin_is_dummy_data(cd, (const uint8_t *)afe_data, length)) {
+ dev_err(cd->dev, "fw info data invalid\n");
+ return -EINVAL;
+ }
+
+ if (!goodix_berlin_checksum_valid((const uint8_t *)afe_data, length)) {
+ dev_info(cd->dev, "fw info checksum error\n");
+ return -EINVAL;
+ }
+
+ error = goodix_berlin_convert_ic_info(cd, afe_data, length);
+ if (error)
+ return error;
+
+ /* check some key info */
+ if (!cd->touch_data_addr) {
+ dev_err(cd->dev, "touch_data_addr is null\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static void goodix_berlin_parse_finger(struct goodix_berlin_core *cd,
+ const void *buf, int touch_num)
+{
+ const struct goodix_berlin_touch_data *touch_data = buf;
+ int i;
+
+ /* Check for data validity */
+ for (i = 0; i < touch_num; i++) {
+ unsigned int id;
+
+ id = FIELD_GET(GOODIX_BERLIN_TOUCH_ID_MASK, touch_data[i].id);
+
+ if (id >= GOODIX_BERLIN_MAX_TOUCH) {
+ dev_warn(cd->dev, "invalid finger id %d\n", id);
+ return;
+ }
+ }
+
+ /* Report finger touches */
+ for (i = 0; i < touch_num; i++) {
+ input_mt_slot(cd->input_dev,
+ FIELD_GET(GOODIX_BERLIN_TOUCH_ID_MASK,
+ touch_data[i].id));
+ input_mt_report_slot_state(cd->input_dev, MT_TOOL_FINGER, true);
+
+ touchscreen_report_pos(cd->input_dev, &cd->props,
+ __le16_to_cpu(touch_data[i].x),
+ __le16_to_cpu(touch_data[i].y),
+ true);
+ input_report_abs(cd->input_dev, ABS_MT_TOUCH_MAJOR,
+ __le16_to_cpu(touch_data[i].w));
+ }
+
+ input_mt_sync_frame(cd->input_dev);
+ input_sync(cd->input_dev);
+}
+
+static void goodix_berlin_touch_handler(struct goodix_berlin_core *cd,
+ const void *pre_buf, u32 pre_buf_len)
+{
+ static u8 buffer[GOODIX_BERLIN_IRQ_READ_LEN(GOODIX_BERLIN_MAX_TOUCH)];
+ u8 point_type, touch_num;
+ int error;
+
+ /* copy pre-data to buffer */
+ memcpy(buffer, pre_buf, pre_buf_len);
+
+ touch_num = FIELD_GET(GOODIX_BERLIN_TOUCH_COUNT_MASK,
+ buffer[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]);
+
+ if (touch_num > GOODIX_BERLIN_MAX_TOUCH) {
+ dev_warn(cd->dev, "invalid touch num %d\n", touch_num);
+ return;
+ }
+
+ if (touch_num) {
+ /* read more data if more than 2 touch events */
+ if (unlikely(touch_num > 2)) {
+ error = regmap_raw_read(cd->regmap,
+ cd->touch_data_addr + pre_buf_len,
+ &buffer[pre_buf_len],
+ (touch_num - 2) * GOODIX_BERLIN_BYTES_PER_POINT);
+ if (error) {
+ dev_err_ratelimited(cd->dev, "failed to get touch data, %d\n",
+ error);
+ return;
+ }
+ }
+
+ point_type = FIELD_GET(GOODIX_BERLIN_POINT_TYPE_MASK,
+ buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN]);
+
+ if (point_type == GOODIX_BERLIN_POINT_TYPE_STYLUS ||
+ point_type == GOODIX_BERLIN_POINT_TYPE_STYLUS_HOVER) {
+ dev_warn_once(cd->dev, "Stylus event type not handled\n");
+ return;
+ }
+
+ if (!goodix_berlin_checksum_valid(&buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN],
+ touch_num * GOODIX_BERLIN_BYTES_PER_POINT + 2)) {
+ dev_err(cd->dev, "touch data checksum error, data: %*ph\n",
+ touch_num * GOODIX_BERLIN_BYTES_PER_POINT + 2,
+ &buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN]);
+ return;
+ }
+ }
+
+ goodix_berlin_parse_finger(cd, &buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN],
+ touch_num);
+}
+
+static int goodix_berlin_request_handle_reset(struct goodix_berlin_core *cd)
+{
+ gpiod_set_value(cd->reset_gpio, 1);
+ usleep_range(2000, 2100);
+ gpiod_set_value(cd->reset_gpio, 0);
+
+ msleep(GOODIX_BERLIN_NORMAL_RESET_DELAY_MS);
+
+ return 0;
+}
+
+static irqreturn_t goodix_berlin_threadirq_func(int irq, void *data)
+{
+ struct goodix_berlin_core *cd = data;
+ u8 buf[GOODIX_BERLIN_IRQ_READ_LEN(2)];
+ u8 event_status;
+ int error;
+
+ /* First, read buffer with space for 2 touch events */
+ error = regmap_raw_read(cd->regmap, cd->touch_data_addr, buf,
+ GOODIX_BERLIN_IRQ_READ_LEN(2));
+ if (error) {
+ dev_err_ratelimited(cd->dev, "failed get event head data, %d\n", error);
+ return IRQ_HANDLED;
+ }
+
+ if (buf[GOODIX_BERLIN_STATUS_OFFSET] == 0)
+ return IRQ_HANDLED;
+
+ if (!goodix_berlin_checksum_valid(buf, GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN)) {
+ dev_warn_ratelimited(cd->dev, "touch head checksum err : %*ph\n",
+ GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN, buf);
+ return IRQ_HANDLED;
+ }
+
+ event_status = buf[GOODIX_BERLIN_STATUS_OFFSET];
+
+ if (event_status & GOODIX_BERLIN_TOUCH_EVENT)
+ goodix_berlin_touch_handler(cd, buf, GOODIX_BERLIN_IRQ_READ_LEN(2));
+
+ if (event_status & GOODIX_BERLIN_REQUEST_EVENT) {
+ switch (buf[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]) {
+ case GOODIX_BERLIN_REQUEST_CODE_RESET:
+ goodix_berlin_request_handle_reset(cd);
+ break;
+
+ default:
+ dev_warn(cd->dev, "unsupported request code 0x%x\n",
+ buf[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]);
+ }
+ }
+
+ /* Clear up status field */
+ regmap_write(cd->regmap, cd->touch_data_addr, 0);
+
+ return IRQ_HANDLED;
+}
+
+static int goodix_berlin_input_dev_config(struct goodix_berlin_core *cd,
+ const struct input_id *id)
+{
+ struct input_dev *input_dev;
+ int error;
+
+ input_dev = devm_input_allocate_device(cd->dev);
+ if (!input_dev)
+ return -ENOMEM;
+
+ cd->input_dev = input_dev;
+ input_set_drvdata(input_dev, cd);
+
+ input_dev->name = "Goodix Berlin Capacitive TouchScreen";
+ input_dev->phys = "input/ts";
+
+ input_dev->id = *id;
+
+ input_set_abs_params(cd->input_dev, ABS_MT_POSITION_X, 0, SZ_64K - 1, 0, 0);
+ input_set_abs_params(cd->input_dev, ABS_MT_POSITION_Y, 0, SZ_64K - 1, 0, 0);
+ input_set_abs_params(cd->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
+
+ touchscreen_parse_properties(cd->input_dev, true, &cd->props);
+
+ error = input_mt_init_slots(cd->input_dev, GOODIX_BERLIN_MAX_TOUCH,
+ INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
+ if (error)
+ return error;
+
+ return input_register_device(cd->input_dev);
+}
+
+static int goodix_berlin_pm_suspend(struct device *dev)
+{
+ struct goodix_berlin_core *cd = dev_get_drvdata(dev);
+
+ disable_irq(cd->irq);
+
+ return goodix_berlin_power_on(cd, false);
+}
+
+static int goodix_berlin_pm_resume(struct device *dev)
+{
+ struct goodix_berlin_core *cd = dev_get_drvdata(dev);
+ int error;
+
+ error = goodix_berlin_power_on(cd, true);
+ if (error)
+ return error;
+
+ enable_irq(cd->irq);
+
+ return 0;
+}
+
+EXPORT_GPL_SIMPLE_DEV_PM_OPS(goodix_berlin_pm_ops,
+ goodix_berlin_pm_suspend,
+ goodix_berlin_pm_resume);
+
+static void goodix_berlin_power_off(void *data)
+{
+ struct goodix_berlin_core *cd = data;
+
+ goodix_berlin_power_on(cd, false);
+}
+
+int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
+ struct regmap *regmap)
+{
+ struct goodix_berlin_core *cd;
+ int error;
+
+ if (irq <= 0) {
+ dev_err(dev, "Missing interrupt number\n");
+ return -EINVAL;
+ }
+
+ cd = devm_kzalloc(dev, sizeof(*cd), GFP_KERNEL);
+ if (!cd)
+ return -ENOMEM;
+
+ cd->dev = dev;
+ cd->regmap = regmap;
+ cd->irq = irq;
+
+ cd->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+ if (IS_ERR(cd->reset_gpio))
+ return dev_err_probe(dev, PTR_ERR(cd->reset_gpio),
+ "Failed to request reset gpio\n");
+
+ cd->avdd = devm_regulator_get(dev, "avdd");
+ if (IS_ERR(cd->avdd))
+ return dev_err_probe(dev, PTR_ERR(cd->avdd),
+ "Failed to request avdd regulator\n");
+
+ cd->iovdd = devm_regulator_get(dev, "iovdd");
+ if (IS_ERR(cd->iovdd))
+ return dev_err_probe(dev, PTR_ERR(cd->iovdd),
+ "Failed to request iovdd regulator\n");
+
+ error = goodix_berlin_power_on(cd, true);
+ if (error) {
+ dev_err(dev, "failed power on");
+ return error;
+ }
+
+ error = devm_add_action_or_reset(dev, goodix_berlin_power_off, cd);
+ if (error)
+ return error;
+
+ error = goodix_berlin_read_version(cd);
+ if (error) {
+ dev_err(dev, "failed to get version info");
+ return error;
+ }
+
+ error = goodix_berlin_get_ic_info(cd);
+ if (error) {
+ dev_err(dev, "invalid ic info, abort");
+ return error;
+ }
+
+ error = goodix_berlin_input_dev_config(cd, id);
+ if (error) {
+ dev_err(dev, "failed set input device");
+ return error;
+ }
+
+ error = devm_request_threaded_irq(dev, irq, NULL,
+ goodix_berlin_threadirq_func,
+ IRQF_ONESHOT, "goodix-berlin", cd);
+ if (error) {
+ dev_err(dev, "request threaded irq failed: %d\n", error);
+ return error;
+ }
+
+ dev_set_drvdata(dev, cd);
+
+ dev_dbg(dev, "Goodix Berlin %s Touchscreen Controller", cd->fw_version.patch_pid);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(goodix_berlin_probe);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Goodix Berlin Core Touchscreen driver");
+MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
--
2.34.1
^ permalink raw reply related
* [PATCH v5 4/4] input: touchscreen: add SPI support for Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2023-08-01 12:15 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, Jeff LaBundy
Cc: linux-input, linux-arm-msm, devicetree, linux-kernel,
Neil Armstrong
In-Reply-To: <20230801-topic-goodix-berlin-upstream-initial-v5-0-079252935593@linaro.org>
Add initial support for the new Goodix "Berlin" touchscreen ICs
over the SPI interface.
The driver doesn't use the regmap_spi code since the SPI messages
needs to be prefixed, thus this custom regmap code.
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.
[1] https://github.com/goodix/goodix_ts_berlin
[2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
drivers/input/touchscreen/Kconfig | 14 +++
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/goodix_berlin_spi.c | 173 ++++++++++++++++++++++++++
3 files changed, 188 insertions(+)
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index cc7b88118158..c821fe3ee794 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -433,6 +433,20 @@ config TOUCHSCREEN_GOODIX_BERLIN_I2C
To compile this driver as a module, choose M here: the
module will be called goodix_berlin_i2c.
+config TOUCHSCREEN_GOODIX_BERLIN_SPI
+ tristate "Goodix Berlin SPI touchscreen"
+ depends on SPI_MASTER
+ select REGMAP
+ select TOUCHSCREEN_GOODIX_BERLIN_CORE
+ help
+ Say Y here if you have a Goodix Berlin IC connected to
+ your system via SPI.
+
+ If unsure, say N.
+
+ To compile this driver as a module, choose M here: the
+ module will be called goodix_berlin_spi.
+
config TOUCHSCREEN_HIDEEP
tristate "HiDeep Touch IC"
depends on I2C
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 7ef677cf7a10..a81cb5aa21a5 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_CORE) += goodix_berlin_core.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_I2C) += goodix_berlin_i2c.o
+obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_SPI) += goodix_berlin_spi.o
obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
diff --git a/drivers/input/touchscreen/goodix_berlin_spi.c b/drivers/input/touchscreen/goodix_berlin_spi.c
new file mode 100644
index 000000000000..1f57c3592918
--- /dev/null
+++ b/drivers/input/touchscreen/goodix_berlin_spi.c
@@ -0,0 +1,173 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Goodix Berlin Touchscreen Driver
+ *
+ * Copyright (C) 2020 - 2021 Goodix, Inc.
+ * Copyright (C) 2023 Linaro Ltd.
+ *
+ * Based on goodix_ts_berlin driver.
+ */
+#include <asm/unaligned.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+
+#include "goodix_berlin.h"
+
+#define GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN 1
+#define GOODIX_BERLIN_REGISTER_WIDTH 4
+#define GOODIX_BERLIN_SPI_READ_DUMMY_LEN 3
+#define GOODIX_BERLIN_SPI_READ_PREFIX_LEN (GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN + \
+ GOODIX_BERLIN_REGISTER_WIDTH + \
+ GOODIX_BERLIN_SPI_READ_DUMMY_LEN)
+#define GOODIX_BERLIN_SPI_WRITE_PREFIX_LEN (GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN + \
+ GOODIX_BERLIN_REGISTER_WIDTH)
+
+#define GOODIX_BERLIN_SPI_WRITE_FLAG 0xF0
+#define GOODIX_BERLIN_SPI_READ_FLAG 0xF1
+
+static int goodix_berlin_spi_read(void *context, const void *reg_buf,
+ size_t reg_size, void *val_buf,
+ size_t val_size)
+{
+ struct spi_device *spi = context;
+ struct spi_transfer xfers;
+ struct spi_message spi_msg;
+ const u32 *reg = reg_buf; /* reg is stored as native u32 at start of buffer */
+ u8 *buf;
+ int error;
+
+ if (reg_size != GOODIX_BERLIN_REGISTER_WIDTH)
+ return -EINVAL;
+
+ buf = kzalloc(GOODIX_BERLIN_SPI_READ_PREFIX_LEN + val_size, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ spi_message_init(&spi_msg);
+ memset(&xfers, 0, sizeof(xfers));
+
+ /* buffer format: 0xF1 + addr(4bytes) + dummy(3bytes) + data */
+ buf[0] = GOODIX_BERLIN_SPI_READ_FLAG;
+ put_unaligned_be32(*reg, buf + GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN);
+ memset(buf + GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN + GOODIX_BERLIN_REGISTER_WIDTH,
+ 0xff, GOODIX_BERLIN_SPI_READ_DUMMY_LEN);
+
+ xfers.tx_buf = buf;
+ xfers.rx_buf = buf;
+ xfers.len = GOODIX_BERLIN_SPI_READ_PREFIX_LEN + val_size;
+ xfers.cs_change = 0;
+ spi_message_add_tail(&xfers, &spi_msg);
+
+ error = spi_sync(spi, &spi_msg);
+ if (error < 0)
+ dev_err(&spi->dev, "spi transfer error, %d", error);
+ else
+ memcpy(val_buf, buf + GOODIX_BERLIN_SPI_READ_PREFIX_LEN, val_size);
+
+ kfree(buf);
+ return error;
+}
+
+static int goodix_berlin_spi_write(void *context, const void *data,
+ size_t count)
+{
+ unsigned int len = count - GOODIX_BERLIN_REGISTER_WIDTH;
+ struct spi_device *spi = context;
+ struct spi_transfer xfers;
+ struct spi_message spi_msg;
+ const u32 *reg = data; /* reg is stored as native u32 at start of buffer */
+ u8 *buf;
+ int error;
+
+ buf = kzalloc(GOODIX_BERLIN_SPI_WRITE_PREFIX_LEN + len, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ spi_message_init(&spi_msg);
+ memset(&xfers, 0, sizeof(xfers));
+
+ buf[0] = GOODIX_BERLIN_SPI_WRITE_FLAG;
+ put_unaligned_be32(*reg, buf + GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN);
+ memcpy(buf + GOODIX_BERLIN_SPI_WRITE_PREFIX_LEN,
+ data + GOODIX_BERLIN_REGISTER_WIDTH, len);
+
+ xfers.tx_buf = buf;
+ xfers.len = GOODIX_BERLIN_SPI_WRITE_PREFIX_LEN + len;
+ xfers.cs_change = 0;
+ spi_message_add_tail(&xfers, &spi_msg);
+
+ error = spi_sync(spi, &spi_msg);
+ if (error < 0)
+ dev_err(&spi->dev, "spi transfer error, %d", error);
+
+ kfree(buf);
+ return error;
+}
+
+static const struct regmap_config goodix_berlin_spi_regmap_conf = {
+ .reg_bits = 32,
+ .val_bits = 8,
+ .read = goodix_berlin_spi_read,
+ .write = goodix_berlin_spi_write,
+};
+
+/* vendor & product left unassigned here, should probably be updated from fw info */
+static const struct input_id goodix_berlin_spi_input_id = {
+ .bustype = BUS_SPI,
+};
+
+static int goodix_berlin_spi_probe(struct spi_device *spi)
+{
+ struct regmap_config regmap_config;
+ struct regmap *regmap;
+ size_t max_size;
+ int error = 0;
+
+ spi->mode = SPI_MODE_0;
+ spi->bits_per_word = 8;
+ error = spi_setup(spi);
+ if (error)
+ return error;
+
+ max_size = spi_max_transfer_size(spi);
+
+ regmap_config = goodix_berlin_spi_regmap_conf;
+ regmap_config.max_raw_read = max_size - GOODIX_BERLIN_SPI_READ_PREFIX_LEN;
+ regmap_config.max_raw_write = max_size - GOODIX_BERLIN_SPI_WRITE_PREFIX_LEN;
+
+ regmap = devm_regmap_init(&spi->dev, NULL, spi, ®map_config);
+ if (IS_ERR(regmap))
+ return PTR_ERR(regmap);
+
+ return goodix_berlin_probe(&spi->dev, spi->irq,
+ &goodix_berlin_spi_input_id, regmap);
+}
+
+static const struct spi_device_id goodix_berlin_spi_ids[] = {
+ { "gt9916" },
+ { },
+};
+MODULE_DEVICE_TABLE(spi, goodix_berlin_spi_ids);
+
+static const struct of_device_id goodix_berlin_spi_of_match[] = {
+ { .compatible = "goodix,gt9916", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, goodix_berlin_spi_of_match);
+
+static struct spi_driver goodix_berlin_spi_driver = {
+ .driver = {
+ .name = "goodix-berlin-spi",
+ .of_match_table = goodix_berlin_spi_of_match,
+ .pm = pm_sleep_ptr(&goodix_berlin_pm_ops),
+ },
+ .probe = goodix_berlin_spi_probe,
+ .id_table = goodix_berlin_spi_ids,
+};
+module_spi_driver(goodix_berlin_spi_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Goodix Berlin SPI Touchscreen driver");
+MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
--
2.34.1
^ permalink raw reply related
* Re: [PATCH] Input: pwm-beeper - Support volume setting via sysfs
From: Takashi Iwai @ 2023-08-01 12:25 UTC (permalink / raw)
To: Marek Vasut
Cc: Jeff LaBundy, Dmitry Torokhov, linux-input, Uwe Kleine-König,
Frieder Schrempf, Manuel Traut, Thierry Reding, linux-pwm,
alsa-devel, Jaroslav Kysela, Takashi Iwai
In-Reply-To: <ce419a1e-bf1a-1087-a7ee-37f85a11c596@denx.de>
On Tue, 01 Aug 2023 13:38:54 +0200,
Marek Vasut wrote:
>
> On 8/1/23 08:11, Takashi Iwai wrote:
> > On Tue, 01 Aug 2023 04:56:09 +0200,
> > Jeff LaBundy wrote:
> >>
> >> 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.
> >
> > Nah, please forget pcsp driver. As mentioned earlier, it's a driver
> > that is present just for fun.
> >
> > I believe what we need is a simple sound card instance providing a
> > mixer control for the beep volume, something like a patch like below
> > (totally untested!)
>
> Do we really want to add dependency on the entire sound subsystem
> (which is currently not needed on the device I care about) only to
> configure one single tunable of the PWM beeper ? It seems to add too
> much bloat to me.
That really depends on the use case. If the driver is supposed to be
used generically as seen in the desktop scenes, it's worth to have a
support of the standard interface like others. OTOH, if the driver is
for limited situations and better to be as slim as possible, a
tailored interface like sysfs would be the way to go. My proposal was
under assumption of the former -- a generic usage. If the latter
scene is expected, a sysfs implementation can be the right way, IMO.
OTOH, we really need to be careful about the blind extension of API.
Although adding a new input event type sounds easy, the influence
could be much more than seen there...
Takashi
^ permalink raw reply
* Re: [PATCH v4 00/11] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together
From: Doug Anderson @ 2023-08-01 14:49 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Frank Rowand, Krzysztof Kozlowski, Conor Dooley,
Neil Armstrong, Sam Ravnborg, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, linux-arm-msm, yangcong5, devicetree,
Daniel Vetter, hsinyi, Chris Morgan, linux-input,
cros-qcom-dts-watchers, Dmitry Torokhov, linux-kernel, dri-devel
In-Reply-To: <CAD=FV=Wo5H-XohCz_sLnV=oT5cRm2tFrSAG9kpCGzK7pjSW_Xg@mail.gmail.com>
Hi,
On Fri, Jul 28, 2023 at 10:24 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Fri, Jul 28, 2023 at 8:31 AM Benjamin Tissoires <bentiss@kernel.org> wrote:
> >
> > On Jul 27 2023, Douglas Anderson wrote:
> > >
> > > The big motivation for this patch series is mostly described in the patch
> > > ("drm/panel: Add a way for other devices to follow panel state"), but to
> > > quickly summarize here: for touchscreens that are connected to a panel we
> > > need the ability to power sequence the two device together. This is not a
> > > new need, but so far we've managed to get by through a combination of
> > > inefficiency, added costs, or perhaps just a little bit of brokenness.
> > > It's time to do better. This patch series allows us to do better.
> > >
> > > Assuming that people think this patch series looks OK, we'll have to
> > > figure out the right way to land it. The panel patches and i2c-hid
> > > patches will go through very different trees and so either we'll need
> > > an Ack from one side or the other or someone to create a tag for the
> > > other tree to pull in. This will _probably_ require the true drm-misc
> > > maintainers to get involved, not a lowly committer. ;-)
> > >
> > > Version 4 of this series adds a new patch that suspends i2c-hid
> > > devices at remove time even for non panel-followers to make things
> > > consistent. It also attempts to isolate the panel follower code a bit
> > > more as per Benjamin's feedback on v3 and adds an item to the DRM todo
> > > list as per Maxime's request. As per Maxime's response to my v3 cover
> > > letter, I added his Reviewed-by tag to all 10 patches that were part
> > > of v3 (but left it off of the new i2c-hid patch in v4).
> > >
> > > Version 3 of this series was a long time coming after v2. Maxime and I
> > > had a very long discussion trying to figure out if there was a beter
> > > way and in the end we didn't find one so he was OK with the series in
> > > general [1]. After that got resolved, I tried to resolve Benjamin's
> > > feedback but got stuck [2]. Eventually I made my best guess. The end
> > > result was a v3 that wasn't that different from v2 but that had a tiny
> > > bit more code split out.
> > >
> > > Version 2 of this patch series didn't change too much. At a high level:
> > > * I added all the forgotten "static" to functions.
> > > * I've hopefully made the bindings better.
> > > * I've integrated into fw_devlink.
> > > * I cleaned up a few descriptions / comments.
> > >
> > > As far as I can tell, as of v4 everyone is on the same page that this
> > > patch series looks like a reasonable solution to the problem and we
> > > just need to get all the nits fixed and figure out how to land it.
> >
> > Thanks a lot for the new version. I like it much more on the HID side:
> >
> > for the HID part:
> > Reviewed-by: Benjamin Tissoires <bentiss@kernel.org>
> >
> > I wouldn't mind having this series taken from the drm tree if that is
> > easier. i2c-hid is a low patch rate driver, so having it updated through
> > DRM should not be an issue.
> >
> > In that case:
> > Acked-by: Benjamin Tissoires <bentiss@kernel.org>
>
> Thanks for your reviews and your help getting this whipped into shape.
>
> Lading through drm makes sense to me. I'm a drm committer, so with
> your Ack I believe it should be fine for me to land the series (minus
> the dts) in drm-misc-next. This series has been around for a while,
> has been reviewed by relevant folks, and the last few changes haven't
> fundamentally changed anything about the design, so I'm not going to
> twiddle my thumbs too long. That being said, I'll still plan to wait
> until early next week (Tuesday?) before landing to allow for any last
> minute shouts.
>
> Given how drm-misc works [1] and the fact that mainline is currently
> at v6.5-rc3 (it will be -rc4 when I land it), I'd expect that these
> commits will find their way into v6.6.
>
> [1] https://drm.pages.freedesktop.org/maintainer-tools/drm-misc.html
Pushed the first 10 patches to drm-misc-next. Bjorn: whenever it's
convenient you could land patch #11 (the device tree change) into the
Qualcomm tree.
76edfcf430cc HID: i2c-hid: Do panel follower work on the system_wq
96a37bfd232a HID: i2c-hid: Support being a panel follower
5f8838e9405d HID: i2c-hid: Suspend i2c-hid devices in remove
d93d28477222 HID: i2c-hid: Make suspend and resume into helper functions
675cd877c952 HID: i2c-hid: Rearrange probe() to power things up later
a889ee12d53d HID: i2c-hid: Switch to SYSTEM_SLEEP_PM_OPS()
fbf0ea2da3c7 of: property: fw_devlink: Add a devlink for panel followers
de0874165b83 drm/panel: Add a way for other devices to follow panel state
d2aacaf07395 drm/panel: Check for already prepared/enabled in drm_panel
2ca376ef18f6 dt-bindings: HID: i2c-hid: Add "panel" property to
i2c-hid backed touchscreens
-Doug
^ permalink raw reply
* Re: [PATCH -next] Input: serio - fix possible memory leak while device_add() fails
From: Dmitry Torokhov @ 2023-08-01 23:37 UTC (permalink / raw)
To: Zhu Wang; +Cc: rafael, christophe.jaillet, gregkh, linux-input
In-Reply-To: <20230801115105.76690-1-wangzhu9@huawei.com>
Hi Zhu,
On Tue, Aug 01, 2023 at 07:51:05PM +0800, Zhu Wang wrote:
> If device_add() returns error, the name allocated by dev_set_name() need
> be freed. As comment of device_add() says, it should use put_device() to
> release the reference in the error path. So fix this by calling
> put_device, then the name can be freed in kobject_cleanp().
Unfortunately this is not as simple. To support serio ports chained
behind other serio ports (to accommodate Synaptics touchpads with
Trackpoints connected to pass-through ports) serio device registration
is done on a thread. So even if you free the device the driver that
ultimately issued serio_add_port() call will not be aware of the failure
and will continue using the device, which will much more likely crash
the kernel as opposed to leaving the device structure around.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Zhu Wang <wangzhu9@huawei.com>
> ---
> drivers/input/serio/serio.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/input/serio/serio.c b/drivers/input/serio/serio.c
> index 767fc9efb4a8..d3bb6ec91326 100644
> --- a/drivers/input/serio/serio.c
> +++ b/drivers/input/serio/serio.c
> @@ -537,10 +537,12 @@ static void serio_add_port(struct serio *serio)
> serio->start(serio);
>
> error = device_add(&serio->dev);
> - if (error)
> + if (error) {
> + put_device(&serio->dev);
> dev_err(&serio->dev,
> "device_add() failed for %s (%s), error: %d\n",
> serio->phys, serio->name, error);
> + }
> }
>
> /*
> --
> 2.17.1
>
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH -next] input: serio: fix the return value handle for platform_get_irq()
From: Dmitry Torokhov @ 2023-08-01 23:54 UTC (permalink / raw)
To: Ruan Jinjie; +Cc: wsa+renesas, christophe.jaillet, linux-input
In-Reply-To: <20230731122246.2028651-1-ruanjinjie@huawei.com>
On Mon, Jul 31, 2023 at 08:22:46PM +0800, Ruan Jinjie wrote:
> 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>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* [PATCH v6 0/2] Add ili9882t bindings and timing
From: Cong Yang @ 2023-08-02 7:19 UTC (permalink / raw)
To: robh+dt, krzysztof.kozlowski+dt, conor+dt, dmitry.torokhov,
dianders, jikos, benjamin.tissoires, hsinyi
Cc: linux-input, devicetree, linux-kernel, Cong Yang
Add bindings for Ilitek. The ili9882t touch screen chip same as
Elan eKTH6915 controller has a reset gpio. The difference is that
ilitek9882 needs to use vccio-supply instead of vcc33-supply.
From Dmitry suggestion, it would make more sense to distinguish the
binging of ili9882 and eKTH6915.
From The datasheet specifies there should be 60ms between touch SDA
sleep and panel RESX. so we can add the 65 ms delay in i2c_hid_core_suspend.
As Doug said, only the panel follower patch series[1] land makes sense, and now the panel follower
patch series[1] has land.
[1]: https://lore.kernel.org/all/20230727171750.633410-1-dianders@chromium.org
Changes in v6:
- PATCH 1/2: Modify subject.
- Link to v5: https://lore.kernel.org/all/20230609063615.758676-1-yangcong5@huaqin.corp-partner.google.com/
Changes in v5:
- PATCH 1/2: Add panel as a required in property and examples.
- PATCH 2/2: Set a NULL to ili9882t_chip_data for vcc33-supply, then not use vcc33 regulator.
- Link to v4: https://lore.kernel.org/all/20230608130147.2835818-1-yangcong5@huaqin.corp-partner.google.com/
Changes in v4:
- PATCH 1/2: Remove compatible items and add reset maxItems.
- PATCH 1/2: Refer to the panel description in Doug serias[1].
[1] https://lore.kernel.org/all/20230607144931.v2.1.Id68e30343bb1e11470582a9078b086176cfec46b@changeid/
- PATCH 2/2: Set a "null" to ili9882t_chip_data for vcc33-supply, then using dummy regulator.
- Link to v3: https://lore.kernel.org/all/20230607133458.4075667-1-yangcong5@huaqin.corp-partner.google.com/
Changes in v3:
- PATCH 1/2: Introduce bindings for Ilitek.
- Link to v2: https://lore.kernel.org/all/20230605060524.1178164-1-yangcong5@huaqin.corp-partner.google.com/
Changes in v2:
- PATCH 1/2: fix ran make dt_binding_check warnings/errors.
- PATCH 1/2: remove oneOf,just enum.
- Link to v1: https://lore.kernel.org/all/20230602140948.2138668-1-yangcong5@huaqin.corp-partner.google.com/
Cong Yang (2):
dt-bindings: input: i2c-hid: Introduce Ilitek ili9882t
HID: i2c-hid: elan: Add ili9882t timing
.../bindings/input/ilitek,ili9882t.yaml | 67 +++++++++++++++++++
drivers/hid/i2c-hid/i2c-hid-of-elan.c | 50 ++++++++++----
2 files changed, 105 insertions(+), 12 deletions(-)
create mode 100644 Documentation/devicetree/bindings/input/ilitek,ili9882t.yaml
--
2.25.1
^ permalink raw reply
* [PATCH v6 1/2] dt-bindings: input: i2c-hid: Introduce Ilitek ili9882t
From: Cong Yang @ 2023-08-02 7:19 UTC (permalink / raw)
To: robh+dt, krzysztof.kozlowski+dt, conor+dt, dmitry.torokhov,
dianders, jikos, benjamin.tissoires, hsinyi
Cc: linux-input, devicetree, linux-kernel, Cong Yang,
Krzysztof Kozlowski
In-Reply-To: <20230802071947.1683318-1-yangcong5@huaqin.corp-partner.google.com>
The ili9882t touch screen chip same as Elan eKTH6915 controller
has a reset gpio. The difference is that ili9882t needs to use
vccio-supply instead of vcc33-supply. Doug's series[1] allows panels
and touchscreens to power on/off together, let's add a phandle for this.
[1]: https://lore.kernel.org/r/20230607215224.2067679-1-dianders@chromium.org
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
---
.../bindings/input/ilitek,ili9882t.yaml | 67 +++++++++++++++++++
1 file changed, 67 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/ilitek,ili9882t.yaml
diff --git a/Documentation/devicetree/bindings/input/ilitek,ili9882t.yaml b/Documentation/devicetree/bindings/input/ilitek,ili9882t.yaml
new file mode 100644
index 000000000000..c5d9e0e919f9
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/ilitek,ili9882t.yaml
@@ -0,0 +1,67 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/ilitek,ili9882t.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Ilitek ili9882t touchscreen controller
+
+maintainers:
+ - Cong Yang <yangcong5@huaqin.corp-partner.google.com>
+
+description:
+ Supports the Ilitek ili9882t touchscreen controller.
+ This touchscreen controller uses the i2c-hid protocol with a reset GPIO.
+
+allOf:
+ - $ref: /schemas/input/touchscreen/touchscreen.yaml#
+
+properties:
+ compatible:
+ const: ilitek,ili9882t
+
+ reg:
+ const: 0x41
+
+ interrupts:
+ maxItems: 1
+
+ panel: true
+
+ reset-gpios:
+ maxItems: 1
+ description: Reset GPIO.
+
+ vccio-supply:
+ description: The 1.8V supply to the touchscreen.
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - panel
+ - vccio-supply
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ touchscreen: touchscreen@41 {
+ compatible = "ilitek,ili9882t";
+ reg = <0x41>;
+
+ interrupt-parent = <&pio>;
+ interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
+
+ panel = <&panel>;
+ reset-gpios = <&pio 60 GPIO_ACTIVE_LOW>;
+ vccio-supply = <&mt6366_vio18_reg>;
+ };
+ };
--
2.25.1
^ permalink raw reply related
* [PATCH v6 2/2] HID: i2c-hid: elan: Add ili9882t timing
From: Cong Yang @ 2023-08-02 7:19 UTC (permalink / raw)
To: robh+dt, krzysztof.kozlowski+dt, conor+dt, dmitry.torokhov,
dianders, jikos, benjamin.tissoires, hsinyi
Cc: linux-input, devicetree, linux-kernel, Cong Yang,
Douglas Anderson
In-Reply-To: <20230802071947.1683318-1-yangcong5@huaqin.corp-partner.google.com>
The ili9882t is a TDDI IC (Touch with Display Driver). The
datasheet specifies there should be 60ms between touch SDA
sleep and panel RESX. Doug's series[1] allows panels and
touchscreens to power on/off together, so we can add the 65 ms
delay in i2c_hid_core_suspend before panel_unprepare.
Because ili9882t touchscrgeen is a panel follower, and
needs to use vccio-supply instead of vcc33-supply, so set
it NULL to ili9882t_chip_data, then not use vcc33 regulator.
[1]: https://lore.kernel.org/all/20230727171750.633410-1-dianders@chromium.org
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
---
drivers/hid/i2c-hid/i2c-hid-of-elan.c | 50 ++++++++++++++++++++-------
1 file changed, 38 insertions(+), 12 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-of-elan.c b/drivers/hid/i2c-hid/i2c-hid-of-elan.c
index 029045d9661c..31abab57ad44 100644
--- a/drivers/hid/i2c-hid/i2c-hid-of-elan.c
+++ b/drivers/hid/i2c-hid/i2c-hid-of-elan.c
@@ -18,9 +18,11 @@
#include "i2c-hid.h"
struct elan_i2c_hid_chip_data {
- unsigned int post_gpio_reset_delay_ms;
+ unsigned int post_gpio_reset_on_delay_ms;
+ unsigned int post_gpio_reset_off_delay_ms;
unsigned int post_power_delay_ms;
u16 hid_descriptor_address;
+ const char *main_supply_name;
};
struct i2c_hid_of_elan {
@@ -38,9 +40,11 @@ static int elan_i2c_hid_power_up(struct i2chid_ops *ops)
container_of(ops, struct i2c_hid_of_elan, ops);
int ret;
- ret = regulator_enable(ihid_elan->vcc33);
- if (ret)
- return ret;
+ if (ihid_elan->vcc33) {
+ ret = regulator_enable(ihid_elan->vcc33);
+ if (ret)
+ return ret;
+ }
ret = regulator_enable(ihid_elan->vccio);
if (ret) {
@@ -52,8 +56,8 @@ static int elan_i2c_hid_power_up(struct i2chid_ops *ops)
msleep(ihid_elan->chip_data->post_power_delay_ms);
gpiod_set_value_cansleep(ihid_elan->reset_gpio, 0);
- if (ihid_elan->chip_data->post_gpio_reset_delay_ms)
- msleep(ihid_elan->chip_data->post_gpio_reset_delay_ms);
+ if (ihid_elan->chip_data->post_gpio_reset_on_delay_ms)
+ msleep(ihid_elan->chip_data->post_gpio_reset_on_delay_ms);
return 0;
}
@@ -64,8 +68,12 @@ static void elan_i2c_hid_power_down(struct i2chid_ops *ops)
container_of(ops, struct i2c_hid_of_elan, ops);
gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1);
+ if (ihid_elan->chip_data->post_gpio_reset_off_delay_ms)
+ msleep(ihid_elan->chip_data->post_gpio_reset_off_delay_ms);
+
regulator_disable(ihid_elan->vccio);
- regulator_disable(ihid_elan->vcc33);
+ if (ihid_elan->vcc33)
+ regulator_disable(ihid_elan->vcc33);
}
static int i2c_hid_of_elan_probe(struct i2c_client *client)
@@ -89,24 +97,42 @@ static int i2c_hid_of_elan_probe(struct i2c_client *client)
if (IS_ERR(ihid_elan->vccio))
return PTR_ERR(ihid_elan->vccio);
- ihid_elan->vcc33 = devm_regulator_get(&client->dev, "vcc33");
- if (IS_ERR(ihid_elan->vcc33))
- return PTR_ERR(ihid_elan->vcc33);
-
ihid_elan->chip_data = device_get_match_data(&client->dev);
+ if (ihid_elan->chip_data->main_supply_name) {
+ ihid_elan->vcc33 = devm_regulator_get(&client->dev,
+ ihid_elan->chip_data->main_supply_name);
+ if (IS_ERR(ihid_elan->vcc33))
+ return PTR_ERR(ihid_elan->vcc33);
+ }
+
return i2c_hid_core_probe(client, &ihid_elan->ops,
ihid_elan->chip_data->hid_descriptor_address, 0);
}
static const struct elan_i2c_hid_chip_data elan_ekth6915_chip_data = {
.post_power_delay_ms = 1,
- .post_gpio_reset_delay_ms = 300,
+ .post_gpio_reset_on_delay_ms = 300,
+ .hid_descriptor_address = 0x0001,
+ .main_supply_name = "vcc33",
+};
+
+static const struct elan_i2c_hid_chip_data ilitek_ili9882t_chip_data = {
+ .post_power_delay_ms = 1,
+ .post_gpio_reset_on_delay_ms = 200,
+ .post_gpio_reset_off_delay_ms = 65,
.hid_descriptor_address = 0x0001,
+ /*
+ * this touchscreen is tightly integrated with the panel and assumes
+ * that the relevant power rails (other than the IO rail) have already
+ * been turned on by the panel driver because we're a panel follower.
+ */
+ .main_supply_name = NULL,
};
static const struct of_device_id elan_i2c_hid_of_match[] = {
{ .compatible = "elan,ekth6915", .data = &elan_ekth6915_chip_data },
+ { .compatible = "ilitek,ili9882t", .data = &ilitek_ili9882t_chip_data },
{ }
};
MODULE_DEVICE_TABLE(of, elan_i2c_hid_of_match);
--
2.25.1
^ permalink raw reply related
* [PATCH] HID: i2c-hid: add more DRM dependencies
From: Arnd Bergmann @ 2023-08-02 12:49 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Douglas Anderson, Maxime Ripard
Cc: Arnd Bergmann, Hans de Goede, linux-input, linux-kernel
From: Arnd Bergmann <arnd@arndb.de>
When a symbol is selected that has extra dependencies,
anything that selects it must have the same dependencies.
With the added CONFIG_DRM reference from I2C_HID_CORE,
this broke a couple of drivers that now also depend
on DRM:
WARNING: unmet direct dependencies detected for I2C_HID_CORE
Depends on [m]: HID_SUPPORT [=y] && I2C_HID [=y] && (DRM [=m] || !DRM [=m])
Selected by [y]:
- I2C_HID_OF [=y] && HID_SUPPORT [=y] && I2C_HID [=y]
- I2C_HID_ACPI [=y] && HID_SUPPORT [=y] && I2C_HID [=y] && ACPI [=y]
- I2C_HID_OF_GOODIX [=y] && HID_SUPPORT [=y] && I2C_HID [=y] && OF [=y]
x86_64-linux-ld: vmlinux.o: in function `i2c_hid_core_remove':
(.text+0xfc8826): undefined reference to `drm_panel_remove_follower'
x86_64-linux-ld: vmlinux.o: in function `i2c_hid_core_probe':
(.text+0xfc8da0): undefined reference to `drm_is_panel_follower'
Add the corresponding DRM||!DRM dependencies on each one that
is affected.
Fixes: 96a37bfd232ae ("HID: i2c-hid: Support being a panel follower")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/hid/i2c-hid/Kconfig | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/hid/i2c-hid/Kconfig b/drivers/hid/i2c-hid/Kconfig
index 2bdb55203104b..ef7c595c9403c 100644
--- a/drivers/hid/i2c-hid/Kconfig
+++ b/drivers/hid/i2c-hid/Kconfig
@@ -9,6 +9,7 @@ if I2C_HID
config I2C_HID_ACPI
tristate "HID over I2C transport layer ACPI driver"
depends on ACPI
+ depends on DRM || !DRM
select I2C_HID_CORE
help
Say Y here if you use a keyboard, a touchpad, a touchscreen, or any
@@ -25,6 +26,7 @@ config I2C_HID_OF
tristate "HID over I2C transport layer Open Firmware driver"
# No "depends on OF" because this can also be used for manually
# (board-file) instantiated "hid-over-i2c" type i2c-clients.
+ depends on DRM || !DRM
select I2C_HID_CORE
help
Say Y here if you use a keyboard, a touchpad, a touchscreen, or any
@@ -41,6 +43,7 @@ config I2C_HID_OF
config I2C_HID_OF_ELAN
tristate "Driver for Elan hid-i2c based devices on OF systems"
depends on OF
+ depends on DRM || !DRM
select I2C_HID_CORE
help
Say Y here if you want support for Elan i2c devices that use
@@ -56,6 +59,7 @@ config I2C_HID_OF_ELAN
config I2C_HID_OF_GOODIX
tristate "Driver for Goodix hid-i2c based devices on OF systems"
depends on OF
+ depends on DRM || !DRM
select I2C_HID_CORE
help
Say Y here if you want support for Goodix i2c devices that use
--
2.39.2
^ permalink raw reply related
* Re: [PATCH 03/22] Input: iqs269a - use device core to create driver-specific device attributes
From: Mattijs Korpershoek @ 2023-08-02 13:01 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input
Cc: linux-kernel, Greg Kroah-Hartman, Jeff LaBundy
In-Reply-To: <20230729005133.1095051-3-dmitry.torokhov@gmail.com>
On ven., juil. 28, 2023 at 17:51, Dmitry Torokhov <dmitry.torokhov@gmail.com> 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: Mattijs Korpershoek <mkorpershoek@baylibre.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
* [dtor-input:next] BUILD SUCCESS 54116d442e001e1b6bd482122043b1870998a1f3
From: kernel test robot @ 2023-08-02 13:06 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
branch HEAD: 54116d442e001e1b6bd482122043b1870998a1f3 Input: rpckbd - fix the return value handle for platform_get_irq()
elapsed time: 726m
configs tested: 146
configs skipped: 8
The following configs have been built successfully.
More configs may be tested in the coming days.
tested configs:
alpha allyesconfig gcc
alpha defconfig gcc
alpha randconfig-r023-20230731 gcc
alpha randconfig-r032-20230731 gcc
alpha randconfig-r036-20230731 gcc
arc allyesconfig gcc
arc defconfig gcc
arc randconfig-r026-20230731 gcc
arc randconfig-r043-20230731 gcc
arc randconfig-r043-20230801 gcc
arm allmodconfig gcc
arm allyesconfig gcc
arm defconfig gcc
arm randconfig-r004-20230801 clang
arm randconfig-r024-20230731 gcc
arm randconfig-r033-20230731 clang
arm randconfig-r034-20230731 clang
arm randconfig-r046-20230731 gcc
arm randconfig-r046-20230801 gcc
arm64 allyesconfig gcc
arm64 defconfig gcc
arm64 randconfig-r004-20230731 gcc
arm64 randconfig-r033-20230731 gcc
csky defconfig gcc
csky randconfig-r012-20230801 gcc
csky randconfig-r031-20230731 gcc
hexagon randconfig-r003-20230731 clang
hexagon randconfig-r012-20230731 clang
hexagon randconfig-r015-20230731 clang
hexagon randconfig-r016-20230801 clang
hexagon randconfig-r021-20230731 clang
hexagon randconfig-r041-20230731 clang
hexagon randconfig-r041-20230801 clang
hexagon randconfig-r045-20230731 clang
hexagon randconfig-r045-20230801 clang
i386 allyesconfig gcc
i386 buildonly-randconfig-r004-20230731 gcc
i386 buildonly-randconfig-r004-20230802 clang
i386 buildonly-randconfig-r005-20230731 gcc
i386 buildonly-randconfig-r005-20230802 clang
i386 buildonly-randconfig-r006-20230731 gcc
i386 buildonly-randconfig-r006-20230802 clang
i386 debian-10.3 gcc
i386 defconfig gcc
i386 randconfig-i001-20230731 gcc
i386 randconfig-i002-20230731 gcc
i386 randconfig-i003-20230731 gcc
i386 randconfig-i004-20230731 gcc
i386 randconfig-i005-20230731 gcc
i386 randconfig-i006-20230731 gcc
i386 randconfig-i011-20230801 clang
i386 randconfig-i012-20230801 clang
i386 randconfig-i013-20230801 clang
i386 randconfig-i014-20230801 clang
i386 randconfig-i015-20230801 clang
i386 randconfig-i016-20230801 clang
i386 randconfig-r014-20230731 clang
loongarch allmodconfig gcc
loongarch allnoconfig gcc
loongarch defconfig gcc
m68k allmodconfig gcc
m68k allyesconfig gcc
m68k defconfig gcc
microblaze randconfig-r005-20230801 gcc
microblaze randconfig-r013-20230801 gcc
microblaze randconfig-r025-20230731 gcc
mips allmodconfig gcc
mips allyesconfig gcc
mips randconfig-r011-20230801 gcc
mips randconfig-r022-20230731 gcc
mips randconfig-r036-20230731 clang
nios2 defconfig gcc
nios2 randconfig-r001-20230801 gcc
nios2 randconfig-r011-20230731 gcc
openrisc randconfig-r003-20230731 gcc
openrisc randconfig-r032-20230731 gcc
parisc allyesconfig gcc
parisc defconfig gcc
parisc randconfig-r023-20230731 gcc
parisc randconfig-r024-20230731 gcc
parisc randconfig-r036-20230731 gcc
parisc64 defconfig gcc
powerpc allmodconfig gcc
powerpc allnoconfig gcc
powerpc randconfig-r004-20230731 gcc
powerpc randconfig-r014-20230801 clang
powerpc randconfig-r035-20230731 gcc
riscv allmodconfig gcc
riscv allnoconfig gcc
riscv allyesconfig gcc
riscv defconfig gcc
riscv randconfig-r006-20230731 gcc
riscv randconfig-r021-20230731 clang
riscv randconfig-r031-20230731 gcc
riscv randconfig-r042-20230731 clang
riscv randconfig-r042-20230801 clang
riscv rv32_defconfig gcc
s390 allmodconfig gcc
s390 allyesconfig gcc
s390 defconfig gcc
s390 randconfig-r015-20230801 clang
s390 randconfig-r031-20230731 gcc
s390 randconfig-r044-20230731 clang
s390 randconfig-r044-20230801 clang
sh allmodconfig gcc
sh randconfig-r005-20230731 gcc
sh randconfig-r006-20230801 gcc
sparc allyesconfig gcc
sparc defconfig gcc
sparc randconfig-r002-20230731 gcc
sparc randconfig-r003-20230801 gcc
sparc64 randconfig-r001-20230731 gcc
sparc64 randconfig-r013-20230731 gcc
sparc64 randconfig-r016-20230731 gcc
um allmodconfig clang
um allnoconfig clang
um allyesconfig clang
um defconfig gcc
um i386_defconfig gcc
um x86_64_defconfig gcc
x86_64 allyesconfig gcc
x86_64 buildonly-randconfig-r001-20230731 gcc
x86_64 buildonly-randconfig-r001-20230802 clang
x86_64 buildonly-randconfig-r002-20230731 gcc
x86_64 buildonly-randconfig-r002-20230802 clang
x86_64 buildonly-randconfig-r003-20230731 gcc
x86_64 buildonly-randconfig-r003-20230802 clang
x86_64 defconfig gcc
x86_64 kexec gcc
x86_64 randconfig-x001-20230731 clang
x86_64 randconfig-x002-20230731 clang
x86_64 randconfig-x003-20230731 clang
x86_64 randconfig-x004-20230731 clang
x86_64 randconfig-x005-20230731 clang
x86_64 randconfig-x006-20230731 clang
x86_64 randconfig-x011-20230731 gcc
x86_64 randconfig-x012-20230731 gcc
x86_64 randconfig-x013-20230731 gcc
x86_64 randconfig-x014-20230731 gcc
x86_64 randconfig-x015-20230731 gcc
x86_64 randconfig-x016-20230731 gcc
x86_64 rhel-8.3-rust clang
x86_64 rhel-8.3 gcc
xtensa randconfig-r022-20230731 gcc
xtensa randconfig-r034-20230731 gcc
xtensa randconfig-r035-20230731 gcc
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ 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