| Message ID | 1406013141-18552-1-git-send-email-thierry.reding@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Thierry, Thanks for the patch. On 07/22/2014 09:12 AM, Thierry Reding wrote: > From: Thierry Reding <treding@nvidia.com> > > This function returns the value of the struct mipi_dsi_host_ops' > .transfer() so make sure the return types are consistent. > > Signed-off-by: Thierry Reding <treding@nvidia.com> Acked-by: Andrzej Hajda <a.hajda@samsung.com> -- Regards Andrzej > --- > drivers/gpu/drm/drm_mipi_dsi.c | 4 ++-- > drivers/gpu/drm/panel/panel-s6e8aa0.c | 4 ++-- > include/drm/drm_mipi_dsi.h | 4 ++-- > 3 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c > index e633df2f68d8..6d2fd2077dae 100644 > --- a/drivers/gpu/drm/drm_mipi_dsi.c > +++ b/drivers/gpu/drm/drm_mipi_dsi.c > @@ -205,8 +205,8 @@ EXPORT_SYMBOL(mipi_dsi_detach); > * @data: pointer to the command followed by parameters > * @len: length of @data > */ > -int mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel, > - const void *data, size_t len) > +ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel, > + const void *data, size_t len) > { > const struct mipi_dsi_host_ops *ops = dsi->host->ops; > struct mipi_dsi_msg msg = { > diff --git a/drivers/gpu/drm/panel/panel-s6e8aa0.c b/drivers/gpu/drm/panel/panel-s6e8aa0.c > index 06e57a26db7a..beb43492b649 100644 > --- a/drivers/gpu/drm/panel/panel-s6e8aa0.c > +++ b/drivers/gpu/drm/panel/panel-s6e8aa0.c > @@ -133,14 +133,14 @@ static int s6e8aa0_clear_error(struct s6e8aa0 *ctx) > static void s6e8aa0_dcs_write(struct s6e8aa0 *ctx, const void *data, size_t len) > { > struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev); > - int ret; > + ssize_t ret; > > if (ctx->error < 0) > return; > > ret = mipi_dsi_dcs_write(dsi, dsi->channel, data, len); > if (ret < 0) { > - dev_err(ctx->dev, "error %d writing dcs seq: %*ph\n", ret, len, > + dev_err(ctx->dev, "error %zd writing dcs seq: %*ph\n", ret, len, > data); > ctx->error = ret; > } > diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h > index efa1b552adc5..4b0112781910 100644 > --- a/include/drm/drm_mipi_dsi.h > +++ b/include/drm/drm_mipi_dsi.h > @@ -127,8 +127,8 @@ struct mipi_dsi_device { > > int mipi_dsi_attach(struct mipi_dsi_device *dsi); > int mipi_dsi_detach(struct mipi_dsi_device *dsi); > -int mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel, > - const void *data, size_t len); > +ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel, > + const void *data, size_t len); > ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, unsigned int channel, > u8 cmd, void *data, size_t len); >
Hi, On 07/22/2014 04:28 PM, Andrzej Hajda wrote: > Hi Thierry, > > Thanks for the patch. > > On 07/22/2014 09:12 AM, Thierry Reding wrote: >> From: Thierry Reding <treding@nvidia.com> >> >> This function returns the value of the struct mipi_dsi_host_ops' >> .transfer() so make sure the return types are consistent. >> >> Signed-off-by: Thierry Reding <treding@nvidia.com> > > Acked-by: Andrzej Hajda <a.hajda@samsung.com> > -- > Regards > Andrzej >> --- >> drivers/gpu/drm/drm_mipi_dsi.c | 4 ++-- >> drivers/gpu/drm/panel/panel-s6e8aa0.c | 4 ++-- >> include/drm/drm_mipi_dsi.h | 4 ++-- >> 3 files changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c >> index e633df2f68d8..6d2fd2077dae 100644 >> --- a/drivers/gpu/drm/drm_mipi_dsi.c >> +++ b/drivers/gpu/drm/drm_mipi_dsi.c >> @@ -205,8 +205,8 @@ EXPORT_SYMBOL(mipi_dsi_detach); >> * @data: pointer to the command followed by parameters >> * @len: length of @data >> */ >> -int mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel, >> - const void *data, size_t len) >> +ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel, >> + const void *data, size_t len) >> { >> const struct mipi_dsi_host_ops *ops = dsi->host->ops; >> struct mipi_dsi_msg msg = { >> diff --git a/drivers/gpu/drm/panel/panel-s6e8aa0.c b/drivers/gpu/drm/panel/panel-s6e8aa0.c >> index 06e57a26db7a..beb43492b649 100644 >> --- a/drivers/gpu/drm/panel/panel-s6e8aa0.c >> +++ b/drivers/gpu/drm/panel/panel-s6e8aa0.c >> @@ -133,14 +133,14 @@ static int s6e8aa0_clear_error(struct s6e8aa0 *ctx) >> static void s6e8aa0_dcs_write(struct s6e8aa0 *ctx, const void *data, size_t len) >> { >> struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev); >> - int ret; >> + ssize_t ret; >> >> if (ctx->error < 0) >> return; >> >> ret = mipi_dsi_dcs_write(dsi, dsi->channel, data, len); >> if (ret < 0) { >> - dev_err(ctx->dev, "error %d writing dcs seq: %*ph\n", ret, len, >> + dev_err(ctx->dev, "error %zd writing dcs seq: %*ph\n", ret, len, >> data); >> ctx->error = ret; One more thing! This 'ctx->error' type is 'int'. So it should be changed from int to ssize_t in struct s6e8aa0. Thank you. Best regards YJ >> } >> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h >> index efa1b552adc5..4b0112781910 100644 >> --- a/include/drm/drm_mipi_dsi.h >> +++ b/include/drm/drm_mipi_dsi.h >> @@ -127,8 +127,8 @@ struct mipi_dsi_device { >> >> int mipi_dsi_attach(struct mipi_dsi_device *dsi); >> int mipi_dsi_detach(struct mipi_dsi_device *dsi); >> -int mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel, >> - const void *data, size_t len); >> +ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel, >> + const void *data, size_t len); >> ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, unsigned int channel, >> u8 cmd, void *data, size_t len); >> > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel >
On 07/22/2014 11:50 AM, YoungJun Cho wrote: > Hi, > > On 07/22/2014 04:28 PM, Andrzej Hajda wrote: >> Hi Thierry, >> >> Thanks for the patch. >> >> On 07/22/2014 09:12 AM, Thierry Reding wrote: >>> From: Thierry Reding <treding@nvidia.com> >>> >>> This function returns the value of the struct mipi_dsi_host_ops' >>> .transfer() so make sure the return types are consistent. >>> >>> Signed-off-by: Thierry Reding <treding@nvidia.com> >> Acked-by: Andrzej Hajda <a.hajda@samsung.com> >> -- >> Regards >> Andrzej >>> --- >>> drivers/gpu/drm/drm_mipi_dsi.c | 4 ++-- >>> drivers/gpu/drm/panel/panel-s6e8aa0.c | 4 ++-- >>> include/drm/drm_mipi_dsi.h | 4 ++-- >>> 3 files changed, 6 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c >>> index e633df2f68d8..6d2fd2077dae 100644 >>> --- a/drivers/gpu/drm/drm_mipi_dsi.c >>> +++ b/drivers/gpu/drm/drm_mipi_dsi.c >>> @@ -205,8 +205,8 @@ EXPORT_SYMBOL(mipi_dsi_detach); >>> * @data: pointer to the command followed by parameters >>> * @len: length of @data >>> */ >>> -int mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel, >>> - const void *data, size_t len) >>> +ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel, >>> + const void *data, size_t len) >>> { >>> const struct mipi_dsi_host_ops *ops = dsi->host->ops; >>> struct mipi_dsi_msg msg = { >>> diff --git a/drivers/gpu/drm/panel/panel-s6e8aa0.c b/drivers/gpu/drm/panel/panel-s6e8aa0.c >>> index 06e57a26db7a..beb43492b649 100644 >>> --- a/drivers/gpu/drm/panel/panel-s6e8aa0.c >>> +++ b/drivers/gpu/drm/panel/panel-s6e8aa0.c >>> @@ -133,14 +133,14 @@ static int s6e8aa0_clear_error(struct s6e8aa0 *ctx) >>> static void s6e8aa0_dcs_write(struct s6e8aa0 *ctx, const void *data, size_t len) >>> { >>> struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev); >>> - int ret; >>> + ssize_t ret; >>> >>> if (ctx->error < 0) >>> return; >>> >>> ret = mipi_dsi_dcs_write(dsi, dsi->channel, data, len); >>> if (ret < 0) { >>> - dev_err(ctx->dev, "error %d writing dcs seq: %*ph\n", ret, len, >>> + dev_err(ctx->dev, "error %zd writing dcs seq: %*ph\n", ret, len, >>> data); >>> ctx->error = ret; > One more thing! > This 'ctx->error' type is 'int'. So it should be changed from int to > ssize_t in struct s6e8aa0. I do not think so. ctx->error contains always error code, and this is guarded here by 'if (ret < 0)' clause. Regards Andrzej > > Thank you. > Best regards YJ > >>> } >>> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h >>> index efa1b552adc5..4b0112781910 100644 >>> --- a/include/drm/drm_mipi_dsi.h >>> +++ b/include/drm/drm_mipi_dsi.h >>> @@ -127,8 +127,8 @@ struct mipi_dsi_device { >>> >>> int mipi_dsi_attach(struct mipi_dsi_device *dsi); >>> int mipi_dsi_detach(struct mipi_dsi_device *dsi); >>> -int mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel, >>> - const void *data, size_t len); >>> +ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel, >>> + const void *data, size_t len); >>> ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, unsigned int channel, >>> u8 cmd, void *data, size_t len); >>> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel >> >
Hi Thierry, YoungJun's comment refreshed my memory about mipi_dsi_dcs_write return value. It should be rather int than ssize_t. Why? .transfer() returns the number of read bytes or error, but in case of dcs write no bytes are read, so it in fact returns error or 0. This is why return value was implemented originally as int. So I do not think this patch is necessary. Regards Andrzej On 07/22/2014 12:05 PM, Andrzej Hajda wrote: > On 07/22/2014 11:50 AM, YoungJun Cho wrote: >> Hi, >> >> On 07/22/2014 04:28 PM, Andrzej Hajda wrote: >>> Hi Thierry, >>> >>> Thanks for the patch. >>> >>> On 07/22/2014 09:12 AM, Thierry Reding wrote: >>>> From: Thierry Reding <treding@nvidia.com> >>>> >>>> This function returns the value of the struct mipi_dsi_host_ops' >>>> .transfer() so make sure the return types are consistent. >>>> >>>> Signed-off-by: Thierry Reding <treding@nvidia.com> >>> Acked-by: Andrzej Hajda <a.hajda@samsung.com> >>> -- >>> Regards >>> Andrzej >>>> --- >>>> drivers/gpu/drm/drm_mipi_dsi.c | 4 ++-- >>>> drivers/gpu/drm/panel/panel-s6e8aa0.c | 4 ++-- >>>> include/drm/drm_mipi_dsi.h | 4 ++-- >>>> 3 files changed, 6 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c >>>> index e633df2f68d8..6d2fd2077dae 100644 >>>> --- a/drivers/gpu/drm/drm_mipi_dsi.c >>>> +++ b/drivers/gpu/drm/drm_mipi_dsi.c >>>> @@ -205,8 +205,8 @@ EXPORT_SYMBOL(mipi_dsi_detach); >>>> * @data: pointer to the command followed by parameters >>>> * @len: length of @data >>>> */ >>>> -int mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel, >>>> - const void *data, size_t len) >>>> +ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel, >>>> + const void *data, size_t len) >>>> { >>>> const struct mipi_dsi_host_ops *ops = dsi->host->ops; >>>> struct mipi_dsi_msg msg = { >>>> diff --git a/drivers/gpu/drm/panel/panel-s6e8aa0.c b/drivers/gpu/drm/panel/panel-s6e8aa0.c >>>> index 06e57a26db7a..beb43492b649 100644 >>>> --- a/drivers/gpu/drm/panel/panel-s6e8aa0.c >>>> +++ b/drivers/gpu/drm/panel/panel-s6e8aa0.c >>>> @@ -133,14 +133,14 @@ static int s6e8aa0_clear_error(struct s6e8aa0 *ctx) >>>> static void s6e8aa0_dcs_write(struct s6e8aa0 *ctx, const void *data, size_t len) >>>> { >>>> struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev); >>>> - int ret; >>>> + ssize_t ret; >>>> >>>> if (ctx->error < 0) >>>> return; >>>> >>>> ret = mipi_dsi_dcs_write(dsi, dsi->channel, data, len); >>>> if (ret < 0) { >>>> - dev_err(ctx->dev, "error %d writing dcs seq: %*ph\n", ret, len, >>>> + dev_err(ctx->dev, "error %zd writing dcs seq: %*ph\n", ret, len, >>>> data); >>>> ctx->error = ret; >> One more thing! >> This 'ctx->error' type is 'int'. So it should be changed from int to >> ssize_t in struct s6e8aa0. > I do not think so. ctx->error contains always error code, and this is > guarded here > by 'if (ret < 0)' clause. > > Regards > Andrzej >> >> Thank you. >> Best regards YJ >> >>>> } >>>> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h >>>> index efa1b552adc5..4b0112781910 100644 >>>> --- a/include/drm/drm_mipi_dsi.h >>>> +++ b/include/drm/drm_mipi_dsi.h >>>> @@ -127,8 +127,8 @@ struct mipi_dsi_device { >>>> >>>> int mipi_dsi_attach(struct mipi_dsi_device *dsi); >>>> int mipi_dsi_detach(struct mipi_dsi_device *dsi); >>>> -int mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel, >>>> - const void *data, size_t len); >>>> +ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel, >>>> + const void *data, size_t len); >>>> ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, unsigned int channel, >>>> u8 cmd, void *data, size_t len); >>>> >>> _______________________________________________ >>> dri-devel mailing list >>> dri-devel@lists.freedesktop.org >>> http://lists.freedesktop.org/mailman/listinfo/dri-devel >>> >>
On Tue, Jul 22, 2014 at 12:23:07PM +0200, Andrzej Hajda wrote: > Hi Thierry, > > YoungJun's comment refreshed my memory about mipi_dsi_dcs_write return > value. It should be rather int than ssize_t. Why? > .transfer() returns the number of read bytes or error, but in case > of dcs write no bytes are read, so it in fact returns error or 0. > This is why return value was implemented originally as int. > So I do not think this patch is necessary. I think it should return the number of bytes written or an error. That way we give callers the maximum amount of information. They may still choose to only handle < 0 for convenience, but at least the information will be there should it become required at some point. Thierry
On 07/23/2014 09:27 AM, Thierry Reding wrote: > On Tue, Jul 22, 2014 at 12:23:07PM +0200, Andrzej Hajda wrote: >> Hi Thierry, >> >> YoungJun's comment refreshed my memory about mipi_dsi_dcs_write return >> value. It should be rather int than ssize_t. Why? >> .transfer() returns the number of read bytes or error, but in case >> of dcs write no bytes are read, so it in fact returns error or 0. >> This is why return value was implemented originally as int. >> So I do not think this patch is necessary. > I think it should return the number of bytes written or an error. That > way we give callers the maximum amount of information. They may still > choose to only handle < 0 for convenience, but at least the information > will be there should it become required at some point. AFAIK DSI write operation is atomic, ie either all requested bytes have been sent either there was an error and in such case we should assume no byte have been sent. Returning number of written bytes in this case is just returning length of write buffer - something caller already knows. Additionally it suggests that partial write is possible which is not true. I know it is implemented such way in i2c (which is non atomic) but the comment in i2c_transfer shows its weakness [1]. [1]: http://lxr.free-electrons.com/source/drivers/i2c/i2c-core.c#L1782 Regards Andrzej > > Thierry
diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c index e633df2f68d8..6d2fd2077dae 100644 --- a/drivers/gpu/drm/drm_mipi_dsi.c +++ b/drivers/gpu/drm/drm_mipi_dsi.c @@ -205,8 +205,8 @@ EXPORT_SYMBOL(mipi_dsi_detach); * @data: pointer to the command followed by parameters * @len: length of @data */ -int mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel, - const void *data, size_t len) +ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel, + const void *data, size_t len) { const struct mipi_dsi_host_ops *ops = dsi->host->ops; struct mipi_dsi_msg msg = { diff --git a/drivers/gpu/drm/panel/panel-s6e8aa0.c b/drivers/gpu/drm/panel/panel-s6e8aa0.c index 06e57a26db7a..beb43492b649 100644 --- a/drivers/gpu/drm/panel/panel-s6e8aa0.c +++ b/drivers/gpu/drm/panel/panel-s6e8aa0.c @@ -133,14 +133,14 @@ static int s6e8aa0_clear_error(struct s6e8aa0 *ctx) static void s6e8aa0_dcs_write(struct s6e8aa0 *ctx, const void *data, size_t len) { struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev); - int ret; + ssize_t ret; if (ctx->error < 0) return; ret = mipi_dsi_dcs_write(dsi, dsi->channel, data, len); if (ret < 0) { - dev_err(ctx->dev, "error %d writing dcs seq: %*ph\n", ret, len, + dev_err(ctx->dev, "error %zd writing dcs seq: %*ph\n", ret, len, data); ctx->error = ret; } diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h index efa1b552adc5..4b0112781910 100644 --- a/include/drm/drm_mipi_dsi.h +++ b/include/drm/drm_mipi_dsi.h @@ -127,8 +127,8 @@ struct mipi_dsi_device { int mipi_dsi_attach(struct mipi_dsi_device *dsi); int mipi_dsi_detach(struct mipi_dsi_device *dsi); -int mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel, - const void *data, size_t len); +ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel, + const void *data, size_t len); ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, unsigned int channel, u8 cmd, void *data, size_t len);