> -----Original Message----- > From: Andy Shevchenko [mailto:andriy.shevchenko@xxxxxxxxxxxxxxx] > Sent: 01 April, 2016 17:05 > To: Tirdea, Irina; Rafael J. Wysocki; Len Brown; Mika Westerberg; Linus Walleij; linux-gpio@xxxxxxxxxxxxxxx; linux- > acpi@xxxxxxxxxxxxxxx > Cc: Rob Herring; Heikki Krogerus; Purdila, Octavian; Ciocan, Cristina; devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx > Subject: Re: [RFC PATCH 2/4] pinctrl: pinconf-generic: Add ACPI support > > On Thu, 2016-03-31 at 14:44 +0300, Irina Tirdea wrote: > > Add ACPI support for the generic device tree properties. > > Convert the pinconf generic code to handle both ACPI and > > device tree by using the fwnode_property API. Also include > > renaming device tree references in names of functions and > > structures from 'dt' to 'fwnode'. > > This is looks good to me, though few style / minor comments below. Thanks for the review, Andy! > > > --- a/drivers/pinctrl/pinconf-generic.c > > +++ b/drivers/pinctrl/pinconf-generic.c > > > > > @@ -175,22 +176,22 @@ static const struct pinconf_generic_params > > dt_params[] = { > > }; > > > > /** > > - * parse_dt_cfg() - Parse DT pinconf parameters > > - * @np: DT node > > + * parse_fwnode_cfg() - Parse FW pinconf parameters > > + * @fw: FW node > > Here and below it should be @fwnode. OK. > > > -int pinconf_generic_dt_subnode_to_map(struct pinctrl_dev *pctldev, > > - struct device_node *np, struct pinctrl_map **map, > > +static int pinconf_generic_fwnode_subnode_to_map(struct pinctrl_dev > > *pctldev, > > + struct fwnode_handle *fwnode, struct pinctrl_map > > **map, > > unsigned *reserved_maps, unsigned *num_maps, > > enum pinctrl_map_type type) > > { > > - int ret; > > + int ret, i; > > Since you change this line anyway, perhaps move it to be last in the > definition block? > > > const char *function; > > struct device *dev = pctldev->dev; > > unsigned long *configs = NULL; > > unsigned num_configs = 0; > > unsigned reserve, strings_count; > > - struct property *prop; > > - const char *group; > > + const char **groups; > > const char *subnode_target_type = "pins"; > > ...to here. > Sure. > > +#ifdef CONFIG_OF > > +inline int pinconf_generic_parse_dt_config(struct device_node *np, > > + struct pinctrl_dev > > *pctldev, > > + unsigned long **configs, > > + unsigned int *nconfigs) > > +{ > > + return pinconf_generic_parse_fwnode_config(&np->fwnode, > > pctldev, > > + configs, > > nconfigs); > > +} > > I didn't see any user of this. pinconf_generic_parse_dt_config is defined in drivers/pinctrl/pinconf.h and is used by various pinctrl drivers(e.g.: drivers/pinctrl/nomadik/pinctrl-abx500.c, drivers/pinctrl/pinctrl-tz1090-pdc.c, drivers/pinctrl/pinctrl-tz1090.c). Taking a closer look at its prototype, I should match it here exactly (by removing inline). ��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
