Re: [PATCH 2/3] pinctrl: ralink: add a pinctrl driver for the rt2880 family

[Sergio Paracuellos <sergio.paracuellos@xxxxxxxxx>]

Hi Dan,

Thanks for the review.

On Tue, Dec 8, 2020 at 11:17 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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