const members of struct iio_chan_spec?

const members of struct iio_chan_spec?

[Hennerich, Michael]

Hi Jonathan,

static int ad7280_channel_init(struct ad7280_state *st)
{
        int dev, ch, cnt;

        st->channels =3D kzalloc(sizeof(*st->channels) *
                        ((st->slave_num + 1) * 12 + 1), GFP_KERNEL);
        if (st->channels =3D=3D NULL)
                return -ENOMEM;

        for (dev =3D 0, cnt =3D 0; dev <=3D st->slave_num; dev++)
                for (ch =3D AD7280A_CELL_VOLTAGE_1; ch <=3D AD7280A_AUX_ADC=
_6; ch++, cnt++) {
                        st->channels[cnt].type =3D IIO_IN;
                        st->channels[cnt].indexed =3D 1;
                        st->channels[cnt].extend_name =3D NULL;
                        st->channels[cnt].channel =3D cnt;
                        st->channels[cnt].info_mask =3D (1 << IIO_CHAN_INFO=
_SCALE_SHARED);
                        st->channels[cnt].address =3D dev << 8 | ch;
                        st->channels[cnt].scan_index =3D cnt;
                        st->channels[cnt].scan_type.sign =3D 'u';
                        st->channels[cnt].scan_type.realbits =3D 12;
                        st->channels[cnt].scan_type.storagebits =3D 32;
                        st->channels[cnt].scan_type.shift =3D 0;
                }
        return cnt;
}

Trying to dynamically allocate and populate a channel spec.
However some members are declared const. Do they really need to be const?

drivers/staging/iio/adc/ad7280a_core.c: In function 'ad7280_channel_init':
drivers/staging/iio/adc/ad7280a_core.c:231: error: assignment of read-only =
member 'info_mask'

Regards,
Michael

------------------------------------------------------------------
********* Analog Devices GmbH
**  *****
**     ** Wilhelm-Wagenfeld-Strasse 6
**  ***** D-80807 Munich
********* Germany
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368;
Geschaeftsfuehrer: Dr.Carsten Suckrow, Thomas Wessel, William A. Martin, Ma=
rgaret Seif

Re: const members of struct iio_chan_spec?

[Jonathan Cameron]

On 06/30/11 15:04, Hennerich, Michael wrote:
> Hi Jonathan,
> 
> static int ad7280_channel_init(struct ad7280_state *st)
> {
>         int dev, ch, cnt;
> 
>         st->channels = kzalloc(sizeof(*st->channels) *
>                         ((st->slave_num + 1) * 12 + 1), GFP_KERNEL);
>         if (st->channels == NULL)
>                 return -ENOMEM;
> 
>         for (dev = 0, cnt = 0; dev <= st->slave_num; dev++)
>                 for (ch = AD7280A_CELL_VOLTAGE_1; ch <= AD7280A_AUX_ADC_6; ch++, cnt++) {
>                         st->channels[cnt].type = IIO_IN;
>                         st->channels[cnt].indexed = 1;
>                         st->channels[cnt].extend_name = NULL;
>                         st->channels[cnt].channel = cnt;
>                         st->channels[cnt].info_mask = (1 << IIO_CHAN_INFO_SCALE_SHARED);
>                         st->channels[cnt].address = dev << 8 | ch;
>                         st->channels[cnt].scan_index = cnt;
>                         st->channels[cnt].scan_type.sign = 'u';
>                         st->channels[cnt].scan_type.realbits = 12;
>                         st->channels[cnt].scan_type.storagebits = 32;
>                         st->channels[cnt].scan_type.shift = 0;
>                 }
>         return cnt;
> }
> 
> Trying to dynamically allocate and populate a channel spec.
> However some members are declared const. Do they really need to be const?
hmm.. technically no, although a large point in the purpose of moving to chan spec
based registration was to allow all this stuff to be constant static arrays.  I guess
there is no particular harm in taking them away from const though.  The functions
should all assume they are constant, but they don't really need to be before any are
called.

As to whether it is the right thing to do here, I guess it depends on how large slave num
can be. Google tells me the answer is really quite large (50)!  Hence I'm convinced you
have a very good reason.

I'll push back on any drivers doing dynamic just to handle small numbers of channels with
variants, but here I'm convinced you have a very good reason.

