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.
[...]
>> + 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.
[...]
>> + while ((reg = __raw_readl(®s->user[0].access)) & USERACCESS_GO)
>> + ;
>
> I know the original code had this issue too, but there should be a
> timeout here to avoid permanent lockup. Also, a cpu_relax() while
> looping tight.
True. Thanks.
[...]
>> +static int davinci_mdio_read(struct mii_bus *bus, int phy_id, int phy_reg)
>> +{
>> + struct davinci_mdio_data *data = bus->priv;
>> + u32 reg;
>> +
>> + if (phy_reg & ~PHY_REG_MASK || phy_id & ~PHY_ID_MASK)
>> + return -EINVAL;
>> +
>> + spin_lock(&data->lock);
>
> The mdiobus serializes the bus access anyway so is this
> lock really needed?
Not needed.
>> +
>> + if (data->suspended) {
>> + spin_unlock(&data->lock);
>> + return -ENODEV;
>> + }
>
> Hmm, the devices should be suspended before the bus is.
> So, there should probably be a WARN_ON() here since this
> indicates some issue elsewhere in the system.
>
> Or may be this is too paranoid and can be rid of altogether?
I wasn't entirely sure of the suspend sequence (and hence the paranoia).
[...]
>> +static inline int count_bits(u32 data)
>> +{
>> + data = ((data & 0xaaaaaaaa) >> 1) + (data & 0x55555555);
>> + data = ((data & 0xcccccccc) >> 2) + (data & 0x33333333);
>> + data = ((data & 0xf0f0f0f0) >> 4) + (data & 0x0f0f0f0f);
>> + data = ((data & 0xff00ff00) >> 8) + (data & 0x00ff00ff);
>> + data = ((data & 0xffff0000) >> 16) + (data & 0x0000ffff);
>> + return data;
>> +}
>
> Kernel already has a function to calculate the hamming weight
> on u32. See hweight32() in lib/hweight.c
Will modify. Thanks.
[...]
>> + mdio_in_freq = clk_get_rate(data->clk);
>> + div = (mdio_in_freq / MDIO_OUT_FREQ) - 1;
>> + if (div > CONTROL_MAX_DIV)
>> + div = CONTROL_MAX_DIV;
>
> Hmm, just wondering if it is safe to just default on
> the max possible value? It could be a result of an issue
> elsewhere and so should be reported?
Should spew a warn at the least.
[...]
>> + if (phy_mask) {
>> + /* restrict mdio bus to live phys only */
>> + dev_info(dev, "detected %d phys (mask %x)\n",
>> + count_bits(phy_mask), ~phy_mask);
>> + phy_mask = ~phy_mask;
>> + } else {
>> + /* desperately scan all phys */
>> + dev_warn(dev, "failed to detect live phys, scanning all\n");
>> + phy_mask = 0;
>
> phy_mask is already zero.
Indeed, it is! :-)
>> + 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.
[...]
>> + kfree(data);
>> +
>> + return 0;
>
> return ret?
Absolutely, what was I thinking?
Regards
Cyril.
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
