Re: [PATCH v5 3/6] drm: Add driver for Solomon SSD130x OLED displays

[Javier Martinez Canillas]

Hello Andy,

On 2/11/22 17:13, Andy Shevchenko wrote:

[snip]

> 
>> +#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)
>

I actually thought about that when using these macros and considered
just using BIT(N) instead but at the end decided to do GENMASK(n, n)
for two reasons:

1) It better documents what this is about, that's bitmask of 1 -bit.
   One of the main advantages of using these macros for me is to better
   express what these are, otherwise could just use 1 << n or whatever.

2) It's consistent with the other definitions for bitmasks that have
   more than one bit.
   
Looked at other drivers using these macros and noticed that is not
uncommon to have GENMASK(n, n), so I went for that.
 
>> +#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).
>

Same. I also considered just using GENMASK() directly, but since I was
already reworking these, I thought that having the _MASK constant macros
would make the code more explicit about these being masks and what for.

> 
> ...
> 
>> +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(...);
>

Sure.

>> +}
> 
> ...
> 
>> +/*
>> + * 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!
>

You are welcome.
 
>> +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.
>

Yeah, I'll drop it too.

[snip]

>> +	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.
> 

Yes. I meant to change it everywhere but seems that one slipped it through.

It's not worth to send a v6 just for the changes you mentioned but I can do
them before pushing the patches to drm-misc (once I get ack for this patch).

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat