mbox series

[0/4] spi: s3c64xx: add support for google,gs101-spi

Message ID 20240206085238.1208256-1-tudor.ambarus@linaro.org
Headers show
Series spi: s3c64xx: add support for google,gs101-spi | expand

Message

Tudor Ambarus Feb. 6, 2024, 8:52 a.m. UTC
Depends on the simple cleanup patches from:
https://lore.kernel.org/linux-spi/20240205124513.447875-1-tudor.ambarus@linaro.org/

A slightly different version of the google,gs101-spi support was sent at:
https://lore.kernel.org/linux-spi/20240125145007.748295-1-tudor.ambarus@linaro.org/

Let's add support for gs101-spi so that I have a testing base for the
driver rework patches that will follow.

Tudor Ambarus (4):
  spi: s3c64xx: explicitly include <linux/types.h>
  spi: dt-bindings: samsung: add google,gs101-spi compatible
  spi: s3c64xx: add s3c64xx_iowrite{8,16}_32_rep accessors
  spi: s3c64xx: add support for google,gs101-spi

 .../devicetree/bindings/spi/samsung,spi.yaml  |  1 +
 drivers/spi/spi-s3c64xx.c                     | 89 +++++++++++++++----
 2 files changed, 75 insertions(+), 15 deletions(-)

Comments

Peter Griffin Feb. 6, 2024, 9:56 a.m. UTC | #1
On Tue, 6 Feb 2024 at 08:52, 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")
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---

Reviewed-by: Peter Griffin <peter.griffin@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>
>
>  #define MAX_SPI_PORTS          12
>  #define S3C64XX_SPI_QUIRK_CS_AUTO      (1 << 1)
> --
> 2.43.0.594.gd9cf4e227d-goog
>
Sam Protsenko Feb. 6, 2024, 6:02 p.m. UTC | #2
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
>
Sam Protsenko Feb. 6, 2024, 6:59 p.m. UTC | #3
On Tue, Feb 6, 2024 at 2:52 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Depends on the simple cleanup patches from:
> https://lore.kernel.org/linux-spi/20240205124513.447875-1-tudor.ambarus@linaro.org/
>
> A slightly different version of the google,gs101-spi support was sent at:
> https://lore.kernel.org/linux-spi/20240125145007.748295-1-tudor.ambarus@linaro.org/
>
> Let's add support for gs101-spi so that I have a testing base for the
> driver rework patches that will follow.
>
> Tudor Ambarus (4):
>   spi: s3c64xx: explicitly include <linux/types.h>
>   spi: dt-bindings: samsung: add google,gs101-spi compatible
>   spi: s3c64xx: add s3c64xx_iowrite{8,16}_32_rep accessors
>   spi: s3c64xx: add support for google,gs101-spi
>

Just a grumpy note: I wish this series (except for the [PATCH 1/4],
which I'd argue doesn't belong here) was submitted before the rest of
SPI cleanups and reworkings. Would've made reviewing much easier,
because this series doesn't apply without SPI cleanup series that has
to be applied prior to that. There are other benefits to that approach
too, as was discussed earlier.

>  .../devicetree/bindings/spi/samsung,spi.yaml  |  1 +
>  drivers/spi/spi-s3c64xx.c                     | 89 +++++++++++++++----
>  2 files changed, 75 insertions(+), 15 deletions(-)
>
> --
> 2.43.0.594.gd9cf4e227d-goog
>
Tudor Ambarus Feb. 7, 2024, 6:20 a.m. UTC | #4
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
>>
Tudor Ambarus Feb. 7, 2024, 7:50 a.m. UTC | #5
On 2/6/24 18:59, Sam Protsenko wrote:
> On Tue, Feb 6, 2024 at 2:52 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>
>> Depends on the simple cleanup patches from:
>> https://lore.kernel.org/linux-spi/20240205124513.447875-1-tudor.ambarus@linaro.org/
>>
>> A slightly different version of the google,gs101-spi support was sent at:
>> https://lore.kernel.org/linux-spi/20240125145007.748295-1-tudor.ambarus@linaro.org/
>>
>> Let's add support for gs101-spi so that I have a testing base for the
>> driver rework patches that will follow.
>>
>> Tudor Ambarus (4):
>>   spi: s3c64xx: explicitly include <linux/types.h>
>>   spi: dt-bindings: samsung: add google,gs101-spi compatible
>>   spi: s3c64xx: add s3c64xx_iowrite{8,16}_32_rep accessors
>>   spi: s3c64xx: add support for google,gs101-spi
>>
> 
> Just a grumpy note: I wish this series (except for the [PATCH 1/4],
> which I'd argue doesn't belong here) was submitted before the rest of
> SPI cleanups and reworkings. Would've made reviewing much easier,
> because this series doesn't apply without SPI cleanup series that has
> to be applied prior to that. There are other benefits to that approach
> too, as was discussed earlier.
> 

I feel we're bike-shedding, it drains my energy. Your reasons were:
1/ easier review
2/ easier backporting of gs101 if that's ever wanted
3/ driver rework takes more review time and I risk not having gs101
integrated for next release

2/ is not true right now, I could cherry-pick the iowrite and gs101
patches on top of v6.7. With 1/ I don't agree as the gs101 patches are
the same with or without the simple cleanup.
3/ is my responsibility and I'm ok with it, I feel there's enough time
for all

What matters, as I specified in the cover letter, is to have the gs101
patches before the functional driver rework which will follow, so that I
can test each functional patch with gs101.

I give up however, I'll do as you want. Will respin all.
Greg Kroah-Hartman Feb. 7, 2024, 9:58 a.m. UTC | #6
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
Tudor Ambarus Feb. 7, 2024, 10:14 a.m. UTC | #7
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
Sam Protsenko Feb. 7, 2024, 3:42 p.m. UTC | #8
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
> >>
Mark Brown Feb. 7, 2024, 4:07 p.m. UTC | #9
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...