On Mon, Apr 6, 2015 at 11:04 AM, Oleksij Rempel <linux@xxxxxxxxxxxxxxxx> wrote:
> This patch adds driver for Alphascale asm9260 pinctrl support.
> Alphascale asm9260t is SoC based on ARM926EJ (240MHz) in LQFP176 package.
> On silicon are:
> - 32MB SDRAM
> - USB2.0 HS/OTG
> - 2x CAN
> - SD/MMC
> - 5x Times/PWM
> - 10x USART
> - 24-channel DMA
> - 2x i2c
> - 2x SPI
> - Quad SPI
> - 10/100 Ethernet MAC
> - Camera IF
> - WD
> - RTC
> - i2s
> - GPIO
> - 12-bit A/D
> - LCD IF
> - 8-channel 12-bit ADC
> - NAND
>
> Signed-off-by: Oleksij Rempel <linux@xxxxxxxxxxxxxxxx>
Nice.
> +#define MUX_TABLE_SIZE ARRAY_SIZE(asm9260_mux_table)
> +struct asm9260_pmx_priv {
> + struct device *dev;
> + struct pinctrl_dev *pctl;
> + void __iomem *iobase;
> +
> + struct clk *clk;
> + spinlock_t lock;
> +
> + struct pinctrl_pin_desc pin_desc[MUX_TABLE_SIZE];
> +};
> +
> +static void __init asm9260_init_mux_pins(struct asm9260_pmx_priv *priv)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < MUX_TABLE_SIZE; i++) {
> + priv->pin_desc[i].name = asm9260_mux_table[i].name;
> + priv->pin_desc[i].number = asm9260_mux_table[i].number;
> + }
> +}
What is the point of copying this data from one array to the other?
Just reference the statically defined array by a pointer instead,
this just takes up a lot o memory for no reason.
> +/* each GPIO pin has it's own pseudo pingroup containing only itself */
> +static int asm9260_pinctrl_get_groups_count(struct pinctrl_dev *pctldev)
> +{
> + return MUX_TABLE_SIZE;
> +}
Use return ARRAY_SIZE(foo) to return the size of a static table.
> +static int asm9260_pinctrl_get_group_pins(struct pinctrl_dev *pctldev,
> + unsigned int group,
> + const unsigned int **pins,
> + unsigned int *num_pins)
> +{
> + struct asm9260_pmx_priv *priv = pinctrl_dev_get_drvdata(pctldev);
> +
> + *pins = &priv->pin_desc[group].number;
> + *num_pins = 1;
So I see you are using groups with one pin each. Is this how the
hardware works?
> +static int asm9260_pinctrl_get_func_groups(struct pinctrl_dev *pctldev,
> + unsigned int function,
> + const char * const **groups,
> + unsigned int * const num_groups)
> +{
(...)
> + for (a = 0; a < MUX_TABLE_SIZE; a++) {
> + table = &asm9260_mux_table[a];
> +
> + for (b = 0; b < MAX_FUNCS_PER_PIN; b++) {
> + if (table->funcs[b] == function) {
> + tmp[count] = a;
> + count++;
> + }
> +
> + }
> +
> + }
Mory copying. I don't see why this is necessary at all.
> + for (a = 0; a < count; a++)
> + gr[a] = asm9260_mux_table[tmp[a]].name;
And more copying.
Try to just reference static tables.
> +
> + asm9260_functions[function].groups = gr;
> + asm9260_functions[function].ngroups = count;
> +done:
> + *groups = asm9260_functions[function].groups;
> + *num_groups = asm9260_functions[function].ngroups;
Same comment.
> +static struct pinmux_ops asm9260_pinmux_ops = {
> + .get_functions_count = asm9260_pinctrl_get_funcs_count,
> + .get_function_name = asm9260_pinctrl_get_func_name,
> + .get_function_groups = asm9260_pinctrl_get_func_groups,
> + .set_mux = asm9260_pinctrl_set_mux,
> + /* TODO: should we care about gpios here? gpio_request_enable? */
I think you should, if you also have a matching GPIO driver.
> +static int asm9260_pinconf_reg(struct pinctrl_dev *pctldev,
> + unsigned int pin,
> + enum pin_config_param param,
> + void __iomem **reg, u32 *val)
> +{
> + struct asm9260_pmx_priv *priv = pinctrl_dev_get_drvdata(pctldev);
> + struct asm9260_pingroup *table;
> + int a;
> +
> + for (a = 0; a < MUX_TABLE_SIZE; a++) {
> + table = &asm9260_mux_table[a];
> + if (table->number == pin)
> + break;
> + }
No error check here. What if pin is not in table? We will never
know for that case...
Apart from that it looks OK.
BTW this is a review of v3, I didn't find your v4 of this patch :/
Yours,
Linus Walleij
--
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
