Messages in this thread | | | From | Andy Shevchenko <> | Date | Sun, 12 Feb 2017 13:50:23 +0200 | Subject | Re: [PATCH v4 2/7] drm/tinydrm: Add helper functions |
| |
On Sat, Feb 11, 2017 at 8:48 PM, Noralf Trønnes <noralf@tronnes.org> wrote: > Add common functionality needed by many tinydrm drivers.
> +int tinydrm_enable_backlight(struct backlight_device *backlight) > +{ > + unsigned int old_state; > + int ret; > + > + if (!backlight) > + return 0; > + > + old_state = backlight->props.state; > + backlight->props.state &= ~BL_CORE_FBBLANK; > + DRM_DEBUG_KMS("Backlight state: 0x%x -> 0x%x\n", old_state, > + backlight->props.state);
"%#x" ? (And elsewhere)
> +#if IS_ENABLED(CONFIG_SPI)
> +size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t max_len) > +{ > + size_t ret; > +
> + ret = min(spi_max_transfer_size(spi), spi->master->max_dma_len);
I don't get why DMA constrain somehow affects this framework?
What if max_dma_len is zero (imagine SPI master that works only by PIO by some reason)?
> + if (max_len) > + ret = min(ret, max_len); > + if (spi_max) > + ret = min_t(size_t, ret, spi_max);
> + ret &= ~0x3;
Why alignment is that? Why do we need it? Isn't a busyness of SPI framework to cope with it?
> + if (ret < 4)
It's effectively check for 0.
> + ret = 4; > + > + return ret;
> +} > +EXPORT_SYMBOL(tinydrm_spi_max_transfer_size);
> +static void > +tinydrm_dbg_spi_print(struct spi_device *spi, struct spi_transfer *tr, > + const void *buf, int idx, bool tx) > +{ > + u32 speed_hz = tr->speed_hz ? tr->speed_hz : spi->max_speed_hz; > + char linebuf[3 * 32]; > +
> + hex_dump_to_buffer(buf, tr->len, 16, > + DIV_ROUND_UP(tr->bits_per_word, 8), > + linebuf, sizeof(linebuf), false);
> + > + printk(KERN_DEBUG > + " tr(%i): speed=%u%s, bpw=%i, len=%u, %s_buf=[%s%s]\n", idx, > + speed_hz > 1000000 ? speed_hz / 1000000 : speed_hz / 1000, > + speed_hz > 1000000 ? "MHz" : "kHz", tr->bits_per_word, tr->len, > + tx ? "tx" : "rx", linebuf, tr->len > 16 ? " ..." : "");
I hope at some point we will have some extension to print speeds, sizes and so on based on algo in string_get_size().
> +}
> +int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz, > + struct spi_transfer *header, u8 bpw, const void *buf, > + size_t len) > +{ > + struct spi_transfer tr = { > + .bits_per_word = bpw, > + .speed_hz = speed_hz, > + }; > + struct spi_message m; > + u16 *swap_buf = NULL; > + size_t max_chunk; > + size_t chunk; > + int ret = 0; > + > + if (WARN_ON_ONCE(bpw != 8 && bpw != 16)) > + return -EINVAL; > + > + max_chunk = tinydrm_spi_max_transfer_size(spi, 0); > +
> + if (drm_debug & DRM_UT_DRIVER) > + pr_debug("[drm:%s] bpw=%u, max_chunk=%zu, transfers:\n", > + __func__, bpw, max_chunk);
For all of your dev_dbg() / pr_debug() __func__ argument might be redundant. Dynamic Debug may include this by request from user.
> +/** > + * tinydrm_machine_little_endian - Machine is little endian > + * > + * Returns: > + * true if *defined(__LITTLE_ENDIAN)*, false otherwise > + */ > +static inline bool tinydrm_machine_little_endian(void) > +{ > +#if defined(__LITTLE_ENDIAN) > + return true; > +#else > + return false; > +#endif > +}
Hmm... What is the typical code of a caller for this?
-- With Best Regards, Andy Shevchenko
| |