On Fri, Feb 11, 2022 at 03:33:55PM +0100, Javier Martinez Canillas wrote:
> This adds a DRM driver for SSD1305, SSD1306, SSD1307 and SSD1309 Solomon
> OLED display controllers.
>
> It's only the core part of the driver and a bus specific driver is needed
> for each transport interface supported by the display controllers.
...
> +#define SSD130X_SET_CLOCK_DIV_MASK GENMASK(3, 0)
> +#define SSD130X_SET_CLOCK_DIV_SET(val)
> FIELD_PREP(SSD130X_SET_CLOCK_DIV_MASK, (val))
> +#define SSD130X_SET_CLOCK_FREQ_MASK GENMASK(7, 4)
> +#define SSD130X_SET_CLOCK_FREQ_SET(val)
> FIELD_PREP(SSD130X_SET_CLOCK_FREQ_MASK, (val))
> +#define SSD130X_SET_PRECHARGE_PERIOD1_MASK GENMASK(3, 0)
> +#define SSD130X_SET_PRECHARGE_PERIOD1_SET(val)
> FIELD_PREP(SSD130X_SET_PRECHARGE_PERIOD1_MASK, (val))
> +#define SSD130X_SET_PRECHARGE_PERIOD2_MASK GENMASK(7, 4)
> +#define SSD130X_SET_PRECHARGE_PERIOD2_SET(val)
> FIELD_PREP(SSD130X_SET_PRECHARGE_PERIOD2_MASK, (val))
> +#define SSD130X_SET_COM_PINS_CONFIG1_MASK GENMASK(4, 4)
BIT(4)
> +#define SSD130X_SET_COM_PINS_CONFIG1_SET(val)
> FIELD_PREP(SSD130X_SET_COM_PINS_CONFIG1_MASK, (!val))
> +#define SSD130X_SET_COM_PINS_CONFIG2_MASK GENMASK(5, 5)
BIT(5)
> +#define SSD130X_SET_COM_PINS_CONFIG2_SET(val)
> FIELD_PREP(SSD130X_SET_COM_PINS_CONFIG2_MASK, (val))
I would put GENMASK() directly into FIELD(), but it's up to you
(and I haven't checked the use of *_MASK anyway).
...
> +static int ssd130x_write_data(struct ssd130x_device *ssd130x, u8 *values,
> int count)
> +{
> + int ret;
> +
> + ret = regmap_bulk_write(ssd130x->regmap, SSD130X_DATA, values, count);
> + if (ret)
> + return ret;
> +
> + return 0;
return regmap_bulk_write(...);
> +}
...
> +/*
> + * Helper to write command (SSD130X_COMMAND). The fist variadic argument
> + * is the command to write and the following are the command options.
> + *
> + * Note that the ssd130x protocol requires each command and option to be
> + * written as a SSD130X_COMMAND device register value. That is why a call
> + * to regmap_write(..., SSD130X_COMMAND, ...) is done for each argument.
> + */
Thanks!
> +static int ssd130x_write_cmd(struct ssd130x_device *ssd130x, int count,
> + /* u8 cmd, u8 option, ... */...)
> +{
> + va_list ap;
> + u8 value;
> + int ret;
> +
> + va_start(ap, count);
> +
> + do {
> + value = va_arg(ap, int);
> + ret = regmap_write(ssd130x->regmap, SSD130X_COMMAND, (u8)value);
Wondering if you really need this casting. value is u8 by definition.
> + if (ret)
> + goto out_end;
> + } while (--count);
> +
> +out_end:
> + va_end(ap);
> +
> + return ret;
> +}
...
> + ssd130x = devm_drm_dev_alloc(dev, &ssd130x_drm_driver,
> + struct ssd130x_device, drm);
> + if (IS_ERR(ssd130x)) {
> + dev_err_probe(dev, PTR_ERR(ssd130x),
> + "Failed to allocate DRM device\n");
> + return ssd130x;
This...
> + }
...
> + bl = devm_backlight_device_register(dev, dev_name(dev), dev, ssd130x,
> + &ssd130xfb_bl_ops, NULL);
> + if (IS_ERR(bl))
> + return ERR_PTR(dev_err_probe(dev, PTR_ERR(bl),
> + "Unable to register backlight
> device\n"));
Can be consistent with this then.
--
With Best Regards,
Andy Shevchenko
