| Message ID | 20240326181027.1418989-1-andriy.shevchenko@linux.intel.com |
|---|---|
| Headers | show |
| Series | spi: pxa2xx: Drop linux/spi/pxa2xx_spi.h | expand |
On Tue, Mar 26, 2024 at 08:07:57PM +0200, Andy Shevchenko wrote: > Since driver can parse num-cs device property, replace platform data > with this new approach. But why? > -static struct pxa2xx_spi_controller spitz_spi_info = { > - .num_chipselect = 3, > -}; > +static const struct property_entry spitz_spi_properties[] = { > + PROPERTY_ENTRY_U32("num-cs", 3), > + { } > +}; This is just platform data with less validation AFAICT.
On Tue, Mar 26, 2024 at 06:21:48PM +0000, Mark Brown wrote: > On Tue, Mar 26, 2024 at 08:07:57PM +0200, Andy Shevchenko wrote: > > > Since driver can parse num-cs device property, replace platform data > > with this new approach. > > But why? To be able to hide the header's contents from public. Should I update the commit message? ... > > +static const struct property_entry spitz_spi_properties[] = { > > + PROPERTY_ENTRY_U32("num-cs", 3), > > + { } > > +}; > > This is just platform data with less validation AFAICT. I'm not sure what validation you are expecting here. It should be done via DT schema ideally when the platform gets converted to DT. This change is an interim to that (at least it makes kernel side better). After the platform code may be gone completely or converted. If the latter happens, we got the validation back. In any case it's not worse than plain DT property handling in the kernel. The validation in that case is done elsewhere. Since the property is defined in board files the assumed validation is done during development/review stages. But OTOH for the legacy code we need not to touch the property provider more than once. We are _not_ expecting this to be spread.
On Tue, Mar 26, 2024 at 08:50:04PM +0200, Andy Shevchenko wrote: > On Tue, Mar 26, 2024 at 06:21:48PM +0000, Mark Brown wrote: > > On Tue, Mar 26, 2024 at 08:07:57PM +0200, Andy Shevchenko wrote: > > > Since driver can parse num-cs device property, replace platform data > > > with this new approach. > > But why? > To be able to hide the header's contents from public. > Should I update the commit message? That would definitely help, but it's hard to see what the actual benefit is here. It's removing platform data without doing the more difficult bit where the platform gets converted to DT. > > > +static const struct property_entry spitz_spi_properties[] = { > > > + PROPERTY_ENTRY_U32("num-cs", 3), > > > + { } > > > +}; > > This is just platform data with less validation AFAICT. > I'm not sure what validation you are expecting here. It should be done via Well, the problem with swnode is that there's no validation to expect - it's an inherent problem with swnode. > DT schema ideally when the platform gets converted to DT. This change is > an interim to that (at least it makes kernel side better). After the platform > code may be gone completely or converted. If the latter happens, we got > the validation back. It is not clear to me that this makes the kernel side better, it just seems to be rewriting the platform data for the sake of it. If it was converting to DT there'd be some stuff from it being DT but this keeps everything as in kernel as board files, just in a more complex form. > In any case it's not worse than plain DT property handling in the kernel. > The validation in that case is done elsewhere. Since the property is defined > in board files the assumed validation is done during development/review > stages. But OTOH for the legacy code we need not to touch the property > provider more than once. We are _not_ expecting this to be spread. I'm guessing you're just checking this by inspection though...
On Tue, Mar 26, 2024 at 08:02:57PM +0000, Mark Brown wrote: > On Tue, Mar 26, 2024 at 08:50:04PM +0200, Andy Shevchenko wrote: > > On Tue, Mar 26, 2024 at 06:21:48PM +0000, Mark Brown wrote: > > > On Tue, Mar 26, 2024 at 08:07:57PM +0200, Andy Shevchenko wrote: > > > > > Since driver can parse num-cs device property, replace platform data > > > > with this new approach. > > > > But why? > > > To be able to hide the header's contents from public. > > Should I update the commit message? > > That would definitely help, but it's hard to see what the actual benefit > is here. It's removing platform data without doing the more difficult > bit where the platform gets converted to DT. Will do in v2. > > > > +static const struct property_entry spitz_spi_properties[] = { > > > > + PROPERTY_ENTRY_U32("num-cs", 3), > > > > + { } > > > > +}; > > > > This is just platform data with less validation AFAICT. > > > I'm not sure what validation you are expecting here. It should be done via > > Well, the problem with swnode is that there's no validation to expect - > it's an inherent problem with swnode. I do not object this. > > DT schema ideally when the platform gets converted to DT. This change is > > an interim to that (at least it makes kernel side better). After the platform > > code may be gone completely or converted. If the latter happens, we got > > the validation back. > > It is not clear to me that this makes the kernel side better, it just > seems to be rewriting the platform data for the sake of it. If it was > converting to DT there'd be some stuff from it being DT but this keeps > everything as in kernel as board files, just in a more complex form. Not really. The benefits with swnode conversion are the following: - reducing custom APIs / data types between _shared_ (in a sense of supporting zillion different platforms) driver and a certain board file - as an effect of the above, reducing kernel code base, and as the result make maintenance easier and bug-free for that parts - preparing a driver to be ready for any old board file conversion to DT as it reduces that churn (you won't need to touch the driver code) - ...anything else I forgot to mention... > > In any case it's not worse than plain DT property handling in the kernel. > > The validation in that case is done elsewhere. Since the property is defined > > in board files the assumed validation is done during development/review > > stages. But OTOH for the legacy code we need not to touch the property > > provider more than once. We are _not_ expecting this to be spread. > > I'm guessing you're just checking this by inspection though... Yes, we seems do not have any tool to perform a such against software nodes.
On Tue, Mar 26, 2024 at 10:12:12PM +0200, Andy Shevchenko wrote: > On Tue, Mar 26, 2024 at 08:02:57PM +0000, Mark Brown wrote: > > It is not clear to me that this makes the kernel side better, it just > > seems to be rewriting the platform data for the sake of it. If it was > > converting to DT there'd be some stuff from it being DT but this keeps > > everything as in kernel as board files, just in a more complex form. > Not really. The benefits with swnode conversion are the following: > - reducing custom APIs / data types between _shared_ (in a sense of > supporting zillion different platforms) driver and a certain board > file > - as an effect of the above, reducing kernel code base, and as the result > make maintenance easier and bug-free for that parts I'm more worried about the possibility of breaking things with swnode support than I am for board files - with board files you've got a good chance of failing to compile if things get messed up, with swnode you can typo a property or whatever and silently fail. > - preparing a driver to be ready for any old board file conversion to DT > as it reduces that churn (you won't need to touch the driver code) The driver appears to already have DT support (there's a compatible for MMP2 in there)?
On Tue, 26 Mar 2024 20:07:50 +0200, Andy Shevchenko wrote: > As Arnd suggested we may drop linux/spi/pxa2xx_spi.h as most of > its content is being used solely internally to SPI subsystem > (PXA2xx drivers). Hence this refactoring series with the additional > win of getting rid of legacy documentation. > > Cc: Arnd Bergmann <arnd@arndb.de> > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [02/10] spi: pxa2xx: Keep PXA*_SSP types together commit: dad983d8812975b53db83f02ae6b0ad15f018a9e [03/10] spi: pxa2xx: Switch to use dev_err_probe() commit: d5449432f794e75cd4f5e46bc33bfe6ce20b657d All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
On Tue, Mar 26, 2024 at 08:26:11PM +0000, Mark Brown wrote: > On Tue, Mar 26, 2024 at 10:12:12PM +0200, Andy Shevchenko wrote: > > On Tue, Mar 26, 2024 at 08:02:57PM +0000, Mark Brown wrote: > > > > It is not clear to me that this makes the kernel side better, it just > > > seems to be rewriting the platform data for the sake of it. If it was > > > converting to DT there'd be some stuff from it being DT but this keeps > > > everything as in kernel as board files, just in a more complex form. > > > Not really. The benefits with swnode conversion are the following: > > > - reducing custom APIs / data types between _shared_ (in a sense of > > supporting zillion different platforms) driver and a certain board > > file > > > - as an effect of the above, reducing kernel code base, and as the result > > make maintenance easier and bug-free for that parts > > I'm more worried about the possibility of breaking things with swnode > support than I am for board files - with board files you've got a good > chance of failing to compile if things get messed up, with swnode you > can typo a property or whatever and silently fail. I understand that, but here it's consolidated in a single series and not supposed to be modified in the future, only dropping or properly converting. Btw, you may say the same about the all patches that converts to GPIO lookup tables (one typo in the not-so-often used GPIO line device ID name), but I don't remember that kind of conversions got much of objection. > > - preparing a driver to be ready for any old board file conversion to DT > > as it reduces that churn (you won't need to touch the driver code) > > The driver appears to already have DT support (there's a compatible for > MMP2 in there)? The MMP2 is using default number of chip select pins. Also note that my reply is generic (I used 'a driver' form).