Please send the patch to make these elements non constant with your driver (as that will
clearly provide justification)

I'll admit I never conceived of anyone having the ability to chain this many sensors.
For smaller chained devices I'd just advise doing them as a big static array then setting
num_channels appropriately. (glad you brought this up as it just made me realise my bug in the
previous set I sent out!)

Jonathan

> 
> drivers/staging/iio/adc/ad7280a_core.c: In function 'ad7280_channel_init':
> drivers/staging/iio/adc/ad7280a_core.c:231: error: assignment of read-only member 'info_mask'
> 
> Regards,
> Michael
> 
> ------------------------------------------------------------------
> ********* Analog Devices GmbH
> **  *****
> **     ** Wilhelm-Wagenfeld-Strasse 6
> **  ***** D-80807 Munich
> ********* Germany
> Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368;
> Geschaeftsfuehrer: Dr.Carsten Suckrow, Thomas Wessel, William A. Martin, Margaret Seif
> 
> 
> 

RE: const members of struct iio_chan_spec?

[Hennerich, Michael]

Jonathan Cameron wrote on 2011-06-30:
> On 06/30/11 15:04, Hennerich, Michael wrote:
>> Hi Jonathan,
>>
>> static int ad7280_channel_init(struct ad7280_state *st) {
>>         int dev, ch, cnt;
>>
>>         st->channels = kzalloc(sizeof(*st->channels) *
>>                         ((st->slave_num + 1) * 12 + 1), GFP_KERNEL);
>>         if (st->channels == NULL)
>>                 return -ENOMEM;
>>         for (dev = 0, cnt = 0; dev <= st->slave_num; dev++)
>>                 for (ch = AD7280A_CELL_VOLTAGE_1; ch <=
> AD7280A_AUX_ADC_6; ch++, cnt++) {
>>                         st->channels[cnt].type = IIO_IN;
>>                         st->channels[cnt].indexed = 1;
>>                         st->channels[cnt].extend_name = NULL;
>>                         st->channels[cnt].channel = cnt;
>>                         st->channels[cnt].info_mask = (1 <<
>>                         IIO_CHAN_INFO_SCALE_SHARED);
>>                         st->channels[cnt].address = dev << 8 | ch;
>>                         st->channels[cnt].scan_index = cnt;
>>                         st->channels[cnt].scan_type.sign = 'u';
>>                         st->channels[cnt].scan_type.realbits = 12;
>>                         st->channels[cnt].scan_type.storagebits = 32;
>>                         st->channels[cnt].scan_type.shift = 0;
>>                 }
>>         return cnt;
>> }
>>
>> Trying to dynamically allocate and populate a channel spec.
>> However some members are declared const. Do they really need to be
> const?
> hmm.. technically no, although a large point in the purpose of moving
> to chan spec based registration was to allow all this stuff to be
> constant static arrays.  I guess there is no particular harm in taking
> them away from const though.  The functions should all assume they are
> constant, but they don't really need to be before any are called.
>
> As to whether it is the right thing to do here, I guess it depends on
> how large slave num can be. Google tells me the answer is really quite
> large (50)!  Hence I'm convinced you have a very good reason.

Well that's true for the AD7280. It supports up to 50 chained devices each handling
6 cells. However this drivers targets the AD7280A which only supports 8 chain devices.

Still a good reason?

> I'll push back on any drivers doing dynamic just to handle small
> numbers of channels with variants, but here I'm convinced you have a
> very good reason.
>
> Please send the patch to make these elements non constant with your
> driver (as that will clearly provide justification)
>
> I'll admit I never conceived of anyone having the ability to chain
> this many sensors.
> For smaller chained devices I'd just advise doing them as a big static
> array then setting num_channels appropriately. (glad you brought this
> up as it just made me realise my bug in the previous set I sent out!)
>
> Jonathan
>
>>
>> drivers/staging/iio/adc/ad7280a_core.c: In function
>> 'ad7280_channel_init': drivers/staging/iio/adc/ad7280a_core.c:231:
>> error: assignment of read-only member 'info_mask'
>>
>> Regards,
>> Michael
>>
>> ------------------------------------------------------------------
>> ********* Analog Devices GmbH
>> **  *****
>> **     ** Wilhelm-Wagenfeld-Strasse 6
>> **  ***** D-80807 Munich
>> ********* Germany
>> Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB
>> 40368;
>> Geschaeftsfuehrer: Dr.Carsten Suckrow, Thomas Wessel, William A.
>> Martin, Margaret Seif
>>
>>
>>
>

