Re: [RFC PATCH 3/4] pinctrl: Add ACPI support

[Andy Shevchenko]

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@xxxxxxxxxxxxxxx>
Intel Finland Oy