Hi Shimoda-San,
Thanks for the feedback.
> Subject: RE: [PATCH V4 04/13] phy: rcar-gen3-usb2: Add support for r8a77470
>
> Hi Biju-san,
>
> Thank you for the patch!
>
> > From: Biju Das, Sent: Wednesday, April 10, 2019 5:08 PM
> >
> > This patch adds support for r8a77470 (RZ/G1C). We can reuse this
> > driver for initializing timing/interrupt generation registers.
> >
> > Signed-off-by: Biju Das <biju.das@xxxxxxxxxxxxxx>
> > ---
> <snip>
> > diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> > b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> > index 0a34782..5a8e673 100644
> > --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> > +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> > @@ -22,6 +22,7 @@
> > #include <linux/regulator/consumer.h> #include <linux/usb/of.h>
> > #include <linux/workqueue.h>
> > +#include <linux/sys_soc.h>
>
> We don't need it.
OK . will take this out.
> > /******* USB2.0 Host registers (original offset is +0x200) *******/
> > #define USB2_INT_ENABLE 0x000
> > @@ -393,6 +394,12 @@ static const struct phy_ops
> rcar_gen3_phy_usb2_ops = {
> > .owner = THIS_MODULE,
> > };
> >
> > +static const struct phy_ops rz_g1c_phy_usb2_ops = {
> > + .init = rcar_gen3_phy_usb2_init,
> > + .exit = rcar_gen3_phy_usb2_exit,
> > + .owner = THIS_MODULE,
> > +};
> > +
> > static irqreturn_t rcar_gen3_phy_usb2_irq(int irq, void *_ch)
> > {
> > struct rcar_gen3_chan *ch = _ch;
> > @@ -411,11 +418,27 @@ static irqreturn_t rcar_gen3_phy_usb2_irq(int irq,
> void *_ch)
> > }
> >
> > static const struct of_device_id rcar_gen3_phy_usb2_match_table[] = {
> > - { .compatible = "renesas,usb2-phy-r8a7795" },
> > - { .compatible = "renesas,usb2-phy-r8a7796" },
> > - { .compatible = "renesas,usb2-phy-r8a77965" },
> > - { .compatible = "renesas,rcar-gen3-usb2-phy" },
> > - { }
> > + {
> > + .compatible = "renesas,usb2-phy-r8a77470",
> > + .data = &rz_g1c_phy_usb2_ops,
> > + },
> > + {
> > + .compatible = "renesas,usb2-phy-r8a7795",
> > + .data = &rcar_gen3_phy_usb2_ops,
> > + },
> > + {
> > + .compatible = "renesas,usb2-phy-r8a7796",
> > + .data = &rcar_gen3_phy_usb2_ops,
> > + },
> > + {
> > + .compatible = "renesas,usb2-phy-r8a77965",
> > + .data = &rcar_gen3_phy_usb2_ops,
> > + },
> > + {
> > + .compatible = "renesas,rcar-gen3-usb2-phy",
> > + .data = &rcar_gen3_phy_usb2_ops,
> > + },
> > + { /* sentinel */ },
> > };
> > MODULE_DEVICE_TABLE(of, rcar_gen3_phy_usb2_match_table);
> >
> > @@ -431,6 +454,7 @@ static int rcar_gen3_phy_usb2_probe(struct
> platform_device *pdev)
> > struct rcar_gen3_chan *channel;
> > struct phy_provider *provider;
> > struct resource *res;
> > + const struct phy_ops *phy_usb2_ops;
> > int irq, ret = 0;
> >
> > if (!dev->of_node) {
> > @@ -481,7 +505,9 @@ static int rcar_gen3_phy_usb2_probe(struct
> platform_device *pdev)
> > * And then, phy-core will manage runtime pm for this device.
> > */
> > pm_runtime_enable(dev);
> > - channel->phy = devm_phy_create(dev, NULL,
> &rcar_gen3_phy_usb2_ops);
> > + phy_usb2_ops = (const struct phy_ops *)
> of_device_get_match_data(dev);
>
> We don't need the cast. Also, I think we have to check the phy_usb2_ops is
> not NULL.
OK. I have checked other rcar drivers and came to the conclusion that there is need for Null check(for eg:- renesas-cpg-mssr.c, gpio-rcar.c)
Any way there is no harm in adding this check, since the api can return null value.
I will send V5 As you suggested. ie, will remove the cast and will add check for not NULL.
if (!phy_usb2_ops)
return -EINVAL;
> After they are fixed:
>
> Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
>
> Best regards,
> Yoshihiro Shimoda
>
> > + channel->phy = devm_phy_create(dev, NULL, phy_usb2_ops);
> > if (IS_ERR(channel->phy)) {
> > dev_err(dev, "Failed to create USB2 PHY\n");
> > ret = PTR_ERR(channel->phy);
> > --
Regards,
Biju
