Hello Krzysztof!
On Tue, Dec 31, 2024 at 4:38 PM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
>
> On Wed, Dec 25, 2024 at 11:58:51AM +0800, Chuanhong Guo wrote:
> > From: Qingfang Deng <qingfang.deng@xxxxxxxxxxxxxxx>
> >
> > Add a driver for the GPIO controller on Siflower SoCs.
> > This controller is found on all current Siflower MIPS and RISC-V
> > chips including SF19A2890, SF21A6826 and SF21H8898.
> >
> > Signed-off-by: Qingfang Deng <qingfang.deng@xxxxxxxxxxxxxxx>
> > Signed-off-by: Chuanhong Guo <gch981213@xxxxxxxxx>
> > ---
> > drivers/gpio/Kconfig | 9 +
> > drivers/gpio/Makefile | 1 +
> > drivers/gpio/gpio-siflower.c | 353 +++++++++++++++++++++++++++++++++++
> > 3 files changed, 363 insertions(+)
> > create mode 100644 drivers/gpio/gpio-siflower.c
> >
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index add5ad29a673..fdc9a89ffbf3 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -637,6 +637,15 @@ config GPIO_SIFIVE
> > help
> > Say yes here to support the GPIO device on SiFive SoCs.
> >
> > +config GPIO_SIFLOWER
> > + tristate "SiFlower GPIO support"
> > + depends on OF_GPIO
> > + depends on MIPS || RISCV || COMPILE_TEST
>
> This is supposed to be dependency on ARCH, not instruction set. I don't
> se anything MIPS or RISCV here.
I haven't sent any arch patches yet. The SoCs basically work with
MIPS/RISC-V generic kernel so I was planning to deal with it last with
some device trees.
Should I simply drop this dependency line for now, or should I add
ARCH_xxx to arch/{mips,riscv}/Kconfig first?
>
> > + select GPIOLIB_IRQCHIP
> > + help
> > + GPIO controller driver for SiFlower MIPS and RISC-V SoCs
> > + including SF19A2890, SF21A6826 and SF21H8898.
>
> ...
>
> > +static void sf_gpio_remove(struct platform_device *pdev)
> > +{
> > + struct sf_gpio_priv *priv = platform_get_drvdata(pdev);
> > +
> > + reset_control_assert(priv->rstc);
> > +}
> > +
> > +static const struct of_device_id sf_gpio_ids[] = {
> > + { .compatible = "siflower,sf19a2890-gpio" },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, sf_gpio_ids);
> > +
> > +static struct platform_driver sf_gpio_driver = {
> > + .probe = sf_gpio_probe,
> > + .remove = sf_gpio_remove,
> > + .driver = {
> > + .name = "siflower_gpio",
> > + .owner = THIS_MODULE,
>
> You sent us some old code with old code style, so probably you sent us
> donwstream poor driver.
I'll drop this owner line.
> Please clean it up before posting.
Do you have other specific review comments? I haven't found other
stuff to clean up currently.
> Please run standard kernel tools for static analysis, like coccinelle,
coccinelle failed on something else:
make ARCH=mips CROSS_COMPILE=mipsel-linux-gnu- C=2
CHECK="scripts/coccicheck" drivers/gpio/gpio-siflower.o
[...]
/usr/bin/spatch -D report --no-show-diff --very-quiet --cocci-file
./scripts/coccinelle/misc/secs_to_jiffies.cocci -I ./arch/mips/include
-I ./arch/mips/include/generated -I ./include -I ./include -I
./arch/mips/include/uapi -I ./arch/mips/include/generated/uapi -I
./include/uapi -I ./include/generated/uapi --include
./include/linux/compiler-version.h --include ./include/linux/kconfig.h
scripts/mod/empty.c
virtual rule report not supported
coccicheck failed
After removing ./scripts/coccinelle/misc/secs_to_jiffies.cocci, I got:
[...]
/usr/bin/spatch -D report --no-show-diff --very-quiet --cocci-file
./scripts/coccinelle/api/stream
_open.cocci -I ./arch/mips/include -I ./arch/mips/include/generated -I
./include -I ./include -I .
/arch/mips/include/uapi -I ./arch/mips/include/generated/uapi -I
./include/uapi -I ./include/gener
ated/uapi --include ./include/linux/compiler-version.h --include
./include/linux/kconfig.h drivers
/gpio/gpio-siflower.c
warning: line 140: should noop_llseek be a metavariable?
warning: line 222: should nonseekable_open be a metavariable?
warning: line 289: should nonseekable_open be a metavariable?
warning: line 337: should nonseekable_open be a metavariable?
[...]
These doesn't seem to be related to gpio-siflower.c. There's no
noop_llseek or nonseekable_open in it.
> smatch
smatch give me nothing:
make ARCH=mips CROSS_COMPILE=mipsel-linux-gnu- C=2 CHECK="smatch
-p=kernel" drivers/gpio/gpio-siflower.o
[...]
CC kernel/bounds.s
CC arch/mips/kernel/asm-offsets.s
CALL scripts/checksyscalls.sh
CHKSHA1 include/linux/atomic/atomic-arch-fallback.h
CHKSHA1 include/linux/atomic/atomic-instrumented.h
CHKSHA1 include/linux/atomic/atomic-long.h
CC drivers/gpio/gpio-siflower.o
CHECK drivers/gpio/gpio-siflower.c
> and sparse
sparse doesn't seem to tell me anything about the driver itself either.
make ARCH=mips CROSS_COMPILE=mipsel-linux-gnu- C=2 CHECK="sparse"
drivers/gpio/gpio-siflower.o
CHECK scripts/mod/empty.c
command-line: note: in included file:
builtin:1:9: warning: preprocessor token __ATOMIC_ACQUIRE redefined
builtin:0:0: this was the original definition
builtin:1:9: warning: preprocessor token __ATOMIC_SEQ_CST redefined
builtin:0:0: this was the original definition
builtin:1:9: warning: preprocessor token __ATOMIC_ACQ_REL redefined
builtin:0:0: this was the original definition
builtin:1:9: warning: preprocessor token __ATOMIC_RELEASE redefined
builtin:0:0: this was the original definition
CALL scripts/checksyscalls.sh
CHECK drivers/gpio/gpio-siflower.c
command-line: note: in included file:
builtin:1:9: warning: preprocessor token __ATOMIC_ACQUIRE redefined
builtin:0:0: this was the original definition
builtin:1:9: warning: preprocessor token __ATOMIC_SEQ_CST redefined
builtin:0:0: this was the original definition
builtin:1:9: warning: preprocessor token __ATOMIC_ACQ_REL redefined
builtin:0:0: this was the original definition
builtin:1:9: warning: preprocessor token __ATOMIC_RELEASE redefined
builtin:0:0: this was the original definition
> , and fix reported warnings. Also please check for
> warnings when building with W=1.
I also got nothing from this one:
rm drivers/gpio/gpio-siflower.o && make ARCH=mips
CROSS_COMPILE=mipsel-linux-gnu- W=1 drivers/gpio/gpio-siflower.o
CC scripts/mod/empty.o
MKELF scripts/mod/elfconfig.h
HOSTCC scripts/mod/modpost.o
CC scripts/mod/devicetable-offsets.s
HOSTCC scripts/mod/file2alias.o
HOSTCC scripts/mod/sumversion.o
HOSTCC scripts/mod/symsearch.o
HOSTLD scripts/mod/modpost
CC kernel/bounds.s
CC arch/mips/kernel/asm-offsets.s
CALL scripts/checksyscalls.sh
CC drivers/gpio/gpio-siflower.o
> Most of these commands (checks or W=1
> build) can build specific targets, like some directory, to narrow the
> scope to only your code. The code here looks like it needs a fix.
Would you mind sharing the warnings you found, and the command to
reproduce those?
> Feel
> free to get in touch if the warning is not clear.
>
> Best regards,
> Krzysztof
>
--
Regards,
Chuanhong Guo
