* Re: [PATCH] dmaengine: Consistently define pci_device_ids using named initializers [not found] ` <afh0-BSmchvY-W-d@ashevche-desk.local> @ 2026-05-04 13:55 ` Uwe Kleine-König (The Capable Hub) 2026-05-04 14:09 ` Andy Shevchenko 0 siblings, 1 reply; 5+ messages in thread From: Uwe Kleine-König (The Capable Hub) @ 2026-05-04 13:55 UTC (permalink / raw) To: Andy Shevchenko Cc: Vinod Koul, Markus Schneider-Pargmann, Basavaraj Natikar, Frank Li, Manivannan Sadhasivam, Viresh Kumar, dmaengine, linux-kernel [-- Attachment #1: Type: text/plain, Size: 3564 bytes --] On Mon, May 04, 2026 at 01:29:12PM +0300, Andy Shevchenko wrote: > On Mon, May 04, 2026 at 12:20:06PM +0200, Uwe Kleine-König (The Capable Hub) wrote: > > The .driver_data member of the various struct pci_device_id arrays were > > initialized by list expressions. This isn't easily readable if you're > > not into PCI. Using named initializers is more explicit and thus easier > > to parse. Also skip explicit assignments of 0 (which the compiler then > > takes care of). > > > > This change doesn't introduce changes to the compiled pci_device_id > > arrays. Tested on x86 and arm64. > > HSU driver has different change ("Also" is a strong sign to the split required). HSU is in the category "skip explicit assignments of 0", so I think that's fine. I could be talked into splitting if that's what is wanted. > ... > > > static const struct pci_device_id pch_dma_id_table[] = { > > - { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_EG20T_PCH_DMA_8CH), 8 }, > > - { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_EG20T_PCH_DMA_4CH), 4 }, > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA1_8CH), 8}, /* UART Video */ > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA2_8CH), 8}, /* PCMIF SPI */ > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA3_4CH), 4}, /* FPGA */ > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA4_12CH), 12}, /* I2S */ > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA1_4CH), 4}, /* UART */ > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA2_4CH), 4}, /* Video SPI */ > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA3_4CH), 4}, /* Security */ > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA4_4CH), 4}, /* FPGA */ > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7831_DMA1_8CH), 8}, /* UART */ > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7831_DMA2_4CH), 4}, /* SPI */ > > - { 0, }, > > + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_EG20T_PCH_DMA_8CH), .driver_data = 8 }, > > + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_EG20T_PCH_DMA_4CH), .driver_data = 4 }, > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA1_8CH), .driver_data = 8 }, /* UART Video */ > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA2_8CH), .driver_data = 8 }, /* PCMIF SPI */ > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA3_4CH), .driver_data = 4 }, /* FPGA */ > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA4_12CH), .driver_data = 12 }, /* I2S */ > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA1_4CH), .driver_data = 4 }, /* UART */ > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA2_4CH), .driver_data = 4 }, /* Video SPI */ > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA3_4CH), .driver_data = 4 }, /* Security */ > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA4_4CH), .driver_data = 4 }, /* FPGA */ > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7831_DMA1_8CH), .driver_data = 8 }, /* UART */ > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7831_DMA2_4CH), .driver_data = 4 }, /* SPI */ > > + { }, > > }; > > Use PCI_DEVICE_DATA() instead. Same may apply to DesignWare, but one needs to > define the device IDs. I think I may help with that. I'm not a fan of PCI_DEVICE_DATA. While it could indeed be used to shorten the assignments here, it's less readable in my opinion. Compare { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_EG20T_PCH_DMA_4CH), .driver_data = 4 }, with { PCI_DEVICE_DATA(INTEL, PCI_DEVICE_ID_EG20T_PCH_DMA_4CH, 4) }, . For someone who doesn't know what PCI_DEVICE_DATA does, the latter is less understandable. Also PCI_DEVICE_DATA has a cast which is something I want to get rid of. Best regards Uwe [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] dmaengine: Consistently define pci_device_ids using named initializers 2026-05-04 13:55 ` [PATCH] dmaengine: Consistently define pci_device_ids using named initializers Uwe Kleine-König (The Capable Hub) @ 2026-05-04 14:09 ` Andy Shevchenko 2026-05-04 16:38 ` Uwe Kleine-König (The Capable Hub) 0 siblings, 1 reply; 5+ messages in thread From: Andy Shevchenko @ 2026-05-04 14:09 UTC (permalink / raw) To: Uwe Kleine-König (The Capable Hub) Cc: Vinod Koul, Markus Schneider-Pargmann, Basavaraj Natikar, Frank Li, Manivannan Sadhasivam, Viresh Kumar, dmaengine, linux-kernel On Mon, May 04, 2026 at 03:55:00PM +0200, Uwe Kleine-König (The Capable Hub) wrote: > On Mon, May 04, 2026 at 01:29:12PM +0300, Andy Shevchenko wrote: > > On Mon, May 04, 2026 at 12:20:06PM +0200, Uwe Kleine-König (The Capable Hub) wrote: > > > The .driver_data member of the various struct pci_device_id arrays were > > > initialized by list expressions. This isn't easily readable if you're > > > not into PCI. Using named initializers is more explicit and thus easier > > > to parse. Also skip explicit assignments of 0 (which the compiler then > > > takes care of). > > > > > > This change doesn't introduce changes to the compiled pci_device_id > > > arrays. Tested on x86 and arm64. > > > > HSU driver has different change ("Also" is a strong sign to the split required). > > HSU is in the category "skip explicit assignments of 0", so I think > that's fine. I could be talked into splitting if that's what is wanted. Yes, please. I will Rb/Ack it immediately when standalone change. ... > > > static const struct pci_device_id pch_dma_id_table[] = { > > > - { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_EG20T_PCH_DMA_8CH), 8 }, > > > - { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_EG20T_PCH_DMA_4CH), 4 }, > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA1_8CH), 8}, /* UART Video */ > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA2_8CH), 8}, /* PCMIF SPI */ > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA3_4CH), 4}, /* FPGA */ > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA4_12CH), 12}, /* I2S */ > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA1_4CH), 4}, /* UART */ > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA2_4CH), 4}, /* Video SPI */ > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA3_4CH), 4}, /* Security */ > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA4_4CH), 4}, /* FPGA */ > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7831_DMA1_8CH), 8}, /* UART */ > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7831_DMA2_4CH), 4}, /* SPI */ > > > - { 0, }, > > > + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_EG20T_PCH_DMA_8CH), .driver_data = 8 }, > > > + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_EG20T_PCH_DMA_4CH), .driver_data = 4 }, > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA1_8CH), .driver_data = 8 }, /* UART Video */ > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA2_8CH), .driver_data = 8 }, /* PCMIF SPI */ > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA3_4CH), .driver_data = 4 }, /* FPGA */ > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA4_12CH), .driver_data = 12 }, /* I2S */ > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA1_4CH), .driver_data = 4 }, /* UART */ > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA2_4CH), .driver_data = 4 }, /* Video SPI */ > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA3_4CH), .driver_data = 4 }, /* Security */ > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA4_4CH), .driver_data = 4 }, /* FPGA */ > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7831_DMA1_8CH), .driver_data = 8 }, /* UART */ > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7831_DMA2_4CH), .driver_data = 4 }, /* SPI */ > > > + { }, > > > }; > > > > Use PCI_DEVICE_DATA() instead. Same may apply to DesignWare, but one needs to > > define the device IDs. I think I may help with that. > > I'm not a fan of PCI_DEVICE_DATA. While it could indeed be used to > shorten the assignments here, it's less readable in my opinion. I'm not fun of these long unreadable lines with tons of repetitions :-) > Compare > > { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_EG20T_PCH_DMA_4CH), .driver_data = 4 }, > > with > > { PCI_DEVICE_DATA(INTEL, PCI_DEVICE_ID_EG20T_PCH_DMA_4CH, 4) }, First of all, with { PCI_DEVICE_DATA(INTEL, EG20T_PCH_DMA_4CH, 4) }, > . For someone who doesn't know what PCI_DEVICE_DATA does, the latter is > less understandable. Same applicable to many other macros. I don't consider this argument viable. > Also PCI_DEVICE_DATA has a cast which is something I want to get rid of. Yes, and you will get rid of in one place instead of tons of them. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] dmaengine: Consistently define pci_device_ids using named initializers 2026-05-04 14:09 ` Andy Shevchenko @ 2026-05-04 16:38 ` Uwe Kleine-König (The Capable Hub) 2026-05-05 7:01 ` Andy Shevchenko 0 siblings, 1 reply; 5+ messages in thread From: Uwe Kleine-König (The Capable Hub) @ 2026-05-04 16:38 UTC (permalink / raw) To: Andy Shevchenko Cc: Vinod Koul, Markus Schneider-Pargmann, Basavaraj Natikar, Frank Li, Manivannan Sadhasivam, Viresh Kumar, dmaengine, linux-kernel [-- Attachment #1: Type: text/plain, Size: 4806 bytes --] Hello Andy, On Mon, May 04, 2026 at 05:09:55PM +0300, Andy Shevchenko wrote: > On Mon, May 04, 2026 at 03:55:00PM +0200, Uwe Kleine-König (The Capable Hub) wrote: > > On Mon, May 04, 2026 at 01:29:12PM +0300, Andy Shevchenko wrote: > > > On Mon, May 04, 2026 at 12:20:06PM +0200, Uwe Kleine-König (The Capable Hub) wrote: > > > > The .driver_data member of the various struct pci_device_id arrays were > > > > initialized by list expressions. This isn't easily readable if you're > > > > not into PCI. Using named initializers is more explicit and thus easier > > > > to parse. Also skip explicit assignments of 0 (which the compiler then > > > > takes care of). > > > > > > > > This change doesn't introduce changes to the compiled pci_device_id > > > > arrays. Tested on x86 and arm64. > > > > > > HSU driver has different change ("Also" is a strong sign to the split required). > > > > HSU is in the category "skip explicit assignments of 0", so I think > > that's fine. I could be talked into splitting if that's what is wanted. > > Yes, please. I will Rb/Ack it immediately when standalone change. > > ... > > > > > static const struct pci_device_id pch_dma_id_table[] = { > > > > - { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_EG20T_PCH_DMA_8CH), 8 }, > > > > - { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_EG20T_PCH_DMA_4CH), 4 }, > > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA1_8CH), 8}, /* UART Video */ > > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA2_8CH), 8}, /* PCMIF SPI */ > > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA3_4CH), 4}, /* FPGA */ > > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA4_12CH), 12}, /* I2S */ > > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA1_4CH), 4}, /* UART */ > > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA2_4CH), 4}, /* Video SPI */ > > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA3_4CH), 4}, /* Security */ > > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA4_4CH), 4}, /* FPGA */ > > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7831_DMA1_8CH), 8}, /* UART */ > > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7831_DMA2_4CH), 4}, /* SPI */ > > > > - { 0, }, > > > > + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_EG20T_PCH_DMA_8CH), .driver_data = 8 }, > > > > + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_EG20T_PCH_DMA_4CH), .driver_data = 4 }, > > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA1_8CH), .driver_data = 8 }, /* UART Video */ > > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA2_8CH), .driver_data = 8 }, /* PCMIF SPI */ > > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA3_4CH), .driver_data = 4 }, /* FPGA */ > > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA4_12CH), .driver_data = 12 }, /* I2S */ > > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA1_4CH), .driver_data = 4 }, /* UART */ > > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA2_4CH), .driver_data = 4 }, /* Video SPI */ > > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA3_4CH), .driver_data = 4 }, /* Security */ > > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA4_4CH), .driver_data = 4 }, /* FPGA */ > > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7831_DMA1_8CH), .driver_data = 8 }, /* UART */ > > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7831_DMA2_4CH), .driver_data = 4 }, /* SPI */ > > > > + { }, > > > > }; > > > > > > Use PCI_DEVICE_DATA() instead. Same may apply to DesignWare, but one needs to > > > define the device IDs. I think I may help with that. > > > > I'm not a fan of PCI_DEVICE_DATA. While it could indeed be used to > > shorten the assignments here, it's less readable in my opinion. > > I'm not fun of these long unreadable lines with tons of repetitions :-) Seems to be subjective. > > Compare > > > > { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_EG20T_PCH_DMA_4CH), .driver_data = 4 }, > > > > with > > > > { PCI_DEVICE_DATA(INTEL, PCI_DEVICE_ID_EG20T_PCH_DMA_4CH, 4) }, > > First of all, with > > { PCI_DEVICE_DATA(INTEL, EG20T_PCH_DMA_4CH, 4) }, Agreed. That doesn't considerably weaken my reasoning however. > > . For someone who doesn't know what PCI_DEVICE_DATA does, the latter is > > less understandable. > > Same applicable to many other macros. I don't consider this argument viable. Also agreed. But other bad macros don't justify using that (admittedly subjectively) bad PCI_DEVICE_DATA macro that mixes device identity (.vendor, .device, .subvendor and .subdevice) with a driver specific struct member. > > Also PCI_DEVICE_DATA has a cast which is something I want to get rid of. > > Yes, and you will get rid of in one place instead of tons of them. This would require another (subjectively bad) macro PCI_DEVICE_DATAPTR. I think I let someone else tackle that quest. Best regards Uwe [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] dmaengine: Consistently define pci_device_ids using named initializers 2026-05-04 16:38 ` Uwe Kleine-König (The Capable Hub) @ 2026-05-05 7:01 ` Andy Shevchenko 2026-05-05 7:04 ` Andy Shevchenko 0 siblings, 1 reply; 5+ messages in thread From: Andy Shevchenko @ 2026-05-05 7:01 UTC (permalink / raw) To: Uwe Kleine-König (The Capable Hub) Cc: Vinod Koul, Markus Schneider-Pargmann, Basavaraj Natikar, Frank Li, Manivannan Sadhasivam, Viresh Kumar, dmaengine, linux-kernel On Mon, May 04, 2026 at 06:38:51PM +0200, Uwe Kleine-König (The Capable Hub) wrote: > On Mon, May 04, 2026 at 05:09:55PM +0300, Andy Shevchenko wrote: > > On Mon, May 04, 2026 at 03:55:00PM +0200, Uwe Kleine-König (The Capable Hub) wrote: > > > On Mon, May 04, 2026 at 01:29:12PM +0300, Andy Shevchenko wrote: > > > > On Mon, May 04, 2026 at 12:20:06PM +0200, Uwe Kleine-König (The Capable Hub) wrote: ... > > > > > static const struct pci_device_id pch_dma_id_table[] = { > > > > > - { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_EG20T_PCH_DMA_8CH), 8 }, > > > > > - { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_EG20T_PCH_DMA_4CH), 4 }, > > > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA1_8CH), 8}, /* UART Video */ > > > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA2_8CH), 8}, /* PCMIF SPI */ > > > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA3_4CH), 4}, /* FPGA */ > > > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA4_12CH), 12}, /* I2S */ > > > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA1_4CH), 4}, /* UART */ > > > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA2_4CH), 4}, /* Video SPI */ > > > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA3_4CH), 4}, /* Security */ > > > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA4_4CH), 4}, /* FPGA */ > > > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7831_DMA1_8CH), 8}, /* UART */ > > > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7831_DMA2_4CH), 4}, /* SPI */ > > > > > - { 0, }, > > > > > + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_EG20T_PCH_DMA_8CH), .driver_data = 8 }, > > > > > + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_EG20T_PCH_DMA_4CH), .driver_data = 4 }, > > > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA1_8CH), .driver_data = 8 }, /* UART Video */ > > > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA2_8CH), .driver_data = 8 }, /* PCMIF SPI */ > > > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA3_4CH), .driver_data = 4 }, /* FPGA */ > > > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA4_12CH), .driver_data = 12 }, /* I2S */ > > > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA1_4CH), .driver_data = 4 }, /* UART */ > > > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA2_4CH), .driver_data = 4 }, /* Video SPI */ > > > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA3_4CH), .driver_data = 4 }, /* Security */ > > > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA4_4CH), .driver_data = 4 }, /* FPGA */ > > > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7831_DMA1_8CH), .driver_data = 8 }, /* UART */ > > > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7831_DMA2_4CH), .driver_data = 4 }, /* SPI */ > > > > > + { }, > > > > > }; > > > > > > > > Use PCI_DEVICE_DATA() instead. Same may apply to DesignWare, but one needs to > > > > define the device IDs. I think I may help with that. > > > > > > I'm not a fan of PCI_DEVICE_DATA. While it could indeed be used to > > > shorten the assignments here, it's less readable in my opinion. > > > > I'm not fun of these long unreadable lines with tons of repetitions :-) > > Seems to be subjective. > > > > Compare > > > > > > { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_EG20T_PCH_DMA_4CH), .driver_data = 4 }, > > > > > > with > > > > > > { PCI_DEVICE_DATA(INTEL, PCI_DEVICE_ID_EG20T_PCH_DMA_4CH, 4) }, > > > > First of all, with > > > > { PCI_DEVICE_DATA(INTEL, EG20T_PCH_DMA_4CH, 4) }, > > Agreed. That doesn't considerably weaken my reasoning however. > > > > . For someone who doesn't know what PCI_DEVICE_DATA does, the latter is > > > less understandable. > > > > Same applicable to many other macros. I don't consider this argument viable. > > Also agreed. But other bad macros don't justify using that (admittedly > subjectively) bad PCI_DEVICE_DATA macro that mixes device identity > (.vendor, .device, .subvendor and .subdevice) with a driver specific > struct member. > > > > Also PCI_DEVICE_DATA has a cast which is something I want to get rid of. > > > > Yes, and you will get rid of in one place instead of tons of them. > > This would require another (subjectively bad) macro PCI_DEVICE_DATAPTR. > I think I let someone else tackle that quest. No, it wouldn't. Since we support C11, we have _Generic(). It may be used. And please use PCI_DEVICE_DATA() in this driver. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] dmaengine: Consistently define pci_device_ids using named initializers 2026-05-05 7:01 ` Andy Shevchenko @ 2026-05-05 7:04 ` Andy Shevchenko 0 siblings, 0 replies; 5+ messages in thread From: Andy Shevchenko @ 2026-05-05 7:04 UTC (permalink / raw) To: Uwe Kleine-König (The Capable Hub) Cc: Vinod Koul, Markus Schneider-Pargmann, Basavaraj Natikar, Frank Li, Manivannan Sadhasivam, Viresh Kumar, dmaengine, linux-kernel On Tue, May 05, 2026 at 10:01:35AM +0300, Andy Shevchenko wrote: > On Mon, May 04, 2026 at 06:38:51PM +0200, Uwe Kleine-König (The Capable Hub) wrote: > > On Mon, May 04, 2026 at 05:09:55PM +0300, Andy Shevchenko wrote: > > > On Mon, May 04, 2026 at 03:55:00PM +0200, Uwe Kleine-König (The Capable Hub) wrote: > > > > On Mon, May 04, 2026 at 01:29:12PM +0300, Andy Shevchenko wrote: > > > > > On Mon, May 04, 2026 at 12:20:06PM +0200, Uwe Kleine-König (The Capable Hub) wrote: ... > > > > > > static const struct pci_device_id pch_dma_id_table[] = { > > > > > > - { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_EG20T_PCH_DMA_8CH), 8 }, > > > > > > - { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_EG20T_PCH_DMA_4CH), 4 }, > > > > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA1_8CH), 8}, /* UART Video */ > > > > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA2_8CH), 8}, /* PCMIF SPI */ > > > > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA3_4CH), 4}, /* FPGA */ > > > > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA4_12CH), 12}, /* I2S */ > > > > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA1_4CH), 4}, /* UART */ > > > > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA2_4CH), 4}, /* Video SPI */ > > > > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA3_4CH), 4}, /* Security */ > > > > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA4_4CH), 4}, /* FPGA */ > > > > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7831_DMA1_8CH), 8}, /* UART */ > > > > > > - { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7831_DMA2_4CH), 4}, /* SPI */ > > > > > > - { 0, }, > > > > > > + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_EG20T_PCH_DMA_8CH), .driver_data = 8 }, > > > > > > + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_EG20T_PCH_DMA_4CH), .driver_data = 4 }, > > > > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA1_8CH), .driver_data = 8 }, /* UART Video */ > > > > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA2_8CH), .driver_data = 8 }, /* PCMIF SPI */ > > > > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA3_4CH), .driver_data = 4 }, /* FPGA */ > > > > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_DMA4_12CH), .driver_data = 12 }, /* I2S */ > > > > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA1_4CH), .driver_data = 4 }, /* UART */ > > > > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA2_4CH), .driver_data = 4 }, /* Video SPI */ > > > > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA3_4CH), .driver_data = 4 }, /* Security */ > > > > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7223_DMA4_4CH), .driver_data = 4 }, /* FPGA */ > > > > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7831_DMA1_8CH), .driver_data = 8 }, /* UART */ > > > > > > + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7831_DMA2_4CH), .driver_data = 4 }, /* SPI */ > > > > > > + { }, > > > > > > }; > > > > > > > > > > Use PCI_DEVICE_DATA() instead. Same may apply to DesignWare, but one needs to > > > > > define the device IDs. I think I may help with that. > > > > > > > > I'm not a fan of PCI_DEVICE_DATA. While it could indeed be used to > > > > shorten the assignments here, it's less readable in my opinion. > > > > > > I'm not fun of these long unreadable lines with tons of repetitions :-) > > > > Seems to be subjective. > > > > > > Compare > > > > > > > > { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_EG20T_PCH_DMA_4CH), .driver_data = 4 }, > > > > > > > > with > > > > > > > > { PCI_DEVICE_DATA(INTEL, PCI_DEVICE_ID_EG20T_PCH_DMA_4CH, 4) }, > > > > > > First of all, with > > > > > > { PCI_DEVICE_DATA(INTEL, EG20T_PCH_DMA_4CH, 4) }, > > > > Agreed. That doesn't considerably weaken my reasoning however. > > > > > > . For someone who doesn't know what PCI_DEVICE_DATA does, the latter is > > > > less understandable. > > > > > > Same applicable to many other macros. I don't consider this argument viable. > > > > Also agreed. But other bad macros don't justify using that (admittedly > > subjectively) bad PCI_DEVICE_DATA macro that mixes device identity > > (.vendor, .device, .subvendor and .subdevice) with a driver specific > > struct member. > > > > > > Also PCI_DEVICE_DATA has a cast which is something I want to get rid of. > > > > > > Yes, and you will get rid of in one place instead of tons of them. > > > > This would require another (subjectively bad) macro PCI_DEVICE_DATAPTR. > > I think I let someone else tackle that quest. > > No, it wouldn't. Since we support C11, we have _Generic(). It may be used. The example you probably want to look at is d7cdbbc93c56 ("software node: allow referencing firmware nodes") (yes, it's not a union there, but I think we can manage unions as well). > And please use PCI_DEVICE_DATA() in this driver. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-05-05 7:04 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20260504102008.1996139-2-u.kleine-koenig@baylibre.com>
[not found] ` <afh0-BSmchvY-W-d@ashevche-desk.local>
2026-05-04 13:55 ` [PATCH] dmaengine: Consistently define pci_device_ids using named initializers Uwe Kleine-König (The Capable Hub)
2026-05-04 14:09 ` Andy Shevchenko
2026-05-04 16:38 ` Uwe Kleine-König (The Capable Hub)
2026-05-05 7:01 ` Andy Shevchenko
2026-05-05 7:04 ` Andy Shevchenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox