Messages in this thread | | | Date | Sun, 25 Aug 2013 10:04:38 +0200 | From | Sylwester Nawrocki <> | Subject | Re: [PATCH v2 3/3] gpio: pcf857x: Add OF support |
| |
On 08/25/2013 02:15 AM, Laurent Pinchart wrote: > On Saturday 24 August 2013 16:13:11 Tomasz Figa wrote: >> On Saturday 24 of August 2013 02:54:07 Laurent Pinchart wrote: >>> On Saturday 24 August 2013 02:41:59 Tomasz Figa wrote: >>>> On Tuesday 20 of August 2013 01:04:54 Laurent Pinchart wrote: >>>>> Add DT bindings for the pcf857x-compatible chips and parse the >>>>> device tree node in the driver. >>>>> >>>>> Signed-off-by: Laurent Pinchart >>>>> <laurent.pinchart+renesas@ideasonboard.com> --- >>>>> >>>>> .../devicetree/bindings/gpio/gpio-pcf857x.txt | 71 +++++++++++++ >>>>> drivers/gpio/gpio-pcf857x.c | 57 ++++++++++--- >>>>> 2 files changed, 119 insertions(+), 9 deletions(-) >>>>> create mode 100644 > > [snip] > >>>>> diff --git a/drivers/gpio/gpio-pcf857x.c >>>>> b/drivers/gpio/gpio-pcf857x.c >>>>> index 070e81f..50a90f1 100644 >>>>> --- a/drivers/gpio/gpio-pcf857x.c >>>>> +++ b/drivers/gpio/gpio-pcf857x.c >>> >>> [snip] >>> >>>>> @@ -50,6 +52,27 @@ static const struct i2c_device_id pcf857x_id[] = >>>>> { >>>>> >>>>> }; >>>>> MODULE_DEVICE_TABLE(i2c, pcf857x_id); >>>>> >>>>> +#ifdef CONFIG_OF >>>>> +static const struct of_device_id pcf857x_of_table[] = { >>>>> + { .compatible = "nxp,pcf8574", .data = (void *)8 }, >>>>> + { .compatible = "nxp,pcf8574a", .data = (void *)8 }, >>>>> + { .compatible = "nxp,pca8574", .data = (void *)8 }, >>>>> + { .compatible = "nxp,pca9670", .data = (void *)8 }, >>>>> + { .compatible = "nxp,pca9672", .data = (void *)8 }, >>>>> + { .compatible = "nxp,pca9674", .data = (void *)8 }, >>>>> + { .compatible = "nxp,pcf8575", .data = (void *)16 }, >>>>> + { .compatible = "nxp,pca8575", .data = (void *)16 }, >>>>> + { .compatible = "nxp,pca9671", .data = (void *)16 }, >>>>> + { .compatible = "nxp,pca9673", .data = (void *)16 }, >>>>> + { .compatible = "nxp,pca9675", .data = (void *)16 }, >>>>> + { .compatible = "maxim,max7328", .data = (void *)8 }, >>>>> + { .compatible = "maxim,max7329", .data = (void *)8 }, >>>>> + { .compatible = "ti,tca9554", .data = (void *)8 }, >>>>> + { } >>>>> +}; >>>>> +MODULE_DEVICE_TABLE(of, pcf857x_of_table); >>>>> +#endif >>>>> + >>>>> >>>>> /* >>>>> * The pcf857x, pca857x, and pca967x chips only expose one read and >>>>> * one write register. Writing a "one" bit (to match the reset >>>>> @@ -257,14 +280,29 @@ fail: >>>>> static int pcf857x_probe(struct i2c_client *client, >>>>> const struct i2c_device_id *id) >>>>> { >>>>> - struct pcf857x_platform_data *pdata; >>>>> + struct pcf857x_platform_data *pdata = client->dev.platform_data; >>>>> + struct device_node *np = client->dev.of_node; > > > > struct pcf857x *gpio; >>>>> + unsigned int n_latch = 0; >>>>> + unsigned int ngpio; >>>>> int status; >>>>> >>>>> - pdata = client->dev.platform_data; >>>>> - if (!pdata) { >>>>> +#ifdef CONFIG_OF >>>>> + if (np) { >>>> >>>> Wouldn't if (IS_ENABLED(CONFIG_OF)&& np) be sufficient here, without >>>> the #ifdef? You would have to move the match table out of the #ifdef >>>> in this case, though... >>> >>> That's the exact reason why I've used #ifdef CONFIG_OF here, I didn't >>> want to add the overhead of the pcf857x_of_table when CONFIG_OF isn't >>> defined. >> >> I'm not sure if I remember correctly, but I think there was something said >> in one of discussions some time ago, that we should be moving away from >> ifdef'ing such things, in favour of just having them compiled >> unconditionally. > > There seems to be a general consensus to favor if (IS_ENABLED(CONFIG_OF)) > instead of #ifdef CONFIG_OF when possible. I'm not sure what the opinion is > regarding using conditional compilation to avoid compiling unnecessary data > tables in. I would vote for using it (there's no need to bloat the kernel > unnecessarily on non-OF platforms), but I'll conform to whatever is decided to > be best. > >> [Adding DT maintainers on Cc for more opinions.] > > I'll resubmit the patch with the DT bindings documentation fixed, and will > submit yet another version if I need to remove the #ifdef.
I think it makes sense to keep this table compiled in conditionally, size of struct of_device_id is relatively large. While absolute increase in size might not be that significant the relative increase is quite large - appr. 130%.
Before $subject patch:
$ size drivers/gpio/gpio-pcf857x.o text data bss dec hex filename 2228 140 0 2368 940 drivers/gpio/gpio-pcf857x.o
After applying the patch:
$ size drivers/gpio/gpio-pcf857x.o text data bss dec hex filename 5284 140 0 5424 1530 drivers/gpio/gpio-pcf857x.o
-- Regards, Sylwester
| |