[OpenWrt-Devel] [PATCH 1/2] b53: add internal API for accessing PHY regs

[Jonas Gorski]

Hi,

On 6 January 2016 at 20:02, Rafał Miłecki <zajec5 at gmail.com> wrote:
> Signed-off-by: Rafał Miłecki <zajec5 at gmail.com>

Please don't leave the commit log empty. E.g. state why you need to
add this new function.
> ---
>  .../generic/files/drivers/net/phy/b53/b53_mdio.c     | 20 ++++++++++++++++++++
>  .../generic/files/drivers/net/phy/b53/b53_priv.h     | 14 ++++++++++++++
>  .../generic/files/drivers/net/phy/b53/b53_srab.c     | 15 +++++++++++++++
>  3 files changed, 49 insertions(+)
>
> diff --git a/target/linux/generic/files/drivers/net/phy/b53/b53_mdio.c b/target/linux/generic/files/drivers/net/phy/b53/b53_mdio.c
> index 3c25f0e..185c95f 100644
> --- a/target/linux/generic/files/drivers/net/phy/b53/b53_mdio.c
> +++ b/target/linux/generic/files/drivers/net/phy/b53/b53_mdio.c
> @@ -238,6 +238,24 @@ static int b53_mdio_write64(struct b53_device *dev, u8 page, u8 reg,
>         return b53_mdio_op(dev, page, reg, REG_MII_ADDR_WRITE);
>  }
>
> +static int b53_mdio_phy_read16(struct b53_device *dev, int addr, u8 reg,
> +                              u16 *value)
> +{
> +       struct mii_bus *bus = dev->priv;
> +
> +       *value = mdiobus_read(bus, addr, reg);
> +
> +       return 0;
> +}
> +
> +static int b53_mdio_phy_write16(struct b53_device *dev, int addr, u8 reg,
> +                               u16 value)
> +{
> +       struct mii_bus *bus = dev->priv;
> +
> +       return mdiobus_write(bus, addr, reg, value);
> +}
> +
>  static struct b53_io_ops b53_mdio_ops = {
>         .read8 = b53_mdio_read8,
>         .read16 = b53_mdio_read16,
> @@ -249,6 +267,8 @@ static struct b53_io_ops b53_mdio_ops = {
>         .write32 = b53_mdio_write32,
>         .write48 = b53_mdio_write48,
>         .write64 = b53_mdio_write64,
> +       .phy_read16 = b53_mdio_phy_read16,
> +       .phy_write16 = b53_mdio_phy_write16,
>  };
>
>  static int b53_phy_probe(struct phy_device *phydev)
> diff --git a/target/linux/generic/files/drivers/net/phy/b53/b53_priv.h b/target/linux/generic/files/drivers/net/phy/b53/b53_priv.h
> index 0c4c394..7891238 100644
> --- a/target/linux/generic/files/drivers/net/phy/b53/b53_priv.h
> +++ b/target/linux/generic/files/drivers/net/phy/b53/b53_priv.h
> @@ -36,6 +36,8 @@ struct b53_io_ops {
>         int (*write32)(struct b53_device *dev, u8 page, u8 reg, u32 value);
>         int (*write48)(struct b53_device *dev, u8 page, u8 reg, u64 value);
>         int (*write64)(struct b53_device *dev, u8 page, u8 reg, u64 value);
> +       int (*phy_read16)(struct b53_device *dev, int addr, u8 reg, u16 *value);
> +       int (*phy_write16)(struct b53_device *dev, int addr, u8 reg, u16 value);
>  };
>
>  enum {
> @@ -299,6 +301,18 @@ static inline int b53_write64(struct b53_device *dev, u8 page, u8 reg,
>         return ret;
>  }
>
> +static inline int b53_phy_read16(struct b53_device *dev, int addr, u8 reg,
> +                                u16 *value)
> +{
> +       return dev->ops->phy_read16(dev, addr, reg, value);

Since you only add the functions to phy and srab and you don't check
for NULL, this will OOPS for spi and mmap.

> +}
> +
> +static inline int b53_phy_write16(struct b53_device *dev, int addr, u8 reg,
> +                                 u16 value)
> +{
> +       return dev->ops->phy_write16(dev, addr, reg, value);
> +}
> +
>  #ifdef CONFIG_BCM47XX
>
>  #include <linux/version.h>
> diff --git a/target/linux/generic/files/drivers/net/phy/b53/b53_srab.c b/target/linux/generic/files/drivers/net/phy/b53/b53_srab.c
> index 012daa3..438c6f8 100644
> --- a/target/linux/generic/files/drivers/net/phy/b53/b53_srab.c
> +++ b/target/linux/generic/files/drivers/net/phy/b53/b53_srab.c
> @@ -21,6 +21,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/platform_data/b53.h>
>
> +#include "b53_regs.h"
>  #include "b53_priv.h"
>
>  /* command and status register of the SRAB */
> @@ -321,6 +322,18 @@ err:
>         return ret;
>  }
>
> +static int b53_srab_phy_read16(struct b53_device *dev, int addr, u8 reg,
> +                              u16 *value)
> +{
> +       return b53_read16(dev, B53_PORT_MII_PAGE(addr), reg, value);
> +}
> +
> +static int b53_srab_phy_write16(struct b53_device *dev, int addr, u8 reg,
> +                               u16 value)
> +{
> +       return b53_write16(dev, B53_PORT_MII_PAGE(addr), reg, value);

Since these will be the default implementations for everything but
mdio, how about rather doing someting like

static inline int b53_phy_read16(...)
{
       if (dev->ops->phy_read16)
              return dev->ops->phy_read16(...);

       return b53_read16(dev, B53_PORT_MII_PAGE(addr), reg, value);
}

That way you don't need to add a version for every connector.

> +}
> +
>  static struct b53_io_ops b53_srab_ops = {
>         .read8 = b53_srab_read8,
>         .read16 = b53_srab_read16,
> @@ -332,6 +345,8 @@ static struct b53_io_ops b53_srab_ops = {
>         .write32 = b53_srab_write32,
>         .write48 = b53_srab_write48,
>         .write64 = b53_srab_write64,
> +       .phy_read16 = b53_srab_phy_read16,
> +       .phy_write16 = b53_srab_phy_write16,
>  };
>
>  static int b53_srab_probe(struct platform_device *pdev)


Jonas
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel