| Message ID | 20211217161654.367782-5-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | External ECC engines & Macronix support | expand |
> Subject: [PATCH v7 04/14] spi: cadence: Provide a capability structure s/cadence/cadence-quadspi/ On 17/12/21 05:16PM, Miquel Raynal wrote: > This controller has DTR support, so advertize it with a capability now > that the spi_controller_mem_ops structure contains this new field. This > will later be used by the core to discriminate whether an operation is > supported or not, in a more generic way than having different helpers. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/spi/spi-cadence-quadspi.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c > index 101cc71bffa7..98e0cc4236e3 100644 > --- a/drivers/spi/spi-cadence-quadspi.c > +++ b/drivers/spi/spi-cadence-quadspi.c > @@ -1388,10 +1388,15 @@ static const char *cqspi_get_name(struct spi_mem *mem) > return devm_kasprintf(dev, GFP_KERNEL, "%s.%d", dev_name(dev), mem->spi->chip_select); > } > > +static const struct spi_controller_mem_caps cqspi_mem_caps = { > + .dtr = true, > +}; > + > static const struct spi_controller_mem_ops cqspi_mem_ops = { > .exec_op = cqspi_exec_mem_op, > .get_name = cqspi_get_name, > .supports_op = cqspi_supports_mem_op, > + .caps = &cqspi_mem_caps, I just noticed you put it under struct spi_mem_ops, not under struct spi_mem. This is not an operation per se so wouldn't it be better if it is moved to struct spi_mem? Anyway, the change itself looks good to me. The cqspi_supports_mem_op() already checks for mixed DTR modes so we should be good. > }; > > static int cqspi_setup_flash(struct cqspi_st *cqspi) > -- > 2.27.0 >
Hi Pratyush, p.yadav@ti.com wrote on Tue, 21 Dec 2021 00:25:18 +0530: > > Subject: [PATCH v7 04/14] spi: cadence: Provide a capability structure > > s/cadence/cadence-quadspi/ Right. > > On 17/12/21 05:16PM, Miquel Raynal wrote: > > This controller has DTR support, so advertize it with a capability now > > that the spi_controller_mem_ops structure contains this new field. This > > will later be used by the core to discriminate whether an operation is > > supported or not, in a more generic way than having different helpers. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > drivers/spi/spi-cadence-quadspi.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c > > index 101cc71bffa7..98e0cc4236e3 100644 > > --- a/drivers/spi/spi-cadence-quadspi.c > > +++ b/drivers/spi/spi-cadence-quadspi.c > > @@ -1388,10 +1388,15 @@ static const char *cqspi_get_name(struct spi_mem *mem) > > return devm_kasprintf(dev, GFP_KERNEL, "%s.%d", dev_name(dev), mem->spi->chip_select); > > } > > > > +static const struct spi_controller_mem_caps cqspi_mem_caps = { > > + .dtr = true, > > +}; > > + > > static const struct spi_controller_mem_ops cqspi_mem_ops = { > > .exec_op = cqspi_exec_mem_op, > > .get_name = cqspi_get_name, > > .supports_op = cqspi_supports_mem_op, > > + .caps = &cqspi_mem_caps, > > I just noticed you put it under struct spi_mem_ops, not under struct > spi_mem. This is not an operation per se so wouldn't it be better if it > is moved to struct spi_mem? I had a hard time taking a decision but my conclusion was that these caps are static controller capabilities and exclusively tight to the controller. The spi_mem structure defines a SPI peripheral. The spi_mem_ops structure is the only spi-mem related field of the spi-controller structure. I could have added my own field there but as these caps are only meant to be used by the spi_mem_ops anyway (exclusively ->supports_op() for now), it seemed to be a good location, at least better than the spi-mem structure. > Anyway, the change itself looks good to me. The cqspi_supports_mem_op() > already checks for mixed DTR modes so we should be good. Yep! > > > }; > > > > static int cqspi_setup_flash(struct cqspi_st *cqspi) > > -- > > 2.27.0 > > > Thanks, Miquèl
On 21/12/21 11:16AM, Miquel Raynal wrote: > Hi Pratyush, > > p.yadav@ti.com wrote on Tue, 21 Dec 2021 00:25:18 +0530: > > > > Subject: [PATCH v7 04/14] spi: cadence: Provide a capability structure > > > > s/cadence/cadence-quadspi/ > > Right. > > > > > On 17/12/21 05:16PM, Miquel Raynal wrote: > > > This controller has DTR support, so advertize it with a capability now > > > that the spi_controller_mem_ops structure contains this new field. This > > > will later be used by the core to discriminate whether an operation is > > > supported or not, in a more generic way than having different helpers. > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > --- > > > drivers/spi/spi-cadence-quadspi.c | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c > > > index 101cc71bffa7..98e0cc4236e3 100644 > > > --- a/drivers/spi/spi-cadence-quadspi.c > > > +++ b/drivers/spi/spi-cadence-quadspi.c > > > @@ -1388,10 +1388,15 @@ static const char *cqspi_get_name(struct spi_mem *mem) > > > return devm_kasprintf(dev, GFP_KERNEL, "%s.%d", dev_name(dev), mem->spi->chip_select); > > > } > > > > > > +static const struct spi_controller_mem_caps cqspi_mem_caps = { > > > + .dtr = true, > > > +}; > > > + > > > static const struct spi_controller_mem_ops cqspi_mem_ops = { > > > .exec_op = cqspi_exec_mem_op, > > > .get_name = cqspi_get_name, > > > .supports_op = cqspi_supports_mem_op, > > > + .caps = &cqspi_mem_caps, > > > > I just noticed you put it under struct spi_mem_ops, not under struct > > spi_mem. This is not an operation per se so wouldn't it be better if it > > is moved to struct spi_mem? > > I had a hard time taking a decision but my conclusion was that these > caps are static controller capabilities and exclusively tight to the > controller. The spi_mem structure defines a SPI peripheral. The > spi_mem_ops structure is the only spi-mem related field of the > spi-controller structure. I could have added my own field there but > as these caps are only meant to be used by the spi_mem_ops anyway > (exclusively ->supports_op() for now), it seemed to be a good location, > at least better than the spi-mem structure. Can we have a 3rd person chime in and break the tie? :-) > > > Anyway, the change itself looks good to me. The cqspi_supports_mem_op() > > already checks for mixed DTR modes so we should be good. > > Yep! > > > > > > }; > > > > > > static int cqspi_setup_flash(struct cqspi_st *cqspi) > > > -- > > > 2.27.0 > > > > > > > > Thanks, > Miquèl
Hi Pratyush, p.yadav@ti.com wrote on Tue, 21 Dec 2021 16:11:10 +0530: > On 21/12/21 11:16AM, Miquel Raynal wrote: > > Hi Pratyush, > > > > p.yadav@ti.com wrote on Tue, 21 Dec 2021 00:25:18 +0530: > > > > > > Subject: [PATCH v7 04/14] spi: cadence: Provide a capability structure > > > > > > s/cadence/cadence-quadspi/ > > > > Right. > > > > > > > > On 17/12/21 05:16PM, Miquel Raynal wrote: > > > > This controller has DTR support, so advertize it with a capability now > > > > that the spi_controller_mem_ops structure contains this new field. This > > > > will later be used by the core to discriminate whether an operation is > > > > supported or not, in a more generic way than having different helpers. > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > --- > > > > drivers/spi/spi-cadence-quadspi.c | 5 +++++ > > > > 1 file changed, 5 insertions(+) > > > > > > > > diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c > > > > index 101cc71bffa7..98e0cc4236e3 100644 > > > > --- a/drivers/spi/spi-cadence-quadspi.c > > > > +++ b/drivers/spi/spi-cadence-quadspi.c > > > > @@ -1388,10 +1388,15 @@ static const char *cqspi_get_name(struct spi_mem *mem) > > > > return devm_kasprintf(dev, GFP_KERNEL, "%s.%d", dev_name(dev), mem->spi->chip_select); > > > > } > > > > > > > > +static const struct spi_controller_mem_caps cqspi_mem_caps = { > > > > + .dtr = true, > > > > +}; > > > > + > > > > static const struct spi_controller_mem_ops cqspi_mem_ops = { > > > > .exec_op = cqspi_exec_mem_op, > > > > .get_name = cqspi_get_name, > > > > .supports_op = cqspi_supports_mem_op, > > > > + .caps = &cqspi_mem_caps, > > > > > > I just noticed you put it under struct spi_mem_ops, not under struct > > > spi_mem. This is not an operation per se so wouldn't it be better if it > > > is moved to struct spi_mem? > > > > I had a hard time taking a decision but my conclusion was that these > > caps are static controller capabilities and exclusively tight to the > > controller. The spi_mem structure defines a SPI peripheral. The > > spi_mem_ops structure is the only spi-mem related field of the > > spi-controller structure. I could have added my own field there but > > as these caps are only meant to be used by the spi_mem_ops anyway > > (exclusively ->supports_op() for now), it seemed to be a good location, > > at least better than the spi-mem structure. > > Can we have a 3rd person chime in and break the tie? :-) I don't quite get why we should put controller specific information into the memory device structure? Anyway, do you mind if we move forward first? Not that I don't think that this choice should be discussed further, but I think this can easily be changed in the near future if there is a desire to reorganize spi-mem objects. In fact, these capabilities are accessed through a helper so that hypothetic change would be almost transparent. Thanks, Miquèl
On 21/12/21 12:19PM, Miquel Raynal wrote: > Hi Pratyush, > > p.yadav@ti.com wrote on Tue, 21 Dec 2021 16:11:10 +0530: > > > On 21/12/21 11:16AM, Miquel Raynal wrote: > > > Hi Pratyush, > > > > > > p.yadav@ti.com wrote on Tue, 21 Dec 2021 00:25:18 +0530: > > > > > > > > Subject: [PATCH v7 04/14] spi: cadence: Provide a capability structure > > > > > > > > s/cadence/cadence-quadspi/ > > > > > > Right. > > > > > > > > > > > On 17/12/21 05:16PM, Miquel Raynal wrote: > > > > > This controller has DTR support, so advertize it with a capability now > > > > > that the spi_controller_mem_ops structure contains this new field. This > > > > > will later be used by the core to discriminate whether an operation is > > > > > supported or not, in a more generic way than having different helpers. > > > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > > --- > > > > > drivers/spi/spi-cadence-quadspi.c | 5 +++++ > > > > > 1 file changed, 5 insertions(+) > > > > > > > > > > diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c > > > > > index 101cc71bffa7..98e0cc4236e3 100644 > > > > > --- a/drivers/spi/spi-cadence-quadspi.c > > > > > +++ b/drivers/spi/spi-cadence-quadspi.c > > > > > @@ -1388,10 +1388,15 @@ static const char *cqspi_get_name(struct spi_mem *mem) > > > > > return devm_kasprintf(dev, GFP_KERNEL, "%s.%d", dev_name(dev), mem->spi->chip_select); > > > > > } > > > > > > > > > > +static const struct spi_controller_mem_caps cqspi_mem_caps = { > > > > > + .dtr = true, > > > > > +}; > > > > > + > > > > > static const struct spi_controller_mem_ops cqspi_mem_ops = { > > > > > .exec_op = cqspi_exec_mem_op, > > > > > .get_name = cqspi_get_name, > > > > > .supports_op = cqspi_supports_mem_op, > > > > > + .caps = &cqspi_mem_caps, > > > > > > > > I just noticed you put it under struct spi_mem_ops, not under struct > > > > spi_mem. This is not an operation per se so wouldn't it be better if it > > > > is moved to struct spi_mem? > > > > > > I had a hard time taking a decision but my conclusion was that these > > > caps are static controller capabilities and exclusively tight to the > > > controller. The spi_mem structure defines a SPI peripheral. The > > > spi_mem_ops structure is the only spi-mem related field of the > > > spi-controller structure. I could have added my own field there but > > > as these caps are only meant to be used by the spi_mem_ops anyway > > > (exclusively ->supports_op() for now), it seemed to be a good location, > > > at least better than the spi-mem structure. > > > > Can we have a 3rd person chime in and break the tie? :-) > > I don't quite get why we should put controller specific information > into the memory device structure? Hmm, you're right. struct spi_controller would probably be a better place. > > Anyway, do you mind if we move forward first? Not that I don't think > that this choice should be discussed further, but I think this can > easily be changed in the near future if there is a desire to > reorganize spi-mem objects. In fact, these capabilities are accessed > through a helper so that hypothetic change would be almost transparent. Okay. I would still like to hear other opinions on this, but fine by me if you want to take this in as-is.
On Tue, 21 Dec 2021 17:35:00 +0530 Pratyush Yadav <p.yadav@ti.com> wrote: > > > > Anyway, do you mind if we move forward first? Not that I don't think > > that this choice should be discussed further, but I think this can > > easily be changed in the near future if there is a desire to > > reorganize spi-mem objects. In fact, these capabilities are accessed > > through a helper so that hypothetic change would be almost transparent. > > Okay. I would still like to hear other opinions on this, but fine by me > if you want to take this in as-is. I think we discussed that with Miquel, and I remember complaining about mixing function pointers and actual data in the spi_mem_ops struct, but honestly, it's just cosmetic concern, and I don't think it matters much in practice. So I'm fine either way, make it a field of spi_controller or spi_mem_ops, spi_mem is definitely not the right place though.
Hello, boris.brezillon@collabora.com wrote on Mon, 3 Jan 2022 09:38:19 +0100: > On Tue, 21 Dec 2021 17:35:00 +0530 > Pratyush Yadav <p.yadav@ti.com> wrote: > > > > > > > Anyway, do you mind if we move forward first? Not that I don't think > > > that this choice should be discussed further, but I think this can > > > easily be changed in the near future if there is a desire to > > > reorganize spi-mem objects. In fact, these capabilities are accessed > > > through a helper so that hypothetic change would be almost transparent. > > > > Okay. I would still like to hear other opinions on this, but fine by me > > if you want to take this in as-is. > > I think we discussed that with Miquel, and I remember complaining about > mixing function pointers and actual data in the spi_mem_ops struct, but > honestly, it's just cosmetic concern, and I don't think it matters much > in practice. So I'm fine either way, make it a field of spi_controller > or spi_mem_ops, spi_mem is definitely not the right place though. Yeah, I don't like the idea of leaking spi-mem information into the spi controller structure, while there is a structure (so far only containing hooks) that is dedicated to spi-mem operations. Extending this structure to contain capabilities appeared the right choice to me. But on the other hand this is a controller information anyway so if you both prefer moving this data into the SPI controller structure I'll find a way to do it. Thanks, Miquèl
diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c index 101cc71bffa7..98e0cc4236e3 100644 --- a/drivers/spi/spi-cadence-quadspi.c +++ b/drivers/spi/spi-cadence-quadspi.c @@ -1388,10 +1388,15 @@ static const char *cqspi_get_name(struct spi_mem *mem) return devm_kasprintf(dev, GFP_KERNEL, "%s.%d", dev_name(dev), mem->spi->chip_select); } +static const struct spi_controller_mem_caps cqspi_mem_caps = { + .dtr = true, +}; + static const struct spi_controller_mem_ops cqspi_mem_ops = { .exec_op = cqspi_exec_mem_op, .get_name = cqspi_get_name, .supports_op = cqspi_supports_mem_op, + .caps = &cqspi_mem_caps, }; static int cqspi_setup_flash(struct cqspi_st *cqspi)
This controller has DTR support, so advertize it with a capability now that the spi_controller_mem_ops structure contains this new field. This will later be used by the core to discriminate whether an operation is supported or not, in a more generic way than having different helpers. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/spi/spi-cadence-quadspi.c | 5 +++++ 1 file changed, 5 insertions(+)