diff mbox series

[v2,2/3] spi: dw-mmio: update dw_spi_mmio_of_match struct with thead

Message ID 20240701121355.262259-4-kanakshilledar@gmail.com (mailing list archive)
State New, archived
Headers show
Series Add basic SPI support on TH1520 | expand

Commit Message

Kanak Shilledar July 1, 2024, 12:13 p.m. UTC
updated the struct of_device_id dw_spi_mmio_of_match to include
the updated compatible value for TH1520 SoC ("thead,th1520-spi")
to initialize with dw_spi_pssi_init().

Signed-off-by: Kanak Shilledar <kanakshilledar@gmail.com>
---
Changes in v2:
- Separated from a single patch file.
---
 drivers/spi/spi-dw-mmio.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Samuel Holland July 1, 2024, 1:17 p.m. UTC | #1
Hi Kanak,

On 2024-07-01 7:13 AM, Kanak Shilledar wrote:
> updated the struct of_device_id dw_spi_mmio_of_match to include
> the updated compatible value for TH1520 SoC ("thead,th1520-spi")
> to initialize with dw_spi_pssi_init().
> 
> Signed-off-by: Kanak Shilledar <kanakshilledar@gmail.com>
> ---
> Changes in v2:
> - Separated from a single patch file.
> ---
>  drivers/spi/spi-dw-mmio.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
> index 819907e332c4..39e3d46ebf5d 100644
> --- a/drivers/spi/spi-dw-mmio.c
> +++ b/drivers/spi/spi-dw-mmio.c
> @@ -419,6 +419,7 @@ static const struct of_device_id dw_spi_mmio_of_match[] = {
>  	{ .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init},
>  	{ .compatible = "canaan,k210-spi", dw_spi_canaan_k210_init},
>  	{ .compatible = "amd,pensando-elba-spi", .data = dw_spi_elba_init},
> +	{ .compatible = "thead,th1520-spi", .data = dw_spi_pssi_init},

Your binding requires snps,dw-apb-ssi as a fallback compatible string, which is
already supported by this driver and uses the same match data. So you don't need
this patch; its only effect is to make the kernel larger.

Regards,
Samuel

>  	{ /* end of table */}
>  };
>  MODULE_DEVICE_TABLE(of, dw_spi_mmio_of_match);
Serge Semin July 1, 2024, 6:57 p.m. UTC | #2
Hi folks

