lkml.org 
[lkml]   [2023]   [Jun]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v1 07/43] pinctrl: add a Cirrus ep93xx SoC pin controller
    Thu, Jun 01, 2023 at 08:33:58AM +0300, Nikita Shubin kirjoitti:
    > This adds a pin control (only multiplexing) driver for ep93xx
    > SoC so we can fully convert ep93xx to device tree.
    >
    > This driver is capable of muxing ep9301/ep9302/ep9307/ep9312/ep9315
    > variants, this is chosen based on "compatible" in device tree.

    I have a déjà vu that I commented on this already...

    ...

    > +enum ep93xx_pinctrl_model {
    > + EP93XX_9301_PINCTRL = 0,

    It's not needed, guaranteed by the C standard.

    > + EP93XX_9307_PINCTRL,
    > + EP93XX_9312_PINCTRL

    Keep trailing comma, might help in case of this being extended.

    > +};

    > +/**
    > + * struct ep93xx_pin_group - describes a ep93xx pin group
    > + * @name: the name of this specific pin group
    > + * @pins: an array of discrete physical pins used in this group, taken
    > + * from the driver-local pin enumeration space
    > + * @num_pins: the number of pins in this group array, i.e. the number of
    > + * elements in .pins so we can iterate over that array
    > + * @mask: bits to clear to enable this when doing pin muxing
    > + * @value: bits to set to enable this when doing pin muxing
    > + */
    > +struct ep93xx_pin_group {
    > + const char *name;
    > + const unsigned int *pins;
    > + const unsigned int num_pins;

    Please, use struct pingroup.

    > + u32 mask;
    > + u32 value;
    > +};

    ...

    > +static const struct ep93xx_pin_group ep9301_pin_groups[] = {
    > + {
    > + .name = "ssp",
    > + .pins = ssp_ep9301_pins,
    > + .num_pins = ARRAY_SIZE(ssp_ep9301_pins),

    Use PINCTRL_PINGROUP() respectively.

    > + .mask = EP93XX_SYSCON_DEVCFG_I2SONSSP,
    > + },

    > +};

    ...

    > +static const struct ep93xx_pin_group ep9307_pin_groups[] = {

    Ditto.

    > +};

    ...
    > +static const struct ep93xx_pin_group ep9312_pin_groups[] = {

    Ditto.

    > +};

    ...

    > + switch (pmx->model) {
    > + case EP93XX_9301_PINCTRL:
    > + return ARRAY_SIZE(ep9301_pin_groups);
    > + case EP93XX_9307_PINCTRL:
    > + return ARRAY_SIZE(ep9307_pin_groups);
    > + case EP93XX_9312_PINCTRL:
    > + return ARRAY_SIZE(ep9312_pin_groups);
    > + }
    > +
    > + return 0;

    Simply make it default: case.

    ...

    > + switch (pmx->model) {
    > + case EP93XX_9301_PINCTRL:
    > + return ep9301_pin_groups[selector].name;
    > + case EP93XX_9307_PINCTRL:
    > + return ep9307_pin_groups[selector].name;
    > + case EP93XX_9312_PINCTRL:
    > + return ep9312_pin_groups[selector].name;
    > + }
    > +
    > + return NULL;

    Ditto.

    ...

    > +static void ep93xx_pin_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s,
    > + unsigned int offset)
    > +{
    > + seq_printf(s, " " DRIVER_NAME);

    How this is useful?

    > +}

    ...

    > +static const struct pinctrl_ops ep93xx_pctrl_ops = {
    > + .get_groups_count = ep93xx_get_groups_count,
    > + .get_group_name = ep93xx_get_group_name,
    > + .get_group_pins = ep93xx_get_group_pins,
    > + .pin_dbg_show = ep93xx_pin_dbg_show,

    > + .dt_node_to_map = pinconf_generic_dt_node_to_map_all,
    > + .dt_free_map = pinconf_generic_dt_free_map,

    Missing ifdeffery?

    > +};

    ...

    > +/**
    > + * struct ep93xx_pmx_func - describes ep93xx pinmux functions
    > + * @name: the name of this specific function
    > + * @groups: corresponding pin groups
    > + */
    > +struct ep93xx_pmx_func {
    > + const char *name;
    > + const char * const *groups;
    > + const unsigned int num_groups;
    > +};

    Use struct pinfunction instead.

    ...

    > +static const struct ep93xx_pmx_func ep93xx_pmx_functions[] = {
    > + {
    > + .name = "spi",
    > + .groups = spigrps,
    > + .num_groups = ARRAY_SIZE(spigrps),
    > + },

    And PINCTRL_PINFUNCTION() respectively.

    > +};

    ...

    > + switch (pmx->model) {
    > + case EP93XX_9301_PINCTRL:
    > + grp = &ep9301_pin_groups[group];
    > + break;
    > + case EP93XX_9307_PINCTRL:
    > + grp = &ep9307_pin_groups[group];
    > + break;
    > + case EP93XX_9312_PINCTRL:
    > + grp = &ep9312_pin_groups[group];
    > + break;

    default?

    > + }

    ...

    > + for_each_set_bit(i, &tmp, PADS_MAXBIT) {
    > + bool enabled = expected & BIT(i);
    > +
    > + dev_err(pmx->dev,
    > + "pin group %s could not be %s: probably a hardware limitation\n",
    > + ep93xx_padgroups[i], enabled ? "enabled" : "disabled");

    str_enabled_disabled()

    > + dev_err(pmx->dev,
    > + "DeviceCfg before: %08x, after %08x, expected %08x\n",
    > + before, after, expected);
    > + }

    ...

    > +static const struct of_device_id ep93xx_pinctrl_of_ids[] = {
    > + { .compatible = "cirrus,ep9301-pinctrl", .data = (void *)EP93XX_9301_PINCTRL},
    > + { .compatible = "cirrus,ep9302-pinctrl", .data = (void *)EP93XX_9301_PINCTRL},
    > + { .compatible = "cirrus,ep9307-pinctrl", .data = (void *)EP93XX_9307_PINCTRL},
    > + { .compatible = "cirrus,ep9312-pinctrl", .data = (void *)EP93XX_9312_PINCTRL},
    > + { .compatible = "cirrus,ep9315-pinctrl", .data = (void *)EP93XX_9312_PINCTRL},
    > + {},

    Comma is not needed in the terminator entry.

    > +};

    ...

    > + const struct of_device_id *match = of_match_node(ep93xx_pinctrl_of_ids, pdev->dev.of_node);

    Why? This is an old API, simply use of_device_get_match_data().

    With it you don't need to locate OF ID table too early in the code.

    > + struct ep93xx_pmx *pmx;
    > + struct regmap *map;
    > + struct device *dev = &pdev->dev;
    > + struct device *parent;

    Longer lines first?

    ...

    > + pmx->dev = &pdev->dev;

    ... = dev; ?

    ...

    > + parent = dev->parent;
    > + if (!parent) {
    > + dev_err(dev, "no parent to pin controller\n");
    > + return -ENODEV;

    return dev_err_probe(...);

    > + }

    ...

    > + map = syscon_node_to_regmap(parent->of_node);
    > + if (IS_ERR(map)) {
    > + dev_err(dev, "no syscon regmap\n");
    > + return PTR_ERR(map);

    Ditto.

    > + }

    ...

    > + switch (pmx->model) {
    > + case EP93XX_9301_PINCTRL:
    > + ep93xx_pmx_desc.pins = ep9301_pins;
    > + ep93xx_pmx_desc.npins = ARRAY_SIZE(ep9301_pins);
    > + dev_info(dev, "detected 9301/9302 chip variant\n");
    > + break;
    > + case EP93XX_9307_PINCTRL:
    > + ep93xx_pmx_desc.pins = ep9307_pins;
    > + ep93xx_pmx_desc.npins = ARRAY_SIZE(ep9307_pins);
    > + dev_info(dev, "detected 9307 chip variant\n");
    > + break;
    > + case EP93XX_9312_PINCTRL:
    > + ep93xx_pmx_desc.pins = ep9312_pins;
    > + ep93xx_pmx_desc.npins = ARRAY_SIZE(ep9312_pins);
    > + dev_info(dev, "detected 9312/9315 chip variant\n");
    > + break;

    default?

    > + }

    ...

    > + pmx->pctl = devm_pinctrl_register(dev, &ep93xx_pmx_desc, pmx);
    > + if (IS_ERR(pmx->pctl)) {
    > + dev_err(dev, "could not register pinmux driver\n");
    > + return PTR_ERR(pmx->pctl);

    return dev_err_probe(...);

    > + }

    > + dev_info(dev, "initialized ep93xx pin control driver\n");

    Noise. Please drop it.

    > + return 0;
    > +};

    --
    With Best Regards,
    Andy Shevchenko


    \
     
     \ /
      Last update: 2023-06-03 21:59    [W:4.601 / U:0.052 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site