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

Lee Jones
2016-09-12T09:57:30Z

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