Re: [PATCH v1 1/1] pinctrl: intel: Add Intel Merrifield pin controller support

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Mon, 2016-06-20 at 12:20 +0300, Mika Westerberg wrote:
> On Sun, Jun 19, 2016 at 06:44:59PM +0300, Andy Shevchenko wrote:
> > This driver adds pinctrl support for Intel Merrifield. The IP block
> > which is
> > called Family-Level Interface Shim is a separate entity in SoC. The
> > GPIO driver
> > (gpio-intel-mid.c) will be updated accordingly to support pinctrl
> > interface.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>

Mika, my answers below. (For the rest I agree with you and fixed that
locally)

> > +
> > +/**
> > + * struct mrfld_pinctrl_soc_data - Intel pin controller per-SoC
> > configuration
> > + * @name: Name of the family
> > + * @pins: Array if pins this pinctrl controls
> > + * @npins: Number of pins in the array
> > + * @groups: Array of pin groups
> > + * @ngroups: Number of groups in the array
> > + * @functions: Array of functions
> > + * @nfunctions: Number of functions in the array
> > + * @families: Array of families this pinctrl handles
> > + * @nfamilies: Number of families in the array
> > + *
> > + * The @families is used as a template by the core driver. It will
> > make
> > + * copy of all families and fill in rest of the information.
> > + */
> > +struct mrfld_pinctrl_soc_data {
> > +	const char *name;
> > +	const struct pinctrl_pin_desc *pins;
> > +	size_t npins;
> > +	const struct intel_pingroup *groups;
> > +	size_t ngroups;
> > +	const struct intel_function *functions;
> > +	size_t nfunctions;
> > +	const struct mrfld_family *families;
> > +	size_t nfamilies;
> > +};
> > +
> > +static const struct mrfld_pinctrl_soc_data mrfld_soc_data = {
> > +	.pins = mrfld_pins,
> > +	.npins = ARRAY_SIZE(mrfld_pins),
> > +	.groups = mrfld_groups,
> > +	.ngroups = ARRAY_SIZE(mrfld_groups),
> > +	.functions = mrfld_functions,
> > +	.nfunctions = ARRAY_SIZE(mrfld_functions),
> > +	.families = mrfld_families,
> > +	.nfamilies = ARRAY_SIZE(mrfld_families),
> > +};
> > +
> 
> These are pointless. There can be only one kind of SoC data in
> Merrifield so please drop these. See for example pinctrl-cherryview.c.

I didn't quite get what should I look for? In mentioned driver we have
struct chv_community which contains same fields. IIRC I took it from
exactly that driver.

> +static int mrfld_pinmux_set_mux(struct pinctrl_dev *pctldev,
> > unsigned function,
> > +				unsigned group)
> > +{
> > +	struct mrfld_pinctrl *mp =
> > pinctrl_dev_get_drvdata(pctldev);
> > +	const struct intel_pingroup *grp = &mp->soc->groups[group];
> > +	unsigned long flags;
> > +	unsigned int i;
> > +
> > +	spin_lock_irqsave(&mp->lock, flags);
> 
> If you ever need to call the backing pinctrl functions from irqchip
> driver, please use raw_spinlock instead.

Frankly speaking I dunno. How may I check this?

> +static int mrfld_pinctrl_remove(struct platform_device *pdev)
> > +{
> > +	return 0;
> > +}
> 
> Is this really needed?

Not for now. I hope we still may remove module without this callback in
place. At least that's how I read __device_release_driver().

-- 

Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html