* Re: [PATCH v4 01/11] can: Add LIN bus as CAN abstraction
From: Simon Horman @ 2024-05-13 9:52 UTC (permalink / raw)
To: Christoph Fritz
Cc: Ilpo Järvinen, Jiri Slaby, Greg Kroah-Hartman,
Marc Kleine-Budde, Oliver Hartkopp, Vincent Mailhol,
David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jiri Kosina,
Benjamin Tissoires, Sebastian Reichel, Linus Walleij,
Andreas Lauser, Jonathan Corbet, Pavel Pisa, linux-can, netdev,
devicetree, linux-input, linux-serial
In-Reply-To: <20240509171736.2048414-2-christoph.fritz@hexdev.de>
On Thu, May 09, 2024 at 07:17:26PM +0200, Christoph Fritz wrote:
> Introduce a LIN (local interconnect network) abstraction on top of CAN.
> This is a glue driver adapting CAN on one side while offering LIN
> abstraction on the other side. So that upcoming LIN device drivers can
> make use of it.
>
> Tested-by: Andreas Lauser <andreas.lauser@mercedes-benz.com>
> Signed-off-by: Christoph Fritz <christoph.fritz@hexdev.de>
...
> +#define LID(_name) \
> + struct device_attribute linid_##_name = __ATTR(_name, 0644, \
> + lin_identifier_show, lin_identifier_store)
> +
> +LID(00); LID(01); LID(02); LID(03); LID(04); LID(05); LID(06); LID(07);
> +LID(08); LID(09); LID(0a); LID(0b); LID(0c); LID(0d); LID(0e); LID(0f);
> +LID(10); LID(11); LID(12); LID(13); LID(14); LID(15); LID(16); LID(17);
> +LID(18); LID(19); LID(1a); LID(1b); LID(1c); LID(1d); LID(1e); LID(1f);
> +LID(20); LID(21); LID(22); LID(23); LID(24); LID(25); LID(26); LID(27);
> +LID(28); LID(29); LID(2a); LID(2b); LID(2c); LID(2d); LID(2e); LID(2f);
> +LID(30); LID(31); LID(32); LID(33); LID(34); LID(35); LID(36); LID(37);
> +LID(38); LID(39); LID(3a); LID(3b); LID(3c); LID(3d); LID(3e); LID(3f);
Hi Christoph,
Sparse flags that the structures defined by the above code are not
declared elsewhere, and therefore likely should be static.
> +
> +static struct attribute *lin_sysfs_attrs[] = {
> + &linid_00.attr, &linid_01.attr, &linid_02.attr, &linid_03.attr,
> + &linid_04.attr, &linid_05.attr, &linid_06.attr, &linid_07.attr,
> + &linid_08.attr, &linid_09.attr, &linid_0a.attr, &linid_0b.attr,
> + &linid_0c.attr, &linid_0d.attr, &linid_0e.attr, &linid_0f.attr,
> + &linid_10.attr, &linid_11.attr, &linid_12.attr, &linid_13.attr,
> + &linid_14.attr, &linid_15.attr, &linid_16.attr, &linid_17.attr,
> + &linid_18.attr, &linid_19.attr, &linid_1a.attr, &linid_1b.attr,
> + &linid_1c.attr, &linid_1d.attr, &linid_1e.attr, &linid_1f.attr,
> + &linid_20.attr, &linid_21.attr, &linid_22.attr, &linid_23.attr,
> + &linid_24.attr, &linid_25.attr, &linid_26.attr, &linid_27.attr,
> + &linid_28.attr, &linid_29.attr, &linid_2a.attr, &linid_2b.attr,
> + &linid_2c.attr, &linid_2d.attr, &linid_2e.attr, &linid_2f.attr,
> + &linid_30.attr, &linid_31.attr, &linid_32.attr, &linid_33.attr,
> + &linid_34.attr, &linid_35.attr, &linid_36.attr, &linid_37.attr,
> + &linid_38.attr, &linid_39.attr, &linid_3a.attr, &linid_3b.attr,
> + &linid_3c.attr, &linid_3d.attr, &linid_3e.attr, &linid_3f.attr,
> + NULL
> +};
...
^ permalink raw reply
* Re: [PATCH v4 02/11] HID: hexLIN: Add support for USB LIN adapter
From: Jiri Slaby @ 2024-05-13 5:26 UTC (permalink / raw)
To: Ilpo Järvinen, Christoph Fritz
Cc: Simon Horman, Greg Kroah-Hartman, Marc Kleine-Budde,
Oliver Hartkopp, Vincent Mailhol, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jiri Kosina, Benjamin Tissoires, Sebastian Reichel,
Linus Walleij, Andreas Lauser, Jonathan Corbet, Pavel Pisa,
linux-can, Netdev, devicetree, linux-input, linux-serial
In-Reply-To: <4bf1a5e9-904c-584e-72df-71abc3f99bd2@linux.intel.com>
On 10. 05. 24, 15:46, Ilpo Järvinen wrote:
>> + reinit_completion(&priv->wait_in_report);
>> +
>> + n = hid_hw_output_report(priv->hid_dev, (__u8 *) out_report, len);
>
> The usual is to not leave space between cast and what is being cast. I
> know hid functions seem to use __u8 but that's intended for uapi and in
> kernel, u8 should be used (somebody should eventually cleanup the hid
> function types too).
Apart from that, you are attached to USB, so this goes down to usbhid
(the ll driver). Are you sure the put-const-away cast is right thing to
do here? (usbhid passes it to usb_interrupt_msg().)
That is the only reason why you have the cast in there in the first place…
regards,
--
js
^ permalink raw reply
* Re: [PATCH] Input: chipone_icn8505 - remove an unused field in struct icn8505_data
From: Hans de Goede @ 2024-05-12 13:55 UTC (permalink / raw)
To: Christophe JAILLET, Dmitry Torokhov
Cc: linux-kernel, kernel-janitors, linux-input
In-Reply-To: <37443a675ca07c91c5f0118ce255406e6e3c08f5.1715502304.git.christophe.jaillet@wanadoo.fr>
Hi,
On 5/12/24 10:25 AM, Christophe JAILLET wrote:
> In "struct icn8505_data", the 'wake_gpio' field is unused.
> There is also nothing about gpio neither in this driver nor in the
> data-sheet.
>
> So, remove it.
>
> Found with cppcheck, unusedStructMember.
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Regards,
Hans
> ---
> Compile tested only.
>
> It was added in the initial commit e7330fa032bb ("Input: add support for
> ChipOne icn8505 based touchscreens") but was never used.
> ---
> drivers/input/touchscreen/chipone_icn8505.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/input/touchscreen/chipone_icn8505.c b/drivers/input/touchscreen/chipone_icn8505.c
> index b56954830b33..c1b4fc28fa8d 100644
> --- a/drivers/input/touchscreen/chipone_icn8505.c
> +++ b/drivers/input/touchscreen/chipone_icn8505.c
> @@ -68,7 +68,6 @@ struct icn8505_touch_data {
> struct icn8505_data {
> struct i2c_client *client;
> struct input_dev *input;
> - struct gpio_desc *wake_gpio;
> struct touchscreen_properties prop;
> char firmware_name[32];
> };
^ permalink raw reply
* Re: [PATCH v4 04/11] tty: serdev: Add method to enable break flags
From: Christoph Fritz @ 2024-05-12 13:08 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: Jiri Slaby, Simon Horman, Greg Kroah-Hartman, Marc Kleine-Budde,
Oliver Hartkopp, Vincent Mailhol, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jiri Kosina, Benjamin Tissoires, Sebastian Reichel,
Linus Walleij, Andreas Lauser, Jonathan Corbet, Pavel Pisa,
linux-can, Netdev, devicetree, linux-input, linux-serial
In-Reply-To: <36b0d460-1d96-89d8-db4a-76d735f7ee6b@linux.intel.com>
On Fri, 2024-05-10 at 17:21 +0300, Ilpo Järvinen wrote:
> On Thu, 9 May 2024, Christoph Fritz wrote:
...
> > diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> > index 613cb356b918d..23a1e76cb553b 100644
> > --- a/drivers/tty/serdev/core.c
> > +++ b/drivers/tty/serdev/core.c
> > @@ -339,6 +339,17 @@ unsigned int serdev_device_set_baudrate(struct serdev_device *serdev, unsigned i
> > }
> > EXPORT_SYMBOL_GPL(serdev_device_set_baudrate);
> >
> > +void serdev_device_set_break_detection(struct serdev_device *serdev, bool enable)
> > +{
> > + struct serdev_controller *ctrl = serdev->ctrl;
> > +
> > + if (!ctrl || !ctrl->ops->set_break_detection)
> > + return;
>
> Why you need to test for !ctrl?
In our case we don't, it's an extra check like all the other functions
here:
https://elixir.bootlin.com/linux/v6.9-rc7/source/drivers/tty/serdev/core.c#L330
>
> > + ctrl->ops->set_break_detection(ctrl, enable);
>
> I'd use positive logic here:
>
> if (ctrl->ops->set_break_detection)
> ctrl->ops->set_break_detection(ctrl, enable);
^ permalink raw reply
* Re: [PATCH v2 1/5] dt-bindings: input: touchscreen: himax,hx83112b: add HX83100A
From: Conor Dooley @ 2024-05-12 12:04 UTC (permalink / raw)
To: Felix Kaechele
Cc: Job Noorman, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-input, devicetree, linux-kernel
In-Reply-To: <dab1dddd-57ae-445e-bce7-879e8d73d809@kaechele.ca>
[-- Attachment #1: Type: text/plain, Size: 873 bytes --]
On Sat, May 11, 2024 at 10:10:08AM -0400, Felix Kaechele wrote:
> On 2024-05-11 08:38, Conor Dooley wrote:
> > On Sat, May 11, 2024 at 08:12:22AM -0400, Felix Kaechele wrote:
> > > Add a compatible string for the Himax HX83100A touch controller.
> > >
> > > Signed-off-by: Felix Kaechele <felix@kaechele.ca>
> >
> > Commit message should mention what makes this device incompatible with
> > the existing device.
>
> Thanks!
>
> I have added this note in the commit message which will be part of a v3,
> coming after I hopefully receive a few more comments on the other parts of
> this change:
>
> The HX83100A presents touch events on its internal bus rather than offering
> a dedicated event register like the other chips in this family do.
Ye, that sounds good. W/ that,
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Cheers,
Conor.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* [PATCH] Input: zet6223 - remove an unused field in struct zet6223_ts
From: Christophe JAILLET @ 2024-05-12 9:58 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-kernel, kernel-janitors, Christophe JAILLET, linux-input
In "struct zet6223_ts", the 'vcc' and 'vio' fields are unused.
So, remove them.
Found with cppcheck, unusedStructMember.
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Compile tested only.
It was added in the initial commit 83f66a6f08fa ("Input: add driver for
Zeitec ZET6223") but was never used.
---
drivers/input/touchscreen/zet6223.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/input/touchscreen/zet6223.c b/drivers/input/touchscreen/zet6223.c
index 1a034471f103..ec3fca4a48c0 100644
--- a/drivers/input/touchscreen/zet6223.c
+++ b/drivers/input/touchscreen/zet6223.c
@@ -25,8 +25,6 @@
struct zet6223_ts {
struct i2c_client *client;
struct input_dev *input;
- struct regulator *vcc;
- struct regulator *vio;
struct touchscreen_properties prop;
struct regulator_bulk_data supplies[2];
u16 max_x;
--
2.45.0
^ permalink raw reply related
* [PATCH] Input: chipone_icn8505 - remove an unused field in struct icn8505_data
From: Christophe JAILLET @ 2024-05-12 8:25 UTC (permalink / raw)
To: Hans de Goede, Dmitry Torokhov
Cc: linux-kernel, kernel-janitors, Christophe JAILLET, linux-input
In "struct icn8505_data", the 'wake_gpio' field is unused.
There is also nothing about gpio neither in this driver nor in the
data-sheet.
So, remove it.
Found with cppcheck, unusedStructMember.
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Compile tested only.
It was added in the initial commit e7330fa032bb ("Input: add support for
ChipOne icn8505 based touchscreens") but was never used.
---
drivers/input/touchscreen/chipone_icn8505.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/input/touchscreen/chipone_icn8505.c b/drivers/input/touchscreen/chipone_icn8505.c
index b56954830b33..c1b4fc28fa8d 100644
--- a/drivers/input/touchscreen/chipone_icn8505.c
+++ b/drivers/input/touchscreen/chipone_icn8505.c
@@ -68,7 +68,6 @@ struct icn8505_touch_data {
struct icn8505_data {
struct i2c_client *client;
struct input_dev *input;
- struct gpio_desc *wake_gpio;
struct touchscreen_properties prop;
char firmware_name[32];
};
--
2.45.0
^ permalink raw reply related
* Re: [PATCH v9 3/8] x86/vmware: Introduce VMware hypercall API
From: Simon Horman @ 2024-05-11 15:02 UTC (permalink / raw)
To: Alexey Makhalov
Cc: linux-kernel, virtualization, bp, hpa, dave.hansen, mingo, tglx,
x86, netdev, richardcochran, linux-input, dmitry.torokhov, zackr,
linux-graphics-maintainer, pv-drivers, timothym, akaher,
dri-devel, daniel, airlied, tzimmermann, mripard,
maarten.lankhorst, kirill.shutemov, Nadav Amit, Jeff Sipek
In-Reply-To: <20240506215305.30756-4-alexey.makhalov@broadcom.com>
On Mon, May 06, 2024 at 02:53:00PM -0700, Alexey Makhalov wrote:
> Introduce vmware_hypercall family of functions. It is a common
> implementation to be used by the VMware guest code and virtual
> device drivers in architecture independent manner.
>
> The API consists of vmware_hypercallX and vmware_hypercall_hb_{out,in}
> set of functions by analogy with KVM hypercall API. Architecture
> specific implementation is hidden inside.
>
> It will simplify future enhancements in VMware hypercalls such
> as SEV-ES and TDX related changes without needs to modify a
> caller in device drivers code.
>
> Current implementation extends an idea from commit bac7b4e84323
> ("x86/vmware: Update platform detection code for VMCALL/VMMCALL
> hypercalls") to have a slow, but safe path in VMWARE_HYPERCALL
> earlier during the boot when alternatives are not yet applied.
> This logic was inherited from VMWARE_CMD from the commit mentioned
> above. Default alternative code was optimized by size to reduce
> excessive nop alignment once alternatives are applied. Total
> default code size is 26 bytes, in worse case (3 bytes alternative)
> remaining 23 bytes will be aligned by only 3 long NOP instructions.
>
> Signed-off-by: Alexey Makhalov <alexey.makhalov@broadcom.com>
> Reviewed-by: Nadav Amit <nadav.amit@gmail.com>
> Reviewed-by: Jeff Sipek <jsipek@vmware.com>
...
> diff --git a/arch/x86/include/asm/vmware.h b/arch/x86/include/asm/vmware.h
...
> +static inline
> +unsigned long vmware_hypercall3(unsigned long cmd, unsigned long in1,
> + uint32_t *out1, uint32_t *out2)
nit: u32 is preferred over uint32_t.
Likewise elsewhere in this patch-set.
...
> /*
> - * The high bandwidth in call. The low word of edx is presumed to have the
> - * HB bit set.
> + * High bandwidth calls are not supported on encrypted memory guests.
> + * The caller should check cc_platform_has(CC_ATTR_MEM_ENCRYPT) and use
> + * low bandwidth hypercall it memory encryption is set.
> + * This assumption simplifies HB hypercall impementation to just I/O port
nit: implementation
checkpatch.pl --codespell is your friend
> + * based approach without alternative patching.
> */
...
^ permalink raw reply
* Re: [PATCH v2 1/5] dt-bindings: input: touchscreen: himax,hx83112b: add HX83100A
From: Felix Kaechele @ 2024-05-11 14:10 UTC (permalink / raw)
To: Conor Dooley
Cc: Job Noorman, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-input, devicetree, linux-kernel
In-Reply-To: <20240511-passage-obstruct-70e4c3b37dbd@spud>
On 2024-05-11 08:38, Conor Dooley wrote:
> On Sat, May 11, 2024 at 08:12:22AM -0400, Felix Kaechele wrote:
>> Add a compatible string for the Himax HX83100A touch controller.
>>
>> Signed-off-by: Felix Kaechele <felix@kaechele.ca>
>
> Commit message should mention what makes this device incompatible with
> the existing device.
Thanks!
I have added this note in the commit message which will be part of a v3,
coming after I hopefully receive a few more comments on the other parts
of this change:
The HX83100A presents touch events on its internal bus rather than
offering a dedicated event register like the other chips in this family do.
Felix
^ permalink raw reply
* Re: [PATCH v2 1/5] dt-bindings: input: touchscreen: himax,hx83112b: add HX83100A
From: Conor Dooley @ 2024-05-11 12:38 UTC (permalink / raw)
To: Felix Kaechele
Cc: Job Noorman, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-input, devicetree, linux-kernel
In-Reply-To: <20240511121245.109644-2-felix@kaechele.ca>
[-- Attachment #1: Type: text/plain, Size: 962 bytes --]
On Sat, May 11, 2024 at 08:12:22AM -0400, Felix Kaechele wrote:
> Add a compatible string for the Himax HX83100A touch controller.
>
> Signed-off-by: Felix Kaechele <felix@kaechele.ca>
Commit message should mention what makes this device incompatible with
the existing device.
Thanks,
Conor.
> ---
> .../devicetree/bindings/input/touchscreen/himax,hx83112b.yaml | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml b/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml
> index f42b23d532eb..f5cfacb5e966 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml
> +++ b/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml
> @@ -15,6 +15,7 @@ allOf:
> properties:
> compatible:
> enum:
> + - himax,hx83100a
> - himax,hx83112b
>
> reg:
> --
> 2.45.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* [PATCH v2 5/5] input: himax_hx83112b: add support for HX83100A
From: Felix Kaechele @ 2024-05-11 12:12 UTC (permalink / raw)
To: Job Noorman, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-input, devicetree, linux-kernel
Cc: Paul Gale
In-Reply-To: <20240511121245.109644-1-felix@kaechele.ca>
The HX83100A is a bit of an outlier in the Himax HX831xxx series of
touch controllers as it requires reading touch events through the AHB
interface of the MCU rather than providing a dedicated FIFO address like
the other chips do.
This patch implements the specific read function and introduces the
HX83100A chip with an appropriate i2c ID and DT compatible string.
The HX83100A doesn't have a straightforward way to do chip
identification, which is why it is not implemented in this patch.
Tested on: Lenovo ThinkSmart View (CD-18781Y) / Innolux P080DDD-AB2 LCM
Signed-off-by: Felix Kaechele <felix@kaechele.ca>
Tested-by: Paul Gale <paul@siliconpixel.com>
---
drivers/input/touchscreen/himax_hx83112b.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/input/touchscreen/himax_hx83112b.c b/drivers/input/touchscreen/himax_hx83112b.c
index 5092a357c332..9ed3bccde4ac 100644
--- a/drivers/input/touchscreen/himax_hx83112b.c
+++ b/drivers/input/touchscreen/himax_hx83112b.c
@@ -4,6 +4,9 @@
*
* Copyright (C) 2022 Job Noorman <job@noorman.info>
*
+ * HX83100A support
+ * Copyright (C) 2024 Felix Kaechele <felix@kaechele.ca>
+ *
* This code is based on "Himax Android Driver Sample Code for QCT platform":
*
* Copyright (C) 2017 Himax Corporation.
@@ -35,6 +38,8 @@
#define HIMAX_REG_ADDR_ICID 0x900000d0
+#define HX83100A_REG_FW_EVENT_STACK 0x90060000
+
#define HIMAX_INVALID_COORD 0xffff
struct himax_event_point {
@@ -288,6 +293,12 @@ static int himax_read_events(struct himax_ts_data *ts,
length);
}
+static int hx83100a_read_events(struct himax_ts_data *ts,
+ struct himax_event *event, size_t length)
+{
+ return himax_bus_read(ts, HX83100A_REG_FW_EVENT_STACK, event, length);
+};
+
static int himax_handle_input(struct himax_ts_data *ts)
{
int error;
@@ -394,6 +405,10 @@ static int himax_resume(struct device *dev)
static DEFINE_SIMPLE_DEV_PM_OPS(himax_pm_ops, himax_suspend, himax_resume);
+static const struct himax_chip hx83100a_chip = {
+ .read_events = hx83100a_read_events,
+};
+
static const struct himax_chip hx83112b_chip = {
.id = 0x83112b,
.check_id = himax_check_product_id,
@@ -401,6 +416,7 @@ static const struct himax_chip hx83112b_chip = {
};
static const struct i2c_device_id himax_ts_id[] = {
+ { "hx83100a", (kernel_ulong_t)&hx83100a_chip },
{ "hx83112b", (kernel_ulong_t)&hx83112b_chip },
{ /* sentinel */ }
};
@@ -408,6 +424,7 @@ MODULE_DEVICE_TABLE(i2c, himax_ts_id);
#ifdef CONFIG_OF
static const struct of_device_id himax_of_match[] = {
+ { .compatible = "himax,hx83100a", .data = &hx83100a_chip },
{ .compatible = "himax,hx83112b", .data = &hx83112b_chip },
{ /* sentinel */ }
};
--
2.45.0
^ permalink raw reply related
* [PATCH v2 4/5] input: himax_hx83112b: add himax_chip struct for multi-chip support
From: Felix Kaechele @ 2024-05-11 12:12 UTC (permalink / raw)
To: Job Noorman, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-input, devicetree, linux-kernel
In-Reply-To: <20240511121245.109644-1-felix@kaechele.ca>
In preparation for HX83100A support allow defining separate functions
for specific chip operations.
Signed-off-by: Felix Kaechele <felix@kaechele.ca>
---
drivers/input/touchscreen/himax_hx83112b.c | 51 +++++++++++++++-------
1 file changed, 35 insertions(+), 16 deletions(-)
diff --git a/drivers/input/touchscreen/himax_hx83112b.c b/drivers/input/touchscreen/himax_hx83112b.c
index 67ef3255cc8b..5092a357c332 100644
--- a/drivers/input/touchscreen/himax_hx83112b.c
+++ b/drivers/input/touchscreen/himax_hx83112b.c
@@ -20,8 +20,6 @@
#include <linux/kernel.h>
#include <linux/regmap.h>
-#define HIMAX_ID_83112B 0x83112b
-
#define HIMAX_MAX_POINTS 10
#define HIMAX_AHB_ADDR_BYTE_0 0x00
@@ -55,7 +53,16 @@ struct himax_event {
static_assert(sizeof(struct himax_event) == 56);
+struct himax_ts_data;
+struct himax_chip {
+ u32 id;
+ int (*check_id)(struct himax_ts_data *ts);
+ int (*read_events)(struct himax_ts_data *ts, struct himax_event *event,
+ size_t length);
+};
+
struct himax_ts_data {
+ const struct himax_chip *chip;
struct gpio_desc *gpiod_rst;
struct input_dev *input_dev;
struct i2c_client *client;
@@ -167,15 +174,12 @@ static int himax_check_product_id(struct himax_ts_data *ts)
dev_dbg(&ts->client->dev, "Product id: %x\n", product_id);
- switch (product_id) {
- case HIMAX_ID_83112B:
+ if (product_id == ts->chip->id)
return 0;
- default:
- dev_err(&ts->client->dev,
- "Unknown product id: %x\n", product_id);
- return -EINVAL;
- }
+ dev_err(&ts->client->dev, "Unknown product id: %x\n",
+ product_id);
+ return -EINVAL;
}
static int himax_input_register(struct himax_ts_data *ts)
@@ -277,13 +281,19 @@ static bool himax_verify_checksum(struct himax_ts_data *ts,
return true;
}
+static int himax_read_events(struct himax_ts_data *ts,
+ struct himax_event *event, size_t length)
+{
+ return regmap_raw_read(ts->regmap, HIMAX_AHB_ADDR_EVENT_STACK, event,
+ length);
+}
+
static int himax_handle_input(struct himax_ts_data *ts)
{
int error;
struct himax_event event;
- error = regmap_raw_read(ts->regmap, HIMAX_AHB_ADDR_EVENT_STACK, &event,
- sizeof(event));
+ error = ts->chip->read_events(ts, &event, sizeof(event));
if (error) {
dev_err(&ts->client->dev, "Failed to read input event: %d\n",
error);
@@ -329,6 +339,7 @@ static int himax_probe(struct i2c_client *client)
i2c_set_clientdata(client, ts);
ts->client = client;
+ ts->chip = i2c_get_match_data(client);
ts->regmap = devm_regmap_init_i2c(client, &himax_regmap_config);
error = PTR_ERR_OR_ZERO(ts->regmap);
@@ -346,9 +357,11 @@ static int himax_probe(struct i2c_client *client)
himax_reset(ts);
- error = himax_check_product_id(ts);
- if (error)
- return error;
+ if (ts->chip->check_id) {
+ error = himax_check_product_id(ts);
+ if (error)
+ return error;
+ }
error = himax_input_register(ts);
if (error)
@@ -381,15 +394,21 @@ static int himax_resume(struct device *dev)
static DEFINE_SIMPLE_DEV_PM_OPS(himax_pm_ops, himax_suspend, himax_resume);
+static const struct himax_chip hx83112b_chip = {
+ .id = 0x83112b,
+ .check_id = himax_check_product_id,
+ .read_events = himax_read_events,
+};
+
static const struct i2c_device_id himax_ts_id[] = {
- { "hx83112b", 0 },
+ { "hx83112b", (kernel_ulong_t)&hx83112b_chip },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(i2c, himax_ts_id);
#ifdef CONFIG_OF
static const struct of_device_id himax_of_match[] = {
- { .compatible = "himax,hx83112b" },
+ { .compatible = "himax,hx83112b", .data = &hx83112b_chip },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, himax_of_match);
--
2.45.0
^ permalink raw reply related
* [PATCH v2 3/5] input: himax_hx83112b: implement MCU register reading
From: Felix Kaechele @ 2024-05-11 12:12 UTC (permalink / raw)
To: Job Noorman, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-input, devicetree, linux-kernel
In-Reply-To: <20240511121245.109644-1-felix@kaechele.ca>
Implement reading from the MCU in a more universal fashion. This allows
properly handling reads of more than 4 bytes using the AHB FIFO
implemented in the chip.
Signed-off-by: Felix Kaechele <felix@kaechele.ca>
---
drivers/input/touchscreen/himax_hx83112b.c | 50 ++++++++++++++++++++--
1 file changed, 47 insertions(+), 3 deletions(-)
diff --git a/drivers/input/touchscreen/himax_hx83112b.c b/drivers/input/touchscreen/himax_hx83112b.c
index 2da2920d43f9..67ef3255cc8b 100644
--- a/drivers/input/touchscreen/himax_hx83112b.c
+++ b/drivers/input/touchscreen/himax_hx83112b.c
@@ -27,9 +27,13 @@
#define HIMAX_AHB_ADDR_BYTE_0 0x00
#define HIMAX_AHB_ADDR_RDATA_BYTE_0 0x08
#define HIMAX_AHB_ADDR_ACCESS_DIRECTION 0x0c
+#define HIMAX_AHB_ADDR_INCR4 0x0d
+#define HIMAX_AHB_ADDR_CONTI 0x13
#define HIMAX_AHB_ADDR_EVENT_STACK 0x30
#define HIMAX_AHB_CMD_ACCESS_DIRECTION_READ 0x00
+#define HIMAX_AHB_CMD_INCR4 0x10
+#define HIMAX_AHB_CMD_CONTI 0x31
#define HIMAX_REG_ADDR_ICID 0x900000d0
@@ -65,10 +69,34 @@ static const struct regmap_config himax_regmap_config = {
.val_format_endian = REGMAP_ENDIAN_LITTLE,
};
-static int himax_read_config(struct himax_ts_data *ts, u32 address, u32 *dst)
+static int himax_bus_enable_burst(struct himax_ts_data *ts)
{
int error;
+ error = regmap_write(ts->regmap, HIMAX_AHB_ADDR_CONTI,
+ HIMAX_AHB_CMD_CONTI);
+ if (error)
+ return error;
+
+ error = regmap_write(ts->regmap, HIMAX_AHB_ADDR_INCR4,
+ HIMAX_AHB_CMD_INCR4);
+ if (error)
+ return error;
+
+ return 0;
+}
+
+static int himax_bus_read(struct himax_ts_data *ts, u32 address, void *dst,
+ size_t length)
+{
+ int error;
+
+ if (length > 4) {
+ error = himax_bus_enable_burst(ts);
+ if (error)
+ return error;
+ }
+
error = regmap_write(ts->regmap, HIMAX_AHB_ADDR_BYTE_0, address);
if (error)
return error;
@@ -78,7 +106,23 @@ static int himax_read_config(struct himax_ts_data *ts, u32 address, u32 *dst)
if (error)
return error;
- error = regmap_read(ts->regmap, HIMAX_AHB_ADDR_RDATA_BYTE_0, dst);
+ if (length > 4)
+ error = regmap_noinc_read(ts->regmap, HIMAX_AHB_ADDR_RDATA_BYTE_0,
+ dst, length);
+ else
+ error = regmap_read(ts->regmap, HIMAX_AHB_ADDR_RDATA_BYTE_0,
+ dst);
+ if (error)
+ return error;
+
+ return 0;
+}
+
+static int himax_read_mcu(struct himax_ts_data *ts, u32 address, u32 *dst)
+{
+ int error;
+
+ error = himax_bus_read(ts, address, dst, sizeof(dst));
if (error)
return error;
@@ -104,7 +148,7 @@ static int himax_read_product_id(struct himax_ts_data *ts, u32 *product_id)
{
int error;
- error = himax_read_config(ts, HIMAX_REG_ADDR_ICID, product_id);
+ error = himax_read_mcu(ts, HIMAX_REG_ADDR_ICID, product_id);
if (error)
return error;
--
2.45.0
^ permalink raw reply related
* [PATCH v2 2/5] input: himax_hx83112b: use more descriptive register defines
From: Felix Kaechele @ 2024-05-11 12:12 UTC (permalink / raw)
To: Job Noorman, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-input, devicetree, linux-kernel
In-Reply-To: <20240511121245.109644-1-felix@kaechele.ca>
Himax uses an AHB-style bus to communicate with different parts of the
display driver and touch controller system.
Use more descriptive names for the register and address defines.
The names were taken from a driver submission for the similar HX83102J
chip.
Signed-off-by: Felix Kaechele <felix@kaechele.ca>
Link: https://lore.kernel.org/all/TY0PR06MB561105A3386E9D76F429110D9E0F2@TY0PR06MB5611.apcprd06.prod.outlook.com/
---
drivers/input/touchscreen/himax_hx83112b.c | 23 ++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/drivers/input/touchscreen/himax_hx83112b.c b/drivers/input/touchscreen/himax_hx83112b.c
index 4f6609dcdef3..2da2920d43f9 100644
--- a/drivers/input/touchscreen/himax_hx83112b.c
+++ b/drivers/input/touchscreen/himax_hx83112b.c
@@ -24,12 +24,14 @@
#define HIMAX_MAX_POINTS 10
-#define HIMAX_REG_CFG_SET_ADDR 0x00
-#define HIMAX_REG_CFG_INIT_READ 0x0c
-#define HIMAX_REG_CFG_READ_VALUE 0x08
-#define HIMAX_REG_READ_EVENT 0x30
+#define HIMAX_AHB_ADDR_BYTE_0 0x00
+#define HIMAX_AHB_ADDR_RDATA_BYTE_0 0x08
+#define HIMAX_AHB_ADDR_ACCESS_DIRECTION 0x0c
+#define HIMAX_AHB_ADDR_EVENT_STACK 0x30
-#define HIMAX_CFG_PRODUCT_ID 0x900000d0
+#define HIMAX_AHB_CMD_ACCESS_DIRECTION_READ 0x00
+
+#define HIMAX_REG_ADDR_ICID 0x900000d0
#define HIMAX_INVALID_COORD 0xffff
@@ -67,15 +69,16 @@ static int himax_read_config(struct himax_ts_data *ts, u32 address, u32 *dst)
{
int error;
- error = regmap_write(ts->regmap, HIMAX_REG_CFG_SET_ADDR, address);
+ error = regmap_write(ts->regmap, HIMAX_AHB_ADDR_BYTE_0, address);
if (error)
return error;
- error = regmap_write(ts->regmap, HIMAX_REG_CFG_INIT_READ, 0x0);
+ error = regmap_write(ts->regmap, HIMAX_AHB_ADDR_ACCESS_DIRECTION,
+ HIMAX_AHB_CMD_ACCESS_DIRECTION_READ);
if (error)
return error;
- error = regmap_read(ts->regmap, HIMAX_REG_CFG_READ_VALUE, dst);
+ error = regmap_read(ts->regmap, HIMAX_AHB_ADDR_RDATA_BYTE_0, dst);
if (error)
return error;
@@ -101,7 +104,7 @@ static int himax_read_product_id(struct himax_ts_data *ts, u32 *product_id)
{
int error;
- error = himax_read_config(ts, HIMAX_CFG_PRODUCT_ID, product_id);
+ error = himax_read_config(ts, HIMAX_REG_ADDR_ICID, product_id);
if (error)
return error;
@@ -235,7 +238,7 @@ static int himax_handle_input(struct himax_ts_data *ts)
int error;
struct himax_event event;
- error = regmap_raw_read(ts->regmap, HIMAX_REG_READ_EVENT, &event,
+ error = regmap_raw_read(ts->regmap, HIMAX_AHB_ADDR_EVENT_STACK, &event,
sizeof(event));
if (error) {
dev_err(&ts->client->dev, "Failed to read input event: %d\n",
--
2.45.0
^ permalink raw reply related
* [PATCH v2 1/5] dt-bindings: input: touchscreen: himax,hx83112b: add HX83100A
From: Felix Kaechele @ 2024-05-11 12:12 UTC (permalink / raw)
To: Job Noorman, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-input, devicetree, linux-kernel
In-Reply-To: <20240511121245.109644-1-felix@kaechele.ca>
Add a compatible string for the Himax HX83100A touch controller.
Signed-off-by: Felix Kaechele <felix@kaechele.ca>
---
.../devicetree/bindings/input/touchscreen/himax,hx83112b.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml b/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml
index f42b23d532eb..f5cfacb5e966 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml
@@ -15,6 +15,7 @@ allOf:
properties:
compatible:
enum:
+ - himax,hx83100a
- himax,hx83112b
reg:
--
2.45.0
^ permalink raw reply related
* [PATCH v2 0/5] input: himax_hx83112b: add support for HX83100A
From: Felix Kaechele @ 2024-05-11 12:12 UTC (permalink / raw)
To: Job Noorman, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-input, devicetree, linux-kernel
Resent, due to being caught in the spam filter.
This set of patches brings support for the Himax HX83100A touch
controller.
I have no access to datasheets. So, like the original driver code
that's being extended here, this code is mostly based on the quite
convoluted, GPLv2 licensed manufacturer drivers for Android.
I included links to sources and references where appropriate.
A number of people tested this patch set on Lenovo ThinkSmart View
(CD-18781Y) devices. That device has a variant utilizing a Innolux
P080DDD-AB2 LCM. This LCM comes with the HX83100A.
I would really appreciate if people using HX83112B chips could give this
set a run to ensure nothing broke.
Thanks,
Felix
Changes in v2:
- removed regulator handling, my test device works fine without it
- some minor formatting fixes
Felix Kaechele (5):
dt-bindings: input: touchscreen: himax,hx83112b: add HX83100A
input: himax_hx83112b: use more descriptive register defines
input: himax_hx83112b: implement MCU register reading
input: himax_hx83112b: add himax_chip struct for multi-chip support
input: himax_hx83112b: add support for HX83100A
.../input/touchscreen/himax,hx83112b.yaml | 1 +
drivers/input/touchscreen/himax_hx83112b.c | 135 ++++++++++++++----
2 files changed, 110 insertions(+), 26 deletions(-)
base-commit: 5128de84d8fc849400d00f7a6982711f129699ea
--
2.45.0
^ permalink raw reply
* Re: kernel panic when build w/ CONFIG_HID_BPF=y
From: Zhixu Liu @ 2024-05-11 10:47 UTC (permalink / raw)
To: rcu; +Cc: linux-input, linux-kernel
In-Reply-To: <CALMA0xaTSMN+p4xUXkzrtR5r6k7hgoswcaXx7baR_z9r5jjskw@mail.gmail.com>
Hi,
As described in the last email, I thought the kernel panic was related
to CONFIG_HID_BPF, but it seems not for today's panic.
Following panic log is found on a macmini7.1 w/ CONFIG_HID_BPF disabled,
the panic log is highly similar w/ the log with CONFIG_HID_BPF enabled.
It seems related to RCU?
[ T15] BUG: unable to handle page fault for address: ffffffffb411c8c8
[ T15] #PF: supervisor read access in kernel mode
[ T15] #PF: error_code(0x0000) - not-present page
[ T15] PGD 165a24067 P4D 165a24067 PUD 165a25063 PMD 166bff063 PTE
800ffffe99ee3062
[ T15] Oops: 0000 [#1] PREEMPT SMP PTI
[ T45] usb usb2: New USB device found, idVendor=1d6b,
idProduct=0002, bcdDevice= 6.06
[ T15] CPU: 0 PID: 15 Comm: rcu_tasks_trace Tainted: G W
6.6.30-gentoo-ks #2 a5ce907d07a3642cbd0dd494822fdaf3f1e8cc1f
[ T15] Hardware name: Apple Inc. Macmini7,1/Mac-35C5E08120C7EEAF,
BIOS MM71.882.0220.B07.1602221546 02/22/2016
[ T15] RIP: 0010:rcu_tasks_need_gpcb+0x25d/0x2c0
[ T15] Code: 30 ff ff ff 49 8b 8f b0 00 00 00 48 c7 c0 a8 8c 92 b3
eb 10 48 83 c0 08 48 3d a0 9c 92 b3 0f 84 5a ff ff ff 48 8b 10 48 01
ca <48> 8b 52 48 48 85 d2 74 e1 0f 0b eb dd 49 8b b7 20 01 00 00 48 c7
[ T15] RSP: 0018:ffffafb5400afe60 EFLAGS: 00010082
[ T15] RAX: ffffffffb3928cc0 RBX: ffffffffb3b3b382 RCX: 0000000000021880
[ T15] RDX: ffffffffb411c880 RSI: 0000000000000000 RDI: 0000000000000000
[ T15] RBP: 0000000000000202 R08: 0000000000000000 R09: 0000000000000000
[ T15] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000003
[ T15] R13: 0000000000000000 R14: 0000000000000001 R15: ffffffffb3b3b320
[ T15] FS: 0000000000000000(0000) GS:ffff8c3ca7200000(0000)
knlGS:0000000000000000
[ T15] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ T15] CR2: ffffffffb411c8c8 CR3: 00000001000c0001 CR4: 00000000001706f0
[ T15] Call Trace:
[ T15] <TASK>
[ T15] ? __die+0x23/0x80
[ T15] ? page_fault_oops+0xa4/0x180
[ T15] ? exc_page_fault+0x152/0x180
[ T15] ? asm_exc_page_fault+0x26/0x40
[ T15] ? rcu_tasks_need_gpcb+0x25d/0x2c0
[ T15] ? __pfx_rcu_tasks_kthread+0x40/0x40
[ T15] rcu_tasks_one_gp+0x69/0x180
[ T15] rcu_tasks_kthread+0x94/0xc0
[ T15] kthread+0xe8/0x140
[ T15] ? __pfx_kthread+0x40/0x40
[ T15] ret_from_fork+0x34/0x80
[ T15] ? __pfx_kthread+0x40/0x40
[ T15] ret_from_fork_asm+0x1b/0x80
[ T15] </TASK>
[ T15] Modules linked in: drm(+) loop fuse
drm_panel_orientation_quirks backlight nfnetlink efivarfs dmi_sysfs
ext4 crc32c_generic_crc16 mbcache jbd2 simplefb cfbfillrect cfbimgblt
cfbcopyarea fb fbdev hid_generic usbhid hid dm_mod sd_mod t10_pi
crc64_rocksoft crc64 crc_t10dif crct10dif_common ahci xhci_pci
xhci_pci_renesas libahci xhci_hcd libata usbcore scsi_mod crc32c_intel
usb_common scsi_common
[ T15] CR2: ffffffffb411c8c8
[ T15] ---[ end trace 0000000000000000 ]---
[ T15] RIP: 0010:rcu_tasks_need_gpcb+0x25d/0x2c0
[ T15] Code: 30 ff ff ff 49 8b 8f b0 00 00 00 48 c7 c0 a8 8c 92 b3
eb 10 48 83 c0 08 48 3d a0 9c 92 b3 0f 84 5a ff ff ff 48 8b 10 48 01
ca <48> 8b 52 48 48 85 d2 74 e1 0f 0b eb dd 49 8b b7 20 01 00 00 48 c7
[ T15] RSP: 0018:ffffafb5400afe60 EFLAGS: 00010082
[ T15] RAX: ffffffffb3928cc0 RBX: ffffffffb3b3b328 RCX: 0000000000021880
[ T15] RDX: ffffffffb411c880 RSI: 0000000000000000 RDI: 0000000000000000
[ T15] RBP: 0000000000000202 R08: 0000000000000000 R09: 0000000000000000
[ T15] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000003
[ T15] R13: 0000000000000000 R14: 0000000000000001 R15: ffffffffb3b3b320
[ T15] FS: 0000000000000000(0000) GS:ffff8c3ca7200000(0000)
knlGS:0000000000000000
[ T15] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ T15] CR2: ffffffffb411c8c8 CR3: 00000001000c0001 CR4: 00000000001706f0
[ T15] Kernel panic - not syncing: Fatal exception
[ T15] Kernel Offset: 0x31000000 from 0xffffffff81000000 (relocation
range: 0xffffffff80000000-0xffffffffbfffffff)
[ T15] ---[ end Kernel panic - not syncing: Fatal exception ]
Zhixu Liu <zhixu.liu@gmail.com> 于2024年4月30日周二 19:51写道：
>
> Hi,
>
> For config.6.6.21 attached, the kernel panic when build w/
> CONFIG_HID_BPF=y, and boot success after disable this option.
>
> In config.6.6.21, I enabled almost all modules, it take about 40
> minute to compile. The panic log shows nothing w/ hid-bpf directly,
> CONFIG_HID_BPF was found after days of compile and test. I'm still
> trying to show this panic w/ a much smaller config, but no success
> yet. :-(
>
> [ 3.697008][ T15] #PF: supervisor read access in kernel mode
> [ 3.697009][ T15] #PF: error_code(0x0000) - not-present page
> [ 3.706233][ T45] ehci-pci 0000:00:1a.0: irq 16, io mem 0xf7e3c000
> [ 3.708152][ T15] PGD 40fa24067 P4D 40fa24067 PUD 40fa25063 PMD 410bff063
> [ 3.720380][ T45] ehci-pci 0000:00:1a.0: USB 2.0 started, EHCI 1.00
> [ 3.720430][ T15] PTE 800ffffbefee3062
> [ 3.720431][ T15] Oops: 0000 [#1] PREEMPT SMP PTI
> [ 3.727873][ T45] usb usb2: New USB device found, idVendor=1d6b,
> idProduct=0002, bcdDevice= 6.06
> [ 3.734009][ T15] CPU: 0 PID: 15 Comm: rcu_tasks_trace Tainted: G
> W 6.6.21-ks #1
> 5dc7acf91a5e8e9ac9dcfc35bee0245691283ea6
> [ 3.734011][ T15] Hardware name: Dell Inc. OptiPlex 9020/005T15,
> BIOS A14 09/14/2015
> [ 3.734012][ T15] RIP: 0010:rcu_tasks_need_gpcb+0x25d/0x2c0
> [ 3.737962][ T45] usb usb2: New USB device strings: Mfr=3,
> Product=2, SerialNumber=1
> [ 3.742876][ T15] Code: 30 ff ff ff 49 8b 8f b0 00 00 00 48 c7 c0
> 88 9c 92 98 eb 10 48 83 c0 08 48 3d 80 ac 92 98 0f 84 5a ff ff ff 48
> 8b 10 48 01 ca <48> 8b 52 48 48 85 d2 74 e1 0f 0b eb dd 49 8b b7 20 01
> 00 00 48 c7
> [ 3.742877][ T15] RSP: 0018:ffffa371c00a3e60 EFLAGS: 00010082
> [ 3.751891][ T45] usb usb2: Product: EHCI Host Controller
> [ 3.764495][ T15]
> [ 3.764496][ T15] RAX: ffffffff98929ca0 RBX: ffffffff98b3b328
> RCX: 0000000000021880
> [ 3.764497][ T15] RDX: ffffffff9911c880 RSI: 0000000000000000
> RDI: 0000000000000000
> [ 3.772461][ T45] usb usb2: Manufacturer: Linux 6.6.21-ks ehci_hcd
> [ 3.778248][ T15] RBP: 0000000000000202 R08: 0000000000000000
> R09: 0000000000000000
> [ 3.778249][ T15] R10: 0000000000000000 R11: 0000000000000000
> R12: 0000000000000003
> [ 3.778249][ T15] R13: 0000000000000000 R14: 0000000000000001
> R15: ffffffff98b3b320
> [ 3.786216][ T45] usb usb2: SerialNumber: 0000:00:1a.0
> [ 3.805811][ T15] FS: 0000000000000000(0000)
> GS:ffff8c781ea00000(0000) knlGS:0000000000000000
> [ 3.805813][ T15] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 3.811993][ T45] hub 2-0:1.0: USB hub found
> [ 3.817383][ T15] CR2: ffffffff9911c8c8 CR3: 000000040fa20005
> CR4: 00000000001706f0
> [ 3.817385][ T15] Call Trace:
> [ 3.817386][ T15] <TASK>
> [ 3.817388][ T15] ? __die+0x23/0x80
> [ 3.819643][ T45] hub 2-0:1.0: 2 ports detected
> [ 3.827481][ T15] ? page_fault_oops+0xa4/0x180
> [ 3.827485][ T15] ? exc_page_fault+0x152/0x180
> [ 3.922376][ T15] ? asm_exc_page_fault+0x26/0x40
> [ 3.927289][ T15] ? rcu_tasks_need_gpcb+0x25d/0x2c0
> [ 3.932459][ T15] ? __pfx_rcu_tasks_kthread+0x40/0x40
> [ 3.937806][ T15] rcu_tasks_one_gp+0x69/0x180
> [ 3.942451][ T15] rcu_tasks_kthread+0x94/0xc0
> [ 3.947096][ T15] kthread+0xe8/0x140
> [ 3.950956][ T15] ? __pfx_kthread+0x40/0x40
> [ 3.955425][ T15] ret_from_fork+0x34/0x80
> [ 3.959721][ T15] ? __pfx_kthread+0x40/0x40
> [ 3.964192][ T15] ret_from_fork_asm+0x1b/0x80
> [ 3.968841][ T15] </TASK>
> [ 3.971735][ T15] Modules linked in: libahci ehci_pci xhci_hcd
> ehci_hcd libata crc32c_intel scsi_mod usbcore scsi_common usb_common
> [ 3.983818][ T15] CR2: ffffffff9911c8c8
> [ 3.987853][ T15] ---[ end trace 0000000000000000 ]---
> [ 3.993195][ T15] RIP: 0010:rcu_tasks_need_gpcb+0x25d/0x2c0
> [ 3.998977][ T15] Code: 30 ff ff ff 49 8b 8f b0 00 00 00 48 c7 c0
> 88 9c 92 98 eb 10 48 83 c0 08 48 3d 80 ac 92 98 0f 84 5a ff ff ff 48
> 8b 10 48 01 ca <48> 8b 52 48 48 85 d2 74 e1 0f 0b eb dd 49 8b b7 20 01
> 00 00 48 c7
> [ 4.018567][ T15] RSP: 0018:ffffa371c00a3e60 EFLAGS: 00010082
> [ 4.024521][ T15] RAX: ffffffff98929ca0 RBX: ffffffff98b3b328
> RCX: 0000000000021880
> [ 4.032411][ T15] RDX: ffffffff9911c880 RSI: 0000000000000000
> RDI: 0000000000000000
> [ 4.040288][ T15] RBP: 0000000000000202 R08: 0000000000000000
> R09: 0000000000000000
> [ 4.048167][ T15] R10: 0000000000000000 R11: 0000000000000000
> R12: 0000000000000003
> [ 4.056043][ T15] R13: 0000000000000000 R14: 0000000000000001
> R15: ffffffff98b3b320
> [ 4.063920][ T15] FS: 0000000000000000(0000)
> GS:ffff8c781ea00000(0000) knlGS:0000000000000000
> [ 4.072761][ T15] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 4.079242][ T15] CR2: ffffffff9911c8c8 CR3: 000000040fa20005
> CR4: 00000000001706f0
> [ 4.087119][ T15] Kernel panic - not syncing: Fatal exception
> [ 4.093097][ T15] Kernel Offset: 0x16000000 from
> 0xffffffff81000000 (relocation range:
> 0xffffffff80000000-0xffffffffbfffffff)
> [ 4.104648][ T15] ---[ end Kernel panic - not syncing: Fatal exception ]---
>
> --
> Z. Liu
--
Z. Liu
--
Z. Liu
^ permalink raw reply
* Re: [PATCH v4 02/11] HID: hexLIN: Add support for USB LIN adapter
From: Christoph Fritz @ 2024-05-11 8:14 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: Jiri Slaby, Simon Horman, Greg Kroah-Hartman, Marc Kleine-Budde,
Oliver Hartkopp, Vincent Mailhol, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jiri Kosina, Benjamin Tissoires, Sebastian Reichel,
Linus Walleij, Andreas Lauser, Jonathan Corbet, Pavel Pisa,
linux-can, Netdev, devicetree, linux-input, linux-serial
In-Reply-To: <4bf1a5e9-904c-584e-72df-71abc3f99bd2@linux.intel.com>
...
> > diff --git a/drivers/hid/hid-hexdev-hexlin.c b/drivers/hid/hid-hexdev-hexlin.c
> > new file mode 100644
> > index 0000000000000..a9ed080b3e33e
> > --- /dev/null
> > +++ b/drivers/hid/hid-hexdev-hexlin.c
> >
...
>
> > +}
> > +
> > +#define HEXLIN_GET_CMD(name, enum_cmd) \
> > + static int hexlin_##name(struct hexlin_priv_data *p) \
> > + { \
> > + u8 *req; \
> > + int ret; \
> > + \
> > + req = kmalloc(sizeof(*req), GFP_KERNEL) ; \
>
> Extra space.
>
> Use:
>
> u8 *req __free(kfree) = kmalloc(sizeof(*req), GFP_KERNEL);
>
> > + if (!req) \
> > + return -ENOMEM; \
> > + \
> > + *req = enum_cmd; \
> > + \
> > + ret = hexlin_tx_req_status(p, req, sizeof(*req)); \
> > + if (ret) \
> > + hid_err(p->hid_dev, "%s failed, error: %d\n", \
> > + #name, ret); \
> > + \
> > + kfree(req); \
>
> Not needed after using __free().
>
> > + return ret; \
> > + }
> > +
> > +HEXLIN_GET_CMD(get_version, HEXLIN_GET_VERSION)
> > +HEXLIN_GET_CMD(reset_dev, HEXLIN_RESET)
> > +HEXLIN_GET_CMD(get_baudrate, HEXLIN_GET_BAUDRATE)
>
> Could you convert the function in the macro into a helper function which
> is just called by a simple function with the relevant parameters for
> these 3 cases?
The device actually has a lot more features that I'm using in my debug
version and which might end up here in the future. So I would like to
keep it. Any objections?
...
> > +
> > +static int hexlin_set_baudrate(struct hexlin_priv_data *priv, u16 baudrate)
> > +{
> > + struct hexlin_baudrate_req *req;
> > + int ret;
> > +
> > + if (baudrate < LIN_MIN_BAUDRATE || baudrate > LIN_MAX_BAUDRATE)
> > + return -EINVAL;
> > +
> > + req = kmalloc(sizeof(*req), GFP_KERNEL);
>
> Hmm... Why do you alloc this small structure (3 bytes?) with kmalloc() and
> not just have it in stack as a local variable?
This buffer must be suitable for DMA (see docu for struct urb).
So with a stack variable we would need to use kmemdup() before the
actual sending call, but that's what you did not like since v3 so I
changed it to this which now you also don't like.
Let's dial it back to the original kmemdup() usage, ok?
...
> > +static int hexlin_queue_frames_insert(struct hexlin_priv_data *priv,
> > + const struct hexlin_frame *hxf)
> > +{
> > + struct hid_device *hdev = priv->hid_dev;
> > + struct lin_frame lf;
> > +
> > + lf.len = hxf->len;
> > + lf.lin_id = hxf->lin_id;
> > + memcpy(lf.data, hxf->data, LIN_MAX_DLEN);
> > + lf.checksum = hxf->checksum;
> > + lf.checksum_mode = hxf->checksum_mode;
> > +
> > + hid_dbg(hdev, "id:%02x, len:%u, data:%*ph, chk:%02x (%s), flg:%08x\n",
> > + lf.lin_id, lf.len, lf.len, lf.data, lf.checksum,
> > + lf.checksum_mode ? "enhanced" : "classic", hxf->flags);
> > +
> > + lin_rx(priv->ldev, &lf);
> > +
> > + return 0;
> > +}
> > +
> > +static int hexlin_raw_event(struct hid_device *hdev,
> > + struct hid_report *report, u8 *data, int sz)
> > +{
> > + struct hexlin_priv_data *priv;
> > + struct hexlin_baudrate_req *br;
> > + struct hexlin_responder_answer_req *rar;
> > + struct hexlin_unconditional_req *hfr;
> > + struct hexlin_val8_req *vr;
> > +
> > + if (sz < 1 || sz > HEXLIN_PKGLEN_MAX)
> > + return -EREMOTEIO;
> > +
> > + priv = hid_get_drvdata(hdev);
> > +
> > + hid_dbg(hdev, "%s, size:%i, data[0]: 0x%02x\n", __func__, sz, data[0]);
> > +
> > + priv->is_error = false;
> > +
> > + switch (data[0]) {
> > + case HEXLIN_SUCCESS:
> > + if (sz != HEXLIN_LEN_RETCODE)
> > + return -EREMOTEIO;
> > + hid_dbg(hdev, "HEXLIN_SUCCESS: 0x%02x\n", data[0]);
> > + complete(&priv->wait_in_report);
> > + break;
> > + case HEXLIN_FAIL:
> > + if (sz != HEXLIN_LEN_RETCODE)
> > + return -EREMOTEIO;
> > + hid_err(hdev, "HEXLIN_FAIL: 0x%02x\n", data[0]);
> > + priv->is_error = true;
> > + complete(&priv->wait_in_report);
> > + break;
> > + case HEXLIN_GET_VERSION:
> > + if (sz != sizeof(*vr))
> > + return -EREMOTEIO;
> > + vr = (struct hexlin_val8_req *) data;
> > + priv->fw_version = vr->v;
> > + complete(&priv->wait_in_report);
> > + break;
> > + case HEXLIN_GET_RESPONDER_ANSWER_ID:
> > + if (sz != sizeof(*rar))
> > + return -EREMOTEIO;
> > + rar = (struct hexlin_responder_answer_req *) data;
> > + memcpy(&priv->answ, &rar->answ, sizeof(priv->answ));
> > + complete(&priv->wait_in_report);
> > + break;
> > + case HEXLIN_GET_BAUDRATE:
> > + if (sz != sizeof(*br))
> > + return -EREMOTEIO;
> > + br = (struct hexlin_baudrate_req *) data;
> > + le16_to_cpus(br->baudrate);
> > + priv->baudrate = br->baudrate;
> > + complete(&priv->wait_in_report);
> > + break;
> > + /* following cases not initiated by us, so no complete() */
> > + case HEXLIN_FRAME:
> > + if (sz != sizeof(*hfr)) {
> > + hid_err_once(hdev, "frame size mismatch: %i\n", sz);
> > + return -EREMOTEIO;
> > + }
> > + hfr = (struct hexlin_unconditional_req *) data;
> > + le32_to_cpus(hfr->frm.flags);
>
> I'm bit worried about this from endianness perspective. Perhaps there's
> some struct reusing that shouldn't be happening because the same field
> cannot be __le32 and u32 at the same time.
Can you propose a solution?
>
...
thanks
-- Christoph
^ permalink raw reply
* [PATCH] Input: wacom_w8001: Check for string overflow from strscpy calls
From: joshua @ 2024-05-10 23:43 UTC (permalink / raw)
To: linux-input, Jiri Kosina, Benjamin Tissoires
Cc: Ping Cheng, Aaron Armstrong Skomra, Jason Gerecke, Joshua Dickens,
Joshua Dickens
From: Joshua Dickens <Joshua@Joshua-Dickens.com>
The strscpy function is able to return an error code when a copy would
overflow the size of the destination. The copy is stopped and the buffer
terminated before overflow actually occurs so it is safe to continue
execution, but we should still produce a warning should this occur.
Signed-off-by: Joshua Dickens <joshua.dickens@wacom.com>
---
drivers/input/touchscreen/wacom_w8001.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/input/touchscreen/wacom_w8001.c b/drivers/input/touchscreen/wacom_w8001.c
index 928c5ee3ac36..b8acf196a09a 100644
--- a/drivers/input/touchscreen/wacom_w8001.c
+++ b/drivers/input/touchscreen/wacom_w8001.c
@@ -625,7 +625,10 @@ static int w8001_connect(struct serio *serio, struct serio_driver *drv)
/* For backwards-compatibility we compose the basename based on
* capabilities and then just append the tool type
*/
- strscpy(basename, "Wacom Serial", sizeof(basename));
+ if (strscpy(basename, "Wacom Serial", sizeof(basename)) < 0) {
+ dev_warn(&w8001->pen_dev->dev,
+ "String overflow while assembling basename");
+ }
err_pen = w8001_setup_pen(w8001, basename, sizeof(basename));
err_touch = w8001_setup_touch(w8001, basename, sizeof(basename));
@@ -635,7 +638,11 @@ static int w8001_connect(struct serio *serio, struct serio_driver *drv)
}
if (!err_pen) {
- strscpy(w8001->pen_name, basename, sizeof(w8001->pen_name));
+ if (strscpy(w8001->pen_name, basename,
+ sizeof(w8001->pen_name)) < 0) {
+ dev_warn(&w8001->pen_dev->dev,
+ "String overflow while assembling pen_name");
+ }
strlcat(w8001->pen_name, " Pen", sizeof(w8001->pen_name));
input_dev_pen->name = w8001->pen_name;
@@ -651,7 +658,11 @@ static int w8001_connect(struct serio *serio, struct serio_driver *drv)
}
if (!err_touch) {
- strscpy(w8001->touch_name, basename, sizeof(w8001->touch_name));
+ if (strscpy(w8001->touch_name, basename,
+ sizeof(w8001->touch_name)) < 0) {
+ dev_warn(&w8001->touch_dev->dev,
+ "String overflow while assembling touch_name");
+ }
strlcat(w8001->touch_name, " Finger",
sizeof(w8001->touch_name));
input_dev_touch->name = w8001->touch_name;
--
2.45.0
^ permalink raw reply related
* Re: [PATCH v2 4/7] HID: i2c-hid: elan: fix reset suspend current leakage
From: Doug Anderson @ 2024-05-10 23:36 UTC (permalink / raw)
To: Johan Hovold
Cc: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Dmitry Torokhov,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Konrad Dybcio,
Linus Walleij, linux-input, devicetree, linux-arm-msm,
linux-kernel, stable, Steev Klimaszewski
In-Reply-To: <20240507144821.12275-5-johan+linaro@kernel.org>
Hi,
On Tue, May 7, 2024 at 7:48 AM Johan Hovold <johan+linaro@kernel.org> wrote:
>
> @@ -40,17 +41,17 @@ static int elan_i2c_hid_power_up(struct i2chid_ops *ops)
> container_of(ops, struct i2c_hid_of_elan, ops);
> int ret;
>
> + gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1);
Could probably use a comment above it saying that this is important
when we have "no_reset_on_power_off" and doesn't do anything bad when
we don't so we can just do it unconditionally.
> +
> if (ihid_elan->vcc33) {
> ret = regulator_enable(ihid_elan->vcc33);
> if (ret)
> - return ret;
> + goto err_deassert_reset;
> }
>
> ret = regulator_enable(ihid_elan->vccio);
> - if (ret) {
> - regulator_disable(ihid_elan->vcc33);
> - return ret;
> - }
> + if (ret)
> + goto err_disable_vcc33;
>
> if (ihid_elan->chip_data->post_power_delay_ms)
> msleep(ihid_elan->chip_data->post_power_delay_ms);
> @@ -60,6 +61,15 @@ static int elan_i2c_hid_power_up(struct i2chid_ops *ops)
> msleep(ihid_elan->chip_data->post_gpio_reset_on_delay_ms);
>
> return 0;
> +
> +err_disable_vcc33:
> + if (ihid_elan->vcc33)
> + regulator_disable(ihid_elan->vcc33);
> +err_deassert_reset:
> + if (ihid_elan->no_reset_on_power_off)
> + gpiod_set_value_cansleep(ihid_elan->reset_gpio, 0);
Small nit about the error label: it sounds as if when you go here you
_will_ deassert reset when in actuality you might or might not
(depending on no_reset_on_power_off). Personally I prefer to label
error jumps based on things I've done instead of things that the error
jump needs to do, so you could call them "err_enabled_vcc33" and
"err_asserted_reset"...
I don't feel that strongly about it, though, so if you love the label
you have then no need to change it.
> @@ -67,7 +77,14 @@ static void elan_i2c_hid_power_down(struct i2chid_ops *ops)
> struct i2c_hid_of_elan *ihid_elan =
> container_of(ops, struct i2c_hid_of_elan, ops);
>
> - gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1);
> + /*
> + * Do not assert reset when the hardware allows for it to remain
> + * deasserted regardless of the state of the (shared) power supply to
> + * avoid wasting power when the supply is left on.
> + */
> + if (!ihid_elan->no_reset_on_power_off)
> + gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1);
> +
> if (ihid_elan->chip_data->post_gpio_reset_off_delay_ms)
> msleep(ihid_elan->chip_data->post_gpio_reset_off_delay_ms);
Shouldn't the above two lines be inside the "if
(!ihid_elan->no_reset_on_power_off)" test? If you're not setting the
reset GPIO then you don't need to do the delay, right?
> @@ -79,6 +96,7 @@ static void elan_i2c_hid_power_down(struct i2chid_ops *ops)
> static int i2c_hid_of_elan_probe(struct i2c_client *client)
> {
> struct i2c_hid_of_elan *ihid_elan;
> + int ret;
>
> ihid_elan = devm_kzalloc(&client->dev, sizeof(*ihid_elan), GFP_KERNEL);
> if (!ihid_elan)
> @@ -93,21 +111,38 @@ static int i2c_hid_of_elan_probe(struct i2c_client *client)
> if (IS_ERR(ihid_elan->reset_gpio))
> return PTR_ERR(ihid_elan->reset_gpio);
>
> + ihid_elan->no_reset_on_power_off = of_property_read_bool(client->dev.of_node,
> + "no-reset-on-power-off");
Personally, I'd rather you query for the property before you request
the GPIO and then request the GPIO in the "powered off" state just to
keep everything in the most consistent state possible.
-Doug
^ permalink raw reply
* [GIT PULL] Immutable branch between Backlight, HID and fbdev due for the v6.10 merge window
From: Lee Jones @ 2024-05-10 14:55 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Daniel Thompson, Jingoo Han, Helge Deller, Bruno Prémont,
Jiri Kosina, Benjamin Tissoires, Alexander Shiyan, Sascha Hauer,
Pengutronix Kernel Team, Shawn Guo, Fabio Estevam, dri-devel,
linux-fbdev, linux-kernel, linux-input, linux-arm-kernel, imx,
linux-omap, Thomas Zimmermann, Thomas Weißschuh
In-Reply-To: <20240424-video-backlight-lcd-ops-v2-0-1aaa82b07bc6@kernel.org>
Enjoy!
The following changes since commit 4cece764965020c22cff7665b18a012006359095:
Linux 6.9-rc1 (2024-03-24 14:10:05 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/lee/backlight.git ib-backlight-hid-fbdev-lcd-scripts-v6.10
for you to fetch changes up to 82b9007bc4f8c22975d640d7df6743366f25a353:
const_structs.checkpatch: add lcd_ops (2024-05-03 10:45:55 +0100)
----------------------------------------------------------------
Immutable branch between Backlight, HID and fbdev due for the v6.10 merge window
----------------------------------------------------------------
Krzysztof Kozlowski (19):
backlight: lcd: Constify lcd_ops
backlight: ams369fg06: Constify lcd_ops
backlight: corgi_lcd: Constify lcd_ops
backlight: hx8357: Constify lcd_ops
backlight: ili922x: Constify lcd_ops
backlight: ili9320: Constify lcd_ops
backlight: jornada720_lcd: Constify lcd_ops
backlight: l4f00242t03: Constify lcd_ops
backlight: lms283gf05: Constify lcd_ops
backlight: lms501kf03: Constify lcd_ops
backlight: ltv350qv: Constify lcd_ops
backlight: otm3225a: Constify lcd_ops
backlight: platform_lcd: Constify lcd_ops
backlight: tdo24m: Constify lcd_ops
HID: picoLCD: Constify lcd_ops
fbdev: clps711x: Constify lcd_ops
fbdev: imx: Constify lcd_ops
fbdev: omap: lcd_ams_delta: Constify lcd_ops
const_structs.checkpatch: add lcd_ops
drivers/hid/hid-picolcd_lcd.c | 2 +-
drivers/video/backlight/ams369fg06.c | 2 +-
drivers/video/backlight/corgi_lcd.c | 2 +-
drivers/video/backlight/hx8357.c | 2 +-
drivers/video/backlight/ili922x.c | 2 +-
drivers/video/backlight/ili9320.c | 2 +-
drivers/video/backlight/jornada720_lcd.c | 2 +-
drivers/video/backlight/l4f00242t03.c | 2 +-
drivers/video/backlight/lcd.c | 4 ++--
drivers/video/backlight/lms283gf05.c | 2 +-
drivers/video/backlight/lms501kf03.c | 2 +-
drivers/video/backlight/ltv350qv.c | 2 +-
drivers/video/backlight/otm3225a.c | 2 +-
drivers/video/backlight/platform_lcd.c | 2 +-
drivers/video/backlight/tdo24m.c | 2 +-
drivers/video/fbdev/clps711x-fb.c | 2 +-
drivers/video/fbdev/imxfb.c | 2 +-
drivers/video/fbdev/omap/lcd_ams_delta.c | 2 +-
include/linux/lcd.h | 6 +++---
scripts/const_structs.checkpatch | 1 +
20 files changed, 23 insertions(+), 22 deletions(-)
--
Lee Jones [李琼斯]
^ permalink raw reply
* Re: [PATCH v4 10/11] can: lin: Support setting LIN mode
From: Ilpo Järvinen @ 2024-05-10 14:39 UTC (permalink / raw)
To: Christoph Fritz
Cc: Jiri Slaby, Simon Horman, Greg Kroah-Hartman, Marc Kleine-Budde,
Oliver Hartkopp, Vincent Mailhol, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jiri Kosina, Benjamin Tissoires, Sebastian Reichel,
Linus Walleij, Andreas Lauser, Jonathan Corbet, Pavel Pisa,
linux-can, Netdev, devicetree, linux-input, linux-serial
In-Reply-To: <20240509171736.2048414-11-christoph.fritz@hexdev.de>
On Thu, 9 May 2024, Christoph Fritz wrote:
> A LIN node can work as commander or responder, so introduce a new
> control mode (CAN_CTRLMODE_LIN_COMMANDER) for configuration.
>
> This enables e.g. the userland tool ip from iproute2 to turn on
> commander mode when the device is being brought up.
>
> Signed-off-by: Christoph Fritz <christoph.fritz@hexdev.de>
> ---
> drivers/net/can/lin.c | 40 +++++++++++++++++++++++++++++++-
> include/net/lin.h | 7 ++++++
> include/uapi/linux/can/netlink.h | 1 +
> 3 files changed, 47 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/can/lin.c b/drivers/net/can/lin.c
> index f77abd7d7d21c..03ddf5d5a31b8 100644
> --- a/drivers/net/can/lin.c
> +++ b/drivers/net/can/lin.c
> @@ -262,11 +262,40 @@ static netdev_tx_t lin_start_xmit(struct sk_buff *skb,
> return NETDEV_TX_OK;
> }
>
> +static int lin_update_mode(struct net_device *ndev)
> +{
> + struct lin_device *ldev = netdev_priv(ndev);
> + u32 ctrlmode = ldev->can.ctrlmode;
> + enum lin_mode lm;
> + int ret = 0;
> +
> + lm = (ctrlmode & CAN_CTRLMODE_LIN_COMMANDER) ? LINBUS_COMMANDER :
> + LINBUS_RESPONDER;
> + if (ldev->lmode != lm) {
> + if (!ldev->ldev_ops->update_lin_mode) {
> + netdev_err(ndev, "setting lin mode unsupported\n");
In user visible messages, it would be best to use the expected
capitalization, which I suppose is LIN given you use capitals in the
commit message yourself?
> + return -EINVAL;
> + }
> + ret = ldev->ldev_ops->update_lin_mode(ldev, lm);
> + if (ret) {
> + netdev_err(ndev, "Failed to set lin mode: %d\n", ret);
Ditto.
There might be other cases in any of the patches, please check.
> + return ret;
> + }
> + ldev->lmode = lm;
> + }
> +
> + return ret;
> +}
> +
> static int lin_open(struct net_device *ndev)
> {
> struct lin_device *ldev = netdev_priv(ndev);
> int ret;
>
> + ret = lin_update_mode(ndev);
> + if (ret)
> + return ret;
> +
> ldev->tx_busy = false;
>
> ret = open_candev(ndev);
> @@ -443,7 +472,7 @@ struct lin_device *register_lin(struct device *dev,
> ndev->sysfs_groups[0] = &lin_sysfs_group;
> ldev->can.bittiming.bitrate = LIN_DEFAULT_BAUDRATE;
> ldev->can.ctrlmode = CAN_CTRLMODE_LIN;
> - ldev->can.ctrlmode_supported = 0;
> + ldev->can.ctrlmode_supported = CAN_CTRLMODE_LIN_COMMANDER;
> ldev->can.bitrate_const = lin_bitrate;
> ldev->can.bitrate_const_cnt = ARRAY_SIZE(lin_bitrate);
> ldev->can.do_set_bittiming = lin_set_bittiming;
> @@ -458,6 +487,15 @@ struct lin_device *register_lin(struct device *dev,
> goto exit_candev;
> }
>
> + ldev->lmode = LINBUS_RESPONDER;
> + if (ldev->ldev_ops->update_lin_mode) {
> + ret = ldev->ldev_ops->update_lin_mode(ldev, ldev->lmode);
> + if (ret) {
> + netdev_err(ndev, "updating lin mode failed\n");
Ditto.
> + goto exit_candev;
> + }
> + }
> +
> ret = register_candev(ndev);
> if (ret)
> goto exit_candev;
> diff --git a/include/net/lin.h b/include/net/lin.h
> index 31bb0feefd188..63ac870a0ab6f 100644
> --- a/include/net/lin.h
> +++ b/include/net/lin.h
> @@ -36,6 +36,11 @@ struct lin_attr {
> struct lin_device *ldev;
> };
>
> +enum lin_mode {
> + LINBUS_RESPONDER = 0,
> + LINBUS_COMMANDER,
> +};
> +
> struct lin_device {
> struct can_priv can; /* must be the first member */
> struct net_device *ndev;
> @@ -45,6 +50,7 @@ struct lin_device {
> struct work_struct tx_work;
> bool tx_busy;
> struct sk_buff *tx_skb;
> + enum lin_mode lmode;
> };
>
> enum lin_checksum_mode {
> @@ -71,6 +77,7 @@ struct lin_device_ops {
> int (*ldo_open)(struct lin_device *ldev);
> int (*ldo_stop)(struct lin_device *ldev);
> int (*ldo_tx)(struct lin_device *ldev, const struct lin_frame *frame);
> + int (*update_lin_mode)(struct lin_device *ldev, enum lin_mode lm);
> int (*update_bitrate)(struct lin_device *ldev, u16 bitrate);
> int (*update_responder_answer)(struct lin_device *ldev,
> const struct lin_responder_answer *answ);
> diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
> index a37f56d86c5f2..cc390f6444d59 100644
> --- a/include/uapi/linux/can/netlink.h
> +++ b/include/uapi/linux/can/netlink.h
> @@ -104,6 +104,7 @@ struct can_ctrlmode {
> #define CAN_CTRLMODE_TDC_AUTO 0x200 /* CAN transiver automatically calculates TDCV */
> #define CAN_CTRLMODE_TDC_MANUAL 0x400 /* TDCV is manually set up by user */
> #define CAN_CTRLMODE_LIN BIT(11) /* LIN bus mode */
> +#define CAN_CTRLMODE_LIN_COMMANDER BIT(12) /* LIN bus specific commander mode */
>
> /*
> * CAN device statistics
>
--
i.
^ permalink raw reply
* Re: [PATCH v4 09/11] can: lin: Handle rx offload config frames
From: Ilpo Järvinen @ 2024-05-10 14:36 UTC (permalink / raw)
To: Christoph Fritz
Cc: Jiri Slaby, Simon Horman, Greg Kroah-Hartman, Marc Kleine-Budde,
Oliver Hartkopp, Vincent Mailhol, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jiri Kosina, Benjamin Tissoires, Sebastian Reichel,
Linus Walleij, Andreas Lauser, Jonathan Corbet, Pavel Pisa,
linux-can, Netdev, devicetree, linux-input, linux-serial
In-Reply-To: <20240509171736.2048414-10-christoph.fritz@hexdev.de>
On Thu, 9 May 2024, Christoph Fritz wrote:
> The CAN Broadcast Manager now has the capability to dispatch CANFD
> frames marked with the id LINBUS_RXOFFLOAD_ID.
>
> Introduce functionality to interpret these specific frames, enabling the
> configuration of RX offloading within the LIN driver.
>
> Signed-off-by: Christoph Fritz <christoph.fritz@hexdev.de>
> ---
> drivers/net/can/lin.c | 29 +++++++++++++++++++++++++++++
> 1 file changed, 29 insertions(+)
>
> diff --git a/drivers/net/can/lin.c b/drivers/net/can/lin.c
> index a22768c17e3f8..f77abd7d7d21c 100644
> --- a/drivers/net/can/lin.c
> +++ b/drivers/net/can/lin.c
> @@ -185,6 +185,27 @@ static const struct attribute_group lin_sysfs_group = {
> .attrs = lin_sysfs_attrs,
> };
>
> +static int lin_setup_rxoffload(struct lin_device *ldev,
> + struct canfd_frame *cfd)
> +{
> + struct lin_responder_answer answ;
> +
> + if (!(cfd->flags & CANFD_FDF))
> + return -EINVAL;
> +
> + BUILD_BUG_ON(sizeof(answ) > sizeof(cfd->data));
> + memcpy(&answ, cfd->data, sizeof(answ));
> +
> + answ.lf.checksum_mode = (cfd->can_id & LIN_ENHANCED_CKSUM_FLAG) ?
> + LINBUS_ENHANCED : LINBUS_CLASSIC;
> +
> + if (answ.lf.lin_id > LIN_ID_MASK ||
> + answ.event_associated_id > LIN_ID_MASK)
> + return -EINVAL;
These can be reverse so that error check occur before the checksum_mode
assignment? It would feel more natural that way.
...Even better, if the error check could be done before the memcpy().
--
i.
> + return ldev->ldev_ops->update_responder_answer(ldev, &answ);
> +}
> +
> static void lin_tx_work_handler(struct work_struct *ws)
> {
> struct lin_device *ldev = container_of(ws, struct lin_device,
> @@ -197,6 +218,14 @@ static void lin_tx_work_handler(struct work_struct *ws)
> ldev->tx_busy = true;
>
> cfd = (struct canfd_frame *)ldev->tx_skb->data;
> +
> + if (cfd->can_id & LIN_RXOFFLOAD_DATA_FLAG) {
> + ret = lin_setup_rxoffload(ldev, cfd);
> + if (ret < 0)
> + netdev_err(ndev, "setting up rx failed %d\n", ret);
> + goto lin_tx_out;
> + }
> +
> lf.checksum_mode = (cfd->can_id & LIN_ENHANCED_CKSUM_FLAG) ?
> LINBUS_ENHANCED : LINBUS_CLASSIC;
> lf.lin_id = cfd->can_id & LIN_ID_MASK;
>
^ permalink raw reply
* Re: [PATCH v4 07/11] can: Add support for hexDEV serial LIN adapter hexLINSER
From: Ilpo Järvinen @ 2024-05-10 14:32 UTC (permalink / raw)
To: Christoph Fritz
Cc: Jiri Slaby, Simon Horman, Greg Kroah-Hartman, Marc Kleine-Budde,
Oliver Hartkopp, Vincent Mailhol, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jiri Kosina, Benjamin Tissoires, Sebastian Reichel,
Linus Walleij, Andreas Lauser, Jonathan Corbet, Pavel Pisa,
linux-can, Netdev, devicetree, linux-input, linux-serial
In-Reply-To: <20240509171736.2048414-8-christoph.fritz@hexdev.de>
On Thu, 9 May 2024, Christoph Fritz wrote:
> Introduce support for the hexDEV serial LIN adapter hexLINSER. These
> devices are equipped with LIN transceivers and are mostly hard-wired to
> serial devices.
>
> This device driver uses CAN_LIN on one side and the serial device bus
> (serdev) interface on the other.
>
> For more details on the adapter, visit: https://hexdev.de/hexlin#hexLINSER
>
> Signed-off-by: Christoph Fritz <christoph.fritz@hexdev.de>
> ---
> drivers/net/can/Kconfig | 15 ++
> drivers/net/can/Makefile | 1 +
> drivers/net/can/hex-linser.c | 505 +++++++++++++++++++++++++++++++++++
> 3 files changed, 521 insertions(+)
> create mode 100644 drivers/net/can/hex-linser.c
>
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index 0934bbf8d03b2..141972d6bbf1e 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -181,6 +181,21 @@ config CAN_LIN
>
> Actual device drivers need to be enabled too.
>
> +config CAN_LIN_HEXLINSER
> + tristate "hexDEV hexLINSER serial LIN Adaptors"
> + depends on CAN_LIN && SERIAL_DEV_BUS && OF
> + help
> + LIN support for serial devices equipped with LIN transceivers.
> + This device driver is using CAN_LIN for a userland connection on
> + one side and the kernel its serial device bus (serdev) interface
> + on the other side.
> +
> + If you have a hexLINSER tty adapter, say Y here and see
> + <https://hexdev.de/hexlin#hexLINSER>.
> +
> + This driver can also be built as a module. If so, the module will be
> + called hex-linser.ko.
> +
> config CAN_SLCAN
> tristate "Serial / USB serial CAN Adaptors (slcan)"
> depends on TTY
> diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
> index 0093ee9219ca8..9fdad4a0fd12a 100644
> --- a/drivers/net/can/Makefile
> +++ b/drivers/net/can/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_CAN_IFI_CANFD) += ifi_canfd/
> obj-$(CONFIG_CAN_JANZ_ICAN3) += janz-ican3.o
> obj-$(CONFIG_CAN_KVASER_PCIEFD) += kvaser_pciefd.o
> obj-$(CONFIG_CAN_LIN) += lin.o
> +obj-$(CONFIG_CAN_LIN_HEXLINSER) += hex-linser.o
> obj-$(CONFIG_CAN_MSCAN) += mscan/
> obj-$(CONFIG_CAN_M_CAN) += m_can/
> obj-$(CONFIG_CAN_PEAK_PCIEFD) += peak_canfd/
> diff --git a/drivers/net/can/hex-linser.c b/drivers/net/can/hex-linser.c
> new file mode 100644
> index 0000000000000..9c2d11d2ed0c0
> --- /dev/null
> +++ b/drivers/net/can/hex-linser.c
> @@ -0,0 +1,505 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/* Copyright (C) 2024 hexDEV GmbH - https://hexdev.de */
> +
> +#include <linux/delay.h>
> +#include <linux/init.h>
> +#include <linux/kfifo.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/serdev.h>
> +#include <linux/slab.h>
> +#include <linux/tty.h>
> +#include <net/lin.h>
> +
> +#define LINSER_SAMPLES_PER_CHAR 10
> +#define LINSER_TX_BUFFER_SIZE 11
> +#define LINSER_RX_FIFO_SIZE 256
> +#define LINSER_PARSE_BUFFER 24
> +
> +struct linser_rx {
> + u8 data;
> + u8 flag;
> +};
> +
> +enum linser_rx_status {
> + NEED_MORE = -1,
> + MODE_OK = 0,
> + NEED_FORCE,
> +};
> +
> +struct linser_priv {
> + struct lin_device *lin_dev;
> + struct serdev_device *serdev;
> + DECLARE_KFIFO_PTR(rx_fifo, struct linser_rx);
> + struct delayed_work rx_work;
> + unsigned long break_usleep_min;
> + unsigned long break_usleep_max;
> + unsigned long post_break_usleep_min;
> + unsigned long post_break_usleep_max;
> + unsigned long force_timeout_jfs;
> + struct lin_responder_answer respond_answ[LIN_NUM_IDS];
> + struct mutex resp_lock; /* protects respond_answ */
> + bool is_stopped;
> +};
> +
> +static int linser_open(struct lin_device *ldev)
> +{
> + struct serdev_device *serdev = to_serdev_device(ldev->dev);
> + struct linser_priv *priv = serdev_device_get_drvdata(serdev);
> + int ret;
> +
> + if (priv->is_stopped) {
> + ret = serdev_device_open(serdev);
> + if (ret) {
> + dev_err(&serdev->dev, "Unable to open device\n");
> + return ret;
> + }
> +
> + serdev_device_set_flow_control(serdev, false);
> + serdev_device_set_break_detection(serdev, true);
> +
> + priv->is_stopped = false;
> + }
> +
> + return 0;
> +}
> +
> +static int linser_stop(struct lin_device *ldev)
> +{
> + struct serdev_device *serdev = to_serdev_device(ldev->dev);
> + struct linser_priv *priv = serdev_device_get_drvdata(serdev);
> +
> + if (priv->is_stopped)
> + return 0;
> +
> + serdev_device_close(serdev);
> + priv->is_stopped = true;
> +
> + return 0;
> +}
> +
> +static int linser_send_break(struct linser_priv *priv)
> +{
> + struct serdev_device *serdev = priv->serdev;
> + int ret;
> +
> + ret = serdev_device_break_ctl(serdev, -1);
> + if (ret)
> + return ret;
> +
> + usleep_range(priv->break_usleep_min, priv->break_usleep_max);
> +
> + ret = serdev_device_break_ctl(serdev, 0);
> + if (ret)
> + return ret;
> +
> + usleep_range(priv->post_break_usleep_min, priv->post_break_usleep_max);
> +
> + return 0;
> +}
> +
> +static int linser_ldo_tx(struct lin_device *ldev, const struct lin_frame *lf)
> +{
> + struct serdev_device *serdev = to_serdev_device(ldev->dev);
> + struct linser_priv *priv = serdev_device_get_drvdata(serdev);
> + u8 pid = LIN_FORM_PID(lf->lin_id);
> + u8 buf[LINSER_TX_BUFFER_SIZE];
> + ssize_t written_len, total_len;
> + u8 checksum;
> + int ret;
> +
> + if (lf->len + 3 > LINSER_TX_BUFFER_SIZE) {
> + dev_err(&serdev->dev, "Frame length %u exceeds buffer size\n", lf->len);
> + return -EINVAL;
> + }
> +
> + buf[0] = LIN_SYNC_BYTE;
> + buf[1] = pid;
> + total_len = 2;
> +
> + if (lf->len) {
> + memcpy(&buf[2], lf->data, lf->len);
> + checksum = lin_get_checksum(pid, lf->len, lf->data,
> + lf->checksum_mode);
> + buf[lf->len + 2] = checksum;
> + total_len += lf->len + 1;
> + }
> +
> + ret = linser_send_break(priv);
> + if (ret)
> + return ret;
> +
> + written_len = serdev_device_write(serdev, buf, total_len, 0);
> + if (written_len < total_len)
> + return written_len < 0 ? (int)written_len : -EIO;
> +
> + dev_dbg(&serdev->dev, "sent out: %*ph\n", (int)total_len, buf);
> +
> + serdev_device_wait_until_sent(serdev, 0);
> +
> + return 0;
> +}
> +
> +static int linser_derive_timings(struct linser_priv *priv, u16 bitrate)
> +{
> + unsigned long break_baud = (bitrate * 2) / 3;
> + struct serdev_device *serdev = priv->serdev;
> + unsigned long timeout_us;
> +
> + if (bitrate < LIN_MIN_BAUDRATE || bitrate > LIN_MAX_BAUDRATE) {
> + dev_err(&serdev->dev, "Bitrate %u out of bounds (%u to %u)\n",
> + bitrate, LIN_MIN_BAUDRATE, LIN_MAX_BAUDRATE);
> + return -EINVAL;
> + }
> +
> + priv->break_usleep_min = (USEC_PER_SEC * LINSER_SAMPLES_PER_CHAR) /
> + break_baud;
> + priv->break_usleep_max = priv->break_usleep_min + 50;
> + priv->post_break_usleep_min = USEC_PER_SEC / break_baud;
> + priv->post_break_usleep_max = priv->post_break_usleep_min + 30;
> +
> + timeout_us = DIV_ROUND_CLOSEST(USEC_PER_SEC * 256, bitrate);
> + priv->force_timeout_jfs = usecs_to_jiffies(timeout_us);
> +
> + return 0;
> +}
> +
> +static int linser_update_bitrate(struct lin_device *ldev, u16 bitrate)
> +{
> + struct serdev_device *serdev = to_serdev_device(ldev->dev);
> + struct linser_priv *priv = serdev_device_get_drvdata(serdev);
> + unsigned int speed;
> + int ret;
> +
> + ret = linser_open(ldev);
> + if (ret)
> + return ret;
> +
> + speed = serdev_device_set_baudrate(serdev, bitrate);
> + if (!bitrate || speed != bitrate)
> + return -EINVAL;
> +
> + ret = linser_derive_timings(priv, bitrate);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int linser_get_responder_answer(struct lin_device *ldev, u8 id,
> + struct lin_responder_answer *answ)
> +{
> + struct serdev_device *serdev = to_serdev_device(ldev->dev);
> + struct linser_priv *priv = serdev_device_get_drvdata(serdev);
> + struct lin_responder_answer *r = &priv->respond_answ[id];
> +
> + if (!answ)
> + return -EINVAL;
> +
> + guard(mutex)(&priv->resp_lock);
> + memcpy(answ, r, sizeof(*answ));
> +
> + return 0;
> +}
> +
> +static int linser_update_resp_answer(struct lin_device *ldev,
> + const struct lin_responder_answer *answ)
> +{
> + struct serdev_device *serdev = to_serdev_device(ldev->dev);
> + struct linser_priv *priv = serdev_device_get_drvdata(serdev);
> + struct lin_responder_answer *r = &priv->respond_answ[answ->lf.lin_id];
> +
> + if (!answ)
> + return -EINVAL;
> +
> + mutex_lock(&priv->resp_lock);
> + memcpy(r, answ, sizeof(*answ));
> + r->lf.checksum = lin_get_checksum(LIN_FORM_PID(answ->lf.lin_id),
> + answ->lf.len,
> + answ->lf.data,
> + answ->lf.checksum_mode);
Can this checksum occur outside of lock using the copy in r?
In anycase, use guard() (or scoped_guard() if the checksum can happen
outside of the lock.)
> + mutex_unlock(&priv->resp_lock);
> +
> + return 0;
> +}
> +
> +static struct lin_device_ops linser_lindev_ops = {
> + .ldo_open = linser_open,
> + .ldo_stop = linser_stop,
> + .ldo_tx = linser_ldo_tx,
> + .update_bitrate = linser_update_bitrate,
> + .get_responder_answer = linser_get_responder_answer,
> + .update_responder_answer = linser_update_resp_answer,
> +};
> +
> +static bool linser_tx_frame_as_responder(struct linser_priv *priv, u8 id)
> +{
> + struct lin_responder_answer *answ = &priv->respond_answ[id];
> + struct serdev_device *serdev = priv->serdev;
> + u8 buf[LINSER_TX_BUFFER_SIZE];
> + u8 checksum, count, n;
> + ssize_t write_len;
> +
> + scoped_guard(mutex, &priv->resp_lock) {
> + if (!answ->is_active)
> + return false;
> +
> + if (answ->is_event_frame) {
> + struct lin_responder_answer *e_answ;
> +
> + e_answ = &priv->respond_answ[answ->event_associated_id];
> + n = min(e_answ->lf.len, LIN_MAX_DLEN);
> +
> + if (memcmp(answ->lf.data, e_answ->lf.data, n) == 0)
> + return false;
> +
> + memcpy(answ->lf.data, e_answ->lf.data, n);
> + checksum = lin_get_checksum(LIN_FORM_PID(answ->lf.lin_id),
> + n, e_answ->lf.data,
> + answ->lf.checksum_mode);
> + answ = e_answ;
> + } else {
> + checksum = answ->lf.checksum;
> + }
> +
> + count = min(answ->lf.len, LIN_MAX_DLEN);
> + memcpy(&buf[0], answ->lf.data, count);
> + buf[count] = checksum;
> + }
> +
> + write_len = serdev_device_write(serdev, buf, count + 1, 0);
> + if (write_len < count + 1)
> + return false;
> +
> + serdev_device_wait_until_sent(serdev, 0);
> +
> + return true;
> +}
> +
> +static void linser_pop_fifo(struct linser_priv *priv, size_t n)
> +{
> + for (size_t i = 0; i < n; i++)
> + kfifo_skip(&priv->rx_fifo);
> +}
> +
> +static int linser_fill_frame(struct linser_priv *priv, struct lin_frame *lf)
> +{
> + struct serdev_device *serdev = priv->serdev;
> + struct linser_rx buf[LINSER_PARSE_BUFFER];
> + unsigned int count, i, brk = 0;
> +
> + count = kfifo_out_peek(&priv->rx_fifo, buf, LINSER_PARSE_BUFFER);
> +
> + memset(lf, 0, sizeof(*lf));
> +
> + for (i = 0; i < count; i++) {
> + dev_dbg(&serdev->dev, "buf[%d]: data=%02x, flag=%02x\n",
> + i, buf[i].data, buf[i].flag);
> + }
> +
> + if (count < 3)
> + return NEED_MORE;
> +
> + if (buf[0].flag != TTY_BREAK || buf[1].data != LIN_SYNC_BYTE) {
> + linser_pop_fifo(priv, 1); /* pop incorrect start */
> + return NEED_MORE;
> + } else if (!LIN_CHECK_PID(buf[2].data)) {
> + linser_pop_fifo(priv, 3); /* pop incorrect header */
> + return NEED_MORE;
> + }
> +
> + lf->lin_id = LIN_GET_ID(buf[2].data);
> +
> + /* from here on we do have a correct LIN header */
> +
> + if (count == 3)
> + return linser_tx_frame_as_responder(priv, lf->lin_id) ?
> + NEED_MORE : NEED_FORCE;
> +
> + for (i = 3; i < count && i < LINSER_PARSE_BUFFER && i < 12; i++) {
> + if (buf[i].flag == TTY_BREAK) {
> + brk = i;
> + break;
> + }
> + lf->len++;
> + }
> + if (lf->len)
> + lf->len -= 1; /* account for checksum */
> +
> + if (brk == 3)
> + return MODE_OK;
> +
> + if (brk == 4) {
> + /* suppress wrong answer data-byte in between PID and break
> + * because checksum is missing
> + */
> + return MODE_OK;
> + }
> +
> + for (i = 0; i < lf->len; i++)
> + lf->data[i] = buf[3 + i].data;
> + lf->checksum = buf[2 + lf->len + 1].data;
> + mutex_lock(&priv->resp_lock);
> + lf->checksum_mode = priv->respond_answ[lf->lin_id].lf.checksum_mode;
> + mutex_unlock(&priv->resp_lock);
> +
> + dev_dbg(&serdev->dev, "brk:%i, len:%u, data:%*ph, checksum:%x (%s)\n",
> + brk, lf->len, lf->len, lf->data, lf->checksum,
> + lf->checksum_mode ? "enhanced" : "classic");
> +
> + if (brk > 4)
> + return MODE_OK; /* frame in between two breaks: so complete */
> +
> + if (lf->len == 8)
> + return MODE_OK;
> +
> + return NEED_FORCE;
> +}
> +
> +static int linser_process_frame(struct linser_priv *priv, bool force)
> +{
> + struct serdev_device *serdev = priv->serdev;
> + struct lin_frame lf;
> + size_t bytes_to_pop;
> + int ret = NEED_MORE;
> +
> + while (kfifo_len(&priv->rx_fifo) >= LIN_HEADER_SIZE) {
> + ret = linser_fill_frame(priv, &lf);
> +
> + if (!(ret == MODE_OK || (ret == NEED_FORCE && force)))
> + return ret;
> +
> + dev_dbg(&serdev->dev, "lin_rx: %s\n", force ?
> + "force" : "normal");
Put to one line.
> + lin_rx(priv->lin_dev, &lf);
> + bytes_to_pop = LIN_HEADER_SIZE + lf.len + (lf.len ? 1 : 0);
> + linser_pop_fifo(priv, bytes_to_pop);
> + force = false;
> + ret = MODE_OK;
> + }
> +
> + return ret;
> +}
--
i.
^ permalink raw reply
* Re: [PATCH v4 04/11] tty: serdev: Add method to enable break flags
From: Ilpo Järvinen @ 2024-05-10 14:21 UTC (permalink / raw)
To: Christoph Fritz
Cc: Jiri Slaby, Simon Horman, Greg Kroah-Hartman, Marc Kleine-Budde,
Oliver Hartkopp, Vincent Mailhol, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jiri Kosina, Benjamin Tissoires, Sebastian Reichel,
Linus Walleij, Andreas Lauser, Jonathan Corbet, Pavel Pisa,
linux-can, Netdev, devicetree, linux-input, linux-serial
In-Reply-To: <20240509171736.2048414-5-christoph.fritz@hexdev.de>
On Thu, 9 May 2024, Christoph Fritz wrote:
> The recently introduced callback function receive_buf_fp() brings flags
> buffer support. To allow signaling of TTY_BREAK flags, this patch
> introduces serdev_device_set_break_detection() and an implementation for
> ttyport. This enables serdev devices to configure their underlying tty
> port to signal or ignore break conditions.
>
> Signed-off-by: Christoph Fritz <christoph.fritz@hexdev.de>
> ---
> drivers/tty/serdev/core.c | 11 +++++++++++
> drivers/tty/serdev/serdev-ttyport.c | 17 +++++++++++++++++
> include/linux/serdev.h | 2 ++
> 3 files changed, 30 insertions(+)
>
> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> index 613cb356b918d..23a1e76cb553b 100644
> --- a/drivers/tty/serdev/core.c
> +++ b/drivers/tty/serdev/core.c
> @@ -339,6 +339,17 @@ unsigned int serdev_device_set_baudrate(struct serdev_device *serdev, unsigned i
> }
> EXPORT_SYMBOL_GPL(serdev_device_set_baudrate);
>
> +void serdev_device_set_break_detection(struct serdev_device *serdev, bool enable)
> +{
> + struct serdev_controller *ctrl = serdev->ctrl;
> +
> + if (!ctrl || !ctrl->ops->set_break_detection)
> + return;
Why you need to test for !ctrl?
> + ctrl->ops->set_break_detection(ctrl, enable);
I'd use positive logic here:
if (ctrl->ops->set_break_detection)
ctrl->ops->set_break_detection(ctrl, enable);
> +}
> +EXPORT_SYMBOL_GPL(serdev_device_set_break_detection);
--
i.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox