Hi Linus,
On Wed, Mar 18, 2015 at 01:33:02PM +0100, Linus Walleij wrote:
> 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.
Good point. Will do.
> > + 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.
The .get_function_groups callback in pinmux_ops expect a char array pointer in
*groups. That is currently implemented as
*groups = pmap->pin_names;
*num_groups = PINS_COUNT;
How can I implement that without a persistent char pointers array, where all I
have is the .name fields scattered in a big pinctrl_pin_desc array?
> > + 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.
OK.
> 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;
>
> ?
I had to have the char pointers array anyway, as explained above. If you have
a better suggestion to that problem I will happily implement it as you suggest
here.
Thanks for reviewing, and for applying all my other little documentation
update patches.
baruch
--
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- baruch@xxxxxxxxxx - tel: +972.2.679.5364, http://www.tkos.co.il -
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
