| Message ID | 20180929105830.13530-2-miquel.raynal@bootlin.com |
|---|---|
| State | Accepted |
| Commit | 739def7253c4b782bcb9cc816edf3b2f2aeafb4e |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
| Series | SPI-NAND support (third batch) | expand |
On Sat, Sep 29, 2018 at 4:29 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > include/mtd.h might be included by files even if CONFIG_DM is not > enabled. In this case, the call to dev_get_uclass_priv() would trigger > a build error. Because this helper has no user, let's drop it off. Why would they use DM include file w/o using it? we need to fix in those places.
Hi Jagan, Jagan Teki <jagan@amarulasolutions.com> wrote on Mon, 1 Oct 2018 15:23:46 +0530: > On Sat, Sep 29, 2018 at 4:29 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > include/mtd.h might be included by files even if CONFIG_DM is not > > enabled. In this case, the call to dev_get_uclass_priv() would trigger > > a build error. Because this helper has no user, let's drop it off. > > Why would they use DM include file w/o using it? we need to fix in those places. There was no user of include/mtd.h not using CONFIG_DM. But now I am creating cmd/mtd.c which should work with both CONFIG_DM and no CONFIG_DM support. So include/mtd.h has reasons to be included. What should be fixed, is the possibility to include DM-related headers without breaking the compilation of several boards. But, with in mind that nothing is sealed in stone and this is easily fixable in the -rc revisions if you want a different solution, are you fine with this approach for now? Thanks, Miquèl
On Mon, Oct 1, 2018 at 3:32 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Jagan, > > Jagan Teki <jagan@amarulasolutions.com> wrote on Mon, 1 Oct 2018 > 15:23:46 +0530: > > > On Sat, Sep 29, 2018 at 4:29 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > include/mtd.h might be included by files even if CONFIG_DM is not > > > enabled. In this case, the call to dev_get_uclass_priv() would trigger > > > a build error. Because this helper has no user, let's drop it off. > > > > Why would they use DM include file w/o using it? we need to fix in those places. > > There was no user of include/mtd.h not using CONFIG_DM. > > But now I am creating cmd/mtd.c which should work with both CONFIG_DM > and no CONFIG_DM support. So include/mtd.h has reasons to be included. Then use ifdef CONFIG_MTD on the same file so it can arrange dm and non-dm include at one place.
Hi Jagan, Jagan Teki <jagan@amarulasolutions.com> wrote on Mon, 1 Oct 2018 17:08:30 +0530: > On Mon, Oct 1, 2018 at 3:32 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hi Jagan, > > > > Jagan Teki <jagan@amarulasolutions.com> wrote on Mon, 1 Oct 2018 > > 15:23:46 +0530: > > > > > On Sat, Sep 29, 2018 at 4:29 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > > > include/mtd.h might be included by files even if CONFIG_DM is not > > > > enabled. In this case, the call to dev_get_uclass_priv() would trigger > > > > a build error. Because this helper has no user, let's drop it off. > > > > > > Why would they use DM include file w/o using it? we need to fix in those places. > > > > There was no user of include/mtd.h not using CONFIG_DM. > > > > But now I am creating cmd/mtd.c which should work with both CONFIG_DM > > and no CONFIG_DM support. So include/mtd.h has reasons to be included. > > Then use ifdef CONFIG_MTD on the same file so it can arrange dm and > non-dm include at one place. The helper is not used, why do we keep it? Simon Glass and Tom Rini are trying to reduce as much as possible the number of pre-processor conditionals to have as much test coverage as they can. What you ask clearly goes into the opposite direction. Anyway, I want this work merged so I dropped the patch, enclosed the definitions from include/mtd.h into a #if defined(CONFIG_DM) block and wrote dummy helpers for them. These changes are done when include/mtd.h gets included from cmd/mtd.c which is the file that can be compiled with and without DM support, see patch cmd: mtd: add 'mtd' command I will resend the seven remaining patches. The whole branch has already been tested by Travis (it is based on your amarula/wip-spi-nand branch) so please send a PR ASAP so that this work can be finally merged (cc me so I'll be aware of it). To be sure there are not regressions I started a Travis job on this branch [1]. If there are any, I will fix them for -rc2. [1] https://travis-ci.org/miquelraynal/u-boot/builds/435575048 Thanks, Miquèl
On Monday 01 October 2018 07:10 PM, Miquel Raynal wrote: > Hi Jagan, > > Jagan Teki <jagan@amarulasolutions.com> wrote on Mon, 1 Oct 2018 > 17:08:30 +0530: > >> On Mon, Oct 1, 2018 at 3:32 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: >>> >>> Hi Jagan, >>> >>> Jagan Teki <jagan@amarulasolutions.com> wrote on Mon, 1 Oct 2018 >>> 15:23:46 +0530: >>> >>>> On Sat, Sep 29, 2018 at 4:29 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: >>>>> >>>>> include/mtd.h might be included by files even if CONFIG_DM is not >>>>> enabled. In this case, the call to dev_get_uclass_priv() would trigger >>>>> a build error. Because this helper has no user, let's drop it off. >>>> >>>> Why would they use DM include file w/o using it? we need to fix in those places. >>> >>> There was no user of include/mtd.h not using CONFIG_DM. >>> >>> But now I am creating cmd/mtd.c which should work with both CONFIG_DM >>> and no CONFIG_DM support. So include/mtd.h has reasons to be included. >> >> Then use ifdef CONFIG_MTD on the same file so it can arrange dm and >> non-dm include at one place. > > The helper is not used, why do we keep it? The whole point of placing ifdef is to support dm and non-dm at one include.
diff --git a/include/mtd.h b/include/mtd.h index 548e7f191b..9e5c807294 100644 --- a/include/mtd.h +++ b/include/mtd.h @@ -8,15 +8,4 @@ #include <linux/mtd/mtd.h> -/* - * Get mtd_info structure of the dev, which is stored as uclass private. - * - * @dev: The MTD device - * @return: pointer to mtd_info, NULL on error - */ -static inline struct mtd_info *mtd_get_info(struct udevice *dev) -{ - return dev_get_uclass_priv(dev); -} - #endif /* _MTD_H_ */
include/mtd.h might be included by files even if CONFIG_DM is not enabled. In this case, the call to dev_get_uclass_priv() would trigger a build error. Because this helper has no user, let's drop it off. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- include/mtd.h | 11 ----------- 1 file changed, 11 deletions(-)