Re: [PATCH v2 2/3] [media] mceusb: Reword messages referring to "urb"

2019-08-10 Thread Sean Young
Hi,

On Sun, Jul 21, 2019 at 05:31:55PM -0400, A Sun wrote:
> FYI, I'm in progress on another mceusb patch to fix, and eliminate, the 
> driver's
> TX IR length limits. Limit causes -EINVAL errors for > ~300 pulse/space 
> samples and
> I've seen reports (and patches for) of appliances with IR over 400 
> pulse/spaces.

This always looked like it needed improvement. Thank you!

> 
> The future patch rewrites:
>   mceusb_tx_ir()
> And revises "write/tx" async I/O to sync I/O to do unlimited multipart TX IR.
> These functions will need rewrite and rename:
>   mce_async_callback() -> mce_tx_callback()
>   mce_request_packet() -> mce_tx()
> The present mce_async_out() name will become misleading. mce_command_out()
> or mce_request_out() (which calls mce_tx()), are probably better names.
> 
> I'm still mulling over whether the more generic "read/write" term
> (e.g. mce_write() and mce_write_callback()) may be a better migration path,
> for future work.

Thanks.

Another thing the mceusb driver could do with is usb wakeup. I've hadn't
had the time to look at that.


Sean


Re: [PATCH v2 2/3] [media] mceusb: Reword messages referring to "urb"

2019-07-21 Thread A Sun


Hi again,

On 7/15/2019 8:28 AM, Sean Young wrote:
> Hi,
> 
> On Tue, Jun 25, 2019 at 05:29:02PM -0400, A Sun wrote:


