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

[Lee Jones <lee.jones@linaro.org>]

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