| Message ID | 20191121165631.5744-1-kieran.bingham@ideasonboard.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | max9286: simplify i2c-mux parsing | expand |
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 >
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 >>
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) {
- 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(-)