Hi Dan, Thanks for the review.
On Tue, Dec 8, 2020 at 11:17 AM Dan Carpenter <dan.carpen...@oracle.com> wrote: > > On Mon, Dec 07, 2020 at 08:21:03PM +0100, Sergio Paracuellos wrote: > > +static struct pinctrl_desc rt2880_pctrl_desc = { > > + .owner = THIS_MODULE, > > + .name = "rt2880-pinmux", > > + .pctlops = &rt2880_pctrl_ops, > > + .pmxops = &rt2880_pmx_group_ops, > > +}; > > + > > +static struct rt2880_pmx_func gpio_func = { > > + .name = "gpio", > > +}; > > + > > +static int rt2880_pinmux_index(struct rt2880_priv *p) > > > This function name is not great. I assumed that it would return the > index. > > > +{ > > + struct rt2880_pmx_func **f; > > Get rid of this "f" variable and use "p->func" instead. > > > + struct rt2880_pmx_group *mux = p->groups; > > + int i, j, c = 0; > > + > > + /* count the mux functions */ > > + while (mux->name) { > > + p->group_count++; > > + mux++; > > + } > > + > > + /* allocate the group names array needed by the gpio function */ > > + p->group_names = devm_kcalloc(p->dev, p->group_count, > > + sizeof(char *), GFP_KERNEL); > > + if (!p->group_names) > > + return -1; > > Return proper error codes in this function. s/-1/-ENOMEM/ > > > + > > + for (i = 0; i < p->group_count; i++) { > > + p->group_names[i] = p->groups[i].name; > > + p->func_count += p->groups[i].func_count; > > + } > > + > > + /* we have a dummy function[0] for gpio */ > > + p->func_count++; > > + > > + /* allocate our function and group mapping index buffers */ > > + f = p->func = devm_kcalloc(p->dev, > > + p->func_count, > > + sizeof(*p->func), > > + GFP_KERNEL); > > + gpio_func.groups = devm_kcalloc(p->dev, p->group_count, sizeof(int), > > + GFP_KERNEL); > > + if (!f || !gpio_func.groups) > > + return -1; > > + > > + /* add a backpointer to the function so it knows its group */ > > + gpio_func.group_count = p->group_count; > > + for (i = 0; i < gpio_func.group_count; i++) > > + gpio_func.groups[i] = i; > > + > > + f[c] = &gpio_func; > > + c++; > > + > > + /* add remaining functions */ > > + for (i = 0; i < p->group_count; i++) { > > + for (j = 0; j < p->groups[i].func_count; j++) { > > + f[c] = &p->groups[i].func[j]; > > + f[c]->groups = devm_kzalloc(p->dev, sizeof(int), > > + GFP_KERNEL); > > Add a NULL check. > > > + f[c]->groups[0] = i; > > + f[c]->group_count = 1; > > + c++; > > + } > > + } > > + return 0; > > +} > > + > > +static int rt2880_pinmux_pins(struct rt2880_priv *p) > > +{ > > + int i, j; > > + > > + /* > > + * loop over the functions and initialize the pins array. > > + * also work out the highest pin used. > > + */ > > + for (i = 0; i < p->func_count; i++) { > > + int pin; > > + > > + if (!p->func[i]->pin_count) > > + continue; > > + > > + p->func[i]->pins = devm_kcalloc(p->dev, > > + p->func[i]->pin_count, > > + sizeof(int), > > + GFP_KERNEL); > > This can fit on two lines. > > p->func[i]->pins = devm_kcalloc(p->dev, p->func[i]->pin_count, > sizeof(int), GFP_KERNEL); > > > + for (j = 0; j < p->func[i]->pin_count; j++) > > + p->func[i]->pins[j] = p->func[i]->pin_first + j; > > + > > + pin = p->func[i]->pin_first + p->func[i]->pin_count; > > + if (pin > p->max_pins) > > + p->max_pins = pin; > > + } > > + > > + /* the buffer that tells us which pins are gpio */ > > + p->gpio = devm_kcalloc(p->dev, p->max_pins, sizeof(u8), GFP_KERNEL); > > + /* the pads needed to tell pinctrl about our pins */ > > + p->pads = devm_kcalloc(p->dev, p->max_pins, > > + sizeof(struct pinctrl_pin_desc), GFP_KERNEL); > > + if (!p->pads || !p->gpio) { > > + dev_err(p->dev, "Failed to allocate gpio data\n"); > > Delete this error message. #checkpatch.pl > > > + return -ENOMEM; > > + } > > + > > + memset(p->gpio, 1, sizeof(u8) * p->max_pins); > > + for (i = 0; i < p->func_count; i++) { > > + if (!p->func[i]->pin_count) > > + continue; > > + > > + for (j = 0; j < p->func[i]->pin_count; j++) > > + p->gpio[p->func[i]->pins[j]] = 0; > > + } > > + > > + /* pin 0 is always a gpio */ > > + p->gpio[0] = 1; > > + > > + /* set the pads */ > > + for (i = 0; i < p->max_pins; i++) { > > + /* strlen("ioXY") + 1 = 5 */ > > + char *name = devm_kzalloc(p->dev, 5, GFP_KERNEL); > > + > > char *name; > > name = kasprintf(GFP_KERNEL, "io%d", i); > > > > + if (!name) > > + return -ENOMEM; > > + snprintf(name, 5, "io%d", i); > > + p->pads[i].number = i; > > + p->pads[i].name = name; > > + } > > + p->desc->pins = p->pads; > > + p->desc->npins = p->max_pins; > > + > > + return 0; > > +} > > + > > +static int rt2880_pinmux_probe(struct platform_device *pdev) > > +{ > > + struct rt2880_priv *p; > > + struct pinctrl_dev *dev; > > + > > + if (!rt2880_pinmux_data) > > + return -ENOTSUPP; > > + > > + /* setup the private data */ > > + p = devm_kzalloc(&pdev->dev, sizeof(struct rt2880_priv), GFP_KERNEL); > > + if (!p) > > + return -ENOMEM; > > + > > + p->dev = &pdev->dev; > > + p->desc = &rt2880_pctrl_desc; > > + p->groups = rt2880_pinmux_data; > > + platform_set_drvdata(pdev, p); > > + > > + /* init the device */ > > + if (rt2880_pinmux_index(p)) { > > + dev_err(&pdev->dev, "failed to load index\n"); > > + return -EINVAL; > > Preserve the error code: > > err = rt2880_pinmux_index(p); > if (err) { > dev_err(&pdev->dev, "failed to load index\n"); > return err; > } > > > > + } > > + if (rt2880_pinmux_pins(p)) { > > + dev_err(&pdev->dev, "failed to load pins\n"); > > + return -EINVAL; > > Same. > > > + } > > + dev = pinctrl_register(p->desc, &pdev->dev, p); > > + if (IS_ERR(dev)) > > + return PTR_ERR(dev); > > + > > + return 0; > > +} This is already applied but needs more work in its correct place afterwards. So I will take into account all of these comments and will send patches to properly address all of them. > > regards, > dan carpenter Thanks again. Best regards, Sergio Paracuellos _______________________________________________ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel