Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1644246970-18305-1-git-send-email-u0084500@gmail.com>
 <1644246970-18305-3-git-send-email-u0084500@gmail.com> <YgO4+mB5xJ5DbX61@kuha.fi.intel.com>
In-Reply-To: <YgO4+mB5xJ5DbX61@kuha.fi.intel.com>
From:   ChiYuan Huang <u0084500@gmail.com>
Date:   Wed, 9 Feb 2022 21:26:14 +0800
Message-ID: <CADiBU3_GuHapJMUen+Ag_JaapzbN6CaAm+RNxN8E16yg5k-O0g@mail.gmail.com>
Subject: Re: [PATCH v3 2/2] usb: typec: rt1719: Add support for Richtek RT1719
To:     Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Greg KH <gregkh@linuxfoundation.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        Linux USB List <linux-usb@vger.kernel.org>,
        lkml <linux-kernel@vger.kernel.org>,
        cy_huang <cy_huang@richtek.com>, will_lin@richtek.com,
        th_chuang@richtek.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Heikki Krogerus <heikki.krogerus@linux.intel.com> =E6=96=BC 2022=E5=B9=B42=
=E6=9C=889=E6=97=A5 =E9=80=B1=E4=B8=89 =E4=B8=8B=E5=8D=888:52=E5=AF=AB=E9=
=81=93=EF=BC=9A
>
> 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 =3D { };
> > +     int ret;
> > +
> > +     data =3D devm_kzalloc(&i2c->dev, sizeof(*data), GFP_KERNEL);
> > +     if (!data)
> > +             return -ENOMEM;
> > +
> > +     data->dev =3D &i2c->dev;
> > +     init_completion(&data->req_completion);
> > +
> > +     data->regmap =3D devm_regmap_init_i2c(i2c, &rt1719_regmap_config)=
;
> > +     if (IS_ERR(data->regmap)) {
> > +             ret =3D PTR_ERR(data->regmap);
> > +             dev_err(&i2c->dev, "Failed to init regmap (%d)\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     ret =3D rt1719_check_exist(data);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret =3D rt1719_get_caps(data);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /*
> > +      * This fwnode has a "compatible" property, but is never populate=
d as a
> > +      * struct device. Instead we simply parse it to read the properti=
es.
> > +      * This it breaks fw_devlink=3Don. To maintain backward compatibi=
lity
> > +      * with existing DT files, we work around this by deleting any
> > +      * fwnode_links to/from this fwnode.
> > +      */
> > +     fwnode =3D 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 =3D fwnode_usb_role_switch_get(fwnode);
> > +     if (IS_ERR(data->role_sw)) {
> > +             ret =3D PTR_ERR(data->role_sw);
> > +             dev_err(&i2c->dev, "Failed to get usb role switch (%d)\n"=
, ret);
> > +             goto err_fwnode_put;
> > +     }
> > +
> > +     ret =3D 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 =3D USB_TYPEC_REV_1_2;
> > +     typec_cap.pd_revision =3D 0x300;  /* USB-PD spec release 3.0 */
> > +     typec_cap.type =3D TYPEC_PORT_SNK;
> > +     typec_cap.data =3D TYPEC_PORT_DRD;
> > +     typec_cap.ops =3D &rt1719_port_ops;
> > +     typec_cap.fwnode =3D fwnode;
> > +     typec_cap.driver_data =3D data;
> > +     typec_cap.accessory[0] =3D TYPEC_ACCESSORY_DEBUG;
> > +
> > +     data->partner_desc.identity =3D &data->partner_ident;
> > +
> > +     data->port =3D typec_register_port(&i2c->dev, &typec_cap);
> > +     if (IS_ERR(data->port)) {
> > +             ret =3D PTR_ERR(data->port);
> > +             dev_err(&i2c->dev, "Failed to register typec port (%d)\n"=
, ret);
> > +             goto err_role_put;
> > +     }
> > +
> > +     ret =3D rt1719_init_attach_state(data);
> > +     if (ret) {
> > +             dev_err(&i2c->dev, "Failed to init attach state (%d)\n", =
ret);
> > +             goto err_role_put;
> > +     }
> > +
> > +     ret =3D 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 =3D 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[] =
=3D {
> > +     { .compatible =3D "richtek,rt1719", },
> > +     { }
> > +};
> > +MODULE_DEVICE_TABLE(of, rt1719_device_table);
> > +
> > +static struct i2c_driver rt1719_driver =3D {
> > +     .driver =3D {
> > +             .name =3D "rt1719",
> > +             .of_match_table =3D rt1719_device_table,
> > +     },
> > +     .probe_new =3D rt1719_probe,
> > +     .remove =3D rt1719_remove,
> > +};
> > +module_i2c_driver(rt1719_driver);
> > +
> > +MODULE_AUTHOR("ChiYuan Huang <cy_huang@richtek.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