On 08/30/2013 04:45 AM, Tomasz Nowicki wrote:
Hi Al,
I have some minor comments:
W dniu 30.08.2013 00:46, al.stone@linaro.org pisze:
From: Al Stone ahs3@redhat.com
This code allows the Samsung pinctrl driver to use either FDT or ACPI in defining pin controllers (i.e., collections of GPIO controllers, in this case). On probe, the driver first attempts to configure a driver using FDT; failing that, using ACPI is attempted. The size of the patch is due to essentially having to duplicate the FDT paths for ACPI, since the FDT code made assumptions about data structures that are not necessarily available with ACPI.
Signed-off-by: Al Stone al.stone@linaro.org
drivers/pinctrl/pinctrl-samsung.c | 507 +++++++++++++++++++++++++++++++++++++- drivers/pinctrl/pinctrl-samsung.h | 3 + 2 files changed, 503 insertions(+), 7 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-samsung.c b/drivers/pinctrl/pinctrl-samsung.c index a7fa9e2..e135007 100644 --- a/drivers/pinctrl/pinctrl-samsung.c +++ b/drivers/pinctrl/pinctrl-samsung.c @@ -29,6 +29,7 @@ #include <linux/irqdomain.h> #include <linux/spinlock.h> #include <linux/syscore_ops.h> +#include <linux/acpi.h>
#include "core.h" #include "pinctrl-samsung.h" @@ -716,12 +717,480 @@ static int samsung_pinctrl_parse_dt(struct platform_device *pdev, return 0; } [snip....]
- if (!grp || ii >= drvdata->nr_groups) {
dev_err(dev, "too many pin groups defined for %s\n",
(char *)name_buffer.pointer);
return AE_BAD_PARAMETER;
- }
- /* see if there is a pin list to get -- no method == not a pin
group */
- status = acpi_evaluate_object(handle, "PINS", NULL, &buf);
- if (ACPI_FAILURE(status)) {
ACPI_FREE(buf.pointer);
Couple line below you check if (info.pointer) before you free memory. Here we can not be sure buf.pointer != NULL too, so maybe we should do the same check here. If ACPI_FREE check for such condition inside we do not need it for (info.pointer) as well.
Actually, checking if (info.pointer) is redundant; ACPI_FREE() essentially becomes kfree() so I'm just being overly paranoid. I'll clean this up.
return AE_OK; /* the device is irrelevant */
- }
- status = acpi_evaluate_integer(handle, "NPIN", NULL, &value);
- if (ACPI_FAILURE(status)) {
/* but, we should have NPIN if we have the PINS method */
ACPI_FREE(buf.pointer);
return status;
- }
- npins = (unsigned int)value;
- grp->num_pins = npins;
- /* find the function number to use for this group */
- status = acpi_evaluate_integer(handle, "FUNC", NULL, &value);
- if (ACPI_FAILURE(status)) {
/* we must have a function number */
ACPI_FREE(buf.pointer);
return status;
- }
- func = (u8)value;
- grp->func = func;
- /* find the name to use for this group */
- memset(buffer, 0, BUFSIZ);
- status = acpi_get_name(handle, ACPI_SINGLE_NAME, &name_buffer);
- if (ACPI_FAILURE(status)) {
ACPI_FREE(buf.pointer);
return status;
- }
- len = strlen(name_buffer.pointer);
- pname = devm_kzalloc(dev, len + GSUFFIX_LEN + 1, GFP_KERNEL);
- if (!pname) {
dev_err(dev, "cannot alloc group name space for %s\n",
(char *)name_buffer.pointer);
ACPI_FREE(buf.pointer);
status = AE_NO_MEMORY;
- }
- memcpy(pname, name_buffer.pointer, len);
- memcpy(pname + len, GROUP_SUFFIX, GSUFFIX_LEN);
- grp->name = pname;
- /* allocate the space needed for the pin list for this group */
- pins = devm_kzalloc(dev, npins * sizeof(*(pcdesc->pins)),
GFP_KERNEL);
- if (!pins) {
dev_err(dev, "cannot alloc group pin list space for %s\n",
(char *)name_buffer.pointer);
ACPI_FREE(buf.pointer);
devm_kfree(dev, pname);
return AE_NO_MEMORY;
- }
- /* extract the strings from the package provided by the PINS
method */
- format.pointer = kzalloc((npins * sizeof(char)) + 1, GFP_KERNEL);
- if (!format.pointer) {
ACPI_FREE(buf.pointer);
devm_kfree(dev, pname);
return AE_NO_MEMORY;
- }
- format.length = npins + 1;
- memset(format.pointer, 'S', npins);
- status = acpi_extract_package(buf.pointer, &format, &info);
- if (ACPI_FAILURE(status)) {
dev_err(dev, "cannot parse pin names for %s\n",
(char *)name_buffer.pointer);
ACPI_FREE(buf.pointer);
if (info.pointer)
ACPI_FREE(info.pointer);
devm_kfree(dev, pname);
kfree(format.pointer);
return status;
- }
- kfree(format.pointer);
How about ACPI_FREE(buf.pointer); here so we don't have to worry about freeing it later on?
Good idea. Thanks.
- /* set up the group pins */
- pp = (char **)info.pointer;
- for (ii = 0; ii < grp->num_pins; ii++, pp++)
pins[ii] = ii;
- grp->pins = pins;
[snip...]
- /* initialize the samsung_pin_ctrl part of
samsung_pinctrl_drv_data */
- ret = samsung_pin_ctrl_acpi_parse(drvdata, pdev);
- if (ret)
return NULL;
- ctrl = drvdata->ctrl;
- /* initialize the samsung_pin_group part of
samsung_pinctrl_drv_data */
- ret = samsung_pin_group_acpi_parse(drvdata, pdev);
- if (ret)
ctrl = NULL;
- else
ctrl = drvdata->ctrl;
- /* return ctrl; */
- return drvdata->ctrl;
Sth is wrong here, if we chose to return drvdata->ctrl, using ctrl pointer does not make sense for me :).
D'oh. I'll clean this up. Some of this is original driver code that I'm not re-using well.
+}
[snip...]
- if (!dev->of_node) {
dev_err(dev, "device tree node not found\n");
- dev_handle = DEVICE_ACPI_HANDLE(&pdev->dev);
Since we already have "dev" pointer we can use it instead &pdev->dev here.
Hrm. WFM. Nice catch.
[snip...]
Other than comments above...
Acked-by: Tomasz Nowicki tomasz.nowicki@linaro.org
Excellent. Thanks for the review.