| Message ID | 20190313122403.248873-1-armax@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spi: mediatek: Attempt to address style issues in spi-mt7621.c | expand |
On 13/03/2019 13:24, Armando Miraglia wrote: > Running Lindent on the mt7621-spi.c file in drivers/staging I noticed that the > file contained style issues. This change attempts to address such style > problems. > > Signed-off-by: Armando Miraglia <armax@google.com> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com> Apart from fixing styling issues it would be usefull to see if we can add support for mt7621 to drivers/spi/spi-mt65xx.c Not sure if that is something you want to work on :) Regards, Matthias > --- > NOTE: resend this patch to include all mainteners listed by get_mantainers.pl. > drivers/staging/mt7621-spi/spi-mt7621.c | 27 +++++++++++++------------ > 1 file changed, 14 insertions(+), 13 deletions(-) > > diff --git a/drivers/staging/mt7621-spi/spi-mt7621.c b/drivers/staging/mt7621-spi/spi-mt7621.c > index b509f9fe3346..03d53845f8c5 100644 > --- a/drivers/staging/mt7621-spi/spi-mt7621.c > +++ b/drivers/staging/mt7621-spi/spi-mt7621.c > @@ -52,14 +52,14 @@ > #define MT7621_LSB_FIRST BIT(3) > > struct mt7621_spi { > - struct spi_master *master; > - void __iomem *base; > - unsigned int sys_freq; > - unsigned int speed; > - struct clk *clk; > - int pending_write; > - > - struct mt7621_spi_ops *ops; > + struct spi_master *master; > + void __iomem *base; > + unsigned int sys_freq; > + unsigned int speed; > + struct clk *clk; > + int pending_write; > + > + struct mt7621_spi_ops *ops; > }; > > static inline struct mt7621_spi *spidev_to_mt7621_spi(struct spi_device *spi) > @@ -303,7 +303,7 @@ static int mt7621_spi_setup(struct spi_device *spi) > struct mt7621_spi *rs = spidev_to_mt7621_spi(spi); > > if ((spi->max_speed_hz == 0) || > - (spi->max_speed_hz > (rs->sys_freq / 2))) > + (spi->max_speed_hz > (rs->sys_freq / 2))) > spi->max_speed_hz = (rs->sys_freq / 2); > > if (spi->max_speed_hz < (rs->sys_freq / 4097)) { > @@ -316,9 +316,10 @@ static int mt7621_spi_setup(struct spi_device *spi) > } > > static const struct of_device_id mt7621_spi_match[] = { > - { .compatible = "ralink,mt7621-spi" }, > + {.compatible = "ralink,mt7621-spi"}, > {}, > }; > + > MODULE_DEVICE_TABLE(of, mt7621_spi_match); > > static int mt7621_spi_probe(struct platform_device *pdev) > @@ -408,9 +409,9 @@ MODULE_ALIAS("platform:" DRIVER_NAME); > > static struct platform_driver mt7621_spi_driver = { > .driver = { > - .name = DRIVER_NAME, > - .of_match_table = mt7621_spi_match, > - }, > + .name = DRIVER_NAME, > + .of_match_table = mt7621_spi_match, > + }, > .probe = mt7621_spi_probe, > .remove = mt7621_spi_remove, > }; >
That might be fun to try :) I should get an mt7621 dev board of sorts though. On Wed, Mar 13, 2019 at 1:28 PM Matthias Brugger <matthias.bgg@gmail.com> wrote: > > > > On 13/03/2019 13:24, Armando Miraglia wrote: > > Running Lindent on the mt7621-spi.c file in drivers/staging I noticed that the > > file contained style issues. This change attempts to address such style > > problems. > > > > Signed-off-by: Armando Miraglia <armax@google.com> > > Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com> > > Apart from fixing styling issues it would be usefull to see if we can add > support for mt7621 to drivers/spi/spi-mt65xx.c > > Not sure if that is something you want to work on :) > > Regards, > Matthias > > > --- > > NOTE: resend this patch to include all mainteners listed by get_mantainers.pl. > > drivers/staging/mt7621-spi/spi-mt7621.c | 27 +++++++++++++------------ > > 1 file changed, 14 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/staging/mt7621-spi/spi-mt7621.c b/drivers/staging/mt7621-spi/spi-mt7621.c > > index b509f9fe3346..03d53845f8c5 100644 > > --- a/drivers/staging/mt7621-spi/spi-mt7621.c > > +++ b/drivers/staging/mt7621-spi/spi-mt7621.c > > @@ -52,14 +52,14 @@ > > #define MT7621_LSB_FIRST BIT(3) > > > > struct mt7621_spi { > > - struct spi_master *master; > > - void __iomem *base; > > - unsigned int sys_freq; > > - unsigned int speed; > > - struct clk *clk; > > - int pending_write; > > - > > - struct mt7621_spi_ops *ops; > > + struct spi_master *master; > > + void __iomem *base; > > + unsigned int sys_freq; > > + unsigned int speed; > > + struct clk *clk; > > + int pending_write; > > + > > + struct mt7621_spi_ops *ops; > > }; > > > > static inline struct mt7621_spi *spidev_to_mt7621_spi(struct spi_device *spi) > > @@ -303,7 +303,7 @@ static int mt7621_spi_setup(struct spi_device *spi) > > struct mt7621_spi *rs = spidev_to_mt7621_spi(spi); > > > > if ((spi->max_speed_hz == 0) || > > - (spi->max_speed_hz > (rs->sys_freq / 2))) > > + (spi->max_speed_hz > (rs->sys_freq / 2))) > > spi->max_speed_hz = (rs->sys_freq / 2); > > > > if (spi->max_speed_hz < (rs->sys_freq / 4097)) { > > @@ -316,9 +316,10 @@ static int mt7621_spi_setup(struct spi_device *spi) > > } > > > > static const struct of_device_id mt7621_spi_match[] = { > > - { .compatible = "ralink,mt7621-spi" }, > > + {.compatible = "ralink,mt7621-spi"}, > > {}, > > }; > > + > > MODULE_DEVICE_TABLE(of, mt7621_spi_match); > > > > static int mt7621_spi_probe(struct platform_device *pdev) > > @@ -408,9 +409,9 @@ MODULE_ALIAS("platform:" DRIVER_NAME); > > > > static struct platform_driver mt7621_spi_driver = { > > .driver = { > > - .name = DRIVER_NAME, > > - .of_match_table = mt7621_spi_match, > > - }, > > + .name = DRIVER_NAME, > > + .of_match_table = mt7621_spi_match, > > + }, > > .probe = mt7621_spi_probe, > > .remove = mt7621_spi_remove, > > }; > >
On Wed, Mar 13, 2019 at 01:24:04PM +0100, Armando Miraglia wrote: > Running Lindent on the mt7621-spi.c file in drivers/staging I noticed that the > file contained style issues. This change attempts to address such style > problems. > Don't run lindent. I think checkpatch.pl has a --fix option that might be better, but once the code is merged then our standard become much higher for follow up patches. > Signed-off-by: Armando Miraglia <armax@google.com> > --- > NOTE: resend this patch to include all mainteners listed by get_mantainers.pl. > drivers/staging/mt7621-spi/spi-mt7621.c | 27 +++++++++++++------------ > 1 file changed, 14 insertions(+), 13 deletions(-) > > diff --git a/drivers/staging/mt7621-spi/spi-mt7621.c b/drivers/staging/mt7621-spi/spi-mt7621.c > index b509f9fe3346..03d53845f8c5 100644 > --- a/drivers/staging/mt7621-spi/spi-mt7621.c > +++ b/drivers/staging/mt7621-spi/spi-mt7621.c > @@ -52,14 +52,14 @@ > #define MT7621_LSB_FIRST BIT(3) > > struct mt7621_spi { > - struct spi_master *master; > - void __iomem *base; > - unsigned int sys_freq; > - unsigned int speed; > - struct clk *clk; > - int pending_write; > - > - struct mt7621_spi_ops *ops; > + struct spi_master *master; > + void __iomem *base; > + unsigned int sys_freq; > + unsigned int speed; > + struct clk *clk; > + int pending_write; > + > + struct mt7621_spi_ops *ops; The original is fine. I don't encourage people to do fancy indenting with their local variable declarations inside functions but for a struct the declarations aren't going to change a lot so people can get fancy if they want. The problem with a local is if you need to add a new variable then you have to re-indent a bunch of unrelated lines or have one out of alignment line. Most people know this intuitively so they don't get fancy. > }; > > static inline struct mt7621_spi *spidev_to_mt7621_spi(struct spi_device *spi) > @@ -303,7 +303,7 @@ static int mt7621_spi_setup(struct spi_device *spi) > struct mt7621_spi *rs = spidev_to_mt7621_spi(spi); > > if ((spi->max_speed_hz == 0) || > - (spi->max_speed_hz > (rs->sys_freq / 2))) > + (spi->max_speed_hz > (rs->sys_freq / 2))) Yeah. Lindent is correct here. > spi->max_speed_hz = (rs->sys_freq / 2); > > if (spi->max_speed_hz < (rs->sys_freq / 4097)) { > @@ -316,9 +316,10 @@ static int mt7621_spi_setup(struct spi_device *spi) > } > > static const struct of_device_id mt7621_spi_match[] = { > - { .compatible = "ralink,mt7621-spi" }, > + {.compatible = "ralink,mt7621-spi"}, The original was better. > {}, > }; > + > MODULE_DEVICE_TABLE(of, mt7621_spi_match); No need for a blank. These are closely related. > > static int mt7621_spi_probe(struct platform_device *pdev) > @@ -408,9 +409,9 @@ MODULE_ALIAS("platform:" DRIVER_NAME); > > static struct platform_driver mt7621_spi_driver = { > .driver = { > - .name = DRIVER_NAME, > - .of_match_table = mt7621_spi_match, > - }, > + .name = DRIVER_NAME, > + .of_match_table = mt7621_spi_match, > + }, The new indenting is very wrong. regards, dan carpenter
Hi! On Wed, Mar 13, 2019 at 8:28 PM Matthias Brugger <matthias.bgg@gmail.com> wrote: > > > > On 13/03/2019 13:24, Armando Miraglia wrote: > [...] > Apart from fixing styling issues it would be usefull to see if we can add > support for mt7621 to drivers/spi/spi-mt65xx.c It's impossible. They are completely different IPs. > > Not sure if that is something you want to work on :) > > Regards, > Matthias
On 13/03/2019 17:34, Chuanhong Guo wrote: > Hi! > On Wed, Mar 13, 2019 at 8:28 PM Matthias Brugger <matthias.bgg@gmail.com> wrote: >> >> >> >> On 13/03/2019 13:24, Armando Miraglia wrote: >> [...] >> Apart from fixing styling issues it would be usefull to see if we can add >> support for mt7621 to drivers/spi/spi-mt65xx.c > It's impossible. They are completely different IPs. Thanks for the info. Do you know the status of the rest of the drivers in staging? Best regards, Matthias
On 13/03/2019 13:34, Dan Carpenter wrote: > On Wed, Mar 13, 2019 at 01:24:04PM +0100, Armando Miraglia wrote: >> Running Lindent on the mt7621-spi.c file in drivers/staging I noticed that the >> file contained style issues. This change attempts to address such style >> problems. >> > > Don't run lindent. I think checkpatch.pl has a --fix option that might > be better, but once the code is merged then our standard become much > higher for follow up patches. > >> Signed-off-by: Armando Miraglia <armax@google.com> >> --- >> NOTE: resend this patch to include all mainteners listed by get_mantainers.pl. >> drivers/staging/mt7621-spi/spi-mt7621.c | 27 +++++++++++++------------ >> 1 file changed, 14 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/staging/mt7621-spi/spi-mt7621.c b/drivers/staging/mt7621-spi/spi-mt7621.c >> index b509f9fe3346..03d53845f8c5 100644 >> --- a/drivers/staging/mt7621-spi/spi-mt7621.c >> +++ b/drivers/staging/mt7621-spi/spi-mt7621.c >> @@ -52,14 +52,14 @@ >> #define MT7621_LSB_FIRST BIT(3) >> >> struct mt7621_spi { >> - struct spi_master *master; >> - void __iomem *base; >> - unsigned int sys_freq; >> - unsigned int speed; >> - struct clk *clk; >> - int pending_write; >> - >> - struct mt7621_spi_ops *ops; >> + struct spi_master *master; >> + void __iomem *base; >> + unsigned int sys_freq; >> + unsigned int speed; >> + struct clk *clk; >> + int pending_write; >> + >> + struct mt7621_spi_ops *ops; > > The original is fine. I don't encourage people to do fancy indenting > with their local variable declarations inside functions but for a struct > the declarations aren't going to change a lot so people can get fancy > if they want. > > The problem with a local is if you need to add a new variable then you > have to re-indent a bunch of unrelated lines or have one out of > alignment line. Most people know this intuitively so they don't get > fancy. > >> }; >> >> static inline struct mt7621_spi *spidev_to_mt7621_spi(struct spi_device *spi) >> @@ -303,7 +303,7 @@ static int mt7621_spi_setup(struct spi_device *spi) >> struct mt7621_spi *rs = spidev_to_mt7621_spi(spi); >> >> if ((spi->max_speed_hz == 0) || >> - (spi->max_speed_hz > (rs->sys_freq / 2))) >> + (spi->max_speed_hz > (rs->sys_freq / 2))) > > Yeah. Lindent is correct here. > >> spi->max_speed_hz = (rs->sys_freq / 2); >> >> if (spi->max_speed_hz < (rs->sys_freq / 4097)) { >> @@ -316,9 +316,10 @@ static int mt7621_spi_setup(struct spi_device *spi) >> } >> >> static const struct of_device_id mt7621_spi_match[] = { >> - { .compatible = "ralink,mt7621-spi" }, >> + {.compatible = "ralink,mt7621-spi"}, > > The original was better. > >> {}, >> }; >> + >> MODULE_DEVICE_TABLE(of, mt7621_spi_match); > > No need for a blank. These are closely related. > >> >> static int mt7621_spi_probe(struct platform_device *pdev) >> @@ -408,9 +409,9 @@ MODULE_ALIAS("platform:" DRIVER_NAME); >> >> static struct platform_driver mt7621_spi_driver = { >> .driver = { >> - .name = DRIVER_NAME, >> - .of_match_table = mt7621_spi_match, >> - }, >> + .name = DRIVER_NAME, >> + .of_match_table = mt7621_spi_match, >> + }, > > The new indenting is very wrong. > Fair enough, I was too fast providing my Reviewed-by tag :-/
On 13.03.19 17:46, Matthias Brugger wrote: > > > On 13/03/2019 17:34, Chuanhong Guo wrote: >> Hi! >> On Wed, Mar 13, 2019 at 8:28 PM Matthias Brugger <matthias.bgg@gmail.com> wrote: >>> >>> >>> >>> On 13/03/2019 13:24, Armando Miraglia wrote: >>> [...] >>> Apart from fixing styling issues it would be usefull to see if we can add >>> support for mt7621 to drivers/spi/spi-mt65xx.c >> It's impossible. They are completely different IPs. > > Thanks for the info. Do you know the status of the rest of the drivers in staging? Just to inform you. We are using this SPI driver from staging in one of our customer projects and I will try to move this driver out of staging into drivers/spi very shortly. Thanks, Stefan
On Wed, Mar 13 2019, Stefan Roese wrote: > On 13.03.19 17:46, Matthias Brugger wrote: >> >> >> On 13/03/2019 17:34, Chuanhong Guo wrote: >>> Hi! >>> On Wed, Mar 13, 2019 at 8:28 PM Matthias Brugger <matthias.bgg@gmail.com> wrote: >>>> >>>> >>>> >>>> On 13/03/2019 13:24, Armando Miraglia wrote: >>>> [...] >>>> Apart from fixing styling issues it would be usefull to see if we can add >>>> support for mt7621 to drivers/spi/spi-mt65xx.c >>> It's impossible. They are completely different IPs. >> >> Thanks for the info. Do you know the status of the rest of the drivers in staging? > > Just to inform you. We are using this SPI driver from staging > in one of our customer projects and I will try to move this > driver out of staging into drivers/spi very shortly. This is good news! I think the driver is ready to move out of staging and would be very happy to see you do it. My only small concern is that this driver was backported to openwrt (4.14 based) and then reverted https://github.com/openwrt/openwrt/commit/749a29f76ca780d8df70a5163d43bbdc6f13ba3f "This breaks some mt7621 devices." Possibly it was backported badly, or possibly there is a problem. John: do you have any more details of the problem other than what is in the commit message? Thanks, NeilBrown
Hi! On Thu, Mar 14, 2019 at 6:14 AM NeilBrown <neil@brown.name> wrote: > > [...] > My only small concern is that this driver was backported to openwrt > (4.14 based) and then reverted > > https://github.com/openwrt/openwrt/commit/749a29f76ca780d8df70a5163d43bbdc6f13ba3f > > "This breaks some mt7621 devices." > > Possibly it was backported badly, or possibly there is a problem. Last time I backported the version with broken SPI modes so it got broken SPI CS1 support. There is one mt7621 router using SPI CS1 in OpenWrt and the backported driver broke it. It has been fixed by my two "drop broken spi modes" patches. > > John: do you have any more details of the problem other than what is in > the commit message? > > Thanks, > NeilBrown Regards, Chuanhong Guo
On Thu, Mar 14 2019, Chuanhong Guo wrote: > Hi! > On Thu, Mar 14, 2019 at 6:14 AM NeilBrown <neil@brown.name> wrote: >> >> [...] >> My only small concern is that this driver was backported to openwrt >> (4.14 based) and then reverted >> >> https://github.com/openwrt/openwrt/commit/749a29f76ca780d8df70a5163d43bbdc6f13ba3f >> >> "This breaks some mt7621 devices." >> >> Possibly it was backported badly, or possibly there is a problem. > Last time I backported the version with broken SPI modes so it got > broken SPI CS1 support. There is one mt7621 router using SPI CS1 in > OpenWrt and the backported driver broke it. > It has been fixed by my two "drop broken spi modes" patches. Ahh, thanks for the clarification. Good to know all known problems are fixed. Thanks, NeilBrown >> >> John: do you have any more details of the problem other than what is in >> the commit message? >> >> Thanks, >> NeilBrown > > Regards, > Chuanhong Guo
My answers are in-line below. BTW bare with me as this is my attempt to get my feet wet in how to contribute to the linux kernel for my own pleasure and interest :) On Wed, Mar 13, 2019 at 03:34:54PM +0300, Dan Carpenter wrote: > On Wed, Mar 13, 2019 at 01:24:04PM +0100, Armando Miraglia wrote: > > Running Lindent on the mt7621-spi.c file in drivers/staging I noticed that the > > file contained style issues. This change attempts to address such style > > problems. > > > > Don't run lindent. I think checkpatch.pl has a --fix option that might > be better, but once the code is merged then our standard become much > higher for follow up patches. > > > Signed-off-by: Armando Miraglia <armax@google.com> > > --- > > NOTE: resend this patch to include all mainteners listed by get_mantainers.pl. > > drivers/staging/mt7621-spi/spi-mt7621.c | 27 +++++++++++++------------ > > 1 file changed, 14 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/staging/mt7621-spi/spi-mt7621.c b/drivers/staging/mt7621-spi/spi-mt7621.c > > index b509f9fe3346..03d53845f8c5 100644 > > --- a/drivers/staging/mt7621-spi/spi-mt7621.c > > +++ b/drivers/staging/mt7621-spi/spi-mt7621.c > > @@ -52,14 +52,14 @@ > > #define MT7621_LSB_FIRST BIT(3) > > > > struct mt7621_spi { > > - struct spi_master *master; > > - void __iomem *base; > > - unsigned int sys_freq; > > - unsigned int speed; > > - struct clk *clk; > > - int pending_write; > > - > > - struct mt7621_spi_ops *ops; > > + struct spi_master *master; > > + void __iomem *base; > > + unsigned int sys_freq; > > + unsigned int speed; > > + struct clk *clk; > > + int pending_write; > > + > > + struct mt7621_spi_ops *ops; > > The original is fine. I don't encourage people to do fancy indenting > with their local variable declarations inside functions but for a struct > the declarations aren't going to change a lot so people can get fancy > if they want. > Is there an explicit intent to deprecate Lindent in favor of checkpatch.pl --fix? If one would like to contribute to fixing the tooling for linting which of the two would be the right target for such an effort? > The problem with a local is if you need to add a new variable then you > have to re-indent a bunch of unrelated lines or have one out of > alignment line. Most people know this intuitively so they don't get > fancy. > > > }; > > > > static inline struct mt7621_spi *spidev_to_mt7621_spi(struct spi_device *spi) > > @@ -303,7 +303,7 @@ static int mt7621_spi_setup(struct spi_device *spi) > > struct mt7621_spi *rs = spidev_to_mt7621_spi(spi); > > > > if ((spi->max_speed_hz == 0) || > > - (spi->max_speed_hz > (rs->sys_freq / 2))) > > + (spi->max_speed_hz > (rs->sys_freq / 2))) > > Yeah. Lindent is correct here. Funny enough, this is something I adjusted manually :) > > spi->max_speed_hz = (rs->sys_freq / 2); > > > > if (spi->max_speed_hz < (rs->sys_freq / 4097)) { > > @@ -316,9 +316,10 @@ static int mt7621_spi_setup(struct spi_device *spi) > > } > > > > static const struct of_device_id mt7621_spi_match[] = { > > - { .compatible = "ralink,mt7621-spi" }, > > + {.compatible = "ralink,mt7621-spi"}, > > The original was better. > > > {}, > > }; > > + > > MODULE_DEVICE_TABLE(of, mt7621_spi_match); > > No need for a blank. These are closely related. Ack. > > > > static int mt7621_spi_probe(struct platform_device *pdev) > > @@ -408,9 +409,9 @@ MODULE_ALIAS("platform:" DRIVER_NAME); > > > > static struct platform_driver mt7621_spi_driver = { > > .driver = { > > - .name = DRIVER_NAME, > > - .of_match_table = mt7621_spi_match, > > - }, > > + .name = DRIVER_NAME, > > + .of_match_table = mt7621_spi_match, > > + }, > > The new indenting is very wrong. Ack. In fact, I was thinking this could be one target to fix the logic in Lindent to do this appropriately. I have a process question here: to post a change for the only accepted change I have in this patch should I send out a new patch? Thanks for the help and the review! Cheers, A.
On Thu, Mar 14, 2019 at 12:13:15PM +0100, Armando Miraglia wrote: > My answers are in-line below. BTW bare with me as this is my attempt to get my > feet wet in how to contribute to the linux kernel for my own pleasure and > interest :) > No problem at all. > Is there an explicit intent to deprecate Lindent in favor of checkpatch.pl > --fix? If one would like to contribute to fixing the tooling for linting which > of the two would be the right target for such an effort? > I've added Jean to the CC list because he worked on Lindent a few years ago. I really think we should just delete Lindent. I haven't used the checkpatch.pl --fix option but Joe Perches has good style. > > > static inline struct mt7621_spi *spidev_to_mt7621_spi(struct spi_device *spi) > > > @@ -303,7 +303,7 @@ static int mt7621_spi_setup(struct spi_device *spi) > > > struct mt7621_spi *rs = spidev_to_mt7621_spi(spi); > > > > > > if ((spi->max_speed_hz == 0) || > > > - (spi->max_speed_hz > (rs->sys_freq / 2))) > > > + (spi->max_speed_hz > (rs->sys_freq / 2))) > > > > Yeah. Lindent is correct here. > > Funny enough, this is something I adjusted manually :) > :) Good. > I have a process question here: to post a change for the only accepted change I > have in this patch should I send out a new patch? > Yeah. If you want. Google for how to send a v2 patch. regards, dan carpenter
Hi Armando, On 14.03.19 12:13, Armando Miraglia wrote: > My answers are in-line below. BTW bare with me as this is my attempt to get my > feet wet in how to contribute to the linux kernel for my own pleasure and > interest :) > > On Wed, Mar 13, 2019 at 03:34:54PM +0300, Dan Carpenter wrote: >> On Wed, Mar 13, 2019 at 01:24:04PM +0100, Armando Miraglia wrote: >>> Running Lindent on the mt7621-spi.c file in drivers/staging I noticed that the >>> file contained style issues. This change attempts to address such style >>> problems. >>> >> >> Don't run lindent. I think checkpatch.pl has a --fix option that might >> be better, but once the code is merged then our standard become much >> higher for follow up patches. >> >>> Signed-off-by: Armando Miraglia <armax@google.com> >>> --- >>> NOTE: resend this patch to include all mainteners listed by get_mantainers.pl. >>> drivers/staging/mt7621-spi/spi-mt7621.c | 27 +++++++++++++------------ >>> 1 file changed, 14 insertions(+), 13 deletions(-) >>> >>> diff --git a/drivers/staging/mt7621-spi/spi-mt7621.c b/drivers/staging/mt7621-spi/spi-mt7621.c >>> index b509f9fe3346..03d53845f8c5 100644 >>> --- a/drivers/staging/mt7621-spi/spi-mt7621.c >>> +++ b/drivers/staging/mt7621-spi/spi-mt7621.c >>> @@ -52,14 +52,14 @@ >>> #define MT7621_LSB_FIRST BIT(3) >>> >>> struct mt7621_spi { >>> - struct spi_master *master; >>> - void __iomem *base; >>> - unsigned int sys_freq; >>> - unsigned int speed; >>> - struct clk *clk; >>> - int pending_write; >>> - >>> - struct mt7621_spi_ops *ops; >>> + struct spi_master *master; >>> + void __iomem *base; >>> + unsigned int sys_freq; >>> + unsigned int speed; >>> + struct clk *clk; >>> + int pending_write; >>> + >>> + struct mt7621_spi_ops *ops; >> >> The original is fine. I don't encourage people to do fancy indenting >> with their local variable declarations inside functions but for a struct >> the declarations aren't going to change a lot so people can get fancy >> if they want. >> > Is there an explicit intent to deprecate Lindent in favor of checkpatch.pl > --fix? If one would like to contribute to fixing the tooling for linting which > of the two would be the right target for such an effort? > >> The problem with a local is if you need to add a new variable then you >> have to re-indent a bunch of unrelated lines or have one out of >> alignment line. Most people know this intuitively so they don't get >> fancy. >> >>> }; >>> >>> static inline struct mt7621_spi *spidev_to_mt7621_spi(struct spi_device *spi) >>> @@ -303,7 +303,7 @@ static int mt7621_spi_setup(struct spi_device *spi) >>> struct mt7621_spi *rs = spidev_to_mt7621_spi(spi); >>> >>> if ((spi->max_speed_hz == 0) || >>> - (spi->max_speed_hz > (rs->sys_freq / 2))) >>> + (spi->max_speed_hz > (rs->sys_freq / 2))) >> >> Yeah. Lindent is correct here. > > Funny enough, this is something I adjusted manually :) > >>> spi->max_speed_hz = (rs->sys_freq / 2); >>> >>> if (spi->max_speed_hz < (rs->sys_freq / 4097)) { >>> @@ -316,9 +316,10 @@ static int mt7621_spi_setup(struct spi_device *spi) >>> } >>> >>> static const struct of_device_id mt7621_spi_match[] = { >>> - { .compatible = "ralink,mt7621-spi" }, >>> + {.compatible = "ralink,mt7621-spi"}, >> >> The original was better. >> >>> {}, >>> }; >>> + >>> MODULE_DEVICE_TABLE(of, mt7621_spi_match); >> >> No need for a blank. These are closely related. > > Ack. > >>> >>> static int mt7621_spi_probe(struct platform_device *pdev) >>> @@ -408,9 +409,9 @@ MODULE_ALIAS("platform:" DRIVER_NAME); >>> >>> static struct platform_driver mt7621_spi_driver = { >>> .driver = { >>> - .name = DRIVER_NAME, >>> - .of_match_table = mt7621_spi_match, >>> - }, >>> + .name = DRIVER_NAME, >>> + .of_match_table = mt7621_spi_match, >>> + }, >> >> The new indenting is very wrong. > > Ack. In fact, I was thinking this could be one target to fix the logic in > Lindent to do this appropriately. > > I have a process question here: to post a change for the only accepted change I > have in this patch should I send out a new patch? Would it be possible for you to wait a bit with this minor cleanup? As I'm preparing a patch to move this driver out of staging right now. You can definitely follow-up with your cleanup, once this move is done. Otherwise the move might be delayed even more. Thanks, Stefan
Absolutely! Cheers, A. On Thu, Mar 14, 2019 at 12:36 PM Stefan Roese <sr@denx.de> wrote: > > Hi Armando, > > On 14.03.19 12:13, Armando Miraglia wrote: > > My answers are in-line below. BTW bare with me as this is my attempt to get my > > feet wet in how to contribute to the linux kernel for my own pleasure and > > interest :) > > > > On Wed, Mar 13, 2019 at 03:34:54PM +0300, Dan Carpenter wrote: > >> On Wed, Mar 13, 2019 at 01:24:04PM +0100, Armando Miraglia wrote: > >>> Running Lindent on the mt7621-spi.c file in drivers/staging I noticed that the > >>> file contained style issues. This change attempts to address such style > >>> problems. > >>> > >> > >> Don't run lindent. I think checkpatch.pl has a --fix option that might > >> be better, but once the code is merged then our standard become much > >> higher for follow up patches. > >> > >>> Signed-off-by: Armando Miraglia <armax@google.com> > >>> --- > >>> NOTE: resend this patch to include all mainteners listed by get_mantainers.pl. > >>> drivers/staging/mt7621-spi/spi-mt7621.c | 27 +++++++++++++------------ > >>> 1 file changed, 14 insertions(+), 13 deletions(-) > >>> > >>> diff --git a/drivers/staging/mt7621-spi/spi-mt7621.c b/drivers/staging/mt7621-spi/spi-mt7621.c > >>> index b509f9fe3346..03d53845f8c5 100644 > >>> --- a/drivers/staging/mt7621-spi/spi-mt7621.c > >>> +++ b/drivers/staging/mt7621-spi/spi-mt7621.c > >>> @@ -52,14 +52,14 @@ > >>> #define MT7621_LSB_FIRST BIT(3) > >>> > >>> struct mt7621_spi { > >>> - struct spi_master *master; > >>> - void __iomem *base; > >>> - unsigned int sys_freq; > >>> - unsigned int speed; > >>> - struct clk *clk; > >>> - int pending_write; > >>> - > >>> - struct mt7621_spi_ops *ops; > >>> + struct spi_master *master; > >>> + void __iomem *base; > >>> + unsigned int sys_freq; > >>> + unsigned int speed; > >>> + struct clk *clk; > >>> + int pending_write; > >>> + > >>> + struct mt7621_spi_ops *ops; > >> > >> The original is fine. I don't encourage people to do fancy indenting > >> with their local variable declarations inside functions but for a struct > >> the declarations aren't going to change a lot so people can get fancy > >> if they want. > >> > > Is there an explicit intent to deprecate Lindent in favor of checkpatch.pl > > --fix? If one would like to contribute to fixing the tooling for linting which > > of the two would be the right target for such an effort? > > > >> The problem with a local is if you need to add a new variable then you > >> have to re-indent a bunch of unrelated lines or have one out of > >> alignment line. Most people know this intuitively so they don't get > >> fancy. > >> > >>> }; > >>> > >>> static inline struct mt7621_spi *spidev_to_mt7621_spi(struct spi_device *spi) > >>> @@ -303,7 +303,7 @@ static int mt7621_spi_setup(struct spi_device *spi) > >>> struct mt7621_spi *rs = spidev_to_mt7621_spi(spi); > >>> > >>> if ((spi->max_speed_hz == 0) || > >>> - (spi->max_speed_hz > (rs->sys_freq / 2))) > >>> + (spi->max_speed_hz > (rs->sys_freq / 2))) > >> > >> Yeah. Lindent is correct here. > > > > Funny enough, this is something I adjusted manually :) > > > >>> spi->max_speed_hz = (rs->sys_freq / 2); > >>> > >>> if (spi->max_speed_hz < (rs->sys_freq / 4097)) { > >>> @@ -316,9 +316,10 @@ static int mt7621_spi_setup(struct spi_device *spi) > >>> } > >>> > >>> static const struct of_device_id mt7621_spi_match[] = { > >>> - { .compatible = "ralink,mt7621-spi" }, > >>> + {.compatible = "ralink,mt7621-spi"}, > >> > >> The original was better. > >> > >>> {}, > >>> }; > >>> + > >>> MODULE_DEVICE_TABLE(of, mt7621_spi_match); > >> > >> No need for a blank. These are closely related. > > > > Ack. > > > >>> > >>> static int mt7621_spi_probe(struct platform_device *pdev) > >>> @@ -408,9 +409,9 @@ MODULE_ALIAS("platform:" DRIVER_NAME); > >>> > >>> static struct platform_driver mt7621_spi_driver = { > >>> .driver = { > >>> - .name = DRIVER_NAME, > >>> - .of_match_table = mt7621_spi_match, > >>> - }, > >>> + .name = DRIVER_NAME, > >>> + .of_match_table = mt7621_spi_match, > >>> + }, > >> > >> The new indenting is very wrong. > > > > Ack. In fact, I was thinking this could be one target to fix the logic in > > Lindent to do this appropriately. > > > > I have a process question here: to post a change for the only accepted change I > > have in this patch should I send out a new patch? > > Would it be possible for you to wait a bit with this minor cleanup? > As I'm preparing a patch to move this driver out of staging right > now. You can definitely follow-up with your cleanup, once this move > is done. Otherwise the move might be delayed even more. > > Thanks, > Stefan
On 14/03/2019 12:37, Armando Miraglia wrote: > Absolutely! Please don't top post :) > > Cheers, > A. > > On Thu, Mar 14, 2019 at 12:36 PM Stefan Roese <sr@denx.de> wrote: [...] >> >> Would it be possible for you to wait a bit with this minor cleanup? >> As I'm preparing a patch to move this driver out of staging right >> now. You can definitely follow-up with your cleanup, once this move >> is done. Otherwise the move might be delayed even more. >> Hm but shouldn't style issues be a criteria for not accepting a move out of staging? I think so. You could add Armandos patch in your series or rebase your series against Greg's tree, once he took the clean-up. Normally Greg is incredibly fast :) Regards, Matthias
On 14.03.19 14:14, Matthias Brugger wrote: > > > On 14/03/2019 12:37, Armando Miraglia wrote: >> Absolutely! > > Please don't top post :) > >> >> Cheers, >> A. >> >> On Thu, Mar 14, 2019 at 12:36 PM Stefan Roese <sr@denx.de> wrote: > [...] >>> >>> Would it be possible for you to wait a bit with this minor cleanup? >>> As I'm preparing a patch to move this driver out of staging right >>> now. You can definitely follow-up with your cleanup, once this move >>> is done. Otherwise the move might be delayed even more. >>> > > Hm but shouldn't style issues be a criteria for not accepting a move out of > staging? I would agree, if those style issues where non trivial. In the end we are talking about one non-optimal identation now. > I think so. You could add Armandos patch in your series or rebase your > series against Greg's tree, once he took the clean-up. Normally Greg is > incredibly fast :) I should have included the history here to make this more clean. I've started pulling this driver out of staging a few weeks ago: https://patchwork.kernel.org/patch/10790537/ ... Here you find Greg's comment, that the style patches should be merged first before the move out of staging. This is what I worked on after this first patch series: https://patchwork.kernel.org/patch/10792455/ ... Now these 9 style issue patches from me have been merged and I would like to proceed with the driver move. Thanks, Stefan
Hi Dan, On Thu, 14 Mar 2019 14:27:32 +0300, Dan Carpenter wrote: > On Thu, Mar 14, 2019 at 12:13:15PM +0100, Armando Miraglia wrote: > > Is there an explicit intent to deprecate Lindent in favor of checkpatch.pl > > --fix? If one would like to contribute to fixing the tooling for linting which > > of the two would be the right target for such an effort? > > I've added Jean to the CC list because he worked on Lindent a few > years ago. I really think we should just delete Lindent. I haven't > used the checkpatch.pl --fix option but Joe Perches has good style. I merely fixed obvious but minor issues in Lindent as I stumbled upon them the one time in my life I used that script. That was years ago. I'm not using it on a regular basis. My principle is that if a script is present in the kernel tree then it can and should be maintained. If it is deemed not worth the maintenance effort then it should be deleted. I don't care either way.
On 14/03/2019 14:24, Stefan Roese wrote: > On 14.03.19 14:14, Matthias Brugger wrote: >> >> >> On 14/03/2019 12:37, Armando Miraglia wrote: >>> Absolutely! >> >> Please don't top post :) >> >>> >>> Cheers, >>> A. >>> >>> On Thu, Mar 14, 2019 at 12:36 PM Stefan Roese <sr@denx.de> wrote: >> [...] >>>> >>>> Would it be possible for you to wait a bit with this minor cleanup? >>>> As I'm preparing a patch to move this driver out of staging right >>>> now. You can definitely follow-up with your cleanup, once this move >>>> is done. Otherwise the move might be delayed even more. >>>> >> >> Hm but shouldn't style issues be a criteria for not accepting a move out of >> staging? > > I would agree, if those style issues where non trivial. In the end we > are talking about one non-optimal identation now.> Fair enough, anyway I'm not the person to decide on that :) Regards, Matthias >> I think so. You could add Armandos patch in your series or rebase your >> series against Greg's tree, once he took the clean-up. Normally Greg is >> incredibly fast :) > > I should have included the history here to make this more clean. I've > started pulling this driver out of staging a few weeks ago: > > https://patchwork.kernel.org/patch/10790537/ > ... > > Here you find Greg's comment, that the style patches should be merged > first before the move out of staging. This is what I worked on after > this first patch series: > > https://patchwork.kernel.org/patch/10792455/ > ... > > Now these 9 style issue patches from me have been merged and I would > like to proceed with the driver move. > > Thanks, > Stefan
On Thu, 2019-03-14 at 15:07 +0100, Jean Delvare wrote: > My principle is that if a script > is present in the kernel tree then it can and should be maintained. If > it is deemed not worth the maintenance effort then it should be > deleted. I've suggested deleting Lindent in the past. https://lkml.org/lkml/2013/2/11/390 Also there is now the clang-format tool that does an OK job of reflowing source to something approximating the typical kernel style. See: Documentation/process/clang-format.rst
diff --git a/drivers/staging/mt7621-spi/spi-mt7621.c b/drivers/staging/mt7621-spi/spi-mt7621.c index b509f9fe3346..03d53845f8c5 100644 --- a/drivers/staging/mt7621-spi/spi-mt7621.c +++ b/drivers/staging/mt7621-spi/spi-mt7621.c @@ -52,14 +52,14 @@ #define MT7621_LSB_FIRST BIT(3) struct mt7621_spi { - struct spi_master *master; - void __iomem *base; - unsigned int sys_freq; - unsigned int speed; - struct clk *clk; - int pending_write; - - struct mt7621_spi_ops *ops; + struct spi_master *master; + void __iomem *base; + unsigned int sys_freq; + unsigned int speed; + struct clk *clk; + int pending_write; + + struct mt7621_spi_ops *ops; }; static inline struct mt7621_spi *spidev_to_mt7621_spi(struct spi_device *spi) @@ -303,7 +303,7 @@ static int mt7621_spi_setup(struct spi_device *spi) struct mt7621_spi *rs = spidev_to_mt7621_spi(spi); if ((spi->max_speed_hz == 0) || - (spi->max_speed_hz > (rs->sys_freq / 2))) + (spi->max_speed_hz > (rs->sys_freq / 2))) spi->max_speed_hz = (rs->sys_freq / 2); if (spi->max_speed_hz < (rs->sys_freq / 4097)) { @@ -316,9 +316,10 @@ static int mt7621_spi_setup(struct spi_device *spi) } static const struct of_device_id mt7621_spi_match[] = { - { .compatible = "ralink,mt7621-spi" }, + {.compatible = "ralink,mt7621-spi"}, {}, }; + MODULE_DEVICE_TABLE(of, mt7621_spi_match); static int mt7621_spi_probe(struct platform_device *pdev) @@ -408,9 +409,9 @@ MODULE_ALIAS("platform:" DRIVER_NAME); static struct platform_driver mt7621_spi_driver = { .driver = { - .name = DRIVER_NAME, - .of_match_table = mt7621_spi_match, - }, + .name = DRIVER_NAME, + .of_match_table = mt7621_spi_match, + }, .probe = mt7621_spi_probe, .remove = mt7621_spi_remove, };
Running Lindent on the mt7621-spi.c file in drivers/staging I noticed that the file contained style issues. This change attempts to address such style problems. Signed-off-by: Armando Miraglia <armax@google.com> --- NOTE: resend this patch to include all mainteners listed by get_mantainers.pl. drivers/staging/mt7621-spi/spi-mt7621.c | 27 +++++++++++++------------ 1 file changed, 14 insertions(+), 13 deletions(-)