Re: [PATCH 1/3] spi: add xtfpga SPI controller driver
On Wed, Mar 12, 2014 at 05:48:16AM +0400, Max Filippov wrote: > On Wed, Mar 12, 2014 at 5:11 AM, Mark Brown wrote: > > So I just looked again - the SPI code isn't in mainline, there must > > be some out of tree patches here that can't have been tested since the > > driver was converted to regmap (or the byte swapping you're doing in the > > controller is buggy for 16 bits per word). > I'm successfully running this driver with the patches in your ASoC tree > branch topic/tlv320aic23. Ah, I missed that during review - the driver shouldn't be forcing 16 bits per word, any swapping that's needed should be being done in the regmap API. Though based on the discussion of the SPI driver it seems like this may be compensating for an unwanted byte swap there. signature.asc Description: Digital signature
Re: [PATCH 1/3] spi: add xtfpga SPI controller driver
On Wed, Mar 12, 2014 at 05:48:16AM +0400, Max Filippov wrote: On Wed, Mar 12, 2014 at 5:11 AM, Mark Brown broo...@kernel.org wrote: So I just looked again - the SPI code isn't in mainline, there must be some out of tree patches here that can't have been tested since the driver was converted to regmap (or the byte swapping you're doing in the controller is buggy for 16 bits per word). I'm successfully running this driver with the patches in your ASoC tree branch topic/tlv320aic23. Ah, I missed that during review - the driver shouldn't be forcing 16 bits per word, any swapping that's needed should be being done in the regmap API. Though based on the discussion of the SPI driver it seems like this may be compensating for an unwanted byte swap there. signature.asc Description: Digital signature
Re: [PATCH 1/3] spi: add xtfpga SPI controller driver
On Wed, Mar 12, 2014 at 05:43:49AM +0400, Max Filippov wrote: > On Wed, Mar 12, 2014 at 5:08 AM, Mark Brown wrote: > > That's buggy, drivers should never configure anything more than 8 bits > > per word with regmap. > Ok, so the driver should allow for 8 bit transfers and regmap will arrange > transfers as 8-bit pairs, making CS to be asserted for 16 bits, right? > Hmmm... I see the only way to support that with that hardware: advertise > 8 bit support, buffer bytes up to 16 bits, send 16 bit words on CS deassertion > request, log violations verbosely. Other ideas? That's about it. Like I keep saying for any sort of generic use you really want to support 8 bits per word. > > You're missing the point. The controller chip select line can do what > > it likes, it's not connected to the target device if a GPIO is being > > used. > In my case SPI controller is wired directly to the codec with three > wires: SDIN, SCLK and CS. There are no registers that can control > either of these wires independently of others. Right, but other users are likely to exist and the framework has support for this so it's not much work to support. signature.asc Description: Digital signature
Re: [PATCH 1/3] spi: add xtfpga SPI controller driver
On Wed, Mar 12, 2014 at 12:20:49AM +0400, Max Filippov wrote:
> On Tue, Mar 11, 2014 at 11:49 PM, Mark Brown wrote:
> > On Tue, Mar 11, 2014 at 04:44:49PM +0400, Max Filippov wrote:
> >> + unsigned long timeout = jiffies + msecs_to_jiffies(100);
> >> + while (xtfpga_spi_read32(xspi, XTFPGA_SPI_BUSY)) {
> >> + if (!time_before(jiffies, timeout))
> >> + return -EBUSY;
> >> + else
> >> + cpu_relax();
> >> + }
> > So we'll busy wait for up to 100ms - that seems like an awfully long
> > time. Perhaps fall back to msleep() if the delay is non-trivial (or
> > just reduce the timeout)?
> The timeout is here for the unlikely case everything went wrong. Normally
> transfers get completed in about 10 useconds on 50 MHz hardware, it
> doesn't seem worth msleeping here. I put the timeout here just because
> otherwise infinite loop polling the device register looks scary.
I appreciate that but even with 5MHz that's three orders of magnitude
longer busy waiting in the error case than the operation is expected to
take. If you must wait for that long busy wait for a bit then start
sleeping.
>
> >> +/* Unused: this device controls its only CS automatically,
> >> + * deactivating it after every 16 bit transfer completion.
> >> + */
> > This is too limited to use with most SPI clients, they'll want to be
> > able to transmit more than one word (and the fact that only 16 bit words
> > are supported is also an issue, though that's easy enough to handle for
> > a bitbanging driver - I'd really strongly suggest supporting 8 bits per
> > word as well). Clients are pretty much going to need to use GPIO based
> > chip select, you should make sure that's supported and covered in the
> > binding.
> There's no hardware for that. This device is really dumb, it is specifically
> suited to control TLV320AIC23 which expects exactly 16 bit words, SPI
> mode 0.
This driver is not actually compatible with the tlv320aic23 driver since
it needs 8 bit words, you need to at least support that. You don't need
hardware in the controller to support a GPIO chip select, the whole
point is that the controller chip select isn't wired up and a GPIO is
used instead.
> >> +static void xtfpga_spi_chipselect(struct spi_device *spi, int is_on)
> >> +{
> >> +}
> > Omit this since it's empty.
> The bitbang side doesn't like when this callback is NULL and returns
> -EINVAL from spi_bitbang_start.
So fix that, but really it's trying to tell you that the hardware is far
too limited to work with many things.
signature.asc
Description: Digital signature
Re: [PATCH 1/3] spi: add xtfpga SPI controller driver
On Wed, Mar 12, 2014 at 5:11 AM, Mark Brown wrote:
> On Wed, Mar 12, 2014 at 04:59:47AM +0400, Max Filippov wrote:
>
>> And tlv320aic23 has the following regmap:
>
>> const struct regmap_config tlv320aic23_regmap = {
>> .reg_bits = 7,
>> .val_bits = 9,
>
>> and its SPI interface accordingly does the following in .probe:
>
>> spi->bits_per_word = 16
>> spi->mode = SPI_MODE_0;
>> ret = spi_setup(spi);
>
> So I just looked again - the SPI code isn't in mainline, there must
> be some out of tree patches here that can't have been tested since the
> driver was converted to regmap (or the byte swapping you're doing in the
> controller is buggy for 16 bits per word).
I'm successfully running this driver with the patches in your ASoC tree
branch topic/tlv320aic23.
--
Thanks.
-- Max
--
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: [PATCH 1/3] spi: add xtfpga SPI controller driver
On Wed, Mar 12, 2014 at 04:59:47AM +0400, Max Filippov wrote:
> On Wed, Mar 12, 2014 at 4:34 AM, Mark Brown wrote:
> > This driver is not actually compatible with the tlv320aic23 driver since
> > it needs 8 bit words, you need to at least support that. You don't need
> That's strange, because the codec datasheet says the following (section
> 3.1.1):
> A control word consists of 16 bits, starting with the MSB. The data bits are
> latched on the rising edge of SCLK. A rising edge on CS after the 16th rising
> clock edge latches the data word into the AIC (see Figure 3-1).
> And tlv320aic23 has the following regmap:
> const struct regmap_config tlv320aic23_regmap = {
> .reg_bits = 7,
> .val_bits = 9,
Yes, and regmap will format that itself for transmission in 8 bit words
so you don't want the SPI controller to also do byte swapping.
> and its SPI interface accordingly does the following in .probe:
> spi->bits_per_word = 16
> spi->mode = SPI_MODE_0;
> ret = spi_setup(spi);
That's buggy, drivers should never configure anything more than 8 bits
per word with regmap.
> > hardware in the controller to support a GPIO chip select, the whole
> > point is that the controller chip select isn't wired up and a GPIO is
> > used instead.
> Actually it's not GPIO. The controller asserts CS line once we set the
> start bit while the busy bit is cleared and deasserts it after 16 SCK
> pulses.
You're missing the point. The controller chip select line can do what
it likes, it's not connected to the target device if a GPIO is being
used.
> > So fix that, but really it's trying to tell you that the hardware is far
> > too limited to work with many things.
> Ok. It's not designed to work with many things. Should I just move this
> driver to the rest of the platform code under arch/xtensa/platform/xtfpga?
No, not if you intend to use generic drivers with it.
signature.asc
Description: Digital signature
Re: [PATCH 1/3] spi: add xtfpga SPI controller driver
On Wed, Mar 12, 2014 at 04:59:47AM +0400, Max Filippov wrote:
> And tlv320aic23 has the following regmap:
> const struct regmap_config tlv320aic23_regmap = {
> .reg_bits = 7,
> .val_bits = 9,
> and its SPI interface accordingly does the following in .probe:
> spi->bits_per_word = 16
> spi->mode = SPI_MODE_0;
> ret = spi_setup(spi);
So I just looked again - the SPI code isn't in mainline, there must
be some out of tree patches here that can't have been tested since the
driver was converted to regmap (or the byte swapping you're doing in the
controller is buggy for 16 bits per word).
signature.asc
Description: Digital signature
Re: [PATCH 1/3] spi: add xtfpga SPI controller driver
On Wed, Mar 12, 2014 at 5:08 AM, Mark Brown wrote:
> On Wed, Mar 12, 2014 at 04:59:47AM +0400, Max Filippov wrote:
>> On Wed, Mar 12, 2014 at 4:34 AM, Mark Brown wrote:
>
>> > This driver is not actually compatible with the tlv320aic23 driver since
>> > it needs 8 bit words, you need to at least support that. You don't need
>
>> That's strange, because the codec datasheet says the following (section
>> 3.1.1):
>
>> A control word consists of 16 bits, starting with the MSB. The data bits are
>> latched on the rising edge of SCLK. A rising edge on CS after the 16th rising
>> clock edge latches the data word into the AIC (see Figure 3-1).
>
>> And tlv320aic23 has the following regmap:
>
>> const struct regmap_config tlv320aic23_regmap = {
>> .reg_bits = 7,
>> .val_bits = 9,
>
> Yes, and regmap will format that itself for transmission in 8 bit words
> so you don't want the SPI controller to also do byte swapping.
>
>> and its SPI interface accordingly does the following in .probe:
>
>> spi->bits_per_word = 16
>> spi->mode = SPI_MODE_0;
>> ret = spi_setup(spi);
>
> That's buggy, drivers should never configure anything more than 8 bits
> per word with regmap.
Ok, so the driver should allow for 8 bit transfers and regmap will arrange
transfers as 8-bit pairs, making CS to be asserted for 16 bits, right?
Hmmm... I see the only way to support that with that hardware: advertise
8 bit support, buffer bytes up to 16 bits, send 16 bit words on CS deassertion
request, log violations verbosely. Other ideas?
>> > hardware in the controller to support a GPIO chip select, the whole
>> > point is that the controller chip select isn't wired up and a GPIO is
>> > used instead.
>
>> Actually it's not GPIO. The controller asserts CS line once we set the
>> start bit while the busy bit is cleared and deasserts it after 16 SCK
>> pulses.
>
> You're missing the point. The controller chip select line can do what
> it likes, it's not connected to the target device if a GPIO is being
> used.
In my case SPI controller is wired directly to the codec with three
wires: SDIN, SCLK and CS. There are no registers that can control
either of these wires independently of others.
--
Thanks.
-- Max
--
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: [PATCH 1/3] spi: add xtfpga SPI controller driver
On Wed, Mar 12, 2014 at 4:34 AM, Mark Brown wrote:
> On Wed, Mar 12, 2014 at 12:20:49AM +0400, Max Filippov wrote:
>> On Tue, Mar 11, 2014 at 11:49 PM, Mark Brown wrote:
>> > On Tue, Mar 11, 2014 at 04:44:49PM +0400, Max Filippov wrote:
>
>> >> + unsigned long timeout = jiffies + msecs_to_jiffies(100);
>> >> + while (xtfpga_spi_read32(xspi, XTFPGA_SPI_BUSY)) {
>> >> + if (!time_before(jiffies, timeout))
>> >> + return -EBUSY;
>> >> + else
>> >> + cpu_relax();
>> >> + }
>
>> > So we'll busy wait for up to 100ms - that seems like an awfully long
>> > time. Perhaps fall back to msleep() if the delay is non-trivial (or
>> > just reduce the timeout)?
>
>> The timeout is here for the unlikely case everything went wrong. Normally
>> transfers get completed in about 10 useconds on 50 MHz hardware, it
>> doesn't seem worth msleeping here. I put the timeout here just because
>> otherwise infinite loop polling the device register looks scary.
>
> I appreciate that but even with 5MHz that's three orders of magnitude
> longer busy waiting in the error case than the operation is expected to
> take. If you must wait for that long busy wait for a bit then start
> sleeping.
Ok, I'll fix that.
>> >> +/* Unused: this device controls its only CS automatically,
>> >> + * deactivating it after every 16 bit transfer completion.
>> >> + */
>
>> > This is too limited to use with most SPI clients, they'll want to be
>> > able to transmit more than one word (and the fact that only 16 bit words
>> > are supported is also an issue, though that's easy enough to handle for
>> > a bitbanging driver - I'd really strongly suggest supporting 8 bits per
>> > word as well). Clients are pretty much going to need to use GPIO based
>> > chip select, you should make sure that's supported and covered in the
>> > binding.
>
>> There's no hardware for that. This device is really dumb, it is specifically
>> suited to control TLV320AIC23 which expects exactly 16 bit words, SPI
>> mode 0.
>
> This driver is not actually compatible with the tlv320aic23 driver since
> it needs 8 bit words, you need to at least support that. You don't need
That's strange, because the codec datasheet says the following (section
3.1.1):
A control word consists of 16 bits, starting with the MSB. The data bits are
latched on the rising edge of SCLK. A rising edge on CS after the 16th rising
clock edge latches the data word into the AIC (see Figure 3-1).
And tlv320aic23 has the following regmap:
const struct regmap_config tlv320aic23_regmap = {
.reg_bits = 7,
.val_bits = 9,
and its SPI interface accordingly does the following in .probe:
spi->bits_per_word = 16
spi->mode = SPI_MODE_0;
ret = spi_setup(spi);
> hardware in the controller to support a GPIO chip select, the whole
> point is that the controller chip select isn't wired up and a GPIO is
> used instead.
Actually it's not GPIO. The controller asserts CS line once we set the
start bit while the busy bit is cleared and deasserts it after 16 SCK
pulses.
>> >> +static void xtfpga_spi_chipselect(struct spi_device *spi, int is_on)
>> >> +{
>> >> +}
>
>> > Omit this since it's empty.
>
>> The bitbang side doesn't like when this callback is NULL and returns
>> -EINVAL from spi_bitbang_start.
>
> So fix that, but really it's trying to tell you that the hardware is far
> too limited to work with many things.
Ok. It's not designed to work with many things. Should I just move this
driver to the rest of the platform code under arch/xtensa/platform/xtfpga?
--
Thanks.
-- Max
--
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: [PATCH 1/3] spi: add xtfpga SPI controller driver
On Tue, Mar 11, 2014 at 11:49 PM, Mark Brown wrote:
> On Tue, Mar 11, 2014 at 04:44:49PM +0400, Max Filippov wrote:
>
>> +static inline int xtfpga_spi_wait_busy(struct xtfpga_spi *xspi)
>> +{
>> + unsigned long timeout = jiffies + msecs_to_jiffies(100);
>> + while (xtfpga_spi_read32(xspi, XTFPGA_SPI_BUSY)) {
>> + if (!time_before(jiffies, timeout))
>> + return -EBUSY;
>> + else
>> + cpu_relax();
>> + }
>> + return 0;
>> +}
>
> So we'll busy wait for up to 100ms - that seems like an awfully long
> time. Perhaps fall back to msleep() if the delay is non-trivial (or
> just reduce the timeout)?
The timeout is here for the unlikely case everything went wrong. Normally
transfers get completed in about 10 useconds on 50 MHz hardware, it
doesn't seem worth msleeping here. I put the timeout here just because
otherwise infinite loop polling the device register looks scary.
> If there are large enough FIFOs dead
> reckoning a sleep for the expected transfer time might be helpful but it
> seems like there's no meaningful FIFO.
Right, there's no FIFO in that device.
>> +/* Unused: this device controls its only CS automatically,
>> + * deactivating it after every 16 bit transfer completion.
>> + */
>
> This is too limited to use with most SPI clients, they'll want to be
> able to transmit more than one word (and the fact that only 16 bit words
> are supported is also an issue, though that's easy enough to handle for
> a bitbanging driver - I'd really strongly suggest supporting 8 bits per
> word as well). Clients are pretty much going to need to use GPIO based
> chip select, you should make sure that's supported and covered in the
> binding.
There's no hardware for that. This device is really dumb, it is specifically
suited to control TLV320AIC23 which expects exactly 16 bit words, SPI
mode 0.
>> +static void xtfpga_spi_chipselect(struct spi_device *spi, int is_on)
>> +{
>> +}
>
> Omit this since it's empty.
The bitbang side doesn't like when this callback is NULL and returns
-EINVAL from spi_bitbang_start.
--
Thanks.
-- Max
--
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: [PATCH 1/3] spi: add xtfpga SPI controller driver
On Tue, Mar 11, 2014 at 04:44:49PM +0400, Max Filippov wrote:
> +static inline int xtfpga_spi_wait_busy(struct xtfpga_spi *xspi)
> +{
> + unsigned long timeout = jiffies + msecs_to_jiffies(100);
> + while (xtfpga_spi_read32(xspi, XTFPGA_SPI_BUSY)) {
> + if (!time_before(jiffies, timeout))
> + return -EBUSY;
> + else
> + cpu_relax();
> + }
> + return 0;
> +}
So we'll busy wait for up to 100ms - that seems like an awfully long
time. Perhaps fall back to msleep() if the delay is non-trivial (or
just reduce the timeout)? If there are large enough FIFOs dead
reckoning a sleep for the expected transfer time might be helpful but it
seems like there's no meaningful FIFO.
> +/* Unused: this device controls its only CS automatically,
> + * deactivating it after every 16 bit transfer completion.
> + */
This is too limited to use with most SPI clients, they'll want to be
able to transmit more than one word (and the fact that only 16 bit words
are supported is also an issue, though that's easy enough to handle for
a bitbanging driver - I'd really strongly suggest supporting 8 bits per
word as well). Clients are pretty much going to need to use GPIO based
chip select, you should make sure that's supported and covered in the
binding.
> +static void xtfpga_spi_chipselect(struct spi_device *spi, int is_on)
> +{
> +}
Omit this since it's empty.
signature.asc
Description: Digital signature
Re: [PATCH 1/3] spi: add xtfpga SPI controller driver
On Tue, Mar 11, 2014 at 04:44:49PM +0400, Max Filippov wrote:
+static inline int xtfpga_spi_wait_busy(struct xtfpga_spi *xspi)
+{
+ unsigned long timeout = jiffies + msecs_to_jiffies(100);
+ while (xtfpga_spi_read32(xspi, XTFPGA_SPI_BUSY)) {
+ if (!time_before(jiffies, timeout))
+ return -EBUSY;
+ else
+ cpu_relax();
+ }
+ return 0;
+}
So we'll busy wait for up to 100ms - that seems like an awfully long
time. Perhaps fall back to msleep() if the delay is non-trivial (or
just reduce the timeout)? If there are large enough FIFOs dead
reckoning a sleep for the expected transfer time might be helpful but it
seems like there's no meaningful FIFO.
+/* Unused: this device controls its only CS automatically,
+ * deactivating it after every 16 bit transfer completion.
+ */
This is too limited to use with most SPI clients, they'll want to be
able to transmit more than one word (and the fact that only 16 bit words
are supported is also an issue, though that's easy enough to handle for
a bitbanging driver - I'd really strongly suggest supporting 8 bits per
word as well). Clients are pretty much going to need to use GPIO based
chip select, you should make sure that's supported and covered in the
binding.
+static void xtfpga_spi_chipselect(struct spi_device *spi, int is_on)
+{
+}
Omit this since it's empty.
signature.asc
Description: Digital signature
Re: [PATCH 1/3] spi: add xtfpga SPI controller driver
On Tue, Mar 11, 2014 at 11:49 PM, Mark Brown broo...@kernel.org wrote:
On Tue, Mar 11, 2014 at 04:44:49PM +0400, Max Filippov wrote:
+static inline int xtfpga_spi_wait_busy(struct xtfpga_spi *xspi)
+{
+ unsigned long timeout = jiffies + msecs_to_jiffies(100);
+ while (xtfpga_spi_read32(xspi, XTFPGA_SPI_BUSY)) {
+ if (!time_before(jiffies, timeout))
+ return -EBUSY;
+ else
+ cpu_relax();
+ }
+ return 0;
+}
So we'll busy wait for up to 100ms - that seems like an awfully long
time. Perhaps fall back to msleep() if the delay is non-trivial (or
just reduce the timeout)?
The timeout is here for the unlikely case everything went wrong. Normally
transfers get completed in about 10 useconds on 50 MHz hardware, it
doesn't seem worth msleeping here. I put the timeout here just because
otherwise infinite loop polling the device register looks scary.
If there are large enough FIFOs dead
reckoning a sleep for the expected transfer time might be helpful but it
seems like there's no meaningful FIFO.
Right, there's no FIFO in that device.
+/* Unused: this device controls its only CS automatically,
+ * deactivating it after every 16 bit transfer completion.
+ */
This is too limited to use with most SPI clients, they'll want to be
able to transmit more than one word (and the fact that only 16 bit words
are supported is also an issue, though that's easy enough to handle for
a bitbanging driver - I'd really strongly suggest supporting 8 bits per
word as well). Clients are pretty much going to need to use GPIO based
chip select, you should make sure that's supported and covered in the
binding.
There's no hardware for that. This device is really dumb, it is specifically
suited to control TLV320AIC23 which expects exactly 16 bit words, SPI
mode 0.
+static void xtfpga_spi_chipselect(struct spi_device *spi, int is_on)
+{
+}
Omit this since it's empty.
The bitbang side doesn't like when this callback is NULL and returns
-EINVAL from spi_bitbang_start.
--
Thanks.
-- Max
--
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: [PATCH 1/3] spi: add xtfpga SPI controller driver
On Wed, Mar 12, 2014 at 4:34 AM, Mark Brown broo...@kernel.org wrote:
On Wed, Mar 12, 2014 at 12:20:49AM +0400, Max Filippov wrote:
On Tue, Mar 11, 2014 at 11:49 PM, Mark Brown broo...@kernel.org wrote:
On Tue, Mar 11, 2014 at 04:44:49PM +0400, Max Filippov wrote:
+ unsigned long timeout = jiffies + msecs_to_jiffies(100);
+ while (xtfpga_spi_read32(xspi, XTFPGA_SPI_BUSY)) {
+ if (!time_before(jiffies, timeout))
+ return -EBUSY;
+ else
+ cpu_relax();
+ }
So we'll busy wait for up to 100ms - that seems like an awfully long
time. Perhaps fall back to msleep() if the delay is non-trivial (or
just reduce the timeout)?
The timeout is here for the unlikely case everything went wrong. Normally
transfers get completed in about 10 useconds on 50 MHz hardware, it
doesn't seem worth msleeping here. I put the timeout here just because
otherwise infinite loop polling the device register looks scary.
I appreciate that but even with 5MHz that's three orders of magnitude
longer busy waiting in the error case than the operation is expected to
take. If you must wait for that long busy wait for a bit then start
sleeping.
Ok, I'll fix that.
+/* Unused: this device controls its only CS automatically,
+ * deactivating it after every 16 bit transfer completion.
+ */
This is too limited to use with most SPI clients, they'll want to be
able to transmit more than one word (and the fact that only 16 bit words
are supported is also an issue, though that's easy enough to handle for
a bitbanging driver - I'd really strongly suggest supporting 8 bits per
word as well). Clients are pretty much going to need to use GPIO based
chip select, you should make sure that's supported and covered in the
binding.
There's no hardware for that. This device is really dumb, it is specifically
suited to control TLV320AIC23 which expects exactly 16 bit words, SPI
mode 0.
This driver is not actually compatible with the tlv320aic23 driver since
it needs 8 bit words, you need to at least support that. You don't need
That's strange, because the codec datasheet says the following (section
3.1.1):
A control word consists of 16 bits, starting with the MSB. The data bits are
latched on the rising edge of SCLK. A rising edge on CS after the 16th rising
clock edge latches the data word into the AIC (see Figure 3-1).
And tlv320aic23 has the following regmap:
const struct regmap_config tlv320aic23_regmap = {
.reg_bits = 7,
.val_bits = 9,
and its SPI interface accordingly does the following in .probe:
spi-bits_per_word = 16
spi-mode = SPI_MODE_0;
ret = spi_setup(spi);
hardware in the controller to support a GPIO chip select, the whole
point is that the controller chip select isn't wired up and a GPIO is
used instead.
Actually it's not GPIO. The controller asserts CS line once we set the
start bit while the busy bit is cleared and deasserts it after 16 SCK
pulses.
+static void xtfpga_spi_chipselect(struct spi_device *spi, int is_on)
+{
+}
Omit this since it's empty.
The bitbang side doesn't like when this callback is NULL and returns
-EINVAL from spi_bitbang_start.
So fix that, but really it's trying to tell you that the hardware is far
too limited to work with many things.
Ok. It's not designed to work with many things. Should I just move this
driver to the rest of the platform code under arch/xtensa/platform/xtfpga?
--
Thanks.
-- Max
--
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: [PATCH 1/3] spi: add xtfpga SPI controller driver
On Wed, Mar 12, 2014 at 05:43:49AM +0400, Max Filippov wrote: On Wed, Mar 12, 2014 at 5:08 AM, Mark Brown broo...@kernel.org wrote: That's buggy, drivers should never configure anything more than 8 bits per word with regmap. Ok, so the driver should allow for 8 bit transfers and regmap will arrange transfers as 8-bit pairs, making CS to be asserted for 16 bits, right? Hmmm... I see the only way to support that with that hardware: advertise 8 bit support, buffer bytes up to 16 bits, send 16 bit words on CS deassertion request, log violations verbosely. Other ideas? That's about it. Like I keep saying for any sort of generic use you really want to support 8 bits per word. You're missing the point. The controller chip select line can do what it likes, it's not connected to the target device if a GPIO is being used. In my case SPI controller is wired directly to the codec with three wires: SDIN, SCLK and CS. There are no registers that can control either of these wires independently of others. Right, but other users are likely to exist and the framework has support for this so it's not much work to support. signature.asc Description: Digital signature
Re: [PATCH 1/3] spi: add xtfpga SPI controller driver
On Wed, Mar 12, 2014 at 04:59:47AM +0400, Max Filippov wrote:
On Wed, Mar 12, 2014 at 4:34 AM, Mark Brown broo...@kernel.org wrote:
This driver is not actually compatible with the tlv320aic23 driver since
it needs 8 bit words, you need to at least support that. You don't need
That's strange, because the codec datasheet says the following (section
3.1.1):
A control word consists of 16 bits, starting with the MSB. The data bits are
latched on the rising edge of SCLK. A rising edge on CS after the 16th rising
clock edge latches the data word into the AIC (see Figure 3-1).
And tlv320aic23 has the following regmap:
const struct regmap_config tlv320aic23_regmap = {
.reg_bits = 7,
.val_bits = 9,
Yes, and regmap will format that itself for transmission in 8 bit words
so you don't want the SPI controller to also do byte swapping.
and its SPI interface accordingly does the following in .probe:
spi-bits_per_word = 16
spi-mode = SPI_MODE_0;
ret = spi_setup(spi);
That's buggy, drivers should never configure anything more than 8 bits
per word with regmap.
hardware in the controller to support a GPIO chip select, the whole
point is that the controller chip select isn't wired up and a GPIO is
used instead.
Actually it's not GPIO. The controller asserts CS line once we set the
start bit while the busy bit is cleared and deasserts it after 16 SCK
pulses.
You're missing the point. The controller chip select line can do what
it likes, it's not connected to the target device if a GPIO is being
used.
So fix that, but really it's trying to tell you that the hardware is far
too limited to work with many things.
Ok. It's not designed to work with many things. Should I just move this
driver to the rest of the platform code under arch/xtensa/platform/xtfpga?
No, not if you intend to use generic drivers with it.
signature.asc
Description: Digital signature
Re: [PATCH 1/3] spi: add xtfpga SPI controller driver
On Wed, Mar 12, 2014 at 5:11 AM, Mark Brown broo...@kernel.org wrote:
On Wed, Mar 12, 2014 at 04:59:47AM +0400, Max Filippov wrote:
And tlv320aic23 has the following regmap:
const struct regmap_config tlv320aic23_regmap = {
.reg_bits = 7,
.val_bits = 9,
and its SPI interface accordingly does the following in .probe:
spi-bits_per_word = 16
spi-mode = SPI_MODE_0;
ret = spi_setup(spi);
So I just looked again - the SPI code isn't in mainline, there must
be some out of tree patches here that can't have been tested since the
driver was converted to regmap (or the byte swapping you're doing in the
controller is buggy for 16 bits per word).
I'm successfully running this driver with the patches in your ASoC tree
branch topic/tlv320aic23.
--
Thanks.
-- Max
--
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: [PATCH 1/3] spi: add xtfpga SPI controller driver
On Wed, Mar 12, 2014 at 04:59:47AM +0400, Max Filippov wrote:
And tlv320aic23 has the following regmap:
const struct regmap_config tlv320aic23_regmap = {
.reg_bits = 7,
.val_bits = 9,
and its SPI interface accordingly does the following in .probe:
spi-bits_per_word = 16
spi-mode = SPI_MODE_0;
ret = spi_setup(spi);
So I just looked again - the SPI code isn't in mainline, there must
be some out of tree patches here that can't have been tested since the
driver was converted to regmap (or the byte swapping you're doing in the
controller is buggy for 16 bits per word).
signature.asc
Description: Digital signature
Re: [PATCH 1/3] spi: add xtfpga SPI controller driver
On Wed, Mar 12, 2014 at 12:20:49AM +0400, Max Filippov wrote:
On Tue, Mar 11, 2014 at 11:49 PM, Mark Brown broo...@kernel.org wrote:
On Tue, Mar 11, 2014 at 04:44:49PM +0400, Max Filippov wrote:
+ unsigned long timeout = jiffies + msecs_to_jiffies(100);
+ while (xtfpga_spi_read32(xspi, XTFPGA_SPI_BUSY)) {
+ if (!time_before(jiffies, timeout))
+ return -EBUSY;
+ else
+ cpu_relax();
+ }
So we'll busy wait for up to 100ms - that seems like an awfully long
time. Perhaps fall back to msleep() if the delay is non-trivial (or
just reduce the timeout)?
The timeout is here for the unlikely case everything went wrong. Normally
transfers get completed in about 10 useconds on 50 MHz hardware, it
doesn't seem worth msleeping here. I put the timeout here just because
otherwise infinite loop polling the device register looks scary.
I appreciate that but even with 5MHz that's three orders of magnitude
longer busy waiting in the error case than the operation is expected to
take. If you must wait for that long busy wait for a bit then start
sleeping.
+/* Unused: this device controls its only CS automatically,
+ * deactivating it after every 16 bit transfer completion.
+ */
This is too limited to use with most SPI clients, they'll want to be
able to transmit more than one word (and the fact that only 16 bit words
are supported is also an issue, though that's easy enough to handle for
a bitbanging driver - I'd really strongly suggest supporting 8 bits per
word as well). Clients are pretty much going to need to use GPIO based
chip select, you should make sure that's supported and covered in the
binding.
There's no hardware for that. This device is really dumb, it is specifically
suited to control TLV320AIC23 which expects exactly 16 bit words, SPI
mode 0.
This driver is not actually compatible with the tlv320aic23 driver since
it needs 8 bit words, you need to at least support that. You don't need
hardware in the controller to support a GPIO chip select, the whole
point is that the controller chip select isn't wired up and a GPIO is
used instead.
+static void xtfpga_spi_chipselect(struct spi_device *spi, int is_on)
+{
+}
Omit this since it's empty.
The bitbang side doesn't like when this callback is NULL and returns
-EINVAL from spi_bitbang_start.
So fix that, but really it's trying to tell you that the hardware is far
too limited to work with many things.
signature.asc
Description: Digital signature
Re: [PATCH 1/3] spi: add xtfpga SPI controller driver
On Wed, Mar 12, 2014 at 5:08 AM, Mark Brown broo...@kernel.org wrote:
On Wed, Mar 12, 2014 at 04:59:47AM +0400, Max Filippov wrote:
On Wed, Mar 12, 2014 at 4:34 AM, Mark Brown broo...@kernel.org wrote:
This driver is not actually compatible with the tlv320aic23 driver since
it needs 8 bit words, you need to at least support that. You don't need
That's strange, because the codec datasheet says the following (section
3.1.1):
A control word consists of 16 bits, starting with the MSB. The data bits are
latched on the rising edge of SCLK. A rising edge on CS after the 16th rising
clock edge latches the data word into the AIC (see Figure 3-1).
And tlv320aic23 has the following regmap:
const struct regmap_config tlv320aic23_regmap = {
.reg_bits = 7,
.val_bits = 9,
Yes, and regmap will format that itself for transmission in 8 bit words
so you don't want the SPI controller to also do byte swapping.
and its SPI interface accordingly does the following in .probe:
spi-bits_per_word = 16
spi-mode = SPI_MODE_0;
ret = spi_setup(spi);
That's buggy, drivers should never configure anything more than 8 bits
per word with regmap.
Ok, so the driver should allow for 8 bit transfers and regmap will arrange
transfers as 8-bit pairs, making CS to be asserted for 16 bits, right?
Hmmm... I see the only way to support that with that hardware: advertise
8 bit support, buffer bytes up to 16 bits, send 16 bit words on CS deassertion
request, log violations verbosely. Other ideas?
hardware in the controller to support a GPIO chip select, the whole
point is that the controller chip select isn't wired up and a GPIO is
used instead.
Actually it's not GPIO. The controller asserts CS line once we set the
start bit while the busy bit is cleared and deasserts it after 16 SCK
pulses.
You're missing the point. The controller chip select line can do what
it likes, it's not connected to the target device if a GPIO is being
used.
In my case SPI controller is wired directly to the codec with three
wires: SDIN, SCLK and CS. There are no registers that can control
either of these wires independently of others.
--
Thanks.
-- Max
--
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/
