[U-Boot] [PATCH 01/15] pinctrl: uniphier: add UniPhier pinctrl core support

[Masahiro Yamada]

2015-09-02 11:48 GMT+09:00 Simon Glass <sjg at chromium.org>:

>> diff --git a/drivers/pinctrl/uniphier/pinctrl-uniphier.h b/drivers/pinctrl/uniphier/pinctrl-uniphier.h
>> new file mode 100644
>> index 0000000..db74838
>> --- /dev/null
>> +++ b/drivers/pinctrl/uniphier/pinctrl-uniphier.h
>> @@ -0,0 +1,57 @@
>> +/*
>> + * Copyright (C) 2015 Masahiro Yamada <yamada.masahiro at socionext.com>
>> + *
>> + * SPDX-License-Identifier:    GPL-2.0+
>> + */
>> +
>> +#ifndef __PINCTRL_UNIPHIER_H__
>> +#define __PINCTRL_UNIPHIER_H__
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/types.h>
>> +
>> +#define UNIPHIER_PINCTRL_PINMUX_BASE   0x0
>> +#define UNIPHIER_PINCTRL_LOAD_PINMUX   0x700
>> +#define UNIPHIER_PINCTRL_IECTRL                0xd00
>
> Since this is local data you don't really need the UNIPHIER prefix,
> but it's up to you.

I want to make sure that these macros are not global ones, but our
SoC-specific ones.

I prefer to add some prefixes.


>> +
>> +struct uniphier_pmx_data {
>
> comments please on these structures.

I will think of that, but I believe this is also up to me.

These are driver-local structures,
so comments for each structure are not the requirement.
Moreover, most of them have clear names.

At least, when I sent some driver patches to Linux, I was never told
to add comment blocks to driver-specific structures.



>> +       unsigned pin;
>> +       unsigned muxval;
>> +};
>> +
>> +struct uniphier_pinctrl_group {
>> +       const char *name;
>> +       const struct uniphier_pmx_data *pmx_data;
>> +       unsigned num_pmx_data;
>> +};
>> +
>> +struct uniphier_pinctrl_socdata {
>> +       const struct uniphier_pinctrl_group *groups;
>> +       int groups_count;
>> +       const char * const *functions;
>> +       int functions_count;
>> +       unsigned mux_bits;
>> +       unsigned reg_stride;
>> +       bool load_pinctrl;
>> +};
>> +
>> +#define UNIPHIER_PINCTRL_GROUP(grp)                    \
>> +       {                                               \
>> +               .name = #grp,                           \
>> +               .pmx_data = grp##_pmx,                  \
>> +               .num_pmx_data = ARRAY_SIZE(grp##_pmx),  \
>> +       }
>> +
>> +struct uniphier_pinctrl_priv {
>> +       void __iomem *base;
>> +       struct uniphier_pinctrl_socdata *socdata;
>> +};
>> +
>> +extern const struct pinctrl_ops uniphier_pinctrl_ops;
>
> It's a shame this cannot be static...


Indeed.




-- 
Best Regards
Masahiro Yamada