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

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

 



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


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

  Powered by Linux