On Mon, Dec 01, 2014 at 02:48:47PM +0100, Hans Verkuil wrote:
> Hi Thierry,
>
> Thanks for the review, see my comments below.
>
> On 12/01/2014 02:15 PM, Thierry Reding wrote:
> > On Fri, Nov 28, 2014 at 03:50:50PM +0100, Hans Verkuil wrote:
[...]
> >> +{
> >> + switch (type) {
> >> + case HDMI_INFOFRAME_TYPE_VENDOR: return "Vendor";
> >> + case HDMI_INFOFRAME_TYPE_AVI: return "Auxiliary Video Information (AVI)";
> >> + case HDMI_INFOFRAME_TYPE_SPD: return "Source Product Description (SPD)";
> >> + case HDMI_INFOFRAME_TYPE_AUDIO: return "Audio";
> >
> > I'd prefer "case ...:" and "return ...;" on separate lines for
> > readability.
>
> I actually think that makes it *less* readable. If you really want that, then I'll
> change it, but I would suggest that you try it yourself first to see if it is
> really more readable for you. It isn't for me, so I'll keep this for the next
> version.
I did, and I still think separate lines are more readable, especially if
you throw in a blank line after the "return ...;". Anyway, I could keep
my OCD in check if it weren't for the fact that half of these are the
cause for checkpatch to complain. And then if you change the ones that
checkpatch wants you to change, all the others would be inconsistent and
then I'd complain about the inconsistency...
checkpatch flagged a couple other issues, please make sure to address
those as well.
> > Maybe include the numerical value here? Of course that either means that
> > callers must pass in a buffer or we sacrifice thread-safety. The buffer
> > could be optional, somewhat like this:
> >
> > const char *hdmi_infoframe_get_name(char *buffer, size_t length,
> > enum hdmi_infoframe_type type)
> > {
> > const char *name = NULL;
> >
> > switch (type) {
> > case HDMI_INFOFRAME_TYPE_VENDOR:
> > name = "Vendor";
> > break;
> > ...
> > }
> >
> > if (buffer) {
> > if (!name)
> > snprintf(buffer, length, "unknown (%d)", type);
> > else
> > snprintf(buffer, length, name);
> >
> > name = buffer;
> > }
> >
> > return name;
> > }
> >
> > That way the function would be generally useful and could even be made
> > publicly available.
>
> I would do this only where it makes sense. Some of these fields have only one or
> two reserved bits left, and in that case is it easier to just say something
> like "Reserved (3)" and do that for each reserved value.
Okay.
> >> +/**
> >> + * hdmi_infoframe_log() - log info of HDMI infoframe
> >> + * @dev: device
> >> + * @frame: HDMI infoframe
> >> + */
> >> +void hdmi_infoframe_log(struct device *dev, union hdmi_infoframe *frame)
> >> +{
> >> + switch (frame->any.type) {
> >> + case HDMI_INFOFRAME_TYPE_AVI:
> >> + hdmi_avi_infoframe_log(dev, &frame->avi);
> >> + break;
> >> + case HDMI_INFOFRAME_TYPE_SPD:
> >> + hdmi_spd_infoframe_log(dev, &frame->spd);
> >> + break;
> >> + case HDMI_INFOFRAME_TYPE_AUDIO:
> >> + hdmi_audio_infoframe_log(dev, &frame->audio);
> >> + break;
> >> + case HDMI_INFOFRAME_TYPE_VENDOR:
> >> + hdmi_vendor_any_infoframe_log(dev, &frame->vendor);
> >> + break;
> >> + default:
> >> + WARN(1, "Bad infoframe type %d\n", frame->any.type);
> >
> > Does it make sense for this to be WARN? It's perfectly legal for future
> > devices to expose new types of infoframes. Perhaps even expected. But if
> > we want to keep this here to help get bug reports so that we don't
> > forget to update this code, then maybe we should do the same wherever we
> > query the name of enum values above.
>
> I'll drop the WARN from the log function. I think it should also be dropped
> from the unpack. The only place it makes sense is for pack() since there the
> data comes from the driver, not from an external source.
Sounds good.
Thierry
Attachment:
pgpO9IPplJMGd.pgp
Description: PGP signature
