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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1459520074.5907.205.camel@linux.intel.com>
Date:	Fri, 01 Apr 2016 17:14:34 +0300
From:	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:	Irina Tirdea <irina.tirdea@...el.com>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Len Brown <lenb@...nel.org>,
	Mika Westerberg <mika.westerberg@...ux.intel.com>,
	Linus Walleij <linus.walleij@...aro.org>,
	linux-gpio@...r.kernel.org, linux-acpi@...r.kernel.org
Cc:	Rob Herring <robh+dt@...nel.org>,
	Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
	Octavian Purdila <octavian.purdila@...el.com>,
	Cristina Ciocan <cristina.ciocan@...el.com>,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
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.

> + */
> +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@...ux.intel.com>
Intel Finland Oy