Re: [PATCH] msm: misc: enable QRC support on msm-5.4 on RB3-Gen2

[Greg Kroah-Hartman]

On Thu, Nov 09, 2023 at 02:39:32PM +0800, zhengjia zhu via B4 Relay wrote:
> From: zhengjia zhu <quic_zhezhu@xxxxxxxxxxx>
> 
> QRC Driver support functions:
> - Read data from serial device port.
> - Write data to serial device port.
> - Pin control reset robotic controller.
> 
> Signed-off-by: zhengjia zhu <quic_zhezhu@xxxxxxxxxxx>
> ---
>  drivers/misc/qrc/Kconfig    |  25 ++++
>  drivers/misc/qrc/Makefile   |   9 ++
>  drivers/misc/qrc/qrc_core.c | 312 +++++++++++++++++++++++++++++++++++++++
>  drivers/misc/qrc/qrc_core.h | 142 ++++++++++++++++++
>  drivers/misc/qrc/qrc_uart.c | 352 ++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 840 insertions(+)
> 
> diff --git a/drivers/misc/qrc/Kconfig b/drivers/misc/qrc/Kconfig
> new file mode 100644
> index 000000000000..59f734c02092
> --- /dev/null
> +++ b/drivers/misc/qrc/Kconfig
> @@ -0,0 +1,25 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# QRC device driver configuration
> +#
> +
> +menu "qrc device driver"
> +
> +config QRC
> +	bool "QRC device driver for Robotic SDK MCU"

Why is this not able to be built as a module?

> +	help
> +	   This kernel configuration is used to enable robotic controller
> +	   device driver. Say Y here if you want to enable robotic
> +	   controller device driver.
> +	   When in doubt, say N.

What is the module name?

> +
> +config QRC_DEBUG
> +	bool "QRC Debugging"
> +	depends on QRC
> +	help
> +	  Say Y here if you want the robotic controller to produce
> +	  a bunch of debug messages to the system log. Select this if you
> +	  are having a problem with robotic controller support and want
> +	  to see more of what is going on.
> +	  When in doubt, say N.

No, please use the built-in kernel debugging functionality, which is NOT
a separate configuration option.  Nothing would work if we had this on a
per-driver level that had to be set at build time.  Use the normal
dynamic debugging logic and all will be fine.

> +endmenu
> diff --git a/drivers/misc/qrc/Makefile b/drivers/misc/qrc/Makefile
> new file mode 100644
> index 000000000000..d811992aa0d9
> --- /dev/null
> +++ b/drivers/misc/qrc/Makefile
> @@ -0,0 +1,9 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# Makefile for the QRC bus specific drivers.
> +
> +
> +obj-$(CONFIG_QRC)		+=  qrc_core.o qrc_uart.o
> +
> +
> +#ccflags-$(CONFIG_QRC_DEBUG) := -DDEBUG
> diff --git a/drivers/misc/qrc/qrc_core.c b/drivers/misc/qrc/qrc_core.c
> new file mode 100644
> index 000000000000..9d03f35e9683
> --- /dev/null
> +++ b/drivers/misc/qrc/qrc_core.c
> @@ -0,0 +1,312 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* driver/misc/qrc/qrc_core.c
> + *
> + * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/types.h>
> +#include <linux/sched.h>
> +#include <linux/init.h>
> +#include <linux/cdev.h>
> +#include <linux/slab.h>
> +#include <linux/poll.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_device.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +#include <linux/gpio.h>
> +#include <linux/delay.h>
> +
> +#include "qrc_core.h"
> +
> +#define FIFO_CLEAR 0x1
> +
> +#define QRC_DEVICE_NAME "qrc"
> +
> +static dev_t qrc_devt;
> +static struct class *qrc_class;
> +
> +static int qrc_cdev_fasync(int fd, struct file *filp, int mode)
> +{
> +	struct qrc_dev *qrc;
> +
> +	qrc = filp->private_data;
> +	return fasync_helper(fd, filp, mode, &qrc->async_queue);
> +}
> +
> +static int qrc_cdev_open(struct inode *inode, struct file *filp)
> +{
> +	struct qrc_dev *qrc;
> +
> +	qrc = container_of(inode->i_cdev,
> +						struct qrc_dev, cdev);

Very odd formatting, where did this come from?

Have you run checkpatch.pl on your patch?

> +	filp->private_data = qrc;
> +	if (qrc->qrc_ops != NULL)
> +		qrc->qrc_ops->qrcops_open(qrc);
> +	return 0;
> +}
> +
> +static int qrc_cdev_release(struct inode *inode, struct file *filp)
> +{
> +	struct qrc_dev *qrc;
> +
> +	qrc = filp->private_data;
> +	if (qrc->qrc_ops != NULL)
> +		qrc->qrc_ops->qrcops_close(qrc);
> +
> +	return 0;
> +}
> +
> +/* GPIO control  */
> +static int
> +qrc_control_gpio_init(struct qrc_dev *qdev, struct device_node *node)
> +{
> +	int ret;
> +
> +	/* QRC BOOT0 GPIO */
> +	qdev->qrc_boot0_gpio = of_get_named_gpio(node,
> +	"qcom,qrc-boot-gpio", 0);

Again, odd formatting.

> +	if (qdev->qrc_boot0_gpio < 0)
> +		pr_err("qrc_boot0_gpio is not available\n");

You are a driver, you should always use dev_err() and friends, never
pr_err().

> +
> +	/* UART RESET GPIO */
> +	qdev->qrc_reset_gpio = of_get_named_gpio(node,
> +	"qcom,qrc-reset-gpio", 0);
> +	if (qdev->qrc_reset_gpio < 0)
> +		pr_err("qrc_reset_gpio is not available\n");
> +
> +	if (gpio_is_valid(qdev->qrc_boot0_gpio)) {
> +		ret = gpio_request(qdev->qrc_boot0_gpio, "QRC_BOOT0_GPIO");
> +		if (unlikely(ret)) {

Unless you can measure it in a benchmark, never use likely/unlikely as
the compiler and cpu will do it better without it.

I've stopped reviewing here, please take some time to fix up the driver
based on this and resubmit.

thanks,

greg k-h