Revisiting ITE8708 on ASUS PN50 uses a 16 byte io region

Revisiting ITE8708 on ASUS PN50 uses a 16 byte io region

[Nikolaos Beredimas]

Hi,
There was a thread on this list last September
https://www.spinics.net/lists/linux-media/msg177724.html
about the IR module on the ASUS PN50.

Even though that discussion never fully resolved,
it did contain the solution to get the IR working on the PN50.
I have documented this at
https://forum.libreelec.tv/thread/23145-asus-pn50-challenge/?postID=152207#post152207

So, what I had to do is edit a single line of drivers/media/rc/ite-cir.h
and change IT8708_IOREG_LENGTH 0x08 to IT8708_IOREG_LENGTH 0x10
and the IR module is now recognized and working

How do I go about submitting this as a patch?
I am a little overwhelmed honestly.
Do I follow https://www.linuxtv.org/wiki/index.php/Development:_How_to_submit_patches
?
And which git tree?

--- a/drivers/media/rc/ite-cir.h
+++ b/drivers/media/rc/ite-cir.h
@@ -406,7 +406,7 @@
 #define IT8708_C0WCR 0x06 /* wakeup code read/write register */
 #define IT8708_C0WPS 0x07 /* wakeup power control/status register */

-#define IT8708_IOREG_LENGTH 0x08 /* length of register file */
+#define IT8708_IOREG_LENGTH 0x10 /* length of register file */

 /* two more registers that are defined in the hacked driver, but can't be
  * found in the data sheets; no idea what they are or how they are accessed,

Re: Revisiting ITE8708 on ASUS PN50 uses a 16 byte io region

[Sean Young]

Hi Nikolaos,

On Wed, Mar 17, 2021 at 04:41:15PM +0200, Nikolaos Beredimas wrote:
> Hi,
> There was a thread on this list last September
> https://www.spinics.net/lists/linux-media/msg177724.html
> about the IR module on the ASUS PN50.
> 
> Even though that discussion never fully resolved,
> it did contain the solution to get the IR working on the PN50.
> I have documented this at
> https://forum.libreelec.tv/thread/23145-asus-pn50-challenge/?postID=152207#post152207
> 
> So, what I had to do is edit a single line of drivers/media/rc/ite-cir.h
> and change IT8708_IOREG_LENGTH 0x08 to IT8708_IOREG_LENGTH 0x10
> and the IR module is now recognized and working
> 
> How do I go about submitting this as a patch?
> I am a little overwhelmed honestly.
> Do I follow https://www.linuxtv.org/wiki/index.php/Development:_How_to_submit_patches
> ?
> And which git tree?

Thanks for fixing this. The patch should be a diff against
https://git.linuxtv.org/media_tree.git/

This is the guide for submitting patches:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html


> 
> --- a/drivers/media/rc/ite-cir.h
> +++ b/drivers/media/rc/ite-cir.h
> @@ -406,7 +406,7 @@
>  #define IT8708_C0WCR 0x06 /* wakeup code read/write register */
>  #define IT8708_C0WPS 0x07 /* wakeup power control/status register */
> 
> -#define IT8708_IOREG_LENGTH 0x08 /* length of register file */
> +#define IT8708_IOREG_LENGTH 0x10 /* length of register file */

I don't think this is correct though. There are other devices that have
length of 8; I think the correct solution.

I think:

        if (!pnp_port_valid(pdev, io_rsrc_no) ||
            pnp_port_len(pdev, io_rsrc_no) != dev_desc->io_region_size) {
                dev_err(&pdev->dev, "IR PNP Port not valid!\n");
                goto exit_free_dev_rdev;
        }


should be changed to:

        if (!pnp_port_valid(pdev, io_rsrc_no) ||
            pnp_port_len(pdev, io_rsrc_no) < dev_desc->io_region_size) {
                dev_err(&pdev->dev, "IR PNP Port not valid!\n");
                goto exit_free_dev_rdev;
        }

Thanks

> 
>  /* two more registers that are defined in the hacked driver, but can't be
>   * found in the data sheets; no idea what they are or how they are accessed,

Re: Revisiting ITE8708 on ASUS PN50 uses a 16 byte io region

[Nikolaos Beredimas]

Thanks for your response Sean

On Thu, Mar 18, 2021 at 11:48 AM Sean Young <sean@mess.org> wrote:
>
> Hi Nikolaos,
>
> On Wed, Mar 17, 2021 at 04:41:15PM +0200, Nikolaos Beredimas wrote:
> > Hi,
> > There was a thread on this list last September
> > https://www.spinics.net/lists/linux-media/msg177724.html
> > about the IR module on the ASUS PN50.
> >
> > Even though that discussion never fully resolved,
> > it did contain the solution to get the IR working on the PN50.
> > I have documented this at
> > https://forum.libreelec.tv/thread/23145-asus-pn50-challenge/?postID=152207#post152207
> >
> > So, what I had to do is edit a single line of drivers/media/rc/ite-cir.h
> > and change IT8708_IOREG_LENGTH 0x08 to IT8708_IOREG_LENGTH 0x10
> > and the IR module is now recognized and working
> >
> > How do I go about submitting this as a patch?
> > I am a little overwhelmed honestly.
> > Do I follow https://www.linuxtv.org/wiki/index.php/Development:_How_to_submit_patches
> > ?
> > And which git tree?
>
> Thanks for fixing this. The patch should be a diff against
> https://git.linuxtv.org/media_tree.git/
>
> This is the guide for submitting patches:
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html
>
>
> >
> > --- a/drivers/media/rc/ite-cir.h
> > +++ b/drivers/media/rc/ite-cir.h
> > @@ -406,7 +406,7 @@
> >  #define IT8708_C0WCR 0x06 /* wakeup code read/write register */
> >  #define IT8708_C0WPS 0x07 /* wakeup power control/status register */
> >
> > -#define IT8708_IOREG_LENGTH 0x08 /* length of register file */
> > +#define IT8708_IOREG_LENGTH 0x10 /* length of register file */
>
> I don't think this is correct though. There are other devices that have
> length of 8; I think the correct solution.
>
> I think:
>
>         if (!pnp_port_valid(pdev, io_rsrc_no) ||
>             pnp_port_len(pdev, io_rsrc_no) != dev_desc->io_region_size) {
>                 dev_err(&pdev->dev, "IR PNP Port not valid!\n");
>                 goto exit_free_dev_rdev;
>         }
>
>
> should be changed to:
>
>         if (!pnp_port_valid(pdev, io_rsrc_no) ||
>             pnp_port_len(pdev, io_rsrc_no) < dev_desc->io_region_size) {
>                 dev_err(&pdev->dev, "IR PNP Port not valid!\n");
>                 goto exit_free_dev_rdev;
>         }
>
> Thanks
>

