We use dev_get_drvdata to get the driver match data associated with a device. This has two shortcomings: - Linux has dev_get_drvdata too, which returns a private pointer for driver specific info to associate with a device. We use dev->priv (or more often container_of) for that in barebox instead - It nearly always involves a cast to a double pointer, which is error-prone as size and alignment match need to be ensured on the programmer's part and can easily be gotten wrong: enum dev_type type dev_get_drvdata(dev, (const void **)&type); Add a new function that instead of using a double pointer argument, returns the pointer directly: - For normal pointer driver data, no cast is necessary - For integer driver data casted to a pointer for storage, the cast is still necessary, but Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> --- drivers/base/driver.c | 11 +++++++++++ include/driver.h | 18 ++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/drivers/base/driver.c b/drivers/base/driver.c index 412db6c40699..3205bbc3c33b 100644 --- a/drivers/base/driver.c +++ b/drivers/base/driver.c @@ -500,3 +500,14 @@ int dev_get_drvdata(struct device_d *dev, const void **data) return -ENODEV; } + +const void *device_get_match_data(struct device_d *dev) +{ + if (dev->of_id_entry) + return dev->of_id_entry->data; + + if (dev->id_entry) + return (void *)dev->id_entry->driver_data; + + return NULL; +} diff --git a/include/driver.h b/include/driver.h index 154525e0fde7..14220431f2d6 100644 --- a/include/driver.h +++ b/include/driver.h @@ -534,8 +534,26 @@ int devfs_create_partitions(const char *devname, #define DRV_OF_COMPAT(compat) \ IS_ENABLED(CONFIG_OFDEVICE) ? (compat) : NULL +/** + * dev_get_drvdata - get driver match data associated with device + * @dev: device instance + * @data: pointer to void *, where match data is stored + * + * Returns 0 on success and error code otherwise. + * + * DEPRECATED: use device_get_match_data instead, which avoids + * common pitfalls due to explicit pointer casts + */ int dev_get_drvdata(struct device_d *dev, const void **data); +/** + * device_get_match_data - get driver match data associated with device + * @dev: device instance + * + * Returns match data on success and NULL otherwise + */ +const void *device_get_match_data(struct device_d *dev); + int device_match_of_modalias(struct device_d *dev, struct driver_d *drv); #endif /* DRIVER_H */ -- 2.28.0 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
dev->id_entry is not populated for devices probed from the device tree. It was used unconditionally however. Use device_get_match_data instead to support device tree probing. Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> --- drivers/led/led-pca955x.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/led/led-pca955x.c b/drivers/led/led-pca955x.c index 27fefce8d524..f89fcbfba5ac 100644 --- a/drivers/led/led-pca955x.c +++ b/drivers/led/led-pca955x.c @@ -349,8 +349,11 @@ static int led_pca955x_probe(struct device_d *dev) struct i2c_client *client; int err; struct pca955x_platform_data *pdata; + enum pca955x_type type; - chip = &pca955x_chipdefs[dev->id_entry->driver_data]; + type = (enum pca955x_type)device_get_match_data(dev); + + chip = &pca955x_chipdefs[type]; client = to_i2c_client(dev); /* Make sure the slave address / chip type combo given is possible */ -- 2.28.0 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
i.MX8 MM & MN are both ARM64 SoCs with a device compatible with "fsl,imx28-dma-apbh". Probing the barebox device driver on these SoCs would invoke undefined behavior however, because of an errant cast. Fix this. This has a side-effect: Whereas before, probing devices matched by driver name failed with -ENODEV, they are now considered to be compatible to IMX23_DMA instead. As we don't depend on this, this is deemed acceptable. Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> --- drivers/dma/apbh_dma.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/dma/apbh_dma.c b/drivers/dma/apbh_dma.c index 3bee89f78b85..186c69388c7f 100644 --- a/drivers/dma/apbh_dma.c +++ b/drivers/dma/apbh_dma.c @@ -596,9 +596,7 @@ static int apbh_dma_probe(struct device_d *dev) enum mxs_dma_id id; int ret, channel; - ret = dev_get_drvdata(dev, (const void **)&id); - if (ret) - return ret; + id = (enum mxs_dma_id)device_get_match_data(dev); apbh_dma = apbh = xzalloc(sizeof(*apbh)); iores = dev_request_mem_resource(dev, 0); -- 2.28.0 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
Probing the lm75 device driver on 64 bit systems invokes undefined behavior, because of an errant cast. Fix this. This has a side-effect: Whereas before, probing devices matched by driver name failed with -ENODEV, they are now considered to be compatible to adt75 instead. As we don't depend on this, this is deemed acceptable. Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> --- drivers/aiodev/lm75.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/aiodev/lm75.c b/drivers/aiodev/lm75.c index 8186fd2c2b97..4bdccaad5828 100644 --- a/drivers/aiodev/lm75.c +++ b/drivers/aiodev/lm75.c @@ -109,9 +109,7 @@ static int lm75_probe(struct device_d *dev) int new, ret; enum lm75_type kind; - ret = dev_get_drvdata(dev, (const void **)&kind); - if (ret) - return ret; + kind = (enum lm75_type)device_get_match_data(dev); data = xzalloc(sizeof(*data)); -- 2.28.0 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
Probing the nand_mxs device driver on 64 bit systems invokes undefined behavior, because of an errant cast. Fix this. No change of behavior for 32-bit SoCs intended. Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> --- drivers/mtd/nand/nand_mxs.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/mtd/nand/nand_mxs.c b/drivers/mtd/nand/nand_mxs.c index 36b6e7ac224e..e2b6e2162076 100644 --- a/drivers/mtd/nand/nand_mxs.c +++ b/drivers/mtd/nand/nand_mxs.c @@ -2145,9 +2145,7 @@ static int mxs_nand_probe(struct device_d *dev) if (mxs_nand_mtd) return -EBUSY; - err = dev_get_drvdata(dev, (const void **)&type); - if (err) - type = GPMI_MXS; + type = (enum gpmi_type)device_get_match_data(dev); nand_info = kzalloc(sizeof(struct mxs_nand_info), GFP_KERNEL); if (!nand_info) { -- 2.28.0 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
The driver has a couple of issues in how it handles match data: - First use of dev_get_drvdata is superfluous as result is unused - Second use of dev_get_drvdata stores a sizeof(const void *) into an enum typed object - hdmi->dev_type contains a truncated pointer to a struct dw_hdmi_data and compares it with an enum, which will always fail Fix these and while it, refactor the code a bit to get rid of dw_hdmi_data, whose only other member is unused. Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> --- drivers/video/imx-ipu-v3/imx-hdmi.c | 39 ++++++----------------------- 1 file changed, 8 insertions(+), 31 deletions(-) diff --git a/drivers/video/imx-ipu-v3/imx-hdmi.c b/drivers/video/imx-ipu-v3/imx-hdmi.c index 17b6e4cc257d..1e55c97d2426 100644 --- a/drivers/video/imx-ipu-v3/imx-hdmi.c +++ b/drivers/video/imx-ipu-v3/imx-hdmi.c @@ -1083,19 +1083,18 @@ static void dw_hdmi_enable_video_path(struct dw_hdmi *hdmi) /* Workaround to clear the overflow condition */ static void dw_hdmi_clear_overflow(struct dw_hdmi *hdmi) { - int count; + int count = 4; u8 val; /* TMDS software reset */ hdmi_writeb(hdmi, (u8)~HDMI_MC_SWRSTZ_TMDSSWRST_REQ, HDMI_MC_SWRSTZ); val = hdmi_readb(hdmi, HDMI_FC_INVIDCONF); - if (hdmi->dev_type == IMX6DL_HDMI) { - hdmi_writeb(hdmi, val, HDMI_FC_INVIDCONF); - return; - } - for (count = 0; count < 4; count++) + if (hdmi->dev_type == IMX6DL_HDMI) + count = 1; + + while (count--) hdmi_writeb(hdmi, val, HDMI_FC_INVIDCONF); } @@ -1193,28 +1192,13 @@ static void initialize_hdmi_ih_mutes(struct dw_hdmi *hdmi) hdmi_writeb(hdmi, ih_mute, HDMI_IH_MUTE); } -struct dw_hdmi_data { - unsigned ipu_mask; - enum dw_hdmi_devtype devtype; -}; - -static struct dw_hdmi_data imx6q_hdmi_data = { - .ipu_mask = 0xf, - .devtype = IMX6Q_HDMI, -}; - -static struct dw_hdmi_data imx6dl_hdmi_data = { - .ipu_mask = 0x3, - .devtype = IMX6DL_HDMI, -}; - static struct of_device_id dw_hdmi_dt_ids[] = { { .compatible = "fsl,imx6q-hdmi", - .data = &imx6q_hdmi_data, + .data = (void *)IMX6Q_HDMI, }, { .compatible = "fsl,imx6dl-hdmi", - .data = &imx6dl_hdmi_data, + .data = (void *)IMX6DL_HDMI, }, { /* sentinel */ } @@ -1276,11 +1260,6 @@ static int dw_hdmi_probe(struct device_d *dev) struct device_node *np = dev->device_node; struct dw_hdmi *hdmi; int ret; - const struct dw_hdmi_data *devtype; - - ret = dev_get_drvdata(dev, (const void **)&devtype); - if (ret) - return ret; hdmi = xzalloc(sizeof(*hdmi)); @@ -1289,9 +1268,7 @@ static int dw_hdmi_probe(struct device_d *dev) hdmi->sample_rate = 48000; hdmi->ratio = 100; - ret = dev_get_drvdata(dev, (const void **)&hdmi->dev_type); - if (ret) - return ret; + hdmi->dev_type = (enum dw_hdmi_devtype)device_get_match_data(dev); hdmi->ddc_node = of_parse_phandle(np, "ddc-i2c-bus", 0); -- 2.28.0 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
The dev_get_drvdata instances here all store a valid pointer in the match data and can be readily converted. Do so. Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> --- drivers/mfd/da9063.c | 4 +--- drivers/pinctrl/imx-iomux-v3.c | 4 ++-- drivers/pinctrl/pinctrl-at91-pio4.c | 4 ++-- drivers/pinctrl/pinctrl-tegra-xusb.c | 2 +- drivers/pinctrl/pinctrl-tegra30.c | 4 ++-- drivers/serial/serial_ns16550.c | 6 ++---- 6 files changed, 10 insertions(+), 14 deletions(-) diff --git a/drivers/mfd/da9063.c b/drivers/mfd/da9063.c index e48c38affac6..31359cf8b822 100644 --- a/drivers/mfd/da9063.c +++ b/drivers/mfd/da9063.c @@ -370,11 +370,9 @@ static int da9063_probe(struct device_d *dev) { struct da9063 *priv = NULL; struct da906x_device_data const *dev_data; - void const *dev_data_tmp; int ret; - ret = dev_get_drvdata(dev, &dev_data_tmp); - dev_data = ret < 0 ? NULL : dev_data_tmp; + dev_data = device_get_match_data(dev); priv = xzalloc(sizeof(struct da9063)); priv->wd.set_timeout = da9063_watchdog_set_timeout; diff --git a/drivers/pinctrl/imx-iomux-v3.c b/drivers/pinctrl/imx-iomux-v3.c index fd0527451276..4276981d94c0 100644 --- a/drivers/pinctrl/imx-iomux-v3.c +++ b/drivers/pinctrl/imx-iomux-v3.c @@ -168,10 +168,10 @@ static struct pinctrl_ops imx_iomux_v3_ops = { static int imx_pinctrl_dt(struct device_d *dev, void __iomem *base) { struct imx_iomux_v3 *iomux; - struct imx_iomux_v3_data *drvdata = NULL; + const struct imx_iomux_v3_data *drvdata; int ret; - dev_get_drvdata(dev, (const void **)&drvdata); + drvdata = device_get_match_data(dev); iomux = xzalloc(sizeof(*iomux)); iomux->base = base; diff --git a/drivers/pinctrl/pinctrl-at91-pio4.c b/drivers/pinctrl/pinctrl-at91-pio4.c index b527114f1ba0..18b1aded6a1b 100644 --- a/drivers/pinctrl/pinctrl-at91-pio4.c +++ b/drivers/pinctrl/pinctrl-at91-pio4.c @@ -229,7 +229,7 @@ static struct gpio_ops at91_gpio4_ops = { static int pinctrl_at91_pio4_gpiochip_add(struct device_d *dev, struct pinctrl_at91_pio4 *pinctrl) { - struct at91_pinctrl_data *drvdata; + const struct at91_pinctrl_data *drvdata; struct clk *clk; int ret; @@ -247,7 +247,7 @@ static int pinctrl_at91_pio4_gpiochip_add(struct device_d *dev, return ret; } - dev_get_drvdata(dev, (const void **)&drvdata); + drvdata = device_get_match_data(dev); pinctrl->gpiochip.ops = &at91_gpio4_ops; pinctrl->gpiochip.base = 0; diff --git a/drivers/pinctrl/pinctrl-tegra-xusb.c b/drivers/pinctrl/pinctrl-tegra-xusb.c index c4d3bbe8d429..558742665ea8 100644 --- a/drivers/pinctrl/pinctrl-tegra-xusb.c +++ b/drivers/pinctrl/pinctrl-tegra-xusb.c @@ -388,7 +388,7 @@ static int pinctrl_tegra_xusb_probe(struct device_d *dev) dev->priv = padctl; padctl->dev = dev; - dev_get_drvdata(dev, (const void **)&padctl->soc); + padctl->soc = device_get_match_data(dev); iores = dev_request_mem_resource(dev, 0); if (IS_ERR(iores)) { diff --git a/drivers/pinctrl/pinctrl-tegra30.c b/drivers/pinctrl/pinctrl-tegra30.c index 278ea8c4a0b3..cc71b79f6c9b 100644 --- a/drivers/pinctrl/pinctrl-tegra30.c +++ b/drivers/pinctrl/pinctrl-tegra30.c @@ -32,7 +32,7 @@ struct pinctrl_tegra30 { u32 __iomem *mux; } regs; struct pinctrl_device pinctrl; - struct pinctrl_tegra30_drvdata *drvdata; + const struct pinctrl_tegra30_drvdata *drvdata; }; struct tegra_pingroup { @@ -893,7 +893,7 @@ static int pinctrl_tegra30_probe(struct device_d *dev) regs[i] = IOMEM(iores->start); } - dev_get_drvdata(dev, (const void **)&ctrl->drvdata); + ctrl->drvdata = device_get_match_data(dev); ctrl->pinctrl.dev = dev; ctrl->pinctrl.ops = &pinctrl_tegra30_ops; diff --git a/drivers/serial/serial_ns16550.c b/drivers/serial/serial_ns16550.c index f117ab9dc10a..fc6fa7dc3e43 100644 --- a/drivers/serial/serial_ns16550.c +++ b/drivers/serial/serial_ns16550.c @@ -468,12 +468,10 @@ static int ns16550_probe(struct device_d *dev) struct ns16550_priv *priv; struct console_device *cdev; struct NS16550_plat *plat = (struct NS16550_plat *)dev->platform_data; - struct ns16550_drvdata *devtype; + const struct ns16550_drvdata *devtype; int ret; - ret = dev_get_drvdata(dev, (const void **)&devtype); - if (ret) - devtype = &ns16550_drvdata; + devtype = device_get_match_data(dev) ?: &ns16550_drvdata; priv = xzalloc(sizeof(*priv)); -- 2.28.0 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
On 9/28/20 5:42 PM, Ahmad Fatoum wrote: > We use dev_get_drvdata to get the driver match data associated with a > device. This has two shortcomings: > > - Linux has dev_get_drvdata too, which returns a private pointer for > driver specific info to associate with a device. We use dev->priv > (or more often container_of) for that in barebox instead > > - It nearly always involves a cast to a double pointer, which is > error-prone as size and alignment match need to be ensured > on the programmer's part and can easily be gotten wrong: > enum dev_type type > dev_get_drvdata(dev, (const void **)&type); > > Add a new function that instead of using a double pointer argument, > returns the pointer directly: > > - For normal pointer driver data, no cast is necessary > - For integer driver data casted to a pointer for storage, > the cast is still necessary, but That ", but" should be removed. Sascha, could you fix this up on apply (assuming there won't be a v2)? > > Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> > --- > drivers/base/driver.c | 11 +++++++++++ > include/driver.h | 18 ++++++++++++++++++ > 2 files changed, 29 insertions(+) > > diff --git a/drivers/base/driver.c b/drivers/base/driver.c > index 412db6c40699..3205bbc3c33b 100644 > --- a/drivers/base/driver.c > +++ b/drivers/base/driver.c > @@ -500,3 +500,14 @@ int dev_get_drvdata(struct device_d *dev, const void **data) > > return -ENODEV; > } > + > +const void *device_get_match_data(struct device_d *dev) > +{ > + if (dev->of_id_entry) > + return dev->of_id_entry->data; > + > + if (dev->id_entry) > + return (void *)dev->id_entry->driver_data; > + > + return NULL; > +} > diff --git a/include/driver.h b/include/driver.h > index 154525e0fde7..14220431f2d6 100644 > --- a/include/driver.h > +++ b/include/driver.h > @@ -534,8 +534,26 @@ int devfs_create_partitions(const char *devname, > #define DRV_OF_COMPAT(compat) \ > IS_ENABLED(CONFIG_OFDEVICE) ? (compat) : NULL > > +/** > + * dev_get_drvdata - get driver match data associated with device > + * @dev: device instance > + * @data: pointer to void *, where match data is stored > + * > + * Returns 0 on success and error code otherwise. > + * > + * DEPRECATED: use device_get_match_data instead, which avoids > + * common pitfalls due to explicit pointer casts > + */ > int dev_get_drvdata(struct device_d *dev, const void **data); > > +/** > + * device_get_match_data - get driver match data associated with device > + * @dev: device instance > + * > + * Returns match data on success and NULL otherwise > + */ > +const void *device_get_match_data(struct device_d *dev); > + > int device_match_of_modalias(struct device_d *dev, struct driver_d *drv); > > #endif /* DRIVER_H */ > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
On 20-09-28 17:42, Ahmad Fatoum wrote: > Probing the nand_mxs device driver on 64 bit systems invokes > undefined behavior, because of an errant cast. Fix this. > > No change of behavior for 32-bit SoCs intended. > > Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> > --- > drivers/mtd/nand/nand_mxs.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/mtd/nand/nand_mxs.c b/drivers/mtd/nand/nand_mxs.c > index 36b6e7ac224e..e2b6e2162076 100644 > --- a/drivers/mtd/nand/nand_mxs.c > +++ b/drivers/mtd/nand/nand_mxs.c > @@ -2145,9 +2145,7 @@ static int mxs_nand_probe(struct device_d *dev) > if (mxs_nand_mtd) > return -EBUSY; > > - err = dev_get_drvdata(dev, (const void **)&type); > - if (err) > - type = GPMI_MXS; > + type = (enum gpmi_type)device_get_match_data(dev); Should we add: if (!type) type = GPMI_MXS; here? > nand_info = kzalloc(sizeof(struct mxs_nand_info), GFP_KERNEL); > if (!nand_info) { > -- > 2.28.0 > > > _______________________________________________ > barebox mailing list > barebox@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/barebox > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
On 9/28/20 6:01 PM, Marco Felsch wrote: > On 20-09-28 17:42, Ahmad Fatoum wrote: >> Probing the nand_mxs device driver on 64 bit systems invokes >> undefined behavior, because of an errant cast. Fix this. >> >> No change of behavior for 32-bit SoCs intended. >> >> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> >> --- >> drivers/mtd/nand/nand_mxs.c | 4 +--- >> 1 file changed, 1 insertion(+), 3 deletions(-) >> >> diff --git a/drivers/mtd/nand/nand_mxs.c b/drivers/mtd/nand/nand_mxs.c >> index 36b6e7ac224e..e2b6e2162076 100644 >> --- a/drivers/mtd/nand/nand_mxs.c >> +++ b/drivers/mtd/nand/nand_mxs.c >> @@ -2145,9 +2145,7 @@ static int mxs_nand_probe(struct device_d *dev) >> if (mxs_nand_mtd) >> return -EBUSY; >> >> - err = dev_get_drvdata(dev, (const void **)&type); >> - if (err) >> - type = GPMI_MXS; >> + type = (enum gpmi_type)device_get_match_data(dev); > > Should we add: > > if (!type) > type = GPMI_MXS; > > here? (enum gpmi_type)0 is already GPMI_MXS. So that's a no-op. > >> nand_info = kzalloc(sizeof(struct mxs_nand_info), GFP_KERNEL); >> if (!nand_info) { >> -- >> 2.28.0 >> >> >> _______________________________________________ >> barebox mailing list >> barebox@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/barebox >> > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
On 20-09-28 18:03, Ahmad Fatoum wrote: > > > On 9/28/20 6:01 PM, Marco Felsch wrote: > > On 20-09-28 17:42, Ahmad Fatoum wrote: > >> Probing the nand_mxs device driver on 64 bit systems invokes > >> undefined behavior, because of an errant cast. Fix this. > >> > >> No change of behavior for 32-bit SoCs intended. > >> > >> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> > >> --- > >> drivers/mtd/nand/nand_mxs.c | 4 +--- > >> 1 file changed, 1 insertion(+), 3 deletions(-) > >> > >> diff --git a/drivers/mtd/nand/nand_mxs.c b/drivers/mtd/nand/nand_mxs.c > >> index 36b6e7ac224e..e2b6e2162076 100644 > >> --- a/drivers/mtd/nand/nand_mxs.c > >> +++ b/drivers/mtd/nand/nand_mxs.c > >> @@ -2145,9 +2145,7 @@ static int mxs_nand_probe(struct device_d *dev) > >> if (mxs_nand_mtd) > >> return -EBUSY; > >> > >> - err = dev_get_drvdata(dev, (const void **)&type); > >> - if (err) > >> - type = GPMI_MXS; > >> + type = (enum gpmi_type)device_get_match_data(dev); > > > > Should we add: > > > > if (!type) > > type = GPMI_MXS; > > > > here? > > (enum gpmi_type)0 is already GPMI_MXS. So that's a no-op. I see, just wanted to be sure :) Regards, Marco > > > > >> nand_info = kzalloc(sizeof(struct mxs_nand_info), GFP_KERNEL); > >> if (!nand_info) { > >> -- > >> 2.28.0 > >> > >> > >> _______________________________________________ > >> barebox mailing list > >> barebox@lists.infradead.org > >> http://lists.infradead.org/mailman/listinfo/barebox > >> > > > > -- > Pengutronix e.K. | | > Steuerwalder Str. 21 | http://www.pengutronix.de/ | > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | > > _______________________________________________ > barebox mailing list > barebox@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/barebox > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
On Mon, Sep 28, 2020 at 05:42:41PM +0200, Ahmad Fatoum wrote: > We use dev_get_drvdata to get the driver match data associated with a > device. This has two shortcomings: > > - Linux has dev_get_drvdata too, which returns a private pointer for > driver specific info to associate with a device. We use dev->priv > (or more often container_of) for that in barebox instead > > - It nearly always involves a cast to a double pointer, which is > error-prone as size and alignment match need to be ensured > on the programmer's part and can easily be gotten wrong: > enum dev_type type > dev_get_drvdata(dev, (const void **)&type); > > Add a new function that instead of using a double pointer argument, > returns the pointer directly: > > - For normal pointer driver data, no cast is necessary > - For integer driver data casted to a pointer for storage, > the cast is still necessary, but but what? Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
On Mon, Sep 28, 2020 at 05:42:42PM +0200, Ahmad Fatoum wrote: > dev->id_entry is not populated for devices probed from the device > tree. It was used unconditionally however. Use device_get_match_data > instead to support device tree probing. > > Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> > --- > drivers/led/led-pca955x.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/led/led-pca955x.c b/drivers/led/led-pca955x.c > index 27fefce8d524..f89fcbfba5ac 100644 > --- a/drivers/led/led-pca955x.c > +++ b/drivers/led/led-pca955x.c > @@ -349,8 +349,11 @@ static int led_pca955x_probe(struct device_d *dev) > struct i2c_client *client; > int err; > struct pca955x_platform_data *pdata; > + enum pca955x_type type; > > - chip = &pca955x_chipdefs[dev->id_entry->driver_data]; > + type = (enum pca955x_type)device_get_match_data(dev); In this driver we have this: static struct pca955x_chipdef pca955x_chipdefs[] = { [pca9550] = { .bits = 2, .slv_addr = /* 110000x */ 0x60, .slv_addr_shift = 1, }, [pca9551] = { .bits = 8, .slv_addr = /* 1100xxx */ 0x60, .slv_addr_shift = 3, }, [pca9552] = { .bits = 16, .slv_addr = /* 1100xxx */ 0x60, .slv_addr_shift = 3, }, [pca9553] = { .bits = 4, .slv_addr = /* 110001x */ 0x62, .slv_addr_shift = 1, }, }; So instead of putting the enum casted to void pointer into the matchdata we could put pointers to struct pca955x_chipdef directly into the matchdata. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
On Mon, Sep 28, 2020 at 05:42:41PM +0200, Ahmad Fatoum wrote: > We use dev_get_drvdata to get the driver match data associated with a > device. This has two shortcomings: > > - Linux has dev_get_drvdata too, which returns a private pointer for > driver specific info to associate with a device. We use dev->priv > (or more often container_of) for that in barebox instead > > - It nearly always involves a cast to a double pointer, which is > error-prone as size and alignment match need to be ensured > on the programmer's part and can easily be gotten wrong: > enum dev_type type > dev_get_drvdata(dev, (const void **)&type); > > Add a new function that instead of using a double pointer argument, > returns the pointer directly: > > - For normal pointer driver data, no cast is necessary > - For integer driver data casted to a pointer for storage, > the cast is still necessary, but > > Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> > --- > drivers/base/driver.c | 11 +++++++++++ > include/driver.h | 18 ++++++++++++++++++ > 2 files changed, 29 insertions(+) > > diff --git a/drivers/base/driver.c b/drivers/base/driver.c > index 412db6c40699..3205bbc3c33b 100644 > --- a/drivers/base/driver.c > +++ b/drivers/base/driver.c > @@ -500,3 +500,14 @@ int dev_get_drvdata(struct device_d *dev, const void **data) > > return -ENODEV; > } > + > +const void *device_get_match_data(struct device_d *dev) > +{ > + if (dev->of_id_entry) > + return dev->of_id_entry->data; > + > + if (dev->id_entry) > + return (void *)dev->id_entry->driver_data; > + > + return NULL; > +} Can we please give the caller a possibility to distinguish between errors and a valid NULL pointer? As you realize in your series some drivers cast the returned value into an integer type and use 0 as a valid possibility. These need an extra explanation why we can accept that case. I can think of different possibilies to get that straight: - Put a real pointer into matchdata. This is really preferred as it motivates people to put flags into a (struct type) matchdata which doesn't lead to excessive if (type == foo || type == bar || type == baz) we sometimes see in drivers. - Return an ERR_PTR from device_get_match_data(). this is less likely interpreted as a valid int value - add a int * ret argument to device_get_match_data() which returns the error code. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
Hello Sascha, On 9/29/20 9:32 AM, Sascha Hauer wrote: > On Mon, Sep 28, 2020 at 05:42:41PM +0200, Ahmad Fatoum wrote: >> We use dev_get_drvdata to get the driver match data associated with a >> device. This has two shortcomings: >> >> - Linux has dev_get_drvdata too, which returns a private pointer for >> driver specific info to associate with a device. We use dev->priv >> (or more often container_of) for that in barebox instead >> >> - It nearly always involves a cast to a double pointer, which is >> error-prone as size and alignment match need to be ensured >> on the programmer's part and can easily be gotten wrong: >> enum dev_type type >> dev_get_drvdata(dev, (const void **)&type); >> >> Add a new function that instead of using a double pointer argument, >> returns the pointer directly: >> >> - For normal pointer driver data, no cast is necessary >> - For integer driver data casted to a pointer for storage, >> the cast is still necessary, but >> >> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> >> --- >> drivers/base/driver.c | 11 +++++++++++ >> include/driver.h | 18 ++++++++++++++++++ >> 2 files changed, 29 insertions(+) >> >> diff --git a/drivers/base/driver.c b/drivers/base/driver.c >> index 412db6c40699..3205bbc3c33b 100644 >> --- a/drivers/base/driver.c >> +++ b/drivers/base/driver.c >> @@ -500,3 +500,14 @@ int dev_get_drvdata(struct device_d *dev, const void **data) >> >> return -ENODEV; >> } >> + >> +const void *device_get_match_data(struct device_d *dev) >> +{ >> + if (dev->of_id_entry) >> + return dev->of_id_entry->data; >> + >> + if (dev->id_entry) >> + return (void *)dev->id_entry->driver_data; >> + >> + return NULL; >> +} > > Can we please give the caller a possibility to distinguish between > errors and a valid NULL pointer? The only error possible is that we matched the device by name and not of_compatible or platform id entry. If the match data is a valid pointer: -> It doesn't matter, why we have no match data either way. If the match data is a casted integer (e.g. enum): The driver author should either: -> place the default enum value as first one, so no match data => default -> should add an initial DEVICE_TYPE_UNKNOWN = 0 in the enum and handle it appropriately I like the function signature like that, I don't really see a need to adjust it. > As you realize in your series some drivers cast the returned value into > an integer type and use 0 as a valid possibility. These need an extra > explanation why we can accept that case. I can think of different > possibilies to get that straight: > > - Put a real pointer into matchdata. This is really preferred as it > motivates people to put flags into a (struct type) matchdata which > doesn't lead to excessive if (type == foo || type == bar || type == > baz) we sometimes see in drivers. We have a real pointer there already. The problem is migrating the existing drivers. > - Return an ERR_PTR from device_get_match_data(). this is less likely > interpreted as a valid int value Doesn't cover all cases. Also for the normal use, it means you need to have to check with IS_ERR_OR_NULL everywhere to be sure you don't dereference a NULL pointer. > - add a int * ret argument to device_get_match_data() which returns > the error code. Would work, but my preference is leaving it to the drivers. The drivers need to handle errors anyway. > > Sascha > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
Hi, On 9/29/20 9:19 AM, Sascha Hauer wrote: > On Mon, Sep 28, 2020 at 05:42:42PM +0200, Ahmad Fatoum wrote: >> dev->id_entry is not populated for devices probed from the device >> tree. It was used unconditionally however. Use device_get_match_data >> instead to support device tree probing. >> >> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> >> --- >> drivers/led/led-pca955x.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/led/led-pca955x.c b/drivers/led/led-pca955x.c >> index 27fefce8d524..f89fcbfba5ac 100644 >> --- a/drivers/led/led-pca955x.c >> +++ b/drivers/led/led-pca955x.c >> @@ -349,8 +349,11 @@ static int led_pca955x_probe(struct device_d *dev) >> struct i2c_client *client; >> int err; >> struct pca955x_platform_data *pdata; >> + enum pca955x_type type; >> >> - chip = &pca955x_chipdefs[dev->id_entry->driver_data]; >> + type = (enum pca955x_type)device_get_match_data(dev); > > In this driver we have this: > > static struct pca955x_chipdef pca955x_chipdefs[] = { > [pca9550] = { > .bits = 2, > .slv_addr = /* 110000x */ 0x60, > .slv_addr_shift = 1, > }, > [pca9551] = { > .bits = 8, > .slv_addr = /* 1100xxx */ 0x60, > .slv_addr_shift = 3, > }, > [pca9552] = { > .bits = 16, > .slv_addr = /* 1100xxx */ 0x60, > .slv_addr_shift = 3, > }, > [pca9553] = { > .bits = 4, > .slv_addr = /* 110001x */ 0x62, > .slv_addr_shift = 1, > }, > }; > > So instead of putting the enum casted to void pointer into the matchdata > we could put pointers to struct pca955x_chipdef directly into the > matchdata. platform id match data will have a cast as well, but casting pointer to long is probably better than pointer to enum. Will send out along with v2 when we reach agreement on the new function's signature. > > Sascha > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
On Tue, Sep 29, 2020 at 11:42:04AM +0200, Ahmad Fatoum wrote: > Hello Sascha, > > On 9/29/20 9:32 AM, Sascha Hauer wrote: > > On Mon, Sep 28, 2020 at 05:42:41PM +0200, Ahmad Fatoum wrote: > >> We use dev_get_drvdata to get the driver match data associated with a > >> device. This has two shortcomings: > >> > >> - Linux has dev_get_drvdata too, which returns a private pointer for > >> driver specific info to associate with a device. We use dev->priv > >> (or more often container_of) for that in barebox instead > >> > >> - It nearly always involves a cast to a double pointer, which is > >> error-prone as size and alignment match need to be ensured > >> on the programmer's part and can easily be gotten wrong: > >> enum dev_type type > >> dev_get_drvdata(dev, (const void **)&type); > >> > >> Add a new function that instead of using a double pointer argument, > >> returns the pointer directly: > >> > >> - For normal pointer driver data, no cast is necessary > >> - For integer driver data casted to a pointer for storage, > >> the cast is still necessary, but > >> > >> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> > >> --- > >> drivers/base/driver.c | 11 +++++++++++ > >> include/driver.h | 18 ++++++++++++++++++ > >> 2 files changed, 29 insertions(+) > >> > >> diff --git a/drivers/base/driver.c b/drivers/base/driver.c > >> index 412db6c40699..3205bbc3c33b 100644 > >> --- a/drivers/base/driver.c > >> +++ b/drivers/base/driver.c > >> @@ -500,3 +500,14 @@ int dev_get_drvdata(struct device_d *dev, const void **data) > >> > >> return -ENODEV; > >> } > >> + > >> +const void *device_get_match_data(struct device_d *dev) > >> +{ > >> + if (dev->of_id_entry) > >> + return dev->of_id_entry->data; > >> + > >> + if (dev->id_entry) > >> + return (void *)dev->id_entry->driver_data; > >> + > >> + return NULL; > >> +} > > > > Can we please give the caller a possibility to distinguish between > > errors and a valid NULL pointer? > > The only error possible is that we matched the device by name > and not of_compatible or platform id entry. > > If the match data is a valid pointer: > -> It doesn't matter, why we have no match data either way. > > If the match data is a casted integer (e.g. enum): > The driver author should either: > -> place the default enum value as first one, > so no match data => default You know that, but "The driver Author" probably doesn't. > -> should add an initial DEVICE_TYPE_UNKNOWN = 0 in the enum > and handle it appropriately > > I like the function signature like that, I don't really see > a need to adjust it. > > > As you realize in your series some drivers cast the returned value into > > an integer type and use 0 as a valid possibility. These need an extra > > explanation why we can accept that case. I can think of different > > possibilies to get that straight: > > > > - Put a real pointer into matchdata. This is really preferred as it > > motivates people to put flags into a (struct type) matchdata which > > doesn't lead to excessive if (type == foo || type == bar || type == > > baz) we sometimes see in drivers. > > We have a real pointer there already. The problem is migrating the > existing drivers. Yes, existing drivers would have to be migrated, that is exactly what I am proposing. > > > - Return an ERR_PTR from device_get_match_data(). this is less likely > > interpreted as a valid int value > > Doesn't cover all cases. Also for the normal use, it means > you need to have to check with IS_ERR_OR_NULL everywhere to > be sure you don't dereference a NULL pointer. Why that? Just don't return NULL when there's no match data, but return -ESOMETHING. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
Hi, On 9/30/20 9:48 AM, Sascha Hauer wrote: >> If the match data is a valid pointer: >> -> It doesn't matter, why we have no match data either way. >> >> If the match data is a casted integer (e.g. enum): >> The driver author should either: >> -> place the default enum value as first one, >> so no match data => default > > You know that, but "The driver Author" probably doesn't. Like with all other functions that return an error code, the author should check those errors: enum lm75_type type = (enum lm75_type)device_get_match_data(dev); if (type == 0) return -ENODEV; I'd expect they will see that 0 shouldn't be part of the enumeration. >> -> should add an initial DEVICE_TYPE_UNKNOWN = 0 in the enum >> and handle it appropriately >> >> I like the function signature like that, I don't really see >> a need to adjust it. >> >>> As you realize in your series some drivers cast the returned value into >>> an integer type and use 0 as a valid possibility. These need an extra >>> explanation why we can accept that case. I can think of different >>> possibilies to get that straight: >>> >>> - Put a real pointer into matchdata. This is really preferred as it >>> motivates people to put flags into a (struct type) matchdata which >>> doesn't lead to excessive if (type == foo || type == bar || type == >>> baz) we sometimes see in drivers. >> >> We have a real pointer there already. The problem is migrating the >> existing drivers. > > Yes, existing drivers would have to be migrated, that is exactly what I > am proposing. > >> >>> - Return an ERR_PTR from device_get_match_data(). this is less likely >>> interpreted as a valid int value >> >> Doesn't cover all cases. Also for the normal use, it means >> you need to have to check with IS_ERR_OR_NULL everywhere to >> be sure you don't dereference a NULL pointer. > > Why that? Just don't return NULL when there's no match data, but return > -ESOMETHING. For the lm75 we would have to do: enum lm75_type { TYPE_1, TYPE_2 }; const void *match = device_get_match_data(dev); if (IS_ERR(match)) return PTR_ERR(match); enum lm75_type type = (enum lm75_type)match; The alternative being: enum lm75_type { TYPE_UNKNOWN = 0, TYPE_1, TYPE_2 }; enum lm75_type type = (enum lm75_type)device_get_match_data(dev); if (type == TYPE_UNKNOWN) return -ENODEV; I prefer the second one very much. > > Sascha > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
Hello Sascha, On 9/30/20 3:13 PM, Ahmad Fatoum wrote: > Hi, > > On 9/30/20 9:48 AM, Sascha Hauer wrote: >>> If the match data is a valid pointer: >>> -> It doesn't matter, why we have no match data either way. >>> >>> If the match data is a casted integer (e.g. enum): >>> The driver author should either: >>> -> place the default enum value as first one, >>> so no match data => default >> >> You know that, but "The driver Author" probably doesn't. > > Like with all other functions that return an error code, > the author should check those errors: > > enum lm75_type type = (enum lm75_type)device_get_match_data(dev); > if (type == 0) > return -ENODEV; > > I'd expect they will see that 0 shouldn't be part of the enumeration. > >>> -> should add an initial DEVICE_TYPE_UNKNOWN = 0 in the enum >>> and handle it appropriately >>> >>> I like the function signature like that, I don't really see >>> a need to adjust it. >>> >>>> As you realize in your series some drivers cast the returned value into >>>> an integer type and use 0 as a valid possibility. These need an extra >>>> explanation why we can accept that case. I can think of different >>>> possibilies to get that straight: >>>> >>>> - Put a real pointer into matchdata. This is really preferred as it >>>> motivates people to put flags into a (struct type) matchdata which >>>> doesn't lead to excessive if (type == foo || type == bar || type == >>>> baz) we sometimes see in drivers. >>> >>> We have a real pointer there already. The problem is migrating the >>> existing drivers. >> >> Yes, existing drivers would have to be migrated, that is exactly what I >> am proposing. >> >>> >>>> - Return an ERR_PTR from device_get_match_data(). this is less likely >>>> interpreted as a valid int value >>> >>> Doesn't cover all cases. Also for the normal use, it means >>> you need to have to check with IS_ERR_OR_NULL everywhere to >>> be sure you don't dereference a NULL pointer. >> >> Why that? Just don't return NULL when there's no match data, but return >> -ESOMETHING. > > For the lm75 we would have to do: > > enum lm75_type { TYPE_1, TYPE_2 }; > > const void *match = device_get_match_data(dev); > if (IS_ERR(match)) > return PTR_ERR(match); > enum lm75_type type = (enum lm75_type)match; > > The alternative being: > > enum lm75_type { TYPE_UNKNOWN = 0, TYPE_1, TYPE_2 }; > > enum lm75_type type = (enum lm75_type)device_get_match_data(dev); > if (type == TYPE_UNKNOWN) > return -ENODEV; > > I prefer the second one very much. Gentle ping. > >> >> Sascha >> > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
On Mon, Oct 05, 2020 at 10:31:32AM +0200, Ahmad Fatoum wrote: > Hello Sascha, > > On 9/30/20 3:13 PM, Ahmad Fatoum wrote: > > Hi, > > > > On 9/30/20 9:48 AM, Sascha Hauer wrote: > >>> If the match data is a valid pointer: > >>> -> It doesn't matter, why we have no match data either way. > >>> > >>> If the match data is a casted integer (e.g. enum): > >>> The driver author should either: > >>> -> place the default enum value as first one, > >>> so no match data => default > >> > >> You know that, but "The driver Author" probably doesn't. > > > > Like with all other functions that return an error code, > > the author should check those errors: > > > > enum lm75_type type = (enum lm75_type)device_get_match_data(dev); > > if (type == 0) > > return -ENODEV; > > > > I'd expect they will see that 0 shouldn't be part of the enumeration. > > > >>> -> should add an initial DEVICE_TYPE_UNKNOWN = 0 in the enum > >>> and handle it appropriately > >>> > >>> I like the function signature like that, I don't really see > >>> a need to adjust it. > >>> > >>>> As you realize in your series some drivers cast the returned value into > >>>> an integer type and use 0 as a valid possibility. These need an extra > >>>> explanation why we can accept that case. I can think of different > >>>> possibilies to get that straight: > >>>> > >>>> - Put a real pointer into matchdata. This is really preferred as it > >>>> motivates people to put flags into a (struct type) matchdata which > >>>> doesn't lead to excessive if (type == foo || type == bar || type == > >>>> baz) we sometimes see in drivers. > >>> > >>> We have a real pointer there already. The problem is migrating the > >>> existing drivers. > >> > >> Yes, existing drivers would have to be migrated, that is exactly what I > >> am proposing. > >> > >>> > >>>> - Return an ERR_PTR from device_get_match_data(). this is less likely > >>>> interpreted as a valid int value > >>> > >>> Doesn't cover all cases. Also for the normal use, it means > >>> you need to have to check with IS_ERR_OR_NULL everywhere to > >>> be sure you don't dereference a NULL pointer. > >> > >> Why that? Just don't return NULL when there's no match data, but return > >> -ESOMETHING. > > > > For the lm75 we would have to do: > > > > enum lm75_type { TYPE_1, TYPE_2 }; > > > > const void *match = device_get_match_data(dev); > > if (IS_ERR(match)) > > return PTR_ERR(match); > > enum lm75_type type = (enum lm75_type)match; > > > > The alternative being: > > > > enum lm75_type { TYPE_UNKNOWN = 0, TYPE_1, TYPE_2 }; > > > > enum lm75_type type = (enum lm75_type)device_get_match_data(dev); > > if (type == TYPE_UNKNOWN) > > return -ENODEV; > > > > I prefer the second one very much. ok then, let's go for it. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox