[PATCH v2 1/3] drm: Add SCDC helpers

[Ville Syrjälä]

On Mon, Dec 05, 2016 at 12:16:52PM +0100, Thierry Reding wrote:
> On Mon, Dec 05, 2016 at 10:12:39AM +0000, Jose Abreu wrote:
> > On 02-12-2016 19:24, Thierry Reding wrote:
> [...]
> > > +/**
> > > + * drm_scdc_write - write a block of data to SCDC
> > > + * @adapter: I2C controller
> > > + * @offset: start offset of block to write
> > > + * @buffer: block of data to write
> > > + * @size: size of the block to write
> > > + *
> > > + * Writes a block of data to SCDC, starting at a given offset.
> > > + *
> > > + * Returns:
> > > + * The number of bytes written to SCDC or a negative error code on failure.
> > > + */
> > > +ssize_t drm_scdc_write(struct i2c_adapter *adapter, u8 offset,
> > > +		       const void *buffer, size_t size)
> > > +{
> > > +	struct i2c_msg msg = {
> > > +		.addr = SCDC_I2C_SLAVE_ADDRESS,
> > > +		.flags = 0,
> > > +		.len = 1 + size,
> > > +		.buf = NULL,
> > > +	};
> > > +	void *data;
> > > +	int err;
> > > +
> > > +	data = kmalloc(1 + size, GFP_TEMPORARY);
> > > +	if (!data)
> > > +		return -ENOMEM;
> > > +
> > > +	msg.buf = data;
> > > +
> > > +	memcpy(data, &offset, sizeof(offset));
> > > +	memcpy(data + 1, buffer, size);
> > 
> > Don't you agree it would be better if you use the same scheme as
> > drm_scdc_read()? Something like:
> > 
> > struct i2c_msg msgs[] = {
> >     {
> >         .addr = SCDC_I2C_SLAVE_ADDRESS,
> >         .flags = 0,
> >         .len = 1,
> >         .buf = &offset,
> >     }, {
> >         .addr = SCDC_I2C_SLAVE_ADDRESS,
> >         .flags = 0,
> >         .len = size,
> >         .buf = buffer,
> >     },
> > };
> 
> Ville had a similar comment on a prior iteration. It looks as if the
> above should work, but it's probably best to test it a little more
> widely to make sure we're not running into cases where it breaks.

AFAICS the spec says we shouldn't use a repeated start for writes.
I guess it might work for some devices, but going against the spec
sounds questionable to me.

> 
> Have you by any chance verified that it works on your hardware?
> 
> Thierry



-- 
Ville Syrjälä
Intel OTC