| Message ID | 1455031585-11113-1-git-send-email-ym0914@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Feb 10, 2016 at 12:26:25AM +0900, Youngmin Nam wrote: > SPI driver on some SoCs only differ in the fifosize of each > SPI channel. It is useless to duplicate the s3c64xx_spi_port_config structure > or create a compatible name for such a change. I disagree that it is useless to do this, it means that if we realize later that there is some other difference between the implementations then we have the information in the DT to handle this without needing to update the ABI. > +Optional SoC Specific properties: > + > +- samsung,spi-fifosize: The fifo size supported by the SPI channel > + > + In units of...? FIFO sizes do get quoted in both words and bytes so it seems good to be clear.
Hello Mark. Thank you for your review. Please ignore previous mail that I sent to you. I sent this mail without CC-ing. I resend including CC list. On 2016? 02? 10? 01:10, Mark Brown wrote: > On Wed, Feb 10, 2016 at 12:26:25AM +0900, Youngmin Nam wrote: > >> SPI driver on some SoCs only differ in the fifosize of each >> SPI channel. It is useless to duplicate the s3c64xx_spi_port_config structure >> or create a compatible name for such a change. > > I disagree that it is useless to do this, it means that if we realize > later that there is some other difference between the implementations > then we have the information in the DT to handle this without needing > to update the ABI. > To clarify, let me check that I understood. Do you mean we can fix fifosize of each SPI channel with DT handling if there is difference on this fifosize with driver code? If I understand your words correctly, let me modify commit messages. >> +Optional SoC Specific properties: >> + >> +- samsung,spi-fifosize: The fifo size supported by the SPI channel >> + >> + > > In units of...? FIFO sizes do get quoted in both words and bytes so it > seems good to be clear. > "samsung,spi-fifosize = <64>" means 64 bytes of fifosize. Let me add the units as bytes. Thank you. -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
W dniu 10.02.2016 o 00:26, Youngmin Nam pisze: > SPI driver on some SoCs only differ in the fifosize of each > SPI channel. It is useless to duplicate the s3c64xx_spi_port_config structure > or create a compatible name for such a change. > > We can get fifosize via the device tree nodes (not mandatory). > Also the device tree binding document was updated. > > Signed-off-by: Youngmin Nam <ym0914@gmail.com> > --- > Documentation/devicetree/bindings/spi/spi-samsung.txt | 6 ++++++ > drivers/spi/spi-s3c64xx.c | 13 +++++++++++++ > 2 files changed, 19 insertions(+) > > diff --git a/Documentation/devicetree/bindings/spi/spi-samsung.txt b/Documentation/devicetree/bindings/spi/spi-samsung.txt > index 6dbdeb3..5c4a08d 100644 > --- a/Documentation/devicetree/bindings/spi/spi-samsung.txt > +++ b/Documentation/devicetree/bindings/spi/spi-samsung.txt > @@ -23,6 +23,11 @@ Required SoC Specific Properties: > - dma-names: Names for the dma channels. There must be at least one channel > named "tx" for transmit and named "rx" for receive. > > +Optional SoC Specific properties: > + > +- samsung,spi-fifosize: The fifo size supported by the SPI channel > + > + > Required Board Specific Properties: > > - #address-cells: should be 1. > @@ -73,6 +78,7 @@ Example: > dma-names = "tx", "rx"; > #address-cells = <1>; > #size-cells = <0>; > + samsung,spi-fifosize = <64>; > }; > That does not look good. Is it a configurable part of SoC? I think not... rather it must contain exact value supported by given device... So this should be part of compatible because AFAIU you just made compatible devices uncompatible... Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Krzysztof, As you mentioned, spi fifosize is not configurable in the given SoC. The point is we can set fifosize without changing driver code. For example, if some SoC in exynos7 series has different spi fifosize of on each channel with current our compatible, we can't cover this situation without adding new compatible into spi driver code. Whenever new SoC kind of exynos7 come out, we should add new compatible into driver code only for fifosize change. I think this is not efficient. I think we can reduces this works through DT handling. Thanks. On 2016? 02? 14? 16:01, Krzysztof Kozlowski wrote: > W dniu 10.02.2016 o 00:26, Youngmin Nam pisze: >> SPI driver on some SoCs only differ in the fifosize of each >> SPI channel. It is useless to duplicate the s3c64xx_spi_port_config structure >> or create a compatible name for such a change. >> >> We can get fifosize via the device tree nodes (not mandatory). >> Also the device tree binding document was updated. >> >> Signed-off-by: Youngmin Nam <ym0914@gmail.com> >> --- >> Documentation/devicetree/bindings/spi/spi-samsung.txt | 6 ++++++ >> drivers/spi/spi-s3c64xx.c | 13 +++++++++++++ >> 2 files changed, 19 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/spi/spi-samsung.txt b/Documentation/devicetree/bindings/spi/spi-samsung.txt >> index 6dbdeb3..5c4a08d 100644 >> --- a/Documentation/devicetree/bindings/spi/spi-samsung.txt >> +++ b/Documentation/devicetree/bindings/spi/spi-samsung.txt >> @@ -23,6 +23,11 @@ Required SoC Specific Properties: >> - dma-names: Names for the dma channels. There must be at least one channel >> named "tx" for transmit and named "rx" for receive. >> >> +Optional SoC Specific properties: >> + >> +- samsung,spi-fifosize: The fifo size supported by the SPI channel >> + >> + >> Required Board Specific Properties: >> >> - #address-cells: should be 1. >> @@ -73,6 +78,7 @@ Example: >> dma-names = "tx", "rx"; >> #address-cells = <1>; >> #size-cells = <0>; >> + samsung,spi-fifosize = <64>; >> }; >> > > That does not look good. Is it a configurable part of SoC? I think > not... rather it must contain exact value supported by given device... > So this should be part of compatible because AFAIU you just made > compatible devices uncompatible... > > Best regards, > Krzysztof > -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
W dniu 14.02.2016 o 17:13, Youngmin Nam pisze: > Hello Krzysztof, > > As you mentioned, spi fifosize is not configurable in the given SoC. > The point is we can set fifosize without changing driver code. > For example, if some SoC in exynos7 series has different spi fifosize of on each channel with current > our compatible, we can't cover this situation without adding new compatible into spi driver code. Yes, because new SoC is not compatible with old ones... so a new compatible is required! > Whenever new SoC kind of exynos7 come out, we should add new compatible into driver code only for fifosize change. > I think this is not efficient. I think we can reduces this works through DT handling. I agree that this is not the most efficient possible way of setting some specific properties of devices but this is the way how DT works. What if the property is not present on board with Exynos7? What value should be used? You did not want to add a new compatible but you are adding a compatible-like property which apparently is required for some devices. That looks bad. It's easy to make a mistake, messes with compatibles. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
When I think about this patch, I was motivated by below patch.
commit 135f07c3252dc77d0245714d0b413ecc711cd823
Author: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
Date: Mon Jul 14 17:07:16 2014 +0530
serial: samsung: get fifosize via device tree
UART modules on some SoCs only differ in the fifosize of each
UART channel. Its useless to duplicate the drv_data structure
or create a compatible name for such a change.
We can get fifosize via the device tree nodes (not mandating it).
Also updates the documentation.
Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
Reviewed-by: Tomasz Figa <t.figa@samsung.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
I thought we can apply above patch to SPI also.
The fifosize should be required for fifo_lvl_mask value in SPI of Exynos series.
I think this patch is useful when we port for the new exynos7xxx SoC come out.
Because SPI of new exynos7xxx SoC has only difference on fifosize.
Thanks.
On 2016? 02? 14? 17:31, Krzysztof Kozlowski wrote:
> W dniu 14.02.2016 o 17:13, Youngmin Nam pisze:
>> Hello Krzysztof,
>>
>> As you mentioned, spi fifosize is not configurable in the given SoC.
>> The point is we can set fifosize without changing driver code.
>> For example, if some SoC in exynos7 series has different spi fifosize of on each channel with current
>> our compatible, we can't cover this situation without adding new compatible into spi driver code.
>
> Yes, because new SoC is not compatible with old ones... so a new
> compatible is required!
>
>> Whenever new SoC kind of exynos7 come out, we should add new compatible into driver code only for fifosize change.
>> I think this is not efficient. I think we can reduces this works through DT handling.
>
> I agree that this is not the most efficient possible way of setting some
> specific properties of devices but this is the way how DT works.
>
> What if the property is not present on board with Exynos7? What value
> should be used?
>
> You did not want to add a new compatible but you are adding a
> compatible-like property which apparently is required for some devices.
>
> That looks bad. It's easy to make a mistake, messes with compatibles.
>
> Best regards,
> Krzysztof
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
2016-02-14 22:22 GMT+09:00 Youngmin Nam <ym0914@gmail.com>: > When I think about this patch, I was motivated by below patch. > > commit 135f07c3252dc77d0245714d0b413ecc711cd823 > Author: Naveen Krishna Chatradhi <ch.naveen@samsung.com> > Date: Mon Jul 14 17:07:16 2014 +0530 > > serial: samsung: get fifosize via device tree > > UART modules on some SoCs only differ in the fifosize of each > UART channel. Its useless to duplicate the drv_data structure > or create a compatible name for such a change. > > We can get fifosize via the device tree nodes (not mandating it). > > Also updates the documentation. > > Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com> > Reviewed-by: Tomasz Figa <t.figa@samsung.com> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > I thought we can apply above patch to SPI also. > The fifosize should be required for fifo_lvl_mask value in SPI of Exynos series. > I think this patch is useful when we port for the new exynos7xxx SoC come out. > Because SPI of new exynos7xxx SoC has only difference on fifosize. > Please, do not top post. You skipped some of arguments from my reply instead saying "patch is useful". That is not a sufficient argument for my doubts... but let me state again the most important question: What happens if this *optional* property is not present on board with Exynos7? What value should be used? Best regards, Krzysztof > Thanks. > > > On 2016? 02? 14? 17:31, Krzysztof Kozlowski wrote: >> W dniu 14.02.2016 o 17:13, Youngmin Nam pisze: >>> Hello Krzysztof, >>> >>> As you mentioned, spi fifosize is not configurable in the given SoC. >>> The point is we can set fifosize without changing driver code. >>> For example, if some SoC in exynos7 series has different spi fifosize of on each channel with current >>> our compatible, we can't cover this situation without adding new compatible into spi driver code. >> >> Yes, because new SoC is not compatible with old ones... so a new >> compatible is required! >> >>> Whenever new SoC kind of exynos7 come out, we should add new compatible into driver code only for fifosize change. >>> I think this is not efficient. I think we can reduces this works through DT handling. >> >> I agree that this is not the most efficient possible way of setting some >> specific properties of devices but this is the way how DT works. >> >> What if the property is not present on board with Exynos7? What value >> should be used? >> >> You did not want to add a new compatible but you are adding a >> compatible-like property which apparently is required for some devices. >> >> That looks bad. It's easy to make a mistake, messes with compatibles. >> >> Best regards, >> Krzysztof >> >> -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Feb 14, 2016 at 03:28:30PM +0900, Youngmin Nam wrote: > On 2016? 02? 10? 01:10, Mark Brown wrote: > > On Wed, Feb 10, 2016 at 12:26:25AM +0900, Youngmin Nam wrote: > > I disagree that it is useless to do this, it means that if we realize > > later that there is some other difference between the implementations > > then we have the information in the DT to handle this without needing > > to update the ABI. > To clarify, let me check that I understood. > Do you mean we can fix fifosize of each SPI channel with DT handling > if there is difference on this fifosize with driver code? > > If I understand your words correctly, let me modify commit messages. I'm having a hard time understanding what you're saying here but it sounds like you've understood the oppositve of my intention here. I'm saying that it's not clear to me that this is better than adding more compatibles. At the very least we need a new compatible which requires this property as Krzysztof was suggesting.
diff --git a/Documentation/devicetree/bindings/spi/spi-samsung.txt b/Documentation/devicetree/bindings/spi/spi-samsung.txt index 6dbdeb3..5c4a08d 100644 --- a/Documentation/devicetree/bindings/spi/spi-samsung.txt +++ b/Documentation/devicetree/bindings/spi/spi-samsung.txt @@ -23,6 +23,11 @@ Required SoC Specific Properties: - dma-names: Names for the dma channels. There must be at least one channel named "tx" for transmit and named "rx" for receive. +Optional SoC Specific properties: + +- samsung,spi-fifosize: The fifo size supported by the SPI channel + + Required Board Specific Properties: - #address-cells: should be 1. @@ -73,6 +78,7 @@ Example: dma-names = "tx", "rx"; #address-cells = <1>; #size-cells = <0>; + samsung,spi-fifosize = <64>; }; - Board Specific Portion: diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c index 5a76a50..44f1aeb 100644 --- a/drivers/spi/spi-s3c64xx.c +++ b/drivers/spi/spi-s3c64xx.c @@ -1032,6 +1032,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev) struct spi_master *master; int ret, irq; char clk_name[16]; + int fifosize; if (!sci && pdev->dev.of_node) { sci = s3c64xx_spi_parse_dt(&pdev->dev); @@ -1083,6 +1084,18 @@ static int s3c64xx_spi_probe(struct platform_device *pdev) sdd->port_id = pdev->id; } + if (pdev->dev.of_node) { + if (of_property_read_u32(pdev->dev.of_node, + "samsung,spi-fifosize", &fifosize)) { + } else { + sdd->port_conf->fifo_lvl_mask[sdd->port_id] = + (fifosize << 1) - 1; + dev_info(&pdev->dev, "PORT %d fifo_lvl_mask = 0x%x\n", + sdd->port_id, + sdd->port_conf->fifo_lvl_mask[sdd->port_id]); + } + } + sdd->cur_bpw = 8; if (!sdd->pdev->dev.of_node && (!sci->dma_tx || !sci->dma_rx)) {
SPI driver on some SoCs only differ in the fifosize of each SPI channel. It is useless to duplicate the s3c64xx_spi_port_config structure or create a compatible name for such a change. We can get fifosize via the device tree nodes (not mandatory). Also the device tree binding document was updated. Signed-off-by: Youngmin Nam <ym0914@gmail.com> --- Documentation/devicetree/bindings/spi/spi-samsung.txt | 6 ++++++ drivers/spi/spi-s3c64xx.c | 13 +++++++++++++ 2 files changed, 19 insertions(+)