Re: [bug report] drx: add initial drx-d driver

[Mauro Carvalho Chehab]

Em Thu, 14 Dec 2017 22:55:02 +0100
Ralph Metzler  escreveu:

> Hello Dan Carpenter,
> 
> Dan Carpenter writes:
>  > Hello Ralph Metzler,
>  > 
>  > The patch 126f1e618870: "drx: add initial drx-d driver" from Mar 12,
>  > 2011, leads to the following static checker warning:
>  > 
>  >drivers/media/dvb-frontends/drxd_hard.c:1305 SC_WaitForReady()
>  >info: return a literal instead of 'status'
>  > 
>  > drivers/media/dvb-frontends/drxd_hard.c
>  >   1298  static int SC_WaitForReady(struct drxd_state *state)
>  >   1299  {
>  >   1300  int i;
>  >   1301  
>  >   1302  for (i = 0; i < DRXD_MAX_RETRIES; i += 1) {
>  >   1303  int status = Read16(state, SC_RA_RAM_CMD__A, NULL, 
> 0);
>  >   1304  if (status == 0)
>  >   1305  return status;
>  > ^
>  > The register is set to zero when ready?  The answer should obviously be
>  > yes, but it wouldn't totally surprise me if this function just always
>  > looped 1000 times...  Few of the callers check the return.  Anyway, it's
>  > more clear to just "return 0;"
>  > 
>  >   1306  }
>  >   1307  return -1;
>  >^^
>  > -1 is not a proper error code.
>  > 
>  >   1308  }
>  > 
>  > regards,
>  > dan carpenter  
> 
> I think I wrote the driver more than 10 years ago and somebody later 
> submitted it
> to the kernel.
> 
> I don't know if there is a anybody still maintaining this. Is it even used 
> anymore?
> I could write a patch but cannot test it (e.g. to see if it really always
> loops 1000 times ...)

It seems that it is used on this board (besides ngene):
EM2880_BOARD_HAUPPAUGE_WINTV_HVR_900_R2
a. k. a.: Hauppauge WinTV HVR 900 (R2)

I might have a HVR-900 rev 2 somewhere, but if so, it is not at the
usual place. I moved a few times since when I touched at the
drxd driver, at the time it was merged upstream. Maybe Michael or
someone at Hauppauge could test a patch for it, if they still have
this device.

Thanks,
Mauro

Re: [bug report] drx: add initial drx-d driver

[Dan Carpenter]

I'm really sorry.  This warning showed up in my new warnings pile and I
didn't look at the date.  :/  Sorry for the noise.

regards,
dan carpenter

Re: [bug report] drx: add initial drx-d driver

[Devin Heitmueller]

> I think I wrote the driver more than 10 years ago and somebody later 
> submitted it
> to the kernel.

I'm pretty sure that was me, or perhaps I was the first person to get
it to work with a device upstream - it was so long ago.

> I don't know if there is a anybody still maintaining this. Is it even used 
> anymore?
> I could write a patch but cannot test it (e.g. to see if it really always
> loops 1000 times ...)

Pretty sure the register was a status register that had bits set for
pending writes, and when the transfer was complete all the bits would
be zero (at which point it should stop polling the register and bail
out).

I would have to test it to be sure, but I'm 80% confident that in
normal operation that loop only does a few iterations.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

[bug report] drx: add initial drx-d driver

[Ralph Metzler]

Hello Dan Carpenter,

Dan Carpenter writes:
 > Hello Ralph Metzler,
 > 
 > The patch 126f1e618870: "drx: add initial drx-d driver" from Mar 12,
 > 2011, leads to the following static checker warning:
 > 
 >  drivers/media/dvb-frontends/drxd_hard.c:1305 SC_WaitForReady()
 >  info: return a literal instead of 'status'
 > 
 > drivers/media/dvb-frontends/drxd_hard.c
 >   1298  static int SC_WaitForReady(struct drxd_state *state)
 >   1299  {
 >   1300  int i;
 >   1301  
 >   1302  for (i = 0; i < DRXD_MAX_RETRIES; i += 1) {
 >   1303  int status = Read16(state, SC_RA_RAM_CMD__A, NULL, 
 > 0);
 >   1304  if (status == 0)
 >   1305  return status;
 > ^
 > The register is set to zero when ready?  The answer should obviously be
 > yes, but it wouldn't totally surprise me if this function just always
 > looped 1000 times...  Few of the callers check the return.  Anyway, it's
 > more clear to just "return 0;"
 > 
 >   1306  }
 >   1307  return -1;
 >^^
 > -1 is not a proper error code.
 > 
 >   1308  }
 > 
 > regards,
 > dan carpenter

I think I wrote the driver more than 10 years ago and somebody later submitted 
it
to the kernel.

I don't know if there is a anybody still maintaining this. Is it even used 
anymore?
I could write a patch but cannot test it (e.g. to see if it really always
loops 1000 times ...)


Regards,
Ralph Metzler

-- 
--

[bug report] drx: add initial drx-d driver

[Dan Carpenter]

Hello Ralph Metzler,

The patch 126f1e618870: "drx: add initial drx-d driver" from Mar 12,
2011, leads to the following static checker warning:

drivers/media/dvb-frontends/drxd_hard.c:1305 SC_WaitForReady()
info: return a literal instead of 'status'

drivers/media/dvb-frontends/drxd_hard.c
  1298  static int SC_WaitForReady(struct drxd_state *state)
  1299  {
  1300  int i;
  1301  
  1302  for (i = 0; i < DRXD_MAX_RETRIES; i += 1) {
  1303  int status = Read16(state, SC_RA_RAM_CMD__A, NULL, 0);
  1304  if (status == 0)
  1305  return status;
^
The register is set to zero when ready?  The answer should obviously be
yes, but it wouldn't totally surprise me if this function just always
looped 1000 times...  Few of the callers check the return.  Anyway, it's
more clear to just "return 0;"

  1306  }
  1307  return -1;
   ^^
-1 is not a proper error code.

  1308  }

regards,
dan carpenter