>> In a quick look though other USB drivers, here are some other possibilities:
>>
>> Endpoint-in / Endpoint-out   (not concise)
>> ep-in / ep-out   (abbrev may be too obscure)
>> in / out ("Error: in urb status.." confusing,
>>   "in" is noun, adj, or verb?)
>> read / write (confusing, "read" is noun, adj, verb?)
>> RD / WR
>> RX / TX
>>
>> I suggest RX/TX. However, I'll have to leave it up to you to make a choice.
> 
> TBH I've been mulling this over. I think you're right that the terms should
> be usb centric. TX seems confusing, there are mceusb devices with no IR 
> transmit ports, so it would be surprising to see errors about TX. Your
> first option is usb centric and can be wordy in errors messages and concise
> in code (option #2).
> 
> However as you're writing patches I'll leave it up to you. It would be nice
> if the usage is consistent in the driver code.
> 

ok, I'll leave this patch proposal as is, where RX/TX is the favored 
terminology.
rx/tx was already in use elsewhere outside the message text of this patch,
and is prevalent in mceusb_dev_printdata() output.

I also found that my later patch submission:
  [PATCH v1] [media] mceusb: USB reset device following USB clear halt error
has an unintended dependency on this [PATCH v2 2/3].


FYI, I'm in progress on another mceusb patch to fix, and eliminate, the driver's
TX IR length limits. Limit causes -EINVAL errors for > ~300 pulse/space samples 
and
I've seen reports (and patches for) of appliances with IR over 400 pulse/spaces.

The future patch rewrites:
  mceusb_tx_ir()
And revises "write/tx" async I/O to sync I/O to do unlimited multipart TX IR.
These functions will need rewrite and rename:
  mce_async_callback() -> mce_tx_callback()
  mce_request_packet() -> mce_tx()
The present mce_async_out() name will become misleading. mce_command_out()
or mce_request_out() (which calls mce_tx()), are probably better names.

I'm still mulling over whether the more generic "read/write" term
(e.g. mce_write() and mce_write_callback()) may be a better migration path,
for future work.

Thanks, ..A Sun


Re: [PATCH v2 2/3] [media] mceusb: Reword messages referring to "urb"

2019-07-15 Thread Sean Young
Hi,

On Tue, Jun 25, 2019 at 05:29:02PM -0400, A Sun wrote:
> 
> Hi again,
> 
> On 6/25/2019 12:12 PM, Sean Young wrote:
> > Hello,
> > 
> > On Tue, Jun 25, 2019 at 11:01:32AM -0400, A Sun wrote:
> >> On 6/25/2019 6:51 AM, Sean Young wrote:
> >>> On Wed, Jun 19, 2019 at 03:54:21AM -0400, A Sun wrote:
> 
> >>
>  -dev_err(ir->dev, "Error: request urb status = %d (TX 
>  HALT)",
>  +dev_err(ir->dev, "Error: tx urb status = %d (TX HALT)",
> >>>
> >>> I am not sure this makes things clearer. Some of the code still refers
> >>> to request, e.g. mce_request_packet.
> >>>
> >>> Since this is an IR device, there is IR TX and RX; however this unrelated
> >>> to that.
> >>>
> >>> There is one urb/endpoint on which commands are sent; on the other, we
> >>> receiver responses and IR data. Calling those TX and RX is not a good
> >>> idea I think.
> >>
> >> The tx urb described is also for IR data TX.
> >> IR TX is one primary function/purpose of the device.
> >>
> >> It happens that the urb/endpoint for IR TX is the same urb/endpoint for
> >> mceusb device commands.
> > 
> > The outgoing urb is for command, e.g. get version, set IR Rx timeout,
> > set IR Rx carrier report, set IR wakeup protocol/scancode, resume
> > device after suspend. Not everything is IR Tx.
> > 
> 
> Right. Not everything is command either.
> The subject USB interface endpoint-out (and -in) is intended for data
> (interrupt/bulk) transfer. The only true data here is IR data.
> 
> It's a quirk of mceusb devices to use endpoint-out as a command channel too,
> where commands masquerade as data.
> 
> A quick sampling of other USB devices, I'm seeing they customarily use
> USB default endpoint 0 and control transfers (rather than data transfers)
> for device management commands.
> 
> >>> The existing code refers to request and response, and also TX and RX in
> >>> places. The microsoft documentation refers to "command and response" which
> >>> would be consistent with the #define's we have (MCE_CMD_* and MCE_RSP_*).
> >>>
> >>
> >> My intent is to try and better word several of the USB layer messages,
> >> and avoid confusing Microsoft MCE terms in those messages.
> >>
> >> The original "(request) urb" phrases expand to "request USB Request Block"
> >> and " USB Request Block" which are particularly confusing, and
> >> non-specific, respectively.
> > 
> > How about command and response?
> > 
> 
> Still seem confusing and awkward to me, as in "response USB Request Block",
> or "command USB Request Block" that might be IR data. "Command" also suggests
> to me USB interface endpoint 0, which isn't the case with mceusb.
> 
> Again, my intent is to avoid Microsoft MCE specific terms in messages 
> pertaining
> to USB.

You are right. Command and response isn't great.

> In a quick look though other USB drivers, here are some other possibilities:
> 
> Endpoint-in / Endpoint-out(not concise)
> ep-in / ep-out(abbrev may be too obscure)
> in / out  ("Error: in urb status.." confusing,
>"in" is noun, adj, or verb?)
> read / write  (confusing, "read" is noun, adj, verb?)
> RD / WR
> RX / TX
> 
> I suggest RX/TX. However, I'll have to leave it up to you to make a choice.

TBH I've been mulling this over. I think you're right that the terms should
be usb centric. TX seems confusing, there are mceusb devices with no IR 
transmit ports, so it would be surprising to see errors about TX. Your
first option is usb centric and can be wordy in errors messages and concise
in code (option #2).

However as you're writing patches I'll leave it up to you. It would be nice
if the usage is consistent in the driver code.


Thanks

Sean


Re: [PATCH v2 2/3] [media] mceusb: Reword messages referring to "urb"

2019-06-25 Thread A Sun


Hi again,

On 6/25/2019 12:12 PM, Sean Young wrote:
> Hello,
> 
> On Tue, Jun 25, 2019 at 11:01:32AM -0400, A Sun wrote:
>> On 6/25/2019 6:51 AM, Sean Young wrote:
>>> On Wed, Jun 19, 2019 at 03:54:21AM -0400, A Sun wrote:

>>
 -  dev_err(ir->dev, "Error: request urb status = %d (TX HALT)",
 +  dev_err(ir->dev, "Error: tx urb status = %d (TX HALT)",
>>>
>>> I am not sure this makes things clearer. Some of the code still refers
>>> to request, e.g. mce_request_packet.
>>>
>>> Since this is an IR device, there is IR TX and RX; however this unrelated
>>> to that.
>>>
>>> There is one urb/endpoint on which commands are sent; on the other, we
>>> receiver responses and IR data. Calling those TX and RX is not a good
>>> idea I think.
>>
>> The tx urb described is also for IR data TX.
>> IR TX is one primary function/purpose of the device.
>>
>> It happens that the urb/endpoint for IR TX is the same urb/endpoint for
>> mceusb device commands.
> 
> The outgoing urb is for command, e.g. get version, set IR Rx timeout,
> set IR Rx carrier report, set IR wakeup protocol/scancode, resume
> device after suspend. Not everything is IR Tx.
> 

Right. Not everything is command either.
The subject USB interface endpoint-out (and -in) is intended for data
(interrupt/bulk) transfer. The only true data here is IR data.

It's a quirk of mceusb devices to use endpoint-out as a command channel too,
where commands masquerade as data.

A quick sampling of other USB devices, I'm seeing they customarily use
USB default endpoint 0 and control transfers (rather than data transfers)
for device management commands.

>>> The existing code refers to request and response, and also TX and RX in
>>> places. The microsoft documentation refers to "command and response" which
>>> would be consistent with the #define's we have (MCE_CMD_* and MCE_RSP_*).
>>>
>>
>> My intent is to try and better word several of the USB layer messages,
>> and avoid confusing Microsoft MCE terms in those messages.
>>
>> The original "(request) urb" phrases expand to "request USB Request Block"
>> and " USB Request Block" which are particularly confusing, and
>> non-specific, respectively.
> 
> How about command and response?
> 

Still seem confusing and awkward to me, as in "response USB Request Block",
or "command USB Request Block" that might be IR data. "Command" also suggests
to me USB interface endpoint 0, which isn't the case with mceusb.

Again, my intent is to avoid Microsoft MCE specific terms in messages pertaining
to USB.

In a quick look though other USB drivers, here are some other possibilities:

Endpoint-in / Endpoint-out  (not concise)
ep-in / ep-out  (abbrev may be too obscure)
in / out("Error: in urb status.." confusing,
 "in" is noun, adj, or verb?)
read / write(confusing, "read" is noun, adj, verb?)
RD / WR
RX / TX

I suggest RX/TX. However, I'll have to leave it up to you to make a choice.

Thanks, ..A Sun


Re: [PATCH v2 2/3] [media] mceusb: Reword messages referring to "urb"

2019-06-25 Thread Sean Young
Hello,

On Tue, Jun 25, 2019 at 11:01:32AM -0400, A Sun wrote:
> On 6/25/2019 6:51 AM, Sean Young wrote:
> > On Wed, Jun 19, 2019 at 03:54:21AM -0400, A Sun wrote:
> >>
> 
> >> -  dev_err(ir->dev, "Error: request urb status = %d (TX HALT)",
> >> +  dev_err(ir->dev, "Error: tx urb status = %d (TX HALT)",
> > 
> > I am not sure this makes things clearer. Some of the code still refers
> > to request, e.g. mce_request_packet.
> > 
> > Since this is an IR device, there is IR TX and RX; however this unrelated
> > to that.
> > 
> > There is one urb/endpoint on which commands are sent; on the other, we
> > receiver responses and IR data. Calling those TX and RX is not a good
> > idea I think.
> 
> The tx urb described is also for IR data TX.
> IR TX is one primary function/purpose of the device.
>
> It happens that the urb/endpoint for IR TX is the same urb/endpoint for
> mceusb device commands.

The outgoing urb is for command, e.g. get version, set IR Rx timeout,
set IR Rx carrier report, set IR wakeup protocol/scancode, resume
device after suspend. Not everything is IR Tx.

> The same is found for IR data RX.
> The urb/endpoint for IR RX is the same urb/endpoint for mceusb device
> responses (to commands).

The response to get version, how many Tx ports are there etc. Responses
to commands and IR data. Not everything is IR Rx here.

> > The existing code refers to request and response, and also TX and RX in
> > places. The microsoft documentation refers to "command and response" which
> > would be consistent with the #define's we have (MCE_CMD_* and MCE_RSP_*).
> > 
> 
> My intent is to try and better word several of the USB layer messages,
> and avoid confusing Microsoft MCE terms in those messages.
> 
> The original "(request) urb" phrases expand to "request USB Request Block"
> and " USB Request Block" which are particularly confusing, and
> non-specific, respectively.

How about command and response?

Sean


Re: [PATCH v2 2/3] [media] mceusb: Reword messages referring to "urb"

2019-06-25 Thread A Sun


Hi,

On 6/25/2019 6:51 AM, Sean Young wrote:
> On Wed, Jun 19, 2019 at 03:54:21AM -0400, A Sun wrote:
>>

>> -dev_err(ir->dev, "Error: request urb status = %d (TX HALT)",
>> +dev_err(ir->dev, "Error: tx urb status = %d (TX HALT)",
> 
> I am not sure this makes things clearer. Some of the code still refers
> to request, e.g. mce_request_packet.
> 
> Since this is an IR device, there is IR TX and RX; however this unrelated
> to that.
> 
> There is one urb/endpoint on which commands are sent; on the other, we
> receiver responses and IR data. Calling those TX and RX is not a good
> idea I think.

The tx urb described is also for IR data TX.
IR TX is one primary function/purpose of the device.
It happens that the urb/endpoint for IR TX is the same urb/endpoint for
mceusb device commands.

The same is found for IR data RX.
The urb/endpoint for IR RX is the same urb/endpoint for mceusb device
responses (to commands).

> 
> The existing code refers to request and response, and also TX and RX in
> places. The microsoft documentation refers to "command and response" which
> would be consistent with the #define's we have (MCE_CMD_* and MCE_RSP_*).
> 

My intent is to try and better word several of the USB layer messages,
and avoid confusing Microsoft MCE terms in those messages.

The original "(request) urb" phrases expand to "request USB Request Block"
and " USB Request Block" which are particularly confusing, and
non-specific, respectively.

Thanks, ..A Sun


Re: [PATCH v2 2/3] [media] mceusb: Reword messages referring to "urb"

2019-06-25 Thread Sean Young
On Wed, Jun 19, 2019 at 03:54:21AM -0400, A Sun wrote:
> 
> Clarify messages referencing "request urb" to mean "tx urb"
> (host transmit/send (to mceusb device)).
> Qualify messages referencing plain "urb" to mean "rx urb"
> (host receive (from mceusb device)).
> Add missing "couldn't allocate rx urb" error message.
> Clean extraneous "\n" in dev_dbg messages.
> 
> Signed-off-by: A Sun 
> ---
>  drivers/media/rc/mceusb.c | 24 +---
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/rc/mceusb.c b/drivers/media/rc/mceusb.c
> index 0cd8f6f57..efffb1795 100644
> --- a/drivers/media/rc/mceusb.c
> +++ b/drivers/media/rc/mceusb.c
> @@ -796,13 +796,13 @@ static void mce_async_callback(struct urb *urb)
>   break;
>  
>   case -EPIPE:
> - dev_err(ir->dev, "Error: request urb status = %d (TX HALT)",
> + dev_err(ir->dev, "Error: tx urb status = %d (TX HALT)",

I am not sure this makes things clearer. Some of the code still refers
to request, e.g. mce_request_packet.

Since this is an IR device, there is IR TX and RX; however this unrelated
to that.

There is one urb/endpoint on which commands are sent; on the other, we
receiver responses and IR data. Calling those TX and RX is not a good
idea I think.

The existing code refers to request and response, and also TX and RX in
places. The microsoft documentation refers to "command and response" which
would be consistent with the #define's we have (MCE_CMD_* and MCE_RSP_*).


Thanks

Sean