Re: [PATCH 3/6] usb: phy: samsung: Consolidate reference clock rate handling

[Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3267 bytes --]

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;
| +               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;
| +               break;
| +       case 19200 * KHZ:
| +               clksel = FSEL_CLKSEL_19200K;
| +               break;
| +       case 20 * MHZ:
| +               clksel = FSEL_CLKSEL_20M;
| +               break;
| +       case 24 * MHZ:
| +               clksel = FSEL_CLKSEL_24M;
| +               break;
| +       case 50 * MHZ:
| +               clksel = FSEL_CLKSEL_50M;
| +               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_4x12);

They both return the same thing and test the same thing. You clearly
don't need this function pointer. The only thing you need to be careful
is that different platforms will have different clock rates, but that
can just as easily be turned into a generic check.

I don't see the need for $SUBJECT.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]