Re: [PATCH 2/2] pinctrl: driver for Conexant Digicolor CX92755 pin mapping

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Mon, Mar 16, 2015 at 4:21 PM, Baruch Siach <baruch@xxxxxxxxxx> wrote:

> This adds pinctrl and gpio driver to the CX92755 SoC "General Purpose Pin
> Mapping" hardware block. The CX92755 is one SoC from the Conexant Digicolor
> series. Pin mapping hardware supports configuring pins as either GPIO, or up to
> 3 other "client select" functions. This driver adds support for pin muxing
> using the generic device tree binding, and a basic gpiolib driver for the GPIO
> functionality.
>
> This driver does not currently support GPIO interrupts, and pad configuration.
>
> Signed-off-by: Baruch Siach <baruch@xxxxxxxxxx>

(...)


> +struct dc_pinmap {
> +       void __iomem            *regs;
> +       struct device           *dev;
> +       struct pinctrl_dev      *pctl;
> +
> +       struct pinctrl_pin_desc *pins;

Instead of storing just an array of pinctrl_pin_desc
use the .pins in struct pinctrl_desc and store a pointer
to your struct pinctrl_desc in this state container.

> +       const char              *pin_names[PINS_COUNT];

This duplicates names that should already be in the
.pins array in struct pinctrl_desc, don't do that.

> +
> +       struct gpio_chip        chip;
> +       spinlock_t              lock;
> +};

> +static const char *dc_get_group_name(struct pinctrl_dev *pctldev,
> +                                    unsigned selector)
> +{
> +       struct dc_pinmap *pmap = pinctrl_dev_get_drvdata(pctldev);
> +
> +       return pmap->pin_names[selector];
> +}

Add some comment explaining that you have exactly one group per pin.

You are also re-implementing the .pins field in struct pinctrl_desc
where each pin is described by a struct pinctrl_pin_desc with
special macros available to define them and all.

If you want to have one group per pin named like this
why not just

return pmap->desc->pins[selector].name;

?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html