RE: [PATCH v2 1/3] pinctrl: Add driver for Sunplus SP7021

[Wells Lu 呂芳騰 <wells.lu@xxxxxxxxxxx>]

Hi Linus,

Thanks for review and sorry for late reply.


> Hi Wells Lu,
> 
> thanks for your patch!
> 
> This driver needs a bit of work, I will point out some things and I think it will be quite
> different if we also change the bindings.
> 
> On Mon, Nov 1, 2021 at 9:11 AM Wells Lu <wellslutw@xxxxxxxxx> wrote:
> 
> > +config PINCTRL_SPPCTL
> > +       bool "Sunplus SP7021 pinmux and GPIO driver"
> > +       depends on SOC_SP7021
> > +       depends on OF && HAS_IOMEM
> > +       select PINMUX
> > +       select GENERIC_PINCTRL_GROUPS
> > +       select GENERIC_PINMUX_FUNCTIONS
> > +       select PINCONF
> > +       select GENERIC_PINCONF
> > +       select OF_GPIO
> > +       select GPIOLIB
> > +       select GPIO_SYSFS
> 
> Don't do this, sysfs is deprecated.

Yes, I'll remove 'select GPIO_SYSFS' in next patch.


> > +obj-$(CONFIG_PINCTRL_SPPCTL) += sppctl.o
> > +obj-$(CONFIG_PINCTRL_SPPCTL) += sppctl_pinctrl.o
> > +obj-$(CONFIG_PINCTRL_SPPCTL) += sppctl_sysfs.o
> > +obj-$(CONFIG_PINCTRL_SPPCTL) += sppctl_gpio_ops.o
> > +obj-$(CONFIG_PINCTRL_SPPCTL) += sppctl_gpio.o
> > +obj-$(CONFIG_PINCTRL_SPPCTL) += pinctrl_inf_sp7021.o
> > +obj-$(CONFIG_PINCTRL_SPPCTL) += gpio_inf_sp7021.o
> 
> This multitide of files makes this a bit hard to read and review, usually pin controllers
> are in one-two files for a single SoC.

Yes, I'll re-arrange files into two files in next patch.

sppctl.c sppctl.h	-- for SoC-irrelevant code.
sppctl_sp7021.c		-- for SoC-relevant code.

Is this arrangement ok?


> > + * This program is free software; you can redistribute it and/or
> > + modify
> > + * it under the terms of the GNU General Public License as published
> > + by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> 
> Drop this boilerplate on all files and just use the SPDX tag.

Yes, I'll remove them in next patch.


> > +const size_t GPIS_listSZ =
> > +sizeof(sppctlgpio_list_s)/sizeof(*(sppctlgpio_list_s));
> 
> Use only lowercase in variable names.

Yes, I'll check and use lowercase for all variable names in next patch.


> This looks like a reimplementation of ARRAY_SIZE(), replace with that if this is the case.

Yes, I'll replace the statement with using ARRAY_SIZE() in next patch.

 
> > +const size_t sppctlpins_allSZ = ARRAY_SIZE(sppctlpins_all);
> 
> Instead of defining consts for random sizes like this, just inline ARRAY_SIZE() where you
> use it.

The array 'sppctlpins_all' is defined without explicit size.
ARRAY_SIZE won't work if it is not placed with define (within the same file).


> > +// gpio: is defined in gpio_inf_sp7021.c const size_t PMUX_listSZ =
> > +sizeof(sppctlpmux_list_s)/sizeof(*(sppctlpmux_list_s));
> 
> Same comment as above. Etc.

Yes, I'll replace the statement with using ARRAY_SIZE() in next patch.


> > +/* CEC pin is not used. Release it for others. */ //static const
> > +unsigned int pins_hdmi1[] = { D(10, 6), D(10, 7), D(12, 2), D(12, 1)
> > +}; //static const unsigned int pins_hdmi2[] = { D(8, 3), D(8, 4),
> > +D(8, 5), D(8, 6) }; //static const unsigned int pins_hdmi3[] = { D(7,
> > +4), D(7, 5), D(7, 6), D(7, 7) };
> 
> Don't leave commented-out code in the driver. Delete all this stuff.

Yes, I'll remove them in next patch.


