Date:   Wed, 9 Feb 2022 21:26:14 +0800
From:   ChiYuan Huang <u0084500@...il.com>
To:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        Linux USB List <linux-usb@...r.kernel.org>,
        lkml <linux-kernel@...r.kernel.org>,
        cy_huang <cy_huang@...htek.com>, will_lin@...htek.com,
        th_chuang@...htek.com
Subject: Re: [PATCH v3 2/2] usb: typec: rt1719: Add support for Richtek RT1719

Heikki Krogerus <heikki.krogerus@...ux.intel.com> 於 2022年2月9日 週三 下午8:52寫道：
>
> Hi,
>
> Sorry, there is still one more problem that I realised - not your
> problem, but a problem with fw_devlink_purge_absent_suppliers()...
>
> On Mon, Feb 07, 2022 at 11:16:10PM +0800, cy_huang wrote:
> > +static int rt1719_probe(struct i2c_client *i2c)
> > +{
> > +     struct rt1719_data *data;
> > +     struct fwnode_handle *fwnode;
> > +     struct typec_capability typec_cap = { };
> > +     int ret;
> > +
> > +     data = devm_kzalloc(&i2c->dev, sizeof(*data), GFP_KERNEL);
> > +     if (!data)
> > +             return -ENOMEM;
> > +
> > +     data->dev = &i2c->dev;
> > +     init_completion(&data->req_completion);
> > +
> > +     data->regmap = devm_regmap_init_i2c(i2c, &rt1719_regmap_config);
> > +     if (IS_ERR(data->regmap)) {
> > +             ret = PTR_ERR(data->regmap);
> > +             dev_err(&i2c->dev, "Failed to init regmap (%d)\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     ret = rt1719_check_exist(data);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = rt1719_get_caps(data);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /*
> > +      * This fwnode has a "compatible" property, but is never populated as a
> > +      * struct device. Instead we simply parse it to read the properties.
> > +      * This it breaks fw_devlink=on. To maintain backward compatibility
> > +      * with existing DT files, we work around this by deleting any
> > +      * fwnode_links to/from this fwnode.
> > +      */
> > +     fwnode = device_get_named_child_node(&i2c->dev, "connector");
> > +     if (!fwnode)
> > +             return -ENODEV;
> > +
> > +     fw_devlink_purge_absent_suppliers(fwnode);
>
> So don't use that function unless you really see some issue that it's
> fixing yourself.
>
> That function is broken. It breaks at least if the fwnode is shared -
> fwnodes can be, and are, shared - most likely it's broken in
> some other ways too. That function seems to be a hack for some
> individual problem that was caused by some deeper problem with the
> device links.
>
> We really need to figure out a fix for the core problem now instead of
> trying to come up with these hacks for every single separate scenario
> that is breaking because of the core problem, what ever that core
> problem may be.
>
OK, I'll remove it.

Originally, I think the others use it to solve fwlink problem.
But I didn't realize actually it's a workaround.
> > +     data->role_sw = fwnode_usb_role_switch_get(fwnode);
> > +     if (IS_ERR(data->role_sw)) {
> > +             ret = PTR_ERR(data->role_sw);
> > +             dev_err(&i2c->dev, "Failed to get usb role switch (%d)\n", ret);
> > +             goto err_fwnode_put;
> > +     }
> > +
> > +     ret = devm_rt1719_psy_register(data);
> > +     if (ret) {
> > +             dev_err(&i2c->dev, "Failed to register psy (%d)\n", ret);
> > +             goto err_role_put;
> > +     }
> > +
> > +     typec_cap.revision = USB_TYPEC_REV_1_2;
> > +     typec_cap.pd_revision = 0x300;  /* USB-PD spec release 3.0 */
> > +     typec_cap.type = TYPEC_PORT_SNK;
> > +     typec_cap.data = TYPEC_PORT_DRD;
> > +     typec_cap.ops = &rt1719_port_ops;
> > +     typec_cap.fwnode = fwnode;
> > +     typec_cap.driver_data = data;
> > +     typec_cap.accessory[0] = TYPEC_ACCESSORY_DEBUG;
> > +
> > +     data->partner_desc.identity = &data->partner_ident;
> > +
> > +     data->port = typec_register_port(&i2c->dev, &typec_cap);
> > +     if (IS_ERR(data->port)) {
> > +             ret = PTR_ERR(data->port);
> > +             dev_err(&i2c->dev, "Failed to register typec port (%d)\n", ret);
> > +             goto err_role_put;
> > +     }
> > +
> > +     ret = rt1719_init_attach_state(data);
> > +     if (ret) {
> > +             dev_err(&i2c->dev, "Failed to init attach state (%d)\n", ret);
> > +             goto err_role_put;
> > +     }
> > +
> > +     ret = rt1719_irq_init(data);
> > +     if (ret) {
> > +             dev_err(&i2c->dev, "Failed to init irq\n");
> > +             goto err_role_put;
> > +     }
> > +
> > +     fwnode_handle_put(fwnode);
> > +
> > +     i2c_set_clientdata(i2c, data);
> > +
> > +     return 0;
> > +
> > +err_role_put:
> > +     usb_role_switch_put(data->role_sw);
> > +err_fwnode_put:
> > +     fwnode_handle_put(fwnode);
> > +
> > +     return ret;
> > +}
> > +
> > +static int rt1719_remove(struct i2c_client *i2c)
> > +{
> > +     struct rt1719_data *data = i2c_get_clientdata(i2c);
> > +
> > +     typec_unregister_port(data->port);
> > +     usb_role_switch_put(data->role_sw);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct of_device_id __maybe_unused rt1719_device_table[] = {
> > +     { .compatible = "richtek,rt1719", },
> > +     { }
> > +};
> > +MODULE_DEVICE_TABLE(of, rt1719_device_table);
> > +
> > +static struct i2c_driver rt1719_driver = {
> > +     .driver = {
> > +             .name = "rt1719",
> > +             .of_match_table = rt1719_device_table,
> > +     },
> > +     .probe_new = rt1719_probe,
> > +     .remove = rt1719_remove,
> > +};
> > +module_i2c_driver(rt1719_driver);
> > +
> > +MODULE_AUTHOR("ChiYuan Huang <cy_huang@...htek.com>");
> > +MODULE_DESCRIPTION("Richtek RT1719 Sink Only USBPD Controller Driver");
> > +MODULE_LICENSE("GPL v2");
>
> Oh, you probable want the make that "GPL" instead. See
> Documentation/process/license-rules.rst.
>
Ack in next.
> thanks,
>
> --
> heikki

Powered by blists - more mailing lists