[PATCH tty-next v2 0/3] hid-ft260 cleanups

[PATCH tty-next v2 0/3] hid-ft260 cleanups

[Christina Quast]

Cleanups in preparation for the serial driver functionality addition
to hid-ft260. Those cleanups include adding braces into macros,
renaming the ft260_i2c_input_report structure and datatype changes
in hw facing structs.

Changelog:
Added Changelog to each of the patch files.

Christina Quast (3):
  hid-ft260: Cleanup macro formatting
  hid-ft260: Rename struct ft260_i2c_input_report
  hid-ft260: Change u8 to __u8 for hw facing structs

 drivers/hid/hid-ft260.c | 100 ++++++++++++++++++++--------------------
 1 file changed, 50 insertions(+), 50 deletions(-)

-- 
2.35.1

[PATCH tty-next v2 1/3] hid-ft260: Cleanup macro formatting

[Christina Quast]

Wrap macro arguments in braces.

Signed-off-by: Christina Quast <contact@christina-quast.de>
---
 drivers/hid/hid-ft260.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index 333341e80b0e..52a63b966ebc 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -29,7 +29,7 @@ MODULE_PARM_DESC(debug, "Toggle FT260 debugging messages");
 	} while (0)
 
 #define FT260_REPORT_MAX_LENGTH (64)
-#define FT260_I2C_DATA_REPORT_ID(len) (FT260_I2C_REPORT_MIN + (len - 1) / 4)
+#define FT260_I2C_DATA_REPORT_ID(len) (FT260_I2C_REPORT_MIN + ((len) - 1) / 4)
 
 #define FT260_WAKEUP_NEEDED_AFTER_MS (4800) /* 5s minus 200ms margin */
 
@@ -132,7 +132,7 @@ enum {
 	FT260_FLAG_START_STOP_REPEATED	= 0x07,
 };
 
-#define FT260_SET_REQUEST_VALUE(report_id) ((FT260_FEATURE << 8) | report_id)
+#define FT260_SET_REQUEST_VALUE(report_id) ((FT260_FEATURE << 8) | (report_id))
 
 /* Feature In reports */
 
-- 
2.35.1

Re: [PATCH tty-next v2 1/3] hid-ft260: Cleanup macro formatting

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1111 bytes --]

On Mon, 26 Dec 2022, Christina Quast wrote:

> Wrap macro arguments in braces.
> 
> Signed-off-by: Christina Quast <contact@christina-quast.de>
> ---
>  drivers/hid/hid-ft260.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> index 333341e80b0e..52a63b966ebc 100644
> --- a/drivers/hid/hid-ft260.c
> +++ b/drivers/hid/hid-ft260.c
> @@ -29,7 +29,7 @@ MODULE_PARM_DESC(debug, "Toggle FT260 debugging messages");
>  	} while (0)
>  
>  #define FT260_REPORT_MAX_LENGTH (64)
> -#define FT260_I2C_DATA_REPORT_ID(len) (FT260_I2C_REPORT_MIN + (len - 1) / 4)
> +#define FT260_I2C_DATA_REPORT_ID(len) (FT260_I2C_REPORT_MIN + ((len) - 1) / 4)
>  
>  #define FT260_WAKEUP_NEEDED_AFTER_MS (4800) /* 5s minus 200ms margin */
>  
> @@ -132,7 +132,7 @@ enum {
>  	FT260_FLAG_START_STOP_REPEATED	= 0x07,
>  };
>  
> -#define FT260_SET_REQUEST_VALUE(report_id) ((FT260_FEATURE << 8) | report_id)
> +#define FT260_SET_REQUEST_VALUE(report_id) ((FT260_FEATURE << 8) | (report_id))

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH tty-next v2 1/3] hid-ft260: Cleanup macro formatting

[Greg KH]

On Mon, Dec 26, 2022 at 06:15:47PM +0100, Christina Quast wrote:
> Wrap macro arguments in braces.

This says _what_ you did here, but no explaination of _why_ you did
this, or why it was even needed.

