linux-kernel - Re: [PATCH v5 2/3] mfd: add support for Allwinner SoCs ADC

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]

Date:   Mon, 12 Sep 2016 14:56:55 +0100
From:   Lee Jones <lee.jones@...aro.org>
To:     Quentin Schulz <quentin.schulz@...e-electrons.com>
Cc:     Maxime Ripard <maxime.ripard@...e-electrons.com>,
        jdelvare@...e.com, linux@...ck-us.net, jic23@...nel.org,
        knaack.h@....de, lars@...afoo.de, pmeerw@...erw.net, wens@...e.org,
        thomas.petazzoni@...e-electrons.com,
        antoine.tenart@...e-electrons.com, linux-kernel@...r.kernel.org,
        linux-hwmon@...r.kernel.org, linux-iio@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v5 2/3] mfd: add support for Allwinner SoCs ADC

On Mon, 12 Sep 2016, Quentin Schulz wrote:
> On 12/09/2016 12:49, Lee Jones wrote:
> > On Mon, 12 Sep 2016, Maxime Ripard wrote:
> > 
> >> On Mon, Sep 12, 2016 at 10:59:23AM +0100, Lee Jones wrote:
> >>>>>> +static const struct of_device_id sun4i_gpadc_mfd_of_match[] = {
> >>>>>> +	{
> >>>>>> +		.compatible = "allwinner,sun4i-a10-ts",
> >>>>>> +		.data = &sun4i_gpadc_mfd_cells,
> >>>>>> +	}, {
> >>>>>> +		.compatible = "allwinner,sun5i-a13-ts",
> >>>>>> +		.data = &sun5i_gpadc_mfd_cells,
> >>>>>> +	}, {
> >>>>>> +		.compatible = "allwinner,sun6i-a31-ts",
> >>>>>> +		.data = &sun6i_gpadc_mfd_cells,
> >>>>>> +	}, { /* sentinel */ }
> >>>>>> +};
> >>>>>
> >>>>> Don't mix OF and MFD functionality.
> >>>>>
> >>>>> Why don't you create a node for "iio_hwmon" and have
> >>>>> platform_of_populate() do your bidding?
> >>>>>
> >>>>
> >>>> We are using a stable binding which we cannot modify. This means, the DT
> >>>> in its current state can only be modified to add features, which is not
> >>>> the case of this driver (it is a rewriting of an existing driver which
> >>>> uses the rtp node).
> >>>
> >>> Then use .data = <defined model ID> and set up a switch() in .probe().
> >>
> >> Uh? Why? It just adds a non-standard indirection, while using
> >> of_match_device is very standard, and used extensively in Linux.
> > 
> > You still use of_match_device() to obtain the ID.
> > 
> > The "don't mix DT with the MFD API" is there to prevent some of the
> > nasty hacks I've seen previously.  This particular example doesn't
> > seem so bad, but it's a gateway to ridiculous hackery!
> 
> How am I supposed to get the .data without of_match_node then?
> What's more hackish in using .data field for specific data for each
> compatible than in using a random ID in .data and switching on it? The
> result is exactly the same, the switching case being more verbose and
> adding complexity to something that can be done in a straightforward manner.

I've already agreed that your implementation isn't terrible, but I'd
still like to remain strict on the rules.

Better still, can you can dynamically test which platform you're on,
via a version register or similar?

Failing that, see how everyone else does it:

 `git grep "\.data" -- drivers/mfd/`

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog