| Message ID | 20170925161637.D7471440058@finisterre.ee.mobilebroadband (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hi Mark, On Mon, Sep 25, 2017 at 6:16 PM, Mark Brown <broonie@kernel.org> wrote: > The patch > > spi: sh-msiof: Add compatible strings for r8a774[35] > > has been applied to the spi tree at > > git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git > > 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. Please drop this patch, as there's no need to add explicit matching for these compatible values. The family-specific compatible values (which the driver already matches against) are sufficient. Thanks! > From bdacfc7b6216dd30d07c10732fd4c0a660c62853 Mon Sep 17 00:00:00 2001 > From: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > Date: Mon, 25 Sep 2017 09:54:19 +0100 > Subject: [PATCH] spi: sh-msiof: Add compatible strings for r8a774[35] > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > Signed-off-by: Mark Brown <broonie@kernel.org> > --- > drivers/spi/spi-sh-msiof.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c > index 0eb1e9583485..00808448cf35 100644 > --- a/drivers/spi/spi-sh-msiof.c > +++ b/drivers/spi/spi-sh-msiof.c > @@ -1021,6 +1021,8 @@ static const struct sh_msiof_chipdata rcar_gen3_data = { > > static const struct of_device_id sh_msiof_match[] = { > { .compatible = "renesas,sh-mobile-msiof", .data = &sh_data }, > + { .compatible = "renesas,msiof-r8a7743", .data = &rcar_gen2_data }, > + { .compatible = "renesas,msiof-r8a7745", .data = &rcar_gen2_data }, > { .compatible = "renesas,msiof-r8a7790", .data = &rcar_gen2_data }, > { .compatible = "renesas,msiof-r8a7791", .data = &rcar_gen2_data }, > { .compatible = "renesas,msiof-r8a7792", .data = &rcar_gen2_data }, Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Mon, Sep 25, 2017 at 07:48:44PM +0200, Geert Uytterhoeven wrote: > Please drop this patch, as there's no need to add explicit matching for these > compatible values. The family-specific compatible values (which the driver > already matches against) are sufficient. While the patch is not needed if people list the fallback property it also does no harm and provides a marginal documentation benefit in saying that someone has considered if any special handling is useful and decided that it isn't.
Hi Mark, On Mon, Sep 25, 2017 at 8:45 PM, Mark Brown <broonie@kernel.org> wrote: > On Mon, Sep 25, 2017 at 07:48:44PM +0200, Geert Uytterhoeven wrote: >> Please drop this patch, as there's no need to add explicit matching for these >> compatible values. The family-specific compatible values (which the driver >> already matches against) are sufficient. > > While the patch is not needed if people list the fallback property it > also does no harm and provides a marginal documentation benefit in > saying that someone has considered if any special handling is useful and > decided that it isn't. All true. My rebuttal is threefold: 1. Listing the fallback property is mandatory for new SoCs. We only keep the per-SoC compatible values in the driver for older SoCs that predate the introduction of fallback properties. 2. Some harm is involved, in the form of increased kernel image size. 3. When updating DT bindings for new SoCs, we usually add "No driver update is needed" to the patch description to clarify. Unfortunately that was missed here. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Mark, I am very sorry about the confusion. Geert has a good point so please drop this patch. Thanks, Fabrizio > -----Original Message----- > From: geert.uytterhoeven@gmail.com [mailto:geert.uytterhoeven@gmail.com] On Behalf Of Geert Uytterhoeven > Sent: 25 September 2017 20:16 > To: Mark Brown <broonie@kernel.org> > Cc: Fabrizio Castro <fabrizio.castro@bp.renesas.com>; Rob Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; > linux-spi <linux-spi@vger.kernel.org>; devicetree@vger.kernel.org; Linux-Renesas <linux-renesas-soc@vger.kernel.org>; Chris > Paterson <Chris.Paterson2@renesas.com>; Biju Das <biju.das@bp.renesas.com> > Subject: Re: Applied "spi: sh-msiof: Add compatible strings for r8a774[35]" to the spi tree > > Hi Mark, > > On Mon, Sep 25, 2017 at 8:45 PM, Mark Brown <broonie@kernel.org> wrote: > > On Mon, Sep 25, 2017 at 07:48:44PM +0200, Geert Uytterhoeven wrote: > >> Please drop this patch, as there's no need to add explicit matching for these > >> compatible values. The family-specific compatible values (which the driver > >> already matches against) are sufficient. > > > > While the patch is not needed if people list the fallback property it > > also does no harm and provides a marginal documentation benefit in > > saying that someone has considered if any special handling is useful and > > decided that it isn't. > > All true. > > My rebuttal is threefold: > 1. Listing the fallback property is mandatory for new SoCs. We only keep > the per-SoC compatible values in the driver for older SoCs that predate > the introduction of fallback properties. > 2. Some harm is involved, in the form of increased kernel image size. > 3. When updating DT bindings for new SoCs, we usually add "No driver > update is needed" to the patch description to clarify. Unfortunately > that was missed here. > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
On Mon, Sep 25, 2017 at 09:15:53PM +0200, Geert Uytterhoeven wrote: > My rebuttal is threefold: > 1. Listing the fallback property is mandatory for new SoCs. We only keep > the per-SoC compatible values in the driver for older SoCs that predate > the introduction of fallback properties. So good practice is always good practice, but I'd argue that it's not bad practice to also explicitly enumerate all the documented compatibles in the driver. Being liberal in what you accept and all that. > 2. Some harm is involved, in the form of increased kernel image size. Is this really a meaningful impact? > 3. When updating DT bindings for new SoCs, we usually add "No driver > update is needed" to the patch description to clarify. Unfortunately > that was missed here. That's basically the same good practice thing, it's just documenting what you're trying to do here with not putting things in code but writing things in changelogs doesn't make them so!
Hi Mark, Geert, everyone, On Wed, Sep 27, 2017 at 2:30 AM, Mark Brown <broonie@kernel.org> wrote: > On Mon, Sep 25, 2017 at 09:15:53PM +0200, Geert Uytterhoeven wrote: > >> My rebuttal is threefold: >> 1. Listing the fallback property is mandatory for new SoCs. We only keep >> the per-SoC compatible values in the driver for older SoCs that predate >> the introduction of fallback properties. > > So good practice is always good practice, but I'd argue that it's not > bad practice to also explicitly enumerate all the documented compatibles > in the driver. Being liberal in what you accept and all that. > >> 2. Some harm is involved, in the form of increased kernel image size. > > Is this really a meaningful impact? > >> 3. When updating DT bindings for new SoCs, we usually add "No driver >> update is needed" to the patch description to clarify. Unfortunately >> that was missed here. > > That's basically the same good practice thing, it's just documenting > what you're trying to do here with not putting things in code but > writing things in changelogs doesn't make them so! My opinion that good practice is to document the per-driver supported hardware by explicitly listing the per-SoC compat strings in the DT binding document. Then exactly which compat strings the driver matches on is really part of the software implementation and it will most likely vary over time. So my view is that only updating the DT binding document should be enough in most cases when fall-back compat strings are used. I guess other people see it differently? Is it too much detail to let the MAINTAINERS file point out both driver files and the DT binding files? Cheers, / magnus
On Wed, Sep 27, 2017 at 02:36:01PM +0900, Magnus Damm wrote: > On Wed, Sep 27, 2017 at 2:30 AM, Mark Brown <broonie@kernel.org> wrote: > > On Mon, Sep 25, 2017 at 09:15:53PM +0200, Geert Uytterhoeven wrote: > >> 3. When updating DT bindings for new SoCs, we usually add "No driver > >> update is needed" to the patch description to clarify. Unfortunately > >> that was missed here. > > That's basically the same good practice thing, it's just documenting > > what you're trying to do here with not putting things in code but > > writing things in changelogs doesn't make them so! > My opinion that good practice is to document the per-driver supported > hardware by explicitly listing the per-SoC compat strings in the DT > binding document. > Then exactly which compat strings the driver matches on is really part > of the software implementation and it will most likely vary over time. > So my view is that only updating the DT binding document should be > enough in most cases when fall-back compat strings are used. I guess > other people see it differently? > Is it too much detail to let the MAINTAINERS file point out both > driver files and the DT binding files? It's supposed to do that already. All I'm saying here is that the patch doesn't seem like something that needs actively reverting since it's a perfectly valid and even potentially useful change to make.
Hi Mark, On Thu, Sep 28, 2017 at 2:01 AM, Mark Brown <broonie@kernel.org> wrote: > On Wed, Sep 27, 2017 at 02:36:01PM +0900, Magnus Damm wrote: >> On Wed, Sep 27, 2017 at 2:30 AM, Mark Brown <broonie@kernel.org> wrote: >> > On Mon, Sep 25, 2017 at 09:15:53PM +0200, Geert Uytterhoeven wrote: > >> >> 3. When updating DT bindings for new SoCs, we usually add "No driver >> >> update is needed" to the patch description to clarify. Unfortunately >> >> that was missed here. > >> > That's basically the same good practice thing, it's just documenting >> > what you're trying to do here with not putting things in code but >> > writing things in changelogs doesn't make them so! > >> My opinion that good practice is to document the per-driver supported >> hardware by explicitly listing the per-SoC compat strings in the DT >> binding document. > >> Then exactly which compat strings the driver matches on is really part >> of the software implementation and it will most likely vary over time. > >> So my view is that only updating the DT binding document should be >> enough in most cases when fall-back compat strings are used. I guess >> other people see it differently? > >> Is it too much detail to let the MAINTAINERS file point out both >> driver files and the DT binding files? > > It's supposed to do that already. All I'm saying here is that the patch > doesn't seem like something that needs actively reverting since it's a > perfectly valid and even potentially useful change to make. Thanks for clarifying the inclusion of DT bindings in the MAINTAINER file. To escape any sort of conflict I will happily let you and Geert discuss what is potentially useful or not! =) Nevertheless I agree that reverting this rather harmless change seems a bit overly aggressive. Cheers, / magnus
diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c index 0eb1e9583485..00808448cf35 100644 --- a/drivers/spi/spi-sh-msiof.c +++ b/drivers/spi/spi-sh-msiof.c @@ -1021,6 +1021,8 @@ static const struct sh_msiof_chipdata rcar_gen3_data = { static const struct of_device_id sh_msiof_match[] = { { .compatible = "renesas,sh-mobile-msiof", .data = &sh_data }, + { .compatible = "renesas,msiof-r8a7743", .data = &rcar_gen2_data }, + { .compatible = "renesas,msiof-r8a7745", .data = &rcar_gen2_data }, { .compatible = "renesas,msiof-r8a7790", .data = &rcar_gen2_data }, { .compatible = "renesas,msiof-r8a7791", .data = &rcar_gen2_data }, { .compatible = "renesas,msiof-r8a7792", .data = &rcar_gen2_data },