On Mon, Jul 01, 2024 at 08:17:29AM -0500, Samuel Holland wrote:
> Hi Kanak,
> 
> On 2024-07-01 7:13 AM, Kanak Shilledar wrote:
> > updated the struct of_device_id dw_spi_mmio_of_match to include
> > the updated compatible value for TH1520 SoC ("thead,th1520-spi")
> > to initialize with dw_spi_pssi_init().
> > 
> > Signed-off-by: Kanak Shilledar <kanakshilledar@gmail.com>
> > ---
> > Changes in v2:
> > - Separated from a single patch file.
> > ---
> >  drivers/spi/spi-dw-mmio.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
> > index 819907e332c4..39e3d46ebf5d 100644
> > --- a/drivers/spi/spi-dw-mmio.c
> > +++ b/drivers/spi/spi-dw-mmio.c
> > @@ -419,6 +419,7 @@ static const struct of_device_id dw_spi_mmio_of_match[] = {
> >  	{ .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init},
> >  	{ .compatible = "canaan,k210-spi", dw_spi_canaan_k210_init},
> >  	{ .compatible = "amd,pensando-elba-spi", .data = dw_spi_elba_init},
> > +	{ .compatible = "thead,th1520-spi", .data = dw_spi_pssi_init},
> 
> Your binding requires snps,dw-apb-ssi as a fallback compatible string, which is
> already supported by this driver and uses the same match data. So you don't need
> this patch; its only effect is to make the kernel larger.

Agree with Samuel comment. Indeed there is no point in adding the
vendor-specific device-name supported in the driver if the fallback
compatible works as-is.

From that perspective we shouldn't have merged in the patch adding the
Renesas RZN1 SPI device name support, since the generic fallback
compatible works for it. On the contrary the Microsemi Ocelot/Jaguar2
SoC SPI DT-bindings shouldn't have been defined with the generic
fallback compatible since should the device be bound via the generic
name it won't work as expected.

Although, it's better to hear out what Rob, Conor or Krzysztof think
about this.

-Serge(y)

> 
> Regards,
> Samuel
> 
> >  	{ /* end of table */}
> >  };
> >  MODULE_DEVICE_TABLE(of, dw_spi_mmio_of_match);
>
Conor Dooley July 3, 2024, 7:28 a.m. UTC | #3
On Mon, Jul 01, 2024 at 09:57:20PM +0300, Serge Semin wrote:
> Hi folks
> 
> On Mon, Jul 01, 2024 at 08:17:29AM -0500, Samuel Holland wrote:
> > Hi Kanak,
> > 
> > On 2024-07-01 7:13 AM, Kanak Shilledar wrote:
> > > updated the struct of_device_id dw_spi_mmio_of_match to include
> > > the updated compatible value for TH1520 SoC ("thead,th1520-spi")
> > > to initialize with dw_spi_pssi_init().
> > > 
> > > Signed-off-by: Kanak Shilledar <kanakshilledar@gmail.com>
> > > ---
> > > Changes in v2:
> > > - Separated from a single patch file.
> > > ---
> > >  drivers/spi/spi-dw-mmio.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
> > > index 819907e332c4..39e3d46ebf5d 100644
> > > --- a/drivers/spi/spi-dw-mmio.c
> > > +++ b/drivers/spi/spi-dw-mmio.c
> > > @@ -419,6 +419,7 @@ static const struct of_device_id dw_spi_mmio_of_match[] = {
> > >  	{ .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init},
> > >  	{ .compatible = "canaan,k210-spi", dw_spi_canaan_k210_init},
> > >  	{ .compatible = "amd,pensando-elba-spi", .data = dw_spi_elba_init},
> > > +	{ .compatible = "thead,th1520-spi", .data = dw_spi_pssi_init},
> > 
> > Your binding requires snps,dw-apb-ssi as a fallback compatible string, which is
> > already supported by this driver and uses the same match data. So you don't need
> > this patch; its only effect is to make the kernel larger.
> 
> Agree with Samuel comment. Indeed there is no point in adding the
> vendor-specific device-name supported in the driver if the fallback
> compatible works as-is.

FWIW, Mark picked up the binding alone so I think there's nothing for
Kanak to do here & the driver patch should just be forgotten about :)

> >From that perspective we shouldn't have merged in the patch adding the
> Renesas RZN1 SPI device name support, since the generic fallback
> compatible works for it. On the contrary the Microsemi Ocelot/Jaguar2
> SoC SPI DT-bindings shouldn't have been defined with the generic
> fallback compatible since should the device be bound via the generic
> name it won't work as expected.
> 
> Although, it's better to hear out what Rob, Conor or Krzysztof think
> about this.

I agree with what you've written. If the fallback works identically, then
the specific compatible shouldn't be added here. And if the fallback
will cause the device to misbehave (or not behave at all), then it
should not have been added.
I'm not sure if the Microsemi stuff is in the "won't work {,properly}"
camp or in the "will work in a limited fashion" camp. The latter would
be suitable for a fallback, the former not.

Cheers,
Conor.
Kanak Shilledar July 3, 2024, 1:12 p.m. UTC | #4
Hi,
So, I will drop this patch.
In the next version (i.e. v2) of this patchset, do I need to include
the dt-binding patch as it is already in for-next.
I am waiting for comments on the devicetree files before sending the
v2 (if required).

Thanks and Regards,
Kanak Shilledar

