linux-kernel - RE: [PATCH V5 4/8] phy: st-miphy-40lp: Add skeleton driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 11 Feb 2014 11:57:46 +0800
From:	Mohit KUMAR DCG <Mohit.KUMAR@...com>
To:	Arnd Bergmann <arnd@...db.de>
Cc:	Pratyush ANAND <pratyush.anand@...com>,
	Kishon Vijay Abraham I <kishon@...com>,
	spear-devel <spear-devel@...t.st.com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH V5 4/8] phy: st-miphy-40lp: Add skeleton driver

Hello Arnd,

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@...db.de]
> Sent: Monday, February 10, 2014 9:24 PM
> To: Mohit KUMAR DCG
> Cc: Pratyush ANAND; Kishon Vijay Abraham I; spear-devel; linux-arm-
> kernel@...ts.infradead.org; devicetree@...r.kernel.org; linux-
> kernel@...r.kernel.org
> Subject: Re: [PATCH V5 4/8] phy: st-miphy-40lp: Add skeleton driver
> 
> On Monday 10 February 2014, Mohit Kumar wrote:
> > diff --git a/Documentation/devicetree/bindings/phy/st-miphy40lp.txt
> > b/Documentation/devicetree/bindings/phy/st-miphy40lp.txt
> > new file mode 100644
> > index 0000000..d0c7096
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/st-miphy40lp.txt
> > @@ -0,0 +1,12 @@
> > +Required properties:
> > +- compatible : should be "st,miphy40lp-phy"
> > +	Other supported soc specific compatible:
> > +		"st,spear1310-miphy"
> > +		"st,spear1340-miphy"
> > +- reg : offset and length of the PHY register set.
> > +- misc: phandle for the syscon node to access misc registers
> > +- phy-id: Instance id of the phy.
> > +- #phy-cells : from the generic PHY bindings, must be 1.
> > +	- 1st cell: phandle to the phy node.
> > +	- 2nd cell: 0 if phy (in 1st cell) is to be used for SATA, 1 for PCIe
> > +	  and 2 for Super Speed USB.
> 
> It's common to start this file with a small header explaining what this
> hardware is.

- OK

> 
> > diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig index
> > afa2354..2f58993 100644
> > --- a/drivers/phy/Kconfig
> > +++ b/drivers/phy/Kconfig
> > @@ -64,4 +64,10 @@ config BCM_KONA_USB2_PHY
> >  	help
> >  	  Enable this to support the Broadcom Kona USB 2.0 PHY.
> >
> > +config PHY_ST_MIPHY40LP
> > +	tristate "ST MIPHY 40LP driver"
> > +	help
> > +	  Support for ST MIPHY 40LP which can be used for PCIe, SATA and
> Super Speed USB.
> > +	select GENERIC_PHY
> > +
> >  endmenu
> 
> The 'select' statement should come before 'help', for consistency with the
> rest of the kernel.

- OK

> Maybe mention that this phy is used inside the spear13xx
> SoC here rather than a standalone phy.

- Yes, for spear13xx its used internally. Do you think that it requires to be mentioned here? 
We have few prototype boards that uses this as external phy.

> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv) {
> > +		dev_err(dev, "can't alloc miphy40lp private date
> memory\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	priv->plat_ops = (struct miphy40lp_plat_ops *)of_id->data;
> 
> The cast would incorrectly remove the 'const' attribute of the pointer.
> Better remove the cast and make priv->plat_ops const.

- OK

> 
> > +static int __init miphy40lp_phy_init(void) {
> > +
> > +	return platform_driver_probe(&miphy40lp_driver,
> > +				miphy40lp_probe);
> > +}
> > +module_init(miphy40lp_phy_init);
> 
> There should certainly be a module_exit() function here so you can unload
> the driver.

- yes, will add it in v6.

Thanks
Mohit

> 
> 	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/