RE: [EXT] Re: [PATCH v16 7/8] i3c: hub: p3h2x4x: Add support for NXP P3H2x4x I3C hub functionality

[Lakshay Piplani]

Hi Krzysztof,

Thanks for the review.

> > +static void p3h2x4x_get_target_port_dt_conf(struct device *dev,
> > +                                         const struct device_node
> > +*node) {
> > +     struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub = dev_get_drvdata(dev);
> > +     struct p3h2x4x *p3h2x4x = dev_get_drvdata(dev->parent);
> > +     u64 tp_port;
> > +
> > +     for_each_available_child_of_node_scoped(node, dev_node) {
>
> Why do you need scoped loop?

I used the scoped form to keep the iterator local to the loop and
consistent with p3h2x4x_parse_tp_dt_settings(), where it handles
node-reference cleanup on early returns. This loop has no
early exits, so the ordinary iterator would also work. I'd prefer to
retain the scoped form for consistency across these helpers.

>
> > +             if (of_property_read_reg(dev_node, 0, &tp_port, NULL))
> > +                     continue;
> > +
> > +             if (tp_port < p3h2x4x->num_target_ports) {
> > +                     if (p3h2x4x_i3c_hub->tp_bus[tp_port].of_node) {
> > +                             dev_warn(dev, "Duplicate target port %llu in DT\n",
> tp_port);
> > +                             continue;
> > +                     }
> > +
> > +                     p3h2x4x_i3c_hub->tp_bus[tp_port].of_node =
> of_node_get(dev_node);
> > +                     p3h2x4x_i3c_hub->tp_bus[tp_port].tp_mask =
> P3H2X4X_SET_BIT(tp_port);
> > +                     p3h2x4x_i3c_hub->tp_bus[tp_port].p3h2x4x_i3c_hub =
> p3h2x4x_i3c_hub;
> > +                     p3h2x4x_i3c_hub->tp_bus[tp_port].tp_port = tp_port;
> > +             }
> > +     }
> > +}
> > +
> > +static const struct platform_device_id p3h2x4x_i3c_hub_id[] = {
> > +     { "p3h2x4x-i3c-hub" },
>
> Use named initializers. In every patch of yours.
>

I'll convert the remaining positional device-ID initializers across
the series.

> > +     { }
> > +};
>
> ..
>
>
> > +
> > +/**
> > + * p3h2x4x_tp_i3c_algo - Register I3C virtual masters for I3C target ports.
> > + * @p3h2x4x_hub: p3h2x4x device structure.
> > + * Return: 0 in case of success, negative error code on failure.
> > + */
> > +int p3h2x4x_tp_i3c_algo(struct p3h2x4x_i3c_hub_dev *p3h2x4x_hub) {
> > +     struct i3c_master_controller *parent =
> i3c_dev_get_master(p3h2x4x_hub->i3cdev->desc);
> > +     struct p3h2x4x *p3h2x4x = dev_get_drvdata(p3h2x4x_hub->dev-
> >parent);
> > +     u8 tp, ntwk_mask = 0;
> > +     int ret;
> > +
> > +     p3h2x4x_hub->hub = devm_kzalloc(p3h2x4x_hub->dev,
> > +                                     sizeof(*p3h2x4x_hub->hub),
> > +                                     GFP_KERNEL);
> > +
>
> In multiple places you added blank lines between the call and if() check. Don't.
>

I'll remove those gaps throughout the series.

> > +     if (!p3h2x4x_hub->hub)
> > +             return -ENOMEM;

Thanks,
Lakshay

NXP Public