[PATCH 1/1] isimodem: Adding version isimodem and isimodem2.5

[PATCH 1/1] isimodem: Adding version isimodem and isimodem2.5

[Jessica Nilsson]

[-- Attachment #1: Type: text/plain, Size: 666 bytes --]

Adding a version enum to differentiate between isimodem2.0 and isimodem2.5.

Needed for isimodem2.5 patches planned to follow shortly.

Best Regards,
Jessica


---
 drivers/isimodem/isimodem.h |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/isimodem/isimodem.h b/drivers/isimodem/isimodem.h
index 37191c7..a2a827d 100644
--- a/drivers/isimodem/isimodem.h
+++ b/drivers/isimodem/isimodem.h
@@ -19,6 +19,11 @@
  *
  */
 
+enum isi_version {
+	ISI_20 = 0,
+	ISI_25 = 1
+};
+
 extern struct ofono_modem *isi_modem_by_interface(char const *ifname);
 
 extern void isi_phonebook_init(void);
-- 
1.7.3.2

Re: [PATCH 1/1] isimodem: Adding version isimodem and isimodem2.5

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 1177 bytes --]

Hi Jessica,

> Adding a version enum to differentiate between isimodem2.0 and isimodem2.5.
> 
> Needed for isimodem2.5 patches planned to follow shortly.
> 
> Best Regards,
> Jessica

same comments about commit body apply here as well.

> ---
>  drivers/isimodem/isimodem.h |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/isimodem/isimodem.h b/drivers/isimodem/isimodem.h
> index 37191c7..a2a827d 100644
> --- a/drivers/isimodem/isimodem.h
> +++ b/drivers/isimodem/isimodem.h
> @@ -19,6 +19,11 @@
>   *
>   */
>  
> +enum isi_version {
> +	ISI_20 = 0,
> +	ISI_25 = 1
> +};
> +

If we follow the same layout as with all other modem drivers, then the
*modem.h should only contain the prototypes for the init callbacks.

>  extern struct ofono_modem *isi_modem_by_interface(char const *ifname);

This one already violates this one and might need to be cleaned up.

The ISI modem is special since it really only has two users anyway in
the end, but maybe drivers/isimodem/version.h analog to vendor.h from
atmodem might be a good idea.

Aki, what is your preference here?

Regards

Marcel

Re: [PATCH 1/1] isimodem: Adding version isimodem and isimodem2.5

[Aki Niemi]

[-- Attachment #1: Type: text/plain, Size: 1269 bytes --]

Hi,

2011/1/25 Marcel Holtmann <marcel@holtmann.org>:
>> +enum isi_version {
>> +     ISI_20 = 0,
>> +     ISI_25 = 1
>> +};
>> +
>
> If we follow the same layout as with all other modem drivers, then the
> *modem.h should only contain the prototypes for the init callbacks.
>
>>  extern struct ofono_modem *isi_modem_by_interface(char const *ifname);
>
> This one already violates this one and might need to be cleaned up.
>
> The ISI modem is special since it really only has two users anyway in
> the end, but maybe drivers/isimodem/version.h analog to vendor.h from
> atmodem might be a good idea.
>
> Aki, what is your preference here?

I don't think there is a need for this, as ISI itself already provides
both ISI version (which we have available always via the
g_isi_client_verify() call), and in many cases also the resource ID as
means to find the correct quirk to use.

Of course, I could be convinced otherwise, but not without seeing
actual patches. It might become extremely ugly with using ISI version
and the resource ID, and using something like vendor.h might then make
sense.

But remember that we also have the isiusb plugin, and I would like it
to keep working with *any* isimodem out there.

Cheers,
Aki

Re: [PATCH 1/1] isimodem: Adding version isimodem and isimodem2.5

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 1502 bytes --]

Hi Aki,

> >> +enum isi_version {
> >> +     ISI_20 = 0,
> >> +     ISI_25 = 1
> >> +};
> >> +
> >
> > If we follow the same layout as with all other modem drivers, then the
> > *modem.h should only contain the prototypes for the init callbacks.
> >
> >>  extern struct ofono_modem *isi_modem_by_interface(char const *ifname);
> >
> > This one already violates this one and might need to be cleaned up.
> >
> > The ISI modem is special since it really only has two users anyway in
> > the end, but maybe drivers/isimodem/version.h analog to vendor.h from
> > atmodem might be a good idea.
> >
> > Aki, what is your preference here?
> 
> I don't think there is a need for this, as ISI itself already provides
> both ISI version (which we have available always via the
> g_isi_client_verify() call), and in many cases also the resource ID as
> means to find the correct quirk to use.
> 
> Of course, I could be convinced otherwise, but not without seeing
> actual patches. It might become extremely ugly with using ISI version
> and the resource ID, and using something like vendor.h might then make
> sense.

if you don't need external version or vendor quirks, then even better.
We are just damn unlucky in the atmodem case. The less the modem plugins
have to specify the better.

So this clearly then is not suppose to be in isimodem.h and while at it,
someone might wanna cleanup isi_modem_by_interface and move it to a
better location.

Regards

Marcel

Re: [PATCH 1/1] isimodem: Adding version isimodem and isimodem2.5

[Aki Niemi]

[-- Attachment #1: Type: text/plain, Size: 283 bytes --]

Hi Marcel,

2011/1/25 Marcel Holtmann <marcel@holtmann.org>:
> So this clearly then is not suppose to be in isimodem.h and while at it,
> someone might wanna cleanup isi_modem_by_interface and move it to a
> better location.

Done. Moved it to /dev/null. ;-)

Cheers,
Aki

Re: [PATCH 1/1] isimodem: Adding version isimodem and isimodem2.5

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 276 bytes --]

Hi Aki,

> > So this clearly then is not suppose to be in isimodem.h and while at it,
> > someone might wanna cleanup isi_modem_by_interface and move it to a
> > better location.
> 
> Done. Moved it to /dev/null. ;-)

no kidding. Good one :)

Regards

Marcel

Re: [PATCH 1/1] isimodem: Adding version isimodem and isimodem2.5

[Jessica Nilsson]

[-- Attachment #1: Type: text/plain, Size: 609 bytes --]

Hi,

On 01/25/2011 01:39 PM, Aki Niemi wrote:

> I don't think there is a need for this, as ISI itself already provides
> both ISI version (which we have available always via the
> g_isi_client_verify() call), and in many cases also the resource ID as
> means to find the correct quirk to use.
>
> Of course, I could be convinced otherwise, but not without seeing
> actual patches. It might become extremely ugly with using ISI version
> and the resource ID, and using something like vendor.h might then make
> sense.
>


Ok, understood. We will see what we can do.

Best Regards,
Jessica