| Message ID | 20221125083526.2422900-1-gerald.loacker@wolfvision.net |
|---|---|
| Headers | show |
| Series | add ti tmag5273 driver | expand |
On Fri, Nov 25, 2022 at 09:35:24AM +0100, Gerald Loacker wrote: > Add structs for iio type arrays such as IIO_AVAIL_LIST which can be used > instead of int arrays. Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> And thank you for doing this! Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> (one comment below) > Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net> > --- > include/linux/iio/iio.h | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h > index f0ec8a5e5a7a..eaf6727445a6 100644 > --- a/include/linux/iio/iio.h > +++ b/include/linux/iio/iio.h > @@ -383,6 +383,21 @@ s64 iio_get_time_ns(const struct iio_dev *indio_dev); > > struct iio_trigger; /* forward declaration */ > > +struct iio_val_int_plus_micro { > + int val_int; > + int val_micro; > +}; > + > +struct iio_val_int_plus_nano { > + int val_int; > + int val_nano; > +}; > + > +struct iio_val_int_plus_micro_db { > + int val_int; int val_int_db; ? > + int val_micro_db; > +}; Actually why can't we simply do typedef iio_val_int_plus_micro_db iio_val_int_plus_micro; ? > /** > * struct iio_info - constant information about device > * @event_attrs: event control attributes > -- > 2.37.2 >
On Fri, Nov 25, 2022 at 09:35:26AM +0100, Gerald Loacker wrote: > Add support for TI TMAG5273 Low-Power Linear 3D Hall-Effect Sensor. > Additionally to temperature and magnetic X, Y and Z-axes the angle and > magnitude are reported. > The sensor is operating in continuous measurement mode and changes to sleep > mode if not used for 5 seconds. Much better now, my comments below. ... > +static int tmag5273_write_scale(struct tmag5273_data *data, int scale_micro) > +{ What about u32 mask; > + int i; > + > + for (i = 0; i < ARRAY_SIZE(tmag5273_scale[0]); i++) { > + if (tmag5273_scale[data->version][i].val_micro == scale_micro) > + break; > + } > + if (i == ARRAY_SIZE(tmag5273_scale[0])) > + return -EINVAL; > + data->scale_index = i; if (data->scale_index == MAGN_RANGE_LOW) mask = 0; else mask = TMAG5273_Z_RANGE_MASK | TMAG5273_X_Y_RANGE_MASK; > + return regmap_update_bits(data->map, > + TMAG5273_SENSOR_CONFIG_2, > + TMAG5273_Z_RANGE_MASK | TMAG5273_X_Y_RANGE_MASK, mask); > + data->scale_index == MAGN_RANGE_LOW ? 0 : > + TMAG5273_Z_RANGE_MASK | > + TMAG5273_X_Y_RANGE_MASK); ? > +} ... > + switch (chan->type) { > + case IIO_MAGN: > + if (val != 0) if (val) > + return -EINVAL; > + return tmag5273_write_scale(data, val2); > + default: > + return -EINVAL; > + } ... > +static const struct regmap_config tmag5273_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > + .max_register = 0xff, Does it indeed have 256 registers? > + .volatile_reg = tmag5273_volatile_reg, > +}; ... > +static void tmag5273_read_device_property(struct tmag5273_data *data) > +{ struct device *dev = data->dev; > + const char *str; > + int ret; > + > + data->angle_measurement = TMAG5273_ANGLE_EN_X_Y; ret = device_property_read_string(dev, "ti,angle-measurement", &str); if (ret) return; > + if (!device_property_read_string(data->dev, "ti,angle-measurement", &str)) { > + ret = match_string(tmag5273_angle_names, > + ARRAY_SIZE(tmag5273_angle_names), str); > + if (ret < 0) > + dev_warn(data->dev, > + "unexpected read angle-measurement property: %s\n", str); > + else > + data->angle_measurement = ret; > + } ret = match_string(tmag5273_angle_names, ARRAY_SIZE(tmag5273_angle_names), str); if (ret < 0) dev_warn(dev, "unexpected read angle-measurement property: %s\n", str); else data->angle_measurement = ret; > +} ... > + return dev_err_probe(data->dev, ret, > + "failed to power on device\n"); I would leave on one line (only 84 characters long). ... > + return dev_err_probe(data->dev, ret, > + "failed to read device ID\n"); Ditto. ... > + switch (data->devid) { > + case TMAG5273_MANUFACTURER_ID: > + snprintf(data->name, sizeof(data->name), "tmag5273x%1u", > + data->version); > + if (data->version < 1 || data->version > 2) > + dev_warn(data->dev, "Unsupported device %s\n", > + data->name); > + break; > + default: > + dev_warn(data->dev, "Unknown device ID 0x%x\n", data->devid); > + break; > + } > + > + return 0; 'break;':s above can be replaced by direct 'return 0;':s. It's up to you. ... > + indio_dev = devm_iio_device_alloc(dev, sizeof(*data)); > + if (!indio_dev) > + return dev_err_probe(dev, -ENOMEM, > + "IIO device allocation failed\n"); We don't print ENOMEM error messages in the drivers, core does this for us. Otherwise you have to explain why this message is so important. ... > + /* > + * Register powerdown deferred callback which suspends the chip > + * after module unloaded. > + * > + * TMAG5273 should be in SUSPEND mode in the two cases: > + * 1) When driver is loaded, but we do not have any data or > + * configuration requests to it (we are solving it using > + * autosuspend feature). > + * 2) When driver is unloaded and device is not used (devm action is Something with indentation of this or other lines. > + * used in this case). > + */ ... > + return dev_err_probe(dev, ret, > + "failed to add powerdown action\n"); One line? ... > + dev_err(dev, "failed to power off device (%pe)\n", > + ERR_PTR(ret)); Ditto.
On Fri, Nov 25, 2022 at 12:59:01PM +0200, Andy Shevchenko wrote: > On Fri, Nov 25, 2022 at 09:35:26AM +0100, Gerald Loacker wrote: ... > > +static int tmag5273_write_scale(struct tmag5273_data *data, int scale_micro) > > +{ > > What about > > u32 mask; After looking again, I guess it should be u32 value; > > + int i; > > + > > + for (i = 0; i < ARRAY_SIZE(tmag5273_scale[0]); i++) { > > + if (tmag5273_scale[data->version][i].val_micro == scale_micro) > > + break; > > + } > > + if (i == ARRAY_SIZE(tmag5273_scale[0])) > > + return -EINVAL; > > + data->scale_index = i; > > if (data->scale_index == MAGN_RANGE_LOW) > mask = 0; > else > mask = TMAG5273_Z_RANGE_MASK | TMAG5273_X_Y_RANGE_MASK; > > > + return regmap_update_bits(data->map, > > + TMAG5273_SENSOR_CONFIG_2, > > + TMAG5273_Z_RANGE_MASK | TMAG5273_X_Y_RANGE_MASK, > > mask); > > > + data->scale_index == MAGN_RANGE_LOW ? 0 : > > + TMAG5273_Z_RANGE_MASK | > > + TMAG5273_X_Y_RANGE_MASK); > > ? > > > +}
On Fri, Nov 25, 2022 at 12:45:06PM +0200, Andy Shevchenko wrote: > On Fri, Nov 25, 2022 at 09:35:24AM +0100, Gerald Loacker wrote: ... > > +struct iio_val_int_plus_micro { > > + int val_int; > > + int val_micro; > > +}; Thinking more about naming, why not drop val_ completely? int integer; int micro; ? > > +struct iio_val_int_plus_nano { > > + int val_int; > > + int val_nano; > > +}; > > + > > +struct iio_val_int_plus_micro_db { > > + int val_int; > > int val_int_db; ? > > > + int val_micro_db; > > +}; > > Actually why can't we simply do > > typedef iio_val_int_plus_micro_db iio_val_int_plus_micro; > > ?
Am 25.11.2022 um 12:01 schrieb Andy Shevchenko: > On Fri, Nov 25, 2022 at 12:45:06PM +0200, Andy Shevchenko wrote: >> On Fri, Nov 25, 2022 at 09:35:24AM +0100, Gerald Loacker wrote: > > ... > >>> +struct iio_val_int_plus_micro { >>> + int val_int; >>> + int val_micro; >>> +}; > > Thinking more about naming, why not drop val_ completely? > > int integer; > int micro; > > ? > Yes, this sounds good to me. I think of adding only typedef struct { int integer; int micro; } iio_val_int_plus_micro; for now, and one can add similar structures when needed, like typedef struct { int integer; int nano; } iio_val_int_plus_nano; or typedef iio_val_int_plus_micro iio_val_int_plus_micro_db; If you think it's better to add them all, I can do that, of course. >>> +struct iio_val_int_plus_nano { >>> + int val_int; >>> + int val_nano; >>> +}; >>> + >>> +struct iio_val_int_plus_micro_db { >>> + int val_int; >> >> int val_int_db; ? >> >>> + int val_micro_db; >>> +}; >> >> Actually why can't we simply do >> >> typedef iio_val_int_plus_micro_db iio_val_int_plus_micro; >> >> ? >
On Mon, Nov 28, 2022 at 01:18:04PM +0100, Gerald Loacker wrote: > Am 25.11.2022 um 12:01 schrieb Andy Shevchenko: > > On Fri, Nov 25, 2022 at 12:45:06PM +0200, Andy Shevchenko wrote: > >> On Fri, Nov 25, 2022 at 09:35:24AM +0100, Gerald Loacker wrote: ... > >>> +struct iio_val_int_plus_micro { > >>> + int val_int; > >>> + int val_micro; > >>> +}; > > > > Thinking more about naming, why not drop val_ completely? > > > > int integer; > > int micro; > > > > ? > > Yes, this sounds good to me. I think of adding only > > typedef struct { > int integer; > int micro; > } iio_val_int_plus_micro; > > for now, and one can add similar structures when needed, like > > typedef struct { > int integer; > int nano; > } iio_val_int_plus_nano; It's a rule to use _t for typedef:s in the kernel. That's why I suggested to leave struct definition and only typedef the same structures (existing) to new names (if needed). > or > typedef iio_val_int_plus_micro iio_val_int_plus_micro_db; This is better as explained above. > If you think it's better to add them all, I can do that, of course. > > >>> +struct iio_val_int_plus_nano { > >>> + int val_int; > >>> + int val_nano; > >>> +}; > >>> + > >>> +struct iio_val_int_plus_micro_db { > >>> + int val_int; > >> > >> int val_int_db; ? > >> > >>> + int val_micro_db; > >>> +}; > >> > >> Actually why can't we simply do > >> > >> typedef iio_val_int_plus_micro_db iio_val_int_plus_micro; > >> > >> ?
Hi Gerald, Andy, On 11/28/22 14:27, Andy Shevchenko wrote: > On Mon, Nov 28, 2022 at 01:18:04PM +0100, Gerald Loacker wrote: >> Am 25.11.2022 um 12:01 schrieb Andy Shevchenko: >>> On Fri, Nov 25, 2022 at 12:45:06PM +0200, Andy Shevchenko wrote: >>>> On Fri, Nov 25, 2022 at 09:35:24AM +0100, Gerald Loacker wrote: > > ... > >>>>> +struct iio_val_int_plus_micro { >>>>> + int val_int; >>>>> + int val_micro; >>>>> +}; >>> >>> Thinking more about naming, why not drop val_ completely? >>> >>> int integer; >>> int micro; >>> >>> ? >> >> Yes, this sounds good to me. I think of adding only >> >> typedef struct { >> int integer; >> int micro; >> } iio_val_int_plus_micro; I think we actually want struct iio_val_int_plus_micro { int integer; int micro; }; here, right? >> for now, and one can add similar structures when needed, like >> >> typedef struct { >> int integer; >> int nano; >> } iio_val_int_plus_nano; +1 for introducing things when they are actually used. > It's a rule to use _t for typedef:s in the kernel. That's why > I suggested to leave struct definition and only typedef the same structures > (existing) to new names (if needed). Andy, excuse our ignorance but we are not sure how this typedef approach is supposed to look like... >> or > >> typedef iio_val_int_plus_micro iio_val_int_plus_micro_db; ... because #include <stdio.h> struct iio_val_int_plus_micro { int integer; int micro; }; typedef iio_val_int_plus_micro iio_val_int_plus_micro_db; int main() { struct iio_val_int_plus_micro a = { .integer = 100, .micro = 10, }; struct iio_val_int_plus_micro_db b = { .integer = 20, .micro = 10, }; return 0; } won't compile. > This is better as explained above. > >> If you think it's better to add them all, I can do that, of course. Anyway, seeing that only struct iio_val_int_plus_micro is used at the moment, I believe the best path forward is to introduce only this struct and move on. Best regards, Michael >>>>> +struct iio_val_int_plus_nano { >>>>> + int val_int; >>>>> + int val_nano; >>>>> +}; >>>>> + >>>>> +struct iio_val_int_plus_micro_db { >>>>> + int val_int; >>>> >>>> int val_int_db; ? >>>> >>>>> + int val_micro_db; >>>>> +}; >>>> >>>> Actually why can't we simply do >>>> >>>> typedef iio_val_int_plus_micro_db iio_val_int_plus_micro; >>>> >>>> ? >
On Mon, Nov 28, 2022 at 02:48:48PM +0100, Michael Riesch wrote: > On 11/28/22 14:27, Andy Shevchenko wrote: > > On Mon, Nov 28, 2022 at 01:18:04PM +0100, Gerald Loacker wrote: > >> Am 25.11.2022 um 12:01 schrieb Andy Shevchenko: ... > > It's a rule to use _t for typedef:s in the kernel. That's why > > I suggested to leave struct definition and only typedef the same structures > > (existing) to new names (if needed). > > Andy, excuse our ignorance but we are not sure how this typedef approach > is supposed to look like... > > >> or > > > >> typedef iio_val_int_plus_micro iio_val_int_plus_micro_db; > > ... because > > #include <stdio.h> > > struct iio_val_int_plus_micro { > int integer; > int micro; > }; > > typedef iio_val_int_plus_micro iio_val_int_plus_micro_db; > > int main() > { > struct iio_val_int_plus_micro a = { .integer = 100, .micro = 10, }; > struct iio_val_int_plus_micro_db b = { .integer = 20, .micro = 10, }; > return 0; > } > > won't compile. I see. Thanks for pointing this out. Then the question is why do we need the two same structures with different names?
Hi Andy, On 11/28/22 15:05, Andy Shevchenko wrote: > On Mon, Nov 28, 2022 at 02:48:48PM +0100, Michael Riesch wrote: >> On 11/28/22 14:27, Andy Shevchenko wrote: >>> On Mon, Nov 28, 2022 at 01:18:04PM +0100, Gerald Loacker wrote: >>>> Am 25.11.2022 um 12:01 schrieb Andy Shevchenko: > > ... > >>> It's a rule to use _t for typedef:s in the kernel. That's why >>> I suggested to leave struct definition and only typedef the same structures >>> (existing) to new names (if needed). >> >> Andy, excuse our ignorance but we are not sure how this typedef approach >> is supposed to look like... >> >>>> or >>> >>>> typedef iio_val_int_plus_micro iio_val_int_plus_micro_db; >> >> ... because >> >> #include <stdio.h> >> >> struct iio_val_int_plus_micro { >> int integer; >> int micro; >> }; >> >> typedef iio_val_int_plus_micro iio_val_int_plus_micro_db; >> >> int main() >> { >> struct iio_val_int_plus_micro a = { .integer = 100, .micro = 10, }; >> struct iio_val_int_plus_micro_db b = { .integer = 20, .micro = 10, }; >> return 0; >> } >> >> won't compile. > > I see. Thanks for pointing this out. > > Then the question is why do we need the two same structures with different > names? Most probably we don't need "struct iio_val_int_plus_micro_db" at all since IIO_VAL_INT_PLUS_MICRO_DB and IIO_VAL_INT_PLUS_MICRO get the same treatment in industrialio-core.c. At least it should not be introduced in the scope of this series. In the end this is up to whoever writes the first driver using the common data structures and IIO_VAL_INT_PLUS_MICRO_DB. Best regards, Michael
On Mon, 28 Nov 2022 15:26:51 +0100 Michael Riesch <michael.riesch@wolfvision.net> wrote: > Hi Andy, > > On 11/28/22 15:05, Andy Shevchenko wrote: > > On Mon, Nov 28, 2022 at 02:48:48PM +0100, Michael Riesch wrote: > >> On 11/28/22 14:27, Andy Shevchenko wrote: > >>> On Mon, Nov 28, 2022 at 01:18:04PM +0100, Gerald Loacker wrote: > >>>> Am 25.11.2022 um 12:01 schrieb Andy Shevchenko: > > > > ... > > > >>> It's a rule to use _t for typedef:s in the kernel. That's why > >>> I suggested to leave struct definition and only typedef the same structures > >>> (existing) to new names (if needed). > >> > >> Andy, excuse our ignorance but we are not sure how this typedef approach > >> is supposed to look like... > >> > >>>> or > >>> > >>>> typedef iio_val_int_plus_micro iio_val_int_plus_micro_db; > >> > >> ... because > >> > >> #include <stdio.h> > >> > >> struct iio_val_int_plus_micro { > >> int integer; > >> int micro; > >> }; > >> > >> typedef iio_val_int_plus_micro iio_val_int_plus_micro_db; > >> > >> int main() > >> { > >> struct iio_val_int_plus_micro a = { .integer = 100, .micro = 10, }; > >> struct iio_val_int_plus_micro_db b = { .integer = 20, .micro = 10, }; > >> return 0; > >> } > >> > >> won't compile. > > > > I see. Thanks for pointing this out. > > > > Then the question is why do we need the two same structures with different > > names? > > Most probably we don't need "struct iio_val_int_plus_micro_db" at all > since IIO_VAL_INT_PLUS_MICRO_DB and IIO_VAL_INT_PLUS_MICRO get the same > treatment in industrialio-core.c. At least it should not be introduced > in the scope of this series. In the end this is up to whoever writes the > first driver using the common data structures and IIO_VAL_INT_PLUS_MICRO_DB. They get same treatment today because we don't attempt to deal with IIO_VAL_INT_PLUS_MICRO_DB in conjunction with any of the analog circuit type front ends yet. Mind you, even though the handling in iio-rescale.c will be different if anyone ever adds support for the DB form (I shudder at the maths of combining this with other scale factors), I don't see the possibility meaning we need a different structure. Jonathan > > Best regards, > Michael >