linux-kernel - Re: [PATCH v4] mfd: tqmx86: IO controller with i2c, wachdog and gpio

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20190201164933.GH30115@lunn.ch>
Date:   Fri, 1 Feb 2019 17:49:33 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Lee Jones <lee.jones@...aro.org>
Cc:     linux-kernel@...r.kernel.org,
        Emeric Dupont <emeric.dupont@....aero>
Subject: Re: [PATCH v4] mfd: tqmx86: IO controller with i2c, wachdog and gpio

> > > For platform data, it should be "*_platform_data" or "*_pdata".
> > 
> > It would of been useful if you had said this in the first review,
> > rather than s/data/ddata/, which is rather ambiguous.
> 
> How is that ambiguous?  I guess it would be confusing if you didn't
> know the syntax, in which case you should have asked.
> 
> s/this/that/ simply means, replace 'this' with 'that'.

It is ambiguous if you mean just that one line, one variable, or the
whole file.

> For platform data, it should be "*_platform_data" or "*_pdata".

This is very unambiguous.

> > > > +static uint gpio_irq;
> > > > +module_param(gpio_irq, uint, 0);
> > > > +MODULE_PARM_DESC(gpio_irq, "GPIO IRQ number (7, 9, 12)");
> > > 
> > > What is the purpose of providing the IRQ number via a module param?
> > > 
> > > These seems like a very bad idea.
> > 
> > I explained this in my reply to your review comments for version 2.
> > 
> > > Can this driver be built-in?
> > 
> > Yes it can. But you can pass module parameters on the command line, so
> > that is not an issue. This is something i actually use.
> 
> What is connected to these IRQs?

MODULE_PARM_DESC says:

"GPIO IRQ number (7, 9, 12)"

One of the devices this MFD instantiates is a GPIO controller. Linus
Walleij accepted it a few days ago:

https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/commit/?h=for-next&id=b868db94a6a704755a33a362cfcf4625abdda115

It can generate interrupts on its GPI's. Unfortunately, the interrupt
the GPIO device uses needs to be configured in the io_ext_int register
which is shared by all devices in the MFD, and it has to unique across
all devices in the MFD. So the module parameter is here, and it then
gets passed to the GPIO driver as a resource in the usual way.

     Andrew