> > +void print_device_tree_node(struct device_node *node, int depth) {
> > +       int i = 0;
> > +       struct device_node *child;
> > +       struct property    *properties;
> > +       char                indent[255] = "";
> > +
> > +       for (i = 0; i < depth * 3; i++)
> > +               indent[i] = ' ';
> > +       indent[i] = '\0';
> > +
> > +       ++depth;
> > +       if (depth == 1) {
> > +               pr_info("%s{ name = %s\n", indent, node->name);
> > +               for (properties = node->properties; properties != NULL;
> > +                       properties = properties->next)
> > +                       pr_info("%s  %s (%d)\n", indent, properties->name,
> properties->length);
> > +               pr_info("%s}\n", indent);
> > +       }
> > +
> > +       for_each_child_of_node(node, child) {
> > +               pr_info("%s{ name = %s\n", indent, child->name);
> > +               for (properties = child->properties; properties != NULL;
> > +                       properties = properties->next)
> > +                       pr_info("%s  %s (%d)\n", indent, properties->name,
> properties->length);
> > +               print_device_tree_node(child, depth);
> > +               pr_info("%s}\n", indent);
> > +       }
> > +}
> 
> This kind of debugging code should be deleted or use what is in the device tree core.

Yes, I'll remove them in next patch.
Could you please teach me which function I can use to print dt-node in core?


