| Message ID | 20240326181027.1418989-8-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| 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, 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).
diff --git a/arch/arm/mach-pxa/spitz.c b/arch/arm/mach-pxa/spitz.c index 318402ad685e..3c5f5a3cb480 100644 --- a/arch/arm/mach-pxa/spitz.c +++ b/arch/arm/mach-pxa/spitz.c @@ -18,10 +18,10 @@ #include <linux/i2c.h> #include <linux/platform_data/i2c-pxa.h> #include <linux/platform_data/pca953x.h> +#include <linux/property.h> #include <linux/spi/spi.h> #include <linux/spi/ads7846.h> #include <linux/spi/corgi_lcd.h> -#include <linux/spi/pxa2xx_spi.h> #include <linux/mtd/sharpsl.h> #include <linux/mtd/physmap.h> #include <linux/input-event-codes.h> @@ -569,10 +569,6 @@ static struct spi_board_info spitz_spi_devices[] = { }, }; -static struct pxa2xx_spi_controller spitz_spi_info = { - .num_chipselect = 3, -}; - static struct gpiod_lookup_table spitz_spi_gpio_table = { .dev_id = "spi2", .table = { @@ -583,10 +579,20 @@ static struct gpiod_lookup_table spitz_spi_gpio_table = { }, }; +static const struct property_entry spitz_spi_properties[] = { + PROPERTY_ENTRY_U32("num-cs", 3), + { } +}; + +static const struct software_node spitz_spi_node = { + .properties = spitz_spi_properties, +}; + static void __init spitz_spi_init(void) { struct platform_device *pd; int id = 2; + int err; if (machine_is_akita()) gpiod_add_lookup_table(&akita_lcdcon_gpio_table); @@ -601,8 +607,13 @@ static void __init spitz_spi_init(void) if (pd == NULL) { pr_err("pxa2xx-spi: failed to allocate device id %d\n", id); } else { - pd->dev.platform_data = &spitz_spi_info; - platform_device_add(pd); + err = device_add_software_node(&pd->dev, &spitz_spi_node); + if (err) { + platform_device_put(pd); + pr_err("pxa2xx-spi: failed to add software node\n"); + } else { + platform_device_add(pd); + } } spi_register_board_info(ARRAY_AND_SIZE(spitz_spi_devices));
Since driver can parse num-cs device property, replace platform data with this new approach. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- arch/arm/mach-pxa/spitz.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)