* [PATCH] HID: logitech-hidpp: HID: make const array consumer_rdesc_start static
From: Colin King @ 2019-05-10 13:17 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, linux-input
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
Don't populate the array consumer_rdesc_start on the stack but instead
make it static. Makes the object code smaller by 88 bytes.
Before:
text data bss dec hex filename
59155 9840 448 69443 10f43 drivers/hid/hid-logitech-hidpp.o
After:
text data bss dec hex filename
59003 9904 448 69355 10eeb drivers/hid/hid-logitech-hidpp.o
(gcc version 8.3.0, amd64)
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/hid/hid-logitech-hidpp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 72fc9c0566db..df960491e473 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2862,7 +2862,7 @@ static u8 *hidpp10_consumer_keys_report_fixup(struct hidpp_device *hidpp,
u8 *_rdesc, unsigned int *rsize)
{
/* Note 0 terminated so we can use strnstr to search for this. */
- const char consumer_rdesc_start[] = {
+ static const char consumer_rdesc_start[] = {
0x05, 0x0C, /* USAGE_PAGE (Consumer Devices) */
0x09, 0x01, /* USAGE (Consumer Control) */
0xA1, 0x01, /* COLLECTION (Application) */
--
2.20.1
^ permalink raw reply related
* [PATCH] HID: logitech-dj: make const array template static
From: Colin King @ 2019-05-10 13:10 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, linux-input
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
Don't populate the array template on the stack but instead make it
static. Makes the object code smaller by 10 bytes. Also reformat
the declaration.
Before:
text data bss dec hex filename
29376 9360 128 38864 97d0 drivers/hid/hid-logitech-dj.o
After:
text data bss dec hex filename
29270 9456 128 38854 97c6 drivers/hid/hid-logitech-dj.o
(gcc version 8.3.0, amd64)
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/hid/hid-logitech-dj.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index b1e894618eed..72d0ab05401f 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -1111,12 +1111,14 @@ static int logi_dj_recv_send_report(struct dj_receiver_dev *djrcv_dev,
static int logi_dj_recv_query_hidpp_devices(struct dj_receiver_dev *djrcv_dev)
{
- const u8 template[] = {REPORT_ID_HIDPP_SHORT,
- HIDPP_RECEIVER_INDEX,
- HIDPP_SET_REGISTER,
- HIDPP_REG_CONNECTION_STATE,
- HIDPP_FAKE_DEVICE_ARRIVAL,
- 0x00, 0x00};
+ static const u8 template[] = {
+ REPORT_ID_HIDPP_SHORT,
+ HIDPP_RECEIVER_INDEX,
+ HIDPP_SET_REGISTER,
+ HIDPP_REG_CONNECTION_STATE,
+ HIDPP_FAKE_DEVICE_ARRIVAL,
+ 0x00, 0x00
+ };
u8 *hidpp_report;
int retval;
--
2.20.1
^ permalink raw reply related
* Re: [RFC v2] iio: input-bridge: optionally bridge iio acceleometers to create a /dev/input interface
From: H. Nikolaus Schaller @ 2019-05-10 10:06 UTC (permalink / raw)
To: Bastien Nocera
Cc: Jonathan Cameron, Dmitry Torokhov, Eric Piel, linux-input,
letux-kernel, kernel, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-kernel, linux-iio
In-Reply-To: <4a77d53966d117bc5c7ff2836634d8591633f4f5.camel@hadess.net>
> Am 10.05.2019 um 11:35 schrieb Bastien Nocera <hadess@hadess.net>:
>
> On Fri, 2019-05-10 at 11:33 +0200, H. Nikolaus Schaller wrote:
>>>
> <snip>
>> It does through "Input device name:" starting with "iio-bridge:" as
>> you can see in the commit message of [RFC v3]:
>
> This makes it ABI, right?
The "Input device name:" is already ABI of the input system (although I guess the string is
built into the evtest tool). I think there are also /sys nodes which carry the same information.
But yes, if someone changes the "iio-bridge:" prefix in kernel code it breaks a user space lib
making use of it:
+ poll_dev->input->name = kasprintf(GFP_KERNEL, "iio-bridge: %s",
+ indio_dev->name);
+ poll_dev->input->phys = kasprintf(GFP_KERNEL, "iio:device%d",
+ indio_dev->id);
This type of exporting names seems to be quite common. E.g. "mmcblk0p1" which may end up
in some /etc/fstab.
>
> Big fat warnings around the code that declares it would be appreciated.
Ok.
^ permalink raw reply
* Re: [RFC v2] iio: input-bridge: optionally bridge iio acceleometers to create a /dev/input interface
From: Bastien Nocera @ 2019-05-10 9:35 UTC (permalink / raw)
To: H. Nikolaus Schaller
Cc: Jonathan Cameron, Dmitry Torokhov, Eric Piel, linux-input,
letux-kernel, kernel, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-kernel, linux-iio
In-Reply-To: <7440F555-5A92-442C-B217-BE17EEF9EF68@goldelico.com>
On Fri, 2019-05-10 at 11:33 +0200, H. Nikolaus Schaller wrote:
> >
<snip>
> It does through "Input device name:" starting with "iio-bridge:" as
> you can see in the commit message of [RFC v3]:
This makes it ABI, right?
Big fat warnings around the code that declares it would be appreciated.
^ permalink raw reply
* Re: [RFC v2] iio: input-bridge: optionally bridge iio acceleometers to create a /dev/input interface
From: H. Nikolaus Schaller @ 2019-05-10 9:33 UTC (permalink / raw)
To: Bastien Nocera
Cc: Jonathan Cameron, Dmitry Torokhov, Eric Piel, linux-input,
letux-kernel, kernel, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-kernel, linux-iio
In-Reply-To: <b5e40ab0d5aad247b7cb9539c198e04096c516c1.camel@hadess.net>
> Am 10.05.2019 um 10:57 schrieb Bastien Nocera <hadess@hadess.net>:
>
> On Mon, 2019-04-22 at 15:20 +0100, Jonathan Cameron wrote:
>>> Different goals usually lead to different solution architectures.
>>
>> Indeed, but in this case we have your proposal which is a subset of
>> what
>> I am suggesting. One architecture can fulfil both requirements.
>>
>> I'll leave it for the other thread, but Bastien has raised the case
>> (that I'd forgotten) that there already userspace stacks that are
>> capable of happily taking in both IIO and Input devices. The
>> confusion
>> here is they will now discover 'both' without the existing userspace
>> knowing that they are the same device. We need to be very careful
>> not
>> to break those userspace programs.
>>
>> So this is an illustration of why the simplistic case doesn't work
>> 'now'.
>
> I don't know what state the whole patch is, but, at the very least, I'd
> expect that patch to export the fact that it's exporting synthesised
> data from another driver, so that it can be easily ignored in user-
> space that already supports IIO devices.
>
It does through "Input device name:" starting with "iio-bridge:" as you can see in the commit message of [RFC v3]:
root@letux:~# evtest /dev/input/event5 | head -19
Input driver version is 1.0.1
Input device ID: bus 0x0 vendor 0x0 product 0x0 version 0x0
Input device name: "iio-bridge: bmc150_accel"
Supported events:
Event type 0 (EV_SYN)
Event type 3 (EV_ABS)
Event code 0 (ABS_X)
Value 8
Min -511
Max 511
Event code 1 (ABS_Y)
Value -44
Min -511
Max 511
Event code 2 (ABS_Z)
Value -265
Min -511
Max 511
^ permalink raw reply
* Re: [RFC v2] iio: input-bridge: optionally bridge iio acceleometers to create a /dev/input interface
From: Bastien Nocera @ 2019-05-10 8:57 UTC (permalink / raw)
To: Jonathan Cameron, H. Nikolaus Schaller
Cc: Dmitry Torokhov, Eric Piel, linux-input, letux-kernel, kernel,
Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
linux-kernel, linux-iio
In-Reply-To: <20190422152014.7c6637ab@archlinux>
On Mon, 2019-04-22 at 15:20 +0100, Jonathan Cameron wrote:
> > Different goals usually lead to different solution architectures.
>
> Indeed, but in this case we have your proposal which is a subset of
> what
> I am suggesting. One architecture can fulfil both requirements.
>
> I'll leave it for the other thread, but Bastien has raised the case
> (that I'd forgotten) that there already userspace stacks that are
> capable of happily taking in both IIO and Input devices. The
> confusion
> here is they will now discover 'both' without the existing userspace
> knowing that they are the same device. We need to be very careful
> not
> to break those userspace programs.
>
> So this is an illustration of why the simplistic case doesn't work
> 'now'.
I don't know what state the whole patch is, but, at the very least, I'd
expect that patch to export the fact that it's exporting synthesised
data from another driver, so that it can be easily ignored in user-
space that already supports IIO devices.
^ permalink raw reply
* Re: [RFC v2] iio: input-bridge: optionally bridge iio acceleometers to create a /dev/input interface
From: Bastien Nocera @ 2019-05-10 8:57 UTC (permalink / raw)
To: Roderick Colenbrander, Jonathan Cameron
Cc: H. Nikolaus Schaller, Dmitry Torokhov, Eric Piel, linux-input,
letux-kernel, kernel, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, lkml, linux-iio
In-Reply-To: <CAEc3jaCnHLA9PV5gLiBvHT5y26mwMGSUxa3YCO=c+FYmcszePQ@mail.gmail.com>
On Sun, 2019-04-14 at 09:26 -0700, Roderick Colenbrander wrote:
>
<snip>
> We at the time were one of the first to expose acceleration and gyro
> data through /dev/input for DualShock 4 as supported by hid-sony. We
> report acceleration in 'g' and angular velocity in 'degree / s'. We
> set the resolution to respectively '1 g' and '1 degree / s'. The range
> we set to the range of the device e.g. for DS4 -4g to +4g for
> acceleration. I need to check though what coordinate system we use,
> but I think it is right handed (gyro counter clockwise relative to
> acceleration axes).
How do you export the gyro information through the input device?
FWIW, we needed to do extra work in iio-sensor-proxy so that the
accelerometer in the Sixaxis/DS4 joypads (and uDraw tablet) didn't
appear as though they were accelerometer for the system:
https://github.com/hadess/iio-sensor-proxy/commit/401d59e54b3123860180d4401e09df8a1e1bc6c3
> The two other drivers using INPUT_PROC_ACCELEROMETER are hid-wacom and
> hid-udraw-ps3 Wacom. Both seem to report resolution in 'g' as well.
I wrote hid-udraw-ps3, and it's reporting accelerometer data through
input because the rest of the driver is input, and it didn't make much
sense to use another subsystem for just that small portion of the
events the device sends out.
^ permalink raw reply
* Re: [RFC v3] iio: input-bridge: optionally bridge iio acceleometers to create a /dev/input interface
From: Bastien Nocera @ 2019-05-10 8:56 UTC (permalink / raw)
To: H. Nikolaus Schaller
Cc: Jonathan Cameron, Dmitry Torokhov, Eric Piel, linux-input,
letux-kernel, kernel, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-kernel, linux-iio
In-Reply-To: <AA2B43FE-E196-4FEC-B4C5-897D0F44A76F@goldelico.com>
On Tue, 2019-04-16 at 21:33 +0200, H. Nikolaus Schaller wrote:
> Hi Bastien,
>
> > Am 16.04.2019 um 18:04 schrieb Bastien Nocera <hadess@hadess.net>:
> > This can be done in user-space, reading the data from the IIO driver,
> > and using uinput to feed it back. Why is doing this at the kernel level
> > better?
>
> Well, I'd estimate that >80% of the current kernel could be done in user-space
> (but not at the same speed/quality).
>
> E.g. TCP could most likely be done by directly accessing the Ethernet layer and
> providing other processes access through named pipes instead of sockets.
>
> But usually a user-space daemon feeding things back into the kernel is slower
> (because it is scheduled differently) and needs more resources for running the
> process and IPC and is less protected against hickups and deadlocks.
This is mostly irrelevant for the amount of data we're treating, but it
doesn't matter too much.
> Two more aspects come to my mind from reading your project page:
>
> a) "It requires libgudev and systemd"
> b) "Note that a number of kernel bugs will prevent it from working correctly"
>
> a) this makes quite significant assumptions about the user-space while a kernel
> driver can be kept independent of this
It's made for modern desktop OSes/"traditional" Linux. I don't think
that those 2 libraries are problematic dependencies unless you're on
Android, where a replacement could be implemented or iio-sensor-proxy
modified for that use case.
> b) if it is in-kernel it will be kept in sync with kernel changes and such bugs
> are less likely
No they're not. This warning was because 1) drivers sometimes have bugs
2) user-space sometimes has bugs 3) user-space sometimes causes the
kernel to have bugs.
The 2 significant breakages for iio-sensor-proxy were caused by runtime
PM bugs in the hid-sensor-hub driver, and in the USB core. I doubt a
kernel-space implementation would have been able to magically fix those
bugs unfortunately.
^ permalink raw reply
* Re: [PATCH v2 2/4] mfd: ioc3: Add driver for SGI IOC3 chip
From: Lee Jones @ 2019-05-10 7:14 UTC (permalink / raw)
To: Thomas Bogendoerfer
Cc: Ralf Baechle, Paul Burton, James Hogan, Dmitry Torokhov,
David S. Miller, Alessandro Zummo, Alexandre Belloni,
Greg Kroah-Hartman, Jiri Slaby, linux-mips, linux-kernel,
linux-input, netdev, linux-rtc, linux-serial
In-Reply-To: <20190509160220.bb5382df931e5bd0972276df@suse.de>
On Thu, 09 May 2019, Thomas Bogendoerfer wrote:
> On Wed, 8 May 2019 11:23:13 +0100
> Lee Jones <lee.jones@linaro.org> wrote:
>
> > On Tue, 09 Apr 2019, Thomas Bogendoerfer wrote:
> >
> > > +static u32 crc8_addr(u64 addr)
> > > +{
> > > + u32 crc = 0;
> > > + int i;
> > > +
> > > + for (i = 0; i < 64; i += 8)
> > > + crc8_byte(&crc, addr >> i);
> > > + return crc;
> > > +}
> >
> > Not looked into these in any detail, but are you not able to use the
> > CRC functions already provided by the kernel?
>
> they are using a different polynomial, so I can't use it.
Would it be worth moving support out to somewhere more central so
others can use this "polynomial"?
> > > + }
> > > + pr_err("ioc3: CRC error in NIC address\n");
> > > +}
> >
> > This all looks like networking code. If this is the case, it should
> > be moved to drivers/networking or similar.
>
> no it's not. nic stands for number in a can produced by Dallas Semi also
> known under the name 1-Wire (https://en.wikipedia.org/wiki/1-Wire).
> SGI used them to provide partnumber, serialnumber and mac addresses.
> By placing the code to read the NiCs inside ioc3 driver there is no need
> for locking and adding library code for accessing these informations.
Great. So it looks like you should be using this, no?
drivers/base/regmap/regmap-w1.c
> > > +static struct resource ioc3_uarta_resources[] = {
> > > + DEFINE_RES_MEM(offsetof(struct ioc3, sregs.uarta),
> >
> > You are the first user of offsetof() in MFD. Could you tell me why
> > it's required please?
>
> to get the offsets of different chip functions out of a struct.
I can see what it does on a coding level.
What are you using it for in practical/real terms?
Why wouldn't any other MFD driver require this, but you do?
> > Please drop all of these and statically create the MFD cells like
> > almost all other MFD drivers do.
>
> I started that way and it blew up the driver and create a bigger mess
> than I wanted to have. What's your concern with my approach ?
>
> I could use static mfd_cell arrays, if there would be a init/startup
> method per cell, which is called before setting up the platform device.
> That way I could do the dynamic setup for ethernet and serial devices.
You can set platform data later. There are plenty of examples of
this in the MFD subsystem. Statically define what you can, and add
the dynamic stuff later.
> > > +static void ioc3_create_devices(struct ioc3_priv_data *ipd)
> > > +{
> > > + struct mfd_cell *cell;
> > > +
> > > + memset(ioc3_mfd_cells, 0, sizeof(ioc3_mfd_cells));
> > > + cell = ioc3_mfd_cells;
> > > +
> > > + if (ipd->info->funcs & IOC3_ETH) {
> > > + memcpy(ioc3_eth_platform_data.mac_addr, ipd->nic_mac,
> > > + sizeof(ioc3_eth_platform_data.mac_addr));
> >
> > Better to pull the MAC address from within the Ethernet driver.
>
> the NiC where the MAC address is provided is connected to the ioc3
> chip outside of the ethernet register set. And there is another
> NiC connected to the same 1-W bus. So moving reading of the MAC
> address to the ethernet driver duplicates code and adds complexity
> (locking). Again what's your concern here ?
Does this go away if you use the already provided 1-wire API?
> > > + if (ipd->info->funcs & IOC3_SER) {
> > > + writel(GPCR_UARTA_MODESEL | GPCR_UARTB_MODESEL,
> > > + &ipd->regs->gpcr_s);
> > > + writel(0, &ipd->regs->gppr[6]);
> > > + writel(0, &ipd->regs->gppr[7]);
> > > + udelay(100);
> > > + writel(readl(&ipd->regs->port_a.sscr) & ~SSCR_DMA_EN,
> > > + &ipd->regs->port_a.sscr);
> > > + writel(readl(&ipd->regs->port_b.sscr) & ~SSCR_DMA_EN,
> > > + &ipd->regs->port_b.sscr);
> > > + udelay(1000);
> >
> > No idea what any of this does.
> >
> > It looks like it belongs in the serial driver (and needs comments).
>
> it configures the IOC3 chip for serial usage. This is not part of
> the serial register set, so it IMHO belongs in the MFD driver.
So it does serial things, but doesn't belong in the serial driver?
Could you please go into a bit more detail as to why you think that?
Why is it better here?
It's also totally unreadable by the way!
> > > + }
> > > +#if defined(CONFIG_SGI_IP27)
> >
> > What is this? Can't you obtain this dynamically by probing the H/W?
>
> that's the machine type and the #ifdef CONFIG_xxx are just for saving space,
> when compiled for other machines and it's easy to remove.
Please find other ways to save the space. #ifery can get very messy,
very quickly and is almost always avoidable.
> > > + if (ipd->info->irq_offset) {
> >
> > What does this really signify?
>
> IOC3 ASICs are most of the time connected to a SGI bridge chip. IOC3 can
> provide two interrupt lines, which are wired to the bridge chip. The first
> interrupt is assigned via the PCI core, but since IOC3 is not a PCI multi
> function device the second interrupt must be treated here. And the used
> interrupt line on the bridge chip differs between boards.
Please provide a MACRO, function or something else which results in
more readable code. Whatever you choose to use, please add this text
above, it will be helpful for future readers.
> Thank you for your review. I'll address all other comments not cited in
> my mail.
NP
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* [PATCH 2/3] Input: wistron_btns: avoid panic if ioreamp fails
From: Kefeng Wang @ 2019-05-10 3:03 UTC (permalink / raw)
To: linux-kernel
Cc: Kefeng Wang, Dmitry Torokhov, Miloslav Trmac, Wolfram Sang,
linux-input, Hulk Robot
In-Reply-To: <20190510030320.109154-1-wangkefeng.wang@huawei.com>
If ioremap fails, NULL pointer dereference will happen and
leading to a kernel panic when access the virtual address
in check_signature().
Fix it by check the return value of ioremap.
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Miloslav Trmac <mitr@volny.cz>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: linux-input@vger.kernel.org
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
drivers/input/misc/wistron_btns.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/input/misc/wistron_btns.c b/drivers/input/misc/wistron_btns.c
index 43e67f546366..a82ec3d102b4 100644
--- a/drivers/input/misc/wistron_btns.c
+++ b/drivers/input/misc/wistron_btns.c
@@ -107,7 +107,10 @@ static int __init map_bios(void)
ssize_t offset;
u32 entry_point;
- base = ioremap(0xF0000, 0x10000); /* Can't fail */
+ base = ioremap(0xF0000, 0x10000);
+ if (!base)
+ return -ENOMEM;
+
offset = locate_wistron_bios(base);
if (offset < 0) {
printk(KERN_ERR "wistron_btns: BIOS entry point not found\n");
--
2.20.1
^ permalink raw reply related
* [PATCH 1/3] Input: apanel: avoid panic if ioreamp fails
From: Kefeng Wang @ 2019-05-10 3:03 UTC (permalink / raw)
To: linux-kernel; +Cc: Kefeng Wang, Dmitry Torokhov, linux-input, Hulk Robot
If ioremap fails, NULL pointer dereference will happen and
leading to a kernel panic when access the virtual address
in check_signature().
Fix it by check the return value of ioremap.
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: linux-input@vger.kernel.org
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
drivers/input/misc/apanel.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/input/misc/apanel.c b/drivers/input/misc/apanel.c
index c1e66f45d552..1c7262ad4b5b 100644
--- a/drivers/input/misc/apanel.c
+++ b/drivers/input/misc/apanel.c
@@ -259,7 +259,9 @@ static int __init apanel_init(void)
unsigned char i2c_addr;
int found = 0;
- bios = ioremap(0xF0000, 0x10000); /* Can't fail */
+ bios = ioremap(0xF0000, 0x10000);
+ if (!bios)
+ return -ENOMEM;
p = bios_signature(bios);
if (!p) {
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] HID: Increase maximum report size allowed by hid_field_extract()
From: Jiri Kosina @ 2019-05-09 19:30 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: Benjamin Tissoires, open list:HID CORE LAYER, lkml,
Ronald Tschalär
In-Reply-To: <08CA35F5-1ADC-4C55-ACF5-04B19CC77A25@canonical.com>
On Fri, 26 Apr 2019, Kai-Heng Feng wrote:
> >Ronald (Cc-ed) raised quite a good point:
> >what's the benefit of removing the error message if this function (and
> >__extract) can only report an unsigned 32 bits value?
>
> I didn’t spot this, sorry.
>
> >
> >My take is we should revert 94a9992f7dbdfb28976b upstream and think at
> >a better solution.
>
> I think using a new fix to replace it will be a better approach, as it at
> least partially solves the issue.
Guys, did this fall in between cracks? Is anyone planning to send a fixup?
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] HID: i2c-hid: add iBall Aer3 to descriptor override
From: Jiri Kosina @ 2019-05-09 19:29 UTC (permalink / raw)
To: Kai-Heng Feng; +Cc: benjamin.tissoires, linux-input, linux-kernel
In-Reply-To: <20190422065704.27234-1-kai.heng.feng@canonical.com>
On Mon, 22 Apr 2019, Kai-Heng Feng wrote:
> This device uses the SIPODEV SP1064 touchpad, which does not
> supply descriptors, so it has to be added to the override
> list.
>
> BugLink: https://bugs.launchpad.net/bugs/1825718
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
> drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
Applied to for-5.2/fixes. Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [Letux-kernel] [RFC v2] iio: input-bridge: optionally bridge iio acceleometers to create a /dev/input interface
From: H. Nikolaus Schaller @ 2019-05-09 17:02 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-input, Dmitry Torokhov, Eric Piel, linux-iio, kernel, lkml,
Lars-Peter Clausen, Peter Meerwald-Stadler, Hartmut Knaack,
Discussions about the Letux Kernel
In-Reply-To: <CA9A9410-C393-49B9-81FA-D9BC55F04468@goldelico.com>
> Am 09.05.2019 um 11:09 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
>
> Hi Jonathan,
>>
>>
>> And how does that work on the common case of a sensor in the lid of a laptop?
>> how do you know what angle the screen is at?
>
> Well, I am not aware of laptops where the sensor is in the lid because I am in the handhelds
> business, but let's assume it is common.
>
> I realized that if the sensor orientation is related to the lid position, while the reference
> frame reported to user space is to be referenced to the lap or keyboard of the laptop, there does
> not exist a static mount-matrix to describe it properly. So no driver can report that correctly.
>
> Therefore, such a device needs a dynamic mount matrix, i.e. there should be a kernel driver that
> reads out the lid angle sensor and modifies the mount-matrix of the accelerometer by some sin()/cos()
> table.
One more thought on this topic.
My answer to the question "how do you know what angle the screen is at?" by requiring an ADC to
measure some potentiometer in the hinge to make the mount matrix dynamic is probably completely
wrong...
If we take the definition for the mount matrix, it defines a specific g-vector pointing to
center of earth if the user is holding the device in a specific position and looking on the display
or the keyboard.
So far the description assumes that there is a single accelerometer and display and keys of a phone
are in a single plane, i.e. there is no angle and everything is fine.
Now if we simply take the two accelerometers separately, one z-axis is going through the keyboard
and the other through the display. Which means if the mount matrices are well defined, the accelerometers
should report almost the same values if the display is fully opened by 180 degrees, i.e. the display
is sitting flat on the table. This is what my RFC does by autoscaling. The values differ only
by noise.
Now what about measuring the lid angle? Well, it is already measured by both accelerometers! If they
do not agree, the angle can be calculated by some arctan() based on y and z axis reports...
If you close the lid, the display is turned upside down and y and z axes reverse sign.
So there remains only the issue that user-space must know which sensor device file is which sensor
and can do the calculation of the lid angle. This is possible because the iio accelerometer name
is available through the input event ioctls.
In summary this case also does not need policy or configuration. Just user space using the information
that is already presented.
BR,
Nikolaus
^ permalink raw reply
* Re: [PATCH v2 2/4] mfd: ioc3: Add driver for SGI IOC3 chip
From: Thomas Bogendoerfer @ 2019-05-09 14:02 UTC (permalink / raw)
To: Lee Jones
Cc: Ralf Baechle, Paul Burton, James Hogan, Dmitry Torokhov,
David S. Miller, Alessandro Zummo, Alexandre Belloni,
Greg Kroah-Hartman, Jiri Slaby, linux-mips, linux-kernel,
linux-input, netdev, linux-rtc, linux-serial
In-Reply-To: <20190508102313.GG3995@dell>
On Wed, 8 May 2019 11:23:13 +0100
Lee Jones <lee.jones@linaro.org> wrote:
> On Tue, 09 Apr 2019, Thomas Bogendoerfer wrote:
>
> > +static u32 crc8_addr(u64 addr)
> > +{
> > + u32 crc = 0;
> > + int i;
> > +
> > + for (i = 0; i < 64; i += 8)
> > + crc8_byte(&crc, addr >> i);
> > + return crc;
> > +}
>
> Not looked into these in any detail, but are you not able to use the
> CRC functions already provided by the kernel?
they are using a different polynomial, so I can't use it.
> > + }
> > + pr_err("ioc3: CRC error in NIC address\n");
> > +}
>
> This all looks like networking code. If this is the case, it should
> be moved to drivers/networking or similar.
no it's not. nic stands for number in a can produced by Dallas Semi also
known under the name 1-Wire (https://en.wikipedia.org/wiki/1-Wire).
SGI used them to provide partnumber, serialnumber and mac addresses.
By placing the code to read the NiCs inside ioc3 driver there is no need
for locking and adding library code for accessing these informations.
> > +static struct resource ioc3_uarta_resources[] = {
> > + DEFINE_RES_MEM(offsetof(struct ioc3, sregs.uarta),
>
> You are the first user of offsetof() in MFD. Could you tell me why
> it's required please?
to get the offsets of different chip functions out of a struct.
> Please drop all of these and statically create the MFD cells like
> almost all other MFD drivers do.
I started that way and it blew up the driver and create a bigger mess
than I wanted to have. What's your concern with my approach ?
I could use static mfd_cell arrays, if there would be a init/startup
method per cell, which is called before setting up the platform device.
That way I could do the dynamic setup for ethernet and serial devices.
> > +static void ioc3_create_devices(struct ioc3_priv_data *ipd)
> > +{
> > + struct mfd_cell *cell;
> > +
> > + memset(ioc3_mfd_cells, 0, sizeof(ioc3_mfd_cells));
> > + cell = ioc3_mfd_cells;
> > +
> > + if (ipd->info->funcs & IOC3_ETH) {
> > + memcpy(ioc3_eth_platform_data.mac_addr, ipd->nic_mac,
> > + sizeof(ioc3_eth_platform_data.mac_addr));
>
> Better to pull the MAC address from within the Ethernet driver.
the NiC where the MAC address is provided is connected to the ioc3
chip outside of the ethernet register set. And there is another
NiC connected to the same 1-W bus. So moving reading of the MAC
address to the ethernet driver duplicates code and adds complexity
(locking). Again what's your concern here ?
> > + if (ipd->info->funcs & IOC3_SER) {
> > + writel(GPCR_UARTA_MODESEL | GPCR_UARTB_MODESEL,
> > + &ipd->regs->gpcr_s);
> > + writel(0, &ipd->regs->gppr[6]);
> > + writel(0, &ipd->regs->gppr[7]);
> > + udelay(100);
> > + writel(readl(&ipd->regs->port_a.sscr) & ~SSCR_DMA_EN,
> > + &ipd->regs->port_a.sscr);
> > + writel(readl(&ipd->regs->port_b.sscr) & ~SSCR_DMA_EN,
> > + &ipd->regs->port_b.sscr);
> > + udelay(1000);
>
> No idea what any of this does.
>
> It looks like it belongs in the serial driver (and needs comments).
it configures the IOC3 chip for serial usage. This is not part of
the serial register set, so it IMHO belongs in the MFD driver.
> > + }
> > +#if defined(CONFIG_SGI_IP27)
>
> What is this? Can't you obtain this dynamically by probing the H/W?
that's the machine type and the #ifdef CONFIG_xxx are just for saving space,
when compiled for other machines and it's easy to remove.
> > + if (ipd->info->irq_offset) {
>
> What does this really signify?
IOC3 ASICs are most of the time connected to a SGI bridge chip. IOC3 can
provide two interrupt lines, which are wired to the bridge chip. The first
interrupt is assigned via the PCI core, but since IOC3 is not a PCI multi
function device the second interrupt must be treated here. And the used
interrupt line on the bridge chip differs between boards.
Thank you for your review. I'll address all other comments not cited in
my mail.
Thomas.
--
SUSE Linux GmbH
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
^ permalink raw reply
* Re: [RFC v2] iio: input-bridge: optionally bridge iio acceleometers to create a /dev/input interface
From: H. Nikolaus Schaller @ 2019-05-09 9:09 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Dmitry Torokhov, Eric Piel, linux-input, letux-kernel, kernel,
Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
linux-kernel, linux-iio
In-Reply-To: <20190422152014.7c6637ab@archlinux>
Hi Jonathan,
sorry for the delayed response, but I have to focus on other topics now.
Just some small comments came to my mind to finish the discussion, mainly on the accelerometer in the lid case (see inline below).
BR and thanks,
Nikolaus
> Am 22.04.2019 um 16:20 schrieb Jonathan Cameron <jic23@kernel.org>:
>
> On Mon, 15 Apr 2019 23:04:15 +0200
> H. Nikolaus Schaller <hns@goldelico.com> wrote:
>
>> Hi Jonathan,
>> we seem to have opposite goals and therefore quite different ideas what the right
>> solution is.
>
> Yup :) Yikes this email has gotten complex. Hopefully I haven't
> lost track of the various points below.
>
>>
>> I come from user-space and the input side, find that input can handle abstract "acceleration input",
>> and some older chip drivers even provide that as their only interface. Therefore I want
>> to provide "acceleration input" in these cases where only iio capable drivers exist by
>> using the existing in-kernel-iio infrastructure. Nothing more.
>>
>> You seem to come from the iio architecture and want to extend it to other APIs as
>> general as possible and input is just one of several of them.
>
> Yes, my target is to produce a subsystem that meets many (all would be nice)
> requirements, including yours. Whilst I'm happy to debate this for ever, I'm not
> sure we are making any substantial progress. As you mention below we
> probably need to 'see the code' to drive the discussion forwards.
>
>>
>> Different goals usually lead to different solution architectures.
>
> Indeed, but in this case we have your proposal which is a subset of what
> I am suggesting. One architecture can fulfil both requirements.
Not exactly. Yours always needs configuration in every case. My RFC works without
user-space config support for the most common cases. This user-space config must
be maintained and spread over all distributions. So we can never be sure that
if a user changes the distro that it still works.
>
> I'll leave it for the other thread, but Bastien has raised the case
> (that I'd forgotten) that there already userspace stacks that are
> capable of happily taking in both IIO and Input devices. The confusion
> here is they will now discover 'both' without the existing userspace
> knowing that they are the same device. We need to be very careful not
> to break those userspace programs.
>
> So this is an illustration of why the simplistic case doesn't work
> 'now'.
>
>>
>>> Am 14.04.2019 um 13:40 schrieb Jonathan Cameron <jic23@kernel.org>:
>>>
>>> On Mon, 8 Apr 2019 15:15:56 +0200
>>> H. Nikolaus Schaller <hns@goldelico.com> wrote:
>>>
>>>> Hi Jonathan,
>>>>
>>>> I still do not fully understand what is worrying you here.
>>>
>>>>
>>>> Do you worry about functionality, flexibility or resources or something else?
>>>
>>> Two main things:
>>> 1) Lack of generality of the approach.
>>> This is a single use trick for input devices. Why does it make sense for
>>> input devices?
>>
>> No, it is not a trick...
> Bad choice of words. Sorry about that.
>
> Any time we register a device as two types of device it is less than ideal.
>
> If we had the true separation of IIO front end and back end then it
> would be perfectly acceptable to 'only' have an input front end for
> a given device. That choice would still, in this sort of usecase,
> have to be made from userspace. It's policy, not design. If there are reasons
> a particular device 'is input' then that mapping should be in DT or similar.
> It's no different from knowing an ADC channel is wired to an analog
> temperature sensor. For example, you could build a joystick with
> an accelerometer in the stick - then the usecase would be obvious!
> Hence I would also argue that any dynamic interface should also support
> a static path (DT or equivalent) for the cases where is really a
> physical characteristic of the system! Perhaps the Sony parts
> fall into this category as well.
>
> For a bit of historical background, there was a concerted effort
> to produced a userspace stack for IIO for android.
> https://01.org/android-iio-sensors-hal
>
> Unfortunately I think it died as a result of other moves in Intel on
> one of their periodic shifts in direction.
>
>>
>> Why does it make sense for input devices?
>
> These are not just 'input' devices. They are accelerometers. One usecase
> is to use them for input. The exact same physical device is used for games
> input uses and counting steps for example (actually a lot wider cases than
> that, but this one is common in android devices).
>
> Keep that in mind at all times. There are lots of usecases.
> So we need a solution that does not result in problems for those
> usecases. We are not writing a subsytem targetting android use of
> accelerometers. We are writing a subsystem addressing as many usecases
> as we can of those devices.
>
> Note that the original reason for IIO was to generalize a whole set of
> proposed individual subsystems targeting particular sensor types. So
> that is our focus - solutions that work for everyone. This isn't
> totally dissimilar from those discussions - at the time I wanted
> to do a small focused ALS subsystem and got a resounding "no" from
> Linus. Generality matters a lot for the long term.
>
>>
>> a) because there is no alternative for accelerometer input devices and there are some
>> (older) accelerometer chips which only present themselves as input devices as soon
>> as the driver is compiled and the chip is found.
>
> Actually that is not accurate. The vast majority of those older devices
> that have had any attempt at mainlining are in IIO. AFAIK no accelerometer
> driver has been merged to mainline input for many years. This is because,
> amongst others, Dmitry has been of the view they didn't belong there for
> a very long time.
>
>>
>> b) because input events define ACCEL input devices. But no Temperature or Voltage
>> input or generic ADC input.So there is no generalization of input devices beyond
>> keyboard, mouse, joystick, accelerometer and some others.
>
> That's not totally inaccurate, but the distinction in the other sensor cases
> is that there is a clear 'additional' element that we can map in devicetree
> which relates the IIO sensor channel to the input device.
> Doesn't matter for the point of view of this discussion though.
>
>>
>>> There are lots of other in kernel users and potential
>>> ones in the future.
>>
>> This bridge addition does not exclude any (additional) in-kernel use.
>
> No but it creates several problems:
>
> 1. Two ways to do the same thing.
> 2. Two sets of code to maintain.
> 3. Confusion over what is the best way of doing it.
> 4. The known issues with multiple consumers (note my solution has
> that problem as well!)
>
> My job here is to maintain the code, which is why I push back on something
> that makes that job harder.
>
> When the next usecases comes along and someone says they want to map
> all ADC channels to hwmon channels because that is the subystem that
> they expect to measure voltages in, then I don't have a good argument
> to stop them doing the same thing you have.
>
> As a side note we have in the past had input drivers for gyroscopes and
> magnetometers. Why are accelerometers special?
>
> I really don't see why we should treat accelerometers differently
Not special. I just did not want to add them yet in the RFC phase.
The principle is the same.
>
> As this discussion runs on, I am increasingly convinced that there *must*
> be a userspace policy control over whether an input device is
> instantiated for a given accelerometer. Once that is the
> case then I cannot see a reason to treat it any differently from
> other channel types.
Well, I believe that we should avoid any user-space policy control that
can be avoided. At least those cases that can live without policy
control should not need to get one because there are other cases that
need one.
This is something we can't solve by discussion, of course.
The decision is that you are maintainer and I am just proposing an RFC.
So you have two votes and a veto right...
But maybe this can be fixed by proper defaults? I don't know.
>
>>
>>> The ability to register additional IIO consumers like
>>> this is useful, lets make it useful to everyone.
>>
>> But not everyone and every iio device can and should be mappable to the "input" abstraction.
>> This does not make any sense to me.
>
> Absolutely. It should not be. I clearly didn't explain this well.
>
> It should be mapped to a consumer.
>
> One type of consumer is iio-input, another is iio-hmwon etc.
>
>>
>> For example, does it make sense to map a temperature sensor to accelerometer input? Or an
>> accelerometer to hwmon? This seems to be possible of your generalization unless I am missing something here.
>> If it is, it ignores that iio sensors are already grouped by what physical property they do measure.
>
> If people want to map crazy channels to crazy sensor outputs, why stop them?
> (at this level of the interface).
>
> This is a policy question for a userspace script. Particular consumer drivers
> could of course perform sanity checking and refuse to do anything if they
> cannot sensibly use the channels.
>
> Yes, the interface has this flexibility. Which is a good thing! Take the example
> of the gyroscope as input I used above. If we want to add that support
> in future to your driver (I have no idea if it actually makes sense)
> then we can - without having to change the interface.
>
>>
>>>
>>> 2) To much generality of the specific usecase. I don't want to put an Input
>>> interface on accelerometers where it makes no sense.
>>
>> I think you can just ignore the input interfaces in that case, if it was created.
>
> Bastien raised a case where this isn't true.
>
>>
>>> The rule of it has
>>> 2-3 axis so it must make sense isn't good enough to my mind. How
>>> does userspace know which accelerometer to use (more and more devices have
>>> multiple?)
>>
>> In our approach user-space can make it known by udev rules based on /dev/input/event*
>> (not on iio but the input created for accelerometers). I think I mentioned that. This
>> comes for free for any device registering as input. So it is no additional code.
>
> Sorry, I'm lost. What in there tells you to use 'this' interface rather than one
> of the other N that were registered? I'm not sure what information you
> have available there.
>
>>
>>> You could do something like looking at the location info from
>>> DT / ACPI in your driver and pick the 'best' but that's policy. Should be
>>> in userspace. Sure you can just use the right input driver, but the moment
>>> we do that, we need aware userspace, if that's the case why not make it
>>> aware from the start.
>>>
>>> Believe me I've been round this one a good few times and thought about it
>>> a lot.
>>
>> That is fine and why we should discuss all the different aspects we have collected.
>>
>>> I'll take a lot of convincing that this isn't a problem that
>>> should be pushed into userspace.
>>>
>>>>
>>>> I think having them mapped always does not need much resources (except a handful of bytes
>>>> in memory and some µs during probing) unless the event device is opened and really used.
>>>> Only then it starts e.g. I2C traffic to read the sensors.
>>>
>>> The bytes don't really mater.
>>
>> Ok, good to know.
>>
>>> The userspace ABI additions do.
>>
>> There are only new /dev/input/event devices with well defined ABI. This approach does
>> not invent anything new here, hence there are no ABI additions which we can break.
>
> But it does - we aren't talking general ABI, but ABI on specific
> devices. Sure, Android doesn't care - though you'd be amazed how much
> individual android device developers will because we just added another
> pile of tests to their CI.
>
> An industrial sensor platform absolutely does. They have to validate
> those interfaces. They can't just ignore them because they feel like
> it because who knows if some future user will use them?
>
> For another case, see Bastien's reply to the later thread.
>
> Instantiating interfaces has testing costs, even when the are standard
> interfaces.
>
>>
>>>
>>>>
>>>> So it is just some unused file sitting around in /dev. Or 2 or could be even 100.
>>>> For devices which have no iio accelerometers configured, there will be no /dev/input
>>>> file. So we are discussing the rare case of devices with more than one or two accelerometers.
>>>
>>> Well they aren't exactly rare in IIO using systems ;)
>>
>> This is another thing where our experiences differ. What specific devices are you thinking
>> of? I am focussed on handhelds where the accelerometer (or two) is a way to do GUI input
>> depending on device orientation in space.
>
> Again, you are introducing this interface for everyone. Including lots of
> 'interesting' usecases.
>
> I have worked with sensor platforms with accelerometers of different parts of humans,
> We have people do bridge vibration measurement, flying UAVs and tracking the motion
> of trucks.
>
> Most are not huge numbers of accelerometers per node but don't rule out the
> possibility. It's normally limited by length of cables rather than anything
> else so you used multiple nodes after a while each with their own set of sensors.
>
> There are lots of plaforms out there that use multiple accelerometers in more
> or less the same place to do very high dynamic range measurement (without losing
> precision when things are nearly still).
>
> Anyhow, it's not a particularly important point anyway!
>
>>>
>>>>
>>>> Now, on every system there are many interfaces and files that are not used because it makes
>>>> no sense to look at them. If I check on one of my systems, I find for example a lot of
>>>> /dev/tty and only a very small portion is used and generic distros have no issue with it.
>>>>
>>>> There is even /dev/iio:device0 to /dev/iio:device5 representing the raw iio devices.
>>>> Not all of them are actively used, but they are simply there and can be scanned for.
>>>
>>> Agreed, in the ideal case we wouldn't have had that either, but we are
>>> stuck with it. The long term plan is to allow use of IIO backends without the
>>> front end being there at all. Lots of SoC ADC users would prefer this. We are
>>> stuck with the legacy intertwining fo the front end and back end of IIO so
>>> this isn't as easy to do as I would like.
>>
>> Ah, ok. I think it is a similar discussion of hiding the serdev /dev/tty* if it is
>> used for accessing an embedded GPS or Bluetooth chip, for example.
>>
>> But is this needed? I think it is not a problem if there are multiple consumers for
>> the same iio channel. Some in-kernel, some through /dev/iio:device* and maybe some
>> through /dev/input (which boils down to in-kernel).
>
> There are quite a few complexities around multiple consumers that we really haven't
> solved. Right now the cases that work are very much restricted. I'd love
> to tidy some of these up, but never enough time and all that.
>
> It's not that relevant here, but in short a few of the issues are:
> 1) Interference over control settings. - Two consumers need different filter
> settings/ sampling frequency / range. How do we negotiate the choice and
> communicate it to the other consumers. More complex questions such
> mediating choices of triggers.
> 2) One driver is doing polled reads, the other is doing interrupt driven.
> Most drivers prevent this combination because the polled reads can lead
> to unlimited delays on the interrupt driven path and hence break it.
>
> The main driver for this separation was to present only the 'right' interface
> to reduced people's validation costs etc. People really do want to have the
> option to strip back the userspace inteface. Obviously these are the rare
> people who would disable your config option, but the point of this was
> that we actually would like to make even the IIO interface optional as
> well but have a fair way to go before we can.
>
>>
>>>
>>>>
>>>> So I do not see a resource problem if every accelerometer /dev/iio:device* gets
>>>> some companion /dev/input/event* for being used on demand - but only if this bridge
>>>> is configured at all.
>>>
>>> That argument does not apply. If we add a config option, distros will enable it.
>>> So the vast majority of systems will ship with this turned on. You cannot
>>> use a config variable to control policy and expect it to be change by anyone
>>> but a very very small subset of users. So please drop the 'you can just not
>>> build it argument'.
>>
>> This is not my point here. I mention this under the (now known to be wrong) assumption
>> that resources do care. I just want to state that kernels built for platforms where every
>> byte counts can be stripped down by disabling it. Others where resources are no concern
>> simply can map them all, even if not used.
>
> Agreed. A subset of users will just build without this.
>
>>
>>> Userspace configuration changing is a lot easier if people actually care.
>>> Sure, many distros will ship the same script to everyone.
>>>
>>>>
>>>>> I think we need some deliberate userspace interaction to instantiate
>>>>> one of these rather than 'always doing it'.
>>>>
>>>> My gut feeling is that this additional user-space interaction needs more resources and
>>>> adds a lot of complexity, independently of how it is done.
>>>
>>> Trivial resources and actually fairly trivial complexity. Key thing is
>>> it puts the burden on the users of this functionality to configure what they
>>> want.
>>
>> Hm. No. My proposal does not need configuration which accelerometers should go where.
>
> Agreed. I was talking about my proposal here :)
>
>>
>> I assumethat input accelerometer users do not want to configure anything, like neither
>> a mouse or keyboard is to be configured to be useable (yes there are keymaps but that
>> is impossible to automate).
>
> The difference is a mouse is only really useful as a mouse and most of the time a keyboard
> is a used only as a keyboard. Here that's not true.
>
>>
>> They just want to be able to read device orientation in a device-independent scale.
>> Therefore my approach already takes the mount-matrix into account to hide sensor position
>> differences.
>
> And how does that work on the common case of a sensor in the lid of a laptop?
> how do you know what angle the screen is at?
Well, I am not aware of laptops where the sensor is in the lid because I am in the handhelds
business, but let's assume it is common.
I realized that if the sensor orientation is related to the lid position, while the reference
frame reported to user space is to be referenced to the lap or keyboard of the laptop, there does
not exist a static mount-matrix to describe it properly. So no driver can report that correctly.
Therefore, such a device needs a dynamic mount matrix, i.e. there should be a kernel driver that
reads out the lid angle sensor and modifies the mount-matrix of the accelerometer by some sin()/cos()
table.
Well, you can delegate this to the user-space. But IMHO this is wrong layering. Every layer
put on top of the lower layers should abstract away implementation details so that the highest
layer has the most general interface.
In my view we have here this "protocol" stack (casting into the ISO 7-layer model):
L7: application - user space
L3: input - to get device orientation information
L2: iio - to get raw data and mount-matrix
L1: i2c, spi, usb, hdq, ... - to get bits from/to chips
L0: chips, ...
My RFC mainly mangles the raw data reported from the iio level and the mount matrix into
device orientation information. I.e. it is a proposal for L3 implementation.
So fixing an issue of L2 (dynamic mount matrix for lid angle) in the user-space layer would be improper
layering.
So there are two possibilites:
a) make the mount-matrix dynamical as described above in L2
b) extend my RFC to handle this special case
> One oddity to note here is that until very recently we deliberately didn't register
> certain ACPI IDs because they confused userspace by reporting two accelerometers
> without any info on which was in the lid. Thankfully proper handling of that
> is no being sorted. It's still mostly a case of just deliberately ignoring one
> of the sensors.
>
>>
>>>
>>>>
>>>> And I think is even less flexible than "always doing it". Let me explain this claim.
>>>>
>>>> For me, the kernel should present everything the hardware supports to user-space
>>>> in better digestable device files or APIs (without making any assumptions about the
>>>> user-space code).
>>>
>>> Agreed, we just have a different view on how this should be done. I want
>>> it to be dynamic and extremely flexible, you want the easy way of just
>>> putting a fixed set out all the time.
>>>
>>>>
>>>> Then, every user-space that will be installed can find out what the hardware supports
>>>> by looking at standard places.
>>>>
>>>> E.g. it can scan for all mice and keyboards. And for all input accelerometers.
>>>
>>> Or, you an have the correct 'fairly trivial' userspace setup to scan for all
>>> registered accelerometers and 'on demand' create the bindings to bring them up as
>>> Input accelerometers if that is what makes sense for your platform.
>>
>> Why not scan for input accelerometers and leave it as an implementation detail that
>> the kernel does serve the physical chips through the iio infrastructure?
>
> If we could separate the IIO front end from the IIO backend I would agree that
> would be another valid -userspace- policy.
>
>>
>> IMHO some user-spaces may already be scanning all */input/event* and check for
>> the device property INPUT_PROP_ACCELEROMETER.
>>
>> This is a discussion mainly about proper encapsulation of lower level differences.
>>
>>>
>>>>
>>>> If the kernel is hiding some chips and needs some initial user-space action before
>>>> presenting them all, this requires that the user-space has some a-priori knowledge
>>>> about which specific devices it should ask for.
>>>
>>> No more that it needs to know which accelerometer to use?
>>
>>>
>>>> So it does not really need to scan
>>>> for them. Because it must already know. Obviously in some mapping table stored at
>>>> a well known location inside the rootfs image.
>>>
>>> No. Let me give some more details of how this would work. It's really just
>>> a more flexible version of what you have.
>>>
>>> A distro, or individual user decides to put the relevant script in place for the
>>> following:
>>>
>>> 1. Userspace detects a new accelerometer driver, via the standard methods (uevent)
>>> 2. Userspace looks to see if it has the required properties. Now this includes things
>>> like detecting that it is the accelerometer in the lid of a laptop - if so do not
>>> register it as an input device. If it's in the keyboard then do register it.
>>> 3. Userspace script then creates the files in configfs
>>> /sys/kernel/config/iio/maps/
>>> (this interface needs appropriate definition)
>>> Maybe...
>>> /sys/kernel/config/iio/maps/iio_input/iio_device:X/accel_x, accel_y, etc
>>> When done it writes to the bind file
>>> /sys/kernel/config/iio/maps/iio_input/iio_device:X/bind
>>> which instantiates the input driver.
>>>
>>> This moves all of the policy decision into userspace, where it belongs. If
>>> we want to enable a particular accelerometer on a particular board because it
>>> actually works better than the one the default policy says to use, then we can
>>> do so.
>>>
>>> The resulting infrastructure is much more general, because it lets us do the
>>> same for any IIO consumer. This input bridge is not a special case. It works
>>> equally well for the existing hwmon bridge any would even let us do things
>>> like provide the information from userspace that we have an analog accelerometer
>>> wired up to an ADC on some hacker board.
>>
>> Ok, understood.
>>
>> My approach triggers input uevents:
>>
>> 1. kernel detects a new iio accelerometer (looks like an analog accelerometer should be
>> the DTS child of an iio adc and then iio should create an accelerometer and not a voltage
>> channel)
>
> Yes ultimately it would be a child device that would be it's own IIO device. We
> already have this for some gyroscopes.
>
>> 2. iio-bridge registers as input event
>> 3. this triggers an uevent
>> 4 an udev-rule can detect the properties and map it to some "speaking" name like
>> /dev/input/main-accelerometer, /dev/input/lid-accelerometer etc. Or if the
>> accelerometer is to be ignored, it does not get a "speaking" name at all.
>>
>> The required udev rules are stored in user space and are of course user-space and application
>> specific. But this does not require to invent some new configfs stuff and special scripts
>> in user-space. Just install some udev rule at a well established location in file-system.
>
> I'm not sure there is any significant difference between you creating a mapping like
> this an udev rule that creates the whole mapping. Bit more to do perhaps but it's
> nothing particularly special that I can see. Sure there is new kernel support to be
> done.
>
>>
>> Yes, this does not cover arbitrary mappings. But what are arbitrary mappings good
>> for? Your scheme seems to be able to map a light sensor to accelerometer input.
>> Does this "full matrix of everything is possible" really make sense?
>
> From a generic interface point of view - yes it absolutely does.
>
> We define an interface that covers all usecases rather than a whole set of
> separate ones that cover individual corner cases. That way we don't have to
> keep defining new interfaces.
>
> The individual drivers can easily do validation of what they are provided with.
>
>>
>> I can't decide because I have no need for it. Others may have.
>>
>> But another thought: does it interfere with this input-bridge? Probably no. You can
>> still add your configfs approach for general iio devices to e.g. hwmon mappings. Even
>> as an alternate method of creating input devices (enabled only if my input-bridge is
>> disabled).
>
> Yes see above. Both approaches meet your requirement (I think anyway).
> I do not want to see two long term solutions to the same problem.
>
> I'm interested in a long term sustainable solution so I want to see
> the generic one.
>
>>
>>>
>>>
>>>>
>>>> This seems to make it impossible to develop a generic distro rootfs image - without
>>>> asking the user for manual configuration. And that where the kernel already knows
>>>> this (which iio accelerometers do exist for a specific piece of hardware).
>>>>
>>>> This is why I believe a mechanism to instantiate only on demand isn't adding but
>>>> removing flexibility because it prevents copying a rootfs from one device to another.
>>>
>>> I disagree, see above.
>>>
>>>>
>>>>>
>>>>> As I mentioned in V1, look at the possibility of a configfs based method
>>>>> to build the map. It's easy for userspace to work out what makes sense to
>>>>> map in principle. There may be some missing info that we also need to
>>>>> look to expose.
>>>>
>>>> With a "may be missing" it is impossible to write code for it...
>>>> Can you please name which information is missing on the input accelerometer
>>>> API?
>>>
>>> See above. It's not the input accelerometer ABI, it's the missing ability
>>> to instantiate IIO maps from user space.
>>>
>>>>
>>>>>
>>>>> In general, userspace created channel maps would be very useful for
>>>>> other things such as maker type boards where they can plug all sorts
>>>>> of odd things into ADC channels for example.
>>>>
>>>> Ok, I understand, but this is a different problem where this iio-input-bridge is not
>>>> intended to be a solution. Generic ADCs are not input devices. Like SD cards are not
>>>> keyboards.
>>>>
>>>> So we should not try to mix the idea of general mapping with this input-bridge for
>>>> input accelerometers.
>>> Yes we should. You are proposing a solution that is a subset of the larger
>>> problem set.
>>
>> Yes, of course. Because I did not see or know about the general problem set.
>> And I still don't see a need for user-space controlled mapping for input-accelerometers.
>
> We are clearly going to differ on this. Bastien gave one example for why
> this is required. There will be others.
>
>>
>>> Why introduce a stop gap like this when we can do it correctly
>>> and provide something useful for all those other use cases.
>>>
>>> The only difference here is the uevent triggered script that creates those maps
>>> for your particular usecase.
>>
>> Well, I am a friend of solving one problem after the other in smaller steps than
>> immediately aiming at a very general solution, which has side-effects of inventing
>> new stuff for things that would work without.
>
> That works in a world where you can drop the previous approach as part of your
> generalization. When you are playing with kernel / userspace ABI then it
> doesn't. Ideally you have to figure out the extensible general solution at the
> start because you are stuck maintaining the 'small steps' for many years to
> come. I don't want to perpetually 'have' to export all 3D accelerometers as
> input devices, because we didn't have the ability to chose which should be
> exported at some point in the past.
>
>>
>>>
>>>
>>>>
>>>> BTW, there is a way to define additional mapping using udev rules which symlink the
>>>> /dev/input/event* paths to stable names like /dev/input/accelerometer.
>>>>
>>>> This comes without additional code and is already provided by udev and the input system.
>>>>
>>>> So in summary, I have not yet seen a convincing scenario where being able to dynamically
>>>> map iio channels to input devices seems beneficial.
>>>
>>> That is true for the narrow case you are talking about. I don't want to see that
>>> narrow case solved in a fashion that effectively breaks solving it properly.
>>
>> How does it break your approach if added later? The more I think about it they are
>> not incompatible. It is just useless to apply both in parallel.
>
> The reality is that if we put one in first that will used for ever because there
> will be devices out there using it. Therefore we have to maintain both for
> ever.
>
>>
>>> If we add this, we have to export all accelerometers for ever under all circumstances
>>> to userspace, because to remove it will break existing userspace.
>>>
>>> If we stand back and work out if we can do the general solution now, we avoid
>>> this problem.
>>
>> We get a different problem that we break existing user-space that simply wants to see
>> an /dev/input/accelerometer without doing more than an existing udev rule.
>
> I would love to say such a userspace doesn't exist, but reality is there are
> all sorts of hideous things out there. There are cases that deal with this
> as an option of course (such as Bastien's sensor-proxy)
>
> The number of devices that are supported under mainline as input accelerometers
> is pretty small. It's not a perfect world unfortunately but having to add a
> small udev script is at least not a major break if we do cause it.
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>> This driver simply collects the first 3 accelerometer channels as X, Y and Z.
>>>>>> If only 1 or 2 channels are available, they are used for X and Y only. Additional
>>>>>> channels are ignored.
>>>>>>
>>>>>> Scaling is done automatically so that 1g is represented by value 256 and
>>>>>> range is assumed to be -511 .. +511 which gives a reasonable precision as an
>>>>>> input device.
>>>>>
>>>>> Why do we do this, rather than letting input deal with it? Input is used
>>>>> to widely differing scales IIRC
>>>>
>>>> Well, it can't be done differently... And what I call scale here is nothing more than
>>>> defining ABSMIN_ACC_VAL and ABSMAX_ACC_VAL.
>>>>
>>>> We need to apply some scale since iio reports in (fractional) units of 1g, i.e. values
>>>> of magnitude 1.
>>>
>>> m/s^2 not g, but doesn't matter for the point of view of this discussion.
>>
>> My fault. The driver takes care of this in the scaling formula so that "input" reports
>> MAX/2 for 1g.
>>
>>>
>>>> These are not adaequate for input events which use integers. So we must
>>>> define some factor for iio_convert_raw_to_processed() to scale from raw value range
>>>> to int value range. We could report raw values but this would be an improper abstraction
>>>> from chip specific differences.
>>>
>>> Hmm. I can see we perhaps need some mapping, but is there a concept of standard scale
>>> for existing input accelerometers? How is this done to give for other input devices
>>> such as touch screens? I'd expect to see a separation between scale, and range.
>>>
>>>
>>>>
>>>> BTW: the range (and therefore the factor) is reported through the evdev driver to user-space
>>>> (evtest reports Min and Max as you can see in the example).
>>>>
>>>> The most important thing is that this is a hardware independent definition. Every accelerometer
>>>> chip will report this range. So you can easily upgrade hardware or switch accelerometers
>>>> without touching user-space calibration. Like you can replace ethernet controller chips but
>>>> networking works the same with all of them.
>>>
>>> Agreed, it needs to be hardware independent by the time it hits userspace, but I would
>>> have thought that scaling would be done in input, rather than IIO. It's hardly
>>> a problem unique to our usecase!
>>>
>>> Perhaps Dmitry can give some advice on this.
>>
>> Yes, that would be helpful.
>>
>>>
>>>>
>>>>
>>>> Hm. Is there an alternative to attach such private data to an struct iio_dev
>>>> allocated by someone else? I have not found one yet.
>>>>
>>>> Or can I add some void *input_mapping; to struct iio_dev? Depending on
>>>> #if defined(CONFIG_IIO_INPUT_BRIDGE)?
>>>
>>> Yes, add a new element.
>>
>> Ok, works fine.
>>
>> I already have found one case of iio accelerometer driver where it did make a problem
>> not using a special element.
>>
>>>>>
>>>>> iio_input_find_accel_channel(indio_dev, chan, &numchans);
>>>>> iio_input_register_device(indio_dev, chan, numchans);
>>>>
>>>> Well, that looks like it needs some temporary storage of dynamic size
>>>> and loop twice over channels for no functional benefit.
>>>
>>> Use fixed size. The worst that happens is we end up with it being
>>> an entry larger that it needs to be.
>>>
>>>> And handle the
>>>> special case of numchans == 0 (the proposed code simply does not call
>>>> iio_input_register_accel_channel and does not register anything).
>>>>
>>>> So I'd prefer to follow the "KISS" principle and register single channels
>>>> instead of a set of channels.
>>>
>>> Well we disagree on this. A singleton approach like used here
>>> is to my mind not KISS. I would rather see what is there then
>>> act as two simple steps, rather than interleave two different
>>> actions with a totally different path for the first channel found.
>>> If there is only one channel you just built a load of infrastructure
>>> that makes no sense. If you scan first then you can know that
>>> before building anything.
>>
>> Ok, this is more a matter of taste and resource requirements can probably
>> be neglected. I'll update the driver.
>>
>> So in summary, I'll post a v3 that fixes some bugs of v2 (because we need
>> them fixed for our production systems as well).
>>
>> Then it is up to you if you want to take this approach or want to write
>> a full version following your concept. Or if it is possible as I assume, we
>> can have both.
>
> Thanks. I think we need at some code for what I was proposing to discuss
> much further. Unfortunately it may be a little while before I get time to
> work on that. Hopefully not too long though!
>
> Jonathan
>
>>
>> BR and thanks,
>> Nikolaus
>>
>
^ permalink raw reply
* RE: [PATCH] input: imx6ul_tsc: use devm_platform_ioremap_resource() to simplify code
From: Anson Huang @ 2019-05-09 1:41 UTC (permalink / raw)
To: Mukesh Ojha, dmitry.torokhov@gmail.com, shawnguo@kernel.org,
s.hauer@pengutronix.de, kernel@pengutronix.de, festevam@gmail.com,
linux-input@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Cc: dl-linux-imx
In-Reply-To: <DB3PR0402MB3916D9B009F5187D52A8A7D3F5380@DB3PR0402MB3916.eurprd04.prod.outlook.com>
Ping...
> -----Original Message-----
> From: Anson Huang
> Sent: Sunday, April 28, 2019 2:30 PM
> To: Mukesh Ojha <mojha@codeaurora.org>; dmitry.torokhov@gmail.com;
> shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> festevam@gmail.com; linux-input@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>
> Subject: RE: [PATCH] input: imx6ul_tsc: use
> devm_platform_ioremap_resource() to simplify code
>
> Ping...
>
> > -----Original Message-----
> > From: Mukesh Ojha [mailto:mojha@codeaurora.org]
> > Sent: Monday, April 1, 2019 4:02 PM
> > To: Anson Huang <anson.huang@nxp.com>; dmitry.torokhov@gmail.com;
> > shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> > festevam@gmail.com; linux-input@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> > Cc: dl-linux-imx <linux-imx@nxp.com>
> > Subject: Re: [PATCH] input: imx6ul_tsc: use
> > devm_platform_ioremap_resource() to simplify code
> >
> >
> > On 4/1/2019 10:49 AM, Anson Huang wrote:
> > > Use the new helper devm_platform_ioremap_resource() which wraps the
> > > platform_get_resource() and devm_ioremap_resource() together, to
> > > simplify the code.
> > >
> > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> >
> >
> > Reviewed-by: Mukesh Ojha <mojha@codeaurora.org>
> >
> > Cheers,
> > -Mukesh
> >
> > > ---
> > > drivers/input/touchscreen/imx6ul_tsc.c | 8 ++------
> > > 1 file changed, 2 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/input/touchscreen/imx6ul_tsc.c
> > > b/drivers/input/touchscreen/imx6ul_tsc.c
> > > index c10fc59..e04eecd 100644
> > > --- a/drivers/input/touchscreen/imx6ul_tsc.c
> > > +++ b/drivers/input/touchscreen/imx6ul_tsc.c
> > > @@ -364,8 +364,6 @@ static int imx6ul_tsc_probe(struct
> > > platform_device
> > *pdev)
> > > struct device_node *np = pdev->dev.of_node;
> > > struct imx6ul_tsc *tsc;
> > > struct input_dev *input_dev;
> > > - struct resource *tsc_mem;
> > > - struct resource *adc_mem;
> > > int err;
> > > int tsc_irq;
> > > int adc_irq;
> > > @@ -403,16 +401,14 @@ static int imx6ul_tsc_probe(struct
> > platform_device *pdev)
> > > return err;
> > > }
> > >
> > > - tsc_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > - tsc->tsc_regs = devm_ioremap_resource(&pdev->dev, tsc_mem);
> > > + tsc->tsc_regs = devm_platform_ioremap_resource(pdev, 0);
> > > if (IS_ERR(tsc->tsc_regs)) {
> > > err = PTR_ERR(tsc->tsc_regs);
> > > dev_err(&pdev->dev, "failed to remap tsc memory: %d\n",
> > err);
> > > return err;
> > > }
> > >
> > > - adc_mem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > > - tsc->adc_regs = devm_ioremap_resource(&pdev->dev, adc_mem);
> > > + tsc->adc_regs = devm_platform_ioremap_resource(pdev, 1);
> > > if (IS_ERR(tsc->adc_regs)) {
> > > err = PTR_ERR(tsc->adc_regs);
> > > dev_err(&pdev->dev, "failed to remap adc memory: %d\n",
> > err);
^ permalink raw reply
* RE: [RESEND] input: keyboard: imx: make sure keyboard can always wake up system
From: Anson Huang @ 2019-05-09 1:41 UTC (permalink / raw)
To: dmitry.torokhov@gmail.com, shawnguo@kernel.org,
s.hauer@pengutronix.de, kernel@pengutronix.de, festevam@gmail.com,
linux-input@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Cc: dl-linux-imx
In-Reply-To: <DB3PR0402MB39167BC7D996F4FF70B5DD2FF53D0@DB3PR0402MB3916.eurprd04.prod.outlook.com>
Ping...
> -----Original Message-----
> From: Anson Huang
> Sent: Thursday, April 25, 2019 9:50 AM
> To: dmitry.torokhov@gmail.com; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> linux-input@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>
> Subject: RE: [RESEND] input: keyboard: imx: make sure keyboard can always
> wake up system
>
> Gentle ping...
>
> > -----Original Message-----
> > From: Anson Huang
> > Sent: Thursday, April 4, 2019 9:40 AM
> > To: dmitry.torokhov@gmail.com; shawnguo@kernel.org;
> > s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> > linux-input@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > linux- kernel@vger.kernel.org
> > Cc: dl-linux-imx <linux-imx@nxp.com>
> > Subject: [RESEND] input: keyboard: imx: make sure keyboard can always
> > wake up system
> >
> > There are several scenarios that keyboard can NOT wake up system from
> > suspend, e.g., if a keyboard is depressed between system device
> > suspend phase and device noirq suspend phase, the keyboard ISR will be
> > called and both keyboard depress and release interrupts will be
> > disabled, then keyboard will no longer be able to wake up system.
> > Another scenario would be, if a keyboard is kept depressed, and then
> > system goes into suspend, the expected behavior would be when keyboard
> > is released, system will be waked up, but current implementation can
> > NOT achieve that, because both depress and release interrupts are
> > disabled in ISR, and the event check is still in progress.
> >
> > To fix these issues, need to make sure keyboard's depress or release
> > interrupt is enabled after noirq device suspend phase, this patch
> > moves the suspend/resume callback to noirq suspend/resume phase, and
> > enable the corresponding interrupt according to current keyboard status.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> > drivers/input/keyboard/imx_keypad.c | 18 ++++++++++++++----
> > 1 file changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/input/keyboard/imx_keypad.c
> > b/drivers/input/keyboard/imx_keypad.c
> > index cf08f4a..97500a2 100644
> > --- a/drivers/input/keyboard/imx_keypad.c
> > +++ b/drivers/input/keyboard/imx_keypad.c
> > @@ -524,11 +524,12 @@ static int imx_keypad_probe(struct
> > platform_device *pdev)
> > return 0;
> > }
> >
> > -static int __maybe_unused imx_kbd_suspend(struct device *dev)
> > +static int __maybe_unused imx_kbd_noirq_suspend(struct device *dev)
> > {
> > struct platform_device *pdev = to_platform_device(dev);
> > struct imx_keypad *kbd = platform_get_drvdata(pdev);
> > struct input_dev *input_dev = kbd->input_dev;
> > + unsigned short reg_val = readw(kbd->mmio_base + KPSR);
> >
> > /* imx kbd can wake up system even clock is disabled */
> > mutex_lock(&input_dev->mutex);
> > @@ -538,13 +539,20 @@ static int __maybe_unused
> imx_kbd_suspend(struct
> > device *dev)
> >
> > mutex_unlock(&input_dev->mutex);
> >
> > - if (device_may_wakeup(&pdev->dev))
> > + if (device_may_wakeup(&pdev->dev)) {
> > + if (reg_val & KBD_STAT_KPKD)
> > + reg_val |= KBD_STAT_KRIE;
> > + if (reg_val & KBD_STAT_KPKR)
> > + reg_val |= KBD_STAT_KDIE;
> > + writew(reg_val, kbd->mmio_base + KPSR);
> > +
> > enable_irq_wake(kbd->irq);
> > + }
> >
> > return 0;
> > }
> >
> > -static int __maybe_unused imx_kbd_resume(struct device *dev)
> > +static int __maybe_unused imx_kbd_noirq_resume(struct device *dev)
> > {
> > struct platform_device *pdev = to_platform_device(dev);
> > struct imx_keypad *kbd = platform_get_drvdata(pdev); @@ -568,7
> > +576,9 @@ static int __maybe_unused imx_kbd_resume(struct device
> *dev)
> > return ret;
> > }
> >
> > -static SIMPLE_DEV_PM_OPS(imx_kbd_pm_ops, imx_kbd_suspend,
> > imx_kbd_resume);
> > +static const struct dev_pm_ops imx_kbd_pm_ops = {
> > + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(imx_kbd_noirq_suspend,
> > +imx_kbd_noirq_resume) };
> >
> > static struct platform_driver imx_keypad_driver = {
> > .driver = {
> > --
> > 2.7.4
^ permalink raw reply
* [GIT PULL] Immutable branch between MFD, GPIO, Input, LEDs and Power due for the v5.2 merge window
From: Lee Jones @ 2019-05-08 11:11 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Pavel Machek, Sebastian Reichel, Liam Girdwood,
Greg Kroah-Hartman, linux-kernel, linux-gpio, devicetree,
linux-input, linux-leds, linux-pm, Bartosz Golaszewski
In-Reply-To: <20190423090451.23711-1-brgl@bgdev.pl>
Enjoy!
The following changes since commit e93c9c99a629c61837d5a7fc2120cd2b6c70dbdd:
Linux 5.1 (2019-05-05 17:42:58 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git ib-mfd-gpio-input-leds-power-v5.2
for you to fetch changes up to 796fad0101d370567c2968bd933b865aa57efaa3:
MAINTAINERS: Add an entry for MAX77650 PMIC driver (2019-05-08 12:07:12 +0100)
----------------------------------------------------------------
Immutable branch between MFD, GPIO, Input, LEDs and Power due for the v5.2 merge window
----------------------------------------------------------------
Bartosz Golaszewski (11):
dt-bindings: mfd: Add DT bindings for max77650
dt-bindings: power: supply: Add DT bindings for max77650
dt-bindings: leds: Add DT bindings for max77650
dt-bindings: input: Add DT bindings for max77650
mfd: mfd-core: Document mfd_add_devices()
mfd: Add new driver for MAX77650 PMIC
power: supply: max77650: Add support for battery charger
gpio: max77650: Add GPIO support
leds: max77650: Add LEDs support
input: max77650: Add onkey support
MAINTAINERS: Add an entry for MAX77650 PMIC driver
.../devicetree/bindings/input/max77650-onkey.txt | 26 ++
.../devicetree/bindings/leds/leds-max77650.txt | 57 ++++
Documentation/devicetree/bindings/mfd/max77650.txt | 46 +++
.../bindings/power/supply/max77650-charger.txt | 28 ++
MAINTAINERS | 14 +
drivers/gpio/Kconfig | 7 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-max77650.c | 190 +++++++++++
drivers/input/misc/Kconfig | 9 +
drivers/input/misc/Makefile | 1 +
drivers/input/misc/max77650-onkey.c | 121 +++++++
drivers/leds/Kconfig | 6 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-max77650.c | 147 ++++++++
drivers/mfd/Kconfig | 14 +
drivers/mfd/Makefile | 1 +
drivers/mfd/max77650.c | 232 +++++++++++++
drivers/mfd/mfd-core.c | 13 +
drivers/power/supply/Kconfig | 7 +
drivers/power/supply/Makefile | 1 +
drivers/power/supply/max77650-charger.c | 368 +++++++++++++++++++++
include/linux/mfd/max77650.h | 59 ++++
22 files changed, 1349 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/max77650-onkey.txt
create mode 100644 Documentation/devicetree/bindings/leds/leds-max77650.txt
create mode 100644 Documentation/devicetree/bindings/mfd/max77650.txt
create mode 100644 Documentation/devicetree/bindings/power/supply/max77650-charger.txt
create mode 100644 drivers/gpio/gpio-max77650.c
create mode 100644 drivers/input/misc/max77650-onkey.c
create mode 100644 drivers/leds/leds-max77650.c
create mode 100644 drivers/mfd/max77650.c
create mode 100644 drivers/power/supply/max77650-charger.c
create mode 100644 include/linux/mfd/max77650.h
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v10 06/11] mfd: max77650: new core mfd driver
From: Lee Jones @ 2019-05-08 11:01 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Pavel Machek, Sebastian Reichel, Liam Girdwood,
Greg Kroah-Hartman, linux-kernel, linux-gpio, devicetree,
linux-input, linux-leds, linux-pm, Bartosz Golaszewski
In-Reply-To: <20190423090451.23711-7-brgl@bgdev.pl>
On Tue, 23 Apr 2019, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Add the core mfd driver for max77650 PMIC. We define five sub-devices
> for which the drivers will be added in subsequent patches.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
> drivers/mfd/Kconfig | 14 +++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/max77650.c | 232 +++++++++++++++++++++++++++++++++++
> include/linux/mfd/max77650.h | 59 +++++++++
> 4 files changed, 306 insertions(+)
> create mode 100644 drivers/mfd/max77650.c
> create mode 100644 include/linux/mfd/max77650.h
For my own reference:
Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v2 2/4] mfd: ioc3: Add driver for SGI IOC3 chip
From: Lee Jones @ 2019-05-08 10:23 UTC (permalink / raw)
To: Thomas Bogendoerfer
Cc: Ralf Baechle, Paul Burton, James Hogan, Dmitry Torokhov,
David S. Miller, Alessandro Zummo, Alexandre Belloni,
Greg Kroah-Hartman, Jiri Slaby, linux-mips, linux-kernel,
linux-input, netdev, linux-rtc, linux-serial
In-Reply-To: <20190409154610.6735-3-tbogendoerfer@suse.de>
On Tue, 09 Apr 2019, Thomas Bogendoerfer wrote:
> SGI IOC3 chip has integrated ethernet, keyboard and mouse interface.
> It also supports connecting a SuperIO chip for serial and parallel
> interfaces. IOC3 is used inside various SGI systemboards and add-on
> cards with different equipped external interfaces.
>
> Support for ethernet and serial interfaces were implemented inside
> the network driver. This patchset moves out the not network related
> parts to a new MFD driver, which takes care of card detection,
> setup of platform devices and interrupt distribution for the subdevices.
>
> Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de>
> ---
> arch/mips/include/asm/sn/ioc3.h | 345 +++---
> arch/mips/sgi-ip27/ip27-timer.c | 20 -
> drivers/mfd/Kconfig | 13 +
> drivers/mfd/Makefile | 1 +
> drivers/mfd/ioc3.c | 802 ++++++++++++++
> drivers/net/ethernet/sgi/Kconfig | 4 +-
> drivers/net/ethernet/sgi/ioc3-eth.c | 1867 ++++++++++++---------------------
> drivers/tty/serial/8250/8250_ioc3.c | 98 ++
> drivers/tty/serial/8250/Kconfig | 11 +
> drivers/tty/serial/8250/Makefile | 1 +
> include/linux/platform_data/ioc3eth.h | 15 +
> 11 files changed, 1762 insertions(+), 1415 deletions(-)
> create mode 100644 drivers/mfd/ioc3.c
> create mode 100644 drivers/tty/serial/8250/8250_ioc3.c
> create mode 100644 include/linux/platform_data/ioc3eth.h
[...]
> +config SGI_MFD_IOC3
> + tristate "SGI IOC3 core driver"
> + depends on PCI && MIPS
> + select MFD_CORE
> + help
> + This option enables basic support for the SGI IOC3-based
> + controller cards. This option does not enable any specific
> + functions on such a card, but provides necessary infrastructure
> + for other drivers to utilize.
> +
> + If you have an SGI Origin, Octane, or a PCI IOC3 card,
> + then say Y. Otherwise say N.
> +
> endmenu
> endif
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index b4569ed7f3f3..07255e499129 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -246,4 +246,5 @@ obj-$(CONFIG_MFD_MXS_LRADC) += mxs-lradc.o
> obj-$(CONFIG_MFD_SC27XX_PMIC) += sprd-sc27xx-spi.o
> obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o
> obj-$(CONFIG_MFD_ROHM_BD718XX) += rohm-bd718x7.o
> +obj-$(CONFIG_SGI_MFD_IOC3) += ioc3.o
>
> diff --git a/drivers/mfd/ioc3.c b/drivers/mfd/ioc3.c
> new file mode 100644
> index 000000000000..ad715805b16e
> --- /dev/null
> +++ b/drivers/mfd/ioc3.c
> @@ -0,0 +1,802 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * SGI IOC3 multifunction device driver
> + *
> + * Copyright (C) 2018, 2019 Thomas Bogendoerfer <tbogendoerfer@suse.de>
> + *
> + * Based on work by:
> + * Stanislaw Skowronek <skylark@unaligned.org>
> + * Joshua Kinard <kumba@gentoo.org>
> + * Brent Casavant <bcasavan@sgi.com> - IOC4 master driver
> + * Pat Gefre <pfg@sgi.com> - IOC3 serial port IRQ demuxer
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/mfd/core.h>
> +#include <linux/platform_device.h>
> +#include <linux/platform_data/ioc3eth.h>
These should be in alphabetical order.
> +#include <asm/sn/ioc3.h>
> +#include <asm/pci/bridge.h>
> +
> +#define IOC3_ETH BIT(0)
> +#define IOC3_SER BIT(1)
> +#define IOC3_PAR BIT(2)
> +#define IOC3_KBD BIT(3)
> +#define IOC3_M48T35 BIT(4)
> +
> +static int ioc3_serial_id = 1;
> +static int ioc3_eth_id = 1;
> +static int ioc3_kbd_id = 1;
> +static struct mfd_cell ioc3_mfd_cells[5];
> +
> +struct ioc3_board_info {
> + const char *name;
> + int irq_offset;
> + int funcs;
> +};
> +
> +struct ioc3_priv_data {
> + struct ioc3_board_info *info;
> + struct irq_domain *domain;
> + struct ioc3 __iomem *regs;
> + struct pci_dev *pdev;
> + char nic_part[32];
> + char nic_mac[6];
> + int irq_io;
> +};
> +
> +#define MCR_PACK(pulse, sample) (((pulse) << 10) | ((sample) << 2))
> +
> +static int ioc3_nic_wait(u32 __iomem *mcr)
> +{
> + u32 mcr_val;
> +
> + do {
> + mcr_val = readl(mcr);
> + } while (!(mcr_val & 2));
Why 2? Is bit 2 always set on a successful read?
Either way, you should provide a comment to improve readability.
> + return (mcr_val & 1);
Reads just a bit at a time? Again, a comment please.
> +}
> +
> +static int ioc3_nic_reset(u32 __iomem *mcr)
> +{
> + int presence;
> + unsigned long flags;
> +
> + local_irq_save(flags);
> + writel(MCR_PACK(520, 65), mcr);
What do these magic values mean? Best to define them.
> + presence = ioc3_nic_wait(mcr);
> + local_irq_restore(flags);
> +
> + udelay(500);
What are you waiting for? Why 500us?
> + return presence;
> +}
> +
> +static int ioc3_nic_read_bit(u32 __iomem *mcr)
> +{
> + int result;
> + unsigned long flags;
> +
> + local_irq_save(flags);
> + writel(MCR_PACK(6, 13), mcr);
Why 6 and 13?
Define all magic values please.
> + result = ioc3_nic_wait(mcr);
> + local_irq_restore(flags);
> +
> + udelay(100);
Why 100?
> + return result;
> +}
> +
> +static u32 ioc3_nic_read_byte(u32 __iomem *mcr)
> +{
> + u32 result = 0;
> + int i;
> +
> + for (i = 0; i < 8; i++)
> + result = ((result >> 1) | (ioc3_nic_read_bit(mcr) << 7));
> +
> + return result;
> +}
> +
> +static void ioc3_nic_write_bit(u32 __iomem *mcr, int bit)
> +{
> + if (bit)
> + writel(MCR_PACK(6, 110), mcr);
> + else
> + writel(MCR_PACK(80, 30), mcr);
> +
> + ioc3_nic_wait(mcr);
> +}
> +
> +static void ioc3_nic_write_byte(u32 __iomem *mcr, int byte)
> +{
> + int i;
> +
> + for (i = 0; i < 8; i++) {
> + ioc3_nic_write_bit(mcr, byte & 1);
> + byte >>= 1;
> + }
> +}
> +
> +static u64 ioc3_nic_find(u32 __iomem *mcr, int *last, u64 addr)
> +{
> + int a, b, index, disc;
> +
> + ioc3_nic_reset(mcr);
> +
> + /* Search ROM. */
> + ioc3_nic_write_byte(mcr, 0xf0);
What does 0xf0 mean?
Please define it and all magic numbers that follow.
> + /* Algorithm from ``Book of iButton Standards''. */
That's great, but what is it actually doing. Please provide suitable
comments such that the reader doesn't have to painstakingly work it
all out.
> + for (index = 0, disc = 0; index < 64; index++) {
> + a = ioc3_nic_read_bit(mcr);
> + b = ioc3_nic_read_bit(mcr);
> +
> + if (unlikely(a && b)) {
> + pr_warn("ioc3: NIC search failed.\n");
> + *last = 0;
> + return 0;
> + }
> +
> + if (!a && !b) {
> + if (index == *last)
> + addr |= 1UL << index;
> + else if (index > *last) {
> + addr &= ~(1UL << index);
> + disc = index;
> + } else if ((addr & (1UL << index)) == 0)
> + disc = index;
> + ioc3_nic_write_bit(mcr, (addr >> index) & 1);
> + continue;
> + } else {
> + if (a)
> + addr |= (1UL << index);
> + else
> + addr &= ~(1UL << index);
> + ioc3_nic_write_bit(mcr, a);
> + continue;
> + }
> + }
> + *last = disc;
> + return addr;
> +}
> +
> +static void ioc3_nic_addr(u32 __iomem *mcr, u64 addr)
> +{
> + int index;
> +
> + ioc3_nic_reset(mcr);
> + ioc3_nic_write_byte(mcr, 0xf0);
> +
> + for (index = 0; index < 64; index++) {
> + ioc3_nic_read_bit(mcr);
> + ioc3_nic_read_bit(mcr);
> + ioc3_nic_write_bit(mcr, ((addr >> index) & 1));
> + }
What is this function doing? Setting the NIC address?
Why are the 2 sequential reads required before each bit write?
> +}
> +
> +static void crc16_byte(u32 *crc, u8 db)
> +{
> + int i;
> +
> + for (i = 0; i < 8; i++) {
> + *crc <<= 1;
> + if ((db ^ (*crc >> 16)) & 1)
> + *crc ^= 0x8005;
> + db >>= 1;
> + }
> + *crc &= 0xffff;
> +}
> +
> +static u32 crc16_area(u8 *dbs, int size, u32 crc)
> +{
> + while (size--)
> + crc16_byte(&crc, *(dbs++));
> +
> + return crc;
> +}
> +
> +static void crc8_byte(u32 *crc, u8 db)
> +{
> + int i, f;
> +
> + for (i = 0; i < 8; i++) {
> + f = ((*crc ^ db) & 1);
> + *crc >>= 1;
> + db >>= 1;
> + if (f)
> + *crc ^= 0x8c;
> + }
> + *crc &= 0xff;
> +}
> +
> +static u32 crc8_addr(u64 addr)
> +{
> + u32 crc = 0;
> + int i;
> +
> + for (i = 0; i < 64; i += 8)
> + crc8_byte(&crc, addr >> i);
> + return crc;
> +}
Not looked into these in any detail, but are you not able to use the
CRC functions already provided by the kernel?
> +static void ioc3_read_redir_page(u32 __iomem *mcr, u64 addr, int page,
What is a redir page?
> + u8 *redir, u8 *data)
> +{
> + int loops = 16, i;
> +
> + while (redir[page] != 0xff) {
> + page = (redir[page] ^ 0xff);
> + loops--;
> + if (unlikely(loops < 0)) {
> + pr_err("ioc3: NIC circular redirection\n");
> + return;
> + }
> + }
> +
> + loops = 3;
> + while (loops > 0) {
> + ioc3_nic_addr(mcr, addr);
> + ioc3_nic_write_byte(mcr, 0xf0);
> + ioc3_nic_write_byte(mcr, (page << 5) & 0xe0);
> + ioc3_nic_write_byte(mcr, (page >> 3) & 0x1f);
> +
> + for (i = 0; i < 0x20; i++)
> + data[i] = ioc3_nic_read_byte(mcr);
> +
> + if (crc16_area(data, 0x20, 0) == 0x800d)
> + return;
> +
> + loops--;
> + }
> +
> + pr_err("ioc3: CRC error in data page\n");
> + for (i = 0; i < 0x20; i++)
> + data[i] = 0x00;
Comments required throughout.
> +}
> +
> +static void ioc3_read_redir_map(u32 __iomem *mcr, u64 addr, u8 *redir)
> +{
> + int i, j, crc_ok, loops = 3;
> + u32 crc;
> +
> + while (loops > 0) {
> + crc_ok = 1;
> + ioc3_nic_addr(mcr, addr);
> + ioc3_nic_write_byte(mcr, 0xaa);
> + ioc3_nic_write_byte(mcr, 0x00);
> + ioc3_nic_write_byte(mcr, 0x01);
> +
> + for (i = 0; i < 64; i += 8) {
> + for (j = 0; j < 8; j++)
> + redir[i + j] = ioc3_nic_read_byte(mcr);
> +
> + crc = crc16_area(redir + i, 8, i == 0 ? 0x8707 : 0);
> +
> + crc16_byte(&crc, ioc3_nic_read_byte(mcr));
> + crc16_byte(&crc, ioc3_nic_read_byte(mcr));
> +
> + if (crc != 0x800d)
> + crc_ok = 0;
> + }
> + if (crc_ok)
> + return;
> + loops--;
> + }
> + pr_err("ioc3: CRC error in redirection page\n");
> + for (i = 0; i < 64; i++)
> + redir[i] = 0xff;
As above.
> +}
> +
> +static void ioc3_read_nic(struct ioc3_priv_data *ipd, u32 __iomem *mcr,
> + u64 addr)
> +{
> + u8 redir[64];
> + u8 data[64], part[32];
> + int i, j;
> +
> + /* Read redirections */
> + ioc3_read_redir_map(mcr, addr, redir);
> +
> + /* Read data pages */
> + ioc3_read_redir_page(mcr, addr, 0, redir, data);
> + ioc3_read_redir_page(mcr, addr, 1, redir, (data + 32));
> +
> + /* Assemble the part # */
> + j = 0;
> + for (i = 0; i < 19; i++)
> + if (data[i + 11] != ' ')
> + part[j++] = data[i + 11];
> +
> + for (i = 0; i < 6; i++)
> + if (data[i + 32] != ' ')
> + part[j++] = data[i + 32];
> +
> + part[j] = 0;
> +
> + /* Skip Octane (IP30) power supplies */
> + if (!(strncmp(part, "060-0035-", 9)) ||
> + !(strncmp(part, "060-0038-", 9)) ||
> + !(strncmp(part, "060-0028-", 9)))
> + return;
> +
> + strlcpy(ipd->nic_part, part, sizeof(ipd->nic_part));
> +}
> +
> +static void ioc3_read_mac(struct ioc3_priv_data *ipd, u64 addr)
> +{
> + int i, loops = 3;
> + u8 data[13];
> + u32 __iomem *mcr = &ipd->regs->mcr;
> +
> + while (loops > 0) {
> + ioc3_nic_addr(mcr, addr);
> + ioc3_nic_write_byte(mcr, 0xf0);
> + ioc3_nic_write_byte(mcr, 0x00);
> + ioc3_nic_write_byte(mcr, 0x00);
> + ioc3_nic_read_byte(mcr);
> +
> + for (i = 0; i < 13; i++)
> + data[i] = ioc3_nic_read_byte(mcr);
> +
> + if (crc16_area(data, 13, 0) == 0x800d) {
> + for (i = 10; i > 4; i--)
> + ipd->nic_mac[10 - i] = data[i];
> + return;
> + }
> + loops--;
> + }
> +
> + pr_err("ioc3: CRC error in MAC address\n");
> + for (i = 0; i < 6; i++)
> + ipd->nic_mac[i] = 0x00;
> +}
> +
> +static void ioc3_probe_nic(struct ioc3_priv_data *ipd, u32 __iomem *mcr)
> +{
> + int save = 0, loops = 3;
> + u64 first, addr;
> +
> + while (loops > 0) {
> + ipd->nic_part[0] = 0;
> + first = ioc3_nic_find(mcr, &save, 0);
> + addr = first;
> +
> + if (unlikely(!first))
> + return;
> +
> + while (1) {
> + if (crc8_addr(addr))
> + break;
> +
> + switch (addr & 0xff) {
> + case 0x0b:
> + ioc3_read_nic(ipd, mcr, addr);
> + break;
> + case 0x09:
> + case 0x89:
> + case 0x91:
> + ioc3_read_mac(ipd, addr);
> + break;
> + }
> +
> + addr = ioc3_nic_find(mcr, &save, addr);
> + if (addr == first)
> + return;
> + }
> + loops--;
> + }
> + pr_err("ioc3: CRC error in NIC address\n");
> +}
This all looks like networking code. If this is the case, it should
be moved to drivers/networking or similar.
> +static void ioc3_irq_ack(struct irq_data *d)
> +{
> + struct ioc3_priv_data *ipd = irq_data_get_irq_chip_data(d);
> + unsigned int hwirq = irqd_to_hwirq(d);
> +
> + writel(BIT(hwirq), &ipd->regs->sio_ir);
> +}
> +
> +static void ioc3_irq_mask(struct irq_data *d)
> +{
> + struct ioc3_priv_data *ipd = irq_data_get_irq_chip_data(d);
> + unsigned int hwirq = irqd_to_hwirq(d);
> +
> + writel(BIT(hwirq), &ipd->regs->sio_iec);
> +}
> +
> +static void ioc3_irq_unmask(struct irq_data *d)
> +{
> + struct ioc3_priv_data *ipd = irq_data_get_irq_chip_data(d);
> + unsigned int hwirq = irqd_to_hwirq(d);
> +
> + writel(BIT(hwirq), &ipd->regs->sio_ies);
> +}
> +
> +static struct irq_chip ioc3_irq_chip = {
> + .name = "IOC3",
> + .irq_ack = ioc3_irq_ack,
> + .irq_mask = ioc3_irq_mask,
> + .irq_unmask = ioc3_irq_unmask,
> +};
> +
> +#define IOC3_LVL_MASK (BIT(0) | BIT(2) | BIT(6) | BIT(9) | BIT(11) | BIT(15))
You need to define what each of these bits are. BIT(2) doesn't tell
the reader anything, meaning the code is unreadable.
> +static int ioc3_irq_domain_map(struct irq_domain *d, unsigned int irq,
> + irq_hw_number_t hwirq)
> +{
> + if (BIT(hwirq) & IOC3_LVL_MASK)
Comment needed.
> + irq_set_chip_and_handler(irq, &ioc3_irq_chip, handle_level_irq);
> + else
> + irq_set_chip_and_handler(irq, &ioc3_irq_chip, handle_edge_irq);
> +
> + irq_set_chip_data(irq, d->host_data);
> + return 0;
> +}
> +
> +
Drop the superfluous '\n'.
> +static const struct irq_domain_ops ioc3_irq_domain_ops = {
> + .map = ioc3_irq_domain_map,
> +};
> +
> +static void ioc3_irq_handler(struct irq_desc *desc)
> +{
> + struct irq_domain *domain = irq_desc_get_handler_data(desc);
> + struct ioc3_priv_data *ipd = domain->host_data;
> + struct ioc3 __iomem *regs = ipd->regs;
> + unsigned int irq = 0;
Why does these need to be pre-assigned?
> + u32 pending;
> +
> + pending = readl(®s->sio_ir);
> + pending &= readl(®s->sio_ies);
Comment required.
> + if (pending)
> + irq = irq_find_mapping(domain, __ffs(pending));
> + else if (!ipd->info->irq_offset &&
> + (readl(®s->eth.eisr) & readl(®s->eth.eier)))
Comment required.
> + irq = irq_find_mapping(domain, 23);
> +
> + if (irq)
> + generic_handle_irq(irq);
> + else
> + spurious_interrupt();
> +}
> +
> +static struct resource ioc3_uarta_resources[] = {
> + DEFINE_RES_MEM(offsetof(struct ioc3, sregs.uarta),
You are the first user of offsetof() in MFD. Could you tell me why
it's required please?
> + sizeof_field(struct ioc3, sregs.uarta)),
> + DEFINE_RES_IRQ(6)
Please define all magic numbers.
> +};
> +
> +static struct resource ioc3_uartb_resources[] = {
> + DEFINE_RES_MEM(offsetof(struct ioc3, sregs.uartb),
> + sizeof_field(struct ioc3, sregs.uartb)),
> + DEFINE_RES_IRQ(15)
> +};
> +
> +static struct resource ioc3_kbd_resources[] = {
> + DEFINE_RES_MEM(offsetof(struct ioc3, serio),
> + sizeof_field(struct ioc3, serio)),
> + DEFINE_RES_IRQ(22)
> +};
> +
> +static struct resource ioc3_eth_resources[] = {
> + DEFINE_RES_MEM(offsetof(struct ioc3, eth),
> + sizeof_field(struct ioc3, eth)),
> + DEFINE_RES_MEM(offsetof(struct ioc3, ssram),
> + sizeof_field(struct ioc3, ssram)),
> + DEFINE_RES_IRQ(0)
> +};
> +
> +static struct ioc3eth_platform_data ioc3_eth_platform_data;
I doubt you'll need this in here. The MAC address info will need to
be moved out to a networking driver of some sort.
> +#ifdef CONFIG_SGI_IP27
#ifery in C files is highly discouraged. Please remove them.
> +static struct resource ioc3_rtc_resources[] = {
> + DEFINE_RES_MEM(IOC3_BYTEBUS_DEV0, 32768)
Define all magic numbers please.
> +};
> +
> +static struct ioc3_board_info ip27_baseio_info = {
> + .name = "IP27 BaseIO",
> + .funcs = IOC3_ETH | IOC3_SER | IOC3_M48T35,
> + .irq_offset = 2
> +};
> +
> +static struct ioc3_board_info ip27_baseio6g_info = {
> + .name = "IP27 BaseIO6G",
> + .funcs = IOC3_ETH | IOC3_SER | IOC3_KBD | IOC3_M48T35,
> + .irq_offset = 2
> +};
> +
> +static struct ioc3_board_info ip27_mio_info = {
> + .name = "MIO",
> + .funcs = IOC3_SER | IOC3_PAR | IOC3_KBD,
> + .irq_offset = 0
> +};
> +
> +static struct ioc3_board_info ip29_baseio_info = {
> + .name = "IP29 System Board",
> + .funcs = IOC3_ETH | IOC3_SER | IOC3_PAR | IOC3_KBD | IOC3_M48T35,
> + .irq_offset = 1
> +};
> +
> +#endif /* CONFIG_SGI_IP27 */
> +
> +static struct ioc3_board_info ioc3_menet_info = {
> + .name = "MENET",
> + .funcs = IOC3_ETH | IOC3_SER,
> + .irq_offset = 4
> +};
> +
> +static struct ioc3_board_info ioc3_cad_duo_info = {
> + .name = "CAD DUO",
> + .funcs = IOC3_ETH | IOC3_KBD,
> + .irq_offset = 0
> +};
Please drop all of these and statically create the MFD cells like
almost all other MFD drivers do.
> +#define IOC3_BOARD(_partno, _info) { .info = _info, .match = _partno }
> +
> +static struct {
> + struct ioc3_board_info *info;
> + const char *match;
> +} ioc3_boards[] = {
> +#ifdef CONFIG_SGI_IP27
> + IOC3_BOARD("030-0734-", &ip27_baseio6g_info),
> + IOC3_BOARD("030-1023-", &ip27_baseio_info),
> + IOC3_BOARD("030-1124-", &ip27_baseio_info),
> + IOC3_BOARD("030-1025-", &ip29_baseio_info),
> + IOC3_BOARD("030-1244-", &ip29_baseio_info),
> + IOC3_BOARD("030-1389-", &ip29_baseio_info),
> + IOC3_BOARD("030-0880-", &ip27_mio_info),
> +#endif
> + IOC3_BOARD("030-0873-", &ioc3_menet_info),
> + IOC3_BOARD("030-1155-", &ioc3_cad_duo_info),
> +};
> +
> +static int ioc3_identify(struct ioc3_priv_data *idp)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(ioc3_boards); i++)
> + if (!strncmp(idp->nic_part, ioc3_boards[i].match,
> + strlen(ioc3_boards[i].match))) {
> + idp->info = ioc3_boards[i].info;
> + return 0;
> + }
> +
> + return -1;
Please return a proper Linux error code.
> +}
> +
> +static void ioc3_create_devices(struct ioc3_priv_data *ipd)
> +{
> + struct mfd_cell *cell;
> +
> + memset(ioc3_mfd_cells, 0, sizeof(ioc3_mfd_cells));
> + cell = ioc3_mfd_cells;
> +
> + if (ipd->info->funcs & IOC3_ETH) {
> + memcpy(ioc3_eth_platform_data.mac_addr, ipd->nic_mac,
> + sizeof(ioc3_eth_platform_data.mac_addr));
Better to pull the MAC address from within the Ethernet driver.
> + cell->name = "ioc3-eth";
> + cell->id = ioc3_eth_id++;
> + cell->resources = ioc3_eth_resources;
> + cell->num_resources = ARRAY_SIZE(ioc3_eth_resources);
> + cell->platform_data = &ioc3_eth_platform_data;
> + cell->pdata_size = sizeof(ioc3_eth_platform_data);
Please define all of this in a static struct, external to this
function.
> + if (ipd->info->irq_offset) {
> + /*
> + * Ethernet interrupt is on an extra interrupt
> + * not inside the irq domain, so we need an
> + * extra mfd_add_devices without the domain
> + * argument
> + */
> + ioc3_eth_resources[2].start = ipd->pdev->irq;
> + ioc3_eth_resources[2].end = ipd->pdev->irq;
Using '2' is fragile.
In this case, seeing as you're using the parent's IRQ, you can obtain
the IRQ using the usual platform_*() handlers, using pdev->parent.
> + mfd_add_devices(&ipd->pdev->dev, -1, cell, 1,
Don't use -1, use the defines instead.
Instead of 1, use ARRAY_SIZE() once the cells are defined statically.
> + &ipd->pdev->resource[0], 0, NULL);
> + memset(cell, 0, sizeof(*cell));
> + } else {
> + /* fake hwirq in domain */
What does this mean?
> + ioc3_eth_resources[2].start = 23;
> + ioc3_eth_resources[2].end = 23;
> + cell++;
> + }
> + }
> + if (ipd->info->funcs & IOC3_SER) {
> + writel(GPCR_UARTA_MODESEL | GPCR_UARTB_MODESEL,
> + &ipd->regs->gpcr_s);
> + writel(0, &ipd->regs->gppr[6]);
> + writel(0, &ipd->regs->gppr[7]);
> + udelay(100);
> + writel(readl(&ipd->regs->port_a.sscr) & ~SSCR_DMA_EN,
> + &ipd->regs->port_a.sscr);
> + writel(readl(&ipd->regs->port_b.sscr) & ~SSCR_DMA_EN,
> + &ipd->regs->port_b.sscr);
> + udelay(1000);
No idea what any of this does.
It looks like it belongs in the serial driver (and needs comments).
> + cell->name = "ioc3-serial8250";
> + cell->id = ioc3_serial_id++;
> + cell->resources = ioc3_uarta_resources;
> + cell->num_resources = ARRAY_SIZE(ioc3_uarta_resources);
> + cell++;
> + cell->name = "ioc3-serial8250";
> + cell->id = ioc3_serial_id++;
> + cell->resources = ioc3_uartb_resources;
> + cell->num_resources = ARRAY_SIZE(ioc3_uartb_resources);
> + cell++;
Please define this statically.
> + }
> + if (ipd->info->funcs & IOC3_KBD) {
> + cell->name = "ioc3-kbd",
> + cell->id = ioc3_kbd_id++;
> + cell->resources = ioc3_kbd_resources,
> + cell->num_resources = ARRAY_SIZE(ioc3_kbd_resources),
> + cell++;
As above.
> + }
> +#if defined(CONFIG_SGI_IP27)
What is this? Can't you obtain this dynamically by probing the H/W?
> + if (ipd->info->funcs & IOC3_M48T35) {
> + cell->name = "rtc-m48t35";
> + cell->id = -1;
> + cell->resources = ioc3_rtc_resources;
> + cell->num_resources = ARRAY_SIZE(ioc3_rtc_resources);
> + cell++;
Static please.
> + }
> +#endif
> + mfd_add_devices(&ipd->pdev->dev, -1, ioc3_mfd_cells,
Use the defines.
> + cell - ioc3_mfd_cells, &ipd->pdev->resource[0],
?
> + 0, ipd->domain);
> +}
> +
> +static int ioc3_mfd_probe(struct pci_dev *pdev,
> + const struct pci_device_id *pci_id)
> +{
> + struct ioc3_priv_data *ipd;
> + int err, ret = -ENODEV, io_irqno;
> + struct ioc3 __iomem *regs;
> + struct irq_domain *domain;
> + struct fwnode_handle *fn;
> +
> + err = pci_enable_device(pdev);
> + if (err)
> + return err;
> +
> + pci_write_config_byte(pdev, PCI_LATENCY_TIMER, 64);
> + pci_set_master(pdev);
> +
> + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> + if (ret) {
> + dev_warn(&pdev->dev, "Warning: couldn_t set 64-bit DMA mask\n");
> + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> + if (ret) {
> + dev_err(&pdev->dev, "Can't set DMA mask, aborting\n");
> + return ret;
> + }
> + }
> +
> + /* Set up per-IOC3 data */
> + ipd = kzalloc(sizeof(struct ioc3_priv_data), GFP_KERNEL);
Does PCI not provide managed (devm_*() like) helpers?
> + if (!ipd) {
> + ret = -ENOMEM;
> + goto out_disable_device;
> + }
> + ipd->pdev = pdev;
'\n'
> + /*
> + * Map all IOC3 registers. These are shared between subdevices
> + * so the main IOC3 module manages them.
> + */
> + regs = pci_ioremap_bar(pdev, 0);
> + if (!regs) {
> + pr_warn("ioc3: Unable to remap PCI BAR for %s.\n",
> + pci_name(pdev));
> + goto out_free_ipd;
> + }
> + ipd->regs = regs;
> +
> + /* Track PCI-device specific data */
> + pci_set_drvdata(pdev, ipd);
Do you don't need 'ipd->pdev = pdev' then.
> + writel(GPCR_MLAN_EN, &ipd->regs->gpcr_s);
> + ioc3_probe_nic(ipd, ®s->mcr);
This should probably be moved to the networking driver.
> +#ifdef CONFIG_SGI_IP27
No #ifery in MFD c-files please.
> + /* BaseIO NIC is attached to bridge */
> + if (!ipd->nic_part[0]) {
> + struct bridge_controller *bc = BRIDGE_CONTROLLER(pdev->bus);
> +
> + ioc3_probe_nic(ipd, &bc->base->b_nic);
> + }
> +#endif
> +
> + if (ioc3_identify(ipd)) {
> + pr_err("ioc3: part: [%s] unknown card\n", ipd->nic_part);
> + goto out_iounmap;
> + }
> +
> + pr_info("ioc3: part: [%s] %s\n", ipd->nic_part, ipd->info->name);
> +
> + /* Clear IRQs */
A comment! I just fell off my chair! =;-)
> + writel(~0, ®s->sio_iec);
> + writel(~0, ®s->sio_ir);
> + writel(0, ®s->eth.eier);
> + writel(~0, ®s->eth.eisr);
> +
> + if (ipd->info->irq_offset) {
What does this really signify?
> + struct pci_host_bridge *hbrg = pci_find_host_bridge(pdev->bus);
> +
> + io_irqno = hbrg->map_irq(pdev,
> + PCI_SLOT(pdev->devfn) + ipd->info->irq_offset, 0);
> + } else {
> + io_irqno = pdev->irq;
> + }
> + ipd->irq_io = io_irqno;
> +
> + fn = irq_domain_alloc_named_fwnode("IOC3");
> + if (!fn)
> + goto out_iounmap;
> +
> + domain = irq_domain_create_linear(fn, 24, &ioc3_irq_domain_ops, ipd);
> + irq_domain_free_fwnode(fn);
> + if (!domain)
> + goto out_iounmap;
> + ipd->domain = domain;
> +
> + irq_set_chained_handler_and_data(io_irqno, ioc3_irq_handler, domain);
> +
> + ioc3_create_devices(ipd);
> +
> + return 0;
> +
> +out_iounmap:
> + iounmap(ipd->regs);
> +
> +out_free_ipd:
> + kfree(ipd);
> +
> +out_disable_device:
> + pci_disable_device(pdev);
> + return ret;
> +}
> +
> +static void ioc3_mfd_remove(struct pci_dev *pdev)
> +{
> + struct ioc3_priv_data *ipd;
> +
> + ipd = pci_get_drvdata(pdev);
> +
> + /* Clear and disable all IRQs */
> + writel(~0, &ipd->regs->sio_iec);
> + writel(~0, &ipd->regs->sio_ir);
> +
> + /* Release resources */
> + irq_domain_remove(ipd->domain);
> + free_irq(ipd->irq_io, (void *)ipd);
> + iounmap(ipd->regs);
> +
> + pci_disable_device(pdev);
> +
> + kfree(ipd);
> +}
> +
> +static struct pci_device_id ioc3_mfd_id_table[] = {
> + { PCI_VENDOR_ID_SGI, PCI_DEVICE_ID_SGI_IOC3, PCI_ANY_ID, PCI_ANY_ID },
> + { 0, },
> +};
> +MODULE_DEVICE_TABLE(pci, ioc3_mfd_id_table);
> +
> +static struct pci_driver ioc3_mfd_driver = {
> + .name = "IOC3",
> + .id_table = ioc3_mfd_id_table,
> + .probe = ioc3_mfd_probe,
> + .remove = ioc3_mfd_remove,
> +};
> +
> +module_pci_driver(ioc3_mfd_driver);
> +
> +MODULE_AUTHOR("Thomas Bogendoerfer <tbogendoerfer@suse.de>");
> +MODULE_DESCRIPTION("SGI IOC3 MFD driver");
> +MODULE_LICENSE("GPL");
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH] Input: libps2 - mark expected switch fall-through
From: Gustavo A. R. Silva @ 2019-05-07 21:37 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, Kees Cook
In-Reply-To: <20190507192744.GA248929@dtor-ws>
On 5/7/19 2:27 PM, Dmitry Torokhov wrote:
> On Tue, May 07, 2019 at 01:24:09PM -0500, Gustavo A. R. Silva wrote:
>> In preparation to enabling -Wimplicit-fallthrough, mark switch
>> cases where we are expecting to fall through.
>>
>> This patch fixes the following warning:
>>
>> drivers/input/serio/libps2.c: In function ‘ps2_handle_ack’:
>> drivers/input/serio/libps2.c:407:6: warning: this statement may fall through [-Wimplicit-fallthrough=]
>> if (ps2dev->flags & PS2_FLAG_NAK) {
>> ^
>> drivers/input/serio/libps2.c:417:2: note: here
>> case 0x00:
>> ^~~~
>>
>> Warning level 3 was used: -Wimplicit-fallthrough=3
>>
>> This patch is part of the ongoing efforts to enable
>> -Wimplicit-fallthrough.
>>
>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>
> Applied, thank you.
>
Awesome. :)
Thanks, Dmitry.
--
Gustavo
^ permalink raw reply
* Re: [PATCH] Input: libps2 - mark expected switch fall-through
From: Dmitry Torokhov @ 2019-05-07 19:27 UTC (permalink / raw)
To: Gustavo A. R. Silva; +Cc: linux-input, linux-kernel, Kees Cook
In-Reply-To: <20190507182409.GA11027@embeddedor>
On Tue, May 07, 2019 at 01:24:09PM -0500, Gustavo A. R. Silva wrote:
> In preparation to enabling -Wimplicit-fallthrough, mark switch
> cases where we are expecting to fall through.
>
> This patch fixes the following warning:
>
> drivers/input/serio/libps2.c: In function ‘ps2_handle_ack’:
> drivers/input/serio/libps2.c:407:6: warning: this statement may fall through [-Wimplicit-fallthrough=]
> if (ps2dev->flags & PS2_FLAG_NAK) {
> ^
> drivers/input/serio/libps2.c:417:2: note: here
> case 0x00:
> ^~~~
>
> Warning level 3 was used: -Wimplicit-fallthrough=3
>
> This patch is part of the ongoing efforts to enable
> -Wimplicit-fallthrough.
>
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
Applied, thank you.
> ---
> drivers/input/serio/libps2.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/input/serio/libps2.c b/drivers/input/serio/libps2.c
> index e6a07e68d1ff..22b8e05aa36c 100644
> --- a/drivers/input/serio/libps2.c
> +++ b/drivers/input/serio/libps2.c
> @@ -409,6 +409,7 @@ bool ps2_handle_ack(struct ps2dev *ps2dev, u8 data)
> ps2dev->nak = PS2_RET_ERR;
> break;
> }
> + /* Fall through */
>
> /*
> * Workaround for mice which don't ACK the Get ID command.
> --
> 2.21.0
>
--
Dmitry
^ permalink raw reply
* [PATCH 3/3] HID: wacom: Sync INTUOSP2_BT touch state after each frame if necessary
From: Gerecke, Jason @ 2019-05-07 18:53 UTC (permalink / raw)
To: linux-input, Benjamin Tissoires
Cc: Ping Cheng, Aaron Armstrong Skomra, Jason Gerecke, stable
In-Reply-To: <20190507185322.7168-1-jason.gerecke@wacom.com>
From: Jason Gerecke <jason.gerecke@wacom.com>
The Bluetooth interface of the 2nd-gen Intuos Pro batches together four
independent "frames" of finger data into a single report. Each frame
is essentially equivalent to a single USB report, with the up-to-10
fingers worth of information being spread across two frames. At the
moment the driver only calls `input_sync` after processing all four
frames have been processed, which can result in the driver sending
multiple updates for a single slot within the same SYN_REPORT. This
can confuse userspace, so modify the driver to sync more often if
necessary (i.e., after reporting the state of all fingers).
Fixes: 4922cd26f0 ("HID: wacom: Support 2nd-gen Intuos Pro's Bluetooth classic interface")
Cc: <stable@vger.kernel.org> # 4.11+
Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
---
drivers/hid/wacom_wac.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index e848445236d8..09b8e4aac82f 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -1371,11 +1371,17 @@ static void wacom_intuos_pro2_bt_touch(struct wacom_wac *wacom)
if (wacom->num_contacts_left <= 0) {
wacom->num_contacts_left = 0;
wacom->shared->touch_down = wacom_wac_finger_count_touches(wacom);
+ input_sync(touch_input);
}
}
- input_report_switch(touch_input, SW_MUTE_DEVICE, !(data[281] >> 7));
- input_sync(touch_input);
+ if (wacom->num_contacts_left == 0) {
+ // Be careful that we don't accidentally call input_sync with
+ // only a partial set of fingers of processed
+ input_report_switch(touch_input, SW_MUTE_DEVICE, !(data[281] >> 7));
+ input_sync(touch_input);
+ }
+
}
static void wacom_intuos_pro2_bt_pad(struct wacom_wac *wacom)
--
2.21.0
^ permalink raw reply related
* [PATCH 2/3] HID: wacom: Correct button numbering 2nd-gen Intuos Pro over Bluetooth
From: Gerecke, Jason @ 2019-05-07 18:53 UTC (permalink / raw)
To: linux-input, Benjamin Tissoires
Cc: Ping Cheng, Aaron Armstrong Skomra, Jason Gerecke, stable,
Aaron Skomra
In-Reply-To: <20190507185322.7168-1-jason.gerecke@wacom.com>
From: Jason Gerecke <jason.gerecke@wacom.com>
The button numbering of the 2nd-gen Intuos Pro is not consistent between
the USB and Bluetooth interfaces. Over USB, the HID_GENERIC codepath
enumerates the eight ExpressKeys first (BTN_0 - BTN_7) followed by the
center modeswitch button (BTN_8). The Bluetooth codepath, however, has
the center modeswitch button as BTN_0 and the the eight ExpressKeys as
BTN_1 - BTN_8. To ensure userspace button mappings do not change
depending on how the tablet is connected, modify the Bluetooth codepath
to report buttons in the same order as USB.
To ensure the mode switch LED continues to toggle in response to the
mode switch button, the `wacom_is_led_toggled` function also requires
a small update.
Link: https://github.com/linuxwacom/input-wacom/pull/79
Fixes: 4922cd26f0 ("HID: wacom: Support 2nd-gen Intuos Pro's Bluetooth classic interface")
Cc: <stable@vger.kernel.org> # 4.11+
Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
Reviewed-by: Aaron Skomra <aaron.skomra@wacom.com>
---
drivers/hid/wacom_wac.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index af62a630fee9..e848445236d8 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -1383,7 +1383,7 @@ static void wacom_intuos_pro2_bt_pad(struct wacom_wac *wacom)
struct input_dev *pad_input = wacom->pad_input;
unsigned char *data = wacom->data;
- int buttons = (data[282] << 1) | ((data[281] >> 6) & 0x01);
+ int buttons = data[282] | ((data[281] & 0x40) << 2);
int ring = data[285] & 0x7F;
bool ringstatus = data[285] & 0x80;
bool prox = buttons || ringstatus;
@@ -3832,7 +3832,7 @@ static void wacom_24hd_update_leds(struct wacom *wacom, int mask, int group)
static bool wacom_is_led_toggled(struct wacom *wacom, int button_count,
int mask, int group)
{
- int button_per_group;
+ int group_button;
/*
* 21UX2 has LED group 1 to the left and LED group 0
@@ -3842,9 +3842,12 @@ static bool wacom_is_led_toggled(struct wacom *wacom, int button_count,
if (wacom->wacom_wac.features.type == WACOM_21UX2)
group = 1 - group;
- button_per_group = button_count/wacom->led.count;
+ group_button = group * (button_count/wacom->led.count);
- return mask & (1 << (group * button_per_group));
+ if (wacom->wacom_wac.features.type == INTUOSP2_BT)
+ group_button = 8;
+
+ return mask & (1 << group_button);
}
static void wacom_update_led(struct wacom *wacom, int button_count, int mask,
--
2.21.0
^ permalink raw reply related
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