On Fri, 2017-07-28 at 17:17 +0300, Dan Carpenter wrote:
> On Fri, Jul 28, 2017 at 02:56:26PM +0200, Elia Geretto wrote:
> > This patch corrects some visibility issues regarding some functions
> > and
> > solves a warning related to a non-matching union. After this patch,
> > sparse produces only one other warning regarding a bitwise
> > operator;
> > however, this behaviour seems to be intended.
>
> I can't understand this changelog at all.... :/ What are we fixing
> exactly? It seems like we're fixing something about bitwise
> operators... I guess let me check the Sparse warnings... Here they
> are
> from the latest linux-next:
>
> drivers/staging/pi433/pi433_if.c:211:9: warning: mixing different
> enum types
> drivers/staging/pi433/pi433_if.c:211:9: int enum
> optionOnOff versus
> drivers/staging/pi433/pi433_if.c:211:9: int enum packetFormat
> drivers/staging/pi433/pi433_if.c:211:9: warning: mixing different
> enum types
> drivers/staging/pi433/pi433_if.c:211:9: int enum
> optionOnOff versus
> drivers/staging/pi433/pi433_if.c:211:9: int enum packetFormat
> drivers/staging/pi433/pi433_if.c:268:9: warning: mixing different
> enum types
> drivers/staging/pi433/pi433_if.c:268:9: int enum
> optionOnOff versus
> drivers/staging/pi433/pi433_if.c:268:9: int enum packetFormat
> drivers/staging/pi433/pi433_if.c:268:9: warning: mixing different
> enum types
> drivers/staging/pi433/pi433_if.c:268:9: int enum
> optionOnOff versus
> drivers/staging/pi433/pi433_if.c:268:9: int enum packetFormat
> drivers/staging/pi433/pi433_if.c:317:1: warning: symbol
> 'pi433_receive' was not declared. Should it be static?
> drivers/staging/pi433/pi433_if.c:467:1: warning: symbol
> 'pi433_tx_thread' was not declared. Should it be static?
> drivers/staging/pi433/pi433_if.c:1155:36: error: incompatible types
> for operation (<)
> drivers/staging/pi433/pi433_if.c:1155:36: left side has type
> struct task_struct *tx_task_struct
> drivers/staging/pi433/pi433_if.c:1155:36: right side has type int
> drivers/staging/pi433/rf69.c:206:17: warning: dubious: x & !y
> drivers/staging/pi433/rf69.c:436:5: warning: symbol
> 'rf69_set_bandwidth_intern' was not declared. Should it be static?
>
> Each type of fix should be sent as a separate fix with a better
> changelog. People have already done the "static" fixes and IS_ERR()
> fixes, so don't worry about those. But I don't think anyway has
> fixed
> the enum issues so resend that. Also the bitwise thing is a real
> bug,
> but there is already a fix for that, it just hasn't been merged yet.
>
> >
> > Signed-off-by: Elia Geretto <elia.f.geretto@xxxxxxxxx>
> > ---
> > drivers/staging/pi433/pi433_if.c | 17 +++++++++++------
> > drivers/staging/pi433/rf69.c | 4 +++-
> > 2 files changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/staging/pi433/pi433_if.c
> > b/drivers/staging/pi433/pi433_if.c
> > index d9328ce5ec1d..f8219a53ce60 100644
> > --- a/drivers/staging/pi433/pi433_if.c
> > +++ b/drivers/staging/pi433/pi433_if.c
> > @@ -208,7 +208,10 @@ rf69_set_rx_cfg(struct pi433_device *dev,
> > struct pi433_rx_cfg *rx_cfg)
> > {
> > SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi,
> > always));
> > }
> > - SET_CHECKED(rf69_set_packet_format (dev->spi, rx_cfg-
> > >enable_length_byte));
> > + if (rx_cfg->enable_length_byte == optionOn)
> > + SET_CHECKED(rf69_set_packet_format(dev->spi,
> > packetLengthVar));
> > + else
> > + SET_CHECKED(rf69_set_packet_format(dev->spi,
> > packetLengthFix));
>
> The SET_CHECKED() macro is total garbage. It has a hidden return and
> it calls the rf69_set_packet_format() twice on error it expands to:
>
> if (rf69_set_packet_format(dev->spi, rx_cfg-
> >enable_length_byte)) < 0)
> return rf69_set_packet_format(dev->spi, rx_cfg-
> >enable_length_byte);
>
> Mega turbo barf! Kill it with fire!
>
> regards,
> dan carpenter
>
I will resend a separate patch containing the enum work; I apologize
for the unclear changelog, I am still trying to understand how much in
detail I should go. Next time I will be more precise.
Regards,
Elia Geretto
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
