Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752572AbbH1M73 (ORCPT ); Fri, 28 Aug 2015 08:59:29 -0400 Received: from mail-yk0-f179.google.com ([209.85.160.179]:35544 "EHLO mail-yk0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752057AbbH1M70 (ORCPT ); Fri, 28 Aug 2015 08:59:26 -0400 MIME-Version: 1.0 In-Reply-To: <1440753128-3288-1-git-send-email-milo.kim@ti.com> References: <1440753128-3288-1-git-send-email-milo.kim@ti.com> From: Rob Herring Date: Fri, 28 Aug 2015 07:59:06 -0500 Message-ID: Subject: Re: [RFC 0/4] of: introduce of_dev_get_platdata() To: Milo Kim Cc: Grant Likely , "devicetree@vger.kernel.org" , Dmitry Torokhov , Felipe Balbi , Greg Kroah-Hartman , Lee Jones , Rob Herring , Samuel Ortiz , Tony Lindgren , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7453 Lines: 213 On Fri, Aug 28, 2015 at 4:12 AM, Milo Kim wrote: > New function, 'of_dev_get_platdata()' > - provides unified handling of getting device platform data > - supports DT and non-DT(legacy) cases > - removes duplicated code from each driver > - keeps driver specific code simple in each driver This works in cases where DT data and platform_data are aligned. In many cases they are not. A common binding problem is people blindly copying platform_data fields to DT properties for things that are not h/w properties. I worry that this would encourage this behavior. We already have a generalized method for retrieving properties independent of DT or ACPI. There's no reason this couldn't be extended to retrieve properties out of platform_data using the same interface. Also, perhaps in some drivers we can remove platform_data now if all users are converted to DT. Rob > > Issues > ------ > On loading a driver, the driver tries to get the platform data. > * Check conditions prior to parsing the DT > You can see various/not general way of checking the platform data. > > case 1) driver checks whether the platform data is null or not. > > foo_platform_data *pdata = dev_get_platdata(dev); > if (!pdata) { > pdata = foo_parse_dt(); > if (IS_ERR(pdata)) > return PTR_ERR(pdata); > } > > case 2) driver checks whether 'of_node' exists or not. > > if (dev.of_node) { > pdata = foo_parse_dt(); > if (IS_ERR(pdata)) > return PTR_ERR(pdata); > } > > case 3) check both > > if (pdata) { > copy pdata into driver private data > } else if (dev.of_node) { > pdata = foo_parse_dt(); > } > > Check conditions are very depend on the driver, but it can be unified. > of_dev_get_platdata() provides unified handling. So the driver can reduce > if-condition statements. > > * Memory allocation in each parser > In most cases, driver allocates the platform data inside its DT parser. > > static struct foo_platform_data *foo_parse_dt() > { > allocates memory for generating platform data. > parsing DT properties and copy them into the platform data. > } > > of_dev_get_platdata() allocates the platform data internally. > And it calls back to the driver specific parser function. It removes > memory allocation code in each driver. > > * Two types of parser definition > Many drivers implement DT parser in two cases, '#ifdef CONFIG_OF' and > '#else'. > > #ifdef CONFIG_OF > static struct foo_platform_data *foo_parse_dt() > { > (snip) > } > #else > static struct foo_platform_data *foo_parse_dt() > { > return NULL; > } > #endif > > of_dev_get_platdata() supports both cases. It removes few lines of code > in each driver. > > What of_dev_get_platdata() does > ------------------------------- > if CONFIG_OF is not defined, return legacy code, 'dev_get_platdata()'. > if platform data exists, just return it. > if platform data is null, check the 'of node'. > if of_node is null, then return NULL. > Otherwise, allocates memory for platform data. > Call back to the driver(caller) with allocated platform data and > private data if needed. > Then, driver will parse the DT internally and handle errors. > > Examples > -------- > Following patches are examples. > > drivers/input/touchscreen/mms114.c > drivers/mfd/tps65910.c > drivers/usb/musb/ux500.c > > Driver list > ----------- > of_dev_get_platdata() can be applied into files below. You may find more > if you're interested in this. :) > > drivers/dma/ste_dma40.c > drivers/gpio/gpio-rcar.c > drivers/gpu/drm/exynos/exynos_dp_core.c > drivers/gpu/drm/exynos/exynos_hdmi.c > drivers/hwmon/ntc_thermistor.c > drivers/i2c/busses/i2c-s3c2410.c > drivers/iio/frequency/adf4350.c > drivers/input/keyboard/matrix_keypad.c > drivers/input/keyboard/samsung-keypad.c > drivers/input/misc/drv260x.c > drivers/input/misc/regulator-haptic.c > drivers/input/misc/rotary_encoder.c > drivers/input/touchscreen/atmel_mxt_ts.c > drivers/input/touchscreen/auo-pixcir-ts.c > drivers/input/touchscreen/bu21013_ts.c > drivers/input/touchscreen/mms114.c > drivers/input/touchscreen/pixcir_i2c_ts.c > drivers/input/touchscreen/zforce_ts.c > drivers/leds/leds-lp5521.c > drivers/leds/leds-lp5523.c > drivers/leds/leds-lp5562.c > drivers/leds/leds-lp55xx-common.c > drivers/leds/leds-lp8501.c > drivers/leds/leds-mc13783.c > drivers/leds/leds-pca963x.c > drivers/leds/leds-tca6507.c > drivers/mfd/max8997.c > drivers/mfd/max8998.c > drivers/mfd/sec-core.c > drivers/mfd/tps6586x.c > drivers/mfd/tps65910.c > drivers/misc/lis3lv02d/lis3lv02d.c > drivers/mmc/host/davinci_mmc.c > drivers/mmc/host/dw_mmc.c > drivers/mmc/host/sdhci-s3c.c > drivers/mtd/devices/spear_smi.c > drivers/mtd/nand/fsmc_nand.c > drivers/mtd/nand/lpc32xx_mlc.c > drivers/mtd/nand/lpc32xx_slc.c > drivers/mtd/nand/sh_flctl.c > drivers/net/can/sja1000/sja1000_platform.c > drivers/net/ethernet/davicom/dm9000.c > drivers/net/ethernet/marvell/mv643xx_eth.c > drivers/net/ethernet/renesas/sh_eth.c > drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c > drivers/power/bq24735-charger.c > drivers/power/gpio-charger.c > drivers/power/sbs-battery.c > drivers/power/tps65090-charger.c > drivers/power/twl4030_charger.c > drivers/pwm/pwm-lp3943.c > drivers/pwm/pwm-samsung.c > drivers/spi/spi-sh-msiof.c > drivers/spi/spi/spi-s3c64xx.c > drivers/thermal/db8500_thermal.c > drivers/usb/misc/usb3503.c > drivers/usb/musb/omap2430.c > drivers/usb/musb/ux500.c > drivers/usb/renesas_usbhs/common.c > drivers/video/fbdev/simplefb.c > drivers/video/backlight/lp855x_bl.c > drivers/video/backlight/pwm_bl.c > drivers/video/backlight/sky81452-backlight.c > drivers/video/backlight/tps65217_bl.c > > This is the RFC, so I would like to get some feedback prior to patching all > drivers. Any comments are welcome. > > Cc: Dmitry Torokhov > Cc: Felipe Balbi > Cc: Grant Likely > Cc: Greg Kroah-Hartman > Cc: Lee Jones > Cc: Rob Herring > Cc: Samuel Ortiz > Cc: Tony Lindgren > Cc: devicetree@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > > Milo Kim (4): > of: introduce of_dev_get_platdata() > input: touchscree: mms114: use of_dev_get_platdata() > mfd: tps65910: use of_dev_get_platdata() > usb: musb: use of_dev_get_platdata() > > drivers/input/touchscreen/mms114.c | 34 ++++++++------------------ > drivers/mfd/tps65910.c | 49 +++++++++++++------------------------- > drivers/of/device.c | 46 +++++++++++++++++++++++++++++++++++ > drivers/usb/musb/ux500.c | 40 +++++++++++++------------------ > include/linux/of_device.h | 12 ++++++++++ > 5 files changed, 100 insertions(+), 81 deletions(-) > > -- > 1.9.1 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/