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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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...)

Best regards,
-- 
Tomasz Figa
Samsung Poland R&D Center
SW Solution Development, Kernel and System Framework

> | +               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.

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux