lkml.org 
[lkml]   [2023]   [Feb]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v4 2/2] pinctrl: pinctrl-mlxbf: Add pinctrl driver support
    On Fri, Feb 17, 2023 at 11:27 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote:
    >
    > NVIDIA BlueField-3 SoC has a few pins that can be used as GPIOs
    > or take the default hardware functionality. Add a driver for
    > the pinmuxing.

    pin muxing

    ...

    > +++ b/drivers/pinctrl/pinctrl-mlxbf.c

    Wondering if it would be better to match the GPIO driver naming
    schema, i.e. by adding 3. In this case the additional explanation in
    Kconfig help won't be necessary.

    ...

    > +#define MLXBF_GPIO0_FW_CONTROL_SET 0
    > +#define MLXBF_GPIO0_FW_CONTROL_CLEAR 0x14
    > +#define MLXBF_GPIO1_FW_CONTROL_SET 0x80
    > +#define MLXBF_GPIO1_FW_CONTROL_CLEAR 0x94

    Unclear if these are commands or register offsets. If they are of the
    same type (semantically), make them fixed width, e.g., 0x00.

    ...

    > +enum {
    > + MLXBF_GPIO_HW_MODE,
    > + MLXBF_GPIO_SW_MODE

    I would leave a comma here as it might be extended in the future.

    > +};

    ...

    > +static const char * const mlxbf_pinctrl_single_group_names[] = {
    > + "gpio0", "gpio1", "gpio2", "gpio3", "gpio4", "gpio5", "gpio6",
    > + "gpio7", "gpio8", "gpio9", "gpio10", "gpio11", "gpio12", "gpio13",
    > + "gpio14", "gpio15", "gpio16", "gpio17", "gpio18", "gpio19", "gpio20",
    > + "gpio21", "gpio22", "gpio23", "gpio24", "gpio25", "gpio26", "gpio27",
    > + "gpio28", "gpio29", "gpio30", "gpio31", "gpio32", "gpio33", "gpio34",
    > + "gpio35", "gpio36", "gpio37", "gpio38", "gpio39", "gpio40", "gpio41",
    > + "gpio42", "gpio43", "gpio44", "gpio45", "gpio46", "gpio47", "gpio48",
    > + "gpio49", "gpio50", "gpio51", "gpio52", "gpio53", "gpio54", "gpio55"

    Ditto.
    Can you group by 8?

    > +};
    > +
    > +/* Set of pin numbers for single-pin groups */
    > +static const unsigned int mlxbf_pinctrl_single_group_pins[] = {
    > + 0, 1, 2, 3, 4, 5, 6,
    > + 7, 8, 9, 10, 11, 12, 13,
    > + 14, 15, 16, 17, 18, 19, 20,
    > + 21, 22, 23, 24, 25, 26, 27,
    > + 28, 29, 30, 31, 32, 33, 34,
    > + 35, 36, 37, 38, 39, 40, 41,
    > + 42, 43, 44, 45, 46, 47, 48,
    > + 49, 50, 51, 52, 53, 54, 55,

    Group by 8 which is the more natural length of subarray per line.

    Is it just 1:1 to the index? If so, why do you need this table at all?

    > +};

    ...

    > +static const struct {
    > + const char *name;
    > + const char * const *group_names;

    Use this instead
    https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/tree/include/linux/pinctrl/pinctrl.h?h=devel#n215
    and this
    https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/tree/include/linux/pinctrl/pinctrl.h?h=devel#n222

    > +} mlxbf_pmx_funcs[] = {

    > +};

    ...

    > +{
    > + struct mlxbf_pinctrl *priv = pinctrl_dev_get_drvdata(pctldev);
    > +
    > + /* disable GPIO functionality by giving control back to hardware */
    > + if (offset < MLXBF_NGPIOS_GPIO0)
    > + writel(BIT(offset), priv->base + MLXBF_GPIO0_FW_CONTROL_CLEAR);
    > + else
    > + writel(BIT(offset % MLXBF_NGPIOS_GPIO0), priv->base + MLXBF_GPIO1_FW_CONTROL_CLEAR);

    > +

    Redundant blank line.

    > +}

    ...

    > +static_assert(ARRAY_SIZE(mlxbf_pinctrl_single_group_names) ==
    > + ARRAY_SIZE(mlxbf_pinctrl_single_group_pins));

    I would put it on a single line, but it's up to you.

    ...

    > + struct resource *res;

    Useless?

    ...

    > + /* This resource is shared so use devm_ioremap */

    Can you elaborate on who actually requests the region? And why is it
    not _this_ driver?

    > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
    > + if (!res)
    > + return -ENODEV;

    ...

    > + ret = devm_pinctrl_register_and_init(priv->dev,

    Is the priv->dev different from dev?

    > + &mlxbf_pin_desc,
    > + priv,
    > + &priv->pctl);
    > + if (ret)
    > + return dev_err_probe(dev, ret, "Failed to register pinctrl\n");

    ...

    > + pinctrl_add_gpio_range(priv->pctl, &mlxbf_pinctrl_gpio_ranges[0]);
    > + pinctrl_add_gpio_range(priv->pctl, &mlxbf_pinctrl_gpio_ranges[1]);

    pinctrl_add_gpio_ranges() ?

    --
    With Best Regards,
    Andy Shevchenko

    \
     
     \ /
      Last update: 2023-03-27 00:27    [W:3.592 / U:0.892 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site