Am Sonntag, den 07.04.2013, 14:11 +0200 schrieb Rafał Miłecki:
> Thanks for comments!
>
> 2013/4/7 Paul Menzel <paulepanter@xxxxxxxxxxxxxxxxxxxxx>:
> > Am Sonntag, den 07.04.2013, 12:52 +0200 schrieb Rafał Miłecki:
> >> +struct cea_sad *drm_edid_to_sad(struct edid *edid)
> >> +{
> >> + struct cea_sad *sads = NULL;
> >
> > No need to set it NULL, as it gets assigned later on? Looks like in the
> > end there is a corner case, where nothing gets assigned in the
> > `for_each_cea_db` loop. So the compiler is going to complain.
>
> Right, I can drop that. Curious, my gcc didn't complain.
Oh, I did not check with GCC. But there is that corner case, if I am not
mistaken.
> >> + if (cea_revision(cea) < 3) {
> >> + DRM_DEBUG_KMS("SAD: wrong CEA revision\n");
> >> + return NULL;
> >> + }
> >> +
> >> + if (cea_db_offsets(cea, &start, &end)) {
> >> + DRM_DEBUG_KMS("SAD: invalid data block offsets\n");
> >> + return NULL;
> >> + }
> >
> > Could the above be put in some helper too:
> > `is_cea_valid_for_version(cea, version)`?
>
> Not sure if it's worth a helper. But maybe I'll go and just use a single check?
> if (cea_revision(cea) < 3 || cea_db_offsets(...))
I kind of like the useful error messages, you put there. So if no
helper, then I’d prefer if you leave it as is.
> >> + for_each_cea_db(cea, i, start, end) {
> >> + db = &cea[i];
> >> + dbl = cea_db_payload_len(db);
> >> +
> >> + if (cea_db_tag(db) == AUDIO_BLOCK) {
> >> + int count = dbl / 3; /* SAD is 3B */
> >> + int j;
> >> +
> >> + sads = kzalloc(count + 1 * sizeof(*sads), GFP_KERNEL);
> >> + if (!sads)
> >
> > Add an error too? »kzallac failed« or something like this?
>
> Allocation failures are aloud enough on the lower level. I was
> corrected few times already to don't put warnings when alloc fails. If
> you want me to point to the discussions, I'll have to search though.
No need. I believe you.
> >> +#define SAD_FORMAT_LPCM 0x01
> >> +#define SAD_FORMAT_AC3 0x02
> >
> > At least in my MUA indentation looks different.
>
> It's just a matter of .patch format. Patch file has "+" sign at a
> beginning of line and \t can look different when viewing patch. Please
> apply a patch and check drm_edid.h then - it'll be OK.
Thanks. Sorry for not checking that myself.
Thanks,
paul
Attachment:
signature.asc
Description: This is a digitally signed message part
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
