Hi,
On Wed, Mar 27, 2013 at 02:39:08PM +0100, Tomasz Figa wrote:
> On Wednesday 27 of March 2013 15:31:49 Felipe Balbi wrote:
> > Hi,
> >
> > On Wed, Mar 27, 2013 at 02:26:08PM +0100, Tomasz Figa wrote:
> > > Hi Felipe,
> > >
> > > On Wednesday 27 of March 2013 15:19:58 Felipe Balbi wrote:
> > > > Hi,
> > > >
> > > > On Tue, Mar 26, 2013 at 03:53:12PM +0100, Tomasz Figa wrote:
> > > > > @@ -307,6 +310,7 @@ static int samsung_usb3phy_remove(struct
> > > > > platform_device *pdev)>
> > > > >
> > > > > static struct samsung_usbphy_drvdata usb3phy_exynos5 = {
> > > > >
> > > > > .cpu_type = TYPE_EXYNOS5250,
> > > > > .devphy_en_mask = EXYNOS_USBPHY_ENABLE,
> > > > >
> > > > > + .rate_to_clksel = samsung_usbphy_rate_to_clksel_4x12,
> > > >
> > > > why isn't this just a clk_get_rate() ???
> > >
> > > As the name suggests, this is a function to get appropriate CLKSEL value
> > > to
> > > write into PHYCLK register for reference clock rate on particular platform
> > > (clk_get_rate is used inside to get the rate).
> >
> > alright, then you don't need this function pointer at all, look at both
> >
> > your rate_to_clksel functions (quoted below):
> > | +int samsung_usbphy_rate_to_clksel_64xx(struct samsung_usbphy *sphy,
> > | + unsigned long
> > | rate)
> > | +{
> > | + unsigned int clksel;
> > | +
> > | + switch (rate) {
> > | + case 12 * MHZ:
> > | + clksel = PHYCLK_CLKSEL_12M;
>
> Please note the PHYCLK_CLKSEL_ prefix here...
>
> > | + break;
> > | + case 24 * MHZ:
> > | + clksel = PHYCLK_CLKSEL_24M;
> > | + break;
> > | + case 48 * MHZ:
> > | + clksel = PHYCLK_CLKSEL_48M;
> > | + break;
> > | + default:
> > | + dev_err(sphy->dev,
> > | + "Invalid reference clock frequency: %lu\n", rate);
> > | + return -EINVAL;
> > | + }
> > | +
> > | + return clksel;
> > | +}
> > | +EXPORT_SYMBOL_GPL(samsung_usbphy_rate_to_clksel_64xx);
> > | +
> > | +int samsung_usbphy_rate_to_clksel_4x12(struct samsung_usbphy *sphy,
> > | + unsigned long
> > | rate)
> > | +{
> > | + unsigned int clksel;
> > | +
> > | + switch (rate) {
> > | + case 9600 * KHZ:
> > | + clksel = FSEL_CLKSEL_9600K;
> > | + break;
> > | + case 10 * MHZ:
> > | + clksel = FSEL_CLKSEL_10M;
> > | + break;
> > | + case 12 * MHZ:
> > | + clksel = FSEL_CLKSEL_12M;
>
> ..and then FSEL_CLKSEL_ here. They have different values. (Their names are a
> bit unfortunate, though...)
indeed, my eyes failed there. So I agree with the patch :-)
sorry for the noise.
--
balbi
Attachment:
signature.asc
Description: Digital signature
