Re: [PATCH v4 1/2] pinctrl: Add driver for Alphascale asm9260 pinctrl

[Oleksij Rempel]

Am 05.05.2015 um 17:12 schrieb Linus Walleij:
> 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.

This two arrays have different types this is why i convert it.
priv->pin_desc[i].name - here i copy pointer any ways, and
priv->pin_desc[i].number can be smaller then pointer.

>> +/* 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.

see #define MUX_TABLE_SIZE          ARRAY_SIZE(asm9260_mux_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?

Yes, each pin can be configured separately.

>> +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.

I hadn't seen the point to define groups statically, especially because
they are used only  to make curious user happy. So, memory will be used
only if you request the list over sysfs. Or miss some thing?

>> +       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.

I fear it would cause unpredictable bugs. GPIO mode is just one of mux
modes. If some one will request gpio some busy or dangerous line it
would do more harm then use. So, i assume limiting this only to device
tree would be better.

> 
>> +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...

Ok, i agree.

> Apart from that it looks OK.
> 
> BTW this is a review of v3, I didn't find your v4 of this patch :/

I'll wait for answers first, then provide next version. Thank you for
review :)

-- 
Regards,
Oleksij

Attachment: signature.asc
Description: OpenPGP digital signature