On 27/01/14 20:03, Phoebe Buckheister wrote:
> The transmit_power field in struct wpan_phy is a field with two parts,
> the lower six bits containing the signed TX power and the upper two bits
> containing a tolerance indicator. This field is already used in some
> places and carries the correct semantics, reuse that field and set the
> tolerance indicator to +-1dB (which is 00b).
>
> This patch only adds support for this in the at86rf230 driver and the
> RF212 chip. Configuration calculation for RF212 is also somewhat basic,
> but does the job - the RF212 datasheet gives a large table with
> suggested values for combinations of TX power and page/channel, if this
> does not work well, we might have to copy the whole table.
Generally this looks ok, please fix few items below:
> Signed-off-by: Phoebe Buckheister <phoebe.buckheis...@itwm.fraunhofer.de>
> diff --git a/drivers/net/ieee802154/at86rf230.c
> b/drivers/net/ieee802154/at86rf230.c
> index 57f94b6..01aeaf9 100644
> --- a/drivers/net/ieee802154/at86rf230.c
> +++ b/drivers/net/ieee802154/at86rf230.c
> @@ -675,6 +675,30 @@ at86rf230_set_hw_addr_filt(struct ieee802154_dev *dev,
> return 0;
> }
>
> +static int
> +at86rf230_set_txpower(struct ieee802154_dev *dev, int db)
It's a at86rf212_set_txpower - it is specific to rf212.
RF230/231 should have it's own version (I'm not requiring it
from you though).
> +{
> + struct at86rf230_local *lp = dev->priv;
> + int rc;
> +
> + if (!is_rf212(lp))
> + return -ENOTSUPP;
> +
> + /* typical maximum output is 5dBm with RG_PHY_TX_PWR 0x60.
> + * lower five bits decrease power in 1dB steps */
> +
> + if (db > 5 || db < -26)
> + return -ENOTSUPP;
> +
> + db = -(db - 5);
> +
> + rc = __at86rf230_write(lp, RG_PHY_TX_PWR, 0x60 | db);
How does this correlate to table 7-15 at page 107 of the datasheet?
> + if (rc)
> + return rc;
> +
> + return 0;
> +}
> +
> static struct ieee802154_ops at86rf230_ops = {
> .owner = THIS_MODULE,
> .xmit = at86rf230_xmit,
=> diff --git a/include/net/mac802154.h b/include/net/mac802154.h
> index 807d6b7..334950f 100644
> --- a/include/net/mac802154.h
> +++ b/include/net/mac802154.h
> @@ -113,6 +113,10 @@ struct ieee802154_dev {
> * Set radio for listening on specific address.
> * Set the device for listening on specified address.
> * Returns either zero, or negative errno.
> + *
> + * set_txpower:
> + * Set radio transmit power in dB.
> + * Returns either zero, er negative errno.
s/er/or/
Isn't the pib_lock required for it?
> diff --git a/include/net/wpan-phy.h b/include/net/wpan-phy.h
> index b52bda8..44e5e49 100644
> --- a/include/net/wpan-phy.h
> +++ b/include/net/wpan-phy.h
> @@ -54,6 +54,8 @@ struct wpan_phy {
> const char *name, int type);
> void (*del_iface)(struct wpan_phy *phy, struct net_device *dev);
>
> + int (*set_txpower)(struct wpan_phy *phy, int txpower);
> +
Please name argument 'db'. Just for the sake of clarity.
> diff --git a/net/ieee802154/nl-phy.c b/net/ieee802154/nl-phy.c
> index 89b265a..3ade8c6 100644
> --- a/net/ieee802154/nl-phy.c
> +++ b/net/ieee802154/nl-phy.c
> @@ -55,7 +55,9 @@ static int ieee802154_nl_fill_phy(struct sk_buff *msg, u32
> portid,
> mutex_lock(&phy->pib_lock);
> if (nla_put_string(msg, IEEE802154_ATTR_PHY_NAME, wpan_phy_name(phy)) ||
> nla_put_u8(msg, IEEE802154_ATTR_PAGE, phy->current_page) ||
> - nla_put_u8(msg, IEEE802154_ATTR_CHANNEL, phy->current_channel))
> + nla_put_u8(msg, IEEE802154_ATTR_CHANNEL, phy->current_channel) ||
> + nla_put_s32(msg, IEEE802154_ATTR_TXPOWER,
> + ((signed char) (phy->transmit_power << 2)) >>
> 2))
Why is it s32? Isn't s8 enough (not for the at86 maybe, but from the
standard point of view)?
> +int ieee802154_set_phyparams(struct sk_buff *skb, struct genl_info *info)
> +{
> + struct wpan_phy *phy;
> + const char *name;
> + int txpower;
> + u8 txpower_wpan;
> + int rc = -EINVAL;
> +
> + pr_debug("%s\n", __func__);
> +
> + if (!info->attrs[IEEE802154_ATTR_PHY_NAME])
> + return -EINVAL;
> +
> + name = nla_data(info->attrs[IEEE802154_ATTR_PHY_NAME]);
> + if (name[nla_len(info->attrs[IEEE802154_ATTR_PHY_NAME]) - 1] != '\0')
> + return -EINVAL; /* phy name should be null-terminated */
> +
> + txpower = nla_get_s32(info->attrs[IEEE802154_ATTR_TXPOWER]);
> + if (txpower < -64 || txpower > 63)
> + return -EINVAL; /* txpower code subfield is 6 bits long */
Ah, so s8 definitely should be enough!
> + txpower_wpan = ((unsigned char) (txpower << 2)) >> 2;
Why are the shifts necessary?
> +
> + phy = wpan_phy_find(name);
> + if (!phy)
> + return -ENODEV;
> +
> + if (!phy->set_txpower)
> + goto out;
> +
> + rc = phy->set_txpower(phy, txpower);
Hmm? I thought we pass signed value to phy, but you have just converted
it to unsigned.
> + if (rc < 0)
> + goto out;
> +
> + phy->transmit_power = txpower_wpan;
locking?
> +
> + [IEEE802154_ATTR_TXPOWER] = { .type = NLA_S32, },
S8 here too?
> +static int mac802154_set_txpower(struct wpan_phy *phy, int txpower)
> +{
> + struct mac802154_priv *priv = wpan_phy_priv(phy);
> +
> + return priv->ops->set_txpower(&priv->hw, txpower);
What if driver provides no set_txpower? Please return -EOPNOTSUP here in
this case.
--
With best wishes
Dmitry
------------------------------------------------------------------------------
CenturyLink Cloud: The Leader in Enterprise Cloud Services.
Learn Why More Businesses Are Choosing CenturyLink Cloud For
Critical Workloads, Development Environments & Everything In Between.
Get a Quote or Start a Free Trial Today.
http://pubads.g.doubleclick.net/gampad/clk?id=119420431&iu=/4140/ostg.clktrk
_______________________________________________
Linux-zigbee-devel mailing list
Linux-zigbee-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
