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@xxxxxxxxxxx> > --- > 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
