Messages in this thread | | | Subject | Re: [RFC PATCH 3/4] pinctrl: Add ACPI support | From | Andy Shevchenko <> | Date | Fri, 01 Apr 2016 17:14:34 +0300 |
| |
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.
> + */ > +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.
> + > + /* 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?
> +err_free_map:
Perhaps err_free_maps?
> + pinctrl_acpi_free_maps(p); > + return ret;
-- Andy Shevchenko <andriy.shevchenko@linux.intel.com> Intel Finland Oy
| |