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

[Conor Dooley <conor.dooley@microchip.com>]

[-- Attachment #1: Type: text/plain, Size: 2728 bytes --]

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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]