max9286: simplify i2c-mux parsing
diff mbox series

Message ID 20191121165631.5744-1-kieran.bingham@ideasonboard.com
State New
Delegated to: Kieran Bingham
Headers show
Series
  • max9286: simplify i2c-mux parsing
Related show

Commit Message

Kieran Bingham Nov. 21, 2019, 4:56 p.m. UTC
- Identify each enabled i2c-mux channel in a single pass

The parse_dt function iterates each node in the i2c-mux for each endpoint looking for a match.
The only purpose of these iterations is to determine if the corresponding i2c-channel
is enabled. (status = 'okay').

Iterate the i2c-mux nodes in a single pass storing the enable state
in a local i2c_mux_mask for use when parsing the endpoints.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 drivers/media/i2c/max9286.c | 84 +++++++++++++++----------------------
 1 file changed, 34 insertions(+), 50 deletions(-)

Comments

Jacopo Mondi Nov. 21, 2019, 5:35 p.m. UTC | #1
Hi Kieran,

On Thu, Nov 21, 2019 at 04:56:31PM +0000, Kieran Bingham wrote:
>  - Identify each enabled i2c-mux channel in a single pass
>
> The parse_dt function iterates each node in the i2c-mux for each endpoint looking for a match.
> The only purpose of these iterations is to determine if the corresponding i2c-channel
> is enabled. (status = 'okay').
>
> Iterate the i2c-mux nodes in a single pass storing the enable state
> in a local i2c_mux_mask for use when parsing the endpoints.
>

I very much agree with this :)

> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  drivers/media/i2c/max9286.c | 84 +++++++++++++++----------------------
>  1 file changed, 34 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 34cb6f3b40c2..a36132becdc7 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -1097,55 +1097,6 @@ static int max9286_is_bound(struct device *dev, void *data)
>  	return 0;
>  }
>
> -static struct device_node *max9286_get_i2c_by_id(struct device_node *parent,
> -						 u32 id)
> -{
> -	struct device_node *i2c_mux;
> -	struct device_node *child;
> -
> -	/* Balance the of_node_put() performed by of_find_node_by_name() */
> -	of_node_get(parent);
> -
> -	i2c_mux = of_find_node_by_name(parent, "i2c-mux");
> -	if (!i2c_mux) {
> -		printk("max9286: Failed to find i2c-mux node\n");
> -		return NULL;
> -	}
> -
> -	for_each_child_of_node(i2c_mux, child) {
> -		u32 i2c_id = 0;
> -
> -		if (of_node_cmp(child->name, "i2c") != 0)
> -			continue;
> -		of_property_read_u32(child, "reg", &i2c_id);
> -		if (id == i2c_id)
> -			return child;
> -	}
> -
> -	return NULL;
> -}
> -
> -static int max9286_check_i2c_bus_by_id(struct device *dev, int id)
> -{
> -	struct device_node *i2c_np;
> -
> -	i2c_np = max9286_get_i2c_by_id(dev->of_node, id);
> -	if (!i2c_np) {
> -		dev_err(dev, "Failed to find corresponding i2c@%u\n", id);
> -		return -ENODEV;
> -	}
> -
> -	if (!of_device_is_available(i2c_np)) {
> -		dev_dbg(dev, "Skipping port %u with disabled I2C bus\n", id);
> -		of_node_put(i2c_np);
> -		return -ENODEV;
> -	}
> -
> -	of_node_put(i2c_np);
> -
> -	return 0;
> -}
> -
>  static void max9286_cleanup_dt(struct max9286_priv *priv)
>  {
>  	struct max9286_source *source;
> @@ -1167,11 +1118,44 @@ static void max9286_cleanup_dt(struct max9286_priv *priv)
>  static int max9286_parse_dt(struct max9286_priv *priv)
>  {
>  	struct device *dev = &priv->client->dev;
> +	struct device_node *i2c_mux;
> +	struct device_node *child = NULL;
>  	struct device_node *ep_np = NULL;

Nit: could you re-use child or ep_np ?

> +	unsigned int i2c_mux_mask = 0;
>  	int ret;
>
> +	/* Balance the of_node_put() performed by of_find_node_by_name() */

Do you need this comment ?

> +	of_node_get(dev->of_node);
> +	i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
> +	if (!i2c_mux) {
> +		dev_err(dev, "Failed to find i2c-mux node\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Identify which i2c-mux channels are enabled */
> +	for_each_child_of_node(i2c_mux, child) {
> +		u32 id = 0;
> +
> +		if (of_node_cmp(child->name, "i2c") != 0)
> +			continue;

With the new bindings in yaml format and the associated verification,
this should not happen.

> +
> +		of_property_read_u32(child, "reg", &id);
> +		if (id >= MAX9286_NUM_GMSL)
> +			continue;
> +
> +		if (!of_device_is_available(child)) {
> +			dev_dbg(dev, "Skipping port %u with disabled I2C bus\n", id);
> +			continue;
> +		}
> +
> +		i2c_mux_mask |= BIT(id);
> +	}
> +	of_node_put(child);
> +	of_node_put(i2c_mux);
> +
>  	v4l2_async_notifier_init(&priv->notifier);
>
> +	/* Parse the endpoints */
>  	for_each_endpoint_of_node(dev->of_node, ep_np) {

dev->of_node is reused here, do you need to get it again ?

All minors though, squash this on the next max9286 submission if you
feel to.

Thanks
   j

>  		struct max9286_source *source;
>  		struct of_endpoint ep;
> @@ -1214,7 +1198,7 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>  		}
>
>  		/* Skip if the corresponding GMSL link is unavailable. */
> -		if (max9286_check_i2c_bus_by_id(dev, ep.port))
> +		if (!(i2c_mux_mask & BIT(ep.port)))
>  			continue;
>
>  		if (priv->sources[ep.port].fwnode) {
> --
> 2.20.1
>
Kieran Bingham Nov. 29, 2019, 11:45 a.m. UTC | #2
Hi Jacopo,

On 21/11/2019 17:35, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Thu, Nov 21, 2019 at 04:56:31PM +0000, Kieran Bingham wrote:
>>  - Identify each enabled i2c-mux channel in a single pass
>>
>> The parse_dt function iterates each node in the i2c-mux for each endpoint looking for a match.
>> The only purpose of these iterations is to determine if the corresponding i2c-channel
>> is enabled. (status = 'okay').
>>
>> Iterate the i2c-mux nodes in a single pass storing the enable state
>> in a local i2c_mux_mask for use when parsing the endpoints.
>>
> 
> I very much agree with this :)

Great,

> 
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>  drivers/media/i2c/max9286.c | 84 +++++++++++++++----------------------
>>  1 file changed, 34 insertions(+), 50 deletions(-)
>>
>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
>> index 34cb6f3b40c2..a36132becdc7 100644
>> --- a/drivers/media/i2c/max9286.c
>> +++ b/drivers/media/i2c/max9286.c
>> @@ -1097,55 +1097,6 @@ static int max9286_is_bound(struct device *dev, void *data)
>>  	return 0;
>>  }
>>
>> -static struct device_node *max9286_get_i2c_by_id(struct device_node *parent,
>> -						 u32 id)
>> -{
>> -	struct device_node *i2c_mux;
>> -	struct device_node *child;
>> -
>> -	/* Balance the of_node_put() performed by of_find_node_by_name() */
>> -	of_node_get(parent);
>> -
>> -	i2c_mux = of_find_node_by_name(parent, "i2c-mux");
>> -	if (!i2c_mux) {
>> -		printk("max9286: Failed to find i2c-mux node\n");
>> -		return NULL;
>> -	}
>> -
>> -	for_each_child_of_node(i2c_mux, child) {
>> -		u32 i2c_id = 0;
>> -
>> -		if (of_node_cmp(child->name, "i2c") != 0)
>> -			continue;
>> -		of_property_read_u32(child, "reg", &i2c_id);
>> -		if (id == i2c_id)
>> -			return child;
>> -	}
>> -
>> -	return NULL;
>> -}
>> -
>> -static int max9286_check_i2c_bus_by_id(struct device *dev, int id)
>> -{
>> -	struct device_node *i2c_np;
>> -
>> -	i2c_np = max9286_get_i2c_by_id(dev->of_node, id);
>> -	if (!i2c_np) {
>> -		dev_err(dev, "Failed to find corresponding i2c@%u\n", id);
>> -		return -ENODEV;
>> -	}
>> -
>> -	if (!of_device_is_available(i2c_np)) {
>> -		dev_dbg(dev, "Skipping port %u with disabled I2C bus\n", id);
>> -		of_node_put(i2c_np);
>> -		return -ENODEV;
>> -	}
>> -
>> -	of_node_put(i2c_np);
>> -
>> -	return 0;
>> -}
>> -
>>  static void max9286_cleanup_dt(struct max9286_priv *priv)
>>  {
>>  	struct max9286_source *source;
>> @@ -1167,11 +1118,44 @@ static void max9286_cleanup_dt(struct max9286_priv *priv)
>>  static int max9286_parse_dt(struct max9286_priv *priv)
>>  {
>>  	struct device *dev = &priv->client->dev;
>> +	struct device_node *i2c_mux;
>> +	struct device_node *child = NULL;
>>  	struct device_node *ep_np = NULL;
> 
> Nit: could you re-use child or ep_np ?

Yes, that would reduce local vars. I'll do this now.


> 
>> +	unsigned int i2c_mux_mask = 0;
>>  	int ret;
>>
>> +	/* Balance the of_node_put() performed by of_find_node_by_name() */
> 
> Do you need this comment ?


It was non-obvious to me at least when I added it /why/ I had to get it.
But perhaps now I've got further along, it's more clear why.

DT references are a pain :-D
Lots of places where they are implicitly reduced in loops etc..



>> +	of_node_get(dev->of_node);
>> +	i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
>> +	if (!i2c_mux) {
>> +		dev_err(dev, "Failed to find i2c-mux node\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Identify which i2c-mux channels are enabled */
>> +	for_each_child_of_node(i2c_mux, child) {
>> +		u32 id = 0;
>> +
>> +		if (of_node_cmp(child->name, "i2c") != 0)
>> +			continue;
> 
> With the new bindings in yaml format and the associated verification,
> this should not happen.

Aha, yes I think you're right - well in that case I'm happy to drop it,
and simplify the code.
>> +
>> +		of_property_read_u32(child, "reg", &id);
>> +		if (id >= MAX9286_NUM_GMSL)
>> +			continue;
>> +
>> +		if (!of_device_is_available(child)) {
>> +			dev_dbg(dev, "Skipping port %u with disabled I2C bus\n", id);
>> +			continue;
>> +		}
>> +
>> +		i2c_mux_mask |= BIT(id);
>> +	}
>> +	of_node_put(child);
>> +	of_node_put(i2c_mux);
>> +
>>  	v4l2_async_notifier_init(&priv->notifier);
>>
>> +	/* Parse the endpoints */
>>  	for_each_endpoint_of_node(dev->of_node, ep_np) {
> 
> dev->of_node is reused here, do you need to get it again ?

Urggh, no idea ... I'll investigate.
The earlier one crashed on me, this one did not ... but better to get it
'right'.


> All minors though, squash this on the next max9286 submission if you
> feel to.

Thanks.

--
Kieran


> 
> Thanks
>    j
> 
>>  		struct max9286_source *source;
>>  		struct of_endpoint ep;
>> @@ -1214,7 +1198,7 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>>  		}
>>
>>  		/* Skip if the corresponding GMSL link is unavailable. */
>> -		if (max9286_check_i2c_bus_by_id(dev, ep.port))
>> +		if (!(i2c_mux_mask & BIT(ep.port)))
>>  			continue;
>>
>>  		if (priv->sources[ep.port].fwnode) {
>> --
>> 2.20.1
>>

Patch
diff mbox series

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 34cb6f3b40c2..a36132becdc7 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -1097,55 +1097,6 @@  static int max9286_is_bound(struct device *dev, void *data)
 	return 0;
 }
 
-static struct device_node *max9286_get_i2c_by_id(struct device_node *parent,
-						 u32 id)
-{
-	struct device_node *i2c_mux;
-	struct device_node *child;
-
-	/* Balance the of_node_put() performed by of_find_node_by_name() */
-	of_node_get(parent);
-
-	i2c_mux = of_find_node_by_name(parent, "i2c-mux");
-	if (!i2c_mux) {
-		printk("max9286: Failed to find i2c-mux node\n");
-		return NULL;
-	}
-
-	for_each_child_of_node(i2c_mux, child) {
-		u32 i2c_id = 0;
-
-		if (of_node_cmp(child->name, "i2c") != 0)
-			continue;
-		of_property_read_u32(child, "reg", &i2c_id);
-		if (id == i2c_id)
-			return child;
-	}
-
-	return NULL;
-}
-
-static int max9286_check_i2c_bus_by_id(struct device *dev, int id)
-{
-	struct device_node *i2c_np;
-
-	i2c_np = max9286_get_i2c_by_id(dev->of_node, id);
-	if (!i2c_np) {
-		dev_err(dev, "Failed to find corresponding i2c@%u\n", id);
-		return -ENODEV;
-	}
-
-	if (!of_device_is_available(i2c_np)) {
-		dev_dbg(dev, "Skipping port %u with disabled I2C bus\n", id);
-		of_node_put(i2c_np);
-		return -ENODEV;
-	}
-
-	of_node_put(i2c_np);
-
-	return 0;
-}
-
 static void max9286_cleanup_dt(struct max9286_priv *priv)
 {
 	struct max9286_source *source;
@@ -1167,11 +1118,44 @@  static void max9286_cleanup_dt(struct max9286_priv *priv)
 static int max9286_parse_dt(struct max9286_priv *priv)
 {
 	struct device *dev = &priv->client->dev;
+	struct device_node *i2c_mux;
+	struct device_node *child = NULL;
 	struct device_node *ep_np = NULL;
+	unsigned int i2c_mux_mask = 0;
 	int ret;
 
+	/* Balance the of_node_put() performed by of_find_node_by_name() */
+	of_node_get(dev->of_node);
+	i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
+	if (!i2c_mux) {
+		dev_err(dev, "Failed to find i2c-mux node\n");
+		return -EINVAL;
+	}
+
+	/* Identify which i2c-mux channels are enabled */
+	for_each_child_of_node(i2c_mux, child) {
+		u32 id = 0;
+
+		if (of_node_cmp(child->name, "i2c") != 0)
+			continue;
+
+		of_property_read_u32(child, "reg", &id);
+		if (id >= MAX9286_NUM_GMSL)
+			continue;
+
+		if (!of_device_is_available(child)) {
+			dev_dbg(dev, "Skipping port %u with disabled I2C bus\n", id);
+			continue;
+		}
+
+		i2c_mux_mask |= BIT(id);
+	}
+	of_node_put(child);
+	of_node_put(i2c_mux);
+
 	v4l2_async_notifier_init(&priv->notifier);
 
+	/* Parse the endpoints */
 	for_each_endpoint_of_node(dev->of_node, ep_np) {
 		struct max9286_source *source;
 		struct of_endpoint ep;
@@ -1214,7 +1198,7 @@  static int max9286_parse_dt(struct max9286_priv *priv)
 		}
 
 		/* Skip if the corresponding GMSL link is unavailable. */
-		if (max9286_check_i2c_bus_by_id(dev, ep.port))
+		if (!(i2c_mux_mask & BIT(ep.port)))
 			continue;
 
 		if (priv->sources[ep.port].fwnode) {