Re: [PATCH v4 2/7] drm/tinydrm: Add helper functions

[Andy Shevchenko]

On Sat, Feb 11, 2017 at 8:48 PM, Noralf TrÃnnes <noralf@xxxxxxxxxxx> 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