On Sun, Jun 01, 2014 at 09:35:47AM -0600, Jean Sacren wrote:
> From: Alexander Aring <alex.ar...@gmail.com>
> Date: Sun, 01 Jun 2014 16:35:53 +0200
> >
> > Hi,
> >
> > On Sun, Jun 01, 2014 at 08:23:17AM -0600, Jean Sacren wrote:
> > > From: Alexander Aring <alex.ar...@gmail.com>
> > > Date: Sun, 01 Jun 2014 09:26:57 +0200
> > >
> > > Hi Alex,
> > >
> > > Thank you very much for the feedback.
> > >
> > > > the at86rf230 driver supports several at86rf2xx chips. You split the
> > > > at86rf212_set_channel which is at86rf212 specific in two function which
> > > > are named at86rf230_foo.
> > >
> > > I didn't "split" at86rf212_set_channel() in two functions. I spliced
> > > those two sections of code and made at86rf212_set_channel() far
> > > succinct and easy to read.
> > >
> >
> > yes, but this driver supports more than one chip and it's easier to read
> > if we have one channel_set function for each chip type. Note you also
> > named the specific channel_set function to a another at86rf230_foo
> > function which is at86rf212 specific only. Sorry that will confuse
> > all the people who will ever read this code.
> >
> > There is a at86rf230_ops and at86rf212_ops struct. The channel_set
> > function it's much easier to have only one callback for each struct,
> > otherwise you have 4 different channel_set functions and nobody knows
> > for which at86rf2xx type that function is for.
>
> You mean something like the following will be less confusing?
Yes that's less but there are issues and I don't see any reason why we
should do that.
>
> diff --git a/drivers/net/ieee802154/at86rf230.c
> b/drivers/net/ieee802154/at86rf230.c
> index 4517b149ed07..06b494bacc44 100644
> --- a/drivers/net/ieee802154/at86rf230.c
> +++ b/drivers/net/ieee802154/at86rf230.c
> @@ -602,20 +602,21 @@ at86rf212_set_channel(struct at86rf230_local *lp, int
> page, int channel)
> {
> int rc;
>
> - if (channel == 0)
> - rc = at86rf230_write_subreg(lp, SR_SUB_MODE, 0);
> - else
> - rc = at86rf230_write_subreg(lp, SR_SUB_MODE, 1);
> + if (channel)
> + channel = 1;
> +
> + rc = at86rf230_write_subreg(lp, SR_SUB_MODE, channel);
> if (rc < 0)
> return rc;
>
First:
This will break the at86rf212_set_channel function. At the end of this
function we need the channel parameter and you overwrite it here.
At the end of this function stands:
"return at86rf230_write_subreg(lp, SR_CHANNEL, channel);"
Second:
The variable channel should be a new variable named sub_mode and initialized
to 0, this fixes the first issue.
But again, I don't see any reasons why we should change that. It's the
same thing like before.
- Alex
------------------------------------------------------------------------------
Time is money. Stop wasting it! Get your web API in 5 minutes.
www.restlet.com/download
http://p.sf.net/sfu/restlet
_______________________________________________
Linux-zigbee-devel mailing list
Linux-zigbee-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
