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

[Kanak Shilledar <kanakshilledar@xxxxxxxxx>]

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@xxxxxxxxxxxxx> 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@xxxxxxxxx>
> > > > ---
> > > > 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.
>