On Sun, 2014-01-19 at 19:07 +0100, Christophe Leroy wrote:
> Pinging this watch as we got no feedback since 22 Nov, although we have taken
> into account reviews from v1 and v2.
>
> The patch adds WAN support for Lantiq FALC56 - PEF2256 E1 Chipset.
trivia:
> diff -urN a/drivers/net/wan/pef2256.c b/drivers/net/wan/pef2256.c
[]
> +static void config_hdlc_timeslot(struct pef2256_dev_priv *priv, int ts)
> +{
> + static struct {
> + u32 ttr;
> + u32 rtr;
> + } regs[] = {
> + { TTR1, RTR1 },
> + { TTR2, RTR2 },
> + { TTR3, RTR3 },
> + { TTR4, RTR4 },
> + };
const
> + int cfg_bit = 1 << (31 - ts);
> + int reg_bit = 1 << (7 - (ts % 8));
> + int j = ts / 8;
looks endian specific
> +
> + if (j >= 4)
> + return;
> +
> + if (priv->Tx_TS & cfg_bit)
> + pef2256_s8(priv, regs[j].ttr, 1 << reg_bit);
> +
> + if (priv->Rx_TS & cfg_bit)
> + pef2256_s8(priv, regs[j].rtr, 1 << reg_bit);
> +}
> +static void init_falc(struct pef2256_dev_priv *priv)
> +{
a lot of the below looks like it should use
switch/case blocks.
> + /* Clocking rate for FALC56 */
> +
> + /* Nothing to do for clocking rate 2M */
> +
> + /* clocking rate 4M */
> + if (priv->clock_rate == CLOCK_RATE_4M)
> + pef2256_s8(priv, SIC1, SIC1_SSC0);
> +
> + /* clocking rate 8M */
> + if (priv->clock_rate == CLOCK_RATE_8M)
> + pef2256_s8(priv, SIC1, SIC1_SSC1);
> +
> + /* clocking rate 16M */
> + if (priv->clock_rate == CLOCK_RATE_16M) {
> + pef2256_s8(priv, SIC1, SIC1_SSC0);
> + pef2256_s8(priv, SIC1, SIC1_SSC1);
> + }
> +
> + /* data rate for FALC56 */
> +
> + /* Nothing to do for data rate 2M on the system data bus */
> +
> + /* data rate 4M on the system data bus */
> + if (priv->data_rate == DATA_RATE_4M)
> + pef2256_s8(priv, FMR1, FMR1_SSD0);
> +
> + /* data rate 8M on the system data bus */
> + if (priv->data_rate == DATA_RATE_8M)
> + pef2256_s8(priv, SIC1, SIC1_SSD1);
> +
> + /* data rate 16M on the system data bus */
> + if (priv->data_rate == DATA_RATE_16M) {
> + pef2256_s8(priv, FMR1, FMR1_SSD0);
> + pef2256_s8(priv, SIC1, SIC1_SSD1);
> + }
> +
> + /* channel phase for FALC56 */
> +
> + /* Nothing to do for channel phase 1 */
> +
> + if (priv->channel_phase == CHANNEL_PHASE_2)
> + pef2256_s8(priv, SIC2, SIC2_SICS0);
> +
> + if (priv->channel_phase == CHANNEL_PHASE_3)
> + pef2256_s8(priv, SIC2, SIC2_SICS1);
> +
> + if (priv->channel_phase == CHANNEL_PHASE_4) {
> + pef2256_s8(priv, SIC2, SIC2_SICS0);
> + pef2256_s8(priv, SIC2, SIC2_SICS1);
> + }
> +
> + if (priv->channel_phase == CHANNEL_PHASE_5)
> + pef2256_s8(priv, SIC2, SIC2_SICS2);
> +
> + if (priv->channel_phase == CHANNEL_PHASE_6) {
> + pef2256_s8(priv, SIC2, SIC2_SICS0);
> + pef2256_s8(priv, SIC2, SIC2_SICS2);
> + }
> +
> + if (priv->channel_phase == CHANNEL_PHASE_7) {
> + pef2256_s8(priv, SIC2, SIC2_SICS1);
> + pef2256_s8(priv, SIC2, SIC2_SICS2);
> + }
> +
> + if (priv->channel_phase == CHANNEL_PHASE_8) {
> + pef2256_s8(priv, SIC2, SIC2_SICS0);
> + pef2256_s8(priv, SIC2, SIC2_SICS1);
> + pef2256_s8(priv, SIC2, SIC2_SICS2);
> + }
> +static ssize_t fs_attr_mode_store(struct device *dev,
> + struct device_attribute *attr, const char *buf,
> + size_t count)
> +{
> + struct net_device *ndev = dev_get_drvdata(dev);
> + struct pef2256_dev_priv *priv = dev_to_hdlc(ndev)->priv;
> + long int value;
> + int ret = kstrtol(buf, 10, &value);
> + int reconfigure = (value != priv->mode);
Ugly test and set before determining if the previous
function was successful.
> + if (value != MASTER_MODE && value != SLAVE_MODE)
> + ret = -EINVAL;
> +
> + if (ret < 0)
> + netdev_info(ndev, "Invalid mode (0 or 1 expected\n");
> + else {
> + priv->mode = value;
> + if (reconfigure && netif_carrier_ok(ndev))
> + init_falc(priv);
> + }
> +
> + return strnlen(buf, count);
odd that you set ret and then don't use it.
> +static ssize_t fs_attr_Tx_TS_store(struct device *dev,
> + struct device_attribute *attr, const char *buf,
> + size_t count)
> +{
> + struct net_device *ndev = dev_get_drvdata(dev);
> + struct pef2256_dev_priv *priv = dev_to_hdlc(ndev)->priv;
> + unsigned long value;
> + int ret = kstrtoul(buf, 16, (long int *)&value);
unportable cast
> + int reconfigure = (value != priv->mode);
again with the test/set before determining function success.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