Indeed. This was also pointed to me by another user at the LibreELEC forum where
I documented my initial "fix" (which although fixed the problem for my
specific hw,
would probably bork every other ITE8708 in existence).
I have implemented the proper fix per your suggestion, and have tested
it against
kernel version 5.10.21 which is the one used by the latest LibreELEC beta.
I think I have reached the limit of my abilities here.
Following is my first attempt at submitting this as a patch,
against the master branch of git://linuxtv.org/media_tree.git
NB
---------------

Accept larger io_region_size for ITE8708 in ite-cir.c
ITE8708 on ASUS PN50 uses a 16 byte io region. This issue was
first identified by Michael Zimmermann in
https://www.spinics.net/lists/linux-media/msg177724.html
and prevents the driver from loading on the ASUS PN50, throwing
the 'IR PNP Port not valid!' error.
Current understanding is that the issue lies on erroneous DSDT
ACPI tables on ASUS part, that advertise the register length as
16 bytes, and not 8 bytes as defined in ite-cir.h
(IT8708_IOREG_LENGTH).
In any case, this patch changes the strict not-equal check in the
ite_probe function to a less-than check, allowing the driver
to load despite the fact that the DSDT claims a larger register.

Signed-off-by: Nikolaos Beredimas <beredim@gmail.com>
---
 drivers/media/rc/ite-cir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/rc/ite-cir.c b/drivers/media/rc/ite-cir.c
index 9388774df9d7..5bc23e8c6d91 100644
--- a/drivers/media/rc/ite-cir.c
+++ b/drivers/media/rc/ite-cir.c
@@ -1333,7 +1333,7 @@ static int ite_probe(struct pnp_dev *pdev, const
struct pnp_device_id

/* validate pnp resources */
if (!pnp_port_valid(pdev, io_rsrc_no) ||
-     pnp_port_len(pdev, io_rsrc_no) != dev_desc->io_region_size) {
+     pnp_port_len(pdev, io_rsrc_no) < dev_desc->io_region_size) {
dev_err(&pdev->dev, "IR PNP Port not valid!\n");
goto exit_free_dev_rdev;
}
--

Re: Revisiting ITE8708 on ASUS PN50 uses a 16 byte io region

[Nikolaos Beredimas]

Hi Andy.

On Sat, Mar 20, 2021 at 11:51 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
>
> Congrats for your first submission! My comments below, starting from important one here: next time (v2 of the patch, you may use -v<N> parameter to `git format-patch ...`) send it with `git send-email ...`.
>
>
> This section basically can be transformed to two tags (should be below near to you SoB tag):
>
> BugLink: https://...
> Reported-by: Michael...

I appreciate your input. I'll start working on a v2 as per your suggestions.
I do have a quick question though. The email address of the original
bug reporter (Michael Zimmermann) is hidden from the list archives.
Can someone provide it, or is it ok for the attribution to contain
only the reporter's name?

NB

Re: Revisiting ITE8708 on ASUS PN50 uses a 16 byte io region

[Sean Young]

Hi Nikolaos,

On Sat, Mar 20, 2021 at 11:58:50PM +0200, Nikolaos Beredimas wrote:
> Hi Andy.
> 
> On Sat, Mar 20, 2021 at 11:51 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> >
> > Congrats for your first submission! My comments below, starting from important one here: next time (v2 of the patch, you may use -v<N> parameter to `git format-patch ...`) send it with `git send-email ...`.
> >
> >
> > This section basically can be transformed to two tags (should be below near to you SoB tag):
> >
> > BugLink: https://...
> > Reported-by: Michael...
> 
> I appreciate your input. I'll start working on a v2 as per your suggestions.
> I do have a quick question though. The email address of the original
> bug reporter (Michael Zimmermann) is hidden from the list archives.
> Can someone provide it, or is it ok for the attribution to contain
> only the reporter's name?

The full line should be:

Reported-by: Michael Zimmermann <sigmaepsilon92@gmail.com>

Thanks,

Sean