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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAKdAkRQZ+ug7tpkE4x_8iTbUm9JnyF5wWrUnPRASi98t25LTbA@mail.gmail.com>
Date:   Tue, 12 Feb 2019 23:30:17 -0800
From:   Dmitry Torokhov <dmitry.torokhov@...il.com>
To:     Lee Jones <lee.jones@...aro.org>
Cc:     Bartosz Golaszewski <brgl@...ev.pl>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Jacek Anaszewski <jacek.anaszewski@...il.com>,
        Pavel Machek <pavel@....cz>,
        Sebastian Reichel <sre@...nel.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        lkml <linux-kernel@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        DTML <devicetree@...r.kernel.org>,
        "linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
        Linux LED Subsystem <linux-leds@...r.kernel.org>,
        Linux PM <linux-pm@...r.kernel.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>
Subject: Re: [PATCH 12/13] input: max77650: add onkey support

Hi Lee,

On Tue, Feb 12, 2019 at 12:34 PM Lee Jones <lee.jones@...aro.org> wrote:
>
> > > From: Bartosz Golaszewski <bgolaszewski@...libre.com>
> > >
> > > Add support for the push- and slide-button events for max77650.
> > >
> > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@...libre.com>
> > > ---
> > >  drivers/input/misc/Kconfig          |   9 ++
> > >  drivers/input/misc/Makefile         |   1 +
> > >  drivers/input/misc/max77650-onkey.c | 135 ++++++++++++++++++++++++++++
> > >  3 files changed, 145 insertions(+)
> > >  create mode 100644 drivers/input/misc/max77650-onkey.c
>
> [...]
>
> Moving things around a bit:
>
> > > +static int max77650_onkey_probe(struct platform_device *pdev)
> > > +{
>
> > > +   irq_f = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_F);
> > > +   if (irq_f <= 0)
> > > +           return -EINVAL;
> > > +
> > > +   irq_r = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_R);
> > > +   if (irq_r <= 0)
> > > +           return -EINVAL;
> >
> > Ugh, it would be better if you handled IRQ mapping in the MFD piece and
> > passed it as resources of platform device. Then you'd simply call
> > platform_get_irq() here and did not have to reach into parent device for
> > "irq_dara".
>
> These device IRQs were defined and registered with the Regmap *set*
> (actually init()) APIs and thus should be pulled out using the
> appropriate reverse Regmap *get* APIs here in the device.
>
> Registering them with Regmap *and* pulling them back out again in the
> same (MFD in this case) driver, only to register them as platform data
> is certainly not how I see the API being designed/used.
>
> > > +   struct regmap_irq_chip_data *irq_data;
> > > +   struct device *dev, *parent;
>
> > > +   dev = &pdev->dev;
> > > +   parent = dev->parent;
> > > +   i2c = to_i2c_client(parent);
> > > +   irq_data = i2c_get_clientdata(i2c);
> > > +
> > > +   map = dev_get_regmap(parent, NULL);
> > > +   if (!map)
> > > +           return -ENODEV;
>
> However, this hoop jumping is a bit crazy and for the most part
> superfluous.  Instead, create a struct of attributes you wish to share
> with the child devices (containing both regmap (which you should call
> regmap and not map by the way) and irq_data) and pass it as either
> platform data (preferred) or as device data.
>
> If you choose the latter, you do not need to convert from 'device' to
> 'i2c' to do the look-up.  Since this function (probe()) is provided
> with a platform_device, you can just use platform_get_drvdata() to
> achieve the same as above.
>
> If you go the preferred (platform data) route, then you should use
> dev_get_platdata() instead.

By doing what you are suggesting (introducing platform data) you
introducing strong dependency between MFD and input piece for no
different reason. With the current implementation the parent can be
reworked completely without involving onkey driver.

I find such clean separation desirable. We are trying to move away
form platform data where it makes sense.

Thanks.

-- 
Dmitry

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