| Message ID | 20240205191808.998754-1-frut3k7@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] spi: spidev: Add Qualcomm spidev device compatible | expand |
On Mon, Feb 05, 2024 at 08:18:05PM +0100, Paweł Owoc wrote: > Add compatible string for Qualcomm spidev device (used for QCA4024). > --- a/drivers/spi/spidev.c > +++ b/drivers/spi/spidev.c > @@ -710,6 +710,7 @@ static const struct spi_device_id spidev_spi_ids[] = { > { .name = "spi-authenta" }, > { .name = "em3581" }, > { .name = "si3210" }, > + { .name = "spidev" }, > {}, Why? > { .compatible = "lwn,bk4", .data = &spidev_of_check }, > { .compatible = "menlo,m53cpld", .data = &spidev_of_check }, > { .compatible = "micron,spi-authenta", .data = &spidev_of_check }, > + { .compatible = "qca,spidev", .data = &spidev_of_check }, No, this needs to correspond to the hardware being controlled via spidev not to an implementation detail. Any new compatibles also need to be documented. I'm also missing patch 2 of this series so don't know what's going on there.
On Tue, Feb 06, 2024 at 02:01:27PM +0100, frut3k7 wrote: > On Tue, Feb 6, 2024 at 10:37 AM Mark Brown <broonie@kernel.org> wrote: > > On Mon, Feb 05, 2024 at 08:18:05PM +0100, Paweł Owoc wrote: > > > + { .compatible = "qca,spidev", .data = &spidev_of_check }, > > No, this needs to correspond to the hardware being controlled via spidev > > not to an implementation detail. Any new compatibles also need to be > > documented. > The device for which I want to add compatibility is originally used in the > router and this is what the dts fragment looks like: > > spi@3 { > compatible = "qca,spidev"; > reg = <0x00>; > spi-max-frequency = <0x16e3600>; > }; > }; > According to this commit, Qualcomm use this compatibility: > https://github.com/dissent1/msm-2/commit/d6160218393552fea1b7973787f2bd154f870ee2 This is out of tree, it's not exactly a good guide for mainline. The DT should describe the hardware, not how some particular software stack chooses to drive it. > > I'm also missing patch 2 of this series so don't know what's going on > > there. > The second patch was sent only to the devicetree bindings project: > https://lore.kernel.org/all/20240205191828.998783-1-frut3k7@gmail.com/ > It's probably done wrong... You should send the bindings change along with the driver change, they usually get merged together.
On Tue, Feb 6, 2024 at 10:37 AM Mark Brown <broonie@kernel.org> wrote: > > On Mon, Feb 05, 2024 at 08:18:05PM +0100, Paweł Owoc wrote: > > Add compatible string for Qualcomm spidev device (used for QCA4024). > > > --- a/drivers/spi/spidev.c > > +++ b/drivers/spi/spidev.c > > @@ -710,6 +710,7 @@ static const struct spi_device_id spidev_spi_ids[] = { > > { .name = "spi-authenta" }, > > { .name = "em3581" }, > > { .name = "si3210" }, > > + { .name = "spidev" }, > > {}, > > Why? > > > { .compatible = "lwn,bk4", .data = &spidev_of_check }, > > { .compatible = "menlo,m53cpld", .data = &spidev_of_check }, > > { .compatible = "micron,spi-authenta", .data = &spidev_of_check }, > > + { .compatible = "qca,spidev", .data = &spidev_of_check }, > > No, this needs to correspond to the hardware being controlled via spidev > not to an implementation detail. Any new compatibles also need to be > documented. The device for which I want to add compatibility is originally used in the router and this is what the dts fragment looks like: spi@78b8000 { #address-cells = <0x01>; #size-cells = <0x00>; clock-names = "core\0iface"; clocks = <0x03 0x5c 0x03 0x54>; compatible = "qcom,spi-qup-v2.2.1"; cs-select = <0x02>; dma-names = "tx\0rx"; dmas = <0x06 0x12 0x06 0x13>; interrupts = <0x00 0x62 0x00>; pinctrl-0 = <0x08>; pinctrl-names = "default"; quartz-reset-gpio = <0x09 0x15 0x01>; reg = <0x78b8000 0x600>; spi-max-frequency = <0x2faf080>; status = "ok"; spi@3 { compatible = "qca,spidev"; reg = <0x00>; spi-max-frequency = <0x16e3600>; }; }; The first part has already been added: https://lore.kernel.org/all/20231123121324.1046164-1-robimarko@gmail.com/ According to this commit, Qualcomm use this compatibility: https://github.com/dissent1/msm-2/commit/d6160218393552fea1b7973787f2bd154f870ee2 > > I'm also missing patch 2 of this series so don't know what's going on > there. The second patch was sent only to the devicetree bindings project: https://lore.kernel.org/all/20240205191828.998783-1-frut3k7@gmail.com/ It's probably done wrong...
On Tue, Feb 6, 2024 at 2:11 PM Mark Brown <broonie@kernel.org> wrote: > > On Tue, Feb 06, 2024 at 02:01:27PM +0100, frut3k7 wrote: > > On Tue, Feb 6, 2024 at 10:37 AM Mark Brown <broonie@kernel.org> wrote: > > > On Mon, Feb 05, 2024 at 08:18:05PM +0100, Paweł Owoc wrote: > > > > > + { .compatible = "qca,spidev", .data = &spidev_of_check }, > > > > No, this needs to correspond to the hardware being controlled via spidev > > > not to an implementation detail. Any new compatibles also need to be > > > documented. > > > The device for which I want to add compatibility is originally used in the > > router and this is what the dts fragment looks like: > > > > > spi@3 { > > compatible = "qca,spidev"; > > reg = <0x00>; > > spi-max-frequency = <0x16e3600>; > > }; > > }; > > > According to this commit, Qualcomm use this compatibility: > > https://github.com/dissent1/msm-2/commit/d6160218393552fea1b7973787f2bd154f870ee2 > > This is out of tree, it's not exactly a good guide for mainline. The DT > should describe the hardware, not how some particular software stack > chooses to drive it. > Will changing from "spidev" to "qca4024" be enough? > > > I'm also missing patch 2 of this series so don't know what's going on > > > there. > > > The second patch was sent only to the devicetree bindings project: > > https://lore.kernel.org/all/20240205191828.998783-1-frut3k7@gmail.com/ > > It's probably done wrong... > > You should send the bindings change along with the driver change, they > usually get merged together.
On Tue, Feb 06, 2024 at 02:57:49PM +0100, frut3k7 wrote: > On Tue, Feb 6, 2024 at 2:11 PM Mark Brown <broonie@kernel.org> wrote: > > This is out of tree, it's not exactly a good guide for mainline. The DT > > should describe the hardware, not how some particular software stack > > chooses to drive it. > Will changing from "spidev" to "qca4024" be enough? Should be I think.
On Tue, Feb 6, 2024 at 3:10 PM Mark Brown <broonie@kernel.org> wrote: > > On Tue, Feb 06, 2024 at 02:57:49PM +0100, frut3k7 wrote: > > On Tue, Feb 6, 2024 at 2:11 PM Mark Brown <broonie@kernel.org> wrote: > > > > This is out of tree, it's not exactly a good guide for mainline. The DT > > > should describe the hardware, not how some particular software stack > > > chooses to drive it. > > > Will changing from "spidev" to "qca4024" be enough? > > Should be I think. Should both patches (spi and devicetree) be sent to two projects (Linux SPI and Devicetree Bindings)?
On Tue, Feb 06, 2024 at 05:48:04PM +0100, frut3k7 wrote: > Should both patches (spi and devicetree) be sent to two projects > (Linux SPI and Devicetree Bindings)? Yes.
diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c index 655f2c959cd4..00bcb77ee597 100644 --- a/drivers/spi/spidev.c +++ b/drivers/spi/spidev.c @@ -710,6 +710,7 @@ static const struct spi_device_id spidev_spi_ids[] = { { .name = "spi-authenta" }, { .name = "em3581" }, { .name = "si3210" }, + { .name = "spidev" }, {}, }; MODULE_DEVICE_TABLE(spi, spidev_spi_ids); @@ -734,6 +735,7 @@ static const struct of_device_id spidev_dt_ids[] = { { .compatible = "lwn,bk4", .data = &spidev_of_check }, { .compatible = "menlo,m53cpld", .data = &spidev_of_check }, { .compatible = "micron,spi-authenta", .data = &spidev_of_check }, + { .compatible = "qca,spidev", .data = &spidev_of_check }, { .compatible = "rohm,dh2228fv", .data = &spidev_of_check }, { .compatible = "semtech,sx1301", .data = &spidev_of_check }, { .compatible = "silabs,em3581", .data = &spidev_of_check },
Add compatible string for Qualcomm spidev device (used for QCA4024). Signed-off-by: Paweł Owoc <frut3k7@gmail.com> --- drivers/spi/spidev.c | 2 ++ 1 file changed, 2 insertions(+)