[U-Boot] [PATCH 2/4] x86: tangier: pinmux: add API to configure protected pins

[Andy Shevchenko]

On Mon, Sep 3, 2018 at 7:39 PM Georgii Staroselskii
<georgii.staroselskii at emlid.com> wrote:
>
> This API is going to be used to configure some pins that are protected
> for simple modification.
>
> It's not a comprehensive pinctrl driver but can be turned into one
> when we need this in the future. Now it is planned to be used only
> in one place. So that's why I decided not to polute the codebase with a

polute -> pollute

> full-blown pinctrl-merrifield nobody will use.
>
> This driver reads corresponding fields in DT and configures pins
> accordingly.
>
> The "protected" flag is used to distinguish configuration of SCU-owned
> pins from the ordinary ones.
>
> The code has been adapted from Linux work done by Andy Shevchenko
> in pinctrl-merrfifield.c


> +static struct mrfld_family mrfld_families[] = {
> +       MRFLD_FAMILY(7, 101, 114),
> +};

This perhaps needs a comment that for now we are only serve I2C Family of pins.

> +static const struct mrfld_family *
> +mrfld_get_family(struct mrfld_pinctrl *mp, unsigned int pin)
> +{
> +       const struct mrfld_family *family;
> +       unsigned int i;
> +
> +       for (i = 0; i < mp->nfamilies; i++) {
> +               family = &mp->families[i];

> +               if (pin >= family->pin_base &&
> +                       pin < family->pin_base + family->npins)

Can it be one line?

> +                       return family;
> +       }


> +       printf("failed to find family for pin %u\n", pin);

I think it should be an error message (I don't remember which function
is good for that, pr_err() maybe?).

> +       return NULL;
> +}


> +       return ret;
> +}

Missed blank line here.

> +static int mrfld_pinctrl_cfg_pin(int pin_node)
> +{

> +       is_protected = fdtdec_get_bool(gd->fdt_blob, pin_node, "protected");
> +       if (!is_protected)
> +               return -ENOTSUPP;

Similar comment perhaps needed, that it's for now we support just
protected Family of pins, i.e. I2C one.

> +       if (mode != 1)
> +               return -ENOTSUPP;

I don't think we need to limit to mode 1. Rather to check for mask (if
mode is in a range 0..7).

> +       u32 mask = MRFLD_PINMODE_MASK;

I'm not sure mixing definitions with code is a good idea.

> +       ret = mrfld_pinconfig_protected(pad_offset, mask, mode);
> +
> +       return ret;
> +}
> +
> +static int tangier_pinctrl_probe(struct udevice *dev)
> +{
> +       void *base_addr = syscon_get_first_range(X86_SYSCON_PINCONF);
> +       struct mrfld_pinctrl *pinctrl = dev_get_priv(dev);
> +       int pin_node;
> +       int ret;
> +
> +       mrfld_setup_families(base_addr, mrfld_families,
> +                            ARRAY_SIZE(mrfld_families));
> +
> +       pinctrl->families = mrfld_families;
> +       pinctrl->nfamilies = ARRAY_SIZE(mrfld_families);
> +
> +       for (pin_node = fdt_first_subnode(gd->fdt_blob, dev_of_offset(dev));
> +            pin_node > 0;
> +            pin_node = fdt_next_subnode(gd->fdt_blob, pin_node)) {
> +               ret = mrfld_pinctrl_cfg_pin(pin_node);

> +               if (ret != 0) {

Simple if (ret) should work.

> +                       pr_err("%s: invalid configuration for the pin %d\n",
> +                             __func__, pin_node);
> +               }
> +       }
> +
> +       return 0;
> +}
> +

> +static const struct udevice_id tangier_pinctrl_match[] = {
> +       { .compatible = "intel,pinctrl-tangier", .data = X86_SYSCON_PINCONF },
> +       { /* sentinel */ }
> +};

Side note: I don't know for sure naming standards for compatible
strings, I guess this one is correct.

> +U_BOOT_DRIVER(tangier_pinctrl) = {
> +       .name = "tangier_pinctrl",
> +       .id = UCLASS_SYSCON,
> +       .of_match = tangier_pinctrl_match,
> +       .probe = tangier_pinctrl_probe,
> +       .priv_auto_alloc_size = sizeof(struct mrfld_pinctrl),
> +};

-- 
With Best Regards,
Andy Shevchenko