[PATCH v3 2/2] pinctrl: Add driver for Sunplus SP7021
呂芳騰
wellslutw at gmail.com
Fri Dec 10 10:33:14 PST 2021
Resend the email because it was rejected due to wrong format!
Hi Linus,
Thank you very much for your review.
Please see my answers below:
> Hi Wells,
>
> this is improving! Keep working on this driver. I now naturally have
> more comments :)
>
> On Tue, Dec 7, 2021 at 5:17 AM Wells Lu <wellslutw at gmail.com> wrote:
>
> > +static void sppctl_func_set(struct sppctl_pdata *pctl, u8 func, u8 val)
> > +{
> > + u32 reg, offset;
> > +
> > + /* Note that upper 16-bit word is mask
> > + * and lower 16-bit word is value.
> > + * Enable mask before write.
> > + */
> > + reg = 0x007f0000 | val; /* Set value and enable mask. */
>
> Define these types of masks and use them like this:
>
> #include <linux/bits.h>
>
> #define SPPCTL_FUNC_MASK GENMASK(22, 16)
>
> Also switch the order with the mask to the right please:
>
> reg = val & SPPCTL_FUNC_MASK;
Yes, I'll modify code next patch.
> > + if (func & 1)
>
> I would write
>
> #define SSPCTL_FUNC_FLAG BIT(0)
>
> if (func & SSPCTL_FUNC_FLAG)
>
> Use the name that bit has in your documentation for the
> define so we know what is going on.
Actually, 'if (func & 1)' is not used to test bit 0,
but test 'func' is an odd number or not.
If 'func' is even number, control-field is at bit 6 ~ 0.
Its corresponding mask-field is at bit 22 ~ 16.
If 'func' is odd number, control-field is at bit 14 ~ 8.
Its corresponding mask-field is at bit 30 ~ 24.
Control and mask fields of 'func' are arranged as shown
below:
func # | register control-field mask-field
-------+------------------------------------
0 | reg[0] ( 6:0) (22 : 6)
1 | reg[0] (14:8) (30 : 24)
2 | reg[1] ( 6:0) (22 : 6)
3 | reg[1] (14:8) (30 : 24)
> > + reg <<= 8;
>
> Likewise
>
> #define SSPCTL_FUNC_UPPER_SHIFT 8
>
> reg <<= SSPCTL_FUNC_UPPER_SHIFT;
> Can also be a comment. The general idea is to break out as many
> of these magic numbers as possible to #defines and give them
> some names from the reference manual, so we understand them
> instead of the numbers being magic.
Yes, I'll modify codes, add defines and more comments next patch.
> > + /* Convert function # to register offset. */
> > + offset = func & ~1;
>
> Step 1 write:
> offset = func & GENMASK(31, 1);
>
> > + offset <<= 1;
>
> I would write:
> offset *= 2;
> because we are dealing with an offset and not an arithmetic
> operation. It will be the same to the compiler.
>
> But the best is to just merge all this and write (if I'm not wrong):
>
> #include <linux/bitfield.h>
>
> /*
> * Bit 1 .. 31 gives the function, index this into a 32-bit offset by
> * multiplying by four to find the register.
> */
> offset = FIELD_GET(GENMASK(31, 1), func);
> offset *= 4;
>
> This gets pretty clear. We see that we remove BIT(0) and use the
> rest as offset index and there are four bytes per register.
>
> (Beware of bugs in my pseudo code, check it!)
Thank you for example code!
Yes, I will use BIT, GENMASK, FIELD_GET, and FIELD_PREP
macros next patch.
> > +static u8 sppctl_func_get(struct sppctl_pdata *pctl, u8 func)
> > +{
> > + u32 reg, offset;
> > + u8 val;
> > +
> > + /* Convert function # to register offset. */
> > + offset = func & ~1;
> > + offset <<= 1;
>
> Same comments.
Yes, I got it. I'll modify codes next patch.
> > + reg = readl(pctl->moon2_base + offset);
> > + if (func & 1)
> > + val = reg >> 8;
> > + else
> > + val = reg;
> > + val &= 0x7f;
>
> #define SSPCTL_*_MASK for this 0x7f so we understand it.
Yes, I'll do it next patch.
> > +static void sppctl_gmx_set(struct sppctl_pdata *pctl, u8 reg_off, u8 bit_off, u8 bit_sz,
> > + u8 val)
> > +{
> > + u32 mask, reg;
> > +
> > + /* Note that upper 16-bit word is mask
> > + * and lower 16-bit word is value.
> > + * Enable mask before write.
> > + */
> > + mask = ~(~0 << bit_sz);
> > + reg = (mask << 16) | (val & mask);
> > + reg <<= bit_off;
>
> Please familiarize yourself with <linux/bitfield.h> and use things like
> FIELD_PREP() for this (I think, at least).
Yes, I will use BIT, GENMASK, FIELD_GET, and FIELD_PREP
macros next patch.
> > +static int sppctl_first_get(struct gpio_chip *chip, unsigned int offset)
> > +{
> > + struct sppctl_gpio_chip *spp_gchip = gpiochip_get_data(chip);
> > + u32 reg;
> > +
> > + reg = readl(spp_gchip->first_base + SPPCTL_GPIO_OFF_FIRST + R32_ROF(offset));
>
> So R32_ROF() is register offset.
Yes, it converts 'offset' to register offset (w.r.t. base register)
R32_ROF() is for 32-bit width registers
R16_ROF() is for 16-bit width registers (higher 16-bit of the register is mask).
> > +
> > + dev_dbg(chip->parent, "%s(%u): addr = %p, reg = %08x, val = %d\n",
> > + __func__, offset, spp_gchip->first_base + SPPCTL_GPIO_OFF_FIRST +
> > + R32_ROF(offset), reg, (int)R32_VAL(reg, R32_BOF(offset)));
> > +
> > + return R32_VAL(reg, R32_BOF(offset));
>
> And R32_BOF is register bit offset.
>
> I think these macros just make it hard to read because the reader has to
> go to another file and look it up and then figure out what does ROF and
> BOF actually mean (no explanation given).
>
> I would just inline the stuff.
>
> u32 reg = (offset / 32) * 4;
> u32 bit = offset % 32;
>
> reg = readl(spp_gchip->first_base + SPPCTL_GPIO_OFF_FIRST + reg);
>
> // Some debug code
>
> return !!(reg & BIT(bit));
Yes, I'll modify codes to follow your suggestions next patch.
> > +static void sppctl_gpio_output_inv_set(struct gpio_chip *chip, unsigned int offset)
> > +{
> > + struct sppctl_gpio_chip *spp_gchip = gpiochip_get_data(chip);
> > + u32 reg;
> > +
> > + /* Upper 16-bit word is mask. Lower 16-bit word is value. */
> > + reg = (BIT(R16_BOF(offset)) << 16) | BIT(R16_BOF(offset));
> > + writel(reg, spp_gchip->gpioxt2_base + SPPCTL_GPIO_OFF_OINV + R16_ROF(offset));
> > +}
>
> Same comments about the BOF and ROF.
>
> This layout with "mask and value" in registers needs to be explained
> somewhere it looks complex. I don't understand why a machine register
> contains a mask for example.
This is a hardware mechanism for protecting some important registers from
being overwritten accidentally. The corresponding mask bit should be set
first and then the control-bits or fields can be written. The design is
originally requested from car makers.
> > +static int stpctl_set_mux(struct pinctrl_dev *pctldev, unsigned int func_selector,
> > + unsigned int group_selector)
> > +{
> > + const struct sppctl_func *f = &sppctl_list_funcs[func_selector];
> > + struct sppctl_pdata *pctl = pinctrl_dev_get_drvdata(pctldev);
> > + struct grp2fp_map g2fpm = pctl->g2fp_maps[group_selector];
> > + int i = -1, j = -1;
>
> Please do not initialize loop variable i to -1, just declare it.
Yes, I'll remove the initialization for loop variables next patch
> > + dev_dbg(pctldev->dev, "%s(func: %d, grp: %d)\n", __func__,
> > + func_selector, group_selector);
> > +
> > + switch (f->freg) {
> > + case f_off_0: /* GPIO. detouch from all funcs - ? */
> > + for (i = 0; i < sppctl_list_funcs_sz; i++) {
> > + if (sppctl_list_funcs[i].freg != f_off_m)
> > + continue;
> > + j++;
>
> Insert a comment that j is set to -1 so this will be zero here after the first
> iteration.
Yes, I'll add comments next patch.
> > + if (sppctl_func_get(pctl, j) != group_selector)
> > + continue;
> > + sppctl_func_set(pctl, j, 0);
> > + }
> > + break;
> > +
> > + case f_off_m: /* Mux */
> > + sppctl_first_master_set(&pctl->spp_gchip->chip, group_selector,
> > + mux_f_mux, mux_m_keep);
> > + sppctl_func_set(pctl, func_selector - 2, (group_selector == 0) ?
> > + group_selector : group_selector - 7);
>
> -2 and -7? Why? Add some comments or maybe #define these
> constants?
Yes, I'll defines and add some comments for the magic numbers
next patch.
> > +static int stpctl_gpio_request_enable(struct pinctrl_dev *pctldev,
> > + struct pinctrl_gpio_range *range, unsigned int offset)
> > +{
> > + struct sppctl_pdata *pctl = pinctrl_dev_get_drvdata(pctldev);
> > + struct pin_desc *pdesc;
> > + int g_f, g_m;
> > +
> > + dev_dbg(pctldev->dev, "%s(%d)\n", __func__, offset);
> > +
> > + g_f = sppctl_first_get(&pctl->spp_gchip->chip, offset);
> > + g_m = sppctl_master_get(&pctl->spp_gchip->chip, offset);
> > + if (g_f == mux_f_gpio && g_m == mux_m_gpio)
> > + return 0;
> > +
> > + pdesc = pin_desc_get(pctldev, offset);
> > + if (pdesc->mux_owner)
> > + return -EACCES;
>
> Do not reimplement the pinmux core please.
>
> What you want to achieve here is "strict pinmux", i.e. setting the field
> "strict" in struct pinmux_ops to true. Then you can just delete this
> check.
Yes, I'll remove the if-statement next patch.
> > +static const struct pinmux_ops sppctl_pinmux_ops = {
> > + .request = stpctl_request,
> > + .free = stpctl_free,
>
> These are just set to empty functions. Delete these entries
> and the empty functions.
Yes, I'll remove all 'empty' functions next patch.
> > + .get_functions_count = stpctl_get_functions_count,
> > + .get_function_name = stpctl_get_function_name,
> > + .get_function_groups = stpctl_get_function_groups,
> > + .set_mux = stpctl_set_mux,
> > + .gpio_request_enable = stpctl_gpio_request_enable,
> > + .gpio_disable_free = stpctl_gpio_disable_free,
> > + .gpio_set_direction = stpctl_gpio_set_direction,
> > + .strict = 1
>
> Use "true" rather than 1. (And do not reimplement the check.)
Yes, I'll do it next patch.
> > +static int sppctl_remove(struct platform_device *pdev)
> > +{
> > + struct sppctl_pdata *sppctl = pdev->dev.platform_data;
> > +
> > + devm_pinctrl_unregister(&pdev->dev, sppctl->pctl_dev);
>
> This defies the idea with devm_* calls. Drop remove() entirely because
> devm_ allocated resources go away by themselves.
Yes, I'll remove devm_pinctrl_unregister() next patch.
Sorry for buggy code. Fortunately, pinctrl driver
will never be removed.
> > +++ b/drivers/pinctrl/sunplus/sppctl.h
> (...)
> > +/* (/16)*4 */
> > +#define R16_ROF(r) (((r) >> 4) << 2)
> > +#define R16_BOF(r) ((r) % 16)
> > +/* (/32)*4 */
> > +#define R32_ROF(r) (((r) >> 5) << 2)
> > +#define R32_BOF(r) ((r) % 32)
>
> As mentioned I prefer explicit inlined code for these.
> The bit shifting here makes it really hard to know what is going
> on, the compiler will get it right if you use the right types
> and just write (n / 32) * 4. Please do not try to help the compiler
> optimizing it just leads to code that is hard to read.
>
> > +#define R32_VAL(r, boff) (((r) >> (boff)) & BIT(0))
>
> To check the value of a certain bit use this pattern:
>
> if (val & BIT(n))
>
> To return a boolean clamped bit (return 0/1) do this idiom:
>
> return !!(val & BIT(n));
Yes, I'll remove the four macros.
Write explicit inline code.
> Other than these things I didn't notice anything more this
> time, but I might find even more stuff, but hey it's getting there!
> Yours,
> Linus Walleij
Thanks, I'll review the whole code again and fix
the similar improper coding you pointed out.
Best regards,
Wells Lu
More information about the linux-arm-kernel
mailing list