Re: [PATCH] max9286: simplify i2c-mux parsing

From: Kieran Bingham <kieran.bingham@ideasonboard.com>
To: Jacopo Mondi <jacopo@jmondi.org>
Cc: linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH] max9286: simplify i2c-mux parsing
Date: Fri, 29 Nov 2019 11:45:55 +0000	[thread overview]
Message-ID: <a951c2c4-b590-491e-77e1-2a8d2793c6f8@ideasonboard.com> (raw)
In-Reply-To: <20191121173541.sb4ycbinjkhvzmso@uno.localdomain>

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
>>

-- 
Regards
--
Kieran