On Thu, May 09, 2019 at 08:32:20AM +0200, Jochen Henneberg wrote:
> Mark Brown <broonie@xxxxxxxxxx> writes:
> > On Mon, May 06, 2019 at 02:06:20PM +0200, Jochen Henneberg wrote:
> >> + mutex_lock(&chip->chn_config_lock);
> >> + for (i = 0; i < CP2130_NUM_GPIOS; i++) {
> >> + chn = &chip->chn_configs[i];
> >> + ret += sprintf(out, "%d\t%d\t%d\t%d\t\t%d\t\t%d\t\t%s\t%d"
> >> + "\t\t%d\t\t\t%d\t\t%d\t\t'%s'\n",
> >> + i, chn->cs_en, chn->irq_pin, chn->clock_phase,
> >> + chn->polarity, chn->cs_pin_mode,
> >> + cp2130_spi_speed_to_string(chn->clock_freq),
> >> + chn->delay_mask, chn->inter_byte_delay,
> >> + chn->pre_deassert_delay, chn->post_assert_delay,
> >> + chn->modalias);
> >> + strcat(buf, out);
> >> + }
> > This looks like a bunch of mostly very generic diagnostic data, if it's
> > useful to have it should be added in the framework so it's available for
> > all drivers.
> The information is quite specific for the CP2130 so I cannot see how
> this could fit into the SPI framework.
All those delays, polarities and speeds look very generic.
> We could use the timing information that comes with each SPI transfer to
> setup the transport parameters of the chip, however, there are several
> settings that may be incomplete. E. g. the IRQ pin. If the SPI slave
> chip IRQ is connected to one of the GPIOs of CP2130 nobody knows upfront
> which IRQ to configure for the slave chip driver. Same issue applies for
> the CS pin, there is pre-numbered GPIO available for CS before the
> CP2130 is plugged so you cannot setup other driver in advance.
This is the same problem as all the plugin modules for non-enumerable
buses like the Raspberry Pi have, they currently use things like DT or
ACPI overlays to enumerate - there are some efforts at improving things
as it's not ideal at the minute. I'd expect you to be trying to use
similar interfaces to them rather than inventing something completely
driver specific, users shouldn't have to figure out some random driver
specific interface for this.
> If the chip is permanently connected (e. g. in an embedded board, which
> is unlikely because those often have host SPI ports anyway) we may have
> an advantage from DT pre-configuration but I think this use-case is
> quite unlikely and then there would still be the problem to know which
> data is valid, the one that comes with the transfer message or the one
> configured from sysfs.
This is one reason why you shouldn't have a random sysfs interface.
> >> + /* iterate through all transfers */
> >> + list_for_each_entry(xfer, &mesg->transfers, transfer_list) {
> >> + dev_dbg(&master->dev, "spi transfer stats: %p, %p, %d",
> >> + xfer->tx_buf, xfer->rx_buf, xfer->len);
> > It's not clear to me why the driver can't use transfer_one() instead of
> > transfer_one_message().
> The documentation says that if both callbacks are provided the framework
> will always use transfer_one_message() which I think is the superior
> callback because we can keep the SPI configuration as it is if the same
No, it's better to use transfer_one() if you can as it means there is
less open coding of standard features in the driver.
> channel is used with subsequent transfers (performance) which we cannot
> do for transfer_one(), at least if the driver should remain stateless
> for the transfers.
It's perfectly OK to cache the last settings that were sent to the
hardware and only reconfigure if there's a change, several drivers do
that already.
Attachment:
signature.asc
Description: PGP signature
