Re: [PATCH v2 2/4] reset: Add APIs to manage array of resets

[Vivek Gautam]

Hi Philipp,


On 04/04/2017 06:17 PM, Philipp Zabel wrote:
> Hi Vivek,
>
> On Tue, 2017-04-04 at 16:09 +0530, Vivek Gautam wrote:
> [...]
> > > I'd prefer to mirror the gpiod API a little, and to have the number
> > > contained in the array structure, similar to struct gpio_descs:
> [...]
> > Alright, i can update this.
> > I took regulator_bulk interface as the reference, but now i see
> > gpio_descs has a smaller footprint.
> Ok, understood.
> I think the smaller footprint API is more convenient for the user.
>
> [...]
> > > > + * Returns 0 on success or error number on failure
> > > > + */
> > > > +static int reset_control_array_get(struct device *dev, int num_rsts,
> > > > +				struct reset_control_array *resets,
> > > > +				bool shared, bool optional)
> > > Can we make this count and return a pointer to the newly created array?
> > >
> > > static struct reset_controls *
> > > reset_control_array_get(struct device *dev, bool shared, bool optional)
> > > {
> > > 	...
> > >
> > > That way the consumer doesn't have to care about counting and array
> > > allocation.
> > Just trying to think again in line with the regulator bulk APIs.
> > Can't a user provide a list of reset controls as data and thus
> > request reset controls with a "id" and num_resets available.
> >
> > e.g.
> >      const char * const rst_names[] = {
> >        "rst1", "rst2" ...
> >      };
> >      xyz_data = {
> >        .resets = rst_names;
> >        .num = ARRAY_SIZE(rst_names);
> >      };
> > and then the driver making use of reset_control_array APIs
> > to request this list of reset controls and assert/deassert them.
> >
> > I am assuming this case when one user driver is used across a bunch
> > of platforms with different number of resets available.
> > We may not want to rely solely on the device tree entries, since
> > the resets can be mandatory in few cases and we may want to
> > return failure.
> The way I understood the array API was as a method of handling a list of
> anonymous resets, specified as
>
> 	resets = <&reset 1>, <&reset 2>, <&reset 3>;
> 	// reset-names = "rst1", "rst2", "rst3"; /* don't care */
>
> in the device tree.
>
> Now if you want to handle a partial list of those as an unordered list,
> with special consideration for others, that could be added as a separate
> API when there is use for it. But I expect that most potential users of
> this array API will not have to make such a distinction.

Alright, I will modify the array APIs as suggested to handle an
unordered list of resets passed from the device tree.
As you said, we can handle the special cases when the need arise.

> > > > @@ -85,6 +107,39 @@ static inline struct reset_control *__devm_reset_control_get(
> > > >    	return -ENOTSUPP;
> > > >    }
> > > >    
> > > > +static inline int reset_control_array_assert(int num_rsts,
> > > > +				struct reset_control_array *resets)
> > > > +{
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static inline int reset_control_array_deassert(int num_rsts,
> > > > +				struct reset_control_array *resets)
> > > > +{
> > > > +	return 0;
> > > > +}
> > > Is this missing a stub for reset_control_array_get?
> > No, that's supposed to be a static function.
> Oh right, it is static. Please ignore.

Thanks

> regards
> Philipp

Best Regards
Vivek

--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project