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

[Simon Glass]

Hi Masahiro,

On 1 September 2015 at 21:30, Masahiro Yamada
<yamada.masahiro at socionext.com> wrote:
> 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.

That might be because Linux is allergic to comments. Everyone is
supposed to know already (presumably from birth) how things work :-)

Some of them I can guess, but others are not clear - e.g. mux_bits,
reg_stride, load_pinctrl... I think it is better that our coding
standard requires comments.

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

Regards,
Simon