Re: [PATCH 01/11] pinctrl: mvebu: pinctrl driver core

[Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>]

On 8/20/12, Linus Walleij <linus.walleij@linaro.org> wrote:
> Are you taking this patch series through some Marvell tree or do you want
> me to carry it in the pinctrl tree?

Hi Linus,

I think it would be better to take it through the Marvell tree of Jason
Cooper. It is only for Marvell SoCs anyway.

>> +uart1: serial@12100 {
>> +       compatible = "ns16550a";
>> +       reg = <0x12100 0x100>;
>> +       reg-shift = <2>;
>> +       interrupts = <7>;
>> +       clock-frequency = <166666667>;
>
> It's got nothing to do with this patch, but getting a clock frequency
> out of the DT instead of getting it from the clk_get_rate(clk) and
> the clock tree seems absurd... (But maybe this platform does not
> even have a clk implementation?)

It's of_serial's implementation. I patched that once for getting
frequency out of "clocks" property but then I got busy with
porting mach-dove and pinctrl.. Marvell SoCs do have a clk
implementation and as soon as of_serial can handle "clocks"
property it will be used for sure. I can remove "clock-frequency"
from the example anyway as it is not really part of pinctrl
binding documentation.

> (...)
>> +#include <linux/pinctrl/consumer.h>
>
> Why are you including this header?
> (There are valid reasons, state them in a comment,
> e.g. /* to access pinctrl_gpio* in GPIO portions */)

I guess it was a left-over from my experiments with
default pinmux configuration until you pointed me to
the correct way of having the pinctrl itself hog muxes.

I'll remove that include.

>> +struct mvebu_pinctrl {
>> +       struct device *dev;
>> +       struct pinctrl_dev *pctldev;
>> +       struct pinctrl_desc desc;
>> +       void __iomem *base;
>> +       struct mvebu_pinctrl_group *groups;
>> +       unsigned num_groups;
>> +       struct mvebu_pinctrl_function *functions;
>> +       unsigned num_functions;
>> +       unsigned variant;
>> +};
>
> There is some other "variant" field elsewhere that is
> a u8, is this correct? In this and the other case, should
> this "variant" rather be an enum?

I'll review the variant types but inside pinctrl-mvebu variant
is used as a bit mask to distinguish different variants. Anyway,
they should always be the same size.

>> +static int mvebu_common_mpp_get(struct mvebu_pinctrl *pctl,
>> +                               struct mvebu_pinctrl_group *grp,
>> +                               unsigned long *config)
>> +{
>> +       unsigned pin = grp->gid;
>> +       unsigned off = (pin / 8) * 4;
>> +       unsigned shift = (pin % 8) * 4;
>
> Some magic numbers here. Either use relevant #define:s or
> put an inline comment that explains what is going on.

Ok.

>> +static void mvebu_pinctrl_dt_free_map(struct pinctrl_dev *pctldev,
>> +                               struct pinctrl_map *map, unsigned
>> num_maps)
>> +{
>> +       kfree(map);
>> +}
>
> Is it possible to use devm_* managed devm_kzalloc() for this map
> so you don't need to free it explicitly?
>
> (Maybe not, just checking.)

Hmm, I guess not as I thought I've read not to use devm_kfree when
you allocate _and_ free stuff on runtime without removing the device
itself, right?

>> diff --git a/drivers/pinctrl/pinctrl-mvebu.h
>> b/drivers/pinctrl/pinctrl-mvebu.h
> (...)
>> +/*
>
> Kerneldoc begins with /** have you really tested to generate
> kerneldoc?

Nope, I wasn't even aware of that, sorry. I just copied it over
from some other pinctrl include. I'll check for compatibility with
kerneldoc.

>> +struct mvebu_mpp_ctrl {
>> +       const char *name;
>> +       u8 pid;
>> +       u8 npins;
>
> So, there will never be > 256 pins on a Marvell platform?

Well, with all current platforms we are well below 100. I guess
256 max (muxable) pins will be enough.

>> + * struct mvebu_mpp_ctrl_setting - describe a mpp ctrl setting
>> + * @val: ctrl setting value
>
> It is not obvious to me what this means, it it possible to elaborate
> on how this member is defined and used?

Well, I see if I can clarify the description but wrt the datasheet it
_should_ be quite obvious.

>> + * @variant: (optional) variant identifier mask
>
> This thing again, there are lots of "variants" around here.
> Is it a candidate for an enum?

As it is a mask, I don't think enum fits here.

>> + * @flags: (private) flags to store gpi/gpo/gpio capabilities
>
> Write something about the two available flags and what they
> mean maybe?

Ok.

>> +#define  MVEBU_SETTING_GPO     (1 << 0)
>> +#define  MVEBU_SETTING_GPI     (1 << 1)
>
> So these are the possible flags I suspect. (Looking good.)

Yes, some mpp pins on Marvell SoCs are out-only, while
normally they are in-out. I haven't seen in-only yet but better
to have the flags ready if someone stumbles upon one.

>> +#if defined(CONFIG_DEBUG_FS)
>> +#define MPP_VAR_FUNCTION(_val, _name, _subname, _mask)         \
>> +       _MPP_VAR_FUNCTION(_val, _name, _subname, _mask)
>> +#else
>> +#define MPP_VAR_FUNCTION(_val, _name, _subname, _mask)         \
>> +       _MPP_VAR_FUNCTION(_val, _name, NULL, _mask)
>> +#endif
>
> This looks a bit kludgy. Is the purpose to save memory
> if debugfs is not used?

Yes, that was the intention. Subnames are not used by pinctrl at all,
other for debugfs files. Although, there are some pins that have two different
settings for the same device name, e.g. sata(prsnt) and sata(act), I don't
think it is a good idea to use the subname but have sata(prsnt) and sata-1(act)
instead. Otherwise, you'll always have to look up the specific function for
each pin. OTOH, I could check for "sata" and take the subname into account
when it is given in marvell,function. But it will not help reducing the required
pinmux descriptions in DT because you still have to distinguish both and
there are only 2 out of ~50 groups that have them for Dove.

> This looks good overall.

Thanks!
In some internal review with Andrew I also added a spinlock to
mvebu_pinconf_get/_set that will protect all calls to generic and specific
_get/_set register accesses. Moreover, I replaced clk_get_sys in pinctrl-dove
with the devm_ counterpart and removed the explicit clk_put.

Sebastian