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 <[email protected]> > 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 [email protected] https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
