Hi Cyril,
On Tue, May 18, 2010 at 19:28:28, Chemparathy, Cyril wrote:
> Hi Sekhar,
>
> Thanks for looking at this.
>
> [...]
> >> +#define MDIO_OUT_FREQ 1100000 /* 2.2 MHz */
> >
> > The comment says 2.2 MHz, but the value is 1.1 MHz?
>
> Thanks.
>
> > Also, can you keep this as a platform variable (with
> > a 2.2 MHz default)? The frequency depends on the board,
> > and although most boards work at 2.2 MHz, not having it
> > as a platform variable will make adding a board with a
> > different frequency requirement difficult.
>
> I am not quite convinced that you'll have too many boards deviate from
> the 2MHz ballpark. That seems to be a nice and safe frequency that
> works well across phys, socs, and boards.
>
> That said, if we see the need to override the bus frequency in future, I
> am all for a patch at that time. As it stands, I don't see the value in
> adding platform data definitions for a capability that is not going to
> be used at present.
Okay, but why regress on existing functionality? That frequency
is a function of board design anyway so having the driver hardcode
that would not be correct.
>
> [...]
> >> + struct {
> >> + u32 access;
> >> +#define USERACCESS_GO BIT(31)
> >> +#define USERACCESS_WRITE BIT(30)
> >> +#define USERACCESS_ACK BIT(29)
> >> +#define USERACCESS_READ (0)
> >> +#define USERACCESS_DATA (0xffff)
> >> +
> >> + u32 physel;
> >> + } user[0];
> >> +};
> >
> > Structure overlays are usually not preferred and
> > is difficult to debug. Can you please keep the
> > direct register access method that was being
> > followed in the original driver?
>
> Agreed, will change since the multiple user modules are not being used
> anyway.
Hmm, just curious how is this related to 'multiple user (mdio?) modules'?
[...]
>
> >> + data->bus->phy_mask = phy_mask;
> >
> > Looks like phy_mask local variable can be eliminated?
>
> Not at the expense of readability. I'd rather not have
> data->bus->phy_mask references all over.
Your call.
Thanks,
Sekhar
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