Please read the section entitled "The canonical patch format" in the
kernel file, Documentation/process/submitting-patches.rst for what is
needed in order to properly describe the change.

thanks,

greg k-h

[PATCH tty-next v2 2/3] hid-ft260: Rename struct ft260_i2c_input_report

[Christina Quast]

Rename ft260_i2c_input_report to ft260_input_report, because depending
on the ft260 device configuration, this device can implement a UART or
I2C functionality.

Signed-off-by: Christina Quast <contact@christina-quast.de>
---
 drivers/hid/hid-ft260.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index 52a63b966ebc..a67051f379a3 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -220,8 +220,8 @@ struct ft260_i2c_read_request_report {
 	__le16 length;		/* data payload length */
 } __packed;
 
-struct ft260_i2c_input_report {
-	u8 report;		/* FT260_I2C_REPORT */
+struct ft260_input_report {
+	u8 report;		/* FT260_I2C_REPORT or FT260_UART_REPORT */
 	u8 length;		/* data payload length */
 	u8 data[2];		/* data payload */
 } __packed;
@@ -1066,7 +1066,7 @@ static int ft260_raw_event(struct hid_device *hdev, struct hid_report *report,
 			   u8 *data, int size)
 {
 	struct ft260_device *dev = hid_get_drvdata(hdev);
-	struct ft260_i2c_input_report *xfer = (void *)data;
+	struct ft260_input_report *xfer = (void *)data;
 
 	if (xfer->report >= FT260_I2C_REPORT_MIN &&
 	    xfer->report <= FT260_I2C_REPORT_MAX) {
-- 
2.35.1

[PATCH tty-next v2 3/3] hid-ft260: Change u8 to __u8 for hw facing structs

[Christina Quast]

Structures that come from a device should use __u8 instead of u8
for their elements. Therefore change all elements in the HID report
structs from u8 to __u8.

Signed-off-by: Christina Quast <contact@christina-quast.de>
---
 drivers/hid/hid-ft260.c | 92 ++++++++++++++++++++---------------------
 1 file changed, 46 insertions(+), 46 deletions(-)

diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index a67051f379a3..c2d1ded4a86b 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -137,93 +137,93 @@ enum {
 /* Feature In reports */
 
 struct ft260_get_chip_version_report {
-	u8 report;		/* FT260_CHIP_VERSION */
-	u8 chip_code[4];	/* FTDI chip identification code */
-	u8 reserved[8];
+	__u8 report;		/* FT260_CHIP_VERSION */
+	__u8 chip_code[4];	/* FTDI chip identification code */
+	__u8 reserved[8];
 } __packed;
 
 struct ft260_get_system_status_report {
-	u8 report;		/* FT260_SYSTEM_SETTINGS */
-	u8 chip_mode;		/* DCNF0 and DCNF1 status, bits 0-1 */
-	u8 clock_ctl;		/* 0 - 12MHz, 1 - 24MHz, 2 - 48MHz */
-	u8 suspend_status;	/* 0 - not suspended, 1 - suspended */
-	u8 pwren_status;	/* 0 - FT260 is not ready, 1 - ready */
-	u8 i2c_enable;		/* 0 - disabled, 1 - enabled */
-	u8 uart_mode;		/* 0 - OFF; 1 - RTS_CTS, 2 - DTR_DSR, */
+	__u8 report;		/* FT260_SYSTEM_SETTINGS */
+	__u8 chip_mode;		/* DCNF0 and DCNF1 status, bits 0-1 */
+	__u8 clock_ctl;		/* 0 - 12MHz, 1 - 24MHz, 2 - 48MHz */
+	__u8 suspend_status;	/* 0 - not suspended, 1 - suspended */
+	__u8 pwren_status;	/* 0 - FT260 is not ready, 1 - ready */
+	__u8 i2c_enable;		/* 0 - disabled, 1 - enabled */
+	__u8 uart_mode;		/* 0 - OFF; 1 - RTS_CTS, 2 - DTR_DSR, */
 				/* 3 - XON_XOFF, 4 - No flow control */
-	u8 hid_over_i2c_en;	/* 0 - disabled, 1 - enabled */
-	u8 gpio2_function;	/* 0 - GPIO,  1 - SUSPOUT, */
+	__u8 hid_over_i2c_en;	/* 0 - disabled, 1 - enabled */
+	__u8 gpio2_function;	/* 0 - GPIO,  1 - SUSPOUT, */
 				/* 2 - PWREN, 4 - TX_LED */
-	u8 gpioA_function;	/* 0 - GPIO, 3 - TX_ACTIVE, 4 - TX_LED */
-	u8 gpioG_function;	/* 0 - GPIO, 2 - PWREN, */
+	__u8 gpioA_function;	/* 0 - GPIO, 3 - TX_ACTIVE, 4 - TX_LED */
+	__u8 gpioG_function;	/* 0 - GPIO, 2 - PWREN, */
 				/* 5 - RX_LED, 6 - BCD_DET */
-	u8 suspend_out_pol;	/* 0 - active-high, 1 - active-low */
-	u8 enable_wakeup_int;	/* 0 - disabled, 1 - enabled */
-	u8 intr_cond;		/* Interrupt trigger conditions */
-	u8 power_saving_en;	/* 0 - disabled, 1 - enabled */
-	u8 reserved[10];
+	__u8 suspend_out_pol;	/* 0 - active-high, 1 - active-low */
+	__u8 enable_wakeup_int;	/* 0 - disabled, 1 - enabled */
+	__u8 intr_cond;		/* Interrupt trigger conditions */
+	__u8 power_saving_en;	/* 0 - disabled, 1 - enabled */
+	__u8 reserved[10];
 } __packed;
 
 struct ft260_get_i2c_status_report {
-	u8 report;		/* FT260_I2C_STATUS */
-	u8 bus_status;		/* I2C bus status */
+	__u8 report;		/* FT260_I2C_STATUS */
+	__u8 bus_status;		/* I2C bus status */
 	__le16 clock;		/* I2C bus clock in range 60-3400 KHz */
-	u8 reserved;
+	__u8 reserved;
 } __packed;
 
 /* Feature Out reports */
 
 struct ft260_set_system_clock_report {
-	u8 report;		/* FT260_SYSTEM_SETTINGS */
-	u8 request;		/* FT260_SET_CLOCK */
-	u8 clock_ctl;		/* 0 - 12MHz, 1 - 24MHz, 2 - 48MHz */
+	__u8 report;		/* FT260_SYSTEM_SETTINGS */
+	__u8 request;		/* FT260_SET_CLOCK */
+	__u8 clock_ctl;		/* 0 - 12MHz, 1 - 24MHz, 2 - 48MHz */
 } __packed;
 
 struct ft260_set_i2c_mode_report {
-	u8 report;		/* FT260_SYSTEM_SETTINGS */
-	u8 request;		/* FT260_SET_I2C_MODE */
-	u8 i2c_enable;		/* 0 - disabled, 1 - enabled */
+	__u8 report;		/* FT260_SYSTEM_SETTINGS */
+	__u8 request;		/* FT260_SET_I2C_MODE */
+	__u8 i2c_enable;		/* 0 - disabled, 1 - enabled */
 } __packed;
 
 struct ft260_set_uart_mode_report {
-	u8 report;		/* FT260_SYSTEM_SETTINGS */
-	u8 request;		/* FT260_SET_UART_MODE */
-	u8 uart_mode;		/* 0 - OFF; 1 - RTS_CTS, 2 - DTR_DSR, */
+	__u8 report;		/* FT260_SYSTEM_SETTINGS */
+	__u8 request;		/* FT260_SET_UART_MODE */
+	__u8 uart_mode;		/* 0 - OFF; 1 - RTS_CTS, 2 - DTR_DSR, */
 				/* 3 - XON_XOFF, 4 - No flow control */
 } __packed;
 
 struct ft260_set_i2c_reset_report {
-	u8 report;		/* FT260_SYSTEM_SETTINGS */
-	u8 request;		/* FT260_SET_I2C_RESET */
+	__u8 report;		/* FT260_SYSTEM_SETTINGS */
+	__u8 request;		/* FT260_SET_I2C_RESET */
 } __packed;
 
 struct ft260_set_i2c_speed_report {
-	u8 report;		/* FT260_SYSTEM_SETTINGS */
-	u8 request;		/* FT260_SET_I2C_CLOCK_SPEED */
+	__u8 report;		/* FT260_SYSTEM_SETTINGS */
+	__u8 request;		/* FT260_SET_I2C_CLOCK_SPEED */
 	__le16 clock;		/* I2C bus clock in range 60-3400 KHz */
 } __packed;
 
 /* Data transfer reports */
 
 struct ft260_i2c_write_request_report {
-	u8 report;		/* FT260_I2C_REPORT */
-	u8 address;		/* 7-bit I2C address */
-	u8 flag;		/* I2C transaction condition */
-	u8 length;		/* data payload length */
-	u8 data[FT260_WR_DATA_MAX]; /* data payload */
+	__u8 report;		/* FT260_I2C_REPORT */
+	__u8 address;		/* 7-bit I2C address */
+	__u8 flag;		/* I2C transaction condition */
+	__u8 length;		/* data payload length */
+	__u8 data[FT260_WR_DATA_MAX]; /* data payload */
 } __packed;
 
 struct ft260_i2c_read_request_report {
-	u8 report;		/* FT260_I2C_READ_REQ */
-	u8 address;		/* 7-bit I2C address */
-	u8 flag;		/* I2C transaction condition */
+	__u8 report;		/* FT260_I2C_READ_REQ */
+	__u8 address;		/* 7-bit I2C address */
+	__u8 flag;		/* I2C transaction condition */
 	__le16 length;		/* data payload length */
 } __packed;
 
 struct ft260_input_report {
-	u8 report;		/* FT260_I2C_REPORT or FT260_UART_REPORT */
-	u8 length;		/* data payload length */
-	u8 data[2];		/* data payload */
+	__u8 report;		/* FT260_I2C_REPORT or FT260_UART_REPORT */
+	__u8 length;		/* data payload length */
+	__u8 data[2];		/* data payload */
 } __packed;
 
 static const struct hid_device_id ft260_devices[] = {
-- 
2.35.1

Re: [PATCH tty-next v2 3/3] hid-ft260: Change u8 to __u8 for hw facing structs

[Johan Hovold]

On Mon, Dec 26, 2022 at 06:15:49PM +0100, Christina Quast wrote:
> Structures that come from a device should use __u8 instead of u8
> for their elements. Therefore change all elements in the HID report
> structs from u8 to __u8.

No, this is not correct. You only need to use __u8 in header files that
are shared with user space.

Johan

Re: [PATCH tty-next v2 3/3] hid-ft260: Change u8 to __u8 for hw facing structs

[Johan Hovold]

On Tue, Dec 27, 2022 at 10:32:25AM +0100, Christina Quast wrote:
> Hi Johan!
> 
> Thanks for your review. I added this change, because that was GKH's
> comment for the struct ft260_configure_uart_request in e previous
> review of the commit:
> 
> And as this is a structure that comes from the device, you should be
> using __u8 and friends.
> 
> So you are sure I should remove this change from the patchset? We want
> to continue using u8?

Yes, I believe Greg is mistaken here. He may prefer that style for some
reason, but there's really no need to go about changing existing drivers
to use __u8 for something which is not shared with user space.

Side note: your mailer seems to be sending mails with HTML which will be
rejected by the mailing lists.

> On 12/27/22 09:45, Johan Hovold wrote:
> > On Mon, Dec 26, 2022 at 06:15:49PM +0100, Christina Quast wrote:
> >> Structures that come from a device should use __u8 instead of u8
> >> for their elements. Therefore change all elements in the HID report
> >> structs from u8 to __u8.
> > No, this is not correct. You only need to use __u8 in header files that
> > are shared with user space.

Johan