Greetings,
Michael

--
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368; Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin, Margaret Seif

RE: const members of struct iio_chan_spec?

[jonathan.cameron@gmail.com]

"Hennerich, Michael" <Michael.Hennerich@analog.com> wrote:

>Jonathan Cameron wrote on 2011-06-30:
>> On 06/30/11 15:04, Hennerich, Michael wrote:
>>> Hi Jonathan,
>>>
>>> static int ad7280_channel_init(struct ad7280_state *st) {
>>>         int dev, ch, cnt;
>>>
>>>         st->channels = kzalloc(sizeof(*st->channels) *
>>>                         ((st->slave_num + 1) * 12 + 1), GFP_KERNEL);
>>>         if (st->channels == NULL)
>>>                 return -ENOMEM;
>>>         for (dev = 0, cnt = 0; dev <= st->slave_num; dev++)
>>>                 for (ch = AD7280A_CELL_VOLTAGE_1; ch <=
>> AD7280A_AUX_ADC_6; ch++, cnt++) {
>>>                         st->channels[cnt].type = IIO_IN;
>>>                         st->channels[cnt].indexed = 1;
>>>                         st->channels[cnt].extend_name = NULL;
>>>                         st->channels[cnt].channel = cnt;
>>>                         st->channels[cnt].info_mask = (1 <<
>>>                         IIO_CHAN_INFO_SCALE_SHARED);
>>>                         st->channels[cnt].address = dev << 8 | ch;
>>>                         st->channels[cnt].scan_index = cnt;
>>>                         st->channels[cnt].scan_type.sign = 'u';
>>>                         st->channels[cnt].scan_type.realbits = 12;
>>>                         st->channels[cnt].scan_type.storagebits =
>32;
>>>                         st->channels[cnt].scan_type.shift = 0;
>>>                 }
>>>         return cnt;
>>> }
>>>
>>> Trying to dynamically allocate and populate a channel spec.
>>> However some members are declared const. Do they really need to be
>> const?
>> hmm.. technically no, although a large point in the purpose of moving
>> to chan spec based registration was to allow all this stuff to be
>> constant static arrays.  I guess there is no particular harm in
>taking
>> them away from const though.  The functions should all assume they
>are
>> constant, but they don't really need to be before any are called.
>>
>> As to whether it is the right thing to do here, I guess it depends on
>> how large slave num can be. Google tells me the answer is really
>quite
>> large (50)!  Hence I'm convinced you have a very good reason.
>
>Well that's true for the AD7280. It supports up to 50 chained devices
>each handling
>6 cells. However this drivers targets the AD7280A which only supports 8
>chain devices.
>
>Still a good reason?
Still a lot of channels so I think yes.
 >
>> I'll push back on any drivers doing dynamic just to handle small
>> numbers of channels with variants, but here I'm convinced you have a
>> very good reason.
>>
>> Please send the patch to make these elements non constant with your
>> driver (as that will clearly provide justification)
>>
>> I'll admit I never conceived of anyone having the ability to chain
>> this many sensors.
>> For smaller chained devices I'd just advise doing them as a big
>static
>> array then setting num_channels appropriately. (glad you brought this
>> up as it just made me realise my bug in the previous set I sent out!)
>>
>> Jonathan
>>
>>>
>>> drivers/staging/iio/adc/ad7280a_core.c: In function
>>> 'ad7280_channel_init': drivers/staging/iio/adc/ad7280a_core.c:231:
>>> error: assignment of read-only member 'info_mask'
>>>
>>> Regards,
>>> Michael
>>>
>>> ------------------------------------------------------------------
>>> ********* Analog Devices GmbH
>>> **  *****
>>> **     ** Wilhelm-Wagenfeld-Strasse 6
>>> **  ***** D-80807 Munich
>>> ********* Germany
>>> Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB
>>> 40368;
>>> Geschaeftsfuehrer: Dr.Carsten Suckrow, Thomas Wessel, William A.
>>> Martin, Margaret Seif
>>>
>>>
>>>
>>
>
>Greetings,
>Michael
>
>--
>Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
>Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368;
>Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin,
>Margaret Seif
>
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.