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@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 ?
> + 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
>
Attachment:
signature.asc
Description: PGP signature
