Re: [RFC PATCH 2/2] dt: spi: s3c64xx: add compatible to controller-data
On Thu, Jul 30, 2015 at 09:50:51AM +0200, Boris Brezillon wrote: > Since you have to patch your DTs anyway, how about putting your > partitions in a subnode and patch the ofpart code to parse this subnode > if it is present (see the following patch). This is the best idea, yes - if we're changing the DT for the system anyway then making a sane binding and using that seems better than trying to mitigate problems with the old bindings. signature.asc Description: Digital signature
Re: [RFC PATCH 2/2] dt: spi: s3c64xx: add compatible to controller-data
On Thu, Jul 30, 2015 at 10:24:37AM +0200, Michal Suchanek wrote: > So my suggestion is to > 1) search of ofpart subnode in mtd node. If present read address and > reg from it and search partitions as subnodes of the ofpart node. In > this case unknown nodes can cause error. > 2) failing that issue a warning and try to parse ofpart partitions > from the mtd node itself. In this case unknown nodes cannot cause > error for compatibility with other drivers including the already > exisitn s3c64xx controller-data node. > The parser code can be the same for both cases and only operate on > different node with a flag to reject unknown subnodes or not. That seems reasonable to me. signature.asc Description: Digital signature
Re: [RFC PATCH 2/2] dt: spi: s3c64xx: add compatible to controller-data
On 29 July 2015 at 20:40, Mark Brown wrote: > On Wed, Jul 29, 2015 at 08:21:34PM +0200, Michal Suchanek wrote: >> On 29 July 2015 at 19:16, Mark Brown wrote: > >> >> It will not break anything. It will just spam dmesg. > >> > I'm confused - if all this change does is to spam dmesg then what's the >> > point? > >> Presumably when your SPI NOR flash fails to probe this message will be >> just above and you will look into the binding doc and add the >> compatible. > > If you're looking to add a warning message when the flash device fails > to probe then add that in the flash driver when it detects an error, > this will cause needless noise for everyone else using this controller > purely to work around the broken binding. Technically nobody needs to see the warning with in-tree boards since the dts can be amended with the compatible. There is no error in flash device driver. There is only error parsing partition scheme. In my opinion this should never cause an error. With disk drives failure to parse partition table results in unpartitioned disk. As there are number of partitioning schemes failure to parse one still does not prevent other in succeding. > > And like I say compatible really seems like it isn't an appropriate > property here. So to sum up the discussion adding compatible to s3c64xx controller-data is not desirable and making ofpart more robust is desirable. I think the suggestion to use a subnode for ofpart gives the most robust solution. Even adding compatibles to the partition subnodes ofpart still monopolizes the address and cells property of the mtd node which does not pass the 'if another driver did the same would it work together?' test. So my suggestion is to 1) search of ofpart subnode in mtd node. If present read address and reg from it and search partitions as subnodes of the ofpart node. In this case unknown nodes can cause error. 2) failing that issue a warning and try to parse ofpart partitions from the mtd node itself. In this case unknown nodes cannot cause error for compatibility with other drivers including the already exisitn s3c64xx controller-data node. The parser code can be the same for both cases and only operate on different node with a flag to reject unknown subnodes or not. Thanks Michal -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 2/2] dt: spi: s3c64xx: add compatible to controller-data
Hi Michal, On Wed, 29 Jul 2015 12:19:57 +0200 Michal Suchanek wrote: > The controller-data subnode has no compatible. This can lead to other > drivers getting confused by it. Add a compatible to make devicetreee > unambiguous. > > Signed-off-by: Michal Suchanek > --- > Documentation/devicetree/bindings/spi/spi-samsung.txt | 3 +++ > drivers/spi/spi-s3c64xx.c | 4 > 2 files changed, 7 insertions(+) > > diff --git a/Documentation/devicetree/bindings/spi/spi-samsung.txt > b/Documentation/devicetree/bindings/spi/spi-samsung.txt > index 6dbdeb3..b1e98d1 100644 > --- a/Documentation/devicetree/bindings/spi/spi-samsung.txt > +++ b/Documentation/devicetree/bindings/spi/spi-samsung.txt > @@ -92,16 +92,19 @@ Example: > spi-max-frequency = <1>; > > controller-data { > + compatible = "samsung,s3c-controller-data"; AFAIK compatible should (as much as possible :-)) only be used to encode HW type, and here you're using it to expose what you want to do with your data in Linux. > samsung,spi-feedback-delay = <0>; > }; > > partition@0 { > + compatible = "linux,ofpart-partition"; Ditto. Since you have to patch your DTs anyway, how about putting your partitions in a subnode and patch the ofpart code to parse this subnode if it is present (see the following patch). Best Regards, Boris --- >8 --- >From e342860932bda3a6354a0a6e17540db5c85a14e0 Mon Sep 17 00:00:00 2001 From: Boris Brezillon Date: Thu, 30 Jul 2015 09:44:07 +0200 Subject: [PATCH] mtd: ofpart: search for a partitions node The DT partition parser currently assumes the parition definitions are directly stored in the MTD device node, but sometime this device node contains other child nodes used to store device specific information. Search for a partitions subnode containing the partition definitions, if it is not there, parse the definitions in the device node. Signed-off-by: Boris Brezillon --- drivers/mtd/ofpart.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/mtd/ofpart.c b/drivers/mtd/ofpart.c index 8a06cfb..ba52b88 100644 --- a/drivers/mtd/ofpart.c +++ b/drivers/mtd/ofpart.c @@ -38,7 +38,10 @@ static int parse_ofpart_partitions(struct mtd_info *master, if (!data) return 0; - node = data->of_node; + node = of_get_child_by_name(data->of_node, "partitions"); + if (!node) + node = data->of_node; + if (!node) return 0; -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 2/2] dt: spi: s3c64xx: add compatible to controller-data
On 29 July 2015 at 20:40, Mark Brown broo...@kernel.org wrote: On Wed, Jul 29, 2015 at 08:21:34PM +0200, Michal Suchanek wrote: On 29 July 2015 at 19:16, Mark Brown broo...@kernel.org wrote: It will not break anything. It will just spam dmesg. I'm confused - if all this change does is to spam dmesg then what's the point? Presumably when your SPI NOR flash fails to probe this message will be just above and you will look into the binding doc and add the compatible. If you're looking to add a warning message when the flash device fails to probe then add that in the flash driver when it detects an error, this will cause needless noise for everyone else using this controller purely to work around the broken binding. Technically nobody needs to see the warning with in-tree boards since the dts can be amended with the compatible. There is no error in flash device driver. There is only error parsing partition scheme. In my opinion this should never cause an error. With disk drives failure to parse partition table results in unpartitioned disk. As there are number of partitioning schemes failure to parse one still does not prevent other in succeding. And like I say compatible really seems like it isn't an appropriate property here. So to sum up the discussion adding compatible to s3c64xx controller-data is not desirable and making ofpart more robust is desirable. I think the suggestion to use a subnode for ofpart gives the most robust solution. Even adding compatibles to the partition subnodes ofpart still monopolizes the address and cells property of the mtd node which does not pass the 'if another driver did the same would it work together?' test. So my suggestion is to 1) search of ofpart subnode in mtd node. If present read address and reg from it and search partitions as subnodes of the ofpart node. In this case unknown nodes can cause error. 2) failing that issue a warning and try to parse ofpart partitions from the mtd node itself. In this case unknown nodes cannot cause error for compatibility with other drivers including the already exisitn s3c64xx controller-data node. The parser code can be the same for both cases and only operate on different node with a flag to reject unknown subnodes or not. Thanks Michal -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 2/2] dt: spi: s3c64xx: add compatible to controller-data
Hi Michal, On Wed, 29 Jul 2015 12:19:57 +0200 Michal Suchanek hramr...@gmail.com wrote: The controller-data subnode has no compatible. This can lead to other drivers getting confused by it. Add a compatible to make devicetreee unambiguous. Signed-off-by: Michal Suchanek hramr...@gmail.com --- Documentation/devicetree/bindings/spi/spi-samsung.txt | 3 +++ drivers/spi/spi-s3c64xx.c | 4 2 files changed, 7 insertions(+) diff --git a/Documentation/devicetree/bindings/spi/spi-samsung.txt b/Documentation/devicetree/bindings/spi/spi-samsung.txt index 6dbdeb3..b1e98d1 100644 --- a/Documentation/devicetree/bindings/spi/spi-samsung.txt +++ b/Documentation/devicetree/bindings/spi/spi-samsung.txt @@ -92,16 +92,19 @@ Example: spi-max-frequency = 1; controller-data { + compatible = samsung,s3c-controller-data; AFAIK compatible should (as much as possible :-)) only be used to encode HW type, and here you're using it to expose what you want to do with your data in Linux. samsung,spi-feedback-delay = 0; }; partition@0 { + compatible = linux,ofpart-partition; Ditto. Since you have to patch your DTs anyway, how about putting your partitions in a subnode and patch the ofpart code to parse this subnode if it is present (see the following patch). Best Regards, Boris --- 8 --- From e342860932bda3a6354a0a6e17540db5c85a14e0 Mon Sep 17 00:00:00 2001 From: Boris Brezillon boris.brezil...@free-electrons.com Date: Thu, 30 Jul 2015 09:44:07 +0200 Subject: [PATCH] mtd: ofpart: search for a partitions node The DT partition parser currently assumes the parition definitions are directly stored in the MTD device node, but sometime this device node contains other child nodes used to store device specific information. Search for a partitions subnode containing the partition definitions, if it is not there, parse the definitions in the device node. Signed-off-by: Boris Brezillon boris.brezil...@free-electrons.com --- drivers/mtd/ofpart.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/mtd/ofpart.c b/drivers/mtd/ofpart.c index 8a06cfb..ba52b88 100644 --- a/drivers/mtd/ofpart.c +++ b/drivers/mtd/ofpart.c @@ -38,7 +38,10 @@ static int parse_ofpart_partitions(struct mtd_info *master, if (!data) return 0; - node = data-of_node; + node = of_get_child_by_name(data-of_node, partitions); + if (!node) + node = data-of_node; + if (!node) return 0; -- 1.9.1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 2/2] dt: spi: s3c64xx: add compatible to controller-data
On Thu, Jul 30, 2015 at 09:50:51AM +0200, Boris Brezillon wrote: Since you have to patch your DTs anyway, how about putting your partitions in a subnode and patch the ofpart code to parse this subnode if it is present (see the following patch). This is the best idea, yes - if we're changing the DT for the system anyway then making a sane binding and using that seems better than trying to mitigate problems with the old bindings. signature.asc Description: Digital signature
Re: [RFC PATCH 2/2] dt: spi: s3c64xx: add compatible to controller-data
On Thu, Jul 30, 2015 at 10:24:37AM +0200, Michal Suchanek wrote: So my suggestion is to 1) search of ofpart subnode in mtd node. If present read address and reg from it and search partitions as subnodes of the ofpart node. In this case unknown nodes can cause error. 2) failing that issue a warning and try to parse ofpart partitions from the mtd node itself. In this case unknown nodes cannot cause error for compatibility with other drivers including the already exisitn s3c64xx controller-data node. The parser code can be the same for both cases and only operate on different node with a flag to reject unknown subnodes or not. That seems reasonable to me. signature.asc Description: Digital signature
Re: [RFC PATCH 2/2] dt: spi: s3c64xx: add compatible to controller-data
On Wed, Jul 29, 2015 at 08:21:34PM +0200, Michal Suchanek wrote: > On 29 July 2015 at 19:16, Mark Brown wrote: > >> It will not break anything. It will just spam dmesg. > > I'm confused - if all this change does is to spam dmesg then what's the > > point? > Presumably when your SPI NOR flash fails to probe this message will be > just above and you will look into the binding doc and add the > compatible. If you're looking to add a warning message when the flash device fails to probe then add that in the flash driver when it detects an error, this will cause needless noise for everyone else using this controller purely to work around the broken binding. And like I say compatible really seems like it isn't an appropriate property here. signature.asc Description: Digital signature
Re: [RFC PATCH 2/2] dt: spi: s3c64xx: add compatible to controller-data
On 29 July 2015 at 19:16, Mark Brown wrote: > On Wed, Jul 29, 2015 at 06:19:24PM +0200, Michal Suchanek wrote: >> On 29 July 2015 at 16:00, Mark Brown wrote: > >> > I can't tell from this commit message what the issue you're trying to >> > fix is, sorry. Nodes without compatible strings are entirely normal and >> > don't need compatible strings. It sounds like a bug in whatever other >> > driver is becoming confused. > >> The driver that gets confused is ofpart. > >> The two-line patch to allow it to just ignore controller-data has been >> rejected on the basis that s3c64xx should use a compatible string >> because ofpart monopolizes all nodes without compatible which are >> children of a mtd device. Devicetrees containing such nodes that are >> not partitions are presumably invalid and should be rejected when >> ofpart is compiled into the kernel. > > That seems like an extremely limited binding, the normal thing here > would be to create a specifically named node to contain the collection > of subnodes like many PMICs do for their regulators. As a fix I'd > suggest just silently ignoring nodes it can't understand, or printing a > warning if that's a serious issue. > >> >> + if (!of_get_property(data_np, "compatible", NULL) || >> >> + strcmp(of_get_property(data_np, "compatible", NULL), >> >> +"samsung,s3c-controller-data")) >> >> + dev_err(>dev, "child node 'controller-data' does not >> >> have correct compatible\n"); > >> > This will break all existing users which is not acceptable for >> > mainline, we need to preserve compatibility with existing device trees. > >> It will not break anything. It will just spam dmesg. > > I'm confused - if all this change does is to spam dmesg then what's the > point? Presumably when your SPI NOR flash fails to probe this message will be just above and you will look into the binding doc and add the compatible. Thanks Michal -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 2/2] dt: spi: s3c64xx: add compatible to controller-data
On Wed, Jul 29, 2015 at 06:19:24PM +0200, Michal Suchanek wrote: > On 29 July 2015 at 16:00, Mark Brown wrote: > > I can't tell from this commit message what the issue you're trying to > > fix is, sorry. Nodes without compatible strings are entirely normal and > > don't need compatible strings. It sounds like a bug in whatever other > > driver is becoming confused. > The driver that gets confused is ofpart. > The two-line patch to allow it to just ignore controller-data has been > rejected on the basis that s3c64xx should use a compatible string > because ofpart monopolizes all nodes without compatible which are > children of a mtd device. Devicetrees containing such nodes that are > not partitions are presumably invalid and should be rejected when > ofpart is compiled into the kernel. That seems like an extremely limited binding, the normal thing here would be to create a specifically named node to contain the collection of subnodes like many PMICs do for their regulators. As a fix I'd suggest just silently ignoring nodes it can't understand, or printing a warning if that's a serious issue. > >> + if (!of_get_property(data_np, "compatible", NULL) || > >> + strcmp(of_get_property(data_np, "compatible", NULL), > >> +"samsung,s3c-controller-data")) > >> + dev_err(>dev, "child node 'controller-data' does not > >> have correct compatible\n"); > > This will break all existing users which is not acceptable for > > mainline, we need to preserve compatibility with existing device trees. > It will not break anything. It will just spam dmesg. I'm confused - if all this change does is to spam dmesg then what's the point? signature.asc Description: Digital signature
Re: [RFC PATCH 2/2] dt: spi: s3c64xx: add compatible to controller-data
On Wed, Jul 29, 2015 at 06:19:24PM +0200, Michal Suchanek wrote: > On 29 July 2015 at 16:00, Mark Brown wrote: > > On Wed, Jul 29, 2015 at 12:19:57PM +0200, Michal Suchanek wrote: > > > > Please use subject lines matching the style for the subsytsem so people > > can spot that the patch is in some way relevant. > > > >> The controller-data subnode has no compatible. This can lead to other > >> drivers getting confused by it. Add a compatible to make devicetreee > >> unambiguous. > > > > I can't tell from this commit message what the issue you're trying to > > fix is, sorry. Nodes without compatible strings are entirely normal and > > don't need compatible strings. It sounds like a bug in whatever other > > driver is becoming confused. > > The driver that gets confused is ofpart. > > The two-line patch to allow it to just ignore controller-data has been > rejected on the basis that s3c64xx should use a compatible string It wasn't outright rejected, but it was questioned. > because ofpart monopolizes all nodes without compatible which are > children of a mtd device. Devicetrees containing such nodes that are > not partitions are presumably invalid and should be rejected when > ofpart is compiled into the kernel. That characterization is currently correct. I'm not a fan of what ofpart does in the first place; I'd really like it if its binding was written in a way that made it more precise, but we have to live with what we have. I also didn't like that you were designing your system such that we were now keying off the fact that your node has no 'reg' property. This seemed awfully fragile. (Admittedly, so is ofpart.c.) All in all, if you have better suggestions, I'm all ears. (At first glance, I'm liking your patch 1 to help disambiguate both MTD partitions and the 'controller-data' node going forward.) Brian -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 2/2] dt: spi: s3c64xx: add compatible to controller-data
On 29 July 2015 at 16:00, Mark Brown wrote: > On Wed, Jul 29, 2015 at 12:19:57PM +0200, Michal Suchanek wrote: > > Please use subject lines matching the style for the subsytsem so people > can spot that the patch is in some way relevant. > >> The controller-data subnode has no compatible. This can lead to other >> drivers getting confused by it. Add a compatible to make devicetreee >> unambiguous. > > I can't tell from this commit message what the issue you're trying to > fix is, sorry. Nodes without compatible strings are entirely normal and > don't need compatible strings. It sounds like a bug in whatever other > driver is becoming confused. The driver that gets confused is ofpart. The two-line patch to allow it to just ignore controller-data has been rejected on the basis that s3c64xx should use a compatible string because ofpart monopolizes all nodes without compatible which are children of a mtd device. Devicetrees containing such nodes that are not partitions are presumably invalid and should be rejected when ofpart is compiled into the kernel. > >> + if (!of_get_property(data_np, "compatible", NULL) || >> + strcmp(of_get_property(data_np, "compatible", NULL), >> +"samsung,s3c-controller-data")) >> + dev_err(>dev, "child node 'controller-data' does not have >> correct compatible\n"); > > This will break all existing users which is not acceptable for > mainline, we need to preserve compatibility with existing device trees. It will not break anything. It will just spam dmesg. Thanks Michal -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 2/2] dt: spi: s3c64xx: add compatible to controller-data
On Wed, Jul 29, 2015 at 12:19:57PM +0200, Michal Suchanek wrote: Please use subject lines matching the style for the subsytsem so people can spot that the patch is in some way relevant. > The controller-data subnode has no compatible. This can lead to other > drivers getting confused by it. Add a compatible to make devicetreee > unambiguous. I can't tell from this commit message what the issue you're trying to fix is, sorry. Nodes without compatible strings are entirely normal and don't need compatible strings. It sounds like a bug in whatever other driver is becoming confused. > + if (!of_get_property(data_np, "compatible", NULL) || > + strcmp(of_get_property(data_np, "compatible", NULL), > +"samsung,s3c-controller-data")) > + dev_err(>dev, "child node 'controller-data' does not have > correct compatible\n"); This will break all existing users which is not acceptable for mainline, we need to preserve compatibility with existing device trees. signature.asc Description: Digital signature
Re: [RFC PATCH 2/2] dt: spi: s3c64xx: add compatible to controller-data
On Wed, Jul 29, 2015 at 06:19:24PM +0200, Michal Suchanek wrote: On 29 July 2015 at 16:00, Mark Brown broo...@kernel.org wrote: On Wed, Jul 29, 2015 at 12:19:57PM +0200, Michal Suchanek wrote: Please use subject lines matching the style for the subsytsem so people can spot that the patch is in some way relevant. The controller-data subnode has no compatible. This can lead to other drivers getting confused by it. Add a compatible to make devicetreee unambiguous. I can't tell from this commit message what the issue you're trying to fix is, sorry. Nodes without compatible strings are entirely normal and don't need compatible strings. It sounds like a bug in whatever other driver is becoming confused. The driver that gets confused is ofpart. The two-line patch to allow it to just ignore controller-data has been rejected on the basis that s3c64xx should use a compatible string It wasn't outright rejected, but it was questioned. because ofpart monopolizes all nodes without compatible which are children of a mtd device. Devicetrees containing such nodes that are not partitions are presumably invalid and should be rejected when ofpart is compiled into the kernel. That characterization is currently correct. I'm not a fan of what ofpart does in the first place; I'd really like it if its binding was written in a way that made it more precise, but we have to live with what we have. I also didn't like that you were designing your system such that we were now keying off the fact that your node has no 'reg' property. This seemed awfully fragile. (Admittedly, so is ofpart.c.) All in all, if you have better suggestions, I'm all ears. (At first glance, I'm liking your patch 1 to help disambiguate both MTD partitions and the 'controller-data' node going forward.) Brian -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 2/2] dt: spi: s3c64xx: add compatible to controller-data
On 29 July 2015 at 16:00, Mark Brown broo...@kernel.org wrote: On Wed, Jul 29, 2015 at 12:19:57PM +0200, Michal Suchanek wrote: Please use subject lines matching the style for the subsytsem so people can spot that the patch is in some way relevant. The controller-data subnode has no compatible. This can lead to other drivers getting confused by it. Add a compatible to make devicetreee unambiguous. I can't tell from this commit message what the issue you're trying to fix is, sorry. Nodes without compatible strings are entirely normal and don't need compatible strings. It sounds like a bug in whatever other driver is becoming confused. The driver that gets confused is ofpart. The two-line patch to allow it to just ignore controller-data has been rejected on the basis that s3c64xx should use a compatible string because ofpart monopolizes all nodes without compatible which are children of a mtd device. Devicetrees containing such nodes that are not partitions are presumably invalid and should be rejected when ofpart is compiled into the kernel. + if (!of_get_property(data_np, compatible, NULL) || + strcmp(of_get_property(data_np, compatible, NULL), +samsung,s3c-controller-data)) + dev_err(spi-dev, child node 'controller-data' does not have correct compatible\n); This will break all existing users which is not acceptable for mainline, we need to preserve compatibility with existing device trees. It will not break anything. It will just spam dmesg. Thanks Michal -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 2/2] dt: spi: s3c64xx: add compatible to controller-data
On Wed, Jul 29, 2015 at 06:19:24PM +0200, Michal Suchanek wrote: On 29 July 2015 at 16:00, Mark Brown broo...@kernel.org wrote: I can't tell from this commit message what the issue you're trying to fix is, sorry. Nodes without compatible strings are entirely normal and don't need compatible strings. It sounds like a bug in whatever other driver is becoming confused. The driver that gets confused is ofpart. The two-line patch to allow it to just ignore controller-data has been rejected on the basis that s3c64xx should use a compatible string because ofpart monopolizes all nodes without compatible which are children of a mtd device. Devicetrees containing such nodes that are not partitions are presumably invalid and should be rejected when ofpart is compiled into the kernel. That seems like an extremely limited binding, the normal thing here would be to create a specifically named node to contain the collection of subnodes like many PMICs do for their regulators. As a fix I'd suggest just silently ignoring nodes it can't understand, or printing a warning if that's a serious issue. + if (!of_get_property(data_np, compatible, NULL) || + strcmp(of_get_property(data_np, compatible, NULL), +samsung,s3c-controller-data)) + dev_err(spi-dev, child node 'controller-data' does not have correct compatible\n); This will break all existing users which is not acceptable for mainline, we need to preserve compatibility with existing device trees. It will not break anything. It will just spam dmesg. I'm confused - if all this change does is to spam dmesg then what's the point? signature.asc Description: Digital signature
Re: [RFC PATCH 2/2] dt: spi: s3c64xx: add compatible to controller-data
On Wed, Jul 29, 2015 at 08:21:34PM +0200, Michal Suchanek wrote: On 29 July 2015 at 19:16, Mark Brown broo...@kernel.org wrote: It will not break anything. It will just spam dmesg. I'm confused - if all this change does is to spam dmesg then what's the point? Presumably when your SPI NOR flash fails to probe this message will be just above and you will look into the binding doc and add the compatible. If you're looking to add a warning message when the flash device fails to probe then add that in the flash driver when it detects an error, this will cause needless noise for everyone else using this controller purely to work around the broken binding. And like I say compatible really seems like it isn't an appropriate property here. signature.asc Description: Digital signature
Re: [RFC PATCH 2/2] dt: spi: s3c64xx: add compatible to controller-data
On 29 July 2015 at 19:16, Mark Brown broo...@kernel.org wrote: On Wed, Jul 29, 2015 at 06:19:24PM +0200, Michal Suchanek wrote: On 29 July 2015 at 16:00, Mark Brown broo...@kernel.org wrote: I can't tell from this commit message what the issue you're trying to fix is, sorry. Nodes without compatible strings are entirely normal and don't need compatible strings. It sounds like a bug in whatever other driver is becoming confused. The driver that gets confused is ofpart. The two-line patch to allow it to just ignore controller-data has been rejected on the basis that s3c64xx should use a compatible string because ofpart monopolizes all nodes without compatible which are children of a mtd device. Devicetrees containing such nodes that are not partitions are presumably invalid and should be rejected when ofpart is compiled into the kernel. That seems like an extremely limited binding, the normal thing here would be to create a specifically named node to contain the collection of subnodes like many PMICs do for their regulators. As a fix I'd suggest just silently ignoring nodes it can't understand, or printing a warning if that's a serious issue. + if (!of_get_property(data_np, compatible, NULL) || + strcmp(of_get_property(data_np, compatible, NULL), +samsung,s3c-controller-data)) + dev_err(spi-dev, child node 'controller-data' does not have correct compatible\n); This will break all existing users which is not acceptable for mainline, we need to preserve compatibility with existing device trees. It will not break anything. It will just spam dmesg. I'm confused - if all this change does is to spam dmesg then what's the point? Presumably when your SPI NOR flash fails to probe this message will be just above and you will look into the binding doc and add the compatible. Thanks Michal -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 2/2] dt: spi: s3c64xx: add compatible to controller-data
On Wed, Jul 29, 2015 at 12:19:57PM +0200, Michal Suchanek wrote: Please use subject lines matching the style for the subsytsem so people can spot that the patch is in some way relevant. The controller-data subnode has no compatible. This can lead to other drivers getting confused by it. Add a compatible to make devicetreee unambiguous. I can't tell from this commit message what the issue you're trying to fix is, sorry. Nodes without compatible strings are entirely normal and don't need compatible strings. It sounds like a bug in whatever other driver is becoming confused. + if (!of_get_property(data_np, compatible, NULL) || + strcmp(of_get_property(data_np, compatible, NULL), +samsung,s3c-controller-data)) + dev_err(spi-dev, child node 'controller-data' does not have correct compatible\n); This will break all existing users which is not acceptable for mainline, we need to preserve compatibility with existing device trees. signature.asc Description: Digital signature