On Wed, Jul 3, 2024 at 12:59 PM Conor Dooley <conor.dooley@microchip.com> wrote:
>
> On Mon, Jul 01, 2024 at 09:57:20PM +0300, Serge Semin wrote:
> > Hi folks
> >
> > On Mon, Jul 01, 2024 at 08:17:29AM -0500, Samuel Holland wrote:
> > > Hi Kanak,
> > >
> > > On 2024-07-01 7:13 AM, Kanak Shilledar wrote:
> > > > updated the struct of_device_id dw_spi_mmio_of_match to include
> > > > the updated compatible value for TH1520 SoC ("thead,th1520-spi")
> > > > to initialize with dw_spi_pssi_init().
> > > >
> > > > Signed-off-by: Kanak Shilledar <kanakshilledar@gmail.com>
> > > > ---
> > > > Changes in v2:
> > > > - Separated from a single patch file.
> > > > ---
> > > >  drivers/spi/spi-dw-mmio.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
> > > > index 819907e332c4..39e3d46ebf5d 100644
> > > > --- a/drivers/spi/spi-dw-mmio.c
> > > > +++ b/drivers/spi/spi-dw-mmio.c
> > > > @@ -419,6 +419,7 @@ static const struct of_device_id dw_spi_mmio_of_match[] = {
> > > >   { .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init},
> > > >   { .compatible = "canaan,k210-spi", dw_spi_canaan_k210_init},
> > > >   { .compatible = "amd,pensando-elba-spi", .data = dw_spi_elba_init},
> > > > + { .compatible = "thead,th1520-spi", .data = dw_spi_pssi_init},
> > >
> > > Your binding requires snps,dw-apb-ssi as a fallback compatible string, which is
> > > already supported by this driver and uses the same match data. So you don't need
> > > this patch; its only effect is to make the kernel larger.
> >
> > Agree with Samuel comment. Indeed there is no point in adding the
> > vendor-specific device-name supported in the driver if the fallback
> > compatible works as-is.
>
> FWIW, Mark picked up the binding alone so I think there's nothing for
> Kanak to do here & the driver patch should just be forgotten about :)
>
> > >From that perspective we shouldn't have merged in the patch adding the
> > Renesas RZN1 SPI device name support, since the generic fallback
> > compatible works for it. On the contrary the Microsemi Ocelot/Jaguar2
> > SoC SPI DT-bindings shouldn't have been defined with the generic
> > fallback compatible since should the device be bound via the generic
> > name it won't work as expected.
> >
> > Although, it's better to hear out what Rob, Conor or Krzysztof think
> > about this.
>
> I agree with what you've written. If the fallback works identically, then
> the specific compatible shouldn't be added here. And if the fallback
> will cause the device to misbehave (or not behave at all), then it
> should not have been added.
> I'm not sure if the Microsemi stuff is in the "won't work {,properly}"
> camp or in the "will work in a limited fashion" camp. The latter would
> be suitable for a fallback, the former not.
>
> Cheers,
> Conor.
>
Conor Dooley July 3, 2024, 2:27 p.m. UTC | #5
On Wed, Jul 03, 2024 at 06:42:46PM +0530, Kanak Shilledar wrote:
> Hi,
> So, I will drop this patch.
> In the next version (i.e. v2) of this patchset, do I need to include
> the dt-binding patch as it is already in for-next.

No, you do not need to include the binding.

> I am waiting for comments on the devicetree files before sending the
> v2 (if required).

I'll try to look at that today, not super sure if I wanna pick up more
patches for that platform with "fixed-clock"s, but I'll comment that on
the dts patch itself.

Cheers,
Conor.
Kanak Shilledar July 3, 2024, 2:37 p.m. UTC | #6
On Wed, Jul 3, 2024 at 7:57 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Wed, Jul 03, 2024 at 06:42:46PM +0530, Kanak Shilledar wrote:
> > Hi,
> > So, I will drop this patch.
> > In the next version (i.e. v2) of this patchset, do I need to include
> > the dt-binding patch as it is already in for-next.
>
> No, you do not need to include the binding.

Alright thanks for the clarification!

> > I am waiting for comments on the devicetree files before sending the
> > v2 (if required).
>
> I'll try to look at that today, not super sure if I wanna pick up more
> patches for that platform with "fixed-clock"s, but I'll comment that on
> the dts patch itself.
>
> Cheers,
> Conor.

Cheers,
Kanak Shilledar
diff mbox series

Patch

diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
index 819907e332c4..39e3d46ebf5d 100644
--- a/drivers/spi/spi-dw-mmio.c
+++ b/drivers/spi/spi-dw-mmio.c
@@ -419,6 +419,7 @@  static const struct of_device_id dw_spi_mmio_of_match[] = {
 	{ .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init},
 	{ .compatible = "canaan,k210-spi", dw_spi_canaan_k210_init},
 	{ .compatible = "amd,pensando-elba-spi", .data = dw_spi_elba_init},
+	{ .compatible = "thead,th1520-spi", .data = dw_spi_pssi_init},
 	{ /* end of table */}
 };
 MODULE_DEVICE_TABLE(of, dw_spi_mmio_of_match);