| Message ID | 20240206085238.1208256-2-tudor.ambarus@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spi: s3c64xx: add support for google,gs101-spi | expand |
On Tue, Feb 6, 2024 at 2:52 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > The driver uses u32 and relies on an implicit inclusion of > <linux/types.h>. > > It is good practice to directly include all headers used, it avoids > implicit dependencies and spurious breakage if someone rearranges > headers and causes the implicit include to vanish. > > Include the missing header. > > Fixes: 230d42d422e7 ("spi: Add s3c64xx SPI Controller driver") Not sure if Fixes tag is needed here. > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > --- > drivers/spi/spi-s3c64xx.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c > index 72c35dbe53b2..c15ca6a910dc 100644 > --- a/drivers/spi/spi-s3c64xx.c > +++ b/drivers/spi/spi-s3c64xx.c > @@ -17,6 +17,7 @@ > #include <linux/platform_device.h> > #include <linux/pm_runtime.h> > #include <linux/spi/spi.h> > +#include <linux/types.h> Is this really needed for the further patches in this series? > > #define MAX_SPI_PORTS 12 > #define S3C64XX_SPI_QUIRK_CS_AUTO (1 << 1) > -- > 2.43.0.594.gd9cf4e227d-goog >
On 2/6/24 18:02, Sam Protsenko wrote: > On Tue, Feb 6, 2024 at 2:52 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: >> >> The driver uses u32 and relies on an implicit inclusion of >> <linux/types.h>. >> >> It is good practice to directly include all headers used, it avoids >> implicit dependencies and spurious breakage if someone rearranges >> headers and causes the implicit include to vanish. >> >> Include the missing header. >> >> Fixes: 230d42d422e7 ("spi: Add s3c64xx SPI Controller driver") > > Not sure if Fixes tag is needed here. We have already talked about this. If a patch that causes the implicit include to vanish is backported to stable, it will reveal the missing header here and will result in backporting this patch as well. So, as a good practice let's allow this patch to be queued to stable, it will avoid possible compilation errors. I guess Mark has to break the tie again. Or Greg if he cares, I added him in To:. > >> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> >> --- >> drivers/spi/spi-s3c64xx.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c >> index 72c35dbe53b2..c15ca6a910dc 100644 >> --- a/drivers/spi/spi-s3c64xx.c >> +++ b/drivers/spi/spi-s3c64xx.c >> @@ -17,6 +17,7 @@ >> #include <linux/platform_device.h> >> #include <linux/pm_runtime.h> >> #include <linux/spi/spi.h> >> +#include <linux/types.h> > > Is this really needed for the further patches in this series? > Yes, because in patch 3/4 I use u8 and u16 and I don't want to use those without having the header included. Do you find this wrong? >> >> #define MAX_SPI_PORTS 12 >> #define S3C64XX_SPI_QUIRK_CS_AUTO (1 << 1) >> -- >> 2.43.0.594.gd9cf4e227d-goog >>
On Wed, Feb 07, 2024 at 06:20:56AM +0000, Tudor Ambarus wrote: > > > On 2/6/24 18:02, Sam Protsenko wrote: > > On Tue, Feb 6, 2024 at 2:52 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > >> > >> The driver uses u32 and relies on an implicit inclusion of > >> <linux/types.h>. > >> > >> It is good practice to directly include all headers used, it avoids > >> implicit dependencies and spurious breakage if someone rearranges > >> headers and causes the implicit include to vanish. > >> > >> Include the missing header. > >> > >> Fixes: 230d42d422e7 ("spi: Add s3c64xx SPI Controller driver") > > > > Not sure if Fixes tag is needed here. > > We have already talked about this. If a patch that causes the implicit > include to vanish is backported to stable, it will reveal the missing > header here and will result in backporting this patch as well. So worry about this then, not now. > So, as a > good practice let's allow this patch to be queued to stable, it will > avoid possible compilation errors. "possible" does not mean "needed". Please only tag stuff that you know is needed, if the stable developers break things, they can fix it up when it happens. Adding .h includes is not a fixes: thing unless it actually fixes something in Linus's tree, otherwise it's an abuse of the tag. Please don't do that. thanks, greg k-h
On 2/7/24 09:58, Greg Kroah-Hartman wrote: > On Wed, Feb 07, 2024 at 06:20:56AM +0000, Tudor Ambarus wrote: >> >> >> On 2/6/24 18:02, Sam Protsenko wrote: >>> On Tue, Feb 6, 2024 at 2:52 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: >>>> >>>> The driver uses u32 and relies on an implicit inclusion of >>>> <linux/types.h>. >>>> >>>> It is good practice to directly include all headers used, it avoids >>>> implicit dependencies and spurious breakage if someone rearranges >>>> headers and causes the implicit include to vanish. >>>> >>>> Include the missing header. >>>> >>>> Fixes: 230d42d422e7 ("spi: Add s3c64xx SPI Controller driver") >>> >>> Not sure if Fixes tag is needed here. >> >> We have already talked about this. If a patch that causes the implicit >> include to vanish is backported to stable, it will reveal the missing >> header here and will result in backporting this patch as well. > > So worry about this then, not now. > >> So, as a >> good practice let's allow this patch to be queued to stable, it will >> avoid possible compilation errors. > > "possible" does not mean "needed". > > Please only tag stuff that you know is needed, if the stable developers > break things, they can fix it up when it happens. > > Adding .h includes is not a fixes: thing unless it actually fixes > something in Linus's tree, otherwise it's an abuse of the tag. Please > don't do that. > Okay, thanks for the clarification. Cheers, ta
On Wed, Feb 7, 2024 at 12:21 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > > > On 2/6/24 18:02, Sam Protsenko wrote: > > On Tue, Feb 6, 2024 at 2:52 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > >> > >> The driver uses u32 and relies on an implicit inclusion of > >> <linux/types.h>. > >> > >> It is good practice to directly include all headers used, it avoids > >> implicit dependencies and spurious breakage if someone rearranges > >> headers and causes the implicit include to vanish. > >> > >> Include the missing header. > >> > >> Fixes: 230d42d422e7 ("spi: Add s3c64xx SPI Controller driver") > > > > Not sure if Fixes tag is needed here. > > We have already talked about this. If a patch that causes the implicit > include to vanish is backported to stable, it will reveal the missing > header here and will result in backporting this patch as well. So, as a > good practice let's allow this patch to be queued to stable, it will > avoid possible compilation errors. > > I guess Mark has to break the tie again. Or Greg if he cares, I added > him in To:. > > > > >> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > >> --- > >> drivers/spi/spi-s3c64xx.c | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c > >> index 72c35dbe53b2..c15ca6a910dc 100644 > >> --- a/drivers/spi/spi-s3c64xx.c > >> +++ b/drivers/spi/spi-s3c64xx.c > >> @@ -17,6 +17,7 @@ > >> #include <linux/platform_device.h> > >> #include <linux/pm_runtime.h> > >> #include <linux/spi/spi.h> > >> +#include <linux/types.h> > > > > Is this really needed for the further patches in this series? > > > > Yes, because in patch 3/4 I use u8 and u16 and I don't want to use those > without having the header included. Do you find this wrong? > I'd say if this header is really needed for your patch 3/4 to have a successful build, just merge this patch into the patch 3/4 then. > >> > >> #define MAX_SPI_PORTS 12 > >> #define S3C64XX_SPI_QUIRK_CS_AUTO (1 << 1) > >> -- > >> 2.43.0.594.gd9cf4e227d-goog > >>
On Wed, Feb 07, 2024 at 09:42:20AM -0600, Sam Protsenko wrote: > On Wed, Feb 7, 2024 at 12:21 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > Yes, because in patch 3/4 I use u8 and u16 and I don't want to use those > > without having the header included. Do you find this wrong? > I'd say if this header is really needed for your patch 3/4 to have a > successful build, just merge this patch into the patch 3/4 then. The series was already resent, not worth a new version just for this...
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c index 72c35dbe53b2..c15ca6a910dc 100644 --- a/drivers/spi/spi-s3c64xx.c +++ b/drivers/spi/spi-s3c64xx.c @@ -17,6 +17,7 @@ #include <linux/platform_device.h> #include <linux/pm_runtime.h> #include <linux/spi/spi.h> +#include <linux/types.h> #define MAX_SPI_PORTS 12 #define S3C64XX_SPI_QUIRK_CS_AUTO (1 << 1)
The driver uses u32 and relies on an implicit inclusion of <linux/types.h>. It is good practice to directly include all headers used, it avoids implicit dependencies and spurious breakage if someone rearranges headers and causes the implicit include to vanish. Include the missing header. Fixes: 230d42d422e7 ("spi: Add s3c64xx SPI Controller driver") Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> --- drivers/spi/spi-s3c64xx.c | 1 + 1 file changed, 1 insertion(+)