Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1494809 > unrolled thread
| Started by | Maxime Ripard <maxime.ripard@free-electrons.com> |
|---|---|
| First post | 2016-10-03 12:30 +0200 |
| Last post | 2016-10-07 15:10 +0200 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[PATCH 4/9] pinctrl: sunxi: Deal with configless pins Maxime Ripard <maxime.ripard@free-electrons.com> - 2016-10-03 12:30 +0200
Re: [PATCH 4/9] pinctrl: sunxi: Deal with configless pins Chen-Yu Tsai <wens@csie.org> - 2016-10-04 04:30 +0200
Re: [PATCH 4/9] pinctrl: sunxi: Deal with configless pins Maxime Ripard <maxime.ripard@free-electrons.com> - 2016-10-07 15:10 +0200
| From | Maxime Ripard <maxime.ripard@free-electrons.com> |
|---|---|
| Date | 2016-10-03 12:30 +0200 |
| Subject | [PATCH 4/9] pinctrl: sunxi: Deal with configless pins |
| Message-ID | <so8Fs-3GE-15@gated-at.bofh.it> |
Even though the our binding had the assumption that the allwinner,pull and
allwinner,drive properties were optional, the code never took that into
account.
Fix that.
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
drivers/pinctrl/sunxi/pinctrl-sunxi.c | 50 +++++++++++++++++++---------
1 file changed, 35 insertions(+), 15 deletions(-)
diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
index 6f6f1e0011e2..cec977fcf98c 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
@@ -218,20 +218,29 @@ static unsigned long *sunxi_pctrl_build_pin_config(struct device_node *node,
{
unsigned long *pinconfig;
unsigned int configlen = 0, idx = 0;
+ int ret;
if (sunxi_pctrl_has_drive_prop(node))
configlen++;
if (sunxi_pctrl_has_bias_prop(node))
configlen++;
+ /*
+ * If we don't have any configuration, bail out
+ */
+ if (!configlen)
+ return NULL;
+
pinconfig = kzalloc(configlen * sizeof(*pinconfig), GFP_KERNEL);
if (!pinconfig)
- return NULL;
+ return ERR_PTR(-ENOMEM);
if (sunxi_pctrl_has_drive_prop(node)) {
int drive = sunxi_pctrl_parse_drive_prop(node);
- if (drive < 0)
+ if (drive < 0) {
+ ret = -EINVAL;
goto err_free;
+ }
pinconfig[idx++] = pinconf_to_config_packed(PIN_CONFIG_DRIVE_STRENGTH,
drive);
@@ -239,8 +248,10 @@ static unsigned long *sunxi_pctrl_build_pin_config(struct device_node *node,
if (sunxi_pctrl_has_bias_prop(node)) {
int pull = sunxi_pctrl_parse_bias_prop(node);
- if (pull < 0)
+ if (pull < 0) {
+ ret = -EINVAL;
goto err_free;
+ }
pinconfig[idx++] = pinconf_to_config_packed(pull, 0);
}
@@ -251,7 +262,7 @@ static unsigned long *sunxi_pctrl_build_pin_config(struct device_node *node,
err_free:
kfree(pinconfig);
- return NULL;
+ return ERR_PTR(ret);
}
static int sunxi_pctrl_dt_node_to_map(struct pinctrl_dev *pctldev,
@@ -285,7 +296,10 @@ static int sunxi_pctrl_dt_node_to_map(struct pinctrl_dev *pctldev,
/*
* We have two maps for each pin: one for the function, one
- * for the configuration (bias, strength, etc)
+ * for the configuration (bias, strength, etc).
+ *
+ * We might be slightly overshooting, since we might not have
+ * any configuration.
*/
nmaps = npins * 2;
*map = kmalloc(nmaps * sizeof(struct pinctrl_map), GFP_KERNEL);
@@ -293,8 +307,8 @@ static int sunxi_pctrl_dt_node_to_map(struct pinctrl_dev *pctldev,
return -ENOMEM;
pinconfig = sunxi_pctrl_build_pin_config(node, &configlen);
- if (!pinconfig) {
- ret = -EINVAL;
+ if (IS_ERR(pinconfig)) {
+ ret = PTR_ERR(pinconfig);
goto err_free_map;
}
@@ -321,15 +335,16 @@ static int sunxi_pctrl_dt_node_to_map(struct pinctrl_dev *pctldev,
i++;
- (*map)[i].type = PIN_MAP_TYPE_CONFIGS_GROUP;
- (*map)[i].data.configs.group_or_pin = group;
- (*map)[i].data.configs.configs = pinconfig;
- (*map)[i].data.configs.num_configs = configlen;
-
- i++;
+ if (pinconfig) {
+ (*map)[i].type = PIN_MAP_TYPE_CONFIGS_GROUP;
+ (*map)[i].data.configs.group_or_pin = group;
+ (*map)[i].data.configs.configs = pinconfig;
+ (*map)[i].data.configs.num_configs = configlen;
+ i++;
+ }
}
- *num_maps = nmaps;
+ *num_maps = i;
return 0;
@@ -342,8 +357,13 @@ static void sunxi_pctrl_dt_free_map(struct pinctrl_dev *pctldev,
struct pinctrl_map *map,
unsigned num_maps)
{
+ unsigned long *pinconfig;
+
/* All the maps have the same pin config, free only the first one */
- kfree(map[0].data.configs.configs);
+ pinconfig = map[0].data.configs.configs;
+ if (pinconfig)
+ kfree(pinconfig);
+
kfree(map);
}
--
git-series 0.8.10
[toc] | [next] | [standalone]
| From | Chen-Yu Tsai <wens@csie.org> |
|---|---|
| Date | 2016-10-04 04:30 +0200 |
| Message-ID | <sonEt-4Jp-1@gated-at.bofh.it> |
| In reply to | #1494809 |
On Mon, Oct 3, 2016 at 6:21 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Even though the our binding had the assumption that the allwinner,pull and
> allwinner,drive properties were optional, the code never took that into
> account.
>
> Fix that.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
> drivers/pinctrl/sunxi/pinctrl-sunxi.c | 50 +++++++++++++++++++---------
> 1 file changed, 35 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> index 6f6f1e0011e2..cec977fcf98c 100644
> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> @@ -218,20 +218,29 @@ static unsigned long *sunxi_pctrl_build_pin_config(struct device_node *node,
> {
> unsigned long *pinconfig;
> unsigned int configlen = 0, idx = 0;
> + int ret;
>
> if (sunxi_pctrl_has_drive_prop(node))
> configlen++;
> if (sunxi_pctrl_has_bias_prop(node))
> configlen++;
>
> + /*
> + * If we don't have any configuration, bail out
> + */
> + if (!configlen)
> + return NULL;
> +
> pinconfig = kzalloc(configlen * sizeof(*pinconfig), GFP_KERNEL);
> if (!pinconfig)
> - return NULL;
> + return ERR_PTR(-ENOMEM);
>
> if (sunxi_pctrl_has_drive_prop(node)) {
> int drive = sunxi_pctrl_parse_drive_prop(node);
> - if (drive < 0)
> + if (drive < 0) {
> + ret = -EINVAL;
Why not just pass the error code returned from the parse function?
> goto err_free;
> + }
>
> pinconfig[idx++] = pinconf_to_config_packed(PIN_CONFIG_DRIVE_STRENGTH,
> drive);
> @@ -239,8 +248,10 @@ static unsigned long *sunxi_pctrl_build_pin_config(struct device_node *node,
>
> if (sunxi_pctrl_has_bias_prop(node)) {
> int pull = sunxi_pctrl_parse_bias_prop(node);
> - if (pull < 0)
> + if (pull < 0) {
> + ret = -EINVAL;
Same here.
> goto err_free;
> + }
>
> pinconfig[idx++] = pinconf_to_config_packed(pull, 0);
> }
> @@ -251,7 +262,7 @@ static unsigned long *sunxi_pctrl_build_pin_config(struct device_node *node,
>
> err_free:
> kfree(pinconfig);
> - return NULL;
> + return ERR_PTR(ret);
> }
>
> static int sunxi_pctrl_dt_node_to_map(struct pinctrl_dev *pctldev,
> @@ -285,7 +296,10 @@ static int sunxi_pctrl_dt_node_to_map(struct pinctrl_dev *pctldev,
>
> /*
> * We have two maps for each pin: one for the function, one
> - * for the configuration (bias, strength, etc)
> + * for the configuration (bias, strength, etc).
> + *
> + * We might be slightly overshooting, since we might not have
> + * any configuration.
> */
> nmaps = npins * 2;
> *map = kmalloc(nmaps * sizeof(struct pinctrl_map), GFP_KERNEL);
> @@ -293,8 +307,8 @@ static int sunxi_pctrl_dt_node_to_map(struct pinctrl_dev *pctldev,
> return -ENOMEM;
>
> pinconfig = sunxi_pctrl_build_pin_config(node, &configlen);
> - if (!pinconfig) {
> - ret = -EINVAL;
> + if (IS_ERR(pinconfig)) {
> + ret = PTR_ERR(pinconfig);
> goto err_free_map;
> }
>
> @@ -321,15 +335,16 @@ static int sunxi_pctrl_dt_node_to_map(struct pinctrl_dev *pctldev,
>
> i++;
>
> - (*map)[i].type = PIN_MAP_TYPE_CONFIGS_GROUP;
> - (*map)[i].data.configs.group_or_pin = group;
> - (*map)[i].data.configs.configs = pinconfig;
> - (*map)[i].data.configs.num_configs = configlen;
> -
> - i++;
> + if (pinconfig) {
> + (*map)[i].type = PIN_MAP_TYPE_CONFIGS_GROUP;
> + (*map)[i].data.configs.group_or_pin = group;
> + (*map)[i].data.configs.configs = pinconfig;
> + (*map)[i].data.configs.num_configs = configlen;
> + i++;
> + }
> }
>
> - *num_maps = nmaps;
> + *num_maps = i;
Thought: should we do a krealloc to shrink the array?
>
> return 0;
>
> @@ -342,8 +357,13 @@ static void sunxi_pctrl_dt_free_map(struct pinctrl_dev *pctldev,
> struct pinctrl_map *map,
> unsigned num_maps)
> {
> + unsigned long *pinconfig;
> +
> /* All the maps have the same pin config, free only the first one */
> - kfree(map[0].data.configs.configs);
> + pinconfig = map[0].data.configs.configs;
> + if (pinconfig)
> + kfree(pinconfig);
Passing NULL to kfree is allowed. (It becomes a no-op.)
So you could leave this function alone.
ChenYu
> +
> kfree(map);
> }
>
> --
> git-series 0.8.10
[toc] | [prev] | [next] | [standalone]
| From | Maxime Ripard <maxime.ripard@free-electrons.com> |
|---|---|
| Date | 2016-10-07 15:10 +0200 |
| Message-ID | <spD4v-QQ-73@gated-at.bofh.it> |
| In reply to | #1495145 |
[Multipart message — attachments visible in raw view] — view raw
Hi,
On Tue, Oct 04, 2016 at 10:28:18AM +0800, Chen-Yu Tsai wrote:
> > if (sunxi_pctrl_has_drive_prop(node)) {
> > int drive = sunxi_pctrl_parse_drive_prop(node);
> > - if (drive < 0)
> > + if (drive < 0) {
> > + ret = -EINVAL;
>
> Why not just pass the error code returned from the parse function?
Yep, I'll change that.
> > - (*map)[i].type = PIN_MAP_TYPE_CONFIGS_GROUP;
> > - (*map)[i].data.configs.group_or_pin = group;
> > - (*map)[i].data.configs.configs = pinconfig;
> > - (*map)[i].data.configs.num_configs = configlen;
> > -
> > - i++;
> > + if (pinconfig) {
> > + (*map)[i].type = PIN_MAP_TYPE_CONFIGS_GROUP;
> > + (*map)[i].data.configs.group_or_pin = group;
> > + (*map)[i].data.configs.configs = pinconfig;
> > + (*map)[i].data.configs.num_configs = configlen;
> > + i++;
> > + }
> > }
> >
> > - *num_maps = nmaps;
> > + *num_maps = i;
>
> Thought: should we do a krealloc to shrink the array?
Yep, I'll make an additional patch to fix that.
>
> >
> > return 0;
> >
> > @@ -342,8 +357,13 @@ static void sunxi_pctrl_dt_free_map(struct pinctrl_dev *pctldev,
> > struct pinctrl_map *map,
> > unsigned num_maps)
> > {
> > + unsigned long *pinconfig;
> > +
> > /* All the maps have the same pin config, free only the first one */
> > - kfree(map[0].data.configs.configs);
> > + pinconfig = map[0].data.configs.configs;
> > + if (pinconfig)
> > + kfree(pinconfig);
>
> Passing NULL to kfree is allowed. (It becomes a no-op.)
> So you could leave this function alone.
And I'll change that as well.
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web