[RFC][PATCH] drm: add helper extracting SADs from EDID

[Paul Menzel]

Am Sonntag, den 07.04.2013, 14:11 +0200 schrieb Rafał Miłecki:
> Thanks for comments!
> 
> 2013/4/7 Paul Menzel <paulepanter at users.sourceforge.net>:
> > 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20130407/d4f707db/attachment.pgp>