max1363 driver comments

max1363 driver comments

[Hennerich, Michael]

Hi Jonathan,


/* Todo: test this */
int max1363_single_channel_from_ring(long mask, struct max1363_state *st)
{
        unsigned long numvals;
        int count =3D 0, ret;
        u8 *ring_data;
        if (!(st->current_mode->modemask & mask)) {
                ret =3D -EBUSY;
                goto error_ret;
        }
        numvals =3D hweight_long(st->current_mode->modemask);

        ring_data =3D kmalloc(numvals*2, GFP_KERNEL);
        if (ring_data =3D=3D NULL) {
                ret =3D -ENOMEM;
                goto error_ret;
        }
        ret =3D st->indio_dev->ring->access.read_last(st->indio_dev->ring,
                                                ring_data);
        if (ret)
                goto error_free_ring_data;
        /* Need a count of channels prior to this one */
        mask >>=3D 1;
        while (mask) {
                if (mask && st->current_mode->modemask)
                        count++;
                mask >>=3D 1;
        }
        if (st->chip_info->bits !=3D 8)
                return ((int)(ring_data[count*2 + 0] & 0x0F) << 8)
                        + (int)(ring_data[count*2 + 1]);
        else
                return ring_data[count];

error_free_ring_data:
        kfree(ring_data);
error_ret:
        return ret;
}

1) Where does ring_data get freed?
2) if (mask && st->current_mode->modemask)? Shouldn't this be a Bitwise AND=
 & ?


static int __devinit max1363_probe(struct i2c_client *client,
                                   const struct i2c_device_id *id)
{

[--snip--]

        /* Find the chip model specific data */
        for (i =3D 0; i < ARRAY_SIZE(max1363_chip_info_tbl); i++)
                if (!strcmp(max1363_chip_info_tbl[i].name, id->name)) {
                        st->chip_info =3D &max1363_chip_info_tbl[i];
                        break;
                };

Isn't this what id->driver_data is for?

st->chip_info =3D &max1363_chip_info_tbl[id->driver_data];

In case you want to make this bullet prove - use distinct offsets.
See example below.

/* max1363 and max1368 tested - rest from data sheet */
static const struct max1363_chip_info max1363_chip_info_tbl[] =3D {
-       {
+       [max1361] =3D {
                .name =3D "max1361",
                .num_inputs =3D 4,
                .bits =3D 10,
                .int_vref_mv =3D 2048,
                .monitor_mode =3D 1,
                .mode_list =3D max1363_mode_list,
                .num_modes =3D ARRAY_SIZE(max1363_mode_list),
                .default_mode =3D s0to3,
                .dev_attrs =3D &max1363_dev_attr_group,
                .scan_attrs =3D &max1363_scan_el_group,
        }, {

-Michael


------------------------------------------------------------------
********* Analog Devices GmbH              Open Platform Solutions
**  *****
**     ** Wilhelm-Wagenfeld-Strasse 6
**  ***** D-80807 Munich
********* Germany
Registergericht M=FCnchen HRB 40368,  Gesch=E4ftsf=FChrer: Thomas Wessel, W=
illiam A. Martin, Margaret K. Seif

Re: max1363 driver comments

[Jonathan Cameron]

Hi Michael,
> 
> /* Todo: test this */
> int max1363_single_channel_from_ring(long mask, struct max1363_state *st)
> {
>         unsigned long numvals;
>         int count = 0, ret;
>         u8 *ring_data;
>         if (!(st->current_mode->modemask & mask)) {
>                 ret = -EBUSY;
>                 goto error_ret;
>         }
>         numvals = hweight_long(st->current_mode->modemask);
> 
>         ring_data = kmalloc(numvals*2, GFP_KERNEL);
>         if (ring_data == NULL) {
>                 ret = -ENOMEM;
>                 goto error_ret;
>         }
>         ret = st->indio_dev->ring->access.read_last(st->indio_dev->ring,
>                                                 ring_data);
>         if (ret)
>                 goto error_free_ring_data;
>         /* Need a count of channels prior to this one */
>         mask >>= 1;
>         while (mask) {
>                 if (mask && st->current_mode->modemask)
>                         count++;
>                 mask >>= 1;
>         }
>         if (st->chip_info->bits != 8)
>                 return ((int)(ring_data[count*2 + 0] & 0x0F) << 8)
>                         + (int)(ring_data[count*2 + 1]);
>         else
>                 return ring_data[count];
> 
> error_free_ring_data:
>         kfree(ring_data);
> error_ret:
>         return ret;
> }
> 
> 1) Where does ring_data get freed?
Good point.  i really should have looked at this more carefully.

I guess at somepoint I took that dynamic and clean forgot to change the exit path
appropriately.

> 2) if (mask && st->current_mode->modemask)? Shouldn't this be a Bitwise AND & ?
Yes.
> 
> 
> static int __devinit max1363_probe(struct i2c_client *client,
>                                    const struct i2c_device_id *id)
> {
> 
> [--snip--]
> 
>         /* Find the chip model specific data */
>         for (i = 0; i < ARRAY_SIZE(max1363_chip_info_tbl); i++)
>                 if (!strcmp(max1363_chip_info_tbl[i].name, id->name)) {
>                         st->chip_info = &max1363_chip_info_tbl[i];
>                         break;
>                 };
> 
> Isn't this what id->driver_data is for?
> 
> st->chip_info = &max1363_chip_info_tbl[id->driver_data];
> 
> In case you want to make this bullet prove - use distinct offsets.
Also a good point.  Ouch that piece of stupidity has been
there a very long time. Might be worth squashing the name field
in that structure as well. Can easily get that from the id table.
> See example below.
> 
> /* max1363 and max1368 tested - rest from data sheet */
> static const struct max1363_chip_info max1363_chip_info_tbl[] = {
> -       {
> +       [max1361] = {
>                 .name = "max1361",
>                 .num_inputs = 4,
>                 .bits = 10,
>                 .int_vref_mv = 2048,
>                 .monitor_mode = 1,
>                 .mode_list = max1363_mode_list,
>                 .num_modes = ARRAY_SIZE(max1363_mode_list),
>                 .default_mode = s0to3,
>                 .dev_attrs = &max1363_dev_attr_group,
>                 .scan_attrs = &max1363_scan_el_group,
>         }, {
> 
I'll put together a patch to fix these sometime in the next few days.

Thanks for taking a look at this driver.  I fear it has previously sneaked
through as the least read of all the drivers (by merit of being complex
and rather dull!)

Jonathan