Re: [RFC PATCH 4/5] iio: accel: Support Kionix/ROHM KX022A accelerometer

[Jonathan Cameron]

On Wed, 28 Sep 2022 14:14:14 +0300
Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:

> Hi Jonathan,
> 
> On 9/22/22 20:03, Jonathan Cameron wrote:
> > On Wed, 21 Sep 2022 14:45:35 +0300
> > Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:  
> >> +
> >> +/*
> >> + * The sensor HW can support ODR up to 1600 Hz - which is beyond what most of
> >> + * Linux CPUs can handle w/o dropping samples. Also, the low power mode is not
> >> + * available for higher sample rates. Thus the driver only supports 200 Hz and
> >> + * slower ODRs. Slowest being 0.78 Hz
> >> + */
> >> +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("0.78 1.563 3.125 6.25 12.5 25 50 100 200");
> >> +static IIO_CONST_ATTR(scale_available,
> >> +		      "598.550415 1197.10083 2394.20166 4788.40332");
> >> +
> >> +static struct attribute *kx022a_attributes[] = {
> >> +	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
> >> +	&iio_const_attr_scale_available.dev_attr.attr,  
> > 
> > Use the read_avail() callback instead of doing these as attributes.
> > That makes the values available to consumer drivers...  
> 
> Am I correct that populating the read_avail() does not add sysfs entries 
> for available scale/frequency? Eg, if I wish to expose the supported 
> values via sysfs I still need these attributes? Implementing the 
> read_avail() as well is not a problem though.

Need to also set the relevant bit in 
info_mask_shared_by_xxx_avail in the channels for the sysfs files to be created
by calling the read_avail() callback.

When I introduced those I thought about making it mandatory to introduce them
for all the info_mask_shared_by_xxx entries and not having the extra bitmap
but that meant figuring out the relevant entries for a mass of stuff whenever
a driver was converted from the old approach like you've used here.

> 
> >> +static int kx022a_turn_on_unlock(struct kx022a_data *data)
> >> +{
> >> +	int ret;
> >> +  
> > This is not used enough that I can see a strong reason for the
> > wrapper.  Just put the two calls inline and rename the unlocked case.  
> 
> In my opinion the kx022a_turn_on_unlock() and  kx022a_turn_off_lock() do 
> simplify functions. Especially after I started using the 
> iio_device_claim_direct_mode() :) Thus I will leave these for the v2 - 
> please ping me again if you still want to see them removed (but I think 
> the usage of iio_device_claim_direct_mode() changed this to favour the 
> kx022a_turn_on_unlock() and kx022a_turn_off_lock()).
Let's see how it looks in v2.

> 
> >> +static int kx022a_chip_init(struct kx022a_data *data)
> >> +{
> >> +	int ret, dummy;
> >> +
> >> +	/*
> >> +	 * Disable IRQs because if the IRQs are left on (for example by
> >> +	 * a shutdown which did not deactivate the accelerometer) we do
> >> +	 * most probably end up flooding the system with unhandled IRQs
> >> +	 * and get the line disabled from SOC side.
> >> +	 */
> >> +	ret = regmap_write(data->regmap, KX022A_REG_INC4, 0);  
> > 
> > Unusual to do this rather than a reset.  Quick look suggests there is
> > a suitable software reset (CNTL2)  
> 
> I switched to the software reset as you suggested. I am not really 
> convinced it is a better way. It seems the software reset requires us to 
> re-init the regmap cache. 

Yup, though if you've provided the reset defaults that should be quick.

Jonathan