| Message ID | 20230523064310.3005-2-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | gnss: updates to support the Renesas KingFisher board | expand |
On Tue, May 23, 2023 at 08:43:06AM +0200, Wolfram Sang wrote: > This is a proof-of-concept how easy it is to merge those two drivers as > they are extremly similar. The differences can be abstracted away > easily. Further work (renaming from 'ubx' to something more generic, > removing the MTK driver, ...) is postponed until there is agrement that > merging these drivers is actually wanted. I vote for it, though. I have > updates to the UBX driver which do make sense for the MTK driver as > well. Code saving is also a plus. We can always fork into a specific > driver again if we'd ever need that. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Johan, just from a gut feeling or a hi level glimpse: is this merging of the driver worth pursuing? Thanks and all the best, Wolfram > --- > drivers/gnss/ubx.c | 30 +++++++++++++++++++++++++----- > 1 file changed, 25 insertions(+), 5 deletions(-) > > diff --git a/drivers/gnss/ubx.c b/drivers/gnss/ubx.c > index c951be202ca2..c01e1e875727 100644 > --- a/drivers/gnss/ubx.c > +++ b/drivers/gnss/ubx.c > @@ -63,12 +63,31 @@ static const struct gnss_serial_ops ubx_gserial_ops = { > .set_power = ubx_set_power, > }; > > +struct ubx_info { > + enum gnss_type type; > + char *v_bckp_name; > +}; > + > +const struct ubx_info ubx_info_ubx = { > + .type = GNSS_TYPE_UBX, > + .v_bckp_name = "v-bckp", > +}; > + > +const struct ubx_info ubx_info_mtk = { > + .type = GNSS_TYPE_MTK, > + .v_bckp_name = "vbackup", > +}; > + > static int ubx_probe(struct serdev_device *serdev) > { > + const struct ubx_info *info = of_device_get_match_data(&serdev->dev); > struct gnss_serial *gserial; > struct ubx_data *data; > int ret; > > + if (!info) > + return -ENOENT; > + > gserial = gnss_serial_allocate(serdev, sizeof(*data)); > if (IS_ERR(gserial)) { > ret = PTR_ERR(gserial); > @@ -77,7 +96,7 @@ static int ubx_probe(struct serdev_device *serdev) > > gserial->ops = &ubx_gserial_ops; > > - gserial->gdev->type = GNSS_TYPE_UBX; > + gserial->gdev->type = info->type; > > data = gnss_serial_get_drvdata(gserial); > > @@ -87,7 +106,7 @@ static int ubx_probe(struct serdev_device *serdev) > goto err_free_gserial; > } > > - data->v_bckp = devm_regulator_get_optional(&serdev->dev, "v-bckp"); > + data->v_bckp = devm_regulator_get_optional(&serdev->dev, info->v_bckp_name); > if (IS_ERR(data->v_bckp)) { > ret = PTR_ERR(data->v_bckp); > if (ret == -ENODEV) > @@ -130,9 +149,10 @@ static void ubx_remove(struct serdev_device *serdev) > > #ifdef CONFIG_OF > static const struct of_device_id ubx_of_match[] = { > - { .compatible = "u-blox,neo-6m" }, > - { .compatible = "u-blox,neo-8" }, > - { .compatible = "u-blox,neo-m8" }, > + { .compatible = "u-blox,neo-6m", .data = &ubx_info_ubx, }, > + { .compatible = "u-blox,neo-8", .data = &ubx_info_ubx, }, > + { .compatible = "u-blox,neo-m8", .data = &ubx_info_ubx, }, > + { .compatible = "globaltop,pa6h", .data = &ubx_info_mtk }, > {}, > }; > MODULE_DEVICE_TABLE(of, ubx_of_match); > -- > 2.35.1 >
Hi Wolfram, and sorry about the late feedback on this. Didn't have time to process my queue before some holiday last week. On Mon, Jun 12, 2023 at 01:35:26PM +0200, Wolfram Sang wrote: > On Tue, May 23, 2023 at 08:43:06AM +0200, Wolfram Sang wrote: > > This is a proof-of-concept how easy it is to merge those two drivers as > > they are extremly similar. The differences can be abstracted away > > easily. Further work (renaming from 'ubx' to something more generic, > > removing the MTK driver, ...) is postponed until there is agrement that > > merging these drivers is actually wanted. I vote for it, though. I have > > updates to the UBX driver which do make sense for the MTK driver as > > well. Code saving is also a plus. We can always fork into a specific > > driver again if we'd ever need that. > > > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > > Johan, just from a gut feeling or a hi level glimpse: is this merging of > the driver worth pursuing? No, sorry, keeping separate drivers per vendor was a deliberate choice. First, some of these devices support vendor specific protocols which we may add driver support for at some point beyond currently just exporting this information to user space (e.g. to change line speed from the driver). Second, device families are expected to share at least some common properties like reset signalling and supplies, which are better kept separate. Johan
Hi Johan, > and sorry about the late feedback on this. Didn't have time to process > my queue before some holiday last week. No worries. I hope you had some nice holiday! > No, sorry, keeping separate drivers per vendor was a deliberate choice. Yes, OK. I am not really surprised. > First, some of these devices support vendor specific protocols which we > may add driver support for at some point beyond currently just exporting > this information to user space (e.g. to change line speed from the > driver). OK, I thought we can seperatae again if this really coming somewhen, but we can also leave it like this. > Second, device families are expected to share at least some common > properties like reset signalling and supplies, which are better kept > separate. Thanks for the feedback! Wolfram
> First, some of these devices support vendor specific protocols which we > may add driver support for at some point beyond currently just exporting > this information to user space (e.g. to change line speed from the > driver). Speaking of proprietary protocols: did you ever get in contact with the gpsd folks? I stumbled over this mail from February this year [1] where nobody knew where this /dev/gnss0 device came from. I'd think we should be in contact with them if we want to support e.g. line speed changes? [1] https://lists.gnu.org/archive/html/gpsd-dev/2023-02/msg00017.html
On Tue, Jun 20, 2023 at 12:21:40PM +0200, Wolfram Sang wrote: > Speaking of proprietary protocols: did you ever get in contact with the > gpsd folks? I stumbled over this mail from February this year [1] where > nobody knew where this /dev/gnss0 device came from. I'd think we should > be in contact with them if we want to support e.g. line speed changes? No, I haven't and I have very little motivation for spending time on this. With the features we have today gpsd works as-is (last time I checked). Line-speed configuration would likely also be handled transparently by the driver without user space interaction, but there is also room for extending the interface should need arise. Johan
diff --git a/drivers/gnss/ubx.c b/drivers/gnss/ubx.c index c951be202ca2..c01e1e875727 100644 --- a/drivers/gnss/ubx.c +++ b/drivers/gnss/ubx.c @@ -63,12 +63,31 @@ static const struct gnss_serial_ops ubx_gserial_ops = { .set_power = ubx_set_power, }; +struct ubx_info { + enum gnss_type type; + char *v_bckp_name; +}; + +const struct ubx_info ubx_info_ubx = { + .type = GNSS_TYPE_UBX, + .v_bckp_name = "v-bckp", +}; + +const struct ubx_info ubx_info_mtk = { + .type = GNSS_TYPE_MTK, + .v_bckp_name = "vbackup", +}; + static int ubx_probe(struct serdev_device *serdev) { + const struct ubx_info *info = of_device_get_match_data(&serdev->dev); struct gnss_serial *gserial; struct ubx_data *data; int ret; + if (!info) + return -ENOENT; + gserial = gnss_serial_allocate(serdev, sizeof(*data)); if (IS_ERR(gserial)) { ret = PTR_ERR(gserial); @@ -77,7 +96,7 @@ static int ubx_probe(struct serdev_device *serdev) gserial->ops = &ubx_gserial_ops; - gserial->gdev->type = GNSS_TYPE_UBX; + gserial->gdev->type = info->type; data = gnss_serial_get_drvdata(gserial); @@ -87,7 +106,7 @@ static int ubx_probe(struct serdev_device *serdev) goto err_free_gserial; } - data->v_bckp = devm_regulator_get_optional(&serdev->dev, "v-bckp"); + data->v_bckp = devm_regulator_get_optional(&serdev->dev, info->v_bckp_name); if (IS_ERR(data->v_bckp)) { ret = PTR_ERR(data->v_bckp); if (ret == -ENODEV) @@ -130,9 +149,10 @@ static void ubx_remove(struct serdev_device *serdev) #ifdef CONFIG_OF static const struct of_device_id ubx_of_match[] = { - { .compatible = "u-blox,neo-6m" }, - { .compatible = "u-blox,neo-8" }, - { .compatible = "u-blox,neo-m8" }, + { .compatible = "u-blox,neo-6m", .data = &ubx_info_ubx, }, + { .compatible = "u-blox,neo-8", .data = &ubx_info_ubx, }, + { .compatible = "u-blox,neo-m8", .data = &ubx_info_ubx, }, + { .compatible = "globaltop,pa6h", .data = &ubx_info_mtk }, {}, }; MODULE_DEVICE_TABLE(of, ubx_of_match);
This is a proof-of-concept how easy it is to merge those two drivers as they are extremly similar. The differences can be abstracted away easily. Further work (renaming from 'ubx' to something more generic, removing the MTK driver, ...) is postponed until there is agrement that merging these drivers is actually wanted. I vote for it, though. I have updates to the UBX driver which do make sense for the MTK driver as well. Code saving is also a plus. We can always fork into a specific driver again if we'd ever need that. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- drivers/gnss/ubx.c | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-)