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