Received-SPF: pass (google.com: domain of linux-kernel+bounces-89842-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Sun, 3 Mar 2024 18:17:55 +0100
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Canfeng Zhuang <quic_czhuang@quicinc.com>
Cc: Derek Kiernan <derek.kiernan@amd.com>,
	Dragan Cvetic <dragan.cvetic@amd.com>,
	Arnd Bergmann <arnd@arndb.de>,
	Bjorn Andersson <andersson@kernel.org>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
	linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
	devicetree@vger.kernel.org
Subject: Re: [PATCH 1/2] misc: qualcomm: QRC driver for Robotic SDK MCU
Message-ID: <2024030343-protrude-unlocking-31ac@gregkh>
References: <20240304-qcom_qrc-v1-0-2a709f95fd61@quicinc.com>
 <20240304-qcom_qrc-v1-1-2a709f95fd61@quicinc.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240304-qcom_qrc-v1-1-2a709f95fd61@quicinc.com>

On Mon, Mar 04, 2024 at 12:53:16AM +0800, Canfeng Zhuang wrote:
> QRC Driver support functions:

What is "QRC"?

> - Read data from serial device port.
> - Write data to serial device port.

Shouldn't you be doing that in userspace?  Why is this a kernel driver?

> - Pin control reset robotic controller.
> 
> Signed-off-by: Canfeng Zhuang <quic_czhuang@quicinc.com>
> ---
>  drivers/misc/Kconfig        |   1 +
>  drivers/misc/Makefile       |   1 +
>  drivers/misc/qrc/Kconfig    |  16 ++
>  drivers/misc/qrc/Makefile   |   6 +
>  drivers/misc/qrc/qrc_core.c | 336 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/misc/qrc/qrc_core.h | 143 ++++++++++++++++++

Why do you have a .h file for a single driver?

>  drivers/misc/qrc/qrc_uart.c | 345 ++++++++++++++++++++++++++++++++++++++++++++
>  7 files changed, 848 insertions(+)
> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 4fb291f0bf7c..a43108af6fde 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -591,4 +591,5 @@ source "drivers/misc/cardreader/Kconfig"
>  source "drivers/misc/uacce/Kconfig"
>  source "drivers/misc/pvpanic/Kconfig"
>  source "drivers/misc/mchp_pci1xxxx/Kconfig"
> +source "drivers/misc/qrc/Kconfig"
>  endmenu
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index ea6ea5bbbc9c..ab3b2c4d99fa 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -68,3 +68,4 @@ obj-$(CONFIG_TMR_INJECT)	+= xilinx_tmr_inject.o
>  obj-$(CONFIG_TPS6594_ESM)	+= tps6594-esm.o
>  obj-$(CONFIG_TPS6594_PFSM)	+= tps6594-pfsm.o
>  obj-$(CONFIG_NSM)		+= nsm.o
> +obj-$(CONFIG_QCOM_QRC)		+= qrc/
> diff --git a/drivers/misc/qrc/Kconfig b/drivers/misc/qrc/Kconfig
> new file mode 100644
> index 000000000000..994985d7c320
> --- /dev/null
> +++ b/drivers/misc/qrc/Kconfig
> @@ -0,0 +1,16 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# QRC device driver configuration
> +#
> +
> +menu "QCOM QRC device driver"
> +
> +config QCOM_QRC
> +	tristate "QCOM QRC device driver for Robotic SDK MCU"

What is "QCOM", "QRC", SDK", and "MCU"?

> +	help
> +	  This kernel configuration is used to enable robotic controller
> +	  device driver. Say M here if you want to enable robotic
> +	  controller device driver.

Please write more here, this doesn't really describe what is happening.

> +	  When in doubt, say N.
> +
> +endmenu
> diff --git a/drivers/misc/qrc/Makefile b/drivers/misc/qrc/Makefile
> new file mode 100644
> index 000000000000..da2cf81f3c59
> --- /dev/null
> +++ b/drivers/misc/qrc/Makefile
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# Makefile for the QRC bus specific drivers.
> +
> +
> +obj-$(CONFIG_QCOM_QRC)		+= qrc_core.o qrc_uart.o

Why is a single driver 2 different files?

> diff --git a/drivers/misc/qrc/qrc_core.c b/drivers/misc/qrc/qrc_core.c
> new file mode 100644
> index 000000000000..5cedb050dac4
> --- /dev/null
> +++ b/drivers/misc/qrc/qrc_core.c
> @@ -0,0 +1,336 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* driver/misc/qrc/qrc_core.c

Why is the filename here?

> + *
> + * 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 QRC_DEVICE_NAME "qrc"

KBUILD_MODNAME.

> +static dev_t qrc_devt;
> +static struct class *qrc_class;

Please use class_register(), not class_create().

I've stopped here, please get someone in your group to review this
before sending it out again, you have internal experience with kernel
drivers, please take advantage of it.

thanks,

greg k-h