> -----Original Message----- > From: Andy Shevchenko [mailto:andriy.shevchenko@xxxxxxxxxxxxxxx] > Sent: 01 April, 2016 17:15 > 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 3/4] pinctrl: Add ACPI support > > On Thu, 2016-03-31 at 14:44 +0300, Irina Tirdea wrote: > > Add ACPI support for pin controller properties. These are > > based on ACPI _DSD properties and follow the device tree > > model based on states and node configurations. The states > > are defined as _DSD properties and configuration nodes > > are defined using the _DSD Hierarchical Properties Extension. > > > > A configuration node supports the generic device tree properties. > > > > The implementation is based on device tree code from devicetree.c. > > > > Patch is good to me, though few minor comments below. > > > +/** > > + * struct pinctrl_acpi_map - mapping table chunk parsed from ACPI > > + * @node: list node for struct pinctrl's @fw_maps field > > + * @pctldev: the pin controller that allocated this struct, and will > > free it > > > + * @maps: the mapping table entries > > We have @map and @num_maps. Right, I'll add num_maps description. > > > + */ > > +struct pinctrl_acpi_map { > > + struct list_head node; > > + struct pinctrl_dev *pctldev; > > + struct pinctrl_map *map; > > + unsigned num_maps; > > +}; > > + > > > > > +static int acpi_remember_or_free_map(struct pinctrl *p, const char > > *statename, > > + struct pinctrl_dev *pctldev, > > + struct pinctrl_map *map, > > unsigned num_maps) > > +{ > > + struct pinctrl_acpi_map *acpi_map; > > + struct list_head *acpi_maps; > > > > + unsigned int i; > > Just unsigned i to be in align with unsigned num_maps. > OK. > > + > > + /* Initialize common mapping table entry fields */ > > + for (i = 0; i < num_maps; i++) { > > + map[i].dev_name = dev_name(p->dev); > > + map[i].name = statename; > > + if (pctldev) > > + map[i].ctrl_dev_name = dev_name(pctldev- > > >dev); > > + } > > > > +int pinctrl_acpi_to_map(struct pinctrl *p) > > +{ > > + const union acpi_object *prop, *statenames, *configs; > > + unsigned int state, nstates, nconfigs, config; > > + char *statename, *propname, *configname; > > + struct fwnode_handle *fw_prop; > > + struct acpi_device *adev; > > + int ret; > > + > > + /* We may store pointers to property names within the node > > */ > > + adev = acpi_bus_get_acpi_device(ACPI_HANDLE(p->dev)); > > + if (!adev) > > + return -ENODEV; > > + > > > + /* Only allow named states (device must have prop 'pinctrl- > > names') */ > > Does it fit one line? > It has 77 characters, so it should fit one line. > > +err_free_map: > > Perhaps err_free_maps? > OK. > > + pinctrl_acpi_free_maps(p); > > + return ret; > > -- > Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> > Intel Finland Oy ��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