> > +void sppctl_gmx_set(struct sppctl_pdata_t *_p, uint8_t _roff, uint8_t _boff, uint8_t
> _bsiz,
> > +                   uint8_t _rval)
> > +{
> > +       uint32_t *r;
> 
> Don't use uint8_t or uint16_t or uint32_t, use the kernel short forms u8, u16 or u32, simply.

Yes, I'll replace them with kernel short forms u8, u16, u32 in next patch.


> Don't start any variable names with _underscore, it i a big confusion for the head because
> it has ambigous semantics.
> 
> Try to find concise descriptive variable names.

Yes, I'll review variables name with underscore prefix and give proper name in next patch


> > +       struct sppctl_reg_t x = { .m = (~(~0 << _bsiz)) << _boff,
> > +                                 .v = ((uint16_t)_rval) << _boff };
> > +
> > +       if (_p->debug > 1)
> > +               KDBG(_p->pcdp->dev, "%s(x%X,x%X,x%X,x%X) m:x%X v:x%X\n",
> > +                    __func__, _roff, _boff, _bsiz, _rval, x.m, x.v);
> 
> Do not reinvent kernel debugging use the dev_dbg() macro.

Yes, I'll replace KDBG with  dev_dbg() in next patch.


> > +       r = (uint32_t *)&x;
> 
> Try to avoid casting like this. It is usually a sign that something is wrong.

Yes, I'll review and remove the casting in next patch.


> 
> > +       if (_fun % 2 == 0)
> > +               ;
> > +       else {
> > +               x.v <<= 8;
> > +               x.m <<= 8;
> > +       }
> 
> This is code that is incredibly terse and deviant from the kernels general style. Please
> read a few other pin control drivers and familiarize with how these drivers usually look.

Yes, I'll study other drivers and modify them.


> > +uint8_t sppctl_fun_get(struct sppctl_pdata_t *_p,  uint8_t _fun) {
> > +       uint8_t pin = 0x00;
> > +       uint8_t func = (_fun >> 1) << 2;
> 
> This looks like shifting to get rid of bit 0.
> Just use bitwise logic instead.

Yes, I'll replace 'shift' with bitwise logic operation in next patch.


> > +       ret = request_firmware_nowait(THIS_MODULE, true, _fwname, _dev, GFP_KERNEL, p,
> > +                                     sppctl_fwload_cb);
> 
> So this pin controller needs a firmware? That is the first time I have ever seen that.
> Please add comments describing what this firmware is and what it does, also explain it
> in the commit message.

No, it restores pins settings from a file.
The driver supports saving pin-settings to a file and restoring pin-settings from a file.


> > +int sppctl_pctl_resmap(struct platform_device *_pd, struct
> > +sppctl_pdata_t *_pc) {
> > +       struct resource *rp;
> > +
> > +       // resF
> > +       rp = platform_get_resource(_pd, IORESOURCE_MEM, 0);
> > +       if (IS_ERR(rp)) {
> > +               KERR(&(_pd->dev), "%s get res#F ERR\n", __func__);
> > +               return PTR_ERR(rp);
> > +       }
> > +       KDBG(&(_pd->dev), "mres #F:%p\n", rp);
> 
> Thes resF etc are very terse and hard to understand. It seems written by someone who knows
> everything of what they are doing but with very little interest to explain it to others.
> Code readability is important.

Yes, I'll rename the variable name to improve code readability in next patch.


> > +static struct platform_driver sppctl_driver = {
> > +       .driver = {
> > +               .name           = MNAME,
> 
> Don't abbreviate so compulsively.
> SP7021_MODULE_NAME is fine.

Yes, I'll rename the defines to SPPCTL_MODULE_NAME in next patch.


> > +static int __init sppctl_drv_reg(void) {
> > +       return platform_driver_register(&sppctl_driver);
> > +}
> > +postcore_initcall(sppctl_drv_reg);
> 
> Why do you need a postcore_initcall()?
> 

postcore_initcall() is added here to make sure pinctrl driver 
will be loaded before other drivers which need pin settings.
With moving pinctrl driver earlier, HDMI-TX driver will be
loaded earlier.


> > +MODULE_AUTHOR(M_AUT1);
> > +MODULE_AUTHOR(M_AUT2);
> > +MODULE_DESCRIPTION(M_NAM);
> > +MODULE_LICENSE(M_LIC);
> 
> Just inline the strings, all other drivers do.

Yes, I'll use fixed string directly here in next patch.


> > +#define MNAME "sppctl"
> > +#define M_LIC "GPL v2"
> > +#define M_AUT1 "Dvorkin Dmitry <dvorkin@xxxxxxxxx>"
> > +#define M_AUT2 "Wells Lu <wells.lu@xxxxxxxxxxx>"
> > +#define M_NAM "SP7021 PinCtl"
> > +#define M_ORG "Sunplus/Tibbo Tech."
> > +#define M_CPR "(C) 2020"
> 
> This is too much and too abbreviated names, just use the strings directly in the macros.

Yes, I'll do it in next patch.


> > +#include <linux/version.h>
> 
> Why?

I'll remove the inclusion in next patch.
It is indeed not necessary.


> > +#include <linux/of_gpio.h>
> 
> Never use this include in new code. It is legacy.

I'll remove the inclusion in next patch.


> > +#define SPPCTL_MAX_NAM 64
> > +#define SPPCTL_MAX_BUF PAGE_SIZE
> > +
> > +#define KINF(pd, fmt, args...) \
> > +       do { \
> > +               if ((pd) != NULL) \
> > +                       dev_info((pd), fmt, ##args); \
> > +               else \
> > +                       pr_info(MNAME ": " fmt, ##args); \
> > +       } while (0)
> > +#define KERR(pd, fmt, args...) \
> > +       do { \
> > +               if ((pd) != NULL) \
> > +                       dev_info((pd), fmt, ##args); \
> > +               else \
> > +                       pr_err(MNAME ": " fmt, ##args); \
> > +       } while (0)
> > +#ifdef CONFIG_PINCTRL_SPPCTL_DEBUG
> > +#define KDBG(pd, fmt, args...) \
> > +       do { \
> > +               if ((pd) != NULL) \
> > +                       dev_info((pd), fmt, ##args); \
> > +               else \
> > +                       pr_debug(MNAME ": " fmt, ##args); \
> > +       } while (0)
> > +#else
> > +#define KDBG(pd, fmt, args...)
> > +#endif
> 
> Don't reimplement kernel debugging use dev_dbg(), dev_info()
> dev_err() etc directly. I don't see why you need CONFIG_PINCTRL_SPPCTL_DEBUG at all, if
> you absolutely want to control debugging for these files only just use this in your Makefile
> 
> subdir-ccflags-$(CONFIG_PINCTRL_SPPCTL_DEBUG)  := -DDEBUG
> 
> This will turn on/off the output from dev_dbg().

Yes, I'll remove KINF, KERR, KDB macros and use kernel debugging 
functions dev_info(), dev_err(), dev_info() in next patch.


> > +struct sppctl_pdata_t {
> > +       char name[SPPCTL_MAX_NAM];
> > +       uint8_t debug;
> 
> Don't use u8 for things like this use bool.

Yes, I'll remove 'debug' member and customized debug function 
will be removed.


> > +       char fwname[SPPCTL_MAX_NAM];
> > +       void *sysfs_sdp;
> > +       void __iomem *baseF;    // functions
> > +       void __iomem *base0;    // MASTER , OE , OUT , IN
> > +       void __iomem *base1;    // I_INV , O_INV , OD
> > +       void __iomem *base2;    // GPIO_FIRST
> > +       void __iomem *baseI;    // IOP
> > +       // pinctrl-related
> > +       struct pinctrl_desc pdesc;
> > +       struct pinctrl_dev *pcdp;
> > +       struct pinctrl_gpio_range gpio_range;
> > +       struct sppctlgpio_chip_t *gpiod;
> 
> *gpiod is a bad name because we use it quite a lot in the kernel for GPIO descriptors.

Yes, I'll rename 'gpiod' with '' in next patch.


> > +struct sppctl_reg_t {
> > +       uint16_t v;     // value part
> > +       uint16_t m;     // mask part
> > +};
> 
> These are not types (no typedef) so don't add *_t suffixes, just drop those everywhere.

Yes, I'll remove all _t suffixes in next patch.


> > +       const char * const name;
> > +       const uint8_t gval;             // value for register
> > +       const unsigned * const pins;    // list of pins
> > +       const unsigned int pnum;        // number of pins
> 
> Use kerneldoc to document struct members.
> 
> There will be many more comments but work on these things to begin with!

I never use kerneldoc. Where should I put my vendor-specified kernel doc?


> 
> Yours,
> Linus Walleij

Thank you very much for your review.

Best regards,
Wells