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]

Message-ID: <20160912095923.GD1873@dell>
Date:   Mon, 12 Sep 2016 10:59:23 +0100
From:   Lee Jones <lee.jones@...aro.org>
To:     Quentin Schulz <quentin.schulz@...e-electrons.com>
Cc:     jdelvare@...e.com, linux@...ck-us.net, jic23@...nel.org,
        knaack.h@....de, lars@...afoo.de, pmeerw@...erw.net,
        maxime.ripard@...e-electrons.com, 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 11:18, Lee Jones wrote:
> > On Thu, 08 Sep 2016, Quentin Schulz wrote:
> > 
> [...]

[...]

> >> +++ b/drivers/mfd/sun4i-gpadc-mfd.c

[...]

> >> +static struct mfd_cell sun4i_gpadc_mfd_cells[] = {
> >> +	{
> >> +		.name	= "sun4i-a10-gpadc-iio",
> >> +		.resources = adc_resources,
> >> +		.num_resources = ARRAY_SIZE(adc_resources),
> >> +	}, {
> >> +		.name = "iio_hwmon",
> >> +	}
> > 
> > Single line please
> > 
> > { .name = "iio_hwmon" }
> >
> 
> +	{
> +		.name	= "sun4i-a10-gpadc-iio",
> +		.resources = adc_resources,
> +		.num_resources = ARRAY_SIZE(adc_resources),
> +	}, { .name = "iio_hwmon" }
> 
> or
> 
> +	{
> +		.name	= "sun4i-a10-gpadc-iio",
> +		.resources = adc_resources,
> +		.num_resources = ARRAY_SIZE(adc_resources),
> +	},
> +	{ .name = "iio_hwmon" }
> 
> ?

The latter.

[...]

> >> +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().

> >> +static int sun4i_gpadc_mfd_probe(struct platform_device *pdev)
> > 
> > Remove all mention of "mfd" from this file.
> > 
> > (Accept the calls to the MFD API of course).
> > 
> [...]
> >> +
> >> +MODULE_DEVICE_TABLE(of, sun4i_gpadc_mfd_of_match);
> > 
> > Place this directly under the table.
> > 
> >> +static struct platform_driver sun4i_gpadc_mfd_driver = {
> >> +	.driver = {
> >> +		.name = "sun4i-adc-mfd",
> >> +		.of_match_table = of_match_ptr(sun4i_gpadc_mfd_of_match),
> >> +	},
> >> +	.probe = sun4i_gpadc_mfd_probe,
> > 
> > No .remove?
> > 
> 
> No, everything in probe is handled with devm functions.

Don't you need to undo the register write you did?

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